Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/server/relay.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1147,6 +1147,7 @@ export function sanitizePassthroughHeaders(upstream: Headers): Headers {
"transfer-encoding",
"connection",
"keep-alive",
"location",
"proxy-authenticate",
"proxy-authorization",
"set-cookie",
Expand Down
2 changes: 0 additions & 2 deletions src/server/responses/compact.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
8 changes: 4 additions & 4 deletions src/server/responses/core.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
12 changes: 12 additions & 0 deletions tests/errors-adapter-failure.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
});
});
8 changes: 4 additions & 4 deletions tests/server-auth.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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();
Expand Down
Loading