From 4b13dadb4df11c53f117946a68a3bb0ef773c726 Mon Sep 17 00:00:00 2001 From: ScreenTinker Date: Sun, 26 Jul 2026 10:23:04 -0500 Subject: [PATCH] fix(logging): gate CF-Connecting-IP on a Cloudflare peer, not any trusted proxy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit getClientIp() decides the value every per-IP control keys on — the auth/pairing rate limiters, lib/pair-lockout, and activity_log.ip_address — so a caller must never be able to choose it. It believed CF-Connecting-IP whenever the immediate peer was in the `trust proxy` list, which includes loopback/linklocal/uniquelocal. Those entries are correct for X-Forwarded-For: a proxy APPENDS to that header and Express walks the chain right-to-left, so a client-supplied value cannot become the resolved address. CF-Connecting-IP has no chain — a local reverse proxy passes through whatever single value the client sent — so treating a loopback peer as evidence the request came through Cloudflare means trusting the client. Gate it on the published Cloudflare ranges alone. This is also the portable behaviour: most self-hosted installs are not behind Cloudflare, and for them the header is now simply ignored, with attribution falling back to req.ip under whatever `trust proxy` the operator configured. Installs that do front with Cloudflare are unaffected — their peer really is a CF edge. Documented the distinction at config/cloudflareIps.js so the two lists are not conflated again. No response shape or DB change; no client impact. Co-Authored-By: Claude Opus 5 (1M context) --- server/config/cloudflareIps.js | 11 ++-- server/services/activity.js | 38 +++++++------ server/test/client-ip-attribution.test.js | 66 +++++++++++++++++++++++ 3 files changed, 95 insertions(+), 20 deletions(-) create mode 100644 server/test/client-ip-attribution.test.js diff --git a/server/config/cloudflareIps.js b/server/config/cloudflareIps.js index ad12c02..d03a3bd 100644 --- a/server/config/cloudflareIps.js +++ b/server/config/cloudflareIps.js @@ -31,9 +31,14 @@ const cloudflareIpv6 = [ const cloudflareIps = [...cloudflareIpv4, ...cloudflareIpv6]; -// What Express's trust-proxy and our CF-Connecting-IP gate both honor. -// 'loopback', 'linklocal', 'uniquelocal' keep local dev and any LAN reverse -// proxy working without further config. +// What Express's `trust proxy` honors. 'loopback', 'linklocal', 'uniquelocal' keep local +// dev and any LAN reverse proxy working without further config, and they are SAFE here +// because a proxy APPENDS to X-Forwarded-For and Express walks that chain right-to-left, +// so a client-supplied value can never end up as the resolved address. +// +// NOTE: this list is deliberately NOT the gate for CF-Connecting-IP. That header carries +// no chain — a local proxy passes through whatever single value the client sent — so +// services/activity.js gates it on `cloudflareIps` alone. See the comment there. const trustedProxies = ['loopback', 'linklocal', 'uniquelocal', ...cloudflareIps]; module.exports = { cloudflareIpv4, cloudflareIpv6, cloudflareIps, trustedProxies }; diff --git a/server/services/activity.js b/server/services/activity.js index f9d94fc..5ceaddf 100644 --- a/server/services/activity.js +++ b/server/services/activity.js @@ -1,28 +1,32 @@ const { db } = require('../db/database'); const proxyaddr = require('proxy-addr'); -const { trustedProxies } = require('../config/cloudflareIps'); +const { cloudflareIps } = require('../config/cloudflareIps'); -// Gate function: returns true when an immediate TCP peer is one we trust -// to populate forwarding headers (Cloudflare edges, loopback, link-local, -// unique-local). Mirrors what `app.set('trust proxy', trustedProxies)` does -// for X-Forwarded-For so that CF-Connecting-IP is held to the same standard. -const isTrustedPeer = proxyaddr.compile(trustedProxies); +// Peer gate for CF-Connecting-IP: ONLY Cloudflare's published edge ranges, deliberately +// NOT the loopback/linklocal/uniquelocal entries that `trust proxy` also carries. +// +// Those entries are right for X-Forwarded-For, because a local reverse proxy APPENDS to +// XFF and Express then walks the chain right-to-left, so a client-supplied value cannot +// end up as the resolved address. CF-Connecting-IP has no chain: nginx passes through +// whatever single value the client sent. Treating a loopback peer as evidence that the +// request came through Cloudflare therefore means trusting the client. +// +// This is also the portable behaviour. Most self-hosted installs do NOT sit behind +// Cloudflare; for them this header is now simply ignored and attribution comes from +// req.ip via whatever `trust proxy` the operator configured. An install that DOES front +// with Cloudflare is unaffected: its peer really is a CF edge. +const isCloudflarePeer = proxyaddr.compile(cloudflareIps); -// Resolve the real client IP for logging. -// -// Cloudflare always sets `CF-Connecting-IP` to the original client address -// when it proxies a request. We prefer that header — but only when the -// connection's immediate peer is a trusted CF/loopback address; otherwise -// any random visitor could spoof the header by hitting the origin directly. -// -// Falls back to req.ip (which Express resolves via the trust-proxy table) -// so local dev and any non-CF deployment keep working unchanged. +// Resolve the real client IP. This value keys every per-IP control (the auth/pairing rate +// limiters, lib/pair-lockout) and the ip_address column in activity_log, so a caller must +// never be able to choose it. function getClientIp(req) { if (!req) return null; const cf = req.headers && req.headers['cf-connecting-ip']; - if (typeof cf === 'string' && cf.length > 0) { + if (typeof cf === 'string' && cf.trim().length > 0) { const peer = req.socket && req.socket.remoteAddress; - if (peer && isTrustedPeer(peer, 0)) return cf; + // Believe it only when the request demonstrably arrived through Cloudflare. + if (peer && isCloudflarePeer(peer, 0)) return cf.trim(); } return req.ip || null; } diff --git a/server/test/client-ip-attribution.test.js b/server/test/client-ip-attribution.test.js new file mode 100644 index 0000000..75326cc --- /dev/null +++ b/server/test/client-ip-attribution.test.js @@ -0,0 +1,66 @@ +'use strict'; + +// getClientIp() decides the value every per-IP control keys on: the login/register/pairing +// rate limiters, lib/pair-lockout, and the ip_address column in activity_log. If a caller +// can choose that value, all of those controls are advisory. +// +// The invariant: `CF-Connecting-IP` is only believed when the request genuinely arrived +// through Cloudflare — i.e. the immediate TCP peer is a Cloudflare edge address. A local +// reverse proxy (loopback / LAN / unique-local) is NOT evidence of that: nginx forwards +// whatever header the client sent, so trusting the header on a loopback peer means +// trusting the client. +// +// This matters most for SELF-HOSTED installs, which are the majority and mostly do NOT +// use Cloudflare: for them the header must simply be ignored, and attribution falls back +// to req.ip via each operator's own `trust proxy` setting. + +const { test } = require('node:test'); +const assert = require('node:assert/strict'); +const { getClientIp } = require('../services/activity'); +const { cloudflareIpv4 } = require('../config/cloudflareIps'); + +// A real address inside Cloudflare's published v4 ranges (173.245.48.0/20). +const CF_EDGE = '173.245.48.1'; +const NOT_CF = '203.0.113.10'; // TEST-NET-3 — never a Cloudflare edge + +function reqFrom(peer, headers = {}, reqIp = undefined) { + return { headers, socket: { remoteAddress: peer }, ip: reqIp === undefined ? peer : reqIp }; +} + +test('a Cloudflare edge peer IS believed (hosted deployments keep working)', () => { + assert.ok(cloudflareIpv4.includes('173.245.48.0/20'), 'fixture address is in a published CF range'); + const ip = getClientIp(reqFrom(CF_EDGE, { 'cf-connecting-ip': '198.51.100.7' }, CF_EDGE)); + assert.equal(ip, '198.51.100.7', 'through Cloudflare, the header is the real client'); +}); + +test('a LOOPBACK peer is not evidence of Cloudflare — the header is ignored', () => { + // This is the self-hosted-behind-nginx shape: nginx is the peer, and it forwards any + // CF-Connecting-IP the client invented. + const ip = getClientIp(reqFrom('127.0.0.1', { 'cf-connecting-ip': '203.0.113.77' }, '203.0.113.200')); + assert.notEqual(ip, '203.0.113.77', 'a forged CF-Connecting-IP must not become the client IP'); + assert.equal(ip, '203.0.113.200', 'attribution falls back to req.ip (operator trust-proxy config)'); +}); + +test('a LAN / unique-local peer is not evidence of Cloudflare either', () => { + for (const peer of ['10.0.0.5', '192.168.1.10', 'fd00::1']) { + const ip = getClientIp(reqFrom(peer, { 'cf-connecting-ip': '203.0.113.77' }, '198.51.100.99')); + assert.notEqual(ip, '203.0.113.77', `forged header honoured for peer ${peer}`); + } +}); + +test('a direct, non-proxied caller cannot self-attribute', () => { + const ip = getClientIp(reqFrom(NOT_CF, { 'cf-connecting-ip': '1.2.3.4' }, NOT_CF)); + assert.equal(ip, NOT_CF, 'the header from an untrusted peer is ignored'); +}); + +test('no CF header at all -> req.ip, unchanged for every non-Cloudflare install', () => { + assert.equal(getClientIp(reqFrom('203.0.113.5', {}, '203.0.113.5')), '203.0.113.5'); +}); + +test('malformed / empty header values fall through rather than throwing', () => { + for (const v of ['', ' ', 'not-an-ip', undefined]) { + const r = reqFrom('127.0.0.1', v === undefined ? {} : { 'cf-connecting-ip': v }, '198.51.100.1'); + assert.doesNotThrow(() => getClientIp(r)); + assert.equal(getClientIp(r), '198.51.100.1'); + } +});