fix(macOS): fix three screen capture permission issues in Electron layer#561
Conversation
…allowlists setPermissionCheckHandler and setPermissionRequestHandler only allowed ["media", "audioCapture", "microphone", "videoCapture", "camera"], causing any renderer-side getUserMedia/desktopCapturer request using a screen source to be silently denied by Electron before macOS TCC is ever consulted. Fix: add "screen" and "display-capture" to both handler allowlists.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughApp now requests and reports macOS screen-recording permission: entitlement updated, new IPC handler to check/trigger screen access, session permission check/request allowlists include ChangesScreen Recording Permissions
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
(also: nit — lowkey risky to fire-and-forget prompts; kinda cursed but pragmatic.) 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 085ac2a5cd
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // desktopCapturer.getSources() calls later (fixes repeated permission dialog). | ||
| const screenStatus = systemPreferences.getMediaAccessStatus("screen"); | ||
| if (screenStatus === "not-determined") { | ||
| systemPreferences.askForMediaAccess("screen"); |
There was a problem hiding this comment.
On macOS with ScreenCapture still not-determined, this calls systemPreferences.askForMediaAccess("screen"), but Electron's API only supports microphone and camera for askForMediaAccess and rejects invalid media types. Because this call is not awaited or caught, startup can emit an unhandled rejection and, more importantly, it will not show the Screen Recording TCC prompt this patch relies on; the same unsupported call was also added in the new request-screen-access handler.
Useful? React with 👍 / 👎.
auberginewly
force-pushed
the
fix/electron-screen-capture-permissions
branch
from
085ac2a to
44188d2
Compare
![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
♻️ Duplicate comments (1)
electron/main.ts (1)
486-493:⚠️ Potential issue | 🔴 Critical | ⚡ Quick wincritical: same
askForMediaAccess("screen")bug here tooline 492 has the identical issue as handlers.ts –
askForMediaAccessdoesn't accept"screen"as a parameter (only"microphone"or"camera"). this will break at runtime.the good news: you don't actually need it. calling
getMediaAccessStatus("screen")on line 490 already triggers the TCC prompt when the status is"not-determined". that's literally how the screen recording permission flow works on macOS.just remove line 492 and you're golden – the getMediaAccessStatus call handles everything.
🛠️ proposed fix
// Request microphone and screen recording permissions from macOS if (process.platform === "darwin") { const micStatus = systemPreferences.getMediaAccessStatus("microphone"); if (micStatus !== "granted") { await systemPreferences.askForMediaAccess("microphone"); } // Screen recording has no programmatic grant API — getMediaAccessStatus triggers // the TCC prompt on first call when status is "not-determined", which is the // intended behaviour. Checking here avoids repeated prompts driven by // desktopCapturer.getSources() calls later (fixes repeated permission dialog). const screenStatus = systemPreferences.getMediaAccessStatus("screen"); - if (screenStatus === "not-determined") { - systemPreferences.askForMediaAccess("screen"); - } + // The getMediaAccessStatus call above already triggered the prompt if needed }🤖 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/main.ts` around lines 486 - 493, The call to systemPreferences.askForMediaAccess("screen") is invalid (askForMediaAccess only accepts "microphone" or "camera") and unnecessary because systemPreferences.getMediaAccessStatus("screen") already triggers the macOS TCC prompt when status === "not-determined"; remove the stray systemPreferences.askForMediaAccess("screen") invocation in electron/main.ts (the code surrounding systemPreferences.getMediaAccessStatus("screen") should remain) to prevent the runtime error.
🤖 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.
Inline comments:
In `@electron/ipc/handlers.ts`:
- Around line 721-745: In the ipcMain.handle("request-screen-access") handler
remove the invalid call to systemPreferences.askForMediaAccess("screen") (it
only accepts "microphone" or "camera") and leave the
getMediaAccessStatus("screen") checks intact; adjust the branch that currently
calls askForMediaAccess to simply return the not-determined response (e.g., {
success: true, granted: false, status: "not-determined" }) so the handler no
longer invokes the unsupported method (refer to the handler for
"request-screen-access", systemPreferences.askForMediaAccess, and
systemPreferences.getMediaAccessStatus).
---
Duplicate comments:
In `@electron/main.ts`:
- Around line 486-493: The call to systemPreferences.askForMediaAccess("screen")
is invalid (askForMediaAccess only accepts "microphone" or "camera") and
unnecessary because systemPreferences.getMediaAccessStatus("screen") already
triggers the macOS TCC prompt when status === "not-determined"; remove the stray
systemPreferences.askForMediaAccess("screen") invocation in electron/main.ts
(the code surrounding systemPreferences.getMediaAccessStatus("screen") should
remain) to prevent the runtime error.
🪄 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: 290d3c00-3c6e-46cd-a9bc-62f0aa6e54bf
📒 Files selected for processing (3)
electron-builder.json5electron/ipc/handlers.tselectron/main.ts
auberginewly
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
electron/ipc/handlers.ts (1)
732-737:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
askForMediaAccess("screen")is invalid here and will break type-check/runtime.lowkey risky + kinda cursed combo: the comment says there’s no API equivalent, but Line 736 still calls it. Remove that call and just return
not-determined(or trigger prompt via actual capture flow elsewhere).Proposed minimal fix
// Screen recording has no askForMediaAccess equivalent — calling // getMediaAccessStatus("screen") when status is "not-determined" is // sufficient to trigger the TCC prompt. if (status === "not-determined") { - systemPreferences.askForMediaAccess("screen"); return { success: true, granted: false, status: "not-determined" }; }#!/bin/bash # Verify there are no unsupported Electron calls left. # Expected result after fix: no matches. rg -nP --type=ts 'askForMediaAccess\("screen"\)'🤖 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/ipc/handlers.ts` around lines 732 - 737, In the ipc handler in handlers.ts where you handle screen media permission status (the branch checking if status === "not-determined"), remove the invalid call to systemPreferences.askForMediaAccess("screen") and simply return { success: true, granted: false, status: "not-determined" } (or keep the existing return) so you don't invoke an unsupported Electron API; update any adjacent comment to note that prompting for screen capture must occur via the actual capture flow instead of askForMediaAccess.
🤖 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.
Inline comments:
In `@electron/main.ts`:
- Around line 490-493: Remove the invalid call to
systemPreferences.askForMediaAccess("screen") and rely solely on
systemPreferences.getMediaAccessStatus("screen") to trigger the TCC prompt;
specifically, delete the askForMediaAccess invocation in the block that checks
screenStatus (look for systemPreferences.getMediaAccessStatus and
systemPreferences.askForMediaAccess in electron/main.ts) and apply the same
removal in the analogous handler in electron/ipc/handlers.ts around the code
that checks screen permission. Ensure no other code expects
askForMediaAccess("screen") to return a promise or value.
---
Duplicate comments:
In `@electron/ipc/handlers.ts`:
- Around line 732-737: In the ipc handler in handlers.ts where you handle screen
media permission status (the branch checking if status === "not-determined"),
remove the invalid call to systemPreferences.askForMediaAccess("screen") and
simply return { success: true, granted: false, status: "not-determined" } (or
keep the existing return) so you don't invoke an unsupported Electron API;
update any adjacent comment to note that prompting for screen capture must occur
via the actual capture flow instead of askForMediaAccess.
🪄 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: 596320c7-b283-470c-bcbf-39208c1bf545
📒 Files selected for processing (3)
electron-builder.json5electron/ipc/handlers.tselectron/main.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- electron-builder.json5
… startup Microphone permission is checked at startup via getMediaAccessStatus, and camera has a dedicated request-camera-access IPC handler, but screen recording relied entirely on desktopCapturer.getSources() to implicitly trigger the TCC prompt — causing the permission dialog to reappear on every launch (issue siddharthvaddem#558). Note: askForMediaAccess() only accepts "microphone" | "camera"; screen recording TCC is triggered via desktopCapturer.getSources() instead. Fix: - Import desktopCapturer in main.ts - Call getMediaAccessStatus("screen") in app.whenReady(); trigger the TCC prompt via getSources when status is "not-determined" - Add request-screen-access IPC handler symmetric to request-camera-access
com.apple.security.device.audio-input is an entitlement key and should only appear in macos.entitlements. Placing it in extendInfo writes it into Info.plist where it has no effect and is misleading. The correct entry already exists in macos.entitlements; this removes the redundant, incorrectly-placed duplicate.
auberginewly
force-pushed
the
fix/electron-screen-capture-permissions
branch
from
44188d2 to
3dd7b85
Compare
|
is this ready for review? |
|
Yes, ready for review! |
2 participants
Background
While investigating the screen picker showing "Screen (0) / Window (0)" on macOS, three independent permission bugs were found — all related to but distinct from the
NSScreenCaptureUsageDescriptionfix in PR #554.Closes #558
Changes
Commit 1 —
electron/main.ts: missing screen permissions in Electron allowlistsBoth
setPermissionCheckHandlerandsetPermissionRequestHandleronly allowed:"screen"and"display-capture"were missing.Impact: when the renderer requests a screen source via
getUserMedia(chromeMediaSource: 'desktop'), Electron's own permission layer silently rejects it before macOS TCC is ever consulted.Fix: add
"screen"and"display-capture"to both handler allowlists.Commit 2 —
electron/main.ts+electron/ipc/handlers.ts: no proactive screen permission checkMicrophone is checked at startup with
getMediaAccessStatus+askForMediaAccess. Camera has arequest-camera-accessIPC handler. Screen recording had neither — it relied entirely ondesktopCapturer.getSources()to implicitly trigger the TCC prompt.Impact: permission status is never settled at startup, so the system dialog can reappear on subsequent launches — the root cause of #558.
Fix:
app.whenReady(), checkgetMediaAccessStatus("screen")and trigger the TCC prompt when status is"not-determined", mirroring the mic flowrequest-screen-accessIPC handler, symmetric torequest-camera-accessCommit 3 —
electron-builder.json5: entitlement key misplaced inextendInfocom.apple.security.device.audio-inputis an entitlement key and belongs only inmacos.entitlements. Writing it intoextendInfoplaces it inInfo.plistwhere it has no effect and is misleading.Fix: remove it from
extendInfo—macos.entitlementsalready has the correct entry.Testing
tccutil reset ScreenCapture com.siddharthvaddem.openscreen): only one permission dialog on first launchdesktopCapturer.getSources()returns screen and window sources correctlySummary by CodeRabbit
New Features
Bug Fixes