MFA hardening: QR fix, callback session, admin reset, Codex security fixes#261
Merged
Systemsaholic merged 7 commits intomainfrom Apr 30, 2026
Merged
MFA hardening: QR fix, callback session, admin reset, Codex security fixes#261Systemsaholic merged 7 commits intomainfrom
Systemsaholic merged 7 commits intomainfrom
Conversation
The Supabase REST API returns raw SVG XML for qr_code, while the SDK returns a data: URI. The <img> tag needs a data URI to render. Normalize the response by wrapping raw SVG in a data:image/svg+xml;utf8 URI. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The callback hash handler was using vanilla @supabase/supabase-js which stores sessions in localStorage. The middleware reads from cookies via @supabase/ssr, so aal2 tokens from MFA verify never reached middleware — causing an infinite redirect loop back to mfa-verify. Switch to createBrowserClient from @supabase/ssr so setSession() writes the upgraded aal2 session to cookies where middleware can read it. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Admin users table: - "Reset MFA" in dropdown for active users (admin only) - Calls POST /users/:id/reset-mfa to unenroll all TOTP factors - Confirmation prompt before reset Profile Security tab: - "Change Device" button — unenrolls current factor and immediately starts new enrollment (QR code for new authenticator app) - Sits alongside existing "Remove 2FA" button Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The unenroll function called mfaRestCall (GET) before the DELETE, which returned 405 and short-circuited the catch block. Removed the unnecessary GET — just DELETE directly. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Set enrolling=true BEFORE unenroll so the UI shows a loading state instead of flashing the 'Enable 2FA' button while the old factor is being deleted and new one enrolled. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1. API MfaGuard now rejects aal1 sessions with 403 when MFA_REQUIRED=true (was returning true for all — no-op enforcement) 2. Grace period cookie is now httpOnly via /api/mfa-skip server route (was client-set document.cookie — forgeable) 3. Profile security tab persists aal2 session through /auth/callback after MFA verify (was treating result as boolean, session stayed aal1) 4. Removed pre-MFA recordLogin from SIGNED_IN event in auth-provider (recordLogin now only fires from MFA pages post-verification) 5. Sanitized redirectTo to internal paths only — prevents open redirect via crafted login URL (login-form, mfa-enroll, mfa-verify) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ 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 (1)
📝 WalkthroughWalkthroughThis PR implements server-side MFA skip functionality, enhances MFA verification with proper session establishment, adds redirect URL sanitization across auth flows, introduces admin-level MFA reset capability, and enforces strict MFA checks via API guards. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Poem
🚥 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)
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. Review rate limit: 0/1 reviews remaining, refill in 43 minutes and 48 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/admin/src/app/auth/callback/route.ts (1)
16-17:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winSanitize and JS-escape
nextbefore embedding it in inline script.
nextis query-controlled and currently interpolated directly into JavaScript (const next = '${next}'), which can be exploited as reflected XSS.Proposed fix
export async function GET(request: Request) { const { searchParams, origin } = new URL(request.url) const supabase = await createClient() - const next = searchParams.get('next') ?? '/trips' + const rawNext = searchParams.get('next') ?? '/trips' + const next = rawNext.startsWith('/') && !rawNext.startsWith('//') ? rawNext : '/trips' @@ function getHashHandlerHtml(origin: string, next: string): string { const supabaseUrl = (process.env.NEXT_PUBLIC_SUPABASE_URL || '').trim() const supabaseAnonKey = (process.env.NEXT_PUBLIC_SUPABASE_ANON_KEY || '').trim() return `<!DOCTYPE html> @@ <script type="module"> import { createBrowserClient } from 'https://esm.sh/@supabase/ssr@0.10.2' - const origin = '${origin}' - const next = '${next}' - const supabase = createBrowserClient('${supabaseUrl}', '${supabaseAnonKey}') + const origin = ${JSON.stringify(origin)} + const next = ${JSON.stringify(next)} + const supabase = createBrowserClient(${JSON.stringify(supabaseUrl)}, ${JSON.stringify(supabaseAnonKey)})Also applies to: 136-138
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin/src/app/auth/callback/route.ts` around lines 16 - 17, The query-controlled variable next is being interpolated directly into inline JS which enables reflected XSS; update the code to JS-escape/sanitize next before embedding by serializing it (e.g., use JSON.stringify(next) or an equivalent JS-escaping helper) when writing it into the inline script and validate it against an allowlist/pattern if possible; change usages around the const next = '${next}' interpolation in route.ts (and the second occurrence at lines 136-138) to embed the serialized/escaped value (and optionally normalize to a safe relative path) so the injected string cannot break out of the JS context.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/admin/src/app/api/mfa-skip/route.ts`:
- Around line 15-19: The code treats supabase.auth.getUser() failures as a
generic 401 and drops the underlying error; update the handler in route.ts to
check the returned error from supabase.auth.getUser() (inspect the returned {
data, error } or the same destructured result) and if an error exists, capture
it with Sentry.captureException including a tag or context set to
process.env.NEXT_PUBLIC_SENTRY_ENVIRONMENT, then return the appropriate
NextResponse.json({ error: 'Unauthorized' }, { status: 401 }) only after
logging; ensure you reference supabase.auth.getUser(), Sentry.captureException,
and NEXT_PUBLIC_SENTRY_ENVIRONMENT so the runtime error is reported to Sentry
with the environment tag.
In `@apps/admin/src/app/auth/mfa-enroll/page.tsx`:
- Around line 65-73: The current handleSkip implementation updates localStorage,
calls recordLogin(), and navigates regardless of whether the POST to
/api/mfa-skip succeeded; change handleSkip to await the fetch response, check
response.ok, and only on success perform localStorage.setItem(MFA_GRACE_KEY,
...), call recordLogin(), and window.location.assign(redirectTo); on failure do
not modify local state or redirect (instead return or surface an error to the
user). Ensure you reference the existing symbols handleSkip, MFA_GRACE_KEY,
/api/mfa-skip, recordLogin, and redirectTo when making the conditional change.
In `@apps/admin/src/app/auth/mfa-verify/page.tsx`:
- Around line 24-25: The current redirect sanitization using
rawRedirect/redirectTo allows payloads with quotes (e.g. "/';alert(1);//") which
can break out of a JS string in the callback; update the logic to reject any
redirectTo values containing quotes or other quote-bearing characters and
restrict to a strict internal-path allowlist (e.g. only ^/[-A-Za-z0-9_/]*$) by
extracting this check into a shared helper (e.g. isValidInternalRedirect or
sanitizeRedirect) and use it in both MFA pages (mfa-verify/page.tsx and the
callback route) — alternatively, if you must embed next into inline script in
callback route, escape/encode the value there before interpolation (escapeJS or
JSON.stringify) so arbitrary JS cannot execute.
In `@apps/admin/src/app/profile/_components/security-tab.tsx`:
- Around line 333-355: The UI flashes the not-enrolled card because
handleChangeDevice sets enrolling=true before enroll() returns, but the render
only treats the enrollment flow as enrolling && qrCode; update the render logic
in this component to explicitly handle the intermediate state enrolling &&
!qrCode (show a loading/progress state) or change the fallback condition to
require !enrolling before rendering the default setup card; locate and modify
the render branch that checks enrolling and qrCode, and use the enrolling,
qrCode, enroll, unenroll, handleChangeDevice and factors symbols to implement
the loading branch so the component does not fall through while enroll() is
pending.
In `@apps/admin/src/app/settings/users/page.tsx`:
- Around line 135-137: The catch block that currently swallows errors when
resetting MFA should capture the exception in Sentry before showing the toast:
modify the catch in the reset handler (the try/catch surrounding the MFA reset
call in apps/admin/src/app/settings/users/page.tsx) to accept the error
parameter, call Sentry.captureException(error) (or Sentry.captureException(err)
depending on import), then proceed to call toast({ title: 'Failed to reset MFA',
variant: 'destructive' }); ensure Sentry is imported/available in that module
and include any relevant context (user id/email) if available from surrounding
variables.
In `@apps/admin/src/hooks/use-mfa.ts`:
- Around line 123-126: The code in use-mfa.ts assumes result.totp.qr_code is a
string when computing rawQr.startsWith, which can throw for
null/undefined/non-string values; update the qr handling around rawQr/qrCode to
first guard the value (e.g., check typeof rawQr === 'string' && rawQr.length >
0) and only call startsWith when it's a string, otherwise provide a safe
fallback (empty data URL or null) and ensure qrCode is produced
deterministically; reference the rawQr variable and qrCode assignment in the
hook to locate and update the logic.
---
Outside diff comments:
In `@apps/admin/src/app/auth/callback/route.ts`:
- Around line 16-17: The query-controlled variable next is being interpolated
directly into inline JS which enables reflected XSS; update the code to
JS-escape/sanitize next before embedding by serializing it (e.g., use
JSON.stringify(next) or an equivalent JS-escaping helper) when writing it into
the inline script and validate it against an allowlist/pattern if possible;
change usages around the const next = '${next}' interpolation in route.ts (and
the second occurrence at lines 136-138) to embed the serialized/escaped value
(and optionally normalize to a safe relative path) so the injected string cannot
break out of the JS context.
🪄 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: d9a208a6-029b-4b07-b971-5f98dbe22dfb
📒 Files selected for processing (12)
apps/admin/src/app/api/mfa-skip/route.tsapps/admin/src/app/auth/callback/route.tsapps/admin/src/app/auth/mfa-enroll/page.tsxapps/admin/src/app/auth/mfa-verify/page.tsxapps/admin/src/app/profile/_components/security-tab.tsxapps/admin/src/app/settings/users/_components/users-table.tsxapps/admin/src/app/settings/users/page.tsxapps/admin/src/components/auth/login-form.tsxapps/admin/src/hooks/use-mfa.tsapps/admin/src/hooks/use-users.tsapps/admin/src/providers/auth-provider.tsxapps/api/src/auth/guards/mfa.guard.ts
Comment on lines
+15
to
+19
| const { data: { user } } = await supabase.auth.getUser() | ||
|
|
||
| if (!user) { | ||
| return NextResponse.json({ error: 'Unauthorized' }, { status: 401 }) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the file exists and examine the relevant lines
cat -n apps/admin/src/app/api/mfa-skip/route.tsRepository: Systemsaholic/tailfire
Length of output: 1089
🏁 Script executed:
# Check for Sentry configuration in admin app
find apps/admin -type f \( -name "*.ts" -o -name "*.tsx" \) | xargs grep -l "Sentry\|sentry" | head -20Repository: Systemsaholic/tailfire
Length of output: 294
🏁 Script executed:
# Check docs/MONITORING.md for Sentry guidance
if [ -f docs/MONITORING.md ]; then cat -n docs/MONITORING.md; else echo "File not found"; fiRepository: Systemsaholic/tailfire
Length of output: 10396
🏁 Script executed:
# Search for existing Sentry usage patterns with getUser in admin app
rg "@sentry|Sentry" apps/admin/src --type ts --type tsx -A 3 | head -50Repository: Systemsaholic/tailfire
Length of output: 94
🏁 Script executed:
# Check environment variables referenced in admin app
rg "NEXT_PUBLIC_SENTRY_ENVIRONMENT" apps/admin --type ts --type tsxRepository: Systemsaholic/tailfire
Length of output: 94
🏁 Script executed:
# Search for getUser usage patterns in admin app
rg "getUser\(\)" apps/admin/src -A 5 | head -80Repository: Systemsaholic/tailfire
Length of output: 1922
🏁 Script executed:
# Search for Sentry.captureException patterns in route handlers
rg "captureException" apps/admin/src -B 2 -A 3 | head -60Repository: Systemsaholic/tailfire
Length of output: 478
🏁 Script executed:
# Check how other route handlers import and use Sentry
find apps/admin/src/app/api -name "*.ts" -type f -exec cat {} \; | head -100Repository: Systemsaholic/tailfire
Length of output: 865
🏁 Script executed:
# Verify Supabase auth types - check if getUser returns error
rg "supabase.auth.getUser" apps -A 3 -B 1 | head -100Repository: Systemsaholic/tailfire
Length of output: 3837
🏁 Script executed:
# Search for error handling patterns with Supabase auth methods in admin app
rg "supabase.auth" apps/admin/src -B 2 -A 8 | grep -A 8 "error"Repository: Systemsaholic/tailfire
Length of output: 9739
🏁 Script executed:
# Check if there are any route handlers in admin with Sentry imports
find apps/admin/src/app/api -name "*.ts" -type fRepository: Systemsaholic/tailfire
Length of output: 107
🏁 Script executed:
# Search for complete getUser patterns including error handling if any exist
rg "getUser\(\)" apps --context=10 | grep -A 10 "error"Repository: Systemsaholic/tailfire
Length of output: 1538
🏁 Script executed:
# Check middleware pattern more carefully - look at the complete context
cat -n apps/admin/src/lib/supabase/middleware.ts | head -50Repository: Systemsaholic/tailfire
Length of output: 1860
Capture getUser() errors and report to Sentry.
supabase.auth.getUser() returns an error object on network failures or invalid sessions. The current code masks these as 401 Unauthorized, losing error telemetry required by the admin app monitoring guidelines.
Proposed fix
+import * as Sentry from '@sentry/nextjs'
import { createClient } from '@/lib/supabase/server'
import { NextResponse } from 'next/server'
export async function POST() {
const supabase = await createClient()
- const { data: { user } } = await supabase.auth.getUser()
+ const { data: { user }, error } = await supabase.auth.getUser()
+
+ if (error) {
+ Sentry.captureException(error, {
+ tags: {
+ environment: process.env.NEXT_PUBLIC_SENTRY_ENVIRONMENT || 'development',
+ },
+ })
+ return NextResponse.json({ error: 'Failed to verify session' }, { status: 500 })
+ }
if (!user) {
return NextResponse.json({ error: 'Unauthorized' }, { status: 401 })
}Per coding guidelines: "Admin application must capture runtime errors with Sentry. Errors must be tagged with NEXT_PUBLIC_SENTRY_ENVIRONMENT."
📝 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.
Suggested change
| const { data: { user } } = await supabase.auth.getUser() | |
| if (!user) { | |
| return NextResponse.json({ error: 'Unauthorized' }, { status: 401 }) | |
| } | |
| import * as Sentry from '@sentry/nextjs' | |
| import { createClient } from '@/lib/supabase/server' | |
| import { NextResponse } from 'next/server' | |
| export async function POST() { | |
| const supabase = await createClient() | |
| const { data: { user }, error } = await supabase.auth.getUser() | |
| if (error) { | |
| Sentry.captureException(error, { | |
| tags: { | |
| environment: process.env.NEXT_PUBLIC_SENTRY_ENVIRONMENT || 'development', | |
| }, | |
| }) | |
| return NextResponse.json({ error: 'Failed to verify session' }, { status: 500 }) | |
| } | |
| if (!user) { | |
| return NextResponse.json({ error: 'Unauthorized' }, { status: 401 }) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/admin/src/app/api/mfa-skip/route.ts` around lines 15 - 19, The code
treats supabase.auth.getUser() failures as a generic 401 and drops the
underlying error; update the handler in route.ts to check the returned error
from supabase.auth.getUser() (inspect the returned { data, error } or the same
destructured result) and if an error exists, capture it with
Sentry.captureException including a tag or context set to
process.env.NEXT_PUBLIC_SENTRY_ENVIRONMENT, then return the appropriate
NextResponse.json({ error: 'Unauthorized' }, { status: 401 }) only after
logging; ensure you reference supabase.auth.getUser(), Sentry.captureException,
and NEXT_PUBLIC_SENTRY_ENVIRONMENT so the runtime error is reported to Sentry
with the environment tag.
Comment on lines
+65
to
73
| const handleSkip = async () => { | ||
| // Set httpOnly grace cookie via server route (unforgeable by client) | ||
| await fetch('/api/mfa-skip', { method: 'POST' }) | ||
| // Track locally for UI display only | ||
| if (!localStorage.getItem(MFA_GRACE_KEY)) { | ||
| localStorage.setItem(MFA_GRACE_KEY, now) | ||
| localStorage.setItem(MFA_GRACE_KEY, String(Date.now())) | ||
| } | ||
| // Set cookie for middleware to read (httpOnly=false so JS can set it) | ||
| document.cookie = `mfa_skipped_at=${now}; path=/; max-age=${MFA_GRACE_DAYS * 86400}; SameSite=Lax` | ||
| recordLogin() | ||
| window.location.assign(redirectTo) |
There was a problem hiding this comment.
Only continue after /api/mfa-skip actually succeeds.
handleSkip() always updates local state, records the login, and redirects even when the POST fails or returns non-2xx. That leaves the UI believing MFA was skipped while the server never issued the httpOnly grace cookie, so the next protected request sends the user straight back into enrollment. Please gate the redirect on response.ok and keep the user on this page on failure.
Suggested fix
const handleSkip = async () => {
- // Set httpOnly grace cookie via server route (unforgeable by client)
- await fetch('/api/mfa-skip', { method: 'POST' })
- // Track locally for UI display only
- if (!localStorage.getItem(MFA_GRACE_KEY)) {
- localStorage.setItem(MFA_GRACE_KEY, String(Date.now()))
- }
- recordLogin()
- window.location.assign(redirectTo)
+ setIsLoading(true)
+ setError('')
+ try {
+ const response = await fetch('/api/mfa-skip', { method: 'POST' })
+ if (!response.ok) {
+ throw new Error('Failed to persist MFA grace period')
+ }
+
+ // Track locally for UI display only
+ if (!localStorage.getItem(MFA_GRACE_KEY)) {
+ localStorage.setItem(MFA_GRACE_KEY, String(Date.now()))
+ }
+
+ recordLogin()
+ window.location.assign(redirectTo)
+ } catch {
+ setError('Unable to postpone MFA setup. Please try again.')
+ setIsLoading(false)
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/admin/src/app/auth/mfa-enroll/page.tsx` around lines 65 - 73, The
current handleSkip implementation updates localStorage, calls recordLogin(), and
navigates regardless of whether the POST to /api/mfa-skip succeeded; change
handleSkip to await the fetch response, check response.ok, and only on success
perform localStorage.setItem(MFA_GRACE_KEY, ...), call recordLogin(), and
window.location.assign(redirectTo); on failure do not modify local state or
redirect (instead return or surface an error to the user). Ensure you reference
the existing symbols handleSkip, MFA_GRACE_KEY, /api/mfa-skip, recordLogin, and
redirectTo when making the conditional change.
Comment on lines
+24
to
+25
| const rawRedirect = searchParams.get('redirectTo') || '/trips' | ||
| const redirectTo = rawRedirect.startsWith('/') && !rawRedirect.startsWith('//') ? rawRedirect : '/trips' |
There was a problem hiding this comment.
Reject quote-bearing paths here, not just absolute URLs.
/';alert(1);// passes this predicate. The callback page later interpolates next into an inline script (apps/admin/src/app/auth/callback/route.ts:68-148), so this value can break out of the string literal and execute arbitrary JS after MFA verification. Please lock this down to a strict internal-path allowlist and reuse that helper in both MFA pages, or escape next in the callback route before embedding it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/admin/src/app/auth/mfa-verify/page.tsx` around lines 24 - 25, The
current redirect sanitization using rawRedirect/redirectTo allows payloads with
quotes (e.g. "/';alert(1);//") which can break out of a JS string in the
callback; update the logic to reject any redirectTo values containing quotes or
other quote-bearing characters and restrict to a strict internal-path allowlist
(e.g. only ^/[-A-Za-z0-9_/]*$) by extracting this check into a shared helper
(e.g. isValidInternalRedirect or sanitizeRedirect) and use it in both MFA pages
(mfa-verify/page.tsx and the callback route) — alternatively, if you must embed
next into inline script in callback route, escape/encode the value there before
interpolation (escapeJS or JSON.stringify) so arbitrary JS cannot execute.
Comment on lines
+333
to
+355
| const handleChangeDevice = async () => { | ||
| if (!factors[0]) return | ||
| setEnrolling(true) | ||
| setError('') | ||
| const success = await unenroll(factors[0].id) | ||
| if (!success) { | ||
| setEnrolling(false) | ||
| toast({ title: 'Failed to remove existing factor', variant: 'destructive' }) | ||
| return | ||
| } | ||
| // Start new enrollment immediately | ||
| const result = await enroll('Tailfire') | ||
| if (!result) { | ||
| setEnrolling(false) | ||
| setError('Failed to start 2FA setup. Please try again.') | ||
| return | ||
| } | ||
| setFactorId(result.factorId) | ||
| setQrCode(result.qrCode) | ||
| setSecret(result.secret) | ||
| } | ||
|
|
||
| // Enrolled state — show status + change device / remove options |
There was a problem hiding this comment.
Change Device still falls through to the generic not-enrolled state while re-provisioning.
handleChangeDevice() flips enrolling before a new qrCode exists, but the render logic only treats enrolling && qrCode as the enrollment flow. Until enroll() resolves, the component drops into the default setup card, which is the flash this change is trying to remove. Add an explicit loading branch for enrolling && !qrCode, or gate the fallback card on !enrolling.
Suggested fix
+ if (enrolling && !qrCode) {
+ return (
+ <Card>
+ <CardHeader>
+ <CardTitle>Set Up Two-Factor Authentication</CardTitle>
+ <CardDescription>Preparing your new authenticator setup...</CardDescription>
+ </CardHeader>
+ <CardContent className="flex justify-center py-6">
+ <Loader2 className="h-6 w-6 animate-spin text-muted-foreground" />
+ </CardContent>
+ </Card>
+ )
+ }
+
// Enrollment flow
if (enrolling && qrCode) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/admin/src/app/profile/_components/security-tab.tsx` around lines 333 -
355, The UI flashes the not-enrolled card because handleChangeDevice sets
enrolling=true before enroll() returns, but the render only treats the
enrollment flow as enrolling && qrCode; update the render logic in this
component to explicitly handle the intermediate state enrolling && !qrCode (show
a loading/progress state) or change the fallback condition to require !enrolling
before rendering the default setup card; locate and modify the render branch
that checks enrolling and qrCode, and use the enrolling, qrCode, enroll,
unenroll, handleChangeDevice and factors symbols to implement the loading branch
so the component does not fall through while enroll() is pending.
Comment on lines
+135
to
+137
| } catch { | ||
| toast({ title: 'Failed to reset MFA', variant: 'destructive' }) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the file exists and view the relevant section
if [ -f "apps/admin/src/app/settings/users/page.tsx" ]; then
echo "=== File exists, checking lines 130-145 ==="
sed -n '130,145p' "apps/admin/src/app/settings/users/page.tsx"
else
echo "File not found at apps/admin/src/app/settings/users/page.tsx"
# Try to find similar files
find apps/admin -name "page.tsx" -type f | head -5
fiRepository: Systemsaholic/tailfire
Length of output: 679
🏁 Script executed:
# Search for existing Sentry imports in the admin app
echo "=== Sentry imports in admin app ==="
rg "import.*[Ss]entry" apps/admin/src --type ts --type tsx | head -20Repository: Systemsaholic/tailfire
Length of output: 148
🏁 Script executed:
# Check for Sentry usage patterns in admin app
echo "=== Sentry.captureException usage patterns ==="
rg "Sentry\.captureException" apps/admin/src --type ts --type tsx -A 3 | head -30Repository: Systemsaholic/tailfire
Length of output: 159
🏁 Script executed:
# Check for NEXT_PUBLIC_SENTRY_ENVIRONMENT usage
echo "=== Environment variable naming ==="
rg "NEXT_PUBLIC_SENTRY_ENVIRONMENT|SENTRY_ENVIRONMENT" apps/admin/src --type ts --type tsx | head -10Repository: Systemsaholic/tailfire
Length of output: 148
🏁 Script executed:
# Search for Sentry imports and usage in admin app without type flag
echo "=== Sentry imports in admin ==="
rg "import.*Sentry|from.*sentry" apps/admin/src | head -20
echo "=== Sentry.captureException usage ==="
rg "Sentry\.captureException" apps/admin/src -A 3 | head -30Repository: Systemsaholic/tailfire
Length of output: 493
🏁 Script executed:
# Check for environment variable usage in admin
echo "=== Environment variable patterns ==="
rg "process\.env\.NEXT_PUBLIC" apps/admin/src | head -20Repository: Systemsaholic/tailfire
Length of output: 2095
🏁 Script executed:
# Look for Sentry configuration/initialization in admin
echo "=== Sentry init files ==="
find apps/admin -name "*sentry*" -o -name "instrumentation.*"Repository: Systemsaholic/tailfire
Length of output: 229
🏁 Script executed:
# Check if there are error capture patterns elsewhere in the file
echo "=== Error handling patterns in page.tsx ==="
rg "captureException|captureMessage|catch" apps/admin/src/app/settings/users/page.tsx -B 2 -A 2Repository: Systemsaholic/tailfire
Length of output: 294
🏁 Script executed:
# Verify the full context of imports in the page.tsx file
echo "=== First 50 lines of page.tsx (imports section) ==="
head -50 apps/admin/src/app/settings/users/page.tsxRepository: Systemsaholic/tailfire
Length of output: 2240
🏁 Script executed:
# Check Sentry configuration files for environment variable setup
echo "=== sentry.client.config.ts ==="
cat apps/admin/sentry.client.config.ts
echo -e "\n=== sentry.server.config.ts ==="
cat apps/admin/sentry.server.config.tsRepository: Systemsaholic/tailfire
Length of output: 1079
🏁 Script executed:
# Search for any SENTRY_ENVIRONMENT variable usage
echo "=== SENTRY_ENVIRONMENT variants ==="
rg "SENTRY_ENVIRONMENT" apps/ --type ts --type jsRepository: Systemsaholic/tailfire
Length of output: 1014
🏁 Script executed:
# Check instrumentation.ts to see how Sentry is initialized
echo "=== instrumentation.ts content ==="
cat apps/admin/src/instrumentation.tsRepository: Systemsaholic/tailfire
Length of output: 424
🏁 Script executed:
# Check if there are other error handlers with Sentry in the page.tsx
echo "=== All catch blocks and try patterns in page.tsx ==="
rg "try|catch" apps/admin/src/app/settings/users/page.tsx -B 2 -A 2Repository: Systemsaholic/tailfire
Length of output: 555
Capture reset-MFA failures in Sentry before showing toast.
The new admin MFA reset flow swallows errors in catch without telemetry. This violates the admin app requirement to capture runtime errors with Sentry for observability and audit purposes.
Proposed fix
+import * as Sentry from '@sentry/nextjs'
// ...existing imports
- } catch {
+ } catch (err) {
+ Sentry.captureException(err, {
+ tags: {
+ environment: process.env.NEXT_PUBLIC_SENTRY_ENVIRONMENT || 'development',
+ },
+ })
toast({ title: 'Failed to reset MFA', variant: 'destructive' })
}📝 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.
Suggested change
| } catch { | |
| toast({ title: 'Failed to reset MFA', variant: 'destructive' }) | |
| } | |
| import * as Sentry from '@sentry/nextjs' | |
| // ...existing imports | |
| } catch (err) { | |
| Sentry.captureException(err, { | |
| tags: { | |
| environment: process.env.NEXT_PUBLIC_SENTRY_ENVIRONMENT || 'development', | |
| }, | |
| }) | |
| toast({ title: 'Failed to reset MFA', variant: 'destructive' }) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/admin/src/app/settings/users/page.tsx` around lines 135 - 137, The catch
block that currently swallows errors when resetting MFA should capture the
exception in Sentry before showing the toast: modify the catch in the reset
handler (the try/catch surrounding the MFA reset call in
apps/admin/src/app/settings/users/page.tsx) to accept the error parameter, call
Sentry.captureException(error) (or Sentry.captureException(err) depending on
import), then proceed to call toast({ title: 'Failed to reset MFA', variant:
'destructive' }); ensure Sentry is imported/available in that module and include
any relevant context (user id/email) if available from surrounding variables.
Comment on lines
+123
to
+126
| const rawQr = result.totp.qr_code | ||
| const qrCode = rawQr.startsWith('data:') | ||
| ? rawQr | ||
| : `data:image/svg+xml;utf8,${encodeURIComponent(rawQr)}` |
There was a problem hiding this comment.
Guard qr_code before calling string methods.
On Line 124, rawQr.startsWith(...) assumes qr_code is always a string. A malformed/partial response will throw and abort enrollment.
Proposed fix
- const rawQr = result.totp.qr_code
- const qrCode = rawQr.startsWith('data:')
+ const rawQr = result.totp.qr_code
+ if (typeof rawQr !== 'string' || rawQr.length === 0) {
+ console.error('[MFA] Enroll: missing qr_code in response', result)
+ return null
+ }
+ const qrCode = rawQr.startsWith('data:')
? rawQr
: `data:image/svg+xml;utf8,${encodeURIComponent(rawQr)}`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/admin/src/hooks/use-mfa.ts` around lines 123 - 126, The code in
use-mfa.ts assumes result.totp.qr_code is a string when computing
rawQr.startsWith, which can throw for null/undefined/non-string values; update
the qr handling around rawQr/qrCode to first guard the value (e.g., check typeof
rawQr === 'string' && rawQr.length > 0) and only call startsWith when it's a
string, otherwise provide a safe fallback (empty data URL or null) and ensure
qrCode is produced deterministically; reference the rawQr variable and qrCode
assignment in the hook to locate and update the logic.
refreshState() used supabase.auth.mfa.getAuthenticatorAssuranceLevel() and listFactors() — both acquire the navigator lock and hang when competing with session refresh. This caused the verify page to see factors=[] and redirect to enroll incorrectly. Now uses REST GET /auth/v1/factors + JWT decode for AAL, consistent with all other MFA operations in this hook. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This was referenced Apr 30, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
@supabase/ssrfor cookie-based session storage (was localStorage — caused infinite redirect loop after MFA verify)useResetUserMfamutationTest plan
?redirectTo=https://evil.com→ redirects to /trips (not evil.com)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Chores