Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughUpdates CORS handling to allow no-origin and localhost in non-production, extend allowed methods/headers, and log blocked origins. Adjusts logout cookie flags to be environment-aware and removes explicit domain. Extends JWT token option interface with an optional domain and adds it to exported token option objects. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Server as Express App
participant CORS as CORS Middleware
Client->>Server: HTTP Request
Server->>CORS: Validate Origin
alt No Origin
CORS-->>Server: Allow (no-origin)
else Origin in allowedOrigins
CORS-->>Server: Allow (whitelist)
else Non-production and origin includes "localhost"
CORS-->>Server: Allow (dev localhost)
else Disallowed
CORS-->>Server: Block (error "Not allowed by CORS")
Server-->>Server: Log blocked origin
end
Server-->>Client: Response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
✨ 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
src/app.ts (4)
63-67: Broaden localhost dev allowance to include 127.0.0.1 and ::1.Covers common local setups beyond “localhost”.
Apply:
- if (process.env.NODE_ENV !== 'production' && origin.includes('localhost')) { + if ( + process.env.NODE_ENV !== 'production' && + /^(https?:\/\/)?(localhost|127\.0\.0\.1|\[::1\])(?::\d+)?$/i.test(origin) + ) { return callback(null, true); }
69-71: Avoid noisy logs in production.Gate the log behind non-prod or use a logger with levels.
- console.log('Blocked origin:', origin); + if (process.env.NODE_ENV !== 'production') { + console.log('Blocked origin:', origin); + }
73-76: Remove forbidden/ineffective CORS headers.
- Request header “Cookie” cannot be set by JS; removing it avoids confusion.
- “Set-Cookie” cannot be exposed to JS; Access-Control-Expose-Headers won’t make it readable.
- methods: ['GET', 'POST', 'PUT', 'DELETE', 'PATCH', 'OPTIONS'], - allowedHeaders: ['Content-Type', 'Authorization', 'Cookie', 'X-Requested-With'], - exposedHeaders: ['Set-Cookie'] + methods: ['GET', 'POST', 'PUT', 'DELETE', 'PATCH', 'OPTIONS'], + allowedHeaders: ['Content-Type', 'Authorization', 'X-Requested-With']
45-47: Trim/clean ORIGIN env list.Prevents subtle mismatches due to whitespace or empty entries.
-const allowedOrigins = Array.from( - new Set([...(process.env.ORIGIN?.split(',') || []), 'http://localhost:3000', 'http://localhost:8000']) -); +const allowedOrigins = Array.from( + new Set([ + ...((process.env.ORIGIN?.split(',').map((s) => s.trim()).filter(Boolean)) || []), + 'http://localhost:3000', + 'http://localhost:8000' + ]) +);src/utils/jwt.ts (1)
27-27: Use the existingisProdconstant for consistency.Minor readability win; no behavior change.
- sameSite: process.env.NODE_ENV === 'production' ? 'none' : 'lax', + sameSite: isProd ? 'none' : 'lax',Also applies to: 37-37
src/controllers/user.controller.ts (1)
245-257: Use a definitive cookie clear (expires epoch + maxAge: 0).
maxAge: 1can be flaky. Setting both ensures removal across browsers while keeping attributes aligned.- res.cookie('access_token', '', { - secure: process.env.NODE_ENV === 'production', - sameSite: process.env.NODE_ENV === 'production' ? 'none' : 'lax', - httpOnly: true, - maxAge: 1, - path: '/' - }); + res.cookie('access_token', '', { + secure: process.env.NODE_ENV === 'production', + sameSite: process.env.NODE_ENV === 'production' ? 'none' : 'lax', + httpOnly: true, + expires: new Date(0), + maxAge: 0, + path: '/' + }); - res.cookie('refresh_token', '', { - secure: process.env.NODE_ENV === 'production', - sameSite: process.env.NODE_ENV === 'production' ? 'none' : 'lax', - httpOnly: true, - maxAge: 1, - path: '/' - }); + res.cookie('refresh_token', '', { + secure: process.env.NODE_ENV === 'production', + sameSite: process.env.NODE_ENV === 'production' ? 'none' : 'lax', + httpOnly: true, + expires: new Date(0), + maxAge: 0, + path: '/' + });
📜 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 (3)
src/app.ts(1 hunks)src/controllers/user.controller.ts(1 hunks)src/utils/jwt.ts(3 hunks)
🔇 Additional comments (2)
src/app.ts (1)
53-56: Allowing no-origin requests is OK given mobile/curl use-cases.With credentials enabled, this path is fine. No change requested.
src/utils/jwt.ts (1)
12-13: Interface extension looks good.Optional domain support is reasonable for future subdomain needs.
| sameSite: process.env.NODE_ENV === 'production' ? 'none' : 'lax', | ||
| domain: process.env.NODE_ENV === 'production' ? undefined : undefined // Let browser set domain automatically | ||
| }; |
There was a problem hiding this comment.
Remove domain: undefined — possible TS type error under strict mode.
domain?: string cannot be explicitly assigned undefined. Omit the field when not used, or make it string | undefined. Recommend omission for clarity.
export const accessTokenOptions: ITokenOptions = {
...
- sameSite: process.env.NODE_ENV === 'production' ? 'none' : 'lax',
- domain: process.env.NODE_ENV === 'production' ? undefined : undefined // Let browser set domain automatically
+ sameSite: process.env.NODE_ENV === 'production' ? 'none' : 'lax'
};
export const refreshTokenOptions: ITokenOptions = {
...
- sameSite: process.env.NODE_ENV === 'production' ? 'none' : 'lax',
- domain: process.env.NODE_ENV === 'production' ? undefined : undefined // Let browser set domain automatically
+ sameSite: process.env.NODE_ENV === 'production' ? 'none' : 'lax'
};Also applies to: 37-39
🤖 Prompt for AI Agents
In src/utils/jwt.ts around lines 27-29 and 37-39, the object literal sets
domain: undefined which can trigger a TypeScript strict-mode error because
domain?: string should be omitted rather than assigned undefined; remove the
domain: undefined entries and only include domain when you have a real string
(e.g., conditionally add domain: process.env.COOKIE_DOMAIN when NODE_ENV ===
'production'), or change the type to accept undefined if you intentionally need
the key — prefer omission for clarity.
1 participant
Summary by CodeRabbit
Bug Fixes
Chores