-
-
Notifications
You must be signed in to change notification settings - Fork 129
fix(db): separate mfa and email otp checks #2454
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
riderx
wants to merge
1
commit into
main
Choose a base branch
from
codex/fix-mfa-otp-aal-bypass
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
64 changes: 64 additions & 0 deletions
64
supabase/migrations/20260608114543_split_mfa_session_and_email_otp_checks.sql
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,64 @@ | ||
| -- Split MFA session assurance from email OTP first-factor checks. | ||
| -- `aal2` is the source of truth for completed MFA. Email OTP can be an `aal1` | ||
| -- login method, so it must not satisfy the MFA gate used by RLS/admin checks. | ||
|
|
||
| CREATE OR REPLACE FUNCTION public.verify_mfa() | ||
| RETURNS boolean | ||
| LANGUAGE sql | ||
| STABLE | ||
| SECURITY DEFINER | ||
| SET search_path = '' | ||
| AS $$ | ||
| SELECT | ||
| 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' | ||
| ); | ||
| $$; | ||
|
|
||
| COMMENT ON FUNCTION public.verify_mfa() IS | ||
| 'Returns true when the current session satisfies Supabase MFA assurance. Users with verified MFA factors require aal2; users without verified factors may use aal1 or aal2.'; | ||
|
|
||
| ALTER FUNCTION public.verify_mfa() OWNER TO postgres; | ||
| REVOKE ALL ON FUNCTION public.verify_mfa() FROM PUBLIC; | ||
| GRANT EXECUTE ON FUNCTION public.verify_mfa() TO anon; | ||
| GRANT EXECUTE ON FUNCTION public.verify_mfa() TO authenticated; | ||
| GRANT EXECUTE ON FUNCTION public.verify_mfa() TO service_role; | ||
|
|
||
| CREATE OR REPLACE FUNCTION public.verify_email_otp_auth() | ||
| RETURNS boolean | ||
| LANGUAGE sql | ||
| STABLE | ||
| SET search_path = '' | ||
| AS $$ | ||
| WITH jwt_claims AS ( | ||
| SELECT auth.jwt() AS claims | ||
| ), | ||
| amr AS ( | ||
| SELECT | ||
| CASE | ||
| WHEN pg_catalog.jsonb_typeof(claims->'amr') = 'array' THEN claims->'amr' | ||
| ELSE '[]'::jsonb | ||
| END AS entries | ||
| FROM jwt_claims | ||
| ) | ||
| SELECT EXISTS ( | ||
| SELECT 1 | ||
| FROM amr, pg_catalog.jsonb_array_elements(amr.entries) AS amr_elem | ||
| WHERE amr_elem->>'method' = 'otp' | ||
| ); | ||
| $$; | ||
|
|
||
| COMMENT ON FUNCTION public.verify_email_otp_auth() IS | ||
| 'Returns true when the current JWT authentication-method reference includes OTP. This is first-factor/email OTP evidence and must not be used as MFA assurance.'; | ||
|
|
||
| ALTER FUNCTION public.verify_email_otp_auth() OWNER TO postgres; | ||
| REVOKE ALL ON FUNCTION public.verify_email_otp_auth() FROM PUBLIC; | ||
| GRANT EXECUTE ON FUNCTION public.verify_email_otp_auth() TO authenticated; | ||
| GRANT EXECUTE ON FUNCTION public.verify_email_otp_auth() TO service_role; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,118 @@ | ||
| BEGIN; | ||
|
|
||
| SELECT plan(6); | ||
|
|
||
| SELECT tests.create_supabase_user( | ||
| 'mfa_session_split_with_mfa', | ||
| 'mfa-session-split-with-mfa@test.local' | ||
| ); | ||
| SELECT tests.create_supabase_user( | ||
| 'mfa_session_split_without_mfa', | ||
| 'mfa-session-split-without-mfa@test.local' | ||
| ); | ||
| SELECT tests.mark_email_otp_verified('mfa_session_split_with_mfa'); | ||
|
|
||
| INSERT INTO auth.mfa_factors ( | ||
| id, | ||
| user_id, | ||
| friendly_name, | ||
| factor_type, | ||
| status, | ||
| created_at, | ||
| updated_at | ||
| ) | ||
| VALUES ( | ||
| gen_random_uuid(), | ||
| tests.get_supabase_uid('mfa_session_split_with_mfa'), | ||
| 'Test TOTP', | ||
| 'totp'::auth.factor_type, | ||
| 'verified'::auth.factor_status, | ||
| NOW(), | ||
| NOW() | ||
| ); | ||
|
|
||
| SELECT tests.authenticate_as('mfa_session_split_without_mfa'); | ||
| SELECT set_config( | ||
| 'request.jwt.claims', | ||
| jsonb_build_object( | ||
| 'sub', tests.get_supabase_uid('mfa_session_split_without_mfa'), | ||
| 'email', 'mfa-session-split-without-mfa@test.local', | ||
| 'aal', 'aal1', | ||
| 'amr', '[]'::jsonb | ||
| )::text, | ||
| true | ||
| ); | ||
| SELECT is( | ||
| public.verify_mfa(), | ||
| true, | ||
| 'verify_mfa allows aal1 when the user has no verified MFA factor' | ||
| ); | ||
| SELECT tests.clear_authentication(); | ||
|
|
||
| SELECT tests.authenticate_as('mfa_session_split_with_mfa'); | ||
| SELECT set_config( | ||
| 'request.jwt.claims', | ||
| jsonb_build_object( | ||
| 'sub', tests.get_supabase_uid('mfa_session_split_with_mfa'), | ||
| 'email', 'mfa-session-split-with-mfa@test.local', | ||
| 'aal', 'aal1', | ||
| 'amr', jsonb_build_array(jsonb_build_object('method', 'password')) | ||
| )::text, | ||
| true | ||
| ); | ||
| SELECT is( | ||
| public.verify_mfa(), | ||
| false, | ||
| 'verify_mfa rejects aal1 when the user has a verified MFA factor' | ||
| ); | ||
| SELECT is( | ||
| public.verify_email_otp_auth(), | ||
| false, | ||
| 'verify_email_otp_auth rejects non-OTP amr methods' | ||
| ); | ||
| SELECT tests.clear_authentication(); | ||
|
|
||
| SELECT tests.authenticate_as('mfa_session_split_with_mfa'); | ||
| SELECT set_config( | ||
| 'request.jwt.claims', | ||
| jsonb_build_object( | ||
| 'sub', tests.get_supabase_uid('mfa_session_split_with_mfa'), | ||
| 'email', 'mfa-session-split-with-mfa@test.local', | ||
| 'aal', 'aal1', | ||
| 'amr', jsonb_build_array(jsonb_build_object('method', 'otp')) | ||
| )::text, | ||
| true | ||
| ); | ||
| SELECT is( | ||
| public.verify_mfa(), | ||
| false, | ||
| 'verify_mfa rejects aal1 OTP first-factor sessions when the user has MFA' | ||
| ); | ||
| SELECT is( | ||
| public.verify_email_otp_auth(), | ||
| true, | ||
| 'verify_email_otp_auth recognizes OTP first-factor sessions separately' | ||
| ); | ||
| SELECT tests.clear_authentication(); | ||
|
|
||
| SELECT tests.authenticate_as('mfa_session_split_with_mfa'); | ||
| SELECT set_config( | ||
| 'request.jwt.claims', | ||
| jsonb_build_object( | ||
| 'sub', tests.get_supabase_uid('mfa_session_split_with_mfa'), | ||
| 'email', 'mfa-session-split-with-mfa@test.local', | ||
| 'aal', 'aal2', | ||
| 'amr', jsonb_build_array(jsonb_build_object('method', 'otp')) | ||
| )::text, | ||
| true | ||
| ); | ||
| SELECT is( | ||
| public.verify_mfa(), | ||
| true, | ||
| 'verify_mfa allows aal2 when the user has a verified MFA factor' | ||
| ); | ||
| SELECT tests.clear_authentication(); | ||
|
|
||
| SELECT * FROM finish(); | ||
|
|
||
| ROLLBACK; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,58 @@ | ||
| import { beforeEach, describe, expect, it, vi } from 'vitest' | ||
|
|
||
| const mocks = vi.hoisted(() => ({ | ||
| rpc: vi.fn(), | ||
| supabaseClient: vi.fn(), | ||
| })) | ||
|
|
||
| vi.mock('../supabase/functions/_backend/utils/supabase.ts', async (importOriginal) => { | ||
| const actual = await importOriginal<typeof import('../supabase/functions/_backend/utils/supabase.ts')>() | ||
| return { | ||
| ...actual, | ||
| supabaseClient: mocks.supabaseClient, | ||
| } | ||
| }) | ||
|
|
||
| const { verifyEmailOtpAuthSession } = await import('../supabase/functions/_backend/private/verify_email_otp.ts') | ||
|
|
||
| describe('verifyEmailOtpAuthSession', () => { | ||
| beforeEach(() => { | ||
| mocks.rpc.mockReset() | ||
| mocks.supabaseClient.mockReset() | ||
| mocks.supabaseClient.mockReturnValue({ rpc: mocks.rpc }) | ||
| }) | ||
|
|
||
| it('checks the OTP access token with verify_email_otp_auth', async () => { | ||
| const context = {} as Parameters<typeof verifyEmailOtpAuthSession>[0] | ||
| mocks.rpc.mockResolvedValue({ data: true, error: null }) | ||
|
|
||
| await expect(verifyEmailOtpAuthSession(context, 'otp-access-token')).resolves.toEqual({ | ||
| verified: true, | ||
| error: null, | ||
| }) | ||
|
|
||
| expect(mocks.supabaseClient).toHaveBeenCalledWith(context, 'Bearer otp-access-token') | ||
| expect(mocks.rpc).toHaveBeenCalledWith('verify_email_otp_auth') | ||
| }) | ||
|
|
||
| it('rejects sessions without OTP authentication evidence', async () => { | ||
| const context = {} as Parameters<typeof verifyEmailOtpAuthSession>[0] | ||
| mocks.rpc.mockResolvedValue({ data: false, error: null }) | ||
|
|
||
| await expect(verifyEmailOtpAuthSession(context, 'password-access-token')).resolves.toEqual({ | ||
| verified: false, | ||
| error: null, | ||
| }) | ||
| }) | ||
|
|
||
| it('surfaces RPC errors as failed verification', async () => { | ||
| const context = {} as Parameters<typeof verifyEmailOtpAuthSession>[0] | ||
| const error = { message: 'rpc failed' } | ||
| mocks.rpc.mockResolvedValue({ data: null, error }) | ||
|
|
||
| await expect(verifyEmailOtpAuthSession(context, 'otp-access-token')).resolves.toEqual({ | ||
| verified: false, | ||
| error, | ||
| }) | ||
| }) | ||
| }) |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.