feat(security): exfiltration prevention, CSP & macOS hardening#360
Open
verivusOSS-releases wants to merge 10 commits intosiddharthvaddem:mainfrom
Open
feat(security): exfiltration prevention, CSP & macOS hardening#360verivusOSS-releases wants to merge 10 commits intosiddharthvaddem:mainfrom
verivusOSS-releases wants to merge 10 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>
…allpaper, IPC, decoder, macOS (U01-U06) U01: Add setWindowOpenHandler (deny all) and will-navigate (allowlist only file://, app-media://, dev server) to all three BrowserWindows. U02: Fix custom font CSS injection — validate URLs with isValidGoogleFontsUrl at load time, reject CSS injection characters, replace @import template literal with <link rel="stylesheet"> tag. U03: Add shared isAllowedWallpaperValue() validator rejecting external HTTP URLs. Applied at project normalization, VideoPlayback preview, and frameRenderer export boundaries. U04: Fix reveal-in-folder fallback to re-validate dirname with isTrustedMediaPath. Add ALLOWED_EXTERNAL_DOMAINS allowlist to open-external-url (initially: github.com). U05: Guard streamingDecoder against external HTTP fetch — reject http(s) URLs, allow only blob:/data: via fetch, route rest to IPC. U06: Enable macOS hardenedRuntime with entitlements: allow-jit, allow-unsigned-executable-memory, audio-input, camera. Separate entitlements.mac.inherit.plist for child processes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add restrictive CSP via session.webRequest.onHeadersReceived for dev mode (loadURL) and <meta> tag in index.html for production (loadFile). Dev mode relaxes script-src and connect-src for Vite dev server + HMR. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…eck (U08) Add 23 new tests covering: - Google Fonts URL validation (accept/reject/malformed) - CSS injection character blocklist verification - Wallpaper value allowlist (local paths, data:, gradients, colors) - Wallpaper sanitization in normalizeProjectEditor - External URL domain allowlist logic - StreamingDecoder URL classification (http rejected, blob/data allowed) - CSP meta tag presence in index.html Also fix: isValidGoogleFontsUrl now enforces https: protocol. Total: 60 tests across 9 files, all passing. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…allpaper paths, realpath fallback 1. Replace will-navigate startsWith() with parsed URL checks via isAllowedNavigation() — validates protocol/origin precisely, blocks UNC paths and origin-spoofing attacks. 2. Tighten isAllowedWallpaperValue() to only allow /wallpapers/*.ext pattern (not arbitrary / paths), data:image/ URIs (not data:text/), and validated hex colors. 3. Use isTrustedMediaPathRealpath() in reveal-in-folder fallback to resolve symlinks before checking trusted roots.
📝 WalkthroughWalkthroughComprehensive security hardening across build config, Electron IPC, protocol handling, and resource validation. Introduces filesystem path allowlisting for IPC operations, custom media protocol ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Multiple high-density security mechanisms (IPC path validation, protocol handling, CSP injection, URL allowlisting) across heterogeneous file types; careful reasoning needed around symlink resolution, protocol-level containment, and cross-layer validation interactions. Possibly related PRs
Suggested reviewers
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
openscreen/electron/ipc/handlers.ts
Line 612 in 7342ff7
Restore removed media-approval helpers before invoking them
This handler now calls approveReadableVideoPath, but that function (along with getApprovedProjectSession and approveFilePath) was deleted earlier in the same commit, so these IPC paths will throw ReferenceError at runtime when users open a video, load a project, or set a restored session. In practice this breaks core launch/editor flows as soon as those handlers execute.
ℹ️ 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".
Comment on lines
+29
to
+30
| const mediaUrl = `app-media://${baseUrl.pathname}${encodedRelativePath}`; | ||
| return mediaUrl; |
There was a problem hiding this comment.
getAssetPath now returns app-media://... URLs, but wallpaper rendering/export code still classifies image URLs using file://, http, /, and data: checks (e.g., VideoPlayback and FrameRenderer). As a result, packaged builds can treat valid wallpaper selections as non-image values, leading to missing/black backgrounds in preview/export when asset paths are resolved through this new scheme.
Useful? React with 👍 / 👎.
src/lib/customFonts.ts
Outdated
| } | ||
|
|
||
| // Reject CSS injection characters | ||
| if (/['";)\\]/.test(font.importUrl)) { |
There was a problem hiding this comment.
The new unsafe-character blocklist rejects any import URL containing ;, but Google Fonts URLs commonly include semicolon-separated weights (for example ...family=Open+Sans:wght@400;700...). These valid URLs pass isValidGoogleFontsUrl but then fail in loadFont, so users cannot add many legitimate fonts.
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/lib/exporter/frameRenderer.ts (1)
234-254: 🧹 Nitpick | 🔵 Trivialdead code branches after validation
lowkey nit: the
file://check at line 235 andhttpcheck at line 238/245 are now unreachable sinceisAllowedWallpaperValuerejects both. not harmful, just slightly confusing to future readers.could either:
- remove them and simplify
- add a comment explaining they're defensive fallbacks
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/exporter/frameRenderer.ts` around lines 234 - 254, The wallpaper handling branch contains unreachable checks for "file://" and plain "http" because isAllowedWallpaperValue now filters those; either remove those dead branches in the image-background handling (the if-block that creates new Image() and the sub-branches that test wallpaper.startsWith("file://") / "data:" / "http") and simplify to the remaining cases, or keep the branches but add a clear defensive comment above the conditional (near the Image creation and the wallpaper.startsWith(...) checks) stating these branches are retained intentionally as defensive fallbacks for unexpected inputs; update code paths that set imageUrl (the assignments inside the http/file/data/absolute-path branches) accordingly so no unused branches remain.src/components/video-editor/VideoPlayback.tsx (1)
1150-1164: 🧹 Nitpick | 🔵 Trivialminor dead code after validation
similar to frameRenderer - the
httpandfile://branches at lines 1150-1152 and 1162-1163 are now unreachable since those get rejected byisAllowedWallpaperValueearlier.not harmful, just slightly confusing. could clean up later or leave as defensive fallback.
🤖 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 1150 - 1164, The conditional in VideoPlayback.tsx around the wallpaper handling contains unreachable checks for wallpaper.startsWith("http") and wallpaper.startsWith("file://") because isAllowedWallpaperValue already rejects those; simplify the logic by removing the "http" and "file://" branches and only handle the absolute-server-path case (wallpaper.startsWith("/")) by resolving via getAssetPath and calling setResolvedWallpaper(p), otherwise just call setResolvedWallpaper(wallpaper); adjust the block that references mounted, getAssetPath, setResolvedWallpaper, and wallpaper accordingly to remove the dead branches.electron/ipc/handlers.ts (2)
455-469:⚠️ Potential issue | 🟠 MajorTelemetry sidecar reads still skip the symlink-safe check.
This guard only vets
targetVideoPath, but the code actually reads${targetVideoPath}.cursor.json. A trusted-looking sidecar symlink under the recordings/project dir can still point outside the allowed roots and get parsed here.🔒 Reuse the realpath check on the file you read
const telemetryPath = `${targetVideoPath}.cursor.json`; + if (!(await isTrustedMediaPathRealpath(telemetryPath))) { + return { success: true, samples: [] }; + } try { const content = await fs.readFile(telemetryPath, "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 455 - 469, The handler registered in ipcMain.handle("get-cursor-telemetry") validates only targetVideoPath but then reads `${targetVideoPath}.cursor.json`, allowing a malicious symlink to escape trusted roots; to fix, after computing telemetryPath (the variable telemetryPath) resolve its real path and re-run the trusted check (e.g., call isTrustedMediaPath or a realpath-based check on telemetryPath) before calling fs.readFile, and return the empty success/samples response if the telemetryPath is not trusted—keep normalizeVideoSourcePath, targetVideoPath, telemetryPath, and the existing return shape unchanged.
816-839:⚠️ Potential issue | 🔴 Criticaldangling helper references will fail to build
approveFilePath(),approveReadableVideoPath(), andgetApprovedProjectSession()are called but don't exist anywhere in the codebase—not defined locally, not exported from other modules. five call sites will break at type-check/build time. looks like the old approval flow got half-deleted. either finish removing the calls (lines 612, 760, 787, 827, 829) or restore the helpers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/ipc/handlers.ts` around lines 816 - 839, The handler for the "set-current-video-path" IPC and other call sites reference missing helper functions (approveFilePath, approveReadableVideoPath, getApprovedProjectSession) which breaks builds; either remove those calls or restore the original approval helpers. Fix by deciding on the desired flow: if you want approval behavior, reintroduce and export the helper implementations (names: approveFilePath, approveReadableVideoPath, getApprovedProjectSession) and import them where used (e.g., in the IPC handler that calls approveFilePath and in locations that call approveReadableVideoPath/getApprovedProjectSession); otherwise remove all calls to these helpers from the IPC handler (and the other affected sites such as the recorded session restore logic around loadRecordedSessionForVideoPath and setCurrentRecordingSessionState) to eliminate dangling references and ensure the module builds cleanly.
🤖 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/main.ts`:
- Around line 347-369: The current code builds a dev-loosening CSP in
cspDirectives using VITE_DEV_URL and applies it via
session.defaultSession.webRequest.onHeadersReceived, but CSP meta in index.html
still exists and CSPs intersect (so the meta can block dev ws); fix by
centralizing CSP generation so only one active policy is used: either remove the
static meta tag from index.html and rely solely on the dynamically generated
cspDirectives (ensure VITE_DEV_URL is injected for dev), or make the index.html
meta CSP environment-aware and derive its value from the same VITE_DEV_URL logic
used for cspDirectives; update the build/dev injection step that produces the
index.html meta (or remove it) so the policy source for
connect-src/ws://localhost:* is identical to the header applied in
session.defaultSession.webRequest.onHeadersReceived.
In `@electron/windows.ts`:
- Around line 14-25: The current isAllowedNavigation allows any file:// URL with
an empty hostname, which effectively permits arbitrary local files; tighten this
by only allowing file:// when the parsed.pathname exactly matches one of your
renderer entry files (e.g., APP_HTML or renderer bundle paths) or matches a
small explicit whitelist constant (e.g., ALLOWED_RENDERER_FILES), instead of
just checking !parsed.hostname. Update isAllowedNavigation to compare
parsed.pathname (or decoded pathname) against that whitelist (and keep existing
app-media: and VITE_DEV_SERVER_URL checks unchanged).
In `@src/hooks/useScreenRecorder.test.ts`:
- Around line 3-10: Tests duplicate constants (BITRATE_4K, BITRATE_QHD,
BITRATE_BASE, HIGH_FRAME_RATE_THRESHOLD, HIGH_FRAME_RATE_BOOST, FOUR_K_PIXELS,
QHD_PIXELS) copied from useScreenRecorder.ts which is fragile; instead export
these constants from the source (or a shared constants module) and import them
into useScreenRecorder.test.ts so tests reference the single source of truth, or
at minimum add a clear comment in the test warning to keep values in sync with
the constants in useScreenRecorder.ts; update references in the test to use the
imported symbols (e.g., BITRATE_4K, HIGH_FRAME_RATE_BOOST) so changes in
useScreenRecorder.ts are reflected in tests.
- Around line 12-24: The test reimplements computeBitrate(width, height,
frameRate) which diverges from the real function in useScreenRecorder.ts that
only accepts (width, height) and uses TARGET_FRAME_RATE for the high-frame-rate
boost; update the test to use the real implementation by exporting
computeBitrate from useScreenRecorder.ts (or alternatively change the real
function signature to accept frameRate if you need that behavior), then import
that exported computeBitrate into useScreenRecorder.test.ts and remove the extra
frameRate parameter so tests like bitrate30fps and bitrate59fps reflect the
actual logic that compares against HIGH_FRAME_RATE_THRESHOLD /
TARGET_FRAME_RATE.
In `@src/hooks/useScreenRecorder.ts`:
- Around line 565-571: The webcam stop logic only checks
activeWebcamRecorder?.recorder.state === "recording" and should also handle the
"paused" state to avoid leaking the webcam stream when the screen recorder
errors; update the condition in the block that calls
activeWebcamRecorder.recorder.stop() to check for state === "recording" || state
=== "paused" (same logic used in stopRecording), keep the try/catch around
activeWebcamRecorder.recorder.stop(), and reference the
activeWebcamRecorder.recorder and stopRecording flow so the change mirrors the
existing stop behavior.
- Around line 554-580: The error event handler attached to
activeScreenRecorder.recorder doesn't reset recording-related state and refs the
way finalizeRecording does; inside the error handler (the callback passed to
activeScreenRecorder.recorder.addEventListener("error", ...)) add the same
cleanup steps as finalizeRecording by calling setPaused(false),
setElapsedSeconds(0) and clearing timing refs accumulatedDurationMs.current = 0
and segmentStartedAt.current = null (before or alongside teardownMedia and
setRecording(false)), so paused/elapsed/duration state won't persist after an
error.
In `@src/lib/compositeLayout.test.ts`:
- Around line 81-98: The vertical-stack test fails because
computeCompositeLayout (for "vertical-stack"/stack transforms) ignores
maxContentSize — maxContentSize is only applied via centerRect for non-stack
transforms — so either the test expectations or the implementation must change;
update computeCompositeLayout to apply maxContentSize when layoutPreset is
"vertical-stack" by constraining the computed screenRect/webcamRect sizes before
positioning (reuse centerRect or its scaling logic) so widths/heights do not
exceed maxContentSize, and ensure centering logic sets screenRect.x > 0;
specifically modify the stack-handling branch in computeCompositeLayout (and any
helper used to compute stacked rects) to clamp dimensions to maxContentSize and
then call the existing centering/positioning routine instead of returning full
canvas-sized rects.
In `@src/lib/customFonts.ts`:
- Around line 85-105: The current rejection regex in loadFont() is overly strict
and blocks valid Google Fonts URLs (it disallows ";" and ")" even though
link.href assignment prevents CSS injection); replace the broad check with a
safer one that only rejects characters that can break HTML attributes such as
quotes and backslashes (e.g., /['"\\]/) and/or defer to the existing
isValidGoogleFontsUrl(font.importUrl) check: first, if
isValidGoogleFontsUrl(font.importUrl) is true, allow it; otherwise validate
against the narrower regex (or reject). Update the block that references
font.importUrl and styleId in loadFont() accordingly so legitimate google font
URLs pass while still preventing real injection vectors.
In `@src/lib/exporter/frameRenderer.ts`:
- Around line 224-230: The validator permits app-media:// URIs but frameRenderer
doesn't handle them, so when isAllowedWallpaperValue(wallpaper) passes and
wallpaper startsWith("app-media://") it falls through and later bgCtx.fillStyle
= wallpaper will be invalid; update the wallpaper handling in frameRenderer (the
block that currently checks file://, data:, /, http) to detect app-media:// and
either resolve it to a usable file/data URL (e.g., via a new resolveAppMediaUri
function) and load it as an image into bgCanvas (like the other image branches)
or throw a clear error; touch symbols: isAllowedWallpaperValue, the local
wallpaper variable, bgCtx, bgCanvas, backgroundSprite and the image-loading
branch so app-media:// is converted into a valid image source before assigning
bgCtx.fillStyle or backgroundSprite.
In `@src/lib/securityValidation.test.ts`:
- Around line 3-12: Tests are duplicating the isAllowedExternalUrl predicate
instead of importing and exercising the real implementation, so extract the URL
allowlist helpers into a shared module (export the current isAllowedExternalUrl
logic as a single exported function) and update the tests to import that shared
function; alternatively, replace the unit tests with an integration-style test
that exercises the actual IPC validation path by invoking the IPC handler
function (e.g., the handler exported in ipc/handlers.ts) through a small
seam/mocked environment to ensure the policy used in runtime is what's being
tested rather than a copied implementation.
---
Outside diff comments:
In `@electron/ipc/handlers.ts`:
- Around line 455-469: The handler registered in
ipcMain.handle("get-cursor-telemetry") validates only targetVideoPath but then
reads `${targetVideoPath}.cursor.json`, allowing a malicious symlink to escape
trusted roots; to fix, after computing telemetryPath (the variable
telemetryPath) resolve its real path and re-run the trusted check (e.g., call
isTrustedMediaPath or a realpath-based check on telemetryPath) before calling
fs.readFile, and return the empty success/samples response if the telemetryPath
is not trusted—keep normalizeVideoSourcePath, targetVideoPath, telemetryPath,
and the existing return shape unchanged.
- Around line 816-839: The handler for the "set-current-video-path" IPC and
other call sites reference missing helper functions (approveFilePath,
approveReadableVideoPath, getApprovedProjectSession) which breaks builds; either
remove those calls or restore the original approval helpers. Fix by deciding on
the desired flow: if you want approval behavior, reintroduce and export the
helper implementations (names: approveFilePath, approveReadableVideoPath,
getApprovedProjectSession) and import them where used (e.g., in the IPC handler
that calls approveFilePath and in locations that call
approveReadableVideoPath/getApprovedProjectSession); otherwise remove all calls
to these helpers from the IPC handler (and the other affected sites such as the
recorded session restore logic around loadRecordedSessionForVideoPath and
setCurrentRecordingSessionState) to eliminate dangling references and ensure the
module builds cleanly.
In `@src/components/video-editor/VideoPlayback.tsx`:
- Around line 1150-1164: The conditional in VideoPlayback.tsx around the
wallpaper handling contains unreachable checks for wallpaper.startsWith("http")
and wallpaper.startsWith("file://") because isAllowedWallpaperValue already
rejects those; simplify the logic by removing the "http" and "file://" branches
and only handle the absolute-server-path case (wallpaper.startsWith("/")) by
resolving via getAssetPath and calling setResolvedWallpaper(p), otherwise just
call setResolvedWallpaper(wallpaper); adjust the block that references mounted,
getAssetPath, setResolvedWallpaper, and wallpaper accordingly to remove the dead
branches.
In `@src/lib/exporter/frameRenderer.ts`:
- Around line 234-254: The wallpaper handling branch contains unreachable checks
for "file://" and plain "http" because isAllowedWallpaperValue now filters
those; either remove those dead branches in the image-background handling (the
if-block that creates new Image() and the sub-branches that test
wallpaper.startsWith("file://") / "data:" / "http") and simplify to the
remaining cases, or keep the branches but add a clear defensive comment above
the conditional (near the Image creation and the wallpaper.startsWith(...)
checks) stating these branches are retained intentionally as defensive fallbacks
for unexpected inputs; update code paths that set imageUrl (the assignments
inside the http/file/data/absolute-path branches) accordingly so no unused
branches remain.
🪄 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: bf3dbdb1-bb9d-47d8-8ee6-6ea05b9bb3f3
📒 Files selected for processing (19)
electron-builder.json5electron/ipc/handlers.tselectron/main.tselectron/windows.tsentitlements.mac.inherit.plistentitlements.mac.plistindex.htmlsrc/components/video-editor/VideoPlayback.tsxsrc/components/video-editor/projectPersistence.test.tssrc/components/video-editor/projectPersistence.tssrc/hooks/useScreenRecorder.test.tssrc/hooks/useScreenRecorder.tssrc/lib/assetPath.tssrc/lib/compositeLayout.test.tssrc/lib/customFonts.test.tssrc/lib/customFonts.tssrc/lib/exporter/frameRenderer.tssrc/lib/exporter/streamingDecoder.tssrc/lib/securityValidation.test.ts
Comment on lines
+347
to
+369
| // Enforce Content Security Policy on all renderer windows | ||
| const VITE_DEV_URL = process.env["VITE_DEV_SERVER_URL"]; | ||
| const cspDirectives = [ | ||
| "default-src 'self' app-media:", | ||
| "script-src 'self'" + (VITE_DEV_URL ? ` ${VITE_DEV_URL}` : ""), | ||
| "style-src 'self' 'unsafe-inline' https://fonts.googleapis.com", | ||
| "font-src 'self' https://fonts.gstatic.com", | ||
| "img-src 'self' app-media: data: blob:", | ||
| "media-src 'self' app-media: blob:", | ||
| "connect-src 'self' https://fonts.googleapis.com" + | ||
| (VITE_DEV_URL ? ` ${VITE_DEV_URL} ws://localhost:*` : ""), | ||
| "frame-src 'none'", | ||
| "object-src 'none'", | ||
| ].join("; "); | ||
|
|
||
| session.defaultSession.webRequest.onHeadersReceived((details, callback) => { | ||
| callback({ | ||
| responseHeaders: { | ||
| ...details.responseHeaders, | ||
| "Content-Security-Policy": [cspDirectives], | ||
| }, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Don't try to loosen dev CSP with a second policy.
CSPs intersect, they don't override — kinda cursed, but it means the static index.html meta policy can still block connect-src for ws://localhost:* even though this header allows it. In dev that can break HMR / other websocket connects unless the meta tag is made env-aware or both policies come from one shared source.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@electron/main.ts` around lines 347 - 369, The current code builds a
dev-loosening CSP in cspDirectives using VITE_DEV_URL and applies it via
session.defaultSession.webRequest.onHeadersReceived, but CSP meta in index.html
still exists and CSPs intersect (so the meta can block dev ws); fix by
centralizing CSP generation so only one active policy is used: either remove the
static meta tag from index.html and rely solely on the dynamically generated
cspDirectives (ensure VITE_DEV_URL is injected for dev), or make the index.html
meta CSP environment-aware and derive its value from the same VITE_DEV_URL logic
used for cspDirectives; update the build/dev injection step that produces the
index.html meta (or remove it) so the policy source for
connect-src/ws://localhost:* is identical to the header applied in
session.defaultSession.webRequest.onHeadersReceived.
Comment on lines
+14
to
+25
| function isAllowedNavigation(url: string): boolean { | ||
| try { | ||
| const parsed = new URL(url); | ||
| // Allow app-media:// custom protocol | ||
| if (parsed.protocol === "app-media:") return true; | ||
| // Allow file:// only for local paths (not UNC/network) | ||
| if (parsed.protocol === "file:" && !parsed.hostname) return true; | ||
| // In dev mode, allow exact Vite dev server origin | ||
| if (VITE_DEV_SERVER_URL) { | ||
| const devOrigin = new URL(VITE_DEV_SERVER_URL).origin; | ||
| if (parsed.origin === devOrigin) return true; | ||
| } |
There was a problem hiding this comment.
file:// is still basically allow-all here.
!parsed.hostname matches file:///etc/passwd and file:///C:/Users/.../secret.txt, so this lowkey turns the guard into “allow any local file” instead of “stay on app-owned content.” Since trusted local media already goes through app-media://, this should be narrowed to the exact renderer entry file(s) you own.
🔒 Narrow the `file:` allowlist
+const RENDERER_ENTRY_PATH = pathToFileURL(path.join(RENDERER_DIST, "index.html")).pathname;
+
function isAllowedNavigation(url: string): boolean {
try {
const parsed = new URL(url);
// Allow app-media:// custom protocol
if (parsed.protocol === "app-media:") return true;
- // Allow file:// only for local paths (not UNC/network)
- if (parsed.protocol === "file:" && !parsed.hostname) return true;
+ // Only allow the packaged renderer entry, not arbitrary local files
+ if (parsed.protocol === "file:" && !parsed.hostname) {
+ return parsed.pathname === RENDERER_ENTRY_PATH;
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@electron/windows.ts` around lines 14 - 25, The current isAllowedNavigation
allows any file:// URL with an empty hostname, which effectively permits
arbitrary local files; tighten this by only allowing file:// when the
parsed.pathname exactly matches one of your renderer entry files (e.g., APP_HTML
or renderer bundle paths) or matches a small explicit whitelist constant (e.g.,
ALLOWED_RENDERER_FILES), instead of just checking !parsed.hostname. Update
isAllowedNavigation to compare parsed.pathname (or decoded pathname) against
that whitelist (and keep existing app-media: and VITE_DEV_SERVER_URL checks
unchanged).
Comment on lines
+3
to
+10
| // These constants mirror the ones in useScreenRecorder.ts | ||
| const BITRATE_4K = 45_000_000; | ||
| const BITRATE_QHD = 28_000_000; | ||
| const BITRATE_BASE = 18_000_000; | ||
| const HIGH_FRAME_RATE_THRESHOLD = 60; | ||
| const HIGH_FRAME_RATE_BOOST = 1.7; | ||
| const FOUR_K_PIXELS = 3840 * 2160; | ||
| const QHD_PIXELS = 2560 * 1440; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
nit: duplicating constants is fragile
these constants are copy-pasted from the source file. if someone updates BITRATE_4K or HIGH_FRAME_RATE_BOOST in useScreenRecorder.ts, the tests will keep passing against stale values and won't catch regressions.
consider exporting them from the source (maybe via a separate constants module) or at minimum add a comment warning maintainers to keep these in sync.
🤖 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 3 - 10, Tests duplicate
constants (BITRATE_4K, BITRATE_QHD, BITRATE_BASE, HIGH_FRAME_RATE_THRESHOLD,
HIGH_FRAME_RATE_BOOST, FOUR_K_PIXELS, QHD_PIXELS) copied from
useScreenRecorder.ts which is fragile; instead export these constants from the
source (or a shared constants module) and import them into
useScreenRecorder.test.ts so tests reference the single source of truth, or at
minimum add a clear comment in the test warning to keep values in sync with the
constants in useScreenRecorder.ts; update references in the test to use the
imported symbols (e.g., BITRATE_4K, HIGH_FRAME_RATE_BOOST) so changes in
useScreenRecorder.ts are reflected in tests.
Comment on lines
+12
to
+24
| // Replicate the fixed computeBitrate logic for testing | ||
| 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.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# verify the actual computeBitrate signature in useScreenRecorder.ts
rg -n 'computeBitrate' src/hooks/useScreenRecorder.tsRepository: siddharthvaddem/openscreen
Length of output: 199
test's computeBitrate doesn't match the actual function signature
confirmed: the actual computeBitrate in useScreenRecorder.ts (line 132) only takes (width, height) and bakes in TARGET_FRAME_RATE for the boost calc. but your test reimplements it with (width, height, frameRate) — so you're testing a modified version that never actually runs.
this means your bitrate30fps and bitrate59fps test cases are testing scenarios that literally can't happen in the real code. the actual function always uses 60fps for the boost check. lowkey risky because tests won't catch regressions if the real behavior changes.
pick one:
- export the actual
computeBitratefromuseScreenRecorder.tsand import it here (best option) - refactor the real function to accept
frameRateas a parameter if you actually need that flexibility
🤖 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 12 - 24, The test
reimplements computeBitrate(width, height, frameRate) which diverges from the
real function in useScreenRecorder.ts that only accepts (width, height) and uses
TARGET_FRAME_RATE for the high-frame-rate boost; update the test to use the real
implementation by exporting computeBitrate from useScreenRecorder.ts (or
alternatively change the real function signature to accept frameRate if you need
that behavior), then import that exported computeBitrate into
useScreenRecorder.test.ts and remove the extra frameRate parameter so tests like
bitrate30fps and bitrate59fps reflect the actual logic that compares against
HIGH_FRAME_RATE_THRESHOLD / TARGET_FRAME_RATE.
Comment on lines
+554
to
+580
| 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.
solid error cleanup, but a few state resets are missing
the new error handler is way more thorough than before — nice. but i noticed it doesn't reset setPaused(false), setElapsedSeconds(0), accumulatedDurationMs.current, or segmentStartedAt.current like finalizeRecording does at lines 218-221.
if a user hits an error mid-recording while paused, paused stays true until the next recording cycle. the elapsed seconds get cleaned up by the useEffect at line 704, but the timing refs staying stale could lead to weird duration calculations on the next recording if something goes sideways.
lowkey risky edge case — might want to align the cleanup with finalizeRecording:
🛠️ proposed fix
teardownMedia();
setRecording(false);
+ setPaused(false);
+ setElapsedSeconds(0);
+ accumulatedDurationMs.current = 0;
+ segmentStartedAt.current = null;
window.electronAPI?.setRecordingState(false);📝 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.
Suggested change
| 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 }, | |
| ); | |
| 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); | |
| setPaused(false); | |
| setElapsedSeconds(0); | |
| accumulatedDurationMs.current = 0; | |
| segmentStartedAt.current = null; | |
| window.electronAPI?.setRecordingState(false); | |
| if (finalizingRecordingId.current === activeRecordingId) { | |
| finalizingRecordingId.current = null; | |
| } | |
| }, | |
| { once: true }, | |
| ); |
🤖 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 event
handler attached to activeScreenRecorder.recorder doesn't reset
recording-related state and refs the way finalizeRecording does; inside the
error handler (the callback passed to
activeScreenRecorder.recorder.addEventListener("error", ...)) add the same
cleanup steps as finalizeRecording by calling setPaused(false),
setElapsedSeconds(0) and clearing timing refs accumulatedDurationMs.current = 0
and segmentStartedAt.current = null (before or alongside teardownMedia and
setRecording(false)), so paused/elapsed/duration state won't persist after an
error.
Comment on lines
+565
to
+571
| if (activeWebcamRecorder?.recorder.state === "recording") { | ||
| try { | ||
| activeWebcamRecorder.recorder.stop(); | ||
| } catch { | ||
| // Webcam recorder may already be stopped. | ||
| } | ||
| } |
There was a problem hiding this comment.
webcam stop condition doesn't handle paused state
stopRecording at lines 314-323 checks state === "recording" || state === "paused" before stopping the webcam recorder. but here you only check for "recording".
if the screen recorder errors while everything is paused, the webcam recorder won't get stopped and could leak the stream.
🔧 quick fix
- if (activeWebcamRecorder?.recorder.state === "recording") {
+ if (
+ activeWebcamRecorder?.recorder.state === "recording" ||
+ activeWebcamRecorder?.recorder.state === "paused"
+ ) {
try {
activeWebcamRecorder.recorder.stop();📝 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.
Suggested change
| if (activeWebcamRecorder?.recorder.state === "recording") { | |
| try { | |
| activeWebcamRecorder.recorder.stop(); | |
| } catch { | |
| // Webcam recorder may already be stopped. | |
| } | |
| } | |
| if ( | |
| activeWebcamRecorder?.recorder.state === "recording" || | |
| activeWebcamRecorder?.recorder.state === "paused" | |
| ) { | |
| try { | |
| activeWebcamRecorder.recorder.stop(); | |
| } catch { | |
| // Webcam recorder may already be stopped. | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks/useScreenRecorder.ts` around lines 565 - 571, The webcam stop logic
only checks activeWebcamRecorder?.recorder.state === "recording" and should also
handle the "paused" state to avoid leaking the webcam stream when the screen
recorder errors; update the condition in the block that calls
activeWebcamRecorder.recorder.stop() to check for state === "recording" || state
=== "paused" (same logic used in stopRecording), keep the try/catch around
activeWebcamRecorder.recorder.stop(), and reference the
activeWebcamRecorder.recorder and stopRecording flow so the change mirrors the
existing stop behavior.
src/lib/compositeLayout.test.ts
Outdated
Comment on lines
+81
to
+98
| 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.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Run the test suite to see which test fails, confirming the contradiction
# Check if both tests exist with same inputs
echo "=== Checking test inputs ==="
rg -n -A 10 "uses cover-style full-width stacking" src/lib/compositeLayout.test.ts
echo ""
rg -n -A 10 "bounds the vertical-stack layout" src/lib/compositeLayout.test.ts
echo ""
echo "=== Looking at implementation to understand expected behavior ==="
ast-grep --pattern $'function computeCompositeLayout($_) {
$$$
}'Repository: siddharthvaddem/openscreen
Length of output: 1132
🏁 Script executed:
sed -n '36,60p' src/lib/compositeLayout.test.tsRepository: siddharthvaddem/openscreen
Length of output: 703
🏁 Script executed:
sed -n '81,98p' src/lib/compositeLayout.test.tsRepository: siddharthvaddem/openscreen
Length of output: 833
🏁 Script executed:
# Find the computeCompositeLayout function
rg -n "function computeCompositeLayout|const computeCompositeLayout|export.*computeCompositeLayout" src/lib/compositeLayout.tsRepository: siddharthvaddem/openscreen
Length of output: 122
🏁 Script executed:
sed -n '122,200p' src/lib/compositeLayout.tsRepository: siddharthvaddem/openscreen
Length of output: 2383
🏁 Script executed:
# Also check if there are any recent changes or conditionals affecting this behavior
rg -n "vertical-stack" src/lib/compositeLayout.ts | head -20Repository: siddharthvaddem/openscreen
Length of output: 170
🏁 Script executed:
# Let me see the full vertical-stack definition to understand the preset
sed -n '85,100p' src/lib/compositeLayout.tsRepository: siddharthvaddem/openscreen
Length of output: 334
🏁 Script executed:
# Run the tests to see which one actually fails
npm test -- src/lib/compositeLayout.test.ts 2>&1 | head -100Repository: siddharthvaddem/openscreen
Length of output: 167
🏁 Script executed:
# Let me trace through the logic step by step for the test inputs
python3 << 'EOF'
# Test inputs
canvasSize_width = 1920
canvasSize_height = 1080
screenSize_width = 1920
screenSize_height = 1080
webcamSize_width = 1280
webcamSize_height = 720
# From vertical-stack implementation when webcam is present
webcamAspect = webcamSize_width / webcamSize_height
print(f"webcamAspect = {webcamSize_width} / {webcamSize_height} = {webcamAspect:.4f}")
resolvedWebcamWidth = canvasSize_width
resolvedWebcamHeight = round(canvasSize_width / webcamAspect)
print(f"resolvedWebcamHeight = round({canvasSize_width} / {webcamAspect:.4f}) = {resolvedWebcamHeight}")
screenRectHeight = canvasSize_height - resolvedWebcamHeight
print(f"screenRectHeight = {canvasSize_height} - {resolvedWebcamHeight} = {screenRectHeight}")
print(f"\nTest 1 expects:")
print(f" screenRect: x=0, width=1920, height=0")
print(f" webcamRect: x=0, width=1920, height=1080")
print(f"\nImplementation returns (for vertical-stack with webcam):")
print(f" screenRect: x=0, width={resolvedWebcamWidth}, height={max(0, screenRectHeight)}")
print(f" webcamRect: x=0, width={resolvedWebcamWidth}, height={resolvedWebcamHeight}")
print(f"\nTest 2 expects:")
print(f" screenRect.width <= 1536 (ACTUAL: {resolvedWebcamWidth})")
print(f" screenRect.x > 0 (ACTUAL: 0)")
print(f" screenRect.height + webcamRect.height <= 865 (ACTUAL: {max(0, screenRectHeight)} + {resolvedWebcamHeight} = {max(0, screenRectHeight) + resolvedWebcamHeight})")
EOFRepository: siddharthvaddem/openscreen
Length of output: 594
this test is gonna fail hard against test at lines 36-60
same identical inputs, but test 2's expectations are completely off. the implementation doesn't respect maxContentSize for vertical-stack layouts — it just fills the full canvas width.
looking at the code, maxContentSize only gets used in the centerRect call for non-"stack" transforms. for "stack" type (which vertical-stack is), it straight up ignores maxContentSize and returns:
screenRect: {x: 0, width: 1920, height: 0}webcamRect: {x: 0, width: 1920, height: 1080}
so test 2's assertions will all fail:
screenRect.width <= 1536— nope, it's 1920screenRect.x > 0— nope, it's 0- combined height <= 865 — nope, it's 1080
test 1 at lines 36-60 will pass. test 2 will fail. either test 2 needs its expectations fixed to match the actual behavior, or the implementation needs to actually respect maxContentSize for stacks.
🤖 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 vertical-stack
test fails because computeCompositeLayout (for "vertical-stack"/stack
transforms) ignores maxContentSize — maxContentSize is only applied via
centerRect for non-stack transforms — so either the test expectations or the
implementation must change; update computeCompositeLayout to apply
maxContentSize when layoutPreset is "vertical-stack" by constraining the
computed screenRect/webcamRect sizes before positioning (reuse centerRect or its
scaling logic) so widths/heights do not exceed maxContentSize, and ensure
centering logic sets screenRect.x > 0; specifically modify the stack-handling
branch in computeCompositeLayout (and any helper used to compute stacked rects)
to clamp dimensions to maxContentSize and then call the existing
centering/positioning routine instead of returning full canvas-sized rects.
src/lib/customFonts.ts
Outdated
Comment on lines
+85
to
+105
| // Reject CSS injection characters | ||
| if (/['";)\\]/.test(font.importUrl)) { | ||
| console.warn(`Rejected font URL with unsafe characters for "${font.name}"`); | ||
| reject(new Error(`Invalid font URL: contains unsafe characters`)); | ||
| return; | ||
| } | ||
|
|
||
| const styleId = `custom-font-${font.id}`; | ||
|
|
||
| // Remove existing style if present | ||
| // Remove existing element if present | ||
| const existing = document.getElementById(styleId); | ||
| if (existing) { | ||
| existing.remove(); | ||
| } | ||
|
|
||
| // Create style element with @import | ||
| const style = document.createElement("style"); | ||
| style.id = styleId; | ||
| style.textContent = `@import url('${font.importUrl}');`; | ||
| document.head.appendChild(style); | ||
| // Use <link> tag instead of @import template literal to prevent CSS injection | ||
| const link = document.createElement("link"); | ||
| link.id = styleId; | ||
| link.rel = "stylesheet"; | ||
| link.href = font.importUrl; | ||
| document.head.appendChild(link); |
There was a problem hiding this comment.
The old @import blocklist is now rejecting legit Google Fonts URLs.
loadFont() now assigns link.href, so ; and ) aren't breakout characters anymore. With this regex, common CSS2 URLs like https://fonts.googleapis.com/css2?family=Open+Sans:wght@400;700&display=swap get rejected even though isValidGoogleFontsUrl() accepts them.
🐛 Minimal fix
- // Reject CSS injection characters
- if (/['";)\\]/.test(font.importUrl)) {
- console.warn(`Rejected font URL with unsafe characters for "${font.name}"`);
- reject(new Error(`Invalid font URL: contains unsafe characters`));
- return;
- }
-
const styleId = `custom-font-${font.id}`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/customFonts.ts` around lines 85 - 105, The current rejection regex in
loadFont() is overly strict and blocks valid Google Fonts URLs (it disallows ";"
and ")" even though link.href assignment prevents CSS injection); replace the
broad check with a safer one that only rejects characters that can break HTML
attributes such as quotes and backslashes (e.g., /['"\\]/) and/or defer to the
existing isValidGoogleFontsUrl(font.importUrl) check: first, if
isValidGoogleFontsUrl(font.importUrl) is true, allow it; otherwise validate
against the narrower regex (or reject). Update the block that references
font.importUrl and styleId in loadFont() accordingly so legitimate google font
URLs pass while still preventing real injection vectors.
Comment on lines
+3
to
+12
| // 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.
🧹 Nitpick | 🔵 Trivial
These tests are copy-pasting the policy instead of exercising it.
Because the predicates live in this file too, the suite can stay green while electron/ipc/handlers.ts drifts. nit: cleaner to extract the URL allowlist helpers into a shared module and import them here, or cover the IPC handler through a tiny seam.
Also applies to: 51-63
🤖 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, Tests are
duplicating the isAllowedExternalUrl predicate instead of importing and
exercising the real implementation, so extract the URL allowlist helpers into a
shared module (export the current isAllowedExternalUrl logic as a single
exported function) and update the tests to import that shared function;
alternatively, replace the unit tests with an integration-style test that
exercises the actual IPC validation path by invoking the IPC handler function
(e.g., the handler exported in ipc/handlers.ts) through a small seam/mocked
environment to ensure the policy used in runtime is what's being tested rather
than a copied implementation.
…llpaper, fonts - Restore approvedPaths/isPathAllowed/approveReadableVideoPath/getApprovedProjectSession infrastructure that was inadvertently removed by exfiltration hardening commit - set-current-video-path: use isPathAllowed so user-picked external videos work - reveal-in-folder: use isPathAllowed so exported files can be revealed - save-exported-video: approve save path for subsequent reveal-in-folder - VideoPlayback: recognize app-media:// URLs in wallpaper resolution and isImageUrl - frameRenderer: recognize app-media:// URLs in wallpaper rendering during export - customFonts: allow semicolons in font URLs (Google Fonts weight lists use them) - Remove vertical-stack layout test that belongs with the layout fix (PR siddharthvaddem#361) Addresses review feedback from Codex, CodeRabbit, and Gemini across all findings.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
electron/ipc/handlers.ts (1)
926-950: 🧹 Nitpick | 🔵 Triviallooks correct, but minor style nit
line 934 passes
videoPathtoloadRecordedSessionForVideoPath()while we already havenormalizedVideoPath. the function normalizes internally so it works, but usingnormalizedVideoPathwould be cleaner for consistency.not a bug since internal normalization handles it.
♻️ optional cleanup
- 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 926 - 950, The call to loadRecordedSessionForVideoPath uses the original videoPath even though you already computed normalizedVideoPath with normalizeVideoSourcePath; update the call to pass normalizedVideoPath to keep inputs consistent (and avoid redundant internal normalization) while leaving the rest of the logic (approveFilePath, setCurrentRecordingSessionState, isPathAllowed) unchanged.src/components/video-editor/VideoPlayback.tsx (1)
1149-1165: 🧹 Nitpick | 🔵 Trivialnit: http branch is now dead code after validation guard
since
isAllowedWallpaperValue()rejects http URLs (per its implementation in projectPersistence.ts), any wallpaper starting with "http" would have been rejected at line 1128 and this branch can never be reached.not a bug, just slightly misleading. the comment at line 1149 still mentions "web/http" but that path is now blocked upstream.
lowkey could clean this up for clarity, but not blocking.
🤖 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 1149 - 1165, The http branch in the Wallpaper resolution block of VideoPlayback.tsx is dead because isAllowedWallpaperValue() already rejects http URLs; update the conditional that checks wallpaper.startsWith("http") to remove the http case (or adjust the comment) so the code and comment reflect current validation. Locate the block handling absolute paths (references: wallpaper, isAllowedWallpaperValue, getAssetPath, setResolvedWallpaper, mounted) and either remove the http check or change the explanatory comment to exclude "http/web" so the logic and documentation remain consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/customFonts.test.ts`:
- Around line 28-44: The tests currently assert the regex directly instead of
exercising the public API: update the tests to call loadFont() from customFonts
(mocking the DOM as needed) so they fail if the implementation changes;
specifically, replace the regex assertions with calls to loadFont(url) for each
dangerous URL and assert it rejects/throws, and for the Google Fonts weight-list
URL call loadFont() and assert it resolves/succeeds, ensuring loadFont() is
imported from customFonts.ts and any DOM methods used by loadFont (e.g.,
document.createElement, appendChild) are stubbed/mocked in the test setup.
---
Outside diff comments:
In `@electron/ipc/handlers.ts`:
- Around line 926-950: The call to loadRecordedSessionForVideoPath uses the
original videoPath even though you already computed normalizedVideoPath with
normalizeVideoSourcePath; update the call to pass normalizedVideoPath to keep
inputs consistent (and avoid redundant internal normalization) while leaving the
rest of the logic (approveFilePath, setCurrentRecordingSessionState,
isPathAllowed) unchanged.
In `@src/components/video-editor/VideoPlayback.tsx`:
- Around line 1149-1165: The http branch in the Wallpaper resolution block of
VideoPlayback.tsx is dead because isAllowedWallpaperValue() already rejects http
URLs; update the conditional that checks wallpaper.startsWith("http") to remove
the http case (or adjust the comment) so the code and comment reflect current
validation. Locate the block handling absolute paths (references: wallpaper,
isAllowedWallpaperValue, getAssetPath, setResolvedWallpaper, mounted) and either
remove the http check or change the explanatory comment to exclude "http/web" so
the logic and documentation remain consistent.
🪄 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: 7cac4b6e-5862-4f90-b6c4-29fe755f8bf7
📒 Files selected for processing (5)
electron/ipc/handlers.tssrc/components/video-editor/VideoPlayback.tsxsrc/lib/customFonts.test.tssrc/lib/customFonts.tssrc/lib/exporter/frameRenderer.ts
Comment on lines
+28
to
+44
| describe("font URL CSS injection prevention", () => { | ||
| it("injection characters would be caught by the character blocklist", () => { | ||
| // These characters are blocked in loadFont() via /['")\\ ]/ | ||
| // Semicolons are allowed (used in Google Fonts weight lists like wght@400;700) | ||
| const dangerousChars = ["'", '"', ")", "\\", " "]; | ||
| for (const char of dangerousChars) { | ||
| const url = `https://fonts.googleapis.com/css2?family=Roboto${char}`; | ||
| expect(/['")\\ ]/.test(url)).toBe(true); | ||
| } | ||
| }); | ||
|
|
||
| it("allows semicolons in Google Fonts weight lists", () => { | ||
| const url = "https://fonts.googleapis.com/css2?family=Open+Sans:wght@400;700&display=swap"; | ||
| // Semicolons should NOT be blocked — they're used for weight lists | ||
| expect(/['")\\ ]/.test(url)).toBe(false); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
nit: tests the regex pattern, not the actual function
these tests verify the regex /['")\\ ]/ catches dangerous chars, but they're testing the pattern directly rather than calling loadFont(). if someone changes the regex in customFonts.ts, these tests would still pass.
lowkey would be more robust to mock the DOM and test loadFont() rejects URLs with these characters, but this is fine for documenting expected behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/customFonts.test.ts` around lines 28 - 44, The tests currently assert
the regex directly instead of exercising the public API: update the tests to
call loadFont() from customFonts (mocking the DOM as needed) so they fail if the
implementation changes; specifically, replace the regex assertions with calls to
loadFont(url) for each dangerous URL and assert it rejects/throws, and for the
Google Fonts weight-list URL call loadFont() and assert it resolves/succeeds,
ensuring loadFont() is imported from customFonts.ts and any DOM methods used by
loadFont (e.g., document.createElement, appendChild) are stubbed/mocked in the
test setup.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Add defense-in-depth layers against data exfiltration: navigation guards, external URL allowlisting, font/wallpaper source validation, Content Security Policy with dual enforcement, macOS hardened runtime with entitlements, and decoder path validation.
Motivation
Even with IPC hardening (#359), a compromised renderer could still exfiltrate data through navigation redirects, font/image loading to attacker URLs,
shell.openExternalabuse, or inline script injection. This PR closes those channels with multiple independent layers so no single bypass defeats all protections.Type of Change
Related Issue(s)
Depends on #359. Extends the security hardening started in #352.
Screenshots / Video
N/A — security hardening with no UI changes.
Testing
npm test— all tests pass, including new regression testswindow.location = 'https://evil.com'→ should be blocked by navigation guarddefault-src 'self'policyhttp://URL → should be rejected byisAllowedWallpaperValuecodesign -dv --verbose)New test files:
src/lib/customFonts.test.ts— font URL protocol validation (http/https blocked, local/app-media allowed)src/lib/securityValidation.test.ts— additional URL scheme validation testssrc/components/video-editor/projectPersistence.test.ts— wallpaper value validation (asset paths, data URIs, colors, gradients; rejects http/traversal)Checklist
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 — parsed URL navigation, strict wallpaper paths, realpath fallback)This multi-LLM approach is particularly valuable for security work, where different models catch different classes of vulnerabilities and edge cases.
Thank you for contributing!
Summary by CodeRabbit
Security Enhancements
Bug Fixes