Skip to content

fix(audit phase 3/3): correct retry-after unit parsing #47

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ndycode wants to merge 3 commits into from
Closed

fix(audit phase 3/3): correct retry-after unit parsing #47

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cbe88d3
fix(rate-limit): honor retry_after_ms units in response parsing
ndycode Feb 27, 2026
a0cc4f2
fix(rate-limit): clamp delays and codify retry_after precedence
ndycode Feb 28, 2026
4309694
fix(rate-limit): normalize header retry delays and add clamp boundary…
ndycode Feb 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 33 additions & 15 deletions lib/request/fetch-helpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -691,15 +691,21 @@ interface RateLimitErrorBody {

function parseRateLimitBody(
body: string,
): { code?: string; resetsAt?: number; retryAfterMs?: number } | undefined {
): {
code?: string;
resetsAt?: number;
retryAfterMs?: number;
retryAfterSeconds?: number;
} | undefined {
if (!body) return undefined;
try {
const parsed = JSON.parse(body) as RateLimitErrorBody;
const error = parsed?.error ?? {};
const code = (error.code ?? error.type ?? "").toString();
const resetsAt = toNumber(error.resets_at ?? error.reset_at);
const retryAfterMs = toNumber(error.retry_after_ms ?? error.retry_after);
return { code, resetsAt, retryAfterMs };
const retryAfterMs = toNumber(error.retry_after_ms);
const retryAfterSeconds = toNumber(error.retry_after);
return { code, resetsAt, retryAfterMs, retryAfterSeconds };
} catch {
return undefined;
}
Expand Down Expand Up @@ -824,25 +830,33 @@ function ensureJsonErrorResponse(response: Response, payload: ErrorPayload): Res

function parseRetryAfterMs(
response: Response,
parsedBody?: { resetsAt?: number; retryAfterMs?: number },
parsedBody?: {
resetsAt?: number;
retryAfterMs?: number;
retryAfterSeconds?: number;
},
): number | null {
if (parsedBody?.retryAfterMs !== undefined) {
return normalizeRetryAfter(parsedBody.retryAfterMs);
return normalizeRetryAfterMilliseconds(parsedBody.retryAfterMs);
}

if (parsedBody?.retryAfterSeconds !== undefined) {
return normalizeRetryAfterSeconds(parsedBody.retryAfterSeconds);
}

const retryAfterMsHeader = response.headers.get("retry-after-ms");
if (retryAfterMsHeader) {
const parsed = Number.parseInt(retryAfterMsHeader, 10);
if (!Number.isNaN(parsed) && parsed > 0) {
return parsed;
return normalizeRetryAfterMilliseconds(parsed);
}
}

const retryAfterHeader = response.headers.get("retry-after");
if (retryAfterHeader) {
const parsed = Number.parseInt(retryAfterHeader, 10);
if (!Number.isNaN(parsed) && parsed > 0) {
return parsed * 1000;
return normalizeRetryAfterSeconds(parsed);
}
}

Expand Down Expand Up @@ -881,16 +895,20 @@ function parseRetryAfterMs(
return null;
}

function normalizeRetryAfter(value: number): number {
function normalizeRetryAfterMilliseconds(value: number): number {
if (!Number.isFinite(value)) return 60000;
let ms: number;
if (value > 0 && value < 1000) {
ms = Math.floor(value * 1000);
} else {
ms = Math.floor(value);
}
const ms = Math.floor(value);
const MIN_RETRY_DELAY_MS = 1;
const MAX_RETRY_DELAY_MS = 5 * 60 * 1000;
return Math.min(ms, MAX_RETRY_DELAY_MS);
return Math.min(Math.max(ms, MIN_RETRY_DELAY_MS), MAX_RETRY_DELAY_MS);
}

function normalizeRetryAfterSeconds(value: number): number {
if (!Number.isFinite(value)) return 60000;
const ms = Math.floor(value * 1000);
const MIN_RETRY_DELAY_MS = 1;
const MAX_RETRY_DELAY_MS = 5 * 60 * 1000;
return Math.min(Math.max(ms, MIN_RETRY_DELAY_MS), MAX_RETRY_DELAY_MS);
}

function toNumber(value: unknown): number | undefined {
Expand Down
56 changes: 54 additions & 2 deletions test/fetch-helpers.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -664,13 +664,13 @@ describe('Fetch Helpers Module', () => {
expect(rateLimit?.retryAfterMs).toBeGreaterThan(0);
});

it('normalizes small retryAfterMs values as seconds', async () => {
it('keeps retry_after_ms values in milliseconds even when small', async () => {
const body = { error: { message: 'rate limited', retry_after_ms: 5 } };
const response = new Response(JSON.stringify(body), { status: 429 });

const { rateLimit } = await handleErrorResponse(response);

expect(rateLimit?.retryAfterMs).toBe(5000);
expect(rateLimit?.retryAfterMs).toBe(5);
});

it('caps retryAfterMs at 5 minutes', async () => {
Expand All @@ -691,6 +691,58 @@ describe('Fetch Helpers Module', () => {
expect(rateLimit?.retryAfterMs).toBe(60000);
});

it('treats retry_after as seconds from body payload', async () => {
const body = { error: { message: 'rate limited', retry_after: 5 } };
const response = new Response(JSON.stringify(body), { status: 429 });

const { rateLimit } = await handleErrorResponse(response);

expect(rateLimit?.retryAfterMs).toBe(5000);
});

it('prefers retry_after_ms over retry_after when both are present', async () => {
const body = { error: { message: 'rate limited', retry_after_ms: 250, retry_after: 5 } };
const response = new Response(JSON.stringify(body), { status: 429 });

const { rateLimit } = await handleErrorResponse(response);

expect(rateLimit?.retryAfterMs).toBe(250);
});

it('clamps retry_after_ms zero and negative values to minimum delay', async () => {
const zeroResponse = new Response(
JSON.stringify({ error: { message: 'rate limited', retry_after_ms: 0 } }),
{ status: 429 },
);
const negativeResponse = new Response(
JSON.stringify({ error: { message: 'rate limited', retry_after_ms: -5 } }),
{ status: 429 },
);

const zeroRateLimit = await handleErrorResponse(zeroResponse);
const negativeRateLimit = await handleErrorResponse(negativeResponse);

expect(zeroRateLimit.rateLimit?.retryAfterMs).toBe(1);
expect(negativeRateLimit.rateLimit?.retryAfterMs).toBe(1);
});

it('clamps retry_after zero and negative values to minimum delay', async () => {
const zeroResponse = new Response(
JSON.stringify({ error: { message: 'rate limited', retry_after: 0 } }),
{ status: 429 },
);
const negativeResponse = new Response(
JSON.stringify({ error: { message: 'rate limited', retry_after: -5 } }),
{ status: 429 },
);

const zeroRateLimit = await handleErrorResponse(zeroResponse);
const negativeRateLimit = await handleErrorResponse(negativeResponse);

expect(zeroRateLimit.rateLimit?.retryAfterMs).toBe(1);
expect(negativeRateLimit.rateLimit?.retryAfterMs).toBe(1);
});

it('handles millisecond unix timestamp in reset header', async () => {
const futureTimestampMs = Date.now() + 45000;
const headers = new Headers({ 'x-ratelimit-reset': String(futureTimestampMs) });
Expand Down