Skip to content

fix(auth): cap validate-invite attempt volume and add per-route lockout isolation#397

Open
DeryFerd wants to merge 1 commit into
myrialabs:mainfrom
DeryFerd:fix/auth-invite-rate-limit-attempt-count
Open

fix(auth): cap validate-invite attempt volume and add per-route lockout isolation#397
DeryFerd wants to merge 1 commit into
myrialabs:mainfrom
DeryFerd:fix/auth-invite-rate-limit-attempt-count

Conversation

@DeryFerd

@DeryFerd DeryFerd commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Why this exists

The AuthRateLimiter was 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-invite was 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, see backend/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 from auth:validate-invite for 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-invite was the only rate-limited route that never called recordSuccess on 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.

  • Storage key changed from ${identifier} to ${identifier}::${action}. The per-(IP, route) split fixes hole 2 directly: auth:login lockouts no longer bleed into auth:validate-invite, and the same applies to every future rate-limited route.
  • AttemptRecord now carries an attempts: number counter and an attemptWindowStart: number timestamp, in addition to the existing failures and lockedUntil fields.
  • New 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 an action parameter 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 in login.ts updated.
  • New constants at the top of the file: 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.
  • recordFailure now also increments attempts and 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-invite now calls recordAttempt on every call before the lookup, and recordSuccess (not just recordFailure) on a valid result. That's the targeted fix for the user-experience papercut and the volume-cap wiring.
  • The other two recordSuccess callers (auth:login and auth:accept-invite) updated to pass the action parameter. 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 small readLockout helper that accepts unknown instead of ReturnType<typeof authRateLimiter.check> on purpose: PR #396 is going to change the return type from string | null to { 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:

  • Failure-tier regression (3 tests): the existing 5/10/20 → 30s/2m/10m tiers still fire and still produce the right lockout duration. These would have caught any regression to the existing behavior.
  • Volume cap (2 tests): 100 attempts on a route lock that route out for 15 minutes, and a successful recordSuccess between bursts resets the counter so a legitimate user with a 99-attempts-then-success pattern isn't penalized.
  • recordSuccess semantics (2 tests): a success clears the failure record and also clears the attempt record, both for the current route only.
  • Isolation (3 tests): a auth:login lockout doesn't affect auth:validate-invite for 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

  • I did not touch auth:setup routing logic. The setup route is one-shot (only callable when no users exist), and the rate limiter already handles it correctly.
  • I did not change the failure-tier durations. Those are the ones that ship today and are explicitly tested in three places; changing them is a separate decision.
  • I did not export the AuthRateLimiter class. 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.
  • I did not add a new admin setting for the cap. Right now ATTEMPT_CAP = 100 and ATTEMPT_LOCKOUT_MS = 15min are module constants. If you want them configurable via the existing system:settings table the same way maxFileSizeMB is 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.
  • I did not integrate with PR fix(auth): prevent client-side rate limit bypass with server timestamp #396's lockedUntil return-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 is backend/auth/session-invalidation.test.ts and is the pre-existing @myrialabs/ptykit/core module-missing error that was failing on b60d663 before any of my changes. Net change from this PR: +10 tests, +0 regressions.
  • bun run check: 19 errors. All 19 are pre-existing Cannot 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 as unknown rather than ReturnType<typeof authRateLimiter.check>.
  • bun run lint: clean exit, zero warnings.

…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).
@DeryFerd DeryFerd changed the title fix(auth): cap validate-invite attempt volume and add per-route locko… fix(auth): cap validate-invite attempt volume and add per-route lockout isolation Jul 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant