feat(auth): make rate limits configurable via env vars

[steilerDev]

Summary

• Added AUTH_RATE_LIMIT_MAX and AUTH_RATE_LIMIT_WINDOW env vars to configure the login endpoint rate limit (defaults: 20 requests / 15 minutes)
• Invalid values (NaN, non-positive, bad duration format) cause startup failure with a descriptive error — no silent fallback
• Setup route remains hardcoded with a code comment explaining why (post-first-user it always returns 403, so the rate limit never fires)

Fixes #1970

Test plan

• Unit tests pass (95%+ coverage)
• Integration tests pass
• CI Quality Gates pass (typecheck, tests, build, audit)

Co-Authored-By: Claude dev-team-lead noreply@anthropic.com
Co-Authored-By: Claude backend-developer noreply@anthropic.com
Co-Authored-By: Claude qa-integration-tester noreply@anthropic.com

[steilerDev]

[security-engineer]

Security review of PR #1989 — feat(auth): make rate limits configurable via env vars.

Summary

Validation logic, route wiring, and test coverage are solid. Two findings: one Medium gap in the window validation that could allow an operator to accidentally disable rate limiting, and one Low cosmetic inconsistency. No High or Critical findings. Verdict: APPROVED_WITH_SUGGESTIONS.

---

[MEDIUM] AUTH_RATE_LIMIT_WINDOW=0s passes validation but completely disables rate limiting

OWASP: A07 – Identification & Authentication Failures
Status: Open

Description: The regex accepts 0 as a valid numeric prefix (e.g. 0s, 0m, 0h, 0 minutes). The ms library correctly parses these to 0. @fastify/rate-limit's LocalStore checks expiry as iterationStartMs + timeWindow <= nowInMs. With timeWindow = 0, this simplifies to iterationStartMs <= nowInMs, which is mathematically always true (≤ is non-strict and time never runs backwards). As a result, every request resets the counter to 1, and the configured max is never reached.

Affected file: server/src/plugins/config.ts — the AUTH_RATE_LIMIT_WINDOW_PATTERN regex.

Proof of concept:

AUTH_RATE_LIMIT_WINDOW=0s
# → ms('0s') = 0
# → LocalStore.incr: condition (iterationStartMs + 0 <= nowInMs) is always true
# → counter resets to 1 on every call
# → rate limit with max:20 is never triggered regardless of request volume

This is the symmetric gap to the MAX validation, which correctly rejects 0 and negatives. The design intent (startup failure on bad config) is undermined if a zero-duration window silently nullifies the limit.

Remediation: After the regex check, parse the value with the ms library and assert the result is a positive number:

import ms from 'ms';

const authRateLimitWindowMs = ms(authRateLimitWindowStr);
if (authRateLimitWindowMs === undefined || authRateLimitWindowMs <= 0) {
  errors.push(
    `AUTH_RATE_LIMIT_WINDOW must resolve to a positive duration, got: ${authRateLimitWindowStr}`,
  );
}

The ms library is already a transitive dependency of @fastify/rate-limit — importing it directly adds no new package. This approach is more robust than a regex tweak because it validates the semantics, not just the syntax.

Risk if unaddressed: An operator who sets AUTH_RATE_LIMIT_WINDOW=0s (perhaps intending "no window" semantics) silently disables brute-force protection on the login endpoint with no startup warning and no obvious error. Risk is bounded to operator-level access; external attackers cannot set this.

---

[LOW] AUTH_RATE_LIMIT_MAX accepts non-integer decimals and silently truncates

Status: Open

Description: parseInt('1.5', 10) returns 1, which passes the > 0 check. So AUTH_RATE_LIMIT_MAX=1.5 is accepted without error and silently becomes 1. The documented contract says "positive integer."

Affected file: server/src/plugins/config.ts — the parseInt(authRateLimitMaxStr, 10) line.

Remediation: Validate the string shape before parsing:

const authRateLimitMaxStr = getValue('AUTH_RATE_LIMIT_MAX') ?? '20';
if (!/^\d+$/.test(authRateLimitMaxStr)) {
  errors.push(`AUTH_RATE_LIMIT_MAX must be a positive integer, got: ${authRateLimitMaxStr}`);
} else {
  const authRateLimitMax = parseInt(authRateLimitMaxStr, 10);
  if (authRateLimitMax <= 0) {
    errors.push(`AUTH_RATE_LIMIT_MAX must be a positive integer, got: ${authRateLimitMaxStr}`);
  }
}

This also normalizes the leading-whitespace behavior: parseInt silently accepts ' 20' (passing), whereas the WINDOW regex correctly rejects ' 1h' (failing ^\d+ anchoring). Using /^\d+$/ first makes both fields reject whitespace-padded inputs consistently.

---

[INFORMATIONAL] X-RateLimit-{Limit,Remaining} headers reveal configured max to clients

Status: Accepted Risk (by design — integration test asserts header presence)

@fastify/rate-limit sends X-RateLimit-Limit and X-RateLimit-Remaining on every login response. An attacker can read the configured authRateLimitMax value directly from any login response. This is standard practice; clients use it for graceful backoff. An attacker can determine the limit empirically anyway. No action required. If a future requirement calls for hiding the configured limit, addHeaders: { 'x-ratelimit-limit': false } is available.

---

[INFORMATIONAL] Startup log includes rate limit configuration

Status: Accepted Risk

authRateLimitMax and authRateLimitWindow appear in the Configuration loaded info log. Appropriate — these are non-secret operational parameters consistent with all other non-sensitive config fields already logged. No action needed.

---

Checklist

• No SQL/command/XSS injection vectors in new code
• Authentication/authorization enforced — no new unprotected endpoints
• No sensitive data exposed in logs, errors, or client responses
• Input validated at startup via loadConfig() with fail-fast on bad values
• MAX=0 and negative values correctly rejected
• WINDOW=0 (zero duration) not rejected — Medium gap (see above)
• No new dependencies introduced
• No hardcoded credentials or secrets
• Setup route hardcoded limit (max:5 / 15 min) is appropriate — route returns 403 unconditionally post-setup, tuning has no operational value (AC6 rationale is sound)
• CORS configuration unchanged
• Error responses do not leak internal details
• Test coverage: unit tests for valid/invalid MAX and WINDOW, integration test confirming configured max triggers 429 at the correct threshold, defaults verified

The zero-window issue is the only non-informational finding and requires operator-level access to trigger. Non-blocking for merge; recommend filing a follow-up hardening issue.

[steilerDev]

[product-architect]

Verdict: CHANGES_REQUIRED

One high-severity finding: a value that passes loadConfig() validation makes POST /api/auth/login return 500 on every request, which is precisely the failure class AC2 and AC7 exist to prevent. Everything else is solid — the error-accumulator integration, plugin ordering, defaults, and AppConfig propagation across the seven makeConfig() factories are all correct.

---

HIGH — AUTH_RATE_LIMIT_WINDOW accepts zero durations that break the login route at runtime (AC2, AC7)

server/src/plugins/config.ts:363-372

@fastify/rate-limit v11 parses string timeWindow values with @lukeed/ms, whose parse() returns undefined for a zero magnitude (its guard is if (arr != null && (num = parseFloat(arr[1]))) — 0 is falsy). mergeParams() is an if / else if chain, so a string that parses to undefined does not fall through to defaultTimeWindow:

// node_modules/@fastify/rate-limit/index.js:163-169
if (Number.isFinite(result.timeWindow) && result.timeWindow >= 0) { ... }
else if (typeof result.timeWindow === 'string') { result.timeWindow = parse(result.timeWindow) } // → undefined
else if (typeof result.timeWindow !== 'function') { result.timeWindow = defaultTimeWindow }      // never reached

At request time it then does await params.timeWindow(req, key) on undefined. Verified against this repo's node_modules:

timeWindow: '0s'  →  status: 500
body: {"statusCode":500,"error":"Internal Server Error","message":"params.timeWindow is not a function"}

The PR's AUTH_RATE_LIMIT_WINDOW_PATTERN accepts 0s, 0 minutes, 0ms, and 0.0h — startup succeeds, and every login attempt 500s. Setting a zero window is the most plausible way an operator tries to disable the limit, so this is exactly AC2's "does not silently fall back / does not silently disable the limit" and AC7's "no value removes the limit entirely."

A second, smaller divergence from the same root cause: the pattern uses \s* where @lukeed/ms uses *, so 15\tminutes and 15\nminutes also pass validation and produce the same 500.

Required fix — pick one:

• (a) Validate with the real parser (preferred). Declare @lukeed/ms as a pinned server dependency and validate with parse(), rejecting undefined and <= 0. This eliminates the hand-rolled-grammar drift class entirely rather than patching two instances of it:

  const windowMs = parseDuration(authRateLimitWindowStr);
  if (windowMs === undefined || windowMs <= 0) {
    errors.push(`AUTH_RATE_LIMIT_WINDOW must be a positive duration string (e.g. '15 minutes', '1h'), got: ${authRateLimitWindowStr}`);
  }

• (b) Minimal fix. Keep the regex but change \s* → * and add a positive-magnitude check: if (!PATTERN.test(str) || parseFloat(str) <= 0). parseFloat('0s') === 0 and parseFloat('0.0h') === 0, so both are rejected.

Either way, add test cases for the new invalid class (0s, 0 minutes) alongside the existing not-a-duration cases — AC2 asks for a test per invalid-input class, and zero-magnitude is a distinct class from unparseable.

MEDIUM — Wiki env-var tables not updated in this PR

Two wiki pages carry auth env-var tables that now omit the new variables:

• wiki/Architecture.md — "Authentication & Sessions" table (~line 393)
• wiki/API-Contract.md — "Environment Variables (Auth)" table (~line 107)

Per CLAUDE.md's wiki discipline, configuration documentation is updated as part of story implementation, in the same PR, with the submodule ref bumped on the feature branch. The PR currently contains no wiki submodule change. Please add the two rows to both tables, push the submodule, and stage the bumped ref on this branch. (docs/src/getting-started/configuration.md is docs-writer-owned — see the AC5 note below.)

MEDIUM — AC4's assertion is at the wrong layer

server/src/plugins/rateLimitPlugin.test.ts — the AC4 test reads the config object:

expect(app.config.authRateLimitMax).toBe(20);

That proves the config default, not the effective login limit. If auth.ts regressed to a hardcoded max: 5, this test stays green — so it does not deliver AC4's "so the default cannot drift silently." The adjacent headers test already has the right vehicle and only asserts toBeDefined(). Tighten both so the route↔config binding is actually asserted:

• default case: expect(response.headers['x-ratelimit-limit']).toBe('20')
• AC3 case (AUTH_RATE_LIMIT_MAX=3): expect(response.headers['x-ratelimit-limit']).toBe('3')

Also, no test observes the effective window, which AC1 covers alongside max. A case setting AUTH_RATE_LIMIT_WINDOW=1 minute and asserting x-ratelimit-reset is <= 60 would close that.

MEDIUM (non-blocking) — parseInt accepts trailing garbage, contradicting the "positive integer" message

AUTH_RATE_LIMIT_MAX=20abc → 20; AUTH_RATE_LIMIT_MAX=20.9 → 20; 1e3 → 1. AC2's "non-numeric max" class is therefore only partly closed. This matches the established pattern for PORT, SESSION_DURATION, LLM_MAX_TOKENS, and BACKUP_RETENTION, so I am not asking you to diverge from it here — either add a /^\d+$/ guard for this variable and accept the local inconsistency, or leave it and file a follow-up to tighten integer parsing across loadConfig() uniformly. Your call; note the decision in the PR thread.

LOW — AC5's docs-site half is unaddressed and no request was filed

AC5 covers both CLAUDE.md (done, and prettier-clean) and the docs site, with "file a request if needed." docs/src/getting-started/configuration.md has no rate-limit rows and I found no open issue tracking it. Please file a docs-writer request — and per the issue's Notes, it should cross-reference TRUST_PROXY, since the NAT/shared-egress-IP operator hitting this limit needs both settings together. (Product-owner has the final call on whether AC5 blocks.)

INFORMATIONAL

• AC6 comment placement (server/src/routes/auth.ts:76-78): the rationale is correct and satisfies AC6, but it sits between the existing JSDoc block and the fastify.post call, splitting the two. Folding it into the JSDoc would read better.
• The global default in rateLimitPlugin.ts (max: 200 / 1 minute) stays hardcoded — explicitly optional per the issue's Notes, so no action needed.

---

Verified as correct

• loadConfig() follows the error-accumulator pattern exactly: errors.push() then the single aggregated throw; the both-invalid test asserts the combined message.
• Plugin ordering is sound — configPlugin (app.ts:84) precedes authRoutes (app.ts:126), so fastify.config is decorated when the route's config.rateLimit object is built at registration time; rateLimitPlugin also declares dependencies: ['config'].
• Empty-string handling is inherited correctly from getValue() and explicitly tested for both variables.
• Defaults preserve existing behaviour (20 / '15 minutes'), asserted in all four full-object loadConfig() tests.
• The two new required AppConfig fields were added to all seven makeConfig() factories — this is the class of change that passes Jest and fails CI's typecheck, so thanks for catching it.
• Naming conventions correct throughout (UPPER_SNAKE_CASE env → camelCase config fields); no any, no new runtime dependencies, no API-contract or schema surface touched.
• Test-file parity holds: both changed production files have co-located test files.

[steilerDev]

[product-owner]

Verdict: CHANGES_REQUIRED

Reviewed against the seven acceptance criteria on #1970. Six are met or substantially met; AC2 has a concrete, reproducible violation of its own stated guarantee, and it is the security-relevant half of the story, so this is blocking rather than a follow-up.

AC-by-AC

AC	Verdict	Evidence
1 — max + window readable from env, defaults 20/15 minutes	Partial	config.ts:354-370 reads both. max is proven end-to-end (rateLimitPlugin.test.ts AC3 test sets AUTH_RATE_LIMIT_MAX=3, 4th request 429s). window is only asserted at the config layer — see M1.
2 — invalid value → startup failure, no silent fallback, no silent disabling	NOT MET	See B1.
3 — 429 + RATE_LIMIT_EXCEEDED, body shape unchanged	Met	rateLimitPlugin.test.ts AC3 test asserts both status and error.code; errorResponseBuilder in rateLimitPlugin.ts untouched, so the shape is unchanged by construction.
4 — unset → exactly 20 / '15 minutes'	Met	Asserted twice: loadConfig({}) in config.test.ts, and app.config after a real buildApp(). Four existing whole-object toEqual assertions also pin the defaults, so they cannot drift silently.
5 — documented in CLAUDE.md and on the docs site	Met (via the AC's own escape hatch)	CLAUDE.md rows added and prettier-clean. The docs-site copy was missing with no request filed; I filed #1990 (Todo, blocked-by #1970), which is what AC5's "file a request if needed" provides for.
6 — setup route explicitly decided	Met	auth.ts:76-78 comment records the decision and the reason. The rationale holds and is in fact stronger than stated: the store is in-memory, so a container restart clears the bootstrap limiter — an operator who fumbles setup 5 times is not locked out until a rebuild.
7 — security-engineer review	Outstanding	No reviews on the PR yet. B1 should be routed to them explicitly — it is exactly the class of defect AC7 was written to catch.

---

B1 (BLOCKING, High) — AUTH_RATE_LIMIT_WINDOW=0s silently disables the login rate limit

AC2 requires that an invalid value "does not silently fall back to the default, and it does not silently disable the limit". AC7 requires "no value that removes the limit entirely unless that is an explicitly documented, deliberately named option".

A zero window passes validation and removes the limit entirely.

The validation is asymmetric: max explicitly rejects zero and negatives (config.ts:357), but the window check (config.ts:366) is a pattern match only, with no bound on the resulting duration. ^\d+(\.\d+)?\s*(ms|s|…)$ accepts 0s, 0ms, 0 minutes, and 0.5ms; ms() maps all of them to 0 or sub-millisecond.

With timeWindow: 0, LocalStore.incr evaluates current.iterationStartMs + 0 <= nowInMs — always true — so every request is treated as a fresh window and the counter resets to 1 on each call. It never reaches max. Verified against the vendored @fastify/rate-limit@11.1.0 local store (the store actually in use — rateLimitPlugin.ts configures no custom store):

timeWindow=0      -> highest counter reached over 50 requests: 1   (max=3, so the limit never fires)
timeWindow=900000 -> highest counter reached over 50 requests: 50

So AUTH_RATE_LIMIT_WINDOW=0s turns the login limiter off with no error, no warning, and no log line distinguishing it from a working configuration. That is precisely the outcome AC2 and AC7 forbid, reached by a value a plausible operator could type while trying to tighten the limit.

Required behaviour (implementation approach is the architect's and the developer's call, not mine):

1. A configured window that resolves to a non-positive or sub-second duration must fail startup with an error naming AUTH_RATE_LIMIT_WINDOW, consistent with how max treats zero.
2. Per AC2's "asserted by a test per invalid-input class", add a test for the zero-window class — at minimum 0s; 0ms and 0.5ms are the same class.

One observation for whoever implements it: the regex re-implements ms's grammar, and the two have already drifted in both directions — the regex accepts 0.5ms (which ms parses to a useless 0.5) and rejects 1y (which ms accepts). Validating by calling ms() and requiring a positive, finite result would close B1 and remove the drift risk in one move, rather than adding a second guard beside a hand-maintained duplicate of the library's grammar. Not a requirement — the required outcome is item 1 above.

M1 (MUST FIX, Medium) — AC1's window half is not asserted at the route

AC1 asks for "a test that sets the env vars and observes the effective limit". That exists for max (the AC3 test) but not for window: nothing proves authRateLimitWindow actually reaches the route's rateLimit config. If the timeWindow line in auth.ts were deleted, every test in this PR would still pass — the route would silently fall back to the plugin's global 1 minute.

@fastify/rate-limit emits x-ratelimit-reset as Math.ceil(ttl / 1000), so a request under AUTH_RATE_LIMIT_WINDOW=30s should report a reset of ~30 rather than the global default's ~60. That closes the gap deterministically. Raising this now rather than as a follow-up only because B1 already opens a change round.

L1 (Low) — header test asserts nothing about the values

The rate-limit headers present on login route test asserts only toBeDefined() on x-ratelimit-limit and x-ratelimit-remaining. It passes for any configured value, including a wrong one. Asserting x-ratelimit-limit === '20' would give AC4's default a second, route-level guard at no cost. Non-blocking.

Informational — deliberately not asking for changes

• parseInt leniency on max. AUTH_RATE_LIMIT_MAX=20abc yields 20 and 1e9 yields 1, both silently. I checked before flagging it: this is the established house pattern in config.ts — BACKUP_RETENTION, LLM_MAX_TOKENS, and LLM_REQUEST_TIMEOUT_MS all use the identical parseInt + isNaN || <= 0 form. Tightening one variable out of four would make the file less consistent, and repo-wide numeric-parsing strictness is a separate tech-debt item, not this story's scope. Explicitly out of scope here. (Note this is not the same as B1: a misparsed max still yields a working limit, whereas a zero window yields none.)
• Global rateLimitPlugin default (max: 200 / 1 minute) left hardcoded. Correct — Make the authentication rate limits configurable by environment variable #1970's Notes make that explicitly optional.

Scope

No scope creep. The +2/-0 in seven unrelated *.test.ts files are mandatory AppConfig fixture updates forced by the two new required interface fields, not incidental changes. #1970's stated component list is respected.

CI

Quality Gates green, all 6 Jest shards green, Docker green. E2E shards 8 and 12 still in progress at review time — E2E Gates is main-only so it does not gate this beta PR, but shard 8 is the known diary flake and is worth a glance before promotion rather than at promotion.

---

To clear this review: fix B1 and M1, then re-request. AC7's security-engineer review is still outstanding independently, and B1 should be surfaced to them as part of it.

[steilerDev]

[product-architect] Re-review of 47ee190 — APPROVED. The blocking finding is fully resolved.

Blocking finding: zero-magnitude window → 500 crash — RESOLVED

Verified in server/src/plugins/config.ts:361-374. Both halves of the fix are correct, and I verified them against the actual dependency chain rather than by inspection alone: @fastify/rate-limit/index.js:78 calls parse() from @lukeed/ms, whose parse() uses an assignment-in-condition (if (arr != null && (num = parseFloat(arr[1])))) — so a zero magnitude is falsy and parse() returns undefined, which is exactly the path that produced the 500.

1. Separator narrowed \s* → *. This now matches @lukeed/ms's own RGX, which likewise uses a space-only separator. Tabs/newlines between magnitude and unit are no longer accepted by config, closing the divergence.

2. Positive-magnitude guard. else if (parseFloat(...) <= 0) is placed correctly after the pattern test, so parseFloat only ever runs on a string the regex has already guaranteed starts with \d+ — no NaN leak, and no negative input is reachable (the pattern has no -).

Exhaustive accept-set cross-check

Rather than trust the reasoning, I generated every combination of the 22 accepted units (both cases) × 8 magnitudes × 3 separator widths and asserted that everything passing config's full gate also parses to a positive finite millisecond count:

config-accepts-but-lukeed-fails: NONE

Zero remaining divergence in the dangerous direction. The only surviving divergence is one-directional and fail-closed: config is strictly narrower than @lukeed/ms, rejecting 1y / 1yr / 2 years, 1wk / 5 wks, 100msec / 1 millisecond, .5s, and -5m at startup with an actionable message instead of accepting them. For a login rate-limit window that narrowing is the right call — a year-long or negative window is never intended — so I'm explicitly accepting it as designed, not flagging it.

Other verifications

• Config tests — server/src/plugins/config.test.ts:1092-1109 adds the three zero-magnitude cases (0s, 0 minutes, 0.0h), each asserting the specific error text rather than just "throws". Ran locally: 18/18 AUTH_RATE_LIMIT tests pass.
• Route-level coverage — server/src/plugins/rateLimitPlugin.test.ts now asserts x-ratelimit-limit on both AC3 ('3', on the 429) and AC4 ('20', on the default path). This is the important upgrade: the previous AC4 only asserted app.config.*, which would have stayed green if the config value never reached the route registration. It is now a real end-to-end assertion.
• Wiki — Architecture.md:397-398 and API-Contract.md:110-111 both carry the two new rows, plus the new "Rate Limiting (Auth)" subsection (API-Contract.md:119-123) documenting the 429 shape, RATE_LIMIT_EXCEEDED code, and the x-ratelimit-* headers. Submodule ref is genuinely published — git ls-remote origin master and the parent tree's wiki entry both resolve to 5c1c7e7, so the ref is committed on the branch and not left dangling. A Deviation Log entry was added covering the missing TRUST_PROXY row found along the way.
• CI on 47ee190 — Quality Gates, Static Analysis, Trailer Check, all 6 Jest shards, and Docker all green.

Non-blocking follow-ups (do not block this merge)

1. docs/src/getting-started/configuration.md has no rate-limit entry. This is the fourth place env vars are documented, and it is the only one the PR did not touch — it already documents SESSION_DURATION and TRUST_PROXY (with a sentence that references rate limiting) but not AUTH_RATE_LIMIT_MAX / AUTH_RATE_LIMIT_WINDOW. Correctly out of scope here, since docs/ is docs-writer-owned and updated at release staging — flagging so it is picked up then. The ## Reverse Proxy section is the natural home given the TRUST_PROXY interaction.
2. parseInt trailing-garbage tolerance on AUTH_RATE_LIMIT_MAX. parseInt('20abc', 10) yields 20 and passes validation. I am explicitly not asking for a change: all 8 numeric env vars in config.ts (PORT, SESSION_DURATION, PHOTO_MAX_FILE_SIZE_MB, DIARY_DRAFT_RETENTION_DAYS, BACKUP_RETENTION, LLM_REQUEST_TIMEOUT_MS, LLM_MAX_TOKENS, and this one) behave identically, so tightening only the new one would make the file inconsistent. Worth a dedicated file-wide pass if we ever want strict numeric parsing.
3. OIDC_REDIRECT_URI documented but nonexistent. Already captured in the Deviation Log as unresolved and deliberately deferred. It spans Architecture.md, API-Contract.md, and the CLAUDE.md env table; keeping it out of this PR was the right scoping call. Needs its own issue.

VERDICT: APPROVED

[steilerDev]

[product-owner]

Verdict: APPROVED — with one MUST FIX before merge

Round 2 review of 47ee190 against the seven acceptance criteria on #1970.

B1 is resolved. M1 is not — the added assertions prove max reaches the route, not timeWindow, so the commit message's claim ("deleting timeWindow from auth.ts would break this test") does not hold. It is still a one-line assertion, still Medium, so it does not block the code gate — but it must land on this branch, and I am not moving #1970 to Done without it.

---

B1 — RESOLVED: zero-magnitude window rejected at startup

server/src/plugins/config.ts:371-375

} else if (parseFloat(authRateLimitWindowStr) <= 0) {
  errors.push(`AUTH_RATE_LIMIT_WINDOW must have a positive duration (zero magnitude is not allowed), got: ${authRateLimitWindowStr}`);
}

Verified:

• 0s, 0 minutes, 0.0h, 00 minutes → parseFloat yields 0 → rejected with a message naming the variable. Three of these are covered by new tests in config.test.ts:1093-1109.
• The pattern change \s* → * (config.ts:365) also closes the second instance product-architect flagged: 15\tminutes / 15\nminutes no longer pass validation while @lukeed/ms would reject them.
• 0 with no unit and +0s were already rejected by the anchored pattern.

Worth recording for the file: my round-1 mechanism was wrong and product-architect's was right. I described timeWindow: 0 as silently disabling the limiter via LocalStore.incr; the verified behaviour is that @lukeed/ms parse() returns undefined for a zero magnitude, mergeParams()'s if/else if chain never falls through to defaultTimeWindow, and every login request then 500s on params.timeWindow is not a function. Same AC2 violation, worse blast radius, same fix — but the diagnosis in my round-1 comment should not be cited later as the mechanism.

AC2 and AC7 are now met: there is no accepted value that removes the login limit.

M1 — STILL OPEN (MUST FIX before merge): no assertion observes the effective window at the route

server/src/plugins/rateLimitPlugin.test.ts:137 and :153

The two new assertions are on x-ratelimit-limit ('3' and '20'). That header is fed by max only. In @fastify/rate-limit's mergeParams() (node_modules/@fastify/rate-limit/index.js:163-175), max and timeWindow are handled in two independent branches, and route options are merged over globalParams with Object.assign. So if timeWindow: fastify.config.authRateLimitWindow were deleted from auth.ts:147:

• result.timeWindow silently inherits the global '1 minute' from rateLimitPlugin.ts:10,
• result.max is unaffected → x-ratelimit-limit is still 3 / 20,
• both new assertions stay green, and the 15-minute window is gone.

That is exactly the regression AC1 ("observes the effective limit") and AC4 ("asserted, so the default cannot drift silently") were written to catch, and it is the half of the pair that still is not caught.

The vehicle is already there. x-ratelimit-reset carries the window as Math.ceil(ttl/1000) (index.js:265, :313) and is emitted on non-exceeded responses too — the pre-existing setup test at rateLimitPlugin.test.ts:50 already reads it. Add to the AC4 default-config test:

// Window reaches the route too: 15 minutes → 900s, not the global 1-minute fallback (60)
expect(response.headers['x-ratelimit-reset']).toBe('900');

Optionally also a non-default case (AUTH_RATE_LIMIT_WINDOW='2 minutes' → '120') to pin the env→route path for the window the way AUTH_RATE_LIMIT_MAX=3 does for max.

To be unambiguous about the gate: merge is a code gate, Done is an acceptance gate. This PR is approved to merge on the strength of B1, but AC1 and AC4 both contain an explicit assertion clause and the window half of each is currently satisfied only at the config layer. If this merges without the assertion, #1970 does not go to Done and I will reopen it rather than file a follow-up.

---

AC status

AC	Status	Note
1	Partially met	max proven at the route; window proven only at the config layer — see M1
2	Met	Non-numeric, zero, negative max; unparseable and zero-magnitude window — each with a test
3	Met	429 + RATE_LIMIT_EXCEEDED, body shape unchanged, now with x-ratelimit-limit: '3'
4	Partially met	Same gap as AC1 — max: 20 proven at the route, '15 minutes' not
5	Met	CLAUDE.md rows present; docs-site copy filed as #1990 (docs-writer, cross-references TRUST_PROXY per the issue Notes)
6	Met	auth.ts:76-78 records why the setup limit stays hardcoded. Accepted: the store is in-memory, so a restart clears the bootstrap limiter, and the route 403s unconditionally once setup is complete
7	Met	security-engineer reviewed; its only non-informational finding was the zero-window Medium, now fixed

Rulings on the other reviewers' open findings

• parseInt leniency (product-architect Medium, security-engineer Low): confirmed as house convention across eight call sites in loadConfig(), and out of scope for Make the authentication rate limits configurable by environment variable #1970 — a local /^\d+$/ guard on one variable would leave seven inconsistent siblings. Filed as Tighten integer parsing for numeric environment variables in loadConfig() #1991 (tech-debt, Could Have, Backlog) with the whitespace-padding inconsistency folded in. This is the decision product-architect asked to have noted in the thread. No change wanted on this branch.
• Wiki env-var tables (product-architect Medium): resolved. Submodule bumped to 5c1c7e71, verified present on origin/master. Both tables carry the two new rows, TRUST_PROXY was backfilled, and a "Rate Limiting (Auth)" subsection documents the 429 shape, the headers, and the proxy caveat.
• OIDC_REDIRECT_URI deviation: the Deviation Log entry flags it as an unresolved follow-up but nothing tracked it. Verified real — config.ts:142 gates OIDC on three variables and never reads OIDC_REDIRECT_URI. Filed as Wiki documents a nonexistent OIDC_REDIRECT_URI env var and a four-variable OIDC gate #1992 (documentation, Should Have, Todo, product-architect). CLAUDE.md and the docs site are already correct, so it is wiki-only. Correctly kept out of this PR's scope.
• AC6 comment placement (informational): no action. The rationale satisfies AC6 wherever it sits.
• Rate-limit headers revealing the configured max (informational, accepted risk): agreed, no product objection — clients need it for backoff and an attacker can measure it anyway.

Quality Gates green on 47ee190; five E2E shards still in progress, not required for a beta merge.

[steilerDev]

[product-owner] Round 3 (final) — M1 resolved.

Verdict: APPROVED

5446b29a applies the fix verbatim on the AC4 test (server/src/plugins/rateLimitPlugin.test.ts:157):

expect(response.headers['x-ratelimit-reset']).toBe('900');

What I verified

1. The assertion is correct. LocalStore.incr sets ttl: timeWindow on the first request in a fresh window (store/LocalStore.js:17), and the header is Math.ceil(res.ttl / 1000) (index.js:265, emitted on non-exceeded responses via addHeaders). Each test in this block builds its own app with a fresh in-memory store, so the first POST /api/auth/login reads ttl === 900_000 exactly — 900 is deterministic, not timing-sensitive. No flake risk introduced.
2. The assertion is meaningful. I mutation-tested the stated counterfactual: deleting timeWindow: fastify.config.authRateLimitWindow from server/src/routes/auth.ts:147 (leaving max in place) makes the route inherit the plugin's global timeWindow: '1 minute' (rateLimitPlugin.ts), and the test fails with Expected: "900" / Received: "60". Reverted immediately; auth.ts is unchanged from HEAD. This is exactly the drift AC1/AC4 were written to catch, and it is now caught.
3. Suite green: 5/5 pass in rateLimitPlugin.test.ts unmutated.

Both halves of the route's rate-limit config are now observed at the route: max via x-ratelimit-limit ('3' in AC3, '20' in AC4) and the window via x-ratelimit-reset ('900'). All 7 acceptance criteria on #1970 are met with no unverifiable AC remaining.

Done gate

My round-2 ruling — merge is a code gate, Done is an acceptance gate — is now satisfied on both sides: #1970 is eligible for Done on merge, no reopen and no substitute follow-up needed.

Follow-ups #1990 (docs-site copy, must cross-reference TRUST_PROXY), #1991 (uniform integer parsing), and #1992 (wiki OIDC_REDIRECT_URI deviation) remain tracked independently and do not gate this PR.

No further findings. Approving; CI gates remain the merge precondition.

[github-actions]

🎉 This PR is included in version 2.14.0-beta.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

[github-actions]

🎉 This PR is included in version 2.14.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀