feat(browser): report realistic permission states from the embedded browser - #1344
Conversation
…rowser Electron answers every permission CHECK "granted", so navigator.permissions .query returned granted for notifications/geolocation/camera/microphone/midi/ clipboard — impossible in a real Chrome and an obvious automation tell. Add a shared policy (isDefaultGrantedPermission) used by both the request handler and a new check handler: grant exactly what a fresh Chrome grants without prompting (midi, clipboard-sanitized-write, background-sync, sensors), deny the rest. Camera/mic/geolocation stay denied. Measured before/after in an Electron 40.8.0 harness.
|
Warning Review limit reached
More reviews will be available in 5 minutes and 58 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
✨ Finishing Touches🧪 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 |
github-actions
Bot
added
the
platform
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Suggested priority: P2 (includes user-path files (packages/desktop-electron/src/main/browser/controller.ts, packages/desktop-electron/src/main/browser/logic.test.ts, packages/desktop-electron/src/main/browser/logic.ts)).
P1/P0 are reserved for maintainer confirmation. Please relabel manually if this is a release blocker, security issue, data-loss risk, or updater/runtime failure.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a consistent permission policy for the embedded Electron browser by aligning both permission requests and checks with standard Chrome defaults to prevent automation detection. Feedback suggests adding "fullscreen" and "pointerLock" to the default granted permissions to avoid breaking standard HTML5 features, along with updating the corresponding unit tests.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
…dependency Two review fixes on the parity doc: - The embedded browser rejects file://, so "load the file" was not runnable. Serve the probe over a local HTTP server and open the same http://127.0.0.1 URL in both browsers (same origin, apples to apples). - The parity table records state that assumes #1343 (UA) and #1344 (permissions) are applied; neither is on dev yet. Say so explicitly and note this PR should land after both — on its own it is a forward-looking baseline, not current dev behavior.
… real Chrome)
Re-measured navigator.permissions.query on both sides (fresh real Chrome 149 +
the embedded browser via the same check-handler). Two corrections to the policy:
- Remove `midi`: Chrome gates Web MIDI behind a prompt since Chrome 124
(BlockMidiByDefault), so the prior grant was both a privacy footgun (silent
requestMIDIAccess via the request handler) and a fingerprint tell (we returned
granted where Chrome returns prompt). It now maps to denied, the faithful
substitute, like the other prompt-type permissions.
- Add `payment-handler`: Chrome returns granted for it by default; we were
returning denied, a mismatch. The check handler does receive "payment-handler",
so granting it now matches Chrome.
Final granted set = {clipboard-sanitized-write, background-sync, sensors,
payment-handler}, which reproduces Chrome's default granted set exactly; all
prompt-type permissions stay denied. Tests updated; 20 pass, typecheck clean.
…tifications The probe only flagged a notifications/Notification.permission mismatch, so a regression that left geolocation/camera/midi/etc. "granted" — exactly the tell PR #1344 fixes — would have rendered green and been missed in manual diffing. Encode the expected default state for each permission (measured against a fresh real Chrome) and flag both directions of mismatch red: a sensitive permission reporting "granted", or a default-granted permission (clipboard-write, background-sync, accelerometer, payment-handler) not "granted". Validated in real Chrome: every permission row is green (zero false positives).
…n gesture) The deny-all-except-N handler also denied `fullscreen` and `pointerLock`, which real Chrome allows on a user gesture without a prompt — so fullscreen video and pointer-lock pages broke in the embedded browser, and a rejected requestFullscreen is itself a non-Chrome tell. `fullscreen` is measurement-confirmed to reach the request handler; `pointerLock` is Electron's documented request-permission name for the same allow-on-gesture API. Neither is exposed via permissions.query, so the query fingerprint is unchanged. Tests added; 21 pass, typecheck clean.
Per review: the policy rationale was duplicated across controller.ts and logic.ts and repeated the PR description. Keep the controller comment to the invariant (one shared policy on both handlers so query state and request outcome agree) and the logic.ts comment to a concise policy statement; the detailed measurement / per-permission rationale lives in the PR description. Behavior unchanged; 21 pass, typecheck clean.
1 participant
Summary
Make the embedded browser report realistic permission states. Electron's default is to answer every permission check "granted", so
navigator.permissions.query(...)returnsgrantedfor notifications, geolocation, camera, microphone, midi and clipboard — a state that is impossible in a real Chrome (you cannot have camera + microphone + geolocation + notifications all granted, unprompted, on a fresh profile) and an obvious automation tell. The app's existing handler only covered permission requests, not checks.This introduces one shared policy (
isDefaultGrantedPermission) used by both the request handler and a new check handler: grant exactly the permissions a fresh Chrome grants without prompting (clipboard-sanitized-write,background-sync,sensors,payment-handler) and deny the rest. Electron's boolean handler cannot express Chrome'spromptdefault, so denied is the faithful substitute — a normal privacy state, and far better than the impossiblegranted. Camera/microphone/geolocation stay denied, preserving the original "a content viewer must not silently grant them" intent.The grant set was verified by measuring
navigator.permissions.queryon both sides — a fresh real Chrome 149 and the embedded browser through the same check handler. Notablymidiis denied, not granted: Chrome gates Web MIDI behind a prompt since Chrome 124 (BlockMidiByDefault), so granting it would be both a privacy footgun (silentrequestMIDIAccess) and a tell; andpayment-handleris granted to match Chrome's default.Second of the embedded-browser stealth-fidelity changes (after the UA work in #1343). Flat/independent of it.
Why this exact shape (measured, not guessed)
I reproduced PawWork's embedded-browser config in a standalone Electron 40.8.0 (Chromium 144) harness and measured
navigator.permissions.querybefore and after:The harness captured the exact Electron check-permission strings (camera and microphone both arrive as
media;payment-handlerandmidiboth reach the handler), so the policy keys on real names rather than guesses. Every "this PR" / "real Chrome" cell above was measured, not assumed.Honest scope: this fixes the permission-state fingerprint only; it is not a behavioral-risk-control fix.
Related Issue
None — follow-up from a maintainer report; no tracking issue yet.
Human Review Status
Pending
Review Focus
isDefaultGrantedPermission(logic.ts) — is "match Chrome's default-granted, deny the rest" the right policy?deniedsubstitutes for Chrome'spromptbecause Electron's handler can't returnprompt.permissions.querystate and an actual request outcome always agree. The original deny-all is relaxed only for the four benign default-granted permissions (no camera/mic/geo/notifications).Risk Notes
clipboard-sanitized-write/background-sync/sensors/payment-handler(previously denied all). These are the permissions a normal Chrome grants without a prompt and carry no camera/mic/geo/personal-data exposure.fullscreenandpointerLock, which Chrome allows on a user gesture without a prompt. They are not exposed viapermissions.query(so the fingerprint table above is unchanged), but the old deny-all broke fullscreen video and pointer-lock pages and a rejectedrequestFullscreenis itself a tell.fullscreenis measurement-confirmed to reach the request handler;pointerLockis Electron's documented request-permission name for the same API.How To Verify
Verified by me (CI-checkable):
Empirical (Electron 40.8.0 harness reproducing the embedded config): the before/after table above — every
permissions.querywasgrantedbefore; after, the sensitive/prompt-type ones aredeniedandNotification.permissionis consistent with its query.Optional runtime re-check (your machine): open the fingerprint probe in
dev:desktop's embedded browser; the permission rows should match the "this PR" column (probe flags the notifications inconsistency red — it stays green here).Screenshots or Recordings
N/A — no visible UI change.
Checklist
bug,enhancement,task,documentation.app,ui,platform,harness,ci.P0,P1,P2,P3.Pending.dev, and my PR title and commit messages use Conventional Commits in English.