diff --git a/.changeset/rate-limit-wire-correctness.md b/.changeset/rate-limit-wire-correctness.md new file mode 100644 index 000000000..7d8f8e909 --- /dev/null +++ b/.changeset/rate-limit-wire-correctness.md @@ -0,0 +1,15 @@ +--- +'nostream': patch +--- + +fix(rate-limit): send the backoff the strategy actually means, name an outage as an error, and bucket messages by type + +Three defects on one wire path, from the spike in #1004. + +**The EWMA backoff was a half-life, sent as a delay.** `RateLimit.period` is the decay half-life under `ewma`, so the time until a limit lifts is `period · log2(R_new / rate)`, not `period`. With `rate: 6, period: 60000`, a client four times over the limit was told to wait 60 s and needed 120 s. The Lua script now returns its score instead of a yes/no, and the limiter returns `period · log2(score / rate)`. It was only accidentally safe before: the hash carried a TTL of exactly `period`, so a quiet client's state expired anyway — an unstated coupling that would break the moment anyone lengthened the TTL to keep state warm. + +**A limiter outage was announced as `rate-limited`.** A failing backend made every connected client back off for a fault on this side. It now answers `error: rate limiter unavailable` and still rejects the event; fail-closed behaviour is unchanged. + +**The message bucket ignored the message type.** One bucket per client meant a `REQ` could be refused because its `EVENT`s had filled it — the wrong message throttled, and the wrong one told to back off. The bucket key now carries the type, read from a bounded prefix of the frame because the limiter runs before validation. + +`IRateLimiter.hit` returns `IRateLimiterResult` (`limited`, plus `retryAfterMs` when the strategy can estimate one) so each strategy answers in its own terms; a caller with no estimate keeps sending the period. diff --git a/src/@types/utils.ts b/src/@types/utils.ts index 5e8674223..e6c0d07b7 100644 --- a/src/@types/utils.ts +++ b/src/@types/utils.ts @@ -3,6 +3,19 @@ export interface IRateLimiterOptions { rate: number } +/** + * The outcome of one limiter hit. + * + * `retryAfterMs` is an estimate of how long the caller must wait before its + * next attempt can be accepted, and it is absent when the strategy cannot say + * so. Absence means "limited, no estimate" — it must never be read as "retry + * immediately", and the caller decides what to send instead. + */ +export interface IRateLimiterResult { + limited: boolean + retryAfterMs?: number +} + export interface IRateLimiter { - hit(key: string, step: number, options: IRateLimiterOptions): Promise + hit(key: string, step: number, options: IRateLimiterOptions): Promise } diff --git a/src/adapters/web-socket-adapter.ts b/src/adapters/web-socket-adapter.ts index 6ac493849..8befbe7aa 100644 --- a/src/adapters/web-socket-adapter.ts +++ b/src/adapters/web-socket-adapter.ts @@ -35,6 +35,36 @@ import { Settings } from '../@types/settings' import { SocketAddress } from 'net' const logger = createLogger('web-socket-adapter') + +/** + * The NIP-01 type of a client frame, or `OTHER` when it is not one. + * + * The message limiter runs before validation — it has to, or a client could + * spend the parse budget first — so it reads a bounded prefix instead of + * decoding the frame. A frame that does not start with `["WORD"` shares the + * `OTHER` bucket; it is refused a moment later anyway. + */ +const THROTTLED_MESSAGE_TYPES = new Set([ + 'REQ', + 'EVENT', + 'COUNT', + 'CLOSE', + 'AUTH', + 'NEG-OPEN', + 'NEG-MSG', + 'NEG-CLOSE', +]) + +const messageType = (raw: Buffer): string => { + const prefix = raw.toString('utf8', 0, Math.min(raw.length, 32)) + const match = /^\s*\[\s*"([A-Za-z-]{1,8})"/.exec(prefix) + const type = match ? match[1].toUpperCase() : '' + + // A closed set, not whatever the frame claims. Bucketing by an arbitrary word + // would let a client rotate invented types (`["AAAA"]`, `["BBBB"]`, …) and + // keep every bucket empty; anything unrecognised shares one bucket. + return THROTTLED_MESSAGE_TYPES.has(type) ? type : 'OTHER' +} const debugHeartbeat = logger.extend('heartbeat') const abortableMessageHandlers: WeakMap = new WeakMap() @@ -218,7 +248,7 @@ export class WebSocketAdapter extends EventEmitter implements IWebSocketAdapter let abortable = false let messageHandler: (IMessageHandler & IAbortable) | undefined = undefined try { - const retryAfterMs = await this.isRateLimited(this.clientAddress.address) + const retryAfterMs = await this.isRateLimited(this.clientAddress.address, messageType(raw)) if (retryAfterMs) { this.sendMessage(this.createRateLimitedMessage(raw, retryAfterMs)) return @@ -276,10 +306,15 @@ export class WebSocketAdapter extends EventEmitter implements IWebSocketAdapter } /** - * Resolves to false when allowed; otherwise to the tripped window's period in ms, - * sent as the NIP-01 `rate-limited` backoff. + * Resolves to false when allowed; otherwise to the backoff in ms, sent as the + * NIP-01 `rate-limited` parameter. + * + * The bucket carries the message type: a client told to back off should back + * off *the message that tripped the limit*. One shared bucket refused a REQ + * because its EVENTs had filled it, which is the wrong message to punish and + * the wrong one to throttle. */ - private async isRateLimited(client: string): Promise { + private async isRateLimited(client: string, type: string): Promise { const { rateLimits, ipWhitelist = [] } = this.settings().limits?.message ?? {} if (!Array.isArray(rateLimits) || !rateLimits.length || ipWhitelist.includes(client)) { @@ -288,15 +323,16 @@ export class WebSocketAdapter extends EventEmitter implements IWebSocketAdapter const rateLimiter = this.rateLimiter() - const hit = (period: number, rate: number) => rateLimiter.hit(`${client}:message:${period}`, 1, { period, rate }) + const hit = (period: number, rate: number) => + rateLimiter.hit(`${client}:message:${type}:${period}`, 1, { period, rate }) for (const { rate, period } of rateLimits) { - const isRateLimited = await hit(period, rate) + const result = await hit(period, rate) - if (isRateLimited) { - logger('rate limited %s: %d messages / %d ms exceeded', client, rate, period) + if (result.limited) { + logger('rate limited %s (%s): %d messages / %d ms exceeded', client, type, rate, period) - return Math.max(1, Math.ceil(period)) + return result.retryAfterMs ?? Math.max(1, Math.ceil(period)) } } diff --git a/src/controllers/admission/get-admission-check-controller.ts b/src/controllers/admission/get-admission-check-controller.ts index f68e8025a..388325495 100644 --- a/src/controllers/admission/get-admission-check-controller.ts +++ b/src/controllers/admission/get-admission-check-controller.ts @@ -53,7 +53,7 @@ export class GetSubmissionCheckController implements IController { if (Array.isArray(ipWhitelist) && !ipWhitelist.includes(remoteAddress)) { const rateLimiter = this.rateLimiter() for (const { rate, period } of rateLimits) { - if (await rateLimiter.hit(`${remoteAddress}:admission-check:${period}`, 1, { period, rate })) { + if ((await rateLimiter.hit(`${remoteAddress}:admission-check:${period}`, 1, { period, rate })).limited) { logger('rate limited %s: %d in %d milliseconds', remoteAddress, rate, period) limited = true } diff --git a/src/controllers/invoices/post-invoice-controller.ts b/src/controllers/invoices/post-invoice-controller.ts index 0e0751afa..bc803e3c1 100644 --- a/src/controllers/invoices/post-invoice-controller.ts +++ b/src/controllers/invoices/post-invoice-controller.ts @@ -183,7 +183,7 @@ export class PostInvoiceController implements IController { if (Array.isArray(ipWhitelist) && !ipWhitelist.includes(remoteAddress)) { const rateLimiter = this.rateLimiter() for (const { rate, period } of rateLimits) { - if (await rateLimiter.hit(`${remoteAddress}:invoice:${period}`, 1, { period, rate })) { + if ((await rateLimiter.hit(`${remoteAddress}:invoice:${period}`, 1, { period, rate })).limited) { logger('rate limited %s: %d in %d milliseconds', remoteAddress, rate, period) limited = true } diff --git a/src/controllers/subscriptions/get-subscription-status-controller.ts b/src/controllers/subscriptions/get-subscription-status-controller.ts index a8d20f9f8..ad76fe60b 100644 --- a/src/controllers/subscriptions/get-subscription-status-controller.ts +++ b/src/controllers/subscriptions/get-subscription-status-controller.ts @@ -124,7 +124,7 @@ export class GetSubscriptionStatusController implements IController { if (Array.isArray(ipWhitelist) && !ipWhitelist.includes(remoteAddress)) { const rateLimiter = this.rateLimiter() for (const { rate, period } of rateLimits) { - if (await rateLimiter.hit(`${remoteAddress}:subscription-status:${period}`, 1, { period, rate })) { + if ((await rateLimiter.hit(`${remoteAddress}:subscription-status:${period}`, 1, { period, rate })).limited) { logger('rate limited %s: %d in %d milliseconds', remoteAddress, rate, period) limited = true } diff --git a/src/handlers/event-message-handler.ts b/src/handlers/event-message-handler.ts index f342908c7..941892c54 100644 --- a/src/handlers/event-message-handler.ts +++ b/src/handlers/event-message-handler.ts @@ -61,13 +61,26 @@ import { createLogger } from '../factories/logger-factory' import { Factory } from '../@types/base' import { ICacheAdapter } from '../@types/adapters' import { IncomingEventMessage } from '../@types/messages' -import { IRateLimiter } from '../@types/utils' +import { IRateLimiter, IRateLimiterResult } from '../@types/utils' import { IWebSocketAdapter } from '../@types/adapters' import { IWotGraphService } from '../@types/services' import { Nip05Verification } from '../@types/nip05' import { WebSocketAdapterEvent } from '../constants/adapter' import { recordModerationRateLimitHitSafely } from '../utils/moderation-rate-limit-stats' +/** + * Why an event was admitted or refused, in the terms the wire needs. + * + * `limited` always carries a backoff — the strategy's estimate when it has one, + * the period when it does not. `unavailable` is our fault, not the client's, and + * must not be announced as `rate-limited`: every client would back off for a + * backend outage. + */ +type RateLimitVerdict = + | { status: 'allowed' } + | { status: 'limited', retryAfterMs: number } + | { status: 'unavailable' } + const logger = createLogger('event-message-handler') export class EventMessageHandler implements IMessageHandler { @@ -106,17 +119,27 @@ export class EventMessageHandler implements IMessageHandler { event = this.addExpirationMetadata(event) - const rateLimited = await this.isRateLimited(event) - if (rateLimited) { + const rateLimit = await this.isRateLimited(event) + if (rateLimit.status === 'limited') { logger('event %s rejected: rate-limited', event.id) - const retryAfterMs = typeof rateLimited === 'number' ? String(rateLimited) : undefined void recordModerationRateLimitHitSafely(this.cache, { pubkey: event.pubkey, ip: event[ContextMetadataKey]?.remoteAddress?.address, }) this.webSocket.emit( WebSocketAdapterEvent.Message, - createEventCommandResult(event.id, false, 'rate-limited: slow down', retryAfterMs), + createEventCommandResult(event.id, false, 'rate-limited: slow down', String(rateLimit.retryAfterMs)), + ) + return + } + + if (rateLimit.status === 'unavailable') { + // Not the client's fault, so it gets `error:` and no backoff: telling it to + // slow down would make every client back off at once for our outage. + logger('event %s rejected: rate limiter unavailable', event.id) + this.webSocket.emit( + WebSocketAdapterEvent.Message, + createEventCommandResult(event.id, false, 'error: rate limiter unavailable'), ) return } @@ -404,12 +427,17 @@ export class EventMessageHandler implements IMessageHandler { } /** - * Resolves to false when allowed; otherwise to the tripped window's period in ms, - * sent as the NIP-01 `rate-limited` backoff, or true when no backoff can be given. + * Why an event was not admitted, in the terms the wire needs. + * + * `limited` carries the backoff when the strategy can estimate one, and the + * caller sends the period when it cannot. `unavailable` is a different thing: + * the limiter backend failed, so the event is rejected fail-closed, but the + * client must not be told to slow down — every client would back off at once + * for a fault that is ours. */ - protected async isRateLimited(event: Event): Promise { + protected async isRateLimited(event: Event): Promise { if (this.getRelayPublicKey() === event.pubkey) { - return false + return { status: 'allowed' } } const currentSettings = this.settings() @@ -418,7 +446,7 @@ export class EventMessageHandler implements IMessageHandler { const rateLimits = (await this.getPlanRateLimits(event, currentSettings)) ?? currentSettings.limits?.event?.rateLimits if (!rateLimits || !rateLimits.length) { - return false + return { status: 'allowed' } } if ( @@ -426,7 +454,7 @@ export class EventMessageHandler implements IMessageHandler { Array.isArray(whitelists?.pubkeys) && whitelists.pubkeys.includes(event.pubkey) ) { - return false + return { status: 'allowed' } } if ( @@ -434,7 +462,7 @@ export class EventMessageHandler implements IMessageHandler { Array.isArray(whitelists?.ipAddresses) && whitelists.ipAddresses.includes(this.webSocket.getClientAddress()) ) { - return false + return { status: 'allowed' } } const rateLimiter = this.rateLimiter() @@ -457,23 +485,28 @@ export class EventMessageHandler implements IMessageHandler { continue } - let isRateLimited = false + let result: IRateLimiterResult try { - isRateLimited = await hit({ period, rate, kinds }) + result = await hit({ period, rate, kinds }) } catch (error) { - // Fail closed when the rate limiter backend is unavailable. + // Fail closed when the rate limiter backend is unavailable, but say so: + // announcing `rate-limited` here made every connected client back off + // for a fault on this side. logger('rate limiter unavailable for %s (%d/%d): %o', event.pubkey, rate, period, error) - return true + return { status: 'unavailable' } } - if (isRateLimited) { + if (result.limited) { logger('rate limited %s: %d events / %d ms exceeded', event.pubkey, rate, period) - return Math.max(1, Math.ceil(period)) + return { + status: 'limited', + retryAfterMs: result.retryAfterMs ?? Math.max(1, Math.ceil(period)), + } } } - return false + return { status: 'allowed' } } /** The rate limits of the plan the sender is subscribed to, if it has its own and the subscription still gives access. */ diff --git a/src/handlers/request-handlers/rate-limiter-middleware.ts b/src/handlers/request-handlers/rate-limiter-middleware.ts index 24ee74a4e..642bea41e 100644 --- a/src/handlers/request-handlers/rate-limiter-middleware.ts +++ b/src/handlers/request-handlers/rate-limiter-middleware.ts @@ -41,7 +41,7 @@ export async function isRateLimited(remoteAddress: string, settings: Settings): let limited = false for (const { rate, period } of rateLimits) { - const isRateLimited = await hit(period, rate) + const isRateLimited = (await hit(period, rate)).limited if (isRateLimited) { logger('rate limited %s: %d messages / %d ms exceeded', remoteAddress, rate, period) diff --git a/src/handlers/subscribe-message-handler.ts b/src/handlers/subscribe-message-handler.ts index f79621e95..ef5667fb9 100644 --- a/src/handlers/subscribe-message-handler.ts +++ b/src/handlers/subscribe-message-handler.ts @@ -188,7 +188,7 @@ export class SubscribeMessageHandler implements IMessageHandler, IAbortable { for (const { period, rate } of rateLimits) { let isRateLimited: boolean try { - isRateLimited = await rateLimiter.hit(`${pubkey}:invites:${period}`, 1, { period, rate }) + isRateLimited = (await rateLimiter.hit(`${pubkey}:invites:${period}`, 1, { period, rate })).limited } catch (error) { // Fail closed: minting writes to the database, so an unavailable limiter // must not leave the gate open. diff --git a/src/utils/admin-rate-limit.ts b/src/utils/admin-rate-limit.ts index 06772ec81..6fff86f6d 100644 --- a/src/utils/admin-rate-limit.ts +++ b/src/utils/admin-rate-limit.ts @@ -27,7 +27,7 @@ export async function isAdminRateLimited( if (!Array.isArray(ipWhitelist) || !ipWhitelist.includes(remoteAddress)) { const rateLimiter = rateLimiterFactory() for (const { rate, period } of rateLimits) { - if (await rateLimiter.hit(`${remoteAddress}:admin-${scope}:${period}`, 1, { period, rate })) { + if ((await rateLimiter.hit(`${remoteAddress}:admin-${scope}:${period}`, 1, { period, rate })).limited) { logger('rate limited %s: %d in %d milliseconds', remoteAddress, rate, period) limited = true } diff --git a/src/utils/ewma-rate-limiter.ts b/src/utils/ewma-rate-limiter.ts index 51e59f7e6..e36035d72 100644 --- a/src/utils/ewma-rate-limiter.ts +++ b/src/utils/ewma-rate-limiter.ts @@ -1,4 +1,4 @@ -import { IRateLimiter, IRateLimiterOptions } from '../@types/utils' +import { IRateLimiter, IRateLimiterOptions, IRateLimiterResult } from '../@types/utils' import { createLogger } from '../factories/logger-factory' import { ICacheAdapter } from '../@types/adapters' @@ -21,11 +21,10 @@ const rateLimitScript = { redis.call('HSET', key, 'rate', R_new, 'timestamp', timestamp) redis.call('EXPIRE', key, math.ceil(period / 1000)) - if R_new > rate then - return 1 - else - return 0 - end + -- A Lua number reaches the client truncated to an integer, and the score + -- is a float: tostring is what keeps the fraction the caller needs to + -- tell 6.4 hits from 24. + return tostring(R_new) `, } @@ -63,7 +62,7 @@ export class EWMARateLimiter implements IRateLimiter { key: string, step: number, options: IRateLimiterOptions, - ): Promise { + ): Promise { const { rate, period } = options const result = await this.cache.eval(rateLimitScript.SCRIPT, @@ -71,9 +70,42 @@ export class EWMARateLimiter implements IRateLimiter { [Date.now().toString(), rate.toString(), period.toString(), step.toString()] ) - debug('ewma rate limited on %s bucket: %s', key, result ? 'yes' : 'no') + // A null/undefined answer means the script returned nil, which is a protocol + // disagreement, not a score of zero: reading it as "allowed" would silently + // disable the limiter. + if (typeof result !== 'string' && typeof result !== 'number') { + throw new Error(`ewma rate limiter returned no score: ${String(result)}`) + } - return result === 1 + const score = Number(result) + if (!Number.isFinite(score)) { + throw new Error(`ewma rate limiter returned a non-numeric score: ${String(result)}`) + } + + const limited = score > rate + + debug('ewma rate limited on %s bucket: %s (score %d, rate %d)', key, limited ? 'yes' : 'no', score, rate) + + if (!limited) { + return { limited: false } + } + + // `period` is the half-life, not the wait: the score decays as + // `score * 2^(-t/period)`, so it takes `period * log2(score / target)` to + // fall back to `target`. Reporting `period` understates a four-times + // overrun by half (rate 6, score 24, period 60 s: ~136 s, not 60 s). + // + // The target is `rate - step`, not `rate`: the script adds this hit's step + // before comparing, so a score that has decayed to exactly `rate` still + // exceeds it. With no headroom (`rate <= step`) no amount of decay admits + // another hit, and the key's TTL is what eventually clears it. + const headroom = rate - step + return { + limited: true, + retryAfterMs: headroom > 0 + ? Math.max(1, Math.ceil(period * Math.log2(score / headroom))) + : Math.max(1, Math.ceil(period)), + } } } diff --git a/src/utils/sliding-window-rate-limiter.ts b/src/utils/sliding-window-rate-limiter.ts index c91efae48..e9f8ec609 100644 --- a/src/utils/sliding-window-rate-limiter.ts +++ b/src/utils/sliding-window-rate-limiter.ts @@ -1,4 +1,4 @@ -import { IRateLimiter, IRateLimiterOptions } from '../@types/utils' +import { IRateLimiter, IRateLimiterOptions, IRateLimiterResult } from '../@types/utils' import { createLogger } from '../factories/logger-factory' import { ICacheAdapter } from '../@types/adapters' @@ -50,7 +50,7 @@ export class SlidingWindowRateLimiter implements IRateLimiter { private readonly cache: ICacheAdapter, ) { } - public async hit(key: string, step: number, options: IRateLimiterOptions): Promise { + public async hit(key: string, step: number, options: IRateLimiterOptions): Promise { const timestamp = Date.now() const { period, rate } = options @@ -65,6 +65,13 @@ export class SlidingWindowRateLimiter implements IRateLimiter { logger('hit on %s bucket: is rate limited? %s', key, isRateLimited) - return isRateLimited + if (!isRateLimited) { + return { limited: false } + } + + // The window is the honest upper bound: the oldest hit ages out at most one + // period from now, and reading the sorted set's first score to narrow that + // down is a second round trip on the hot path. + return { limited: true, retryAfterMs: period } } } diff --git a/test/integration/features/rate-limiter/rate-limiter.ts b/test/integration/features/rate-limiter/rate-limiter.ts index 8c4e3b4b3..a4c8878cc 100644 --- a/test/integration/features/rate-limiter/rate-limiter.ts +++ b/test/integration/features/rate-limiter/rate-limiter.ts @@ -26,9 +26,10 @@ Given(/(\w+)'s message rate is already at the limit/, async function(name: strin const ws = this.parameters.clients[name] as WebSocket const address = (ws as any)._socket?.remoteAddress ?? '::1' const period = 60000 - // EWMA reads its own namespace; seeding the un-namespaced key would leave the - // limiter on a fresh window and the scenario would not rate limit at all. - const key = ewmaKey(`${address}:message:${period}`) + // EWMA reads its own namespace, and the message bucket carries the message + // type (a REQ is not refused because its EVENTs filled the bucket), so the + // seed has to name both: `:message:EVENT:` rather than `:message:`. + const key = ewmaKey(`${address}:message:EVENT:${period}`) await testCacheClient.hSet(key, { rate: '999', @@ -62,8 +63,10 @@ Given(/(\w+)'s Redis holds a sliding-window key left by an older version/, async const address = (ws as any)._socket?.remoteAddress ?? '::1' const period = 60000 // Exactly what a 2.x relay, or a sliding_window run, leaves behind: a sorted - // set under the key name EWMA used to share. Reading it as a hash raised - // WRONGTYPE on every message, which is the regression this guards. + // set under the un-namespaced, un-typed key name. EWMA reads + // `:message:EVENT:` behind its own prefix, so this data is + // inert rather than fatal — and reading it as a hash used to raise WRONGTYPE + // on every message, which is the regression this guards. const key = `${address}:message:${period}` await testCacheClient.del(key) diff --git a/test/unit/adapters/web-socket-adapter.spec.ts b/test/unit/adapters/web-socket-adapter.spec.ts index 751094dbe..76bc776a5 100644 --- a/test/unit/adapters/web-socket-adapter.spec.ts +++ b/test/unit/adapters/web-socket-adapter.spec.ts @@ -576,7 +576,7 @@ describe('WebSocketAdapter', () => { }) slidingWindowRateLimiter.returns({ - hit: sandbox.stub().resolves(true), + hit: sandbox.stub().resolves({ limited: true }), }) onMessage = client.on.getCalls().find((call: any) => call.args[0] === 'message').args[1] @@ -611,8 +611,8 @@ describe('WebSocketAdapter', () => { } const hitStub = sandbox.stub() - hitStub.onFirstCall().resolves(false) - hitStub.resolves(true) + hitStub.onFirstCall().resolves({ limited: false }) + hitStub.resolves({ limited: true }) slidingWindowRateLimiter.returns({ hit: hitStub }) createMessageHandler.returns(inFlightHandler) @@ -641,6 +641,46 @@ describe('WebSocketAdapter', () => { expect(sentFrame()).to.deep.equal(['CLOSED', 'q1', 'rate-limited: slow down', '60000']) }) + // The strategy's own estimate wins when it has one: a sliding window's + // period is the upper bound, an EWMA half-life is not. + it('sends the limiter\'s own backoff when it has one', async () => { + slidingWindowRateLimiter.returns({ + hit: sandbox.stub().resolves({ limited: true, retryAfterMs: 120000 }), + }) + + await onMessage(Buffer.from(JSON.stringify(['REQ', 'sub1', { kinds: [1] }]))) + + expect(sentFrame()).to.deep.equal(['CLOSED', 'sub1', 'rate-limited: slow down', '120000']) + }) + + it('buckets the limit by message type, so a REQ is not refused for EVENTs', async () => { + const hitStub = sandbox.stub() + hitStub.withArgs(Sinon.match(/:message:EVENT:/)).resolves({ limited: true }) + hitStub.withArgs(Sinon.match(/:message:REQ:/)).resolves({ limited: false }) + slidingWindowRateLimiter.returns({ hit: hitStub }) + + await onMessage(Buffer.from(JSON.stringify(['REQ', 'sub1', { kinds: [1] }]))) + + expect(hitStub).to.have.been.calledOnceWithExactly( + '127.0.0.1:message:REQ:60000', + 1, + { period: 60000, rate: 1 }, + ) + }) + + // A closed set of types, or a client could rotate invented ones and keep + // every bucket empty. + it('buckets an invented message type with everything else unrecognised', async () => { + const hitStub = sandbox.stub().resolves({ limited: false }) + slidingWindowRateLimiter.returns({ hit: hitStub }) + + await onMessage(Buffer.from(JSON.stringify(['AAAA']))) + await onMessage(Buffer.from(JSON.stringify(['BBBB']))) + + expect(hitStub.firstCall.args[0]).to.equal('127.0.0.1:message:OTHER:60000') + expect(hitStub.secondCall.args[0]).to.equal('127.0.0.1:message:OTHER:60000') + }) + it('falls back to the notice when the frame is not valid JSON', async () => { await onMessage(Buffer.from('not-json')) @@ -652,7 +692,7 @@ describe('WebSocketAdapter', () => { it('stops hitting subsequent rate limit windows once one is exceeded', async () => { client.readyState = WebSocket.OPEN - const hitStub = sandbox.stub().resolves(true) + const hitStub = sandbox.stub().resolves({ limited: true }) settingsFactory.returns({ network: { remoteIpHeader: '' }, diff --git a/test/unit/controllers/invoices/post-invoice-controller.spec.ts b/test/unit/controllers/invoices/post-invoice-controller.spec.ts index a6e68cc21..b53840aa6 100644 --- a/test/unit/controllers/invoices/post-invoice-controller.spec.ts +++ b/test/unit/controllers/invoices/post-invoice-controller.spec.ts @@ -44,7 +44,7 @@ const makeController = ( }), }, overrides.settings ?? (() => baseSettings), - overrides.rateLimiter ?? (() => ({ hit: sinon.stub().resolves(false) })), + overrides.rateLimiter ?? (() => ({ hit: sinon.stub().resolves({ limited: false }) })), ) } @@ -80,7 +80,7 @@ describe('PostInvoiceController', () => { describe('rate limiting', () => { it('returns 429 when rate limited', async () => { - const rateLimiter = { hit: sinon.stub().resolves(true) } + const rateLimiter = { hit: sinon.stub().resolves({ limited: true }) } const settings = () => ({ ...baseSettings, limits: { diff --git a/test/unit/controllers/subscriptions/get-subscription-status-controller.spec.ts b/test/unit/controllers/subscriptions/get-subscription-status-controller.spec.ts index 017157f14..9601b327e 100644 --- a/test/unit/controllers/subscriptions/get-subscription-status-controller.spec.ts +++ b/test/unit/controllers/subscriptions/get-subscription-status-controller.spec.ts @@ -65,7 +65,7 @@ describe('GetSubscriptionStatusController', () => { beforeEach(() => { repository = { findByPubkey: sinon.stub().resolves(subscription()) } - rateLimiterHit = sinon.stub().resolves(false) + rateLimiterHit = sinon.stub().resolves({ limited: false }) currentSettings = baseSettings() }) @@ -333,7 +333,7 @@ describe('GetSubscriptionStatusController', () => { currentSettings = baseSettings({ limits: { invoice: { rateLimits: [{ rate: 2, period: 60000 }], ipWhitelist: [] } }, }) - rateLimiterHit.resolves(true) + rateLimiterHit.resolves({ limited: true }) const res = makeRes() await makeController().handleRequest(makeReq(), res) @@ -349,7 +349,7 @@ describe('GetSubscriptionStatusController', () => { it('returns 429 when the invoice limits are exceeded, without reading anything', async () => { currentSettings = limited() - rateLimiterHit.resolves(true) + rateLimiterHit.resolves({ limited: true }) const res = makeRes() await makeController().handleRequest(makeReq(), res) @@ -373,7 +373,7 @@ describe('GetSubscriptionStatusController', () => { currentSettings = baseSettings({ limits: { invoice: { rateLimits: [{ rate: 2, period: 60000 }], ipWhitelist: ['1.2.3.4'] } }, }) - rateLimiterHit.resolves(true) + rateLimiterHit.resolves({ limited: true }) const res = makeRes() await makeController().handleRequest(makeReq(), res) diff --git a/test/unit/handlers/event-message-handler.spec.ts b/test/unit/handlers/event-message-handler.spec.ts index aa222e46e..f265f8b74 100644 --- a/test/unit/handlers/event-message-handler.spec.ts +++ b/test/unit/handlers/event-message-handler.spec.ts @@ -92,7 +92,7 @@ describe('EventMessageHandler', () => { webSocket = new EventEmitter() as any webSocket.on(WebSocketAdapterEvent.Message, onMessageSpy) message = [MessageType.EVENT, event] - isRateLimitedStub = sandbox.stub(EventMessageHandler.prototype, 'isRateLimited' as any) + isRateLimitedStub = sandbox.stub(EventMessageHandler.prototype, 'isRateLimited' as any).resolves({ status: 'allowed' }) handler = new EventMessageHandler( webSocket as any, strategyFactoryStub, @@ -104,7 +104,7 @@ describe('EventMessageHandler', () => { }) as any, {} as any, { hasKey: async () => false, setKey: async () => true } as any, - () => ({ hit: async () => false }), + () => ({ hit: async () => ({ limited: false }) }), {} as any, ) }) @@ -126,7 +126,7 @@ describe('EventMessageHandler', () => { }) it('rejects event if rate-limited with the backoff in ms', async () => { - isRateLimitedStub.resolves(60000) + isRateLimitedStub.resolves({ status: 'limited', retryAfterMs: 60000 }) await handler.handleMessage(message) @@ -141,8 +141,10 @@ describe('EventMessageHandler', () => { expect(strategyFactoryStub).not.to.have.been.called }) - it('rejects event if rate-limited without a backoff when none is known', async () => { - isRateLimitedStub.resolves(true) + // A limiter outage is our fault. Announcing `rate-limited` made every + // connected client back off for it, so the refusal names the error instead. + it('rejects event with an error, not a backoff, when the limiter is unavailable', async () => { + isRateLimitedStub.resolves({ status: 'unavailable' }) await handler.handleMessage(message) @@ -150,7 +152,7 @@ describe('EventMessageHandler', () => { MessageType.OK, event.id, false, - 'rate-limited: slow down', + 'error: rate limiter unavailable', ]) expect(strategyFactoryStub).not.to.have.been.called }) @@ -280,7 +282,7 @@ describe('EventMessageHandler', () => { powSettings, {} as any, { hasKey: async () => false, setKey: async () => true } as any, - () => ({ hit: async () => false }), + () => ({ hit: async () => ({ limited: false }) }), {} as any, ) canAcceptEventStub.returns('rejected: pow') @@ -299,10 +301,10 @@ describe('EventMessageHandler', () => { powSettings, {} as any, { hasKey: async () => false, setKey: async () => true } as any, - () => ({ hit: async () => false }), + () => ({ hit: async () => ({ limited: false }) }), {} as any, ) - isRateLimitedStub.resolves(true) + isRateLimitedStub.resolves({ status: 'limited', retryAfterMs: 60000 }) await powHandler.handleMessage(message) @@ -318,7 +320,7 @@ describe('EventMessageHandler', () => { powSettings, {} as any, { hasKey: async () => false, setKey: async () => true } as any, - () => ({ hit: async () => false }), + () => ({ hit: async () => ({ limited: false }) }), {} as any, ) isEventValidStub.returns(undefined) @@ -378,7 +380,7 @@ describe('EventMessageHandler', () => { () => settings, {} as any, { hasKey: async () => false, setKey: async () => true } as any, - () => ({ hit: async () => false }), + () => ({ hit: async () => ({ limited: false }) }), {} as any, ) }) @@ -718,7 +720,7 @@ describe('EventMessageHandler', () => { () => settings, {} as any, { hasKey: async () => false, setKey: async () => true } as any, - () => ({ hit: async () => false }), + () => ({ hit: async () => ({ limited: false }) }), { getDistance: getDistanceStub } as any, ) @@ -1037,7 +1039,7 @@ describe('EventMessageHandler', () => { }) as any, {} as any, { hasKey: async () => false, setKey: async () => true } as any, - () => ({ hit: async () => false }), + () => ({ hit: async () => ({ limited: false }) }), {} as any, ) }) @@ -1075,7 +1077,7 @@ describe('EventMessageHandler', () => { event: eventLimits, }, } as any - rateLimiterHitStub = sandbox.stub() + rateLimiterHitStub = sandbox.stub().resolves({ limited: false }) getClientAddressStub = sandbox.stub() webSocket = { getClientAddress: getClientAddressStub, @@ -1098,22 +1100,22 @@ describe('EventMessageHandler', () => { it('fulfills with false if limits setting is not set', async () => { settings.limits = undefined - return expect((handler as any).isRateLimited(event)).to.eventually.be.false + return expect((handler as any).isRateLimited(event)).to.eventually.deep.equal({ status: 'allowed' }) }) it('fulfills with false if event limits setting is not set', async () => { settings.limits.event = undefined - return expect((handler as any).isRateLimited(event)).to.eventually.be.false + return expect((handler as any).isRateLimited(event)).to.eventually.deep.equal({ status: 'allowed' }) }) it('fulfills with false if rate limits setting is not set', async () => { eventLimits.rateLimits = undefined - return expect((handler as any).isRateLimited(event)).to.eventually.be.false + return expect((handler as any).isRateLimited(event)).to.eventually.deep.equal({ status: 'allowed' }) }) it('fulfills with false if rate limits setting is empty', async () => { eventLimits.rateLimits = [] - return expect((handler as any).isRateLimited(event)).to.eventually.be.false + return expect((handler as any).isRateLimited(event)).to.eventually.deep.equal({ status: 'allowed' }) }) it("fulfills with false if event pubkey equals relay's own public key", async () => { @@ -1127,7 +1129,7 @@ describe('EventMessageHandler', () => { const actualResult = await (handler as any).isRateLimited(event) - expect(actualResult).to.be.false + expect(actualResult.status).to.equal('allowed') expect(rateLimiterHitStub).not.to.have.been.called }) @@ -1144,7 +1146,7 @@ describe('EventMessageHandler', () => { const actualResult = await (handler as any).isRateLimited(event) - expect(actualResult).to.be.false + expect(actualResult.status).to.equal('allowed') expect(rateLimiterHitStub).not.to.have.been.called }) @@ -1176,7 +1178,7 @@ describe('EventMessageHandler', () => { const actualResult = await (handler as any).isRateLimited(event) - expect(actualResult).to.be.false + expect(actualResult.status).to.equal('allowed') expect(rateLimiterHitStub).not.to.have.been.called }) @@ -1255,7 +1257,7 @@ describe('EventMessageHandler', () => { }, ] - rateLimiterHitStub.resolves(false) + rateLimiterHitStub.resolves({ limited: false }) const actualResult = await (handler as any).isRateLimited(event) @@ -1267,7 +1269,7 @@ describe('EventMessageHandler', () => { rate: 1, }, ) - expect(actualResult).to.be.false + expect(actualResult.status).to.equal('allowed') }) it('fulfills with the tripped period if rate limited by second rate limit setting', async () => { @@ -1288,8 +1290,8 @@ describe('EventMessageHandler', () => { }, ] - rateLimiterHitStub.onFirstCall().resolves(false) - rateLimiterHitStub.onSecondCall().resolves(true) + rateLimiterHitStub.onFirstCall().resolves({ limited: false }) + rateLimiterHitStub.onSecondCall().resolves({ limited: true, retryAfterMs: 180 }) const actualResult = await (handler as any).isRateLimited(event) @@ -1309,7 +1311,7 @@ describe('EventMessageHandler', () => { rate: 3, }, ) - expect(actualResult).to.equal(180) + expect(actualResult).to.deep.equal({ status: 'limited', retryAfterMs: 180 }) }) it('stops hitting subsequent rate limit windows once one is exceeded', async () => { @@ -1324,12 +1326,12 @@ describe('EventMessageHandler', () => { }, ] - rateLimiterHitStub.onFirstCall().resolves(true) + rateLimiterHitStub.onFirstCall().resolves({ limited: true, retryAfterMs: 60000 }) const actualResult = await (handler as any).isRateLimited(event) expect(rateLimiterHitStub).to.have.been.calledOnce - expect(actualResult).to.equal(60000) + expect(actualResult).to.deep.equal({ status: 'limited', retryAfterMs: 60000 }) }) it('fails closed when the rate limiter backend is unavailable', async () => { @@ -1343,7 +1345,7 @@ describe('EventMessageHandler', () => { const actualResult = await (handler as any).isRateLimited(event) - expect(actualResult).to.be.true + expect(actualResult).to.deep.equal({ status: 'unavailable' }) expect(rateLimiterHitStub).to.have.been.calledOnce }) @@ -1384,7 +1386,7 @@ describe('EventMessageHandler', () => { { id: 'basic', amount: 1000000n, intervalSeconds: 2_592_000, graceSeconds: 259_200 }, ], } as any - rateLimiterHitStub.resolves(false) + rateLimiterHitStub.resolves({ limited: false }) subscriptionFindByPubkeyStub = sandbox.stub().resolves(subscription()) cacheStub = { getKey: sandbox.stub().resolves(null), setKey: sandbox.stub().resolves(true) } handler = new EventMessageHandler( @@ -1402,7 +1404,7 @@ describe('EventMessageHandler', () => { }) it('holds a subscriber to the limits of their plan instead of the global ones, keyed the same way', async () => { - await expect((handler as any).isRateLimited(subscriber)).to.eventually.equal(false) + await expect((handler as any).isRateLimited(subscriber)).to.eventually.deep.equal({ status: 'allowed' }) expect(rateLimiterHitStub).to.have.been.calledOnceWithExactly(`${PUBKEY}:events:60000`, 1, { period: 60000, @@ -1422,9 +1424,9 @@ describe('EventMessageHandler', () => { }) it('limits a subscriber who tops the plan limit', async () => { - rateLimiterHitStub.resolves(true) + rateLimiterHitStub.resolves({ limited: true }) - await expect((handler as any).isRateLimited(subscriber)).to.eventually.equal(60000) + await expect((handler as any).isRateLimited(subscriber)).to.eventually.deep.equal({ status: 'limited', retryAfterMs: 60000 }) }) it('keeps the global limits for a plan that has none of its own', async () => { @@ -1438,7 +1440,7 @@ describe('EventMessageHandler', () => { it('lets a plan with an empty list of limits send without limit', async () => { settings.payments.subscriptionPlans[0].limits.rateLimits = [] - await expect((handler as any).isRateLimited(subscriber)).to.eventually.equal(false) + await expect((handler as any).isRateLimited(subscriber)).to.eventually.deep.equal({ status: 'allowed' }) expect(rateLimiterHitStub).not.to.have.been.called }) @@ -1509,7 +1511,7 @@ describe('EventMessageHandler', () => { it('still lets a whitelisted pubkey send without limit', async () => { ;(eventLimits as any).whitelists = { pubkeys: [PUBKEY] } - await expect((handler as any).isRateLimited(subscriber)).to.eventually.equal(false) + await expect((handler as any).isRateLimited(subscriber)).to.eventually.deep.equal({ status: 'allowed' }) expect(rateLimiterHitStub).not.to.have.been.called }) @@ -1640,7 +1642,7 @@ describe('EventMessageHandler', () => { it('falls back to the global limits when the subscription cannot be read, and caches nothing', async () => { subscriptionFindByPubkeyStub.rejects(new Error('db unavailable')) - await expect((handler as any).isRateLimited(subscriber)).to.eventually.equal(false) + await expect((handler as any).isRateLimited(subscriber)).to.eventually.deep.equal({ status: 'allowed' }) expect(rateLimiterHitStub.firstCall.args[2]).to.deep.equal({ period: 60000, rate: 1 }) expect(cacheStub.setKey).not.to.have.been.called @@ -1718,7 +1720,7 @@ describe('EventMessageHandler', () => { () => settings, {} as any, cacheStub, - () => ({ hit: async () => false }), + () => ({ hit: async () => ({ limited: false }) }), {} as any, ) }) @@ -1979,7 +1981,7 @@ describe('EventMessageHandler', () => { () => settings, {} as any, cacheStub, - () => ({ hit: async () => false }), + () => ({ hit: async () => ({ limited: false }) }), {} as any, { findByPubkey: subscriptionFindByPubkeyStub } as any, ) @@ -2164,7 +2166,7 @@ describe('EventMessageHandler', () => { () => settings, nip05VerificationRepository, { hasKey: async () => false, setKey: async () => true, getKey: async () => null } as any, - () => ({ hit: async () => false }), + () => ({ hit: async () => ({ limited: false }) }), {} as any, ) }) @@ -2320,7 +2322,7 @@ describe('EventMessageHandler', () => { }) as any, {} as any, { hasKey: async () => false, setKey: async () => true } as any, - () => ({ hit: async () => false }), + () => ({ hit: async () => ({ limited: false }) }), {} as any, ) }) @@ -2371,7 +2373,7 @@ describe('EventMessageHandler', () => { () => settings, nip05VerificationRepository, { hasKey: async () => false, setKey: async () => true, getKey: async () => null } as any, - () => ({ hit: async () => false }), + () => ({ hit: async () => ({ limited: false }) }), {} as any, ) }) @@ -2615,7 +2617,7 @@ describe('EventMessageHandler', () => { () => settings, nip05VerificationRepository, { hasKey: async () => false, setKey: async () => true, getKey: async () => null } as any, - () => ({ hit: async () => false }), + () => ({ hit: async () => ({ limited: false }) }), {} as any, ) }) @@ -2792,7 +2794,7 @@ describe('EventMessageHandler', () => { () => settings, nip05VerificationRepository, { hasKey: async () => false, setKey: async () => true, getKey: async () => null } as any, - () => ({ hit: async () => false }), + () => ({ hit: async () => ({ limited: false }) }), {} as any, ) }) @@ -2991,7 +2993,7 @@ describe('EventMessageHandler', () => { () => ({ info: { relay_url: 'relay_url' }, nip42: { authRequired: false } }) as any, {} as any, { hasKey: async () => false, setKey: async () => true } as any, - () => ({ hit: async () => false }), + () => ({ hit: async () => ({ limited: false }) }), {} as any, ) @@ -3007,7 +3009,7 @@ describe('EventMessageHandler', () => { () => ({ info: { relay_url: 'relay_url' }, nip42: { authRequired: true } }) as any, {} as any, { hasKey: async () => false, setKey: async () => true } as any, - () => ({ hit: async () => false }), + () => ({ hit: async () => ({ limited: false }) }), {} as any, ) @@ -3025,7 +3027,7 @@ describe('EventMessageHandler', () => { () => ({ info: { relay_url: 'relay_url' }, nip42: { authRequired: true } }) as any, {} as any, { hasKey: async () => false, setKey: async () => true } as any, - () => ({ hit: async () => false }), + () => ({ hit: async () => ({ limited: false }) }), {} as any, ) @@ -3048,7 +3050,7 @@ describe('EventMessageHandler', () => { }) as any, {} as any, { hasKey: async () => false, setKey: async () => true } as any, - () => ({ hit: async () => false }), + () => ({ hit: async () => ({ limited: false }) }), {} as any, ) }) @@ -3081,7 +3083,7 @@ describe('EventMessageHandler', () => { () => ({ info: { relay_url: 'relay_url' } }) as any, {} as any, { hasKey: async () => false, setKey: async () => true } as any, - () => ({ hit: async () => false }), + () => ({ hit: async () => ({ limited: false }) }), {} as any, ) expect(await (handler as any).isProtectedEventBlocked(event)).to.be.undefined diff --git a/test/unit/handlers/subscribe-message-handler.spec.ts b/test/unit/handlers/subscribe-message-handler.spec.ts index 74536a565..b14562cc9 100644 --- a/test/unit/handlers/subscribe-message-handler.spec.ts +++ b/test/unit/handlers/subscribe-message-handler.spec.ts @@ -81,7 +81,7 @@ describe('SubscribeMessageHandler', () => { updatedAt: new Date(), })) inviteCodeRepository = { create: inviteCodeRepositoryCreateStub } as any - rateLimiterHitStub = sandbox.stub().resolves(false) + rateLimiterHitStub = sandbox.stub().resolves({ limited: false }) rateLimiter = { hit: rateLimiterHitStub } as any rateLimiterFactory = sandbox.stub().returns(rateLimiter) handler = new SubscribeMessageHandler( @@ -405,7 +405,7 @@ describe('SubscribeMessageHandler', () => { limits: { client: { subscription: {} }, invite: { rateLimits: [{ period: 3600000, rate: 5 }] } }, }), ) - rateLimiterHitStub.resolves(true) + rateLimiterHitStub.resolves({ limited: true }) await request([{ kinds: [28935] }]) diff --git a/test/unit/utils/admin-rate-limit.spec.ts b/test/unit/utils/admin-rate-limit.spec.ts index 8ff26fa04..e9120a2a3 100644 --- a/test/unit/utils/admin-rate-limit.spec.ts +++ b/test/unit/utils/admin-rate-limit.spec.ts @@ -29,7 +29,7 @@ describe('isAdminRateLimited', () => { }) as any it('returns false when no rate limits are configured', async () => { - const rateLimiter = { hit: sinon.stub().resolves(false) } + const rateLimiter = { hit: sinon.stub().resolves({ limited: false }) } const settings = { ...baseSettings, limits: { admin: { ipWhitelist: [] } } } const limited = await isAdminRateLimited(makeRequest(), settings as any, () => rateLimiter, 'admin') @@ -39,7 +39,7 @@ describe('isAdminRateLimited', () => { }) it('returns true when login rate limit is exceeded', async () => { - const rateLimiter = { hit: sinon.stub().resolves(true) } + const rateLimiter = { hit: sinon.stub().resolves({ limited: true }) } const limited = await isAdminRateLimited(makeRequest(), baseSettings as any, () => rateLimiter, 'login') @@ -51,7 +51,7 @@ describe('isAdminRateLimited', () => { }) it('returns true when admin route rate limit is exceeded', async () => { - const rateLimiter = { hit: sinon.stub().resolves(true) } + const rateLimiter = { hit: sinon.stub().resolves({ limited: true }) } const limited = await isAdminRateLimited(makeRequest(), baseSettings as any, () => rateLimiter, 'admin') diff --git a/test/unit/utils/ewma-rate-limiter.spec.ts b/test/unit/utils/ewma-rate-limiter.spec.ts index 72678789c..40f32f2cb 100644 --- a/test/unit/utils/ewma-rate-limiter.spec.ts +++ b/test/unit/utils/ewma-rate-limiter.spec.ts @@ -57,16 +57,64 @@ describe('EWMARateLimiter', () => { }) describe('hit', () => { - it('returns false on first request', async () => { - evalStub.resolves(0) + it('reports no limit below the rate', async () => { + evalStub.resolves('1') const result = await rateLimiter.hit('key', 1, { period: 120000, rate: 10 }) - expect(result).to.be.false + expect(result.limited).to.be.false + expect(result.retryAfterMs).to.be.undefined }) - it('returns true when rate limit exceeded', async () => { - evalStub.resolves(1) - const result = await rateLimiter.hit('key', 1, { period: 120000, rate: 10 }) - expect(result).to.be.true + it('reports a limit above the rate', async () => { + evalStub.resolves('24') + const result = await rateLimiter.hit('key', 1, { period: 60000, rate: 6 }) + expect(result.limited).to.be.true + }) + + // The script returns the score, not a verdict, so a truncated integer + // would make the backoff wrong: 6.4 must not arrive as 6. + it('reads a fractional score', async () => { + evalStub.resolves('6.4') + const result = await rateLimiter.hit('key', 1, { period: 60000, rate: 6 }) + expect(result.limited).to.be.true + expect(result.retryAfterMs).to.equal(Math.ceil(60000 * Math.log2(6.4 / 5))) + }) + + // `period` is the half-life, and the next hit adds its step before the + // comparison, so the score has to fall to `rate - step`: 24 down to 5 at + // half-life 60 s is 2.26 periods, ~136 s, not the 60 s we used to send. + it('sends the half-life backoff the next hit actually needs', async () => { + evalStub.resolves('24') + const result = await rateLimiter.hit('key', 1, { period: 60000, rate: 6 }) + expect(result.retryAfterMs).to.equal(135783) + }) + + // rate 1 with step 1 leaves no headroom: no decay admits another hit, so + // the honest answer is one period, which is also the key's TTL. + it('falls back to the period when no decay can admit a hit', async () => { + evalStub.resolves('3') + const result = await rateLimiter.hit('key', 1, { period: 60000, rate: 1 }) + expect(result.retryAfterMs).to.equal(60000) + }) + + it('never sends a zero or sub-millisecond backoff', async () => { + evalStub.resolves('6.0000001') + const result = await rateLimiter.hit('key', 1, { period: 60000, rate: 6 }) + expect(result.limited).to.be.true + expect(result.retryAfterMs).to.be.at.least(1) + }) + + // A non-numeric answer means the script and the client disagree about + // the protocol. Reading it as "allowed" would silently disable the + // limiter; the caller fails closed on a throw. + it('throws when the script returns something that is not a score', async () => { + evalStub.resolves(null) + let error: unknown + try { + await rateLimiter.hit('key', 1, { period: 120000, rate: 10 }) + } catch (caught) { + error = caught + } + expect(error).to.be.instanceOf(Error) }) // EWMA and sliding_window store different Redis types under the same @@ -74,7 +122,7 @@ describe('EWMARateLimiter', () => { // volume, or an operator switching strategy, leaves a sorted set where // EWMA would then HSET and fail with WRONGTYPE on every request. it('namespaces the key it writes', async () => { - evalStub.resolves(0) + evalStub.resolves('0') await rateLimiter.hit('1.2.3.4:connection:120000', 1, { period: 120000, rate: 10 }) expect(evalStub.calledOnce).to.be.true diff --git a/test/unit/utils/sliding-window-rate-limiter.spec.ts b/test/unit/utils/sliding-window-rate-limiter.spec.ts index a864a93df..cce30d112 100644 --- a/test/unit/utils/sliding-window-rate-limiter.spec.ts +++ b/test/unit/utils/sliding-window-rate-limiter.spec.ts @@ -53,11 +53,12 @@ describe('SlidingWindowRateLimiter', () => { }) it('returns true if rate limited', async () => { - evalStub.resolves(1) + evalStub.resolves('1') const actualResult = await rateLimiter.hit('key', 1, { period: 60000, rate: 10 }) - expect(actualResult).to.be.true + expect(actualResult.limited).to.be.true + expect(actualResult.retryAfterMs).to.equal(60000) expect(evalStub).to.have.been.calledOnce const args = evalStub.firstCall.args expect(args[1]).to.deep.equal(['key']) @@ -67,11 +68,11 @@ describe('SlidingWindowRateLimiter', () => { }) it('returns false if not rate limited', async () => { - evalStub.resolves(0) + evalStub.resolves('0') const actualResult = await rateLimiter.hit('key', 1, { period: 60000, rate: 10 }) - expect(actualResult).to.be.false + expect(actualResult.limited).to.be.false }) it('robustly handles string return types from Redis', async () => { @@ -79,6 +80,6 @@ describe('SlidingWindowRateLimiter', () => { const actualResult = await rateLimiter.hit('key', 1, { period: 60000, rate: 10 }) - expect(actualResult).to.be.true + expect(actualResult.limited).to.be.true }) })