Feat/settings security tab #29
Conversation
- Add immediate logout on production digest errors and repeated token failures - Create secure MFA factor deletion with TOTP verification - Add DeleteMfaFactorDialog component with proper validation - Remove unused MFA challenge API endpoint - Refactor MFA APIs to use NextResponse.json directly - Harden logout-all route to handle already-ended sessions gracefully - Add 401 redirect handling in entitlements API - Strengthen ConvexClientProvider with fallback logout on repeated failures
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughRefactors entitlements API to use shared JSON helpers and organization-scoped refresh; adds MFA endpoints (enroll/verify/list/delete), a logout-all endpoint, new Security UI (tabs and dialogs), client auth failure handling, a Convex token logout counter, minor provider and styling tweaks. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as SecurityTab / SettingsDialog
participant EnrollAPI as POST /api/mfa/enroll
participant VerifyAPI as POST /api/mfa/verify
participant WorkOS as WorkOS
User->>UI: Enable MFA
UI->>EnrollAPI: POST enroll (userId)
EnrollAPI->>WorkOS: enrollAuthFactor (TOTP)
WorkOS-->>EnrollAPI: { factor, challenge }
EnrollAPI-->>UI: { factor, challenge }
UI->>User: Show QR/manual + code input
User->>UI: Enter 6-digit code
UI->>VerifyAPI: POST verify { challengeId, code }
VerifyAPI->>WorkOS: verifyChallenge
WorkOS-->>VerifyAPI: { valid, challenge }
VerifyAPI-->>UI: { valid }
UI->>UI: Close dialog, refresh factors
sequenceDiagram
autonumber
actor User
participant UI as SecurityTab
participant DeleteAPI as POST /api/mfa/delete
participant FactorsAPI as GET /api/mfa/factors
participant WorkOS as WorkOS
User->>UI: Delete MFA factor
UI->>DeleteAPI: POST { factorId, code }
DeleteAPI->>WorkOS: createChallenge -> verifyChallenge -> deleteFactor
WorkOS-->>DeleteAPI: success
DeleteAPI-->>UI: { success }
UI->>FactorsAPI: GET
FactorsAPI->>WorkOS: listAuthFactors(userId)
WorkOS-->>FactorsAPI: factors
FactorsAPI-->>UI: { factors }
sequenceDiagram
autonumber
actor User
participant UI as SecurityTab
participant LogoutAllAPI as POST /api/logout-all
participant WorkOS as WorkOS
User->>UI: Log out of all devices
UI->>LogoutAllAPI: POST
LogoutAllAPI->>WorkOS: listSessions(userId)
loop sessions
LogoutAllAPI->>WorkOS: revokeSession(sessionId)
WorkOS-->>LogoutAllAPI: ok / already-ended
end
LogoutAllAPI-->>UI: { revokedSessions }
UI->>User: show toast + redirect to /logout
sequenceDiagram
autonumber
participant App as GlobalState / ConvexClientProvider
participant EntAPI as GET /api/entitlements
participant Auth as AuthKit / WorkOS
participant Browser as window
App->>EntAPI: GET entitlements
EntAPI->>Auth: validate/refresh session (org-scoped)
alt valid
Auth-->>EntAPI: entitlements
EntAPI-->>App: { entitlements, hasProPlan }
else unauthorized / invalid_grant
EntAPI-->>App: 401 / error digest
App->>Browser: redirect /logout
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/contexts/GlobalState.tsx (1)
50-53: Use Id<'chats'> instead of string for chat IDs (per coding guidelines).-import type { Doc } from "@/convex/_generated/dataModel"; +import type { Doc, Id } from "@/convex/_generated/dataModel"; ... - currentChatId: string | null; - setCurrentChatId: (chatId: string | null) => void; + currentChatId: Id<"chats"> | null; + setCurrentChatId: (chatId: Id<"chats"> | null) => void; ... - initializeChat: (chatId: string, fromRoute?: boolean) => void; + initializeChat: (chatId: Id<"chats">, fromRoute?: boolean) => void; ... - const initializeChat = useCallback((chatId: string, _fromRoute?: boolean) => { + const initializeChat = useCallback((chatId: Id<"chats">, _fromRoute?: boolean) => { ... - const activateChat = useCallback((chatId: string) => { + const activateChat = useCallback((chatId: Id<"chats">) => {Also applies to: 98-101, 275-279
🧹 Nitpick comments (34)
lib/ai/providers.ts (1)
39-55: Tighten the tracked models type (avoidany).Use the provider’s model type instead of
Record<string, any>to keep type safety.- const trackedModels: Record<string, any> = {}; + const trackedModels: Record<string, (typeof baseProviders)[keyof typeof baseProviders]> = {};app/contexts/GlobalState.tsx (2)
186-195: MoveclearTimeouttofinallyto avoid timer leaks on thrown/aborted fetch.- const response = await fetch("/api/entitlements", { - credentials: "include", - signal: controller.signal, - }); - - clearTimeout(timeoutId); + const response = await fetch("/api/entitlements", { + credentials: "include", + signal: controller.signal, + });And in the existing
finallyblock:} finally { + clearTimeout(timeoutId); setIsCheckingProPlan(false); // Remove the refresh param to avoid repeated refreshes
206-211: Don’t downgrade pro state on transient/network failures.Instead of forcing
hasProPlanto false on non-OK or catch, consider preserving prior state to reduce UI flicker.- } else { + } else { if (response.status === 401) { if (typeof window !== "undefined") { window.location.href = "/logout"; return; } } - setHasProPlan(false); + // Preserve prior state on non-401 failures } } catch { - setHasProPlan(false); + // Preserve prior state on transient errorslib/api/response.ts (2)
3-10: Clarify whetherCache-Controlshould be enforced or overridable.Currently,
init.headersoverride the default. If you want to always prevent caching, put"Cache-Control": "no-store"last.- NextResponse.json(data, { - ...init, - headers: { - "Cache-Control": "no-store", - ...(init?.headers || {}), - }, - }); + NextResponse.json(data, { + ...init, + headers: { + ...(init?.headers || {}), + "Cache-Control": "no-store", + }, + });
21-29: Deduplicate unauthorized detection across server and client.
isUnauthorizedErroris needed in the client (ConvexClientProvider) too; consider moving it to a server-agnostic module (e.g.,lib/errors.ts) and reusing it.Additional file (new):
// lib/errors.ts export const extractErrorMessage = (err: unknown): string => { if (typeof err === "string") return err; if (err && typeof err === "object" && "message" in err) { // eslint-disable-next-line @typescript-eslint/no-explicit-any return (err as any).message ?? ""; } return ""; }; export const isUnauthorizedError = (err: unknown): boolean => { const normalized = extractErrorMessage(err).toLowerCase(); return ( normalized.includes("invalid_grant") || normalized.includes("session has already ended") || normalized.includes("no session cookie") || normalized.includes("unauthorized") ); };Then here keep only
jsonor re-export fromlib/errorsto avoid duplicating logic.components/ui/switch.tsx (1)
16-16: Dark checked state looks fine; verify contrast.Ensure
dark:bg-blue-400meets contrast for the thumb/track in dark mode.components/ConvexClientProvider.tsx (3)
20-23: Guard missing Convex URL at runtime.Fail fast with a clear error if
NEXT_PUBLIC_CONVEX_URLis unset.- const [convex] = useState(() => { - return new ConvexReactClient(process.env.NEXT_PUBLIC_CONVEX_URL!, { + const [convex] = useState(() => { + const url = process.env.NEXT_PUBLIC_CONVEX_URL; + if (!url) throw new Error("NEXT_PUBLIC_CONVEX_URL is not set"); + return new ConvexReactClient(url, { expectAuth, }); });
76-90: Fix boolean logic and make the failure window explicit.Avoid
&&returning a number; compute a strict boolean and keep the window constant named.- const now = Date.now(); - const withinWindow = lastFailureAtRef.current && now - lastFailureAtRef.current < 10000; // 10s + const now = Date.now(); + const WINDOW_MS = 10_000; + const withinWindow = + lastFailureAtRef.current !== null && + now - lastFailureAtRef.current < WINDOW_MS;
39-53: Reuse shared unauthorized check instead of duplicatingshouldForceLogout.Import a shared
isUnauthorizedError(see suggestion inlib/api/response.ts) for consistency.- const shouldForceLogout = (value: unknown): boolean => { - const message = - typeof value === "string" - ? value - : value && typeof value === "object" && "message" in value - ? // eslint-disable-next-line @typescript-eslint/no-explicit-any - (value as any).message - : undefined; - if (!message) return false; - const normalized = String(message).toLowerCase(); - if (normalized.includes("invalid_grant")) return true; - if (normalized.includes("session has already ended")) return true; - if (normalized.includes("failed to refresh session")) return true; - return false; - }; + // import { isUnauthorizedError } from "@/lib/errors";And in the catch:
- if ((shouldForceLogout(error) || hasDigest) && typeof window !== "undefined") { + if ((isUnauthorizedError(error) || hasDigest) && typeof window !== "undefined") {app/api/entitlements/route.ts (4)
83-91: Deduplicate error mapping using shared isUnauthorizedError()Leverage the existing helper instead of re‑implementing string checks here.
- const normalized = extractErrorMessage(error).toLowerCase(); - const should401 = - normalized.includes("invalid_grant") || - normalized.includes("session has already ended"); - - return json( - { error: should401 ? "Unauthorized" : "Failed to refresh session" }, - { status: should401 ? 401 : 500 }, - ); + const should401 = (await import("@/lib/api/response")).isUnauthorizedError(error); + return json( + { error: should401 ? "Unauthorized" : "Failed to refresh session" }, + { status: should401 ? 401 : 500 }, + );
27-57: Short‑circuit unauthenticated sessions before refreshIf authenticate() returns unauthenticated, return 401 early instead of relying on refresh() to fail.
- if (authResult.authenticated) { + if (!authResult.authenticated) { + return json({ error: "Unauthorized" }, { status: 401 }); + } + + if (authResult.authenticated) {
64-70: Avoid any; type the refresh result minimallyRemove any casts and harden entitlements typing.
- const { sealedSession, entitlements } = refreshResult as any; - - const hasProPlan = (entitlements || []).includes("pro-monthly-plan"); - - // Create response with entitlements - const response = json({ entitlements: entitlements || [], hasProPlan }); + type RefreshResult = { sealedSession?: string; entitlements?: Array<string> }; + const { sealedSession, entitlements }: RefreshResult = refreshResult as RefreshResult; + const entitlementsList = Array.isArray(entitlements) ? entitlements : []; + const hasProPlan = entitlementsList.includes("pro-monthly-plan"); + const response = json({ entitlements: entitlementsList, hasProPlan });
73-78: Set cookie path for consistent scopeExplicitly set path: "/" to ensure the cookie is visible across routes.
response.cookies.set("wos-session", sealedSession, { httpOnly: true, sameSite: "lax", secure: true, + path: "/", });app/components/SettingsDialog.tsx (2)
21-22: Strongly type tabs and activeTab (use union and as const)Prevents typos, improves DX, and aligns with guidelines for discriminated unions and explicit generics.
- const [activeTab, setActiveTab] = useState("Personalization"); + const TABS = [ + { id: "Personalization", label: "Personalization", icon: Settings }, + { id: "Security", label: "Security", icon: Shield }, + ] as const; + type TabId = typeof TABS[number]["id"]; + const [activeTab, setActiveTab] = useState<TabId>(TABS[0].id); @@ - const tabs = [ - { id: "Personalization", label: "Personalization", icon: Settings }, - { id: "Security", label: "Security", icon: Shield }, - ]; + const tabs = TABS; @@ - {tabs.map((tab) => { + {tabs.map((tab: typeof TABS[number]) => {Also applies to: 34-37, 88-109
75-113: Add ARIA roles/attributes for tabsImprove accessibility by using role="tablist"/"tab" and aria-selected/aria-controls.
- {/* Tabs */} + {/* Tabs */} <div className={`${isMobile ? "overflow-x-auto md:overflow-x-visible border-r pb-2 md:pb-0 relative" : "md:w-[221px] border-r"}`} > @@ - <div className="flex-1 flex-shrink-0 flex items-start self-stretch px-3 overflow-auto w-max md:w-full pb-0 border-b md:border-b-0 md:flex-col md:gap-3 md:px-2 max-md:gap-2.5"> + <div role="tablist" aria-orientation={isMobile ? "horizontal" : "vertical"} className="flex-1 flex-shrink-0 flex items-start self-stretch px-3 overflow-auto w-max md:w-full pb-0 border-b md:border-b-0 md:flex-col md:gap-3 md:px-2 max-md:gap-2.5"> @@ - <button + <button + role="tab" + aria-selected={activeTab === tab.id} + aria-controls={`panel-${tab.id}`} key={tab.id} type="button" onClick={() => setActiveTab(tab.id)} @@ - <div className="flex-1 self-stretch items-start overflow-y-auto px-4 pt-4 pb-4 md:px-6 md:pt-4"> + <div className="flex-1 self-stretch items-start overflow-y-auto px-4 pt-4 pb-4 md:px-6 md:pt-4"> {activeTab === "Personalization" && ( - <div className="space-y-6"> + <div id="panel-Personalization" role="tabpanel" className="space-y-6"> @@ - {activeTab === "Security" && <SecurityTab />} + {activeTab === "Security" && ( + <div id="panel-Security" role="tabpanel"> + <SecurityTab /> + </div> + )}Also applies to: 121-189
app/api/mfa/delete/route.ts (2)
35-37: Validate 6‑digit numeric OTP (not just length)Prevent non‑numeric codes from hitting WorkOS.
- if (code.length !== 6) { - return NextResponse.json({ error: "Invalid code" }, { status: 400 }); - } + if (!/^\d{6}$/.test(code)) { + return NextResponse.json({ error: "Invalid code" }, { status: 400 }); + }
1-19: Standardize responses via json() helperOther routes (e.g., entitlements) use json() to set no‑store. Mirror that here for consistency.
-import { NextRequest, NextResponse } from "next/server"; +import { NextRequest, NextResponse } from "next/server"; +import { json } from "@/lib/api/response"; @@ - return NextResponse.json({ error: status === 401 ? "Unauthorized" : "Failed to remove MFA factor" }, { status }); + return json({ error: status === 401 ? "Unauthorized" : "Failed to remove MFA factor" }, { status }); @@ - return NextResponse.json({ error: "Unauthorized" }, { status: 401 }); + return json({ error: "Unauthorized" }, { status: 401 }); @@ - return NextResponse.json({ error: "Invalid JSON" }, { status: 400 }); + return json({ error: "Invalid JSON" }, { status: 400 }); @@ - return NextResponse.json({ error: "factorId and code are required" }, { status: 400 }); + return json({ error: "factorId and code are required" }, { status: 400 }); @@ - return NextResponse.json({ error: "Invalid code" }, { status: 400 }); + return json({ error: "Invalid code" }, { status: 400 }); @@ - return NextResponse.json({ error: "Factor not found" }, { status: 404 }); + return json({ error: "Factor not found" }, { status: 404 }); @@ - return NextResponse.json({ error: "Invalid verification code" }, { status: 400 }); + return json({ error: "Invalid verification code" }, { status: 400 }); @@ - return NextResponse.json({ success: true }); + return json({ success: true }); @@ - return NextResponse.json({ error: "Challenge has expired" }, { status: 400 }); + return json({ error: "Challenge has expired" }, { status: 400 }); @@ - return NextResponse.json( + return json( { error: status === 401 ? "Unauthorized" : "Failed to remove MFA factor" }, { status }, );Also applies to: 63-74
app/api/mfa/factors/route.ts (2)
1-15: Standardize responses via json() helperUse the shared helper for GET/DELETE responses for consistent no‑store caching.
Example changes:
-import { NextRequest, NextResponse } from "next/server"; +import { NextRequest, NextResponse } from "next/server"; +import { json } from "@/lib/api/response"; @@ - return NextResponse.json({ error: status === 401 ? "Unauthorized" : "Failed to get MFA factors" }, { status }); + return json({ error: status === 401 ? "Unauthorized" : "Failed to get MFA factors" }, { status }); @@ - return NextResponse.json({ + return json({ factors: transformedFactors, }); @@ - return NextResponse.json( + return json( { error: status === 401 ? "Unauthorized" : "Failed to get MFA factors" }, { status }, );Also applies to: 32-35, 38-41
22-31: PII surface check: confirm necessity of issuer/user fieldsEnsure returning issuer and user is necessary for the UI; otherwise, omit to minimize exposure.
app/api/mfa/enroll/route.ts (1)
1-4: Standardize responses via json() helperAlign with lib/api/response.json to set Cache-Control: no-store automatically.
-import { NextRequest, NextResponse } from "next/server"; +import { NextRequest, NextResponse } from "next/server"; +import { json } from "@/lib/api/response"; @@ - return NextResponse.json({ error: status === 401 ? "Unauthorized" : "Failed to enroll MFA factor" }, { status }); + return json({ error: status === 401 ? "Unauthorized" : "Failed to enroll MFA factor" }, { status }); @@ - return NextResponse.json({ error: "Unauthorized" }, { status: 401 }); + return json({ error: "Unauthorized" }, { status: 401 }); @@ - return NextResponse.json({ + return json({ factor: { id: result.authenticationFactor.id, type: result.authenticationFactor.type, qrCode: result.authenticationFactor.totp?.qrCode, secret: result.authenticationFactor.totp?.secret, issuer: result.authenticationFactor.totp?.issuer, user: result.authenticationFactor.totp?.user, }, challenge: { id: result.authenticationChallenge.id, expiresAt: result.authenticationChallenge.expiresAt, }, }); @@ - return NextResponse.json( + return json( { error: status === 401 ? "Unauthorized" : "Failed to enroll MFA factor" }, { status }, );Also applies to: 14-19, 28-43, 45-49
app/api/logout-all/route.ts (1)
36-36: Consider using Promise.all instead of Promise.allSettled for better error propagationUsing
Promise.allSettledwill silently ignore errors that aren't caught in the individual promises. Since you're already handling the specific errors you want to ignore,Promise.allwould be more appropriate to ensure any unexpected errors are properly propagated.- await Promise.allSettled(revokePromises); + await Promise.all(revokePromises);app/api/mfa/verify/route.ts (2)
9-19: Simplify error handling for getUserIDThe nested try-catch and subsequent null check for userId are redundant since getUserID throws on failure.
- // Get authenticated user - let userId: string; - try { - userId = await getUserID(req); - } catch (e) { - const status = isUnauthorizedError(e) ? 401 : 500; - return NextResponse.json({ error: status === 401 ? "Unauthorized" : "Failed to verify MFA challenge" }, { status }); - } - - if (!userId) { - return NextResponse.json({ error: "Unauthorized" }, { status: 401 }); - } + // Get authenticated user + let userId: string; + try { + userId = await getUserID(req); + } catch (e) { + const status = isUnauthorizedError(e) ? 401 : 500; + return NextResponse.json({ error: status === 401 ? "Unauthorized" : "Failed to verify MFA challenge" }, { status }); + }
51-64: Use WorkOS MFA error codes instead of string-matching error messagesWorkOS returns explicit MFA error codes (e.g. authentication_challenge_previously_verified, authentication_challenge_expired) — check those instead of doing error.message.includes(...). (workos.com)
- Replace the message-based checks in app/api/mfa/verify/route.ts (lines 51–64) to inspect error.code (or error.response?.data?.code if the SDK wraps the API response); apply the same change in app/api/mfa/delete/route.ts where message-matching is used.
app/components/DeleteMfaFactorDialog.tsx (3)
85-85: Add pattern attribute for better mobile UXAdding a pattern attribute will improve the mobile experience by ensuring the numeric keyboard shows the correct layout.
- onChange={(e) => setCode(e.target.value.replace(/\D/g, ""))} + onChange={(e) => setCode(e.target.value.replace(/\D/g, ""))} + pattern="[0-9]*"
59-61: Consider using router.push instead of window.location.hrefUsing Next.js router would provide better navigation handling and maintain SPA behavior.
Add at the top of the component:
import { useRouter } from "next/navigation";Inside the component:
const router = useRouter();Then apply this diff:
- if (response.status === 401) { - window.location.href = "/logout"; - } + if (response.status === 401) { + router.push("/logout"); + }
70-70: Fix dialog onOpenChange handlerThe Dialog's
onOpenChangeprop should receive the function directly, not callhandleClose.- <Dialog open={open} onOpenChange={handleClose}> + <Dialog open={open} onOpenChange={onOpenChange}>app/components/MfaVerificationDialog.tsx (4)
47-50: Validate code length consistentlyThe validation checks for trimmed code but doesn't validate the length to be exactly 6 characters like other validations in the codebase.
- if (!enrollmentData?.challenge.id || !verificationCode.trim()) { - toast.error("Please enter the verification code"); + if (!enrollmentData?.challenge.id || verificationCode.trim().length !== 6) { + toast.error("Please enter a valid 6-digit verification code");
166-167: Add input restrictions for verification codeThe input should restrict to numeric values only, similar to DeleteMfaFactorDialog.
value={verificationCode} - onChange={(e) => setVerificationCode(e.target.value)} + onChange={(e) => setVerificationCode(e.target.value.replace(/\D/g, ""))} + inputMode="numeric" + pattern="[0-9]*"
133-137: Add success feedback for copy actionWhen copying the secret code, users should receive feedback that the copy was successful.
onClick={() => { if (enrollmentData?.factor.secret) { navigator.clipboard.writeText( enrollmentData.factor.secret, ); + toast.success("Code copied to clipboard"); } }}
74-74: Avoid catching errors without proper type checkingThe catch block catches any error but doesn't handle it specifically. Consider logging for debugging.
- } catch (error) { + } catch (error) { + console.error("MFA verification error:", error); toast.error("Failed to verify code");app/components/SecurityTab.tsx (4)
113-114: Use Next.js router for consistent navigationUsing
window.location.hreffor navigation bypasses Next.js routing. Consider using the Next.js router for better SPA behavior.Add at the top:
import { useRouter } from "next/navigation";In the component:
const router = useRouter();Then update both logout handlers:
- window.location.href = "/logout"; + await router.push("/logout");
140-142: Remove unused formatDate functionThe
formatDatefunction is defined but never used in the component.- const formatDate = (dateString: string) => { - return new Date(dateString).toLocaleDateString(); - };
10-16: Type document IDs using Id helper typeAccording to the coding guidelines, document IDs should be typed using
Id<'table'>instead of string.interface MfaFactor { - id: string; + id: Id<'mfa_factors'>; issuer?: string; user?: string; createdAt: string; updatedAt: string; }Note: Adjust the table name (
'mfa_factors') based on your actual Convex data model.
33-33: Explicitly type the array declarationAccording to the coding guidelines, arrays should be declared with explicit generic types.
- const [factors, setFactors] = useState<MfaFactor[]>([]); + const [factors, setFactors] = useState<Array<MfaFactor>>([]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
app/api/entitlements/route.ts(4 hunks)app/api/logout-all/route.ts(1 hunks)app/api/mfa/delete/route.ts(1 hunks)app/api/mfa/enroll/route.ts(1 hunks)app/api/mfa/factors/route.ts(1 hunks)app/api/mfa/verify/route.ts(1 hunks)app/components/DeleteMfaFactorDialog.tsx(1 hunks)app/components/MfaVerificationDialog.tsx(1 hunks)app/components/SecurityTab.tsx(1 hunks)app/components/SettingsDialog.tsx(3 hunks)app/contexts/GlobalState.tsx(1 hunks)components/ConvexClientProvider.tsx(3 hunks)components/ui/switch.tsx(1 hunks)lib/ai/providers.ts(1 hunks)lib/api/response.ts(1 hunks)lib/auth/get-user-id.ts(0 hunks)
💤 Files with no reviewable changes (1)
- lib/auth/get-user-id.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/convex_rules.mdc)
**/*.{ts,tsx}: Use Id helper type from ./_generated/dataModel to type document IDs (e.g., Id<'users'>) instead of string
When defining Record types, specify key and value types matching validators (e.g., Record<Id<'users'>, string>)
Be strict with types for document IDs; prefer Id<'table'> over string in function args and variables
Useas constfor string literals in discriminated unions
Declare arrays with explicit generic type: const arr: Array = [...]
Declare records with explicit generic types: const record: Record<KeyType, ValueType> = {...}
Files:
app/api/mfa/verify/route.tsapp/api/mfa/delete/route.tslib/api/response.tsapp/components/SecurityTab.tsxapp/components/DeleteMfaFactorDialog.tsxapp/components/SettingsDialog.tsxapp/api/logout-all/route.tsapp/api/mfa/enroll/route.tscomponents/ui/switch.tsxapp/contexts/GlobalState.tsxcomponents/ConvexClientProvider.tsxapp/components/MfaVerificationDialog.tsxlib/ai/providers.tsapp/api/entitlements/route.tsapp/api/mfa/factors/route.ts
🧬 Code graph analysis (8)
app/api/mfa/verify/route.ts (3)
lib/auth/get-user-id.ts (1)
getUserID(13-31)lib/api/response.ts (1)
isUnauthorizedError(21-29)app/api/workos.ts (1)
workos(7-7)
app/api/mfa/delete/route.ts (4)
app/api/mfa/enroll/route.ts (1)
POST(6-51)app/api/mfa/verify/route.ts (1)
POST(6-72)lib/auth/get-user-id.ts (1)
getUserID(13-31)lib/api/response.ts (1)
isUnauthorizedError(21-29)
app/components/DeleteMfaFactorDialog.tsx (4)
components/ui/dialog.tsx (4)
Dialog(133-133)DialogContent(135-135)DialogTitle(141-141)DialogDescription(136-136)components/ui/label.tsx (1)
Label(26-26)components/ui/input.tsx (1)
Input(21-21)components/ui/button.tsx (1)
Button(59-59)
app/components/SettingsDialog.tsx (1)
app/components/SecurityTab.tsx (1)
SecurityTab(235-235)
app/api/logout-all/route.ts (2)
lib/auth/get-user-id.ts (1)
getUserID(13-31)app/api/workos.ts (1)
workos(7-7)
app/api/mfa/enroll/route.ts (2)
lib/auth/get-user-id.ts (1)
getUserID(13-31)lib/api/response.ts (1)
isUnauthorizedError(21-29)
app/api/entitlements/route.ts (1)
lib/api/response.ts (2)
json(3-10)extractErrorMessage(12-19)
app/api/mfa/factors/route.ts (2)
lib/auth/get-user-id.ts (1)
getUserID(13-31)lib/api/response.ts (1)
isUnauthorizedError(21-29)
🔇 Additional comments (3)
app/contexts/GlobalState.tsx (1)
200-205: LGTM on 401 redirect path.Early return avoids incorrectly flipping
hasProPlanto false;finallywill still clean URL/search params.lib/ai/providers.ts (1)
8-9: Confirm OpenRouter availability for the default model slugWeb checks show deepseek/deepseek-chat-v3-0324 is a real DeepSeek release but OpenRouter lists deepseek-ai/deepseek-chat-v3-0324 as unavailable — verify this slug is enabled for your OpenRouter org before merging. If unavailable, update the default in lib/ai/providers.ts (lines 8–9) to an OpenRouter-accessible slug (e.g., the canonical deepseek-chat or your host's supported DeepSeek V3).
app/api/logout-all/route.ts (1)
11-11: Verified — workos.userManagement.listSessions(userId) is correct. Matches WorkOS Sessions docs and Node example (accepts a user ID, returns a paginated list of session objects).
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
app/api/mfa/factors/route.ts (1)
1-48: Confirm DELETE is removed/disabled on this route.Prior feedback flagged an unsafe DELETE that allowed factor removal without OTP. Ensure it’s gone or returns 405 in favor of
/api/mfa/delete.#!/bin/bash # Verify no DELETE handler remains for factors route rg -nP 'export\s+async\s+function\s+DELETE' app/api/mfa/factors/route.tscomponents/ConvexClientProvider.tsx (1)
71-76: Failure counter reset on successful token retrieval looks good.The implementation correctly resets the failure tracking state when tokens are successfully obtained, addressing the previous concern about persistent failure counts.
Also applies to: 79-84
🧹 Nitpick comments (8)
app/api/mfa/factors/route.ts (3)
22-35: Limit response to non-PII fields (avoid leaking factor.user).Exposing
totp.useris redundant for the authenticated user and may leak provider-side identifiers. Recommend omitting it.Apply this diff:
- const transformedFactors = factors.data.map((factor) => ({ + const transformedFactors = factors.data.map((factor) => ({ id: factor.id, type: factor.type, issuer: factor.totp?.issuer, - user: factor.totp?.user, createdAt: factor.createdAt, updatedAt: factor.updatedAt, }));
37-39: Disable caching of sensitive account data.Add
Cache-Control: no-storeto prevent intermediaries from caching MFA metadata.- return NextResponse.json({ - factors: transformedFactors, - }); + return NextResponse.json( + { factors: transformedFactors }, + { headers: { "Cache-Control": "no-store" } }, + );
6-20: Optional: Use shared JSON/response helper for consistency.Other routes use the shared helpers from
lib/api/response.ts. Consider aligning here for uniform behavior and easier changes later.app/api/mfa/verify/route.ts (3)
28-30: Remove redundant userId null check.
getUserIDeither throws or returns a string; this guard is unreachable.- if (!userId) { - return NextResponse.json({ error: "Unauthorized" }, { status: 401 }); - }
60-74: Make provider error checks case-insensitive.Avoids brittle matching against message casing.
- if (error instanceof Error) { - if (error.message.includes("already verified")) { + if (error instanceof Error) { + const msg = error.message.toLowerCase(); + if (msg.includes("already verified")) { return NextResponse.json( { error: "Challenge has already been verified" }, { status: 400 }, ); } - if (error.message.includes("expired")) { + if (msg.includes("expired")) { return NextResponse.json( { error: "Challenge has expired" }, { status: 400 }, ); } }
6-9: Nit: avoid unnecessary type assertion on destructure.
bodyis already typed; no need foras VerifyMfaRequestduring destructuring.- const { challengeId, code } = body as VerifyMfaRequest; + const { challengeId, code } = body;Also applies to: 32-39
components/ConvexClientProvider.tsx (2)
39-53: Consider extracting error messages to constants for maintainability.The hardcoded error strings in
shouldForceLogoutcould be centralized as constants to improve maintainability and ensure consistency across the codebase.+const FORCE_LOGOUT_ERROR_PATTERNS = [ + 'invalid_grant', + 'session has already ended', + 'failed to refresh session' +] as const; + const shouldForceLogout = (value: unknown): boolean => { const message = typeof value === "string" ? value : value && typeof value === "object" && "message" in value ? // eslint-disable-next-line @typescript-eslint/no-explicit-any (value as any).message : undefined; if (!message) return false; const normalized = String(message).toLowerCase(); - if (normalized.includes("invalid_grant")) return true; - if (normalized.includes("session has already ended")) return true; - if (normalized.includes("failed to refresh session")) return true; - return false; + return FORCE_LOGOUT_ERROR_PATTERNS.some(pattern => + normalized.includes(pattern) + ); };
86-90: Consider using a type guard for better type safety.The digest check could be improved with a proper type guard for better type safety.
+const hasErrorDigest = (error: unknown): error is { digest: unknown } => { + return !!( + error && + typeof error === "object" && + "digest" in error + ); +}; + // In fetchAccessToken: -const hasDigest = !!( - error && - typeof error === "object" && - "digest" in (error as Record<string, unknown>) -); +const hasDigest = hasErrorDigest(error);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
app/api/logout-all/route.ts(1 hunks)app/api/mfa/delete/route.ts(1 hunks)app/api/mfa/enroll/route.ts(1 hunks)app/api/mfa/factors/route.ts(1 hunks)app/api/mfa/verify/route.ts(1 hunks)app/components/DeleteMfaFactorDialog.tsx(1 hunks)app/components/SecurityTab.tsx(1 hunks)components/ConvexClientProvider.tsx(3 hunks)lib/api/response.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- lib/api/response.ts
- app/components/SecurityTab.tsx
- app/api/logout-all/route.ts
- app/api/mfa/enroll/route.ts
- app/api/mfa/delete/route.ts
- app/components/DeleteMfaFactorDialog.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/convex_rules.mdc)
**/*.{ts,tsx}: Use Id helper type from ./_generated/dataModel to type document IDs (e.g., Id<'users'>) instead of string
When defining Record types, specify key and value types matching validators (e.g., Record<Id<'users'>, string>)
Be strict with types for document IDs; prefer Id<'table'> over string in function args and variables
Useas constfor string literals in discriminated unions
Declare arrays with explicit generic type: const arr: Array = [...]
Declare records with explicit generic types: const record: Record<KeyType, ValueType> = {...}
Files:
app/api/mfa/verify/route.tsapp/api/mfa/factors/route.tscomponents/ConvexClientProvider.tsx
🧬 Code graph analysis (2)
app/api/mfa/verify/route.ts (4)
app/api/mfa/delete/route.ts (1)
POST(11-97)app/api/mfa/enroll/route.ts (1)
POST(6-59)lib/auth/get-user-id.ts (1)
getUserID(13-31)lib/api/response.ts (1)
isUnauthorizedError(21-29)
app/api/mfa/factors/route.ts (2)
lib/auth/get-user-id.ts (1)
getUserID(13-31)lib/api/response.ts (1)
isUnauthorizedError(21-29)
🔇 Additional comments (2)
app/api/mfa/verify/route.ts (1)
53-56: LGTM: clear, minimal success payload.Returning
{ valid, challenge }aligns with the client dialog flow.components/ConvexClientProvider.tsx (1)
100-106: Time-window logic validated — no change needed.
Successful token fetch resets failureCountRef/lastFailureAtRef (components/ConvexClientProvider.tsx:73–74, 81–82); two failures within 10s trigger logout (components/ConvexClientProvider.tsx:102–107).
| let body: VerifyMfaRequest; | ||
| try { | ||
| body = await req.json(); | ||
| } catch { | ||
| return NextResponse.json({ error: "Invalid JSON" }, { status: 400 }); | ||
| } | ||
| const { challengeId, code } = body as VerifyMfaRequest; | ||
|
|
There was a problem hiding this comment.
Validate and normalize OTP code (length/digits) before verification.
Prevents malformed input and reduces noisy provider calls; aligns with delete flow.
- let body: VerifyMfaRequest;
+ let body: VerifyMfaRequest;
try {
body = await req.json();
} catch {
return NextResponse.json({ error: "Invalid JSON" }, { status: 400 });
}
- const { challengeId, code } = body as VerifyMfaRequest;
+ const { challengeId, code } = body;
- if (!challengeId || !code) {
+ const normalizedCode = (code ?? "").toString().trim().replace(/\s+/g, "");
+ if (!challengeId || !normalizedCode) {
return NextResponse.json(
{ error: "Challenge ID and verification code are required" },
{ status: 400 },
);
}
+ if (!/^\d{6}$/.test(normalizedCode)) {
+ return NextResponse.json({ error: "Invalid code" }, { status: 400 });
+ }
// Verify challenge with WorkOS
const verification = await workos.mfa.verifyChallenge({
authenticationChallengeId: challengeId,
- code: code,
+ code: normalizedCode,
});Also applies to: 40-45, 47-51
🤖 Prompt for AI Agents
In app/api/mfa/verify/route.ts around lines 32-39 (and similarly at 40-45 and
47-51), the request body is parsed but the OTP "code" is not validated or
normalized; add validation to ensure the code is present, consists only of ASCII
digits, and has the expected length (e.g. 6 digits) before calling the provider.
Normalize the code by trimming whitespace and removing internal spaces, then
check /^[0-9]+$/ and length === expected; if validation fails, return
NextResponse.json with a 400 status and a clear error message to avoid malformed
inputs and unnecessary provider calls.
| // Verify challenge with WorkOS | ||
| const verification = await workos.mfa.verifyChallenge({ | ||
| authenticationChallengeId: challengeId, | ||
| code: code, | ||
| }); | ||
|
|
||
| return NextResponse.json({ | ||
| valid: verification.valid, | ||
| challenge: verification.challenge, | ||
| }); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider verifying challenge ownership (defense-in-depth).
If the WorkOS verification result exposes factor/user identifiers, assert they belong to userId. Otherwise, fetch ownership info before accepting verification. Please confirm SDK support.
🌐 Web query:
For WorkOS MFA:
- Does verifyChallenge response include the challenge's authenticationFactorId or userId?
- Is there an endpoint to fetch a challenge by ID to validate ownership?
- Recommended pattern for verifying that a challenge belongs to the authenticated user?
💡 Result:
-
Does verifyChallenge include the challenge's authenticationFactorId or userId?
- The verifyChallenge response includes the challenge object with authentication_factor_id (authentication_factor_id shown in the response) but it does not include a userId. [1]
-
Is there an endpoint to fetch a challenge by ID to validate ownership?
- WorkOS docs do not document a public "get challenge by ID" endpoint. challenge objects are returned by challengeFactor() and verifyChallenge() calls, but there is no documented standalone GET /challenge/:id. [1][2]
-
Recommended pattern to verify a challenge belongs to the authenticated user
- Persist the authentication_factor_id (factor ID) in your own user record when enrolling a factor.
- When you create or verify a challenge, compare the returned challenge.authentication_factor_id to the factor ID(s) you have stored for the currently authenticated user; only accept the verification if they match.
- Alternatively, use the WorkOS authenticate / pending_authentication_token flow: the MFA challenge error/flow returns a pending_authentication_token and the user object (and their factors); use that token + a challenge that corresponds to a factor you’ve associated with that user to complete authentication. This ties the challenge to the concrete user in your app. [1][2]
Sources:
- WorkOS MFA docs (challenge & verify responses). [1]
- WorkOS user-management authenticate / MFA pending_authentication_token flow. [2]
Verify challenge ownership before accepting verification
- verifyChallenge returns challenge.authentication_factor_id but not a userId and there is no public GET /challenge/:id endpoint.
- In app/api/mfa/verify/route.ts (lines 47–56) assert verification.challenge.authentication_factor_id matches an authentication_factor_id stored for the authenticated userId; reject the verification on mismatch.
- If you don’t persist factor IDs at enrollment, persist authentication_factor_id when enrolling factors and use that for comparison, or adopt the WorkOS pending_authentication_token flow to tie the challenge to the concrete user.
| // Redirect immediately if the session has ended / invalid_grant | ||
| window.location.href = "/logout"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using Next.js router for navigation.
Using window.location.href for navigation causes a full page reload. Consider using Next.js router for smoother client-side navigation.
Add the router import at the top of the file:
import { useRouter } from 'next/navigation';Then in the useAuthFromAuthKit function:
function useAuthFromAuthKit() {
const { user, loading } = useAuth();
const { accessToken, getAccessToken, refresh } = useAccessToken();
+ const router = useRouter();
const failureCountRef = useRef(0);
const lastFailureAtRef = useRef<number | null>(null);And replace the window.location redirects:
- window.location.href = "/logout";
+ router.push("/logout");Also applies to: 108-109
🤖 Prompt for AI Agents
In components/ConvexClientProvider.tsx around lines 95-96 and 108-109, replace
direct window.location.href assignments with Next.js client-side navigation:
import useRouter from 'next/navigation' at the top, get the router instance
inside the component/hook (e.g., const router = useRouter()), and call
router.push('/logout') (or router.replace('/logout') if you want to avoid
history entries) where window.location.href is currently used; ensure these
redirects occur inside client-side effects/callbacks so router is available.
Summary by CodeRabbit
New Features
Improvements
Style
Chores