From 07cdb618847b6b742c3dde72ea5f345932663fc7 Mon Sep 17 00:00:00 2001 From: Memet Date: Sat, 24 Oct 2026 15:25:47 +0300 Subject: [PATCH 01/13] fix(security): the Redis rate-limit store sets TTLs on node-redis MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit store.set() called client.set(k, v, 'PX', ttl). node-redis does not understand the positional form and stores the key with no expiry, so on node-redis every withBan ban marker had no TTL, read back as already expired, and was never enforced — while the key lived forever. set() now goes through the shared setWithTTL, which dispatches per driver. The live suite covers set and withBan on ioredis and node-redis. --- .../security/src/rate-limit/stores/redis.js | 9 ++++--- .../rate-limit.redis.integration.test.js | 24 +++++++++++++++++++ 2 files changed, 30 insertions(+), 3 deletions(-) diff --git a/packages/security/src/rate-limit/stores/redis.js b/packages/security/src/rate-limit/stores/redis.js index fbd8810e..1b3df131 100644 --- a/packages/security/src/rate-limit/stores/redis.js +++ b/packages/security/src/rate-limit/stores/redis.js @@ -1,4 +1,5 @@ import { assertRedisClient } from '@exortek/shared/redis-guard'; +import { createRedisHelpers } from '@exortek/shared/redis-helpers'; import { isArray, isFunction } from '@exortek/shared/predicates'; import { invalidArgument } from '../../internal/guards.js'; @@ -113,6 +114,7 @@ export function redisStore(client, options = {}) { const prefix = options.prefix ?? 'rl:'; const k = key => `${prefix}${key}`; + const helpers = createRedisHelpers(client); // ioredis exposes `defineCommand` — cache the scripts once so subsequent // calls go out as EVALSHA (saves bandwidth + parse cost per request). @@ -216,9 +218,10 @@ export function redisStore(client, options = {}) { }, async set(key, count, ttlMs) { - // node-redis v4 accepts positional 'PX', ttl on its `set` command; - // ioredis and Upstash do too. - await client.set(k(key), String(count), 'PX', ttlMs); + // Dispatched per driver: node-redis takes `{ PX }` and silently + // ignores the ioredis-style positional `'PX', ttl` — storing the key + // with no expiry, which made every `withBan` ban unenforceable. + await helpers.setWithTTL(k(key), String(count), ttlMs); }, async decr(key) { diff --git a/packages/security/tests/rate-limit.redis.integration.test.js b/packages/security/tests/rate-limit.redis.integration.test.js index 6d898488..2cde32fc 100644 --- a/packages/security/tests/rate-limit.redis.integration.test.js +++ b/packages/security/tests/rate-limit.redis.integration.test.js @@ -53,6 +53,30 @@ forEachRedisDriver('security rate-limit redis store', ({ test, client, ns, raw, assert.equal(c.count, 2); }); + test('set stores the value with its TTL on every driver', async () => { + const prefix = ns('set'); + const store = rateLimit.stores.redis(client(), { prefix }); + await store.set('k', 1, 5000); + const pttl = await raw.pttl(`${prefix}k`); + assert.ok(pttl > 0 && pttl <= 5000, `set must keep its TTL; got PTTL ${pttl} (-1 means none)`); + const got = await store.get('k'); + assert.ok(got && got.expiresAt > Date.now()); + }); + + test('withBan bans after the threshold on every driver', async () => { + const store = rateLimit.stores.redis(client(), { prefix: ns('ban') }); + const limiter = rateLimit.withBan(rateLimit.fixed({ requests: 1, window: '2s', store }), { + store, + threshold: 1, + banDuration: '1m', + }); + assert.equal((await limiter.check({ key: 'ip' })).allowed, true); + assert.equal((await limiter.check({ key: 'ip' })).allowed, false); // trips the ban + const banned = await limiter.check({ key: 'ip' }); + assert.equal(banned.allowed, false); + assert.ok(banned.retryAfter > 2, 'the ban window, not the base window, should drive Retry-After'); + }); + test('read returns null for missing keys', async () => { const store = rateLimit.stores.redis(client(), { prefix: ns('miss') }); assert.equal(await store.read('nope'), null); From be85beb71f68f210e8b06f68df621d09c1faf247 Mon Sep 17 00:00:00 2001 From: Memet Date: Sat, 24 Oct 2026 15:26:59 +0300 Subject: [PATCH 02/13] fix(security)!: getClientIp follows Express trust-proxy semantics Several ways to get a spoofed client IP out of getClientIp: - X-Real-IP was read whenever X-Forwarded-For was absent, though most proxies never set or strip it - only the first X-Forwarded-For header line was read, so a client could put its own line first - proxyCount counted forwarded entries only, one hop off from Express's trust proxy: N, and a non-number silently fell through to the left-most (client-controlled) entry - ::ffff:-mapped peers never matched an IPv4 trust list The chain is now [socket peer, ...X-Forwarded-For right-to-left] across every header line, IPv4-mapped addresses are folded, proxyCount means exactly what trust proxy: N means (0 = the socket peer) and must be a non-negative integer, and X-Real-IP is only used when named in headers. BREAKING CHANGE: proxyCount is now Express-compatible (previously proxyCount: N behaved like trust proxy: N + 1), and X-Real-IP is no longer read by default. --- packages/security/src/helpers/net.js | 134 ++++++++++++++++-------- packages/security/tests/helpers.test.js | 39 +++++-- 2 files changed, 122 insertions(+), 51 deletions(-) diff --git a/packages/security/src/helpers/net.js b/packages/security/src/helpers/net.js index 9e7a4a0d..36514ca2 100644 --- a/packages/security/src/helpers/net.js +++ b/packages/security/src/helpers/net.js @@ -1,34 +1,44 @@ import { createHmac } from 'node:crypto'; import { timingSafeEqual } from '@exortek/shared/timing-safe'; -import { isArray, isBuffer, isNumber, isString } from '@exortek/shared/predicates'; +import { isArray, isBuffer, isInteger, isString } from '@exortek/shared/predicates'; import { invalidArgument } from '../internal/guards.js'; /** * Extract the real client IP from a request-like object, honouring a - * trust-proxy allowlist. Node behind a load balancer sees the LB's IP - * in `req.socket.remoteAddress`; the real client sits inside - * `X-Forwarded-For` — but only reachable safely by walking the header + * trust-proxy policy. Node behind a load balancer sees the LB's IP in + * `req.socket.remoteAddress`; the real client sits inside + * `X-Forwarded-For` — but only reachable safely by walking the chain * **right-to-left** past the trusted proxy hops, because a client can * forge left-most entries and every conforming proxy *appends* the - * real address. + * address it saw. + * + * The walk runs over `[socket.remoteAddress, ...X-Forwarded-For reversed]` + * — the same address list Express's `trust proxy` uses — with every + * repeated header value included and IPv4-mapped IPv6 (`::ffff:1.2.3.4`) + * folded to plain IPv4 before comparison. * * ### `trustProxy` semantics * * | Value | Meaning | * | ------------------ | ------------------------------------------------------------------------------------------- | - * | `false` (default) | Ignore XFF entirely; return `socket.remoteAddress`. Only correct when Node is edge-facing. | - * | `true` | Trust every hop. Returns the left-most XFF entry — **spoofable unless the first proxy strips inbound XFF**. | - * | `string[]` | Right-to-left walk skipping entries whose value is in the set; returns the first untrusted hop. | + * | `false` (default) | Ignore forwarding headers; return `socket.remoteAddress`. Only correct when Node is edge-facing. | + * | `true` | Trust every hop. Returns the left-most entry — **spoofable unless the first proxy strips inbound XFF**. | + * | `string[]` | Skip addresses in the set (starting with the socket peer); return the first untrusted one. | * * ### `proxyCount` * - * Alternative to `trustProxy: string[]` when the proxy chain depth is - * known but the addresses aren't stable (e.g. Cloudflare + a k8s - * ingress). Skips **N** rightmost XFF entries and returns the - * `(N + 1)`-th from the right — i.e. the last hop before the trusted - * chain begins. Wins over `trustProxy` when both are set. + * Same meaning as Express `trust proxy: N`: the N hops closest to Node — + * counting the socket peer as the first — are trusted, and the client is + * the next address out. `0` trusts nothing (the socket peer). When the + * chain is shorter than N the furthest address is returned. Use it when + * the chain depth is known but the addresses aren't stable (e.g. Cloudflare + * + a k8s ingress). Wins over `trustProxy` when both are set. + * + * Only `X-Forwarded-For` is read by default; `X-Real-IP` and similar + * single-value headers are spoofable unless your proxy overwrites them, so + * they are used only when named in `headers`. * * @param {{ * headers?: Record, @@ -45,51 +55,85 @@ import { invalidArgument } from '../internal/guards.js'; export function getClientIp(req, options = {}) { const trustProxy = options.trustProxy ?? false; const proxyCount = options.proxyCount; - const headerNames = options.headers ?? ['x-forwarded-for', 'x-real-ip']; - - const remote = req.socket?.remoteAddress || req.ip; + if (proxyCount !== undefined && (!isInteger(proxyCount) || proxyCount < 0)) { + throw invalidArgument('getClientIp.options.proxyCount must be a non-negative integer'); + } + const headerNames = options.headers ?? ['x-forwarded-for']; + const remote = normalizeIp(req.socket?.remoteAddress || req.ip); if (trustProxy === false && proxyCount === undefined) { - return remote || undefined; + return remote; } - const trustedSet = isArray(trustProxy) ? new Set(trustProxy) : null; - if (trustedSet && remote && !trustedSet.has(remote) && proxyCount === undefined) { - return remote || undefined; + + // Closest first: the socket peer, then each forwarded hop right-to-left. + const forwarded = readForwarded(req.headers, headerNames); + const addrs = remote === undefined ? forwarded.reverse() : [remote, ...forwarded.reverse()]; + if (addrs.length === 0) { + return undefined; } - for (const name of headerNames) { - const raw = req.headers?.[name]; - const value = isArray(raw) ? raw[0] : raw; - if (!isString(value) || value.length === 0) { - continue; + if (proxyCount !== undefined) { + // Without a socket peer the first forwarded entry stands in for hop 0. + const index = remote === undefined ? proxyCount - 1 : proxyCount; + if (index < 0) { + return undefined; } - const parts = value - .split(',') - .map(s => s.trim()) - .filter(Boolean); - if (parts.length === 0) { - continue; - } - - if (isNumber(proxyCount) && proxyCount >= 0) { - const idx = parts.length - proxyCount - 1; - return idx >= 0 ? parts[idx] : parts[0]; + return addrs[Math.min(index, addrs.length - 1)]; + } + if (isArray(trustProxy)) { + const trusted = new Set(trustProxy.map(normalizeIp)); + for (const addr of addrs) { + if (!trusted.has(addr)) { + return addr; + } } + return addrs[addrs.length - 1]; // every hop trusted — the furthest one + } + // trustProxy === true → every hop is trusted; the client sits furthest out. + // This is spoofable unless the first proxy strips inbound XFF. + return addrs[addrs.length - 1]; +} - if (trustedSet) { - for (let i = parts.length - 1; i >= 0; i--) { - if (!trustedSet.has(parts[i])) { - return parts[i]; +/** + * Every address in the named forwarding headers, left-to-right, across + * repeated header lines. + * + * @param {Record | undefined} headers + * @param {string[]} names + * @returns {string[]} + */ +function readForwarded(headers, names) { + for (const name of names) { + const raw = headers?.[name]; + const lines = isArray(raw) ? raw : [raw]; + const parts = []; + for (const line of lines) { + if (!isString(line)) { + continue; + } + for (const part of line.split(',')) { + const addr = normalizeIp(part.trim()); + if (addr) { + parts.push(addr); } } - return parts[0]; // every hop trusted — fall back to the original client } + if (parts.length > 0) { + return parts; + } + } + return []; +} - // trustProxy === true → every hop is trusted; the client sits at the - // left. This is spoofable unless the first proxy strips inbound XFF. - return parts[0]; +/** + * @param {string | undefined} addr + * @returns {string | undefined} + */ +function normalizeIp(addr) { + if (!isString(addr) || addr.length === 0) { + return undefined; } - return remote || undefined; + return /^::ffff:\d+\.\d+\.\d+\.\d+$/i.test(addr) ? addr.slice(7) : addr; } /** diff --git a/packages/security/tests/helpers.test.js b/packages/security/tests/helpers.test.js index 39ba1fd7..fea5df36 100644 --- a/packages/security/tests/helpers.test.js +++ b/packages/security/tests/helpers.test.js @@ -73,18 +73,45 @@ test('getClientIp: trustProxy=list — every hop trusted falls back to left-most assert.equal(getClientIp(req, { trustProxy: ['10.0.0.1', '10.0.0.2', '10.0.0.3'] }), '10.0.0.2'); }); -test('getClientIp: proxyCount=N skips N rightmost hops', () => { +test('getClientIp: proxyCount=N matches Express trust proxy: N (socket peer is hop 1)', () => { const req = { socket: { remoteAddress: '10.0.0.1' }, headers: { 'x-forwarded-for': '1.2.3.4, 203.0.113.42, 172.16.0.5, 10.0.0.2' }, }; - // 2 trusted proxy hops from the right → return the 3rd-from-right - assert.equal(getClientIp(req, { proxyCount: 2 }), '203.0.113.42'); + // Trusted: 10.0.0.1 (peer), 10.0.0.2, 172.16.0.5 → the client is the next one out. + assert.equal(getClientIp(req, { proxyCount: 3 }), '203.0.113.42'); + assert.equal(getClientIp(req, { proxyCount: 1 }), '10.0.0.2'); }); -test('getClientIp: proxyCount=0 returns the right-most hop', () => { - const req = { headers: { 'x-forwarded-for': '1.2.3.4, 203.0.113.42, 10.0.0.2' } }; - assert.equal(getClientIp(req, { proxyCount: 0 }), '10.0.0.2'); +test('getClientIp: proxyCount=0 trusts nothing — the socket peer', () => { + const req = { socket: { remoteAddress: '10.0.0.1' }, headers: { 'x-forwarded-for': '1.2.3.4, 10.0.0.2' } }; + assert.equal(getClientIp(req, { proxyCount: 0 }), '10.0.0.1'); +}); + +test('getClientIp: X-Real-IP is not trusted unless named', () => { + const req = { socket: { remoteAddress: '10.0.0.1' }, headers: { 'x-real-ip': '6.6.6.6' } }; + assert.equal(getClientIp(req, { proxyCount: 1 }), '10.0.0.1'); + assert.equal(getClientIp(req, { proxyCount: 1, headers: ['x-real-ip'] }), '6.6.6.6'); +}); + +test('getClientIp: repeated XFF header lines are all part of the chain', () => { + const req = { + socket: { remoteAddress: '10.0.0.1' }, + headers: { 'x-forwarded-for': ['6.6.6.6', '203.0.113.42'] }, + }; + // Reading only the first line would hand back the attacker-supplied value. + assert.equal(getClientIp(req, { proxyCount: 1 }), '203.0.113.42'); +}); + +test('getClientIp: IPv4-mapped IPv6 peers match an IPv4 trust list', () => { + const req = { socket: { remoteAddress: '::ffff:10.0.0.1' }, headers: { 'x-forwarded-for': '203.0.113.42' } }; + assert.equal(getClientIp(req, { trustProxy: ['10.0.0.1'] }), '203.0.113.42'); + assert.equal(getClientIp({ socket: { remoteAddress: '::ffff:10.0.0.9' } }), '10.0.0.9'); +}); + +test('getClientIp: a non-integer proxyCount is rejected', () => { + assert.throws(() => getClientIp({}, { proxyCount: '1' }), /proxyCount/); + assert.throws(() => getClientIp({}, { proxyCount: -1 }), /proxyCount/); }); test('getClientIp: proxyCount larger than chain returns left-most', () => { From e023268220a363fdd6645d99a511a5fe79b325b4 Mon Sep 17 00:00:00 2001 From: Memet Date: Sat, 24 Oct 2026 15:27:29 +0300 Subject: [PATCH 03/13] fix(security): webhook verifiers reject an empty Buffer secret webhookVerify and webhookVerifyStripe guarded the secret with !secret, which is false for Buffer.alloc(0). An empty key (an unset env var read into a Buffer) was accepted, and an HMAC under an empty key can be computed by anyone. Both now require a non-empty string or Buffer. --- packages/security/src/helpers/net.js | 16 ++++++++++++++-- packages/security/tests/helpers.test.js | 6 ++++++ 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/packages/security/src/helpers/net.js b/packages/security/src/helpers/net.js index 36514ca2..38d07bd7 100644 --- a/packages/security/src/helpers/net.js +++ b/packages/security/src/helpers/net.js @@ -136,6 +136,18 @@ function normalizeIp(addr) { return /^::ffff:\d+\.\d+\.\d+\.\d+$/i.test(addr) ? addr.slice(7) : addr; } +/** + * A webhook HMAC key must actually be a key: a non-empty string or Buffer. + * `Buffer.alloc(0)` is truthy, so a bare `!secret` check let an empty key + * through — and an HMAC under an empty key is forgeable by anyone. + * + * @param {unknown} secret + * @returns {boolean} + */ +function isNonEmptySecret(secret) { + return (isString(secret) || isBuffer(secret)) && secret.length > 0; +} + /** * Parse an `Authorization: Bearer ` header. Case-insensitive on the * scheme (RFC 7235 §2.1). Returns the token, or `null` when the header is @@ -229,7 +241,7 @@ export function webhookVerify(payload, signatureHeader, secret, options = {}) { if (!isString(signatureHeader) || signatureHeader.length === 0) { return false; } - if (!secret || (!isString(secret) && !isBuffer(secret))) { + if (!isNonEmptySecret(secret)) { throw invalidArgument('webhookVerify.secret must be a non-empty string or Buffer'); } const algorithm = options.algorithm ?? 'sha256'; @@ -310,7 +322,7 @@ export function webhookVerifyStripe(payload, signatureHeader, secret, options = throw invalidArgument('webhookVerifyStripe.secret must be a non-empty string / Buffer, or a non-empty array'); } for (const s of secrets) { - if (!s || (!isString(s) && !isBuffer(s))) { + if (!isNonEmptySecret(s)) { throw invalidArgument('webhookVerifyStripe.secret entries must be non-empty strings or Buffers'); } } diff --git a/packages/security/tests/helpers.test.js b/packages/security/tests/helpers.test.js index fea5df36..266599d2 100644 --- a/packages/security/tests/helpers.test.js +++ b/packages/security/tests/helpers.test.js @@ -572,3 +572,9 @@ test('sanitizeFilename: strips trailing spaces/dots without ReDoS (linear)', () const flooded = 'file' + ' '.repeat(500000); assert.equal(sanitizeFilename(flooded), 'file'); }); + +test('webhookVerify / webhookVerifyStripe: an empty Buffer secret is rejected', () => { + assert.throws(() => webhookVerify('{}', 'sha256=00', Buffer.alloc(0)), /secret/); + assert.throws(() => webhookVerifyStripe('{}', 't=1,v1=00', Buffer.alloc(0)), /secret/); + assert.throws(() => webhookVerifyStripe('{}', 't=1,v1=00', ['ok-secret', Buffer.alloc(0)]), /secret/); +}); From ac206cfaa2f2864baf8de01280b5b8b53a5d2b03 Mon Sep 17 00:00:00 2001 From: Memet Date: Sat, 24 Oct 2026 15:28:37 +0300 Subject: [PATCH 04/13] fix(security)!: rate-limit middleware fails closed and groups IPv6 /64s MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three gaps in the rate-limit middleware: - a request with no derivable key (no client IP, or keyGenerator returning nothing) skipped limiting entirely - the default key was the raw IPv6 address, so one host rotating through its /64 got 2^64 fresh buckets - the documented trustProxy option was never read Unattributable requests now share one bucket (onMissingKey: 'shared', default) or are refused with 429 (onMissingKey: 'deny'); IPv6 clients are keyed by their /64 (ipv6Subnet, configurable); IPv4-mapped IPv6 is keyed as IPv4. Passing trustProxy now throws — proxy trust belongs to the framework (req.ip / request.ip), which is what the key uses. BREAKING CHANGE: unattributable requests are limited instead of passing, IPv6 keys change to /64 prefixes, and rateLimit.trustProxy is rejected. --- packages/security/src/middleware/core.js | 104 +++++++++++++++--- .../security/tests/middleware.express.test.js | 51 +++++++++ 2 files changed, 142 insertions(+), 13 deletions(-) diff --git a/packages/security/src/middleware/core.js b/packages/security/src/middleware/core.js index 48cae737..fdd3b66f 100644 --- a/packages/security/src/middleware/core.js +++ b/packages/security/src/middleware/core.js @@ -1,5 +1,7 @@ import { parseCookies as sharedParseCookies, serialiseCookie as sharedSerialiseCookie } from '@exortek/shared/cookie'; -import { isBuffer, isFunction, isObject, isString } from '@exortek/shared/predicates'; +import { isIP } from 'node:net'; + +import { isBuffer, isFunction, isInteger, isObject, isString } from '@exortek/shared/predicates'; import { cors as buildCorsCheck } from '../cors/index.js'; import { headers as buildHeaders } from '../headers/index.js'; @@ -26,14 +28,18 @@ import { invalidArgument } from '../internal/guards.js'; * @property {{ check: Function }} limiter * @property {(req: unknown) => string | undefined} [keyGenerator] * @property {(req: unknown, res: unknown, result: object) => unknown} [onDenied] - * @property {boolean} [trustProxy=false] - * Whether the default key generator may read the client IP from the - * `X-Forwarded-For` header. That header is client-controlled, so trusting - * it without a proxy in front lets an attacker rotate it to mint unlimited - * rate-limit buckets (limit bypass) or spoof another user's IP. Leave - * `false` unless your app sits behind a proxy/CDN that overwrites XFF. - * Ignored when `keyGenerator` is supplied. (Express adapter uses `req.ip`, - * which already honours the framework's own `trust proxy` setting.) + * @property {'shared' | 'deny'} [onMissingKey='shared'] + * What to do when no key can be derived (no client IP, or `keyGenerator` + * returned nothing). `'shared'` counts every such request against one + * common bucket; `'deny'` answers 429. Never unlimited. + * @property {number} [ipv6Subnet=64] + * The default key groups IPv6 clients by this prefix length — one host + * usually owns a whole /64, so a per-address key lets it rotate through + * 2^64 buckets. + * + * The default key is the framework's client IP (`req.ip` / `request.ip`), + * which honours the framework's own trust-proxy setting — configure proxy + * trust there (Express `app.set('trust proxy', …)`, Fastify `trustProxy`). */ /** @@ -89,11 +95,27 @@ export function normalizeRateLimit(options) { 'rateLimit.limiter must be a compiled limiter (e.g. rateLimit.sliding({...})).', ); } + if (options.trustProxy !== undefined) { + // Never read: the client IP comes from the framework, which owns proxy + // trust. Failing loudly beats letting callers believe it does something. + throw invalidArgument( + "rateLimit.trustProxy is not supported — configure your framework's trust-proxy setting instead", + ); + } + const onMissingKey = options.onMissingKey ?? 'shared'; + if (onMissingKey !== 'shared' && onMissingKey !== 'deny') { + throw invalidArgument("rateLimit.onMissingKey must be 'shared' or 'deny'"); + } + const ipv6Subnet = options.ipv6Subnet ?? 64; + if (!isInteger(ipv6Subnet) || ipv6Subnet < 1 || ipv6Subnet > 128) { + throw invalidArgument('rateLimit.ipv6Subnet must be an integer between 1 and 128'); + } return { limiter: options.limiter, keyGenerator: options.keyGenerator, onDenied: options.onDenied, - trustProxy: options.trustProxy === true, + onMissingKey, + ipv6Subnet, headers: normalizeRateLimitHeaders(options.headers ?? 'legacy'), }; } @@ -338,15 +360,71 @@ export async function runCors(corsCheck, ctx, staticHeaders = null) { return null; } +const MISSING_KEY = '__unattributed__'; + +/** + * The default rate-limit key: the client IP, with IPv6 addresses reduced to + * their `/subnet` prefix (IPv4-mapped IPv6 is treated as IPv4). + * + * @param {string | undefined} ip + * @param {number} subnet + * @returns {string | undefined} + */ +export function ipKey(ip, subnet) { + if (!isString(ip) || ip.length === 0) { + return undefined; + } + const addr = ip.replace(/%.*$/, ''); + const mapped = addr.match(/^::ffff:(\d+\.\d+\.\d+\.\d+)$/i); + if (mapped) { + return mapped[1]; + } + if (isIP(addr) !== 6) { + return addr; + } + const groups = expandIpv6(addr); + let bits = subnet; + const out = groups.map(group => { + const keep = Math.max(0, Math.min(16, bits)); + bits -= 16; + const mask = keep === 0 ? 0 : (0xffff << (16 - keep)) & 0xffff; + return (group & mask).toString(16); + }); + return `${out.join(':')}/${subnet}`; +} + +/** + * @param {string} addr a valid IPv6 address + * @returns {number[]} eight 16-bit groups + */ +function expandIpv6(addr) { + let text = addr; + // An embedded dotted IPv4 tail (e.g. ::1.2.3.4) becomes two groups. + const v4 = text.match(/(\d+)\.(\d+)\.(\d+)\.(\d+)$/); + if (v4) { + const [a, b, c, d] = v4.slice(1).map(Number); + text = `${text.slice(0, -v4[0].length)}${((a << 8) | b).toString(16)}:${((c << 8) | d).toString(16)}`; + } + const [head, tail] = text.split('::'); + const left = head ? head.split(':') : []; + const right = tail !== undefined && tail.length > 0 ? tail.split(':') : []; + const fill = tail === undefined ? [] : new Array(8 - left.length - right.length).fill('0'); + return [...left, ...fill, ...right].map(h => Number.parseInt(h, 16)); +} + /** * @param {ReturnType} rateLimit * @param {AdapterContext} ctx * @returns {Promise} */ export async function runRateLimit(rateLimit, ctx) { - const key = rateLimit.keyGenerator ? rateLimit.keyGenerator(ctx.rawReq()) : ctx.ip(); - if (!key) { - return null; + let key = rateLimit.keyGenerator ? rateLimit.keyGenerator(ctx.rawReq()) : ipKey(ctx.ip(), rateLimit.ipv6Subnet); + if (!isString(key) || key.length === 0) { + // Fail closed: a request we cannot attribute is still limited. + if (rateLimit.onMissingKey === 'deny') { + return ctx.json(429, { error: 'RateLimited', message: 'Request could not be attributed to a client' }); + } + key = MISSING_KEY; } const result = await rateLimit.limiter.check({ key }); const hn = rateLimit.headers; diff --git a/packages/security/tests/middleware.express.test.js b/packages/security/tests/middleware.express.test.js index 103b1e8e..5b32c6e8 100644 --- a/packages/security/tests/middleware.express.test.js +++ b/packages/security/tests/middleware.express.test.js @@ -156,3 +156,54 @@ test('express: securityMiddleware composes all four', async () => { assert.ok(r.headers['x-ratelimit-remaining']); store._stop(); }); + +test('express: an unattributable request is still rate-limited (shared bucket)', async () => { + const store = rateLimit.stores.memory(); + const app = makeApp(a => + a.use( + rateLimitMiddleware({ + limiter: rateLimit.fixed({ requests: 1, window: '1m', store }), + keyGenerator: () => undefined, + }), + ), + ); + assert.equal((await fetchTest(app, { path: '/ping' })).status, 200); + assert.equal((await fetchTest(app, { path: '/ping' })).status, 429); + store._stop(); +}); + +test("express: onMissingKey 'deny' refuses an unattributable request", async () => { + const store = rateLimit.stores.memory(); + const app = makeApp(a => + a.use( + rateLimitMiddleware({ + limiter: rateLimit.fixed({ requests: 10, window: '1m', store }), + keyGenerator: () => '', + onMissingKey: 'deny', + }), + ), + ); + assert.equal((await fetchTest(app, { path: '/ping' })).status, 429); + store._stop(); +}); + +test('express: IPv6 clients in one /64 share a bucket', async () => { + const store = rateLimit.stores.memory(); + const app = makeApp(a => + a.use(rateLimitMiddleware({ limiter: rateLimit.fixed({ requests: 1, window: '1m', store }) })), + ); + const from = ip => fetchTest(app, { path: '/ping', headers: { 'x-forwarded-for': ip } }); + assert.equal((await from('2001:db8:1:2::1')).status, 200); + assert.equal((await from('2001:db8:1:2::ffff')).status, 429); + assert.equal((await from('2001:db8:1:3::1')).status, 200); + store._stop(); +}); + +test('express: the unused rateLimit.trustProxy option is refused', () => { + const store = rateLimit.stores.memory(); + assert.throws( + () => rateLimitMiddleware({ limiter: rateLimit.fixed({ requests: 1, window: '1m', store }), trustProxy: true }), + /trust-proxy/, + ); + store._stop(); +}); From 6cdad53329616a6a1dd3e700718f6d20f3aeeb1e Mon Sep 17 00:00:00 2001 From: Memet Date: Sat, 24 Oct 2026 15:28:52 +0300 Subject: [PATCH 05/13] fix(security): opaque origins are never the same origin MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit isSameOrigin compared URL.origin strings, and every opaque origin (javascript:, data:, file:, custom schemes) serializes as 'null' — so isSameOrigin('javascript:alert(1)', 'myapp://cb') returned true. Either side being opaque now means not the same origin. --- packages/security/src/redirect/index.js | 6 ++++++ packages/security/tests/redirect.test.js | 6 ++++++ 2 files changed, 12 insertions(+) diff --git a/packages/security/src/redirect/index.js b/packages/security/src/redirect/index.js index 4d3a0e6b..6fce626e 100644 --- a/packages/security/src/redirect/index.js +++ b/packages/security/src/redirect/index.js @@ -259,6 +259,12 @@ export function isSameOrigin(a, b) { try { const oa = a instanceof URL ? a.origin : new URL(a).origin; const ob = b instanceof URL ? b.origin : new URL(b).origin; + // Opaque origins (`data:`, `javascript:`, `file:`, custom schemes) + // serialize as the string 'null' — equal as strings, never the same + // origin (WHATWG HTML §7.5). + if (oa === 'null' || ob === 'null') { + return false; + } return oa === ob; } catch { return false; diff --git a/packages/security/tests/redirect.test.js b/packages/security/tests/redirect.test.js index 132d3397..39ec22a7 100644 --- a/packages/security/tests/redirect.test.js +++ b/packages/security/tests/redirect.test.js @@ -216,3 +216,9 @@ test('isSameOrigin: null / undefined / malformed → false', () => { assert.equal(isSameOrigin('https://x', undefined), false); assert.equal(isSameOrigin('not-a-url', 'https://x'), false); }); + +test('isSameOrigin: opaque origins are never the same origin', () => { + assert.equal(isSameOrigin('javascript:alert(1)', 'myapp://cb'), false); + assert.equal(isSameOrigin('data:text/html,x', 'data:text/html,x'), false); + assert.equal(isSameOrigin('https://a.example/x', 'https://a.example/y'), true); +}); From b943fc730e5612d3a4f81cd9b8a0cab0962a1b65 Mon Sep 17 00:00:00 2001 From: Memet Date: Sat, 24 Oct 2026 15:29:46 +0300 Subject: [PATCH 06/13] fix(security): origin allowlist patterns are stateless cors() and checkOrigin() called RegExp.test() on the caller's pattern directly. With the g or y flag set, lastIndex carries over between calls, so the same allowed origin alternately passed and failed. Patterns are now compiled once without g / y, and a pattern with no $ end anchor (which also matches origins with anything appended) emits a one-time EXORTEK_SECURITY_UNANCHORED_ORIGIN warning. A CORS denial from an allowlist also carries Vary: Origin, so a shared cache cannot replay it to (or a grant from) another origin. --- packages/security/src/cors/index.js | 10 +++++- packages/security/src/helpers/net.js | 4 ++- packages/security/src/internal/patterns.js | 36 ++++++++++++++++++++++ packages/security/tests/cors.test.js | 14 +++++++++ packages/security/tests/helpers.test.js | 7 +++++ 5 files changed, 69 insertions(+), 2 deletions(-) create mode 100644 packages/security/src/internal/patterns.js diff --git a/packages/security/src/cors/index.js b/packages/security/src/cors/index.js index 9dbdf69f..5601478e 100644 --- a/packages/security/src/cors/index.js +++ b/packages/security/src/cors/index.js @@ -1,5 +1,6 @@ import { SecurityError, ErrorCode } from '../internal/errors.js'; import { invalidArgument } from '../internal/guards.js'; +import { statelessPattern } from '../internal/patterns.js'; import { isArray, isFunction, isNull, isNumber, isString, isUndefined } from '@exortek/shared/predicates'; /** @@ -87,7 +88,9 @@ function compileOriginCheck(origin) { if (isFunction(origin)) { return origin; } - const matchers = isArray(origin) ? origin : [origin]; + const matchers = (isArray(origin) ? origin : [origin]).map(m => + m instanceof RegExp ? statelessPattern(m, 'cors.origin') : m, + ); return incoming => { if (!incoming) { return false; @@ -172,6 +175,11 @@ export function cors(options = {}) { /** @type {Record} */ const headers = {}; if (!allowed) { + // The answer depends on Origin whenever an allowlist decides it, so a + // shared cache must key the denial on it too. + if (!reflectAny) { + headers.Vary = 'Origin'; + } return { headers, allowed: false, diff --git a/packages/security/src/helpers/net.js b/packages/security/src/helpers/net.js index 38d07bd7..3e3d9385 100644 --- a/packages/security/src/helpers/net.js +++ b/packages/security/src/helpers/net.js @@ -4,6 +4,8 @@ import { timingSafeEqual } from '@exortek/shared/timing-safe'; import { isArray, isBuffer, isInteger, isString } from '@exortek/shared/predicates'; import { invalidArgument } from '../internal/guards.js'; +import { statelessPattern } from '../internal/patterns.js'; + /** * Extract the real client IP from a request-like object, honouring a @@ -209,7 +211,7 @@ export function checkOrigin(req, options) { if (isString(rule) && rule === compared) { return true; } - if (rule instanceof RegExp && rule.test(compared)) { + if (rule instanceof RegExp && statelessPattern(rule, 'checkOrigin.allowedOrigins').test(compared)) { return true; } } diff --git a/packages/security/src/internal/patterns.js b/packages/security/src/internal/patterns.js new file mode 100644 index 00000000..44626dd5 --- /dev/null +++ b/packages/security/src/internal/patterns.js @@ -0,0 +1,36 @@ +/** + * Origin allowlist patterns. A `RegExp` with the `g` or `y` flag keeps + * `lastIndex` between `.test()` calls, so the same origin alternately + * matches and fails — a CORS or Origin check that flips per request. And an + * unanchored pattern such as `/https:\/\/app\.example\.com/` also matches + * `https://app.example.com.evil.net`. + * + * `statelessPattern` strips the stateful flags (memoised per pattern) and + * warns once about a pattern with no `$` end anchor — the omission that lets + * an attacker append their own domain. (A `^`-less suffix pattern like + * `/\.example\.com$/` is a deliberate subdomain match and stays quiet.) + */ + +/** @type {WeakMap} */ +const compiled = new WeakMap(); + +/** + * @param {RegExp} pattern + * @param {string} where option name, for the warning + * @returns {RegExp} + */ +export function statelessPattern(pattern, where) { + let out = compiled.get(pattern); + if (out) { + return out; + } + out = /[gy]/.test(pattern.flags) ? new RegExp(pattern.source, pattern.flags.replace(/[gy]/g, '')) : pattern; + if (!pattern.source.endsWith('$')) { + process.emitWarning( + `${where} pattern ${pattern} has no $ end anchor, so it also matches origins with anything appended`, + { code: 'EXORTEK_SECURITY_UNANCHORED_ORIGIN' }, + ); + } + compiled.set(pattern, out); + return out; +} diff --git a/packages/security/tests/cors.test.js b/packages/security/tests/cors.test.js index 393b2ab3..14b50a45 100644 --- a/packages/security/tests/cors.test.js +++ b/packages/security/tests/cors.test.js @@ -215,3 +215,17 @@ test('cors: rejects non-2xx optionsSuccessStatus', () => { assert.throws(() => cors({ optionsSuccessStatus: 500 }), SecurityError); assert.throws(() => cors({ optionsSuccessStatus: 100 }), SecurityError); }); + +test('cors: a /g origin pattern matches on every request, not every other one', () => { + const check = cors({ origin: [/^https:\/\/app\.example\.com$/g], credentials: true }); + for (let i = 0; i < 4; i++) { + assert.equal(check({ method: 'GET', origin: 'https://app.example.com' }).allowed, true, `request ${i}`); + } +}); + +test('cors: a denied allowlist response still varies on Origin', () => { + const check = cors({ origin: ['https://app.example.com'] }); + const d = check({ method: 'GET', origin: 'https://evil.example' }); + assert.equal(d.allowed, false); + assert.equal(d.headers.Vary, 'Origin'); +}); diff --git a/packages/security/tests/helpers.test.js b/packages/security/tests/helpers.test.js index 266599d2..ec393a24 100644 --- a/packages/security/tests/helpers.test.js +++ b/packages/security/tests/helpers.test.js @@ -578,3 +578,10 @@ test('webhookVerify / webhookVerifyStripe: an empty Buffer secret is rejected', assert.throws(() => webhookVerifyStripe('{}', 't=1,v1=00', Buffer.alloc(0)), /secret/); assert.throws(() => webhookVerifyStripe('{}', 't=1,v1=00', ['ok-secret', Buffer.alloc(0)]), /secret/); }); + +test('checkOrigin: a /g allowlist pattern is stateless', () => { + const opts = { allowedOrigins: [/^https:\/\/app\.example\.com$/g] }; + const req = { method: 'POST', headers: { origin: 'https://app.example.com' } }; + assert.equal(checkOrigin(req, opts), true); + assert.equal(checkOrigin(req, opts), true); +}); From 3a14b308a0695d041a761e570d845598f07fe17d Mon Sep 17 00:00:00 2001 From: Memet Date: Sat, 24 Oct 2026 15:29:46 +0300 Subject: [PATCH 07/13] perf(security): checkOrigin no longer builds a Set per call The safe-method set was rebuilt from options.safeMethods on every request. The default set is now a module constant and custom lists are memoised per array. --- packages/security/src/helpers/net.js | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/packages/security/src/helpers/net.js b/packages/security/src/helpers/net.js index 3e3d9385..52659fbc 100644 --- a/packages/security/src/helpers/net.js +++ b/packages/security/src/helpers/net.js @@ -6,6 +6,9 @@ import { isArray, isBuffer, isInteger, isString } from '@exortek/shared/predicat import { invalidArgument } from '../internal/guards.js'; import { statelessPattern } from '../internal/patterns.js'; +const DEFAULT_SAFE_METHODS = new Set(['GET', 'HEAD', 'OPTIONS']); +/** @type {WeakMap>} */ +const safeMethodSets = new WeakMap(); /** * Extract the real client IP from a request-like object, honouring a @@ -138,6 +141,22 @@ function normalizeIp(addr) { return /^::ffff:\d+\.\d+\.\d+\.\d+$/i.test(addr) ? addr.slice(7) : addr; } +/** + * @param {string[] | undefined} methods + * @returns {Set} + */ +function safeMethodSet(methods) { + if (methods === undefined) { + return DEFAULT_SAFE_METHODS; + } + let set = safeMethodSets.get(methods); + if (!set) { + set = new Set(methods.map(m => m.toUpperCase())); + safeMethodSets.set(methods, set); + } + return set; +} + /** * A webhook HMAC key must actually be a key: a non-empty string or Buffer. * `Buffer.alloc(0)` is truthy, so a bare `!secret` check let an empty key @@ -193,7 +212,7 @@ export function bearer(headerValue) { * @returns {boolean} true if the request may proceed */ export function checkOrigin(req, options) { - const safe = new Set((options.safeMethods ?? ['GET', 'HEAD', 'OPTIONS']).map(m => m.toUpperCase())); + const safe = safeMethodSet(options.safeMethods); if (safe.has(String(req.method || '').toUpperCase())) { return true; } From fadc439ee78e78034c4a851e41cab2d19c40367b Mon Sep 17 00:00:00 2001 From: Memet Date: Sat, 24 Oct 2026 15:30:10 +0300 Subject: [PATCH 08/13] fix(security): Fastify CSRF reads a token from the request body The Fastify CSRF hook ran in onRequest, before the body is parsed, so the form-field fallback never saw a token and form posts without an x-csrf-token header always failed. CSRF now runs in preHandler, after body parsing, in both csrfPlugin and the umbrella securityPlugin. --- packages/security/src/middleware/fastify.js | 6 ++++-- .../security/tests/middleware.fastify.test.js | 17 +++++++++++++++++ 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/packages/security/src/middleware/fastify.js b/packages/security/src/middleware/fastify.js index ef8ad54a..e9f6b964 100644 --- a/packages/security/src/middleware/fastify.js +++ b/packages/security/src/middleware/fastify.js @@ -130,7 +130,9 @@ export const csrfPlugin = fastifyPlugin( "@exortek/security: csrf requires '@fastify/cookie' to be registered first. Do `await app.register(import('@fastify/cookie'))` before enabling CSRF.", ); } - attachRunner(fastify, ctx => runCsrf(csrf, ctx)); + // preHandler, not onRequest: the form-field token lives in the body, + // which Fastify has not parsed yet during onRequest. + attachRunner(fastify, ctx => runCsrf(csrf, ctx), 'preHandler'); }, { name: '@exortek/security/csrf', fastify: '>=4' }, ); @@ -181,7 +183,7 @@ export const securityPlugin = fastifyPlugin( "@exortek/security: csrf requires '@fastify/cookie' to be registered first. Do `await app.register(import('@fastify/cookie'))` before enabling CSRF.", ); } - attachRunner(fastify, ctx => runCsrf(cfg.csrf, ctx)); + attachRunner(fastify, ctx => runCsrf(cfg.csrf, ctx), 'preHandler'); } }, { name: '@exortek/security', fastify: '>=4' }, diff --git a/packages/security/tests/middleware.fastify.test.js b/packages/security/tests/middleware.fastify.test.js index 1cc9b9f3..b1934768 100644 --- a/packages/security/tests/middleware.fastify.test.js +++ b/packages/security/tests/middleware.fastify.test.js @@ -204,3 +204,20 @@ test('fastify: securityPlugin composes all four when enabled', async () => { await app.close(); store._stop(); }); + +test('fastify: csrfPlugin accepts the token from the parsed body', async () => { + const app = await build(async a => { + await a.register(fastifyCookie); + await a.register(csrfPlugin, { secret: SECRET, formField: '_csrf' }); + }); + const get = await app.inject({ method: 'GET', url: '/ping' }); + const token = get.headers['set-cookie'].match(/__Host-csrf=([^;]+)/)?.[1]; + const res = await app.inject({ + method: 'POST', + url: '/echo', + headers: { cookie: `__Host-csrf=${token}`, 'content-type': 'application/json' }, + payload: JSON.stringify({ _csrf: token }), + }); + assert.equal(res.statusCode, 200); + await app.close(); +}); From 51743d9f37c4780a41b7488feeddb8709122305a Mon Sep 17 00:00:00 2001 From: Memet Date: Sat, 24 Oct 2026 15:30:29 +0300 Subject: [PATCH 09/13] fix(security): warn when the CSRF cookie loses its __Host- prefix The signed double-submit token is not bound to a session; the default __Host-csrf cookie name is what stops an attacker on a sibling subdomain (or plain http) from planting their own valid cookie + token pair. Renaming the cookie silently dropped that protection. The CSRF middleware now emits EXORTEK_SECURITY_CSRF_COOKIE_PREFIX when the name lacks __Host- and points at generateForSession for session binding. --- packages/security/src/middleware/core.js | 11 +++++++++++ packages/security/tests/csrf.test.js | 12 ++++++++++++ 2 files changed, 23 insertions(+) diff --git a/packages/security/src/middleware/core.js b/packages/security/src/middleware/core.js index fdd3b66f..53bd3ae8 100644 --- a/packages/security/src/middleware/core.js +++ b/packages/security/src/middleware/core.js @@ -74,6 +74,17 @@ function assertCsrfSecret(secret) { */ export function normalizeCsrf(options) { assertCsrfSecret(options.secret); + const cookieName = options.cookieName ?? '__Host-csrf'; + if (!cookieName.startsWith('__Host-')) { + // The signed double-submit token is not bound to a session; what stops + // an attacker planting their own valid cookie+token pair is the + // __Host- prefix (no Domain, Secure, Path=/ — unwritable from a + // sibling subdomain or over plain http). + process.emitWarning( + `csrf.cookieName ${JSON.stringify(cookieName)} lacks the __Host- prefix; a sibling subdomain can then plant a valid token. Keep __Host- or bind tokens to the session with csrf.generateForSession().`, + { code: 'EXORTEK_SECURITY_CSRF_COOKIE_PREFIX' }, + ); + } return { secret: options.secret, cookieName: options.cookieName ?? '__Host-csrf', diff --git a/packages/security/tests/csrf.test.js b/packages/security/tests/csrf.test.js index ddc55af1..0d1d1f19 100644 --- a/packages/security/tests/csrf.test.js +++ b/packages/security/tests/csrf.test.js @@ -131,3 +131,15 @@ test('session-bound generate throws on empty sessionId', () => { assert.throws(() => generateForSession('', SECRET)); assert.throws(() => generateForSession(123, SECRET)); }); + +test('csrf middleware warns when the cookie name drops the __Host- prefix', async () => { + const { normalizeCsrf } = await import('../src/middleware/core.js'); + const warnings = []; + const onWarning = w => warnings.push(w.code); + process.on('warning', onWarning); + normalizeCsrf({ secret: 'x'.repeat(32), cookieName: 'csrf' }); + normalizeCsrf({ secret: 'x'.repeat(32) }); + await new Promise(r => setImmediate(r)); + process.off('warning', onWarning); + assert.deepEqual(warnings, ['EXORTEK_SECURITY_CSRF_COOKIE_PREFIX']); +}); From a52ecc66ad4bf8f086522e6f4f8ca0baf21ce340 Mon Sep 17 00:00:00 2001 From: Memet Date: Sat, 24 Oct 2026 15:31:24 +0300 Subject: [PATCH 10/13] fix(security): bucket limiters do not rewrite state on a denial MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit tokenBucket and leakyBucket wrote their state back on every call, including denials. Each write re-quantized the fractional refill to milli-tokens and moved updatedAt forward, so a client polling every millisecond rounded its way to more refill than configured — about 1.6x the allowed rate at refillRate 0.6/s (100 allowed in 100 s instead of ~61). A denial now leaves the state untouched. --- .../src/rate-limit/algorithms/leaky-bucket.js | 9 ++++++- .../src/rate-limit/algorithms/token-bucket.js | 9 ++++++- packages/security/tests/rate-limit.test.js | 27 +++++++++++++++++++ 3 files changed, 43 insertions(+), 2 deletions(-) diff --git a/packages/security/src/rate-limit/algorithms/leaky-bucket.js b/packages/security/src/rate-limit/algorithms/leaky-bucket.js index 73d40a7d..e5ddfdd7 100644 --- a/packages/security/src/rate-limit/algorithms/leaky-bucket.js +++ b/packages/security/src/rate-limit/algorithms/leaky-bucket.js @@ -64,7 +64,11 @@ export function leakyBucket(config) { if (level + 1 > capacity) { const overflow = level + 1 - capacity; const retryAfter = Math.max(1, Math.ceil(overflow / leakRate)); - nextState = encodeState(level, now); + // A denial leaves the stored state untouched. Rewriting it would + // re-quantize the fractional refill to milli-units and move + // updatedAt forward on every rejected call — a client polling + // every millisecond then gains (or loses) refill to rounding. + nextState = null; result = { allowed: false, remaining: 0, @@ -83,6 +87,9 @@ export function leakyBucket(config) { }; } + if (nextState === null) { + return result; + } if (hasCas) { const wrote = await store.compareAndSet(storeKey, rawState, nextState, ttlMs); if (!wrote) { diff --git a/packages/security/src/rate-limit/algorithms/token-bucket.js b/packages/security/src/rate-limit/algorithms/token-bucket.js index 2a91f92d..6c1ee90f 100644 --- a/packages/security/src/rate-limit/algorithms/token-bucket.js +++ b/packages/security/src/rate-limit/algorithms/token-bucket.js @@ -71,7 +71,11 @@ export function tokenBucket(config) { if (tokens < 1) { const deficitTokens = 1 - tokens; const retryAfter = Math.max(1, Math.ceil(deficitTokens / refillRate)); - nextState = encodeState(tokens, now); + // A denial leaves the stored state untouched. Rewriting it would + // re-quantize the fractional refill to milli-units and move + // updatedAt forward on every rejected call — a client polling + // every millisecond then gains (or loses) refill to rounding. + nextState = null; result = { allowed: false, remaining: 0, @@ -90,6 +94,9 @@ export function tokenBucket(config) { }; } + if (nextState === null) { + return result; + } if (hasCas) { const wrote = await store.compareAndSet(storeKey, rawState, nextState, ttlMs); if (!wrote) { diff --git a/packages/security/tests/rate-limit.test.js b/packages/security/tests/rate-limit.test.js index bb98bcbd..3575b55a 100644 --- a/packages/security/tests/rate-limit.test.js +++ b/packages/security/tests/rate-limit.test.js @@ -684,3 +684,30 @@ test('tokenBucket: still works on stores without compareAndSet (fallback path)', assert.equal((await limiter.check({ key: 'u' })).allowed, false); base._stop(); }); + +test('token / leaky bucket: polling every millisecond does not speed up the refill', async () => { + for (const algo of ['tokenBucket', 'leakyBucket']) { + const realNow = Date.now; + let now = 1_000_000; + Date.now = () => now; + const store = rateLimit.stores.memory(); + try { + const rate = 0.6; + const lim = rateLimit[algo]( + algo === 'tokenBucket' ? { capacity: 1, refillRate: rate, store } : { capacity: 1, leakRate: rate, store }, + ); + let allowed = 0; + for (let t = 0; t < 20_000; t++) { + if ((await lim.check({ key: 'k' })).allowed) { + allowed++; + } + now += 1; + } + // 1 initial + 0.6/s over 20 s. + assert.ok(allowed <= 13, `${algo}: ${allowed} allowed in 20 s at 0.6/s`); + } finally { + Date.now = realNow; + store._stop(); + } + } +}); From 90ff409a33e36108d629372c036a7f95da2f9a67 Mon Sep 17 00:00:00 2001 From: Memet Date: Sat, 24 Oct 2026 15:31:51 +0300 Subject: [PATCH 11/13] fix(security): sanitizeParams drops prototype-pollution keys sanitizeParams copied keys with out[key] = value, so an own __proto__ key in the parsed query (JSON bodies, qs with allowPrototypes) replaced the result's prototype and injected inherited properties such as isAdmin. It now skips __proto__ / constructor / prototype, matching sanitizeBody. --- packages/security/src/helpers/input.js | 5 +++++ packages/security/tests/helpers.test.js | 8 ++++++++ 2 files changed, 13 insertions(+) diff --git a/packages/security/src/helpers/input.js b/packages/security/src/helpers/input.js index ce74f6f0..3a5fd027 100644 --- a/packages/security/src/helpers/input.js +++ b/packages/security/src/helpers/input.js @@ -135,6 +135,11 @@ export function sanitizeParams(query, options = {}) { /** @type {Record} */ const out = {}; for (const key of keys) { + // Same rule as sanitizeBody: prototype-pollution keys never survive, + // so `out[key] = …` can't rewrite the result's prototype. + if (PROTO_KEYS.has(key)) { + continue; + } const v = query[key]; if (!isArray(v)) { out[key] = v; diff --git a/packages/security/tests/helpers.test.js b/packages/security/tests/helpers.test.js index ec393a24..bac20452 100644 --- a/packages/security/tests/helpers.test.js +++ b/packages/security/tests/helpers.test.js @@ -585,3 +585,11 @@ test('checkOrigin: a /g allowlist pattern is stateless', () => { assert.equal(checkOrigin(req, opts), true); assert.equal(checkOrigin(req, opts), true); }); + +test('sanitizeParams: a __proto__ key cannot rewrite the result prototype', () => { + const query = JSON.parse('{"__proto__": {"isAdmin": true}, "constructor": "x", "a": ["1", "2"]}'); + const out = sanitizeParams(query); + assert.equal(out.isAdmin, undefined); + assert.equal(Object.getPrototypeOf(out), Object.prototype); + assert.deepEqual(Object.keys(out), ['a']); +}); From 5340ede80f162e22276d9a53e68fa3ce174c569e Mon Sep 17 00:00:00 2001 From: Memet Date: Sat, 24 Oct 2026 15:32:28 +0300 Subject: [PATCH 12/13] docs(security): trust-proxy semantics, fail-closed keys, origin patterns Web docs for the Express-compatible getClientIp proxyCount, the X-Forwarded-For-only default, the rate-limit middleware's onMissingKey / ipv6Subnet options (and the removed trustProxy), anchored CORS / origin patterns, the CSRF __Host- prefix warning and opaque origins in isSameOrigin. --- web/content/security/cors.mdx | 2 +- web/content/security/csrf.mdx | 2 +- web/content/security/helpers.mdx | 14 ++++++++++---- web/content/security/middleware.mdx | 10 ++++++++-- web/content/security/production.mdx | 4 ++++ web/content/security/redirect.mdx | 3 ++- 6 files changed, 26 insertions(+), 9 deletions(-) diff --git a/web/content/security/cors.mdx b/web/content/security/cors.mdx index 2042625b..479cf547 100644 --- a/web/content/security/cors.mdx +++ b/web/content/security/cors.mdx @@ -77,7 +77,7 @@ Sync predicates keep `check()` synchronous — no allocation, no `await`. Async ```ts cors({ - origin?: boolean | string | RegExp + origin?: boolean | string | RegExp // RegExp: anchor it with ^…$; g / y flags are ignored | Array | ((origin: string | undefined) => boolean | Promise), methods?: string | string[], // default: GET,HEAD,PUT,PATCH,POST,DELETE diff --git a/web/content/security/csrf.mdx b/web/content/security/csrf.mdx index db107bda..239d32cf 100644 --- a/web/content/security/csrf.mdx +++ b/web/content/security/csrf.mdx @@ -158,7 +158,7 @@ Middleware options: | Option | Default | What | | ------------------ | ----------------------------------------------------- | ---------------------------------------------------- | | `secret` | _required_ | ≥ 32 bytes | -| `cookieName` | `'__Host-csrf'` | Set-Cookie name (strip `__Host-` for local http dev) | +| `cookieName` | `'__Host-csrf'` | Set-Cookie name. Without the `__Host-` prefix a sibling subdomain can plant a valid token pair, so the middleware warns (`EXORTEK_SECURITY_CSRF_COOKIE_PREFIX`) — keep it in production or bind tokens with `generateForSession` | | `headerName` | `'x-csrf-token'` | Header the client submits | | `ignoreMethods` | `['GET','HEAD','OPTIONS']` | Methods NOT verified | | `cookieOptions` | `{ httpOnly, secure, sameSite: 'strict', path: '/' }` | Override individual flags | diff --git a/web/content/security/helpers.mdx b/web/content/security/helpers.mdx index 4898122d..2c001f02 100644 --- a/web/content/security/helpers.mdx +++ b/web/content/security/helpers.mdx @@ -53,18 +53,24 @@ const ip = getClientIp(req, { }); // Chain-depth allowlist — Cloudflare + a k8s ingress, addresses unstable. +// Same meaning as Express `trust proxy: 2`. const ip = getClientIp(req, { proxyCount: 2, }); ``` +The walk runs over `[socket.remoteAddress, ...X-Forwarded-For right-to-left]`, every repeated `X-Forwarded-For` line +included, with IPv4-mapped IPv6 (`::ffff:10.0.0.1`) folded to IPv4. Only `X-Forwarded-For` is read by default — +`X-Real-IP` is trusted only when you name it in `headers`. + - `trustProxy: false` (default) — return `req.socket.remoteAddress`; ignore XFF. Correct when Node is edge-facing. - `trustProxy: true` — trust every hop and return the **left-most** XFF entry. **Spoofable unless the first proxy strips inbound XFF headers** — prefer `trustProxy: string[]` or `proxyCount` in production. -- `trustProxy: string[]` — walk XFF right-to-left, skipping entries whose value is in the set. Return the first - untrusted hop. -- `proxyCount: N` — skip **N** rightmost XFF entries and return the `(N + 1)`-th from the right. Wins over - `trustProxy` when both are set. +- `trustProxy: string[]` — skip addresses in the set, starting with the socket peer, and return the first untrusted + one (the furthest address when every hop is trusted). +- `proxyCount: N` — exactly Express's `trust proxy: N`: the N hops closest to Node, counting the socket peer as the + first, are trusted and the next address out is the client. `0` returns the socket peer; a chain shorter than N + returns its furthest address. Must be a non-negative integer. Wins over `trustProxy` when both are set. ## `bearer` diff --git a/web/content/security/middleware.mdx b/web/content/security/middleware.mdx index 6e0e9bba..0c0c6382 100644 --- a/web/content/security/middleware.mdx +++ b/web/content/security/middleware.mdx @@ -127,8 +127,9 @@ Both adapters accept the same `securityMiddleware(options)` shape: } | false, rateLimit?: { limiter: Limiter, - keyGenerator?: (req) => string | undefined, - trustProxy?: boolean, // default false + keyGenerator?: (req) => string | undefined, // default: the framework's client IP + onMissingKey?: 'shared' | 'deny', // default 'shared' — never unlimited + ipv6Subnet?: number, // default 64 — IPv6 clients keyed by prefix onDenied?: (req, res, result) => unknown, headers?: 'legacy' | 'draft' | false | { // default 'legacy' remaining?: string | false, // default 'X-RateLimit-Remaining' @@ -139,6 +140,11 @@ Both adapters accept the same `securityMiddleware(options)` shape: } ``` +The default rate-limit key is the framework's client IP (`req.ip` / `request.ip`), so proxy trust is configured where +the framework expects it (Express `app.set('trust proxy', …)`, Fastify `trustProxy`); passing `rateLimit.trustProxy` +throws. A request with no derivable key is counted against one shared bucket (`onMissingKey: 'shared'`) or refused +with 429 (`'deny'`). IPv6 clients are keyed by their `/64` so one host can't rotate through its prefix. + Set any concern to `false` (or omit) to skip. The per-concern middlewares (`corsMiddleware`, `csrfMiddleware`, `rateLimitMiddleware`, `headersMiddleware`) accept just that concern's options directly, not nested. diff --git a/web/content/security/production.mdx b/web/content/security/production.mdx index a420cfb7..2ec8cf6d 100644 --- a/web/content/security/production.mdx +++ b/web/content/security/production.mdx @@ -199,6 +199,10 @@ bucket for all clients. +The rate-limit middleware keys on that framework IP, so this is the one place proxy trust is configured (it has no +`trustProxy` option of its own). Calling `getClientIp` directly? Its `proxyCount: N` means the same as +`trust proxy: N`. + **Test it:** with trust-proxy off, curl through your proxy — every request should see the same `req.ip`. With trust-proxy on, distinct callers should see distinct IPs. diff --git a/web/content/security/redirect.mdx b/web/content/security/redirect.mdx index d9fba522..18d2d299 100644 --- a/web/content/security/redirect.mdx +++ b/web/content/security/redirect.mdx @@ -144,7 +144,8 @@ if (!isSameOrigin(req.headers.referer, 'https://app.example.com')) { Complements [`checkOrigin`](/security/helpers#checkorigin), which reads `Origin` / `Referer` headers off a request. `isSameOrigin` is the primitive both callers land on when they need direct URL-to-URL comparison — OAuth callback -validation, Referer-based flow checks, allowlist-of-one matching. +validation, Referer-based flow checks, allowlist-of-one matching. Opaque origins (`javascript:`, `data:`, `file:`, +custom schemes) all serialize as `'null'`, so they never compare as the same origin. ## A worked example From 98c105acc114f3a33639e410e61f8eebaf0ca709 Mon Sep 17 00:00:00 2001 From: Memet Date: Sat, 24 Oct 2026 15:32:41 +0300 Subject: [PATCH 13/13] chore: changeset for security hardening --- .changeset/security-audit-hardening.md | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) create mode 100644 .changeset/security-audit-hardening.md diff --git a/.changeset/security-audit-hardening.md b/.changeset/security-audit-hardening.md new file mode 100644 index 00000000..a19af5fc --- /dev/null +++ b/.changeset/security-audit-hardening.md @@ -0,0 +1,26 @@ +--- +'@exortek/security': major +--- + +Rate-limit, proxy-trust and origin hardening. + +**Security fixes** + +- The Redis rate-limit store's `set()` dropped its TTL on node-redis, so `withBan` bans were never enforced there; it now dispatches per driver. +- `getClientIp` follows Express `trust proxy` semantics: the chain is `[socket peer, ...X-Forwarded-For right-to-left]` across every header line, `proxyCount: N` means `trust proxy: N` (it was one hop off), IPv4-mapped IPv6 is folded, `proxyCount` must be a non-negative integer, and `X-Real-IP` is read only when named in `headers`. +- Rate-limit middleware fails closed: a request with no derivable key shares one bucket (`onMissingKey: 'shared'`, default) or is refused (`'deny'`) instead of skipping the limit; IPv6 clients are keyed by their `/64` (`ipv6Subnet`); the never-read `trustProxy` option now throws. +- `webhookVerify` / `webhookVerifyStripe` reject an empty Buffer secret. +- `isSameOrigin` never treats opaque origins (`'null'`) as equal. +- CORS / `checkOrigin` RegExp patterns are stateless (`g` / `y` ignored), an un-anchored pattern warns once, and a CORS denial carries `Vary: Origin`. +- Fastify CSRF runs in `preHandler`, so a body form-field token is actually read. +- Token / leaky buckets no longer rewrite state on a denial (polling every millisecond inflated the refill ~1.6×). +- `sanitizeParams` drops `__proto__` / `constructor` / `prototype` keys. +- The CSRF middleware warns when the cookie name lacks the `__Host-` prefix. + +**Migration** + +- `getClientIp(req, { proxyCount })`: add 1 to keep the old behaviour, or better, set it to the number of proxies in front of Node (Express semantics). +- If you relied on `X-Real-IP`, pass `headers: ['x-real-ip']` explicitly (only behind a proxy that overwrites it). +- Remove `rateLimit.trustProxy` from middleware options and configure trust proxy on the framework. +- Unkeyed requests are now limited; pass `onMissingKey: 'deny'` to reject them instead. +- Existing IPv6 rate-limit counters re-key to `/64` prefixes on upgrade.