feat: Visitor authentication #96
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
WalkthroughAdds visitor magic-link authentication: DB migration and typings, JWT-based server helpers and cookie handling, API endpoints for request/verify/me/logout, Inngest email sender, client provider/hook and UI (modal/status), integrates auth into footer/reactions/roadmap, and centralizes visitor ID retrieval. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Visitor
participant UI as Page UI (Footer/Reactions/Roadmap)
participant Modal as VisitorAuthModal
participant API as /api/auth/request-magic-link
participant Inngest as Inngest Email Worker
Visitor->>UI: Trigger sign-in or gated action
UI->>Modal: Open modal (enter email)
Modal->>API: POST { email }
API-->>Inngest: Emit event email/visitor.magic-link
Inngest->>Visitor: Send magic-link email
API-->>Modal: 200 (email sent)
sequenceDiagram
autonumber
actor Visitor
participant VerifyPage as /auth/verify page
participant VerifyAPI as /api/auth/verify-magic-link
participant AuthLib as visitor-auth (JWT & cookies)
Visitor->>VerifyPage: Open link with ?token=...
VerifyPage->>VerifyAPI: POST { token }
VerifyAPI->>AuthLib: Validate token, mark verified, create JWT, set cookies
VerifyAPI-->>VerifyPage: success + visitor
VerifyPage->>Visitor: Redirect to stored auth_redirect or homepage
sequenceDiagram
autonumber
participant App as _app (VisitorAuthProvider)
participant Hook as useVisitorAuth
participant MeAPI as /api/auth/me
participant Component as Footer/Reactions/Roadmap
App->>Hook: Mount Provider
Hook->>MeAPI: GET current visitor
MeAPI-->>Hook: 200 visitor OR 401
Component->>Hook: Read visitor/isLoading
alt Not authenticated
Component->>Component: Gate action -> open modal or call onAuthRequired
else Authenticated
Component->>Component: Proceed with action
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🔇 Additional comments (3)
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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![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 (11)
apps/page/.env.example (1)
5-6: Add documentation for JWT secret requirements.Consider adding a comment documenting the security requirements for the JWT secret (minimum length, entropy). This helps developers generate sufficiently strong secrets for production.
Apply this diff to add guidance:
+# JWT secret for visitor authentication (minimum 32 characters, use a cryptographically random string) VISITOR_JWT_SECRET=your-secure-secret-key-change-this-in-productionpackages/supabase/migrations/19_page_visitors_auth.sql (2)
14-17: Index strategy is reasonable.The three indexes serve clear purposes:
verification_token: Essential for magic link verificationverification_expires_at: Useful for batch cleanup of expired tokensThe expires_at index is less critical than the others. Consider monitoring query patterns to verify all three indexes provide value as the table grows.
1-24: Consider implementing expired token cleanup.The migration doesn't include a mechanism to clean up expired verification tokens. Consider implementing:
- A scheduled job (e.g., pg_cron) to delete rows where
verification_expires_at < NOW()- Or an application-level cleanup task
This prevents unbounded table growth from expired tokens.
apps/page/pages/api/pa/view.ts (1)
39-39: Remove redundant fallback in visitor_id.The
getVisitorIdfunction always returns a string (it generates a new UUID if none exists), so the?? v4()fallback is unnecessary and suggests defensive code against a condition that cannot occur.Apply this diff to remove the redundant fallback:
- visitor_id: visitor_id ?? v4(), + visitor_id: visitor_id,apps/page/pages/_sites/[site]/roadmap/[roadmap_slug].tsx (1)
52-52: Unused variable:isAuthLoadingis never referenced.The
isAuthLoadingvariable is destructured but never used in the component logic. Consider removing it to reduce noise, or use it to show loading states in the UI if appropriate.Apply this diff:
- const { visitor, isLoading: isAuthLoading } = useVisitorAuth(); + const { visitor } = useVisitorAuth();apps/page/pages/api/roadmap/vote.ts (1)
22-23: Unused variable:authenticatedVisitoris never referenced.The
authenticatedVisitoris retrieved but not used anywhere in the function. SincegetVisitorIdalready handles authenticated visitors internally (checking JWT first, then falling back to legacy cookies), this variable appears redundant.Apply this diff to remove the unused variable:
const visitor_id = await getVisitorId(req); - const authenticatedVisitor = await getAuthenticatedVisitor(req); - if (!req.cookies.cp_visitor_token && !req.cookies.cp_pa_vid) {apps/page/pages/_sites/[site]/auth/verify.tsx (2)
25-58: Check response.ok before parsing JSON and prefer router.push for client-side navigation.The function has a few areas for improvement:
Missing response.ok check: If the fetch fails (network error, 500, etc.),
response.json()might throw or return unexpected data. Checkresponse.okbefore parsing.Hard navigation loses SPA benefits: Using
window.location.hrefforces a full page reload. Consider usingrouter.push()for client-side navigation unless a full reload is intentional for security/session reasons.sessionStorage access: While unlikely to fail in this context, wrapping in
typeof window !== "undefined"would be defensive (though this page is client-only).Apply this diff to improve robustness:
const verifyToken = async (verificationToken: string) => { try { const response = await fetch("/api/auth/verify-magic-link", { method: "POST", headers: { "Content-Type": "application/json", }, body: JSON.stringify({ token: verificationToken, }), credentials: "include", }); + if (!response.ok) { + const errorData = await response.json().catch(() => ({})); + throw new Error(errorData.message || `Server error: ${response.status}`); + } + const data = await response.json(); if (data.success) { setStatus("success"); setMessage("Email verified successfully! You are now signed in."); // Redirect to the previous page or homepage after 2 seconds setTimeout(() => { - const redirectTo = sessionStorage.getItem("auth_redirect") || "/"; - sessionStorage.removeItem("auth_redirect"); - window.location.href = redirectTo; + if (typeof window !== "undefined") { + const redirectTo = sessionStorage.getItem("auth_redirect") || "/"; + sessionStorage.removeItem("auth_redirect"); + router.push(redirectTo); + } }, 2000); } else { setStatus("error"); setMessage(data.message || "Failed to verify email"); } } catch (error) { setStatus("error"); - setMessage("Something went wrong. Please try again."); + setMessage(error instanceof Error ? error.message : "Something went wrong. Please try again."); } };
60-144: Consider adding noindex to NextSeo.The UI rendering is well-structured with appropriate feedback for each state and good dark mode support.
Consider adding
noindexto prevent search engines from indexing transient verification pages:<NextSeo title="Verify Email - changes.page" description="Verify your email to complete sign-in" + noindex={true} />apps/page/components/visitor-auth-modal.tsx (1)
49-136: Consider updating to Heroicons v2 import path.The UI is well-structured with good accessibility, clear user feedback, and comprehensive dark mode support. The conditional rendering between form and success states is clean.
According to the project's dependencies, you're using
@heroicons/react ^1.0.3. The v2 package uses scoped import paths for better tree-shaking. Consider updating:-import { XIcon } from "@heroicons/react/outline"; +import { XMarkIcon } from "@heroicons/react/24/outline";And update the component usage:
- <XIcon className="h-5 w-5" /> + <XMarkIcon className="h-5 w-5" />Note: This requires upgrading
@heroicons/reactto v2. If staying on v1, the current import is correct.Based on learnings
apps/page/hooks/useVisitorAuth.tsx (1)
25-50: Consider refactoring the useCallback dependency pattern.The
fetchVisitorcallback includeshasFetchedin its dependency array (line 50), which causes the callback to be recreated whenhasFetchedchanges. This triggers theuseEffect(line 70-72) to re-run, though the guard (line 26) prevents execution. While functionally safe, this pattern creates unnecessary callback recreation and effect re-runs.Consider one of these alternatives:
Option 1: Remove hasFetched from dependencies and use a ref
+const hasFetchedRef = useRef(false); const [isLoading, setIsLoading] = useState(true); -const [hasFetched, setHasFetched] = useState(false); const fetchVisitor = useCallback(async () => { - if (hasFetched) return; + if (hasFetchedRef.current) return; // ... fetch logic ... finally { setIsLoading(false); - setHasFetched(true); + hasFetchedRef.current = true; } -}, [hasFetched]); +}, []);Option 2: Move the guard outside the callback
useEffect(() => { + if (!hasFetched) { fetchVisitor(); + } -}, [fetchVisitor]); +}, [hasFetched, fetchVisitor]);apps/page/lib/visitor-auth.ts (1)
35-49: Consider adding input validation.The function doesn't validate the input visitor data (e.g., email format, non-empty strings). While validation may occur upstream, defensive checks here would prevent JWT creation with invalid data.
export async function createVisitorJWT( visitor: Pick<IPageVisitor, "id" | "email" | "email_verified"> ): Promise<string> { + if (!visitor.id || !visitor.email) { + throw new Error("Visitor ID and email are required"); + } + + // Basic email format validation + if (!/^[^\s@]+@[^\s@]+\.[^\s@]+$/.test(visitor.email)) { + throw new Error("Invalid email format"); + } + const jwt = await new SignJWT({ visitor_id: visitor.id, email: visitor.email, email_verified: visitor.email_verified, })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (24)
apps/page/.env.example(1 hunks)apps/page/components/footer.tsx(3 hunks)apps/page/components/reactions.tsx(6 hunks)apps/page/components/visitor-auth-modal.tsx(1 hunks)apps/page/components/visitor-status.tsx(1 hunks)apps/page/hooks/useVisitorAuth.tsx(1 hunks)apps/page/lib/visitor-auth.ts(1 hunks)apps/page/package.json(1 hunks)apps/page/pages/_app.tsx(2 hunks)apps/page/pages/_sites/[site]/auth/verify.tsx(1 hunks)apps/page/pages/_sites/[site]/roadmap/[roadmap_slug].tsx(4 hunks)apps/page/pages/api/auth/logout.ts(1 hunks)apps/page/pages/api/auth/me.ts(1 hunks)apps/page/pages/api/auth/request-magic-link.ts(1 hunks)apps/page/pages/api/auth/verify-magic-link.ts(1 hunks)apps/page/pages/api/pa/view.ts(2 hunks)apps/page/pages/api/post/react.ts(1 hunks)apps/page/pages/api/roadmap/vote.ts(2 hunks)apps/page/utils/http.ts(1 hunks)apps/web/inngest/email/send-visitor-magic-link.ts(1 hunks)apps/web/pages/api/inngest.ts(2 hunks)packages/supabase/migrations/19_page_visitors_auth.sql(1 hunks)packages/supabase/types/index.ts(1 hunks)packages/supabase/types/page.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (17)
packages/supabase/types/page.ts (1)
packages/supabase/types/index.ts (1)
Tables(864-891)
apps/page/components/footer.tsx (1)
apps/page/components/visitor-status.tsx (1)
VisitorStatus(10-71)
apps/web/pages/api/inngest.ts (1)
apps/web/inngest/email/send-visitor-magic-link.ts (1)
sendVisitorMagicLink(12-162)
apps/page/components/reactions.tsx (3)
packages/supabase/types/page.ts (1)
IReactions(72-78)apps/page/hooks/useVisitorAuth.tsx (1)
useVisitorAuth(96-102)apps/page/components/visitor-auth-modal.tsx (1)
VisitorAuthModal(11-137)
apps/page/lib/visitor-auth.ts (1)
packages/supabase/types/page.ts (1)
IPageVisitor(8-8)
apps/page/pages/api/pa/view.ts (1)
apps/page/lib/visitor-auth.ts (2)
getVisitorId(139-155)setLegacyVisitorCookie(160-168)
apps/web/inngest/email/send-visitor-magic-link.ts (1)
packages/supabase/admin.ts (1)
supabaseAdmin(4-7)
apps/page/pages/_sites/[site]/roadmap/[roadmap_slug].tsx (2)
apps/page/hooks/useVisitorAuth.tsx (1)
useVisitorAuth(96-102)apps/page/components/visitor-auth-modal.tsx (1)
VisitorAuthModal(11-137)
apps/page/pages/api/auth/me.ts (1)
apps/page/lib/visitor-auth.ts (1)
getAuthenticatedVisitor(113-133)
apps/page/pages/api/post/react.ts (1)
apps/page/lib/visitor-auth.ts (2)
getVisitorId(139-155)setLegacyVisitorCookie(160-168)
apps/page/pages/_app.tsx (2)
apps/page/hooks/useVisitorAuth.tsx (1)
VisitorAuthProvider(20-94)apps/page/components/core/progress-bar.component.tsx (1)
ProgressBar(16-18)
apps/page/pages/api/auth/request-magic-link.ts (3)
apps/page/lib/data.ts (2)
translateHostToPageIdentifier(450-450)fetchRenderData(447-447)packages/supabase/admin.ts (1)
supabaseAdmin(4-7)apps/page/lib/visitor-auth.ts (2)
generateVerificationToken(28-30)getTokenExpiry(183-185)
apps/page/components/visitor-status.tsx (2)
apps/page/hooks/useVisitorAuth.tsx (1)
useVisitorAuth(96-102)apps/page/components/visitor-auth-modal.tsx (1)
VisitorAuthModal(11-137)
apps/page/components/visitor-auth-modal.tsx (1)
apps/page/utils/http.ts (1)
httpPost(1-21)
apps/page/pages/api/auth/logout.ts (1)
apps/page/lib/visitor-auth.ts (1)
clearVisitorAuthCookie(103-108)
apps/page/pages/api/auth/verify-magic-link.ts (2)
packages/supabase/admin.ts (1)
supabaseAdmin(4-7)apps/page/lib/visitor-auth.ts (3)
isTokenExpired(173-178)createVisitorJWT(35-49)setVisitorAuthCookie(88-98)
apps/page/pages/api/roadmap/vote.ts (1)
apps/page/lib/visitor-auth.ts (3)
getVisitorId(139-155)getAuthenticatedVisitor(113-133)setLegacyVisitorCookie(160-168)
🪛 GitHub Check: CodeQL
apps/page/pages/api/auth/request-magic-link.ts
[failure] 47-47: Polynomial regular expression used on uncontrolled data
This regular expression that depends on a user-provided value may run slow on strings starting with '!@!.' and with many repetitions of '!.'.
🔇 Additional comments (70)
apps/page/package.json (1)
27-27: LGTM! Appropriate JWT library for visitor authentication.The
joselibrary is a well-maintained, modern choice for JWT operations. The version range^6.1.0is appropriate for this stable library.packages/supabase/types/page.ts (1)
8-8: LGTM! Type export follows existing patterns.The
IPageVisitortype export is consistent with other table type exports in this file and properly uses theTableshelper.packages/supabase/types/index.ts (2)
245-274: LGTM! Well-designed visitor authentication table.The
page_visitorstable schema is well-structured with:
- Unique email constraint preventing duplicates
- Secure default (
email_verified: false)- Nullable verification fields supporting optional verification flow
- Proper timestamp handling
785-847: No references to removed database functions
Verified via comprehensive search across.tsand.tsxfiles that neitherroadmap_item_has_votednorroadmap_item_votes_countis referenced outsidepackages/supabase/types. Removal is safe.packages/supabase/migrations/19_page_visitors_auth.sql (3)
1-10: LGTM! Well-structured visitor authentication table.The table schema is clean with appropriate constraints:
- Unique email prevents duplicates
- Secure default for
email_verified- Nullable verification fields support the magic link flow
- Proper timestamp defaults
19-23: Approve trigger_set_timestamp() trigger usage. Thetrigger_set_timestamp()function is defined inpackages/supabase/migrations/0_schema.sqland is consistently applied across multiple migrations.
12-12: RLS on page_visitors is safe—no policies required
All accesses to page_visitors use the service-role client (supabaseAdmin), which bypasses row-level security; the anon key is never used for this table.apps/web/pages/api/inngest.ts (2)
7-7: LGTM!The import follows the established pattern for email handlers.
21-21: LGTM!The registration of the new visitor magic link handler is correctly placed with other email functions.
apps/page/pages/_app.tsx (2)
5-5: LGTM!The import of VisitorAuthProvider is correctly placed.
25-28: LGTM!The VisitorAuthProvider correctly wraps the application components within the ThemeProvider, ensuring visitor authentication context is available throughout the app.
apps/page/pages/api/pa/view.ts (2)
9-9: LGTM!The import of centralized visitor authentication utilities aligns with the PR's goal of standardizing visitor ID handling.
25-29: LGTM!The refactored visitor ID retrieval using
getVisitorIdcentralizes the logic and correctly sets the legacy cookie only when neither authentication token nor legacy cookie exists.apps/page/utils/http.ts (1)
16-18: Enhanced error detection logic.The updated condition now catches API errors that return HTTP 2xx status codes but indicate failure in the payload. The logic
(!payload.ok && !payload.success)throws only when both fields are falsy, allowing backward compatibility where either field can signal success.Note: If the payload is malformed or missing both fields entirely, this will throw an error, which is reasonable defensive behavior.
apps/page/components/footer.tsx (4)
1-1: LGTM!The import consolidation is clean.
17-17: LGTM!The VisitorStatus component import integrates the new authentication feature into the footer.
29-31: LGTM!The VisitorStatus component is well-positioned in the footer with appropriate styling and spacing.
34-34: LGTM!The padding adjustment from
pt-8topt-4appropriately compensates for the new VisitorStatus block above, maintaining consistent visual spacing.apps/page/pages/_sites/[site]/roadmap/[roadmap_slug].tsx (3)
18-18: LGTM: Clean integration of visitor auth components.The imports are properly structured and align with the new authentication flow.
Also applies to: 20-20
545-548: LGTM: Clean modal integration.The authentication modal is properly wired with state management and follows the expected pattern.
82-85: Authentication check is sufficient.
isAuthenticatedis implemented as!!visitor, soif (!visitor)is functionally identical toif (!isAuthenticated). No changes needed.apps/page/pages/api/post/react.ts (2)
4-4: LGTM: Clean migration to centralized visitor ID handling.The refactor properly uses the centralized
getVisitorIdhelper, which handles both JWT-based and legacy cookie-based visitor identification.Also applies to: 12-12
14-16: LGTM: Proper legacy cookie fallback.The conditional legacy cookie setting correctly handles the migration path by only setting the legacy cookie when neither the new JWT token nor the old cookie exists.
apps/page/pages/api/auth/logout.ts (3)
1-12: LGTM: Clean API route structure.The imports, type definitions, and function signature follow Next.js best practices.
13-18: LGTM: Proper HTTP method enforcement.The 405 Method Not Allowed response for non-POST requests follows HTTP standards.
20-34: LGTM: Straightforward logout implementation.The logout logic correctly clears the authentication cookie and provides appropriate success/error responses.
apps/page/pages/api/roadmap/vote.ts (2)
4-4: LGTM: Consistent migration to centralized visitor ID handling.The refactor aligns with the pattern used in other API routes, properly delegating visitor identification to the centralized helper.
Also applies to: 20-20
24-26: LGTM: Consistent legacy cookie fallback pattern.The conditional cookie setting matches the pattern used in other migrated API routes and correctly handles the transition from legacy to JWT-based authentication.
apps/web/inngest/email/send-visitor-magic-link.ts (5)
1-10: LGTM: Clean structure and type definitions.The imports and EventData interface are well-structured. The optional
page_idfield correctly handles cases where branding information may not be available.
12-39: LGTM: Solid function setup with fallback branding.The function correctly fetches page-specific branding when available and falls back to default branding. The
.single()query will throw if the page isn't found, but the outer try-catch (implicit in Inngest) will handle it gracefully.
42-42: LGTM: Standard magic link URL construction.The magic link URL is constructed properly using the page-specific URL and verification token. The token in the query string is standard practice for magic links.
44-160: Consider: Email template is comprehensive and well-structured.The email template includes:
- Responsive HTML with fallback text version
- Clear CTA button with copy-paste fallback
- Security note about expiration
- Dynamic branding support
The inline HTML approach is acceptable for this use case and keeps the template co-located with the sending logic.
44-160: Confirm Postmark server token and sender signature
EnsurePOSTMARK_SERVER_TOKENis set in your environment and thatno-reply@changes.page(or thechanges.pagedomain) is verified in your Postmark account; unverified senders will prevent emails from being delivered.apps/page/pages/_sites/[site]/auth/verify.tsx (1)
1-10: LGTM! Clean component setup.The imports are appropriate and the state management is well-structured for handling the verification flow states.
apps/page/pages/api/auth/me.ts (2)
1-12: LGTM! Well-defined API response types.The interface clearly defines the response structure with appropriate optional fields.
14-50: LGTM! Clean API handler with proper authentication flow.The handler correctly validates the HTTP method, checks authentication, and returns appropriate status codes. The reliance on
getAuthenticatedVisitorfor JWT validation is appropriate.Minor note: Consider sanitizing error logs in production to avoid leaking sensitive information, though the current approach is acceptable for debugging.
apps/page/components/visitor-auth-modal.tsx (4)
1-9: LGTM! Appropriate dependencies for an accessible modal.The use of Headless UI Dialog ensures proper accessibility handling and focus management.
11-18: LGTM! Well-structured state for the auth flow.The state management covers all necessary UI states for the magic link request flow.
20-40: LGTM! Solid form submission handling.The function properly manages loading states, error handling, and stores the redirect URL for a seamless post-verification experience. The email validation relies on the HTML5
requiredandtype="email"attributes, which is acceptable for this use case.
42-47: LGTM! Proper state cleanup on close.The handler correctly resets the form state when the modal is closed, ensuring a clean slate for the next interaction.
apps/page/components/visitor-status.tsx (5)
1-8: LGTM: Clean imports and well-defined interface.The imports are minimal and the
VisitorStatusPropsinterface clearly defines the component's API with sensible defaults.
10-23: LGTM: Clean delegation pattern for authentication.The
handleSignInlogic correctly delegates toonAuthRequiredwhen provided, otherwise falls back to the built-in modal. This provides flexibility for different integration contexts.
25-32: LGTM: Clear loading state.The loading indicator with pulse animation provides good visual feedback during authentication state resolution.
34-53: LGTM: Well-structured authenticated state.The defensive check for both
isAuthenticated && visitorprevents rendering issues, and theshowEmailprop provides useful flexibility for different UI contexts.
55-71: LGTM: Clean unauthenticated state with proper modal wiring.The Sign in button and modal integration is straightforward and correct. The fragment wrapper is appropriate for returning multiple elements.
apps/page/components/reactions.tsx (3)
26-51: LGTM: Clean authentication gate in reaction handler.The early return when
onAuthRequiredis provided correctly prevents unauthenticated reactions. The dependency array is properly updated to includeonAuthRequired.
272-278: LGTM: Proper authentication gating for reaction picker.The
handleReactionClickcorrectly checks for visitor authentication before toggling the picker and prompts for sign-in when necessary.
328-343: LGTM: Correct conditional authentication requirement.The
onAuthRequiredcallback is correctly passed only when the visitor is not authenticated, and the modal integration is properly wired.apps/page/pages/api/auth/verify-magic-link.ts (5)
1-38: LGTM: Proper validation and type definitions.Method and input validation are correctly implemented with appropriate error responses.
40-62: LGTM: Secure token lookup with proper error handling.Using
maybeSingle()is appropriate, and the error handling correctly returns generic messages without leaking information about whether an email exists in the system.
64-70: LGTM: Clear expiry handling with helpful messaging.The token expiry check provides a user-friendly error message guiding users to request a new link.
72-91: LGTM: Secure visitor update with token cleanup.The update correctly sets
email_verifiedand clears the verification token to prevent reuse. The defensive check for!updatedVisitoris good practice.
105-121: LGTM: Appropriate success response and error handling.The success response returns the necessary visitor information, and the catch block provides comprehensive error logging.
apps/page/pages/api/auth/request-magic-link.ts (7)
1-24: LGTM: Clean imports and reasonable rate limit.The 2-minute rate limit for magic link requests provides good protection against abuse while not being overly restrictive for legitimate users.
91-119: LGTM: Robust page context resolution with proper error handling.The nested try-catch for page data fetching is appropriate, and the requirement for both
pageUrlandpageIdensures the magic link has proper context.
121-140: LGTM: Correct rate limiting implementation.The rate limit calculation correctly derives the original request time from
verification_expires_atand enforces the 2-minute window. The 429 status code is appropriate for rate limiting.
145-159: LGTM: Smart visitor ID continuity logic.The logic correctly maintains visitor identity by reusing the cookie-based ID when available and not taken, which provides good continuity when anonymous visitors authenticate.
161-182: LGTM: Proper upsert with conflict resolution.The
onConflict: "email"strategy correctly updates existing visitors when they request a new magic link, and the conditional ID inclusion is handled properly.
184-192: LGTM: Proper event dispatching with await.The Inngest event includes all necessary data for email delivery, and the
awaitensures the event is queued before responding to the client.
194-206: LGTM: Clean success response and error handling.The response structure is consistent with the API contract, and error handling provides appropriate logging and user feedback.
apps/page/hooks/useVisitorAuth.tsx (4)
1-17: LGTM! Clean interface definitions.The type definitions are well-structured and provide a clear contract for the authentication system.
18-24: LGTM! Good state management pattern.The
hasFetchedflag effectively prevents redundant API calls, which is especially important in React 18's StrictMode where effects run twice in development.
52-63: LGTM! Logout logic is correct.The logout flow properly clears the visitor state and resets the
hasFetchedflag to allow re-authentication.
80-102: LGTM! Proper context pattern with good error boundaries.The context provider and consumer hook follow React best practices, including enforcing provider usage with a clear error message.
apps/page/lib/visitor-auth.ts (6)
11-24: LGTM! Well-defined type contracts.The interfaces clearly separate JWT payload structure from the consumer-facing authenticated visitor type.
25-30: LGTM! Secure token generation.Using 32 bytes of entropy (256 bits) is appropriate for verification tokens and provides strong security against brute-force attacks.
54-83: LGTM! Robust JWT verification with proper type checking.The type guard ensures payload structure validity, and the error handling appropriately returns
nullfor any verification failures, allowing callers to handle unauthenticated states gracefully.
113-133: LGTM! Clean authentication retrieval logic.The function properly handles unauthenticated states by returning
nulland maps the JWT payload to a clean consumer-facing interface.
139-155: LGTM! Good backward compatibility strategy.The fallback logic ensures visitor tracking works for both authenticated and anonymous users. Note that when a new visitor ID is generated (line 151), the caller is responsible for setting the cookie via
setLegacyVisitorCookieto persist it across requests.
173-185: LGTM! Clean token expiry utilities.The 15-minute expiry for verification tokens is appropriate for magic links, balancing security with user convenience.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/page/pages/api/auth/request-magic-link.ts (1)
79-83: Redact email from logs to protect PII.The email address is still being logged in plaintext when Arcjet blocks a request. This was flagged in a previous review and remains unaddressed. Logging PII may violate privacy regulations (GDPR, CCPA).
Apply this diff to remove the email from logs:
- console.log( - "auth/request-magic-link: [Arcjet Block]", - email, - decision.reason - ); + console.log( + "auth/request-magic-link: [Arcjet Block]", + decision.reason + );
🧹 Nitpick comments (1)
apps/page/pages/api/auth/request-magic-link.ts (1)
129-140: Consider simplifying rate limit calculation.The current logic works but is confusing: it derives the original request time by subtracting 15 minutes from
verification_expires_at. This is error-prone and tightly couples the rate limit logic to the token expiry duration.Consider using
updated_atdirectly or adding alast_request_atcolumn:- if (existingVisitor?.verification_expires_at) { - const lastRequestTime = new Date(existingVisitor.verification_expires_at); - const timeSinceLastRequest = - Date.now() - (lastRequestTime.getTime() - 15 * 60 * 1000); + if (existingVisitor?.updated_at) { + const timeSinceLastRequest = + Date.now() - new Date(existingVisitor.updated_at).getTime();Update the select statement to include
updated_at:const { data: existingVisitor } = await supabaseAdmin .from("page_visitors") - .select("id, verification_expires_at") + .select("id, verification_expires_at, updated_at") .eq("email", email.toLowerCase())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
apps/page/package.json(3 hunks)apps/page/pages/api/auth/request-magic-link.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/page/package.json
🧰 Additional context used
🧬 Code graph analysis (1)
apps/page/pages/api/auth/request-magic-link.ts (3)
apps/page/lib/data.ts (2)
translateHostToPageIdentifier(450-450)fetchRenderData(447-447)packages/supabase/admin.ts (1)
supabaseAdmin(4-7)apps/page/lib/visitor-auth.ts (2)
generateVerificationToken(28-30)getTokenExpiry(183-185)
🔇 Additional comments (5)
apps/page/pages/api/auth/request-magic-link.ts (5)
1-16: LGTM! ReDoS vulnerability addressed.The imports correctly include the
validatorlibrary, and the code now usesvalidator.isEmail()(line 47) instead of a custom regex pattern. This addresses the ReDoS vulnerability flagged in previous reviews.
47-52: LGTM! Safe email validation.Using
validator.isEmail()is a secure, well-tested approach that avoids the ReDoS vulnerability present in custom regex patterns.
91-119: LGTM! Robust page context resolution.The page context resolution logic correctly handles both custom domains and standard slugs, with appropriate error handling and user-friendly error messages when the context cannot be determined.
145-182: LGTM! Visitor ID resolution handles edge cases correctly.The logic correctly handles visitor ID resolution across multiple scenarios: existing visitor by email, reusing cookie ID when available, and allowing DB to generate new IDs. The upsert with
onConflict: "email"ensures email uniqueness while preserving visitor identity.
184-192: LGTM! Event dispatch is well-structured.The Inngest event includes all necessary data for magic link email delivery, with consistent email normalization.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (6)
apps/page/pages/_sites/[site]/auth/verify.tsx (1)
14-25: Add verifyToken to useEffect dependencies or wrap in useCallback.As noted in the previous review,
verifyTokenis called inside the effect but not listed in the dependency array, violating React's exhaustive-deps rule.apps/page/pages/api/auth/request-magic-link.ts (2)
157-178: Remove database error details from API response.Line 176 exposes
upsertError.messageto the client, which may leak sensitive database schema or internal details. The error is already logged on line 172 for debugging.Apply this diff:
if (upsertError) { console.error("Failed to upsert page_visitor:", upsertError); return res.status(500).json({ success: false, message: "Failed to process request", - error: upsertError.message, }); }
194-201: Remove internal error details from API response.Line 199 exposes
error.messageto the client, which may leak sensitive internal information. The error is already logged on line 195 for support investigation.Apply this diff:
} catch (error: any) { console.error("requestMagicLink error:", error); return res.status(500).json({ success: false, message: "Internal server error", - error: error.message, }); }apps/page/lib/visitor-auth.ts (3)
89-102: Fix cookie handling to support multiple cookies and local development.The current implementation has two issues:
Multiple cookies: Using
res.setHeaderdirectly overwrites any existingSet-Cookieheaders. If the API needs to set multiple cookies in one response, only the last will be sent.Local development: The
Secureflag on line 98 requires HTTPS and will prevent the cookie from being set in local development onhttp://localhost.Apply this diff to fix both issues:
+const isProduction = process.env.NODE_ENV === "production"; + export function setVisitorAuthCookie( res: NextApiResponse, token: string ): void { - res.setHeader( - "Set-Cookie", - `${JWT_COOKIE_NAME}=${token}; Path=/; Secure; HttpOnly; SameSite=Lax; Max-Age=${ + const existingCookies = res.getHeader("Set-Cookie") || []; + const cookiesArray = Array.isArray(existingCookies) ? existingCookies : [String(existingCookies)]; + const secureFlag = isProduction ? "Secure; " : ""; + + cookiesArray.push( + `${JWT_COOKIE_NAME}=${token}; Path=/; ${secureFlag}HttpOnly; SameSite=Lax; Max-Age=${ 30 * 24 * 60 * 60 }` // 30 days ); + res.setHeader("Set-Cookie", cookiesArray); }
104-112: Apply the same cookie handling fixes as in setVisitorAuthCookie.This function has the same multi-cookie overwrite and local development issues noted in lines 89-102.
Apply this diff:
+const isProduction = process.env.NODE_ENV === "production"; + export function clearVisitorAuthCookie(res: NextApiResponse): void { - res.setHeader( - "Set-Cookie", - `${JWT_COOKIE_NAME}=; Path=/; Secure; HttpOnly; SameSite=Lax; Max-Age=0` + const existingCookies = res.getHeader("Set-Cookie") || []; + const cookiesArray = Array.isArray(existingCookies) ? existingCookies : [String(existingCookies)]; + const secureFlag = isProduction ? "Secure; " : ""; + + cookiesArray.push( + `${JWT_COOKIE_NAME}=; Path=/; ${secureFlag}HttpOnly; SameSite=Lax; Max-Age=0` ); + res.setHeader("Set-Cookie", cookiesArray); }
159-170: Apply the same cookie handling fixes to legacy cookie setter.This function has the same multi-cookie overwrite and local development issues noted in lines 89-102.
Apply this diff:
+const isProduction = process.env.NODE_ENV === "production"; + export function setLegacyVisitorCookie( res: NextApiResponse, visitorId: string ): void { - res.setHeader( - "Set-Cookie", - `cp_pa_vid=${visitorId}; Path=/; Secure; HttpOnly; SameSite=Lax; Max-Age=31536000` // 1 year + const existingCookies = res.getHeader("Set-Cookie") || []; + const cookiesArray = Array.isArray(existingCookies) ? existingCookies : [String(existingCookies)]; + const secureFlag = isProduction ? "Secure; " : ""; + + cookiesArray.push( + `cp_pa_vid=${visitorId}; Path=/; ${secureFlag}HttpOnly; SameSite=Lax; Max-Age=31536000` // 1 year ); + res.setHeader("Set-Cookie", cookiesArray); }
🧹 Nitpick comments (2)
apps/page/pages/_sites/[site]/auth/verify.tsx (1)
58-144: Consider dynamic page branding in metadata.The NextSeo title is hardcoded as "Verify Email - changes.page". Since other parts of the auth flow use dynamic page branding (e.g., the email templates), consider fetching and using the actual page name here for consistency.
This is a minor polish item and can be deferred if the page context is not easily accessible at this route.
apps/page/components/visitor-auth-modal.tsx (1)
20-44: Add client-side email validation before submission.The form only uses HTML5
requiredattribute for email validation. Consider adding explicit validation using thevalidatorlibrary (already a project dependency per learnings) to provide better error messages before making the API call.+import validator from 'validator'; const handleSubmit = async (e: React.FormEvent) => { e.preventDefault(); + + if (!validator.isEmail(email)) { + setError("Please enter a valid email address"); + return; + } + setIsLoading(true); setError("");This provides immediate feedback and reduces unnecessary API calls for obviously invalid emails.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/page/components/visitor-auth-modal.tsx(1 hunks)apps/page/hooks/useVisitorAuth.tsx(1 hunks)apps/page/lib/visitor-auth.ts(1 hunks)apps/page/pages/_sites/[site]/auth/verify.tsx(1 hunks)apps/page/pages/api/auth/request-magic-link.ts(1 hunks)apps/web/inngest/email/send-visitor-magic-link.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
apps/page/pages/_sites/[site]/auth/verify.tsx (1)
apps/page/utils/http.ts (1)
httpPost(1-21)
apps/page/lib/visitor-auth.ts (1)
packages/supabase/types/page.ts (1)
IPageVisitor(8-8)
apps/page/components/visitor-auth-modal.tsx (1)
apps/page/utils/http.ts (1)
httpPost(1-21)
apps/page/pages/api/auth/request-magic-link.ts (3)
apps/page/lib/data.ts (2)
translateHostToPageIdentifier(450-450)fetchRenderData(447-447)packages/supabase/admin.ts (1)
supabaseAdmin(4-7)apps/page/lib/visitor-auth.ts (2)
generateVerificationToken(32-34)getTokenExpiry(185-187)
apps/web/inngest/email/send-visitor-magic-link.ts (1)
packages/supabase/admin.ts (1)
supabaseAdmin(4-7)
🔇 Additional comments (23)
apps/page/components/visitor-auth-modal.tsx (1)
53-141: LGTM - Well-structured modal with good UX.The modal implementation is clean with:
- Proper state management for loading/success/error states
- Good accessibility (Dialog.Title, aria-hidden)
- Dark mode support
- Clear user feedback at each step
apps/web/inngest/email/send-visitor-magic-link.ts (1)
18-22: Remove verification_token from logs - security risk.The
verification_tokenis sensitive authentication data that should never be logged. Logs may be persisted to external systems (log aggregators, monitoring tools) or accessed by unauthorized personnel, exposing magic link tokens and potentially allowing account takeover.Apply this diff to remove the token from logs:
console.log("Sending magic link email", { email, page_url, page_id, - // Do not log verification_token - it's sensitive });Alternatively, if logging is required for debugging, ensure tokens are never included in any log output in production environments.
Likely an incorrect or invalid review comment.
apps/page/pages/api/auth/request-magic-link.ts (9)
1-16: LGTM! Imports and configuration are well-organized.The imports are appropriate and the rate limit constant is clearly defined.
17-25: LGTM! Type definitions are clear and appropriate.The interfaces provide good type safety for the API contract.
27-36: LGTM! Method guard is properly implemented.Standard and correct implementation for Next.js API routes.
38-52: LGTM! Email validation uses a well-tested library.Using
validator.isEmailfrom the validator.js library eliminates the ReDoS vulnerability that was previously flagged. This is a best practice for email validation.
54-85: LGTM! Arcjet protection is well-configured and PII concern is addressed.The Arcjet integration properly validates email, blocks bots, and applies rate limiting. Line 79 now logs only
decision.reasonwithout the email, addressing the previous PII logging concern.
87-115: LGTM! Page context resolution is well-handled.The logic correctly determines the page URL from custom domain or slug, with appropriate error handling and user-friendly messaging.
138-155: LGTM! Visitor ID logic maintains identity across auth flows.The logic correctly preserves existing visitor IDs from the database or cookies while handling conflicts gracefully.
180-188: LGTM! Inngest event is properly structured.The event payload contains all necessary information for the magic link email handler.
190-193: LGTM! Success response is clear and appropriate.apps/page/hooks/useVisitorAuth.tsx (3)
1-24: LGTM! Type definitions are clear and well-structured.The interfaces properly model the visitor authentication state and API.
25-86: LGTM! Provider implementation is clean and race condition is resolved.The previous race condition concern has been addressed by removing the
refetchfunction from the public API. ThefetchVisitorlogic is now internal and guarded byhasFetched, ensuring only one fetch occurs. Thelogoutfunction correctly resetshasFetchedto allow re-authentication on the next mount or effect trigger.This design choice means logout does not immediately refetch but instead relies on the natural effect cycle or component remounting, which is appropriate for the logout flow.
88-94: LGTM! Hook enforces provider usage correctly.Standard React context pattern with proper error messaging.
apps/page/lib/visitor-auth.ts (9)
1-14: LGTM! Environment validation is properly implemented.The startup-time validation for
VISITOR_JWT_SECRETaddresses the previous critical security concern. The constants are clearly defined.
15-27: LGTM! Type definitions are comprehensive.The interfaces properly model the JWT payload and authenticated visitor state.
29-34: LGTM! Token generation is cryptographically secure.Using
randomBytes(32)is appropriate for verification tokens.
36-53: LGTM! JWT creation follows best practices.The implementation uses jose library correctly with appropriate algorithm (HS256) and expiry time.
55-66: LGTM! Type guard is comprehensive.The type guard validates all required fields and types before accepting the payload.
68-87: LGTM! JWT verification is properly implemented.The verification logic correctly validates the token and payload structure with appropriate error handling.
114-135: LGTM! Authenticated visitor retrieval is correctly implemented.The function properly chains token retrieval and verification with appropriate null checks.
137-157: LGTM! Visitor ID retrieval maintains backward compatibility.The fallback logic correctly handles migration from legacy cookies to JWT-based authentication while generating new IDs when needed.
172-187: LGTM! Token expiry helpers are correctly implemented.The helpers provide clear, reusable logic for token expiration handling.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
apps/web/inngest/email/send-visitor-magic-link.ts (1)
29-37: Wrap database query in try-catch for network failures.While the code now checks
pageError, the query itself can still throw on network failures or connection issues before returning a result. This would crash the function and prevent the email from being sent (potentially leaving the user waiting).Apply this diff to handle network errors:
+ let page, pageError; + try { - const { data: page, error: pageError } = await supabaseAdmin + ({ data: page, error: pageError } = await supabaseAdmin .from("pages") .select("title, page_settings(page_logo)") .eq("id", page_id) - .maybeSingle(); + .maybeSingle()); + } catch (err) { + console.error("Network error fetching page:", err); + throw new Error("Failed to fetch page information"); + } + if (!page || pageError) { console.error("Error fetching page:", pageError); throw new Error("Page not found"); }apps/page/pages/api/auth/request-magic-link.ts (1)
117-136: Simplify rate-limit check by using updated_atThe page_visitors table defines an updated_at timestamp with a trigger (trigger_set_timestamp) that refreshes it on every update, so you can avoid back-calculating from verification_expires_at.
const { data: existingVisitor } = await supabaseAdmin .from("page_visitors") - .select("id, verification_expires_at") + .select("id, updated_at") .eq("email", email.toLowerCase()) .order("updated_at", { ascending: false }) .limit(1) .maybeSingle(); - if (existingVisitor?.verification_expires_at) { - const lastRequestTime = new Date(existingVisitor.verification_expires_at); - const timeSinceLastRequest = - Date.now() - (lastRequestTime.getTime() - 15 * 60 * 1000); + if (existingVisitor?.updated_at) { + const timeSinceLastRequest = + Date.now() - new Date(existingVisitor.updated_at).getTime(); if (timeSinceLastRequest < RATE_LIMIT_MINUTES * 60 * 1000) { return res.status(429).json({ success: false, message: `Please wait before requesting another magic link`, }); }No additional verification needed—
updated_atis auto-set viatrigger_set_timestampin migration 19_page_visitors_auth.sql.
🧹 Nitpick comments (2)
apps/web/components/page-settings/notifications.tsx (1)
72-75: Consider removing redundant validation.The Yup schema at line 45 already validates that
email_reply_tois a valid email format. After fixing the conditional logic, consider removing this redundantvalidator.isEmail()check and relying solely on Yup's built-in validation, which already runs via Formik.apps/page/pages/api/auth/request-magic-link.ts (1)
180-188: Consider handling Inngest dispatch failures.The
inngestClient.send()call has no error handling. If Inngest is unavailable, the API returns success to the user even though no email will be sent.Consider wrapping in try-catch to provide accurate feedback:
+ try { await inngestClient.send({ name: "email/visitor.magic-link", data: { email: email.toLowerCase(), verification_token: verificationToken, page_url: pageUrl, page_id: pageId, }, }); + } catch (inngestError) { + console.error("Failed to dispatch magic link email event:", inngestError); + return res.status(500).json({ + success: false, + message: "Failed to send email. Please try again.", + }); + }Alternatively, if fire-and-forget is intentional, add a comment explaining this design decision.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
apps/page/pages/api/auth/request-magic-link.ts(1 hunks)apps/web/components/page-settings/notifications.tsx(2 hunks)apps/web/inngest/email/send-visitor-magic-link.ts(1 hunks)apps/web/package.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
apps/page/pages/api/auth/request-magic-link.ts (3)
apps/page/lib/data.ts (2)
translateHostToPageIdentifier(450-450)fetchRenderData(447-447)packages/supabase/admin.ts (1)
supabaseAdmin(4-7)apps/page/lib/visitor-auth.ts (2)
generateVerificationToken(32-34)getTokenExpiry(185-187)
apps/web/inngest/email/send-visitor-magic-link.ts (1)
packages/supabase/admin.ts (1)
supabaseAdmin(4-7)
🔇 Additional comments (10)
apps/web/package.json (1)
79-79: Type-only dependency correctly scoped.Thanks for shifting
@types/validatorintodevDependencies; keeps the runtime bundle lean while still satisfying TypeScript.apps/web/inngest/email/send-visitor-magic-link.ts (3)
1-10: LGTM!The imports and interface definition are clean and appropriate for the magic link email functionality.
12-27: LGTM!Good security practice not logging the
verification_token. The validation forpage_idis appropriate and ensures required data is present before proceeding.
42-44: LGTM!The use of
URLconstructor andsearchParams.set()properly encodes the verification token, preventing malformed URLs. This addresses the previous concern about URL encoding.apps/page/pages/api/auth/request-magic-link.ts (6)
1-25: LGTM!Clean imports and well-defined TypeScript interfaces. The use of
validatorlibrary addresses the previous ReDoS concern.
38-52: LGTM!The use of
validator.isEmail()properly addresses the previous ReDoS vulnerability. Email validation is now safe and robust.
54-85: LGTM!The Arcjet protection configuration is well-structured. Line 79 no longer logs the email address, addressing the previous PII concern.
87-115: LGTM!Page context extraction is well-handled with appropriate error handling and validation. The fallback behavior when page context cannot be determined is user-friendly.
138-155: LGTM!The visitor ID resolution logic correctly handles ID reuse from cookies and prevents conflicts by checking if the cookie ID is already taken.
190-200: LGTM!The success response and error handling are appropriate. The generic error message on line 198 correctly avoids exposing internal details while the error is logged for debugging.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/web/inngest/email/send-visitor-magic-link.ts (1)
52-145: Sanitize user-controlled content before HTML interpolation.
pageName(frompage.title) andpageLogoUrl(frompage.page_settings.page_logo) are inserted directly into the HTML email template without sanitization. While email clients strip JavaScript, unsanitized content can still break HTML structure or inject misleading attributes.A past review comment indicated this was addressed, but the current code does not include the sanitization logic.
Apply this diff to add sanitization:
+import validator from "validator";Then after line 40:
const pageName = page.title; const pageLogoUrl = page.page_settings?.page_logo || null; + +const safePageName = validator.escape(pageName); +const safePageLogoUrl = pageLogoUrl ? validator.escape(pageLogoUrl) : null;Update the email template to use the sanitized values:
${ pageLogoUrl - ? `<img src="${pageLogoUrl}" alt="${pageName}" style="max-width: 150px; height: auto; margin: 0 auto;"/>` - : `<h1 style="color: #4f46e5; margin: 0;">${pageName}</h1>` + ? `<img src="${safePageLogoUrl}" alt="${safePageName}" style="max-width: 150px; height: auto; margin: 0 auto;"/>` + : `<h1 style="color: #4f46e5; margin: 0;">${safePageName}</h1>` }And throughout the rest of the template (lines 122, 140, 147, 160):
- <p>You requested to sign in to ${pageName}. Click the button below to complete your authentication:</p> + <p>You requested to sign in to ${safePageName}. Click the button below to complete your authentication:</p> - <p>This email was sent on behalf of ${pageName} via changes.page.</p> + <p>This email was sent on behalf of ${safePageName} via changes.page.</p> - Complete your sign-in to ${pageName} + Complete your sign-in to ${safePageName} - You requested to sign in to ${pageName}. Click or copy the link below to complete your authentication: + You requested to sign in to ${safePageName}. Click or copy the link below to complete your authentication: - This email was sent on behalf of ${pageName} via changes.page. + This email was sent on behalf of ${safePageName} via changes.page.
🧹 Nitpick comments (1)
apps/page/pages/api/auth/request-magic-link.ts (1)
141-155: Visitor ID reuse logic is clever but consider adding a comment.The visitor ID handling (lines 141-155) cleverly preserves anonymous visitor identity when they authenticate. However, the logic flow—checking
existingVisitor, thenexistingVisitorIdfrom cookie, then verifying the cookie ID isn't already taken—is non-obvious.Consider adding an explanatory comment:
+ // Preserve visitor identity across auth: + // 1. Reuse DB record ID if visitor already exists for this email + // 2. Otherwise, try to claim the cookie-provided visitor ID + // 3. Only claim cookie ID if it's not already taken by another visitor const existingVisitorId = req.cookies.cp_pa_vid; let visitorId: string | undefined;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/page/pages/_sites/[site]/auth/verify.tsx(1 hunks)apps/page/pages/api/auth/request-magic-link.ts(1 hunks)apps/page/utils/http.ts(2 hunks)apps/web/components/page-settings/notifications.tsx(2 hunks)apps/web/inngest/email/send-visitor-magic-link.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/page/pages/_sites/[site]/auth/verify.tsx
- apps/web/components/page-settings/notifications.tsx
- apps/page/utils/http.ts
🧰 Additional context used
🧬 Code graph analysis (2)
apps/web/inngest/email/send-visitor-magic-link.ts (1)
packages/supabase/admin.ts (1)
supabaseAdmin(4-7)
apps/page/pages/api/auth/request-magic-link.ts (3)
apps/page/lib/data.ts (2)
translateHostToPageIdentifier(450-450)fetchRenderData(447-447)packages/supabase/admin.ts (1)
supabaseAdmin(4-7)apps/page/lib/visitor-auth.ts (2)
generateVerificationToken(32-34)getTokenExpiry(185-187)
🔇 Additional comments (2)
apps/page/pages/api/auth/request-magic-link.ts (2)
27-199: LGTM on security and error handling improvements.The following concerns from previous reviews have been properly addressed:
- ReDoS vulnerability: Now using
validator.isEmail()instead of vulnerable regex- PII logging: Email is no longer logged when Arcjet blocks
- Internal error exposure: Error details are no longer returned to clients
- Method validation and input checks are solid
The Arcjet integration provides good defense-in-depth with disposable email detection, bot protection, and rate limiting.
117-136: Use updated_at for rate limiting and confirm auto‐updateReplace the confusing back-calculation from
verification_expires_atwith a directupdated_atcheck:const { data: existingVisitor } = await supabaseAdmin .from("page_visitors") - .select("id, verification_expires_at") + .select("id, updated_at") .eq("email", email.toLowerCase()) .order("updated_at", { ascending: false }) .limit(1) .maybeSingle(); - if (existingVisitor?.verification_expires_at) { - const lastRequestTime = new Date(existingVisitor.verification_expires_at); - const timeSinceLastRequest = - Date.now() - (lastRequestTime.getTime() - 15 * 60 * 1000); + if (existingVisitor?.updated_at) { + const timeSinceLastRequest = + Date.now() - new Date(existingVisitor.updated_at).getTime(); if (timeSinceLastRequest < RATE_LIMIT_MINUTES * 60 * 1000) { return res.status(429).json({ success: false, message: `Please wait before requesting another magic link`, }); } }Ensure
page_visitors.updated_atis automatically refreshed on every UPSERT (e.g. via a trigger). If not present, add:CREATE OR REPLACE FUNCTION trigger_set_timestamp() RETURNS TRIGGER AS $$ BEGIN NEW.updated_at = NOW(); RETURN NEW; END; $$ LANGUAGE plpgsql; CREATE TRIGGER set_timestamp_page_visitors BEFORE UPDATE ON page_visitors FOR EACH ROW EXECUTE FUNCTION trigger_set_timestamp();
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/page/utils/http.ts (1)
8-12: Missing credentials configuration for cookie-based authentication.Unlike
httpGet(line 31),httpPostdoesn't includecredentials: "include"in the fetch options. Since this PR introduces visitor authentication using JWT cookies (per the AI summary),httpPostshould also send credentials to ensure authentication cookies are included in requests.Apply this diff to add credentials to the fetch options:
const res = await fetch(url, { method: "POST", headers: new Headers({ "Content-Type": "application/json" }), + credentials: "include", body: JSON.stringify(data), });
🧹 Nitpick comments (1)
apps/page/utils/http.ts (1)
16-16: Drop redundantpayload.okfallback or standardize API responses
Auth endpoints under/api/authonly returnsuccess(neverok), so(!payload.ok && !payload.success)is effectively!payload.success. Either remove theokfallback inhttpPostor update your endpoints to includeokalongsidesuccess.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/page/pages/_sites/[site]/roadmap/[roadmap_slug].tsx(4 hunks)apps/page/utils/http.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/page/pages/_sites/[site]/roadmap/[roadmap_slug].tsx
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/page/pages/api/post/react.ts (1)
14-18: Secure-only cookie breaks local developmentMarking
cp_pa_vidasSecureprevents the browser from storing it overhttp://localhost, so every dev request keeps generating a new visitor_id and reactions won’t persist. Gate theSecureattribute on production (or detected HTTPS) so the cookie survives in local testing.- res.setHeader( - "Set-Cookie", - `cp_pa_vid=${visitor_id}; Path=/; Secure; HttpOnly; SameSite=Lax; Max-Age=31536000` - ); + const secure = process.env.NODE_ENV === "production" ? "Secure; " : ""; + res.setHeader( + "Set-Cookie", + `cp_pa_vid=${visitor_id}; Path=/; ${secure}HttpOnly; SameSite=Lax; Max-Age=31536000` + );
♻️ Duplicate comments (1)
apps/page/utils/http.ts (1)
3-3: Unsafe type assertion remains.While the parameter is now correctly marked as optional (
data?: U), the default value{} as Uis still unsafe. IfUhas required properties, this assertion will create an invalid empty object at runtime that violates the type contract.This is particularly concerning for authentication flows where type safety is critical.
Apply this diff to safely handle the default:
- data = {} as U, + data, }: { url: string; data?: U; }): Promise<T> => { const res = await fetch(url, { method: "POST", headers: new Headers({ "Content-Type": "application/json" }), - body: JSON.stringify(data), + body: JSON.stringify(data ?? {}), credentials: "include", });This removes the unsafe assertion and coalesces to an empty object only when serializing, avoiding runtime type contract violations.
Based on learnings
🧹 Nitpick comments (1)
apps/page/utils/http.ts (1)
17-17: API response format inconsistency.Checking both
!payload.okand!payload.successsuggests different API endpoints use different response formats. This dual condition indicates technical debt and increases maintenance burden.For authentication-critical flows, consistent error handling is essential.
Consider standardizing API responses to use a single success indicator (preferably
successto align with common REST conventions), then update this condition to check only one field:- if (res.status >= 400 || (!payload.ok && !payload.success)) { + if (res.status >= 400 || !payload.success) { throw new Error(payload.message); }Alternatively, if both formats must coexist temporarily, add a comment explaining why and create a tracking issue to unify the response format.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/page/lib/visitor-auth.ts(1 hunks)apps/page/pages/api/auth/logout.ts(1 hunks)apps/page/pages/api/auth/verify-magic-link.ts(1 hunks)apps/page/pages/api/pa/view.ts(2 hunks)apps/page/pages/api/post/react.ts(1 hunks)apps/page/pages/api/roadmap/vote.ts(2 hunks)apps/page/utils/http.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/page/pages/api/auth/logout.ts
- apps/page/pages/api/roadmap/vote.ts
- apps/page/pages/api/auth/verify-magic-link.ts
- apps/page/pages/api/pa/view.ts
🧰 Additional context used
🧬 Code graph analysis (2)
apps/page/pages/api/post/react.ts (1)
apps/page/lib/visitor-auth.ts (1)
getVisitorId(116-132)
apps/page/lib/visitor-auth.ts (1)
packages/supabase/types/page.ts (1)
IPageVisitor(8-8)
🔇 Additional comments (1)
apps/page/utils/http.ts (1)
12-12: Verify CORS and CSRF protectionsAPI routes set cookies with
Secure; HttpOnly; SameSite=Lax(mitigating CSRF on unsafe methods), but confirm your CORS configuration doesn’t allow credentialed cross-origin requests; if it does, add CSRF tokens or tighten SameSite toStrict.
1 participant
Summary by CodeRabbit
New Features
Improvements
Database
Chores