From 8086d2878b6078c74f5e3fb4ceea0e336056b8fb Mon Sep 17 00:00:00 2001 From: diegosouzapw Date: Tue, 2 Jun 2026 06:02:49 -0300 Subject: [PATCH] =?UTF-8?q?fix(ws):=20codex=20Responses-over-WebSocket=20u?= =?UTF-8?q?pgrade=20=E2=80=94=20clean=20handshake=20+=20bridge-secret=20au?= =?UTF-8?q?th?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two bugs made `wscat ws://host/v1/responses` fail with "Transfer-Encoding can't be present with Content-Length": 1. authz/management policy 401'd the proxy's own internal authenticate/prepare loopback call to /api/internal/codex-responses-ws (MANAGEMENT-classified, the per-process bridge secret wasn't recognized one layer up). Added a tightly-scoped carve-out: isValidWsBridgeRequest() honors a timing-safe sha256 match of OMNIROUTE_WS_BRIDGE_SECRET (x-omniroute-ws-bridge-secret header) for that exact internal path; the route still re-validates the secret. → auth now succeeds → 101. 2. On auth failure the proxy spread the internal fetch's response headers onto the raw upgrade socket — a chunked Transfer-Encoding + Next CSP/route-class headers collided with writeHttpError's Content-Length framing (and duplicated Content-Type via a case-mismatched spread). writeHttpError now strips framing + pipeline/security headers (case-insensitive), and the auth-fail callsite no longer forwards them. Regression test: tests/unit/responses-ws-proxy-headers.test.mjs (exports writeHttpError; asserts no TE+CL, single Content-Type, no CSP/route-class leak, safe headers forwarded). --- scripts/dev/responses-ws-proxy.mjs | 46 +++++++++--- src/server/authz/policies/management.ts | 29 +++++++- .../unit/responses-ws-proxy-headers.test.mjs | 72 +++++++++++++++++++ 3 files changed, 138 insertions(+), 9 deletions(-) create mode 100644 tests/unit/responses-ws-proxy-headers.test.mjs diff --git a/scripts/dev/responses-ws-proxy.mjs b/scripts/dev/responses-ws-proxy.mjs index a30a9b649d..0a8201dc7a 100644 --- a/scripts/dev/responses-ws-proxy.mjs +++ b/scripts/dev/responses-ws-proxy.mjs @@ -150,16 +150,47 @@ export function decodeClientFrames( }; } -function writeHttpError(socket, status, body, headers = {}) { +const WRITE_ERROR_RESERVED_HEADERS = new Set([ + // Framing — must never collide with our Content-Length default. + "transfer-encoding", + "content-length", + "content-type", + "connection", + "keep-alive", + // Next pipeline / security headers are meaningless on a raw JSON error socket + // and must not leak from a forwarded internal-fetch response. + "content-security-policy", + "x-frame-options", + "x-content-type-options", + "referrer-policy", + "permissions-policy", + "strict-transport-security", + "x-omniroute-route-class", + "x-request-id", + "date", +]); + +export function writeHttpError(socket, status, body, headers = {}) { if (!socket.writable || socket.destroyed) return; const bodyBuffer = Buffer.from(body || "", "utf8"); const statusText = STATUS_CODES[status] || "Error"; + // Strip any caller-supplied framing / duplicate-prone headers (case-insensitive) + // so our Content-Length/Connection/Content-Type defaults always win. Forwarding + // an upstream fetch's chunked Transfer-Encoding here would collide with + // Content-Length ("Transfer-Encoding can't be present with Content-Length") and + // break the client's HTTP parser on a raw upgrade socket. + const safeHeaders = {}; + for (const [name, value] of Object.entries(headers || {})) { + if (!WRITE_ERROR_RESERVED_HEADERS.has(String(name).toLowerCase())) { + safeHeaders[name] = value; + } + } const responseHeaders = { Connection: "close", "Content-Length": String(bodyBuffer.length), "Content-Type": "application/json; charset=utf-8", - ...headers, + ...safeHeaders, }; const head = [ @@ -637,12 +668,11 @@ export function createResponsesWsProxy({ headers: getAuthHeaders(req.url || pathname, req.headers), }); if (!auth.ok) { - writeHttpError( - socket, - auth.status, - auth.text || "{}", - Object.fromEntries(auth.headers.entries()) - ); + // Do NOT forward the internal fetch's response headers onto the raw + // upgrade socket — they carry chunked transfer-encoding + Next security + // headers that collide with writeHttpError's Content-Length framing. + // The sanitized JSON body alone is enough for the client. + writeHttpError(socket, auth.status, auth.text || "{}"); return true; } diff --git a/src/server/authz/policies/management.ts b/src/server/authz/policies/management.ts index 8fbc15d6b5..534d3592f6 100644 --- a/src/server/authz/policies/management.ts +++ b/src/server/authz/policies/management.ts @@ -1,4 +1,4 @@ -import { timingSafeEqual } from "node:crypto"; +import { createHash, timingSafeEqual } from "node:crypto"; import { isModelSyncInternalRequest } from "../../../shared/services/modelSyncScheduler"; import { isAuthRequired, isDashboardSessionAuthenticated } from "../../../shared/utils/apiAuth"; import { getLegacyCliTokenSync, getMachineTokenSync } from "../../../lib/machineToken"; @@ -69,11 +69,38 @@ function isInternalModelSyncRequest(ctx: PolicyContext): boolean { return isModelSyncInternalRequest(ctx.request); } +const WS_BRIDGE_INTERNAL_PATH = "/api/internal/codex-responses-ws"; +const WS_BRIDGE_SECRET_HEADER = "x-omniroute-ws-bridge-secret"; + +// The in-process codex Responses-over-WebSocket proxy authenticates its internal +// authenticate/prepare calls with a per-process, unguessable secret minted by +// server-ws.mjs (OMNIROUTE_WS_BRIDGE_SECRET). Without this carve-out the MANAGEMENT +// classification 401s that loopback call, which then leaks chunked/security headers +// back onto the upgrade socket. The internal route re-validates the secret timing-safe +// (bridgeSecretMatches), so this is the same trust boundary, surfaced one layer up. +function isValidWsBridgeRequest(ctx: PolicyContext): boolean { + if (ctx.classification.normalizedPath !== WS_BRIDGE_INTERNAL_PATH) return false; + const expected = process.env.OMNIROUTE_WS_BRIDGE_SECRET || ""; + if (!expected) return false; + const provided = ctx.request.headers?.get?.(WS_BRIDGE_SECRET_HEADER) ?? ""; + if (!provided) return false; + const expectedHash = createHash("sha256").update(expected).digest(); + const providedHash = createHash("sha256").update(provided).digest(); + return timingSafeEqual(expectedHash, providedHash); +} + export const managementPolicy: RoutePolicy = { routeClass: "MANAGEMENT", async evaluate(ctx: PolicyContext): Promise { const path = ctx.classification.normalizedPath; + // Codex Responses-over-WS bridge: honor the per-process bridge secret before + // the loopback/auth gates so the proxy's internal calls aren't 401'd (which + // would corrupt the WS upgrade response). The internal route re-checks it. + if (isValidWsBridgeRequest(ctx)) { + return allow({ kind: "management_key", id: "ws-bridge", label: "codex-ws-bridge-secret" }); + } + // Tier 1: local-only gate — block spawn-capable routes from non-loopback. // // Carve-out: a small allow-list of LOCAL_ONLY paths (see diff --git a/tests/unit/responses-ws-proxy-headers.test.mjs b/tests/unit/responses-ws-proxy-headers.test.mjs new file mode 100644 index 0000000000..5ed57a319c --- /dev/null +++ b/tests/unit/responses-ws-proxy-headers.test.mjs @@ -0,0 +1,72 @@ +/** + * tests/unit/responses-ws-proxy-headers.test.mjs + * + * Regression for the codex Responses-over-WebSocket upgrade bug: + * writeHttpError used to spread the internal fetch's response headers onto the + * raw upgrade socket. Those headers include a chunked `transfer-encoding` (the + * internal 401 has no Content-Length) plus Next security headers, which collide + * with writeHttpError's own `Content-Length` framing → the client's HTTP parser + * fails with "Transfer-Encoding can't be present with Content-Length". + * + * writeHttpError must now strip framing / duplicate-prone headers (case-insensitive) + * so its Content-Length/Connection/Content-Type defaults always win. + */ + +import test from "node:test"; +import assert from "node:assert/strict"; + +const { writeHttpError } = await import("../../scripts/dev/responses-ws-proxy.mjs"); + +function fakeSocket() { + return { + writable: true, + destroyed: false, + _head: "", + _body: null, + write(chunk) { + this._head += String(chunk); + }, + end(chunk) { + if (chunk !== undefined) this._body = chunk; + }, + }; +} + +test("writeHttpError strips chunked transfer-encoding + leaked pipeline headers from the caller", () => { + const sock = fakeSocket(); + // Simulate the exact offending input: undici Object.fromEntries of a chunked + // Next 401 (no content-length) with security + pipeline headers. + writeHttpError(sock, 401, JSON.stringify({ error: { message: "ws_auth_required" } }), { + "transfer-encoding": "chunked", + connection: "keep-alive", + "content-type": "application/json", + "content-security-policy": "default-src 'self'", + "x-frame-options": "DENY", + "x-omniroute-route-class": "MANAGEMENT", + "x-request-id": "abc", + }); + + const head = sock._head; + const lower = head.toLowerCase(); + + // The single most important invariant: never both framing headers. + assert.ok(lower.includes("content-length:"), "must emit Content-Length"); + assert.ok(!lower.includes("transfer-encoding"), "must NOT emit Transfer-Encoding alongside Content-Length"); + assert.ok(!lower.includes("keep-alive"), "must not forward the upstream keep-alive Connection"); + // Exactly one Content-Type (no duplicate from a case-mismatched spread). + assert.equal((lower.match(/content-type:/g) || []).length, 1, "exactly one Content-Type header"); + // Pipeline / security headers must not leak onto the raw upgrade socket. + assert.ok(!lower.includes("content-security-policy"), "must not leak CSP"); + assert.ok(!lower.includes("x-omniroute-route-class"), "must not leak route-class"); + // Our own framing defaults win. + assert.ok(head.startsWith("HTTP/1.1 401 "), "status line preserved"); + assert.ok(lower.includes("connection: close"), "Connection: close default wins"); +}); + +test("writeHttpError still forwards safe non-framing headers (e.g. retry-after)", () => { + const sock = fakeSocket(); + writeHttpError(sock, 429, "{}", { "retry-after": "5", "transfer-encoding": "chunked" }); + const lower = sock._head.toLowerCase(); + assert.ok(lower.includes("retry-after: 5"), "safe header forwarded"); + assert.ok(!lower.includes("transfer-encoding"), "framing header still stripped"); +});