feat(security): IPC hardening, trusted-access contract & app-media protocol#359
feat(security): IPC hardening, trusted-access contract & app-media protocol#359verivusOSS-releases wants to merge 6 commits intosiddharthvaddem:mainfrom
Conversation
… error lifecycle (U01, U02, U05) U01: Add isTrustedMediaPath() and isAllowedExternalUrl() helpers to centralize security validation for renderer-driven file and URL access. U02: Apply trusted-access contract to read-binary-file (reject paths outside recordings/project dirs), open-external-url (https/http only), and reveal-in-folder (trusted paths only). U05: Route recorder error handling through full teardown path — clear media resources, sync main-process state, and stop webcam recorder instead of only updating React state. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… (U03) Re-enable webSecurity on the editor BrowserWindow. Register a custom app-media:// protocol that validates requested paths are under RECORDINGS_DIR or the app assets directory before serving, returning 403 for untrusted paths. Update assetPath.ts to use app-media:// URLs instead of raw file:// URLs in Electron builds. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… symlink safety, complete path validation, error race 1. Sanitize renderer-supplied filenames with path.basename() before writing to RECORDINGS_DIR (prevents traversal via ../../../). 2. Add isTrustedMediaPathRealpath() that resolves symlinks before checking trusted roots, used by read-binary-file. 3. Add isTrustedMediaPath checks to loadRecordedSessionForVideoPath, get-cursor-telemetry, and set-current-video-path handlers. 4. Set allowAutoFinalize.current = false in recorder error handler to prevent race with the stop listener's finalizeRecording call. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Resolve symlinks with fs.realpath() before serving files through the app-media:// custom protocol. This prevents symlink-based escapes where a symlink inside a trusted root points to a file outside it. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…es (U07) Add 16 new tests covering: - URL scheme validation (https/http allow, file/javascript/data/blob reject) - Bitrate computation with actual frame rate (threshold behavior, resolution tiers) - Vertical-stack bounded layout with webcam present - Project validation (invalid media, non-objects, missing version/editor) Total: 37 tests across 8 files, all passing. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe PR adds comprehensive security hardening for media file handling and external URL access. Changes include media path trust validation functions, a custom Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 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. 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 |
![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: 2078088a3c
ℹ️ 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".
electron/ipc/handlers.ts
Outdated
| return { success: false, message: "Video path has not been approved" }; | ||
| ipcMain.handle("set-current-video-path", async (_, videoPath: string) => { | ||
| const normalizedVideoPath = normalizeVideoSourcePath(videoPath); | ||
| if (!normalizedVideoPath || !isTrustedMediaPath(normalizedVideoPath)) { |
There was a problem hiding this comment.
open-video-file-picker still uses approveReadableVideoPath(...) to allow user-selected files outside RECORDINGS_DIR, but this guard now rejects anything not under trusted roots. In the launch flow, selecting a valid video from Downloads/Desktop succeeds in the picker and then fails in setCurrentVideoPath, so importing external videos is effectively broken despite the approval mechanism remaining in place.
Useful? React with 👍 / 👎.
electron/ipc/handlers.ts
Outdated
| ipcMain.handle("reveal-in-folder", async (_, filePath: string) => { | ||
| try { | ||
| // shell.showItemInFolder doesn't return a value, it throws on error | ||
| if (!isTrustedMediaPath(filePath)) { |
There was a problem hiding this comment.
The export flow lets users save to arbitrary locations, and the UI calls revealInFolder(saveResult.path) afterward. This new trust-root check denies paths outside recordings/project directories, so the “Show in Folder” action now fails for common destinations like Downloads even though the path came from a user-approved save dialog.
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: 10
🤖 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 234-240: sanitizeFileName currently only calls path.basename, so
embedded NULs and other unsafe characters can survive and the manifest is still
built from raw payload.screen.fileName; update sanitizeFileName to reject or
strip NULs (and other dangerous control chars) and return a canonical
safeScreenFileName, then use that safeScreenFileName everywhere (including where
the session manifest is assembled and any sibling paths under RECORDINGS_DIR) so
the manifest write and all blob writes derive from the same sanitized name and
cannot leave partial state behind; update references that use
payload.screen.fileName to instead use the returned safeScreenFileName.
- Around line 206-219: isTrustedMediaPath currently only trusts RECORDINGS_DIR
and the project dir, which breaks files explicitly approved by
approveReadableVideoPath/open-video-file-picker; update isTrustedMediaPath to
also consult the approvedPaths set (or collection used elsewhere) so that an
explicitly approved file path returns trusted. Specifically, modify the trust
check inside isTrustedMediaPath to return true if normalized matches or
startsWith any trustedRoots OR if normalized is present in approvedPaths (or
approvedPaths contains a canonical/normalized equivalent); ensure the same
change/logic is applied at the other occurrences called out (around the other
blocks that perform similar trust checks such as where read-binary-file and
set-current-video-path call it).
- Around line 275-277: The code currently checks
isTrustedMediaPath(normalizedVideoPath) then directly reads sidecar files like
`${normalizedVideoPath}.session.json` and `${normalizedVideoPath}.cursor.json`,
which can be symlinked outside trusted roots; change the flow so after
constructing the actual sidecar path you call fs.realpath (or equivalent) and
validate the resolved path with isTrustedMediaPathRealpath (the same function
used for read-binary-file) before calling read-binary-file (or reading the
file). Update both the handler that uses normalizedVideoPath and the other flow
around the same logic (the flow referenced near lines 599-601) to realpath each
sidecar file and re-run the realpath-based allow check (using
isTrustedMediaPathRealpath) prior to reading to ensure the final resolved file
is inside trusted roots.
In `@electron/main.ts`:
- Around line 343-345: Register the custom app-media scheme with privileged
permissions before the app is ready: add a top-level call to
protocol.registerSchemesAsPrivileged(...) specifying scheme 'app-media' with
privileges standard, secure, and supportFetchAPI (so fetch/URL handling works),
then leave the existing registerMediaProtocol() call inside app.whenReady().
Ensure the registerSchemesAsPrivileged invocation occurs before app.whenReady()
and references the same 'app-media' scheme used by registerMediaProtocol().
In `@electron/windows.ts`:
- Line 63: The current net.fetch call in the route handler drops incoming
request metadata and breaks Range requests; update the call that returns
net.fetch(pathToFileURL(realResolved).toString()) to forward the incoming
request's method, headers, and body (e.g., use request.method, request.headers,
request.body) so Range and other header-dependent behavior is preserved when
delegating to net.fetch; locate the usage in electron/windows.ts where
realResolved, request, and net.fetch are referenced and add those options to the
net.fetch invocation.
In `@src/hooks/useScreenRecorder.test.ts`:
- Around line 13-24: The test's computeBitrate helper accepts frameRate and
conditions on HIGH_FRAME_RATE_THRESHOLD, but the runtime in useScreenRecorder.ts
still uses TARGET_FRAME_RATE (so the boost is always applied); fix by making the
runtime computeBitrate behavior match the test: update useScreenRecorder's
bitrate calculation (the code around useScreenRecorder function lines that
reference TARGET_FRAME_RATE and HIGH_FRAME_RATE_BOOST) to accept or derive the
actual frameRate and apply HIGH_FRAME_RATE_BOOST only when frameRate >=
HIGH_FRAME_RATE_THRESHOLD, or alternatively change the test to call the real
helper from useScreenRecorder.ts so both use the same computeBitrate
implementation.
In `@src/hooks/useScreenRecorder.ts`:
- Around line 554-580: The error listener is attached too late—move the cleanup
handler to recorder creation so errors between recorder.start() and the current
listener don't bypass teardown; inside createRecorderHandle(), attach the same
"error" listener to activeScreenRecorder.recorder (the logic that sets
allowAutoFinalize.current = false, clears
screenRecorder.current/webcamRecorder.current, stops
activeWebcamRecorder.recorder if recording, calls teardownMedia(), calls
setRecording(false) and window.electronAPI?.setRecordingState(false), and clears
finalizingRecordingId.current when it matches activeRecordingId) before calling
recorder.start(), and also ensure any promise(s) like recordedBlobPromise that
may reject on error have their rejection explicitly consumed/handled in the
error path to avoid unhandled rejections.
In `@src/lib/compositeLayout.test.ts`:
- Around line 81-98: The test assumes the "vertical-stack" branch of
computeCompositeLayout honors maxContentSize but the current implementation
still uses canvasSize; update the vertical-stack branch in
computeCompositeLayout to constrain screenRect and webcamRect dimensions using
maxContentSize.width and maxContentSize.height (apply the same stacking/spacing
and rounding logic currently used for canvasSize), ensure total stacked height
(screenRect.height + webcamRect.height) is bounded by maxContentSize.height
(allowing the existing +1 rounding tolerance), and center the composed content
within the canvas so screenRect.x is > 0; keep existing behavior for other
presets unchanged.
- Around line 90-98: The test is too permissive by using
layout!.webcamRect?.height ?? 0; explicitly assert that the webcam rect exists
before measuring its height: add an assertion like
expect(layout!.webcamRect).not.toBeNull() (or truthy) and then use
layout!.webcamRect!.height in the subsequent constraint check (the code paths
using layout and webcamRect are the targets to update).
In `@src/lib/securityValidation.test.ts`:
- Around line 3-12: The test defines its own isAllowedExternalUrl which masks
regressions; extract the real isAllowedExternalUrl into a shared utility module
and have both the IPC handler and this test import that shared function instead
of redefining it. Move the current implementation (the URL parsing + protocol
check) into the new shared export, update the IPC handler to import and use that
export, and change src/lib/securityValidation.test.ts to import
isAllowedExternalUrl from the shared module so the test exercises the actual
production logic.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: e7b0738f-d3b1-4688-8836-3563629c5983
📒 Files selected for processing (9)
electron/ipc/handlers.tselectron/main.tselectron/windows.tssrc/components/video-editor/projectPersistence.test.tssrc/hooks/useScreenRecorder.test.tssrc/hooks/useScreenRecorder.tssrc/lib/assetPath.tssrc/lib/compositeLayout.test.tssrc/lib/securityValidation.test.ts
| function isTrustedMediaPath(filePath: string): boolean { | ||
| if (!filePath || typeof filePath !== "string") { | ||
| return false; | ||
| } | ||
| const normalized = normalizePath(filePath); | ||
|
|
||
| const trustedRoots: string[] = [normalizePath(RECORDINGS_DIR)]; | ||
|
|
||
| if (currentProjectPath) { | ||
| trustedRoots.push(normalizePath(path.dirname(currentProjectPath))); | ||
| } | ||
|
|
||
| return trustedRoots.some((root) => normalized === root || normalized.startsWith(root + path.sep)); | ||
| } |
There was a problem hiding this comment.
This drops the explicit-approval path entirely.
approveReadableVideoPath() / open-video-file-picker() still bless external videos, but isTrustedMediaPath() now only trusts RECORDINGS_DIR plus the current project directory. So a user-picked file from Downloads succeeds in the picker and then gets denied here by read-binary-file() / set-current-video-path(). If the new contract is “trusted roots + explicitly approved files,” approvedPaths still needs to participate in this check.
Also applies to: 548-553, 936-943
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@electron/ipc/handlers.ts` around lines 206 - 219, isTrustedMediaPath
currently only trusts RECORDINGS_DIR and the project dir, which breaks files
explicitly approved by approveReadableVideoPath/open-video-file-picker; update
isTrustedMediaPath to also consult the approvedPaths set (or collection used
elsewhere) so that an explicitly approved file path returns trusted.
Specifically, modify the trust check inside isTrustedMediaPath to return true if
normalized matches or startsWith any trustedRoots OR if normalized is present in
approvedPaths (or approvedPaths contains a canonical/normalized equivalent);
ensure the same change/logic is applied at the other occurrences called out
(around the other blocks that perform similar trust checks such as where
read-binary-file and set-current-video-path call it).
| function sanitizeFileName(fileName: string): string { | ||
| // Strip any path separators and parent-directory references to prevent traversal | ||
| const baseName = path.basename(fileName); | ||
| if (!baseName || baseName === "." || baseName === "..") { | ||
| throw new Error("Invalid file name"); | ||
| } | ||
| return baseName; |
There was a problem hiding this comment.
The sanitized filename isn’t what the manifest write uses.
sanitizeFileName() currently just basename()s, so \0 survives, and the session manifest a few lines below is still derived from raw payload.screen.fileName. A crafted name can therefore fail only at the final manifest write, after the video blobs were already written, which leaves partial state behind in RECORDINGS_DIR. Please reject/strip NULs up front and derive every sibling path from safeScreenFileName.
nit: cleaner if the sanitized name becomes the single source of truth
function sanitizeFileName(fileName: string): string {
// Strip any path separators and parent-directory references to prevent traversal
- const baseName = path.basename(fileName);
+ const baseName = path.basename(fileName.replace(/\0/g, ""));
if (!baseName || baseName === "." || baseName === "..") {
throw new Error("Invalid file name");
}
return baseName;
}
...
- const sessionManifestPath = path.join(
- RECORDINGS_DIR,
- `${path.parse(payload.screen.fileName).name}${RECORDING_SESSION_SUFFIX}`,
- );
+ const sessionManifestPath = path.join(
+ RECORDINGS_DIR,
+ `${path.parse(safeScreenFileName).name}${RECORDING_SESSION_SUFFIX}`,
+ );Also applies to: 315-322
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@electron/ipc/handlers.ts` around lines 234 - 240, sanitizeFileName currently
only calls path.basename, so embedded NULs and other unsafe characters can
survive and the manifest is still built from raw payload.screen.fileName; update
sanitizeFileName to reject or strip NULs (and other dangerous control chars) and
return a canonical safeScreenFileName, then use that safeScreenFileName
everywhere (including where the session manifest is assembled and any sibling
paths under RECORDINGS_DIR) so the manifest write and all blob writes derive
from the same sanitized name and cannot leave partial state behind; update
references that use payload.screen.fileName to instead use the returned
safeScreenFileName.
| if (!isTrustedMediaPath(normalizedVideoPath)) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Sidecar JSON reads still skip the realpath gate.
read-binary-file() moved to isTrustedMediaPathRealpath(), but these two flows still read ${video}.session.json / ${video}.cursor.json after only a logical-path allow check. A symlinked sidecar inside RECORDINGS_DIR can still resolve outside the trusted roots and get read back into the renderer. Please realpath the file you actually open and re-validate it the same way.
Also applies to: 599-601
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@electron/ipc/handlers.ts` around lines 275 - 277, The code currently checks
isTrustedMediaPath(normalizedVideoPath) then directly reads sidecar files like
`${normalizedVideoPath}.session.json` and `${normalizedVideoPath}.cursor.json`,
which can be symlinked outside trusted roots; change the flow so after
constructing the actual sidecar path you call fs.realpath (or equivalent) and
validate the resolved path with isTrustedMediaPathRealpath (the same function
used for read-binary-file) before calling read-binary-file (or reading the
file). Update both the handler that uses normalizedVideoPath and the other flow
around the same logic (the flow referenced near lines 599-601) to realpath each
sidecar file and re-run the realpath-based allow check (using
isTrustedMediaPathRealpath) prior to reading to ensure the final resolved file
is inside trusted roots.
| app.whenReady().then(async () => { | ||
| // Register custom protocol for safe local media loading | ||
| registerMediaProtocol(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the electron directory and key files
find . -type f -name "main.ts" | grep electron
find . -type f -name "windows.ts" | grep electronRepository: siddharthvaddem/openscreen
Length of output: 110
🏁 Script executed:
# Read the relevant section of electron/main.ts around lines 343-345
wc -l electron/main.tsRepository: siddharthvaddem/openscreen
Length of output: 90
🏁 Script executed:
# Look for registerMediaProtocol and registerSchemesAsPrivileged
rg "registerMediaProtocol|registerSchemesAsPrivileged" --no-heading -B 2 -A 2Repository: siddharthvaddem/openscreen
Length of output: 792
🏁 Script executed:
# Read the registerMediaProtocol function fully
sed -n '/^export function registerMediaProtocol/,/^}/p' electron/windows.ts | head -30Repository: siddharthvaddem/openscreen
Length of output: 1027
🏁 Script executed:
# Read context around lines 343-350 in main.ts
sed -n '330,360p' electron/main.ts | cat -nRepository: siddharthvaddem/openscreen
Length of output: 1405
🏁 Script executed:
# Check if there are any scheme registrations at module level or app startup
rg "protocol\." electron/ -B 1 -A 1Repository: siddharthvaddem/openscreen
Length of output: 258
🏁 Script executed:
# Check Electron version and dependencies
cat package.json | grep -A 5 -B 5 "electron"Repository: siddharthvaddem/openscreen
Length of output: 1454
🌐 Web query:
Electron protocol.handle() timing requirements app.whenReady()
💡 Result:
In Electron, the timing requirements for using protocol.handle in relation to app.whenReady are as follows: - protocol.handle(scheme, handler) must be called after app.whenReady resolves (i.e., after the 'ready' event). The official Electron documentation states: "All methods unless specified can only be used after the ready event of the app module gets emitted." All examples in the docs place protocol.handle inside app.whenReady.then( => { ... }). - For custom schemes requiring privileges (e.g., standard: true, secure: true, supportFetchAPI: true), you must first call protocol.registerSchemesAsPrivileged([{ scheme: 'your-scheme', privileges: { ... } }]) before app.whenReady. This method "can only be used before the ready event ... and can be called only once." Standard pattern from docs: protocol.registerSchemesAsPrivileged([ { scheme: 'app', privileges: { standard: true, secure: true, supportFetchAPI: true } } ]); app.whenReady.then( => { protocol.handle('app', (req) => { /* handler */ }); }); Calling protocol.handle before app.whenReady will fail silently or not work, as the protocol module is not fully initialized until after ready. For built-in schemes like 'file:', additional fixes apply in older versions, but timing remains post-ready.
Citations:
- 1: https://www.electronjs.org/docs/api/protocol/
- 2: https://electronjs.org/docs/latest/api/protocol
- 3: [Bug]: protocol.handle() is not working with file protocol electron/electron#39031
- 4: fix:
protocol.handlenot intercepting file protocol electron/electron#39048
🌐 Web query:
Electron protocol.registerSchemesAsPrivileged vs protocol.handle() differences
💡 Result:
protocol.registerSchemesAsPrivileged and protocol.handle are complementary methods in Electron's protocol module for handling custom protocols. protocol.registerSchemesAsPrivileged(customSchemes) must be called BEFORE the 'ready' event of the app module, and only once (recently made tolerant of multiple calls by accumulating schemes). It registers custom schemes with special privileges like standard (RFC 3986 URI syntax for proper relative/absolute URL resolution), secure, bypassCSP, supportFetchAPI, allowServiceWorkers, stream, and codeCache. Without these privileges (especially 'standard: true'), custom schemes behave like 'file:' (no relative URLs, limited APIs). Example: protocol.registerSchemesAsPrivileged([{ scheme: 'app', privileges: { standard: true, secure: true, supportFetchAPI: true } }]) protocol.handle(scheme, handler) is called AFTER 'ready', registers the actual handler function for the scheme that receives a GlobalRequest and returns a GlobalResponse (or Promise). It replaces deprecated registerProtocol and interceptProtocol methods (deprecated since v25+). Example: protocol.handle('app', (request) => new Response(...)) Key differences and usage: - Timing: registerSchemesAsPrivileged pre-ready (setup privileges), handle post-ready (attach handler). - Purpose: Former grants privileges (required for full functionality like fetch, ServiceWorkers, streaming); latter defines request handling logic. - Both needed for privileged custom protocols: Register scheme with privileges first, then handle it. Protocols are session-specific; use session.protocol for custom sessions. - handle supports modern Web API Response objects; older methods used callbacks with paths/buffers/strings. Older methods (registerFileProtocol etc.) are deprecated in favor of handle.
Citations:
- 1: https://electronjs.org/docs/latest/api/protocol
- 2: https://www.electronjs.org/docs/api/protocol/
- 3: https://stackoverflow.com/questions/78270788/custom-protocol-scheme-not-working-in-electron
- 4: feat: add protocol.handle electron/electron#36674
🏁 Script executed:
# Check webSecurity settings in windows.ts
rg "webSecurity" electron/Repository: siddharthvaddem/openscreen
Length of output: 52
🏁 Script executed:
# Check how app-media:// URLs are used in assetPath.ts
cat src/lib/assetPath.tsRepository: siddharthvaddem/openscreen
Length of output: 1280
🏁 Script executed:
# Search for window creation and webPreferences configuration
rg "webPreferences|new BrowserWindow" electron/ -A 10Repository: siddharthvaddem/openscreen
Length of output: 2905
🏁 Script executed:
# Look for where windows are created to see full configuration
rg "createWindow|createEditorWindow" electron/windows.ts -A 20 | head -60Repository: siddharthvaddem/openscreen
Length of output: 593
🏁 Script executed:
# Check all window configurations more thoroughly
sed -n '/webPreferences/,/},/p' electron/windows.ts | head -40Repository: siddharthvaddem/openscreen
Length of output: 517
🏁 Script executed:
# Check if webSecurity is set anywhere in the codebase
rg "webSecurity" --no-ignore -r .Repository: siddharthvaddem/openscreen
Length of output: 52
🏁 Script executed:
# Check if there are any explicit security settings or defaults
rg "contextIsolation|sandbox|nodeIntegration" electron/ | grep -i "true\|false"Repository: siddharthvaddem/openscreen
Length of output: 351
Need to register app-media:// scheme with privileges before app.ready().
With webSecurity enabled by default and app-media:// now being used for asset loading, the current protocol.handle() inside whenReady() is incomplete. Electron requires protocol.registerSchemesAsPrivileged() to be called before the ready event for custom schemes to behave properly with security enabled. Without it, the scheme won't support fetch APIs or modern URL handling, which can cause weird failures.
Add this at the top level of main.ts (before app.whenReady()):
protocol.registerSchemesAsPrivileged([
{
scheme: 'app-media',
privileges: {
standard: true,
secure: true,
supportFetchAPI: true,
}
}
]);Then keep the registerMediaProtocol() handler call inside whenReady() as-is.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@electron/main.ts` around lines 343 - 345, Register the custom app-media
scheme with privileged permissions before the app is ready: add a top-level call
to protocol.registerSchemesAsPrivileged(...) specifying scheme 'app-media' with
privileges standard, secure, and supportFetchAPI (so fetch/URL handling works),
then leave the existing registerMediaProtocol() call inside app.whenReady().
Ensure the registerSchemesAsPrivileged invocation occurs before app.whenReady()
and references the same 'app-media' scheme used by registerMediaProtocol().
| return new Response("Forbidden", { status: 403 }); | ||
| } | ||
|
|
||
| return net.fetch(pathToFileURL(realResolved).toString()); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the electron/windows.ts file
find . -name "windows.ts" -path "*/electron/*" -type fRepository: siddharthvaddem/openscreen
Length of output: 91
🏁 Script executed:
# Let's also check the general structure to understand the repo
ls -laRepository: siddharthvaddem/openscreen
Length of output: 1987
🏁 Script executed:
# Examine the electron/windows.ts file, focusing on line 63 and surrounding context
cat -n ./electron/windows.tsRepository: siddharthvaddem/openscreen
Length of output: 6818
🏁 Script executed:
# Let's check the package.json to see what version of Electron is being used
cat package.json | grep -A 2 -B 2 electronRepository: siddharthvaddem/openscreen
Length of output: 1067
🏁 Script executed:
# Search for any other protocol handlers or media serving code in the project
rg "protocol\.handle|net\.fetch" -A 3 -B 3Repository: siddharthvaddem/openscreen
Length of output: 694
🏁 Script executed:
# Check if there are any tests for the media protocol handler
rg "registerMediaProtocol|app-media" -A 5 -B 5Repository: siddharthvaddem/openscreen
Length of output: 2566
🏁 Script executed:
# Search for any Range header usage or HTTP tests
rg "Range|range|seek|scrub" -iRepository: siddharthvaddem/openscreen
Length of output: 16319
🏁 Script executed:
# Check the main.ts to see how registerMediaProtocol is used
find . -name "main.ts" -path "*/electron/*" -type f -exec cat {} \;Repository: siddharthvaddem/openscreen
Length of output: 10750
🏁 Script executed:
# Check Electron version and look for any fetch-related utilities or patterns
grep -r "net\.fetch" --include="*.ts" --include="*.js"Repository: siddharthvaddem/openscreen
Length of output: 149
🏁 Script executed:
# Look for any HTTP header handling or request forwarding patterns in the codebase
rg "headers|Headers" --type ts --type js -A 2 -B 2 | head -50Repository: siddharthvaddem/openscreen
Length of output: 52
🌐 Web query:
Electron protocol.handle request object properties headers
💡 Result:
In Electron's protocol.handle(scheme, handler), the request object passed to the handler (typed as GlobalRequest) has the following properties, including headers: - url: string - The full URL of the request. - method: string - The HTTP method (e.g., 'GET', 'POST'). - referrer: string - The referrer URL. - headers: Record<string, string> - An object containing the request headers, where keys are header names and values are strings. - uploadData: UploadData[] (optional) - Array of upload data for POST requests. - body: (inferred from examples) - The request body, usable for forwarding with net.fetch. This structure aligns with the Fetch API's Request object (per MDN references in docs), allowing usage like net.fetch('https://...', { method: req.method, headers: req.headers, body: req.body }). The headers is a plain object mapping header names to string values, as shown in official examples and type definitions. Note: protocol.handle uses modern Request-like objects (introduced ~v25+), replacing deprecated ProtocolRequest from older register*Protocol APIs.
Citations:
- 1: https://electronjs.org/docs/latest/api/protocol
- 2: https://www.electronjs.org/docs/api/protocol/
- 3: https://electronjs.org/docs/latest/api/structures/protocol-request
- 4: https://www.electronjs.org/docs/latest/api/structures/protocol-request
🌐 Web query:
Electron net.fetch forward headers Range request
💡 Result:
Electron's net.fetch uses Chromium's native networking stack and behaves like the web fetch API in the renderer process. It forwards custom request headers, including the Range header, to the server when specified in the init options. To use a Range request: js const { net } = require('electron'); async function fetchRange(url, start, end) { const response = await net.fetch(url, { headers: { Range: bytes=${start}-${end} } }); if (response.status === 206) { const data = await response.arrayBuffer; // Handle partial content } } Key points from official docs and sources: - net.fetch(input, init) accepts standard RequestInit, where headers can include Range (Electron net docs). - As it mirrors renderer fetch, Range headers are preserved and sent, similar to Chromium fetch behavior (WPT tests passed in implementation PR with no Range-specific exclusions). - Common use case: Forwarding intercepted requests in protocol.handle, where request.headers.get('Range') is accessible and net.fetch(request) passes all headers including Range to the origin (e.g., file:// for video seeking). For custom protocols serving videos/audio: - Register with supportFetchAPI: true, stream: true privileges. - In handler: Check request.headers.get('Range'), parse bytes, respond with 206 status, Content-Range, Accept-Ranges: bytes. - Or forward to net.fetch('file://path') which handles Range automatically if server supports it. No known bugs or limitations for direct net.fetch Range support as of 2026; issues are typically with custom protocol responses not setting proper range headers (e.g., #38749, #40447 resolved with privileges). Headers are downcased internally but functional.
Citations:
- 1: https://www.electronjs.org/docs/api/net/
- 2: https://electronjs.org/docs/latest/api/net
- 3: https://www.electronjs.org/docs/api/protocol/
- 4: feat: implement net.fetch electron/electron#36733
- 5: [Bug]: video files not seekable with protocol.handle electron/electron#38749
- 6: feat: net.fetch() supports custom protocols electron/electron#36606
🏁 Script executed:
# Check if there are any tests for the protocol handler
rg "registerMediaProtocol|app-media" -r tests/ --type tsRepository: siddharthvaddem/openscreen
Length of output: 421
🏁 Script executed:
# Look for any video playback or media testing
rg "HTMLVideoElement|video" --type ts tests/ -A 2 -B 2 | head -40Repository: siddharthvaddem/openscreen
Length of output: 2567
🏁 Script executed:
# Check if there's actual usage of video/media playback in the app
rg "<video|HTMLVideoElement" src/ --type tsx --type ts -B 2 -A 2 | head -50Repository: siddharthvaddem/openscreen
Length of output: 97
🏁 Script executed:
# Search for video element usage in source files
rg "video" src/ --type ts -i | grep -i "video" | head -20Repository: siddharthvaddem/openscreen
Length of output: 1616
🏁 Script executed:
# Check how the app-media protocol is being used in the renderer
rg "app-media" src/ -A 3 -B 3Repository: siddharthvaddem/openscreen
Length of output: 642
🏁 Script executed:
# Look at the VideoPlayback component which handles video playback
find src -name "*VideoPlayback*" -o -name "*video*" -type f | head -10Repository: siddharthvaddem/openscreen
Length of output: 246
🏁 Script executed:
# Read the VideoPlayback.tsx to understand how the app plays video
cat -n src/components/video-editor/VideoPlayback.tsx | head -100Repository: siddharthvaddem/openscreen
Length of output: 3846
🏁 Script executed:
# Check the full VideoPlayback file
wc -l src/components/video-editor/VideoPlayback.tsxRepository: siddharthvaddem/openscreen
Length of output: 120
🏁 Script executed:
# Look for where the videoPath is used in VideoPlayback
rg "videoPath" src/components/video-editor/VideoPlayback.tsx -A 3 -B 3 | head -80Repository: siddharthvaddem/openscreen
Length of output: 846
🏁 Script executed:
# Search for VideoSource usage in the file
rg "VideoSource|video\.src" src/components/video-editor/VideoPlayback.tsx -A 5 -B 5Repository: siddharthvaddem/openscreen
Length of output: 674
🏁 Script executed:
# Look at how the video is actually loaded and where the app-media protocol is invoked
rg "app-media" -A 2 -B 2 | grep -v node_modulesRepository: siddharthvaddem/openscreen
Length of output: 762
🏁 Script executed:
# Check if there are any tests that specifically test video playback or seeking
find tests -name "*.spec.ts" -exec grep -l "video\|seek\|Range" {} \;Repository: siddharthvaddem/openscreen
Length of output: 98
🏁 Script executed:
# Let's check the actual Electron version to confirm Range support
grep -A 2 '"electron"' package.jsonRepository: siddharthvaddem/openscreen
Length of output: 164
🏁 Script executed:
# Quick confirmation: check that the video element indeed triggers Range requests in browsers
rg "currentTime|seeking|seeked" src/components/video-editor/VideoPlayback.tsx | head -5Repository: siddharthvaddem/openscreen
Length of output: 189
Forward request headers to net.fetch to preserve Range requests.
line 63 drops headers when delegating to net.fetch(), so Range requests from video seeking get lost. When the browser tries to scrub a large recording, it ends up fetching the entire file instead of just the chunk it needs, which is kinda terrible for seek performance. Pass through method, headers, and body from the incoming request:
Example fix
return net.fetch(pathToFileURL(realResolved).toString(), {
method: request.method,
headers: request.headers,
body: request.body,
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@electron/windows.ts` at line 63, The current net.fetch call in the route
handler drops incoming request metadata and breaks Range requests; update the
call that returns net.fetch(pathToFileURL(realResolved).toString()) to forward
the incoming request's method, headers, and body (e.g., use request.method,
request.headers, request.body) so Range and other header-dependent behavior is
preserved when delegating to net.fetch; locate the usage in electron/windows.ts
where realResolved, request, and net.fetch are referenced and add those options
to the net.fetch invocation.
| function computeBitrate(width: number, height: number, frameRate: number) { | ||
| const pixels = width * height; | ||
| const highFrameRateBoost = frameRate >= HIGH_FRAME_RATE_THRESHOLD ? HIGH_FRAME_RATE_BOOST : 1; | ||
|
|
||
| if (pixels >= FOUR_K_PIXELS) { | ||
| return Math.round(BITRATE_4K * highFrameRateBoost); | ||
| } | ||
| if (pixels >= QHD_PIXELS) { | ||
| return Math.round(BITRATE_QHD * highFrameRateBoost); | ||
| } | ||
| return Math.round(BITRATE_BASE * highFrameRateBoost); | ||
| } |
There was a problem hiding this comment.
This helper doesn’t match useScreenRecorder today.
The test version takes frameRate, but src/hooks/useScreenRecorder.ts:132-146 still keys off TARGET_FRAME_RATE. So the 30fps/59fps assertions can pass here while runtime still always applies the boost. Please test the real helper, or align the runtime implementation first.
Also applies to: 27-49
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks/useScreenRecorder.test.ts` around lines 13 - 24, The test's
computeBitrate helper accepts frameRate and conditions on
HIGH_FRAME_RATE_THRESHOLD, but the runtime in useScreenRecorder.ts still uses
TARGET_FRAME_RATE (so the boost is always applied); fix by making the runtime
computeBitrate behavior match the test: update useScreenRecorder's bitrate
calculation (the code around useScreenRecorder function lines that reference
TARGET_FRAME_RATE and HIGH_FRAME_RATE_BOOST) to accept or derive the actual
frameRate and apply HIGH_FRAME_RATE_BOOST only when frameRate >=
HIGH_FRAME_RATE_THRESHOLD, or alternatively change the test to call the real
helper from useScreenRecorder.ts so both use the same computeBitrate
implementation.
| activeScreenRecorder.recorder.addEventListener( | ||
| "error", | ||
| () => { | ||
| // Suppress auto-finalize to prevent the stop listener from racing this cleanup | ||
| allowAutoFinalize.current = false; | ||
| if (screenRecorder.current === activeScreenRecorder) { | ||
| screenRecorder.current = null; | ||
| } | ||
| if (webcamRecorder.current === activeWebcamRecorder) { | ||
| webcamRecorder.current = null; | ||
| } | ||
| if (activeWebcamRecorder?.recorder.state === "recording") { | ||
| try { | ||
| activeWebcamRecorder.recorder.stop(); | ||
| } catch { | ||
| // Webcam recorder may already be stopped. | ||
| } | ||
| } | ||
| teardownMedia(); | ||
| setRecording(false); | ||
| window.electronAPI?.setRecordingState(false); | ||
| if (finalizingRecordingId.current === activeRecordingId) { | ||
| finalizingRecordingId.current = null; | ||
| } | ||
| }, | ||
| { once: true }, | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/hooks/useScreenRecorder.ts | head -100Repository: siddharthvaddem/openscreen
Length of output: 4205
🏁 Script executed:
cat -n src/hooks/useScreenRecorder.ts | sed -n '54,90p'Repository: siddharthvaddem/openscreen
Length of output: 1473
🏁 Script executed:
cat -n src/hooks/useScreenRecorder.ts | sed -n '540,600p'Repository: siddharthvaddem/openscreen
Length of output: 2456
Move the error listener to recorder creation time.
There's a window between recorder.start() (line 85) and the error listener attachment (line 554) where an error event would only trigger the onerror callback that rejects recordedBlobPromise, skipping all the cleanup logic you've got in the error listener—teardown, ref nulling, webcam stop, etc. If that promise rejection goes unconsumed, it'll bite you. Hook up that error listener inside createRecorderHandle() before calling start(), and explicitly handle the promise rejection on the error path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks/useScreenRecorder.ts` around lines 554 - 580, The error listener is
attached too late—move the cleanup handler to recorder creation so errors
between recorder.start() and the current listener don't bypass teardown; inside
createRecorderHandle(), attach the same "error" listener to
activeScreenRecorder.recorder (the logic that sets allowAutoFinalize.current =
false, clears screenRecorder.current/webcamRecorder.current, stops
activeWebcamRecorder.recorder if recording, calls teardownMedia(), calls
setRecording(false) and window.electronAPI?.setRecordingState(false), and clears
finalizingRecordingId.current when it matches activeRecordingId) before calling
recorder.start(), and also ensure any promise(s) like recordedBlobPromise that
may reject on error have their rejection explicitly consumed/handled in the
error path to avoid unhandled rejections.
src/lib/compositeLayout.test.ts
Outdated
| it("bounds the vertical-stack layout within maxContentSize when webcam is present", () => { | ||
| const layout = computeCompositeLayout({ | ||
| canvasSize: { width: 1920, height: 1080 }, | ||
| maxContentSize: { width: 1536, height: 864 }, | ||
| screenSize: { width: 1920, height: 1080 }, | ||
| webcamSize: { width: 1280, height: 720 }, | ||
| layoutPreset: "vertical-stack", | ||
| }); | ||
|
|
||
| expect(layout).not.toBeNull(); | ||
| // Screen and webcam should be bounded within maxContentSize, not filling the canvas | ||
| expect(layout!.screenRect.width).toBeLessThanOrEqual(1536); | ||
| expect(layout!.screenRect.height + (layout!.webcamRect?.height ?? 0)).toBeLessThanOrEqual( | ||
| 864 + 1, | ||
| ); // +1 for rounding | ||
| // Should be centered in canvas | ||
| expect(layout!.screenRect.x).toBeGreaterThan(0); | ||
| }); |
There was a problem hiding this comment.
Behavior/test contract mismatch with current vertical-stack implementation
nice regression intent, but Line 92–Line 95 currently assert a contract that computeCompositeLayout (stack branch) doesn’t implement yet. In src/lib/compositeLayout.ts (Line 150–Line 184), vertical-stack still uses canvasWidth/canvasHeight and ignores maxContentSize, so this will fail unless that implementation is updated in the same PR.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/compositeLayout.test.ts` around lines 81 - 98, The test assumes the
"vertical-stack" branch of computeCompositeLayout honors maxContentSize but the
current implementation still uses canvasSize; update the vertical-stack branch
in computeCompositeLayout to constrain screenRect and webcamRect dimensions
using maxContentSize.width and maxContentSize.height (apply the same
stacking/spacing and rounding logic currently used for canvasSize), ensure total
stacked height (screenRect.height + webcamRect.height) is bounded by
maxContentSize.height (allowing the existing +1 rounding tolerance), and center
the composed content within the canvas so screenRect.x is > 0; keep existing
behavior for other presets unchanged.
src/lib/compositeLayout.test.ts
Outdated
| expect(layout).not.toBeNull(); | ||
| // Screen and webcam should be bounded within maxContentSize, not filling the canvas | ||
| expect(layout!.screenRect.width).toBeLessThanOrEqual(1536); | ||
| expect(layout!.screenRect.height + (layout!.webcamRect?.height ?? 0)).toBeLessThanOrEqual( | ||
| 864 + 1, | ||
| ); // +1 for rounding | ||
| // Should be centered in canvas | ||
| expect(layout!.screenRect.x).toBeGreaterThan(0); | ||
| }); |
There was a problem hiding this comment.
Add an explicit webcamRect assertion (lowkey safer)
On Line 93, layout!.webcamRect?.height ?? 0 makes this pass path a bit too forgiving. Since this case is explicitly “webcam is present,” assert non-null first so a missing webcam rect can’t slip through.
nit: cleaner assertion sequence
expect(layout).not.toBeNull();
+ expect(layout!.webcamRect).not.toBeNull();
// Screen and webcam should be bounded within maxContentSize, not filling the canvas
expect(layout!.screenRect.width).toBeLessThanOrEqual(1536);
- expect(layout!.screenRect.height + (layout!.webcamRect?.height ?? 0)).toBeLessThanOrEqual(
+ expect(layout!.screenRect.height + layout!.webcamRect!.height).toBeLessThanOrEqual(
864 + 1,
); // +1 for rounding📝 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.
| expect(layout).not.toBeNull(); | |
| // Screen and webcam should be bounded within maxContentSize, not filling the canvas | |
| expect(layout!.screenRect.width).toBeLessThanOrEqual(1536); | |
| expect(layout!.screenRect.height + (layout!.webcamRect?.height ?? 0)).toBeLessThanOrEqual( | |
| 864 + 1, | |
| ); // +1 for rounding | |
| // Should be centered in canvas | |
| expect(layout!.screenRect.x).toBeGreaterThan(0); | |
| }); | |
| expect(layout).not.toBeNull(); | |
| expect(layout!.webcamRect).not.toBeNull(); | |
| // Screen and webcam should be bounded within maxContentSize, not filling the canvas | |
| expect(layout!.screenRect.width).toBeLessThanOrEqual(1536); | |
| expect(layout!.screenRect.height + layout!.webcamRect!.height).toBeLessThanOrEqual( | |
| 864 + 1, | |
| ); // +1 for rounding | |
| // Should be centered in canvas | |
| expect(layout!.screenRect.x).toBeGreaterThan(0); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/compositeLayout.test.ts` around lines 90 - 98, The test is too
permissive by using layout!.webcamRect?.height ?? 0; explicitly assert that the
webcam rect exists before measuring its height: add an assertion like
expect(layout!.webcamRect).not.toBeNull() (or truthy) and then use
layout!.webcamRect!.height in the subsequent constraint check (the code paths
using layout and webcamRect are the targets to update).
| // Replicate the isAllowedExternalUrl logic for testing | ||
| function isAllowedExternalUrl(url: string): boolean { | ||
| if (!url || typeof url !== "string") return false; | ||
| try { | ||
| const parsed = new URL(url); | ||
| return parsed.protocol === "https:" || parsed.protocol === "http:"; | ||
| } catch { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
This security test is only exercising its own copy.
electron/ipc/handlers.ts already has the production isAllowedExternalUrl() implementation. Re-defining it here means a future regression in the IPC handler still goes green in tests, which is lowkey risky for a security boundary. Please extract this helper into a shared module and import the real implementation instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/securityValidation.test.ts` around lines 3 - 12, The test defines its
own isAllowedExternalUrl which masks regressions; extract the real
isAllowedExternalUrl into a shared utility module and have both the IPC handler
and this test import that shared function instead of redefining it. Move the
current implementation (the URL parsing + protocol check) into the new shared
export, update the IPC handler to import and use that export, and change
src/lib/securityValidation.test.ts to import isAllowedExternalUrl from the
shared module so the test exercises the actual production logic.
…-in-folder, and exports - set-current-video-path: use isPathAllowed (checks approvedPaths + trusted dirs) instead of isTrustedMediaPath so user-picked external videos are accepted - reveal-in-folder: use isPathAllowed so exported files in Downloads etc. can be revealed - save-exported-video: approve the save path so reveal-in-folder works after export - Remove unused resolveRecordingOutputPath (replaced by sanitizeFileName) - Remove vertical-stack layout test that belongs with the layout fix (PR siddharthvaddem#361) Addresses review feedback from Codex, CodeRabbit, and Gemini.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
electron/ipc/handlers.ts (1)
917-941: 🧹 Nitpick | 🔵 Trivialminor inconsistency: passing raw
videoPathinstead ofnormalizedVideoPathto session loader.line 919 validates
normalizedVideoPath, but line 925 passes the originalvideoPathtoloadRecordedSessionForVideoPath. if the input was afile://URL, these would differ, and the session load would re-normalize internally anyway. kinda redundant but also kinda risky if normalization behavior ever diverges.- const restoredSession = await loadRecordedSessionForVideoPath(videoPath); + const restoredSession = await loadRecordedSessionForVideoPath(normalizedVideoPath);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/ipc/handlers.ts` around lines 917 - 941, The handler registered for "set-current-video-path" validates and produces normalizedVideoPath using normalizeVideoSourcePath but still calls loadRecordedSessionForVideoPath(videoPath); change that call to pass normalizedVideoPath instead (i.e., use loadRecordedSessionForVideoPath(normalizedVideoPath)) so session lookup uses the same canonical path you validated, leaving the rest of the logic (approveFilePath, setCurrentRecordingSessionState) unchanged.
♻️ Duplicate comments (5)
electron/ipc/handlers.ts (5)
255-261:⚠️ Potential issue | 🟠 Majormanifest file read still skips realpath validation.
you check
isTrustedMediaPath(normalizedVideoPath)but then readmanifestPathwithout resolving symlinks. if someone plants a symlink at/recordings/foo.session.json → /sensitive/data.json, the logical path passes but you read outside the trusted root.suggested fix
try { const manifestPath = getSessionManifestPathForVideo(normalizedVideoPath); + let realManifestPath: string; + try { + realManifestPath = await fs.realpath(manifestPath); + if (!isTrustedMediaPath(realManifestPath)) { + return null; + } + } catch { + // File doesn't exist + return null; + } - const content = await fs.readFile(manifestPath, "utf-8"); + const content = await fs.readFile(realManifestPath, "utf-8");,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/ipc/handlers.ts` around lines 255 - 261, The manifest is read via manifestPath from getSessionManifestPathForVideo(normalizedVideoPath) without resolving symlinks, so an attacker could point that manifest to an outside file; resolve the manifest's real path (e.g., fs.realpath or equivalent) after computing manifestPath and before fs.readFile, then re-run the trust check (use isTrustedMediaPath or a new isTrustedMediaPathForManifest) against the resolved manifest path and only proceed to fs.readFile if the resolved path is trusted; ensure you reference getSessionManifestPathForVideo, manifestPath, normalizedVideoPath, isTrustedMediaPath and fs.readFile when making the change.
579-585:⚠️ Potential issue | 🟠 Majorcursor telemetry read also skips realpath validation on the sidecar file.
same issue as manifest: you validate
targetVideoPathbut then read${targetVideoPath}.cursor.jsonwithout resolving symlinks. need to realpath the telemetry file itself before reading.const telemetryPath = `${targetVideoPath}.cursor.json`; +let realTelemetryPath: string; try { + realTelemetryPath = await fs.realpath(telemetryPath); + if (!isTrustedMediaPath(realTelemetryPath)) { + return { success: true, samples: [] }; + } - const content = await fs.readFile(telemetryPath, "utf-8"); + const content = await fs.readFile(realTelemetryPath, "utf-8");,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/ipc/handlers.ts` around lines 579 - 585, The telemetry sidecar file is read without realpath/symlink validation: before calling fs.readFile on telemetryPath (the `${targetVideoPath}.cursor.json` variable) resolve its real path (e.g., fs.realpath/realpathSync) and run isTrustedMediaPath against the resolved path; if the resolved telemetry path is not trusted, return the same safe response ({ success: true, samples: [] }) and only then proceed to read the file and parse its contents. Ensure you update the telemetryPath usage in this block (and any related error handling) to use the resolved path.
186-199:⚠️ Potential issue | 🟠 Major
isTrustedMediaPathstill doesn't consultapprovedPaths, breaking user-picked external files.tracing the flow: user picks file from Downloads via
open-video-file-picker→ file gets added toapprovedPaths→set-current-video-pathusesisPathAllowed(passes) → butread-binary-fileusesisTrustedMediaPathRealpath(fails because Downloads isn't a trusted root).either
isTrustedMediaPathneeds to also checkapprovedPaths, or you need a separate function that combines both checks for the read handlers.suggested fix
function isTrustedMediaPath(filePath: string): boolean { if (!filePath || typeof filePath !== "string") { return false; } const normalized = normalizePath(filePath); + + // Check explicitly approved paths first + if (approvedPaths.has(normalized)) { + return true; + } const trustedRoots: string[] = [normalizePath(RECORDINGS_DIR)]; if (currentProjectPath) { trustedRoots.push(normalizePath(path.dirname(currentProjectPath))); } return trustedRoots.some((root) => normalized === root || normalized.startsWith(root + path.sep)); },
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/ipc/handlers.ts` around lines 186 - 199, isTrustedMediaPath currently only checks trustedRoots and thus rejects user-approved files; update isTrustedMediaPath to also consult the approvedPaths store (or call the existing isPathAllowed helper) so that user-picked external files accepted by set-current-video-path and open-video-file-picker are treated as trusted by read-binary-file and isTrustedMediaPathRealpath; ensure you compare canonical/normalized (or realpath-resolved) values when checking approvedPaths to avoid symlink/path variation issues and reference isTrustedMediaPath, approvedPaths (or isPathAllowed), isTrustedMediaPathRealpath, read-binary-file, set-current-video-path, and open-video-file-picker when making the change.
322-325:⚠️ Potential issue | 🟡 Minormanifest path still derives from raw
payload.screen.fileNameinstead ofsafeScreenFileName.if sanitization ever diverges (or you add more stripping later), this creates inconsistency. safer to use the same sanitized name everywhere:
const sessionManifestPath = path.join( RECORDINGS_DIR, - `${path.parse(payload.screen.fileName).name}${RECORDING_SESSION_SUFFIX}`, + `${path.parse(safeScreenFileName).name}${RECORDING_SESSION_SUFFIX}`, );,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/ipc/handlers.ts` around lines 322 - 325, The manifest path is built from the raw payload.screen.fileName causing inconsistency with sanitized names; change the sessionManifestPath construction to use the already-computed safeScreenFileName (or, if safeScreenFileName excludes extension, use it directly; otherwise use path.parse(safeScreenFileName).name) instead of path.parse(payload.screen.fileName).name so RECORDINGS_DIR + RECORDING_SESSION_SUFFIX always aligns with the sanitized filename.
214-221:⚠️ Potential issue | 🟡 MinorNULs and control chars still pass through
sanitizeFileName.
path.basenamehandles path traversal but embedded\0bytes survive. depending on the OS/filesystem, this can cause string truncation or weird behavior. safer to strip them:suggested fix
function sanitizeFileName(fileName: string): string { - // Strip any path separators and parent-directory references to prevent traversal - const baseName = path.basename(fileName); + // Strip NULs, control chars, path separators, and parent-directory references + const cleaned = fileName.replace(/[\x00-\x1f]/g, ""); + const baseName = path.basename(cleaned); if (!baseName || baseName === "." || baseName === "..") { throw new Error("Invalid file name"); } return baseName; },
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/ipc/handlers.ts` around lines 214 - 221, sanitizeFileName currently uses path.basename but allows embedded NULs/control characters; update the sanitizeFileName(fileName: string) function to remove or reject control characters (at minimum NUL \x00) after computing baseName—e.g., strip all characters in the control range [\x00-\x1F\x7F] or throw an error if any are present—so the function returns a safe filename without embedded NULs/control bytes that could truncate or misbehave on some OS/filesystems.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@electron/ipc/handlers.ts`:
- Around line 917-941: The handler registered for "set-current-video-path"
validates and produces normalizedVideoPath using normalizeVideoSourcePath but
still calls loadRecordedSessionForVideoPath(videoPath); change that call to pass
normalizedVideoPath instead (i.e., use
loadRecordedSessionForVideoPath(normalizedVideoPath)) so session lookup uses the
same canonical path you validated, leaving the rest of the logic
(approveFilePath, setCurrentRecordingSessionState) unchanged.
---
Duplicate comments:
In `@electron/ipc/handlers.ts`:
- Around line 255-261: The manifest is read via manifestPath from
getSessionManifestPathForVideo(normalizedVideoPath) without resolving symlinks,
so an attacker could point that manifest to an outside file; resolve the
manifest's real path (e.g., fs.realpath or equivalent) after computing
manifestPath and before fs.readFile, then re-run the trust check (use
isTrustedMediaPath or a new isTrustedMediaPathForManifest) against the resolved
manifest path and only proceed to fs.readFile if the resolved path is trusted;
ensure you reference getSessionManifestPathForVideo, manifestPath,
normalizedVideoPath, isTrustedMediaPath and fs.readFile when making the change.
- Around line 579-585: The telemetry sidecar file is read without
realpath/symlink validation: before calling fs.readFile on telemetryPath (the
`${targetVideoPath}.cursor.json` variable) resolve its real path (e.g.,
fs.realpath/realpathSync) and run isTrustedMediaPath against the resolved path;
if the resolved telemetry path is not trusted, return the same safe response ({
success: true, samples: [] }) and only then proceed to read the file and parse
its contents. Ensure you update the telemetryPath usage in this block (and any
related error handling) to use the resolved path.
- Around line 186-199: isTrustedMediaPath currently only checks trustedRoots and
thus rejects user-approved files; update isTrustedMediaPath to also consult the
approvedPaths store (or call the existing isPathAllowed helper) so that
user-picked external files accepted by set-current-video-path and
open-video-file-picker are treated as trusted by read-binary-file and
isTrustedMediaPathRealpath; ensure you compare canonical/normalized (or
realpath-resolved) values when checking approvedPaths to avoid symlink/path
variation issues and reference isTrustedMediaPath, approvedPaths (or
isPathAllowed), isTrustedMediaPathRealpath, read-binary-file,
set-current-video-path, and open-video-file-picker when making the change.
- Around line 322-325: The manifest path is built from the raw
payload.screen.fileName causing inconsistency with sanitized names; change the
sessionManifestPath construction to use the already-computed safeScreenFileName
(or, if safeScreenFileName excludes extension, use it directly; otherwise use
path.parse(safeScreenFileName).name) instead of
path.parse(payload.screen.fileName).name so RECORDINGS_DIR +
RECORDING_SESSION_SUFFIX always aligns with the sanitized filename.
- Around line 214-221: sanitizeFileName currently uses path.basename but allows
embedded NULs/control characters; update the sanitizeFileName(fileName: string)
function to remove or reject control characters (at minimum NUL \x00) after
computing baseName—e.g., strip all characters in the control range
[\x00-\x1F\x7F] or throw an error if any are present—so the function returns a
safe filename without embedded NULs/control bytes that could truncate or
misbehave on some OS/filesystems.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b1b9dc97-a78d-4726-9e5c-5a9d3644dafc
📒 Files selected for processing (1)
electron/ipc/handlers.ts
1 participant
Description
Harden Electron IPC file-read handlers with a trusted-access contract, replace permissive path checks with strict
isTrustedMediaPathvalidation, introduce theapp-media://custom protocol to restorewebSecurity: true, and add symlink resolution to prevent filesystem escapes.Motivation
The existing IPC handlers allowed reading files from any directory once a path was normalized, making them vulnerable to path traversal and symlink-based attacks. With
webSecurity: falseon the editor window, the renderer could directly access local files. These changes establish a proper security boundary between renderer and main process.Type of Change
Related Issue(s)
Related to #352 (path traversal in IPC file read handlers) — this PR provides a more comprehensive fix.
Screenshots / Video
N/A — security hardening with no UI changes.
Testing
npm test— all 74 tests pass, including new regression tests.openscreenproject referencing a file outside the recordings directory → should be rejected with "Access denied"app-media://protocol (record a video, play it back in editor)app-media://should resolve it and rejectNew test files:
src/lib/securityValidation.test.ts—isTrustedMediaPath,sanitizeFileName,normalizeVideoSourcePathsrc/hooks/useScreenRecorder.test.ts— recorder error lifecyclesrc/components/video-editor/projectPersistence.test.ts— project persistence securitysrc/lib/compositeLayout.test.ts— vertical-stack layout boundsChecklist
Methodology
These changes were developed using a multi-LLM review workflow orchestrated through llm-cli-gateway, an MCP-based tool for coordinating parallel code analysis across multiple AI models.
Process:
fix(security): address Codex review findingscommits)This approach surfaces issues that a single-model workflow might miss, since each model brings different heuristics and blind spots to security analysis.
Thank you for contributing!
Summary by CodeRabbit
Bug Fixes
Tests