diff --git a/extensions/openai/openai-codex-cli-auth.test.ts b/extensions/openai/openai-codex-cli-auth.test.ts index 097ec9194ed..2c528ff5f86 100644 --- a/extensions/openai/openai-codex-cli-auth.test.ts +++ b/extensions/openai/openai-codex-cli-auth.test.ts @@ -233,6 +233,7 @@ describe("readOpenAICodexCliOAuthProfile", () => { access: "expired-local-access", refresh: "expired-local-refresh", expires: Date.now() - 60_000, + accountId: "acct_123", }, }, }, @@ -249,6 +250,43 @@ describe("readOpenAICodexCliOAuthProfile", () => { }); }); + it("refuses cli bootstrap when the stored default profile is expired but identity mismatches", () => { + const accessToken = buildJwt({ + exp: Math.floor(Date.now() / 1000) + 600, + "https://api.openai.com/profile": { + email: "codex@example.com", + }, + }); + vi.spyOn(fs, "readFileSync").mockReturnValue( + JSON.stringify({ + auth_mode: "chatgpt", + tokens: { + access_token: accessToken, + refresh_token: "refresh-token", + account_id: "acct_123", + }, + }), + ); + + const parsed = readOpenAICodexCliOAuthProfile({ + store: { + version: 1, + profiles: { + [OPENAI_CODEX_DEFAULT_PROFILE_ID]: { + type: "oauth", + provider: "openai-codex", + access: "expired-local-access", + refresh: "expired-local-refresh", + expires: Date.now() - 60_000, + accountId: "acct_local", + }, + }, + }, + }); + + expect(parsed).toBeNull(); + }); + it("allows the runtime-only Codex CLI profile when the stored default already matches", () => { const accessToken = buildJwt({ exp: Math.floor(Date.now() / 1000) + 600, diff --git a/extensions/openai/openai-codex-cli-auth.ts b/extensions/openai/openai-codex-cli-auth.ts index 9b24a1cf471..f847ac18a40 100644 --- a/extensions/openai/openai-codex-cli-auth.ts +++ b/extensions/openai/openai-codex-cli-auth.ts @@ -89,29 +89,30 @@ function normalizeAuthEmailToken(value: string | undefined): string | undefined return normalizeAuthIdentityToken(value)?.toLowerCase(); } -// Keep this overwrite guard aligned with the canonical OAuth identity-copy rule -// in src/agents/auth-profiles/oauth.ts without widening the plugin SDK surface. -function isSafeToReplaceStoredIdentity( - existing: Pick, - incoming: Pick, +function hasIdentityContinuity( + existing: Pick | undefined, + incoming: OAuthCredential, ): boolean { + if (!existing) { + return true; + } + if (oauthCredentialMatches(existing as OAuthCredential, incoming)) { + return true; + } + const existingAccountId = normalizeAuthIdentityToken(existing.accountId); const incomingAccountId = normalizeAuthIdentityToken(incoming.accountId); - const existingEmail = normalizeAuthEmailToken(existing.email); - const incomingEmail = normalizeAuthEmailToken(incoming.email); - if (existingAccountId !== undefined && incomingAccountId !== undefined) { return existingAccountId === incomingAccountId; } + + const existingEmail = normalizeAuthEmailToken(existing.email); + const incomingEmail = normalizeAuthEmailToken(incoming.email); if (existingEmail !== undefined && incomingEmail !== undefined) { return existingEmail === incomingEmail; } - const existingHasIdentity = existingAccountId !== undefined || existingEmail !== undefined; - if (existingHasIdentity) { - return false; - } - return true; + return false; } export function readOpenAICodexCliOAuthProfile(params: { @@ -152,17 +153,13 @@ export function readOpenAICodexCliOAuthProfile(params: { }); return null; } - if ( - existingOAuth && - hasUsableOAuthCredential(existingOAuth) && - !oauthCredentialMatches(existingOAuth, credential) - ) { + if (!hasIdentityContinuity(existingOAuth, credential)) { return null; } if ( existingOAuth && - !oauthCredentialMatches(existingOAuth, credential) && - !isSafeToReplaceStoredIdentity(existingOAuth, credential) + hasUsableOAuthCredential(existingOAuth) && + !oauthCredentialMatches(existingOAuth, credential) ) { return null; } diff --git a/src/agents/auth-health.test.ts b/src/agents/auth-health.test.ts index c40d7f9cf6e..1f7dbb1a6ff 100644 --- a/src/agents/auth-health.test.ts +++ b/src/agents/auth-health.test.ts @@ -127,6 +127,7 @@ describe("buildAuthHealthSummary", () => { access: "expired-access", refresh: "expired-refresh", expires: now - 10_000, + accountId: "acct-cli", }, }, }; diff --git a/src/agents/auth-profiles.external-cli-sync.test.ts b/src/agents/auth-profiles.external-cli-sync.test.ts index 4b46f002c8e..ae8461b4fcc 100644 --- a/src/agents/auth-profiles.external-cli-sync.test.ts +++ b/src/agents/auth-profiles.external-cli-sync.test.ts @@ -154,6 +154,7 @@ describe("external cli oauth resolution", () => { access: "fresh-cli-access", refresh: "fresh-cli-refresh", expires: Date.now() + 5 * 24 * 60 * 60_000, + accountId: "acct-123", }); expect( @@ -174,6 +175,7 @@ describe("external cli oauth resolution", () => { access: "expired-local-access", refresh: "expired-local-refresh", expires: Date.now() - 60_000, + accountId: "acct-123", }), imported, }), @@ -255,6 +257,7 @@ describe("external cli oauth resolution", () => { access: "codex-fresh-access", refresh: "codex-fresh-refresh", expires: Date.now() + 5 * 24 * 60 * 60_000, + accountId: "acct-codex", }), ); mocks.readMiniMaxCliCredentialsCached.mockReturnValue( @@ -263,6 +266,7 @@ describe("external cli oauth resolution", () => { access: "minimax-fresh-access", refresh: "minimax-fresh-refresh", expires: Date.now() + 5 * 24 * 60 * 60_000, + email: "minimax@example.com", }), ); @@ -274,12 +278,14 @@ describe("external cli oauth resolution", () => { access: "codex-stale-access", refresh: "codex-stale-refresh", expires: Date.now() - 5_000, + accountId: "acct-codex", }), [MINIMAX_CLI_PROFILE_ID]: makeOAuthCredential({ provider: "minimax-portal", access: "minimax-stale-access", refresh: "minimax-stale-refresh", expires: Date.now() - 5_000, + email: "minimax@example.com", }), }, }); diff --git a/src/agents/auth-profiles/external-cli-sync.ts b/src/agents/auth-profiles/external-cli-sync.ts index ab1104eb7cd..4a30e22a953 100644 --- a/src/agents/auth-profiles/external-cli-sync.ts +++ b/src/agents/auth-profiles/external-cli-sync.ts @@ -11,6 +11,7 @@ import { log } from "./constants.js"; import { areOAuthCredentialsEquivalent, hasUsableOAuthCredential, + isSafeToOverwriteStoredOAuthIdentity, shouldBootstrapFromExternalCliCredential, shouldReplaceStoredOAuthCredential, } from "./oauth-manager.js"; @@ -19,6 +20,7 @@ import type { AuthProfileStore, OAuthCredential } from "./types.js"; export { areOAuthCredentialsEquivalent, hasUsableOAuthCredential, + isSafeToOverwriteStoredOAuthIdentity, shouldBootstrapFromExternalCliCredential, shouldReplaceStoredOAuthCredential, } from "./oauth-manager.js"; @@ -147,6 +149,17 @@ export function resolveExternalCliAuthProfiles( }); continue; } + if ( + existingOAuth && + !isSafeToOverwriteStoredOAuthIdentity(existingOAuth, creds) && + !areOAuthCredentialsEquivalent(existingOAuth, creds) + ) { + log.warn("refused external cli oauth bootstrap: identity mismatch or missing binding", { + profileId: providerConfig.profileId, + provider: providerConfig.provider, + }); + continue; + } if ( !shouldBootstrapFromExternalCliCredential({ existing: existingOAuth, diff --git a/src/agents/auth-profiles/external-oauth.test.ts b/src/agents/auth-profiles/external-oauth.test.ts index af4de0d251b..6f88462099e 100644 --- a/src/agents/auth-profiles/external-oauth.test.ts +++ b/src/agents/auth-profiles/external-oauth.test.ts @@ -130,6 +130,8 @@ describe("auth external oauth helpers", () => { access: "fresh-cli-access-token", refresh: "fresh-cli-refresh-token", expires: createUsableOAuthExpiry(), + accountId: "acct-cli", + expires: createUsableOAuthExpiry(), }), ); @@ -139,6 +141,7 @@ describe("auth external oauth helpers", () => { access: "stale-store-access-token", refresh: "stale-store-refresh-token", expires: Date.now() - 60_000, + accountId: "acct-cli", }), }), ); diff --git a/src/agents/auth-profiles/oauth-manager.test.ts b/src/agents/auth-profiles/oauth-manager.test.ts new file mode 100644 index 00000000000..71a3fbdd746 --- /dev/null +++ b/src/agents/auth-profiles/oauth-manager.test.ts @@ -0,0 +1,71 @@ +import { describe, expect, it } from "vitest"; +import { isSafeToOverwriteStoredOAuthIdentity, OAuthManagerRefreshError } from "./oauth-manager.js"; +import type { AuthProfileStore, OAuthCredential } from "./types.js"; + +function createCredential(overrides: Partial = {}): OAuthCredential { + return { + type: "oauth", + provider: "openai-codex", + access: "access-token", + refresh: "refresh-token", + expires: Date.now() + 60_000, + ...overrides, + }; +} + +describe("isSafeToOverwriteStoredOAuthIdentity", () => { + it("accepts matching account identities", () => { + expect( + isSafeToOverwriteStoredOAuthIdentity( + createCredential({ accountId: "acct-123" }), + createCredential({ access: "rotated-access", accountId: "acct-123" }), + ), + ).toBe(true); + }); + + it("refuses overwriting an existing identity-less credential with a different token", () => { + expect( + isSafeToOverwriteStoredOAuthIdentity( + createCredential({}), + createCredential({ access: "rotated-access", accountId: "acct-123" }), + ), + ).toBe(false); + }); + + it("refuses non-overlapping identity evidence", () => { + expect( + isSafeToOverwriteStoredOAuthIdentity( + createCredential({ accountId: "acct-123" }), + createCredential({ access: "rotated-access", email: "user@example.com" }), + ), + ).toBe(false); + }); +}); + +describe("OAuthManagerRefreshError", () => { + it("serializes without leaking credential or store secrets", () => { + const refreshedStore: AuthProfileStore = { + version: 1, + profiles: { + "openai-codex:default": createCredential({ + access: "store-access", + refresh: "store-refresh", + }), + }, + }; + const error = new OAuthManagerRefreshError({ + credential: createCredential({ access: "error-access", refresh: "error-refresh" }), + profileId: "openai-codex:default", + refreshedStore, + cause: new Error("boom"), + }); + + const serialized = JSON.stringify(error); + expect(serialized).toContain("openai-codex"); + expect(serialized).toContain("openai-codex:default"); + expect(serialized).not.toContain("error-access"); + expect(serialized).not.toContain("error-refresh"); + expect(serialized).not.toContain("store-access"); + expect(serialized).not.toContain("store-refresh"); + }); +}); diff --git a/src/agents/auth-profiles/oauth-manager.ts b/src/agents/auth-profiles/oauth-manager.ts index 3d5d4bab611..9456709a582 100644 --- a/src/agents/auth-profiles/oauth-manager.ts +++ b/src/agents/auth-profiles/oauth-manager.ts @@ -36,10 +36,10 @@ export type ResolvedOAuthAccess = { }; export class OAuthManagerRefreshError extends Error { - readonly credential: OAuthCredential; readonly profileId: string; readonly provider: string; - readonly refreshedStore: AuthProfileStore; + readonly #refreshedStore: AuthProfileStore; + readonly #credential: OAuthCredential; constructor(params: { credential: OAuthCredential; @@ -52,10 +52,27 @@ export class OAuthManagerRefreshError extends Error { { cause: params.cause }, ); this.name = "OAuthManagerRefreshError"; - this.credential = params.credential; + this.#credential = params.credential; this.profileId = params.profileId; this.provider = params.credential.provider; - this.refreshedStore = params.refreshedStore; + this.#refreshedStore = params.refreshedStore; + } + + getRefreshedStore(): AuthProfileStore { + return this.#refreshedStore; + } + + getCredential(): OAuthCredential { + return this.#credential; + } + + toJSON(): { name: string; message: string; profileId: string; provider: string } { + return { + name: this.name, + message: this.message, + profileId: this.profileId, + provider: this.provider, + }; } } @@ -122,6 +139,60 @@ export function hasUsableOAuthCredential( return resolveTokenExpiryState(credential.expires, now) === "valid"; } +function normalizeAuthIdentityToken(value: string | undefined): string | undefined { + const trimmed = value?.trim(); + return trimmed ? trimmed : undefined; +} + +function normalizeAuthEmailToken(value: string | undefined): string | undefined { + return normalizeAuthIdentityToken(value)?.toLowerCase(); +} + +function hasOAuthIdentity(credential: Pick): boolean { + return ( + normalizeAuthIdentityToken(credential.accountId) !== undefined || + normalizeAuthEmailToken(credential.email) !== undefined + ); +} + +function hasMatchingOAuthIdentity( + existing: Pick, + incoming: Pick, +): boolean { + const existingAccountId = normalizeAuthIdentityToken(existing.accountId); + const incomingAccountId = normalizeAuthIdentityToken(incoming.accountId); + if (existingAccountId !== undefined && incomingAccountId !== undefined) { + return existingAccountId === incomingAccountId; + } + + const existingEmail = normalizeAuthEmailToken(existing.email); + const incomingEmail = normalizeAuthEmailToken(incoming.email); + if (existingEmail !== undefined && incomingEmail !== undefined) { + return existingEmail === incomingEmail; + } + + return false; +} + +export function isSafeToOverwriteStoredOAuthIdentity( + existing: OAuthCredential | undefined, + incoming: OAuthCredential, +): boolean { + if (!existing || existing.type !== "oauth") { + return true; + } + if (existing.provider !== incoming.provider) { + return false; + } + if (areOAuthCredentialsEquivalent(existing, incoming)) { + return true; + } + if (!hasOAuthIdentity(existing)) { + return false; + } + return hasMatchingOAuthIdentity(existing, incoming); +} + export function shouldBootstrapFromExternalCliCredential(params: { existing: OAuthCredential | undefined; imported: OAuthCredential; @@ -224,6 +295,13 @@ export function resolveEffectiveOAuthCredential(params: { }); return params.credential; } + if (!isSafeToOverwriteStoredOAuthIdentity(params.credential, imported)) { + log.warn("refused external oauth bootstrap credential: identity mismatch or missing binding", { + profileId: params.profileId, + provider: params.credential.provider, + }); + return params.credential; + } const shouldBootstrap = shouldBootstrapFromExternalCliCredential({ existing: params.credential, imported, @@ -322,7 +400,7 @@ export function createOAuthManager(adapter: OAuthManagerAdapter) { if (existing && existing.provider !== params.refreshed.provider) { return false; } - if (existing && !adapter.isSafeToCopyOAuthIdentity(existing, params.refreshed)) { + if (existing && !isSafeToOverwriteStoredOAuthIdentity(existing, params.refreshed)) { log.warn("refused to mirror OAuth credential: identity mismatch or regression", { profileId: params.profileId, }); @@ -423,18 +501,33 @@ export function createOAuthManager(adapter: OAuthManagerAdapter) { credential: cred, }); if (externallyManaged) { - if ( - shouldReplaceStoredOAuthCredential(cred, externallyManaged) && - !areOAuthCredentialsEquivalent(cred, externallyManaged) - ) { - store.profiles[params.profileId] = externallyManaged; - saveAuthProfileStore(store, params.agentDir); - } - if (hasUsableOAuthCredential(externallyManaged)) { - return { - apiKey: await adapter.buildApiKey(externallyManaged.provider, externallyManaged), - credential: externallyManaged, - }; + if (externallyManaged.provider !== cred.provider) { + log.warn("refused external oauth bootstrap credential: provider mismatch", { + profileId: params.profileId, + provider: cred.provider, + }); + } else if (!isSafeToOverwriteStoredOAuthIdentity(cred, externallyManaged)) { + log.warn( + "refused external oauth bootstrap credential: identity mismatch or missing binding", + { + profileId: params.profileId, + provider: cred.provider, + }, + ); + } else { + if ( + shouldReplaceStoredOAuthCredential(cred, externallyManaged) && + !areOAuthCredentialsEquivalent(cred, externallyManaged) + ) { + store.profiles[params.profileId] = { ...externallyManaged }; + saveAuthProfileStore(store, params.agentDir); + } + if (hasUsableOAuthCredential(externallyManaged)) { + return { + apiKey: await adapter.buildApiKey(externallyManaged.provider, externallyManaged), + credential: externallyManaged, + }; + } } } diff --git a/src/agents/auth-profiles/oauth.mirror-refresh.test.ts b/src/agents/auth-profiles/oauth.mirror-refresh.test.ts index b89ad72582c..911c6e90007 100644 --- a/src/agents/auth-profiles/oauth.mirror-refresh.test.ts +++ b/src/agents/auth-profiles/oauth.mirror-refresh.test.ts @@ -652,11 +652,10 @@ describe("resolveApiKeyForProfile OAuth refresh mirror-to-main (#26322)", () => }); }); - it("mirrors an identity-carrying refresh into a main store that has no identity (upgrade)", async () => { - // The Codex P1 scenario: main holds a pre-capture OAuth record (no - // accountId), the fresh sub-agent refresh response carries accountId. - // Mirror must accept so subsequent peers can adopt from main instead - // of hitting refresh_token_reused. + it("refuses to mirror into a main store that lacks identity binding", async () => { + // Authoritative main-store overwrites now require a positive identity + // match. A pre-capture main credential must not be replaced by a + // sub-agent refresh from an arbitrary account. const profileId = "openai-codex:default"; const provider = "openai-codex"; const freshExpiry = Date.now() + 60 * 60 * 1000; @@ -704,13 +703,14 @@ describe("resolveApiKeyForProfile OAuth refresh mirror-to-main (#26322)", () => }); expect(result?.apiKey).toBe("sub-refreshed-access"); - // Main must have accepted the mirror, with the identity marker added. + // Main must remain unchanged because there was no positive identity + // binding to authorize the overwrite. const mainRaw = JSON.parse( await fs.readFile(path.join(mainAgentDir, "auth-profiles.json"), "utf8"), ) as AuthProfileStore; expect(mainRaw.profiles[profileId]).toMatchObject({ - access: "sub-refreshed-access", - accountId: "acct-sub", + access: "main-pre-capture-access", + refresh: "main-pre-capture-refresh", }); }); diff --git a/src/agents/auth-profiles/oauth.openai-codex-refresh-fallback.test.ts b/src/agents/auth-profiles/oauth.openai-codex-refresh-fallback.test.ts index a9e74088480..d2679cd82e8 100644 --- a/src/agents/auth-profiles/oauth.openai-codex-refresh-fallback.test.ts +++ b/src/agents/auth-profiles/oauth.openai-codex-refresh-fallback.test.ts @@ -271,6 +271,7 @@ describe("resolveApiKeyForProfile openai-codex refresh fallback", () => { access: "expired-access-token", refresh: "expired-refresh-token", expires: Date.now() - 60_000, + accountId: "acct-cli", }, }, }, @@ -313,6 +314,7 @@ describe("resolveApiKeyForProfile openai-codex refresh fallback", () => { access: "expired-access-token", refresh: "expired-refresh-token", expires: Date.now() - 60_000, + accountId: "acct-cli", }, }, }, @@ -470,6 +472,56 @@ describe("resolveApiKeyForProfile openai-codex refresh fallback", () => { expect(refreshProviderOAuthCredentialWithPluginMock).not.toHaveBeenCalled(); }); + it("refuses mismatched imported Codex CLI credentials when the stored default is expired", async () => { + const profileId = "openai-codex:default"; + saveAuthProfileStore( + { + version: 1, + profiles: { + [profileId]: { + type: "oauth", + provider: "openai-codex", + access: "expired-access-token", + refresh: "expired-refresh-token", + expires: Date.now() - 60_000, + accountId: "acct-local", + }, + }, + }, + agentDir, + ); + readCodexCliCredentialsCachedMock.mockReturnValueOnce({ + type: "oauth", + provider: "openai-codex", + access: "fresh-cli-access-token", + refresh: "fresh-cli-refresh-token", + expires: Date.now() + 86_400_000, + accountId: "acct-cli", + }); + refreshProviderOAuthCredentialWithPluginMock.mockResolvedValueOnce({ + type: "oauth", + provider: "openai-codex", + access: "rotated-local-access-token", + refresh: "rotated-local-refresh-token", + expires: Date.now() + 86_400_000, + accountId: "acct-local", + }); + + await expect( + resolveApiKeyForProfile({ + store: ensureAuthProfileStore(agentDir), + profileId, + agentDir, + }), + ).resolves.toEqual({ + apiKey: "rotated-local-access-token", + provider: "openai-codex", + email: undefined, + }); + + expect(refreshProviderOAuthCredentialWithPluginMock).toHaveBeenCalledTimes(1); + }); + it("keeps the canonical refresh token when imported Codex CLI state is stale", async () => { const profileId = "openai-codex:default"; saveAuthProfileStore( diff --git a/src/agents/auth-profiles/oauth.ts b/src/agents/auth-profiles/oauth.ts index 9bc170c84d6..a0191ede27b 100644 --- a/src/agents/auth-profiles/oauth.ts +++ b/src/agents/auth-profiles/oauth.ts @@ -469,7 +469,7 @@ export async function resolveApiKeyForProfile( } catch (error) { const refreshedStore = error instanceof OAuthManagerRefreshError - ? error.refreshedStore + ? error.getRefreshedStore() : loadAuthProfileStoreForSecretsRuntime(params.agentDir); const fallbackProfileId = suggestOAuthProfileIdForLegacyDefault({ cfg,