Fix security and reliability issues#401
Fix security and reliability issues#401hobostay wants to merge 1 commit intosiddharthvaddem:mainfrom
Conversation
1. Validate URL scheme in open-external-url handler - Prevent opening file:// or other dangerous schemes via shell.openExternal - Only allow http:, https:, and mailto: protocols 2. Fix latest video detection using mtime instead of lexicographic sort - Lexicographic sort gives wrong results (e.g. recording-9 > recording-10) - Now sorts by file modification time for reliable latest-file detection 3. Add null guard for AudioData.format in cloneWithTimestamp - Replace non-null assertion (!) with proper validation - Throws descriptive error if format is unexpectedly null 4. Prevent encodeQueue counter underflow in VideoExporter - Use Math.max(0, ...) to prevent negative queue count Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThree targeted robustness improvements: video recording lookup now uses filesystem modification timestamps instead of filename sorting, the URL handler validates and restricts to safe protocols ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 636-648: The code validates the URL but calls
shell.openExternal(url) with the original string; instead re-serialize the
parsed URL to ensure a normalized, validated value is used—after the
ALLOWED_SCHEMES check and using the parsed variable (e.g., parsed.toString() or
parsed.href) pass that into shell.openExternal rather than the raw url string to
follow Electron's recommended pattern.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0bcf54bc-0dcf-4d1d-a577-ef47abd7f55c
📒 Files selected for processing (3)
electron/ipc/handlers.tssrc/lib/exporter/audioEncoder.tssrc/lib/exporter/videoExporter.ts
| const ALLOWED_SCHEMES = ["http:", "https:", "mailto:"]; | ||
| let parsed: URL; | ||
| try { | ||
| parsed = new URL(url); | ||
| } catch { | ||
| return { success: false, error: "Invalid URL" }; | ||
| } | ||
|
|
||
| if (!ALLOWED_SCHEMES.includes(parsed.protocol)) { | ||
| return { success: false, error: `Unsupported URL scheme: ${parsed.protocol}` }; | ||
| } | ||
|
|
||
| await shell.openExternal(url); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Electron shell.openExternal security guidance for untrusted URLs: should validated URLs be normalized/canonicalized (for example with new URL(url).toString()) before calling shell.openExternal?
💡 Result:
Electron’s own guidance is not “normalize then call shell.openExternal”; it’s don’t call shell.openExternal on untrusted input at all, and if you must support user-provided links, parse with a real URL parser and enforce a strict allowlist (protocol and often host/origin). (electronjs.org)
So: canonicalizing (e.g., new URL(url).toString()) is not a security control by itself. It may make the string more consistent, but it doesn’t address the core risk (opening attacker-chosen schemes/handlers that can lead to code execution via OS protocol/file associations). (electronjs.org)
Practical recommendation (safe pattern):
- Parse:
const u = new URL(input) - Allowlist: permit only the exact schemes you intend (commonly
https:/http:; maybemailto:), and ideally also allowlist hosts/origins for web links. (electronjs.org) - Then call
shell.openExternal(u.toString())(oru.href) only after the allowlist checks. (Re-serializing is fine for consistency, just don’t treat it as the “security step”.) (electronjs.org)
Citations:
- 1: https://www.electronjs.org/docs/latest/tutorial/security
- 2: https://www.electronjs.org/docs/latest/tutorial/security
- 3: https://www.electronjs.org/docs/latest/tutorial/security
- 4: https://www.electronjs.org/docs/latest/tutorial/security
Use the normalized URL for consistency with Electron's recommended pattern.
The allowlist check is solid, but Electron's official security guidance suggests passing parsed.toString() (or parsed.href) to shell.openExternal after validation. Since you're already parsing and checking, re-serializing the normalized version is cleaner and follows the standard pattern.
fix
if (!ALLOWED_SCHEMES.includes(parsed.protocol)) {
return { success: false, error: `Unsupported URL scheme: ${parsed.protocol}` };
}
- await shell.openExternal(url);
+ await shell.openExternal(parsed.toString());
return { success: true };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@electron/ipc/handlers.ts` around lines 636 - 648, The code validates the URL
but calls shell.openExternal(url) with the original string; instead re-serialize
the parsed URL to ensure a normalized, validated value is used—after the
ALLOWED_SCHEMES check and using the parsed variable (e.g., parsed.toString() or
parsed.href) pass that into shell.openExternal rather than the raw url string to
follow Electron's recommended pattern.
1 participant
Summary
open-external-urlhandler — Theshell.openExternal(url)call accepted any URL without validation, allowingfile://,javascript:, or other dangerous schemes. Now onlyhttp:,https:, andmailto:are permitted.get-recorded-video-pathused lexicographic sort to find the latest recording (videoFiles.sort().reverse()[0]), which is unreliable (e.g.recording-9.webmsorts afterrecording-10.webm). Now sorts by file modification time.AudioData.format—cloneWithTimestampused a non-null assertion (src.format!) that could throw an unhelpful error ifformatis null. Now validates upfront with a descriptive error message.encodeQueueunderflow — TheencodeQueuecounter inVideoExportercould go negative if the encoder's output callback fires more times than expected. AddedMath.max(0, ...)guard.Test plan
open-external-urlblocksfile:///etc/passwdand allowshttps://example.com🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes