diff --git a/src/server/relay.ts b/src/server/relay.ts index 3b5aae236..7cc97d0ab 100644 --- a/src/server/relay.ts +++ b/src/server/relay.ts @@ -1147,6 +1147,7 @@ export function sanitizePassthroughHeaders(upstream: Headers): Headers { "transfer-encoding", "connection", "keep-alive", + "location", "proxy-authenticate", "proxy-authorization", "set-cookie", diff --git a/src/server/responses/compact.ts b/src/server/responses/compact.ts index 59f2fd273..4ee09ff59 100644 --- a/src/server/responses/compact.ts +++ b/src/server/responses/compact.ts @@ -194,8 +194,6 @@ const COMPACT_PASSTHROUGH_HEADERS = [ "x-codex-primary-reset-at", "x-codex-secondary-reset-at", "x-codex-tertiary-reset-at", - // A relayed 3xx keeps its Location so the client can follow it (#914). - "location", ]; function compactResponseHeaders(upstream: Response): Headers { diff --git a/src/server/responses/core.ts b/src/server/responses/core.ts index 2d00ab0c4..59a9fcfd2 100644 --- a/src/server/responses/core.ts +++ b/src/server/responses/core.ts @@ -2014,10 +2014,10 @@ async function handleResponsesInner( // Codex renders that as the opaque "Unknown error" (#452). Combo attempts // keep their typed failure envelope. Non-empty bodies are relayed verbatim // (headers included) so pool-retry Activation B/D and client diagnostics stay intact. - // Manual-redirect policy (#914): a 3xx is relayed as-is (Location preserved - // through sanitizePassthroughHeaders) so a redirect to a dead host can never - // masquerade as a pre-connection failure after the credential was seen. - // The numeric outcome above already classified it neutral — no streak. + // Manual-redirect policy (#914): relay the upstream status without Location. + // Following it client-side could replay the caller's body and custom admission + // headers to an attacker-controlled host. The numeric outcome above already + // classified the real HTTP response as neutral — no streak. if (upstreamResponse.status >= 300 && upstreamResponse.status < 400) { return new Response(upstreamResponse.body, { status: upstreamResponse.status, diff --git a/tests/errors-adapter-failure.test.ts b/tests/errors-adapter-failure.test.ts index 62cc54251..ccbcd4e31 100644 --- a/tests/errors-adapter-failure.test.ts +++ b/tests/errors-adapter-failure.test.ts @@ -119,4 +119,16 @@ describe("adapterFailureFromMessage", () => { const body = await response.json() as { error?: { type?: string; code?: string } }; expect(body.error).toMatchObject({ type: "client_cancelled", code: "client_cancelled" }); }); + + test("a compact redirect cannot expose its upstream Location", async () => { + const upstream = new Response(null, { + status: 307, + headers: { location: "https://attacker.example/collect" }, + }); + + const response = await bufferCompactResponse(upstream, new AbortController().signal); + + expect(response.status).toBe(307); + expect(response.headers.get("location")).toBeNull(); + }); }); diff --git a/tests/server-auth.test.ts b/tests/server-auth.test.ts index 849a5a204..29a063f23 100644 --- a/tests/server-auth.test.ts +++ b/tests/server-auth.test.ts @@ -2691,7 +2691,7 @@ describe("server local API auth", () => { } }); - test("passthrough pool send relays a 307 with Location and records no health evidence (#914)", async () => { + test("passthrough pool send strips a 307 Location and records no health evidence (#914)", async () => { if (existsSync(TEST_DIR)) rmSync(TEST_DIR, { recursive: true }); mkdirSync(TEST_DIR, { recursive: true }); process.env.OPENCODEX_HOME = TEST_DIR; @@ -2700,8 +2700,8 @@ describe("server local API auth", () => { clearAccountNeedsReauth("pool-a"); clearUpstreamHostHealth(); - // The upstream answers 307 -> dead.invalid. Manual redirects must relay it - // (with Location) instead of following into a dead-host rejection. + // The upstream answers 307 -> dead.invalid. Manual redirects must avoid + // following it, while the response sanitizer must not expose its target. const redirectTarget = "https://dead.invalid/x"; globalThis.fetch = (async (input: RequestInfo | URL, init?: RequestInit) => { const requestUrl = typeof input === "string" ? input : input instanceof URL ? input.toString() : input.url; @@ -2754,7 +2754,7 @@ describe("server local API auth", () => { }); expect(response.status).toBe(307); - expect(response.headers.get("location")).toBe(redirectTarget); + expect(response.headers.get("location")).toBeNull(); // Neutral class: no account streak, no soft-avoid, no rotation, and the // real response cleared the seeded host streak. expect(getCodexUpstreamHealth("pool-a")).toBeNull();