fix(auth): require verified email before account deletion#1798
Conversation
📝 WalkthroughWalkthroughAdds an email-verification gate to account-deletion flows in two Vue pages: unverified users are redirected to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8e92cc9c99
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!env.SUPABASE_URL || !env.SUPABASE_ANON_KEY) | ||
| throw new Error('SUPABASE_URL or SUPABASE_ANON_KEY is missing for delete-user SDK test') | ||
|
|
||
| return createClient<Database>(env.SUPABASE_URL, env.SUPABASE_ANON_KEY, { |
There was a problem hiding this comment.
This helper passes env.SUPABASE_URL directly into createClient, but our test utilities intentionally normalize localhost to 127.0.0.1 to avoid Node/Undici preferring ::1 while Supabase is bound on IPv4. In environments where SUPABASE_URL is http://localhost:... (common in local runs), this new test can fail with connection errors unrelated to auth behavior, creating flaky CI/local results.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/pages/delete_account.vue (1)
44-53:⚠️ Potential issue | 🔴 CriticalMount-time session gating breaks the standalone delete flow.
/delete_accountauthenticates with the submitted credentials, so a verified user can legitimately arrive here without an existing session.getSession()then marks them unverified and hides the form, while the new test suite already shows that an unverified user never reaches the RPC path because normal SDK sign-in is rejected first. This route needs to treat “no session yet” as “not reauthenticated yet” and redirect from the reauth failure instead of blocking on mount.Also applies to: 114-120, 167-200, 229-241
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/delete_account.vue` around lines 44 - 53, The mount-time gating wrongly treats “no session” as an unverified user; update checkEmailVerification (and the other similar blocks around the other occurrences) to distinguish between a session fetch error and an absent session: call supabase.auth.getSession(), and if there's no sessionResult?.session, do not mark isEmailVerified.value = false — instead set a reauth-pending flag (e.g., isReauthenticated.value = false) or simply return so the UI remains in the reauthentication flow; only set isEmailVerified.value = false when you receive an explicit sessionError indicating a failure, and set isEmailVerified from sessionResult.session.user.email_confirmed_at when a session exists. Ensure the same change is applied to the other identical checks referenced (the blocks at the other occurrences) so the standalone delete flow isn't blocked on mount.src/pages/settings/account/index.vue (1)
232-241:⚠️ Potential issue | 🟠 MajorClose the confirmation modal before redirecting.
The danger button is
preventClose: trueon Line 234 and only callscloseDialog()whenperformAccountDeletion()returnstrue. In bothemail_not_verifiedpaths you navigate away and returnfalse, so the dialog is never dismissed; that can leave/resend_emailunder stale modal state and keeponDialogDismiss()unresolved.🛠️ Suggested fix
+async function redirectToEmailVerificationFromDialog() { + dialogStore.closeDialog({ text: t('button-cancel'), role: 'cancel' }) + await redirectToEmailVerification() +} + async function performAccountDeletion(password: string) { if (!main.auth || main.auth?.email == null) return false const supabaseClient = useSupabase() if (!isEmailVerified.value) { - await redirectToEmailVerification() + await redirectToEmailVerificationFromDialog() return false } @@ if (deleteError.message?.includes('email_not_verified')) { - await redirectToEmailVerification() + await redirectToEmailVerificationFromDialog() return false }Also applies to: 258-261, 333-336
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/settings/account/index.vue` around lines 232 - 241, The confirmation dialog isn't closed on the error/navigation branches, leaving a stale modal; update the danger-button handlers (the async handler passed to the dialog where performAccountDeletion(deleteAccountPassword.value) is called) to always call dialogStore.closeDialog(...) before navigating or returning false (i.e., close the dialog immediately on the email_not_verified branches and any early-return/error branches), and apply the same change to the other similar handlers referenced (the handlers around the blocks at the other occurrences). Use the same dialog identifier used elsewhere (dialogStore.closeDialog({ text: t('i-am-sure'), role: 'danger' }) or the appropriate dialog text/role in those spots) so onDialogDismiss() resolves before navigation or returning.tests/delete-user-reauth.test.ts (1)
172-247: 🛠️ Refactor suggestion | 🟠 MajorAdd the browser regression for the new resend-email redirects.
These tests cover the RPC/auth boundary, but this PR also changes
/delete_accountand/settings/account. We still need a Playwright case proving an unverified user is sent to/resend_emailfrom both entry points.As per coding guidelines, "Always cover database changes with Postgres-level tests and complement them with end-to-end tests for affected user flows".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/delete-user-reauth.test.ts` around lines 172 - 247, Add a Playwright E2E test that proves an unverified user is redirected to /resend_email when they try to access both /delete_account and /settings/account: create or reuse an unverified test user (matching the unverified user used in delete-user-reauth.test.ts), sign in via the browser session, navigate to /delete_account and assert the page URL or navigation landed on /resend_email (and repeat for /settings/account); ensure assertions also verify any expected query param (e.g., original path) so the redirect is the new resend-email flow.
🧹 Nitpick comments (1)
tests/delete-user-reauth.test.ts (1)
19-28: Reuse the shared Supabase test fetch policy for the anon client.
getSupabaseClient()already wraps Supabase calls with retry-on-503 intests/test-utils.ts:538-584. This client skips that wrapper, so the new SDK-session regression can fail on transient test infra noise instead of the email-verification behavior you actually want to assert.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/delete-user-reauth.test.ts` around lines 19 - 28, The anon Supabase client in createAnonSupabaseClient bypasses the shared retry-on-503 test fetch policy; change it to reuse the shared test wrapper from tests/test-utils.ts (getSupabaseClient) instead of directly calling createClient. Update createAnonSupabaseClient to call or delegate to getSupabaseClient (or to construct the client using the same fetch/retry wrapper exported by tests/test-utils.ts) while preserving the anon URL/key and auth.persistSession: false so the anon client gets the retry-on-503 behavior used by other tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/pages/delete_account.vue`:
- Around line 44-53: The mount-time gating wrongly treats “no session” as an
unverified user; update checkEmailVerification (and the other similar blocks
around the other occurrences) to distinguish between a session fetch error and
an absent session: call supabase.auth.getSession(), and if there's no
sessionResult?.session, do not mark isEmailVerified.value = false — instead set
a reauth-pending flag (e.g., isReauthenticated.value = false) or simply return
so the UI remains in the reauthentication flow; only set isEmailVerified.value =
false when you receive an explicit sessionError indicating a failure, and set
isEmailVerified from sessionResult.session.user.email_confirmed_at when a
session exists. Ensure the same change is applied to the other identical checks
referenced (the blocks at the other occurrences) so the standalone delete flow
isn't blocked on mount.
In `@src/pages/settings/account/index.vue`:
- Around line 232-241: The confirmation dialog isn't closed on the
error/navigation branches, leaving a stale modal; update the danger-button
handlers (the async handler passed to the dialog where
performAccountDeletion(deleteAccountPassword.value) is called) to always call
dialogStore.closeDialog(...) before navigating or returning false (i.e., close
the dialog immediately on the email_not_verified branches and any
early-return/error branches), and apply the same change to the other similar
handlers referenced (the handlers around the blocks at the other occurrences).
Use the same dialog identifier used elsewhere (dialogStore.closeDialog({ text:
t('i-am-sure'), role: 'danger' }) or the appropriate dialog text/role in those
spots) so onDialogDismiss() resolves before navigation or returning.
In `@tests/delete-user-reauth.test.ts`:
- Around line 172-247: Add a Playwright E2E test that proves an unverified user
is redirected to /resend_email when they try to access both /delete_account and
/settings/account: create or reuse an unverified test user (matching the
unverified user used in delete-user-reauth.test.ts), sign in via the browser
session, navigate to /delete_account and assert the page URL or navigation
landed on /resend_email (and repeat for /settings/account); ensure assertions
also verify any expected query param (e.g., original path) so the redirect is
the new resend-email flow.
---
Nitpick comments:
In `@tests/delete-user-reauth.test.ts`:
- Around line 19-28: The anon Supabase client in createAnonSupabaseClient
bypasses the shared retry-on-503 test fetch policy; change it to reuse the
shared test wrapper from tests/test-utils.ts (getSupabaseClient) instead of
directly calling createClient. Update createAnonSupabaseClient to call or
delegate to getSupabaseClient (or to construct the client using the same
fetch/retry wrapper exported by tests/test-utils.ts) while preserving the anon
URL/key and auth.persistSession: false so the anon client gets the retry-on-503
behavior used by other tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 633bc88b-e618-4635-9978-a91855cb0157
📒 Files selected for processing (3)
src/pages/delete_account.vuesrc/pages/settings/account/index.vuetests/delete-user-reauth.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/delete-user-reauth.test.ts (1)
184-259:⚠️ Potential issue | 🟠 MajorAdd E2E coverage for the two user-facing deletion entry points.
These tests validate RPC/DB behavior, but the PR also changes customer-facing redirect behavior. Please add Playwright coverage to assert unverified users are redirected to
/resend_emailfrom both account-deletion entry points.I can draft a minimal Playwright spec covering both entry points if you want.
As per coding guidelines: "tests/**/*.test.ts: Always cover database changes with Postgres-level tests and complement them with end-to-end tests for affected user flows".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/delete-user-reauth.test.ts` around lines 184 - 259, Add Playwright E2E tests that assert unverified users are redirected to /resend_email for both deletion entry points mentioned in the PR; locate the related test utilities and constants used in tests/delete-user-reauth.test.ts (e.g., deleteUserAs, createAnonSupabaseClient, USER_EMAIL_DELETE_USER_UNVERIFIED, userIdDeleteUserUnverified) and implement a new Playwright spec that (1) visits the user-facing deletion endpoint (explicit UI route) and confirms the browser is redirected to /resend_email for the unverified user, and (2) repeats the check for the alternate deletion entry point (the other customer-facing route) using the same unverified account, asserting the redirect and any expected UI text. Ensure the new spec is placed under tests/e2e or tests/playwright per repo conventions and uses the existing test constants and helpers to sign in or simulate the unverified user session.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/delete-user-reauth.test.ts`:
- Around line 248-251: Replace the fragile string-match against the
human-readable error message with an assertion on the stable server error code:
change the expectation that checks signInError?.message to assert the Auth
error's code (e.g. expect(signInError?.code).toBe('email_not_confirmed')) and/or
its status if needed; locate the assertions around the signInData/signInError
variables in delete-user-reauth.test.ts and update the test to use
signInError?.code (and optionally signInError?.status) instead of the message
text.
---
Outside diff comments:
In `@tests/delete-user-reauth.test.ts`:
- Around line 184-259: Add Playwright E2E tests that assert unverified users are
redirected to /resend_email for both deletion entry points mentioned in the PR;
locate the related test utilities and constants used in
tests/delete-user-reauth.test.ts (e.g., deleteUserAs, createAnonSupabaseClient,
USER_EMAIL_DELETE_USER_UNVERIFIED, userIdDeleteUserUnverified) and implement a
new Playwright spec that (1) visits the user-facing deletion endpoint (explicit
UI route) and confirms the browser is redirected to /resend_email for the
unverified user, and (2) repeats the check for the alternate deletion entry
point (the other customer-facing route) using the same unverified account,
asserting the redirect and any expected UI text. Ensure the new spec is placed
under tests/e2e or tests/playwright per repo conventions and uses the existing
test constants and helpers to sign in or simulate the unverified user session.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 958ca9df-59eb-4df7-8c65-f4bb1f92bf8e
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
tests/delete-user-reauth.test.ts
| expect(signInData.session).toBeNull() | ||
| expect(signInError).toBeTruthy() | ||
| expect(signInError?.message ?? '').toContain('Email not confirmed') | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In @supabase/supabase-js v2.93.3, what stable fields are guaranteed on AuthApiError returned by auth.signInWithPassword for an unconfirmed email (for example, status and code)?
💡 Result:
In @supabase/supabase-js@2.93.3, auth.signInWithPassword() failures coming back from the Auth HTTP API are returned as an AuthApiError (from @supabase/auth-js). For an unconfirmed email, the stable/guaranteed fields you can rely on are:
name: "AuthApiError"(set by the class) [1]message: string(standardError.message, passed into the constructor) [1]status: number(HTTP status code for the response;AuthApiErroralways has a numericstatus) [1]code: string(server error code; for this case it isemail_not_confirmed) [2]
Supabase’s Auth docs also state that server-originated AuthApiErrors always have code and status present. [2]
Sources:
[1] AuthApiError implementation in auth-js (src/lib/errors.ts) showing name, status, code being set. (github.com)
[2] Supabase Auth “Error Codes” docs (notes on AuthApiError having code+status, and the email_not_confirmed code). (supabase.com)
Citations:
- 1: https://github.com/supabase/auth-js/blob/25c6b42/src/lib/errors.ts
- 2: https://supabase.com/docs/guides/auth/debugging/error-codes
Use stable code field instead of message text for auth error assertion.
Line 250 pins the error message text ('Email not confirmed') which is not guaranteed stable across Supabase SDK versions. The AuthApiError exposes stable fields: status (HTTP code) and code (server error code: 'email_not_confirmed' for this case).
Suggested fix
expect(signInData.session).toBeNull()
expect(signInError).toBeTruthy()
- expect(signInError?.message ?? '').toContain('Email not confirmed')
+ expect(signInError?.code).toBe('email_not_confirmed')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| expect(signInData.session).toBeNull() | |
| expect(signInError).toBeTruthy() | |
| expect(signInError?.message ?? '').toContain('Email not confirmed') | |
| expect(signInData.session).toBeNull() | |
| expect(signInError).toBeTruthy() | |
| expect(signInError?.code).toBe('email_not_confirmed') |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/delete-user-reauth.test.ts` around lines 248 - 251, Replace the fragile
string-match against the human-readable error message with an assertion on the
stable server error code: change the expectation that checks
signInError?.message to assert the Auth error's code (e.g.
expect(signInError?.code).toBe('email_not_confirmed')) and/or its status if
needed; locate the assertions around the signInData/signInError variables in
delete-user-reauth.test.ts and update the test to use signInError?.code (and
optionally signInError?.status) instead of the message text.
|
1 participant
Summary (AI generated)
/resend_emailwhen the backend rejects deletion.delete_user()rejects unverified users and that an unverified user cannot obtain a normal Supabase SDK session.Motivation (AI generated)
The account-deletion flow must enforce verified email ownership consistently at every user-facing entry point and keep explicit backend coverage for the RPC boundary.
Business Impact (AI generated)
This reduces the risk of account-lifecycle abuse and support escalations around deletion while making the security rule visible and consistent for users.
Test Plan (AI generated)
bun test tests/delete-user-reauth.test.tsbunx eslint tests/delete-user-reauth.test.ts src/pages/settings/account/index.vue src/pages/delete_account.vueScreenshots (AI generated)
Checklist (AI generated)
bun run lint:backend && bun run lint.Generated with AI
Summary by CodeRabbit
New Features
Bug Fixes
Tests