diff --git a/.changeset/rate-limiter-continues-after-hit.md b/.changeset/rate-limiter-continues-after-hit.md new file mode 100644 index 00000000..dfdeb57b --- /dev/null +++ b/.changeset/rate-limiter-continues-after-hit.md @@ -0,0 +1,10 @@ +--- +"nostream": patch +--- + +fix: stop checking additional rate limit windows once a client is already rate-limited + +`isRateLimited()` in `EventMessageHandler` and `WebSocketAdapter` looped through every +configured rate limit window even after one had already tripped, calling `rateLimiter.hit()` +(a Redis write) for each remaining window. Both now return as soon as the first exceeded +window is found, avoiding redundant Redis writes for clients that are already being limited. diff --git a/src/adapters/web-socket-adapter.ts b/src/adapters/web-socket-adapter.ts index 38655dfb..c11be279 100644 --- a/src/adapters/web-socket-adapter.ts +++ b/src/adapters/web-socket-adapter.ts @@ -236,18 +236,17 @@ export class WebSocketAdapter extends EventEmitter implements IWebSocketAdapter const hit = (period: number, rate: number) => rateLimiter.hit(`${client}:message:${period}`, 1, { period, rate }) - let limited = false for (const { rate, period } of rateLimits) { const isRateLimited = await hit(period, rate) if (isRateLimited) { logger('rate limited %s: %d messages / %d ms exceeded', client, rate, period) - limited = true + return true } } - return limited + return false } private onClientPong() { diff --git a/src/handlers/event-message-handler.ts b/src/handlers/event-message-handler.ts index 5ac88b71..a3f12d7c 100644 --- a/src/handlers/event-message-handler.ts +++ b/src/handlers/event-message-handler.ts @@ -351,7 +351,6 @@ export class EventMessageHandler implements IMessageHandler { return rateLimiter.hit(key, 1, { period, rate }) } - let limited = false for (const { rate, period, kinds } of rateLimits) { // skip if event kind does not apply if (Array.isArray(kinds) && !kinds.some(isEventKindOrRangeMatch(event))) { @@ -363,11 +362,11 @@ export class EventMessageHandler implements IMessageHandler { if (isRateLimited) { logger('rate limited %s: %d events / %d ms exceeded', event.pubkey, rate, period) - limited = true + return true } } - return limited + return false } protected async isUserAdmitted(event: Event): Promise { diff --git a/test/unit/adapters/web-socket-adapter.spec.ts b/test/unit/adapters/web-socket-adapter.spec.ts index f3d48066..9436e75d 100644 --- a/test/unit/adapters/web-socket-adapter.spec.ts +++ b/test/unit/adapters/web-socket-adapter.spec.ts @@ -423,6 +423,34 @@ describe('WebSocketAdapter', () => { expect(client.send).to.have.been.called }) + it('stops hitting subsequent rate limit windows once one is exceeded', async () => { + client.readyState = WebSocket.OPEN + + const hitStub = sandbox.stub().resolves(true) + + settingsFactory.returns({ + network: { remoteIpHeader: '' }, + limits: { + message: { + rateLimits: [ + { period: 60000, rate: 1 }, + { period: 180, rate: 3 }, + ], + ipWhitelist: [], + }, + }, + }) + + slidingWindowRateLimiter.returns({ hit: hitStub }) + + const messageCall = client.on.getCalls().find((call: any) => call.args[0] === 'message') + const onMessage = messageCall.args[1] + + await onMessage(Buffer.from(JSON.stringify(['EVENT', {}]))) + + expect(hitStub).to.have.been.calledOnce + }) + it('does not rate limit when no rateLimits are configured', async () => { client.readyState = WebSocket.OPEN diff --git a/test/unit/handlers/event-message-handler.spec.ts b/test/unit/handlers/event-message-handler.spec.ts index 14c894da..750878e8 100644 --- a/test/unit/handlers/event-message-handler.spec.ts +++ b/test/unit/handlers/event-message-handler.spec.ts @@ -1022,6 +1022,26 @@ describe('EventMessageHandler', () => { ) expect(actualResult).to.be.true }) + + it('stops hitting subsequent rate limit windows once one is exceeded', async () => { + eventLimits.rateLimits = [ + { + period: 60000, + rate: 1, + }, + { + period: 180, + rate: 3, + }, + ] + + rateLimiterHitStub.onFirstCall().resolves(true) + + const actualResult = await (handler as any).isRateLimited(event) + + expect(rateLimiterHitStub).to.have.been.calledOnce + expect(actualResult).to.be.true + }) }) describe('isUserAdmitted', () => {