Skip to content

fix(rest): bound buckets and rate limit verified JWT identities #1389

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Erosenin2 wants to merge 2 commits into
base: main
Choose a base branch
from
+230 −24
Open

fix(rest): bound buckets and rate limit verified JWT identities #1389

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
27fc768
fix(rest): bound rate-limit buckets and use verified JWT identity
Erosenin2 Sep 29, 2026
4ae2dd4
test(rest): freeze quota clock for Retry-After assertions
Erosenin2 Sep 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 19 additions & 2 deletions src/middleware/requireAuth.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*/
Expand Down Expand Up @@ -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 ")) {
Expand Down Expand Up @@ -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
Expand All @@ -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) {
Expand Down
168 changes: 152 additions & 16 deletions src/middleware/restRateLimit.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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();
Expand All @@ -30,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 {
Expand All @@ -43,9 +48,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');
Expand All @@ -58,24 +63,24 @@ 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',
walletAddress: 'GDTEST123STELLAR',
});

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);
Expand All @@ -99,9 +104,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;
Expand Down Expand Up @@ -170,3 +175,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<string, string>) {
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');
});
});
65 changes: 59 additions & 6 deletions src/middleware/restRateLimit.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -16,27 +16,72 @@ interface RateLimitCheckResult {
export interface RestRateLimitOptions {
windowMs: number;
maxRequests: number;
maxBuckets?: number;
}

export class InMemoryRestRateLimiter {
private readonly buckets = new Map<string, RateLimitBucket>();
private cleanupTimer?: ReturnType<typeof setInterval>;

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,
Expand All @@ -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 };
}

Expand All @@ -67,21 +113,28 @@ 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)}`;
}

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);
Expand Down