fix(desktop): fix browser pane not loading images#1571
Conversation
The persistent webview refactor (#1535) introduced two issues that prevented the browser pane from loading/rendering images: 1. Missing display styling on programmatically-created webview element. The old JSX-rendered <webview> had `display: flex; flex: 1` but the new createElement approach only set width/height, causing the custom element to default to `display: inline` where dimensions may not apply correctly. 2. Restrictive Content Security Policy. The renderer CSP lacked explicit `frame-src` (falling back to `default-src 'self'`) which could block the webview from loading external content. Also added `http:` and `blob:` to `img-src` for full image source compatibility.
📝 WalkthroughWalkthroughThe pull request updates the Content-Security-Policy in the desktop renderer to permit additional image sources and webview-related content, while also adding CSS flex styling properties to webview elements to improve their layout behavior. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 (2)
apps/desktop/src/renderer/index.html (1)
18-20: Useworker-srcinstead of (or alongside)child-srcfor explicit worker control.In CSP Level 3,
frame-srchas been undeprecated and continues to defer tochild-srcif not present. Aworker-srcdirective has been added, deferring tochild-srcif not present. Sinceframe-srcis now explicitly set in this policy,child-srcwill only be consulted as a fallback for workers — its frame-control role is effectively superseded. Modern best practices recommend usingworker-srcinstead ofchild-srcfor clarity and control, thoughchild-srcshould be retained for backward compatibility with older browsers.♻️ Proposed addition for explicit worker control
- - child-src 'self' blob:: Allow workers from same origin + blob workers + - worker-src 'self' blob:: Allow workers from same origin + blob workers (CSP Level 3) + - child-src 'self' blob:: Fallback for worker-src in older browsers- content="...; frame-src https: http: data: blob:; child-src 'self' blob:;" + content="...; frame-src https: http: blob:; worker-src 'self' blob:; child-src 'self' blob:;"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/index.html` around lines 18 - 20, The Content-Security-Policy meta tag currently uses child-src for worker control; add an explicit worker-src directive (e.g., worker-src 'self' blob:) alongside the existing child-src to ensure modern browsers use the intended worker policy while retaining child-src for backward compatibility—update the Content-Security-Policy meta tag content attribute in the renderer index.html to include worker-src with the same sources as child-src.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/hooks/usePersistentWebview/usePersistentWebview.ts (1)
143-144:display: flexon the webview is a no-op for its rendered content —flex: 1is the actual fix.
display: flexmakes the<webview>element itself a flex container (meaningless since the webview renders an isolated page). The property that enables it to fill the parent flex container as a flex item isflex: 1. If the original JSX used Tailwind'sflexclass (which emitsdisplay: flex), this is a faithful restore, so no action is strictly required — but if you want semantic precision,display: blockis sufficient alongsideflex: 1.♻️ Minimal alternative if you want to remove the no-op
- webview.style.display = "flex"; webview.style.flex = "1"; webview.style.width = "100%"; webview.style.height = "100%";Or if the original JSX class set
display: flexintentionally (e.g., Tailwindflex), keep it as-is — it is harmless.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/hooks/usePersistentWebview/usePersistentWebview.ts` around lines 143 - 144, The webview.style.display = "flex" is a no-op for the webview's rendered page and should be removed or changed to a semantic value; in usePersistentWebview.ts update the webview styling so only the flex item behavior remains (keep webview.style.flex = "1" and either remove the webview.style.display assignment or change it to "block") — locate the webview DOM reference in the hook where webview.style.display and webview.style.flex are set and apply this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/renderer/index.html`:
- Around line 17-20: Remove the insecure "data:" source from the
Content-Security-Policy meta tag's frame-src directive in the renderer
index.html (the meta tag with http-equiv="Content-Security-Policy"); update the
content attribute so frame-src only contains https: and http: (and blob: if
still required for other uses) — ensure you do not reintroduce "data:" anywhere
in the frame-src value while keeping other directives (script-src, style-src,
connect-src, img-src, child-src) unchanged.
---
Nitpick comments:
In `@apps/desktop/src/renderer/index.html`:
- Around line 18-20: The Content-Security-Policy meta tag currently uses
child-src for worker control; add an explicit worker-src directive (e.g.,
worker-src 'self' blob:) alongside the existing child-src to ensure modern
browsers use the intended worker policy while retaining child-src for backward
compatibility—update the Content-Security-Policy meta tag content attribute in
the renderer index.html to include worker-src with the same sources as
child-src.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/hooks/usePersistentWebview/usePersistentWebview.ts`:
- Around line 143-144: The webview.style.display = "flex" is a no-op for the
webview's rendered page and should be removed or changed to a semantic value; in
usePersistentWebview.ts update the webview styling so only the flex item
behavior remains (keep webview.style.flex = "1" and either remove the
webview.style.display assignment or change it to "block") — locate the webview
DOM reference in the hook where webview.style.display and webview.style.flex are
set and apply this change.
| - frame-src https: http: data: blob:: Allow webview browser pane to load any URL | ||
| - child-src 'self' blob:: Allow workers from same origin + blob workers | ||
| --> | ||
| <meta http-equiv="Content-Security-Policy" content="default-src 'self'; script-src 'self' 'wasm-unsafe-eval' https://*.posthog.com; style-src 'self' 'unsafe-inline'; connect-src 'self' ws: wss: %NEXT_PUBLIC_API_URL% %NEXT_PUBLIC_ELECTRIC_URL% %NEXT_PUBLIC_STREAMS_URL% https://*.posthog.com https://*.sentry.io sentry-ipc:; img-src 'self' data: https:; font-src 'self';" /> | ||
| <meta http-equiv="Content-Security-Policy" content="default-src 'self'; script-src 'self' 'wasm-unsafe-eval' https://*.posthog.com; style-src 'self' 'unsafe-inline'; connect-src 'self' ws: wss: %NEXT_PUBLIC_API_URL% %NEXT_PUBLIC_ELECTRIC_URL% %NEXT_PUBLIC_STREAMS_URL% https://*.posthog.com https://*.sentry.io sentry-ipc:; img-src 'self' data: https: http: blob:; font-src 'self'; frame-src https: http: data: blob:; child-src 'self' blob:;" /> |
There was a problem hiding this comment.
data: in frame-src enables arbitrary-HTML frames — remove it unless explicitly required.
data: URIs as a content source allows loading arbitrary content; this is insecure since an attacker can inject arbitrary data: URIs. It should be avoided and is not recommended. Allowing data: in frame-src means any code path that can influence a frame's src could inject a data:text/html,<script>...</script> payload. For the browser-pane use case (loading http:/https: sites in a webview), data: is not needed. The http: and https: sources already cover all legitimate frame/webview loading scenarios.
🛡️ Proposed fix
- - frame-src https: http: data: blob:: Allow webview browser pane to load any URL
+ - frame-src https: http: blob:: Allow webview browser pane to load any URL- <meta http-equiv="Content-Security-Policy" content="...; frame-src https: http: data: blob:; child-src 'self' blob:;" />
+ <meta http-equiv="Content-Security-Policy" content="...; frame-src https: http: blob:; child-src 'self' blob:;" />📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - frame-src https: http: data: blob:: Allow webview browser pane to load any URL | |
| - child-src 'self' blob:: Allow workers from same origin + blob workers | |
| --> | |
| <meta http-equiv="Content-Security-Policy" content="default-src 'self'; script-src 'self' 'wasm-unsafe-eval' https://*.posthog.com; style-src 'self' 'unsafe-inline'; connect-src 'self' ws: wss: %NEXT_PUBLIC_API_URL% %NEXT_PUBLIC_ELECTRIC_URL% %NEXT_PUBLIC_STREAMS_URL% https://*.posthog.com https://*.sentry.io sentry-ipc:; img-src 'self' data: https:; font-src 'self';" /> | |
| <meta http-equiv="Content-Security-Policy" content="default-src 'self'; script-src 'self' 'wasm-unsafe-eval' https://*.posthog.com; style-src 'self' 'unsafe-inline'; connect-src 'self' ws: wss: %NEXT_PUBLIC_API_URL% %NEXT_PUBLIC_ELECTRIC_URL% %NEXT_PUBLIC_STREAMS_URL% https://*.posthog.com https://*.sentry.io sentry-ipc:; img-src 'self' data: https: http: blob:; font-src 'self'; frame-src https: http: data: blob:; child-src 'self' blob:;" /> | |
| - frame-src https: http: blob:: Allow webview browser pane to load any URL | |
| - child-src 'self' blob:: Allow workers from same origin + blob workers | |
| --> | |
| <meta http-equiv="Content-Security-Policy" content="default-src 'self'; script-src 'self' 'wasm-unsafe-eval' https://*.posthog.com; style-src 'self' 'unsafe-inline'; connect-src 'self' ws: wss: %NEXT_PUBLIC_API_URL% %NEXT_PUBLIC_ELECTRIC_URL% %NEXT_PUBLIC_STREAMS_URL% https://*.posthog.com https://*.sentry.io sentry-ipc:; img-src 'self' data: https: http: blob:; font-src 'self'; frame-src https: http: blob:; child-src 'self' blob:;" /> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/index.html` around lines 17 - 20, Remove the
insecure "data:" source from the Content-Security-Policy meta tag's frame-src
directive in the renderer index.html (the meta tag with
http-equiv="Content-Security-Policy"); update the content attribute so frame-src
only contains https: and http: (and blob: if still required for other uses) —
ensure you do not reintroduce "data:" anywhere in the frame-src value while
keeping other directives (script-src, style-src, connect-src, img-src,
child-src) unchanged.
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
1 participant
Summary
<webview>to programmaticcreateElement, but lost thedisplay: flex; flex: 1styling needed for proper element sizingframe-src(falling back to restrictivedefault-src 'self') and was missinghttp:/blob:inimg-src, which could block the webview from loading external content and imagesChanges
usePersistentWebview.ts: Addeddisplay: flexandflex: 1to the programmatically-created webview element, matching the old JSX implementationindex.html(CSP): Addedhttp:andblob:toimg-srcfor full image source compatibility; added explicitframe-src https: http: data: blob:to allow the webview browser pane to load any URL; addedchild-src 'self' blob:for worker compatibilityTest Plan
Summary by CodeRabbit
Chores
Style