feat: enhance NextAuth configuration and user session management with additional fields and cookie domain validation - #3607
Conversation
… additional fields and cookie domain validation
|
Warning Review limit reached
More reviews will be available in 14 minutes and 27 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR extends NextAuth authentication across two applications (platform and vertex) by introducing environment-based cookie domain configuration, augmenting session/JWT/user types with profile identity fields, enriching credential providers to populate those fields from OAuth tokens and login data, and switching post-logout navigation from Next.js router to full-page redirects. ChangesPlatform App Authentication Enhancement
Vertex App Authentication Enhancement
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Stopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a 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 |
|
New azure analytics_platform changes available for preview here |
|
New azure vertex changes available for preview here |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/platform/src/shared/lib/auth.ts (1)
237-247:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't stringify raw backend error payloads into the NextAuth error message.
error?.datais being copied straight intoError.message. In this flow that message is very likely to get logged or propagated through auth error handling, which can leak backend response bodies and user identifiers into logs, monitoring, or redirect/query surfaces. Throw a sanitized code/message instead and keep the raw payload server-side only if it's redacted.As per coding guidelines, "Keep NextAuth session tokens out of logs and sanitize error payloads."
Suggested direction
} catch (error: any) { - // Enhanced error handling to include status and full response data - const errorData = { - message: error?.message || 'Login failed', - status: error?.status || 500, - data: error?.data || null, - success: false, - }; - - // Throw a structured error that includes all the details - throw new Error(JSON.stringify(errorData)); + const status = + typeof error?.status === 'number' ? error.status : 500; + const message = + status === 401 ? 'Invalid credentials' : 'Login failed'; + + throw new Error(message); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/platform/src/shared/lib/auth.ts` around lines 237 - 247, The catch block in src/shared/lib/auth.ts currently serializes backend payloads into Error.message (the errorData object) which can leak sensitive info; instead, change the catch handling in the login/auth flow so you throw a sanitized Error containing only a non-sensitive code and brief message (e.g., "AUTH_LOGIN_FAILED" / "Login failed") and do NOT include error?.data in the Error.message; keep the full backend payload private by assigning it to a non-serializing property (e.g., error.raw or a local server-side log) and redact any tokens/identifiers before logging, and ensure this change is applied where errorData is created/thrown in the catch block so NextAuth receives only the safe code/message.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/platform/src/shared/hooks/useLogout.ts`:
- Around line 119-127: The catch block in useLogout currently treats a failed
signOut() as success by clearing local state and forcing navigation, which can
leave the NextAuth cookie intact; update the catch so you do NOT reset local
auth state or immediately redirect when signOut({ redirect: false }) throws.
Instead, on error attempt a fallback server-side sign-out (e.g. POST to your
server sign-out route or call a server-side endpoint) and await its result; only
perform window.location.href navigation after the server-side sign-out succeeds,
and if that also fails, leave logging-out state (dispatch(setLoggingOut(false))
or set an error state) and do not navigate so the client doesn’t silently
recreate a session. Ensure the changes are made around the signOut call in
useLogout and reference signOut and setLoggingOut when modifying the control
flow.
In `@src/vertex/app/api/auth/`[...nextauth]/options.ts:
- Line 337: The session callback currently assigns session.user._id from
token.id but the JWT callback stores the Mongo-style identifier on token._id;
update the session construction to read session.user._id from token._id (not
token.id) so the persisted `_id` contract is preserved—locate the session
callback in options.ts where session.user._id is assigned and change the source
to token._id.
---
Outside diff comments:
In `@src/platform/src/shared/lib/auth.ts`:
- Around line 237-247: The catch block in src/shared/lib/auth.ts currently
serializes backend payloads into Error.message (the errorData object) which can
leak sensitive info; instead, change the catch handling in the login/auth flow
so you throw a sanitized Error containing only a non-sensitive code and brief
message (e.g., "AUTH_LOGIN_FAILED" / "Login failed") and do NOT include
error?.data in the Error.message; keep the full backend payload private by
assigning it to a non-serializing property (e.g., error.raw or a local
server-side log) and redact any tokens/identifiers before logging, and ensure
this change is applied where errorData is created/thrown in the catch block so
NextAuth receives only the safe code/message.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 08dc9ffe-6686-42eb-90ac-10c4564f597e
📒 Files selected for processing (10)
src/platform/.env.examplesrc/platform/middleware.tssrc/platform/src/next-auth.d.tssrc/platform/src/shared/hooks/useLogout.tssrc/platform/src/shared/lib/auth.tssrc/vertex/.env.examplesrc/vertex/app/api/auth/[...nextauth]/options.tssrc/vertex/core/hooks/useLogout.tssrc/vertex/middleware.tssrc/vertex/types/next-auth.d.ts
…nhance user session data assignment
|
New azure docs changes available for preview here |
|
New azure analytics_platform changes available for preview here |
|
New azure vertex changes available for preview here |
|
New azure vertex changes available for preview here |
|
New azure analytics_platform changes available for preview here |
2 participants
Status of maturity (all need to be checked before merging):
Screenshots (optional)
Summary by CodeRabbit
New Features
Bug Fixes