fix: reject getDisplayMedia calls without user gesture on server view#3313
fix: reject getDisplayMedia calls without user gesture on server view#3313arijiiiitttt wants to merge 2 commits into
Conversation
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe display-media request handler in server-view screen sharing now rejects requests that lack Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.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
🧹 Nitpick comments (1)
src/screenSharing/serverViewScreenSharing.ts (1)
74-85: Unify and type the deny response instead of mixedas anypayloads.Using both
cb({} as any)andcb({ video: false } as any)makes behavior/API compatibility harder to reason about and can mask Electron typing drift. Prefer one deny path helper with a verified payload shape.Suggested hardening
guestWebContents.session.setDisplayMediaRequestHandler( (request, cb) => { + const deny = () => cb({ video: false } as any); if (!request.userGesture) { - cb({} as any); + deny(); return; } try { currentProvider.handleDisplayMediaRequest(cb); } catch (error) { console.error( 'Server view screen sharing: error in handler:', error ); - cb({ video: false } as any); + deny(); } }, { useSystemPicker: false } );For the Electron version used by this project, what is the documented callback payload to deny/cancel a request in `session.setDisplayMediaRequestHandler`? Is `callback({})` supported, or should denial be done with `callback({ video: false })` or another explicit shape?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/screenSharing/serverViewScreenSharing.ts` around lines 74 - 85, Unify the denial path by replacing the mixed payloads with a single typed helper (e.g., denyDisplayMedia) and use it everywhere instead of cb({} as any) or cb({ video: false } as any); implement denyDisplayMedia(cb: DisplayMediaRequestHandlerCallback) that invokes cb with the explicit, Electron-typed denial payload (use the exact callback payload shape from Electron's typings) and update the branch that currently calls cb({} as any) and the catch block that calls cb({ video: false } as any) to call denyDisplayMedia(cb); ensure currentProvider.handleDisplayMediaRequest(cb) remains unchanged.
🤖 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/screenSharing/serverViewScreenSharing.ts`:
- Around line 72-88: The callback block passed to the screen-sharing API is
mis-indented and causing Prettier lint failures; reformat the anonymous callback
so its opening line, body (if check for request.userGesture, try/catch,
console.error, and cb calls), and the trailing options object ({
useSystemPicker: false }) follow project indentation rules—locate the block
around the call that invokes currentProvider.handleDisplayMediaRequest and the
surrounding callback, align braces and statements consistently (ensure the
if/return, try/catch and cb(...) lines are indented one level inside the
callback) to satisfy Prettier.
---
Nitpick comments:
In `@src/screenSharing/serverViewScreenSharing.ts`:
- Around line 74-85: Unify the denial path by replacing the mixed payloads with
a single typed helper (e.g., denyDisplayMedia) and use it everywhere instead of
cb({} as any) or cb({ video: false } as any); implement denyDisplayMedia(cb:
DisplayMediaRequestHandlerCallback) that invokes cb with the explicit,
Electron-typed denial payload (use the exact callback payload shape from
Electron's typings) and update the branch that currently calls cb({} as any) and
the catch block that calls cb({ video: false } as any) to call
denyDisplayMedia(cb); ensure currentProvider.handleDisplayMediaRequest(cb)
remains unchanged.
🪄 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: b70bda59-4b65-4acd-b3e3-13826c0a11c5
📒 Files selected for processing (1)
src/screenSharing/serverViewScreenSharing.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript for all new code in this codebase unless explicitly told otherwise
Use Fuselage components from@rocket.chat/fuselagefor all UI work — only create custom components when Fuselage doesn't provide the needed functionality
Check Theme.d.ts for valid color tokens when working with Fuselage components
Use optional chaining with fallbacks for platform-specific APIs instead of mocks (e.g., process.getuid?.() ?? 1000) to ensure code works across all platforms without requiring mocks
TypeScript code must use strict mode
Use React functional components with hooks instead of class components
Redux actions must follow the FSA (Flux Standard Action) pattern
Use camelCase for file naming
Use PascalCase for component file names (React components)
Write self-documenting code through clear naming — avoid unnecessary comments
Files:
src/screenSharing/serverViewScreenSharing.ts
🪛 ESLint
src/screenSharing/serverViewScreenSharing.ts
[error] 72-72: Insert ······
(prettier/prettier)
[error] 73-73: Insert ······
(prettier/prettier)
[error] 74-74: Replace ······ with ············
(prettier/prettier)
[error] 75-75: Insert ······
(prettier/prettier)
[error] 76-76: Insert ······
(prettier/prettier)
[error] 77-77: Insert ······
(prettier/prettier)
[error] 78-78: Insert ······
(prettier/prettier)
[error] 79-79: Insert ······
(prettier/prettier)
[error] 80-80: Replace ······ with ············
(prettier/prettier)
[error] 81-81: Insert ······
(prettier/prettier)
[error] 82-82: Insert ······
(prettier/prettier)
[error] 83-83: Replace ······ with ············
(prettier/prettier)
[error] 84-84: Replace ······ with ············
(prettier/prettier)
[error] 85-85: Insert ······
(prettier/prettier)
[error] 86-86: Insert ······
(prettier/prettier)
[error] 87-87: Insert ······
(prettier/prettier)
[error] 88-88: Insert ······
(prettier/prettier)
🔇 Additional comments (1)
src/screenSharing/serverViewScreenSharing.ts (1)
72-76: Nice fix: user-gesture gate addresses the startup picker regression.This guard cleanly blocks automatic
getDisplayMedia()probes from opening the picker while preserving user-initiated flow.
2 participants
Closes #3308
Problem
Since 4.14.0, the screen share picker opens automatically on every app launch on Linux/Wayland/Flatpak. PR #3266 registered
setDisplayMediaRequestHandler on every server view webview. The Rocket.Chat web app calls getDisplayMedia() at load time as a capability probe, and the newly registered handler catches this and opens the picker unconditionally.
Fix
Added a userGesture guard inside setupServerViewDisplayMedia() in serverViewScreenSharing.ts. If the request has no user gesture (i.e. triggered automatically by the page), it is silently rejected and the picker is not shown.
Testing
Summary by CodeRabbit
Bug Fixes
Refactor