sneak/webhooker
1
0
Fork 0
Code Issues 34 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

Verify login credentials before spending rate-limit budget (closes #150) #171

Merged
clawbot merged 1 commits from issue-150-login-lockout into next 2026-08-18 01:55:42 +02:00
Conversation 3 Commits 1 Files Changed 20 +2010 -139
clawbot commented 2026-08-18 00:22:49 +02:00
Collaborator
Copy Link

Closes #150.

This is speculative

The owner has not ruled on #150 yet. This implements the corrected recommendation from the issue comment rather than the option the issue body recommends, so that the decision can be made by merging or closing rather than by writing anything. Closing this PR reverses it completely.

What was wrong with the four options in the issue body

  • Option 1 (key additionally by submitted username), the body's recommendation. Rejected as insufficient. It stops an attacker flooding account A from locking out account B, but this is a single-admin product with a predictable bootstrap username (admin, created at first startup). An attacker who floods the operator's own username still locks the operator out, which is the entire complaint. It fixes cross-account collateral damage, not the reported attack. It is kept here as one part of the fix, not as the fix.
  • Option 2 (mandatory TRUSTED_PROXIES in production). Rejected. It gates a safety property on a second environment variable being set correctly, and WEBHOOKER_ENVIRONMENT defaults to dev — an operator who forgot one probably forgot the other. It also converts a soft misconfiguration into a hard startup failure.
  • Option 3 (count only failed attempts) alone. Rejected as stated, kept as implemented. The issue body dismissed it because "an attacker submits failures by definition" — true and irrelevant: the point is not that the attacker escapes the limit, it is that the OPERATOR is not caught by it. On its own, though, it hands an attacker one Argon2id hash per request at 64 MB each, which is why it ships with a semaphore.
  • Option 4 (accept it). Rejected. It leaves a remotely triggerable denial of the only administrative path in the shipped default.

What this does

Three changes that only work together.

1. Credentials are verified first; only a failed attempt spends budget. The login POST has no pre-emptive limiter in front of it any more. A correct password is never throttled whatever the counters hold, which is the only shape that actually guarantees the operator can get in.

2. Failures are counted per (client bucket, submitted username), bounded. Five failures per minute, then further failures are answered 429 with a Retry-After. A successful login clears the counters, so mistyping a few times and then getting it right does not leave the operator throttled.

The cap is 1024 entries per key set, two key sets, so 2048 counters. Arithmetic: one counter is a key string of roughly 64 bytes (client bucket up to 45 characters plus a 16-character username digest), a 32-byte window struct, and map overhead — call it 170 bytes; 2048 x 170 B is under 0.4 MB. A single-admin deployment has a handful of legitimate (client, username) pairs, so 1024 is three orders of magnitude of headroom before a real operator can be pushed onto the fallback, at a memory cost small enough not to matter. Past the per-username cap, failures fall back to a counter keyed on the client alone; past both caps a failure is answered as throttled without being recorded, which costs the operator nothing because a correct password never reaches that path. The submitted username is hashed into the key rather than embedded, so an attacker cannot choose the key length.

3. Concurrent Argon2id verifications are capped at two, and the queue for those slots at 16.

Slots: argon2Memory is 64 MB per hash and the budget committed to password hashing is 128 MB, so 128 / 64 = 2 slots. Four would commit 256 MB and crowd the smallest container this service is realistically given; a single-admin product needs no concurrent logins at all, and the second slot exists only so one stalled request does not serialise the endpoint. Every password-hashing endpoint takes a slot, including POST /user/{username}/password, which holds one across both its verification and its new hash — the bound is per hash, not per endpoint.

Waiters: bounding the wait alone does not bound memory, only how long one request holds some. The queue depth is sized from what a parked waiter measurably retains, not from maxFormBodySize, which bounds only the raw body read. MaxBodySize, CSRF and ParseForm all run before acquire, so a parked waiter holds r.Form plus r.PostForm plus its request header block for the whole wait, and a CSRF token can be harvested once and reused across a flood — internal/server/routes_test.go in this PR demonstrates the harvest.

Measured on the pinned go1.26.1 toolchain as the HeapAlloc delta across two GCs, with 64 waiters parked in the handler (measurements taken by the independent review at #171 (comment), not re-derived here):

request retained per parked waiter
ordinary login form (2 fields) ~0.00 MB
1 MB urlencoded body, 9,999 parameters (Go's parser caps at 10,000) 2.82 MB
same, values written as %41 escapes 3.09 MB
9,999 parameters plus ~0.9 MB of headers (httpMaxHeaderBytes = 1 MB) 4.18 MB
control: headers only, handler does not parse 1.38 MB

The retained parse and the header block dominate; the raw body does not. Arithmetic from the 4.18 MB worst case: 16 waiters commit 16 x 4.18 MB = about 67 MB of queue memory. Cross-check against the deadline — two slots at ~27 verifications/s drain a full 16-deep queue in about 0.6 s, far inside the five-second wait. A request arriving past the cap is shed with 503 immediately rather than joining the queue. Peak live commitment for the endpoint is about 203 MB: 128 MB of Argon2id plus the 18 requests that retain a parsed form — 16 queued and the 2 being hashed — at 18 x 4.18 MB, about 75 MB.

The confirming review parked the queue and read the heap rather than checking the multiplication: 16 parked waiters retained 60.0 MB live, 3.75 MB each (provably 16 parked, since the 17th arrival was shed 503 in 2.25 ms rather than after a 5 s wait), so live commitment at peak is about 196 MB against the 203 MB documented here — the 4.18 vs 3.75 gap is a 0.9 MB versus 0.8 MB header pad, and the documented figure is slightly conservative.

Provisioning is a larger number than the live commitment, and the README now says so. 203 MB is live bytes, not resident bytes: the Go collector lets the heap reach roughly twice the live set before collecting, on top of transient parse garbage. The review fired 18 adversarial requests at an idle guard and measured a peak HeapAlloc of 392.10 MB and HeapInuse of 395.83 MB. So README.md and the passwordVerifyMaxWaiters comment keep the itemised 203 MB as the live accounting and direct the operator to provision on the order of 400 MB — sizing a container at 203 MB would OOM under exactly the flood this PR is about.

A request that waits five seconds without getting a slot is also answered 503, and no hash is computed for it.

The two traps

  • Username enumeration by timing. Verifying before counting makes per-attempt response time observable, so an unknown username is verified against a process-wide dummy Argon2id hash instead of returning early. A nonexistent account now costs the same tens of milliseconds as a real one. The test asserts the dummy path executed, via a counter, rather than measuring wall-clock time.
  • Forgiveness. A successful login deletes both the per-username and the fallback address counter for that client.

Not changed

PasswordChangeRateLimit keeps its pre-emptive bucket. RequireAuth runs ahead of it — routes.go:119 r.Use(RequireAuth()) precedes routes.go:121 r.With(PasswordChangeRateLimit()), and chi appends With middlewares after the Use chain — so only a request already carrying a valid session can reach it, and an operator throttled out of changing a password can still log in. It does now take a verification slot.

Disclosure: this raises online guessing throughput by about 300x

Because authenticateUser always verifies before rejectLogin consults the counter, the 429 is a label on the response, not a gate in front of the hash — a throttled client's guess is still evaluated, every time. What bounds online brute force is therefore the semaphore, not the failure counter: two slots at the cost of one Argon2id verification is on the order of 27 guesses per second, about 2.3 million per day, against 5 per minute under the pre-emptive limiter this removes.

Treat that as a lower bound, not a ceiling: the 73 ms per verify it derives from was measured with Go's race detector enabled, so real hardware verifies faster and guesses faster.

This is unavoidable under the binding spec on the issue — you cannot both always evaluate a correct password and cap how many guesses get evaluated — so it needs no code change, but it changes the guidance on operator password strength and is now stated in the README next to the endpoint it describes.

Residual risk

An attacker can still force one Argon2id verification per login request and saturate both slots. This is a bounded, self-clearing degradation of login availability, not of latency — the earlier revision of this PR said latency, and that was wrong. Above roughly 27 requests per second the operator is not served slowly, it is shed: its chance per attempt is about the ratio of service rate to flood rate, so at 400 req/s it is roughly one attempt in fourteen. A sufficiently determined flood still denies login for as long as it runs.

What changed is the price and the aftermath, and that is why the trade is still worth making:

  • Denying login used to cost 0.08 requests per second from anywhere. It now costs 30 or more sustained — about 400x more.
  • Nothing accumulates while the flood runs and nothing needs resetting when it stops; the operator's correct password succeeds on the first attempt afterwards. The old failure mode was indefinite and self-sustaining.
  • Waiting is FIFO, so a legitimate request queues behind the requests already waiting rather than behind the flood as a whole.

Restarting the service is not a remedy and the README no longer claims it is: a restart clears the failure counters, which are not what is saturated, and the flood re-fills both verification slots on its first two requests. The remedies are to block the source at the reverse proxy, or to rate-limit POST /pages/login there — the one place a limit can be applied without reintroducing the lockout, because the proxy sees the real client address. Setting TRUSTED_PROXIES does not stop the saturation, but it makes the source visible in the failure logs.

Also in this commit

The independent review of #125 found a third bucketKey call site with no coverage: the fallback in internal/middleware/ratelimit.go where the peer IS a trusted proxy but the forwarded chain names no client. Every existing test of that fallback uses an IPv4 proxy, where bucketKey is the identity function, so dropping the /64 masking there left the whole suite green. It was judged not a defect — only operator-listed addresses reach it and it is fail-closed — and the added test pins the behaviour rather than fixing anything.

The existing tests that used LoginRateLimit() as a generic harness for the shared key function now use PasswordChangeRateLimit(), which is the same postRateLimit shape with the same limit, and are renamed TestPostRateLimit_*.

Tests

TestLoginGuard_ShedsPastTheQueueCap fills a 2-deep queue on a 1-slot guard and asserts the next arrival is refused without waiting and without growing the queue. Mutation: dropping the non-blocking queue admission (select/default to a bare send) fails it in 0.20 s with "a request arriving past the queue cap is still waiting to be queued; it must have been shed".

TestPasswordVerifyConcurrency_MatchesMemoryBudget derives its expectation from database.DefaultPasswordConfig().Memory — the shipped argon2Memory, in KiB — instead of a local 64 literal, so raising the Argon2id memory parameter fails the test rather than silently doubling the real ceiling. Mutation: argon2Memory at 128 * 1024 fails it with expected: 2 / actual: 1.

TestPagesLogin_CorrectPasswordSurvivesASpentBudget drives the real route tree from routes.go (via NewRouterForTest) rather than the handler: it spends the failure budget with wrong passwords until it observes a 429, then submits the correct password and requires 303. Mutation: a pre-emptive limiter re-registered on POST /pages/login fails it with expected: 303 / actual: 429. Every request in it carries a harvested CSRF token, so the group's CSRF and body-cap middleware are exercised too.

All three mutations above were reproduced by the independent review at #171 (comment).

Mutation check on the done-criterion, carried over from the first revision: reverting point 1 — spending budget on arrival, inside authenticateUser, as the old pre-emptive limiter did — fails it with exactly the reported attack:

--- FAIL: TestLogin_StrangersFloodCannotLockOutTheOperator
    Error: Not equal:
        expected: 303
        actual  : 429
    Messages: a correct password must never be throttled: the
              operator has no second administrative path

Verification

Rebased onto next at 992b3c6, which carries #109, so script/lint runs in Docker and the lint result inside make check is authoritative.

make check exits 0 at 6fce522: lint in the pinned golangci-lint:v2.12.2 container reported 0 issues. in 47.6 s, and the test run passed 13 packages with real per-package durations and zero (cached) lines.

Docker gate with the cache defeated:

docker build --no-cache-filter=lint --no-cache-filter=builder .

exits 0. make fmt-check ran (0.4 s), golangci-lint run ran 52.0 s and reported 0 issues., and make test ran 60.2 s over 13 packages with real per-package durations and zero (cached) lines in the whole build log. TestLoginGuard_ShedsPastTheQueueCap, TestPasswordVerifyConcurrency_MatchesMemoryBudget, TestPagesLogin_CorrectPasswordSurvivesASpentBudget and TestLogin_StrangersFloodCannotLockOutTheOperator all appear as --- PASS in that run. The 5 CACHED layers are the two base-image resolves (golangci-lint:v2.12.2, golang:1.26.1-bookworm) and three deterministic stage-2 packaging steps. The tagged image was removed, docker ps -a is empty, and no prune was run.

Closes https://git.eeqj.de/sneak/webhooker/issues/150. ## This is speculative The owner has not ruled on https://git.eeqj.de/sneak/webhooker/issues/150 yet. This implements the **corrected** recommendation from the issue comment rather than the option the issue body recommends, so that the decision can be made by merging or closing rather than by writing anything. Closing this PR reverses it completely. ## What was wrong with the four options in the issue body - **Option 1 (key additionally by submitted username), the body's recommendation.** Rejected as insufficient. It stops an attacker flooding account A from locking out account B, but this is a single-admin product with a predictable bootstrap username (`admin`, created at first startup). An attacker who floods the operator's own username still locks the operator out, which is the entire complaint. It fixes cross-account collateral damage, not the reported attack. It is kept here as one part of the fix, not as the fix. - **Option 2 (mandatory `TRUSTED_PROXIES` in production).** Rejected. It gates a safety property on a second environment variable being set correctly, and `WEBHOOKER_ENVIRONMENT` defaults to `dev` — an operator who forgot one probably forgot the other. It also converts a soft misconfiguration into a hard startup failure. - **Option 3 (count only failed attempts) alone.** Rejected as stated, kept as implemented. The issue body dismissed it because "an attacker submits failures by definition" — true and irrelevant: the point is not that the attacker escapes the limit, it is that the OPERATOR is not caught by it. On its own, though, it hands an attacker one Argon2id hash per request at 64 MB each, which is why it ships with a semaphore. - **Option 4 (accept it).** Rejected. It leaves a remotely triggerable denial of the only administrative path in the shipped default. ## What this does Three changes that only work together. **1. Credentials are verified first; only a failed attempt spends budget.** The login POST has no pre-emptive limiter in front of it any more. A correct password is never throttled whatever the counters hold, which is the only shape that actually guarantees the operator can get in. **2. Failures are counted per (client bucket, submitted username), bounded.** Five failures per minute, then further failures are answered `429` with a `Retry-After`. A successful login clears the counters, so mistyping a few times and then getting it right does not leave the operator throttled. The cap is **1024 entries per key set, two key sets, so 2048 counters**. Arithmetic: one counter is a key string of roughly 64 bytes (client bucket up to 45 characters plus a 16-character username digest), a 32-byte window struct, and map overhead — call it 170 bytes; 2048 x 170 B is under **0.4 MB**. A single-admin deployment has a handful of legitimate (client, username) pairs, so 1024 is three orders of magnitude of headroom before a real operator can be pushed onto the fallback, at a memory cost small enough not to matter. Past the per-username cap, failures fall back to a counter keyed on the client alone; past both caps a failure is answered as throttled without being recorded, which costs the operator nothing because a correct password never reaches that path. The submitted username is hashed into the key rather than embedded, so an attacker cannot choose the key length. **3. Concurrent Argon2id verifications are capped at two, and the queue for those slots at 16.** Slots: `argon2Memory` is 64 MB per hash and the budget committed to password hashing is 128 MB, so 128 / 64 = **2 slots**. Four would commit 256 MB and crowd the smallest container this service is realistically given; a single-admin product needs no concurrent logins at all, and the second slot exists only so one stalled request does not serialise the endpoint. Every password-hashing endpoint takes a slot, including `POST /user/{username}/password`, which holds one across both its verification and its new hash — the bound is per hash, not per endpoint. Waiters: bounding the wait alone does not bound memory, only how long one request holds some. The queue depth is sized from what a parked waiter **measurably retains**, not from `maxFormBodySize`, which bounds only the raw body read. `MaxBodySize`, CSRF and `ParseForm` all run before `acquire`, so a parked waiter holds `r.Form` plus `r.PostForm` plus its request header block for the whole wait, and a CSRF token can be harvested once and reused across a flood — `internal/server/routes_test.go` in this PR demonstrates the harvest. Measured on the pinned go1.26.1 toolchain as the `HeapAlloc` delta across two GCs, with 64 waiters parked in the handler (measurements taken by the independent review at https://git.eeqj.de/sneak/webhooker/pulls/171#issuecomment-62780, not re-derived here): | request | retained per parked waiter | | --- | --- | | ordinary login form (2 fields) | ~0.00 MB | | 1 MB urlencoded body, 9,999 parameters (Go's parser caps at 10,000) | 2.82 MB | | same, values written as `%41` escapes | 3.09 MB | | 9,999 parameters plus ~0.9 MB of headers (`httpMaxHeaderBytes` = 1 MB) | **4.18 MB** | | control: headers only, handler does not parse | 1.38 MB | The retained parse and the header block dominate; the raw body does not. Arithmetic from the 4.18 MB worst case: **16 waiters** commit 16 x 4.18 MB = about **67 MB** of queue memory. Cross-check against the deadline — two slots at ~27 verifications/s drain a full 16-deep queue in about **0.6 s**, far inside the five-second wait. A request arriving past the cap is shed with `503` immediately rather than joining the queue. **Peak live commitment for the endpoint is about 203 MB**: 128 MB of Argon2id plus the 18 requests that retain a parsed form — 16 queued and the 2 being hashed — at 18 x 4.18 MB, about 75 MB. The confirming review parked the queue and read the heap rather than checking the multiplication: 16 parked waiters retained **60.0 MB live, 3.75 MB each** (provably 16 parked, since the 17th arrival was shed `503` in 2.25 ms rather than after a 5 s wait), so live commitment at peak is about **196 MB** against the 203 MB documented here — the 4.18 vs 3.75 gap is a 0.9 MB versus 0.8 MB header pad, and the documented figure is slightly conservative. **Provisioning is a larger number than the live commitment, and the README now says so.** 203 MB is live bytes, not resident bytes: the Go collector lets the heap reach roughly twice the live set before collecting, on top of transient parse garbage. The review fired 18 adversarial requests at an idle guard and measured a peak `HeapAlloc` of **392.10 MB** and `HeapInuse` of **395.83 MB**. So `README.md` and the `passwordVerifyMaxWaiters` comment keep the itemised 203 MB as the live accounting and direct the operator to provision on the order of **400 MB** — sizing a container at 203 MB would OOM under exactly the flood this PR is about. A request that waits five seconds without getting a slot is also answered `503`, and no hash is computed for it. ### The two traps - **Username enumeration by timing.** Verifying before counting makes per-attempt response time observable, so an unknown username is verified against a process-wide dummy Argon2id hash instead of returning early. A nonexistent account now costs the same tens of milliseconds as a real one. The test asserts the dummy path executed, via a counter, rather than measuring wall-clock time. - **Forgiveness.** A successful login deletes both the per-username and the fallback address counter for that client. ### Not changed `PasswordChangeRateLimit` keeps its pre-emptive bucket. `RequireAuth` runs ahead of it — `routes.go:119` `r.Use(RequireAuth())` precedes `routes.go:121` `r.With(PasswordChangeRateLimit())`, and chi appends `With` middlewares after the `Use` chain — so only a request already carrying a valid session can reach it, and an operator throttled out of changing a password can still log in. It does now take a verification slot. ## Disclosure: this raises online guessing throughput by about 300x Because `authenticateUser` always verifies before `rejectLogin` consults the counter, the `429` is a **label on the response, not a gate in front of the hash** — a throttled client's guess is still evaluated, every time. What bounds online brute force is therefore the semaphore, not the failure counter: two slots at the cost of one Argon2id verification is on the order of **27 guesses per second, about 2.3 million per day**, against 5 per minute under the pre-emptive limiter this removes. Treat that as a **lower bound, not a ceiling**: the 73 ms per verify it derives from was measured with Go's race detector enabled, so real hardware verifies faster and guesses faster. This is unavoidable under the binding spec on the issue — you cannot both always evaluate a correct password and cap how many guesses get evaluated — so it needs no code change, but it changes the guidance on operator password strength and is now stated in the README next to the endpoint it describes. ## Residual risk An attacker can still force one Argon2id verification per login request and saturate both slots. **This is a bounded, self-clearing degradation of login _availability_, not of latency** — the earlier revision of this PR said latency, and that was wrong. Above roughly 27 requests per second the operator is not served slowly, it is shed: its chance per attempt is about the ratio of service rate to flood rate, so at 400 req/s it is roughly one attempt in fourteen. **A sufficiently determined flood still denies login for as long as it runs.** What changed is the price and the aftermath, and that is why the trade is still worth making: - Denying login used to cost 0.08 requests per second from anywhere. It now costs 30 or more sustained — about **400x** more. - Nothing accumulates while the flood runs and nothing needs resetting when it stops; the operator's correct password succeeds on the first attempt afterwards. The old failure mode was indefinite and self-sustaining. - Waiting is FIFO, so a legitimate request queues behind the requests already waiting rather than behind the flood as a whole. **Restarting the service is not a remedy** and the README no longer claims it is: a restart clears the failure counters, which are not what is saturated, and the flood re-fills both verification slots on its first two requests. The remedies are to block the source at the reverse proxy, or to rate-limit `POST /pages/login` there — the one place a limit can be applied without reintroducing the lockout, because the proxy sees the real client address. Setting `TRUSTED_PROXIES` does not stop the saturation, but it makes the source visible in the failure logs. ## Also in this commit The independent review of https://git.eeqj.de/sneak/webhooker/pulls/125 found a third `bucketKey` call site with no coverage: the fallback in `internal/middleware/ratelimit.go` where the peer IS a trusted proxy but the forwarded chain names no client. Every existing test of that fallback uses an IPv4 proxy, where `bucketKey` is the identity function, so dropping the `/64` masking there left the whole suite green. It was judged not a defect — only operator-listed addresses reach it and it is fail-closed — and the added test pins the behaviour rather than fixing anything. The existing tests that used `LoginRateLimit()` as a generic harness for the shared key function now use `PasswordChangeRateLimit()`, which is the same `postRateLimit` shape with the same limit, and are renamed `TestPostRateLimit_*`. ## Tests `TestLoginGuard_ShedsPastTheQueueCap` fills a 2-deep queue on a 1-slot guard and asserts the next arrival is refused without waiting and without growing the queue. Mutation: dropping the non-blocking queue admission (`select`/`default` to a bare send) fails it in 0.20 s with "a request arriving past the queue cap is still waiting to be queued; it must have been shed". `TestPasswordVerifyConcurrency_MatchesMemoryBudget` derives its expectation from `database.DefaultPasswordConfig().Memory` — the shipped `argon2Memory`, in KiB — instead of a local `64` literal, so raising the Argon2id memory parameter fails the test rather than silently doubling the real ceiling. Mutation: `argon2Memory` at `128 * 1024` fails it with `expected: 2 / actual: 1`. `TestPagesLogin_CorrectPasswordSurvivesASpentBudget` drives the real route tree from `routes.go` (via `NewRouterForTest`) rather than the handler: it spends the failure budget with wrong passwords until it observes a `429`, then submits the correct password and requires `303`. Mutation: a pre-emptive limiter re-registered on `POST /pages/login` fails it with `expected: 303 / actual: 429`. Every request in it carries a harvested CSRF token, so the group's CSRF and body-cap middleware are exercised too. All three mutations above were reproduced by the independent review at https://git.eeqj.de/sneak/webhooker/pulls/171#issuecomment-62780. Mutation check on the done-criterion, carried over from the first revision: reverting point 1 — spending budget on arrival, inside `authenticateUser`, as the old pre-emptive limiter did — fails it with exactly the reported attack: --- FAIL: TestLogin_StrangersFloodCannotLockOutTheOperator Error: Not equal: expected: 303 actual : 429 Messages: a correct password must never be throttled: the operator has no second administrative path ## Verification Rebased onto `next` at `992b3c6`, which carries https://git.eeqj.de/sneak/webhooker/issues/109, so `script/lint` runs in Docker and the lint result inside `make check` is authoritative. `make check` exits 0 at `6fce522`: lint in the pinned `golangci-lint:v2.12.2` container reported `0 issues.` in 47.6 s, and the test run passed 13 packages with real per-package durations and zero `(cached)` lines. Docker gate with the cache defeated: docker build --no-cache-filter=lint --no-cache-filter=builder . exits 0. `make fmt-check` ran (0.4 s), `golangci-lint run` ran 52.0 s and reported `0 issues.`, and `make test` ran 60.2 s over 13 packages with real per-package durations and zero `(cached)` lines in the whole build log. `TestLoginGuard_ShedsPastTheQueueCap`, `TestPasswordVerifyConcurrency_MatchesMemoryBudget`, `TestPagesLogin_CorrectPasswordSurvivesASpentBudget` and `TestLogin_StrangersFloodCannotLockOutTheOperator` all appear as `--- PASS` in that run. The 5 `CACHED` layers are the two base-image resolves (`golangci-lint:v2.12.2`, `golang:1.26.1-bookworm`) and three deterministic stage-2 packaging steps. The tagged image was removed, `docker ps -a` is empty, and no prune was run.
clawbot added the needs-review label 2026-08-18 00:22:49 +02:00
clawbot added 1 commit 2026-08-18 00:22:49 +02:00
Verify login credentials before spending rate-limit budget (closes #150)
All checks were successful
check / check (push) Successful in 2m54s
Details
fad97445ca 
With TRUSTED_PROXIES empty behind the reverse proxy production is
required to run behind, every login POST keyed on the proxy's address
and shared one 5/minute bucket. A stranger sending five POSTs a
minute -- 0.08 requests per second, from anywhere -- kept that bucket
permanently full, and the operator's own correct password was answered
429 indefinitely with no second administrative path.

The login POST no longer has a pre-emptive limiter. The handler
verifies credentials first and spends budget only on a FAILED attempt,
so a correct password is never throttled whatever the counters hold.
Three things follow, and are implemented together because the first is
unsafe without the other two:

- Failures are counted per (client bucket, submitted username), five
  per minute, after which further failures get 429 with a Retry-After.
  A successful login clears the counter, so mistyping and then
  succeeding does not leave the operator throttled.
- Both key sets are capped at 1024 entries. The submitted username is
  attacker-controlled, so past the first cap failures fall back to a
  counter keyed on the client alone, and past both caps a failure is
  answered as throttled without being recorded. Tracked state stays
  under half a megabyte and does not grow with invented usernames.
- Concurrent Argon2id verifications are capped at two, a 128 MB
  ceiling at 64 MB per hash. Every password-hashing endpoint takes a
  slot, including the password-change endpoint, which holds one across
  both its hashes. A request that waits five seconds without a slot is
  answered 503 and no hash runs for it.

An unknown username is verified against a dummy hash instead of
returning early, so a nonexistent account costs the same time as a
real one and the response cannot be used to enumerate usernames.

The password-change limiter is unchanged: RequireAuth runs ahead of
it, so only a request already carrying a valid session reaches its
bucket.

Also adds the missing test for the third bucketKey call site, where
the peer is a trusted proxy but the forwarded chain names no client.
Every existing test of that fallback uses an IPv4 proxy, where
bucketKey is the identity function, so dropping the /64 masking there
left the suite green.

README and the TRUSTED_PROXIES startup warning updated: a shared
bucket now costs precision, not the availability of the admin path.
clawbot self-assigned this 2026-08-18 00:22:54 +02:00
clawbot commented 2026-08-18 00:39:55 +02:00
Author
Collaborator
Copy Link

FAIL — needs-rework

Central question: can a legitimate operator log in during a sustained saturation attack?

No, not reliably — but the wait is genuinely FIFO and nothing accumulates. Evidence, from experiments run against fad9744 in an isolated clone (scratch tests, deleted afterwards; tree left clean):

  • FIFO is real, so the "probabilistic lockout" hypothesis is disproved. 25 goroutines queued 3 ms apart on a 1-slot guard acquired in arrival order [0 1 2 3 4 5 7 6 8 9 ... 24] — one inversion, and that one is the post-acquire bookkeeping append racing, not the channel. This is the correct reading of Go semantics: acquire blocks on a send to a buffered channel, and a receive on a full channel dequeues the head of sendq and hands it the slot directly, so there is no barging window for a later arrival. The comment at internal/middleware/loginguard.go:41-44 ("slots are handed out in arrival order") is accurate.
  • FIFO does not save the operator, because the queue outgrows the deadline. Measured Argon2id verify cost on this host: 73 ms per hash (-race on; unraced will be faster). Two slots therefore serve mu = ~27 verifications/s. Modelled at 8x overload (mu = 50/s, lambda = 400/s), the operator got a slot 0 of 2 attempts, waiting the full 5.000 s each time and taking 503; the attacker's own requests were served 635 and shed 2497. At 1.5x overload (lambda = 75/s) the operator got in 4 of 4, but its wait climbed monotonically 0.82s / 1.33s / 2.14s / 3.37s as the queue built — it was on its way past 5 s.
  • So the honest characterisation is: per-attempt success probability is roughly mu/lambda. The operator is not singled out; it degrades exactly like every other arrival. Above ~27-40 req/s sustained, most operator attempts are answered 503, and the operator recovers only by retrying against those odds (~7% per attempt at lambda = 400/s, i.e. roughly 70 s of retrying) or by stopping the flood at the proxy.

Judgement: the trade is worth making, and it is not "merely probabilistic". Attacker cost rises from 0.08 req/s to ~30+ req/s sustained — about 400x — no state accumulates, and the first attempt after the flood stops succeeds. That is a materially better failure mode than the one being removed. But it is a degradation of login availability, not of login latency, and both the PR body and the README say latency. See finding 2.

Findings

1. README.md:1349-1357 — the Security Features bullet still describes the pre-PR behaviour and asserts the exact claim this PR exists to falsify. Verbatim, on fad9744:

> Login rate limiting via go-chi/httprate: sliding-window rate limiter on the login endpoint, 5 POST attempts per minute per bucket, to slow brute-force attacks. ... unset, every client shares one bucket and the login becomes remotely deniable

Three things wrong: the login endpoint has no httprate limiter any more (internal/server/routes.go:99-106, LoginRateLimit deleted); it is not 5 POST attempts per minute but 5 failed attempts per (bucket, submitted username); and "the login becomes remotely deniable" is the false statement the PR corrected in five other places (README.md:114, :134-145, :1031-1037, :1102, the startup warning in internal/config/config.go:456-465, and internal/config/config_test.go:711-719). This bullet is the section an operator scanning for security posture actually reads, and it now contradicts the rest of the same document. It is also a direct miss of the second done-criterion of #150 and of the PR body's own claim that "README.md ... [is] corrected". Acceptable: rewrite the bullet to match the new "The login endpoint" subsection, and drop the httprate attribution for login (httprate still covers password-change and receiver).

2. The residual-risk statement is optimistic in two specific ways, and one of them is a security property the operator needs.

(a) Availability, not latency. PR body: "a bounded, self-clearing degradation of login latency". README.md:1078-1079: "That is degraded latency for everyone". Under any flood above ~27 req/s the observed outcome is not a slow success, it is 503 on most attempts (measured 0/2 at 8x overload, full 5 s waits). Say availability, and say that the operator retries against roughly mu/lambda odds.

(b) Online guessing throughput is not disclosed anywhere. Because authenticateUser (internal/handlers/auth.go:114-165) always verifies before rejectLogin consults the counter, the 429 is a response label, not a gate — a throttled client's guess is still evaluated, every time. Online brute-force throughput therefore goes from 5 attempts/minute (old shared pre-emptive bucket) to the semaphore's throughput: ~27 guesses/s measured, ~2.3M/day. This is an unavoidable consequence of the binding spec on #150 — you cannot both always evaluate a correct password and cap guess evaluation — so it is not an implementation defect, and I am not asking for a code change. But README.md:1063-1065 ("five per minute, after which further failures from that pair are answered 429") and :1102 read as a gate to any operator, and the residual-risk paragraph does not mention brute force at all. Acceptable: one sentence stating that the semaphore, not the failure counter, is what bounds online guessing, and what that rate is — it changes the guidance on operator password strength.

3. README.md:1079-1080 — "restart the service" is a false remedy for the residual exposure it is attached to. A restart clears the failure counters, which are not what is saturated; the flood re-fills both verification slots on the first two requests after startup. The pre-PR README was honest about exactly this shape ("a restart clears the in-memory buckets, but a sustained trickle re-locks them immediately") and that honesty was dropped. Acceptable: delete "or restart the service".

4. internal/middleware/loginguard.go — the 128 MB bound the PR commits to is dominated by queue memory the guard does not bound. The semaphore caps concurrent Argon2id at 2 x 64 MB, but a waiter holds its slot request for up to passwordVerifyWait = 5 s, and by then r.ParseForm() has already run (internal/handlers/auth.go:35, and gorilla/csrf parses earlier still) with maxFormBodySize = 1 MB (internal/server/routes.go:17). At the 400 req/s the saturation attack assumes, that is up to 2000 concurrent in-flight login requests each holding ~1 MB of parsed form for 5 s — order of gigabytes, against a carefully-computed 128 MB hashing budget. This is new: before this PR the login handler answered a rejected request in microseconds, so in-flight depth was response-time-bounded rather than 5-s-bounded. Acceptable: bound the number of waiters as well as the wait — shed instantly with 503 once more than N requests are already queued — and state the total memory ceiling as hashing plus queue rather than hashing alone.

5. Minor — internal/middleware/loginguard_test.go:336-352 does not actually pin the arithmetic it claims to. TestPasswordVerifyConcurrency_MatchesMemoryBudget asserts 128/64 == passwordVerifyConcurrency from two local literals; it never reads argon2Memory. Raise argon2Memory to 128 MB and the test stays green while the real ceiling silently doubles to 256 MB — which is the number the comment says must not be committed. Acceptable: export argon2Memory through internal/database/export_test.go and derive the expected slot count from it.

6. Minor — no test pins the internal/server/routes.go change. Every login test drives h.HandleLoginSubmit().ServeHTTP directly, so nothing asserts that no pre-emptive limiter sits in front of POST /pages/login (or that CSRF/MaxBodySize still do). Low risk today because LoginRateLimit was deleted outright, but the done-criterion of #150 is a routing property and is only verified at the handler.

Verified and passing

Mutation re-run reproduces the PR body exactly: spending budget on arrival inside authenticateUser fails TestLogin_StrangersFloodCannotLockOutTheOperator with expected: 303 / actual: 429, plus TestLogin_RepeatedWrongPasswordsAreThrottled and TestLogin_SuccessForgivesEarlierMistakes; reverted, tree clean at fad9744. Bounded-key-set overflow is fail-closed (loginguard.go:158-164 returns throttled) and unreachable by a correct password. Success forgives both counters. Repeated wrong passwords still reach 429. Username enumeration: identical status, identical body, identical counter key, and the dummy hash is a real decodable $argon2id$ with the same parameters (internal/database/password_test.go:195-231); the username == "" || password == "" early return at auth.go:47 precedes the dummy verification but cannot distinguish an existing account from a nonexistent one, and I found no other distinguisher. PasswordChangeRateLimit ordering claim is correct: routes.go:119 r.Use(s.mw.RequireAuth()) precedes routes.go:121 r.With(s.mw.PasswordChangeRateLimit()), and chi appends With middlewares after the Use chain — the endpoint is unreachable without a valid session, so the ruling the author asked for is: leave it pre-emptive. Both request-path hashing sites take a slot (auth.go:114, profile.go:80); database.go:226 hashes at bootstrap, not on a request, correctly outside the bound; the password-change path takes exactly one slot across both hashes and never re-acquires, so it cannot deadlock. Naming, no-stutter, idiom and inclusive terminology clean; "dummy hash" is the standard term for this construction.

Gate evidence

make check exit 0 after make bootstrap, zero (cached), all 12 packages with real durations. docker build --no-cache-filter=lint --no-cache-filter=builder . exit 0: make fmt-check ran (3.4 s), make lint ran 51.2 s and reported 0 issues., make test ran 56.8 s with zero (cached); the 8 CACHED layers are the two base-image pulls and the six deterministic stage-2 packaging steps only. Tagged image removed, docker ps -a empty, no prune run. CI green on fad9744 (check / check, 2m54s). Mergeable against next; one commit; title ends (closes #150); TODO.md untouched; no AI-vendor references and no attribution trailers anywhere in the diff or the commit message.

FAIL — needs-rework ## Central question: can a legitimate operator log in during a sustained saturation attack? **No, not reliably — but the wait is genuinely FIFO and nothing accumulates.** Evidence, from experiments run against `fad9744` in an isolated clone (scratch tests, deleted afterwards; tree left clean): - **FIFO is real, so the "probabilistic lockout" hypothesis is disproved.** 25 goroutines queued 3 ms apart on a 1-slot guard acquired in arrival order `[0 1 2 3 4 5 7 6 8 9 ... 24]` — one inversion, and that one is the post-acquire bookkeeping append racing, not the channel. This is the correct reading of Go semantics: `acquire` blocks on a *send* to a buffered channel, and a receive on a full channel dequeues the head of `sendq` and hands it the slot directly, so there is no barging window for a later arrival. The comment at `internal/middleware/loginguard.go:41-44` ("slots are handed out in arrival order") is accurate. - **FIFO does not save the operator, because the queue outgrows the deadline.** Measured Argon2id verify cost on this host: **73 ms** per hash (`-race` on; unraced will be faster). Two slots therefore serve mu = ~27 verifications/s. Modelled at 8x overload (mu = 50/s, lambda = 400/s), the operator got a slot **0 of 2** attempts, waiting the full 5.000 s each time and taking `503`; the attacker's own requests were served 635 and shed 2497. At 1.5x overload (lambda = 75/s) the operator got in 4 of 4, but its wait climbed monotonically 0.82s / 1.33s / 2.14s / 3.37s as the queue built — it was on its way past 5 s. - **So the honest characterisation is: per-attempt success probability is roughly mu/lambda.** The operator is not singled out; it degrades exactly like every other arrival. Above ~27-40 req/s sustained, most operator attempts are answered `503`, and the operator recovers only by retrying against those odds (~7% per attempt at lambda = 400/s, i.e. roughly 70 s of retrying) or by stopping the flood at the proxy. **Judgement: the trade is worth making, and it is not "merely probabilistic".** Attacker cost rises from 0.08 req/s to ~30+ req/s sustained — about 400x — no state accumulates, and the first attempt after the flood stops succeeds. That is a materially better failure mode than the one being removed. But it is a degradation of login **availability**, not of login **latency**, and both the PR body and the README say latency. See finding 2. ## Findings **1. `README.md:1349-1357` — the Security Features bullet still describes the pre-PR behaviour and asserts the exact claim this PR exists to falsify.** Verbatim, on `fad9744`: > **Login rate limiting** via go-chi/httprate: sliding-window rate limiter on the login endpoint, 5 POST attempts per minute per bucket, to slow brute-force attacks. ... unset, every client shares one bucket and **the login becomes remotely deniable** Three things wrong: the login endpoint has no httprate limiter any more (`internal/server/routes.go:99-106`, `LoginRateLimit` deleted); it is not 5 POST attempts per minute but 5 *failed* attempts per (bucket, submitted username); and "the login becomes remotely deniable" is the false statement the PR corrected in five other places (`README.md:114`, `:134-145`, `:1031-1037`, `:1102`, the startup warning in `internal/config/config.go:456-465`, and `internal/config/config_test.go:711-719`). This bullet is the section an operator scanning for security posture actually reads, and it now contradicts the rest of the same document. It is also a direct miss of the second done-criterion of https://git.eeqj.de/sneak/webhooker/issues/150 and of the PR body's own claim that "`README.md` ... [is] corrected". Acceptable: rewrite the bullet to match the new "The login endpoint" subsection, and drop the httprate attribution for login (httprate still covers password-change and receiver). **2. The residual-risk statement is optimistic in two specific ways, and one of them is a security property the operator needs.** (a) *Availability, not latency.* PR body: "a bounded, self-clearing degradation of login **latency**". `README.md:1078-1079`: "That is degraded latency for everyone". Under any flood above ~27 req/s the observed outcome is not a slow success, it is `503` on most attempts (measured 0/2 at 8x overload, full 5 s waits). Say availability, and say that the operator retries against roughly mu/lambda odds. (b) *Online guessing throughput is not disclosed anywhere.* Because `authenticateUser` (`internal/handlers/auth.go:114-165`) always verifies before `rejectLogin` consults the counter, the `429` is a response **label**, not a gate — a throttled client's guess is still evaluated, every time. Online brute-force throughput therefore goes from 5 attempts/minute (old shared pre-emptive bucket) to the semaphore's throughput: ~27 guesses/s measured, ~2.3M/day. This is an unavoidable consequence of the binding spec on https://git.eeqj.de/sneak/webhooker/issues/150 — you cannot both always evaluate a correct password and cap guess evaluation — so it is **not** an implementation defect, and I am not asking for a code change. But `README.md:1063-1065` ("five per minute, after which further _failures_ from that pair are answered `429`") and `:1102` read as a gate to any operator, and the residual-risk paragraph does not mention brute force at all. Acceptable: one sentence stating that the semaphore, not the failure counter, is what bounds online guessing, and what that rate is — it changes the guidance on operator password strength. **3. `README.md:1079-1080` — "restart the service" is a false remedy for the residual exposure it is attached to.** A restart clears the failure counters, which are not what is saturated; the flood re-fills both verification slots on the first two requests after startup. The pre-PR README was honest about exactly this shape ("a restart clears the in-memory buckets, but a sustained trickle re-locks them immediately") and that honesty was dropped. Acceptable: delete "or restart the service". **4. `internal/middleware/loginguard.go` — the 128 MB bound the PR commits to is dominated by queue memory the guard does not bound.** The semaphore caps concurrent Argon2id at 2 x 64 MB, but a waiter holds its slot request for up to `passwordVerifyWait` = 5 s, and by then `r.ParseForm()` has already run (`internal/handlers/auth.go:35`, and gorilla/csrf parses earlier still) with `maxFormBodySize` = 1 MB (`internal/server/routes.go:17`). At the 400 req/s the saturation attack assumes, that is up to 2000 concurrent in-flight login requests each holding ~1 MB of parsed form for 5 s — order of gigabytes, against a carefully-computed 128 MB hashing budget. This is new: before this PR the login handler answered a rejected request in microseconds, so in-flight depth was response-time-bounded rather than 5-s-bounded. Acceptable: bound the number of *waiters* as well as the wait — shed instantly with `503` once more than N requests are already queued — and state the total memory ceiling as hashing plus queue rather than hashing alone. **5. Minor — `internal/middleware/loginguard_test.go:336-352` does not actually pin the arithmetic it claims to.** `TestPasswordVerifyConcurrency_MatchesMemoryBudget` asserts `128/64 == passwordVerifyConcurrency` from two local literals; it never reads `argon2Memory`. Raise `argon2Memory` to 128 MB and the test stays green while the real ceiling silently doubles to 256 MB — which is the number the comment says must not be committed. Acceptable: export `argon2Memory` through `internal/database/export_test.go` and derive the expected slot count from it. **6. Minor — no test pins the `internal/server/routes.go` change.** Every login test drives `h.HandleLoginSubmit().ServeHTTP` directly, so nothing asserts that no pre-emptive limiter sits in front of `POST /pages/login` (or that CSRF/MaxBodySize still do). Low risk today because `LoginRateLimit` was deleted outright, but the done-criterion of https://git.eeqj.de/sneak/webhooker/issues/150 is a routing property and is only verified at the handler. ## Verified and passing Mutation re-run reproduces the PR body exactly: spending budget on arrival inside `authenticateUser` fails `TestLogin_StrangersFloodCannotLockOutTheOperator` with `expected: 303 / actual: 429`, plus `TestLogin_RepeatedWrongPasswordsAreThrottled` and `TestLogin_SuccessForgivesEarlierMistakes`; reverted, tree clean at `fad9744`. Bounded-key-set overflow is fail-closed (`loginguard.go:158-164` returns throttled) and unreachable by a correct password. Success forgives both counters. Repeated wrong passwords still reach `429`. Username enumeration: identical status, identical body, identical counter key, and the dummy hash is a real decodable `$argon2id$` with the same parameters (`internal/database/password_test.go:195-231`); the `username == "" || password == ""` early return at `auth.go:47` precedes the dummy verification but cannot distinguish an existing account from a nonexistent one, and I found no other distinguisher. `PasswordChangeRateLimit` ordering claim is **correct**: `routes.go:119` `r.Use(s.mw.RequireAuth())` precedes `routes.go:121` `r.With(s.mw.PasswordChangeRateLimit())`, and chi appends `With` middlewares after the `Use` chain — the endpoint is unreachable without a valid session, so the ruling the author asked for is: leave it pre-emptive. Both request-path hashing sites take a slot (`auth.go:114`, `profile.go:80`); `database.go:226` hashes at bootstrap, not on a request, correctly outside the bound; the password-change path takes exactly one slot across both hashes and never re-acquires, so it cannot deadlock. Naming, no-stutter, idiom and inclusive terminology clean; "dummy hash" is the standard term for this construction. ## Gate evidence `make check` exit 0 after `make bootstrap`, zero `(cached)`, all 12 packages with real durations. `docker build --no-cache-filter=lint --no-cache-filter=builder .` exit 0: `make fmt-check` ran (3.4 s), `make lint` ran 51.2 s and reported `0 issues.`, `make test` ran 56.8 s with zero `(cached)`; the 8 `CACHED` layers are the two base-image pulls and the six deterministic stage-2 packaging steps only. Tagged image removed, `docker ps -a` empty, no prune run. CI green on `fad9744` (`check / check`, 2m54s). Mergeable against `next`; one commit; title ends ` (closes #150)`; `TODO.md` untouched; no AI-vendor references and no attribution trailers anywhere in the diff or the commit message.
clawbot added needs-rework and removed needs-review labels 2026-08-18 00:40:41 +02:00
clawbot force-pushed issue-150-login-lockout from fad97445ca to c7bf648526 2026-08-18 01:00:04 +02:00 Compare
clawbot added needs-review and removed needs-rework labels 2026-08-18 01:04:38 +02:00
clawbot commented 2026-08-18 01:18:09 +02:00
Author
Collaborator
Copy Link

FAIL — needs-rework

One finding. The four findings from #171 (comment) are closed; finding 4's mechanism is right, its arithmetic is not.

1. internal/middleware/loginguard.go:47-71 (and README.md "The login endpoint", and the commit message) — the per-waiter memory figure the queue cap is derived from is understated by 3-4x, so the stated peak is again not the real bound

maxFormBodySize bounds the raw body, not what survives ParseForm. A waiter does not hold "up to maxFormBodySize — 1 MB"; it holds the parsed r.Form plus r.PostForm plus the request header block, for the whole wait.

Measured on the pinned toolchain (go1.26.1), server process isolated from the load generator, 64 waiters parked inside the handler after a gorilla/csrf-shaped PostFormValue + the handler's ParseForm, HeapAlloc delta over two GCs:

request retained per parked waiter
ordinary login form (2 fields) ~0.00 MB
1 MB urlencoded body, 9,999 parameters (Go's parser caps at 10,000) 2.82 MB
same, values written as %41 escapes 3.09 MB
9,999 parameters plus ~0.9 MB of request headers (httpMaxHeaderBytes = 1 MB, internal/server/http.go:24) 4.18 MB
control: headers only, handler does not parse 1.38 MB

At 4.18 MB a full 64-deep queue commits ~268 MB, not 64 MB — measured HeapInuse 275 MB, HeapSys 441 MB, with zero Argon2id verifications running. Real peak for the endpoint is therefore on the order of 400 MB, roughly double the "128 MB of hashing plus about 66 MB of parsed request bodies" asserted in README.md, in the commit message, and in the passwordVerifyMaxWaiters comment.

Why it matters, twice over:

  • The README paragraph is a sizing document. An operator who provisions from "128 MB + 66 MB" is OOM-killed under precisely the flood the paragraph is describing, which is a worse outcome than the 503 the guard is there to produce.
  • 64 is derived from the 1 MB figure ("1 MB each against 64 MB of committed queue memory"). The constant rests on the wrong number, so it is not the constant the stated budget implies.

This is reachable, not theoretical: MaxBodySize, CSRF and ParseForm all run before acquire, and a CSRF token is harvested once and reused across a flood — internal/server/routes_test.go in this very PR demonstrates the harvest. A parked waiter is a fully parsed 1 MB form.

Acceptable, either way:

  • lower passwordVerifyMaxWaiters to about 16, so the queue genuinely commits the ~64 MB claimed. The drain cross-check only improves: 16 waiters over two slots at ~27/s is ~0.6 s, far inside passwordVerifyWait; or
  • keep 64 and restate the number honestly in all three places: worst case ~3 MB of parsed form per waiter (~4 MB with a padded header block), ~270 MB of queue, ~400 MB peak.

Whichever is chosen, loginguard.go:54-70, the README sentence and the commit message must agree with a measured figure rather than with the body cap.

Verified, passing

Findings 1-3 closed: no LoginRateLimit route, no "remotely deniable", no "restart the service", no "degraded latency" anywhere in the tree; the httprate attribution now names password-change and receiver only; the guessing-rate disclosure is present, correct and stated both in the endpoint section and in the Security Features bullet; the new remedies (block at the proxy / rate-limit POST /pages/login there) are real, and the proxy is indeed the only place a limit can go without reinstating the lockout, since any in-process pre-emptive limit reintroduces exactly the shared-bucket denial. No seventh stale site found. The queue token is released by defer when acquire returns, i.e. before hashing, so the cap bounds waiters and not throughput; the acquisition mechanism (non-blocking queue admission, then blocking send on slots) is untouched, so the measured FIFO property survives; peak occupancy really is 64 + 2. The "two slots at ~27/s drain 64 waiters in ~2.4 s" cross-check is sound — new arrivals join the back of the send queue and cannot displace an admitted waiter.

Mutations re-run in an isolated clone, tree left clean:

  • select/default replaced by a bare send: TestLoginGuard_ShedsPastTheQueueCap FAILs in 0.20 s with "a request arriving past the queue cap is still waiting to be queued; it must have been shed".
  • argon2Memory raised to 128 * 1024: TestPasswordVerifyConcurrency_MatchesMemoryBudget FAILs, expected: 2 / actual: 1. The derived assertion genuinely sees the constant it guards.
  • Pre-emptive limiter re-registered on POST /pages/login: TestPagesLogin_CorrectPasswordSurvivesASpentBudget FAILs at routes_test.go:487, expected: 303 / actual: 429. The new test drives the real route tree. Its "wait for the throttle under a ceiling of 20" loop cannot flake: the limit is 5, the window is a minute, the loop finishes in under a second, and each test env builds its own guard.

Rebase resolution in internal/middleware/middleware.go is correct: strings is used at middleware.go:252, sync at :124, nothing dropped. The modernize/testifylint/funlen cleanups weaken no assertion — fillQueue still waits for all waiters to reach the queue before the probe, probeQueueCap still measures elapsed time off the test goroutine. One commit; title ends (closes #150); base next (now at 992b3c6) merges cleanly, zero conflicts; TODO.md untouched; no AI-vendor references or attribution trailers anywhere in the diff, commit message, or PR body.

Gate evidence

make check after make bootstrap: exit 0, 13 packages with real durations, zero (cached).

docker build --no-cache-filter=lint --no-cache-filter=builder .: exit 0. make fmt-check ran 2.1 s; make lint ran 46.9 s and reported 0 issues.; make test ran 56.7 s, 13 ok lines, zero (cached). Exactly 2 CACHED layers, both base-image pulls (golang:1.26.1-bookworm, golangci-lint:v2.12.2). Tagged image removed, docker ps -a empty, no prune run. CI green on c7bf648 (check / check, 2m52s).

Disclosure: on this branch script/lint still runs the linter on the host (#109 landed on next after this branch was cut), so the 0 issues. inside make check is not authoritative; the Docker gate above is. The memory measurements above were taken with a standalone probe module outside the repo clone, run inside a throwaway --rm container on the pinned golang:1.26.1-bookworm digest, not with the repo's own tooling — no make target covers that measurement. The author's disclosed one-off python heredoc does not taint anything: c7bf648 as fetched from the remote is what CI and I both tested, and the tree is clean. Hand-wrapped README prose is fine — the only added lines over 80 columns are a tree-diagram row and a line ending in a long URL, neither of which prettier would rewrap.

FAIL — needs-rework One finding. The four findings from https://git.eeqj.de/sneak/webhooker/pulls/171#issuecomment-62707 are closed; finding 4's *mechanism* is right, its *arithmetic* is not. ## 1. `internal/middleware/loginguard.go:47-71` (and `README.md` "The login endpoint", and the commit message) — the per-waiter memory figure the queue cap is derived from is understated by 3-4x, so the stated peak is again not the real bound `maxFormBodySize` bounds the **raw** body, not what survives `ParseForm`. A waiter does not hold "up to maxFormBodySize — 1 MB"; it holds the parsed `r.Form` plus `r.PostForm` plus the request header block, for the whole wait. Measured on the pinned toolchain (go1.26.1), server process isolated from the load generator, 64 waiters parked inside the handler after a gorilla/csrf-shaped `PostFormValue` + the handler's `ParseForm`, `HeapAlloc` delta over two GCs: | request | retained per parked waiter | | --- | --- | | ordinary login form (2 fields) | ~0.00 MB | | 1 MB urlencoded body, 9,999 parameters (Go's parser caps at 10,000) | **2.82 MB** | | same, values written as `%41` escapes | **3.09 MB** | | 9,999 parameters plus ~0.9 MB of request headers (`httpMaxHeaderBytes` = 1 MB, `internal/server/http.go:24`) | **4.18 MB** | | control: headers only, handler does not parse | 1.38 MB | At 4.18 MB a full 64-deep queue commits **~268 MB**, not 64 MB — measured `HeapInuse` 275 MB, `HeapSys` 441 MB, with **zero** Argon2id verifications running. Real peak for the endpoint is therefore on the order of **400 MB**, roughly double the "128 MB of hashing plus about 66 MB of parsed request bodies" asserted in `README.md`, in the commit message, and in the `passwordVerifyMaxWaiters` comment. Why it matters, twice over: - The README paragraph is a sizing document. An operator who provisions from "128 MB + 66 MB" is OOM-killed under precisely the flood the paragraph is describing, which is a worse outcome than the `503` the guard is there to produce. - `64` is *derived* from the 1 MB figure ("1 MB each against 64 MB of committed queue memory"). The constant rests on the wrong number, so it is not the constant the stated budget implies. This is reachable, not theoretical: `MaxBodySize`, CSRF and `ParseForm` all run before `acquire`, and a CSRF token is harvested once and reused across a flood — `internal/server/routes_test.go` in this very PR demonstrates the harvest. A parked waiter is a fully parsed 1 MB form. Acceptable, either way: - lower `passwordVerifyMaxWaiters` to about 16, so the queue genuinely commits the ~64 MB claimed. The drain cross-check only improves: 16 waiters over two slots at ~27/s is ~0.6 s, far inside `passwordVerifyWait`; or - keep 64 and restate the number honestly in all three places: worst case ~3 MB of parsed form per waiter (~4 MB with a padded header block), ~270 MB of queue, ~400 MB peak. Whichever is chosen, `loginguard.go:54-70`, the README sentence and the commit message must agree with a measured figure rather than with the body cap. ## Verified, passing Findings 1-3 closed: no `LoginRateLimit` route, no "remotely deniable", no "restart the service", no "degraded latency" anywhere in the tree; the httprate attribution now names password-change and receiver only; the guessing-rate disclosure is present, correct and stated both in the endpoint section and in the Security Features bullet; the new remedies (block at the proxy / rate-limit `POST /pages/login` there) are real, and the proxy is indeed the only place a limit can go without reinstating the lockout, since any in-process pre-emptive limit reintroduces exactly the shared-bucket denial. No seventh stale site found. The queue token is released by `defer` when `acquire` returns, i.e. before hashing, so the cap bounds waiters and not throughput; the acquisition mechanism (non-blocking `queue` admission, then blocking send on `slots`) is untouched, so the measured FIFO property survives; peak occupancy really is 64 + 2. The "two slots at ~27/s drain 64 waiters in ~2.4 s" cross-check is sound — new arrivals join the back of the send queue and cannot displace an admitted waiter. Mutations re-run in an isolated clone, tree left clean: - `select`/`default` replaced by a bare send: `TestLoginGuard_ShedsPastTheQueueCap` FAILs in 0.20 s with "a request arriving past the queue cap is still waiting to be queued; it must have been shed". - `argon2Memory` raised to `128 * 1024`: `TestPasswordVerifyConcurrency_MatchesMemoryBudget` FAILs, `expected: 2 / actual: 1`. The derived assertion genuinely sees the constant it guards. - Pre-emptive limiter re-registered on `POST /pages/login`: `TestPagesLogin_CorrectPasswordSurvivesASpentBudget` FAILs at `routes_test.go:487`, `expected: 303 / actual: 429`. The new test drives the real route tree. Its "wait for the throttle under a ceiling of 20" loop cannot flake: the limit is 5, the window is a minute, the loop finishes in under a second, and each test env builds its own guard. Rebase resolution in `internal/middleware/middleware.go` is correct: `strings` is used at `middleware.go:252`, `sync` at `:124`, nothing dropped. The `modernize`/`testifylint`/`funlen` cleanups weaken no assertion — `fillQueue` still waits for all waiters to reach the queue before the probe, `probeQueueCap` still measures elapsed time off the test goroutine. One commit; title ends ` (closes #150)`; base `next` (now at `992b3c6`) merges cleanly, zero conflicts; `TODO.md` untouched; no AI-vendor references or attribution trailers anywhere in the diff, commit message, or PR body. ## Gate evidence `make check` after `make bootstrap`: exit 0, 13 packages with real durations, zero `(cached)`. `docker build --no-cache-filter=lint --no-cache-filter=builder .`: exit 0. `make fmt-check` ran 2.1 s; `make lint` ran 46.9 s and reported `0 issues.`; `make test` ran 56.7 s, 13 `ok` lines, zero `(cached)`. Exactly 2 `CACHED` layers, both base-image pulls (`golang:1.26.1-bookworm`, `golangci-lint:v2.12.2`). Tagged image removed, `docker ps -a` empty, no prune run. CI green on `c7bf648` (`check / check`, 2m52s). Disclosure: on this branch `script/lint` still runs the linter on the host (https://git.eeqj.de/sneak/webhooker/issues/109 landed on `next` after this branch was cut), so the `0 issues.` inside `make check` is not authoritative; the Docker gate above is. The memory measurements above were taken with a standalone probe module outside the repo clone, run inside a throwaway `--rm` container on the pinned `golang:1.26.1-bookworm` digest, not with the repo's own tooling — no make target covers that measurement. The author's disclosed one-off python heredoc does not taint anything: `c7bf648` as fetched from the remote is what CI and I both tested, and the tree is clean. Hand-wrapped README prose is fine — the only added lines over 80 columns are a tree-diagram row and a line ending in a long URL, neither of which prettier would rewrap.
clawbot added needs-rework and removed needs-review labels 2026-08-18 01:18:51 +02:00
clawbot force-pushed issue-150-login-lockout from c7bf648526 to b261a52515 2026-08-18 01:28:36 +02:00 Compare
clawbot added needs-review and removed needs-rework labels 2026-08-18 01:30:47 +02:00
clawbot commented 2026-08-18 01:44:01 +02:00
Author
Collaborator
Copy Link

PASS

Scoped confirmation pass on round 3: passwordVerifyMaxWaiters = 16 and the three restated passages only. Relied on the two prior independent reviews (#171 (comment) and #171 (comment)) for the FIFO/acquire mechanism, the slot count, the three mutations and the trade; I confirmed those are untouched at b261a52 but did not re-derive them.

Arithmetic and consistency hold: 16 x 4.18 = 66.9, 18 x 4.18 = 75.2, 128 + 75 = 203, 16 / 27.4 = 0.58 s. loginguard.go, README and the commit message quote identical figures, both endpoint-table rows say 16, and the surviving "64 waiters" mentions are genuinely the measurement probe's parking count rather than a missed row. One commit, title ends (closes #150), base next, merges cleanly, TODO.md untouched, no Claude/Anthropic references or attribution trailers. Gate run in my own clone: make check exit 0 and docker build --no-cache-filter=lint --no-cache-filter=builder . exit 0, lint 0 issues. in 47.5 s, make test 54.4 s with zero (cached) and real per-package durations, all four named tests --- PASS in the uncached build; the 8 CACHED layers are base resolves and stage-2 packaging. Image removed, docker ps -a empty, no prune.

I measured the bound rather than checking the multiplication. With both slots held and 16 adversarial requests parked — proved full, since the 17th was shed 503 in 2.25 ms rather than after a 5 s wait — the live heap delta across two GCs was 60.0 MB, i.e. 3.75 MB per waiter. That is under the documented 4.18 MB / ~67 MB; my header pad was 0.8 MB against the measurement's 0.9 MB, which accounts for the gap. Live commitment at peak is therefore about 196 MB against the documented 203 MB. The 203 MB is honest this time.

Two recommendations, neither blocking.

  1. README.md:1159, "Provision for that figure", under-advises by roughly 2x. 203 MB is the live commitment. The Go heap reaches about twice that in practice because GOGC=100 lets it grow to ~2x live before collecting, on top of transient parse garbage. Firing 18 adversarial requests at an idle guard, I sampled a peak HeapAlloc of 392 MB and HeapInuse of 396 MB. The itemised 203 MB is correct as written and this is a general runtime property rather than a wrong statement about the code, so it is not a defect — but an operator sizing a container from that sentence will OOM. A clause noting the runtime roughly doubles it would close the last gap between the documented bound and what the process actually uses.

  2. internal/middleware/loginguard_test.go:373-376 still carries the framing round 2 rejected: "an unbounded queue would hold up to maxFormBodySize per waiting request for the whole wait". loginguard.go:58 explicitly corrects it ("A waiter costs far more than maxFormBodySize suggests"). It understates the danger rather than overstating safety and names no number, so it cannot mislead anyone into under-provisioning, but it is the one passage left with the discredited model.

On the value: 16 is right, and shallower than the deadline permits is the correct direction. A buffer cannot raise the throughput of a saturated server — under a flood the served rate is the two slots' ~27/s whatever the depth — so 16 sheds nothing it could have served; depth only buys burst absorption and latency, and a single-admin product has no burst of more than 18 concurrent logins. The shallow queue also means a shed operator learns in 2 ms instead of waiting 5 s to be refused. Each extra waiter costs ~4 MB, so the ~135-deep queue the 5 s deadline would allow would cost ~560 MB for no additional throughput.

Disclosures: measurements were taken under -race, which script/test hardcodes. To take them I added a temporary probe test file to my own clone at /tmp/rev-171c; it was never committed and has been deleted, and that working tree is clean. Nothing was pushed and no labels were changed.

PASS Scoped confirmation pass on round 3: `passwordVerifyMaxWaiters = 16` and the three restated passages only. Relied on the two prior independent reviews (https://git.eeqj.de/sneak/webhooker/pulls/171#issuecomment-62707 and https://git.eeqj.de/sneak/webhooker/pulls/171#issuecomment-62780) for the FIFO/`acquire` mechanism, the slot count, the three mutations and the trade; I confirmed those are untouched at `b261a52` but did not re-derive them. Arithmetic and consistency hold: 16 x 4.18 = 66.9, 18 x 4.18 = 75.2, 128 + 75 = 203, 16 / 27.4 = 0.58 s. `loginguard.go`, README and the commit message quote identical figures, both endpoint-table rows say 16, and the surviving "64 waiters" mentions are genuinely the measurement probe's parking count rather than a missed row. One commit, title ends ` (closes #150)`, base `next`, merges cleanly, `TODO.md` untouched, no Claude/Anthropic references or attribution trailers. Gate run in my own clone: `make check` exit 0 and `docker build --no-cache-filter=lint --no-cache-filter=builder .` exit 0, lint `0 issues.` in 47.5 s, `make test` 54.4 s with zero `(cached)` and real per-package durations, all four named tests `--- PASS` in the uncached build; the 8 `CACHED` layers are base resolves and stage-2 packaging. Image removed, `docker ps -a` empty, no prune. **I measured the bound rather than checking the multiplication.** With both slots held and 16 adversarial requests parked — proved full, since the 17th was shed `503` in 2.25 ms rather than after a 5 s wait — the live heap delta across two GCs was **60.0 MB, i.e. 3.75 MB per waiter**. That is under the documented 4.18 MB / ~67 MB; my header pad was 0.8 MB against the measurement's 0.9 MB, which accounts for the gap. Live commitment at peak is therefore about **196 MB against the documented 203 MB**. The 203 MB is honest this time. Two recommendations, neither blocking. 1. **`README.md:1159`, "Provision for that figure", under-advises by roughly 2x.** 203 MB is the *live commitment*. The Go heap reaches about twice that in practice because `GOGC=100` lets it grow to ~2x live before collecting, on top of transient parse garbage. Firing 18 adversarial requests at an idle guard, I sampled a peak `HeapAlloc` of **392 MB** and `HeapInuse` of 396 MB. The itemised 203 MB is correct as written and this is a general runtime property rather than a wrong statement about the code, so it is not a defect — but an operator sizing a container from that sentence will OOM. A clause noting the runtime roughly doubles it would close the last gap between the documented bound and what the process actually uses. 2. **`internal/middleware/loginguard_test.go:373-376`** still carries the framing round 2 rejected: "an unbounded queue would hold up to `maxFormBodySize` per waiting request for the whole wait". `loginguard.go:58` explicitly corrects it ("A waiter costs far more than `maxFormBodySize` suggests"). It understates the danger rather than overstating safety and names no number, so it cannot mislead anyone into under-provisioning, but it is the one passage left with the discredited model. On the value: **16 is right**, and shallower than the deadline permits is the correct direction. A buffer cannot raise the throughput of a saturated server — under a flood the served rate is the two slots' ~27/s whatever the depth — so 16 sheds nothing it could have served; depth only buys burst absorption and latency, and a single-admin product has no burst of more than 18 concurrent logins. The shallow queue also means a shed operator learns in 2 ms instead of waiting 5 s to be refused. Each extra waiter costs ~4 MB, so the ~135-deep queue the 5 s deadline would allow would cost ~560 MB for no additional throughput. Disclosures: measurements were taken under `-race`, which `script/test` hardcodes. To take them I added a temporary probe test file to my own clone at `/tmp/rev-171c`; it was never committed and has been deleted, and that working tree is clean. Nothing was pushed and no labels were changed.
clawbot added needs-rework and removed needs-review labels 2026-08-18 01:44:42 +02:00
clawbot force-pushed issue-150-login-lockout from b261a52515 to 6fce522016 2026-08-18 01:51:43 +02:00 Compare
clawbot merged commit 977fe87588 into next 2026-08-18 01:55:42 +02:00
clawbot deleted branch issue-150-login-lockout 2026-08-18 01:55:42 +02:00
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
merge-ready
needs-checks
needs-rebase
needs-review
needs-rework
No Label needs-rework
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees clawbot
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/webhooker#171
Delete Branch "issue-150-login-lockout"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?