Require AAL2 for platform admin MFA#2103
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe pull request implements AAL2 (multi-factor authentication) authorization for platform admin access. It adds two PostgreSQL functions ( ChangesAAL2 and MFA Authorization for Platform Admin
🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 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 unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
ShravanthReddy
left a comment
ShravanthReddy
left a comment
There was a problem hiding this comment.
I found one test-path issue in the new regression test: the direct insert into auth.mfa_factors should hit the existing trg_enforce_email_otp_for_mfa trigger on a reset test DB. That trigger calls public.is_recent_email_otp_verified(), which only returns true when public.user_security.email_otp_verified_at is within the last hour, and the seed does not create a user_security row for the admin user.\n\nSo this statement is likely to fail before the assertions with email otp verification required for mfa enrollment:\n\nsql\nINSERT INTO auth.mfa_factors (...)\nVALUES (..., 'verified'::auth.factor_status, ...);\n\n\nA minimal fix would be to upsert a recent public.user_security row for legacyAdmin inside the transaction before inserting the MFA factor, similar to the existing delete-user reauth test helper. I also checked migration ordering locally with scripts/check-supabase-migration-order.sh; the timestamp itself is fine.
teixr12
force-pushed
the
bounty/platform-admin-mfa-aal2
branch
from
61a639d to
e30d66e
Compare
|
Fixed the test setup issue by seeding a recent public.user_security.email_otp_verified_at row for the platform admin before inserting the verified MFA factor, so the existing MFA enrollment trigger can pass on reset test DBs. |
![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/migrations/20260510200000_require_aal2_for_platform_admin_mfa.sql`:
- Around line 1-20: Add PR notes documenting the runtime and performance proof
for the PostgreSQL function "public".verify_mfa(): state the execution model (it
runs as SECURITY DEFINER and is invoked per-row by RESTRICTIVE RLS policies on
the authenticated role for the seven tables apikeys, app_versions, apps,
channel_devices, channels, org_users, orgs), provide cardinality expectations
for auth.mfa_factors (typical row counts and worst-case), confirm presence of an
index on auth.mfa_factors(user_id, status) (or add one if missing), and include
EXPLAIN (ANALYZE, BUFFERS) outputs for worst-case queries that exercise
verify_mfa (showing timings and buffer usage) so reviewers can validate
scalability.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5aa70166-3170-44bd-8324-d829ec6e189e
📒 Files selected for processing (2)
supabase/migrations/20260510200000_require_aal2_for_platform_admin_mfa.sqltests/is-admin-functions.test.ts
| CREATE OR REPLACE FUNCTION "public"."verify_mfa"() | ||
| RETURNS boolean | ||
| LANGUAGE "plpgsql" | ||
| SECURITY DEFINER | ||
| SET "search_path" TO '' | ||
| AS $$ | ||
| BEGIN | ||
| RETURN ( | ||
| array[(SELECT coalesce(auth.jwt()->>'aal', 'aal1'))] <@ ( | ||
| SELECT | ||
| CASE | ||
| WHEN count(id) > 0 THEN array['aal2'] | ||
| ELSE array['aal1', 'aal2'] | ||
| END AS aal | ||
| FROM auth.mfa_factors | ||
| WHERE (SELECT auth.uid()) = user_id AND status = 'verified' | ||
| ) | ||
| ); | ||
| END; | ||
| $$; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check indexes on auth.mfa_factors and get execution plan
cat > /tmp/verify_mfa_perf.sql << 'EOF'
-- Show indexes on auth.mfa_factors
SELECT
schemaname,
tablename,
indexname,
indexdef
FROM pg_indexes
WHERE schemaname = 'auth' AND tablename = 'mfa_factors';
-- EXPLAIN the subquery (using a sample UUID)
EXPLAIN (ANALYZE, BUFFERS)
SELECT
CASE
WHEN count(id) > 0 THEN array['aal2']
ELSE array['aal1', 'aal2']
END AS aal
FROM auth.mfa_factors
WHERE user_id = '00000000-0000-0000-0000-000000000000'::uuid
AND status = 'verified';
EOF
# Execute via psql using POSTGRES_URL from test environment
psql "$POSTGRES_URL" -f /tmp/verify_mfa_perf.sqlRepository: Cap-go/capgo
Length of output: 102
🏁 Script executed:
# Find and read the migration file
find . -name "*require_aal2_for_platform_admin_mfa*" -type fRepository: Cap-go/capgo
Length of output: 133
🏁 Script executed:
# Search for invocations of verify_mfa function
rg "verify_mfa" -t sql -t ts -t jsRepository: Cap-go/capgo
Length of output: 6237
🏁 Script executed:
# Check the auth schema in Supabase to understand mfa_factors table structure
find . -name "*.sql" -path "*/migrations/*" | head -20Repository: Cap-go/capgo
Length of output: 1292
🏁 Script executed:
# Look for any existing documentation about this function
rg "verify_mfa" -A 5 -B 5Repository: Cap-go/capgo
Length of output: 21851
🏁 Script executed:
cat -n ./supabase/migrations/20260510200000_require_aal2_for_platform_admin_mfa.sqlRepository: Cap-go/capgo
Length of output: 1027
Document execution model and performance in PR notes per guideline requirement.
Per coding guidelines: "Every PostgreSQL function must be proven to scale before shipping; document execution model (where it runs, how often, which roles, cardinality, and indexes) and provide EXPLAIN (ANALYZE, BUFFERS) output for worst-case scenarios in PR notes."
This function is invoked on every SELECT/INSERT/UPDATE/DELETE operation across 7 RLS-protected tables (apikeys, app_versions, apps, channel_devices, channels, org_users, orgs). PR notes must include:
- Execution model (called per row via RESTRICTIVE RLS policy by authenticated role)
- Cardinality expectations for
auth.mfa_factors - Confirmation that indexes exist on
auth.mfa_factors(user_id, status) - EXPLAIN (ANALYZE, BUFFERS) output for worst-case scenarios
🤖 Prompt for 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.
In `@supabase/migrations/20260510200000_require_aal2_for_platform_admin_mfa.sql`
around lines 1 - 20, Add PR notes documenting the runtime and performance proof
for the PostgreSQL function "public".verify_mfa(): state the execution model (it
runs as SECURITY DEFINER and is invoked per-row by RESTRICTIVE RLS policies on
the authenticated role for the seven tables apikeys, app_versions, apps,
channel_devices, channels, org_users, orgs), provide cardinality expectations
for auth.mfa_factors (typical row counts and worst-case), confirm presence of an
index on auth.mfa_factors(user_id, status) (or add one if missing), and include
EXPLAIN (ANALYZE, BUFFERS) outputs for worst-case queries that exercise
verify_mfa (showing timings and buffer usage) so reviewers can validate
scalability.
There was a problem hiding this comment.
Runtime/performance note for reviewers:
- Execution model:
public.verify_mfa()isSECURITY DEFINERand is already used by existing restrictive RLS policies for authenticated-role access onapikeys,app_versions,apps,channel_devices,channels,org_users, andorgs. - Scope of this PR: it does not add new RLS call sites or a new table lookup. It removes the previous
jsonb_array_elements(auth.jwt()->'amr')OTP fallback branch and keeps the existingauth.mfa_factorsuser/status check as the deciding path. - Cardinality expectation: MFA factors are per-user auth records, normally 0-1 verified TOTP factor for a user and bounded by the number of enrolled factors for the current authenticated user, not by tenant data rows.
- Index evidence: this repo's
supabase/schemas/prod.sqlreferencesauth.mfa_factorsbut does not dump Supabase-managed auth schema indexes, so I cannot truthfully confirm the exact(user_id, status)index from repository evidence alone. - EXPLAIN evidence: I cannot produce
EXPLAIN (ANALYZE, BUFFERS)locally because this fresh environment has no project dependencies,bun,supabase,psql, or livePOSTGRES_URL. I do not want to invent timings. The query to run in a maintainer DB is:
EXPLAIN (ANALYZE, BUFFERS)
SELECT CASE WHEN count(id) > 0 THEN array['aal2'] ELSE array['aal1', 'aal2'] END AS aal
FROM auth.mfa_factors
WHERE user_id = '<sample-user-id>'::uuid
AND status = 'verified';Given the current diff removes JSONB AMR expansion and does not broaden RLS invocation, I expect this to be no worse than the existing function and likely slightly cheaper per call. If maintainers want repository-backed proof in the PR itself, I can add a follow-up once a DB-backed EXPLAIN result is available.
There was a problem hiding this comment.
🧩 Analysis chain
@teixr12, thank you for the thorough runtime/performance note — this is a good-faith response to the guideline requirement, and the key points hold up:
What's confirmed ✅
- Execution model: Correctly identified —
SECURITY DEFINER, called by restrictive RLS policies on the same 7 tables as before. - No new call sites: The diff is a pure logic replacement (AAL claim check instead of
jsonb_array_elementsAMR scan), not an expansion of whereverify_mfa()is invoked. - Cardinality: The
auth.mfa_factorssubquery is bounded per-user. Users can enroll more than 1 TOTP factor, with an upper limit of 10, so the worst-case scan is a handful of rows, not a tenant-data-scale scan. Combined withWHERE user_id = auth.uid() AND status = 'verified', this is inherently a tiny result set. - This PR is likely cheaper than the previous version: Replacing
jsonb_array_elements(auth.jwt()->'amr')with a directauth.jwt()->>'aal'string comparison eliminates JSONB set expansion entirely.
What remains unconfirmed
auth.mfa_factorsindex: Theauthschema is fully Supabase-managed — users cannot create indexes on it (it's a restricted operation per the platform). This means the index must be confirmed from a live project'spg_indexes, not from the repo. A project maintainer should verifySELECT indexname, indexdef FROM pg_indexes WHERE schemaname='auth' AND tablename='mfa_factors';before merge.- EXPLAIN output: The
EXPLAIN (ANALYZE, BUFFERS)you've provided as a template query is the right one for maintainers to run. Given the low cardinality and the narrowingWHERE user_id = auth.uid()predicate, the expected outcome is an index scan returning 0-1 rows — but this should be validated in the actual project DB.
Suggested action for maintainers: Please run the index check and EXPLAIN query above against the project database and paste the output as a comment before merging. The logic change itself is sound.
ʕ •ᴥ•ʔ 🔍
🌐 Web query:
Supabase auth.mfa_factors table indexes user_id status
💡 Result:
In Supabase Auth (GoTrue), the auth.mfa_factors table ships with these indexes/constraints relevant to user_id (and no built-in index on status):
- Primary key:
mfa_factors_pkeyon(id)(viaPRIMARY KEY(id)). [1] - Unique (partial) index (friendly name per user, excluding blank):
create unique index ... on auth.mfa_factors (friendly_name, user_id) where trim(friendly_name) <> ''. [1] - Btree index on
(user_id, created_at):
create index ... factor_id_created_at_idx on auth.mfa_factors (user_id, created_at). [2] - Btree index on
(user_id):
create index ... mfa_factors_user_id_idx on auth.mfa_factors(user_id). [3] - (If Phone MFA is enabled) Unique index on
(user_id, phone):
create unique index ... unique_verified_phone_factor on auth.mfa_factors (user_id, phone). [4]
Although Supabase docs commonly query factors like ... where user_id = ... and status = 'verified', there is no default (user_id, status) (or status) index in the upstream migrations. [5]
Sources:
[1] 20221003041349_add_mfa_schema.up.sql (table + unique partial index) (github.com)
[2] 20221011041400_add_mfa_indexes.up.sql ((user_id, created_at) index) (github.com)
[3] 20230914180801_add_mfa_factors_user_id_idx.up.sql ((user_id) index) (github.com)
[4] 20240729123726_add_mfa_phone_config.up.sql ((user_id, phone) unique index) (github.com)
[5] Supabase MFA docs example querying status = 'verified' (supabase.com)
Citations:
- 1: https://github.com/supabase/auth/blob/master/migrations/20221003041349_add_mfa_schema.up.sql
- 2: https://github.com/supabase/auth/blob/master/migrations/20221011041400_add_mfa_indexes.up.sql
- 3: https://github.com/supabase/auth/blob/master/migrations/20230914180801_add_mfa_factors_user_id_idx.up.sql
- 4: https://github.com/supabase/auth/blob/master/migrations/20240729123726_add_mfa_phone_config.up.sql
- 5: https://supabase.com/docs/guides/auth/auth-mfa?utm_source=openai
@teixr12, the web search confirmed the exact index situation in Supabase Auth upstream — here's what we know:
auth.mfa_factors index inventory (Supabase-managed)
| Index | Columns |
|---|---|
mfa_factors_pkey |
(id) — primary key |
factor_id_created_at_idx |
(user_id, created_at) — btree |
mfa_factors_user_id_idx |
(user_id) — btree |
| (partial unique) | (friendly_name, user_id) where trim(friendly_name) <> '' |
There is no built-in (user_id, status) composite index in the upstream migrations, contrary to what I originally implied might be needed. However, for the query in verify_mfa():
FROM auth.mfa_factors
WHERE user_id = auth.uid() AND status = 'verified'The mfa_factors_user_id_idx btree index on (user_id) will be used to narrow the scan to the current user's rows. The status = 'verified' filter is then a residual predicate applied on that already-tiny result set (0–10 rows at absolute most, typically 0–1). A (user_id, status) composite index would be marginally more efficient but the practical difference is negligible at this cardinality.
Updated assessment:
- ✅ An index on
user_idexists and will be used — the performance concern is adequately addressed. - ✅ No
(user_id, status)index is needed given the row counts involved. ⚠️ AnEXPLAIN (ANALYZE, BUFFERS)from a live project DB is still the cleanest way to close out the guideline requirement before merge — but given confirmed low cardinality and index usage, I'm lowering the severity of this concern. The template query you provided in your note is the right one for a maintainer to run.
The implementation looks sound from a performance perspective. Thank you for the detailed note — this is sufficient context for reviewers.
ʕ •ᴥ•ʔ 🔍
🧠 Learnings used
Learnt from: WcaleNieWolny
Repo: Cap-go/capgo PR: 1300
File: supabase/migrations/20251224103713_2fa_enforcement.sql:85-96
Timestamp: 2025-12-25T11:22:13.039Z
Learning: In SQL migrations under the repository (e.g., supabase/migrations), enforce that when an org has enforcing_2fa=true, all users (including super_admins) must have 2FA enabled before accessing any org functions, including check_org_members_2fa_enabled. Do not grant admin exceptions to 2FA requirements. This ensures consistent security enforcement across all org-related operations; implement this rule within relevant migrations and associated stored procedures/tests.
Learnt from: Dalanir
Repo: Cap-go/capgo PR: 1724
File: supabase/migrations/20260228000000_role_bindings_rls_assignable.sql:6-6
Timestamp: 2026-02-28T16:27:25.073Z
Learning: In SQL migrations (e.g., supabase/migrations/...), enforce that API key identities resolve through the authenticated role. RLS policies used for admin operations like role_bindings must reference only TO "authenticated" (not anon). Use SECURITY DEFINER helpers like get_identity_org_appid() to resolve API key identities internally, and ensure policies restrict access accordingly. This guidance applies to all migration files under any migrations directory that deal with RBAC/RLS policy changes.
Learnt from: Dalanir
Repo: Cap-go/capgo PR: 1821
File: supabase/migrations/20260317160518_sso_skip_org_on_sso_domain.sql:15-52
Timestamp: 2026-03-17T15:56:41.603Z
Learning: In supabase/migrations/**/*.sql for Cap-go/capgo, when auditing function permissions, do not require REVOKE ALL ON FUNCTION ... FROM PUBLIC for PostgreSQL trigger functions (RETURNS trigger). Triggered functions cannot be called directly by roles. Apply REVOKE only to regular (callable) functions, not to trigger functions. This guidance prevents unnecessary revokes in migration scripts related to trigger functions.
Learnt from: riderx
Repo: Cap-go/capgo PR: 2069
File: supabase/migrations/20260507130640_restore_accessible_apps_apikey_rpc.sql:1-4
Timestamp: 2026-05-07T13:58:13.674Z
Learning: In this repo’s Supabase migrations directory (supabase/migrations/*.sql), when you need to restore an RPC/function that was removed by an already-committed migration on main but is still referenced by the committed test suite, fix it via a new forward restoration migration (create a new migration file) instead of editing the already-committed migration. In code review, do not flag this new restoration migration as a duplicate or recommend folding it into an in-progress migration—follow the project rule “never edit previously committed migrations,” and treat the new forward migration as the canonical approach.
Learnt from: riderx
Repo: Cap-go/capgo PR: 2069
File: supabase/migrations/20260507130640_restore_accessible_apps_apikey_rpc.sql:56-64
Timestamp: 2026-05-07T13:58:21.937Z
Learning: In supabase/migrations/**/*.sql, do not flag an `SECURITY DEFINER` RPC function grant of `EXECUTE` to the `anon` role as a security issue when it is explicitly designed for CLI API-key access. This exemption applies when the RPC enforces the API key from request context (e.g., reads the key from the `capgkey`/request header via `public.get_apikey_header()`), rejects/does not return data unless the provided inputs match the header key, and performs an authorization check before returning rows (e.g., `public.rbac_check_permission_direct()`). In this scenario, the CLI calls the function using the `anon` Postgres role but authenticates via the API key header, so the `anon` grant is intentional. Note: the earlier rule about restricting grants to `authenticated` applies to RLS/admin/RBAC-table policy surfaces (e.g., `role_bindings`), not to these CLI-facing API-key–protected RPCs.
|
Thanks, this addresses the test setup issue I flagged. The added |
Merging this PR will not alter performance
Comparing 
Footnotes
|
|
I think there is still a zero-factor platform-admin gap in this version. The new regression test covers an admin who already has a verified TOTP factor, but it does not cover the no-factor platform-admin case. If the intended invariant is "platform admin access requires MFA", this should either require If no-factor platform admins are intentionally grandfathered, I would make that explicit in the PR description and function comment, because the current |
zinc-builds
left a comment
zinc-builds
left a comment
There was a problem hiding this comment.
🔍 Security Review — Hermes Agent
Verdict: ✅ Approved — The fix correctly addresses the MFA bypass and has good test coverage.
What the PR fixes
The original verify_mfa() had a flaw: it returned true for any session containing amr.method = "otp", even aal1 magic-link/email-OTP sessions. A platform admin with a verified TOTP factor could access admin endpoints using only mailbox access — bypassing the MFA requirement entirely.
The fix removes the problematic OR clause and relies exclusively on the Supabase aal claim, which is the authoritative source for authentication assurance level.
✅ Confirmed correct
- The
CASEexpression correctly returnsarray["aal2"]when the user has MFA enrolled, andarray["aal1", "aal2"]for users without MFA — so non-MFA users arent locked out of non-admin functionality search_pathis set to empty string — prevents function injection via schema search path poisoning- Grant statements match the existing convention (anon, authenticated, service_role)
- Test covers both the bypass case (
aal1 + otp → false) and the legitimate case (aal2 + totp → true)
⚠️ Suggestions (non-blocking)
- SQL comment: The migration replaces a non-trivial function with no inline comment explaining why. Consider adding a brief comment noting that
amr.method = "otp"was intentionally removed because it acceptedaal1sessions:-- v2: removed amr.method=otp fallback — email OTP / magic-link -- sessions at aal1 must not satisfy the platform-admin MFA gate.
- Test cleanup: The test inserts into
auth.mfa_factorsinside a transaction but theuser_securityupsert andmfa_factorsinsert dont appear to be rolled back ifexpect()fails mid-test. Consider wrapping with a cleanup block or verifying thatwithTransactionhandles rollback on assertion failures. - Backward compatibility: The migration file creates at timestamp
20260510200000— confirm this is after all other migrations that referenceverify_mfa()to avoid dependency ordering issues in fresh installs.
Nice work catching this — the amr.method vs aal distinction is subtle and requires understanding Supabases JWT claim model.
|
Nice catch on the /claim #1667 |
|
Good catch, thank you. Fixed in 1176f2a by keeping public.verify_mfa() compatible with ordinary non-MFA users, but making the no-argument public.is_platform_admin() wrapper require an aal2 session before checking the admin_users secret. I also added a regression test for the zero-factor platform-admin aal1 case and added an inline SQL comment explaining why amr.method = 'otp' is not accepted for this gate. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
supabase/migrations/20260510200000_require_aal2_for_platform_admin_mfa.sql (1)
11-21: 💤 Low valueZero-factor
aal1pass-through is preserved by design — consider documenting it explicitly.For a caller with zero verified rows in
auth.mfa_factors, the allowed set is['aal1','aal2'], soverify_mfa()still returns true on anaal1session. That is consistent with the existing semantics on non-platform-admin RLS surfaces (which is why the new platform-admin path is gated separately viais_platform_admin()checkingaal = 'aal2'directly). Given a reviewer already raised confusion about the zero-factor case, a brief note in the function comment stating "zero-factor users are intentionally allowed at aal1; platform-admin paths must additionally require aal2 themselves" would close out future questions without changing behavior.📝 Suggested comment tweak
-- Email OTP and magic-link first-factor sessions can carry amr.method = 'otp' -- while remaining aal1, so MFA authorization must use the authoritative aal -- claim instead of accepting OTP method metadata. + -- Note: users with zero verified factors are intentionally allowed at aal1 + -- here; callers that must require step-up (e.g. platform-admin paths) must + -- independently require aal=aal2 instead of relying on this helper.🤖 Prompt for 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. In `@supabase/migrations/20260510200000_require_aal2_for_platform_admin_mfa.sql` around lines 11 - 21, Add an explicit comment in the verify_mfa() function explaining that users with zero verified rows in auth.mfa_factors are intentionally allowed to pass as aal1 (the allowed set becomes ['aal1','aal2'] for zero-factor users), and note that platform-admin checks must separately require aal = 'aal2' (see the is_platform_admin() logic) so this behavior is by design; place the comment near the RETURN expression that uses auth.jwt()->>'aal' and the query against auth.mfa_factors to make the rationale discoverable for future reviewers.
🤖 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/migrations/20260510200000_require_aal2_for_platform_admin_mfa.sql`:
- Around line 48-51: The GRANT for "service_role" on the zero-argument
is_platform_admin() overload should be removed so the user-context overload only
grants "authenticated"; edit the migration to delete the line granting EXECUTE
on function "public"."is_platform_admin"() TO "service_role" and ensure
"service_role" remains granted only on the uuid overload of is_platform_admin
(used for internal lookups).
---
Nitpick comments:
In `@supabase/migrations/20260510200000_require_aal2_for_platform_admin_mfa.sql`:
- Around line 11-21: Add an explicit comment in the verify_mfa() function
explaining that users with zero verified rows in auth.mfa_factors are
intentionally allowed to pass as aal1 (the allowed set becomes ['aal1','aal2']
for zero-factor users), and note that platform-admin checks must separately
require aal = 'aal2' (see the is_platform_admin() logic) so this behavior is by
design; place the comment near the RETURN expression that uses
auth.jwt()->>'aal' and the query against auth.mfa_factors to make the rationale
discoverable for future reviewers.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1b1f3fb3-28b1-43fb-9af0-1cb1c523f286
📒 Files selected for processing (2)
supabase/migrations/20260510200000_require_aal2_for_platform_admin_mfa.sqltests/is-admin-functions.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/is-admin-functions.test.ts
|
Review note: Could we scope this cleanup to factors created by this helper, for example by using a deterministic |
mingisrookie
left a comment
mingisrookie
left a comment
There was a problem hiding this comment.
One touched-scope lint blocker remains in the new TOTP helper. Running:
bun x eslint tests/test-utils.ts tests/is-admin-functions.test.ts tests/admin-credits.test.ts tests/admin-stats.test.ts tests/audit-logs.test.ts
reports these PR-local errors from the added decodeBase32() / generateTotp() code:
tests/test-utils.ts
336:39 error Unexpected use of the global variable 'Buffer'. Use 'require("buffer").Buffer' instead node/prefer-global/buffer
351:10 error Unexpected use of the global variable 'Buffer'. Use 'require("buffer").Buffer' instead node/prefer-global/buffer
357:18 error Unexpected use of the global variable 'Buffer'. Use 'require("buffer").Buffer' instead node/prefer-global/buffer
The same command also reports two pre-existing style/export findings near the top of tests/test-utils.ts, but the Buffer findings are introduced by this PR's helper. A minimal fix should be to import Buffer from node:buffer and use that imported binding in the helper, then rerun the touched-file eslint command before merge.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test-utils.ts (1)
402-407: 💤 Low valueConsider error handling for cleanup query.
The initial cleanup query (lines 402-407) runs before enrollment but doesn't catch/log errors. If this DELETE fails, it won't prevent the test from running, but it could leave stale factors accumulating over time.
Consider wrapping in try/catch with a warning log to aid debugging.
🔧 Optional improvement
+ try { await executeSQL(` DELETE FROM auth.mfa_factors WHERE user_id = $1::uuid AND friendly_name LIKE $2 AND created_at < NOW() - INTERVAL '10 minutes'; `, [signInData.user.id, MFA_TEST_FACTOR_LIKE]) + } + catch (cleanupError) { + console.warn('Failed to clean up old MFA test factors:', cleanupError) + }🤖 Prompt for 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. In `@tests/test-utils.ts` around lines 402 - 407, Wrap the pre-enrollment cleanup DELETE executed via executeSQL(...) in a try/catch so failures are caught and logged; specifically catch around the call to executeSQL that uses signInData.user.id and MFA_TEST_FACTOR_LIKE and on error call the test logger/warn (or console.warn) with a descriptive message including the error and identifying info (user id and MFA_TEST_FACTOR_LIKE) so stale factors are noticed without failing the test.
🤖 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.
Nitpick comments:
In `@tests/test-utils.ts`:
- Around line 402-407: Wrap the pre-enrollment cleanup DELETE executed via
executeSQL(...) in a try/catch so failures are caught and logged; specifically
catch around the call to executeSQL that uses signInData.user.id and
MFA_TEST_FACTOR_LIKE and on error call the test logger/warn (or console.warn)
with a descriptive message including the error and identifying info (user id and
MFA_TEST_FACTOR_LIKE) so stale factors are noticed without failing the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 897fbed6-f534-4417-a36e-de6d2fae08a9
📒 Files selected for processing (7)
supabase/config.tomlsupabase/migrations/20260511001500_require_aal2_for_platform_admin_mfa.sqlsupabase/tests/07_auth_functions.sqltests/admin-credits.test.tstests/admin-stats.test.tstests/audit-logs.test.tstests/test-utils.ts
mingisrookie
left a comment
mingisrookie
left a comment
There was a problem hiding this comment.
Rechecked current head 14f4242: the PR-local node/prefer-global/buffer lint issue I flagged is resolved by importing Buffer from node:buffer and using that binding in the TOTP helper.
Verification:
NODE_OPTIONS=--max-old-space-size=4096 bun x eslint tests/test-utils.ts tests/is-admin-functions.test.ts tests/admin-credits.test.ts tests/admin-stats.test.ts tests/audit-logs.test.tsnow reports only the two pre-existingtests/test-utils.tsfindings near the top of the file (style/indenton the existing multiline env fallback andimport/no-mutable-exportson the existingPOSTGRES_URLexport); the three Buffer errors are gone.git diff --check b11b7be..HEADpasses.
I also attempted the focused bun x vitest run tests/is-admin-functions.test.ts, but this local checkout does not have Supabase Postgres running (ECONNREFUSED 127.0.0.1:54322), so I cannot truthfully report DB-backed test results from here.
|
KCDaemon
left a comment
KCDaemon
left a comment
There was a problem hiding this comment.
Rechecked latest head (14f4242). The platform-admin wrapper now requires the JWT aal claim to be aal2 before it checks the admin secret, while verify_mfa() stays compatible with ordinary zero-factor users.
The earlier shared-test-factor cleanup concern is also addressed: the helper only deletes stale factors with the Capgo test prefix and then removes the exact enrolled factor in finally. GitHub CI is green, and I do not see a remaining blocker.
7 participants
Summary
public.verify_mfa()to rely on the Supabaseaalclaim instead of accepting anyamr.method = "otp"session as MFA.aal1session for a platform admin with a verified TOTP factor.aal2session while making the platform-admin wrapper requireaal2before checking the admin secret.Root Cause
is_platform_admin()delegates toverify_mfa()before allowing platform-admin-only actions such as impersonation, admin credits, admin stats, and replication status.verify_mfa()correctly requiredaal2when the user had a verified MFA factor, but it also returned true when the JWTamrarray containedmethod = "otp". Supabase email OTP / magic-link authentication can produce an OTP first-factor session while still staying ataal1, so mailbox access could satisfy the platform-admin MFA gate without the admin's TOTP factor.Security Impact
For users listed in
admin_users, platform-admin endpoints should require an MFA-verifiedaal2session. This change prevents email OTP / magic-link sessions and zero-factoraal1sessions from being treated as platform-admin MFA.Test plan
git diff --checktests/is-admin-functions.test.tsandsupabase/tests/07_auth_functions.sql.bun,supabase, orpsqlavailable. CI is expected to exercise the added regression tests.Screenshots
N/A. This is a backend SQL authorization change with regression test coverage and no frontend/CLI UI changes.
Checklist
bun run lint:backend && bun run lint.Refs #1667
/claim #1667
Summary by CodeRabbit
New Features
Tests