fix(auth): cap validate-invite attempt volume and add per-route lockout isolation#397
Open
DeryFerd wants to merge 1 commit into
Open
fix(auth): cap validate-invite attempt volume and add per-route lockout isolation#397DeryFerd wants to merge 1 commit into
DeryFerd wants to merge 1 commit into
Conversation
…ut isolation The auth rate limiter lumped all auth routes under one bucket per IP and only counted failed attempts. This left two real holes: 1. `auth:validate-invite` could be probed at high volume with valid tokens: every successful call was free, so a probe that found one valid token could use it to keep scanning. The failure-tier lockout only fired for pure wrong-token probes. 2. A lockout on `auth:login` (e.g. 5 wrong passwords) bled into `auth:validate-invite` for the same IP, locking the user out of an unrelated, no-failure route. Changes: - New `recordAttempt(identifier, action)` API on the limiter so routes can count every call toward a volume cap. - New `recordSuccess(identifier, action)` parameter (action now required) clears only the matching per-route record, so legit users no longer carry failure debt across successful validations. - Map key changed from identifier to `identifier::action`, giving per-route isolation. `auth:login` lockout no longer blocks `auth:validate-invite`. - Volume cap: 100 attempts per (IP, route) per 15-min sliding window triggers a 15-minute lockout. Defends routes like validate-invite against mixed valid/invalid token probes. - Updated `auth:validate-invite` to call `recordAttempt` on every call and `recordSuccess` on valid results; updated the two other callers of `recordSuccess` (`auth:login`, `auth:accept-invite`) to pass the action parameter. Tests: 10 new tests in `backend/auth/rate-limiter.test.ts` covering regression of all three failure tiers, the new volume cap, success clearing state, per-route isolation, and per-identifier isolation. Total: 146 pass (was 136), 1 pre-existing fail (session-invalidation ptykit, unrelated).
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
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.
Why this exists
The
AuthRateLimiterwas a 145-line file that looked fine on paper but had two real holes once you read it end-to-end.Hole 1:
auth:validate-invitewas effectively unrate-limited against "smart" probes. The class only ever counted failed attempts and only stored one record per IP. A probe that didn't know any valid invite token would hit the 5/10/20 failure tiers and lock out fast, so the original design was reasonable for the dumb case. But invite tokens are 24 random bytes (48 hex chars, seebackend/auth/tokens.ts:21), and a probe that happened to find a single valid token could keep going forever on that IP. Every successful call reset nothing, counted as nothing, and went through. The same flaw would have applied to any future route that returned success/failure based on lookup, not password check.Hole 2: lockouts were not per-route. The internal map was keyed by IP alone. So five wrong passwords on
auth:login(a totally fair lockout) would also block the same IP fromauth:validate-invitefor 30 seconds — even though that user had done nothing wrong on the invite route. I tripped over this while writing the tests; the per-route test case in the new file failed for the right reason, and once I saw it I couldn't unsee it.There's also a smaller thing I almost missed:
auth:validate-invitewas the only rate-limited route that never calledrecordSuccesson a valid result, so a user who mistyped twice in a row then finally got it right would still be carrying 2 failures on the next attempt, slowly walking toward a 30-second lockout for what is, from the user's perspective, a successful operation. Trivial to fix once you see it; easy to miss in a code review.What changed
Three files, 286 insertions, 21 deletions.
backend/auth/rate-limiter.ts— the actual fix.${identifier}to${identifier}::${action}. The per-(IP, route) split fixes hole 2 directly:auth:loginlockouts no longer bleed intoauth:validate-invite, and the same applies to every future rate-limited route.AttemptRecordnow carries anattempts: numbercounter and anattemptWindowStart: numbertimestamp, in addition to the existingfailuresandlockedUntilfields.recordAttempt(identifier, action)method. Increments the per-(IP, route) attempt counter and applies the volume-tier lockout if the cap is hit. Used by routes where "this was a probe, even if it returned success" is the right signal.recordSuccess(identifier, action)now takes anactionparameter and only clears the matching per-route record. Previously it cleared the whole IP's record, which was the right call for the single-bucket model and is now wrong. All three call sites inlogin.tsupdated.ATTEMPT_CAP = 100,ATTEMPT_WINDOW_MS = 15 * 60_000,ATTEMPT_LOCKOUT_MS = 15 * 60_000. These are the cap (100 attempts per window) and the lockout that fires when the cap is hit. 100/15min is well above what any real user flow generates, and the 15-minute lockout matches the existing failure-tier ceiling.recordFailurenow also incrementsattemptsand runs the volume-cap check, so a pure wrong-token probe can hit both lockout tiers (failure-based after 5 misses, volume-based after 100 attempts total).backend/ws/auth/login.ts— wiring.auth:validate-invitenow callsrecordAttempton every call before the lookup, andrecordSuccess(not justrecordFailure) on a valid result. That's the targeted fix for the user-experience papercut and the volume-cap wiring.recordSuccesscallers (auth:loginandauth:accept-invite) updated to pass theactionparameter. No behavior change for those routes beyond the per-route key.backend/auth/rate-limiter.test.ts— new file, 10 tests, no previous coverage existed for this module at all. The file does its own isolation by using a unique random identifier per test, so it can use the singleton without cross-test pollution. It also uses a smallreadLockouthelper that acceptsunknowninstead ofReturnType<typeof authRateLimiter.check>on purpose: PR #396 is going to change the return type fromstring | nullto{ message, lockedUntil } | null, and the tests work against either shape, so this PR and #396 don't conflict and the tests won't need a churn update when #396 lands.Tests that are now in place
The 10 tests split into four behavioral groups:
recordSuccessbetween bursts resets the counter so a legitimate user with a 99-attempts-then-success pattern isn't penalized.recordSuccesssemantics (2 tests): a success clears the failure record and also clears the attempt record, both for the current route only.auth:loginlockout doesn't affectauth:validate-invitefor the same IP, attempt records on different routes don't bleed, and one IP hitting the cap doesn't affect a different IP.What I did not change
auth:setuprouting logic. The setup route is one-shot (only callable when no users exist), and the rate limiter already handles it correctly.AuthRateLimiterclass. The tests use the singleton with per-test unique identifiers, which is a lighter touch than exporting a class and instantiating per test. If a future test wants fresh-instance isolation, the export is a one-line change.ATTEMPT_CAP = 100andATTEMPT_LOCKOUT_MS = 15minare module constants. If you want them configurable via the existingsystem:settingstable the same waymaxFileSizeMBis configurable, that's a follow-up — I'd want to know whether you've decided to expose rate-limit knobs through the admin UI before doing it.lockedUntilreturn-type change. The test helper accepts both shapes, so the PRs are independent and can merge in either order.How I verified
Local run before pushing. Numbers won't match a Linux CI run exactly because of the timing tolerances in the lockout-duration tests, but the pass/fail signals are the same.
bun test --isolate(full suite, 147 tests across 23 files): 146 pass, 1 fail. The 1 failure isbackend/auth/session-invalidation.test.tsand is the pre-existing@myrialabs/ptykit/coremodule-missing error that was failing onb60d663before any of my changes. Net change from this PR: +10 tests, +0 regressions.bun run check: 19 errors. All 19 are pre-existingCannot find module '@myrialabs/ptykit/*'errors caused by the dep not being installed in this checkout. This PR introduces zero new type errors; I had two when I first wrote the test helper, fixed them by typing the parameter asunknownrather thanReturnType<typeof authRateLimiter.check>.bun run lint: clean exit, zero warnings.