-
Notifications
You must be signed in to change notification settings - Fork 1.2k
chore: auth secure to true #941
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThe sessionToken cookie’s secure flag in packages/database/auth/auth-options.tsx was changed from conditional (based on NODE_ENV) to always true. No other logic, exports, or control flow were modified. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
packages/database/auth/auth-options.tsx (1)
97-101: Do not log OTP codes or email identifiers on the production pathWhen
RESEND_API_KEYis present, we’re in the real email-sending branch, but the code logs both the user email and the 6‑digit token. This is sensitive (PII + one-time secret) and will be stored in logs. Remove these logs or replace with a sanitized message.Proposed diff:
- console.log({ identifier, token }); const { OTPEmail } = await import("../emails/otp-email"); const email = OTPEmail({ code: token, email: identifier }); - console.log({ email }); await sendEmail({ email: identifier, subject: `Your Cap Verification Code`, react: email, }); + console.info("Verification email dispatched via provider");
🧹 Nitpick comments (3)
packages/database/auth/auth-options.tsx (3)
115-116: Harden the session cookie with the __Host- prefixSince you have
path: "/", nodomainattribute, and nowsecure: true, you can safely adopt the__Host-prefix for stronger isolation (prevents cookie injection from subdomains).Applying this is optional but recommended. Note: this will invalidate existing sessions once deployed.
Proposed diff:
- name: `next-auth.session-token`, + name: `__Host-next-auth.session-token`,
33-34: Turn off NextAuth debug in production
debug: truein production can leak sensitive details into logs. Gate it by environment.Proposed diff:
- debug: true, + debug: process.env.NODE_ENV !== "production",
169-188: Wrap organization setup in a DB transaction to avoid partial stateThe multi-step sequence (create organization, add member, update user) can leave partial records if any step fails. Use a transaction to guarantee atomicity.
Example (Drizzle):
await db().transaction(async (tx) => { const organizationId = nanoId(); await tx.insert(organizations).values({ id: organizationId, name: "My Organization", ownerId: user.id, }); await tx.insert(organizationMembers).values({ id: nanoId(), userId: user.id, organizationId, role: "owner", }); await tx.update(users) .set({ activeOrganizationId: organizationId }) .where(eq(users.id, user.id)); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/database/auth/auth-options.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use strict TypeScript and avoid any; prefer shared types from packages
Files:
packages/database/auth/auth-options.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (2)
packages/database/auth/auth-options.tsx (2)
117-121: Secure=true with SameSite=None is correctSetting the session cookie to Secure when SameSite is "none" aligns with current browser requirements and prevents rejection in Safari/Chrome. This should reduce "Set-Cookie blocked because it had the 'SameSite=None' attribute but was not set with the 'Secure' attribute" issues.
114-121: Confirm local development runs over HTTPS, otherwise auth will silently failWith
secure: true, browsers won't set or send the cookie over plain HTTP (including http://localhost). If any teammate still uses HTTP locally, they'll see sign-in loops. Please verify your dev setup terminates TLS (e.g., HTTPS-enabled dev domain, local reverse proxy, or Next.js dev server with HTTPS) and update the README accordingly.Quick manual check:
- In DevTools → Application → Cookies, confirm
next-auth.session-tokenis created without a "Blocked" reason.- Inspect the Network "Set-Cookie" response header; it should not be flagged as "Secure-only over HTTPS".
I can draft a short doc snippet for local HTTPS setup if helpful.
This is needed for auth to work in local dev.
Previously was modified due to the belief that it affected Safari.
Summary by CodeRabbit