fix(rate-limit): key authenticated requests by verified JWT, not unverified decode (#1412)

The hybrid rate-limit key generator bucketed authenticated requests by a username read from an unverified jwt.decode of the session cookie or Bearer JWT. One source could fragment the per-source cap by rotating a forged JWT with a varying username, minting a fresh bucket per value. Each forged request is still rejected at auth, so this is DoS amplification, not an auth bypass; the same class as the API-token vector hardened earlier. Verify the JWT signature against the cached signing secret before keying by username; forged, expired, or otherwise invalid credentials fall back to per-IP. Enforce strict bearer-over-cookie precedence so a present-but-invalid Bearer cannot be rescued into a valid cookie's bucket. The verification helper fails closed: any error degrades to per-IP rather than throwing out of the key generator.
2026-07-26 11:49:16 +00:00 · 2026-06-22 16:54:30 -04:00
parent 8d9e6574cc
commit 9480cc98bb
2 changed files with 146 additions and 15 deletions
@@ -5,6 +5,9 @@
 *   - rateLimitKeyGenerator: per-token budget only for live tokens; forged,
 *     bad-checksum, revoked, or expired token-shaped bearers fall back to
 *     per-IP keying so they cannot fragment the limiter (H-1).
+ *   - rateLimitKeyGenerator (JWT / cookie): only a signature-verified session
+ *     username is keyed per-user; forged or expired JWTs (bearer or cookie) fall
+ *     back to per-IP, and a present Bearer never defers to the cookie.
 *
 * Token rows are seeded through the shared apiTokenTestHelper and their stored
 * hash is read back from the row, so this suite hashes nothing itself.
@@ -218,3 +221,107 @@ describe('rateLimitKeyGenerator (API token branch)', () => {
    expect(rateLimitKeyGenerator(req)).toBe('user:cookie-user');
  });
 });
+
+describe('rateLimitKeyGenerator (JWT / cookie username branch)', () => {
+  // A signature the server never issued: the baseline test DB seeds
+  // auth_jwt_secret = TEST_JWT_SECRET (vitestGlobalSetup), so a JWT signed with
+  // any other key fails verification and must never yield a per-user bucket.
+  const WRONG_SECRET = 'attacker-controlled-secret';
+  const IP = '198.51.100.9';
+  // The per-IP bucket an unauthenticated request from `ip` gets; a verified-out
+  // credential must collapse to exactly this key.
+  const anonKey = (ip: string) =>
+    rateLimitKeyGenerator({ cookies: {}, headers: {}, ip } as unknown as Request);
+
+  it('falls back to per-IP for a forged Bearer JWT (wrong signature) despite a valid username claim', () => {
+    const forged = jwt.sign({ username: 'victim' }, WRONG_SECRET);
+    const key = rateLimitKeyGenerator(bearerReq(forged));
+    expect(key).not.toMatch(/^user:/);
+    expect(key).toBe(anonKey('203.0.113.7'));
+  });
+
+  it('collapses distinct forged Bearer JWTs from one IP into the anonymous bucket (no username fragmentation)', () => {
+    const forgedFrom = (username: string) =>
+      ({ cookies: {}, headers: { authorization: `Bearer ${jwt.sign({ username }, WRONG_SECRET)}` }, ip: IP } as unknown as Request);
+    const keyA = rateLimitKeyGenerator(forgedFrom('rotated-1'));
+    const keyB = rateLimitKeyGenerator(forgedFrom('rotated-2'));
+    expect(keyA).not.toMatch(/^user:/);
+    expect(keyA).toBe(keyB);
+    expect(keyA).toBe(anonKey(IP));
+  });
+
+  it('falls back to per-IP for a forged session cookie (wrong signature)', () => {
+    const forged = jwt.sign({ username: 'victim' }, WRONG_SECRET);
+    const req = { cookies: { [COOKIE_NAME]: forged }, headers: {}, ip: IP } as unknown as Request;
+    const key = rateLimitKeyGenerator(req);
+    expect(key).not.toMatch(/^user:/);
+    expect(key).toBe(anonKey(IP));
+  });
+
+  it('falls back to per-IP for a validly-signed but expired Bearer JWT', () => {
+    const expired = jwt.sign({ username: 'ci-bot' }, TEST_JWT_SECRET, { expiresIn: -10 });
+    const key = rateLimitKeyGenerator(bearerReq(expired));
+    expect(key).not.toMatch(/^user:/);
+    expect(key).toBe(anonKey('203.0.113.7'));
+  });
+
+  it('falls back to per-IP for a validly-signed but expired session cookie', () => {
+    const expired = jwt.sign({ username: 'cookie-user' }, TEST_JWT_SECRET, { expiresIn: -10 });
+    const req = { cookies: { [COOKIE_NAME]: expired }, headers: {}, ip: IP } as unknown as Request;
+    const key = rateLimitKeyGenerator(req);
+    expect(key).not.toMatch(/^user:/);
+    expect(key).toBe(anonKey(IP));
+  });
+
+  it('does not let a valid session cookie rescue a forged Bearer JWT (bearer precedence)', () => {
+    // auth uses bearerToken || cookieToken, so a present-but-invalid Bearer is the
+    // credential auth rejects; the limiter must not key it as the cookie's user.
+    const validCookie = jwt.sign({ username: 'real-user' }, TEST_JWT_SECRET);
+    const forgedBearer = jwt.sign({ username: 'attacker' }, WRONG_SECRET);
+    const req = {
+      cookies: { [COOKIE_NAME]: validCookie },
+      headers: { authorization: `Bearer ${forgedBearer}` },
+      ip: IP,
+    } as unknown as Request;
+    const key = rateLimitKeyGenerator(req);
+    expect(key).not.toMatch(/^user:/);
+    expect(key).toBe(anonKey(IP));
+  });
+
+  it('does not let a valid session cookie rescue an expired Bearer JWT (bearer precedence)', () => {
+    const validCookie = jwt.sign({ username: 'real-user' }, TEST_JWT_SECRET);
+    const expiredBearer = jwt.sign({ username: 'real-user' }, TEST_JWT_SECRET, { expiresIn: -10 });
+    const req = {
+      cookies: { [COOKIE_NAME]: validCookie },
+      headers: { authorization: `Bearer ${expiredBearer}` },
+      ip: IP,
+    } as unknown as Request;
+    const key = rateLimitKeyGenerator(req);
+    expect(key).not.toMatch(/^user:/);
+    expect(key).toBe(anonKey(IP));
+  });
+
+  it('keys a valid Bearer JWT by its own username even when a valid cookie for another user is present (bearer precedence)', () => {
+    // The positive mirror: auth would use the Bearer, so the limiter must key the
+    // Bearer user, never the unrelated cookie user. Guards against a refactor that
+    // consults the cookie before the verified Bearer.
+    const validBearer = jwt.sign({ username: 'bearer-user' }, TEST_JWT_SECRET);
+    const validCookie = jwt.sign({ username: 'cookie-user' }, TEST_JWT_SECRET);
+    const req = {
+      cookies: { [COOKIE_NAME]: validCookie },
+      headers: { authorization: `Bearer ${validBearer}` },
+      ip: IP,
+    } as unknown as Request;
+    expect(rateLimitKeyGenerator(req)).toBe('user:bearer-user');
+  });
+
+  it('fails closed to per-IP when the JWT signing secret is unset', () => {
+    // The explicit no-secret branch in verifiedJwtPayload: a validly-signed token
+    // must still collapse to per-IP rather than throw or grant a per-user bucket.
+    vi.spyOn(DatabaseService.getInstance(), 'getGlobalSettings').mockReturnValue({});
+    const validBearer = jwt.sign({ username: 'ci-bot' }, TEST_JWT_SECRET);
+    const key = rateLimitKeyGenerator(bearerReq(validBearer));
+    expect(key).not.toMatch(/^user:/);
+    expect(key).toBe(anonKey('203.0.113.7'));
+  });
+});
@@ -5,6 +5,7 @@ import { COOKIE_NAME } from '../helpers/constants';
 import { WEBHOOK_TRIGGER_RE } from '../helpers/routePatterns';
 import { looksLikeApiToken } from '../utils/apiTokenFormat';
 import { validateApiToken } from '../utils/apiTokenAuth';
+import { DatabaseService } from '../services/DatabaseService';

 // ── Rate Limiting ─────────────────────────────────────────────────────────────
 //
@@ -62,14 +63,36 @@ function isNodeProxyRequest(req: Request): boolean {
  }
 }

+/**
+ * Verify a session/bearer JWT against the cached signing secret and return its
+ * payload, or null when the secret is unset, the signature or expiry is invalid,
+ * or the settings read itself fails. `getGlobalSettings()` is a frozen in-memory
+ * cache (no per-call DB hit) and `jwt.verify` costs microseconds, so this is safe
+ * on the rate-limiter hot path. Verifying here is what stops one source from
+ * fragmenting the limiter by rotating a forged JWT with a varying username: a
+ * credential that does not verify yields no per-user key. Fails closed: any error
+ * returns null, so the caller degrades to per-IP keying rather than throwing out
+ * of the key generator.
+ */
+function verifiedJwtPayload(token: string): { username?: string; sub?: string } | null {
+  try {
+    const secret = DatabaseService.getInstance().getGlobalSettings().auth_jwt_secret;
+    if (!secret) return null;
+    return jwt.verify(token, secret) as { username?: string; sub?: string };
+  } catch {
+    return null;
+  }
+}
+
 /**
 * Hybrid rate limit key: per-token / per-user for authenticated requests, IP
 * otherwise. Mirrors authMiddleware's bearer-over-cookie precedence (auth.ts
 * uses `bearerToken || cookieToken`) so the limiter keys off the same credential
- * auth will use. Checking the cookie first would let a request authenticated by
- * a Bearer API token be bucketed under an unrelated (or forged) cookie username,
- * sidestepping the per-token / per-IP keying. `jwt.decode()` avoids
- * double-verification; `authMiddleware` handles signature checks downstream.
+ * auth will use, and a JWT is trusted only once its signature verifies. When a
+ * Bearer header is present the cookie is never consulted (auth ignores it too),
+ * so a forged or expired Bearer cannot be rescued into a valid cookie's bucket.
+ * Anything that does not resolve to a live token or a verified username falls
+ * back to per-IP keying.
 */
 export function rateLimitKeyGenerator(req: Request): string {
  const auth = req.headers.authorization;
@@ -82,8 +105,8 @@ export function rateLimitKeyGenerator(req: Request): string {
    // that is not a live token therefore falls through to per-IP keying. The
    // validated row is memoized on the request so authMiddleware reuses it
    // without a second lookup (and a request crossing two limiters reuses it
-    // here too). Like the jwt.decode branches, a lookup failure degrades to
-    // per-IP keying rather than throwing out of the key generator.
+    // here too). Like the verified-JWT branches below, a validation failure
+    // degrades to per-IP keying rather than throwing out of the key generator.
    if (looksLikeApiToken(bearer)) {
      if (req._apiToken) return `user:sk:${req._apiToken.token_hash.slice(0, 16)}`;
      try {
@@ -97,18 +120,19 @@ export function rateLimitKeyGenerator(req: Request): string {
      } catch { /* fall through to IP */ }
      return ipKeyGenerator(req.ip || 'unknown');
    }
-    try {
-      const decoded = jwt.decode(bearer) as { username?: string; sub?: string } | null;
-      if (decoded?.username) return `user:${decoded.username}`;
-      if (decoded?.sub) return `user:${decoded.sub}`;
-    } catch { /* fall through to cookie / IP */ }
+    // Non-API Bearer JWT: key by the VERIFIED username (sub is a legacy
+    // fallback, now signature-gated, kept only for back-compat). A Bearer header
+    // present means auth ignores the cookie, so an unverified Bearer returns
+    // per-IP here rather than falling through to the cookie branch below.
+    const payload = verifiedJwtPayload(bearer);
+    if (payload?.username) return `user:${payload.username}`;
+    if (payload?.sub) return `user:${payload.sub}`;
+    return ipKeyGenerator(req.ip || 'unknown');
  }
  const cookie = req.cookies?.[COOKIE_NAME];
  if (cookie) {
-    try {
-      const decoded = jwt.decode(cookie) as { username?: string } | null;
-      if (decoded?.username) return `user:${decoded.username}`;
-    } catch { /* fall through to IP */ }
+    const payload = verifiedJwtPayload(cookie);
+    if (payload?.username) return `user:${payload.username}`;
  }
  return ipKeyGenerator(req.ip || 'unknown');
 }