fix(electron): scope capture permissions to the HUD#740
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe change adds centralized URL-, origin-, frame-, and window-based authorization for Electron media and display capture requests. It integrates these checks into Electron handlers, denies device permissions by default, and adds comprehensive Vitest coverage. ChangesCapture permission policy
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Renderer
participant Electron
participant PermissionPolicy
participant DesktopCapturer
Renderer->>Electron: Request media or display capture
Electron->>PermissionPolicy: Validate HUD window, frame, URL, and origin
PermissionPolicy-->>Electron: Grant or deny
Electron->>DesktopCapturer: Resolve sources when authorized
DesktopCapturer-->>Renderer: Capture sources or empty response
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
electron/permissionPolicy.test.ts (1)
135-182: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd display-capture grant tests for loopback and remaining file-origin variants.
shouldGrantMediaPermissiontests cover the packaged loopback renderer (lines 83-94) and all three Chromium file-origin forms"null","file://","file:///"(lines 96-111), butshouldGrantDisplayCaptureonly tests the dev HUD URL and the"file://"origin. The PR objectives state validation across "Vite, loopback, and packaged file URLs," so the loopback grant case and the additional file-origin forms should be covered here too.Since both functions delegate to the same
isSecurityOriginForDocument, the risk is low, but adding these cases ensures display-capture stays correct if the origin-validation path diverges in future refactors.♻️ Suggested additional display-capture tests
it("accepts the exact packaged file HUD with an opaque origin", () => { expect( shouldGrantDisplayCapture( makeRequest({ currentDocumentUrl: FILE_HUD_URL, securityOrigin: "file://" }), TRUSTED_DOCUMENT_BASE_URLS, ), ).toBe(true); }); + it("accepts the packaged loopback renderer with its exact origin", () => { + expect( + shouldGrantDisplayCapture( + makeRequest({ + currentDocumentUrl: PACKAGED_HUD_URL, + securityOrigin: "http://127.0.0.1:43127", + }), + TRUSTED_DOCUMENT_BASE_URLS, + ), + ).toBe(true); + }); + + it.each(["null", "file://", "file:///"])( + "accepts Chromium's packaged file origin form: %s", + (securityOrigin) => { + expect( + shouldGrantDisplayCapture( + makeRequest({ currentDocumentUrl: FILE_HUD_URL, securityOrigin }), + TRUSTED_DOCUMENT_BASE_URLS, + ), + ).toBe(true); + }, + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/permissionPolicy.test.ts` around lines 135 - 182, Add display-capture grant coverage in the shouldGrantDisplayCapture test suite for the packaged loopback renderer and file HUD using each remaining opaque origin form: "null" and "file:///". Reuse makeRequest with the appropriate loopback/file document URLs and securityOrigin values, asserting each request is granted alongside the existing "file://" case.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@electron/permissionPolicy.test.ts`:
- Around line 135-182: Add display-capture grant coverage in the
shouldGrantDisplayCapture test suite for the packaged loopback renderer and file
HUD using each remaining opaque origin form: "null" and "file:///". Reuse
makeRequest with the appropriate loopback/file document URLs and securityOrigin
values, asserting each request is granted alongside the existing "file://" case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6403e114-1bb7-40b4-b74f-bb4bf9a665cb
📒 Files selected for processing (3)
electron/main.tselectron/permissionPolicy.test.tselectron/permissionPolicy.ts
1 participant
Description
Scopes Electron capture permissions to Recordly's actual HUD instead of the entire default session.
mediafrom the live HUD main frameMotivation
The existing handlers granted media to every renderer regardless of window/origin, default-granted
HID/serial/USB, and ignored display-capture frame/origin details. A compromised non-recording
renderer could therefore request capabilities it does not need.
Electron 39 source review showed that
requestingOriginmay be a full URL and HTTP origins may endin
/. Both forms work while hostname, port, credential, path, query, frame, and window lookalikesare rejected.
Type of Change
Related Issue(s)
No linked issue. Independent of draft PRs #737-#739.
Testing Guide
npx vitest --run electron/permissionPolicy.test.ts src/hooks/useScreenRecorder.test.ts: 103/103npx biome lint electron/main.ts electron/permissionPolicy.ts electron/permissionPolicy.test.tsnpx tsc --noEmit --noUnusedLocals falsenpm test -- --reporter=dot: 97/97 files, 891/891 testsDefault
tsc --noEmitretains only the baseline failure addressed by #737.Risk / limits
Verified on Windows with unit, type, full-suite, and production-build gates. Physical devices and
packaged interactive capture were not run; macOS/Linux are source/policy-tested, not runtime-tested.
Async countdown and legacy desktop capture remain compatible: no user gesture or media subtype is
required.
Checklist
Summary by CodeRabbit
Security Improvements
windowTyperules.Tests