fix(auth): block sensitive account actions for unverified users#1690
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds frontend guards that redirect users with unverified emails to a resend-email flow and synchronizes in-memory auth with session data; introduces a PostgreSQL function Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as Client
participant Router as Router/Guard
participant Auth as Auth Store
participant UI as ResendEmail UI
participant DB as Postgres (public.delete_user)
Client->>Router: navigate to protected route (/settings /delete_account)
Router->>Auth: ensure sessionUser / sync main.auth (hadAuth?)
Auth-->>Router: sessionUser + email_confirmed_at
alt email not verified and needsVerifiedEmail
Router->>Client: redirect to /resend_email?reason=email_not_verified&return_to=...
Client->>UI: show verification notice (optionally display return_to)
else email verified
Router->>Client: allow route
Client->>DB: call stored procedure public.delete_user()
DB-->>Client: schedules deletion (to_delete_accounts), enqueues pgmq, deletes apikeys
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ebd8f08f53
ℹ️ 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".
| // User is already authenticated, but check if account got disabled | ||
| // (only if not already on account disabled page) | ||
| if (to.path !== '/accountDisabled') { | ||
| if (!main.auth.email_confirmed_at && needsVerifiedEmail) { |
There was a problem hiding this comment.
Use fresh session data for verified-email checks
The verified-email gate in the authenticated branch reads main.auth.email_confirmed_at, but main.auth is only set once when it is initially empty and is never refreshed from getSession(). If a user confirms their email after login (for example via a link opened in another tab/device), the in-memory store can remain stale and keep redirecting them to /resend_email, blocking settings access until they fully sign out or reload.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/modules/auth.ts`:
- Around line 172-174: The redirect uses a stale store value
main.auth.email_confirmed_at which can trap recently-verified users; instead use
the fresh sessionUser returned by getSession() (the sessionUser variable in
scope) when checking verification. Replace the condition that reads
main.auth.email_confirmed_at with a check against sessionUser.email_confirmed_at
(or sessionUser?.email_confirmed_at) while keeping the needsVerifiedEmail logic
and the redirect next('/resend_email') the same so the guard uses the up-to-date
session data.
In
`@supabase/migrations/20260224193302_auto_restore_account_after_password_reset.sql`:
- Around line 1-15: The SECURITY DEFINER function
restore_pending_delete_on_password_change currently has no explicit owner and
will default to the migration-runner role; add an ALTER FUNCTION statement to
set its owner to "postgres" after the function definition (use ALTER FUNCTION
public.restore_pending_delete_on_password_change() OWNER TO "postgres") so the
function runs with the intended postgres privileges.
In
`@supabase/migrations/20260226090000_require_verified_email_for_delete_user.sql`:
- Around line 61-70: The INSERT into to_delete_accounts can raise a unique-key
violation on account_id if delete_user() is called twice before the row is
processed; modify the INSERT that uses user_id_fn to include an ON CONFLICT
(account_id) clause (e.g., ON CONFLICT (account_id) DO NOTHING) so duplicate
attempts inside the reauth window don't error the transaction and cause the
pgmq.send rollback.
- Around line 29-32: The INSERT into to_delete_accounts (used by delete_user())
can fail with a unique constraint if called twice; modify the INSERT statement
that adds account_id into to_delete_accounts to include an ON CONFLICT
(account_id) DO NOTHING clause (or DO UPDATE/RETURNING as appropriate) so
repeated calls within the 5-minute window are idempotent and do not raise an
unhandled exception; locate the INSERT related to to_delete_accounts.account_id
in the delete_user() flow and add the ON CONFLICT handling there.
In `@tests/test-utils.ts`:
- Around line 83-84: Add a seed row for the reset user constants so tests can
find USER_ID_DELETE_USER_RESET and USER_EMAIL_DELETE_USER_RESET: insert a
user/profile record in your seed SQL with id = USER_ID_DELETE_USER_RESET and
email = USER_EMAIL_DELETE_USER_RESET, mirroring the same columns/values used for
the other seeded users (auth.users and any profiles/metadata the tests expect,
e.g., created_at, confirmed/protected fields, and matching stale/fresh pattern
if applicable) so the test constants resolve to an actual seeded user.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/modules/auth.tssupabase/migrations/20260224193302_auto_restore_account_after_password_reset.sqlsupabase/migrations/20260226090000_require_verified_email_for_delete_user.sqltests/test-utils.ts
| CREATE OR REPLACE FUNCTION "public"."restore_pending_delete_on_password_change"() | ||
| RETURNS "trigger" | ||
| LANGUAGE "plpgsql" | ||
| SECURITY DEFINER | ||
| SET "search_path" TO '' | ||
| AS $$ | ||
| BEGIN | ||
| IF NEW."encrypted_password" IS DISTINCT FROM OLD."encrypted_password" THEN | ||
| DELETE FROM "public"."to_delete_accounts" | ||
| WHERE "account_id" = NEW."id"; | ||
| END IF; | ||
|
|
||
| RETURN NEW; | ||
| END; | ||
| $$; |
There was a problem hiding this comment.
Add ALTER FUNCTION ... OWNER TO "postgres" for the SECURITY DEFINER function.
The companion migration (20260226090000) explicitly sets the function owner to postgres. Without the same statement here, the owner defaults to the migration-runner role, which on hosted Supabase is supabase_admin rather than postgres. A SECURITY DEFINER function running under supabase_admin may have different effective privileges than intended.
🛡️ Proposed addition (after line 15)
+
+ALTER FUNCTION "public"."restore_pending_delete_on_password_change"() OWNER TO "postgres";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@supabase/migrations/20260224193302_auto_restore_account_after_password_reset.sql`
around lines 1 - 15, The SECURITY DEFINER function
restore_pending_delete_on_password_change currently has no explicit owner and
will default to the migration-runner role; add an ALTER FUNCTION statement to
set its owner to "postgres" after the function definition (use ALTER FUNCTION
public.restore_pending_delete_on_password_change() OWNER TO "postgres") so the
function runs with the intended postgres privileges.
| export const USER_ID_DELETE_USER_RESET = 'd9c3f1a7-3e6b-4b2f-9a1d-8c7e5f4a3210' | ||
| export const USER_EMAIL_DELETE_USER_RESET = 'delete-user-reset@capgo.app' |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for the new test user in seed.sql
rg -n 'd9c3f1a7-3e6b-4b2f-9a1d-8c7e5f4a3210|delete-user-reset@capgo.app' supabase/seed.sqlRepository: Cap-go/capgo
Length of output: 38
🏁 Script executed:
# Check seed.sql exists and examine test user seeding patterns
cat supabase/seed.sql | head -100
echo "---"
# Search for existing test users mentioned in test-utils.ts
rg -i 'stale|fresh' supabase/seed.sql | head -20Repository: Cap-go/capgo
Length of output: 11451
🏁 Script executed:
# Search for usage of the new constants in test files
rg -r 'USER_ID_DELETE_USER_RESET|USER_EMAIL_DELETE_USER_RESET' tests/ --type ts --type jsRepository: Cap-go/capgo
Length of output: 1217
🏁 Script executed:
# Search for literal usage of the new constants
rg 'USER_ID_DELETE_USER_RESET|USER_EMAIL_DELETE_USER_RESET' tests/ -g '*.test.ts' -g '*.test.js' -v 'include:' -v 'globalSetup'Repository: Cap-go/capgo
Length of output: 50488
🏁 Script executed:
# Search for the new constants in test files - simpler approach
rg 'USER_ID_DELETE_USER_RESET|USER_EMAIL_DELETE_USER_RESET' tests/ --max-count 5Repository: Cap-go/capgo
Length of output: 249
Add corresponding seed data for the new reset user constants before tests can reliably use them.
USER_ID_DELETE_USER_RESET and USER_EMAIL_DELETE_USER_RESET match the stale/fresh pattern but lack corresponding entries in supabase/seed.sql. The stale and fresh variants are both seeded; the reset user must be added to prevent test failures when these constants are used.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test-utils.ts` around lines 83 - 84, Add a seed row for the reset user
constants so tests can find USER_ID_DELETE_USER_RESET and
USER_EMAIL_DELETE_USER_RESET: insert a user/profile record in your seed SQL with
id = USER_ID_DELETE_USER_RESET and email = USER_EMAIL_DELETE_USER_RESET,
mirroring the same columns/values used for the other seeded users (auth.users
and any profiles/metadata the tests expect, e.g., created_at,
confirmed/protected fields, and matching stale/fresh pattern if applicable) so
the test constants resolve to an actual seeded user.
e14ddef to
cf1467a
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/modules/auth.ts (1)
96-96: Consider replacing hard-coded sensitive paths with route meta policy.
to.path.startsWith('/settings') || to.path === '/delete_account'works, but a route-levelmeta.requiresVerifiedEmailflag would be easier to maintain and less error-prone as routes evolve.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/auth.ts` at line 96, Replace the hard-coded path checks in the auth guard (the needsVerifiedEmail calculation) with a route meta flag: check for to.meta?.requiresVerifiedEmail and use that boolean to determine needsVerifiedEmail; keep the existing startsWith('/settings') || to.path === '/delete_account' as a backward-compatible fallback only when the meta flag is undefined to avoid breaking routes that haven't been migrated; update places referencing needsVerifiedEmail so new routes can signal the requirement via their route definitions (meta.requiresVerifiedEmail).
🤖 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/modules/auth.ts`:
- Line 96: Replace the hard-coded path checks in the auth guard (the
needsVerifiedEmail calculation) with a route meta flag: check for
to.meta?.requiresVerifiedEmail and use that boolean to determine
needsVerifiedEmail; keep the existing startsWith('/settings') || to.path ===
'/delete_account' as a backward-compatible fallback only when the meta flag is
undefined to avoid breaking routes that haven't been migrated; update places
referencing needsVerifiedEmail so new routes can signal the requirement via
their route definitions (meta.requiresVerifiedEmail).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/modules/auth.tssupabase/migrations/20260226090000_require_verified_email_for_delete_user.sql
🚧 Files skipped from review as they are similar to previous changes (1)
- supabase/migrations/20260226090000_require_verified_email_for_delete_user.sql
299bba4 to
b463239
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pages/resend_email.vue`:
- Around line 50-59: Replace the hardcoded English strings in the verification
banner with i18n keys so non-English locales render correctly: use the Vue i18n
translate function (e.g. $t or i18n.t) for the three texts inside the div
controlled by emailVerificationBlockingReason — the main message ("You cannot
access this action right now because your email is not verified yet."), the
helper sentence about verifying email and account actions, and the attempted
destination label shown when returnTo is present (interpolating returnTo). Add
corresponding i18n keys such as verification.banner.blocked,
verification.banner.details, and verification.banner.attemptedDestination in
your locale files and update the template to call the translator for these keys
where those hardcoded strings currently are.
In
`@supabase/migrations/20260226090000_require_verified_email_for_delete_user.sql`:
- Around line 48-74: The pgmq.send call (on_user_delete) runs before the INSERT
into to_delete_accounts so retries can enqueue duplicate events; change the
ordering so the scheduling INSERT into "public"."to_delete_accounts" (currently
using user_id_fn, removal_date, removed_data) is executed first with INSERT ...
RETURNING (or check GET DIAGNOSTICS row_count) and then only call PERFORM
"pgmq"."send"('on_user_delete', ...) when the INSERT actually inserted a row
(i.e., when RETURNING returned a row or row_count > 0); keep the DELETE FROM
"public"."apikeys" as-is or run it after the send if intended, but ensure
pgmq.send and downstream cleanup happen only on a new insert to make
on_user_delete idempotent.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/modules/auth.tssrc/pages/delete_account.vuesrc/pages/resend_email.vuesupabase/migrations/20260226090000_require_verified_email_for_delete_user.sql
b463239 to
8661fc4
Compare
|
* fix(api): preserve channel owner on channel upsert * fix(auth): block sensitive account actions for unverified users (#1690) * fix(auth): block account deletion for unverified users * fix(auth): refresh session fields for email verification gate * fix(auth): make delete_user insert idempotent * fix(auth): explain blocked delete/settings when email unverified * fix(auth): block delete action when email is unverified * fix(auth): localize resend email block and make delete_user idempotent * Restrict invite_user_to_org RPC to authenticated callers (#1710) * fix(db): restrict invite_user_to_org public rpc * fix(db): use caller identity in invite 2FA check * fix(security): restrict webhook select to admin users (#1705) * Secure record_build_time RPC for authorized callers (#1711) * fix(db): secure record_build_time rpc writes * fix(db): preserve service-role record_build_time path * fix(api): preserve channel owner on channel upsert
* fix(security): restrict apikey oracle rpc access * fix: webapp url * fix: fix * chore(release): 12.116.9 * fix: envs * Revert "Merge pull request #1707 from Cap-go/fix_webapp_url" This reverts commit ff20d1a. * fix: typo * chore(release): 12.116.10 * fix(security): restrict apikey oracle rpc access * fix: return 503 instead of 400 for service_unavailable build errors Builder availability errors (not configured, call failed, error response, missing upload URL) are transient server-side failures, not client errors. Returning 503 allows the CLI retry logic to automatically retry these requests instead of treating them as terminal 400 errors. * chore(release): 12.116.11 * fix: update PWD script * fix: env vars * fix: modal responsive * feat: forward buildOptions + buildCredentials to builder (pass-through) * fix: correct vue/html-indent in DemoOnboardingModal * fix: use snake_case (build_options, build_credentials) in public API, map to camelCase for builder * fix(security): sanitize SQL interpolation in Cloudflare Analytics Engine queries (#1702) * chore(release): 12.116.12 * Add unit tests for builder payload shape contract Extract buildBuilderPayload() from the inline fetch body so the snake_case → camelCase mapping and exact key set can be tested. 6 vitest cases verify: camelCase output, no legacy credentials field, correct metadata keys, and pass-through of contents. * Reject deprecated `credentials` field with clear upgrade error Old CLI clients sending the flat `credentials` field would have it silently dropped, causing confusing builder failures. Now the proxy explicitly rejects non-empty `credentials` with a migration message pointing to `build_credentials`. * fix(security): clean up role_bindings on member removal (#1722) * chore(release): 12.116.13 * fix(security): use parameterized query in getStoreAppByIdCF to prevent SQL injection The appId parameter was directly interpolated into the D1 SQL query string, creating a SQL injection vulnerability. Switched to bound parameter via .bind(). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(security): prevent privilege escalation in role_bindings endpoint Add priority_rank check so callers cannot assign or update roles with higher privileges than their own. Without this, any user with org.update_user_roles could escalate to org_super_admin. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(security): enforce is_assignable in role_bindings INSERT RLS policy Direct PostgREST inserts could bypass the endpoint's is_assignable check and assign non-assignable roles (e.g. platform_super_admin). The RLS INSERT policy now requires the target role to have is_assignable = true. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(security): cascade all role bindings on member removal delete_org_member_role previously only deleted the org-level binding, leaving orphaned app/channel bindings. A removed member could retain app-level access. Now deletes all bindings for the user in the org. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(security): add trigger to prevent deleting last super_admin binding Direct PostgREST DELETEs on role_bindings could bypass the last super_admin guards in delete_org_member_role. A BEFORE DELETE trigger now rejects deletion of the last org_super_admin binding in any org. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(security): support hashed API keys in rbac_check_permission_direct The RBAC path in rbac_check_permission_direct looked up API keys with WHERE key = p_apikey, which silently failed for hashed keys. Switched to find_apikey_by_value() which handles both plain-text and hashed keys. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: reword comment to pass typos CI check Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: remove unused desc import from role_bindings.ts Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(security): add FOR UPDATE lock to prevent write-skew on last super_admin delete Two concurrent DELETE transactions could both pass the count check and both delete their rows, leaving zero super_admins. A SELECT ... FOR UPDATE on the super_admin binding set now serializes concurrent deletes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: prevent API key privilege escalation and fix organization member deletion test - Add validation to prevent limited API keys from creating unlimited keys - Fix organization-api test to work with sync_org_user_to_role_binding trigger - Change test user_right from 'invite_read' to 'read' (trigger-compatible) - Verify trigger-created role_bindings instead of manually inserting them * fix: allow CASCADE deletions in prevent_last_super_admin_binding_delete and fix RBAC test compatibility - Add org existence check in trigger to allow CASCADE deletions when org is being deleted - Add service_role bypass for administrative operations and tests - Update tests to work with RBAC security constraints: - 34_test_rbac_rls.sql: Remove DELETE operation that violated super_admin protection - 35_test_is_admin_rbac.sql: Use service_role for test setup INSERT - All SQL database tests now pass (860 tests) - Backend tests remain passing (68 tests) * fix(security): make getCallerMaxPriorityRank auth-type-aware and remove API key data leak * chore(release): 12.116.14 * fix(security): correct API key RBAC principal mapping and remove service_role bypass * fix(security): correct RBAC migration comments and add privilege check on delete - Update migration comments to accurately reflect that service_role is NOT exempt from the last super_admin protection trigger - Replace FOR UPDATE scan with pg_advisory_xact_lock to avoid cross-transaction deadlocks - Add privilege-rank check in delete handler to prevent deleting higher-ranked role bindings - Aligns with established advisory lock patterns in codebase Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai> * fix: add self-2fa-required message for 2FA enforcement in multiple languages * chore(release): 12.116.15 * fix(frontend): validate 2fa before enabling org enforcement (#1729) * chore(release): 12.116.16 * fix(deps): update vue monorepo to v3.5.29 (#1731) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * chore(release): 12.116.17 * chore: remove unused cloudflare function getStoreAppByIdCF * chore(release): 12.116.18 * chore: stop editing immutable base migration * fix(frontend): disable auto demo onboarding modal (#1733) * chore(release): 12.116.19 * fix(auth): block sensitive account actions for unverified users (#1690) * fix(auth): block account deletion for unverified users * fix(auth): refresh session fields for email verification gate * fix(auth): make delete_user insert idempotent * fix(auth): explain blocked delete/settings when email unverified * fix(auth): block delete action when email is unverified * fix(auth): localize resend email block and make delete_user idempotent * Restrict invite_user_to_org RPC to authenticated callers (#1710) * fix(db): restrict invite_user_to_org public rpc * fix(db): use caller identity in invite 2FA check * fix(security): restrict webhook select to admin users (#1705) * Secure record_build_time RPC for authorized callers (#1711) * fix(db): secure record_build_time rpc writes * fix(db): preserve service-role record_build_time path * fix(security): restrict apikey oracle rpc access * chore: stop editing immutable base migration * fix(security): restrict apikey oracle rpc access * chore(release): 12.116.20 * fix(security): restrict apikey oracle rpc access * chore: stop editing immutable base migration * fix(security): restrict apikey oracle rpc access * chore: stop editing immutable base migration * fix(security): restrict apikey oracle rpc access --------- Co-authored-by: WcaleNieWolny <isupermichael007@gmail.com> Co-authored-by: WcaleNieWolny <50914789+WcaleNieWolny@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: LOLO <131777939+artylobos@users.noreply.github.com> Co-authored-by: Jordan Lorho <jordan.lorho@gmail.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>



Summary (AI generated)
auth.users.email_confirmed_at.public.delete_user()to reject deletes when email is not verified./settings*and/delete_account, redirecting them to/resend_email.Motivation (AI generated)
Prevent account lifecycle denial-of-service where attackers can register arbitrary unverified emails and place legitimate owners into a 30-day pending deletion state without proof of ownership.
Business Impact (AI generated)
This blocks abuse that can lock customer emails out of the platform, reduces support overhead, and preserves account integrity during onboarding and recovery.
Test Plan (AI generated)
bun lintGenerated with AI
Summary by CodeRabbit