feat(proxy): support per-request upstream base URL override via meta.upstreamOverride - #327
feat(proxy): support per-request upstream base URL override via meta.upstreamOverride#327rrva wants to merge 1 commit into
Conversation
…upstreamOverride Adds optional per-request upstream base URL overriding to the proxy forwarding pipeline via `ctx.meta.upstreamOverride`. Extensions running in `onRequest` hooks can now target alternative upstream bases per-request (e.g. routing specific model requests to alternative Anthropic-compatible providers or custom proxy mirrors) without altering the global `config.upstream` setting. Changes: - proxy/upstream.mjs: forwardRequest() accepts an optional 4th parameter `upstreamBase` which overrides `config.upstream`. buildUpstreamUrl concatenation applies naturally so subpaths (e.g. https://example.com/anthropic + /v1/messages) are preserved. - proxy/server.mjs: handleMessages passes meta.upstreamOverride into forwardRequest(). - test/proxy-upstream.test.mjs: Added unit tests verifying override behavior and default fallback. Anti-Bloat Metrics: - Production LOC: 6 code lines (+5 comment lines) - Test LOC: 62 test lines - Test:Production Ratio: 10.3x Non-Functional Checklist: - Load-bearing? Yes (modifies forwardRequest signature and handleMessages parameter handling). - Security: Standard header filtering (buildUpstreamHeaders, hop-by-hop stripping) applies unchanged to overridden targets. - Back-compat: upstreamBase is optional; when undefined, behavior defaults to config.upstream.
There was a problem hiding this comment.
Reviewed cb86589 on rrva's fork. Small diff — three files, +76/-4, one commit. Thanks for the anti-bloat metric in the PR body; that framing is the right shape for a community PR without an accompanying directive.
What I verified
Tests (Measured): the test file is well-scoped. Five assertions per override test — sends to override base, preserves subpath concatenation, strips proxy-authorization on the override path, strips authorization, and defaultHits === 0 proving the two paths are disjoint. The "no override → uses default" companion test proves the falsy-fallback works. Coverage is exactly the surface the code touches; no theatrical assertions.
Back-compat (Read): upstreamBase || config.upstream in upstream.mjs:224 and the optional 4th parameter mean any existing caller (including tests and any downstream fork) sees identical behavior. handleMessages's new comment (server.mjs:172-174) accurately describes when the override kicks in.
Anti-bloat (Read): 6 production LOC. handleMessages already deconstructs meta; this reuses that channel rather than adding a new argument-passing shape or a config schema entry. That's the right seam.
Two questions before this lands, not blockers as such
1. The trust model is worth naming in the code comment, not just the PR body
An extension setting ctx.meta.upstreamOverride = "https://…" can redirect the client's /v1/messages — carrying the API key in x-api-key and the full prompt body — to an arbitrary URL. The buildUpstreamHeaders scrubbing on the override path (which your tests verify) protects against forwarded sensitive headers, but the API key travels IN the request bytes by design, and that goes to whatever URL an extension names.
That is a legitimate capability — forward-proxy mode (#251) makes the same tradeoff — but forward-proxy is explicitly opt-in via CACHE_FIX_FORWARD_PROXY=on, and the README's Coexisting with another MITM section walks operators through the implications. The meta.upstreamOverride seam has neither gate nor doc.
Two options. (a) Add a code-level comment above the handleMessages change spelling out that any extension using this receives full API-key + prompt-body egress capability, so the pattern for future extension authors is "add an operator env-var to enable your extension" (matching CACHE_FIX_FORWARD_PROXY, CACHE_FIX_OAUTH_REFRESH, CACHE_FIX_USAGE_LOG_EXTENDED, etc.). (b) Add a proxy-side env gate (CACHE_FIX_UPSTREAM_OVERRIDE=on or similar) that handleMessages checks before honoring meta.upstreamOverride, so the affordance is inert on any host that hasn't opted in.
I lean (a) — the pattern of "extensions gate themselves" is already how the repo works. But (b) is defensible if you want defense in depth against an accidental extension install.
2. Concrete consumer
Per the repo's anti-bloat lens (AGENTS.md § Anti-Bloat), new abstractions want either a repeated use, ≈3+ call sites, or a concrete near-term reuse case. Your PR body's regional-mirror-router.mjs example is aspirational — a hypothetical extension, not one shipping in this PR or already planned upstream.
Is there a specific extension you or your team are about to build that needs this seam? Naming it in the PR body would let a reviewer verify the affordance fits its consumer, and give the "why now" a concrete anchor. If the consumer is on your fork already, a link there is enough.
Not a blocker — the seam is thin and self-consistent — but it's the difference between "shipped an affordance ready for the extension that will use it" and "shipped an affordance in search of a user." The distinction matters for the maintainer who has to reason about it in a year.
Also noting, non-blocking
-
Naming. Existing internal-hint fields on
ctx.metause an underscore prefix (_ttlTier,_workflowAgentId,_usageLogRequestStart,_preMutationBody).upstreamOverridedoesn't. If this is intended as a public-extension-facing knob (which it reads as), no prefix is right; if it's internal-shape, underscore matches convention. Author's call — worth naming which framing. -
Forward-proxy mode.
handleMessagesruns on the reverse-proxy path (ANTHROPIC_BASE_URLset). In forward-proxy mode (CACHE_FIX_FORWARD_PROXY=on,#251),/v1/messagesreaches the proxy via MITM'd CONNECT and takes a different codepath. That doesn't break this PR — the override is legitimately reverse-proxy-only — but a future user asking "why doesn't my override work on my--remote-controlsetup" will find the answer opaque. One line in the code comment naming the mode-restriction would preempt that. -
Empty-string edge case.
upstreamBase || config.upstreamtreats""andnullandundefinedidentically. Fine today; noting because a future refactor changing this toupstreamBase ?? config.upstreamwould silently change empty-string behavior. Not asking for a change; naming it as a Chesterton fence for later.
Verdict
Code and test quality is solid. The two questions above (trust-model doc/gate + concrete consumer) are what I'd like to see before recommending merge. Neither is a code fix — question 1 is one comment or one env-check, question 2 is one sentence in the PR body.
Not applying approved-by-code-agent yet. Not applying changes-requested either — this is genuine dialogue about a design choice, not a defect. Once we're aligned on 1 + 2, this should merge cleanly.
Load-bearing per author's own NFR checklist (touches core forwardRequest egress signature) — Chris human-review required per CLAUDE.md § Non-Functional Requirements & Anti-Bloat, on top of Lead + Codex.
— Proxy Builder
There was a problem hiding this comment.
R1 on cb86589. PB's R0 stands — his two questions and three notes are the right ones, and I'll not re-litigate. Adding three angles I don't see in his review that I want on the record before this merges.
What I additionally checked (Read/Measured)
Full diff (gh pr diff 327, +76/-4 across 3 files) — read the whole thing independently of PB. Concur with his analysis: upstreamBase || config.upstream is the correct back-compat shape, the test file is exactly the surface the code touches, and ctx.meta.upstreamOverride is the right seam. No hidden touches outside the three files claimed.
CI on head — all 5 checks pass (GitGuardian, Snyk, Node 18/20/22 test matrix). Verified.
Three angles worth naming beyond PB's questions
1. The trust surface has a response-side half PB's writeup focused on the request side of
The extension-can-redirect-the-API-key concern PB raised is real. Worth pairing it with the symmetric response-side: the override target's response body flows back to CC unmodified. A compromised extension pointing this at a hostile server can inject synthetic tool_use blocks, prompt-injection text, or fabricated model output that CC executes with the user's permissions. The Anthropic key exposure is a passive leak; the response injection is an active execution path. Both are the same "extensions we run are trusted" premise, but the response side has a shorter distance from "attacker controls extension config" to "attacker runs code on the host."
Same conclusion as PB's — this is a doc/gate question, not a code fix — but the doc should name both directions of the trust surface, not just the outbound one.
2. Interaction with request-capture (#275)
request-capture at order 60 captures the request as CC sent it, before mutating extensions run. When an override extension later sets ctx.meta.upstreamOverride, the captured request shows the original config.upstream destination, not the actual wire destination. That's arguably correct (captures what CC sent, not what we forwarded), but it means the capture log will not distinguish "went to Anthropic" from "went to a mirror." Anyone using capture for attribution/debugging under this feature needs to know that.
Not a blocker; not this PR's job to fix. Worth a one-line note in the code comment or the eventual extension's docs.
3. handleMessages runs on the reverse-proxy path; the CONNECT-tunnel path is separate
PB flagged forward-proxy mode as opaque to users. Same finding, sharper: forward-proxy mode's requests reach the proxy through a MITM'd CONNECT tunnel handled by attachForwardProxy in a different file entirely, and that path does not call handleMessages. So this feature is silently reverse-proxy-only. Anyone running claude --remote-control (which most modern users do since CC ≥ 2.1.196) will get zero override behavior with no error. The wrapper's own /health probe checks forward_proxy:true and refuses to launch otherwise — those users will not see this feature at all.
Not this PR's job to fix either. Documenting it in the code comment is the low-effort right move.
Verdict
Code and test quality is solid. Every issue PB and I raised is doc/design polish, not defect. First-timer PR with author responsiveness on their own NFR checklist is a good signal.
Not applying approved-by-lead — the author has open dialogue on the trust-model documentation, and I want to see their answer before advancing the label. Applying reviewed-by-lead to record my pass.
Load-bearing per author's own NFR checklist (touches core forwardRequest egress signature) — @chris human review required per project convention, on top of lead + codex.
— AI Team Lead
|
@rrva — friendly nudge on the two questions from my R0 review and the three angles AITL added. Both of us landed on "not defect, but worth aligning before merge." Recap:
All of these are one-liner comment adds or a short PR-body edit — no code changes to the logic. If you're around and can either address them or say "these are aspirational; happy to add the gate/comment," we can advance the labels quickly. If this is more scope than you'd like and the feature isn't blocking work on your end, an alternative is to close this PR and I can carry the same 6-LOC change forward in a repo-internal PR with the trust-model doc + env gate baked in from the start, crediting you as the source. Just let us know which path suits. — Proxy Builder |
What Changed & Why
Adds optional per-request upstream base URL overriding to the proxy forwarding pipeline via
ctx.meta.upstreamOverride.Extensions running in
onRequesthooks can now target alternative upstream bases per-request (e.g., routing specific model requests to alternative Anthropic-compatible endpoints or custom proxy mirrors) without altering the globalconfig.upstreamsetting.proxy/upstream.mjs:forwardRequest()accepts an optional 4th parameterupstreamBasewhich overridesconfig.upstream.buildUpstreamUrlconcatenation applies naturally so subpaths (e.g.,https://example.com/anthropic+/v1/messages) are preserved.proxy/server.mjs:handleMessagespassesmeta.upstreamOverrideintoforwardRequest().test/proxy-upstream.test.mjs: Added unit tests verifying override behavior and default fallback.Example Extension Usage
Anti-Bloat Metrics
Non-Functional Checklist
forwardRequestegress signature andhandleMessagesparameter handling).buildUpstreamHeaders, hop-by-hop stripping) applies unchanged to overridden targets.upstreamBaseis optional; whenundefined, behavior defaults toconfig.upstream.