cameri / nostream / 41238161825

78%

push

github

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

Closes #1007.

## What

`RateLimit.period` is milliseconds everywhere in practice —
`resources/default-settings.yaml` ships `1000`, `60000`, `900000`,
`3600000` — but nothing said so, and the validator only required `period
> 0`. 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: twelve hits per minute becomes twelve
hits per 60 ms. Nothing complained, and nothing in review caught it.

Three changes, as the issue asked:

| where | change |
|---|---|
| `src/@types/settings.ts` | `RateLimit.period` documents the unit, with
the direction of the mistake spelled out |
| `resources/default-settings.yaml` | every `rateLimits` /
`loginRateLimits` block is annotated, plus a note at `limits:` |
| `validateRateLimitPeriods` | rejects a non-number, a fraction, and
anything below 1 ms; the message names milliseconds |

## What it deliberately does not do

It cannot tell `60` from `60000` — both are whole numbers, and 60 is a
valid millisecond value. No validator can infer the unit from the
number, which is why the unit is documented at the point where it is
set, and why the message says "milliseconds" so a rejected value is
never read as seconds.

## Review

Greptile's P2 on the first wording was correct and is fixed: the comment
said a shorter period makes the limit *stricter*, and it makes it
looser.

## Verification

- `test/unit/utils/settings-config.spec.ts`: 112 passing (4 new cases:
fractional, string, NaN, and the message mentioning milliseconds)
- reverting the validator: those cases fail apart from NaN, which the
old `> 0` check already caught

4390 of 6207 branches covered (70.73%)

Branch coverage included in aggregate %.

6 of 6 new or added lines in 1 file covered. (100.0%)

9558 of 11821 relevant lines covered (80.86%)

774.26 hits per line