feat: Improve content recording prevention across main and child windows#3
feat: Improve content recording prevention across main and child windows#3KarthikrajanTP wants to merge 4 commits intomainfrom
Conversation
|
@coderabbitai review |
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes update the project version in Changes
Sequence Diagram(s)sequenceDiagram
participant ElectronApp as Electron App
participant Finder as find-process
participant System as macOS System
participant Logger as Logger
ElectronApp->>ElectronApp: On app ready
ElectronApp->>ElectronApp: registerCheckerForRecordersOnMacOS()
loop Every 10 seconds
ElectronApp->>Finder: Query for forbidden process names
Finder->>System: Scan running processes
System-->>Finder: Return list of running processes
Finder-->>ElectronApp: Return matches
alt Forbidden app found
ElectronApp->>Logger: Log warning
ElectronApp->>ElectronApp: app.exit()
ElectronApp->>System: process.exit(0)
else No forbidden app
ElectronApp->>Logger: No action
end
end
ElectronApp->>ElectronApp: On web-contents-created
ElectronApp->>ElectronApp: Set content protection on child window
Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Actions performedReview triggered.
|
1 similar comment
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://waps.l3s.uni-hannover.de/live/im_/https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/main.ts (2)
41-43: Redundant double-exit – stick to one shutdown pathCalling both
app.exit(1)andprocess.exit(1)is overkill.
app.exitalready terminates the Electron process; a secondprocess.exitcan skip Electron’s graceful shutdown hooks.Prefer a single call:
- app.exit(1); - process.exit(1); + app.quit(); // lets Electron run its shutdown sequence
85-89: Content protection listener misses windows created via your ownnew BrowserWindowcalls
did-create-windowfires only for windows spawned by existing web-contents (e.g.window.open).
If elsewhere in code you instantiatenew BrowserWindow(...), you still need to callsetContentProtection(true)manually.Consider wrapping
BrowserWindowcreation:export function secureBrowserWindow(opts: BrowserWindowConstructorOptions) { const win = new BrowserWindow(opts); win.setContentProtection(true); return win; }This guarantees protection regardless of the creation path.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
package.json(2 hunks)src/main.ts(3 hunks)
🔇 Additional comments (1)
package.json (1)
29-31: Double-check security & licensing of the newfind-processruntime dependencyAdding
find-process@1.4.10brings native process-scanning to production builds.
Before shipping, validate:
- The package still receives security updates (last publish was 2019).
- Its MIT licence – or any change thereof – is compatible with your distribution model.
- No known CVEs are registered for versions ≤ 1.4.10.
If anything looks stale, consider a maintained alternative (e.g.
ps-list) or vendor a minimal in-house helper.
src/main.ts
Outdated
| async function registerCheckerForRecordersOnMacOS(): Promise<void> { | ||
| if (process.platform === 'darwin') { | ||
| const forbiddenApps = [ | ||
| 'screencapture', | ||
| 'obs', | ||
| 'obs-ffmpeg-mux', | ||
| 'quicktime', | ||
| 'quicktimeplayerd', | ||
| 'camtasia', | ||
| 'snagit', | ||
| 'screenflow', | ||
| 'kap', | ||
| 'screenflick', | ||
| 'loom', | ||
| 'recordit', | ||
| 'cleanshot', | ||
| 'monosnap', | ||
| 'vlc', | ||
| 'gyazo', | ||
| ]; | ||
| setInterval(async () => { | ||
| for (const appName of forbiddenApps) { | ||
| try { | ||
| const found = await find('name', appName); | ||
| if (found.length > 0) { | ||
| console.warn(`Forbidden screen recording app detected: ${appName}`); | ||
| app.exit(1); | ||
| process.exit(1); | ||
| } | ||
| } catch (error) { | ||
| console.error(`Error while checking for process "${appName}":`, error); | ||
| } | ||
| } | ||
| }, 10000); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Interval scanner is unnecessarily slow & can hog CPU
The current implementation issues sequential ps invocations – one per forbidden app – every 10 s:
for (const appName of forbiddenApps) {
const found = await find('name', appName); // 20× serial
}On macOS this spawns ~20 shell processes each cycle; that’s ~12,000 per hour and shows up in Activity Monitor.
A single process list fetch (or a Promise.all) is plenty:
- setInterval(async () => {
- for (const appName of forbiddenApps) {
- const found = await find('name', appName);
- if (found.length > 0) {
- terminate();
- return;
- }
- }
- }, 10000);
+ const intervalId = setInterval(async () => {
+ try {
+ const processes = await find('name'); // one `ps` call
+ const hit = processes.find(p =>
+ forbiddenApps.some(f => p.name.toLowerCase().includes(f)));
+ if (hit) terminate();
+ } catch (err) {
+ console.error('Recorder scan failed:', err);
+ }
+ }, 10_000);
+
+ app.once('will-quit', () => clearInterval(intervalId));This slashes process-spawn overhead and cleans up the timer on quit.
🤖 Prompt for AI Agents
In src/main.ts between lines 15 and 50, the current code sequentially checks
each forbidden app by awaiting find calls one after another inside setInterval,
causing high CPU and process spawn overhead. To fix this, refactor the code to
fetch the full process list once per interval, then check all forbidden apps
against that list in memory, or use Promise.all to run all find calls
concurrently. Also, ensure to clear the interval when the app quits to avoid
lingering timers.
src/main.ts
Outdated
| @@ -1,5 +1,6 @@ | |||
| import { app, BrowserWindow } from 'electron'; | |||
| import electronUnhandled from 'electron-unhandled'; | |||
| import find from 'find-process'; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Type safety might be lost – find-process ships no bundled typings
find-process does not provide first-party .d.ts files and there is no @types/find-process package.
With typescript strict mode, import find from 'find-process' will be typed as any, silently disabling type-checking for every call site.
Options:
-import find from 'find-process';
+// 1. Keep type safety with an ad-hoc declaration file:
+// declare module 'find-process' { export default function
+// find(type: 'name' | 'pid', value?: string | number): Promise<any[]>; }
+// 2. Or fall back to CommonJS import with explicit any:
+// const find: any = require('find-process');Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/main.ts at line 3, the import of 'find-process' lacks TypeScript typings,
causing loss of type safety under strict mode. To fix this, create a custom type
declaration file (e.g., find-process.d.ts) in your project with appropriate type
definitions for the 'find-process' module, or declare the module with minimal
types to avoid using 'any'. Then ensure TypeScript picks up this declaration to
restore type checking for all uses of 'find-process'.
1 participant
setContentProtection(true)was applied only to the main window, leaving child BrowserWindow instances unprotected.Summary by CodeRabbit
New Features
Chores