diff --git a/deploy/aimock-docs.yaml b/deploy/aimock-docs.yaml index 2b345d8..73e6a68 100644 --- a/deploy/aimock-docs.yaml +++ b/deploy/aimock-docs.yaml @@ -4,11 +4,23 @@ server: max_sessions: 1000 session_ttl_minutes: 30 session_unused_ttl_minutes: 15 - # Anthropic crawler — bypasses per-IP session limit (requires trust_proxy: true to match the forwarded IP) + # Anthropic crawler — bypasses per-IP session limit (requires trust_proxy to be + # set so the allowlist matches the forwarded client IP, not the proxy's) allowlist: ["160.79.106.35"] - # Safe to enable: Railway's edge discards any client-supplied X-Forwarded-For - # and sets its own trusted value, so the leftmost XFF entry is the real peer. - trust_proxy: true + # Numeric hop count, NOT `true`. Exactly one proxy sits in front of this + # container: Railway's edge terminates TLS and forwards to the Node process + # (no CDN, no in-container reverse proxy). `trust proxy: 1` therefore tells + # Express to trust that single hop and resolve req.ip to the address the + # edge itself appended. + # + # `true` would be a vulnerability here: it trusts EVERY hop and resolves + # req.ip to the LEFTMOST X-Forwarded-For entry, which is client-supplied. + # Any caller could send `X-Forwarded-For: 160.79.106.35` and be attributed + # to the allowlisted crawler above — bypassing the per-IP session limit and + # poisoning query_log IP attribution. With a hop count, a forged prefix is + # ignored because the edge appends the real peer to the right of it. + # See src/ip-util.ts for the full rationale. + trust_proxy: 1 sources: - name: docs diff --git a/deploy/copilotkit-docs.yaml b/deploy/copilotkit-docs.yaml index 628d3d9..8ed9f78 100644 --- a/deploy/copilotkit-docs.yaml +++ b/deploy/copilotkit-docs.yaml @@ -4,11 +4,23 @@ server: max_sessions: 1000 session_ttl_minutes: 30 session_unused_ttl_minutes: 15 - # Anthropic crawler — bypasses per-IP session limit (requires trust_proxy: true to match the forwarded IP) + # Anthropic crawler — bypasses per-IP session limit (requires trust_proxy to be + # set so the allowlist matches the forwarded client IP, not the proxy's) allowlist: ["160.79.106.35"] - # Safe to enable: Railway's edge discards any client-supplied X-Forwarded-For - # and sets its own trusted value, so the leftmost XFF entry is the real peer. - trust_proxy: true + # Numeric hop count, NOT `true`. Exactly one proxy sits in front of this + # container: Railway's edge terminates TLS and forwards to the Node process + # (no CDN, no in-container reverse proxy). `trust proxy: 1` therefore tells + # Express to trust that single hop and resolve req.ip to the address the + # edge itself appended. + # + # `true` would be a vulnerability here: it trusts EVERY hop and resolves + # req.ip to the LEFTMOST X-Forwarded-For entry, which is client-supplied. + # Any caller could send `X-Forwarded-For: 160.79.106.35` and be attributed + # to the allowlisted crawler above — bypassing the per-IP session limit and + # poisoning query_log IP attribution. With a hop count, a forged prefix is + # ignored because the edge appends the real peer to the right of it. + # See src/ip-util.ts for the full rationale. + trust_proxy: 1 sources: - name: docs diff --git a/deploy/pathfinder-docs.yaml b/deploy/pathfinder-docs.yaml index dd6d513..1c7059d 100644 --- a/deploy/pathfinder-docs.yaml +++ b/deploy/pathfinder-docs.yaml @@ -4,11 +4,23 @@ server: max_sessions: 1000 session_ttl_minutes: 30 session_unused_ttl_minutes: 15 - # Anthropic crawler — bypasses per-IP session limit (requires trust_proxy: true to match the forwarded IP) + # Anthropic crawler — bypasses per-IP session limit (requires trust_proxy to be + # set so the allowlist matches the forwarded client IP, not the proxy's) allowlist: ["160.79.106.35"] - # Safe to enable: Railway's edge discards any client-supplied X-Forwarded-For - # and sets its own trusted value, so the leftmost XFF entry is the real peer. - trust_proxy: true + # Numeric hop count, NOT `true`. Exactly one proxy sits in front of this + # container: Railway's edge terminates TLS and forwards to the Node process + # (no CDN, no in-container reverse proxy). `trust proxy: 1` therefore tells + # Express to trust that single hop and resolve req.ip to the address the + # edge itself appended. + # + # `true` would be a vulnerability here: it trusts EVERY hop and resolves + # req.ip to the LEFTMOST X-Forwarded-For entry, which is client-supplied. + # Any caller could send `X-Forwarded-For: 160.79.106.35` and be attributed + # to the allowlisted crawler above — bypassing the per-IP session limit and + # poisoning query_log IP attribution. With a hop count, a forged prefix is + # ignored because the edge appends the real peer to the right of it. + # See src/ip-util.ts for the full rationale. + trust_proxy: 1 sources: - name: pathfinder-docs diff --git a/pathfinder.example.yaml b/pathfinder.example.yaml index efc8281..7ce88ed 100644 --- a/pathfinder.example.yaml +++ b/pathfinder.example.yaml @@ -22,7 +22,10 @@ server: # - "160.79.106.35" # Example: Anthropic Assistant crawler # - "10.0.0.0/8" # Example: internal health-probe CIDR # NOTE: behind a reverse proxy (Railway, Fly, etc.), allowlist entries only match - # when trust_proxy: true. Otherwise the server sees the proxy IP for every request. + # when trust_proxy is set. Otherwise the server sees the proxy IP for every + # request. Prefer a hop count (e.g. 1) over `true` — with `true`, a client can + # forge X-Forwarded-For and be attributed to an allowlisted IP. See the + # SECURITY WARNING below. # # ⚠️ SECURITY WARNING — trust_proxy ⚠️ # When true, the server honors X-Forwarded-For and populates req.ip from diff --git a/scripts/atlas-harvest/SANDBOX.md b/scripts/atlas-harvest/SANDBOX.md index f518579..3b6e953 100644 --- a/scripts/atlas-harvest/SANDBOX.md +++ b/scripts/atlas-harvest/SANDBOX.md @@ -48,14 +48,14 @@ migration step. (`deploy/copilotkit-docs.yaml`), not a from-scratch config. Start from the prod file and apply these deltas: -| # | Prod | Sandbox | Why | -| --- | ----------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------- | -| 1 | `server.name`, `allowlist`, `trust_proxy: true` | name suffixed `-sandbox`; allowlist and trust_proxy **dropped** | Crawler-IP allowlist and proxy trust are Railway-edge concerns | -| 2 | 4 file sources over real docs/code repos | same 4 source names/types/chunking, but `repo` → `https://github.com/CopilotKit/pathfinder.git`, `path: "."`, and `file_patterns` deliberately **non-matching** (`**/*.sandbox-none.*`) | Startup `checkAndIndex` clones one small repo and indexes **0 chunks per source** — zero embedding calls, so the dummy key is never exercised | -| 3 | no `type: atlas` source | `type: atlas` source block **added** | Step-6 prerequisite — `POST /admin/reindex` 400s `unknown_source` for `--source atlas` without it | -| 4 | `indexing.auto_reindex: true` | `false` | No nightly reindex in the sandbox | -| 5 | `webhook:` block (repo_sources/path_triggers) | **removed** | No webhook surface locally | -| 6 | tools (4 search + 2 bash + 1 collect) | identical structure, descriptions shortened | Keep the tool surface real | +| # | Prod | Sandbox | Why | +| --- | --------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------- | +| 1 | `server.name`, `allowlist`, `trust_proxy: 1` | name suffixed `-sandbox`; allowlist and trust_proxy **dropped** | Crawler-IP allowlist and proxy trust are Railway-edge concerns | +| 2 | 4 file sources over real docs/code repos | same 4 source names/types/chunking, but `repo` → `https://github.com/CopilotKit/pathfinder.git`, `path: "."`, and `file_patterns` deliberately **non-matching** (`**/*.sandbox-none.*`) | Startup `checkAndIndex` clones one small repo and indexes **0 chunks per source** — zero embedding calls, so the dummy key is never exercised | +| 3 | no `type: atlas` source | `type: atlas` source block **added** | Step-6 prerequisite — `POST /admin/reindex` 400s `unknown_source` for `--source atlas` without it | +| 4 | `indexing.auto_reindex: true` | `false` | No nightly reindex in the sandbox | +| 5 | `webhook:` block (repo_sources/path_triggers) | **removed** | No webhook surface locally | +| 6 | tools (4 search + 2 bash + 1 collect) | identical structure, descriptions shortened | Keep the tool surface real | The two snippets that matter. Each file source keeps its prod shape but matches nothing: @@ -96,16 +96,16 @@ source at boot — that is the design, not a failure. Run `node dist/index.js` from the built checkout with this environment: -| Variable | Value | Notes | -| ------------------- | ---------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------- | -| `DATABASE_URL` | `postgresql://mcp:mcp_local@localhost:5433/mcp_docs` | The Step-1 container | -| `PATHFINDER_CONFIG` | `/sandbox.yaml` | Absolute path | -| `ANALYTICS_TOKEN` | `sandbox-smoke` | The bearer for every API call below; any string works | -| `OPENAI_API_KEY` | `sandbox-dummy` | Must be SET (search tools + `embedding.provider: openai` exist in the config) but is never called over an empty corpus | -| `PORT` | `3001` | | -| `NODE_ENV` | `production` | **Required to exercise the 401 path** — in dev mode `bearerTokenAuth` bypasses the token check for localhost requests | -| `MCP_JWT_SECRET` | random (`openssl rand -hex 32`) | **Required whenever `NODE_ENV=production`** — startup is fatal without it. The two settings travel together | -| `PATHFINDER_CONSENT_HMAC_KEY` | random (`openssl rand -hex 32`) | **Also required whenever `NODE_ENV=production`** — signs the OAuth consent-nonce; startup is fatal without it. Comma-separate values to rotate (all accepted on verify) | +| Variable | Value | Notes | +| ----------------------------- | ---------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `DATABASE_URL` | `postgresql://mcp:mcp_local@localhost:5433/mcp_docs` | The Step-1 container | +| `PATHFINDER_CONFIG` | `/sandbox.yaml` | Absolute path | +| `ANALYTICS_TOKEN` | `sandbox-smoke` | The bearer for every API call below; any string works | +| `OPENAI_API_KEY` | `sandbox-dummy` | Must be SET (search tools + `embedding.provider: openai` exist in the config) but is never called over an empty corpus | +| `PORT` | `3001` | | +| `NODE_ENV` | `production` | **Required to exercise the 401 path** — in dev mode `bearerTokenAuth` bypasses the token check for localhost requests | +| `MCP_JWT_SECRET` | random (`openssl rand -hex 32`) | **Required whenever `NODE_ENV=production`** — startup is fatal without it. The two settings travel together | +| `PATHFINDER_CONSENT_HMAC_KEY` | random (`openssl rand -hex 32`) | **Also required whenever `NODE_ENV=production`** — signs the OAuth consent-nonce; startup is fatal without it. Comma-separate values to rotate (all accepted on verify) | On macOS, daemonize via Python — `nohup` + `disown` dies with a spawning subagent shell, and **`env=env` must be passed explicitly** (omitting it diff --git a/src/__tests__/deploy-trust-proxy.test.ts b/src/__tests__/deploy-trust-proxy.test.ts new file mode 100644 index 0000000..611ac5f --- /dev/null +++ b/src/__tests__/deploy-trust-proxy.test.ts @@ -0,0 +1,228 @@ +import { describe, it, expect, afterEach } from "vitest"; +import express from "express"; +import path from "node:path"; +import { readFileSync } from "node:fs"; +import { fileURLToPath } from "node:url"; +import { parse as parseYaml } from "yaml"; +import type { Server } from "node:http"; +import type { AddressInfo } from "node:net"; +import type { Request, Response } from "express"; +import { clientIp } from "../ip-util.js"; +import { IpSessionLimiter } from "../ip-limiter.js"; + +/** + * Regression suite for the shipped `trust_proxy` value in `deploy/*.yaml`. + * + * Background — the shipped configs used to 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 fully client-supplied. Because these deployments also carry an + * `allowlist` that exempts the Anthropic crawler IP from the per-IP session + * cap, any caller could send `X-Forwarded-For: 160.79.106.35` and be + * attributed to the allowlisted crawler — bypassing `max_sessions_per_ip` + * (default 20) and poisoning `query_log.client_ip` attribution. + * + * The fix is a numeric hop count. Exactly one proxy fronts these containers + * (Railway's edge terminates TLS and forwards to the Node process; there is no + * CDN and no in-container reverse proxy), so `trust_proxy: 1` is the correct + * depth. 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. + * + * These tests read `trust_proxy` out of the shipped YAML rather than + * hard-coding it, so reverting the production line to `true` fails the + * behavioral assertions and not merely the shape assertion. + */ + +const DEPLOY_DIR = path.join( + path.dirname(fileURLToPath(import.meta.url)), + "..", + "..", + "deploy", +); + +const DEPLOY_CONFIGS = [ + "copilotkit-docs.yaml", + "pathfinder-docs.yaml", + "aimock-docs.yaml", +] as const; + +type ShippedServerConfig = { + allowlist?: string[]; + trust_proxy?: boolean | number | string[]; +}; + +function loadShippedServerConfig(file: string): ShippedServerConfig { + const raw = readFileSync(path.join(DEPLOY_DIR, file), "utf-8"); + const parsed = parseYaml(raw) as { server?: ShippedServerConfig }; + expect(parsed.server, `${file} must define a server block`).toBeDefined(); + return parsed.server as ShippedServerConfig; +} + +/** + * The allowlisted crawler IP an attacker would forge to win the session-cap + * bypass. Asserted to actually be on the shipped allowlist below, so the + * suite can't silently drift into testing a meaningless address. + */ +const ALLOWLISTED_CRAWLER_IP = "160.79.106.35"; + +/** Stand-in for the real peer address the fronting proxy appends. */ +const REAL_CLIENT_IP = "203.0.113.7"; + +/** Stand-in for a legitimate single-hop XFF set by the fronting proxy. */ +const LEGITIMATE_PROXY_HOP_IP = "198.51.100.9"; + +let server: Server | undefined; + +afterEach(async () => { + if (server) { + await new Promise((resolve) => server!.close(() => resolve())); + server = undefined; + } +}); + +/** + * Boot a real Express app wired exactly the way `startServer()` wires it: + * `app.set("trust proxy", )` and IP resolution through the + * shared `clientIp()` helper. Returns the base URL. + */ +async function startProbeApp( + trustProxy: boolean | number | string[], + limiter?: IpSessionLimiter, +): Promise { + const app = express(); + app.set("trust proxy", trustProxy); + let sessionSeq = 0; + app.get("/whoami", (req: Request, res: Response) => { + const ip = clientIp(req, trustProxy); + if (limiter) { + const allowlisted = limiter.isAllowlisted(ip); + // Mirrors the real admission path in sse-handlers/server: resolve the + // IP, then hand it to the limiter, which exempts allowlisted IPs from + // the per-IP session cap. + const admitted = limiter.tryAdd(ip, `probe-${++sessionSeq}`); + res.status(admitted ? 200 : 429).json({ ip, allowlisted }); + return; + } + res.status(200).json({ ip, allowlisted: false }); + }); + + server = await new Promise((resolve) => { + const s = app.listen(0, "127.0.0.1", () => resolve(s)); + }); + const { port } = server.address() as AddressInfo; + return `http://127.0.0.1:${port}`; +} + +async function whoami( + baseUrl: string, + xff?: string, +): Promise<{ status: number; ip: string; allowlisted: boolean }> { + const res = await fetch(`${baseUrl}/whoami`, { + headers: xff ? { "X-Forwarded-For": xff } : {}, + }); + const body = (await res.json()) as { ip: string; allowlisted: boolean }; + return { status: res.status, ip: body.ip, allowlisted: body.allowlisted }; +} + +describe("deploy configs pin a numeric trust_proxy hop count", () => { + for (const file of DEPLOY_CONFIGS) { + it(`${file} sets trust_proxy to a positive integer hop count, not boolean true`, () => { + const { trust_proxy: trustProxy } = loadShippedServerConfig(file); + + // Boolean `true` is the vulnerability: it resolves req.ip to the + // leftmost (client-supplied) XFF entry. + expect(trustProxy).not.toBe(true); + expect(typeof trustProxy).toBe("number"); + expect(Number.isInteger(trustProxy as number)).toBe(true); + // One proxy hop: Railway's edge. Anything larger would trust an + // attacker-supplied entry again. + expect(trustProxy).toBe(1); + }); + + it(`${file} still allowlists the crawler IP the hop count protects`, () => { + const { allowlist } = loadShippedServerConfig(file); + // If the allowlist ever drops, the forgery tests below stop being + // meaningful — pin it so they fail loudly instead of passing vacuously. + expect(allowlist).toContain(ALLOWLISTED_CRAWLER_IP); + }); + } +}); + +describe("resolved client IP under the shipped trust_proxy value", () => { + for (const file of DEPLOY_CONFIGS) { + describe(file, () => { + it("ignores a forged multi-entry X-Forwarded-For and uses the proxy-appended peer", async () => { + const { trust_proxy: trustProxy } = loadShippedServerConfig(file); + const baseUrl = await startProbeApp( + trustProxy as boolean | number | string[], + ); + + // What the app sees when a client forges XFF and the single fronting + // proxy appends the real peer to the right of it. + const result = await whoami( + baseUrl, + `${ALLOWLISTED_CRAWLER_IP}, ${REAL_CLIENT_IP}`, + ); + + expect(result.ip).toBe(REAL_CLIENT_IP); + // The whole point: the forged allowlisted IP must never win. + expect(result.ip).not.toBe(ALLOWLISTED_CRAWLER_IP); + }); + + it("resolves a legitimate single proxy hop correctly", async () => { + const { trust_proxy: trustProxy } = loadShippedServerConfig(file); + const baseUrl = await startProbeApp( + trustProxy as boolean | number | string[], + ); + + // Real proxy attribution must keep working — a fix that hardened + // forgery by breaking legitimate XFF would silently collapse every + // rate-limit bucket and analytics row onto the proxy address. + const result = await whoami(baseUrl, LEGITIMATE_PROXY_HOP_IP); + + expect(result.ip).toBe(LEGITIMATE_PROXY_HOP_IP); + }); + + it("falls back to the socket peer when no X-Forwarded-For is present", async () => { + const { trust_proxy: trustProxy } = loadShippedServerConfig(file); + const baseUrl = await startProbeApp( + trustProxy as boolean | number | string[], + ); + + const result = await whoami(baseUrl); + + // Node reports loopback as either form depending on stack config. + expect(["127.0.0.1", "::ffff:127.0.0.1"]).toContain(result.ip); + }); + + it("does not let a forged X-Forwarded-For win the allowlist session-cap bypass", async () => { + const { trust_proxy: trustProxy, allowlist } = + loadShippedServerConfig(file); + // Cap of 1 with the shipped allowlist: an allowlisted IP bypasses the + // cap entirely, so a successful forgery would return 200 twice. + const limiter = new IpSessionLimiter(1, { allowlist }); + const baseUrl = await startProbeApp( + trustProxy as boolean | number | string[], + limiter, + ); + + const forged = `${ALLOWLISTED_CRAWLER_IP}, ${REAL_CLIENT_IP}`; + + const first = await whoami(baseUrl, forged); + expect(first.status).toBe(200); + expect(first.allowlisted).toBe(false); + expect(first.ip).toBe(REAL_CLIENT_IP); + + // Second forged request must be rate-limited. Under `trust_proxy: + // true` this returned 200 with allowlisted=true — the bypass. + const second = await whoami(baseUrl, forged); + expect(second.status).toBe(429); + expect(second.allowlisted).toBe(false); + + // The forged crawler IP must never have been counted or exempted. + expect(limiter.getSessionCount(ALLOWLISTED_CRAWLER_IP)).toBe(0); + }); + }); + } +});