diff --git a/server/lib/limiter-telemetry.js b/server/lib/limiter-telemetry.js new file mode 100644 index 0000000..97a39e1 --- /dev/null +++ b/server/lib/limiter-telemetry.js @@ -0,0 +1,80 @@ +'use strict'; + +// Why this exists: the auth limiters run as app.use middleware BEFORE the handler that writes +// activity_log, so a 429 leaves no trace anywhere. The limit censors the evidence of itself — +// four production IPs sit at exactly 10 logins/min and there is no way to tell whether that is +// one attacker or a NATed site whose staff are colliding on a shared egress IP. +// +// This records rejections so the question becomes measurable. The discriminating signal is NOT +// the rejection count, it is how many DISTINCT accounts a single IP is rejected for: +// +// 1 identifier, many rejections -> someone hammering one account (the limiter is working) +// many identifiers, few each -> a shared egress IP; real users are being denied +// +// Identifiers are salted-hashed and only ever counted, never stored or logged in the clear — +// this is a diagnostic, not an audit trail, and it must not become a place where credentials +// or a roster of a customer's email addresses accumulate. + +const crypto = require('crypto'); + +const MAX_KEYS = 2000; // distinct endpoint|ip pairs held +const MAX_IDS_PER_KEY = 64; // enough to tell "one" from "many"; caps memory per key +const IDLE_MS = 60 * 60 * 1000; // drop a key after an hour of quiet + +// Per-process salt: makes the digests useless outside this process lifetime, so nothing +// persisted or logged can be walked back to an address. +const SALT = crypto.randomBytes(16); +const digest = (s) => crypto.createHash('sha256').update(SALT).update(String(s).toLowerCase()).digest('hex').slice(0, 16); + +const state = new Map(); + +function prune(now) { + for (const [k, v] of state) if (now - v.lastSeen > IDLE_MS) state.delete(k); + if (state.size <= MAX_KEYS) return; + // Still oversized: evict the least recently seen. + const byAge = [...state.entries()].sort((a, b) => a[1].lastSeen - b[1].lastSeen); + for (let i = 0; i < byAge.length - MAX_KEYS; i++) state.delete(byAge[i][0]); +} + +// Returns the running tally for this endpoint+ip, so the caller can log it. +function recordRejection({ endpoint, ip, identifier }, now = Date.now()) { + const key = `${endpoint}|${ip}`; + let e = state.get(key); + if (!e) { e = { rejections: 0, ids: new Set(), idsTruncated: false, firstSeen: now, lastSeen: now }; state.set(key, e); } + e.rejections++; + e.lastSeen = now; + if (identifier) { + if (e.ids.size < MAX_IDS_PER_KEY) e.ids.add(digest(identifier)); + else e.idsTruncated = true; + } + if (state.size > MAX_KEYS) prune(now); + return { + endpoint, ip, + rejections: e.rejections, + distinctIdentifiers: e.ids.size, + identifiersTruncated: e.idsTruncated, + windowMs: now - e.firstSeen, + }; +} + +// Read-only view for a debug endpoint or a test. No digests are exposed — only counts. +function snapshot() { + return [...state.entries()].map(([key, e]) => { + const i = key.lastIndexOf('|'); + return { + endpoint: key.slice(0, i), + ip: key.slice(i + 1), + rejections: e.rejections, + distinctIdentifiers: e.ids.size, + identifiersTruncated: e.idsTruncated, + firstSeen: e.firstSeen, + lastSeen: e.lastSeen, + // The whole point: many identifiers from one IP reads as a shared egress, not an attack. + likelySharedEgress: e.ids.size >= 3, + }; + }).sort((a, b) => b.rejections - a.rejections); +} + +function reset() { state.clear(); } + +module.exports = { recordRejection, snapshot, reset, MAX_IDS_PER_KEY, MAX_KEYS }; diff --git a/server/routes/admin.js b/server/routes/admin.js index d8b03ae..dabf9e6 100644 --- a/server/routes/admin.js +++ b/server/routes/admin.js @@ -424,4 +424,21 @@ router.post('/trigger-update', requirePlatformAdmin, async (req, res) => { }); }); +// QA-SNAT diagnostic. Auth rate-limit rejections are invisible everywhere else: the limiter is +// app.use middleware that returns 429 before the handler that would write activity_log, so the +// limit suppresses the record of itself. This exposes the in-memory tally so "is that IP one +// attacker or a NATed office?" can be answered from data instead of argued from a hunch. +// +// distinct_accounts is the signal, not rejections. Values are counts only — the identifiers are +// salted-hashed inside the telemetry module and never leave it, so this cannot become a roster +// of a customer's email addresses. Platform-admin only, and in-memory (a restart clears it). +router.get('/limiter-rejections', requirePlatformAdmin, (req, res) => { + const rows = require('../lib/limiter-telemetry').snapshot(); + res.json({ + rows, + shared_egress_suspects: rows.filter(r => r.likelySharedEgress).length, + note: 'In-memory since last restart. distinct_accounts >= 3 from one IP suggests a shared egress rather than a single attacker.', + }); +}); + module.exports = router; diff --git a/server/server.js b/server/server.js index a050fa4..be9d87a 100644 --- a/server/server.js +++ b/server/server.js @@ -327,6 +327,9 @@ app.use('/socket.io-client', express.static( )); // Simple rate limiter for auth endpoints +// Required here rather than relying on the log-coalescer const further down: that one is only +// safe because the callback runs at request time, which is a subtle thing to depend on. +const limiterTelemetry = require('./lib/limiter-telemetry'); const rateLimits = new Map(); function rateLimit(windowMs, maxRequests) { return (req, res, next) => { @@ -341,6 +344,23 @@ function rateLimit(windowMs, maxRequests) { let hits = rateLimits.get(key) || []; hits = hits.filter(t => t > windowStart); if (hits.length >= maxRequests) { + // QA-SNAT: a 429 returns before any handler runs, so nothing else in the system ever + // records that it happened — the limit hides its own evidence. Count it here. The + // number that matters is distinct identifiers per IP: one means the limiter is doing + // its job, several means a shared egress IP is denying real users. Identifiers are + // salted-hashed inside the telemetry module and only ever counted. Response unchanged. + try { + const endpoint = (req.originalUrl || req.url || req.path).split('?')[0]; + const ip = getClientIp(req); + const ident = req.body && (req.body.email || req.body.username); + const t = limiterTelemetry.recordRejection({ endpoint, ip, identifier: ident }); + logCoalescer.record( + `limit-reject:${endpoint}:${ip}`, + `[limit] 429 ${endpoint} ip=${ip} rejections=${t.rejections} distinct_accounts=${t.distinctIdentifiers}` + + (t.distinctIdentifiers >= 3 ? ' (looks like a SHARED egress, not one attacker)' : ''), + { warn: t.distinctIdentifiers >= 3 }, + ); + } catch (_) { /* telemetry must never break the limiter */ } return res.status(429).json({ error: 'Too many requests, try again later' }); } hits.push(now); diff --git a/server/test/limiter-rejection-recorded.test.js b/server/test/limiter-rejection-recorded.test.js new file mode 100644 index 0000000..b0def3b --- /dev/null +++ b/server/test/limiter-rejection-recorded.test.js @@ -0,0 +1,106 @@ +'use strict'; + +// The companion to limiter-telemetry.test.js: that one tests the counter, this one proves the +// counter is actually WIRED to a real 429 and that adding it changed nothing about the limiter. +// A diagnostic that alters the thing it measures is worse than no diagnostic. + +const { test, before, after } = require('node:test'); +const assert = require('node:assert/strict'); +const { spawn } = require('node:child_process'); +const path = require('node:path'); +const os = require('node:os'); +const fs = require('node:fs'); +const crypto = require('node:crypto'); + +const { freePort } = require('./helpers/free-port'); +let PORT, BASE, proc, adminToken; +const DATA_DIR = path.join(os.tmpdir(), 'st-limrej-' + crypto.randomBytes(4).toString('hex')); +const LOG = path.join(os.tmpdir(), 'st-limrej-' + crypto.randomBytes(4).toString('hex') + '.log'); + +const jfetch = async (p, opts = {}) => { + const res = await fetch(BASE + p, opts); + let body = null; try { body = await res.json(); } catch { /* */ } + return { status: res.status, body }; +}; +const login = (email, ip) => jfetch('/api/auth/login', { + method: 'POST', + headers: { 'Content-Type': 'application/json', 'X-Forwarded-For': ip }, + body: JSON.stringify({ email, password: 'definitely-wrong-password' }), +}); + +before(async () => { + PORT = await freePort(); + BASE = `http://127.0.0.1:${PORT}`; + const logFd = fs.openSync(LOG, 'w'); + proc = spawn('node', ['server.js'], { + cwd: path.join(__dirname, '..'), + env: { ...process.env, DATA_DIR, SELF_HOSTED: 'true', PORT: String(PORT), NODE_ENV: 'test' }, + stdio: ['ignore', logFd, logFd], + }); + let up = false; + for (let i = 0; i < 80; i++) { + try { const r = await fetch(BASE + '/api/status'); if (r.ok) { up = true; break; } } catch { /* */ } + await new Promise(r => setTimeout(r, 250)); + } + if (!up) throw new Error('server did not boot:\n' + fs.readFileSync(LOG, 'utf8').slice(-2000)); + + // First account on a fresh self-hosted instance is platform_admin — the only role that may + // read the diagnostic. + const reg = await jfetch('/api/auth/register', { + method: 'POST', headers: { 'Content-Type': 'application/json', 'X-Forwarded-For': '198.51.10.1' }, + body: JSON.stringify({ email: 'admin' + crypto.randomBytes(4).toString('hex') + '@x.local', password: 'Passw0rd123' }), + }); + adminToken = reg.body.token; +}); +after(() => { try { proc.kill('SIGKILL'); } catch { /* */ } }); + +const snapshot = () => jfetch('/api/admin/limiter-rejections', { headers: { Authorization: 'Bearer ' + adminToken } }); + +test('the limiter still behaves exactly as before: 10 through, then 429', async () => { + const IP = '198.51.99.10'; + const codes = []; + for (let i = 0; i < 12; i++) codes.push((await login(`u${i}@corp.test`, IP)).status); + assert.equal(codes.filter(c => c === 429).length, 2, 'the 11th and 12th are rejected'); + assert.ok(!codes.slice(0, 10).includes(429), 'the first ten are not'); + const body = (await login('u0@corp.test', IP)); + assert.equal(body.status, 429); + assert.deepEqual(body.body, { error: 'Too many requests, try again later' }, + 'response shape unchanged — the diagnostic is invisible to clients'); +}); + +test('the rejection is recorded, with the distinct-account signal that answers QA-SNAT', async () => { + const snap = await snapshot(); + assert.equal(snap.status, 200); + const row = snap.body.rows.find(r => r.ip === '198.51.99.10'); + assert.ok(row, 'the 429 left a trace — previously it left none at all'); + assert.equal(row.endpoint, '/api/auth/login'); + assert.ok(row.rejections >= 2); + assert.ok(row.distinctIdentifiers >= 3, 'several accounts from one IP'); + assert.equal(row.likelySharedEgress, true, 'which reads as a NATed site, not one attacker'); + assert.ok(snap.body.shared_egress_suspects >= 1); +}); + +test('one account hammered is NOT flagged as a shared egress', async () => { + const IP = '198.51.99.11'; + for (let i = 0; i < 12; i++) await login('one.victim@corp.test', IP); + const row = (await snapshot()).body.rows.find(r => r.ip === IP); + assert.equal(row.distinctIdentifiers, 1); + assert.equal(row.likelySharedEgress, false, 'the limiter is doing its job here — do not widen it'); +}); + +test('the diagnostic never leaks the addresses it counted', async () => { + const dump = JSON.stringify((await snapshot()).body); + assert.doesNotMatch(dump, /one\.victim/); + assert.doesNotMatch(dump, /corp\.test/); +}); + +test('it is platform-admin gated, not readable by an ordinary tenant', async () => { + const reg = await jfetch('/api/auth/register', { + method: 'POST', headers: { 'Content-Type': 'application/json', 'X-Forwarded-For': '198.51.10.2' }, + body: JSON.stringify({ email: 'plain' + crypto.randomBytes(4).toString('hex') + '@x.local', password: 'Passw0rd123' }), + }); + const r = await jfetch('/api/admin/limiter-rejections', { headers: { Authorization: 'Bearer ' + reg.body.token } }); + assert.ok(r.status === 403 || r.status === 401, `ordinary user refused (got ${r.status})`); + const anon = await jfetch('/api/admin/limiter-rejections'); + assert.ok(anon.status === 401 || anon.status === 403, 'and anonymous too'); +}); diff --git a/server/test/limiter-telemetry.test.js b/server/test/limiter-telemetry.test.js new file mode 100644 index 0000000..9ffdce2 --- /dev/null +++ b/server/test/limiter-telemetry.test.js @@ -0,0 +1,84 @@ +'use strict'; + +// QA-SNAT. The auth limiters are app.use middleware that return 429 before the handler that +// writes activity_log, so a rejection leaves no trace anywhere — the limit censors the evidence +// of itself. Four production IPs sit at exactly 10 logins/min and there is no way to tell +// whether that is one attacker or a NATed office colliding on a shared egress address. +// +// The count of rejections does NOT answer that. The count of DISTINCT ACCOUNTS per IP does: +// one account hammered means the limiter is working as intended; several accounts each denied +// a few times means real users are being locked out by a shared IP. That is the measurement +// pinned here. +// +// The second thing pinned here is restraint: this must not quietly become a store of customer +// email addresses. Identifiers are salted-hashed and only ever counted. + +const { test, beforeEach } = require('node:test'); +const assert = require('node:assert/strict'); +const tel = require('../lib/limiter-telemetry'); + +beforeEach(() => tel.reset()); + +const rej = (ip, identifier, endpoint = '/api/auth/login') => + tel.recordRejection({ endpoint, ip, identifier }); + +test('THE POINT: one account hammered reads differently from many accounts denied', () => { + for (let i = 0; i < 20; i++) rej('203.0.113.1', 'victim@example.com'); + for (const who of ['a@corp.test', 'b@corp.test', 'c@corp.test', 'd@corp.test']) rej('203.0.113.2', who); + + const [attacker] = tel.snapshot().filter(r => r.ip === '203.0.113.1'); + const [office] = tel.snapshot().filter(r => r.ip === '203.0.113.2'); + + assert.equal(attacker.distinctIdentifiers, 1); + assert.equal(attacker.likelySharedEgress, false, 'many rejections, one account -> limiter working'); + assert.equal(office.distinctIdentifiers, 4); + assert.equal(office.likelySharedEgress, true, 'few rejections each, many accounts -> shared egress'); + assert.ok(office.rejections < attacker.rejections, + 'and the rejection COUNT alone would have ranked these the wrong way round'); +}); + +test('identifiers are never exposed — counts only', () => { + rej('203.0.113.3', 'secret.person@customer.example'); + const dump = JSON.stringify(tel.snapshot()); + assert.doesNotMatch(dump, /secret\.person/, 'no address in the snapshot'); + assert.doesNotMatch(dump, /customer\.example/, 'not even the domain'); + assert.match(dump, /"distinctIdentifiers":1/); +}); + +test('the same account in different case is one account, not two', () => { + rej('203.0.113.4', 'Bob@Example.COM'); + rej('203.0.113.4', 'bob@example.com'); + assert.equal(tel.snapshot()[0].distinctIdentifiers, 1, 'or an attacker could inflate the count and look like an office'); +}); + +test('endpoint and IP are keyed separately', () => { + rej('203.0.113.5', 'x@y.z', '/api/auth/login'); + rej('203.0.113.5', 'x@y.z', '/api/auth/register'); + rej('203.0.113.6', 'x@y.z', '/api/auth/login'); + assert.equal(tel.snapshot().length, 3, 'three distinct buckets'); +}); + +test('a rejection with no identifier still counts', () => { + // Not every limited endpoint carries an email (reset-password redeem, totp verify). + const t = rej('203.0.113.7', undefined, '/api/auth/reset-password'); + assert.equal(t.rejections, 1); + assert.equal(t.distinctIdentifiers, 0); +}); + +test('memory is bounded, and says so rather than silently undercounting', () => { + for (let i = 0; i < tel.MAX_IDS_PER_KEY + 25; i++) rej('203.0.113.8', `u${i}@x.test`); + const row = tel.snapshot()[0]; + assert.equal(row.distinctIdentifiers, tel.MAX_IDS_PER_KEY, 'capped'); + assert.equal(row.identifiersTruncated, true, 'and flagged, so the number is not read as exact'); +}); + +test('key count is bounded under a flood of distinct IPs', () => { + for (let i = 0; i < tel.MAX_KEYS + 500; i++) rej(`198.51.100.${i}`, 'x@y.z'); + assert.ok(tel.snapshot().length <= tel.MAX_KEYS, 'cannot be grown without limit by spraying IPs'); +}); + +test('busiest first, so the snapshot is readable at a glance', () => { + rej('203.0.113.9', 'a@x.t'); + for (let i = 0; i < 5; i++) rej('203.0.113.10', 'b@x.t'); + assert.equal(tel.snapshot()[0].ip, '203.0.113.10'); +});