Repository navigation
Fix/login endpoint protection - #129
Open
panesar-ke wants to merge 6 commits into
Open
panesar-ke wants to merge 6 commits into
panesar-ke wants to merge 6 commits into
Conversation
The sliding window keyed on `sessionId` alone, and every unauthenticated request resolves to the literal 'anonymous', so all pre-login traffic on the internet shared one 100/min bucket. One client could exhaust it and 403 sign-in company-wide. Adding Arcjet's built-in `ip.src` makes the key a tuple: anonymous traffic buckets per-IP, while `sessionId` still keeps signed-in staff off a single office-NAT bucket. Also handle `isErrored()`, which was never checked — a bad key or an unreachable Arcjet silently allowed everything. It still allows, but says so on a greppable line. And read the key from the validated env module so a missing ARCJET_KEY fails at load rather than at request time. Denied responses now carry Retry-After when the reason is a rate limit. The body stays empty; which rule fired is not the caller's business. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Login had no per-account limit of any kind: no attempt counting, no backoff, no lockout. The only ceiling was the shared Arcjet window, which also permitted ~144k guesses/day against a single account. Recreate `login_attempts` — the legacy table was dead code with a text `success` column, no FK and no index — and count failures against it: 5 per normalized identifier per rolling 15 minutes. Keyed on the identifier string rather than a resolved user id, so an identifier matching no account throttles identically; otherwise whether a request gets throttled becomes the enumeration oracle everything else here removes. A successful login clears prior failures so a legitimate user who mistyped is not left one slip from the threshold. Unify the login failure messages. "User not found" and "Invalid credentials" are now one byte-identical string, and when no user matches we compare against a dummy hash built at module load so that branch costs the same bcrypt work — skipping it made "no such account" measurably faster to return. Move the `active` check after password verification. The deactivated message now only reaches someone who has proved they hold the password, so the account's owner gets a useful explanation and nobody else learns the account exists. A nightly Inngest job prunes attempt rows past 30 days. Two migrations, generated: 0041 drops the legacy table, 0042 creates the new one. Split so neither diff is ambiguous enough to need the interactive rename prompt. Known and deliberately untouched: the legacy-lowercase fallback in password.ts runs a second bcrypt.compare for accounts whose password predates the casing fix, making that cohort measurably slower. It distinguishes a group of accounts, not account existence. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
pnpm lint:check died on the first file it reached, so nothing in the repo had actually been linted: TypeError: Error while loading rule 'react/display-name': contextOrFilename.getFilename is not a function eslint-plugin-react resolves `version: 'detect'` through context.getFilename(), which ESLint 10 removed. 7.37.5 is the latest published and peers on eslint <=9, so there is no version to upgrade to. Naming the version explicitly skips that code path; reading it from the installed react keeps it from drifting. Also fixes the three errors that surfaced once the run got past the first file, both in session.test.ts: an import() type annotation and two `any`s in the react cache mock. Verified with pnpm lint:check (exit 0, 25 pre-existing warnings), pnpm typecheck and pnpm test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011P83TdcFVERSjAF4JRU8kc
Nine findings from a review of the two commits on this branch. An audit-write failure denied a valid login: recordLoginAttempt and clearLoginFailures were awaited unguarded on the success path, so a dropped connection or an unmigrated environment answered a correct password with 'Something went wrong'. Both now sit in a try/catch that logs, matching the rehash below them. The dummy-hash equalization did not close the timing oracle it was added for. verifyPassword ran a second bcrypt.compare only for a legacy hash and a mixed-case input, so an existing legacy account cost ~2 bcrypt against ~1 for a nonexistent identifier. It now compares twice on every path, and the no-user branch compares twice as well. This doubles bcrypt cost on login until the legacy fallback is deleted. A successful login erased the brute-force evidence: clearLoginFailures had no time bound, so a slow grind that finally guessed right wiped every prior failure. It is now bounded to the same window isThrottled counts. Recording refusals made the window extendable: isThrottled reads retryAfter off the row at `count - LIMIT`, so each retry pushed it later and the block only expired for a caller who stopped trying. Refusals are no longer recorded. Per-identifier keying doubled the budget. loginAction resolves email OR contact but the throttle counted the submitted string, so an account reachable both ways had two independent budgets. The user is now resolved first and every identifier that reaches the account shares one budget. Two trade-offs documented in place: a throttled caller now costs one users lookup, and a shared budget links an account's email and contact for anyone who already knows both. The prune materialized every deleted row id to count them, on the one table sized to absorb spray traffic. It now deletes in 5,000-row chunks and counts through rowCount. A dummyPasswordHash rejection re-opened the enumeration oracle: the no-user branch would have returned a generic failure while a wrong password still returned the shared message. It is caught and falls through. proxy.ts is comment-only. An errored Arcjet decision carries no rule attribution, so the fail-open cannot be narrowed to the rate limit; the comment now states that shield and bot detection are off matcher-wide while Arcjet is unreachable, and that an unresolvable ip.src looks identical. The test named "counts only failures inside the window" asserted only the shape of the call, so deleting the cutoff left the suite green. It now asserts the whole where clause — confirmed by deleting the clause and watching it fail. Verified with pnpm test (344 passing), pnpm typecheck and pnpm lint:check. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011P83TdcFVERSjAF4JRU8kc
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Seven findings from the review of #129, all on the throttle added earlier on this branch. - The throttle gate is now a reservation, not a check. Counting the budget and recording the attempt as two statements let simultaneous submissions all read the same under-limit count before any of them wrote a row, so five-per-window only held for callers who took turns; a burst bought as many bcrypt checks as it had connections, capped only by Arcjet — which fails open. reserveLoginAttempt counts and inserts under one transaction-scoped advisory lock and hands back the row id, which the success path flips. A conditional INSERT would not have held: under READ COMMITTED each statement counts against a snapshot taken before its peers committed. - The login identifier is bounded. It leads a btree index, so an oversized submission made the insert fail outright, which turned the shared invalid-credentials answer into a generic error AND left the attempt uncounted. Bounded in loginSchema and again at the insert, alongside the ip and user-agent audit columns. - Every login now performs the same one write, before the password check, so a broken or unmigrated table denies right and wrong passwords alike. Recording only on the failure paths made a broken table answer 'Something went wrong' for every failure while successes sailed through. - The prune runs one Inngest step per chunk instead of the whole loop in one step. Chunking bounds lock duration, not wall time: a first prune facing millions of rows ran past the client's 50s maxRuntime, burned both retries and failed the run. - The timing-equalization dummy tracks the cost factor of real stored hashes rather than BCRYPT_ROUNDS, so raising the rounds cannot make the "no such account" branch several times slower than a real check and reopen the enumeration oracle in the opposite direction. - x-forwarded-for is read with `||`, not `??`, in both the login action and session.ts: a present-but-empty header trims to '', which is not nullish, so the columns held a mix of NULL and '' for the same "unknown". Migrations: 0041 now drops with IF EXISTS, so a database that never had the old table does not abort before reaching 0042. The drop is still deliberate and still destroys whatever the pre-existing login_attempts audit table holds in each environment it is applied to. Both migrations remain unapplied. Verified with pnpm vitest run (350 pass), tsc --noEmit, pnpm lint:check (0 errors), and the advisory-lock statement against the dev database. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015btsgNtB7vMrRX2um7qMvN
This branch was successfully deployed
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.
No description provided.