security(CORE-1126): tighten CSP on local renderer windows#3316
security(CORE-1126): tighten CSP on local renderer windows#3316jeanfbrito wants to merge 4 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent review details⏰ 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)
🔇 Additional comments (1)
WalkthroughThree HTML files in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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. Review rate limit: 4/8 reviews remaining, refill in 24 minutes and 50 seconds.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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/public/index.html`:
- Line 5: The meta-delivered CSP includes frame-ancestors which is ignored for
clickjacking protection; remove the frame-ancestors directive from the <meta>
tag in src/public/index.html and instead inject it as an HTTP response header
from the Electron main process by adding/adjusting a handler like
session.defaultSession.webRequest.onHeadersReceived (or
webContents.session.webRequest.onHeadersReceived) to append/replace the
Content-Security-Policy header with a value that includes frame-ancestors
'none'; ensure the header name is "Content-Security-Policy" and that other CSP
directives from the meta tag are preserved when building the header value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 318c385b-6af6-491b-9cb9-d1b50953c516
📒 Files selected for processing (3)
src/public/index.htmlsrc/public/log-viewer-window.htmlsrc/public/video-call-window.html
📜 Review details
⏰ 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). (4)
- GitHub Check: check (ubuntu-latest)
- GitHub Check: check (windows-latest)
- GitHub Check: Analyze (actions)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
src/public/log-viewer-window.html (1)
5-5: Sameframe-ancestorsmeta-CSP caveat as the shared policy.Tracking this at the root comment on
src/public/index.htmlLine 5 to avoid duplication.src/public/video-call-window.html (1)
5-5: CSP does not restrict webview guest URLs in Electron.Page-level CSP on the embedder (including
default-src 'self') does not restrict<webview>guest URL loading. Electron's webview operates in a separate security context. The code properly validates the guest URL viavalidateVideoCallUrl()before assignment, which is the appropriate security layer.
| <head> | ||
| <meta charset="utf-8" /> | ||
| <meta http-equiv="Content-Security-Policy" content="script-src 'self'"> | ||
| <meta http-equiv="Content-Security-Policy" content="default-src 'self'; script-src 'self'; style-src 'self' 'unsafe-inline'; img-src 'self' data:; font-src 'self' data:; connect-src 'self'; object-src 'none'; base-uri 'self'; form-action 'self'; frame-ancestors 'none'"> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify whether CSP is also injected as an HTTP response header at runtime.
rg -nP -C3 'onHeadersReceived|Content-Security-Policy|webRequest|setHeader' --type=js --type=tsRepository: RocketChat/Rocket.Chat.Electron
Length of output: 702
🌐 Web query:
Is the CSP directive frame-ancestorsenforced when Content-Security-Policy is delivered via (Chromium/Electron context)?
💡 Result:
No, the frame-ancestors directive is not enforced when Content-Security-Policy is delivered via in Chromium or Electron. Browsers, including Chromium-based ones like Chrome and Electron, ignore the frame-ancestors directive (along with report-uri and sandbox) in meta-delivered CSP policies, as required by specification. This behavior is observed in console warnings like "The Content Security Policy directive 'frame-ancestors' is ignored when delivered via a element."
Citations:
- 1: https://stackoverflow.com/questions/53094845/content-security-policy-frame-ancestors-directive-not-working-in-meta-elemen
- 2: https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/Content-Security-Policy/frame-ancestors
- 3: https://content-security-policy.com/frame-ancestors
- 4: https://w3.org/TR/CSP3
- 5: https://stackoverflow.com/questions/52505525/content-security-policy-directive-frame-ancestors-missing-but-there
- 6: Unable to frame site whose CSP contains
frame-ancestors *inside <iframe> or <webview> electron/electron#26369 - 7: https://stackoverflow.com/questions/51969512/define-csp-http-header-in-electron-app
🏁 Script executed:
#!/bin/bash
# Search for Electron main process files that might set CSP headers
fd -e js -e ts -e mjs | grep -E '(main|preload|app|background)\.(js|ts|mjs)$' | head -20Repository: RocketChat/Rocket.Chat.Electron
Length of output: 555
🏁 Script executed:
#!/bin/bash
# Search for response header configuration or session CSP in source files
rg -nP 'session\.|\.webRequest|\.onHeadersReceived|Content-Security-Policy' --type=js --type=ts src/ 2>/dev/null | head -50Repository: RocketChat/Rocket.Chat.Electron
Length of output: 2863
🏁 Script executed:
#!/bin/bash
# Search for webRequest handlers and response header manipulation
rg -nP 'webRequest|onHeadersReceived|onBeforeSendHeaders|responseHeaders' --type=js --type=ts src/Repository: RocketChat/Rocket.Chat.Electron
Length of output: 57
🏁 Script executed:
#!/bin/bash
# Check main.ts for CSP configuration
head -100 src/main.ts && echo "..." && tail -100 src/main.tsRepository: RocketChat/Rocket.Chat.Electron
Length of output: 5800
🏁 Script executed:
#!/bin/bash
# Check app.ts for CSP or response header configuration
cat -n src/app/main/app.ts | head -150Repository: RocketChat/Rocket.Chat.Electron
Length of output: 5764
frame-ancestors in meta-delivered CSP is ignored and will not prevent clickjacking.
Move this directive to an HTTP response header set by the Electron main process (e.g., via session.webRequest.onHeadersReceived or equivalent header injection) to enforce the anti-framing policy.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/public/index.html` at line 5, The meta-delivered CSP includes
frame-ancestors which is ignored for clickjacking protection; remove the
frame-ancestors directive from the <meta> tag in src/public/index.html and
instead inject it as an HTTP response header from the Electron main process by
adding/adjusting a handler like
session.defaultSession.webRequest.onHeadersReceived (or
webContents.session.webRequest.onHeadersReceived) to append/replace the
Content-Security-Policy header with a value that includes frame-ancestors
'none'; ensure the header name is "Content-Security-Policy" and that other CSP
directives from the meta tag are preserved when building the header value.
`connect-src 'self'` blocked the fetch() calls in src/servers/renderer.ts that probe a Rocket.Chat server's home page and /api/info endpoint when adding or resolving a server. Widen connect-src to https: http: ws: wss: so server resolution works against both HTTPS and plain-HTTP hosts. video-call-window.html and log-viewer-window.html are unchanged — neither window issues remote fetch() calls from its renderer JS.
Fix: connect-src regression (found by Codex review)A Codex review identified a P1 regression introduced by this PR:
With Fix (commit 11798d8): Widened
Verification: lint clean, |
Electron <webview> elements may inherit CSP restrictions from the host page in certain renderer modes. Both video-call-window.html (Jitsi) and index.html (RC server webviews) set webview.src to remote http/https URLs. Adding frame-src and child-src https: http: ensures those webview loads are not blocked by the host page CSP.
|
Codex review v2 finding addressed (commit 6f00ea4) P1: frame-src / child-src missing from webview host pagesElectron
Fix applied to both files: Both http: and https: included to match Verification: |
Per W3C spec, frame-ancestors directive is only recognized in HTTP response headers. When delivered via <meta http-equiv>, it is silently ignored by Chromium. These are file:// renderer windows in an Electron desktop app with no realistic embedding vector — the directive provided false security.
Codex v4 P2 Finding AddressedIssue: Scope: Meta CSP in renderer windows ( Fix: Removed Verification:
Final CSP (all three files): |
1 participant
Summary
Addresses Doyensec audit finding ROC-Q422-4 — Weak CSP Directives Set In Main Renderer.
Applies a hardened, tailored
Content-Security-Policymeta tag to all three local renderer HTML files.Before / After
src/public/index.htmlBefore:
After:
(Replaced
frame-src 'none'with explicitfont-src 'self' data:'—frame-srcis redundant whendefault-src 'self'is present;font-src data:covers icon fonts loaded via data URIs fromrocketchat.css.)src/public/video-call-window.htmlBefore:
script-src 'self'After:
src/public/log-viewer-window.htmlBefore:
script-src 'self'After:
Why each directive
default-src 'self'script-src 'self'style-src 'self' 'unsafe-inline'style=attributes and<style>blocksimg-src 'self' data:error.cssusesdata:background-imagefont-src 'self' data:rocketchat.css) loads fonts via data URIsconnect-src 'self'object-src 'none'<object>/<embed>/<applet>in repobase-uri 'self'form-action 'self'frame-ancestors 'none'Out of scope (not in this PR)
nodeIntegration/contextIsolationhardening — covered by ROC-Q422-5 (separate ticket). ChangingwebPreferencesis a larger refactor with regression risk; intentionally deferred.script-src— current'self'restriction is already a meaningful improvement. Nonce-based allowlisting can be added in a follow-up once a build pipeline for nonce injection is in place.<webview>CSP —video-call-window.htmluses a<webview>tag to load Jitsi. In Electron,<webview>is not governed by the parent window'sframe-src/child-src; its CSP is controlled separately viawebview.setAttribute('partition', ...)orwebContentssession handlers. No change towebPreferencesor session handlers in this PR.Test plan
yarn lint— clean (no ESLint errors)npx tsc --noEmit— clean (no TypeScript errors)yarn test— 213 tests passed, 17 suitesyarn start, open each window (main shell, video call, log viewer), open DevTools Console, and confirm zero CSP violation errors appear on load.Summary by CodeRabbit