diff --git a/scripts/test-layout/layout.json b/scripts/test-layout/layout.json index 162b4ba20fb..07f71ce4d9f 100644 --- a/scripts/test-layout/layout.json +++ b/scripts/test-layout/layout.json @@ -1204,6 +1204,7 @@ "response-model-identity.test.ts": "server", "responses-account-label.test.ts": "responses", "responses-canonical-only-top-level-fields.test.ts": "responses", + "responses-compact-handoff-admission.test.ts": "responses", "responses-compaction-routing.test.ts": "responses", "responses-compaction.test.ts": "responses", "responses-console-go-upload-retry.test.ts": "responses", diff --git a/src/server/responses/compact.ts b/src/server/responses/compact.ts index 6d428194b94..ab16ce31b6e 100644 --- a/src/server/responses/compact.ts +++ b/src/server/responses/compact.ts @@ -107,6 +107,7 @@ import { } from "../../codex/upstream-host-health"; import { ForwardAdmissionCredentialError, + contextPrincipalIdOf, hasForwardableCodexBearer, validateForwardAdmissionCredential, } from "../auth-cors"; @@ -214,8 +215,33 @@ function pruneCompactHandoffRoutes(now: number): void { } } -function rememberCompactHandoffRoute(req: Request, model: string, now = Date.now()): void { - const key = sessionLaneIdFromRequest(req.headers); +/** + * The handoff map is process-global, so a caller-controlled lane header alone + * cannot be the key: two authenticated clients sending the same lane header + * would share one fallback route. Namespace the lane by the admitted principal. + * A loopback or missing admission has no authenticated identity to bind this + * cross-request state to, so it is ineligible rather than trusted. + */ +function compactHandoffRouteKey(req: Request, admission: DataPlaneAdmission | undefined): string | null { + const lane = sessionLaneIdFromRequest(req.headers); + if (!lane || !admission || admission.kind === "loopback") return null; + // Fail closed rather than substituting a weaker identity. `keyId` survives a + // rotation and every identity-less environment admission would collapse into + // one bucket, which is the collision this key exists to prevent. Production + // admission always mints `contextPrincipalId` for configured and environment + // holders, so no real authenticated caller loses the route. + const principal = contextPrincipalIdOf(admission); + if (!principal) return null; + return `${principal}\u0000${lane}`; +} + +function rememberCompactHandoffRoute( + req: Request, + admission: DataPlaneAdmission | undefined, + model: string, + now = Date.now(), +): void { + const key = compactHandoffRouteKey(req, admission); if (!key || model.length > COMPACT_HANDOFF_MODEL_MAX_LENGTH) return; pruneCompactHandoffRoutes(now); compactHandoffRoutes.delete(key); @@ -223,13 +249,18 @@ function rememberCompactHandoffRoute(req: Request, model: string, now = Date.now pruneCompactHandoffRoutes(now); } -function forgetCompactHandoffRoute(req: Request): void { - const key = sessionLaneIdFromRequest(req.headers); +function forgetCompactHandoffRoute(req: Request, admission?: DataPlaneAdmission): void { + const key = compactHandoffRouteKey(req, admission); if (key) compactHandoffRoutes.delete(key); } -function compactHandoffRoute(req: Request, previousModel: string, now = Date.now()): string | null { - const key = sessionLaneIdFromRequest(req.headers); +function compactHandoffRoute( + req: Request, + admission: DataPlaneAdmission | undefined, + previousModel: string, + now = Date.now(), +): string | null { + const key = compactHandoffRouteKey(req, admission); if (!key) return null; pruneCompactHandoffRoutes(now); const entry = compactHandoffRoutes.get(key); @@ -1272,10 +1303,10 @@ export async function handleResponsesCompact( // synthetic buffer errors are not upstream bodies and stay uninspected. if (buffered.ok) { inspectResponseLogJson(logCtx, await buffered.clone().text()); - forgetCompactHandoffRoute(req); + forgetCompactHandoffRoute(req, admission); rememberServingConversationStateIssuer(outcomeCtx, codexPoolAffinityKey(req.headers)); } else if (quotaFailure && !storedPool401ReplayAttempted) { - const fallbackModel = compactHandoffRoute(req, raw.model); + const fallbackModel = compactHandoffRoute(req, admission, raw.model); if (fallbackModel && !req.signal.aborted) { const fallbackReq = new Request(req.url, { method: "POST", @@ -1407,7 +1438,7 @@ export async function handleResponsesCompact( const result = new Response(JSON.stringify({ output: compactionItems }), { headers: { "Content-Type": "application/json" }, }); - rememberCompactHandoffRoute(req, raw.model); + rememberCompactHandoffRoute(req, admission, raw.model); return result; } const encrypted = compactionItems[0]!.encrypted_content; @@ -1418,6 +1449,6 @@ export async function handleResponsesCompact( } const summary = decoded; const output = buildCompactV1Output(extractCompactUserMessages(inputItems), summary); - rememberCompactHandoffRoute(req, raw.model); + rememberCompactHandoffRoute(req, admission, raw.model); return new Response(JSON.stringify({ output }), { headers: { "Content-Type": "application/json" } }); } diff --git a/tests/fixtures/test-layout-expected.json b/tests/fixtures/test-layout-expected.json index efdee7fadc8..456f3ff967f 100644 --- a/tests/fixtures/test-layout-expected.json +++ b/tests/fixtures/test-layout-expected.json @@ -1031,6 +1031,7 @@ "responses-account-label.test.ts": "responses", "responses-bare-echo-helper-fence.test.ts": "responses", "responses-canonical-only-top-level-fields.test.ts": "responses", + "responses-compact-handoff-admission.test.ts": "responses", "responses-compaction-routing.test.ts": "responses", "responses-compaction.test.ts": "responses", "responses-console-go-upload-retry.test.ts": "responses", diff --git a/tests/responses/responses-compact-handoff-admission.test.ts b/tests/responses/responses-compact-handoff-admission.test.ts new file mode 100644 index 00000000000..5fcb2aa67e3 --- /dev/null +++ b/tests/responses/responses-compact-handoff-admission.test.ts @@ -0,0 +1,188 @@ +import { afterEach, describe, expect, test } from "bun:test"; +import { mkdtempSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { saveCodexAccountCredential } from "../../src/codex/account-store"; +import { clearAccountQuota, updateAccountQuota } from "../../src/codex/auth-api"; +import { clearCodexUpstreamHealth } from "../../src/codex/routing"; +import { clearUpstreamHostHealth } from "../../src/codex/upstream-host-health"; +import { handleResponsesCompact } from "../../src/server/responses"; +import type { OcxConfig } from "../../src/types"; +import { removeTreeWithRetry } from "../helpers/remove-tree"; + +const originalFetch = globalThis.fetch; + +afterEach(() => { + globalThis.fetch = originalFetch; +}); + +/** + * The compact handoff map is process-global while its lane key arrives in a + * caller-controlled header. These tests pin the admission-principal namespacing + * that keeps one authenticated client from claiming another client's remembered + * fallback route by re-sending the same lane header. + */ +describe("compact handoff route admission namespacing", () => { + function poolConfig(): OcxConfig { + return { + defaultProvider: "openai", + activeCodexAccountId: "pool-a", + providers: { + openai: { + adapter: "openai-responses", + baseUrl: "https://chatgpt.com/backend-api/codex", + authMode: "forward", + codexAccountMode: "pool", + }, + }, + codexAccounts: [{ + id: "pool-a", + email: "pool@example.test", + isMain: false, + chatgptAccountId: "pool_acc", + }], + } as OcxConfig; + } + + function withPoolEnv(run: (config: OcxConfig) => Promise): Promise { + const testDir = mkdtempSync(join(tmpdir(), "ocx-compact-handoff-admission-")); + const previousOpencodexHome = process.env.OPENCODEX_HOME; + const previousCodexHome = process.env.CODEX_HOME; + process.env.OPENCODEX_HOME = testDir; + process.env.CODEX_HOME = testDir; + clearCodexUpstreamHealth(); + clearUpstreamHostHealth(); + clearAccountQuota(); + saveCodexAccountCredential("pool-a", { + accessToken: "pool-a-access-token", + refreshToken: "pool-a-refresh-token", + expiresAt: Date.now() + 300_000, + chatgptAccountId: "pool_acc", + }); + updateAccountQuota("pool-a", 10); + return run(poolConfig()).finally(() => { + globalThis.fetch = originalFetch; + clearCodexUpstreamHealth(); + clearUpstreamHostHealth(); + clearAccountQuota(); + removeTreeWithRetry(testDir); + if (previousOpencodexHome === undefined) delete process.env.OPENCODEX_HOME; + else process.env.OPENCODEX_HOME = previousOpencodexHome; + if (previousCodexHome === undefined) delete process.env.CODEX_HOME; + else process.env.CODEX_HOME = previousCodexHome; + }); + } + + function compactionRequest( + body: Record, + extraHeaders: Record, + ): Request { + return new Request("http://localhost/v1/responses", { + method: "POST", + headers: { "content-type": "application/json", ...extraHeaders }, + body: JSON.stringify(body), + }); + } + + function compactionBody(model: string): Record { + return { + model, + stream: false, + input: [ + { type: "message", role: "user", content: [{ type: "input_text", text: "earlier turn" }] }, + { type: "compaction_trigger" }, + ], + tools: [{ type: "function", name: "shell" }], + tool_choice: "auto", + parallel_tool_calls: true, + }; + } + + test("a remembered route is claimed only by the principal that stored it", async () => { + await withPoolEnv(async config => { + config.providers.deepseek = { + adapter: "openai-chat", + baseUrl: "https://api.deepseek.com", + authMode: "key", + apiKey: "deepseek-test-key", + models: ["deepseek-v4-flash"], + }; + config.providers["openai-apikey"] = { + adapter: "openai-responses", + baseUrl: "https://api.openai.com/v1", + authMode: "key", + apiKey: "openai-test-key", + models: ["gpt-5.6-sol"], + }; + const headers = { "x-codex-parent-thread-id": "compact-handoff-admission-thread" }; + const owner = { + kind: "configured", + keyId: "compact-client", + source: "dedicated", + contextPrincipalId: "principal-owner", + } as const; + const calls: Array<{ model: string; nativeCompact: boolean }> = []; + globalThis.fetch = (async (input: string | URL | Request, init?: RequestInit) => { + const url = typeof input === "string" + ? input + : input instanceof URL + ? input.toString() + : input.url; + const body = JSON.parse(String(init?.body ?? "{}")) as { model?: string }; + const nativeCompact = url.endsWith("/responses/compact"); + calls.push({ model: body.model ?? "", nativeCompact }); + if (nativeCompact) { + return Response.json({ error: { message: "The usage limit has been reached" } }, { + status: 502, + }); + } + return new Response(JSON.stringify({ + id: "resp_1", + status: "completed", + output: [{ type: "message", role: "assistant", content: [{ type: "output_text", text: "DeepSeek handoff summary" }] }], + usage: { input_tokens: 10, output_tokens: 5, total_tokens: 15 }, + }), { status: 200, headers: { "content-type": "application/json" } }); + }) as typeof fetch; + + const compact = (model: string, admission: Parameters[4]) => + handleResponsesCompact( + compactionRequest(compactionBody(model), headers), + config, + { model: "", provider: "" }, + undefined, + admission, + ); + + // The owner stores the deepseek route under (its principal, this lane). + const stored = await compact("deepseek/deepseek-v4-flash", owner); + expect(stored.status).toBe(200); + expect(calls).toEqual([{ model: "deepseek-v4-flash", nativeCompact: false }]); + + // A different admitted principal re-sending the same lane header must not + // claim it: every attempt stays on the requested model's native compact. + for (const intruder of [ + // Same key id, rotated secret. Admission mints a new principal, and this + // is precisely the pair a keyId-derived key would have collapsed. + { kind: "configured", keyId: "compact-client", source: "dedicated", contextPrincipalId: "principal-rotated" }, + // Authenticated but carrying no minted principal: ineligible, not pooled. + { kind: "environment", source: "bearer" }, + { kind: "loopback", source: "loopback" }, + undefined, + ] as const) { + calls.length = 0; + const res = await compact("openai-apikey/gpt-5.6-sol", intruder); + expect(res.status).toBe(502); + expect(calls.length).toBeGreaterThan(0); + expect(calls.every(call => call.model === "gpt-5.6-sol" && call.nativeCompact)).toBe(true); + } + + // The owner's own quota-blocked retry still finds the route and hands off. + calls.length = 0; + const handoff = await compact("openai-apikey/gpt-5.6-sol", owner); + expect(handoff.status).toBe(200); + expect(calls.at(-1)).toEqual({ model: "deepseek-v4-flash", nativeCompact: false }); + }); + // Five request sequences ride the transient-502 retry ladder; the default + // 5s budget is not enough on a contended host. + }, 20000); +}); diff --git a/tests/responses/responses-compaction-routing.test.ts b/tests/responses/responses-compaction-routing.test.ts index c612aed502a..3250e696c6a 100644 --- a/tests/responses/responses-compaction-routing.test.ts +++ b/tests/responses/responses-compaction-routing.test.ts @@ -1481,6 +1481,9 @@ describe("compact alternate-account attempt (#913)", () => { models: ["gpt-5.6-sol"], }; const headers = { "x-codex-parent-thread-id": "compact-routed-handoff-thread" }; + // The remembered route is keyed by the admitted principal, so every call in + // this scenario authenticates as the same configured client. + const admission = { kind: "configured", keyId: "compact-client", source: "dedicated", contextPrincipalId: "compact-client-principal" } as const; const calls: Array<{ model: string; nativeCompact: boolean }> = []; globalThis.fetch = (async (input: string | URL | Request, init?: RequestInit) => { const url = typeof input === "string" @@ -1505,8 +1508,7 @@ describe("compact alternate-account attempt (#913)", () => { undefined, headers, ), - config, - { model: "", provider: "" }, + config, { model: "", provider: "" }, undefined, admission, ); expect(manual.status).toBe(200); expect(calls).toEqual([{ model: "deepseek-v4-flash", nativeCompact: false }]); @@ -1518,8 +1520,7 @@ describe("compact alternate-account attempt (#913)", () => { undefined, { "x-codex-parent-thread-id": "different-compact-thread" }, ), - config, - { model: "", provider: "" }, + config, { model: "", provider: "" }, undefined, admission, ); expect(unrelated.status).toBe(502); expect(calls.length).toBeGreaterThan(0); @@ -1533,8 +1534,7 @@ describe("compact alternate-account attempt (#913)", () => { undefined, headers, ), - config, - logCtx, + config, logCtx, undefined, admission, ); expect(automatic.status).toBe(200);