diff --git a/src/config.ts b/src/config.ts index 12ef55b..3229700 100644 --- a/src/config.ts +++ b/src/config.ts @@ -34,7 +34,15 @@ interface Config { S3_ENDPOINT: string | null; S3_REGION: string | null; STORAGE: "filesystem" | "s3"; - TRUST_PROXY: number; + /** + * Value for Express "trust proxy": either a fixed hop count ("1", "2") + * or a comma-separated list of trusted subnets — named subnets + * ("loopback", "linklocal", "uniquelocal"), IPs/CIDRs, and the keyword + * "cloudflare" (expands to Cloudflare's published ranges). The subnet + * form keeps request.ip pointing at the real visitor even when the + * proxy chain gains or loses hops. See src/server/trustProxy.ts. + */ + TRUST_PROXY: string; RATE_LIMIT: number; } const config: Config = { @@ -55,7 +63,11 @@ const config: Config = { AUTH_CALLBACK: "http://localhost:5000/github/auth", ANONYMIZATION_MASK: "XXXX", PORT: 5000, - TRUST_PROXY: 1, + // Trust the local reverse proxy (docker network / host nginx) and + // Cloudflare's edges by subnet rather than by hop count, so client-IP + // resolution survives Cloudflare adding or removing X-Forwarded-For + // entries. Set a plain number to restore the legacy hop-count behavior. + TRUST_PROXY: "loopback,uniquelocal,cloudflare", RATE_LIMIT: 350, APP_HOSTNAME: "anonymous.4open.science", DB_USERNAME: "admin", diff --git a/src/server/index.ts b/src/server/index.ts index c96c379..f3b58c3 100644 --- a/src/server/index.ts +++ b/src/server/index.ts @@ -29,6 +29,7 @@ import { import DailyStatsModel from "../core/model/dailyStats/dailyStats.model"; import { getUser } from "./routes/route-utils"; import config from "../config"; +import { resolveTrustProxy, isCloudflareIP } from "./trustProxy"; import { createLogger, serializeError } from "../core/logger"; const logger = createLogger("server"); @@ -109,10 +110,14 @@ export default async function start() { }) ); app.set("etag", "strong"); - // Trust exactly TRUST_PROXY proxy hops so Express derives request.ip from - // the right X-Forwarded-For entry. This is what makes request.ip - // trustworthy for rate limiting instead of a client-spoofable header. - app.set("trust proxy", config.TRUST_PROXY); + // Trust the proxies declared in TRUST_PROXY (subnet list or legacy hop + // count) so Express derives request.ip from the right X-Forwarded-For + // entry. With the subnet form, Express skips every trusted address no + // matter how many entries the proxies add, so request.ip stays the real + // visitor even when Cloudflare changes how it builds the header. This is + // what makes request.ip trustworthy for rate limiting instead of a + // client-spoofable header. + app.set("trust proxy", resolveTrustProxy(config.TRUST_PROXY)); // handle session and connection app.use(initSession()); @@ -139,16 +144,30 @@ export default async function start() { _response: express.Response ): string { // Use request.ip, which Express resolves from X-Forwarded-For honouring - // the configured "trust proxy" hop count. Do NOT key off the - // cf-connecting-ip header directly: when the server isn't actually behind - // Cloudflare a client can set that header to an arbitrary value per - // request and trivially bypass the rate limiter (CWE-290). - if (!request.ip && request.socket.remoteAddress) { + // the configured "trust proxy" setting. Do NOT key off the + // cf-connecting-ip header unconditionally: when the server isn't actually + // behind Cloudflare a client can set that header to an arbitrary value + // per request and trivially bypass the rate limiter (CWE-290). + let ip = request.ip; + if (!ip && request.socket.remoteAddress) { logger.warn("request.ip is missing"); - return request.socket.remoteAddress; + ip = request.socket.remoteAddress; + } + // If resolution stopped at a Cloudflare edge address (X-Forwarded-For no + // longer contains the visitor, e.g. after a Cloudflare-side change), fall + // back to cf-connecting-ip. This is safe: request.ip can only be a + // Cloudflare address when every hop up to it is trusted, i.e. the request + // genuinely traversed Cloudflare. A direct client forging the header is + // keyed by its own untrusted address instead, so the CWE-290 bypass above + // does not apply. + if (ip && isCloudflareIP(ip)) { + const cfConnectingIP = request.headers["cf-connecting-ip"]; + if (typeof cfConnectingIP === "string" && cfConnectingIP) { + ip = cfConnectingIP.trim(); + } } // remove port number from IPv4 addresses - return (request.ip || "").replace(/:\d+[^:]*$/, ""); + return (ip || "").replace(/:\d+[^:]*$/, ""); } const rate = rateLimit({ diff --git a/src/server/trustProxy.ts b/src/server/trustProxy.ts new file mode 100644 index 0000000..08d599d --- /dev/null +++ b/src/server/trustProxy.ts @@ -0,0 +1,89 @@ +import { BlockList, isIP } from "net"; + +/** + * Cloudflare's published egress ranges (https://www.cloudflare.com/ips/). + * They change rarely; refresh from https://www.cloudflare.com/ips-v4 and + * https://www.cloudflare.com/ips-v6 if Cloudflare announces new ranges. + */ +export const CLOUDFLARE_IP_RANGES: readonly string[] = [ + // IPv4 + "173.245.48.0/20", + "103.21.244.0/22", + "103.22.200.0/22", + "103.31.4.0/22", + "141.101.64.0/18", + "108.162.192.0/18", + "190.93.240.0/20", + "188.114.96.0/20", + "197.234.240.0/22", + "198.41.128.0/17", + "162.158.0.0/15", + "104.16.0.0/13", + "104.24.0.0/14", + "172.64.0.0/13", + "131.0.72.0/22", + // IPv6 + "2400:cb00::/32", + "2606:4700::/32", + "2803:f800::/32", + "2405:b500::/32", + "2405:8100::/32", + "2a06:98c0::/29", + "2c0f:f248::/32", +]; + +/** + * Translate the TRUST_PROXY setting into a value for Express's + * "trust proxy". Two forms are accepted: + * + * - a plain integer ("1", "2"): the legacy fixed hop count. Fragile: if the + * proxy chain gains or loses a hop (e.g. Cloudflare changes how it builds + * X-Forwarded-For), request.ip silently becomes a proxy address and the + * rate limiter starts keying every visitor on a handful of shared IPs. + * - a comma-separated list of subnets: named subnets Express understands + * ("loopback", "linklocal", "uniquelocal"), literal IPs/CIDRs, and the + * keyword "cloudflare" which expands to CLOUDFLARE_IP_RANGES. Express then + * skips every trusted address in X-Forwarded-For regardless of how many + * entries the proxies add, so request.ip stays the real visitor. + */ +export function resolveTrustProxy(value: string): number | string[] { + const trimmed = value.trim(); + if (/^-?\d+$/.test(trimmed)) { + return Number(trimmed); + } + const subnets: string[] = []; + for (const token of trimmed.split(",")) { + const subnet = token.trim(); + if (!subnet) continue; + if (subnet.toLowerCase() === "cloudflare") { + subnets.push(...CLOUDFLARE_IP_RANGES); + } else { + subnets.push(subnet); + } + } + return subnets; +} + +const cloudflareBlockList = new BlockList(); +for (const range of CLOUDFLARE_IP_RANGES) { + const [address, prefix] = range.split("/"); + cloudflareBlockList.addSubnet( + address, + Number(prefix), + isIP(address) === 6 ? "ipv6" : "ipv4" + ); +} + +/** + * Check whether an IP belongs to Cloudflare's published ranges. Used to + * detect that client-IP resolution stopped short at a Cloudflare edge + * address (i.e. X-Forwarded-For no longer contains the visitor). + */ +export function isCloudflareIP(ip: string): boolean { + // Express may report IPv4 clients as IPv4-mapped IPv6 (::ffff:1.2.3.4); + // compare them against the IPv4 ranges. + const normalized = ip.replace(/^::ffff:(?=\d+\.\d+\.\d+\.\d+$)/i, ""); + const family = isIP(normalized); + if (family === 0) return false; + return cloudflareBlockList.check(normalized, family === 6 ? "ipv6" : "ipv4"); +} diff --git a/test/config.test.js b/test/config.test.js index 7de0f8d..df87d79 100644 --- a/test/config.test.js +++ b/test/config.test.js @@ -38,7 +38,7 @@ describe("Config environment variable parsing", function () { MAX_REPO_SIZE: 60000, ENABLE_DOWNLOAD: true, RATE_LIMIT: 350, - TRUST_PROXY: 1, + TRUST_PROXY: "loopback,uniquelocal,cloudflare", SESSION_SECRET: "SESSION_SECRET", CLIENT_ID: "CLIENT_ID", APP_HOSTNAME: "anonymous.4open.science", @@ -80,14 +80,14 @@ describe("Config environment variable parsing", function () { }); it("handles zero correctly", function () { - const config = parseConfigFromEnv(defaults, { TRUST_PROXY: "0" }); - expect(config.TRUST_PROXY).to.equal(0); - expect(config.TRUST_PROXY).to.be.a("number"); + const config = parseConfigFromEnv(defaults, { RATE_LIMIT: "0" }); + expect(config.RATE_LIMIT).to.equal(0); + expect(config.RATE_LIMIT).to.be.a("number"); }); it("handles negative numbers", function () { - const config = parseConfigFromEnv(defaults, { TRUST_PROXY: "-1" }); - expect(config.TRUST_PROXY).to.equal(-1); + const config = parseConfigFromEnv(defaults, { RATE_LIMIT: "-1" }); + expect(config.RATE_LIMIT).to.equal(-1); }); it("correctly compares parsed numbers (no string comparison bug)", function () { @@ -163,6 +163,13 @@ describe("Config environment variable parsing", function () { const config = parseConfigFromEnv(defaults, { STORAGE: "s3" }); expect(config.STORAGE).to.equal("s3"); }); + + it("keeps TRUST_PROXY as a string (legacy hop counts included)", function () { + // resolveTrustProxy() interprets the string later; a numeric env value + // like "2" must survive as-is. + const config = parseConfigFromEnv(defaults, { TRUST_PROXY: "2" }); + expect(config.TRUST_PROXY).to.equal("2"); + }); }); // --------------------------------------------------------------- diff --git a/test/trust-proxy.test.js b/test/trust-proxy.test.js new file mode 100644 index 0000000..423ac64 --- /dev/null +++ b/test/trust-proxy.test.js @@ -0,0 +1,142 @@ +const { expect } = require("chai"); +require("ts-node/register/transpile-only"); + +const { + resolveTrustProxy, + isCloudflareIP, + CLOUDFLARE_IP_RANGES, +} = require("../src/server/trustProxy"); + +describe("trustProxy", function () { + describe("resolveTrustProxy", function () { + it("parses a plain integer as a legacy hop count", function () { + expect(resolveTrustProxy("1")).to.equal(1); + expect(resolveTrustProxy("2")).to.equal(2); + expect(resolveTrustProxy(" 3 ")).to.equal(3); + }); + + it("parses zero as a number", function () { + expect(resolveTrustProxy("0")).to.equal(0); + }); + + it("expands the cloudflare keyword to the published ranges", function () { + const result = resolveTrustProxy("cloudflare"); + expect(result).to.be.an("array"); + expect(result).to.have.members([...CLOUDFLARE_IP_RANGES]); + }); + + it("keeps named subnets and mixes them with cloudflare ranges", function () { + const result = resolveTrustProxy("loopback,uniquelocal,cloudflare"); + expect(result).to.include("loopback"); + expect(result).to.include("uniquelocal"); + expect(result).to.include("173.245.48.0/20"); + expect(result).to.include("2400:cb00::/32"); + expect(result).to.have.length(2 + CLOUDFLARE_IP_RANGES.length); + }); + + it("accepts literal CIDRs and trims whitespace and empty tokens", function () { + const result = resolveTrustProxy(" 10.0.0.0/8 , , 192.168.1.1 "); + expect(result).to.deep.equal(["10.0.0.0/8", "192.168.1.1"]); + }); + + it("is case-insensitive for the cloudflare keyword", function () { + const result = resolveTrustProxy("Cloudflare"); + expect(result).to.have.length(CLOUDFLARE_IP_RANGES.length); + }); + }); + + describe("isCloudflareIP", function () { + it("matches IPv4 addresses inside Cloudflare ranges", function () { + expect(isCloudflareIP("104.16.132.229")).to.be.true; // 104.16.0.0/13 + expect(isCloudflareIP("172.64.0.1")).to.be.true; // 172.64.0.0/13 + expect(isCloudflareIP("173.245.48.10")).to.be.true; // 173.245.48.0/20 + }); + + it("matches IPv6 addresses inside Cloudflare ranges", function () { + expect(isCloudflareIP("2400:cb00::1")).to.be.true; + expect(isCloudflareIP("2606:4700:4700::1111")).to.be.true; + }); + + it("matches IPv4-mapped IPv6 representations", function () { + expect(isCloudflareIP("::ffff:104.16.0.1")).to.be.true; + }); + + it("rejects addresses outside Cloudflare ranges", function () { + expect(isCloudflareIP("8.8.8.8")).to.be.false; + expect(isCloudflareIP("127.0.0.1")).to.be.false; + expect(isCloudflareIP("192.168.1.1")).to.be.false; + expect(isCloudflareIP("2001:4860:4860::8888")).to.be.false; + expect(isCloudflareIP("::ffff:8.8.8.8")).to.be.false; + }); + + it("rejects garbage input without throwing", function () { + expect(isCloudflareIP("")).to.be.false; + expect(isCloudflareIP("not-an-ip")).to.be.false; + expect(isCloudflareIP("104.16.0")).to.be.false; + }); + }); + + describe("integration with Express trust proxy resolution", function () { + const express = require("express"); + + function requestIp(trustProxy, headers) { + const app = express(); + app.set("trust proxy", trustProxy); + let captured = null; + app.get("/", (req, res) => { + captured = req.ip; + res.end(); + }); + return new Promise((resolve, reject) => { + const server = app.listen(0, "127.0.0.1", () => { + const { port } = server.address(); + const http = require("http"); + const req = http.get( + { host: "127.0.0.1", port, path: "/", headers }, + (res) => { + res.resume(); + res.on("end", () => { + server.close(); + resolve(captured); + }); + } + ); + req.on("error", (err) => { + server.close(); + reject(err); + }); + }); + }); + } + + it("resolves the visitor IP through Cloudflare hops regardless of count", async function () { + const trust = resolveTrustProxy("loopback,uniquelocal,cloudflare"); + // Simulate: visitor -> Cloudflare edge -> local nginx -> app, where + // Cloudflare appended an extra internal hop to X-Forwarded-For. + const ip = await requestIp(trust, { + "x-forwarded-for": "203.0.113.7, 104.16.0.9, 172.64.0.3", + }); + expect(ip).to.equal("203.0.113.7"); + }); + + it("does not honour X-Forwarded-For from an untrusted public peer entry", async function () { + const trust = resolveTrustProxy("loopback,uniquelocal,cloudflare"); + // The last entry is a non-Cloudflare public address: resolution stops + // there, so a forged visitor entry further left is ignored. + const ip = await requestIp(trust, { + "x-forwarded-for": "1.2.3.4, 8.8.8.8", + }); + expect(ip).to.equal("8.8.8.8"); + }); + + it("legacy hop count still works", async function () { + const ip = await requestIp(resolveTrustProxy("1"), { + "x-forwarded-for": "203.0.113.7, 104.16.0.9", + }); + // hop count 1 trusts only the direct peer, so req.ip is the last + // X-Forwarded-For entry — the fragile behavior this change moves + // away from. + expect(ip).to.equal("104.16.0.9"); + }); + }); +});