mirror of
https://github.com/tdurieux/anonymous_github.git
synced 2026-07-23 21:40:59 +02:00
fix: rate-limit visitors by real IP regardless of Cloudflare proxy hops (#750)
Since the night of 2026-07-16 production keyed the rate limiter on Cloudflare edge IPs instead of visitor IPs: every visitor routed through the same edge shared one 175-req/15min bucket, causing widespread 429s (confirmed by probing: sequential requests alternated 200/429 across fresh and exhausted buckets). Root cause: 'trust proxy' used a fixed hop count (TRUST_PROXY=1), which silently breaks whenever the proxy chain gains or loses an X-Forwarded-For entry (e.g. a Cloudflare-side change). - TRUST_PROXY now accepts a comma-separated subnet list; the new default 'loopback,uniquelocal,cloudflare' expands Cloudflare's published IP ranges so Express skips trusted proxies no matter how many entries they add. Plain numbers keep the legacy hop-count behavior. - If resolution still stops at a Cloudflare address (visitor missing from X-Forwarded-For entirely), the limiter key falls back to cf-connecting-ip — safe because request.ip can only be a Cloudflare address when the whole chain to it is trusted. - CIDR matching uses Node's built-in net.BlockList (no new dependency). Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
+14
-2
@@ -34,7 +34,15 @@ interface Config {
|
|||||||
S3_ENDPOINT: string | null;
|
S3_ENDPOINT: string | null;
|
||||||
S3_REGION: string | null;
|
S3_REGION: string | null;
|
||||||
STORAGE: "filesystem" | "s3";
|
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;
|
RATE_LIMIT: number;
|
||||||
}
|
}
|
||||||
const config: Config = {
|
const config: Config = {
|
||||||
@@ -55,7 +63,11 @@ const config: Config = {
|
|||||||
AUTH_CALLBACK: "http://localhost:5000/github/auth",
|
AUTH_CALLBACK: "http://localhost:5000/github/auth",
|
||||||
ANONYMIZATION_MASK: "XXXX",
|
ANONYMIZATION_MASK: "XXXX",
|
||||||
PORT: 5000,
|
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,
|
RATE_LIMIT: 350,
|
||||||
APP_HOSTNAME: "anonymous.4open.science",
|
APP_HOSTNAME: "anonymous.4open.science",
|
||||||
DB_USERNAME: "admin",
|
DB_USERNAME: "admin",
|
||||||
|
|||||||
+30
-11
@@ -29,6 +29,7 @@ import {
|
|||||||
import DailyStatsModel from "../core/model/dailyStats/dailyStats.model";
|
import DailyStatsModel from "../core/model/dailyStats/dailyStats.model";
|
||||||
import { getUser } from "./routes/route-utils";
|
import { getUser } from "./routes/route-utils";
|
||||||
import config from "../config";
|
import config from "../config";
|
||||||
|
import { resolveTrustProxy, isCloudflareIP } from "./trustProxy";
|
||||||
import { createLogger, serializeError } from "../core/logger";
|
import { createLogger, serializeError } from "../core/logger";
|
||||||
|
|
||||||
const logger = createLogger("server");
|
const logger = createLogger("server");
|
||||||
@@ -109,10 +110,14 @@ export default async function start() {
|
|||||||
})
|
})
|
||||||
);
|
);
|
||||||
app.set("etag", "strong");
|
app.set("etag", "strong");
|
||||||
// Trust exactly TRUST_PROXY proxy hops so Express derives request.ip from
|
// Trust the proxies declared in TRUST_PROXY (subnet list or legacy hop
|
||||||
// the right X-Forwarded-For entry. This is what makes request.ip
|
// count) so Express derives request.ip from the right X-Forwarded-For
|
||||||
// trustworthy for rate limiting instead of a client-spoofable header.
|
// entry. With the subnet form, Express skips every trusted address no
|
||||||
app.set("trust proxy", config.TRUST_PROXY);
|
// 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
|
// handle session and connection
|
||||||
app.use(initSession());
|
app.use(initSession());
|
||||||
@@ -139,16 +144,30 @@ export default async function start() {
|
|||||||
_response: express.Response
|
_response: express.Response
|
||||||
): string {
|
): string {
|
||||||
// Use request.ip, which Express resolves from X-Forwarded-For honouring
|
// Use request.ip, which Express resolves from X-Forwarded-For honouring
|
||||||
// the configured "trust proxy" hop count. Do NOT key off the
|
// the configured "trust proxy" setting. Do NOT key off the
|
||||||
// cf-connecting-ip header directly: when the server isn't actually behind
|
// cf-connecting-ip header unconditionally: when the server isn't actually
|
||||||
// Cloudflare a client can set that header to an arbitrary value per
|
// behind Cloudflare a client can set that header to an arbitrary value
|
||||||
// request and trivially bypass the rate limiter (CWE-290).
|
// per request and trivially bypass the rate limiter (CWE-290).
|
||||||
if (!request.ip && request.socket.remoteAddress) {
|
let ip = request.ip;
|
||||||
|
if (!ip && request.socket.remoteAddress) {
|
||||||
logger.warn("request.ip is missing");
|
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
|
// remove port number from IPv4 addresses
|
||||||
return (request.ip || "").replace(/:\d+[^:]*$/, "");
|
return (ip || "").replace(/:\d+[^:]*$/, "");
|
||||||
}
|
}
|
||||||
|
|
||||||
const rate = rateLimit({
|
const rate = rateLimit({
|
||||||
|
|||||||
@@ -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");
|
||||||
|
}
|
||||||
+13
-6
@@ -38,7 +38,7 @@ describe("Config environment variable parsing", function () {
|
|||||||
MAX_REPO_SIZE: 60000,
|
MAX_REPO_SIZE: 60000,
|
||||||
ENABLE_DOWNLOAD: true,
|
ENABLE_DOWNLOAD: true,
|
||||||
RATE_LIMIT: 350,
|
RATE_LIMIT: 350,
|
||||||
TRUST_PROXY: 1,
|
TRUST_PROXY: "loopback,uniquelocal,cloudflare",
|
||||||
SESSION_SECRET: "SESSION_SECRET",
|
SESSION_SECRET: "SESSION_SECRET",
|
||||||
CLIENT_ID: "CLIENT_ID",
|
CLIENT_ID: "CLIENT_ID",
|
||||||
APP_HOSTNAME: "anonymous.4open.science",
|
APP_HOSTNAME: "anonymous.4open.science",
|
||||||
@@ -80,14 +80,14 @@ describe("Config environment variable parsing", function () {
|
|||||||
});
|
});
|
||||||
|
|
||||||
it("handles zero correctly", function () {
|
it("handles zero correctly", function () {
|
||||||
const config = parseConfigFromEnv(defaults, { TRUST_PROXY: "0" });
|
const config = parseConfigFromEnv(defaults, { RATE_LIMIT: "0" });
|
||||||
expect(config.TRUST_PROXY).to.equal(0);
|
expect(config.RATE_LIMIT).to.equal(0);
|
||||||
expect(config.TRUST_PROXY).to.be.a("number");
|
expect(config.RATE_LIMIT).to.be.a("number");
|
||||||
});
|
});
|
||||||
|
|
||||||
it("handles negative numbers", function () {
|
it("handles negative numbers", function () {
|
||||||
const config = parseConfigFromEnv(defaults, { TRUST_PROXY: "-1" });
|
const config = parseConfigFromEnv(defaults, { RATE_LIMIT: "-1" });
|
||||||
expect(config.TRUST_PROXY).to.equal(-1);
|
expect(config.RATE_LIMIT).to.equal(-1);
|
||||||
});
|
});
|
||||||
|
|
||||||
it("correctly compares parsed numbers (no string comparison bug)", function () {
|
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" });
|
const config = parseConfigFromEnv(defaults, { STORAGE: "s3" });
|
||||||
expect(config.STORAGE).to.equal("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");
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
// ---------------------------------------------------------------
|
// ---------------------------------------------------------------
|
||||||
|
|||||||
@@ -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");
|
||||||
|
});
|
||||||
|
});
|
||||||
|
});
|
||||||
Reference in New Issue
Block a user