From 9480cc98bb44503648c5e35587b5652abcf52254 Mon Sep 17 00:00:00 2001 From: Anso Date: Mon, 22 Jun 2026 16:54:30 -0400 Subject: [PATCH] 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. --- .../src/__tests__/api-token-hardening.test.ts | 107 ++++++++++++++++++ backend/src/middleware/rateLimiters.ts | 54 ++++++--- 2 files changed, 146 insertions(+), 15 deletions(-) diff --git a/backend/src/__tests__/api-token-hardening.test.ts b/backend/src/__tests__/api-token-hardening.test.ts index 6ba61d68..d43f8e10 100644 --- a/backend/src/__tests__/api-token-hardening.test.ts +++ b/backend/src/__tests__/api-token-hardening.test.ts @@ -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')); + }); +}); diff --git a/backend/src/middleware/rateLimiters.ts b/backend/src/middleware/rateLimiters.ts index 9e18f98c..ba107875 100644 --- a/backend/src/middleware/rateLimiters.ts +++ b/backend/src/middleware/rateLimiters.ts @@ -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'); }