From 27fc7688439014f54f263ee066ad143ed0a127b5 Mon Sep 17 00:00:00 2001 From: Erosenin2 <179307300+Erosenin2@users.noreply.github.com> Date: Tue, 29 Sep 2026 16:04:11 -0700 Subject: [PATCH 1/2] fix(rest): bound rate-limit buckets and use verified JWT identity --- src/middleware/requireAuth.ts | 21 +++- src/middleware/restRateLimit.test.ts | 164 ++++++++++++++++++++++++--- src/middleware/restRateLimit.ts | 65 ++++++++++- 3 files changed, 226 insertions(+), 24 deletions(-) diff --git a/src/middleware/requireAuth.ts b/src/middleware/requireAuth.ts index 6f57f73..c2984c1 100644 --- a/src/middleware/requireAuth.ts +++ b/src/middleware/requireAuth.ts @@ -19,6 +19,10 @@ export interface ResolvedRequestUserId { error?: UnauthorizedError; } +export interface ResolvedRequestJwtUserId extends ResolvedRequestUserId { + subject?: string; +} + /** * Compute the HMAC-SHA256 signature for a forwarded user identity. */ @@ -67,7 +71,8 @@ export function verifyGatewaySignature( }); } -export function resolveRequestUserId(req: Request): ResolvedRequestUserId { +/** Resolve only cryptographically verified JWT claims, never forwarded headers. */ +export function resolveRequestJwtUserId(req: Request): ResolvedRequestJwtUserId { const authHeader = req.header("authorization"); if (authHeader !== undefined) { if (!authHeader.startsWith("Bearer ")) { @@ -117,7 +122,10 @@ export function resolveRequestUserId(req: Request): ResolvedRequestUserId { }; } - return { userId: uid }; + const subject = typeof payload.sub === "string" && payload.sub.trim() !== "" + ? payload.sub + : undefined; + return { userId: uid, subject }; } catch (err) { const code = err instanceof jwt.TokenExpiredError @@ -136,6 +144,15 @@ export function resolveRequestUserId(req: Request): ResolvedRequestUserId { } } + return {}; +} + +export function resolveRequestUserId(req: Request): ResolvedRequestUserId { + if (req.header("authorization") !== undefined) { + const result = resolveRequestJwtUserId(req); + return result.userId ? { userId: result.userId } : result; + } + // Only accept x-user-id if TRUST_FORWARDED_USER_ID is explicitly enabled AND a valid internal gateway signature is present const trustForwardedUserId = process.env.TRUST_FORWARDED_USER_ID === "true"; if (trustForwardedUserId) { diff --git a/src/middleware/restRateLimit.test.ts b/src/middleware/restRateLimit.test.ts index 07e31f5..58651fe 100644 --- a/src/middleware/restRateLimit.test.ts +++ b/src/middleware/restRateLimit.test.ts @@ -1,9 +1,10 @@ import express from 'express'; import request from 'supertest'; +import jwt from 'jsonwebtoken'; import { errorHandler } from './errorHandler.js'; -import { InMemoryRestRateLimiter, createRestRateLimitMiddleware } from './restRateLimit.js'; +import { InMemoryRestRateLimiter, createRestRateLimitMiddleware, getRestRateLimitKey } from './restRateLimit.js'; import { requireAuth, type AuthenticatedLocals } from './requireAuth.js'; -import { TEST_JWT_SECRET, signTestToken } from '../../tests/helpers/jwt.js'; +import { TEST_JWT_SECRET, signTestToken, createTestGatewaySignature } from '../../tests/helpers/jwt.js'; function buildProtectedApp() { const app = express(); @@ -43,9 +44,9 @@ describe('restRateLimit middleware', () => { test('returns 429 with Retry-After after the per-user limit is exceeded', async () => { const app = buildProtectedApp(); - await request(app).get('/protected').set('x-user-id', 'user-1').expect(200); - await request(app).get('/protected').set('x-user-id', 'user-1').expect(200); - const response = await request(app).get('/protected').set('x-user-id', 'user-1'); + await request(app).get('/protected').set('Authorization', `Bearer ${signTestToken({ userId: 'user-1' })}`).expect(200); + await request(app).get('/protected').set('Authorization', `Bearer ${signTestToken({ userId: 'user-1' })}`).expect(200); + const response = await request(app).get('/protected').set('Authorization', `Bearer ${signTestToken({ userId: 'user-1' })}`); expect(response.status).toBe(429); expect(response.body.code).toBe('TOO_MANY_REQUESTS'); @@ -58,16 +59,16 @@ describe('restRateLimit middleware', () => { test('tracks limits separately per authenticated user id', async () => { const app = buildProtectedApp(); - await request(app).get('/protected').set('x-user-id', 'user-1').expect(200); - await request(app).get('/protected').set('x-user-id', 'user-1').expect(200); - await request(app).get('/protected').set('x-user-id', 'user-2').expect(200); - await request(app).get('/protected').set('x-user-id', 'user-2').expect(200); + await request(app).get('/protected').set('Authorization', `Bearer ${signTestToken({ userId: 'user-1' })}`).expect(200); + await request(app).get('/protected').set('Authorization', `Bearer ${signTestToken({ userId: 'user-1' })}`).expect(200); + await request(app).get('/protected').set('Authorization', `Bearer ${signTestToken({ userId: 'user-2' })}`).expect(200); + await request(app).get('/protected').set('Authorization', `Bearer ${signTestToken({ userId: 'user-2' })}`).expect(200); - await request(app).get('/protected').set('x-user-id', 'user-1').expect(429); - await request(app).get('/protected').set('x-user-id', 'user-2').expect(429); + await request(app).get('/protected').set('Authorization', `Bearer ${signTestToken({ userId: 'user-1' })}`).expect(429); + await request(app).get('/protected').set('Authorization', `Bearer ${signTestToken({ userId: 'user-2' })}`).expect(429); }); - test('shares the same bucket across valid auth methods for the same user id', async () => { + test('shares the same bucket across JWTs for the same verified user id', async () => { const app = buildProtectedApp(); const token = signTestToken({ userId: 'user-1', @@ -75,7 +76,7 @@ describe('restRateLimit middleware', () => { }); await request(app).get('/protected').set('Authorization', `Bearer ${token}`).expect(200); - await request(app).get('/protected').set('x-user-id', 'user-1').expect(200); + await request(app).get('/protected').set('Authorization', `Bearer ${signTestToken({ userId: 'user-1' })}`).expect(200); const response = await request(app).get('/protected').set('Authorization', `Bearer ${token}`); expect(response.status).toBe(429); @@ -99,9 +100,9 @@ describe('restRateLimit middleware', () => { test('retryAfterMs is consistent with Retry-After header (within same second)', async () => { const app = buildProtectedApp(); - await request(app).get('/protected').set('x-user-id', 'user-boundary').expect(200); - await request(app).get('/protected').set('x-user-id', 'user-boundary').expect(200); - const response = await request(app).get('/protected').set('x-user-id', 'user-boundary'); + await request(app).get('/protected').set('Authorization', `Bearer ${signTestToken({ userId: 'user-boundary' })}`).expect(200); + await request(app).get('/protected').set('Authorization', `Bearer ${signTestToken({ userId: 'user-boundary' })}`).expect(200); + const response = await request(app).get('/protected').set('Authorization', `Bearer ${signTestToken({ userId: 'user-boundary' })}`); expect(response.status).toBe(429); const retryAfterMs: number = response.body.retryAfterMs; @@ -170,3 +171,134 @@ describe('InMemoryRestRateLimiter.peek', () => { expect(limiter.peek('elapsing-key', now + 1000)).toEqual({ allowed: true }); }); }); + + +describe('REST verified identity', () => { + const originalEnv = { ...process.env }; + beforeEach(() => { process.env.JWT_SECRET = TEST_JWT_SECRET; }); + afterEach(() => { process.env = { ...originalEnv }; }); + + function key(headers: Record) { + return getRestRateLimitKey({ + header: (name: string) => headers[name], + ip: '192.0.2.1', + } as express.Request); + } + + test('ignores rotated forwarded identities even with gateway trust enabled', async () => { + process.env.TRUST_FORWARDED_USER_ID = 'true'; + process.env.FORWARDED_USER_ID_SECRET = 'gateway-test-secret'; + const app = express(); + app.use(createRestRateLimitMiddleware({ windowMs: 60_000, maxRequests: 2 })); + app.get('/public', (_req, res) => { res.sendStatus(200); }); + await request(app).get('/public').set('x-user-id', 'one').expect(200); + await request(app).get('/public').set('x-user-id', 'two').expect(200); + await request(app).get('/public').set('x-user-id', 'three').expect(429); + expect(key({ 'x-user-id': 'one' })).toBe('ip:192.0.2.1'); + for (const userId of ['one', 'two']) { + expect(key({ + 'x-user-id': userId, + 'x-gateway-signature': createTestGatewaySignature(userId, 'gateway-test-secret'), + })).toBe('ip:192.0.2.1'); + } + }); + + test('rotating headers and JWT userId claims cannot reset a verified subject bucket', async () => { + const app = buildProtectedApp(); + for (const [index, status] of [200, 200, 429].entries()) { + const token = jwt.sign({ sub: 'same-subject', userId: `alias-${index}` }, TEST_JWT_SECRET); + await request(app).get('/protected') + .set('Authorization', `Bearer ${token}`) + .set('x-user-id', `spoof-${index}`) + .expect(status); + } + }); + + test('prefers the verified subject and ignores unsigned identity sources', () => { + const token = jwt.sign({ sub: 'subject', userId: 'legacy' }, TEST_JWT_SECRET); + expect(key({ authorization: `Bearer ${token}`, 'x-user-id': 'spoof' })).toBe('user:subject'); + expect(key({ authorization: `Bearer ${signTestToken({ userId: 'legacy' })}` })).toBe('user:legacy'); + }); + + test.each([ + ['forged', jwt.sign({ sub: 'forged' }, 'wrong-secret')], + ['expired', jwt.sign({ sub: 'expired' }, TEST_JWT_SECRET, { expiresIn: -1 })], + ['inactive', jwt.sign({ sub: 'inactive' }, TEST_JWT_SECRET, { notBefore: 60 })], + ['wrong algorithm', jwt.sign({ sub: 'wrong-alg' }, TEST_JWT_SECRET, { algorithm: 'HS384' })], + ['missing claims', jwt.sign({}, TEST_JWT_SECRET)], + ['malformed', 'garbage'], + ])('falls back to IP for %s JWTs', (_name, token) => { + expect(key({ authorization: `Bearer ${token}`, 'x-user-id': 'spoof' })).toBe('ip:192.0.2.1'); + }); + + test('missing signing secret falls back to IP', () => { + delete process.env.JWT_SECRET; + expect(key({ authorization: `Bearer ${signTestToken({ userId: 'one' })}` })).toBe('ip:192.0.2.1'); + }); +}); + +describe('REST bucket eviction', () => { + let limiter: InMemoryRestRateLimiter; + beforeEach(() => { jest.useFakeTimers(); jest.setSystemTime(100_000); }); + afterEach(() => { limiter?.dispose(); jest.useRealTimers(); }); + + test('bounds storage throughout a 100k unique-key load', () => { + limiter = new InMemoryRestRateLimiter(1000, 2); + for (let i = 0; i < 100_000; i++) { + if (!limiter.check(`user:${i}`).allowed) throw new Error('new key unexpectedly denied'); + if (limiter.size > 10_000) throw new Error('bucket bound exceeded'); + } + expect(limiter.size).toBe(10_000); + expect(limiter.check('user:99999').allowed).toBe(true); + expect(limiter.check('user:99999').allowed).toBe(false); + }); + + test('evicts the least recently checked key, retaining active exhausted buckets', () => { + limiter = new InMemoryRestRateLimiter(1000, 1, 2); + limiter.check('a'); + limiter.check('b'); + expect(limiter.check('a').allowed).toBe(false); + limiter.check('c'); + expect(limiter.size).toBe(2); + expect(limiter.peek('a').allowed).toBe(false); + expect(limiter.peek('b').allowed).toBe(true); + expect(limiter.peek('c').allowed).toBe(false); + }); + + test('prunes idle staggered buckets within one window after expiry without traffic', () => { + limiter = new InMemoryRestRateLimiter(1000, 1); + limiter.check('first'); + jest.advanceTimersByTime(1); + limiter.check('second'); + jest.advanceTimersByTime(999); + expect(limiter.size).toBe(1); + expect(limiter.peek('second').allowed).toBe(false); + jest.advanceTimersByTime(1000); + expect(limiter.size).toBe(0); + expect(jest.getTimerCount()).toBe(0); + }); + + test('expired peek deletes a bucket without consuming quota', () => { + limiter = new InMemoryRestRateLimiter(1000, 1); + limiter.check('a'); + expect(limiter.peek('a', Date.now() + 1000)).toEqual({ allowed: true }); + expect(limiter.size).toBe(0); + }); + + test('reset clears timers and allows reuse; dispose releases all state', () => { + limiter = new InMemoryRestRateLimiter(1000, 1); + limiter.check('a'); + limiter.reset(); + expect(limiter.size).toBe(0); + expect(jest.getTimerCount()).toBe(0); + expect(limiter.check('a').allowed).toBe(true); + expect(jest.getTimerCount()).toBe(1); + limiter.dispose(); + expect(limiter.size).toBe(0); + expect(jest.getTimerCount()).toBe(0); + }); + + test.each([0, -1, 1.5, NaN, Infinity])('rejects invalid bucket capacity %s', (capacity) => { + expect(() => new InMemoryRestRateLimiter(1000, 1, capacity)).toThrow('maxBuckets'); + }); +}); diff --git a/src/middleware/restRateLimit.ts b/src/middleware/restRateLimit.ts index 0ba94e9..12a6632 100644 --- a/src/middleware/restRateLimit.ts +++ b/src/middleware/restRateLimit.ts @@ -1,7 +1,7 @@ import type { NextFunction, Request, RequestHandler, Response } from 'express'; import { config } from '../config/index.js'; import { getClientIp } from '../lib/clientIp.js'; -import { resolveRequestUserId } from './requireAuth.js'; +import { resolveRequestJwtUserId } from './requireAuth.js'; interface RateLimitBucket { count: number; @@ -16,27 +16,72 @@ interface RateLimitCheckResult { export interface RestRateLimitOptions { windowMs: number; maxRequests: number; + maxBuckets?: number; } export class InMemoryRestRateLimiter { private readonly buckets = new Map(); + private cleanupTimer?: ReturnType; constructor( private readonly windowMs: number, private readonly maxRequests: number, - ) {} + private readonly maxBuckets = 10_000, + ) { + for (const [name, value] of Object.entries({ windowMs, maxRequests, maxBuckets })) { + if (!Number.isSafeInteger(value) || value <= 0) { + throw new Error(`${name} must be a positive safe integer.`); + } + } + if (windowMs > 2_147_483_647) { + throw new Error('windowMs exceeds the supported timer interval.'); + } + } + + get size(): number { + return this.buckets.size; + } + + private startCleanup(): void { + if (this.cleanupTimer) return; + this.cleanupTimer = setInterval(() => { + const now = Date.now(); + for (const [key, bucket] of this.buckets) { + if (now >= bucket.resetAt) this.buckets.delete(key); + } + if (this.buckets.size === 0) this.stopCleanup(); + }, this.windowMs); + this.cleanupTimer.unref(); + } + + private stopCleanup(): void { + if (this.cleanupTimer) clearInterval(this.cleanupTimer); + this.cleanupTimer = undefined; + } check(key: string, now = Date.now()): RateLimitCheckResult { const bucket = this.buckets.get(key); if (!bucket || now >= bucket.resetAt) { + this.buckets.delete(key); + // Map insertion order tracks least-recently checked keys. At capacity, + // evict one bucket rather than letting unique identities exhaust memory. + if (this.buckets.size >= this.maxBuckets) { + const oldestKey = this.buckets.keys().next().value; + if (oldestKey !== undefined) this.buckets.delete(oldestKey); + } this.buckets.set(key, { count: 1, resetAt: now + this.windowMs, }); + this.startCleanup(); return { allowed: true }; } + // Keep active clients (including denied requests) in the LRU working set. + this.buckets.delete(key); + this.buckets.set(key, bucket); + if (bucket.count >= this.maxRequests) { return { allowed: false, @@ -52,6 +97,7 @@ export class InMemoryRestRateLimiter { const bucket = this.buckets.get(key); if (!bucket || now >= bucket.resetAt) { + if (bucket) this.buckets.delete(key); return { allowed: true }; } @@ -67,13 +113,20 @@ export class InMemoryRestRateLimiter { reset(): void { this.buckets.clear(); + this.stopCleanup(); + } + + /** Release the cleanup timer when a custom limiter is no longer used. */ + dispose(): void { + this.reset(); } } export function getRestRateLimitKey(req: Request): string { - const { userId } = resolveRequestUserId(req); - if (userId) { - return `user:${userId}`; + const { subject, userId } = resolveRequestJwtUserId(req); + const identity = subject ?? userId; + if (identity) { + return `user:${identity}`; } return `ip:${getClientIp(req)}`; @@ -81,7 +134,7 @@ export function getRestRateLimitKey(req: Request): string { export function createRestRateLimitMiddleware( options: RestRateLimitOptions, - rateLimiter = new InMemoryRestRateLimiter(options.windowMs, options.maxRequests), + rateLimiter = new InMemoryRestRateLimiter(options.windowMs, options.maxRequests, options.maxBuckets), ): RequestHandler { return (req: Request, res: Response, next: NextFunction): void => { const key = getRestRateLimitKey(req); From 4ae2dd40d65505cb7790b44a29413ed239fff4de Mon Sep 17 00:00:00 2001 From: Erosenin2 <179307300+Erosenin2@users.noreply.github.com> Date: Tue, 29 Sep 2026 16:07:41 -0700 Subject: [PATCH 2/2] test(rest): freeze quota clock for Retry-After assertions --- src/middleware/restRateLimit.test.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/middleware/restRateLimit.test.ts b/src/middleware/restRateLimit.test.ts index 58651fe..d371aac 100644 --- a/src/middleware/restRateLimit.test.ts +++ b/src/middleware/restRateLimit.test.ts @@ -31,9 +31,13 @@ describe('restRateLimit middleware', () => { beforeEach(() => { process.env.JWT_SECRET = TEST_JWT_SECRET; + // Freeze the quota clock while keeping HTTP I/O on real timers. Exact + // Retry-After assertions should not depend on request execution speed. + jest.spyOn(Date, 'now').mockReturnValue(Date.now()); }); afterEach(() => { + jest.restoreAllMocks(); if (originalSecret !== undefined) { process.env.JWT_SECRET = originalSecret; } else {