fix(auth): harden oauth bootstrap identity checks

This commit is contained in:
Vincent Koc
2026-04-17 15:15:13 -07:00
committed by Peter Steinberger
parent 6f450c2d1f
commit d97d5c04f0
11 changed files with 320 additions and 46 deletions

View File

@@ -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,

View File

@@ -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<OAuthCredential, "accountId" | "email">,
incoming: Pick<OAuthCredential, "accountId" | "email">,
function hasIdentityContinuity(
existing: Pick<OAuthCredential, "accountId" | "email"> | 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;
}

View File

@@ -127,6 +127,7 @@ describe("buildAuthHealthSummary", () => {
access: "expired-access",
refresh: "expired-refresh",
expires: now - 10_000,
accountId: "acct-cli",
},
},
};

View File

@@ -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",
}),
},
});

View File

@@ -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,

View File

@@ -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",
}),
}),
);

View File

@@ -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> = {}): 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");
});
});

View File

@@ -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<OAuthCredential, "accountId" | "email">): boolean {
return (
normalizeAuthIdentityToken(credential.accountId) !== undefined ||
normalizeAuthEmailToken(credential.email) !== undefined
);
}
function hasMatchingOAuthIdentity(
existing: Pick<OAuthCredential, "accountId" | "email">,
incoming: Pick<OAuthCredential, "accountId" | "email">,
): 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,
};
}
}
}

View File

@@ -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",
});
});

View File

@@ -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(

View File

@@ -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,