fix: harden gateway requests, upstream redirects, and patch substitutions

[thandal]

DISCLAIMER: AI-assisted PR (Codex sol)

Summary

This PR adds three targeted security and correctness fixes:

• Protect the unauthenticated loopback gateway from browser-originated requests and DNS rebinding.
• Refuse credential-bearing upstream redirects instead of following them automatically.
• Treat $ sequences in patcher model IDs as literal text rather than replacement patterns.

Changes

Protect the loopback gateway from browser access

Local mode binds to loopback without a server password. Although it is not reachable directly from the network, pages opened in a browser can still issue requests to local ports.

Loopback-mode requests now:

• Require a loopback literal in the Host header.
• Reject requests containing an Origin header.
• Apply these checks before /health, preventing server fingerprinting through DNS rebinding.

Network mode is unchanged: it binds to 0.0.0.0, requires a password, and must continue accepting LAN hostnames.

Upstream now removes sensitive fields such as credentials, headers, OAuth account IDs, and provider data from /models, which reduces the disclosure impact. It does not prevent browser-triggered inference or DNS-rebinding access to the gateway, so the request-level guards remain necessary.

Refuse credential-bearing redirects

Anthropic passthrough requests attach credentials, including x-api-key. The default fetch behavior follows redirects, and custom credential headers can survive cross-origin redirects along with the request body.

This PR:

• Uses redirect: 'manual' for passthrough inference requests.
• Returns a clear 502 when an inference upstream attempts a redirect.
• Applies the same no-redirect policy to authenticated OAuth model-list refreshes.
• Records attempted redirects through the existing upstream error hooks.

This matches the defensive behavior already used by the template-model and custom-endpoint fetch paths.

Preserve literal model IDs in patch transforms

The patcher previously passed generated model IDs to String.replace as a replacement string. Sequences such as $&, $` , and $' were therefore interpreted as replacement directives even after JSON encoding, potentially producing invalid patched JavaScript.

The replacement now uses a callback so all generated model IDs are inserted literally.

Testing

Added regression coverage for:

• Allowed loopback callers.
• Rebound and non-loopback Host headers.
• Browser Origin headers.
• Password-gated network mode.
• Redirect refusal without replaying credentials to the redirect target.
• Model IDs containing replacement-pattern $ sequences.

Validation completed:

• pnpm typecheck
• 83 focused tests across the server auth, server router, upstream forwarding, and patcher suites

[bman654]

Thanks for this — and welcome. This is a notably well-put-together first PR: correct conventional commit types and scopes, accurate technical bodies, and real regression tests for every fix. I verified the claims by tracing and measuring rather than reading, and the substance holds up. Three things to change before merge, none of them large.

Verified working

The patcher fix is correct and provably behavior-preserving. CLAUDE.md requires byte-for-byte equivalence evidence for changes to the patch transforms, so I ran old and new applyClodexPatches side by side over realistic configs. Identical output — a 4-model config with aliases + context + display + a [1m] id produced the same 1333 bytes on both, an alias-less config the same 665 bytes, per-site results identical, idempotency preserved. And the bug is real: on main, an id containing $` emits

var KNOWN=[...,"clodex:openai:ev["sonnet","opus","haiku","fable","opusplan"il"];

which is unparseable JS — a Claude Code binary that no longer starts. extendAliasArray was genuinely the only string-form .replace() site carrying model-id payload; every other PATCH 1–7 replacement already goes through applyOnce's function form, so the fix is complete rather than partial.

The rebinding defense works and does run ahead of /health — a raw request with Host: attacker.example gets 403 at /health. Host: 0.0.0.0 is blocked too, which closes the "0.0.0.0-day" vector. Network mode is genuinely untouched and genuinely password-gated. rawRequest is the right call, not overengineering — fetch refuses to set Host, so the rebinding case cannot be expressed any other way.

The redirect rationale in your comment is exactly right, empirically. On a cross-origin 308, undici strips Authorization but forwards x-api-key verbatim and replays the full POST body. Since anthropicUpstreamHeaders puts the key in x-api-key, the inference passthrough fix closes a real leak of both the key and the conversation.

Mutation-testing your tests: reverting each fix turns a specific test red, and — nice to see — two over-blocking mutations also fail (applying the guard in network mode, and rejecting all Origins), so the allow-side is protected, not just the reject-side.

Please change

1. The OAuth token endpoints are the bigger redirect exposure, and they were left alone. refresh-models.ts::fetchJsonWithAuth — the site you hardened — attaches only Authorization: Bearer, which undici already strips on a cross-origin redirect. The sites still following redirects POST secrets in the request body, which undici replays verbatim on a 307/308:

• src/oauth/refresh-http.ts:28 — grant_type=refresh_token&refresh_token=<long-lived RT>
• src/oauth/openai.ts:70 and src/oauth/openai.ts:84 — authorization_code + code_verifier

Demonstrated on Node 24 with a real 308 to a different origin: the sink received auth= null, but x-api-key= sk-LEAKED and body= "grant_type=refresh_token&refresh_token=RT-SUPER-SECRET". The threat model is narrower here (it needs the issuer itself or a TLS-terminating MITM to emit the 3xx), but your own stated rationale applies with more force, and it is one line each — the existing if (!response.ok) branches already turn a 3xx into a clean thrown error.

2. Narrow the Origin rejection to non-loopback http(s) origins. Right now any non-blank Origin gets a 403, including null (sandboxed/file://), app://…, and http://localhost:*. README names Claude Desktop / Cowork as intended endpoint-gateway clients; both are Electron, and an Electron renderer's fetch attaches an Origin. The check is also unconditional in local mode — a request carrying a valid x-api-key password and an Origin is still 403'd (verified against a live startServer({host:'127.0.0.1', serverPassword:'pw'}): origin: app://claude → 403). There is no escape hatch, and the only workaround is network mode, which binds 0.0.0.0 — strictly more exposure to escape less.

Rejecting only when Origin parses as http(s) with a non-loopback host still blocks both attacks you cite (a rebound page's Origin is the attacker's name; a remote page's simple POST carries https://evil.example) while removing the local-client failure mode.

3. The docs describe behavior this PR changes. The loopback gateway now 403s requests that README.md:156,158 and the Server paragraph in CLAUDE.md:100 still describe as working — in particular --no-discovery is marketed as "a dedicated endpoint you point another tool at". The repo treats CLAUDE.md as the architecture contract and past PRs have updated it in-band. A sentence in each is enough.

Also worth a rebase — the branch is based at cac6a11 (v2.1.0) and main is now at e272754 (v2.1.1). It merges clean, just keeps CI honest.

Optional

• src/registry/refresh-models.ts:158 has no test — removing the redirect: 'manual' there leaves the entire suite green. tests/registry-refresh-models.test.ts already stubs global.fetch, so one expect.objectContaining({ redirect: 'manual' }) assertion closes it.
• The redirect branch never drains or cancels upstreamRes.body (src/upstream-forward.ts:146-153). With redirect: 'manual' undici returns the real 3xx with a live unconsumed body, so the connection is held until GC — on an error path a hostile upstream controls the rate of. await upstreamRes.body?.cancel().catch(() => {}) matches the pattern fetchWithOAuthRetry already uses.
• src/proxy.ts stands up a second loopback gateway for clodex claude --endpoint that got none of these guards — HEAD / and GET /v1/models answer before any token check. Either apply the same guards there (it always binds loopback, so no mode check needed) or say explicitly that it is out of scope.
• expect(res.headers.get('access-control-allow-origin')).toBeNull() (tests/server-router.test.ts:240) passes unconditionally — the gateway never emits a CORS header on any path. expect(...generateAnthropicResponse).not.toHaveBeenCalled() would carry real weight.
• Small thing: the PR says "83 focused tests" — that is the post-PR total across those four files; the PR adds 10. The 10 are good tests, the count just reads bigger than it is.

[bman654]

Thanks for the thorough turnaround — every one of the round-1 asks has real code behind it, and I verified each by measurement rather than reading. Summary of what I confirmed, then two things I'd still like before merge.

Verified good

The OAuth redirect MAJOR is genuinely fixed. Live repro on Node 24 against real loopback servers: on this branch, postOAuthRefresh against a 307/308/302 responder throws refresh failed (307) and the redirect target receives zero requests. Reverting just src/oauth/refresh-http.ts + src/oauth/openai.ts to the base commit replays grant_type=refresh_token&refresh_token=<secret> to the target verbatim, and the device-code flow completes a full token exchange through the redirect. Confirms the asymmetry: undici strips authorization, but never the request body. All six redirect:'manual' sites now redden a named test when reverted — including the refresh-models.ts one that had no test in round 1.

pollOpenAiDeviceCodeToken also fails safely rather than spinning: a 3xx is !response.ok, the body is cancelled, and the status !== 403 && status !== 404 branch throws — measured as OpenAI device authorization failed (302) with 1 fetch and 0 sleeps.

The Origin narrowing is exactly right. Measured against a real router on 127.0.0.1: app://., file://, vscode-webview://xyz, http://localhost:3000, http://127.0.0.1:5173, http://[::1]:5173, https://localhost, empty/malformed, and no-Origin all → 200; http://evil.example.com and the https variant → 403. No in-repo caller breaks (all build http://127.0.0.1:${port}, verified round-trips for v4, localhost, and ::1). Network mode is untouched — src/server/index.ts:392 hardcodes host to 0.0.0.0/127.0.0.1 so isLoopbackBind's undefined case is unreachable from any CLI path, and a real 0.0.0.0 start with Host: lan-box.local returns 200. Guard placement is correct: ahead of /health, ahead of isAuthorized, with no route preceding it. Reverting to the round-1 blanket rejection now reddens serves Electron and loopback-web callers.

The patcher fix is broader than your test claims. Byte-identical to main for realistic ids (1356-byte output, identical per-site results), idempotent on re-run, and $&, $`, $', $1, $$, $<x> all round-trip as literal text with the output parsing under new Function. Worth knowing: on main, $` fails to parse (the case you reported) but $&, $', and $$ silently corrupt the model id — a quieter failure than the loud one. Re-swept every .replace( in patch-transforms.ts and patcher.ts: extendAliasArray was indeed the only string-form site; PATCH 4/5/6/7 build payload strings but return them from arrow functions, whose return values aren't rescanned.

Integration-tested on top of current main (2.1.2): clean merge, 1034/1034 tests, typecheck clean, build clean.

Two asks

1. MAJOR — Origin: null bypasses the Origin guard from an ordinary attacker page, and a test asserts that as intended.

tests/server-auth.test.ts lists 'null' alongside app://claude-desktop as acceptable, and new URL('null') throws so isDisallowedGatewayOrigin returns false. Measured with real Chrome against a real gateway (serverPassword: null, page served from a non-loopback name resolving to 127.0.0.1):

POST /anthropic/v1/messages  origin=http://127.0.0.1.nip.io:8898  -> 403
POST /anthropic/v1/messages  origin=null                          -> 500

The null case is a six-line <iframe sandbox="allow-scripts" srcdoc=...> doing a text/plain (CORS-simple, no preflight) fetch. The 500 is the handler attempting the upstream inference call — so the guard's stated purpose ("reject web pages hosted away from loopback, including CORS-simple text/plain POSTs that skip preflight") is defeated without any DNS rebinding.

Impact is blind CSRF only, and I want to be fair about that: grep -rn "Access-Control" src/ returns zero emissions, OPTIONS 404s so anything non-simple dies at preflight, and Chrome reported status:0 type:opaque — nothing is readable. The state-changing surface is the two POST inference routes, so the cost is the user's OpenAI/ChatGPT quota plus inference/trace log writes. No config, credential, or catalog mutation is reachable.

I'm not asking for a specific fix, because the obvious one has a real trap: Chromium sends Origin: null for file:// initiators, so rejecting the literal string may break the very Electron case this narrowing was meant to protect, and Sec-Fetch-Site: cross-site (present on both requests above) conflates them too. The durable fix is the pattern already in src/proxy.ts:282,352 — a per-launch randomUUID() token required on POSTs, which local mode currently forgoes. Either implement something like that, or keep null allowed and change the test to document why with the residual risk spelled out. What I don't want is the current state, where a hole in the guard's own threat model is silently encoded as intended behavior.

2. MAJOR (docs, carried over) — the redirect behavior change is still undocumented.

grep -rn -i redirect README.md CLAUDE.md docs/ returns one unrelated hit in docs/credential-helpers.md. The Origin/Host guard docs are good — I checked every prose claim against the code, including the network-mode sentence. But src/upstream-forward.ts:146 now turns any upstream 3xx into a client-visible 502 "Upstream attempted a redirect; refusing to forward credentials", and that costs two things:

• A user with a custom Anthropic-format baseUrl that 3xx-redirects (http→https upgrade, a moved path) gets a hard 502 with nothing in the docs explaining why or that the fix is to configure the final URL.
• This repo's CLAUDE.md is where the "don't undo this" invariants live. The rule — credential-bearing upstream fetches must set redirect:'manual', because x-api-key and request bodies survive cross-origin redirects even though authorization doesn't — is recorded nowhere, so a future agent can quietly revert it. That's the durable half of this fix.

Smaller items (your call, none blocking)

• src/upstream-forward.ts:154 — onUpstreamError(upstreamRes.status, ...) logs the raw 3xx while the client receives 502, recording a status the client never saw. Cosmetic; both consumers only write a log line, and there's no double-write.
• tests/upstream-forward.test.ts:170 uses a 302, which undici would downgrade to a body-less GET anyway. A 307 is the actually-dangerous shape (body + x-api-key replayed verbatim).
• The tautological ACAO assertion is still there (tests/server-router.test.ts:257). Proof: with the whole guard block deleted and the status assertion relaxed to [200,400,403,500], the test still passes. grep -rn access-control src/ → zero matches, so it can never fail.
• cancelResponseBody (src/oauth/openai.ts:13, 3 call sites) has no test asserting the cancel; the new upstream-forward redirect test asserts neither the cancel nor onUpstreamError/log.
• The patcher test covers only $` and never parses the output — consider new Function(out) over several $ forms, since $&/$'/$$ are the silent-corruption cases.
• LOOPBACK_HOSTNAMES is an exact-string set, so Origin: http://127.0.0.2:9/ and Host: localhost.:PORT (trailing dot) both 403 despite being genuine loopback. No in-repo caller uses either shape.
• src/proxy.ts still has none of these guards, and I'm satisfied that's a fair follow-up rather than part of this PR: it binds an ephemeral port and its only state-changing route already requires a per-launch randomUUID token, leaving just HEAD / and GET /v1/models unauthenticated.
• Your "Merge branch 'main'" commit merged your fork's own main at v2.1.0, so it was a no-op — the branch is still 7 commits behind upstream. Not a problem for merging (I verified the merge is clean and green), just flagging that the rebase didn't take effect.

Beyond this PR's scope, but found while sweeping for missed sites: src/provider-factory.ts's AI-SDK fetches still run on the default redirect:'follow', and a measured cross-origin 307 replayed x-api-key, chatgpt-account-id, and the full prompt body. Same bug class you just fixed in upstream-forward.ts, low reachability, and it needs a wrapper fetch since the SDK exposes no redirect option. I'll track it separately — no need to grow this PR. The WebSocket transport (ws defaults to followRedirects: false) and the https.request paths are confirmed non-exposed, so leaving those alone was correct.

[bman654]

Follow-up to recalibrate my last review — I want to be straight with you about where the value actually is, so you don't spend round 3 in the wrong place. This doesn't change the requested-changes state; the Origin: null item still stands.

Priorities, honestly stated. Of the three fixes here, the loopback gateway guard is the one addressing a threat that happens in real life: a site the user is browsing firing inference requests at an unauthenticated gateway on a known default port. The redirect hardening is defense-in-depth against a compromised or hostile provider — and for that to matter, api.openai.com / chatgpt.com (or a base URL the user configured) has to be the thing emitting the malicious 3xx, at which point it already holds the API key and the prompts by definition. TLS plus a fixed hostname stops anyone else from injecting one. It's still worth having, and the specific correction I asked for in round 1 was fair as a relative point — you'd hardened the header-only path while the body-carrying path was the exposed one — but I shouldn't have let it read as a live vulnerability. Reachability there is essentially nil.

So the inversion I'd flag: most of this PR's surface went into the defense-in-depth fix, while the guard that addresses the real threat is the one with a hole in it. The Origin: null decision is the high-value item — put round 3 there and don't over-invest in the redirect paths.

Withdrawing one item entirely. Please ignore my note about src/provider-factory.ts's AI-SDK fetches following redirects. Same reasoning as above kills it: the configured provider host is the only party that can trigger it, and it already has the credential. I'd measured the mechanism, not a reachable case. Nothing for you to do there.

And an alternative to one of my asks. Instead of documenting the new 502, consider narrowing when it fires. Right now src/upstream-forward.ts:146 refuses every upstream 3xx, which buys protection only in the compromised-provider case but costs a genuine failure mode in the ordinary one: a custom Anthropic-format baseUrl that does an http→https upgrade or serves a moved path now hard-fails with an opaque 502 Upstream attempted a redirect. A same-origin redirect leaks nothing — same host, same credential destination — so redirect: 'manual' plus manually following same-origin redirects and refusing only cross-origin ones keeps the protection and removes the regression. If you go that way, the user-facing half of my docs ask disappears and only the CLAUDE.md invariant line is left (which I'd still like, since it's what stops a future change from quietly undoing this).

Either shape is fine by me — narrow it, or keep the blanket refusal and document it. Your call.