Merge branch 'fix/client-ip-attribution' into release/auth-campaign

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 };

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;
 }

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');
+  }
+});