fix(db): separate mfa and email otp checks#2454
Conversation
|
Warning Review limit reached
More reviews will be available in 37 minutes and 47 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThis PR adds email OTP session authentication verification by introducing SQL security-definer functions to check JWT OTP methods and MFA assurance, updating TypeScript types, implementing a session-verification helper, integrating it into the OTP handler, and adding comprehensive tests. ChangesEmail OTP Session Authentication
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 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. Comment |
Merging this PR will not alter performance
Comparing 
Footnotes
|
riderx
force-pushed
the
codex/fix-mfa-otp-aal-bypass
branch
from
542d05a to
b78afd4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@supabase/functions/_backend/private/verify_email_otp.ts`:
- Around line 116-125: The current logic treats any non-verified result from
verifyEmailOtpAuthSession(c, verifyData.session.access_token) the same and calls
recordFailedAccountAuth(c, auth.userId), which will penalize users when the
RPC/backend fails; change the conditional to only call recordFailedAccountAuth
and return an "invalid_otp_auth" response when emailOtpAuth.verified === false
and emailOtpAuth.error is falsy (i.e., an explicit user auth failure); if
emailOtpAuth.error exists (indicating an RPC/internal failure), log the error
via cloudlog with context and return a 500/appropriate transient error without
recording a failed account auth. Ensure you reference verifyEmailOtpAuthSession,
emailOtpAuth.error, emailOtpAuth.verified, and recordFailedAccountAuth when
making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 328714a1-dc4f-4c11-8b11-393278b7c083
📒 Files selected for processing (7)
src/types/supabase.types.tssupabase/functions/_backend/private/verify_email_otp.tssupabase/functions/_backend/utils/supabase.types.tssupabase/migrations/20260608114543_split_mfa_session_and_email_otp_checks.sqlsupabase/tests/58_test_mfa_session_otp_split.sqltests/security-definer-execute-hardening.test.tstests/verify-email-otp-auth-session.unit.test.ts
riderx
force-pushed
the
codex/fix-mfa-otp-aal-bypass
branch
from
b78afd4 to
e3c8e89
Compare
|
WcaleNieWolny
left a comment
WcaleNieWolny
left a comment
There was a problem hiding this comment.
⚠️ This breaks log_as (admin impersonation) for users who have 2FA enabled
Removing the amr.method = 'otp' branch from verify_mfa() closes a real bypass, but that branch is load-bearing for the admin spoof (private/log_as). As written, this PR locks platform admins out of exactly the 2FA accounts they most need to support, and log_as.ts isn't touched to compensate.
How log_as actually works
- Verify the calling admin is a platform admin →
is_platform_admin()→ checks the admin's ownverify_mfa()(admin has real TOTP →aal2, fine). - Mint a session for the target user:
supabaseAdmin.auth.admin.generateLink({ type: 'magiclink' })→verifyOtp({ type: 'email' }). - Return that JWT to the frontend, which
setSessions it. The admin is now browsing as the target user.
The session minted in step 2 is single-factor, so its claims are:
aal = aal1(the target user's TOTP was never completed — the admin can't complete it)amr = [{ method: 'otp' }]
Why removing the branch breaks it
The target user's apps, orgs, channels, etc. carry the RESTRICTIVE policy USING (verify_mfa()) ("Prevent non 2FA access"). For a 2FA-enrolled user, the impersonation session must pass verify_mfa() or the admin sees an empty account.
aal1 + amr.otp, user has TOTP |
|
|---|---|
Old verify_mfa() |
first branch fails, OR (amr has 'otp') → TRUE ✅ spoof works |
New verify_mfa() (this PR) |
first branch fails, no otp branch → FALSE ❌ RLS denies every row |
This isn't incidental — the otp branch was added in commit 292ad2741 "fix: 2fa for admins" specifically to make impersonation of 2FA users work.
Why the obvious workarounds don't help
- "Just disable magic-link sign-in in Supabase" — Supabase exposes magic link and email OTP as one
/otpprovider; there's no toggle to disable first-factor magic-link login while keeping email OTP. And the app needs that endpoint on:emailOtp.ts(sendEmailOtpVerification→signInWithOtp) andManageTwoFactor.vueuse it for MFA-enrollment / sensitive-action step-up — the veryemail_otp_verified_atflow this PR hardens. So the same endpoint is both the attack vector and a required feature; you can't disable it. - Can't tell impersonation from abuse by
amr— both a real first-factor email-OTP login and alog_asimpersonation token areaal1+method: 'otp'. They differ only by provenance:log_asmints server-side via service-rolegenerateLink, a path the public can't reach.
Suggested fix (ship alongside the branch removal)
Mark the impersonation session explicitly so verify_mfa() can trust impersonation without trusting any otp session. A session-bound marker is the tightest, lowest-footprint option (no auth hook, no Supabase config change):
1. log_as.ts — record the minted session (service_role):
const sessionId = JSON.parse(atob(jwt.split('.')[1])).session_id
await supabaseAdmin.from('impersonation_sessions').insert({
session_id: sessionId,
target_user_id: user_id,
admin_user_id: callerId,
expires_at: new Date(Date.now() + 60 * 60_000).toISOString(),
})2. verify_mfa() — replace the removed otp branch with a narrowly-scoped, trusted one:
OR EXISTS (
SELECT 1 FROM public.impersonation_sessions s
WHERE s.session_id = (auth.jwt() ->> 'session_id')
AND s.expires_at > now()
)3. Lock impersonation_sessions behind deny-all RLS, GRANT to service_role only — so the public signInWithOtp flow can never forge it.
Why this beats a custom access-token hook here:
- Bypass is bound to the one
session_idlog_asminted — not totarget_user_id, so the real user's ownaal1sessions never inherit it. session_idis stable across refresh and auto-expires viaexpires_at— no global hook re-firing on every login/refresh to reason about.- Provenance enforced in the code path (service-role insert), not a remote console toggle that can be silently flipped back on.
TL;DR
The amr.otp branch is simultaneously the vulnerability and the mechanism behind admin impersonation of 2FA users. Removing it without giving log_as a trusted, distinguishable marker ships an admin-impersonation regressin for 2FA accounts. log_as.ts + verify_mfa() need to change together.
2 participants
Summary (AI generated)
public.verify_mfa()so MFA-gated RLS/admin checks rely only on Supabase AAL session assurance.public.verify_email_otp_auth()as the separate first-factor OTP auth-method helper.private/verify_email_otpcallverify_email_otp_auth()with the verified OTP session before recordingemail_otp_verified_at.Motivation (AI generated)
Email OTP can be a first-factor
aal1login method, while completed MFA must be represented byaal2. Keeping those checks in one function let OTP first-factor sessions satisfy MFA-gated paths for users with enrolled MFA. Splitting the semantics keeps expectedaal1access for users without MFA while preventing OTP first-factor sessions from being treated as MFA.Business Impact (AI generated)
This preserves org 2FA enforcement and platform-admin MFA expectations, reducing account-takeover blast radius for customers who enable 2FA or depend on admin impersonation protections. The email OTP reauthentication flow remains supported, but now records its marker only after the OTP-specific helper validates the returned OTP session.
Test Plan (AI generated)
bun lintbun lint:backendbun run supabase:with-env -- bunx vitest run tests/verify-email-otp-auth-session.unit.test.ts tests/verify-email-otp.test.tsbunx supabase test db --local --workdir .context/supabase-worktrees/b3ffd30a supabase/tests/00-supabase_test_helpers.sql supabase/tests/58_test_mfa_session_otp_split.sqlbunx supabase test db --local --workdir .context/supabase-worktrees/b3ffd30a supabase/testsbun run supabase:with-env -- bunx vitest run tests/security-definer-execute-hardening.test.tsbun test:backendbun typecheckuser_id: local plan used an index-backed scan onauth.mfa_factors,Execution Time: 0.082 ms,Buffers: shared hit=1.Generated with AI
Summary by CodeRabbit
New Features
Tests