Differentiate deleted account login errors#232
Merged
Conversation
Co-authored-by: Bretton Auerbach <auerbachb@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds account-deletion tracking (DB table + schema), email-hash helpers, transactional deletion that logs hashed emails, login route detection returning HTTP 410 for deleted accounts, unit and e2e tests, and an accessibility/layout update for auth error display. Changes
Sequence DiagramsequenceDiagram
actor User
participant Client as Client
participant LoginAPI as Login Endpoint
participant DB as Database
participant DeletionLog as Deletion Log
User->>Client: Enter email & password
Client->>LoginAPI: POST /api/auth/login
LoginAPI->>DB: Query users by email
alt User Found
DB-->>LoginAPI: user record
LoginAPI->>LoginAPI: verifyPassword(...)
alt Password Valid
LoginAPI-->>Client: 200 OK + token
else Password Invalid
LoginAPI-->>Client: 401 Invalid credentials
end
else User Not Found
DB-->>LoginAPI: no record
LoginAPI->>DeletionLog: wasAccountDeleted(email)
DeletionLog->>DeletionLog: compute SHA-256 hash & query account_deletion_log
alt Deletion Record Found
DeletionLog-->>LoginAPI: true
LoginAPI-->>Client: 410 DELETED_ACCOUNT_MESSAGE
else No Deletion Record
DeletionLog-->>LoginAPI: false
LoginAPI-->>Client: 401 Invalid credentials
end
end
Client->>User: Display message (accessible alert)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Co-authored-by: Bretton Auerbach <auerbachb@users.noreply.github.com>
Co-authored-by: Bretton Auerbach <auerbachb@users.noreply.github.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 35b11a9. Configure here.
| if (!user) { | ||
| if (await wasAccountDeleted(email)) { | ||
| return NextResponse.json({ error: DELETED_ACCOUNT_MESSAGE }, { status: 410 }); | ||
| } |
There was a problem hiding this comment.
Deletion check enables email enumeration via timing
Medium Severity
When a user is not found, wasAccountDeleted performs an additional database query before responding, while an incorrect-password attempt does not. This timing difference allows an attacker to distinguish "never existed" from "exists but wrong password" via response time, undermining the uniform "Invalid credentials" response intended to prevent email enumeration. The wasAccountDeleted call also distinguishes deleted accounts from truly unknown ones via the 410 status, which is intentional per the PR, but the timing side-channel on the non-deleted unknown path is likely unintentional.
Reviewed by Cursor Bugbot for commit 35b11a9. Configure here.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/lib/accountDeletion.test.ts (1)
76-89: Strengthen this test by asserting insert happens before delete.You already verify both calls occur; adding call-order assertions would enforce the transactional sequencing requirement explicitly.
🧪 Suggested assertion add-on
expect(txInsert).toHaveBeenCalled(); expect(txInsertValues).toHaveBeenCalledWith({ userId: "user-1", emailHash: getAccountDeletionEmailHash("Deleted@Example.com"), }); expect(txDelete).toHaveBeenCalled(); expect(txDeleteWhere).toHaveBeenCalled(); + const insertCallOrder = txInsertValues.mock.invocationCallOrder[0]; + const deleteCallOrder = txDeleteWhere.mock.invocationCallOrder[0]; + expect(insertCallOrder).toBeLessThan(deleteCallOrder);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/accountDeletion.test.ts` around lines 76 - 89, Add assertions that the insert occurs before the delete by checking the mocks' invocation order: after importing deleteUserAccount and calling it, assert that txInsert.mock.invocationCallOrder[0] (or txInsertValues if you prefer) is less than txDelete.mock.invocationCallOrder[0] (and/or txDeleteWhere) to enforce sequencing; reference the test's deleteUserAccount and getAccountDeletionEmailHash, and the mocks txInsert, txInsertValues, txDelete, and txDeleteWhere when adding these order checks.src/components/AuthScreen.tsx (1)
152-164: Expose auth errors as a live region for screen readers.This block is visually improved, but adding
role="alert"+aria-livewould make API error updates announced immediately.♿ Suggested tweak
- {error && ( - <div style={{ + {error && ( + <div + role="alert" + aria-live="polite" + style={{ fontFamily: "var(--font-jetbrains), 'JetBrains Mono', monospace", fontSize: "11px", color: "var(--accent-danger)", textAlign: "center", lineHeight: 1.5, overflowWrap: "break-word", width: "100%", maxWidth: "28ch", margin: "0 auto", - }}> + }} + > {error} </div> )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/AuthScreen.tsx` around lines 152 - 164, The error display in the AuthScreen component should be made accessible to screen readers by turning the error container into a live region: update the error <div> used in AuthScreen to include role="alert" and aria-live="assertive" (and optionally aria-atomic="true") so that API error messages are announced immediately when the {error} state changes; locate the error rendering block in AuthScreen and add these attributes to the div wrapping {error}.src/app/api/auth/login/route.ts (1)
8-8: Consider centralizing the deleted-account message string.This literal is now contract-like across route/unit/E2E paths; moving it to a shared auth-error constant would reduce drift risk.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/auth/login/route.ts` at line 8, The literal DELETED_ACCOUNT_MESSAGE in route.ts should be moved to a shared auth error constants module (e.g., export const DELETED_ACCOUNT_MESSAGE in a new or existing auth-errors/auth-constants file) and then replaced with an import in src/app/api/auth/login/route.ts and any unit/E2E tests or other routes that rely on the same message; update code references to use the exported symbol DELETED_ACCOUNT_MESSAGE and adjust imports in tests and route handlers so all places consume the single canonical constant.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/app/api/auth/login/route.ts`:
- Line 8: The literal DELETED_ACCOUNT_MESSAGE in route.ts should be moved to a
shared auth error constants module (e.g., export const DELETED_ACCOUNT_MESSAGE
in a new or existing auth-errors/auth-constants file) and then replaced with an
import in src/app/api/auth/login/route.ts and any unit/E2E tests or other routes
that rely on the same message; update code references to use the exported symbol
DELETED_ACCOUNT_MESSAGE and adjust imports in tests and route handlers so all
places consume the single canonical constant.
In `@src/components/AuthScreen.tsx`:
- Around line 152-164: The error display in the AuthScreen component should be
made accessible to screen readers by turning the error container into a live
region: update the error <div> used in AuthScreen to include role="alert" and
aria-live="assertive" (and optionally aria-atomic="true") so that API error
messages are announced immediately when the {error} state changes; locate the
error rendering block in AuthScreen and add these attributes to the div wrapping
{error}.
In `@src/lib/accountDeletion.test.ts`:
- Around line 76-89: Add assertions that the insert occurs before the delete by
checking the mocks' invocation order: after importing deleteUserAccount and
calling it, assert that txInsert.mock.invocationCallOrder[0] (or txInsertValues
if you prefer) is less than txDelete.mock.invocationCallOrder[0] (and/or
txDeleteWhere) to enforce sequencing; reference the test's deleteUserAccount and
getAccountDeletionEmailHash, and the mocks txInsert, txInsertValues, txDelete,
and txDeleteWhere when adding these order checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b6facee7-a0d2-4cf4-a097-55c36992e1d6
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
drizzle/account_deletion_log_incremental.sqle2e/auth/login.spec.tspackage.jsonsrc/app/api/auth/login/route.test.tssrc/app/api/auth/login/route.tssrc/components/AuthScreen.tsxsrc/db/schema.tssrc/lib/accountDeletion.test.tssrc/lib/accountDeletion.ts
Co-authored-by: Bretton Auerbach <auerbachb@users.noreply.github.com>
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/lib/accountDeletion.test.ts (2)
76-92: Consider asserting delete predicate target user ID.You already check operation order; adding a predicate assertion would guard against accidental broad deletes.
Suggested assertion addition
expect(txDelete).toHaveBeenCalled(); - expect(txDeleteWhere).toHaveBeenCalled(); + expect(txDeleteWhere).toHaveBeenCalledWith({ + left: "userId", + right: "user-1", + }); expect(txInsertValues.mock.invocationCallOrder[0]).toBeLessThan( txDeleteWhere.mock.invocationCallOrder[0], );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/accountDeletion.test.ts` around lines 76 - 92, Add an assertion that the delete predicate targets the intended user id to prevent accidental broad deletes: after calling deleteUserAccount in the test, assert that txDeleteWhere was called with a predicate (or args) that includes/filters for "user-1" (use the same shape your code uses for predicates in txDeleteWhere) and keep the existing order check; reference deleteUserAccount, txDeleteWhere, txInsertValues and getAccountDeletionEmailHash to locate where to add the new expectation.
65-74: Tighten lookup assertions to reduce false positives.This test should also assert the query targets the deletion-log table and applies
limit(1).Suggested test hardening
await expect(wasAccountDeleted(" Deleted@Example.com ")).resolves.toBe(true); + expect(selectFrom).toHaveBeenCalledWith( + expect.objectContaining({ emailHash: "emailHash" }), + ); + expect(selectLimit).toHaveBeenCalledWith(1); expect(selectWhere).toHaveBeenCalledWith({ left: "emailHash", right: getAccountDeletionEmailHash("Deleted@Example.com"), });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/accountDeletion.test.ts` around lines 65 - 74, The test "finds deletion log entries by normalized email hash" needs stronger assertions: after calling wasAccountDeleted(" Deleted@Example.com "), assert that selectWhere was invoked with the deletion log table (e.g., include table: "deletion-log" alongside left: "emailHash" and right: getAccountDeletionEmailHash("Deleted@Example.com")), and assert that the query applied a limit of 1 by checking selectLimit was called with 1 (or that limit(1) was applied). Update the assertions around selectWhere and selectLimit in this test to include those checks using the existing getAccountDeletionEmailHash and wasAccountDeleted references.src/app/api/auth/login/route.test.ts (1)
54-104: Add explicit “no auth side effects” assertions for failure responses.Recommend asserting
createTokenandsetAuthCookieare not called in these error-path tests.Suggested assertions
await expect(response.json()).resolves.toEqual({ error: "Invalid credentials" }); expect(response.status).toBe(401); expect(wasAccountDeleted).toHaveBeenCalledWith("missing@example.com"); expect(verifyPassword).toHaveBeenCalledWith("password123", expect.stringMatching(/^\$2b\$12\$/)); + expect(createToken).not.toHaveBeenCalled(); + expect(setAuthCookie).not.toHaveBeenCalled(); @@ await expect(response.json()).resolves.toEqual({ error: DELETED_ACCOUNT_MESSAGE, }); expect(response.status).toBe(410); expect(wasAccountDeleted).toHaveBeenCalledWith("deleted@example.com"); + expect(createToken).not.toHaveBeenCalled(); + expect(setAuthCookie).not.toHaveBeenCalled(); @@ await expect(response.json()).resolves.toEqual({ error: "Invalid credentials" }); expect(response.status).toBe(401); expect(wasAccountDeleted).toHaveBeenCalledWith("user@example.com"); expect(verifyPassword).toHaveBeenCalledWith("bad-password", "hash"); + expect(createToken).not.toHaveBeenCalled(); + expect(setAuthCookie).not.toHaveBeenCalled();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/auth/login/route.test.ts` around lines 54 - 104, Add explicit assertions in the error-path tests to ensure no authentication side effects occur: in the three tests (the "keeps invalid credentials for unknown emails without a deletion log", "returns a deleted-account message when the deletion log matches", and "does not expose account existence when a password is wrong" tests) assert that the mocked createToken and setAuthCookie functions were not called (e.g., expect(createToken).not.toHaveBeenCalled() and expect(setAuthCookie).not.toHaveBeenCalled()). Place these assertions after the existing response assertions so they run for POST from "./route" and use the existing mocked names createToken and setAuthCookie.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/app/api/auth/login/route.test.ts`:
- Around line 54-104: Add explicit assertions in the error-path tests to ensure
no authentication side effects occur: in the three tests (the "keeps invalid
credentials for unknown emails without a deletion log", "returns a
deleted-account message when the deletion log matches", and "does not expose
account existence when a password is wrong" tests) assert that the mocked
createToken and setAuthCookie functions were not called (e.g.,
expect(createToken).not.toHaveBeenCalled() and
expect(setAuthCookie).not.toHaveBeenCalled()). Place these assertions after the
existing response assertions so they run for POST from "./route" and use the
existing mocked names createToken and setAuthCookie.
In `@src/lib/accountDeletion.test.ts`:
- Around line 76-92: Add an assertion that the delete predicate targets the
intended user id to prevent accidental broad deletes: after calling
deleteUserAccount in the test, assert that txDeleteWhere was called with a
predicate (or args) that includes/filters for "user-1" (use the same shape your
code uses for predicates in txDeleteWhere) and keep the existing order check;
reference deleteUserAccount, txDeleteWhere, txInsertValues and
getAccountDeletionEmailHash to locate where to add the new expectation.
- Around line 65-74: The test "finds deletion log entries by normalized email
hash" needs stronger assertions: after calling wasAccountDeleted("
Deleted@Example.com "), assert that selectWhere was invoked with the deletion
log table (e.g., include table: "deletion-log" alongside left: "emailHash" and
right: getAccountDeletionEmailHash("Deleted@Example.com")), and assert that the
query applied a limit of 1 by checking selectLimit was called with 1 (or that
limit(1) was applied). Update the assertions around selectWhere and selectLimit
in this test to include those checks using the existing
getAccountDeletionEmailHash and wasAccountDeleted references.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2eb343dd-cd46-41e2-8991-033d62f933f1
📒 Files selected for processing (6)
e2e/auth/login.spec.tssrc/app/api/auth/login/route.test.tssrc/app/api/auth/login/route.tssrc/components/AuthScreen.tsxsrc/lib/accountDeletion.test.tssrc/lib/authErrors.ts
✅ Files skipped from review due to trivial changes (1)
- src/lib/authErrors.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- e2e/auth/login.spec.ts
- src/app/api/auth/login/route.ts
- src/components/AuthScreen.tsx
Co-authored-by: Bretton Auerbach <auerbachb@users.noreply.github.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/lib/accountDeletion.test.ts (2)
4-21: Consider extracting the repeated select-chain mock builder.Line 4 through Line 21 repeat near-identical query-chain scaffolding for
dbandtx. A small factory would reduce maintenance cost when query shape evolves.♻️ Suggested refactor
-const selectLimit = vi.fn(); -const selectWhere = vi.fn(() => ({ limit: selectLimit })); -const selectFrom = vi.fn(() => ({ where: selectWhere })); -const dbSelect = vi.fn(() => ({ from: selectFrom })); -const txSelectLimit = vi.fn(); -const txSelectWhere = vi.fn(() => ({ limit: txSelectLimit })); -const txSelectFrom = vi.fn(() => ({ where: txSelectWhere })); -const txSelect = vi.fn(() => ({ from: txSelectFrom })); +const makeSelectChain = () => { + const limit = vi.fn(); + const where = vi.fn(() => ({ limit })); + const from = vi.fn(() => ({ where })); + const select = vi.fn(() => ({ from })); + return { select, from, where, limit }; +}; + +const dbChain = makeSelectChain(); +const txChain = makeSelectChain(); + +const dbSelect = dbChain.select; +const selectFrom = dbChain.from; +const selectWhere = dbChain.where; +const selectLimit = dbChain.limit; + +const txSelect = txChain.select; +const txSelectFrom = txChain.from; +const txSelectWhere = txChain.where; +const txSelectLimit = txChain.limit;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/accountDeletion.test.ts` around lines 4 - 21, The test repeats an identical query-chain mock for db and tx (selectLimit, selectWhere, selectFrom, dbSelect and txSelect, txSelectFrom, txSelectWhere, etc.); extract a small factory function (e.g., makeSelectChain or createQueryChainMock) that returns the chain objects and their jest/vi spies for limit/where/from and use it to build both dbSelect and txSelect as well as any insert/delete chains (txInsert, txDelete) to remove duplication and keep the test DRY.
80-99: Good sequencing check; add one failure-path transaction test.Line 80 through Line 99 correctly verifies insert-before-delete ordering. Consider adding one test where
txInsertValues(ortxDeleteWhere) rejects, to ensure errors propagate and are not swallowed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/accountDeletion.test.ts` around lines 80 - 99, Add a new test in src/lib/accountDeletion.test.ts that simulates a failure in the transaction path (e.g., have txInsertValues.mockRejectedValueOnce(new Error("boom")) or txDeleteWhere.mockRejectedValueOnce(...)) and assert that deleteUserAccount("user-1") rejects with that error and that the other transaction call (txDelete or txInsert respectively) is not invoked after the failing step; reference the existing helpers/exports deleteUserAccount, getAccountDeletionEmailHash, txInsertValues, txInsert, txDeleteWhere and txDelete to locate where to hook the mocks and assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/lib/accountDeletion.test.ts`:
- Around line 4-21: The test repeats an identical query-chain mock for db and tx
(selectLimit, selectWhere, selectFrom, dbSelect and txSelect, txSelectFrom,
txSelectWhere, etc.); extract a small factory function (e.g., makeSelectChain or
createQueryChainMock) that returns the chain objects and their jest/vi spies for
limit/where/from and use it to build both dbSelect and txSelect as well as any
insert/delete chains (txInsert, txDelete) to remove duplication and keep the
test DRY.
- Around line 80-99: Add a new test in src/lib/accountDeletion.test.ts that
simulates a failure in the transaction path (e.g., have
txInsertValues.mockRejectedValueOnce(new Error("boom")) or
txDeleteWhere.mockRejectedValueOnce(...)) and assert that
deleteUserAccount("user-1") rejects with that error and that the other
transaction call (txDelete or txInsert respectively) is not invoked after the
failing step; reference the existing helpers/exports deleteUserAccount,
getAccountDeletionEmailHash, txInsertValues, txInsert, txDeleteWhere and
txDelete to locate where to hook the mocks and assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cb5849b3-a484-44d5-9c04-dd9151773932
📒 Files selected for processing (2)
src/app/api/auth/login/route.test.tssrc/lib/accountDeletion.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app/api/auth/login/route.test.ts
Co-authored-by: Bretton Auerbach <auerbachb@users.noreply.github.com>
This was referenced Apr 25, 2026
Merged
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
2 participants
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.
Summary
account_deletion_logtable and transactional deletion logging before hard-deleting users.Walkthrough
deleted_account_login_error_unclipped_demo.mp4
Deleted account login error
Testing
npx tsc --noEmitnpm run test:unitnpx playwright test e2e/auth/login.spec.ts --project=mobile-chromium-narrow --grep "login shows deleted account errors"npm run buildcoderabbit review --prompt-onlycould not run because thecoderabbitexecutable is unavailable in this environment.To show artifacts inline, enable in settings.
Summary by CodeRabbit
New Features
Style
Tests