From 7100d8b12d98164542b0589f19e88a04f850e408 Mon Sep 17 00:00:00 2001 From: roboomp Date: Fri, 31 Jul 2026 14:04:27 +0000 Subject: [PATCH] fix(ai): refresh expired oauth credential on observed mismatch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit refreshStoredOAuthCredential's observed-credential mismatch guard returned the stored row without a freshness check. When a concurrent usage-fetch cycle rotated the in-memory selected credential out from under a request and the stored row was itself expired, the guard handed back the dead token, which getOAuthApiKey then refused — surfacing as a misleading "No API key found for " during plan finalization. Gate both the pre-lease and post-lease mismatch guards on the stored credential still being fresh; expired stored copies now fall through to a normal refresh. Fixes #7179 --- packages/ai/CHANGELOG.md | 4 + packages/ai/src/auth-storage.ts | 24 ++++- .../auth-storage-oauth-refresh-race.test.ts | 92 +++++++++++++++++++ 3 files changed, 116 insertions(+), 4 deletions(-) diff --git a/packages/ai/CHANGELOG.md b/packages/ai/CHANGELOG.md index 14ccbea6f..ab54e6d9c 100644 --- a/packages/ai/CHANGELOG.md +++ b/packages/ai/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Fixed `AuthStorage.refreshStoredOAuthCredential` returning an expired-but-refreshable OAuth credential without refreshing it when the caller's observed credential mismatched the stored row. A concurrent usage-fetch cycle could rotate the in-memory selected credential out from under a request (e.g. plan finalization); the observed-mismatch guard then adopted the stored copy verbatim, and if that copy was itself expired it flowed into `getOAuthApiKey`, which refused it — surfacing as a misleading `No API key found for `. The mismatch guard now only short-circuits when the stored credential is still fresh; expired stored copies fall through to a normal refresh ([#7179](https://github.com/can1357/oh-my-pi/issues/7179)). + ## [17.2.1] - 2026-07-30 ### Added diff --git a/packages/ai/src/auth-storage.ts b/packages/ai/src/auth-storage.ts index 492cbc804..45dac9817 100644 --- a/packages/ai/src/auth-storage.ts +++ b/packages/ai/src/auth-storage.ts @@ -2327,10 +2327,19 @@ export class AuthStorage { if (!current) { return { credential: undefined, refreshed: false, removed: false }; } - if (options.observedCredential && !authCredentialEquals(current, options.observedCredential)) { + const currentIsFresh = Date.now() + refreshSkewMs < current.expires; + // A peer rotated the credential out from under the caller's observation. + // Adopt the stored copy only when it is still usable; a stored copy that + // is itself expired must fall through to a refresh rather than be handed + // back and fail the downstream `getOAuthApiKey` expiry precondition. + if ( + options.observedCredential && + !authCredentialEquals(current, options.observedCredential) && + currentIsFresh + ) { return { credential: current, refreshed: false, removed: false }; } - if (!options.forceRefresh && Date.now() + refreshSkewMs < current.expires) { + if (!options.forceRefresh && currentIsFresh) { return { credential: current, refreshed: false, removed: false }; } if (options.canRefresh && !options.canRefresh(current)) { @@ -2370,10 +2379,17 @@ export class AuthStorage { if (!current) { return { credential: undefined, refreshed: false, removed: false }; } - if (options.observedCredential && !authCredentialEquals(current, options.observedCredential)) { + const currentIsFresh = Date.now() + refreshSkewMs < current.expires; + // Re-check after acquiring the lease: only adopt the stored copy on an + // observed mismatch when it is still usable, mirroring the pre-lease guard. + if ( + options.observedCredential && + !authCredentialEquals(current, options.observedCredential) && + currentIsFresh + ) { return { credential: current, refreshed: false, removed: false }; } - if (!options.forceRefresh && Date.now() + refreshSkewMs < current.expires) { + if (!options.forceRefresh && currentIsFresh) { return { credential: current, refreshed: false, removed: false }; } if (options.canRefresh && !options.canRefresh(current)) { diff --git a/packages/ai/test/auth-storage-oauth-refresh-race.test.ts b/packages/ai/test/auth-storage-oauth-refresh-race.test.ts index 1a2e9c32f..233de7b61 100644 --- a/packages/ai/test/auth-storage-oauth-refresh-race.test.ts +++ b/packages/ai/test/auth-storage-oauth-refresh-race.test.ts @@ -757,4 +757,96 @@ describe("AuthStorage OAuth refresh race", () => { refresh: "refresh-old", }); }); + + test("refreshes an expired stored credential even when it mismatches the observed copy", async () => { + if (!authStorage || !store) throw new Error("test setup failed"); + + const now = Date.parse("2026-07-10T13:00:00.000Z"); + setSystemTime(new Date(now)); + await authStorage.set("unit-oauth-observed-mismatch-expired", [ + { + type: "oauth", + access: "stored-access-expired", + refresh: "stored-refresh", + expires: now - 60_000, + }, + ]); + + // The caller observed a different (also stale) copy of the credential — the + // exact state the finalize path hits when a concurrent usage-fetch cycle + // rotates the in-memory credential out from under the request between the + // selection sync and the refresh read. Before the fix the observed-mismatch + // guard returned the stored copy verbatim without checking whether it was + // usable, so the expired token flowed straight into getOAuthApiKey. + const observed = { + type: "oauth" as const, + access: "observed-access-different", + refresh: "stored-refresh", + expires: now - 120_000, + }; + + let refreshCalled = false; + const result = await authStorage.refreshStoredOAuthCredential("unit-oauth-observed-mismatch-expired", { + observedCredential: observed, + credentialFromRow: row => row, + forceRefresh: false, + refresh: async credential => { + refreshCalled = true; + return { + ...credential, + access: "refreshed-access", + refresh: "stored-refresh", + expires: now + 60 * 60_000, + }; + }, + }); + + // The expired stored credential must be refreshed, not returned as-is. + expect(refreshCalled).toBe(true); + expect(result).toMatchObject({ refreshed: true, removed: false }); + expect(result.credential).toMatchObject({ type: "oauth", access: "refreshed-access" }); + const stored = store.listAuthCredentials("unit-oauth-observed-mismatch-expired"); + expect(stored[0]?.credential).toMatchObject({ type: "oauth", access: "refreshed-access" }); + }); + + test("adopts a fresh stored credential without refreshing when it mismatches the observed copy", async () => { + if (!authStorage || !store) throw new Error("test setup failed"); + + const now = Date.parse("2026-07-10T13:30:00.000Z"); + setSystemTime(new Date(now)); + await authStorage.set("unit-oauth-observed-mismatch-fresh", [ + { + type: "oauth", + access: "peer-rotated-access", + refresh: "peer-rotated-refresh", + expires: now + 60 * 60_000, + }, + ]); + + // A peer already rotated the row to a fresh token; the caller's observed + // copy is stale. The fresh stored copy must be adopted without a redundant + // refresh (which would waste a rotation and could invalidate the peer's + // token). + const observed = { + type: "oauth" as const, + access: "stale-observed-access", + refresh: "stale-observed-refresh", + expires: now - 60_000, + }; + + let refreshCalled = false; + const result = await authStorage.refreshStoredOAuthCredential("unit-oauth-observed-mismatch-fresh", { + observedCredential: observed, + credentialFromRow: row => row, + forceRefresh: false, + refresh: async credential => { + refreshCalled = true; + return { ...credential, access: "should-not-be-used", expires: now + 60 * 60_000 }; + }, + }); + + expect(refreshCalled).toBe(false); + expect(result).toMatchObject({ refreshed: false, removed: false }); + expect(result.credential).toMatchObject({ type: "oauth", access: "peer-rotated-access" }); + }); });