diff --git a/server/src/__tests__/trust-proxy.test.ts b/server/src/__tests__/trust-proxy.test.ts new file mode 100644 index 00000000..27cd48eb --- /dev/null +++ b/server/src/__tests__/trust-proxy.test.ts @@ -0,0 +1,99 @@ +import { describe, expect, it } from "vitest"; +import express from "express"; +import { applyTrustProxy, parseTrustProxyEnv } from "../middleware/trust-proxy.js"; + +function appWithEnv(raw: string | undefined): express.Express { + const app = express(); + applyTrustProxy(app, parseTrustProxyEnv(raw)); + return app; +} + +describe("parseTrustProxyEnv", () => { + it("unset leaves Express at its safe default (trust nothing)", () => { + // Express's default trust-proxy setting is `false`. We verify the + // setting is unchanged by comparing against a vanilla express() + // instance that never had `applyTrustProxy` called on it. + const baseline = express().get("trust proxy"); + const app = appWithEnv(undefined); + expect(parseTrustProxyEnv(undefined)).toBeUndefined(); + expect(app.get("trust proxy")).toBe(baseline); + }); + + it("empty / false / 0 are treated as unset", () => { + expect(parseTrustProxyEnv("")).toBeUndefined(); + expect(parseTrustProxyEnv("false")).toBeUndefined(); + expect(parseTrustProxyEnv("0")).toBeUndefined(); + const baseline = express().get("trust proxy"); + expect(appWithEnv("0").get("trust proxy")).toBe(baseline); + }); + + it("'true' yields boolean true and sets app accordingly", () => { + expect(parseTrustProxyEnv("true")).toBe(true); + expect(appWithEnv("true").get("trust proxy")).toBe(true); + }); + + it("positive integer is parsed as a number", () => { + expect(parseTrustProxyEnv("2")).toBe(2); + expect(appWithEnv("2").get("trust proxy")).toBe(2); + }); + + it("'01' throws (strict integer, no leading zeros)", () => { + expect(() => parseTrustProxyEnv("01")).toThrow(/invalid integer/); + }); + + it("integer with internal whitespace throws", () => { + // A value like "1 2" (digits + whitespace + digits) is clearly not + // a single int and not a subnet list either — must be rejected. + // This is distinct from " 2 " (surrounding whitespace), which the + // outer `raw.trim()` accepts; see the next test for that contract. + // The parser happens to reach the subnet-token path for "1 2" + // (the inner-whitespace integer guard only fires when the whole + // string is `^\s*\d+\s*$`), so we match the unrecognized-token + // error rather than the "invalid integer" branch. + expect(() => parseTrustProxyEnv("1 2")).toThrow(/unrecognized token "1 2"/); + }); + + it("integer with surrounding whitespace is accepted (trimmed)", () => { + // The parser intentionally trims the *outer* value before matching, + // so " 2 " is equivalent to "2". Locking this in so the contract + // doesn't drift relative to the "internal whitespace throws" case. + expect(parseTrustProxyEnv(" 2 ")).toBe(2); + expect(appWithEnv(" 2 ").get("trust proxy")).toBe(2); + }); + + it("'loopback' yields a single-element array", () => { + const v = parseTrustProxyEnv("loopback"); + expect(v).toEqual(["loopback"]); + const app = appWithEnv("loopback"); + expect(app.get("trust proxy")).toEqual(["loopback"]); + }); + + it("'loopback,uniquelocal' yields a 2-element array", () => { + expect(parseTrustProxyEnv("loopback,uniquelocal")).toEqual([ + "loopback", + "uniquelocal", + ]); + }); + + it("IPv4 CIDR is accepted", () => { + expect(parseTrustProxyEnv("10.0.0.0/8")).toEqual(["10.0.0.0/8"]); + }); + + it("mixed IPv4 + IPv6 CIDR list is accepted with whitespace tolerance", () => { + expect(parseTrustProxyEnv(" 10.0.0.0/8 , fd00::/8 ")).toEqual([ + "10.0.0.0/8", + "fd00::/8", + ]); + }); + + it("'bogus' throws with a helpful message naming the bad token", () => { + expect(() => parseTrustProxyEnv("bogus")).toThrow(/bogus/); + expect(() => parseTrustProxyEnv("bogus")).toThrow(/loopback/); + }); + + it("partial-garbage list throws on the bad token, not silently dropped", () => { + expect(() => parseTrustProxyEnv("loopback,not-a-cidr")).toThrow( + /not-a-cidr/, + ); + }); +}); diff --git a/server/src/app.ts b/server/src/app.ts index 687ab72b..6658ecfa 100644 --- a/server/src/app.ts +++ b/server/src/app.ts @@ -9,6 +9,7 @@ import { httpLogger, errorHandler } from "./middleware/index.js"; import { actorMiddleware } from "./middleware/auth.js"; import { boardMutationGuard } from "./middleware/board-mutation-guard.js"; import { privateHostnameGuard, resolvePrivateHostnameAllowSet } from "./middleware/private-hostname-guard.js"; +import { applyTrustProxy, parseTrustProxyEnv } from "./middleware/trust-proxy.js"; import { healthRoutes } from "./routes/health.js"; import { companyRoutes } from "./routes/companies.js"; import { companySkillRoutes } from "./routes/company-skills.js"; @@ -160,6 +161,11 @@ export async function createApp( (req as unknown as { rawBody: Buffer }).rawBody = buf; }; + // Respect the operator's `TRUST_PROXY` env var (see middleware/trust-proxy.ts). + // Default is unset → Express trusts nothing, which is the only safe choice + // when the server may be reachable without a known reverse proxy in front. + applyTrustProxy(app, parseTrustProxyEnv(process.env.TRUST_PROXY)); + app.use(COMPANY_IMPORT_API_PATH, express.json({ limit: PORTABLE_JSON_BODY_LIMIT, verify: captureRawBody, diff --git a/server/src/middleware/trust-proxy.ts b/server/src/middleware/trust-proxy.ts new file mode 100644 index 00000000..e4e18499 --- /dev/null +++ b/server/src/middleware/trust-proxy.ts @@ -0,0 +1,117 @@ +/** + * Parser for the `TRUST_PROXY` env var, which mirrors Express 5's + * `trust proxy` setting. The default is intentionally *unset* — Express + * then trusts nothing and `req.ip` / `X-Forwarded-For` cannot be spoofed + * by arbitrary clients. Operators opt in only when there is a real LB + * in front of the server. + * + * Accepted forms (case-sensitive for the keywords, matching Express): + * + * unset | "" | "false" | "0" -> undefined (caller skips `app.set`) + * "true" -> true (UNSAFE behind untrusted LBs) + * "" -> number (trust N hops) + * comma-separated tokens -> string[] of named subnets + CIDRs + * + * Named subnets accepted verbatim by Express: loopback, linklocal, + * uniquelocal. CIDR validation is intentionally lax (Postel's law): + * Express only uses the array form for prefix matching, so we just + * reject obvious garbage. Anything that doesn't match the regex or + * keyword whitelist throws at startup with a clear error. + */ + +export type TrustProxyValue = boolean | number | string[]; + +const NAMED_SUBNETS: ReadonlySet = new Set([ + "loopback", + "linklocal", + "uniquelocal", +]); + +// IPv4 with optional /CIDR (0-32), or IPv6 with optional /CIDR (0-128). +// Not 100% RFC-correct on purpose — Express tolerates loose forms and +// we just want to reject obvious typos / shell-quoting accidents. +const IPV4_RE = /^(?:\d{1,3}\.){3}\d{1,3}(?:\/(?:3[0-2]|[12]?\d))?$/; +// IPV6_RE is intentionally lax — it is *not* a full RFC 4291 validator. +// It matches any non-empty string of hex digits and colons (plus an +// optional /CIDR suffix in 0-128), which means tokens like ":::", +// "aaaa", or "gggg::1" (the last only if it had hex chars — "gggg" is +// hex-valid) can pass this regex. The colon-presence guard in +// `isValidSubnetToken` rejects purely-numeric strings, but malformed +// IPv6-shaped tokens still slip through to Express. +// +// Trade-off: the goal here is *fast rejection of obvious typos* (e.g. +// "10.0.0/8" missing an octet, shell-quoting accidents, stray +// alphabetics) — not strict parse. When a malformed-but-passed-through +// token reaches Express, Express silently ignores entries it cannot +// parse rather than throwing at startup. The practical impact is a +// silently-unconfigured trust-proxy entry, not a crash. +// +// Operators are therefore expected to verify their TRUST_PROXY config +// by hitting the server through their LB and confirming `req.ip` in the +// request log matches the real client IP. If you tighten this regex, +// also add tests for the rejected forms; see trust-proxy.test.ts. +const IPV6_RE = /^[0-9A-Fa-f:]+(?:\/(?:12[0-8]|1[01]\d|\d{1,2}))?$/; +// Strict positive integer: no leading zeros, no whitespace, no sign. +const STRICT_POS_INT_RE = /^[1-9]\d*$/; + +function isValidSubnetToken(token: string): boolean { + if (NAMED_SUBNETS.has(token)) return true; + if (IPV4_RE.test(token)) return true; + // IPv6 must contain at least one colon; the regex above is loose enough + // that bare numbers like "10" would otherwise sneak through. + if (token.includes(":") && IPV6_RE.test(token)) return true; + return false; +} + +/** + * Parse a raw env-var value into the form Express's `app.set("trust proxy", …)` + * accepts, or `undefined` to mean "leave Express at its safe default." + * + * Throws `Error` with an explanatory message if the value is malformed. + */ +export function parseTrustProxyEnv(raw: string | undefined): TrustProxyValue | undefined { + if (raw === undefined) return undefined; + // We intentionally trim only the *outer* value — tokens inside the + // comma list are trimmed individually below. Leading/trailing whitespace + // around the whole value (e.g. " 2 ") is accepted because trim() reduces + // it to "2" before STRICT_POS_INT_RE is applied; only *internal* + // whitespace (e.g. "1 2") falls through to the subnet path and errors as + // an unrecognised token. + const value = raw.trim(); + if (value === "" || value === "false" || value === "0") return undefined; + if (value === "true") return true; + if (STRICT_POS_INT_RE.test(value)) return Number(value); + // Reject the "01" / " 2" forms explicitly — if the value is *purely* + // digits-or-whitespace but didn't match STRICT_POS_INT_RE, it's a + // typo, not a subnet list. + if (/^\s*\d+\s*$/.test(raw)) { + throw new Error( + `TRUST_PROXY: invalid integer value ${JSON.stringify(raw)} — use a positive integer with no leading zeros or whitespace`, + ); + } + const tokens = value + .split(",") + .map((t) => t.trim()) + .filter((t) => t.length > 0); + if (tokens.length === 0) return undefined; + for (const token of tokens) { + if (!isValidSubnetToken(token)) { + throw new Error( + `TRUST_PROXY: unrecognized token ${JSON.stringify(token)} — expected one of {loopback, linklocal, uniquelocal} or a CIDR like 10.0.0.0/8 or fd00::/8`, + ); + } + } + return tokens; +} + +/** + * Apply the parsed value to the given Express app. No-op when the value + * is `undefined`, preserving Express's default (trust nothing). + */ +export function applyTrustProxy( + app: { set: (key: string, value: TrustProxyValue) => unknown }, + value: TrustProxyValue | undefined, +): void { + if (value === undefined) return; + app.set("trust proxy", value); +}