A login limiter that cannot lock you out, and a TOTP guard that fails closed - #368
Merged
Conversation
… closed Two findings from the review of #367. 1. /api/user/login incremented paramant:user:ratelimit:email:<email> before it called findUserByEmail, refused at eleven inside fifteen minutes, and never deleted the key on a success. The address is request input, so eleven posts over three IP addresses put the owner of that address on 429 for the full window with no way to clear it. relay.js carried the same shape one layer down: userMfaAttemptOk counted attempts against a caller-supplied user_id. The per-IP refusal stays as it was, because an IP is the caller's own resource. The per-account counters now count failures only, are cleared by a successful sign-in, and impose cost rather than denial past the threshold: a proof-of-work at the admin edge (the challenge signup and password reset already use), a capped delay at the relay, which has no client to run one. 2. relay/lib/totp.js swallowed a replay-store error with .catch(() => 'OK'), so a Redis failure accepted a code the single-use key could not mark as spent. On the path that mints admin sessions that is a replay window inside the 30 second slot, and the availability it bought was imaginary: the session store is the same Redis. It fails closed now, with an error the call sites answer as 503 rather than 401, and the admin login passes that through instead of reporting a wrong code. Docs, and the pages that described the old behaviour, follow the code: SECURITY.md carries both decisions and what was given up, docs/api.md gets the two limits and the new status codes, and /security no longer advertises a per-email limit that is not one.
Measured on a booted relay with redis killed underneath it, the previous commit did not fail closed on the login path: it did not answer at all. node-redis queues commands while it reconnects, so the secret read in front of the single-use guard neither resolved nor rejected and the request hung, twelve seconds and counting, without ever reaching the guard. Two bounds, both one second. verifyTotpGeneric races the replay SET against a deadline and reports a timeout exactly as it reports a thrown error, so the guard itself can no longer hang. relay.js wraps the secret read in front of it, so the route answers 503 instead of parking. Same test, same outage: 503 replay_store_unavailable in about a second, logged. The queue-forever behaviour belongs to the relay's redis client, not to this path, and every other redis-backed route still inherits it. Fixing that means disableOfflineQueue or a global command deadline, which changes the failure mode of every call site at once, so it is written up in SECURITY.md as open rather than smuggled in here. route-user-mfa-lockout.test.js runs the attack and the outage against a really booted relay with a real redis behind a proxy the test cuts mid-run.
…bed is gone #367 landed while this branch was open and pinned the lockout it found, on purpose: /help/session-issues was made to carry the caveat that "attempts someone else makes on your address count against you too" for exactly as long as the handler kept an address-keyed counter that ran before authentication and survived a success. That is no longer the handler, so the caveat is no longer true and the row's own else-branch says to drop it. Row 15 now reads the three properties instead of requiring the old shape to exist, so the page follows whichever way they fall. Row 6 and row 18 move to the numbers and the fail-closed behaviour this branch implements. The page states the per-IP limit as the only refusal and the ten failures as the point where an attempt starts costing work.
Apolloccrypt
force-pushed
the
fix/login-ratelimit-and-nx
branch
from
September 2, 2026 22:42
8b7ea84 to
5066fa8
Compare
…onest numbers 1. Both redis deadlines were hardcoded at 1000. PARAMANT_REDIS_DEADLINE_MS and PARAMANT_TOTP_REPLAY_TIMEOUT_MS now carry them, default unchanged, documented in deploy/.env.example (which tests/env-documented.test.mjs requires) and in SECURITY.md. Zero, a negative and a non-number all fall back to the default rather than switching the bound off: an unbounded guard does not fail closed, it hangs, which is the failure the deadline exists to prevent. 2. Under proof-of-work the per-IP counter was charged twice per sign-in, once for the 428 that asked for the proof and once for the attempt that carried it, so a budget of five left two real tries. The 428 path refunds it. Nothing was evaluated there: no account looked up, no code checked. refundIp will not create a counter that is not there, because DECR on a missing key seeds it at -1 with no TTL and would hand that IP a larger budget than it started with. 3. The docs priced the proof-of-work at "a second of CPU". Measured on this repository it is 150 to 250 ms for a native solver and one to two seconds only in a browser, where every hash goes through an awaited WebCrypto call. So the pages say what it does: it prices each automated guess and it stops a stranger from switching an account off, and the brake on guessing is the per-IP limit, which is why that one stayed a refusal.
Apolloccrypt
added a commit
that referenced
this pull request
Sep 3, 2026
…d an admin test that boots the admin Three findings from the review of #368. 1. #368 bounded one redis read on the TOTP verify path and wrote the rest up in SECURITY.md as open: "every other redis-backed route in relay.js still inherits it and will still hang in an outage". It does not any more, and it turned out to be three problems rather than one. node-redis holds commands on an offline queue while it reconnects and never retries out of it, so against a store that is gone a command neither resolves nor rejects. disableOfflineQueue refuses it instead. That covers a socket that died; it cannot see a connection that stays open and goes silent, where isReady stays true and the reply simply never comes, and that is the outage a dropped firewall rule or a wedged proxy actually produces. Only a per-command deadline catches that one. And a deadline makes the caller safe, not the client: after a command is lost that way node-redis goes on waiting for its reply and holds every later command behind it, so the connection never recovers even once the network does. Measured on 5.12.1 and 6.2.1, it reports itself ready and answers nothing until the process restarts. The guard rebuilds the connection after two unanswered commands in a row, not one, because a single slow command is a large SCAN and not a wedge. The bound sits on the client (lib/redis-deadline.js) rather than on the call sites, because there are about 300 of them across relay.js, relay/lib, envelope.js, admin/server.js and admin/lib, including the sMembers and sRem pair behind /v2/user/consume-backup that #368 bounded on the route next to it and not on this one. A per-call-site list is wrong the first time somebody adds a route. An exceeded deadline, a closed socket and an offline client all arrive as one RedisUnavailableError; redisOutage503 answers it on the 31 route-level catches that used to report an outage as 400 bad_request or 500 internal, a new top-level catch on the request handler answers it for everything else, and the admin's error middleware does the same. That top-level catch is a fix in its own right: http.createServer was handed a 4000 line async callback with nothing behind it, so any throw was an unhandled rejection, which is no answer to the client and a process exit on Node 22. PARAMANT_REDIS_DEADLINE_MS keeps the name #368 gave it and is now the single configuration source for both services. What is given up: during an outage every request pays the deadline once before it is refused, and a deployment whose redis routinely takes longer than a second must raise the knob rather than remove it. The health routes were part of the same blind spot. /v2/health/deep listed eight checks and never mentioned the store that holds every TOTP secret, so it stayed green through an outage. It now carries a redis check. The admin had no health route at all: its container probe was GET /api/auth/check, which answers 401 when nobody is signed in, so healthy meant the process still refuses me. It now has GET /health, always 200, saying degraded when redis cannot be reached. The relay's own /health touches no redis and still answers 200, which is correct. 2. POST /api/user/login did more work for an address that exists than for one that does not: a second relay call and one more redis read. The status codes were already identical, deliberately, but the clock was not. Measured on a booted admin over 200 requests per case, interleaved, one source address each, with the relay stub given a realistic 3 ms cost for the verify call, the median was 6.44 ms against 2.20 ms and the two ranges did not overlap at all, so one request classified an address with no credentials at all. Every credential answer is now held to t0 plus PARAMANT_LOGIN_MIN_ANSWER_MS, default 250 ms, and the not-found branch makes the same number of redis calls as the found one. The same measurement afterwards reads 251.37 against 251.27 ms with the ranges fully overlapping. The 429 and the 428 are not padded: their status codes tell them apart whatever the clock says, and holding the 428 back would only delay the page that is waiting to start hashing. 3. admin/test/login-ratelimit.test.js drove the limiter module through an attemptLogin() helper written inside the test file and then read server.js as a string to check the order of three calls. Both halves are worth keeping and neither ever ran the handler, so the suite could pass while the route was wrong. admin/test/_admin-server.js is the counterpart of relay/test/_relay-server.js: it spawns the real admin/server.js against a stub relay and speaks HTTP to it. login-http.test.js runs the reviewer's scenario on that, ten wrong codes on one address from three source addresses followed by the owner solving a real 2^18 proof-of-work, plus the per-IP refusal, the IP refund on a priced attempt and the relay 503 being passed through. Checked against the code it is meant to catch: three of its five tests fail against the pre-#368 admin, and so does login-timing.test.js. The admin CI job now gets a redis service and npm ci, because until now it installed nothing, which is the reason every admin suite was a lib test or a source-text assertion in the first place.
Apolloccrypt
added a commit
that referenced
this pull request
Sep 3, 2026
…d an admin test that boots the admin Three findings from the review of #368. 1. #368 bounded one redis read on the TOTP verify path and wrote the rest up in SECURITY.md as open: "every other redis-backed route in relay.js still inherits it and will still hang in an outage". It does not any more, and it turned out to be three problems rather than one. node-redis holds commands on an offline queue while it reconnects and never retries out of it, so against a store that is gone a command neither resolves nor rejects. disableOfflineQueue refuses it instead. That covers a socket that died; it cannot see a connection that stays open and goes silent, where isReady stays true and the reply simply never comes, and that is the outage a dropped firewall rule or a wedged proxy actually produces. Only a per-command deadline catches that one. And a deadline makes the caller safe, not the client: after a command is lost that way node-redis goes on waiting for its reply and holds every later command behind it, so the connection never recovers even once the network does. Measured on 5.12.1 and 6.2.1, it reports itself ready and answers nothing until the process restarts. The guard rebuilds the connection after two unanswered commands in a row, not one, because a single slow command is a large SCAN and not a wedge. The bound sits on the client (lib/redis-deadline.js) rather than on the call sites, because there are about 300 of them across relay.js, relay/lib, envelope.js, admin/server.js and admin/lib, including the sMembers and sRem pair behind /v2/user/consume-backup that #368 bounded on the route next to it and not on this one. A per-call-site list is wrong the first time somebody adds a route. An exceeded deadline, a closed socket and an offline client all arrive as one RedisUnavailableError; redisOutage503 answers it on the 31 route-level catches that used to report an outage as 400 bad_request or 500 internal, a new top-level catch on the request handler answers it for everything else, and the admin's error middleware does the same. That top-level catch is a fix in its own right: http.createServer was handed a 4000 line async callback with nothing behind it, so any throw was an unhandled rejection, which is no answer to the client and a process exit on Node 22. PARAMANT_REDIS_DEADLINE_MS keeps the name #368 gave it and is now the single configuration source for both services. What is given up: during an outage every request pays the deadline once before it is refused, and a deployment whose redis routinely takes longer than a second must raise the knob rather than remove it. The health routes were part of the same blind spot. /v2/health/deep listed eight checks and never mentioned the store that holds every TOTP secret, so it stayed green through an outage. It now carries a redis check. The admin had no health route at all: its container probe was GET /api/auth/check, which answers 401 when nobody is signed in, so healthy meant the process still refuses me. It now has GET /health, always 200, saying degraded when redis cannot be reached. The relay's own /health touches no redis and still answers 200, which is correct. 2. POST /api/user/login did more work for an address that exists than for one that does not: a second relay call and one more redis read. The status codes were already identical, deliberately, but the clock was not. Measured on a booted admin over 200 requests per case, interleaved, one source address each, with the relay stub given a realistic 3 ms cost for the verify call, the median was 6.44 ms against 2.20 ms and the two ranges did not overlap at all, so one request classified an address with no credentials at all. Every credential answer is now held to t0 plus PARAMANT_LOGIN_MIN_ANSWER_MS, default 250 ms, and the not-found branch makes the same number of redis calls as the found one. The same measurement afterwards reads 251.37 against 251.27 ms with the ranges fully overlapping. The 429 and the 428 are not padded: their status codes tell them apart whatever the clock says, and holding the 428 back would only delay the page that is waiting to start hashing. 3. admin/test/login-ratelimit.test.js drove the limiter module through an attemptLogin() helper written inside the test file and then read server.js as a string to check the order of three calls. Both halves are worth keeping and neither ever ran the handler, so the suite could pass while the route was wrong. admin/test/_admin-server.js is the counterpart of relay/test/_relay-server.js: it spawns the real admin/server.js against a stub relay and speaks HTTP to it. login-http.test.js runs the reviewer's scenario on that, ten wrong codes on one address from three source addresses followed by the owner solving a real 2^18 proof-of-work, plus the per-IP refusal, the IP refund on a priced attempt and the relay 503 being passed through. Checked against the code it is meant to catch: three of its five tests fail against the pre-#368 admin, and so does login-timing.test.js. The admin CI job now gets a redis service and npm ci, because until now it installed nothing, which is the reason every admin suite was a lib test or a source-text assertion in the first place.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two findings from reviewing #367. Both are about a counter or a guard that was
pointed the wrong way, and both are pinned from the code side and the page side.
1. The login rate limit was a lockout weapon
Old.
admin/server.js:975-979:The counter was keyed on the e-mail address, incremented before any
authentication ran, and never deleted on a successful sign-in (the handler,
965-1030, has no
del). The address is request input, so it measured "how oftendid anyone type this address", not "how often did anyone fail on it". Eleven
posts over three IP addresses, the per-IP cap being five, put the owner of that
address on 429 for the full fifteen minutes, and a correct code could not clear
it.
relay.jscarried the same shape one layer down and would have kept the lockoutalive after an admin-only fix:
userMfaAttemptOk(user_id)incremented on theway in and refused at ten inside five minutes, on a
user_idthat comes off therequest body. That is
/v2/user/verify-totpand/v2/user/consume-backup.New. The rule the fix follows: a counter keyed on an identity the attacker
gets to name may impose cost on that identity, never denial.
admin/lib/login-ratelimit.jsadmin/lib/login-ratelimit.jsrelay/lib/auth-throttle.jsuser_idThe per-IP refusal stays exactly as it was, because an IP address is the
caller's own resource: refusing it costs the guesser. The per-account counters
count only attempts that actually failed, a successful sign-in deletes them, and
past the threshold the next attempt is priced rather than refused. The price at
the admin edge is the 2^18 proof-of-work that signup and password reset already
use (
admin/lib/pow-captcha.js,GET /api/captcha/challenge);auth-login.jssolves it and re-posts by itself, so the honest user sees a second or two of
"Verifying" and nothing else. The request that asks for the proof is refunded to
the per-IP counter, so five attempts stay five real attempts. The relay has no client to run a challenge, so it
delays instead.
The e-mail key is hashed (
paramant:user:loginfail:<sha256>), matching thelogin-with-backup limiter, and it is a new namespace, so no counter written
under the old shape survives a deploy still holding somebody out.
What was given up. There is no longer any per-account ceiling that refuses.
A distributed attacker can keep guessing one address, at one proof-of-work per
attempt plus five attempts per IP per fifteen minutes. Against a six-digit code
with a one-slot window under two algorithms that is botnet-scale cost for
roughly six-in-a-million per guess. If the trade needs revisiting, raise the
difficulty for a hot address rather than reintroducing the refusal. Written up
in SECURITY.md with the reasoning.
Sabotage. Each break applied to the source, the run confirmed red, then
restored. Ten of ten caught:
findUserByEmailwith its 429:admin/test/login-ratelimit.test.jsred,tests/site-claims.test.mjsred;clearEmailFailureson success removed:login-ratelimit.test.jsred;login-ratelimit.test.jsred;userMfaAttemptOkrestored in relay.js:relay/test/auth-throttle.test.jsred, and
relay/test/route-user-mfa-lockout.test.jsred against a reallybooted relay;
auth-throttle.test.jsred;tests/ui-truthfulness.test.mjsred;site-claims.test.mjsred.2. TOTP single-use failed open, and then did not fail at all
Old.
relay/lib/totp.js:103:totp.js:93-94documented the fail-open as deliberate: availability over replayprotection, so a Redis blip could not block a legitimate first use.
Assessment: not defensible here, fail-closed. The NX key is the only thing
between an observed six-digit code and its replay inside the same 30-second
slot, on the path that mints admin-panel sessions. And the availability it
bought was imaginary: the session store is the same Redis, so a login that skips
the replay check still cannot be issued a session. Nothing else in the codebase
argues the other way.
New. A store failure yields
{ valid: false, error: 'replay_store_unavailable' }./v2/user/verify-totpand both/v2/user/signing-keyroutes answer 503 and logtotp_replay_store_unavailable; the admin login passes it through astotp_unavailablerather than reporting a wrong code, and does not count it asa failed attempt. A refused replay is
error: 'replay', a wrong code carries noerror at all, so the three cases stay apart.
And a second finding underneath it. Fail-closed only means something if a
decision arrives. Measured on a booted relay with the redis server killed
underneath it, the endpoint did not fail open or closed: it hung, no answer
after twelve seconds. node-redis queues commands while it reconnects, so the
secret read in front of the guard never resolved and the guard was never
reached. Two one-second bounds fix it on this path:
storeTimeoutMsinsideverifyTotpGeneric, andredisDeadlinearound the secret read in relay.js.Same outage, after:
That queue-forever behaviour belongs to the relay's redis client, not to this
path, and every other redis-backed route still inherits it. The fix is
disableOfflineQueueor a global command deadline, which changes the failuremode of every call site at once, so it is recorded in SECURITY.md as open rather
than smuggled in behind an auth fix.
Sabotage. Four of four caught:
.catch(() => 'OK')restored:relay/test/verify-totp.test.jsred;verify-totp.test.jsred (the hanging-store case);auth-throttle.test.jsred, androute-user-mfa-lockout.test.jsred against a live relay with the redisconnection cut mid-suite.
One honest limit on the live suite: with the store gone the request is answered
by the secret-read bound, so that suite proves the endpoint decides and decides
against the code, not that the NX guard itself fails closed. The guard is pinned
where it can be reached in isolation, in
verify-totp.test.js, against athrowing store, a synchronously throwing store and a store that never answers.
The test comment says so.
#367 landed while this was open, and this branch is rebased onto it
#367 merged as b913f9e mid-run. Rebased onto it, and the three files it and
this branch both touch are resolved rather than left to conflict.
#367 did the right thing with the lockout it found: it did not just describe it,
it pinned it. Row 15 makes
/help/session-issuescarry the caveat "attemptssomeone else makes on your address count against you too" for exactly as long as
the handler keeps an address-keyed counter that runs before authentication and
survives a success, and it forbids any page from denying it while that holds.
That is what makes the page follow the code instead of the other way round.
All three properties are now false, so:
/help/session-issuesdrops the caveat (row 15's own else-branch says to)and states the per-IP refusal as the only one, with the ten failures as the
point where an attempt starts costing work rather than as a second limit. This
is the file the brief said not to touch, and the reason has expired: Ten more site claims pinned to the code, and the work list retaken #367 is in
main, and its test requires the page to move with the handler.
still exists, so the page keeps following whichever way they fall. Put the
address-keyed counter back and the caveat is demanded again.
rather than failing the login"). Correct then, false now; it states the
fail-closed behaviour and what the store outage answers instead.
/securitytake the new numbers. Ten more site claims pinned to the code, and the work list retaken #367's phrasing for thesingle-use half is kept, because it is better; only the clause after it flips.
Security review round
Approved after a live attack: lockout gone, no bypass through address
normalisation, no existence oracle through the 428, fail-closed in 1004 ms,
frontend solving the proof-of-work in 1.6 s. Three points came back and are
fixed in this branch, in the commit
Three from the security review:PARAMANT_REDIS_DEADLINE_MS(relay.js) andPARAMANT_TOTP_REPLAY_TIMEOUT_MS(lib/totp.js) now carry them, default 1000 msunchanged, documented in
deploy/.env.exampleand SECURITY.md. Zero, anegative and a non-number all fall back to the default instead of switching
the bound off, because an unbounded guard does not fail closed, it hangs.
Pinned behaviourally:
verify-totp.test.jsreads the constant back out of asubprocess under six different environments.
of five left two real attempts: each sign-in cost a quote and an answer. The
428 path refunds it, since nothing was evaluated there.
refundIpwill notcreate a counter that is absent, because
DECRon a missing key seeds it at-1 with no TTL and would hand that IP a larger budget than a fresh one.
repository: 150 to 250 ms median for a native solver, one to two seconds only
in a browser, where every hash is an awaited WebCrypto call. docs/api.md,
SECURITY.md, the help page and the comment in
pow-captcha.jsnow say whatthe proof-of-work does (prices each automated guess, stops a stranger from
switching an account off) and name the per-IP limit as the actual brake.
Sabotage on all three, five of five caught: the refund removed
(
login-ratelimit.test.jsred), either deadline hardcoded again(
verify-totp.test.js/auth-throttle.test.jsred), a zero allowed to disablethe bound (red), the env documentation dropped (
env-documented.test.mjsred).Follow-ups, deliberately not in this PR
consume-backuphas no bound onsMembers. Already open in SECURITY.md.It sits behind the same throttle this PR gives the MFA path, which slows a
caller down but does not bound the read itself. Its own change.
login-ratelimit.test.jsdrivesthe module directly and pins the handler's shape by reading
server.js; therelay side is exercised over real HTTP against a booted process
(
route-user-mfa-lockout.test.js) but the admin side has no equivalent, becausenothing in
admin/test/bootsserver.js. Worth building the admin counterpartof
relay/test/_relay-server.jsand moving these assertions onto it; that is aharness, not a fix, so it should not ride along here.
Docs
SECURITY.md: both decisions, the table, what was given up, and the client-levelqueue-forever finding as open.
docs/api.md: the two counters as a table on the login endpoint, plus 428 and 503in the error-code table.
docs/site-claims.mdrow 6 andfrontend/security.htmlfollow the code.Tests
admin/test/login-ratelimit.test.js: 8 new, module behaviour plus source pins onthe handler order, including the IP refund on the priced path.
relay/test/auth-throttle.test.js: 7 new, the throttle plus the relay wiring.relay/test/route-user-mfa-lockout.test.js: 4 new, a booted relay, a real redis,the attack, and the outage with the connection cut mid-suite.
relay/test/verify-totp.test.js: the fail-open test replaced by three fail-closedcases, a source pin, and a subprocess test for the configurable deadline.
tests/site-claims.test.mjs6 andtests/ui-truthfulness.test.mjs4 updated to thebehaviour that exists.
Runs, all local, redis on a scratch port:
node --test $(grep -L "from 'playwright'" tests/*.mjs): 150 pass, 1 fail. Thefailure is
tests/heartbeat-lib.test.mjs, which needs@noble/post-quantum; itfails identically without
npm ciand is unrelated to this branch (Ten more site claims pinned to the code, and the work list retaken #367 reportsthe same).
bash tests/static-sanity.sh: PASS, all 11 checks clear.node --test tests/env-documented.test.mjs: 6 pass, 0 fail (83 variables documented).scripts/check-test-declarations.sh: 114 suites clean. New test blocks keep theirdeclarations in function scope.
the geometry they measure; the only page edits are one sentence on /security and a
script tag plus three error strings on /auth/login.