Added security headers to prevent clickjacking on sign in #1068
Conversation
Klakurka
added
bug
WalkthroughAdds an async Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Browser
participant NextJS
Browser->>NextJS: HTTP request (any path)
Note right of NextJS #F0F7FF: next.config.js async headers() applies to `/(.*)`\nadds security response headers
NextJS-->>Browser: HTTP response with headers\nX-Frame-Options: DENY\nContent-Security-Policy: frame-ancestors 'none'
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![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 (1)
next.config.js (1)
32-42: ESLint formatting issues detected.Static analysis flagged indentation inconsistencies (expecting 2-space increments matching the existing file style) and a missing space before function parentheses on Line 32.
Apply this diff to fix formatting:
- async headers() { - return [ - { - source: '/(.*)', - headers: [ - { key: 'X-Frame-Options', value: 'DENY' }, - { key: 'Content-Security-Policy', value: "frame-ancestors 'self'" }, - ], - }, - ] - }, + async headers () { + return [ + { + source: '/(.*)', + headers: [ + { key: 'X-Frame-Options', value: 'DENY' }, + { key: 'Content-Security-Policy', value: "frame-ancestors 'self'" }, + ], + }, + ] + },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
next.config.js(1 hunks)
🧰 Additional context used
🪛 ESLint
next.config.js
[error] 32-32: Expected indentation of 4 spaces but found 6.
(indent)
[error] 32-32: Missing space before function parentheses.
(space-before-function-paren)
[error] 33-33: Expected indentation of 6 spaces but found 8.
(indent)
[error] 34-34: Expected indentation of 8 spaces but found 10.
(indent)
[error] 35-35: Expected indentation of 10 spaces but found 12.
(indent)
[error] 36-36: Expected indentation of 10 spaces but found 12.
(indent)
[error] 37-37: Expected indentation of 12 spaces but found 14.
(indent)
[error] 38-38: Expected indentation of 12 spaces but found 14.
(indent)
[error] 39-39: Expected indentation of 10 spaces but found 12.
(indent)
[error] 40-40: Expected indentation of 8 spaces but found 10.
(indent)
[error] 41-41: Expected indentation of 6 spaces but found 8.
(indent)
[error] 42-42: Expected indentation of 4 spaces but found 6.
(indent)
⏰ 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). (1)
- GitHub Check: Run Tests
🔇 Additional comments (1)
next.config.js (1)
35-35: Verify that applying clickjacking protection to all routes is intended.The pattern
/(.*)applies these headers globally to every route in the application. Since the PR title mentions "prevent clickjacking on sign in," confirm whether the protection should be scoped specifically to authentication routes (e.g.,/signin,/login) or if global application is the desired behavior.If you need to scope this to specific routes only, here's an example targeting sign-in pages:
return [ { - source: '/(.*)', + source: '/(signin|login|auth)/:path*', headers: [ { key: 'X-Frame-Options', value: 'DENY' }, { key: 'Content-Security-Policy', value: "frame-ancestors 'self'" }, ], }, ]
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
next.config.js(1 hunks)
🧰 Additional context used
🪛 ESLint
next.config.js
[error] 32-32: Expected indentation of 4 spaces but found 6.
(indent)
[error] 32-32: Missing space before function parentheses.
(space-before-function-paren)
[error] 33-33: Expected indentation of 6 spaces but found 8.
(indent)
[error] 34-34: Expected indentation of 8 spaces but found 10.
(indent)
[error] 35-35: Expected indentation of 10 spaces but found 12.
(indent)
[error] 36-36: Expected indentation of 10 spaces but found 12.
(indent)
[error] 37-37: Expected indentation of 12 spaces but found 14.
(indent)
[error] 38-38: Expected indentation of 12 spaces but found 14.
(indent)
[error] 39-39: Expected indentation of 10 spaces but found 12.
(indent)
[error] 40-40: Expected indentation of 8 spaces but found 10.
(indent)
[error] 41-41: Expected indentation of 6 spaces but found 8.
(indent)
[error] 42-42: Expected indentation of 4 spaces but found 6.
(indent)
🔇 Additional comments (2)
next.config.js (2)
32-42: Security headers correctly aligned!The clickjacking protection is now properly configured with both headers using the most restrictive settings (
DENYand'none'). This addresses the contradiction flagged in the previous review.
35-35: No legitimate iframe use cases detected — security headers are appropriate.Comprehensive codebase search found no iframe elements, postMessage calls, or embedding features. The PayButton Widget reference is an internal UI component, not an embeddable widget. The
source: '/(.*)'pattern correctly applies the restrictive headers (X-Frame-Options: DENYandframe-ancestors 'none') globally across all routes, which is appropriate for this payment processing application.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
next.config.js(1 hunks)
🧰 Additional context used
🪛 ESLint
next.config.js
[error] 32-32: Missing space before function parentheses.
(space-before-function-paren)
[error] 43-43: Missing space before function parentheses.
(space-before-function-paren)
🔇 Additional comments (2)
next.config.js (2)
3-22: LGTM - Phase-based configuration infrastructure added.The phase constants and conditional logic are correctly implemented following Next.js patterns. While the phase variables (isDev, isBuild, isProd, isStaging) aren't currently used to conditionally apply the security headers, this provides good infrastructure for future environment-specific configuration.
32-42: Verify that all-routes scope is intentional.The security headers are correctly configured to prevent clickjacking. However, the PR title mentions "prevent clickjacking on sign in" but the implementation applies these headers to all routes via the
/(.*)pattern.While applying security headers broadly is often recommended as defense-in-depth, please confirm this was intentional and not an oversight. If the headers should only apply to sign-in routes, update the source pattern:
{ - source: '/(.*)', + source: '/signin/:path*', headers: [
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
next.config.js(1 hunks)
🔇 Additional comments (1)
next.config.js (1)
32-42: Verify that applying anti-framing headers globally aligns with PR intent.The codebase search found no current iframe usage, postMessage communication, or integrations requiring iframe embedding. However, the PR title mentions "prevent clickjacking on sign in" while the implementation applies headers globally via
source: '/(.*)'.Confirm whether:
- The global scope is intentional (if so, this is acceptable)
- Headers should be scoped to authentication routes only (e.g.,
/auth/:path*,/signin)
Description
Reported via contact email
Test plan
Forwarded separately.
Summary by CodeRabbit