Pin trust_proxy to a hop count so a forged X-Forwarded-For can't impersonate the allowlisted crawler - #148
Draft
jpr5 wants to merge 1 commit into
Draft
Pin trust_proxy to a hop count so a forged X-Forwarded-For can't impersonate the allowlisted crawler#148jpr5 wants to merge 1 commit into
jpr5 wants to merge 1 commit into
Conversation
…rsonate the allowlisted crawler All three shipped deploy configs set `trust_proxy: true`. Express's boolean `trust proxy = true` does not mean "one proxy" — it trusts every hop on the X-Forwarded-For chain and resolves `req.ip` to the LEFTMOST entry, which is entirely client-supplied. These deployments also allowlist the Anthropic crawler IP (160.79.106.35) to exempt it from the per-IP session cap (default 20 sessions/IP). Combined, any caller could send `X-Forwarded-For: 160.79.106.35` and be attributed to the crawler: the per-IP session limiter was bypassed outright, and query_log client_ip attribution was poisoned with whatever the caller chose. Exactly one proxy fronts these containers — Railway's edge terminates TLS and forwards to the Node process, with no CDN and no in-container reverse proxy — so `trust_proxy: 1` is the correct depth, not merely a smaller one. With a hop count Express counts inward from the socket, so a client-forged XFF prefix is ignored because the edge appends the real peer to the right of it. Legitimate single-hop proxy attribution is unchanged. This is the setting src/ip-util.ts already recommended for single-hop deployments; the shipped configs contradicted the code's own guidance. Also corrects the two places that pointed operators at `true`: pathfinder.example.yaml's allowlist note and the stale prod-value reference in the Atlas sandbox doc. Regression coverage in src/__tests__/deploy-trust-proxy.test.ts reads trust_proxy out of the shipped YAML rather than hard-coding it, and pins the resolved IP for a forged multi-entry XFF, a legitimate single hop, and no XFF at all, plus the allowlist session-cap bypass.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The problem
All three shipped deploy configs set
trust_proxy: true:Express's boolean
trust proxy = truedoes not mean "one proxy". It trusts every hop on theX-Forwarded-Forchain and resolvesreq.ipto the leftmost entry — which is entirely client-supplied.The same configs allowlist the Anthropic crawler IP to exempt it from the per-IP session cap:
max_sessions_per_ipdefaults to 20 (src/server.ts:4120), and the allowlist is checked against the resolved IP (src/ip-limiter.ts:271,[mcp] IP … on allowlist, bypassing session limit). So the two settings together mean any caller could sendX-Forwarded-For: 160.79.106.35and be attributed to the crawler — bypassing the per-IP session limiter outright, and poisoningquery_log.client_ip(src/db/analytics.ts:371) with whatever value they chose.src/ip-util.ts:23-33already documents the hop count as the correct setting for a single-hop deployment. The shipped configs contradicted the code's own guidance.Topology and the value chosen
trust_proxy: 1, not merely "something smaller thantrue".Exactly one proxy sits in front of these containers:
railway.tomldeploys the container directly;Dockerfileprod stage runsCMD ["node", "dist/cli.js", "serve"]— no sidecar, no in-container nginx/caddy/envoy (grepped: zero hits outside a prose mention).cf-ray, no second proxy signature):pathfinder.example.yamlstates the value for this topology itself:<integer> — trust N hops (e.g. 1 for single-proxy Railway).With a hop count, Express counts inward from the socket, so a client-forged XFF prefix is ignored — the edge appends the real peer to the right of anything the client sent. Note this is correct regardless of whether Railway's edge strips or appends client-supplied XFF, whereas
trueis only safe under the strip assumption that the old comment asserted without evidence.RED — before the fix (
trust_proxy: true)Real server booted from source (
npx tsx src/index.ts,NODE_ENV=production) with a config carrying the sameallowlist: ["160.79.106.35"]andtrust_proxy: true, andPATHFINDER_TRACE_CLAUDE_AI=1so the trace middleware logs the resolved IP:Three
POST /mcpinitialize requests — forged multi-entry XFF, legitimate single hop, no XFF:The first request sent
X-Forwarded-For: 160.79.106.35, 203.0.113.7— a forged allowlisted IP followed by the real peer as a fronting proxy would append it. The server attributed it to160.79.106.35and loggedon allowlist, bypassing session limit. Full impersonation, limiter bypassed, and the session tagged with the forged IP.GREEN — after the fix (
trust_proxy: 1)Same server, same three requests, byte-identical headers:
Both halves hold:
160.79.106.35prefix is ignored; the request resolves to203.0.113.7, the peer the proxy appended. Theon allowlist, bypassing session limitline is gone, so the limiter now applies.X-Forwarded-For: 198.51.100.9still resolves to198.51.100.9, not to the proxy address. A fix that hardened forgery by collapsing every request onto the proxy IP would have broken rate-limit bucketing and analytics; this one doesn't.Regression test
src/__tests__/deploy-trust-proxy.test.ts(18 tests) readstrust_proxyout of the shipped YAML rather than hard-coding it, so a revert totruefails the behavioral assertions and not just a shape check. Per deploy config it pins:getSessionCount("160.79.106.35") === 0)Plus shape assertions that the value is a positive integer
1and explicitlynot true, and that the crawler IP is still on the allowlist so the forgery cases can't pass vacuously.Mutation check
Reverted the production line (
trust_proxy: 1→trueindeploy/copilotkit-docs.yaml) and re-ran — CAUGHT, 3 failures, including the vulnerability itself:The 15 that still passed are the legitimate-hop and no-XFF cases, which are genuinely unaffected by the mutation — so the suite discriminates rather than failing wholesale.
No-op control: with
trust_proxy: 1restored, a comment-only edit to the same file → 18 passed (18). The CAUGHT verdict is attributable to the value, not to touching the file.Gates
npx prettier --check "src/**/*.ts"— cleannpm run build— cleannpx tsc --noEmit -p tsconfig.scripts.json— cleannpm test— 3572 passed, 1 skipped (179 files)Notes for the reviewer
scripts/atlas-harvest/SANDBOX.mddocumented the prod value astrust_proxy: true, which my change made stale. The one-token correction shortened a markdown table cell, so Prettier realigned the whole table — the 18-line diff there is alignment only, one token of real change. (CI's prettier job covers TS only, but the file was Prettier-clean onmainand I kept it that way.)pathfinder.example.yamlkeeps its hardenedtrust_proxy: falsedefault; only the allowlist note that pointed attruechanged.docs/.