fix: restrict HTML preview network access - #574
Conversation
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughHTML previews now include a restrictive Content-Security-Policy meta tag in their sandboxed iframe documents. A new test verifies CSP directives, sandboxing, and transformed script output. ChangesHTML Preview Security
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
🧹 Nitpick comments (2)
src/ui/file-preview/src/components/html-renderer.ts (1)
10-10: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider using the shared
escapeHtmlfromsrc/ui/shared/escape-html.ts.The
escapeHtmlimport from./highlighting.jsappears functionally equivalent to the shared helper insrc/ui/shared/escape-html.ts(both escape&,<,>,",'). Using the shared module would improve consistency and reduce duplication across the codebase. This is a nice-to-have and can be deferred.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ui/file-preview/src/components/html-renderer.ts` at line 10, Update the html-renderer import to use the shared escapeHtml helper from the shared module instead of the local highlighting export, preserving all existing call sites and behavior.test/test-html-preview-csp.js (1)
6-10: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd assertions for remaining CSP directives to prevent silent regressions.
The test covers
connect-srcandform-actionbut notdefault-src 'none',script-src 'unsafe-inline',style-src 'unsafe-inline',base-uri 'none',img-src, ormedia-src. Adding assertions for these would ensure all directives are locked in and prevent accidental removal in future edits.♻️ Suggested additional assertions
assert.ok(rendered.html.includes('Content-Security-Policy')); +assert.ok(rendered.html.includes('default-src &`#39`;none&`#39`;')); +assert.ok(rendered.html.includes('script-src &`#39`;unsafe-inline&`#39`;')); +assert.ok(rendered.html.includes('style-src &`#39`;unsafe-inline&`#39`;')); +assert.ok(rendered.html.includes('base-uri &`#39`;none&`#39`;')); assert.ok(rendered.html.includes('connect-src &`#39`;none&`#39`;')); assert.ok(rendered.html.includes('form-action &`#39`;none&`#39`;')); assert.ok(rendered.html.includes('sandbox="allow-scripts allow-forms allow-popups"')); assert.ok(rendered.html.includes('<script>fetch("https://example.com")</script>'));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/test-html-preview-csp.js` around lines 6 - 10, Add assertions in the HTML preview CSP test alongside the existing checks to verify the rendered policy includes default-src 'none', script-src 'unsafe-inline', style-src 'unsafe-inline', base-uri 'none', img-src, and media-src. Match the rendered HTML’s existing escaping format and preserve the current assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/ui/file-preview/src/components/html-renderer.ts`:
- Line 10: Update the html-renderer import to use the shared escapeHtml helper
from the shared module instead of the local highlighting export, preserving all
existing call sites and behavior.
In `@test/test-html-preview-csp.js`:
- Around line 6-10: Add assertions in the HTML preview CSP test alongside the
existing checks to verify the rendered policy includes default-src 'none',
script-src 'unsafe-inline', style-src 'unsafe-inline', base-uri 'none', img-src,
and media-src. Match the rendered HTML’s existing escaping format and preserve
the current assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1ca3f9ef-ca90-4189-9c3e-e9ab270aa399
📒 Files selected for processing (2)
src/ui/file-preview/src/components/html-renderer.tstest/test-html-preview-csp.js
1 participant
Summary
srcdocSecurity boundary
Preview content is attacker-controlled HTML. The iframe sandbox isolates its origin but does not prevent
fetch, XHR, WebSocket, form, image, or script requests. The CSP now limits scripts and styles to inline preview content, images/media todata:andblob:, and denies outbound connections and form actions.Fixes #507.
Validation
npm run buildnode test/test-html-preview-csp.jsgit diff --checkSummary by CodeRabbit
Security Enhancements
Bug Fixes
Tests