feat: add CSP and some security headers to HTML pages#2075
feat: add CSP and some security headers to HTML pages#2075
Conversation
Add a global Nitro middleware that sets CSP and security headers (X-Content-Type-Options, X-Frame-Options, Referrer-Policy) on page responses. API routes and internal paths (/_nuxt/, /_v/, etc.) are skipped. The CSP img-src directive imports TRUSTED_IMAGE_DOMAINS from the image proxy module so the two stay in sync automatically.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
serhalp
changed the title
📝 WalkthroughWalkthroughThis pull request centralises security header management and provider API origins across the application. A new Nuxt module enforces security headers including Content-Security-Policy, X-Content-Type-Options, X-Frame-Options, and Referrer-Policy. Provider API origins are extracted into a shared utility module with GIT_PROVIDER_API_ORIGINS and ALL_KNOWN_GIT_API_ORIGINS exports, allowing components to reference centralised constants instead of hardcoded URLs. TRUSTED_IMAGE_DOMAINS is exported as a public constant. End-to-end tests validate security header delivery and detect CSP violations. Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 (2)
modules/security-headers.ts (1)
35-37: Consider scoping localhostconnect-srcto dev or a feature flag.Allowing
http://127.0.0.1:*on all builds broadens the outbound policy surface. If this is only needed for local CLI integration in specific scenarios, gate it to reduce exposure.Possible narrowing diff
+ const localhostConnectSrc = nuxt.options.dev ? ['http://127.0.0.1:*'] : [] + const connectSrc = [ "'self'", 'https://*.algolia.net', 'https://registry.npmjs.org', 'https://api.npmjs.org', 'https://npm.antfu.dev', ...ALL_KNOWN_GIT_API_ORIGINS, // Local CLI connector (npmx CLI communicates via localhost) - 'http://127.0.0.1:*', + ...localhostConnectSrc, ].join(' ')server/utils/image-proxy.ts (1)
32-32: Harden TRUSTED_IMAGE_DOMAINS as read-only constant to prevent accidental mutation.
TRUSTED_IMAGE_DOMAINSis a security-sensitive constant shared across modules. Although current usage is read-only, exporting as a mutable array risks future code accidentally widening the trust boundary viapush(),splice(), or similar operations. Applyas constorObject.freeze()to enforce immutability.Suggested hardening
-export const TRUSTED_IMAGE_DOMAINS = [ +export const TRUSTED_IMAGE_DOMAINS = [ // First-party 'npmx.dev', ... -] +] as const
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bf5a1962-4e1b-45ce-82ba-9996da979595
📒 Files selected for processing (6)
app/composables/useRepoMeta.tsmodules/security-headers.tsserver/utils/image-proxy.tsshared/utils/git-providers.tstest/e2e/security-headers.spec.tstest/e2e/test-utils.ts
| nuxt.options.routeRules['/**'] = { | ||
| ...nuxt.options.routeRules['/**'], | ||
| headers: { | ||
| 'X-Content-Type-Options': 'nosniff', | ||
| 'X-Frame-Options': 'DENY', | ||
| 'Referrer-Policy': 'strict-origin-when-cross-origin', | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether other catch-all headers exist and could be overwritten.
rg -n --type=ts -C4 "routeRules"
rg -n --type=ts -C4 "['\"]/\\*\\*['\"]\\s*:"
rg -n --type=ts -C3 "headers\\s*:"Repository: npmx-dev/npmx.dev
Length of output: 29908
Merge existing headers instead of clobbering them when setting catch-all route rules.
The current code spreads ...nuxt.options.routeRules['/**'] at the object level but then immediately overwrites the headers property, discarding any pre-existing headers. Use optional chaining to safely merge the new headers with any existing ones.
Proposed merge-safe fix
- nuxt.options.routeRules['/**'] = {
- ...nuxt.options.routeRules['/**'],
- headers: {
- 'X-Content-Type-Options': 'nosniff',
- 'X-Frame-Options': 'DENY',
- 'Referrer-Policy': 'strict-origin-when-cross-origin',
- },
- }
+ const existingCatchAll = nuxt.options.routeRules['/**']
+ nuxt.options.routeRules['/**'] = {
+ ...existingCatchAll,
+ headers: {
+ ...(existingCatchAll?.headers ?? {}),
+ 'X-Content-Type-Options': 'nosniff',
+ 'X-Frame-Options': 'DENY',
+ 'Referrer-Policy': 'strict-origin-when-cross-origin',
+ },
+ }1 participant
🔗 Linked issue
N/A but discussed on Discord here: https://discord.com/channels/1464542801676206113/1465055739293991033/1481201825242681394
🧭 Context
We're missing a few basic security protections such as a Content Security Policy on HTML responses,
X-Frame-Options, etc.You can see the result of a basic scan here: https://developer.mozilla.org/en-US/observatory/analyze?host=main.npmx.dev.
📚 Description
Add a CSP to the HTML layout and a few security headers (
X-Content-Type-Options,X-Frame-Options,Referrer-Policy) on server responses.Adding the CSP to the HTML layout directly with a
<meta>tag is way simpler than trying to configure a response header conditionally only on html responses while excluding API responses, static assets, special internal paths, and whatever else, and then setting the CSP on pre-rendered pages also by either configuring static headers for those or using the<meta>approach only for those 😓.The CSP
img-srcdirective importsTRUSTED_IMAGE_DOMAINSfrom the image proxy module so the two stay in sync automatically, and similarly for supported git hosts.Warning
It's unlikely I found all the hosts we need to allowlist. Please help me find any missing ones!
This PR also adds an e2e test that should fail on future PRs if a request is blocked by a CSP violation in the app's main happy path.