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.
Heyzerohey
left a comment
Heyzerohey
left a comment
There was a problem hiding this comment.
Code Review: hardens split MFA and OTP session verification
Thanks for this PR! The split of MFA session assurance from email OTP first-factor checks is a great security improvement. Here are two optimizations that simplify the implementation and improve performance.
1. Optimize verifyEmailOtpAuthSession by verifying JWT locally (TypeScript-level)
Since the accessToken is returned directly from a successful supabase.auth.verifyOtp call, we already know the token is authentic and signed. Instead of making an extra network HTTP RPC roundtrip to PostgreSQL via PostgREST to check the amr claim, we can decode the claims locally using the existing getClaimsFromJWT utility.
This saves a database network roundtrip (saving 15–50ms latency) and completely eliminates the database function verify_email_otp_auth from migrations, security-definer audit lists, and TypeScript type updates.
Proposed TypeScript Refactoring
In supabase/functions/_backend/private/verify_email_otp.ts:
import { getClaimsFromJWT } from '../utils/hono.ts'
export async function verifyEmailOtpAuthSession(c: Parameters<typeof getClaimsFromJWT>[0], accessToken: string) {
const claims = await getClaimsFromJWT(c, accessToken)
const amr = claims?.amr
// Verify that the JWT authentication methods references (amr) contains 'otp'
const verified = Array.isArray(amr) && amr.some((elem: any) => elem?.method === 'otp')
return {
verified,
error: claims ? null : new Error('Invalid token claims'),
}
}If we adopt this local check:
- We can completely remove
verify_email_otp_authfromsupabase/migrations/20260608114543_split_mfa_session_and_email_otp_checks.sql. - We can remove the changes in
tests/security-definer-execute-hardening.test.ts. - We don't need to update
supabase.types.ts.
2. Optimize SQL verify_mfa() using EXISTS
If we keep verify_mfa() in SQL, we can optimize the count(id) > 0 and array comparison pattern. Using EXISTS allows the database planner to short-circuit index scanning as soon as the first verified factor is found, which is more performant than scanning/counting all rows.
Proposed SQL Optimization
In supabase/migrations/20260608114543_split_mfa_session_and_email_otp_checks.sql:
CREATE OR REPLACE FUNCTION public.verify_mfa()
RETURNS boolean
LANGUAGE sql
STABLE
SECURITY DEFINER
SET search_path = ''
AS $$
SELECT
CASE
WHEN EXISTS (
SELECT 1
FROM auth.mfa_factors
WHERE user_id = auth.uid()
AND status = 'verified'
) THEN COALESCE(auth.jwt()->>'aal', 'aal1') = 'aal2'
ELSE true
END;
$$;Why this is cleaner:
- Short-circuiting:
EXISTSis the standard SQL pattern for checks like this. - Simpler Logic: Avoids array construction (
array['aal2']) and array containment operators (<@), making the function much more readable. - Redundancy: Removes the redundant
(SELECT auth.uid())subquery wrapper in theWHEREclause.
Heyzerohey
left a comment
Heyzerohey
left a comment
There was a problem hiding this comment.
Follow-up: Resolving the log_as (Admin Impersonation) bypass cleanly
To resolve @WcaleNieWolny's concern about breaking admin impersonation for 2FA-enabled accounts, we need a session-bound trust marker. Here is a complete implementation that defines the impersonation_sessions table, integrates it into log_as.ts, and updates the optimized verify_mfa() function.
1. Database Migration: Create impersonation_sessions table
We lock down the table entirely so only service_role (and verify_mfa() running under SECURITY DEFINER) can read/write it:
CREATE TABLE public.impersonation_sessions (
session_id uuid PRIMARY KEY,
target_user_id uuid NOT NULL,
admin_user_id uuid NOT NULL,
expires_at timestamp with time zone NOT NULL,
created_at timestamp with time zone DEFAULT now() NOT NULL
);
-- Enable RLS and deny all public access
ALTER TABLE public.impersonation_sessions ENABLE ROW LEVEL SECURITY;
REVOKE ALL ON public.impersonation_sessions FROM PUBLIC;
GRANT SELECT, INSERT, UPDATE, DELETE ON public.impersonation_sessions TO service_role;2. TypeScript: Record Impersonation Session in /log_as
In supabase/functions/_backend/private/log_as.ts, after generating and verifying the target user's OTP, we record the session:
const jwt = authData.session?.access_token
const refreshToken = authData.session?.refresh_token
if (!jwt) {
throw simpleError('no_jwt', 'No jwt', { authData })
}
// Record impersonation session to bypass MFA check in verify_mfa()
try {
const payload = JSON.parse(atob(jwt.split('.')[1]))
const sessionId = payload.session_id
const callerId = c.get('auth')?.userId
if (sessionId && callerId) {
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(), // 1 hour expiration
})
}
} catch (err) {
throw simpleError('record_impersonation_session_error', 'Failed to record impersonation session', { err })
}
return c.json({ jwt, refreshToken })3. SQL: Trust impersonation sessions in optimized verify_mfa()
We update the optimized verify_mfa() to allow either aal2 or a valid impersonation session:
CREATE OR REPLACE FUNCTION public.verify_mfa()
RETURNS boolean
LANGUAGE sql
STABLE
SECURITY DEFINER
SET search_path = ''
AS $$
SELECT
CASE
WHEN EXISTS (
SELECT 1
FROM auth.mfa_factors
WHERE user_id = auth.uid()
AND status = 'verified'
) THEN (
COALESCE(auth.jwt()->>'aal', 'aal1') = 'aal2'
OR EXISTS (
SELECT 1
FROM public.impersonation_sessions s
WHERE s.session_id = (auth.jwt()->>'session_id')::uuid
AND s.expires_at > now()
)
)
ELSE true
END;
$$;3 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