fix(security): rate-limit, proxy-trust and origin hardening

.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.

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<string, string>} */
     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,

packages/security/src/helpers/input.js

@@ -135,6 +135,11 @@ export function sanitizeParams(query, options = {}) {
   /** @type {Record<string, unknown>} */
   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;

packages/security/src/helpers/net.js

@@ -1,34 +1,49 @@
 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';
+import { statelessPattern } from '../internal/patterns.js';
+
+const DEFAULT_SAFE_METHODS = new Set(['GET', 'HEAD', 'OPTIONS']);
+/** @type {WeakMap<string[], Set<string>>} */
+const safeMethodSets = new WeakMap();
 
 /**
  * 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<string, string | string[] | undefined>,
@@ -45,51 +60,113 @@ 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<string, string | string[] | undefined> | 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 [];
+}
+
+/**
+ * @param {string | undefined} addr
+ * @returns {string | undefined}
+ */
+function normalizeIp(addr) {
+  if (!isString(addr) || addr.length === 0) {
+    return undefined;
+  }
+  return /^::ffff:\d+\.\d+\.\d+\.\d+$/i.test(addr) ? addr.slice(7) : addr;
+}
 
-    // 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} methods
+ * @returns {Set<string>}
+ */
+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 remote || undefined;
+  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
+ * 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;
 }
 
 /**
@@ -135,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;
   }
@@ -153,7 +230,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;
     }
   }
@@ -185,7 +262,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';
@@ -266,7 +343,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');
     }
   }

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<RegExp, RegExp>} */
+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;
+}