lz/agent-nexus

Fork 0

Rate-limit /api/auth/unlock with one global counter #160

Merged

lz merged 3 commits from fix/unlock-rate-limit into main

2026-09-16 11:27:57 +02:00

lz commented

2026-09-15 20:14:53 +02:00

Owner

Closes #149. Implements item 1 of #110.

/api/auth/unlock had no attempt counter, no backoff and no failure logging, on an instance that is internet-facing today. Argon2id bounds the cost per guess, not the number of guesses — and the server pays that cost, so unbounded attempts were a guessing channel and a CPU-exhaustion vector at once.

Global only — per-IP was not shippable, and the reason is worse than "unverified"

#149's comment said to ship the global limiter alone if the right XFF_DEPTH could not be established with confidence. It cannot, and while checking I found a harder blocker than the one the issue describes.

Verified by reading @sveltejs/adapter-node@5.5.4 (files/handler.js:1274-1381):

ADDRESS_HEADER is read once at module scope. It is process-global — there is no per-route form.
getClientAddress() throws when ADDRESS_HEADER names a header the request lacks.

So turning it on for /unlock also redirects getClientAddress() for the four /api/agent/* callbacks, which reach host-mode Nexus directly over the workers bridge carrying no such header. Those callbacks would start throwing, taking the bridge-IP guard (fact #17) with them. Per-IP on this deployment is not "unverified", it is mutually exclusive with the worker callbacks.

Independently: X-Forwarded-For is client-appendable, and DEPLOYMENT.md's own nginx snippet does not set it at all — it sets X-Real-IP only. Under the documented config the header is whatever the caller sent, so a left-most read lets anyone mint a fresh bucket per request while the limiter looks like it works.

There is therefore no per-IP key in this PR, and a test forges a different X-Forwarded-For on every request so a naive one added later goes red instead of shipping.

One consequence worth stating plainly: a global counter means a determined attacker can keep the operator out for as long as they keep guessing. That is the trade, it is bounded (below), and DEPLOYMENT.md says so rather than implying a safety the design does not have.

What ships

UnlockLimiter (nexus/src/lib/server/auth/unlock-limiter.ts), one instance on singletons:


Free attempts	5 in a row
After that	each further attempt runs but arms a delay the next must wait out — 2s, 4s, 8s …
Ceiling	15 minutes, reached at attempt 15 — one guess per 15 min sustained
Cleared by	a correct passphrase, or 60 minutes of quiet

Four properties the issue asked for, and how each is met structurally rather than by comment:

Attempts are counted, not failures. Counting failures records nothing until the KDF returns, so one burst of concurrent requests would all pass the check and all run Argon2id at 64 MiB. Consuming up front bounds a burst of any size to 6 KDF runs.
A refusal changes no state. Refusals never reach the KDF, so they are free for the attacker; re-arming on one would sell a permanent operator lockout for the price of cheap requests. ATTEMPT_DECAY_MS is strictly longer than MAX_DELAY_MS for the mirror-image reason.
Nothing to bound. The memory-exhaustion primitive #149 warns about needs an attacker-controlled key. A single counter has none — there is no map.
A 400 costs nothing. Schema validation runs before the counter, so malformed bodies cannot spend the operator's window.

Operator lockout recovery — the design decision

Never permanent, and there is no recovery secret to lose:

Wait. The ceiling is 15 minutes; the counter clears entirely after an hour of quiet.
Restart Nexus. The counter is in-memory, so docker compose restart nexus clears it at once.

(2) is deliberate, not incidental: an operator locked out of a single-operator instance has shell access to the host by definition, and there is no second channel. Documented in DEPLOYMENT.md's new "Unlock rate limiting" section alongside the trade above.

Gates

Gate	Result
`pnpm typecheck`	4963 files, 0 errors, 0 warnings
`pnpm test`	159 files, 1715 passed (1709 on main + 19 new, 13 of which replaced none)
`pnpm lint`	clean

The two new test files were also run 8 consecutive times, with and without thread constraints, to confirm they are deterministic.

Guards proven able to fail

Every one of the 19 tests was mutation-tested: the thing it protects was broken and the test confirmed red, then restored. 20 mutations, each killing its guard:

Mutation	Tests killed
free window not honoured (`past = attempts`)	10
delay never armed	8
no doubling (flat delay)	2
lockout ceiling removed	1
a refusal re-arms the window	1
`succeeded()` does not clear the counter	7
decay shortened to the lockout ceiling	1
route's refusal path disabled	3
KDF runs before the limiter	1
a 400 consumes an attempt	1
naive per-IP key from left-most `X-Forwarded-For`	4
route never clears on success	1
no session cookie on success	1
clock-step guard removed	2
`Retry-After` sent in ms, not seconds	2
`humanDelay` drops its minutes branch	1
route consumes with a frozen clock	2
counts failures instead of attempts	8
failure log loses its identity	1
refusal logs at `warn` again	1

Two mutations did not behave as expected, and both changed the code rather than the test:

succeeded() survived its first mutation. It zeroed lastAttemptAt as well as attempts, and the decay check then reset the count a second way — so a mutation to either line passed all 13 tests. The redundant line is gone; the deadline is now derived from the count rather than stored, which removes the second field entirely.
Deriving that deadline introduced a regression, which the existing suite caught. Read naively it refuses whenever now is before the last attempt — a clock stepping backwards (NTP correction, restored snapshot) — including right after a successful unlock had cleared the count, i.e. a lockout with no counter behind it. The refusal is now gated on a delay actually being armed, with a named test for it.

An earlier mutation was also discarded as vacuous: setting FREE_ATTEMPTS = 0 left the free-window test comparing [] to []. It is a policy constant, not a mechanism, so the mechanism was mutated instead.

Review

/simplify (4 agents) and pr-review-toolkit:review-pr (5 agents) both ran. Acted on:

Dropped the redundant stored deadline (above).
The refusal logged at warn — the one path whose rate an attacker fully controls, at a level production keeps enabled. A disk-fill vector that also buried the signal. Now debug; the failed-guess warn is the attack signature and the limiter throttles it by construction.
Five test gaps, the largest being that nothing fired overlapping requests — the concurrency bound that consuming-before-the-KDF exists to enforce had never been exercised. Also added: lockout recovery (nothing proved the block ENDS, or that the route reads the live clock), exact Retry-After value, humanDelay's minutes branch, and the logging the issue explicitly requires.
Two documentation defects. Fact #29 cited fact #16 for the bridge-IP guard, which is fact #17 — a mis-citation that survived the commit whose subject was correcting false claims. And that same commit inlined delayFor() while a test comment went on naming it.
Corrected a claim I had written myself: "the only unauthenticated route that reaches the KDF" is false for a pre-setup instance, where /api/auth/setup derives a key. Both statements now say "once setup is complete".
Reused the shared readJsonBody from $server/lib/http instead of the route's local copy; cut unlock-limiter.ts from 54% comments to a pointer at fact #29.

Rejected, with reasons:

A discriminated union for UnlockAttempt — the reviewer scoped it itself to "if and when a second consumer appears". One producer, one consumer today.
The four constants as instance settings — correctness depends on ATTEMPT_DECAY_MS > MAX_DELAY_MS, and the settings registry has no cross-field validation, so exposing them would let an operator silently reconfigure the vulnerability back in.
Moving succeeded() after the session is seated — if seating throws, the operator has still proved the passphrase; leaving the counter armed would let correct guesses escalate their own lockout, which is the one outcome this file exists to prevent.
A one-shot log when getClientAddress() throws — the same misconfiguration already 500s every worker callback loudly, since those call it unwrapped.
A _handleUnlock export for DI-style testing — a test-only seam; driving the real exported POST is what makes the forged-header guard meaningful.

Deliberately not done

/api/auth/setup is unguarded before first-run completes, where it does reach the KDF with no lock around isSetupComplete. Out of scope for #149, and the marginal risk is low: an instance that has not been set up holds no secret and can be claimed outright by anyone who can reach it, so CPU exhaustion is not what is at stake in that window. Worth its own issue.
No per-IP key, for the reasons above.
No metric, UI surface or health-endpoint signal for a limiter under sustained attack — the operator learns from logs only.

Closes #149. Implements item 1 of #110. `/api/auth/unlock` had no attempt counter, no backoff and no failure logging, on an instance that is internet-facing today. Argon2id bounds the cost per guess, not the number of guesses — and the **server** pays that cost, so unbounded attempts were a guessing channel and a CPU-exhaustion vector at once. ## Global only — per-IP was not shippable, and the reason is worse than "unverified" #149's comment said to ship the global limiter alone if the right `XFF_DEPTH` could not be established with confidence. It cannot, and while checking I found a harder blocker than the one the issue describes. Verified by reading `@sveltejs/adapter-node@5.5.4` (`files/handler.js:1274-1381`): - `ADDRESS_HEADER` is read **once at module scope**. It is process-global — there is no per-route form. - `getClientAddress()` **throws** when `ADDRESS_HEADER` names a header the request lacks. So turning it on for `/unlock` also redirects `getClientAddress()` for the four `/api/agent/*` callbacks, which reach host-mode Nexus **directly** over the workers bridge carrying no such header. Those callbacks would start throwing, taking the bridge-IP guard (fact #17) with them. Per-IP on this deployment is not "unverified", it is mutually exclusive with the worker callbacks. Independently: `X-Forwarded-For` is client-appendable, and DEPLOYMENT.md's own nginx snippet **does not set it at all** — it sets `X-Real-IP` only. Under the documented config the header is whatever the caller sent, so a left-most read lets anyone mint a fresh bucket per request while the limiter looks like it works. There is therefore **no per-IP key in this PR**, and a test forges a different `X-Forwarded-For` on every request so a naive one added later goes red instead of shipping. One consequence worth stating plainly: a global counter means a determined attacker can keep the operator out for as long as they keep guessing. That is the trade, it is bounded (below), and DEPLOYMENT.md says so rather than implying a safety the design does not have. ## What ships `UnlockLimiter` (`nexus/src/lib/server/auth/unlock-limiter.ts`), one instance on `singletons`: | | | |---|---| | Free attempts | 5 in a row | | After that | each further attempt runs but arms a delay the next must wait out — 2s, 4s, 8s … | | Ceiling | 15 minutes, reached at attempt 15 — one guess per 15 min sustained | | Cleared by | a correct passphrase, or 60 minutes of quiet | Four properties the issue asked for, and how each is met structurally rather than by comment: - **Attempts are counted, not failures.** Counting failures records nothing until the KDF returns, so one burst of concurrent requests would all pass the check and all run Argon2id at 64 MiB. Consuming up front bounds a burst of any size to 6 KDF runs. - **A refusal changes no state.** Refusals never reach the KDF, so they are free for the attacker; re-arming on one would sell a permanent operator lockout for the price of cheap requests. `ATTEMPT_DECAY_MS` is strictly longer than `MAX_DELAY_MS` for the mirror-image reason. - **Nothing to bound.** The memory-exhaustion primitive #149 warns about needs an attacker-controlled key. A single counter has none — there is no map. - **A 400 costs nothing.** Schema validation runs before the counter, so malformed bodies cannot spend the operator's window. ### Operator lockout recovery — the design decision Never permanent, and there is no recovery secret to lose: 1. **Wait.** The ceiling is 15 minutes; the counter clears entirely after an hour of quiet. 2. **Restart Nexus.** The counter is in-memory, so `docker compose restart nexus` clears it at once. (2) is deliberate, not incidental: an operator locked out of a single-operator instance has shell access to the host by definition, and there is no second channel. Documented in DEPLOYMENT.md's new "Unlock rate limiting" section alongside the trade above. ## Gates | Gate | Result | |---|---| | `pnpm typecheck` | 4963 files, **0 errors**, 0 warnings | | `pnpm test` | 159 files, **1715 passed** (1709 on main + 19 new, 13 of which replaced none) | | `pnpm lint` | clean | The two new test files were also run 8 consecutive times, with and without thread constraints, to confirm they are deterministic. ## Guards proven able to fail Every one of the 19 tests was mutation-tested: the thing it protects was broken and the test confirmed red, then restored. 20 mutations, each killing its guard: | Mutation | Tests killed | |---|---| | free window not honoured (`past = attempts`) | 10 | | delay never armed | 8 | | no doubling (flat delay) | 2 | | lockout ceiling removed | 1 | | a refusal re-arms the window | 1 | | `succeeded()` does not clear the counter | 7 | | decay shortened to the lockout ceiling | 1 | | route's refusal path disabled | 3 | | KDF runs before the limiter | 1 | | a 400 consumes an attempt | 1 | | **naive per-IP key from left-most `X-Forwarded-For`** | 4 | | route never clears on success | 1 | | no session cookie on success | 1 | | clock-step guard removed | 2 | | `Retry-After` sent in ms, not seconds | 2 | | `humanDelay` drops its minutes branch | 1 | | route consumes with a frozen clock | 2 | | **counts failures instead of attempts** | 8 | | failure log loses its identity | 1 | | refusal logs at `warn` again | 1 | Two mutations did not behave as expected, and both changed the code rather than the test: - **`succeeded()` survived its first mutation.** It zeroed `lastAttemptAt` as well as `attempts`, and the decay check then reset the count a second way — so a mutation to *either* line passed all 13 tests. The redundant line is gone; the deadline is now derived from the count rather than stored, which removes the second field entirely. - **Deriving that deadline introduced a regression, which the existing suite caught.** Read naively it refuses whenever `now` is before the last attempt — a clock stepping backwards (NTP correction, restored snapshot) — including right after a successful unlock had cleared the count, i.e. a lockout with no counter behind it. The refusal is now gated on a delay actually being armed, with a named test for it. An earlier mutation was also discarded as **vacuous**: setting `FREE_ATTEMPTS = 0` left the free-window test comparing `[]` to `[]`. It is a policy constant, not a mechanism, so the mechanism was mutated instead. ## Review `/simplify` (4 agents) and `pr-review-toolkit:review-pr` (5 agents) both ran. Acted on: - Dropped the redundant stored deadline (above). - **The refusal logged at `warn`** — the one path whose rate an attacker fully controls, at a level production keeps enabled. A disk-fill vector that also buried the signal. Now `debug`; the failed-guess `warn` is the attack signature and the limiter throttles it by construction. - **Five test gaps**, the largest being that *nothing fired overlapping requests* — the concurrency bound that consuming-before-the-KDF exists to enforce had never been exercised. Also added: lockout recovery (nothing proved the block ENDS, or that the route reads the live clock), exact `Retry-After` value, `humanDelay`'s minutes branch, and the logging the issue explicitly requires. - **Two documentation defects.** Fact #29 cited fact #16 for the bridge-IP guard, which is fact #17 — a mis-citation that survived the commit whose subject was correcting false claims. And that same commit inlined `delayFor()` while a test comment went on naming it. - Corrected a claim I had written myself: "the only unauthenticated route that reaches the KDF" is false for a **pre-setup** instance, where `/api/auth/setup` derives a key. Both statements now say "once setup is complete". - Reused the shared `readJsonBody` from `$server/lib/http` instead of the route's local copy; cut `unlock-limiter.ts` from 54% comments to a pointer at fact #29. Rejected, with reasons: - **A discriminated union for `UnlockAttempt`** — the reviewer scoped it itself to "if and when a second consumer appears". One producer, one consumer today. - **The four constants as instance settings** — correctness depends on `ATTEMPT_DECAY_MS > MAX_DELAY_MS`, and the settings registry has no cross-field validation, so exposing them would let an operator silently reconfigure the vulnerability back in. - **Moving `succeeded()` after the session is seated** — if seating throws, the operator has still proved the passphrase; leaving the counter armed would let correct guesses escalate their own lockout, which is the one outcome this file exists to prevent. - **A one-shot log when `getClientAddress()` throws** — the same misconfiguration already 500s every worker callback loudly, since those call it unwrapped. - **A `_handleUnlock` export for DI-style testing** — a test-only seam; driving the real exported `POST` is what makes the forged-header guard meaningful. ## Deliberately not done - **`/api/auth/setup` is unguarded before first-run completes**, where it does reach the KDF with no lock around `isSetupComplete`. Out of scope for #149, and the marginal risk is low: an instance that has not been set up holds no secret and can be claimed outright by anyone who can reach it, so CPU exhaustion is not what is at stake in that window. Worth its own issue. - **No per-IP key**, for the reasons above. - **No metric, UI surface or health-endpoint signal** for a limiter under sustained attack — the operator learns from logs only.

lz added this to the MCP support (#140) milestone

2026-09-15 20:14:53 +02:00

lz added 3 commits

2026-09-15 20:14:54 +02:00

fix(auth): rate-limit /api/auth/unlock with one global counter cdebe507e2

/api/auth/unlock is the only unauthenticated route that reaches the KDF, and
it had no attempt counter, no backoff and no failure logging. Argon2id bounds
the cost per guess, not the number of guesses — and the SERVER pays that cost,
so unbounded attempts are a guessing channel and a CPU-exhaustion vector at
the same time. The instance is internet-facing today.

The limiter is one bucket for the whole instance, not one per source address.
Nothing in this repo configures client-IP resolution, so getClientAddress()
behind nginx returns the proxy's address: a per-IP key would have been a
global key wearing a per-IP name, reporting a safety it did not provide.
Verified in adapter-node 5.5.4 (files/handler.js) that the honest fix is worse
than none here — ADDRESS_HEADER is read once at module scope, so it is
process-global, and it throws when the named header is absent. Turning it on
for /unlock would also redirect getClientAddress() for the four /api/agent/*
callbacks, which arrive directly over the workers bridge carrying no such
header, and would break the bridge-IP guard they depend on. X-Forwarded-For is
client-appendable, and DEPLOYMENT.md's own nginx snippet does not set it at
all. So per-IP is deferred rather than approximated, and the route test sends
a different forged X-Forwarded-For on every request so that a naive per-IP key
added later goes red instead of shipping.

Attempts are counted, not failures. Counting failures records nothing until
the KDF returns, so a single burst of concurrent requests would all pass the
check and all run Argon2id at 64 MiB — which is the denial-of-service half of
the problem, reachable in one round. Consuming up front bounds concurrent KDF
runs to the free window.

A refused request changes no state. Refusals never reach the KDF, so they are
free for the attacker; were they also to re-arm the timer, cheap requests
would buy a permanent operator lockout. For the mirror-image reason the decay
period is strictly longer than the lockout ceiling: a shorter one hands the
whole free window back to an attacker who simply sits out one lockout.

Schema validation stays ahead of the counter so a malformed body cannot spend
the operator's window, and there is no map to bound because there is no
attacker-controlled key.

Lockout recovery is a decision, not an omission. The window is capped at 15
minutes rather than being permanent, and the counter is in-memory, so
restarting Nexus clears it — the documented escape for a single-operator
instance with no second channel. The trade this accepts is that a global
counter lets a determined attacker keep the operator out for as long as they
keep guessing; that is a bounded outage on a machine the operator can already
reach, and it is the price of not pretending to have a per-source key.

succeeded() deliberately leaves lastAttemptAt alone. Zeroing it as well reset
the count a second way through the decay check, which made the explicit
attempts = 0 unobservable: a mutation to either line passed all 13 tests.

refactor(auth): derive the lockout deadline, and correct a false claim 0a048e32c2

Review pass over the limiter. Three changes, one of which is a defect in the
documentation this branch itself added.

The stored `blockedUntil` field was redundant: it was only ever assigned
`lastAttemptAt + delayFor(attempts)`, so it carried nothing the other two
fields did not already encode. Deriving it makes succeeded() one line, which
matters beyond tidiness — with two fields to clear, a mutation to either one
still passed all thirteen tests, and the previous commit needed a comment to
warn about that rather than a structure that prevented it.

Deriving it did change behaviour once, and the existing suite caught it. Read
naively, a derived deadline refuses whenever `now` sits before the last
recorded attempt, which is a clock stepping backwards — an NTP correction, a
restored snapshot — and it did so even straight after a successful unlock had
cleared the count, i.e. a lockout with no counter behind it. The refusal is now
gated on a delay actually being armed, and there is a named test for it rather
than the incidental coverage that found it.

The claim that /api/auth/unlock is "the only unauthenticated route that reaches
the KDF" was false as written, in both the route comment and DEPLOYMENT.md.
/api/auth/setup also derives a key, and its isSetupComplete() check is a plain
read with no lock, so on a not-yet-configured instance it takes an unbounded
number of concurrent Argon2id runs. Both claims now say "once setup is
complete", which is the true version. The pre-setup window is deliberately left
unguarded here: an instance that has not been set up holds no secret and can be
claimed outright by anyone who can reach it, so CPU exhaustion is not the
marginal risk in that state, and bounding it belongs to its own issue rather
than to #149.

The rest is comment weight. unlock-limiter.ts was 49 comment lines to 33 of
code; the header re-derived arguments that AGENTS.md fact #29 and DEPLOYMENT.md
now hold, which is the third copy this repo keeps deleting. The route's local
readJsonBody was a byte-identical duplicate of the one in $server/lib/http that
a dozen other routes already import.

fix(auth): stop the refusal log being the flood, and close five test gaps

ci / nexus (pull_request) Successful in 12m13s

Details

ci / images (pull_request) Successful in 9m0s

Details

pr-image-cleanup / delete-pr-images (pull_request) Successful in 13s

Details

e373697d84

Review pass. One behaviour fix, five new tests, two documentation defects.

The refusal branch logged at warn, and a refusal is the one path an attacker
fully controls the rate of: it is pure arithmetic, no database and no KDF, so
sustaining a lockout wrote a structured line per request at a level production
keeps enabled. That is a disk-fill vector, and it also buried the signal the
line exists to carry. It now logs at debug. Nothing is lost: the warn on the
failed-guess path is the attack signature, and the limiter throttles that one
to roughly a line per window by construction.

The tests had five holes, and the concurrency one was the feature's whole
point. Nothing fired overlapping requests, so the bound that consuming before
the KDF exists to enforce — six Argon2id runs from a burst of any size — had
never been exercised; twenty concurrent requests now pin it. Nothing advanced
the clock, so no test proved the lockout ENDS, which is the promise
DEPLOYMENT.md makes to an operator and the one a permanent-outage bug would
break; nothing proved the route reads the live clock either, and pinning
consume() to a fixed instant passed every test before this. Retry-After was
asserted only to be positive, so dropping the division by a thousand and
sending milliseconds stayed green. humanDelay's minutes branch — the one an
operator actually meets at the cap — was never reached. And the logging the
issue explicitly asks for was asserted nowhere, so deleting every log call
passed.

Two documentation defects, both found by re-reading rather than by a test.
Fact #29 cited fact #16 for the bridge-IP guard, which lives in fact #17 — a
mis-citation that survived the commit whose subject was correcting false
claims. And that same commit inlined delayFor() into armedDelay() while a test
comment went on naming the function, which is the comment rot this repo keeps
deleting, introduced by the pass that was meant to remove it.

Kept deliberately: succeeded() still runs before the session is seated. If
seating throws the operator has already proved the passphrase, and leaving the
counter armed would let correct guesses escalate their own lockout.

lz referenced this pull request

2026-09-15 21:17:25 +02:00

Per-IP unlock rate limiting via X-Real-IP #161

lz referenced this pull request

2026-09-15 21:17:36 +02:00

/api/auth/setup reaches the KDF unguarded before first-run completes #162

lz referenced this pull request

2026-09-15 21:21:28 +02:00

Migrate to zod v4 #163

lz referenced this pull request

2026-09-15 23:02:12 +02:00

Unify the unlock redirect onto one builder #165