Harden Electron renderer security#532
Conversation
📝 WalkthroughWalkthroughTightens media permission checks to require both an allowlisted permission and a trusted renderer origin; registers a privileged local media protocol ("openscreen-media"); centralizes navigation guards to block unknown navigations and open allowed externals; maps file:// media into openscreen-media:// URLs in the renderer; adds CSP and test/runtime adjustments. ChangesApp security, navigation, and local-media handling
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches⚔️ Resolve merge conflicts
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: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f6822642b9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| return `//${url.host}${pathname}`; | ||
| } | ||
|
|
||
| return pathname; |
There was a problem hiding this comment.
Handle Windows file:///C:/... paths here the same way fromFileUrl does, otherwise pathname stays /C:/... and gets sent to readBinaryFile as an invalid local path on Windows. In this commit the fallback setPlayableUrl(mediaPath) points back to the original file:// URL while webSecurity is now enabled, so affected recordings can fail to load at all in the editor on Windows machines.
Useful? React with 👍 / 👎.
| let objectUrl: string | null = null; | ||
|
|
||
| async function loadLocalMedia() { | ||
| const result = await window.electronAPI.readBinaryFile(localPath); |
There was a problem hiding this comment.
This now loads each local video via readBinaryFile (full fs.readFile) and wraps it in a Blob before playback, which forces complete file materialization in memory rather than streaming from disk. For long recordings this can cause large memory spikes (potentially twice when both main + webcam tracks are present) and degrade or crash editor sessions.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@index.html`:
- Around line 5-8: The Content-Security-Policy meta tag
(http-equiv="Content-Security-Policy") currently uses a permissive connect-src
directive in the content attribute (connect-src 'self' data: blob: file: https:
http://localhost:* http://127.0.0.1:* ws://localhost:* ws://127.0.0.1:*) which
allows arbitrary HTTPS connections; update the connect-src value to remove the
broad "https:" and "file:" entries and instead enumerate only 'self', the
localhost dev endpoints (http://localhost:*, http://127.0.0.1:*,
ws://localhost:*, ws://127.0.0.1:*), and any explicit production API origins
your app requires (add exact hostnames), so the meta content string is hardened
while preserving necessary local/dev and production connectivity.
In `@src/components/video-editor/VideoPlayback.tsx`:
- Around line 81-90: The fileUrlToPath helper returns a leading slash for
Windows file URLs (e.g., "/C:/...") which breaks readBinaryFile lookups; update
the fileUrlToPath function so after decoding pathname it detects Windows
drive-letter paths (pattern like /^\/[A-Za-z]:[\\/]/) and strips the leading '/'
in that case, then continue with the existing host/localhost logic; reference
the fileUrlToPath function and ensure the adjusted pathname (without the extra
leading slash for drive letters) is returned so readBinaryFile can find the
file.
- Around line 116-157: Initialize playableUrl to undefined instead of mediaPath
in usePlayableMediaUrl and avoid setting the raw mediaPath into state until
you've validated it or completed the read; specifically, change
useState(mediaPath) to useState<string|undefined>(undefined), and only call
setPlayableUrl(mediaPath) as a deliberate fallback after determining the path is
not a blocked local file (i.e., when getReadableMediaPath(mediaPath) is falsy or
when window.electronAPI is unavailable) or after readBinaryFile fails—this
prevents the <video> from trying to load the blocked local path before the blob
URL is ready; keep the existing loadLocalMedia flow, cleanup of objectUrl, and
use the same symbols: usePlayableMediaUrl, getReadableMediaPath,
window.electronAPI.readBinaryFile, objectUrl, and setPlayableUrl.
🪄 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: 9b098cf8-7543-4cd5-bfda-e848f84411e4
📒 Files selected for processing (7)
electron/main.tselectron/windows.tsindex.htmlsrc/components/video-editor/VideoPlayback.tsxsrc/hooks/useScreenRecorder.tssrc/main.tsxtests/e2e/gif-export.spec.ts
| function usePlayableMediaUrl(mediaPath?: string): string | undefined { | ||
| const [playableUrl, setPlayableUrl] = useState(mediaPath); | ||
|
|
||
| useEffect(() => { | ||
| if (!mediaPath) { | ||
| setPlayableUrl(undefined); | ||
| return; | ||
| } | ||
|
|
||
| const readablePath = getReadableMediaPath(mediaPath); | ||
| if (!readablePath || !window.electronAPI?.readBinaryFile) { | ||
| setPlayableUrl(mediaPath); | ||
| return; | ||
| } | ||
| const localPath = readablePath; | ||
|
|
||
| let canceled = false; | ||
| let objectUrl: string | null = null; | ||
|
|
||
| async function loadLocalMedia() { | ||
| const result = await window.electronAPI.readBinaryFile(localPath); | ||
| if (canceled) return; | ||
|
|
||
| if (!result.success || !result.data) { | ||
| setPlayableUrl(mediaPath); | ||
| return; | ||
| } | ||
|
|
||
| const blob = new Blob([result.data], { type: getVideoMimeType(localPath) }); | ||
| objectUrl = URL.createObjectURL(blob); | ||
| setPlayableUrl(objectUrl); | ||
| } | ||
|
|
||
| void loadLocalMedia(); | ||
|
|
||
| return () => { | ||
| canceled = true; | ||
| if (objectUrl) { | ||
| URL.revokeObjectURL(objectUrl); | ||
| } | ||
| }; | ||
| }, [mediaPath]); |
There was a problem hiding this comment.
Don’t feed the blocked local path into <video> first
useState(mediaPath) plus the setPlayableUrl(mediaPath) fallback means local files still get rendered as raw paths before readBinaryFile finishes, and again on read failure. With webSecurity back on, that’s exactly the load Chromium rejects, so the hidden video can fire onError before the blob URL arrives. kinda cursed race.
safer loading flow
function usePlayableMediaUrl(mediaPath?: string): string | undefined {
- const [playableUrl, setPlayableUrl] = useState(mediaPath);
+ const [playableUrl, setPlayableUrl] = useState<string | undefined>(() => {
+ if (!mediaPath) return undefined;
+ return getReadableMediaPath(mediaPath) ? undefined : mediaPath;
+ });
useEffect(() => {
if (!mediaPath) {
setPlayableUrl(undefined);
return;
@@
const readablePath = getReadableMediaPath(mediaPath);
if (!readablePath || !window.electronAPI?.readBinaryFile) {
setPlayableUrl(mediaPath);
return;
}
+ setPlayableUrl(undefined);
const localPath = readablePath;
@@
- if (!result.success || !result.data) {
- setPlayableUrl(mediaPath);
+ if (!result.success || !result.data) {
+ setPlayableUrl(undefined);
return;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/video-editor/VideoPlayback.tsx` around lines 116 - 157,
Initialize playableUrl to undefined instead of mediaPath in usePlayableMediaUrl
and avoid setting the raw mediaPath into state until you've validated it or
completed the read; specifically, change useState(mediaPath) to
useState<string|undefined>(undefined), and only call setPlayableUrl(mediaPath)
as a deliberate fallback after determining the path is not a blocked local file
(i.e., when getReadableMediaPath(mediaPath) is falsy or when window.electronAPI
is unavailable) or after readBinaryFile fails—this prevents the <video> from
trying to load the blocked local path before the blob URL is ready; keep the
existing loadLocalMedia flow, cleanup of objectUrl, and use the same symbols:
usePlayableMediaUrl, getReadableMediaPath, window.electronAPI.readBinaryFile,
objectUrl, and setPlayableUrl.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@electron/ipc/handlers.ts`:
- Around line 103-116: The handler registered with protocol.handle for
LOCAL_MEDIA_PROTOCOL (the function starting with
parseLocalMediaPath/normalizeVideoSourcePath and validation via isPathAllowed
and hasAllowedImportVideoExtension) must forward the incoming request headers to
net.fetch instead of calling net.fetch(fileUrl) with no headers; extract the
headers from the request (request.headers or request.headers.getEntries()),
convert them into a plain headers object (ensuring Range and related headers are
preserved), and pass that object as the second argument to
net.fetch(pathToFileURL(normalizedPath).toString(), { headers: /* forwarded
headers */ }) so byte-range/seek semantics work correctly. Ensure header keys
and values are preserved and handle empty/undefined headers gracefully.
In `@src/components/video-editor/VideoPlayback.tsx`:
- Around line 95-102: getPlayableMediaPath is double-wrapping URLs (e.g., when
mediaPath is already a transformed URL like "openscreen-media://..." or already
prefixed with LOCAL_MEDIA_PROTOCOL), producing a broken nested URL; fix by
early-returning mediaPath when it already starts with the local protocol or
known transformed schemes before calling getReadableMediaPath/encoding. Update
the guard in getPlayableMediaPath to check for prefixes (referencing
getPlayableMediaPath, getReadableMediaPath and LOCAL_MEDIA_PROTOCOL) and only
perform encodeURIComponent wrapping when the path is not already a transformed
URL.
🪄 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: 8a00a2d4-0ada-490c-8aaa-487d3c86bbc1
📒 Files selected for processing (4)
electron/ipc/handlers.tselectron/main.tsindex.htmlsrc/components/video-editor/VideoPlayback.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- index.html
- electron/main.ts
| protocol.handle(LOCAL_MEDIA_PROTOCOL, async (request) => { | ||
| const requestedPath = parseLocalMediaPath(request.url); | ||
| const normalizedPath = normalizeVideoSourcePath(requestedPath); | ||
|
|
||
| if ( | ||
| !normalizedPath || | ||
| !isPathAllowed(normalizedPath) || | ||
| !hasAllowedImportVideoExtension(normalizedPath) | ||
| ) { | ||
| return new Response("Not found", { status: 404 }); | ||
| } | ||
|
|
||
| return net.fetch(pathToFileURL(normalizedPath).toString()); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Electron 41, when using protocol.handle(), does net.fetch(url) automatically preserve incoming Range headers from request, or must handlers pass request.headers explicitly?
💡 Result:
In Electron 41, when using protocol.handle, calling net.fetch(url) with just a URL string does NOT automatically preserve incoming Range headers (or other request headers) from the handler's request object. Handlers must explicitly pass the request headers, typically by calling net.fetch(request, { bypassCustomProtocolHandlers: true }) where request is the GlobalRequest object received by the handler, or by constructing a new fetch with { method: request.method, headers: request.headers, body: request.body }.
Citations:
- 1: https://electronjs.org/docs/latest/api/protocol
- 2: https://www.electronjs.org/docs/api/net/
- 3: https://www.electronjs.org/docs/latest/api/structures/protocol-request
- 4: https://www.electrondelta.com/protocol.html
- 5: feat: net.fetch() supports custom protocols electron/electron#36606
forward request headers to preserve media seek/range semantics.
line 115 calling bare net.fetch(fileUrl) drops Range and other headers from the incoming request. electron doesn't auto-preserve them—you gotta pass headers explicitly. lowkey risky for seek/scrub on bigger files since the backend won't know you're asking for a byte range.
nit: cleaner proxy
protocol.handle(LOCAL_MEDIA_PROTOCOL, async (request) => {
const requestedPath = parseLocalMediaPath(request.url);
const normalizedPath = normalizeVideoSourcePath(requestedPath);
@@
- return net.fetch(pathToFileURL(normalizedPath).toString());
+ return net.fetch(pathToFileURL(normalizedPath).toString(), {
+ method: request.method,
+ headers: request.headers,
+ });
});📝 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.
| protocol.handle(LOCAL_MEDIA_PROTOCOL, async (request) => { | |
| const requestedPath = parseLocalMediaPath(request.url); | |
| const normalizedPath = normalizeVideoSourcePath(requestedPath); | |
| if ( | |
| !normalizedPath || | |
| !isPathAllowed(normalizedPath) || | |
| !hasAllowedImportVideoExtension(normalizedPath) | |
| ) { | |
| return new Response("Not found", { status: 404 }); | |
| } | |
| return net.fetch(pathToFileURL(normalizedPath).toString()); | |
| }); | |
| protocol.handle(LOCAL_MEDIA_PROTOCOL, async (request) => { | |
| const requestedPath = parseLocalMediaPath(request.url); | |
| const normalizedPath = normalizeVideoSourcePath(requestedPath); | |
| if ( | |
| !normalizedPath || | |
| !isPathAllowed(normalizedPath) || | |
| !hasAllowedImportVideoExtension(normalizedPath) | |
| ) { | |
| return new Response("Not found", { status: 404 }); | |
| } | |
| return net.fetch(pathToFileURL(normalizedPath).toString(), { | |
| method: request.method, | |
| headers: request.headers, | |
| }); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@electron/ipc/handlers.ts` around lines 103 - 116, The handler registered with
protocol.handle for LOCAL_MEDIA_PROTOCOL (the function starting with
parseLocalMediaPath/normalizeVideoSourcePath and validation via isPathAllowed
and hasAllowedImportVideoExtension) must forward the incoming request headers to
net.fetch instead of calling net.fetch(fileUrl) with no headers; extract the
headers from the request (request.headers or request.headers.getEntries()),
convert them into a plain headers object (ensuring Range and related headers are
preserved), and pass that object as the second argument to
net.fetch(pathToFileURL(normalizedPath).toString(), { headers: /* forwarded
headers */ }) so byte-range/seek semantics work correctly. Ensure header keys
and values are preserved and handle empty/undefined headers gracefully.
| function getPlayableMediaPath(mediaPath: string): string { | ||
| const readablePath = getReadableMediaPath(mediaPath); | ||
| if (!readablePath) { | ||
| return mediaPath; | ||
| } | ||
|
|
||
| return `${LOCAL_MEDIA_PROTOCOL}://local/${encodeURIComponent(readablePath)}`; | ||
| } |
There was a problem hiding this comment.
Avoid double-wrapping already-converted media URLs.
If mediaPath is already openscreen-media://..., this function re-encodes it and generates a broken nested URL (kinda cursed edge case, but real if upstream ever passes back the transformed value).
quick guard
function getPlayableMediaPath(mediaPath: string): string {
+ if (new RegExp(`^${LOCAL_MEDIA_PROTOCOL}://`, "i").test(mediaPath)) {
+ return mediaPath;
+ }
+
const readablePath = getReadableMediaPath(mediaPath);
if (!readablePath) {
return mediaPath;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/video-editor/VideoPlayback.tsx` around lines 95 - 102,
getPlayableMediaPath is double-wrapping URLs (e.g., when mediaPath is already a
transformed URL like "openscreen-media://..." or already prefixed with
LOCAL_MEDIA_PROTOCOL), producing a broken nested URL; fix by early-returning
mediaPath when it already starts with the local protocol or known transformed
schemes before calling getReadableMediaPath/encoding. Update the guard in
getPlayableMediaPath to check for prefixes (referencing getPlayableMediaPath,
getReadableMediaPath and LOCAL_MEDIA_PROTOCOL) and only perform
encodeURIComponent wrapping when the path is not already a transformed URL.
1 participant
Summary
Testing
Summary by CodeRabbit
Security
New Features
Bug Fixes
Tests