From 8d6f91b558d25cc83b35cf99ded1baf73d1bc515 Mon Sep 17 00:00:00 2001 From: Nguyen Thanh Dat Date: Mon, 24 Aug 2026 11:49:37 +0700 Subject: [PATCH] fix(security): refuse proxy-authorization and proxy-authenticate upstream (#11328) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Validated on a 17-PR combined board: upstream-headers-proxy-auth within the board's 287/287, typecheck:core clean, gates within baseline. proxy-authorization and proxy-authenticate join the FORBIDDEN denylist — forwarding proxy-authorization to a model provider would hand that provider the operator's own proxy credential. Thank you @ntdat812! --- .../11328-upstream-headers-proxy-auth.md | 1 + src/shared/constants/upstreamHeaders.ts | 10 +++ .../unit/upstream-headers-proxy-auth.test.ts | 66 +++++++++++++++++++ 3 files changed, 77 insertions(+) create mode 100644 changelog.d/fixes/11328-upstream-headers-proxy-auth.md create mode 100644 tests/unit/upstream-headers-proxy-auth.test.ts diff --git a/changelog.d/fixes/11328-upstream-headers-proxy-auth.md b/changelog.d/fixes/11328-upstream-headers-proxy-auth.md new file mode 100644 index 0000000000..2155b9ea90 --- /dev/null +++ b/changelog.d/fixes/11328-upstream-headers-proxy-auth.md @@ -0,0 +1 @@ +- **fix(security):** `proxy-authorization` and `proxy-authenticate` are refused as upstream/custom headers, so a proxy credential is no longer forwarded to the model provider — the canonical denylist now matches the RFC 7230 §6.1 set the rest of the codebase already strips ([#11328](https://github.com/diegosouzapw/OmniRoute/pull/11328)) diff --git a/src/shared/constants/upstreamHeaders.ts b/src/shared/constants/upstreamHeaders.ts index f4502aacfa..5d9d7f7f08 100644 --- a/src/shared/constants/upstreamHeaders.ts +++ b/src/shared/constants/upstreamHeaders.ts @@ -10,6 +10,16 @@ const FORBIDDEN = new Set( "content-length", "keep-alive", "proxy-connection", + // The two RFC 7230 §6.1 hop-by-hop names this list was missing. They belong + // to the connection between the client and OmniRoute (or its upstream + // proxy), never to the request OmniRoute makes to the model provider — + // forwarding `proxy-authorization` hands that proxy credential to the + // provider. `src/lib/services/reverseProxy.ts` (HOP_BY_HOP), + // `src/mitm/sanitizeHeaders.ts`, `src/mitm/inspector/httpProxyServer.ts`, + // `src/mitm/tproxy/tlsCapture.ts` and `src/app/api/openapi/try/route.ts` + // all already strip them; this list, the canonical one, did not. + "proxy-authenticate", + "proxy-authorization", "transfer-encoding", "te", "trailer", diff --git a/tests/unit/upstream-headers-proxy-auth.test.ts b/tests/unit/upstream-headers-proxy-auth.test.ts new file mode 100644 index 0000000000..6cca43ead0 --- /dev/null +++ b/tests/unit/upstream-headers-proxy-auth.test.ts @@ -0,0 +1,66 @@ +// `FORBIDDEN` in src/shared/constants/upstreamHeaders.ts is documented as the +// hop-by-hop / Host / framing denylist, and it was missing two of the RFC 7230 +// §6.1 names. Measured before the fix: +// +// proxy-authorization upstream=allow custom=allow +// proxy-authenticate upstream=allow custom=allow +// proxy-connection upstream=BLOCK custom=BLOCK +// +// `proxy-authorization` is the one that costs something: it authenticates the +// hop to the operator's own proxy, so forwarding it hands that credential to +// the model provider. Five other modules in this repo already strip it +// (reverseProxy HOP_BY_HOP, mitm/sanitizeHeaders, inspector/httpProxyServer, +// tproxy/tlsCapture, openapi/try) — the canonical list did not. +import { test } from "node:test"; +import assert from "node:assert/strict"; + +import { + isForbiddenUpstreamHeaderName, + isForbiddenCustomHeaderName, +} from "../../src/shared/constants/upstreamHeaders.ts"; +import { HOP_BY_HOP } from "../../src/lib/services/reverseProxy.ts"; +import { sanitizeUpstreamHeadersMap } from "../../src/lib/db/models.ts"; + +test("proxy-authorization and proxy-authenticate are refused", () => { + for (const name of ["proxy-authorization", "proxy-authenticate"]) { + assert.equal(isForbiddenUpstreamHeaderName(name), true, name); + assert.equal(isForbiddenCustomHeaderName(name), true, name); + } +}); + +test("the refusal is case-insensitive, like every other name in the list", () => { + for (const name of ["Proxy-Authorization", "PROXY-AUTHENTICATE", " Proxy-Authorization "]) { + assert.equal(isForbiddenUpstreamHeaderName(name), true, name); + } +}); + +test("sanitizeUpstreamHeadersMap drops them and keeps the rest", () => { + const out = sanitizeUpstreamHeadersMap({ + "Proxy-Authorization": "Basic c2VjcmV0", + "Proxy-Authenticate": "Basic realm=x", + "X-Custom": "ok", + }); + + assert.deepEqual(out, { "X-Custom": "ok" }); +}); + +test("the canonical list now covers every hop-by-hop name reverseProxy strips", () => { + // `reverseProxy.HOP_BY_HOP` is the repo's own RFC 7230 §6.1 list. The two + // lists drifting apart is what this fix repairs, so compare them directly — + // `trailers` is the TE token, spelled `trailer` as a header name. + const missing = [...HOP_BY_HOP] + .map((name) => (name === "trailers" ? "trailer" : name)) + .filter((name) => !isForbiddenUpstreamHeaderName(name)); + + assert.deepEqual(missing, []); +}); + +test("ordinary headers are still allowed", () => { + for (const name of ["x-custom", "x-forwarded-for", "user-agent", "accept"]) { + assert.equal(isForbiddenUpstreamHeaderName(name), false, name); + } + // Auth headers stay allowed as *upstream* headers (the credential layer owns + // them) while remaining forbidden as operator-supplied custom headers. + assert.equal(isForbiddenUpstreamHeaderName("authorization"), false); + assert.equal(isForbiddenCustomHeaderName("authorization"), true); +});