diff --git a/.changeset/rate-limit-period-milliseconds.md b/.changeset/rate-limit-period-milliseconds.md new file mode 100644 index 00000000..3b7ea6cd --- /dev/null +++ b/.changeset/rate-limit-period-milliseconds.md @@ -0,0 +1,9 @@ +--- +"nostream": patch +--- + +fix(settings): state that rate-limit periods are milliseconds and reject a period that is not one + +Every shipped limit counts in milliseconds — `1000`, `60000`, `900000`, `3600000` — but nothing said so. `RateLimit.period` is a bare `number`, so an operator writing `period: 60` for a minute got a 60 ms window — and because `rate` is hits per window, that is a limit a thousand times **looser** than intended, honoured without complaint, and caught by nothing. + +The unit is now stated on `RateLimit` and on every `rateLimits` block in `resources/default-settings.yaml`, and `validateRateLimitPeriods` rejects what cannot be a count of milliseconds: a fraction, a string, a non-number, or anything below one millisecond. A whole number is not enough to tell 60 from 60000, which is why the unit is documented where it is set. diff --git a/resources/default-settings.yaml b/resources/default-settings.yaml index 54c7f4bc..19abd3e9 100755 --- a/resources/default-settings.yaml +++ b/resources/default-settings.yaml @@ -216,10 +216,19 @@ eventStore: dvm: workers: [] limits: + # Every `period` below is a count of milliseconds, and `rate` is how many hits + # one window allows. `period: 60`, written as if it meant 60 seconds, is read + # as a 60 ms window - which makes the limit a thousand times LOOSER, not + # stricter: twelve hits per minute becomes twelve hits per 60 ms. Nothing + # rejects it, because 60 is a valid count of milliseconds. The unit cannot be + # inferred from the number, so it is stated here and on RateLimit in + # src/@types/settings.ts. + # # strategy selection configuration for rate limiting: rateLimiter: strategy: ewma invoice: + # period is milliseconds (60000 = one minute) rateLimits: - period: 60000 rate: 12 @@ -232,11 +241,13 @@ limits: invite: # Per-pubkey limit on kind 28935 invite requests. Each mint writes a row to # invite_codes, so this is the main defence against a code flood. + # period is milliseconds (60000 = one minute) rateLimits: - description: 5 invite requests per hour per pubkey period: 3600000 rate: 5 admissionCheck: + # period is milliseconds (60000 = one minute) rateLimits: - description: 30 admission checks/min or 1 check every 2 seconds period: 60000 @@ -246,10 +257,12 @@ limits: - "10.10.10.1" - "::ffff:10.10.10.1" admin: + # period is milliseconds (60000 = one minute) rateLimits: - description: 30 admin requests/min period: 60000 rate: 30 + # period is milliseconds (60000 = one minute) loginRateLimits: - description: 10 login attempts per 15 minutes period: 900000 @@ -259,6 +272,7 @@ limits: - "10.10.10.1" - "::ffff:10.10.10.1" connection: + # period is milliseconds (60000 = one minute) rateLimits: - period: 1000 rate: 12 @@ -323,6 +337,7 @@ limits: - 39 - - 50 - 9007199254740991 + # period is milliseconds (60000 = one minute) rateLimits: - description: 6 events/min for event kinds 0, 3, 40 and 41 kinds: @@ -386,6 +401,7 @@ limits: maxLimit: 5000 minPrefixLength: 4 message: + # period is milliseconds (60000 = one minute) rateLimits: - description: 240 raw messages/min period: 60000 diff --git a/src/@types/settings.ts b/src/@types/settings.ts index e19daca1..133c03f9 100644 --- a/src/@types/settings.ts +++ b/src/@types/settings.ts @@ -24,6 +24,18 @@ export interface Network { export interface RateLimit { description?: string + /** + * Length of the window in milliseconds. + * + * Every shipped limit counts this way (`1000`, `60000`, `900000`, `3600000`), + * and `rate` is how many hits one window allows. `period: 60`, written as if + * it meant 60 seconds, is therefore read as a 60 ms window and makes the + * limit a thousand times LOOSER: twelve hits per minute becomes twelve hits + * per 60 ms. The relay honours it without complaint, and nothing can tell the + * two apart from the number alone, which is why the unit is documented where + * the value is set. The validator rejects anything that is not a whole count + * of milliseconds, at least one. + */ period: number rate: number } diff --git a/src/utils/settings-config.ts b/src/utils/settings-config.ts index 4abe1a96..d8020179 100644 --- a/src/utils/settings-config.ts +++ b/src/utils/settings-config.ts @@ -595,8 +595,16 @@ const validateRateLimitPeriods = ( issues.push({ path: `${pathPrefix}[${index}]`, message: 'rateLimits entry must be an object' }) return } - if (!(entry.period > 0)) { - issues.push({ path: `${pathPrefix}[${index}].period`, message: 'period must be greater than 0' }) + // The window is in milliseconds, and 60 is as valid a number as 60000, so + // the unit cannot be inferred here: it is stated on RateLimit and on each + // block in resources/default-settings.yaml. What this can refuse is a value + // that is not a whole count of milliseconds at all. + if (typeof entry.period !== 'number' || !Number.isFinite(entry.period)) { + issues.push({ path: `${pathPrefix}[${index}].period`, message: 'period must be a number of milliseconds' }) + } else if (!Number.isInteger(entry.period)) { + issues.push({ path: `${pathPrefix}[${index}].period`, message: 'period must be a whole number of milliseconds' }) + } else if (entry.period < 1) { + issues.push({ path: `${pathPrefix}[${index}].period`, message: 'period must be at least 1 millisecond' }) } }) } diff --git a/test/unit/utils/settings-config.spec.ts b/test/unit/utils/settings-config.spec.ts index 97b24065..320c02ea 100644 --- a/test/unit/utils/settings-config.spec.ts +++ b/test/unit/utils/settings-config.spec.ts @@ -387,6 +387,34 @@ describe('settings-config', () => { expect(issues.some((issue) => issue.path.startsWith('limits.event.rateLimits'))).to.equal(false) }) + // The window is milliseconds everywhere, but 60 is as plausible a number as + // 60000, so the unit cannot be inferred. What can be refused is a value that + // is not a whole count of milliseconds at all: a fraction (0.5 ms), a string + // from a hand-edited YAML, and a value that is not a number. + const invalidPeriodCases: { label: string; period: unknown }[] = [ + { label: 'a fractional period', period: 0.5 }, + { label: 'a string period', period: '60000' }, + { label: 'a NaN period', period: Number.NaN }, + ] + + for (const { label, period } of invalidPeriodCases) { + it(`rejects ${label} on limits.event.rateLimits`, () => { + const settings = baseSettings() + settings.limits = { event: { rateLimits: [{ period, rate: 10 }] } } + + const issues = validateSettings(settings) + expect(issues.some((issue) => issue.path === 'limits.event.rateLimits[0].period')).to.equal(true) + }) + } + + it('names milliseconds in the period message, so a rejected value is not read as seconds', () => { + const settings = baseSettings() + settings.limits = { event: { rateLimits: [{ period: 0.5, rate: 10 }] } } + + const message = validateSettings(settings).find((issue) => issue.path === 'limits.event.rateLimits[0].period')?.message + expect(message).to.contain('milliseconds') + }) + it('rejects a null entry in limits.event.rateLimits without throwing', () => { const settings = baseSettings() settings.limits = { event: { rateLimits: [null] } }