fix(security): add CSP and X-Frame-Options headers to nginx.conf - #685
Conversation
birme
left a comment
There was a problem hiding this comment.
Code Review
Verdict: Needs Changes
Summary: A clean, minimal, header-only change that faithfully implements issue #624 and correctly follows the merged HSTS precedent (always, server-level inheritance). No security backdoors, no weakened validation, no secrets, no scope creep — the external contribution is honest and well-scoped. However, the CSP is inconsistent with the CSP already shipped in index.html and is stricter in connect-src and img-src in ways that will break a real, user-facing feature (the ws:// WebSocket control channel). This is a Blocking correctness issue plus two Warnings, so it needs changes before merge.
Blocking
nginx/nginx.conf:5— The newconnect-src 'self' wss: https:omitsws:(insecure WebSocket), but the app ships a WebSocket control feature whose modal defaults tows://. Insrc/components/calls-page/connect-to-ws-modal.tsx:56the default isconst PROTOCOL = "ws://"with placeholderlocalhost:12345, andsrc/hooks/use-websocket.tsopensnew WebSocket(url)directly. Because CSP headers combine restrictively with the existing<meta http-equiv="Content-Security-Policy">inindex.html(which explicitly allowsconnect-src 'self' ws: wss: http://localhost:* https:), adding this stricter header makes the effective policy the intersection — droppingws:andhttp://localhost:*. Any user connecting to aws://endpoint (the modal's default, used for Stream Deck/Companion control per the linked docs) will be blocked by CSP. Required: align the nginxconnect-srcwith the shipped meta CSP — at minimumconnect-src 'self' ws: wss: https:(addhttp://localhost:*if local-dev serving through this nginx config is intended). Copying #624's recommendation verbatim is the root cause; #624's table predates/overlooks thews://need thatindex.htmlalready encodes.
Warnings
-
nginx/nginx.conf:5— CSP has noimg-src, so image loading falls back todefault-src 'self', which disallowsdata:URIs. The shippedindex.htmlmeta CSP explicitly grantsimg-src 'self' data:. The only currentdata:imagereference is in README.md (not runtime), so app breakage risk is low today, but this is a silent divergence from the intended policy and will bite the moment any inline SVG/data-URI icon is used. Addimg-src 'self' data:to matchindex.html. -
nginx/nginx.conf:5— Two CSPs will now be delivered for the same document: this HTTP header and the pre-existing<meta http-equiv="Content-Security-Policy">inindex.html. The browser enforces both simultaneously (most-restrictive-wins per directive), which is the mechanism behind the Blocking issue above and is confusing to maintain. Recommend consolidating to a single source of truth — either remove the meta CSP fromindex.htmland make the nginx header a superset of what the app needs, or keep only the meta tag. At minimum, the two must not contradict each other.
Suggestions
nginx/nginx.conf:5—style-src 'unsafe-inline'is necessary today for Emotion CSS-in-JS injected<style>tags and is correctly included (not a defect). Note for a future hardening pass: Emotion supports nonce-based CSP, which would let you drop'unsafe-inline'.- PR body — The claim that
location /inherits the server-level headers is correct only becauselocation /defines noadd_headerof its own (nginxadd_headerinheritance is all-or-nothing per context). If any futurelocationblock adds its own header, these three will silently stop applying there. statusCheckRollupis empty (no CI configured/triggered for this change). Sincenginx -twas not run (per PR body), a maintainer should validate the config loads before merge.
Security assessment (external-contributor scrutiny)
- No backdoors, no
eval/new Function, no hardcoded secrets or tokens. - No weakened validation — the change is purely additive security headers.
- No scope creep: exactly two headers added, matching the PR title and #624;
X-Content-Type-OptionsandReferrer-Policyare correctly deferred to PR #682. - The CSP is neither meaninglessly loose (no
'unsafe-eval', no wildcarddefault-src *, noframe-ancestors *) nor gratuitously strict beyond the twoconnect-src/img-srcgaps flagged above.X-Frame-Options: DENYis appropriate for this app.
|
Housekeeping update (daily-backlog-pr): I approved the first-time-contributor CI gate, and all checks are now green (builds, lint, prettier, tests). Thanks for the contribution. However, the |
birme
left a comment
There was a problem hiding this comment.
Reviewed via daily-backlog-pr (Phase 3.5, orphan-PR housekeeping). Right direction for #624 and CI is green, but the CSP would introduce functional regressions versus the policy already shipped in index.html's <meta http-equiv="Content-Security-Policy">:
Needs Changes
connect-src 'self' wss: https:omitsws:, which the existing meta CSP includes. Since both CSPs are enforced simultaneously (browser takes the intersection), plain-ws://connections used in dev / non-TLS deployments would be blocked. Addws:.- No
img-srcdirective, so it falls back todefault-src 'self'and drops thedata:images the existing meta CSP allows (img-src 'self' data:). Addimg-src 'self' data:. - Dual CSP hazard: this adds a second CSP via nginx while
index.htmlstill carries a meta CSP. Please remove the meta CSP (preferred, so the header is canonical) or document why both exist, to avoid silent drift.
X-Frame-Options: DENY, style-src 'unsafe-inline' (needed for Emotion), media-src blob:, and the always flags are all correct.
|
daily-backlog-pr Phase 2 re-entry: this PR is both merge-conflicting with Routing to a human/author: please rebase |
… + data-URI needs (Eyevinn#624)
8b1e93b to
2a42e77
Compare
birme
left a comment
There was a problem hiding this comment.
Code Review — LGTM
Minimal, well-targeted security hardening: adds X-Frame-Options: DENY and a Content-Security-Policy to the production nginx.conf (+2/-0). CI green.
The CSP is sound — it mirrors the app's already-shipped index.html meta CSP, keeps script-src 'self' strict (no unsafe-inline/unsafe-eval), and its one relaxation (style-src 'unsafe-inline') is genuinely required by the Emotion CSS-in-JS runtime. connect-src correctly permits WebRTC signaling/API traffic across the OSC (https://$OSC_HOSTNAME/) and reverse-proxy (/) deployment models. No backdoors, secrets, weakened validation, or scope creep; nothing removed from the existing header set.
Blocking: None.
Suggestions (non-blocking, follow-up):
- The CSP now lives in two places (this
add_headerand theindex.htmlmeta tag) — a future drift hazard; consider making the nginx header the single source of truth. - Optional later hardening: add
frame-ancestors 'none',base-uri 'self',form-action 'self'.
Approving on review. Note: this repo's main currently reports mergeStateStatus: BLOCKED on every PR (an org base-branch policy beyond the visible classic branch protection), so the actual merge needs a human/admin — flagging rather than force-merging.
Problem
nginx/nginx.conf sets no browser security headers except HSTS — no clickjacking protection (X-Frame-Options) and no XSS/exfiltration baseline (Content-Security-Policy). Fixes #624.
Approach
X-Frame-Options DENYand aContent-Security-Policy(default-src 'self'; connect-src 'self' wss: https:; media-src 'self' blob:; style-src 'self' 'unsafe-inline'; script-src 'self';) to theserver {}block, both withalwaysso they apply to error responses too, following the exact values recommended in Security: Add Content-Security-Policy and X-Frame-Options headers to nginx.conf #624 and the merged HSTS precedent (security: add HSTS header to nginx config #674).X-Content-Type-Options+Referrer-Policyare covered separately by open PR fix(security): add X-Content-Type-Options and Referrer-Policy headers #682 (for Security: Add X-Content-Type-Options and Referrer-Policy headers to nginx.conf #664). Merging both completes all four headers from Security: Add Content-Security-Policy and X-Frame-Options headers to nginx.conf #624's table with no conflicting lines.location /defines noadd_header, so it inherits all server-level headers per nginx block rules — no repetition needed.Tests
always;and balanced quotes, matching merged security: add HSTS header to nginx config #674 pattern.nginx -tnot run (no nginx binary in this env — same as fix(security): add X-Content-Type-Options and Referrer-Policy headers #682, manual review confirms valid syntax).Risk
Low. Header-only addition at server level; no app code touched. If the CSP proves too strict for any inline assets,
style-src 'unsafe-inline'is already allowed per the issue and can be tightened with nonces later.Fixes #624