feat: production hardening + Supabase MFA (TOTP)#256
Conversation
- Replace console.* with NestJS Logger in bootstrap (main.ts) - Disable sendDefaultPii in Sentry (instrument.ts) - Add rate limiting (5 req/min) to credential reveal endpoint - Harden HttpExceptionFilter: generic message for 5xx, log details - Add security headers to admin (X-Frame-Options, X-Content-Type-Options, Referrer-Policy, Permissions-Policy) — exempt /auth/callback Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…e gating - useMfa hook: wraps Supabase mfa.* APIs (enroll, verify, challenge, unenroll) - /auth/mfa-verify page: TOTP code entry with Phoenix Voyages branding - /auth/mfa-enroll page: QR code setup flow with manual secret + verify - Feature flag: MFA_REQUIRED (NEXT_PUBLIC + server-side MFA_REQUIRED) - Auth provider: exposes aal + mfaEnrolled, moves record-login to post-aal2 - Middleware: redirects aal1 users to verify/enroll when enforcement on - Login form: post-login check redirects to mfa-verify if factors enrolled MFA enforcement is OFF by default (grace period). Flip MFA_REQUIRED=true to enforce. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- MfaGuard: checks aal claim, rejects aal1 when MFA_REQUIRED=true - @BypassMfa() decorator for MFA management endpoints - aal added to AuthContext and JwtPayload types - JWT strategy extracts aal from payload - Guard registered in main.ts: JwtAuth → MFA → Impersonation → Roles - Skips: @public routes, @BypassMfa routes, pending users 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 (2)
📝 WalkthroughWalkthroughImplements multi-factor authentication (MFA) support across admin and API applications, including enrollment and verification UI pages, MFA state hooks, backend JWT claims, middleware-based enforcement with redirects, API guards, and security hardening through headers, improved error logging, and PII redaction. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Client as Admin Client
participant AuthAPI as Supabase Auth
participant AppServer as App Server
User->>Client: Enter login credentials
Client->>AuthServer: Submit login form
AuthServer->>AuthAPI: Authenticate with email/password
AuthAPI-->>AuthServer: Return session + aal='aal1'
AuthServer->>AuthServer: Check mfa state
alt MFA enrolled & aal='aal1'
AuthServer-->>Client: Redirect to /auth/mfa-verify
User->>Client: Enter 6-digit TOTP code
Client->>AuthAPI: Verify MFA challenge
AuthAPI-->>Client: MFA verified, aal='aal2'
Client->>AppServer: Record login after MFA
AppServer-->>Client: Navigate to original destination
else MFA not enrolled & MFA_REQUIRED
AuthServer-->>Client: Redirect to /auth/mfa-enroll
User->>Client: Initiate MFA setup
Client->>AuthAPI: Enroll TOTP factor
AuthAPI-->>Client: Return QR + secret
User->>Client: Scan QR & enter 6-digit code
Client->>AuthAPI: Verify enrollment code
AuthAPI-->>Client: Factor enrolled successfully
Client->>AppServer: Record login after enrollment
AppServer-->>Client: Navigate to original destination
else No MFA required
AuthServer-->>Client: Record login immediately
AppServer-->>Client: Navigate to dashboard
end
sequenceDiagram
participant Client as API Client
participant AppGateway as API Gateway<br/>(Global Guards)
participant JWTGuard as JwtAuthGuard
participant MFAGuard as MfaGuard
participant Endpoint as Protected Endpoint
Client->>AppGateway: HTTP request with JWT
AppGateway->>JWTGuard: Validate & extract JWT claims
JWTGuard->>JWTGuard: Decode token, extract aal
JWTGuard-->>AppGateway: Return AuthContext with aal
AppGateway->>MFAGuard: Check if MFA_REQUIRED enabled
alt MFA_REQUIRED == true
MFAGuard->>MFAGuard: Read `@Public/`@BypassMfa metadata
alt Handler is `@Public` or `@BypassMfa`
MFAGuard-->>Endpoint: Allow request to proceed
else Handler requires MFA
alt aal == 'aal2'
MFAGuard-->>Endpoint: Allow request to proceed
else aal != 'aal2'
MFAGuard-->>Client: ForbiddenException: MFA verification required
end
end
else MFA_REQUIRED == false
MFAGuard-->>Endpoint: Allow request to proceed
end
Endpoint->>Endpoint: Process request
Endpoint-->>Client: Return response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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. 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: 8
🧹 Nitpick comments (3)
apps/admin/src/hooks/use-mfa.ts (1)
38-62: Don't silently swallow MFA errors — report to Sentry.
refreshState,enroll,verify, andunenrollall swallow errors with empty catches or by returningnull/false. That hides genuine outages (network errors, Supabase outages, malformed responses) from operators and from the admin Sentry stream. Per the admin Sentry guideline, runtime errors here should be captured (tagged withNEXT_PUBLIC_SENTRY_ENVIRONMENT).At minimum, consider
Sentry.captureException(error)in the catch blocks while keeping the user-facing fallback behavior. As per coding guidelines: "Admin application must capture runtime errors with Sentry. Errors must be tagged withNEXT_PUBLIC_SENTRY_ENVIRONMENTasdevelopment,preview, orproduction."🤖 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 38 - 62, refreshState, enroll, verify, and unenroll currently swallow exceptions; update each catch block to call Sentry.captureException(error) (or captureException(e)) before returning the existing fallback so errors appear in Sentry. Include the NEXT_PUBLIC_SENTRY_ENVIRONMENT value as a tag/extra on the capture (e.g., using Sentry.setContext or passing { tags: { environment: process.env.NEXT_PUBLIC_SENTRY_ENVIRONMENT } } with the capture) so the error is attributed to dev/preview/production, and keep the original user-facing return behavior (null/false) after capturing.apps/admin/src/providers/auth-provider.tsx (2)
119-140: DuplicategetAuthenticatorAssuranceLevel()call per SIGNED_IN event.When
event === 'SIGNED_IN'with a session,checkMfaLevel()(line 120) already issues a Supabase MFA round-trip; then lines 130-139 immediately issue a secondgetAuthenticatorAssuranceLevel()to decide whether to record login. Two back-to-back identical network calls per sign-in. Reusing the data fromcheckMfaLevel(e.g. by having it return{ currentLevel, nextLevel }) would remove the second call and consolidate the decision logic.Also note: this branch hand-rolls the record-login fetch instead of calling the new
recordLogin()callback, which then bypasses theloginRecordedguard's symmetry (it setsloginRecorded(true)on line 138 directly). Folding the non-MFA path intorecordLogin()(or a shared helper) would keep one source of truth.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin/src/providers/auth-provider.tsx` around lines 119 - 140, checkMfaLevel currently calls supabase.auth.mfa.getAuthenticatorAssuranceLevel() but its result is discarded; modify checkMfaLevel to return the MFA assurance info (e.g., { currentLevel, nextLevel }) and then in the SIGNED_IN branch reuse that returned value instead of calling getAuthenticatorAssuranceLevel() a second time; move the non‑MFA "record login" logic into the existing recordLogin() callback (or a shared helper) and call recordLogin() when nextLevel !== 'aal2', removing the direct fetch and the direct setLoginRecorded(true) so the loginRecorded guard remains symmetric and single-sourced.
87-98: WraprecordLogininuseCallback.
recordLoginis recreated on everyAuthProviderrender and exposed via context. Consumers (e.g.mfa-verify/mfa-enrollpages) typically call this from auseEffect; ifrecordLoginends up in their dependency arrays it triggers an effect every render. The internalloginRecordedflag prevents duplicate POSTs but won't prevent the re-run cycle.♻️ Suggested fix
- const recordLogin = () => { + const recordLogin = useCallback(() => { if (loginRecorded) return const token = session?.access_token if (!token) return setLoginRecorded(true) const apiUrl = process.env.NEXT_PUBLIC_API_URL || '/api/v1' fetch(`${apiUrl}/user-profiles/me/record-login`, { method: 'POST', headers: { Authorization: `Bearer ${token}` }, }).catch(() => {/* non-critical */}) - } + }, [loginRecorded, session?.access_token])(Add
useCallbackto the existing react import.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin/src/providers/auth-provider.tsx` around lines 87 - 98, The recordLogin function inside AuthProvider is recreated on every render and should be memoized to avoid causing consumer useEffect re-runs; wrap recordLogin with React's useCallback (add useCallback to the react import) and have it depend only on the stable values it uses (session?.access_token, loginRecorded, and setLoginRecorded) so that recordLogin, loginRecorded, session, setLoginRecorded remain the referenced symbols and the fetch call to `${process.env.NEXT_PUBLIC_API_URL || '/api/v1'}/user-profiles/me/record-login` still executes unchanged.
🤖 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/next.config.ts`:
- Around line 82-95: The current headers() config uses a broad
negative-lookahead source pattern '/((?!auth/callback).*)' which unintentionally
exempts paths like '/auth/callbackfoo' or '/auth/callback/something'; tighten
the pattern to only exempt the exact route by changing the source used in the
headers() return (the object with source property) so it matches the literal
'/auth/callback' only (while leaving all other header entries like
'X-Frame-Options', 'X-Content-Type-Options', 'Referrer-Policy', and
'Permissions-Policy' unchanged).
In `@apps/admin/src/app/auth/mfa-enroll/page.tsx`:
- Around line 72-76: The handleCopySecret currently calls
navigator.clipboard.writeText(secret) without awaiting or handling rejection and
immediately sets setSecretCopied(true) and a timeout, which can show success
when copy failed and can fire after unmount; change handleCopySecret to be
async/await (or use .then/.catch), await navigator.clipboard.writeText(secret),
only call setSecretCopied(true) when the Promise resolves, handle errors by
showing a user-visible failure state (e.g., set an error flag or fallback UI)
and do not mark success on rejection, and replace the bare setTimeout with a
timerId stored in a ref (e.g., copyTimerRef) that you clear in a useEffect
cleanup to avoid calling setSecretCopied after unmount.
- Line 36: The MFA enrollment page uses the hardcoded TOTP issuer string
"Tailfire" (see call to enroll('Tailfire')) and the completion button text
"Continue to Tailfire", but the PR description and other UI elements use
"Phoenix Voyages"; decide which public-facing brand should appear in the admin
UI, then update the strings accordingly: change the enroll(...) issuer argument
and any button/label text in apps/admin/src/app/auth/mfa-enroll/page.tsx (and
related UI strings in that component) to the chosen brand (either "Phoenix
Voyages" or "Tailfire") so implementation matches the PR description, and update
the PR description to reflect the final choice.
In `@apps/admin/src/app/auth/mfa-verify/page.tsx`:
- Around line 36-40: The page currently dead-ends when no MFA factors exist;
instead of setError('No authenticator...') replace that branch to
navigate/redirect to /auth/mfa-enroll with the current redirectTo param (same
pattern as login-form.tsx) — note factors and factor come from useMfa which
already filters verified factors so no extra status guard needed. Also add
Sentry error capture: wrap the handleVerify logic and the refreshState call in
try/catch blocks and call Sentry.captureException(err) inside the catches
(update the handleVerify and refreshState invocations in this page accordingly)
so runtime errors are reported.
In `@apps/admin/src/lib/supabase/middleware.ts`:
- Around line 61-70: Avoid calling
supabase.auth.mfa.getAuthenticatorAssuranceLevel() on every request: only
perform the MFA HTTP lookup when MFA is actually required and when we haven't
already determined the session is at aal2. Modify the block that sets
hasMfaFactors so it first checks the server flag (mfaRequired) and that aal !==
'aal2' (and user exists); only then call getAuthenticatorAssuranceLevel() and
set hasMfaFactors from data?.nextLevel === 'aal2'. Ensure existing try/catch
remains to ignore non-critical failures and return the same shape { user,
userStatus, aal, hasMfaFactors, supabaseResponse }.
In `@apps/admin/src/middleware.ts`:
- Around line 80-98: The MFA check currently allows requests through when aal is
null (decode failure); update the logic in the middleware (the block using
mfaRequired, mfaExemptRoutes, hasMfaFactors, and aal) to fail-closed by treating
anything other than aal2 as requiring MFA: replace the condition that checks for
"hasMfaFactors && aal === 'aal1'" with a predicate that redirects whenever
hasMfaFactors is true and aal !== 'aal2' (so null/undefined also triggers the
verify redirect), keeping the existing enroll redirect when !hasMfaFactors; no
other behavior should change and this uses the same NextResponse.redirect flow
and verify/enroll URLs.
In `@apps/api/src/api-credentials/api-credentials.controller.ts`:
- Around line 94-98: The doc comment on the reveal endpoint is inaccurate: the
current `@UseGuards`(ThrottlerGuard) + `@Throttle` uses IP-based tracking (not
per-user), so either update the comment to say "5 reveals per minute per IP" or
implement a per-user throttle guard; to implement, add a UserThrottleGuard
subclassing ThrottlerGuard (similar to CatalogThrottleGuard) that overrides
getTracker(req) to return the authenticated user's id (e.g.
`user:${req.user.userId}`) falling back to `ip:${req.ip}` and replace
`@UseGuards`(ThrottlerGuard) on the :id/reveal handler with
`@UseGuards`(UserThrottleGuard).
In `@apps/api/src/auth/auth.types.ts`:
- Line 15: The impersonated AuthContext created in ImpersonationGuard omits the
required aal field, causing a type-contract violation and potential runtime
undefined reads; update the impersonation logic in impersonation.guard.ts (the
code that sets request.user / constructs the impersonated AuthContext) to copy
the admin's aal value (from the original request.user or admin context validated
by MfaGuard) into the new impersonated object so that the resulting request.user
implements AuthContext.aal (e.g., ensure the impersonated context includes aal:
adminUser.aal) and remove any unsafe casts that hide the missing property.
---
Nitpick comments:
In `@apps/admin/src/hooks/use-mfa.ts`:
- Around line 38-62: refreshState, enroll, verify, and unenroll currently
swallow exceptions; update each catch block to call
Sentry.captureException(error) (or captureException(e)) before returning the
existing fallback so errors appear in Sentry. Include the
NEXT_PUBLIC_SENTRY_ENVIRONMENT value as a tag/extra on the capture (e.g., using
Sentry.setContext or passing { tags: { environment:
process.env.NEXT_PUBLIC_SENTRY_ENVIRONMENT } } with the capture) so the error is
attributed to dev/preview/production, and keep the original user-facing return
behavior (null/false) after capturing.
In `@apps/admin/src/providers/auth-provider.tsx`:
- Around line 119-140: checkMfaLevel currently calls
supabase.auth.mfa.getAuthenticatorAssuranceLevel() but its result is discarded;
modify checkMfaLevel to return the MFA assurance info (e.g., { currentLevel,
nextLevel }) and then in the SIGNED_IN branch reuse that returned value instead
of calling getAuthenticatorAssuranceLevel() a second time; move the non‑MFA
"record login" logic into the existing recordLogin() callback (or a shared
helper) and call recordLogin() when nextLevel !== 'aal2', removing the direct
fetch and the direct setLoginRecorded(true) so the loginRecorded guard remains
symmetric and single-sourced.
- Around line 87-98: The recordLogin function inside AuthProvider is recreated
on every render and should be memoized to avoid causing consumer useEffect
re-runs; wrap recordLogin with React's useCallback (add useCallback to the react
import) and have it depend only on the stable values it uses
(session?.access_token, loginRecorded, and setLoginRecorded) so that
recordLogin, loginRecorded, session, setLoginRecorded remain the referenced
symbols and the fetch call to `${process.env.NEXT_PUBLIC_API_URL ||
'/api/v1'}/user-profiles/me/record-login` still executes unchanged.
🪄 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: 651d7180-79fb-48da-93f7-09362de3c979
📒 Files selected for processing (17)
apps/admin/next.config.tsapps/admin/src/app/auth/mfa-enroll/page.tsxapps/admin/src/app/auth/mfa-verify/page.tsxapps/admin/src/components/auth/login-form.tsxapps/admin/src/hooks/use-mfa.tsapps/admin/src/lib/feature-flags.tsapps/admin/src/lib/supabase/middleware.tsapps/admin/src/middleware.tsapps/admin/src/providers/auth-provider.tsxapps/api/src/api-credentials/api-credentials.controller.tsapps/api/src/auth/auth.types.tsapps/api/src/auth/decorators/bypass-mfa.decorator.tsapps/api/src/auth/guards/mfa.guard.tsapps/api/src/auth/strategies/jwt.strategy.tsapps/api/src/common/filters/http-exception.filter.tsapps/api/src/instrument.tsapps/api/src/main.ts
| // Security headers (exempt /auth/callback which uses inline scripts) | ||
| async headers() { | ||
| return [ | ||
| { | ||
| source: '/((?!auth/callback).*)', | ||
| headers: [ | ||
| { key: 'X-Frame-Options', value: 'DENY' }, | ||
| { key: 'X-Content-Type-Options', value: 'nosniff' }, | ||
| { key: 'Referrer-Policy', value: 'strict-origin-when-cross-origin' }, | ||
| { key: 'Permissions-Policy', value: 'camera=(), microphone=(), geolocation=()' }, | ||
| ], | ||
| }, | ||
| ] | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Next.js headers() config source pattern with negative lookahead syntax
💡 Result:
In Next.js, the headers function in next.config.js uses path-to-regexp for matching the source pattern. Negative lookahead regex syntax is supported but must follow path-to-regexp conventions: wrap the regex in parentheses as an unnamed parameter after a path segment, typically /:path((?!excluded).). Example to match all paths except those containing "api": module.exports = { async headers { return [ { source: '/:path((?!api).)', headers: [ { key: 'X-Custom-Header', value: 'value' } ] } ] } } More precise exclusion of paths starting with "/api": { source: '/((?!api|_next/static|_next/image|favicon.ico).)', headers: [...] } Common patterns from Next.js GitHub discussions and docs: - Exclude specific paths: '/((?!abc).)' [9,19] - Exclude multiple paths: '/((?!_next|foo$|bar$).)' [9] - For catch-all with exclusion: '/((?!books).*)' [10] Direct ^(?!...) or standalone negative lookaheads fail with "Pattern cannot start with '?'" because Next.js wraps the source in ^/(?:(...))$/i [15,22]. Use the parameter-wrapped syntax instead. Official docs confirm regex support via parentheses [2,3,13], and path-to-regexp handles lookaheads [22]. This works for headers, redirects, and rewrites.
Citations:
- 1: https://nextjs.org/docs/advanced-features/security-headers
- 2: https://nextjs.org/docs/pages/api-reference/config/next-config-js/headers
- 3: Exclude path from matching in redirects/rewrites/headers vercel/next.js#24255
- 4: https://stackoverflow.com/questions/73104326/pattern-cannot-start-with-facing-regex-path-matching-issue-in-next-config
- 5: https://nextjs.org/docs/pages/api-reference/config/next-config-js/rewrites
- 6: https://nextjs.org/docs/app/api-reference/file-conventions/proxy
- 7: https://stackoverflow.com/questions/69518118/nextjs-redirect-based-on-regex
- 8: Exclude path from matching in redirects/rewrites/headers vercel/next.js#34323
- 9: https://nextjs.org/docs/messages/invalid-route-source
🏁 Script executed:
# Verify the file exists and examine the relevant lines
if [ -f "apps/admin/next.config.ts" ]; then
echo "=== File exists, examining lines 82-95 ==="
sed -n '80,100p' apps/admin/next.config.ts
else
echo "File not found at apps/admin/next.config.ts"
# Try to locate it
find . -name "next.config.ts" -o -name "next.config.js" | head -5
fiRepository: Systemsaholic/tailfire
Length of output: 737
🏁 Script executed:
# Check if there's an auth/callback page with inline scripts
find . -type f \( -name "*.ts" -o -name "*.tsx" -o -name "*.js" -o -name "*.jsx" \) | xargs grep -l "auth.*callback" | head -10Repository: Systemsaholic/tailfire
Length of output: 445
🏁 Script executed:
# Check the auth callback route implementation in apps/admin
cat -n apps/admin/src/app/auth/callback/route.tsRepository: Systemsaholic/tailfire
Length of output: 7935
Exemption is justified, but pattern can be tightened for precision.
The exemption is actually sound: the /auth/callback route returns HTML with inline <script type="module"> and inline <style> blocks (see getHashHandlerHtml() in the route). The route code even includes a comment noting "If CSP is enforced, this page needs 'unsafe-inline' for scripts," which indicates this exemption is future-proofing for when Content-Security-Policy may be added.
However, the negative lookahead pattern /((?!auth/callback).*) is broader than intended—it also excludes paths like /auth/callbackfoo or /auth/callback-reset. To match only the literal route, tighten the boundary:
- source: '/((?!auth/callback).*)',
+ source: '/((?!auth/callback(?:/|$)).*)',This ensures /auth/callback/something and /auth/callbackfoo still receive security headers.
📝 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.
| // Security headers (exempt /auth/callback which uses inline scripts) | |
| async headers() { | |
| return [ | |
| { | |
| source: '/((?!auth/callback).*)', | |
| headers: [ | |
| { key: 'X-Frame-Options', value: 'DENY' }, | |
| { key: 'X-Content-Type-Options', value: 'nosniff' }, | |
| { key: 'Referrer-Policy', value: 'strict-origin-when-cross-origin' }, | |
| { key: 'Permissions-Policy', value: 'camera=(), microphone=(), geolocation=()' }, | |
| ], | |
| }, | |
| ] | |
| }, | |
| // Security headers (exempt /auth/callback which uses inline scripts) | |
| async headers() { | |
| return [ | |
| { | |
| source: '/((?!auth/callback(?:/|$)).*)', | |
| headers: [ | |
| { key: 'X-Frame-Options', value: 'DENY' }, | |
| { key: 'X-Content-Type-Options', value: 'nosniff' }, | |
| { key: 'Referrer-Policy', value: 'strict-origin-when-cross-origin' }, | |
| { key: 'Permissions-Policy', value: 'camera=(), microphone=(), geolocation=()' }, | |
| ], | |
| }, | |
| ] | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/admin/next.config.ts` around lines 82 - 95, The current headers() config
uses a broad negative-lookahead source pattern '/((?!auth/callback).*)' which
unintentionally exempts paths like '/auth/callbackfoo' or
'/auth/callback/something'; tighten the pattern to only exempt the exact route
by changing the source used in the headers() return (the object with source
property) so it matches the literal '/auth/callback' only (while leaving all
other header entries like 'X-Frame-Options', 'X-Content-Type-Options',
'Referrer-Policy', and 'Permissions-Policy' unchanged).
| setIsLoading(true) | ||
| setError('') | ||
|
|
||
| const result = await enroll('Tailfire') |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look at all visible brand strings in admin UI to confirm consistency.
rg -nP --type=tsx --type=ts -C1 -i 'Tailfire|Phoenix\s*Voyages'Repository: Systemsaholic/tailfire
Length of output: 94
🏁 Script executed:
#!/bin/bash
# Search for Tailfire and Phoenix Voyages mentions in TS/TSX files
echo "=== Searching for branding mentions ==="
rg -nP -i 'Tailfire|Phoenix.*Voyages' --glob='*.{ts,tsx}' -C 2
echo ""
echo "=== Reading the specific file mentioned in the review ==="
cat -n apps/admin/src/app/auth/mfa-enroll/page.tsx | head -n 220 | tail -n 190Repository: Systemsaholic/tailfire
Length of output: 50381
🏁 Script executed:
cat -n apps/admin/src/app/auth/mfa-enroll/page.tsx | sed -n '1,220p'Repository: Systemsaholic/tailfire
Length of output: 10056
Confirm branding alignment between PR description and implementation.
The MFA enrollment page uses "Tailfire" as the TOTP issuer (line 36) and in the completion button text (line 205: "Continue to Tailfire"), but the PR description states "Phoenix Voyages branding". Across the codebase, Phoenix Voyages is the public-facing brand (Footer, social links, email), while Tailfire is internal technical branding (monorepo, package scopes, internal domains). Clarify whether the admin UI should display Tailfire or Phoenix Voyages, and update the PR description to reflect the actual implementation.
🤖 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` at line 36, The MFA enrollment
page uses the hardcoded TOTP issuer string "Tailfire" (see call to
enroll('Tailfire')) and the completion button text "Continue to Tailfire", but
the PR description and other UI elements use "Phoenix Voyages"; decide which
public-facing brand should appear in the admin UI, then update the strings
accordingly: change the enroll(...) issuer argument and any button/label text in
apps/admin/src/app/auth/mfa-enroll/page.tsx (and related UI strings in that
component) to the chosen brand (either "Phoenix Voyages" or "Tailfire") so
implementation matches the PR description, and update the PR description to
reflect the final choice.
| const handleCopySecret = () => { | ||
| navigator.clipboard.writeText(secret) | ||
| setSecretCopied(true) | ||
| setTimeout(() => setSecretCopied(false), 2000) | ||
| } |
There was a problem hiding this comment.
navigator.clipboard.writeText silently fails; secret may not actually be copied.
writeText returns a Promise that can reject (permission denied, insecure HTTP context, sandboxed iframe, focus/transient activation rules). The current code unconditionally flips secretCopied to true, so the user sees the "✓" indicator even when nothing landed on the clipboard — a real risk during MFA setup where the secret is the only recovery path. Also, the setTimeout will fire on an unmounted component if the user navigates away.
🛡️ Suggested fix
- const handleCopySecret = () => {
- navigator.clipboard.writeText(secret)
- setSecretCopied(true)
- setTimeout(() => setSecretCopied(false), 2000)
- }
+ const handleCopySecret = async () => {
+ try {
+ await navigator.clipboard.writeText(secret)
+ setSecretCopied(true)
+ setTimeout(() => setSecretCopied(false), 2000)
+ } catch {
+ toast({
+ title: 'Could not copy',
+ description: 'Select the code and copy it manually.',
+ variant: 'destructive',
+ })
+ }
+ }🤖 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 72 - 76, The
handleCopySecret currently calls navigator.clipboard.writeText(secret) without
awaiting or handling rejection and immediately sets setSecretCopied(true) and a
timeout, which can show success when copy failed and can fire after unmount;
change handleCopySecret to be async/await (or use .then/.catch), await
navigator.clipboard.writeText(secret), only call setSecretCopied(true) when the
Promise resolves, handle errors by showing a user-visible failure state (e.g.,
set an error flag or fallback UI) and do not mark success on rejection, and
replace the bare setTimeout with a timerId stored in a ref (e.g., copyTimerRef)
that you clear in a useEffect cleanup to avoid calling setSecretCopied after
unmount.
| const factor = factors[0] | ||
| if (!factor) { | ||
| setError('No authenticator found. Please contact support.') | ||
| return | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect what the useMfa hook returns for `factors` to confirm shape and whether unverified factors are included.
fd -t f 'use-mfa' apps/admin/src
rg -nP --type=ts --type=tsx -C5 'export\s+function\s+useMfa|factors\s*[:=]'Repository: Systemsaholic/tailfire
Length of output: 144
🏁 Script executed:
cat -n apps/admin/src/hooks/use-mfa.tsRepository: Systemsaholic/tailfire
Length of output: 4278
🏁 Script executed:
cat -n apps/admin/src/app/auth/mfa-verify/page.tsx | head -50Repository: Systemsaholic/tailfire
Length of output: 2051
🏁 Script executed:
fd -t f 'mfa-enroll' apps/admin/srcRepository: Systemsaholic/tailfire
Length of output: 48
🏁 Script executed:
find apps/admin/src -type d -name '*enroll*'Repository: Systemsaholic/tailfire
Length of output: 101
🏁 Script executed:
find apps/admin/src -type f -name '*enroll*'Repository: Systemsaholic/tailfire
Length of output: 48
🏁 Script executed:
rg 'mfa-enroll|auth/mfa' apps/admin/src --type tsRepository: Systemsaholic/tailfire
Length of output: 516
🏁 Script executed:
cat -n apps/admin/src/app/auth/mfa-verify/page.tsxRepository: Systemsaholic/tailfire
Length of output: 5629
🏁 Script executed:
rg 'Sentry|captureException|sentry' apps/admin/src/app/auth/mfa-verify/page.tsxRepository: Systemsaholic/tailfire
Length of output: 48
First concern is unfounded; second concern valid plus missing Sentry error capture.
-
factors[0]does not pick unverified factors. TheuseMfahook explicitly filters to verified factors only (line 46 ofuse-mfa.ts:const verifiedFactors = totpFactors.filter(f => f.status === 'verified')), so the array is already safe. The suggestedf.status === 'verified'guard is redundant. -
Empty factors should redirect to enroll, not dead-end. When a user reaches this page with no MFA set up, they see "Please contact support." Instead, redirect to
/auth/mfa-enrollwith theredirectToparam so they can set up an authenticator (pattern already used inlogin-form.tsx). -
Add Sentry error capture. Per the coding guidelines, admin pages must capture runtime errors with Sentry. The
handleVerifyfunction andrefreshStatecall have no error monitoring; wrap them in try-catch and callSentry.captureException.
🤖 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 36 - 40, The page
currently dead-ends when no MFA factors exist; instead of setError('No
authenticator...') replace that branch to navigate/redirect to /auth/mfa-enroll
with the current redirectTo param (same pattern as login-form.tsx) — note
factors and factor come from useMfa which already filters verified factors so no
extra status guard needed. Also add Sentry error capture: wrap the handleVerify
logic and the refreshState call in try/catch blocks and call
Sentry.captureException(err) inside the catches (update the handleVerify and
refreshState invocations in this page accordingly) so runtime errors are
reported.
| // Check if user has MFA factors enrolled (for middleware gating) | ||
| let hasMfaFactors = false | ||
| if (user) { | ||
| try { | ||
| const { data } = await supabase.auth.mfa.getAuthenticatorAssuranceLevel() | ||
| hasMfaFactors = data?.nextLevel === 'aal2' | ||
| } catch { /* ignore — MFA check non-critical */ } | ||
| } | ||
|
|
||
| return { user, userStatus, aal, hasMfaFactors, supabaseResponse } |
There was a problem hiding this comment.
Avoid the MFA round-trip on every authenticated middleware request.
getAuthenticatorAssuranceLevel() is an HTTP call to Supabase Auth and runs for every authenticated request hitting middleware, even when MFA_REQUIRED=false (the documented grace-period default). The caller only consults hasMfaFactors under the mfaRequired branch, so the round-trip is wasted on every other request. On Edge middleware this adds noticeable per-navigation latency.
Consider gating the lookup on the server flag (and short-circuiting when aal === 'aal2'):
♻️ Suggested gating
// Check if user has MFA factors enrolled (for middleware gating)
let hasMfaFactors = false
- if (user) {
+ const mfaRequired = process.env.MFA_REQUIRED === 'true'
+ if (user && mfaRequired && aal !== 'aal2') {
try {
const { data } = await supabase.auth.mfa.getAuthenticatorAssuranceLevel()
hasMfaFactors = data?.nextLevel === 'aal2'
} catch { /* ignore — MFA check non-critical */ }
}📝 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.
| // Check if user has MFA factors enrolled (for middleware gating) | |
| let hasMfaFactors = false | |
| if (user) { | |
| try { | |
| const { data } = await supabase.auth.mfa.getAuthenticatorAssuranceLevel() | |
| hasMfaFactors = data?.nextLevel === 'aal2' | |
| } catch { /* ignore — MFA check non-critical */ } | |
| } | |
| return { user, userStatus, aal, hasMfaFactors, supabaseResponse } | |
| // Check if user has MFA factors enrolled (for middleware gating) | |
| let hasMfaFactors = false | |
| const mfaRequired = process.env.MFA_REQUIRED === 'true' | |
| if (user && mfaRequired && aal !== 'aal2') { | |
| try { | |
| const { data } = await supabase.auth.mfa.getAuthenticatorAssuranceLevel() | |
| hasMfaFactors = data?.nextLevel === 'aal2' | |
| } catch { /* ignore — MFA check non-critical */ } | |
| } | |
| return { user, userStatus, aal, hasMfaFactors, supabaseResponse } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/admin/src/lib/supabase/middleware.ts` around lines 61 - 70, Avoid
calling supabase.auth.mfa.getAuthenticatorAssuranceLevel() on every request:
only perform the MFA HTTP lookup when MFA is actually required and when we
haven't already determined the session is at aal2. Modify the block that sets
hasMfaFactors so it first checks the server flag (mfaRequired) and that aal !==
'aal2' (and user exists); only then call getAuthenticatorAssuranceLevel() and
set hasMfaFactors from data?.nextLevel === 'aal2'. Ensure existing try/catch
remains to ignore non-critical failures and return the same shape { user,
userStatus, aal, hasMfaFactors, supabaseResponse }.
| // MFA enforcement (only when MFA_REQUIRED is true) | ||
| const mfaRequired = process.env.MFA_REQUIRED === 'true' | ||
| if (mfaRequired) { | ||
| const isMfaExempt = mfaExemptRoutes.some((route) => pathname.startsWith(route)) | ||
| if (!isMfaExempt) { | ||
| if (hasMfaFactors && aal === 'aal1') { | ||
| // Has factors but hasn't verified yet → verify page | ||
| const verifyUrl = new URL('/auth/mfa-verify', request.url) | ||
| verifyUrl.searchParams.set('redirectTo', pathname) | ||
| return NextResponse.redirect(verifyUrl) | ||
| } | ||
| if (!hasMfaFactors) { | ||
| // No factors enrolled → enrollment page | ||
| const enrollUrl = new URL('/auth/mfa-enroll', request.url) | ||
| enrollUrl.searchParams.set('redirectTo', pathname) | ||
| return NextResponse.redirect(enrollUrl) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
MFA bypass when aal cannot be decoded.
updateSession swallows JWT-decode errors and returns aal: null. When MFA_REQUIRED=true, hasMfaFactors=true, and aal is null, the first branch (aal === 'aal1') is false and the second branch (!hasMfaFactors) is also false — so the request proceeds without MFA. A user with enrolled factors can therefore reach protected routes at less than aal2 if the access token decode failed for any reason.
Invert the predicate to fail-closed: redirect whenever aal !== 'aal2'.
🛡️ Suggested fix
- if (hasMfaFactors && aal === 'aal1') {
+ if (hasMfaFactors && aal !== 'aal2') {
// Has factors but hasn't verified yet → verify page
const verifyUrl = new URL('/auth/mfa-verify', request.url)
verifyUrl.searchParams.set('redirectTo', pathname)
return NextResponse.redirect(verifyUrl)
}📝 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.
| // MFA enforcement (only when MFA_REQUIRED is true) | |
| const mfaRequired = process.env.MFA_REQUIRED === 'true' | |
| if (mfaRequired) { | |
| const isMfaExempt = mfaExemptRoutes.some((route) => pathname.startsWith(route)) | |
| if (!isMfaExempt) { | |
| if (hasMfaFactors && aal === 'aal1') { | |
| // Has factors but hasn't verified yet → verify page | |
| const verifyUrl = new URL('/auth/mfa-verify', request.url) | |
| verifyUrl.searchParams.set('redirectTo', pathname) | |
| return NextResponse.redirect(verifyUrl) | |
| } | |
| if (!hasMfaFactors) { | |
| // No factors enrolled → enrollment page | |
| const enrollUrl = new URL('/auth/mfa-enroll', request.url) | |
| enrollUrl.searchParams.set('redirectTo', pathname) | |
| return NextResponse.redirect(enrollUrl) | |
| } | |
| } | |
| } | |
| // MFA enforcement (only when MFA_REQUIRED is true) | |
| const mfaRequired = process.env.MFA_REQUIRED === 'true' | |
| if (mfaRequired) { | |
| const isMfaExempt = mfaExemptRoutes.some((route) => pathname.startsWith(route)) | |
| if (!isMfaExempt) { | |
| if (hasMfaFactors && aal !== 'aal2') { | |
| // Has factors but hasn't verified yet → verify page | |
| const verifyUrl = new URL('/auth/mfa-verify', request.url) | |
| verifyUrl.searchParams.set('redirectTo', pathname) | |
| return NextResponse.redirect(verifyUrl) | |
| } | |
| if (!hasMfaFactors) { | |
| // No factors enrolled → enrollment page | |
| const enrollUrl = new URL('/auth/mfa-enroll', request.url) | |
| enrollUrl.searchParams.set('redirectTo', pathname) | |
| return NextResponse.redirect(enrollUrl) | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/admin/src/middleware.ts` around lines 80 - 98, The MFA check currently
allows requests through when aal is null (decode failure); update the logic in
the middleware (the block using mfaRequired, mfaExemptRoutes, hasMfaFactors, and
aal) to fail-closed by treating anything other than aal2 as requiring MFA:
replace the condition that checks for "hasMfaFactors && aal === 'aal1'" with a
predicate that redirects whenever hasMfaFactors is true and aal !== 'aal2' (so
null/undefined also triggers the verify redirect), keeping the existing enroll
redirect when !hasMfaFactors; no other behavior should change and this uses the
same NextResponse.redirect flow and verify/enroll URLs.
| * Rate limited: 5 reveals per minute per user. | ||
| */ | ||
| @Post(':id/reveal') | ||
| @UseGuards(ThrottlerGuard) | ||
| @Throttle({ default: { limit: 5, ttl: 60000 } }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect JWT strategy validate() return shape and AuthContext.
fd -t f 'jwt.strategy.ts' apps/api/src | xargs -I {} sh -c 'echo "=== {} ==="; cat {}'
ast-grep --pattern $'validate($_) {
$$$
}'Repository: Systemsaholic/tailfire
Length of output: 7973
🏁 Script executed:
#!/bin/bash
# Check the api-credentials controller structure and auth setup
fd -t f 'api-credentials.controller.ts' apps/api/src | xargs -I {} sh -c 'echo "=== {} ==="; head -120 {}'Repository: Systemsaholic/tailfire
Length of output: 3440
🏁 Script executed:
#!/bin/bash
# Find AdminOnly decorator and verify which endpoints it applies to
rg -A 10 -B 5 '@AdminOnly\|class ApiCredentialsController' apps/api/src/api-credentials/Repository: Systemsaholic/tailfire
Length of output: 48
🏁 Script executed:
#!/bin/bash
# Check the ThrottlerGuard implementation to confirm it uses IP-based tracking
fd -t f 'throttler.guard.ts' | xargs -I {} sh -c 'echo "=== {} ==="; cat {}'Repository: Systemsaholic/tailfire
Length of output: 48
🏁 Script executed:
#!/bin/bash
# Check if there's a custom throttle guard in the codebase
fd -t f 'throttle' apps/api/src | grep -i guardRepository: Systemsaholic/tailfire
Length of output: 119
🏁 Script executed:
#!/bin/bash
# Check how AdminOnly is implemented and what guards it uses
fd -t f 'admin-only.decorator.ts' apps/api/src | xargs -I {} sh -c 'echo "=== {} ==="; cat {}'Repository: Systemsaholic/tailfire
Length of output: 538
🏁 Script executed:
#!/bin/bash
# Verify NestJS ThrottlerGuard behavior by checking dependencies and documentation in package.json
cd apps/api && cat package.json | grep -A 2 -B 2 throttlerRepository: Systemsaholic/tailfire
Length of output: 234
🏁 Script executed:
#!/bin/bash
# Check the CatalogThrottleGuard implementation to see how custom tracking works
cat apps/api/src/common/guards/catalog-throttle.guard.tsRepository: Systemsaholic/tailfire
Length of output: 1657
🏁 Script executed:
#!/bin/bash
# Verify if req.user is populated with the JWT payload in guards context
fd -t f 'admin-role.guard.ts' apps/api/src | xargs -I {} sh -c 'echo "=== {} ==="; cat {}'Repository: Systemsaholic/tailfire
Length of output: 816
🏁 Script executed:
#!/bin/bash
# Check if there are any other uses of ThrottlerGuard to understand the pattern
rg -B 3 -A 3 '@UseGuards.*ThrottlerGuard|ThrottlerGuard' apps/api/srcRepository: Systemsaholic/tailfire
Length of output: 11251
Rate limit is per-IP, not per-user — the doc comment is inaccurate.
The default ThrottlerGuard tracks requests by client IP. The doc on line 94 says "5 reveals per minute per user," but two admins behind the same office NAT will share the 5/min budget, while a compromised JWT rotated across IPs bypasses the limit. Given this endpoint reveals decrypted secrets, per-user rate limiting is the stronger guarantee.
Two options:
- Fix the doc to reflect actual behavior (per-IP).
- Implement a per-user tracker by subclassing
ThrottlerGuard(similar toCatalogThrottleGuardinapps/api/src/common/guards/catalog-throttle.guard.ts) and overridinggetTracker()to return the authenticated user's ID:
♻️ Sketch of a per-user throttle guard
// apps/api/src/common/guards/user-throttle.guard.ts
import { ExecutionContext, Injectable } from '@nestjs/common'
import { ThrottlerGuard } from '@nestjs/throttler'
`@Injectable`()
export class UserThrottleGuard extends ThrottlerGuard {
protected async getTracker(req: Record<string, any>): Promise<string> {
const userId = req.user?.userId
return userId ? `user:${userId}` : `ip:${req.ip}`
}
}- `@UseGuards`(ThrottlerGuard)
+ `@UseGuards`(UserThrottleGuard)
`@Throttle`({ default: { limit: 5, ttl: 60000 } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/api-credentials/api-credentials.controller.ts` around lines 94 -
98, The doc comment on the reveal endpoint is inaccurate: the current
`@UseGuards`(ThrottlerGuard) + `@Throttle` uses IP-based tracking (not per-user), so
either update the comment to say "5 reveals per minute per IP" or implement a
per-user throttle guard; to implement, add a UserThrottleGuard subclassing
ThrottlerGuard (similar to CatalogThrottleGuard) that overrides getTracker(req)
to return the authenticated user's id (e.g. `user:${req.user.userId}`) falling
back to `ip:${req.ip}` and replace `@UseGuards`(ThrottlerGuard) on the :id/reveal
handler with `@UseGuards`(UserThrottleGuard).
| agencyId: string | ||
| role: 'admin' | 'user' | ||
| userStatus: UserStatus | ||
| aal: 'aal1' | 'aal2' |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find every place an AuthContext is constructed and verify aal is set.
# The `as AuthContext` cast hides missing fields, so we need to inspect literals.
rg -nP --type=ts -C5 'as\s+AuthContext\b'
rg -nP --type=ts -C2 '\brequest\.user\s*=\s*\{'Repository: Systemsaholic/tailfire
Length of output: 8073
🏁 Script executed:
# Check the AuthContext type definition to confirm aal field requirement
cat -n apps/api/src/auth/auth.types.ts | head -30Repository: Systemsaholic/tailfire
Length of output: 804
🏁 Script executed:
# Get complete context around impersonation.guard.ts request.user assignment
cat -n apps/api/src/auth/guards/impersonation.guard.ts | sed -n '110,140p'Repository: Systemsaholic/tailfire
Length of output: 1157
🏁 Script executed:
# Search for usage of user.aal or request.user.aal downstream
rg -n '\buser\.aal\b|\brequest\.user\.aal\b' apps/api/src/Repository: Systemsaholic/tailfire
Length of output: 138
🏁 Script executed:
# Check MfaGuard for how it reads aal
cat -n apps/api/src/auth/guards/mfa.guard.ts | sed -n '35,75p'Repository: Systemsaholic/tailfire
Length of output: 904
🏁 Script executed:
# Check guard order in main.ts or app module setup
rg -n "useGlobalGuards|@UseGuards" apps/api/src/main.ts apps/api/src/app.module.ts apps/api/src/app.controller.ts | head -20Repository: Systemsaholic/tailfire
Length of output: 114
🏁 Script executed:
# Find where guards are registered globally
fd -e ts -e module.ts apps/api/src/ | xargs rg -l "ImpersonationGuard|MfaGuard|JwtAuthGuard" | head -10Repository: Systemsaholic/tailfire
Length of output: 937
🏁 Script executed:
# Check app.module.ts to see guard order
cat -n apps/api/src/app.module.ts 2>/dev/null || echo "Trying alternative paths..." && find apps/api/src -name "*.module.ts" -type f | head -3Repository: Systemsaholic/tailfire
Length of output: 11898
🏁 Script executed:
# Verify what happens at runtime: if user.aal is undefined, does 'undefined !== 'aal2'' throw or pass?
node -e "const u = { aal: undefined }; console.log(u.aal !== 'aal2'); const v = {}; console.log(v.aal !== 'aal2')"Repository: Systemsaholic/tailfire
Length of output: 76
🏁 Script executed:
# Check if targetUser comes from DB with an aal field or if it's missing
rg -n "targetUser\s*=" apps/api/src/auth/guards/impersonation.guard.ts -A3 -B3Repository: Systemsaholic/tailfire
Length of output: 276
🏁 Script executed:
# Check main.ts around line 106 for full guard setup
cat -n apps/api/src/main.ts | sed -n '95,120p'Repository: Systemsaholic/tailfire
Length of output: 1021
🏁 Script executed:
# Find where MfaGuard is applied (route decorator level, not global)
rg -n "@UseGuards.*MfaGuard|MfaGuard\)" apps/api/src/ -A2 -B2 | head -30Repository: Systemsaholic/tailfire
Length of output: 48
🏁 Script executed:
# Check auth.module.ts to see if guards are registered there
cat -n apps/api/src/auth/auth.module.ts 2>/dev/null | head -50Repository: Systemsaholic/tailfire
Length of output: 1830
🏁 Script executed:
# See if targetUser has aal field - check userProfiles schema
rg -n "aal" apps/api/src/auth/guards/impersonation.guard.tsRepository: Systemsaholic/tailfire
Length of output: 48
🏁 Script executed:
# Verify jwt.strategy.ts to confirm aal comes from JWT and default behavior
cat -n apps/api/src/auth/strategies/jwt.strategy.ts | head -60Repository: Systemsaholic/tailfire
Length of output: 2661
🏁 Script executed:
# Check if there are any other files that might read user.aal that we missed
rg -n "\.aal\b" apps/api/src/ --type=tsRepository: Systemsaholic/tailfire
Length of output: 221
Required aal field is missing from impersonated AuthContext.
apps/api/src/auth/guards/impersonation.guard.ts constructs the impersonated request.user without including the aal field, which is required on AuthContext. The as AuthContext cast suppresses the TypeScript error, creating a type contract violation.
While MfaGuard (which checks user.aal !== 'aal2') runs before ImpersonationGuard in the global chain and thus validates the admin's credentials before impersonation occurs, the impersonated user object lacks the required aal property. This creates a latent bug: any code added downstream that reads user.aal after impersonation will encounter undefined.
Propagate the admin's aal into the impersonated context:
Suggested fix in impersonation.guard.ts
request.user = {
userId: targetUser.id,
email: targetUser.email,
agencyId: user.agencyId,
role: targetUser.role,
userStatus: targetUser.status,
+ aal: user.aal,
} as AuthContext📝 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.
| aal: 'aal1' | 'aal2' | |
| request.user = { | |
| userId: targetUser.id, | |
| email: targetUser.email, | |
| agencyId: user.agencyId, | |
| role: targetUser.role, | |
| userStatus: targetUser.status, | |
| aal: user.aal, | |
| } as AuthContext |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/auth/auth.types.ts` at line 15, The impersonated AuthContext
created in ImpersonationGuard omits the required aal field, causing a
type-contract violation and potential runtime undefined reads; update the
impersonation logic in impersonation.guard.ts (the code that sets request.user /
constructs the impersonated AuthContext) to copy the admin's aal value (from the
original request.user or admin context validated by MfaGuard) into the new
impersonated object so that the resulting request.user implements
AuthContext.aal (e.g., ensure the impersonated context includes aal:
adminUser.aal) and remove any unsafe casts that hide the missing property.
useSearchParams requires Suspense wrapper in Next.js 15 App Router. Split MFA pages into wrapper (default export) + content component. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 participant
Summary
Production readiness: security hardening + two-factor authentication for all admin users.
Phase 1: Security Hardening
console.*with NestJS Logger in bootstrapsendDefaultPii: falsein Sentry (stops capturing IP/cookies)Phase 2: MFA Frontend
useMfahook wrapping Supabasemfa.*APIs/auth/mfa-verifypage — TOTP code entry with Phoenix Voyages branding/auth/mfa-enrollpage — QR code setup flow (scan, verify, complete)MFA_REQUIREDfeature flag (off by default = grace period)aal+mfaEnrolled, moves record-login to post-aal2Phase 3: MFA API Enforcement
MfaGuard: rejects aal1 tokens whenMFA_REQUIRED=true@BypassMfa()decorator for MFA management endpointsaaladded toAuthContext+JwtPayloadtypes + JWT strategy extractionEnv Vars (new)
MFA_REQUIRED— server-side enforcement (API + middleware)NEXT_PUBLIC_MFA_REQUIRED— client-side displayfalse(grace period)Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Security
Improvements