fix(ai): refresh expired oauth credential on observed mismatch
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 <provider>" 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
This commit is contained in:
@@ -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 <provider>`. 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
|
||||
|
||||
@@ -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)) {
|
||||
|
||||
@@ -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" });
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user