From a100e2c906bd6eb139ce31d71a805380b68c0201 Mon Sep 17 00:00:00 2001 From: Ai Jeremi Date: Sat, 19 Sep 2026 05:05:00 +0200 Subject: [PATCH] fix(oauth): skip a second POST of a consumed Claude refresh token (#13874) HealthCheck queued behind a Layer 2 refresh still presented the old refresh_token after serializeRefresh released, which burned the family and then nulled the row. Re-check rotation inside the lane, record Layer 2 rotations, and keep the Claude refresh token on invalid_grant. Co-authored-by: Cursor Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com> --- .../claude-oauth-serialized-stale-refresh.md | 1 + open-sse/services/tokenRefresh.ts | 60 +++---- src/lib/tokenHealthCheck.ts | 43 +++-- .../token-refresh-race-comprehensive.test.ts | 24 +++ ...-refresh-serialized-stale-rotation.test.ts | 154 ++++++++++++++++++ tests/unit/token-refresh-service.test.ts | 3 +- ...enHealthCheck-unrecoverable-reread.test.ts | 65 ++++++++ 7 files changed, 290 insertions(+), 60 deletions(-) create mode 100644 changelog.d/fixes/claude-oauth-serialized-stale-refresh.md create mode 100644 tests/unit/token-refresh-serialized-stale-rotation.test.ts create mode 100644 tests/unit/tokenHealthCheck-unrecoverable-reread.test.ts diff --git a/changelog.d/fixes/claude-oauth-serialized-stale-refresh.md b/changelog.d/fixes/claude-oauth-serialized-stale-refresh.md new file mode 100644 index 0000000000..1ed817834f --- /dev/null +++ b/changelog.d/fixes/claude-oauth-serialized-stale-refresh.md @@ -0,0 +1 @@ +- **fix(oauth):** stop posting a Claude refresh token that another in-process refresh already consumed. Re-check the rotation map and DB inside `serializeRefresh` (both Layer 1 and Layer 2), record Layer 2 rotations, re-read the connection uncached on `invalid_grant`, and keep the Claude `refreshToken` instead of nulling it into sticky `no_refresh_token`. diff --git a/open-sse/services/tokenRefresh.ts b/open-sse/services/tokenRefresh.ts index 65b9bf5806..8ef561f43a 100755 --- a/open-sse/services/tokenRefresh.ts +++ b/open-sse/services/tokenRefresh.ts @@ -583,8 +583,11 @@ export async function getAccessToken( // the legacy `connectionId`-less path would silently swallow the callback, // leaving DB rows out of sync with rotated tokens (Codex/OpenAI). We still // resolve the promise to all waiters with the refreshed credentials. - const refreshPromise = serializeRefresh(provider, () => - _getAccessTokenInternal(provider, credentials, log, proxyConfig) + const refreshPromise = _getAccessTokenWithStalenessCheck( + provider, + credentials, + log, + proxyConfig ) .then(async (result) => { if (result?.accessToken && effectiveOnPersist) { @@ -620,17 +623,19 @@ export async function getAccessToken( } /** - * Internal helper: performs the DB staleness check then calls the actual refresh. - * Only called from the per-connection mutex path (Layer 1 above). + * Internal helper: waits for the rotation-group lane, then re-checks freshness + * BEFORE the network POST. Lookup/DB re-read must live inside serializeRefresh: + * a HealthCheck that snapshotted the old refresh_token can sit on the lane + * while Layer 2 consumes it; checking only before the wait still POSTs the + * consumed token and burns the family (Claude/Anthropic, Auth0 Codex). */ async function _getAccessTokenWithStalenessCheck(provider, credentials, log, proxyConfig) { - // ROTATION MAP CHECK (codex-multi-auth pattern): if this refresh_token was - // rotated very recently (within ROTATION_MAP_TTL_MS), reuse the cached new - // tokens INSTEAD of hitting upstream. Auth0 treats re-use of a rotated token - // as a security event and revokes the entire token family — fatal for - // multi-account Codex setups. The in-memory rotation map catches this even - // when the caller bypasses the DB staleness path (no connectionId, stale - // in-memory credentials in retries, etc.). + return serializeRefresh(provider, () => + _refreshWithFreshCredentials(provider, credentials, log, proxyConfig) + ); +} + +async function _refreshWithFreshCredentials(provider, credentials, log, proxyConfig) { const rotated = lookupRotation(provider, credentials.refreshToken); if (rotated) { log?.info?.( @@ -640,11 +645,6 @@ async function _getAccessTokenWithStalenessCheck(provider, credentials, log, pro return rotated.result; } - // RACE CONDITION PREVENTION: - // If the credentials object in memory is stale (e.g. it waited in a semaphore while another - // request refreshed the token), using its OLD refreshToken will cause the provider (e.g. OpenAI) - // to reject it with 'refresh_token_reused' and revoke the new token family. - // We MUST check if the DB has a newer token before proceeding with a network refresh. if (credentials.connectionId) { try { const { getProviderConnectionById } = await import("@/lib/db/providers"); @@ -659,31 +659,17 @@ async function _getAccessTokenWithStalenessCheck(provider, credentials, log, pro `Stale token detected in memory for ${provider}. Using refreshed token from DB.` ); - // If the DB token is not expired, we can just return it! if (dbExpiresAt > now + 60000) { - // 60 seconds buffer log?.info?.("TOKEN_REFRESH", `DB token is still valid. Skipping OAuth refresh.`); return { accessToken: dbConnection.accessToken, refreshToken: dbConnection.refreshToken, - // Return absolute expiresAt so downstream callers do NOT recompute lifetime - // from a relative expiresIn value (which would incorrectly extend the TTL). - // expiresIn intentionally omitted here. expiresAt: dbConnection.expiresAt, }; - } else { - // DB token is also expired, but it's the NEWEST one. We must use it to refresh. - credentials.refreshToken = dbConnection.refreshToken; - credentials.accessToken = dbConnection.accessToken; } + credentials.refreshToken = dbConnection.refreshToken; + credentials.accessToken = dbConnection.accessToken; } - // NOTE: Fix F (skip when DB == memory and DB > now+60s) was intentionally - // removed. The caller (checkAndRefreshToken) already decided to refresh - // because the token is within TOKEN_EXPIRY_BUFFER_MS of expiry. Re-checking - // with a tighter 60-second window here would skip legitimate refreshes and - // let near-expired tokens hit the upstream. Layer-1 mutex (per-connection) - // and Layer-2 dedup (token-hash) already prevent concurrent refreshes for - // the import-burst scenario. } } catch (e) { log?.warn?.( @@ -694,16 +680,8 @@ async function _getAccessTokenWithStalenessCheck(provider, credentials, log, pro } const oldRefreshToken = credentials.refreshToken; - // Front 1: serialize the network refresh across all connections of the same - // rotation group (e.g. Codex+openai share one Auth0 client) so two sibling - // accounts never refresh concurrently and trip Auth0 family revocation. - const result = await serializeRefresh(provider, () => - _getAccessTokenInternal(provider, credentials, log, proxyConfig) - ); + const result = await _getAccessTokenInternal(provider, credentials, log, proxyConfig); - // Record the rotation so subsequent stale callers can be redirected to the - // new tokens without re-hitting upstream (which would trigger Auth0 family - // revocation). Only records when the refresh actually rotated the token. if ( result && typeof result === "object" && diff --git a/src/lib/tokenHealthCheck.ts b/src/lib/tokenHealthCheck.ts index 6d27bd44d4..f9c380a95b 100644 --- a/src/lib/tokenHealthCheck.ts +++ b/src/lib/tokenHealthCheck.ts @@ -11,7 +11,11 @@ * updates the DB, and logs the result. */ -import { getProviderConnections, updateProviderConnection } from "@/lib/db/providers"; +import { + getProviderConnections, + getProviderConnectionById, + updateProviderConnection, +} from "@/lib/db/providers"; import { getCachedProviderConnectionById } from "@/lib/db/readCache"; import { getSettings } from "@/lib/db/settings"; import { resolveGuardedProxyConfig } from "@/lib/tokenHealthCheckProxyGuard"; @@ -42,6 +46,24 @@ const TICK_MS = 60 * 1000; // sweep interval: every 60 seconds (restored — #77 const DEFAULT_BATCH_SIZE = 20; const DEFAULT_HEALTH_CHECK_INTERVAL_MIN = 60; // default per-connection interval const EXPIRED_RETRY_MAX = 3; // max retry attempts for expired connections before giving up +const ROTATING_REFRESH_PROVIDERS = new Set([ + "codex", + "openai", + "kimi-coding", + "cline", + "kiro", + "amazon-q", + "gitlab-duo", + "claude", + "openference", +]); + +export function shouldNullRefreshTokenAfterUnrecoverable(provider: unknown): boolean { + const id = String(provider || "").toLowerCase(); + if (id === "claude") return false; + return ROTATING_REFRESH_PROVIDERS.has(id); +} + const EXPIRED_RETRY_BACKOFF_MIN = 5; // backoff between expired retries (minutes) function isBuildProcess(): boolean { @@ -897,17 +919,6 @@ export async function checkConnection(conn) { // and is the root cause of "adding account B invalidates account A" reports. // The interval path is kept ONLY for non-rotating providers where token state can // drift silently (e.g. cookie-based, opaque sessions without expires_at). - const ROTATING_REFRESH_PROVIDERS = new Set([ - "codex", - "openai", - "kimi-coding", - "cline", - "kiro", - "amazon-q", - "gitlab-duo", - "claude", - "openference", - ]); const isRotatingProvider = ROTATING_REFRESH_PROVIDERS.has( String(conn.provider || "").toLowerCase() ); @@ -1097,7 +1108,7 @@ export async function checkConnection(conn) { // Once used, the old token is permanently invalidated. // Retrying will never succeed → deactivate and stop the loop. if (isUnrecoverableRefreshError(result)) { - const currentConnection = await getCachedProviderConnectionById(conn.id); + const currentConnection = await getProviderConnectionById(conn.id); const credentialsChangedSinceSweep = !!currentConnection && (currentConnection.refreshToken !== attemptedRefreshToken || @@ -1157,11 +1168,7 @@ export async function checkConnection(conn) { // gemini) the stored refresh_token is the user's only recovery // artifact — nulling it caused #3679 (the connection reports "No valid refresh // token available" and can never recover even after re-activation). Preserve it. - // PRESERVE_REFRESH_TOKEN_PROVIDERS (Claude) opt out too: nulling on the first - // failure makes the #11414 retry budget above unreachable (#13183). - ...(isRotatingProvider && !preservesRefreshTokenOnUnrecoverable(conn.provider) - ? { refreshToken: null } - : {}), + ...(shouldNullRefreshTokenAfterUnrecoverable(conn.provider) ? { refreshToken: null } : {}), }); logError( `${LOG_PREFIX} ✗ ${conn.provider}/${getConnectionLogLabel(conn)} — ` + diff --git a/tests/unit/token-refresh-race-comprehensive.test.ts b/tests/unit/token-refresh-race-comprehensive.test.ts index 070cd85601..37df7ba9f4 100644 --- a/tests/unit/token-refresh-race-comprehensive.test.ts +++ b/tests/unit/token-refresh-race-comprehensive.test.ts @@ -176,3 +176,27 @@ test("Imports: base.ts imports runWithOnPersist from open-sse tokenRefresh", asy assert.match(src, /runWithOnPersist/); assert.match(src, /from\s+"\.\.\/services\/tokenRefresh\.ts"/); }); + + +test("serialized refresh re-checks rotation inside the lane, not before waiting", async () => { + const src = await read("open-sse/services/tokenRefresh.ts"); + const start = src.indexOf("async function _getAccessTokenWithStalenessCheck"); + const inner = src.indexOf("async function _refreshWithFreshCredentials"); + assert.ok(start >= 0 && inner > start, "staleness helper must wrap the freshness re-check"); + const wrapper = src.slice(start, inner); + assert.match( + wrapper, + /serializeRefresh\(provider,\s*\(\)\s*=>/, + "the network POST must stay behind serializeRefresh" + ); + assert.match(wrapper, /_refreshWithFreshCredentials/); + assert.doesNotMatch( + wrapper, + /lookupRotation/, + "lookupRotation before serializeRefresh is the race that burns a Claude refresh token" + ); + const body = src.slice(inner, inner + 2500); + assert.match(body, /lookupRotation\(/); + assert.match(body, /recordRotation\(/); + assert.match(body, /_getAccessTokenInternal\(/); +}); diff --git a/tests/unit/token-refresh-serialized-stale-rotation.test.ts b/tests/unit/token-refresh-serialized-stale-rotation.test.ts new file mode 100644 index 0000000000..4c9f468a43 --- /dev/null +++ b/tests/unit/token-refresh-serialized-stale-rotation.test.ts @@ -0,0 +1,154 @@ +import test from "node:test"; +import assert from "node:assert/strict"; + +const tokenRefresh = await import("../../open-sse/services/tokenRefresh.ts"); +const { __resetRefreshSerializerForTest } = await import("../../open-sse/services/refreshSerializer.ts"); +const { lookupRotation } = await import("../../open-sse/services/tokenRefresh/rotationMap.ts"); + +const { getAccessToken } = tokenRefresh; + +type LogLevel = "debug" | "info" | "warn" | "error"; +type LogEntry = { level: LogLevel; message: unknown }; + +function createLog() { + const entries: LogEntry[] = []; + const push = (level: LogLevel, args: unknown[]) => { + entries.push({ level, message: args[1] }); + }; + return { + entries, + debug: (...args: unknown[]) => push("debug", args), + info: (...args: unknown[]) => push("info", args), + warn: (...args: unknown[]) => push("warn", args), + error: (...args: unknown[]) => push("error", args), + }; +} + +function jsonResponse(body: unknown, status = 200) { + return new Response(JSON.stringify(body), { + status, + headers: { "content-type": "application/json" }, + }); +} + +function bodyToString(body: BodyInit | null | undefined) { + if (typeof body === "string") return body; + if (body instanceof URLSearchParams) return body.toString(); + return String(body ?? ""); +} + +function refreshTokenFromBody(body: BodyInit | null | undefined) { + return new URLSearchParams(bodyToString(body)).get("refresh_token"); +} + +async function withMockedFetch(fetchImpl: typeof fetch, fn: () => Promise) { + const originalFetch = globalThis.fetch; + globalThis.fetch = fetchImpl; + try { + return await fn(); + } finally { + globalThis.fetch = originalFetch; + } +} + +function resetRefreshState() { + tokenRefresh._clearTokenRotationMap(); + __resetRefreshSerializerForTest(); +} + +test.beforeEach(() => { + resetRefreshState(); +}); + +test("getAccessToken_Layer1QueuedBehindLayer2_DoesNotPostConsumedClaudeRefreshToken", async () => { + const previousSpacing = process.env.CODEX_REFRESH_SPACING_MS; + process.env.CODEX_REFRESH_SPACING_MS = "0"; + const log = createLog(); + const presented: string[] = []; + let firstPostEntered = false; + let releaseFirstPost!: () => void; + const holdFirstPost = new Promise((resolve) => { + releaseFirstPost = resolve; + }); + + try { + await withMockedFetch(async (_url, options = {}) => { + const presentedToken = refreshTokenFromBody(options.body); + presented.push(presentedToken || ""); + if (presentedToken === "old-rt" && !firstPostEntered) { + firstPostEntered = true; + await holdFirstPost; + return jsonResponse({ + access_token: "new-access", + refresh_token: "new-rt", + expires_in: 28800, + }); + } + if (presentedToken === "old-rt") { + return jsonResponse({ error: "invalid_grant", error_description: "refresh_token_reused" }, 400); + } + throw new Error(`unexpected refresh_token ${presentedToken}`); + }, async () => { + const layer2 = getAccessToken("claude", { refreshToken: "old-rt" }, log); + await new Promise((resolve, reject) => { + const started = Date.now(); + const tick = () => { + if (firstPostEntered) { + resolve(); + return; + } + if (Date.now() - started > 2000) { + reject(new Error("Layer 2 never reached the Anthropic token endpoint")); + return; + } + setTimeout(tick, 5); + }; + tick(); + }); + + const layer1 = getAccessToken( + "claude", + { connectionId: "healthcheck-conn", refreshToken: "old-rt" }, + log + ); + await new Promise((resolve) => setTimeout(resolve, 30)); + releaseFirstPost(); + + const [layer2Result, layer1Result] = await Promise.all([layer2, layer1]); + + assert.deepEqual(presented, ["old-rt"], "the consumed refresh token must be POSTed once"); + assert.equal(layer2Result?.accessToken, "new-access"); + assert.equal(layer2Result?.refreshToken, "new-rt"); + assert.equal(layer1Result?.accessToken, "new-access"); + assert.equal(layer1Result?.refreshToken, "new-rt"); + assert.notEqual( + (layer1Result as { error?: string } | null)?.error, + "unrecoverable_refresh_error", + "Layer 1 must reuse the rotated tokens instead of burning the family" + ); + }); + } finally { + if (previousSpacing === undefined) delete process.env.CODEX_REFRESH_SPACING_MS; + else process.env.CODEX_REFRESH_SPACING_MS = previousSpacing; + resetRefreshState(); + } +}); + +test("getAccessToken_Layer2Refresh_RecordsRotationForTheConsumedToken", async () => { + const log = createLog(); + + await withMockedFetch(async () => { + return jsonResponse({ + access_token: "layer2-access", + refresh_token: "layer2-new-rt", + expires_in: 28800, + }); + }, async () => { + const result = await getAccessToken("claude", { refreshToken: "layer2-old-rt" }, log); + assert.equal(result?.refreshToken, "layer2-new-rt"); + const cached = lookupRotation("claude", "layer2-old-rt"); + assert.ok(cached, "Layer 2 must record the rotation so a later stale caller can skip upstream"); + assert.equal(cached.result.refreshToken, "layer2-new-rt"); + assert.equal(cached.result.accessToken, "layer2-access"); + }); +}); diff --git a/tests/unit/token-refresh-service.test.ts b/tests/unit/token-refresh-service.test.ts index aa675efb2b..ffdf97d9f8 100644 --- a/tests/unit/token-refresh-service.test.ts +++ b/tests/unit/token-refresh-service.test.ts @@ -962,9 +962,10 @@ test("getAccessToken cleans the in-flight cache after resolve and separates diff log ); - assert.equal(fetchCount, 3); + assert.equal(fetchCount, 2, "same consumed refresh token is served from the rotation map"); assert.equal(first.accessToken, "access-refresh-a"); assert.equal(second.accessToken, "access-refresh-a"); + assert.equal(second.refreshToken, "next-refresh-a"); assert.equal(third.accessToken, "access-refresh-b"); } ); diff --git a/tests/unit/tokenHealthCheck-unrecoverable-reread.test.ts b/tests/unit/tokenHealthCheck-unrecoverable-reread.test.ts new file mode 100644 index 0000000000..3547d6894b --- /dev/null +++ b/tests/unit/tokenHealthCheck-unrecoverable-reread.test.ts @@ -0,0 +1,65 @@ +import test from "node:test"; +import assert from "node:assert/strict"; +import { readFile } from "node:fs/promises"; +import path from "node:path"; + +const root = path.resolve(import.meta.dirname, "../.."); +const src = await readFile(path.join(root, "src/lib/tokenHealthCheck.ts"), "utf8"); + +function unrecoverableSlice() { + const idx = src.indexOf("if (isUnrecoverableRefreshError(result))"); + assert.ok(idx >= 0, "unrecoverable refresh branch must exist"); + return src.slice(idx, idx + 3500); +} + +test("tokenHealthCheck_UnrecoverableRefresh_RereadsConnectionUncached", () => { + const slice = unrecoverableSlice(); + assert.match( + slice, + /getProviderConnectionById\(/, + "a concurrent Layer 2 persist can land between the sweep snapshot and invalid_grant; the cached row still holds the consumed refresh token and would skip the changed-since-sweep guard" + ); + assert.doesNotMatch( + slice, + /getCachedProviderConnectionById/, + "the 5s connection-by-id cache is how credentialsChangedSinceSweep missed the just-persisted rotation" + ); +}); + +test("tokenHealthCheck_UnrecoverableRefresh_DoesNotNullClaudeRefreshToken", () => { + const slice = unrecoverableSlice(); + assert.match( + slice, + /shouldNullRefreshTokenAfterUnrecoverable/, + "Claude rotating tokens must not be wiped on the first invalid_grant; the live access token plus the new refresh token in DB are still recoverable" + ); + assert.doesNotMatch( + slice, + /\.\.\.\(isRotatingProvider\s*\?\s*\{\s*refreshToken:\s*null\s*\}\s*:\s*\{\}\)/, + "the blanket rotating-provider null is what turns a dual-refresh race into sticky no_refresh_token" + ); +}); + +test("shouldNullRefreshTokenAfterUnrecoverable_Claude_IsFalse", async () => { + const { shouldNullRefreshTokenAfterUnrecoverable, stopTokenHealthCheck } = + await import("../../src/lib/tokenHealthCheck.ts"); + stopTokenHealthCheck(); + assert.equal(shouldNullRefreshTokenAfterUnrecoverable("claude"), false); + assert.equal(shouldNullRefreshTokenAfterUnrecoverable("Claude"), false); +}); + +test("shouldNullRefreshTokenAfterUnrecoverable_Codex_IsTrue", async () => { + const { shouldNullRefreshTokenAfterUnrecoverable, stopTokenHealthCheck } = + await import("../../src/lib/tokenHealthCheck.ts"); + stopTokenHealthCheck(); + assert.equal(shouldNullRefreshTokenAfterUnrecoverable("codex"), true); + assert.equal(shouldNullRefreshTokenAfterUnrecoverable("openai"), true); +}); + +test("shouldNullRefreshTokenAfterUnrecoverable_Google_IsFalse", async () => { + const { shouldNullRefreshTokenAfterUnrecoverable, stopTokenHealthCheck } = + await import("../../src/lib/tokenHealthCheck.ts"); + stopTokenHealthCheck(); + assert.equal(shouldNullRefreshTokenAfterUnrecoverable("gemini"), false); + assert.equal(shouldNullRefreshTokenAfterUnrecoverable("antigravity"), false); +});