diff --git a/src/codex/auth-context.ts b/src/codex/auth-context.ts index 8f14167157..b74ef0bfa4 100644 --- a/src/codex/auth-context.ts +++ b/src/codex/auth-context.ts @@ -11,7 +11,7 @@ import { isCodexAccountPaused } from "./account-pause"; import { ConfigMutationLockError } from "../config"; import { isCodexAccountUsable } from "./account-usability"; import { reconcileMainCodexAccountRuntimeState } from "./account-lifecycle"; -import { MAIN_CODEX_ACCOUNT_ID, getMainAccountToken } from "./main-account"; +import { MAIN_CODEX_ACCOUNT_ID, getMainAccountToken, isMainAccountTokenLive } from "./main-account"; import { isNativeMainTrafficBlocked } from "./native-profile-startup"; import { codexQuotaScopeForModel, @@ -451,7 +451,32 @@ export function applyCodexAuthContextToProvider( }; } -export function headersForCodexAuthContext(headers: Headers, ctx: CodexAuthContext): Headers { +export class CodexMainSubstitutionUnavailableError extends Error { + constructor() { + super("No usable Codex main credential to substitute for an admission bearer"); + this.name = "CodexMainSubstitutionUnavailableError"; + } +} + +/** + * Build the upstream auth headers for one Codex turn. + * + * The two credential domains meet here, and only here: + * + * - `pool` / `main-pool` always OVERWRITE with the stored account credential. Whatever the + * caller sent is irrelevant to what we send upstream. + * - `main` with an admission-bearer caller (#1686) must substitute the stored main credential. + * The caller proved admission with one of OUR secrets, which must never leave the process, so + * the only two acceptable outcomes are replaced-with-stored-main or fail-before-any-IO. + * Silently forwarding would be the leak validateForwardAdmissionCredential exists to prevent. + * - `main` with a dedicated-header caller keeps the existing intentional passthrough: the bearer + * there is the user's own ChatGPT credential, not ours. + */ +export function materializeCodexUpstreamAuth( + headers: Headers, + ctx: CodexAuthContext, + options: { substituteMainCredential?: boolean } = {}, +): Headers { const selected = new Headers(); for (const name of FORWARD_HEADERS) { const value = headers.get(name); @@ -460,10 +485,26 @@ export function headersForCodexAuthContext(headers: Headers, ctx: CodexAuthConte if (ctx.kind === "pool" || ctx.kind === "main-pool") { selected.set("authorization", `Bearer ${ctx.accessToken}`); selected.set("chatgpt-account-id", ctx.chatgptAccountId); + return selected; + } + if (ctx.kind === "main" && options.substituteMainCredential === true) { + const stored = getMainAccountToken(); + // Fail BEFORE any upstream I/O. Falling through here would send the admission secret. + if (!stored?.accessToken || !isMainAccountTokenLive()) { + throw new CodexMainSubstitutionUnavailableError(); + } + selected.set("authorization", `Bearer ${stored.accessToken}`); + if (stored.chatgptAccountId) selected.set("chatgpt-account-id", stored.chatgptAccountId); + return selected; } return selected; } +/** @deprecated Prefer materializeCodexUpstreamAuth; kept for call sites without admission context. */ +export function headersForCodexAuthContext(headers: Headers, ctx: CodexAuthContext): Headers { + return materializeCodexUpstreamAuth(headers, ctx); +} + export function isCodexAuthContextUsable(ctx: CodexAuthContext, config: OcxConfig): boolean { if (ctx.kind === "main") return true; if (ctx.kind === "main-pool") return isCodexAccountUsable(config, ctx.accountId); diff --git a/src/server/auth-cors.ts b/src/server/auth-cors.ts index ecc488c551..d76ac1797f 100644 --- a/src/server/auth-cors.ts +++ b/src/server/auth-cors.ts @@ -311,10 +311,20 @@ function secretEquals(actual: string, expected: string | undefined): boolean { * point at, and a sentinel string in the id would collide with a hand-edited * entry that happens to be named `loopback`. */ +/** + * HOW an admission credential was presented. + * + * The credential IDENTITY (which key matched) and its PRESENTATION (which header carried it) + * are different facts, and #1686 needs both: a proxy secret arriving as a bearer on the + * Responses transport is admissible, but only if the upstream credential is then guaranteed to + * be substituted. Collapsing the two is what made that flow unexpressible. + */ +export type DataPlaneAdmissionSource = "loopback" | "dedicated" | "bearer" | "x-api-key"; + export type DataPlaneAdmission = - | { kind: "configured"; keyId: string } - | { kind: "environment" } - | { kind: "loopback" }; + | { kind: "configured"; keyId: string; source: DataPlaneAdmissionSource } + | { kind: "environment"; source: DataPlaneAdmissionSource } + | { kind: "loopback"; source: "loopback" }; /** * Which admission secret `token` is, or null when it is none of them. @@ -325,12 +335,16 @@ export type DataPlaneAdmission = * discarded, which is what makes per-key attribution possible without touching * the admission decision itself. */ -export function resolveDataPlaneAdmissionSecret(token: string, config: Pick): DataPlaneAdmission | null { +export function resolveDataPlaneAdmissionSecret( + token: string, + config: Pick, + source: DataPlaneAdmissionSource = "dedicated", +): DataPlaneAdmission | null { const actual = token.trim(); if (!actual) return null; - if (secretEquals(actual, configuredApiAuthToken(config))) return { kind: "environment" }; + if (secretEquals(actual, configuredApiAuthToken(config))) return { kind: "environment", source }; for (const k of config.apiKeys ?? []) { - if (secretEquals(actual, k.key)) return { kind: "configured", keyId: k.id }; + if (secretEquals(actual, k.key)) return { kind: "configured", keyId: k.id, source }; } return null; } @@ -377,8 +391,12 @@ export interface ApiAuthMatrixRow { * against every cell rather than reading the table back to itself. */ export const AUTH_MATRIX: readonly ApiAuthMatrixRow[] = [ - { endpoint: "/v1/responses", bearer: "rejected", dedicated: "required", xApiKey: "rejected" }, - { endpoint: "/v1/chat/completions", bearer: "rejected", dedicated: "required", xApiKey: "rejected" }, + // #1686: a bearer that is one of OUR admission secrets is now accepted here. It is safe + // because materializeCodexUpstreamAuth substitutes the stored main credential rather than + // forwarding it; a bearer that is NOT our secret stays unadmitted and remains Codex Direct + // passthrough, so the two bearer domains still never mix. `x-api-key` is still rejected. + { endpoint: "/v1/responses", bearer: "accepted", dedicated: "accepted", xApiKey: "rejected" }, + { endpoint: "/v1/chat/completions", bearer: "accepted", dedicated: "accepted", xApiKey: "rejected" }, { endpoint: "/v1/messages", bearer: "accepted", dedicated: "accepted", xApiKey: "accepted" }, { endpoint: "/v1/models", bearer: "accepted", dedicated: "accepted", xApiKey: "accepted" }, ]; @@ -415,13 +433,15 @@ export function validateForwardAdmissionCredential(headers: Headers, config: Ocx */ export function resolveApiAuth(req: Request, config: RequestPolicyView): DataPlaneAdmission | null { // A loopback bind never reads a token at all, so there is no key to name. - if (!isApiAuthRequired(config)) return { kind: "loopback" }; - const actual = req.headers.get("x-opencodex-api-key")?.trim() - || req.headers.get("authorization")?.replace(/^Bearer\s+/i, "").trim() - // Anthropic-SDK clients (Claude Code with ANTHROPIC_API_KEY) authenticate via x-api-key. - || req.headers.get("x-api-key")?.trim(); - if (!actual) return null; - return resolveDataPlaneAdmissionSecret(actual, config); + if (!isApiAuthRequired(config)) return { kind: "loopback", source: "loopback" }; + const dedicated = req.headers.get("x-opencodex-api-key")?.trim(); + if (dedicated) return resolveDataPlaneAdmissionSecret(dedicated, config, "dedicated"); + const bearer = req.headers.get("authorization")?.replace(/^Bearer\s+/i, "").trim(); + if (bearer) return resolveDataPlaneAdmissionSecret(bearer, config, "bearer"); + // Anthropic-SDK clients (Claude Code with ANTHROPIC_API_KEY) authenticate via x-api-key. + const apiKey = req.headers.get("x-api-key")?.trim(); + if (apiKey) return resolveDataPlaneAdmissionSecret(apiKey, config, "x-api-key"); + return null; } export function hasValidApiAuth(req: Request, config: RequestPolicyView): boolean { @@ -439,14 +459,23 @@ export function requireApiAuth(req: Request, config: RequestPolicyView, _kind: " * domains can never be confused. */ export function resolveResponsesApiAuth(req: Request, config: RequestPolicyView): DataPlaneAdmission | null { - if (!isApiAuthRequired(config)) return { kind: "loopback" }; - // Dedicated header ONLY. `Authorization` on these transports may belong to - // Codex Direct passthrough, and the two bearer domains must stay unconfusable. - const actual = req.headers.get("x-opencodex-api-key")?.trim(); - if (!actual) return null; - return resolveDataPlaneAdmissionSecret(actual, config); + if (!isApiAuthRequired(config)) return { kind: "loopback", source: "loopback" }; + // The dedicated header still WINS, because it is unambiguous. + const dedicated = req.headers.get("x-opencodex-api-key")?.trim(); + if (dedicated) return resolveDataPlaneAdmissionSecret(dedicated, config, "dedicated"); + // #1686: a bearer may also be one of OUR admission secrets. Rejecting it outright meant a + // Codex client configured with `env_key` could not reach Direct at all. Admitting it is only + // safe because the upstream credential is then SUBSTITUTED rather than forwarded -- see + // materializeCodexUpstreamAuth. A bearer that is NOT our secret stays unadmitted here and + // remains Codex Direct passthrough, so the two bearer domains still never mix. + const bearer = req.headers.get("authorization")?.replace(/^Bearer\s+/i, "").trim(); + if (bearer) return resolveDataPlaneAdmissionSecret(bearer, config, "bearer"); + // `x-api-key` is deliberately NOT accepted on this transport. + return null; } + + export function requireResponsesApiAuth(req: Request, config: RequestPolicyView): Response | null { if (resolveResponsesApiAuth(req, config)) return null; return formatErrorResponse(401, "authentication_error", "opencodex API key required"); diff --git a/structure/05_gui-and-management-api.md b/structure/05_gui-and-management-api.md index dc05fb046e..a4bbcbe6db 100644 --- a/structure/05_gui-and-management-api.md +++ b/structure/05_gui-and-management-api.md @@ -72,6 +72,16 @@ Proxy admission credentials must never reach an upstream provider. The forwardin `^ocx_[0-9a-f]{40}$`, both environment tokens by constant-time comparison, and manually configured data keys by constant-time comparison. +Admission records HOW the credential was presented, not only which one matched +(`DataPlaneAdmission.source`: `loopback | dedicated | bearer | x-api-key`). The Responses and Chat +transports accept a bearer that is one of our own admission secrets; the dedicated header still +wins when both are present, and `x-api-key` is still refused there. That admission is safe only +because `materializeCodexUpstreamAuth` SUBSTITUTES the stored main credential for it and throws +before any upstream I/O when none is usable — the forwarding guard is NOT relaxed, and widening +admission without guaranteed substitution would create exactly the leak it prevents. A bearer that +is not one of our secrets stays unadmitted and remains Codex Direct passthrough, so the two bearer +domains never mix. + Audit item #16 remains partially deferred. This credential split protects new WebSocket handshakes, but the following established-connection controls are intentionally outside this batch and must not be treated as implemented: diff --git a/tests/codex-auth-context.test.ts b/tests/codex-auth-context.test.ts index 8a3f1fa43c..6ff8b481fa 100644 --- a/tests/codex-auth-context.test.ts +++ b/tests/codex-auth-context.test.ts @@ -16,6 +16,8 @@ import { cooldownErrorMessage, cooldownErrorResponse, headersForCodexAuthContext, + materializeCodexUpstreamAuth, + CodexMainSubstitutionUnavailableError, isCodexAuthContextUsable, resolveCodexAuthContext, shouldMarkAccountNeedsReauthForCodexAuthFailure, @@ -185,6 +187,12 @@ const forwardProvider: OcxProviderConfig = { authMode: "forward", }; + +/** A JWT whose `exp` is far in the future, so isMainAccountTokenLive() accepts it. */ +function liveJwt(): string { + const payload = Buffer.from(JSON.stringify({ exp: Math.floor(Date.now() / 1000) + 86_400 })).toString("base64url"); + return `header.${payload}.signature`; +} describe("Codex auth context", () => { test("main-profile drain routes a non-main pool account without native reads or quota priming", async () => { saveCodexAccountCredential("pool-a", { @@ -617,6 +625,49 @@ describe("Codex auth context", () => { } }); + + test("an admission bearer on main substitutes the stored credential, never forwards it (#1686)", () => { + // The caller proved admission with one of OUR secrets. That secret must never leave the + // process, so the only acceptable outcome is the stored main credential in its place. + const admissionSecret = "ocx_data_localsecret"; + writeFileSync(join(testDir, "auth.json"), JSON.stringify({ + tokens: { access_token: liveJwt(), account_id: "stored_main_acc" }, + })); + + const headers = materializeCodexUpstreamAuth( + new Headers({ authorization: `Bearer ${admissionSecret}`, "openai-beta": "responses=experimental" }), + { kind: "main", accountId: null }, + { substituteMainCredential: true }, + ); + + expect(headers.get("authorization")).not.toContain(admissionSecret); + expect(headers.get("authorization")).toBe(`Bearer ${liveJwt()}`); + expect(headers.get("chatgpt-account-id")).toBe("stored_main_acc"); + // Unrelated forwarded headers still ride along. + expect(headers.get("openai-beta")).toBe("responses=experimental"); + }); + + test("substitution fails closed when no usable main credential exists (#1686)", () => { + // Falling through here would forward the admission secret upstream, which is exactly + // the leak the forward guard exists to prevent. Throw before any I/O instead. + writeFileSync(join(testDir, "auth.json"), JSON.stringify({ tokens: {} })); + + expect(() => materializeCodexUpstreamAuth( + new Headers({ authorization: "Bearer ocx_data_localsecret" }), + { kind: "main", accountId: null }, + { substituteMainCredential: true }, + )).toThrow(CodexMainSubstitutionUnavailableError); + }); + + test("a dedicated-header main caller keeps its own bearer as passthrough (#1686)", () => { + // Without the substitution flag this is the user's own ChatGPT credential on the + // canonical forward provider, and rewriting it would break Direct. + const headers = materializeCodexUpstreamAuth( + new Headers({ authorization: "Bearer user_chatgpt_token" }), + { kind: "main", accountId: null }, + ); + expect(headers.get("authorization")).toBe("Bearer user_chatgpt_token"); + }); test("selected pool headers replace inbound main auth", () => { const headers = headersForCodexAuthContext( new Headers({ authorization: "Bearer main_token", "chatgpt-account-id": "main_acc", "openai-beta": "responses=experimental" }), diff --git a/tests/data-plane-admission-identity.test.ts b/tests/data-plane-admission-identity.test.ts index 6b0e5cd094..13f4d6b76e 100644 --- a/tests/data-plane-admission-identity.test.ts +++ b/tests/data-plane-admission-identity.test.ts @@ -61,6 +61,7 @@ describe("resolveDataPlaneAdmissionSecret", () => { expect(resolveDataPlaneAdmissionSecret("ocx_data_firstsecret", config)).toEqual({ kind: "configured", keyId: "first-key", + source: "dedicated", }); }); @@ -71,12 +72,13 @@ describe("resolveDataPlaneAdmissionSecret", () => { expect(resolveDataPlaneAdmissionSecret("ocx_data_secondsecret", config)).toEqual({ kind: "configured", keyId: "second-key", + source: "dedicated", }); }); test("the environment token has no configured key to name", () => { process.env.OPENCODEX_API_AUTH_TOKEN = "env-secret"; - expect(resolveDataPlaneAdmissionSecret("env-secret", remoteConfig())).toEqual({ kind: "environment" }); + expect(resolveDataPlaneAdmissionSecret("env-secret", remoteConfig())).toEqual({ kind: "environment", source: "dedicated" }); }); test.each([ @@ -114,18 +116,40 @@ describe("no admission decision changed", () => { }); describe("the two wrappers still differ", () => { - test("bearer is accepted by the broad path and rejected by the Responses path", () => { + test("bearer admission is accepted on both paths and names its source (#1686)", () => { const config = remoteConfig(); const bearer = request({ authorization: "Bearer ocx_data_firstsecret" }); // /v1/models and /v1/messages take bearer... - expect(resolveApiAuth(bearer, config)).toEqual({ kind: "configured", keyId: "first-key" }); + expect(resolveApiAuth(bearer, config)).toEqual({ kind: "configured", keyId: "first-key", source: "bearer" }); expect(hasValidApiAuth(bearer, config)).toBe(true); - // ...but Responses/Chat must not, because Authorization there may belong to - // Codex Direct passthrough. - expect(resolveResponsesApiAuth(request({ authorization: "Bearer ocx_data_firstsecret" }), config)).toBeNull(); - expect(requireResponsesApiAuth(request({ authorization: "Bearer ocx_data_firstsecret" }), config)?.status).toBe(401); + // ...and Responses now does too. Rejecting it meant a Codex client configured with + // `env_key` could not reach Direct at all. It is safe ONLY because the upstream + // credential is substituted rather than forwarded -- see materializeCodexUpstreamAuth. + // The source is recorded so that substitution can be made conditional on it. + expect(resolveResponsesApiAuth(request({ authorization: "Bearer ocx_data_firstsecret" }), config)) + .toEqual({ kind: "configured", keyId: "first-key", source: "bearer" }); + expect(requireResponsesApiAuth(request({ authorization: "Bearer ocx_data_firstsecret" }), config)).toBeNull(); + }); + + test("a bearer that is NOT our secret stays unadmitted on the Responses path (#1686)", () => { + const config = remoteConfig(); + // This is the Codex Direct passthrough case: an upstream ChatGPT bearer must not be + // mistaken for admission, or the two bearer domains would mix after all. + const foreign = request({ authorization: "Bearer sk-some-upstream-key" }); + expect(resolveResponsesApiAuth(foreign, config)).toBeNull(); + expect(requireResponsesApiAuth(foreign, config)?.status).toBe(401); + }); + + test("the dedicated header still wins over a bearer (#1686)", () => { + const config = remoteConfig(); + const both = request({ + "x-opencodex-api-key": "ocx_data_secondsecret", + authorization: "Bearer ocx_data_firstsecret", + }); + expect(resolveResponsesApiAuth(both, config)) + .toEqual({ kind: "configured", keyId: "second-key", source: "dedicated" }); }); test("x-api-key is accepted only by the broad path", () => { @@ -137,8 +161,8 @@ describe("the two wrappers still differ", () => { test("the dedicated header works on both", () => { const config = remoteConfig(); const dedicated = () => request({ "x-opencodex-api-key": "ocx_data_secondsecret" }); - expect(resolveApiAuth(dedicated(), config)).toEqual({ kind: "configured", keyId: "second-key" }); - expect(resolveResponsesApiAuth(dedicated(), config)).toEqual({ kind: "configured", keyId: "second-key" }); + expect(resolveApiAuth(dedicated(), config)).toEqual({ kind: "configured", keyId: "second-key", source: "dedicated" }); + expect(resolveResponsesApiAuth(dedicated(), config)).toEqual({ kind: "configured", keyId: "second-key", source: "dedicated" }); expect(requireResponsesApiAuth(dedicated(), config)).toBeNull(); }); }); @@ -146,8 +170,8 @@ describe("the two wrappers still differ", () => { describe("loopback binds", () => { test("admit without reading a token, and say so", () => { const config = loopbackConfig(); - expect(resolveApiAuth(request(), config)).toEqual({ kind: "loopback" }); - expect(resolveResponsesApiAuth(request(), config)).toEqual({ kind: "loopback" }); + expect(resolveApiAuth(request(), config)).toEqual({ kind: "loopback", source: "loopback" }); + expect(resolveResponsesApiAuth(request(), config)).toEqual({ kind: "loopback", source: "loopback" }); expect(hasValidApiAuth(request(), config)).toBe(true); expect(requireResponsesApiAuth(request(), config)).toBeNull(); }); @@ -208,8 +232,8 @@ describe("the Responses WebSocket handshake", () => { // dropping `admission` from the payload fails here rather than passing a // socket-opened assertion that never looked at it. const headers = new Headers({ "x-forwarded-for": "ignored" }); - const payload = buildResponsesWsData(headers, { kind: "configured", keyId: "second-key" }); - expect(payload.admission).toEqual({ kind: "configured", keyId: "second-key" }); + const payload = buildResponsesWsData(headers, { kind: "configured", keyId: "second-key", source: "dedicated" }); + expect(payload.admission).toEqual({ kind: "configured", keyId: "second-key", source: "dedicated" }); expect(payload.headers).toBe(headers); }); diff --git a/tests/loopback-listener-admission.test.ts b/tests/loopback-listener-admission.test.ts index 080fa1bdea..ea7baea3a4 100644 --- a/tests/loopback-listener-admission.test.ts +++ b/tests/loopback-listener-admission.test.ts @@ -39,7 +39,7 @@ describe("loopback listener policy view", () => { test("the loopback view admits without a credential and names it loopback", () => { const policy = requestPolicyView(wildcardConfig, "127.0.0.1"); - expect(resolveResponsesApiAuth(request(), policy)).toEqual({ kind: "loopback" }); + expect(resolveResponsesApiAuth(request(), policy)).toEqual({ kind: "loopback", source: "loopback" }); }); test("the view carries no bind address other than the one it was given", () => { @@ -56,7 +56,7 @@ describe("loopback listener policy view", () => { expect(resolveResponsesApiAuth( request("/v1/responses", { "x-opencodex-api-key": "ocx_data_realsecret" }), wildcardConfig, - )).toEqual({ kind: "configured", keyId: "k1" }); + )).toEqual({ kind: "configured", keyId: "k1", source: "dedicated" }); }); test("both Anthropic routes finish CORS with the listener-effective policy", () => {