diff --git a/src/codex/account-store.ts b/src/codex/account-store.ts index 09630ca312..5932a614b7 100644 --- a/src/codex/account-store.ts +++ b/src/codex/account-store.ts @@ -491,11 +491,22 @@ export async function getValidCodexToken(id: string): Promise throw new TokenRefreshError(reason, `Codex token refresh failed (${reason}); reauthenticate the account.`); } const data = (await res.json()) as { access_token: string; refresh_token?: string; expires_in: number }; + // Guard against a missing/non-finite/negative expires_in (malformed upstream + // response): a NaN expiry would never compare as expired, and a negative + // duration would stamp an already-past expiry — both block refresh semantics. + const expiresIn = + typeof data.expires_in === "number" && Number.isFinite(data.expires_in) && data.expires_in >= 0 + ? data.expires_in + : 3600; + // The computed timestamp itself must stay finite: Number.MAX_VALUE passes + // Number.isFinite but overflows to Infinity once multiplied by 1000. + const expiresAt = Date.now() + expiresIn * 1000; + const safeExpiresAt = Number.isFinite(expiresAt) ? expiresAt : Date.now() + 3600 * 1000; const updated: CodexAccountCredentials = { accessToken: data.access_token, refreshToken: data.refresh_token ?? lockedCred.refreshToken, - expiresAt: Date.now() + data.expires_in * 1000, + expiresAt: safeExpiresAt, chatgptAccountId: lockedCred.chatgptAccountId, }; if (!saveCodexAccountCredentialIfGeneration(id, startGeneration, updated)) { diff --git a/src/oauth/anthropic.ts b/src/oauth/anthropic.ts index a4a94eea7b..d4e0acdc03 100644 --- a/src/oauth/anthropic.ts +++ b/src/oauth/anthropic.ts @@ -81,10 +81,21 @@ function parseTokenResponse(responseBody: string): AnthropicTokenResponse { function credsFrom(data: AnthropicTokenResponse, refreshFallback?: string): OAuthCredentials { const accountUuid = data.account?.uuid; const email = data.account?.email_address; + // Guard against a missing/non-finite/negative expires_in (malformed upstream + // response): a NaN expiry would never compare as expired, and a negative + // duration would stamp an already-past expiry — both block refresh semantics. + const expiresIn = + typeof data.expires_in === "number" && Number.isFinite(data.expires_in) && data.expires_in >= 0 + ? data.expires_in + : 3600; + // The computed timestamp itself must stay finite: Number.MAX_VALUE passes + // Number.isFinite but overflows to Infinity once multiplied by 1000. + const computedExpires = Date.now() + expiresIn * 1000 - 5 * 60 * 1000; + const expires = Number.isFinite(computedExpires) ? computedExpires : Date.now() + 3600 * 1000 - 5 * 60 * 1000; return { refresh: data.refresh_token || refreshFallback || "", access: data.access_token, - expires: Date.now() + data.expires_in * 1000 - 5 * 60 * 1000, + expires, accountId: typeof accountUuid === "string" && accountUuid.length > 0 ? accountUuid : undefined, email: typeof email === "string" && email.length > 0 ? email : undefined, }; diff --git a/src/oauth/chatgpt.ts b/src/oauth/chatgpt.ts index b8089737c0..f4ecc7f8a9 100644 --- a/src/oauth/chatgpt.ts +++ b/src/oauth/chatgpt.ts @@ -49,10 +49,21 @@ export function extractEmail(idToken?: string, accessToken?: string): string | u function credsFromToken(data: Record): OAuthCredentials { const idToken = typeof data.id_token === "string" ? data.id_token : undefined; const accessToken = data.access_token as string; + // ?? only guards null/undefined; NaN or a string expires_in would otherwise + // produce a NaN expiry that never compares as expired, and a negative duration + // would stamp an already-past expiry — both block refresh semantics. + const expiresIn = + typeof data.expires_in === "number" && Number.isFinite(data.expires_in) && data.expires_in >= 0 + ? data.expires_in + : 3600; + // The computed timestamp itself must stay finite: Number.MAX_VALUE passes + // Number.isFinite but overflows to Infinity once multiplied by 1000. + const computedExpires = Date.now() + expiresIn * 1000; + const expires = Number.isFinite(computedExpires) ? computedExpires : Date.now() + 3600 * 1000; return { access: accessToken, refresh: (data.refresh_token as string) ?? "", - expires: Date.now() + ((data.expires_in as number) ?? 3600) * 1000, + expires, accountId: extractAccountId(idToken, accessToken), email: extractEmail(idToken, accessToken), }; diff --git a/src/oauth/kimi.ts b/src/oauth/kimi.ts index be14640e9b..bfba15a587 100644 --- a/src/oauth/kimi.ts +++ b/src/oauth/kimi.ts @@ -151,7 +151,21 @@ async function requestDeviceAuthorization(): Promise<{ } function parseTokenPayload(payload: TokenResponse, refreshFallback?: string): OAuthCredentials { - if (!payload.access_token || typeof payload.expires_in !== "number") { + // Number.isFinite is required here: typeof NaN === "number", so the type check + // alone would let a NaN expires_in through and produce a never-refreshing expiry. + // Negative durations would stamp an already-past expiry — also malformed. + // The computed timestamp itself must also stay finite: Number.MAX_VALUE passes + // Number.isFinite but overflows to Infinity once multiplied by 1000. + if ( + !payload.access_token + || typeof payload.expires_in !== "number" + || !Number.isFinite(payload.expires_in) + || payload.expires_in < 0 + ) { + throw new Error("Kimi token response missing required fields"); + } + const expires = Date.now() + payload.expires_in * 1000 - OAUTH_EXPIRY_SKEW_MS; + if (!Number.isFinite(expires)) { throw new Error("Kimi token response missing required fields"); } const refresh = payload.refresh_token ?? refreshFallback; @@ -160,7 +174,7 @@ function parseTokenPayload(payload: TokenResponse, refreshFallback?: string): OA return { access: payload.access_token, refresh, - expires: Date.now() + payload.expires_in * 1000 - OAUTH_EXPIRY_SKEW_MS, + expires, ...identity, }; } diff --git a/tests/anthropic-hardening.test.ts b/tests/anthropic-hardening.test.ts index fe78b500bd..6b01c4e972 100644 --- a/tests/anthropic-hardening.test.ts +++ b/tests/anthropic-hardening.test.ts @@ -43,6 +43,50 @@ describe("anthropic provider hardening", () => { await expect(refreshAnthropicToken("secret")).rejects.toMatchObject({ httpStatus: 503, oauthError: undefined }); }); + test("refresh with a non-finite expires_in falls back to the 3600s default (3300s after skew)", async () => { + globalThis.fetch = (async () => new Response( + // JSON.stringify would turn Infinity into null; hand-write 1e999 so JSON.parse + // yields Infinity, which would previously produce expires: NaN (never refreshing). + '{"access_token":"at","refresh_token":"rt","expires_in":1e999}', + { status: 200 }, + )) as typeof fetch; + + const before = Date.now(); + const cred = await refreshAnthropicToken("secret"); + expect(Number.isFinite(cred.expires)).toBe(true); + expect(cred.expires).toBeGreaterThan(before); + // 3600s default minus the 5-minute refresh skew. + expect(Math.abs(cred.expires - (before + 3300 * 1000))).toBeLessThan(30_000); + }); + + test("refresh with an overflowing expires_in falls back to the 3600s default (3300s after skew)", async () => { + globalThis.fetch = (async () => new Response( + // Number.MAX_VALUE passes Number.isFinite but overflows to Infinity when + // multiplied by 1000 — the computed expiry must still be guarded. + '{"access_token":"at","refresh_token":"rt","expires_in":1.7976931348623157e308}', + { status: 200 }, + )) as typeof fetch; + + const before = Date.now(); + const cred = await refreshAnthropicToken("secret"); + expect(Number.isFinite(cred.expires)).toBe(true); + expect(cred.expires).toBeGreaterThan(before); + expect(Math.abs(cred.expires - (before + 3300 * 1000))).toBeLessThan(30_000); + }); + + test("refresh with a negative expires_in falls back to the 3600s default (3300s after skew)", async () => { + globalThis.fetch = (async () => new Response( + JSON.stringify({ access_token: "at", refresh_token: "rt", expires_in: -1 }), + { status: 200 }, + )) as typeof fetch; + + const before = Date.now(); + const cred = await refreshAnthropicToken("secret"); + expect(Number.isFinite(cred.expires)).toBe(true); + expect(cred.expires).toBeGreaterThan(before); + expect(Math.abs(cred.expires - (before + 3300 * 1000))).toBeLessThan(30_000); + }); + test("key mode rejects a blank API key", async () => { const adapter = createAnthropicAdapter(provider({ apiKey: " " })); diff --git a/tests/chatgpt-token-expiry.test.ts b/tests/chatgpt-token-expiry.test.ts new file mode 100644 index 0000000000..7b0460e1f9 --- /dev/null +++ b/tests/chatgpt-token-expiry.test.ts @@ -0,0 +1,66 @@ +import { afterEach, describe, expect, test } from "bun:test"; +import { refreshChatGPTToken } from "../src/oauth/chatgpt"; + +const originalFetch = globalThis.fetch; +afterEach(() => { globalThis.fetch = originalFetch; }); + +const FALLBACK_MS = 3600 * 1000; +const TOLERANCE_MS = 30_000; + +describe("ChatGPT OAuth token response parsing", () => { + test("refresh with a non-finite expires_in falls back to the 3600s default", async () => { + globalThis.fetch = (async () => new Response( + // JSON.stringify would turn Infinity into null; hand-write 1e999 so JSON.parse + // yields Infinity, which ?? 3600 alone would let through (NaN expiry, never refreshing). + '{"access_token":"at","refresh_token":"rt","expires_in":1e999}', + { status: 200 }, + )) as typeof fetch; + + const before = Date.now(); + const cred = await refreshChatGPTToken("secret"); + expect(Number.isFinite(cred.expires)).toBe(true); + expect(cred.expires).toBeGreaterThan(before); + expect(Math.abs(cred.expires - (before + FALLBACK_MS))).toBeLessThan(TOLERANCE_MS); + }); + + test("refresh with a string expires_in falls back to the 3600s default", async () => { + globalThis.fetch = (async () => new Response( + JSON.stringify({ access_token: "at", refresh_token: "rt", expires_in: "garbage" }), + { status: 200 }, + )) as typeof fetch; + + const before = Date.now(); + const cred = await refreshChatGPTToken("secret"); + expect(Number.isFinite(cred.expires)).toBe(true); + expect(cred.expires).toBeGreaterThan(before); + expect(Math.abs(cred.expires - (before + FALLBACK_MS))).toBeLessThan(TOLERANCE_MS); + }); + + test("refresh with an overflowing expires_in falls back to the 3600s default", async () => { + globalThis.fetch = (async () => new Response( + // Number.MAX_VALUE passes Number.isFinite but overflows to Infinity when + // multiplied by 1000 — the computed expiry must still be guarded. + '{"access_token":"at","refresh_token":"rt","expires_in":1.7976931348623157e308}', + { status: 200 }, + )) as typeof fetch; + + const before = Date.now(); + const cred = await refreshChatGPTToken("secret"); + expect(Number.isFinite(cred.expires)).toBe(true); + expect(cred.expires).toBeGreaterThan(before); + expect(Math.abs(cred.expires - (before + FALLBACK_MS))).toBeLessThan(TOLERANCE_MS); + }); + + test("refresh with a negative expires_in falls back to the 3600s default", async () => { + globalThis.fetch = (async () => new Response( + JSON.stringify({ access_token: "at", refresh_token: "rt", expires_in: -1 }), + { status: 200 }, + )) as typeof fetch; + + const before = Date.now(); + const cred = await refreshChatGPTToken("secret"); + expect(Number.isFinite(cred.expires)).toBe(true); + expect(cred.expires).toBeGreaterThan(before); + expect(Math.abs(cred.expires - (before + FALLBACK_MS))).toBeLessThan(TOLERANCE_MS); + }); +}); diff --git a/tests/codex-account-store.test.ts b/tests/codex-account-store.test.ts index eed7c09959..63f19cf0b3 100644 --- a/tests/codex-account-store.test.ts +++ b/tests/codex-account-store.test.ts @@ -261,6 +261,85 @@ describe("codex-account-store CRUD", () => { } }); + test("refresh with a non-finite expires_in falls back to the 3600s default", async () => { + const { + getCodexAccountCredential, + getValidCodexToken, + saveCodexAccountCredential, + } = await import("../src/codex/account-store"); + saveCodexAccountCredential("refresh-bad-expiry", { accessToken: "old", refreshToken: "old-r", expiresAt: 0, chatgptAccountId: "acc" }); + const originalFetch = globalThis.fetch; + // JSON.stringify turns NaN into null; hand-write 1e999 so JSON.parse yields Infinity, + // the realistic corrupt shape that would previously produce expiresAt: NaN. + globalThis.fetch = (async () => new Response( + '{"access_token":"new","refresh_token":"new-r","expires_in":1e999}', + { status: 200 }, + )) as typeof fetch; + + try { + const before = Date.now(); + await getValidCodexToken("refresh-bad-expiry"); + const stored = getCodexAccountCredential("refresh-bad-expiry")!; + expect(Number.isFinite(stored.expiresAt)).toBe(true); + expect(stored.expiresAt).toBeGreaterThan(before); + expect(Math.abs(stored.expiresAt - (before + 3600 * 1000))).toBeLessThan(30_000); + } finally { + globalThis.fetch = originalFetch; + } + }); + + test("refresh with an overflowing expires_in falls back to the 3600s default", async () => { + const { + getCodexAccountCredential, + getValidCodexToken, + saveCodexAccountCredential, + } = await import("../src/codex/account-store"); + saveCodexAccountCredential("refresh-overflow-expiry", { accessToken: "old", refreshToken: "old-r", expiresAt: 0, chatgptAccountId: "acc" }); + const originalFetch = globalThis.fetch; + // Number.MAX_VALUE passes Number.isFinite but overflows to Infinity when + // multiplied by 1000 — the computed expiresAt must still be guarded. + globalThis.fetch = (async () => new Response( + '{"access_token":"new","refresh_token":"new-r","expires_in":1.7976931348623157e308}', + { status: 200 }, + )) as typeof fetch; + + try { + const before = Date.now(); + await getValidCodexToken("refresh-overflow-expiry"); + const stored = getCodexAccountCredential("refresh-overflow-expiry")!; + expect(Number.isFinite(stored.expiresAt)).toBe(true); + expect(stored.expiresAt).toBeGreaterThan(before); + expect(Math.abs(stored.expiresAt - (before + 3600 * 1000))).toBeLessThan(30_000); + } finally { + globalThis.fetch = originalFetch; + } + }); + + test("refresh with a negative expires_in falls back to the 3600s default", async () => { + const { + getCodexAccountCredential, + getValidCodexToken, + saveCodexAccountCredential, + } = await import("../src/codex/account-store"); + saveCodexAccountCredential("refresh-negative-expiry", { accessToken: "old", refreshToken: "old-r", expiresAt: 0, chatgptAccountId: "acc" }); + const originalFetch = globalThis.fetch; + globalThis.fetch = (async () => new Response( + JSON.stringify({ access_token: "new", refresh_token: "new-r", expires_in: -1 }), + { status: 200 }, + )) as typeof fetch; + + try { + const before = Date.now(); + await getValidCodexToken("refresh-negative-expiry"); + const stored = getCodexAccountCredential("refresh-negative-expiry")!; + expect(Number.isFinite(stored.expiresAt)).toBe(true); + expect(stored.expiresAt).toBeGreaterThan(before); + expect(Math.abs(stored.expiresAt - (before + 3600 * 1000))).toBeLessThan(30_000); + } finally { + globalThis.fetch = originalFetch; + } + }); + test("refresh waits behind file lock and reuses credential refreshed by another process", async () => { const { getValidCodexToken, diff --git a/tests/kimi-oauth-identity.test.ts b/tests/kimi-oauth-identity.test.ts index cc3bc78a48..00070311d9 100644 --- a/tests/kimi-oauth-identity.test.ts +++ b/tests/kimi-oauth-identity.test.ts @@ -75,6 +75,37 @@ describe("Kimi token-response wiring (production parseTokenPayload path)", () => expect(cred.accountId).toBe("wired-user"); expect(cred.email).toBe(["w", String.fromCharCode(64), "kimi.example"].join("")); }); + + test("refreshKimiToken rejects a non-finite expires_in (would otherwise yield NaN expiry)", async () => { + globalThis.fetch = (async () => new Response( + // JSON.stringify would turn Infinity into null; hand-write 1e999 so JSON.parse + // yields Infinity, which typeof === "number" alone would let through. + `{"access_token":"at","refresh_token":"rt","expires_in":1e999}`, + { status: 200 }, + )) as typeof fetch; + + await expect(refreshKimiToken("old-refresh")).rejects.toThrow("missing required fields"); + }); + + test("refreshKimiToken rejects an overflowing expires_in (finite input, Infinity expiry)", async () => { + globalThis.fetch = (async () => new Response( + // Number.MAX_VALUE passes Number.isFinite but overflows to Infinity when + // multiplied by 1000 — the computed expiry must be rejected as malformed. + `{"access_token":"at","refresh_token":"rt","expires_in":1.7976931348623157e308}`, + { status: 200 }, + )) as typeof fetch; + + await expect(refreshKimiToken("old-refresh")).rejects.toThrow("missing required fields"); + }); + + test("refreshKimiToken rejects a negative expires_in (already-past expiry)", async () => { + globalThis.fetch = (async () => new Response( + JSON.stringify({ access_token: "at", refresh_token: "rt", expires_in: -1 }), + { status: 200 }, + )) as typeof fetch; + + await expect(refreshKimiToken("old-refresh")).rejects.toThrow("missing required fields"); + }); }); describe("Kimi multiauth via saveCredential", () => {