fix(settings): state that rate-limit periods are milliseconds and reject a period that is not one

.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.

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

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
 }

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' })
     }
   })
 }

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] } }