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. 📝 WalkthroughWalkthroughReplaces client-side OTP verification and separate RPC recording with a single serverless endpoint (/private/verify_email_otp) that verifies the OTP using JWT-authenticated context, records verification via a Postgres RPC, and returns a verified_at timestamp to the client. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant CF as Cloudflare Worker
participant Fn as verify_email_otp (Hono)
participant Auth as Supabase Auth
participant DB as Postgres (record_email_otp_verified)
Client->>CF: POST /private/verify_email_otp { token } + Authorization
CF->>Fn: forward request
Fn->>Auth: verifyOtp(email from JWT, token)
Auth-->>Fn: verification result (owner info)
alt OTP verified and owner matches JWT
Fn->>DB: call record_email_otp_verified()
DB-->>Fn: returns verified_at
Fn-->>Client: 200 { verified_at }
else verification fails or mismatch
Fn-->>Client: 4xx error (invalid_otp / otp_user_mismatch / auth error)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
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.
Pull request overview
This PR implements a security hardening for email OTP verification used in the MFA enrollment flow. The change moves OTP verification from client-side to a new backend endpoint that validates the OTP token and enforces that only OTP-authenticated sessions can record verification timestamps.
Changes:
- Adds a new database function that validates OTP authentication before recording verification timestamps
- Creates a new backend endpoint
/private/verify_email_otpto handle OTP verification securely - Updates the frontend to call the new backend endpoint instead of direct Supabase auth calls
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| supabase/migrations/20260205031305_mfa_email_otp_hardening.sql | Replaces the record_email_otp_verified function to add OTP authentication requirement by checking JWT 'amr' claim for 'otp' method |
| supabase/functions/_backend/private/verify_email_otp.ts | New backend endpoint that validates OTP tokens, verifies user identity, obtains OTP-authenticated session, and calls the database function |
| supabase/functions/private/index.ts | Adds routing for the new verify_email_otp endpoint in Supabase Edge Functions deployment |
| src/pages/settings/account/index.vue | Updates frontend to use the new backend endpoint instead of direct Supabase auth calls |
| app.post('/', middlewareAuth, async (c) => { | ||
| const rawBody = await parseBody<{ token?: string }>(c) | ||
| const token = rawBody.token?.replaceAll(' ', '') ?? '' | ||
|
|
||
| const validation = bodySchema.safeParse({ token }) | ||
| if (!validation.success) { | ||
| throw simpleError('invalid_body', 'Invalid request body', { errors: z.prettifyError(validation.error) }) | ||
| } | ||
|
|
||
| const auth = c.get('auth') | ||
| if (!auth?.userId) { | ||
| return quickError(401, 'not_authenticated', 'Not authenticated') | ||
| } | ||
|
|
||
| const authorization = c.get('authorization') | ||
| if (!authorization) { | ||
| return quickError(401, 'no_authorization', 'No authorization header provided') | ||
| } | ||
|
|
||
| const claims = getClaimsFromJWT(authorization) | ||
| if (!claims?.email) { | ||
| return quickError(400, 'missing_email', 'Email is required to verify OTP') | ||
| } | ||
|
|
||
| const supabase = emptySupabase(c) | ||
| const { data: verifyData, error: verifyError } = await supabase.auth.verifyOtp({ | ||
| email: claims.email, | ||
| token, | ||
| type: 'email', | ||
| }) | ||
|
|
||
| if (verifyError || !verifyData?.session?.access_token) { | ||
| cloudlog({ requestId: c.get('requestId'), context: 'verify_email_otp - verifyOtp failed', error: verifyError?.message }) | ||
| return quickError(401, 'invalid_otp', 'Invalid or expired OTP') | ||
| } | ||
|
|
||
| if (verifyData.user?.id && verifyData.user.id !== auth.userId) { | ||
| return quickError(403, 'otp_user_mismatch', 'OTP does not match current user') | ||
| } | ||
|
|
||
| const otpSupabase = supabaseClient(c, `Bearer ${verifyData.session.access_token}`) | ||
| const { data: verifiedAt, error: recordError } = await otpSupabase | ||
| .rpc('record_email_otp_verified') | ||
|
|
||
| if (recordError || !verifiedAt) { | ||
| cloudlog({ requestId: c.get('requestId'), context: 'verify_email_otp - record failed', error: recordError?.message }) | ||
| return quickError(500, 'record_failed', 'Failed to record OTP verification') | ||
| } | ||
|
|
||
| return c.json({ verified_at: verifiedAt }) | ||
| }) |
There was a problem hiding this comment.
This new security-critical endpoint lacks test coverage. Given the comprehensive test suite in the tests/ directory (e.g., tests/private-error-cases.test.ts, tests/password-policy.test.ts), this endpoint should have tests covering:
- Successful OTP verification flow
- Invalid token format validation
- Missing authentication header
- Email mismatch scenarios
- Invalid/expired OTP codes
- User ID mismatch between token and session
- Integration with the database function
The tests should verify both the Supabase Edge Functions and Cloudflare Workers deployments (using USE_CLOUDFLARE_WORKERS=true).
| import { app as stripe_portal } from '../_backend/private/stripe_portal.ts' | ||
| import { app as upload_link } from '../_backend/private/upload_link.ts' | ||
| import { app as validate_password_compliance } from '../_backend/private/validate_password_compliance.ts' | ||
| import { app as verify_email_otp } from '../_backend/private/verify_email_otp.ts' |
There was a problem hiding this comment.
The new verify_email_otp endpoint needs to be imported and routed in the Cloudflare Workers API file. The endpoint is correctly added to the Supabase private/index.ts, but it's missing from cloudflare_workers/api/index.ts.
You need to:
- Add the import at the top of cloudflare_workers/api/index.ts
- Add the route to the appPrivate router (around line 86)
This is critical because according to the project architecture, 99% of production traffic runs on Cloudflare Workers (ports 8787/8788/8789 locally), and the backend code must be deployed to all platforms.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@supabase/functions/_backend/private/verify_email_otp.ts`:
- Around line 56-59: The CI failure is due to the new RPC
"record_email_otp_verified" not being present in the generated Supabase types,
causing the call otpSupabase.rpc('record_email_otp_verified') to fail
type-checking; regenerate or update ~/types/supabase.types (and any backend type
files that import it) so Database['public']['Functions'] includes
"record_email_otp_verified", then re-run type generation and ensure the function
name is recognized where supabaseClient(...) and otpSupabase.rpc(...) are used.
- Around line 1-13: The file initializes the app with new Hono(), which bypasses
shared backend setup; replace that with the shared factory by importing and
using createHono instead. Update the top imports to include createHono from
'../utils/hono.ts' (alongside existing imports like middlewareAuth, parseBody)
and change the exported app instantiation from new
Hono<MiddlewareKeyVariables>() to createHono<MiddlewareKeyVariables>() so this
endpoint uses the common middleware/error handling.
- Around line 25-33: The code currently trusts c.get('auth')?.userId without
verifying authType; update the guard in verify_email_otp handler to check
c.get('auth')?.authType === 'jwt' (in addition to presence of auth.userId) and
return quickError(401, 'not_authenticated', 'Not authenticated') for non-jwt
auth (e.g., apikey or missing authType), so only JWT-authenticated requests
proceed; apply this check where auth is read (the const auth = c.get('auth')
block) and adjust the subsequent authorization flow accordingly.
| const otpSupabase = supabaseClient(c, `Bearer ${verifyData.session.access_token}`) | ||
| const { data: verifiedAt, error: recordError } = await otpSupabase | ||
| .rpc('record_email_otp_verified') | ||
|
|
There was a problem hiding this comment.
CI failure: record_email_otp_verified missing from generated RPC types.
The pipeline error indicates the RPC name isn’t in the Database['public']['Functions'] union, so rpc() fails type-checking. Regenerate/update ~/types/supabase.types (and any backend types that import it) to include the new function.
🧰 Tools
🪛 GitHub Actions: Run tests
[error] 58-58: Command failed: vue-tsc --noEmit. TS2345: Argument of type '"record_email_otp_verified"' is not assignable to parameter of type '"accept_invitation_to_org" | "apply_usage_overage" | "calculate_credit_cost" | "check_min_rights" | "check_min_rights_legacy" | "check_org_encrypted_bundle_enforcement" | ... 256 more ... | "verify_mfa"'.
🤖 Prompt for AI Agents
In `@supabase/functions/_backend/private/verify_email_otp.ts` around lines 56 -
59, The CI failure is due to the new RPC "record_email_otp_verified" not being
present in the generated Supabase types, causing the call
otpSupabase.rpc('record_email_otp_verified') to fail type-checking; regenerate
or update ~/types/supabase.types (and any backend type files that import it) so
Database['public']['Functions'] includes "record_email_otp_verified", then
re-run type generation and ensure the function name is recognized where
supabaseClient(...) and otpSupabase.rpc(...) are used.
fd7819b to
6c678b0
Compare
2f34287 to
cfce68d
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@tests/verify-email-otp.test.ts`:
- Around line 4-6: Replace the generic OTHER_USER_EMAIL constant with a
file-prefixed unique address to avoid cross-test collisions; update the
OTHER_USER_EMAIL declaration in tests/verify-email-otp.test.ts (near
OTP_ENDPOINT and OTHER_USER_EMAIL) to use a name that includes the test file
identifier, e.g., "verify_email_otp_test_user@capgo.app", and ensure any tests
referencing OTHER_USER_EMAIL continue to use that new value.
- Around line 1-3: Change the non-concurrent test to run in parallel by
replacing the use of it() with it.concurrent() for the test in this file; also
make the shared test user email unique to this file by updating the
OTHER_USER_EMAIL constant value to include the test file prefix (e.g.,
"test_verify_email_otp_<original_local>@capgo.app") so the mutations to
user_security for OTHER_USER_EMAIL won't collide across files; locate and update
the OTHER_USER_EMAIL constant and the test declaration (it -> it.concurrent) in
tests/verify-email-otp.test.ts to apply these fixes.
- Around line 18-49: The test mutates shared user_security state and is not
concurrent-safe; create a dedicated test user (constants like
USER_EMAIL_VERIFY_OTP and USER_ID_VERIFY_OTP) in test-utils.ts mirroring the
existing USER_ID_EMAIL_PREFS/USER_ID_STATS patterns, add corresponding seed rows
in supabase/seed.sql (including a user_security row for that user), then update
the test in verify-email-otp.test.ts to reference
USER_EMAIL_VERIFY_OTP/USER_ID_VERIFY_OTP (instead of USER_EMAIL/USER_ID), call
generateEmailOtp(USER_EMAIL_VERIFY_OTP) and getAuthHeaders() for that user, and
change the spec to it.concurrent('verifies OTP and records verification
timestamp', async () => ...) so it can run in parallel safely while still
asserting the DB email_otp_verified_at via getSupabaseClient().
| import { beforeAll, describe, expect, it } from 'vitest' | ||
| import { getAuthHeaders, getEndpointUrl, getSupabaseClient, USER_EMAIL, USER_ID } from './test-utils.ts' | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for USE_CLOUDFLARE_WORKERS configuration in the repository
rg -n 'USE_CLOUDFLARE_WORKERS' -S --type ts --type js --type jsonRepository: Cap-go/capgo
Length of output: 668
🏁 Script executed:
# Find and view the test file
fd 'verify-email-otp.test.ts' --type fRepository: Cap-go/capgo
Length of output: 86
🏁 Script executed:
# Check vitest configuration
fd -e 'vitest.config' -e 'vite.config' -e 'vitest.setup' --type fRepository: Cap-go/capgo
Length of output: 38
🏁 Script executed:
# View test-utils.ts to understand helper setup
fd 'test-utils.ts' --type f --exec cat -n {} \;Repository: Cap-go/capgo
Length of output: 27095
🏁 Script executed:
cat -n tests/verify-email-otp.test.tsRepository: Cap-go/capgo
Length of output: 4120
Use it.concurrent() and file-prefixed unique test data for shared resources.
The USE_CLOUDFLARE_WORKERS flag is already configured in vitest.config.cloudflare.ts and cloudflare-plugin.ts, so your getEndpointUrl() calls will route correctly to Cloudflare Workers.
However, the test has two design issues:
- Line 25 uses
it()instead ofit.concurrent(). Per guidelines, all tests should useit.concurrent()to run in parallel within the same file. - Line 5's
OTHER_USER_EMAIL = 'test2@capgo.app'should be prefixed with the test file name (e.g.,'test_verify_email_otp_test2@capgo.app'). The first test (lines 25–49) mutates theuser_securitytable for this user; without unique file-prefixed naming, parallel test execution risks interference with other test files using the same email.
🤖 Prompt for AI Agents
In `@tests/verify-email-otp.test.ts` around lines 1 - 3, Change the non-concurrent
test to run in parallel by replacing the use of it() with it.concurrent() for
the test in this file; also make the shared test user email unique to this file
by updating the OTHER_USER_EMAIL constant value to include the test file prefix
(e.g., "test_verify_email_otp_<original_local>@capgo.app") so the mutations to
user_security for OTHER_USER_EMAIL won't collide across files; locate and update
the OTHER_USER_EMAIL constant and the test declaration (it -> it.concurrent) in
tests/verify-email-otp.test.ts to apply these fixes.
| const OTP_ENDPOINT = '/private/verify_email_otp' | ||
| const OTHER_USER_EMAIL = 'test2@capgo.app' | ||
|
|
There was a problem hiding this comment.
Use a file-prefixed unique email for test users.
OTHER_USER_EMAIL = 'test2@capgo.app' is generic and can collide across tests. Please switch to a unique, file-prefixed address.
🔧 Suggested update
-const OTHER_USER_EMAIL = 'test2@capgo.app'
+const OTHER_USER_EMAIL = 'verify_email_otp_test2@capgo.app'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const OTP_ENDPOINT = '/private/verify_email_otp' | |
| const OTHER_USER_EMAIL = 'test2@capgo.app' | |
| const OTP_ENDPOINT = '/private/verify_email_otp' | |
| const OTHER_USER_EMAIL = 'verify_email_otp_test2@capgo.app' | |
🤖 Prompt for AI Agents
In `@tests/verify-email-otp.test.ts` around lines 4 - 6, Replace the generic
OTHER_USER_EMAIL constant with a file-prefixed unique address to avoid
cross-test collisions; update the OTHER_USER_EMAIL declaration in
tests/verify-email-otp.test.ts (near OTP_ENDPOINT and OTHER_USER_EMAIL) to use a
name that includes the test file identifier, e.g.,
"verify_email_otp_test_user@capgo.app", and ensure any tests referencing
OTHER_USER_EMAIL continue to use that new value.
| describe('[POST] /private/verify_email_otp', () => { | ||
| let authHeaders: Record<string, string> | ||
|
|
||
| beforeAll(async () => { | ||
| authHeaders = await getAuthHeaders() | ||
| }) | ||
|
|
||
| it('verifies OTP and records verification timestamp', async () => { | ||
| const otp = await generateEmailOtp(USER_EMAIL) | ||
|
|
||
| const response = await fetch(getEndpointUrl(OTP_ENDPOINT), { | ||
| method: 'POST', | ||
| headers: authHeaders, | ||
| body: JSON.stringify({ token: otp, type: 'magiclink' }), | ||
| }) | ||
|
|
||
| expect(response.status).toBe(200) | ||
| const data = await response.json() as { verified_at?: string } | ||
| expect(data.verified_at).toBeTruthy() | ||
|
|
||
| const { data: securityRow, error } = await getSupabaseClient() | ||
| .from('user_security') | ||
| .select('email_otp_verified_at') | ||
| .eq('user_id', USER_ID) | ||
| .single() | ||
|
|
||
| // Use the email match to locate the user record (auth headers already tied to USER_EMAIL) | ||
| if (error) { | ||
| throw error | ||
| } | ||
| expect(securityRow?.email_otp_verified_at).toBeTruthy() | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n tests/verify-email-otp.test.tsRepository: Cap-go/capgo
Length of output: 4120
🏁 Script executed:
cat -n tests/test-utils.ts | head -100Repository: Cap-go/capgo
Length of output: 7322
🏁 Script executed:
cat -n tests/test-utils.ts | grep -A 5 -B 5 "USER_EMAIL"Repository: Cap-go/capgo
Length of output: 2708
🏁 Script executed:
cat -n tests/test-utils.ts | sed -n '130,160p'Repository: Cap-go/capgo
Length of output: 1128
🏁 Script executed:
rg "USER_EMAIL|USER_ID" tests/ -t ts --max-count=50 | head -60Repository: Cap-go/capgo
Length of output: 5170
🏁 Script executed:
rg "verify.*email|email.*otp" tests/ -t ts -iRepository: Cap-go/capgo
Length of output: 1210
🏁 Script executed:
rg "generateEmailOtp|email_otp|verify_email_otp" tests/ -t tsRepository: Cap-go/capgo
Length of output: 791
🏁 Script executed:
head -200 supabase/seed.sql | grep -A 5 -B 5 "user_security\|email_otp"Repository: Cap-go/capgo
Length of output: 38
🏁 Script executed:
find . -name "seed.sql" -o -name "*.sql" | grep -E "seed|init" | head -10Repository: Cap-go/capgo
Length of output: 194
🏁 Script executed:
cat supabase/seed.sql | grep -A 10 -B 5 "email_otp\|test@capgo.app" | head -80Repository: Cap-go/capgo
Length of output: 10860
🏁 Script executed:
grep -n "user_security\|email_otp_verified" supabase/seed.sqlRepository: Cap-go/capgo
Length of output: 38
🏁 Script executed:
grep -n "CREATE TABLE.*user_security\|INSERT INTO.*user_security" supabase/migrations/*.sql | head -20Repository: Cap-go/capgo
Length of output: 487
🏁 Script executed:
grep -A 20 "CREATE TABLE.*user_security" supabase/migrations/20260204103000_mfa_email_otp_guard.sqlRepository: Cap-go/capgo
Length of output: 891
Switch to concurrent execution and isolate user seed data.
This test mutates user_security for shared USER_EMAIL/USER_ID and uses it() instead of it.concurrent(). Create dedicated user constants in test-utils.ts (following the pattern of USER_ID_EMAIL_PREFS, USER_ID_STATS, etc.) and seed them in supabase/seed.sql, then update the test to use it.concurrent().
♻️ Suggested change (after isolation)
- it('verifies OTP and records verification timestamp', async () => {
+ it.concurrent('verifies OTP and records verification timestamp', async () => {🤖 Prompt for AI Agents
In `@tests/verify-email-otp.test.ts` around lines 18 - 49, The test mutates shared
user_security state and is not concurrent-safe; create a dedicated test user
(constants like USER_EMAIL_VERIFY_OTP and USER_ID_VERIFY_OTP) in test-utils.ts
mirroring the existing USER_ID_EMAIL_PREFS/USER_ID_STATS patterns, add
corresponding seed rows in supabase/seed.sql (including a user_security row for
that user), then update the test in verify-email-otp.test.ts to reference
USER_EMAIL_VERIFY_OTP/USER_ID_VERIFY_OTP (instead of USER_EMAIL/USER_ID), call
generateEmailOtp(USER_EMAIL_VERIFY_OTP) and getAuthHeaders() for that user, and
change the spec to it.concurrent('verifies OTP and records verification
timestamp', async () => ...) so it can run in parallel safely while still
asserting the DB email_otp_verified_at via getSupabaseClient().
|
|
/tip $250 @nancyhunter191 |
|
🎉🎈 @nancyhunter191 has been awarded $250 by Capgo! 🎈🎊 |
Summary (AI generated)\n\n- Route email OTP verification through a private function and return the verified timestamp\n\n## Motivation (AI generated)\n\n- Require an OTP-authenticated session before recording verification\n\n## Business Impact (AI generated)\n\n- Reduces risk of spoofed OTP verification records\n\n## Test plan (AI generated)\n\n- [x] bun run lint:backend\n- [x] bun run lint\n\n## Screenshots (AI generated)\n\n- N/A\n\n## Checklist (AI generated)\n\n- [x] My code follows the code style of this project and passes\n .\n- [ ] My change requires a change to the documentation.\n- [ ] I have updated the documentation\n accordingly.\n- [ ] My change has adequate E2E test coverage.\n- [ ] I have tested my code manually, and I have provided steps how to reproduce\n my tests\n\nGenerated with AI\n
Summary by CodeRabbit
New Features
Bug Fixes
Tests