security(CORE-1129): validate ipcMain senders against allowlist#3315
security(CORE-1129): validate ipcMain senders against allowlist#3315jeanfbrito wants to merge 5 commits into
Conversation
Add isTrustedSender helper and apply it to all audited ipcMain handlers. Server-webview channels validate sender origin against Redux state.servers; local-window channels validate by WebContents instance identity (via registered getters). log-viewer-window/get-server-tag now derives origin from event.sender.getURL() and ignores the renderer-supplied parameter.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds IPC sender validation with a per-class window getter registry and an Changes
Sequence Diagram(s)sequenceDiagram
participant Sender as IPC Sender (WebContents)
participant Handler as IPC Handler
participant Validator as isTrustedSender()
participant Store as Redux Store
participant Registry as Window Registry
Sender->>Handler: send IPC message
Handler->>Validator: isTrustedSender(sender, allow)
alt server-webview check
Validator->>Sender: ensure type == "webview"
Validator->>Sender: call sender.getURL()
Validator->>Store: select servers list
Store-->>Validator: return server URLs
Validator->>Validator: compare origins
else local-window check
Validator->>Registry: call registered getter for class
Registry-->>Validator: return trusted WebContents
Validator->>Validator: compare sender.id or sender.hostWebContents.id
end
alt trusted
Validator-->>Handler: true
Handler->>Handler: process request
Handler-->>Sender: respond
else untrusted
Validator-->>Handler: false
Handler->>Handler: log warning / exit early
Handler-->>Sender: return empty/null
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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. Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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: 5/8 reviews remaining, refill in 19 minutes and 16 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: 2
🧹 Nitpick comments (1)
src/logging/index.ts (1)
286-289: Prefer origin equality instead of URL prefix matching.
startsWithcan misclassify look-alike URLs. Usenew URL(...).originequality for robust matching.♻️ Proposed refactor
- const matchedServer = servers.find( - (s: any) => - s.url && senderUrl.startsWith(s.url.replace(/\/$/, '')) - ); + const senderOrigin = new URL(senderUrl).origin; + const matchedServer = servers.find((s: any) => { + try { + return s.url && new URL(s.url).origin === senderOrigin; + } catch { + return false; + } + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/logging/index.ts` around lines 286 - 289, The current servers.find that sets matchedServer using startsWith on s.url (in the matchedServer assignment) can misclassify similar URLs; change the logic to compare origins by constructing URL objects and comparing new URL(senderUrl).origin === new URL(s.url).origin (handle possible trailing slashes and wrap in try/catch to guard bad URLs) instead of using senderUrl.startsWith(s.url.replace(/\/$/, '')) so matching is done by origin equality.
🤖 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/servers/supportedVersions/main.ts`:
- Around line 509-517: The handler for 'refresh-supported-versions' only
verifies sender type but doesn't bind the serverUrl to the sender origin; modify
the ipcMain.handle callback so after isTrustedSender succeeds you derive the
origin from event.sender.getURL() (or parse event.sender.getURL() into an
origin) and either validate that the provided serverUrl has the same origin or
replace serverUrl with the sender origin before calling
updateSupportedVersionsData; update the logic around isTrustedSender,
event.sender.getURL(), and updateSupportedVersionsData to enforce origin
binding.
In `@src/videoCallWindow/ipc.ts`:
- Around line 311-317: The early return when isTrustedSender(onceEvent.sender,
['video-call']) fails leaves the caller waiting; before returning, notify the
original caller (callerWebContents) with the
'video-call-window/screen-sharing-source-responded' channel indicating
rejection/failure and include any error reason, then safely return. Locate the
check around isTrustedSender and onceEvent.sender and ensure you call
callerWebContents.send('video-call-window/screen-sharing-source-responded', {
success: false, error: 'untrusted-sender' }) (or equivalent payload) before the
existing return so the caller-side listener is always completed.
---
Nitpick comments:
In `@src/logging/index.ts`:
- Around line 286-289: The current servers.find that sets matchedServer using
startsWith on s.url (in the matchedServer assignment) can misclassify similar
URLs; change the logic to compare origins by constructing URL objects and
comparing new URL(senderUrl).origin === new URL(s.url).origin (handle possible
trailing slashes and wrap in try/catch to guard bad URLs) instead of using
senderUrl.startsWith(s.url.replace(/\/$/, '')) so matching is done by origin
equality.
🪄 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: 4ea7440f-fc3e-443c-9e17-9a797ab7e144
📒 Files selected for processing (6)
src/ipc/main/validateSender.spec.tssrc/ipc/validateSender.tssrc/logViewerWindow/ipc.tssrc/logging/index.tssrc/servers/supportedVersions/main.tssrc/videoCallWindow/ipc.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: check (ubuntu-latest)
- GitHub Check: check (windows-latest)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{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/logViewerWindow/ipc.tssrc/servers/supportedVersions/main.tssrc/ipc/main/validateSender.spec.tssrc/logging/index.tssrc/videoCallWindow/ipc.tssrc/ipc/validateSender.ts
**/*.spec.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use *.spec.ts file naming convention for Renderer process tests
Files:
src/ipc/main/validateSender.spec.ts
**/*.{spec,main.spec}.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{spec,main.spec}.ts: Tests must run and pass on Windows, macOS, and Linux CI environments — always verify cross-platform compatibility
Only mock Linux-only APIs (process.getuid(), process.getgid(), process.geteuid(), process.getegid()) when defensive coding with optional chaining isn't possible
Files:
src/ipc/main/validateSender.spec.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: Ram-sah19
Repo: RocketChat/Rocket.Chat.Electron PR: 3254
File: .github/workflows/build-release.yml:80-94
Timestamp: 2026-03-11T06:38:40.426Z
Learning: In the RocketChat/Rocket.Chat.Electron repository, the issues flagged in `.github/workflows/build-release.yml` (e.g., `node12` runtime in the release action and missing `snapcraft_token` input), i18n files, and `electron-builder.json` are pre-existing in the `develop` branch and are pulled in during merge conflict resolution. Do not flag these as new issues introduced by PRs that only modify `src/injected.ts` and `src/ui/main/rootWindow.ts`.
📚 Learning: 2026-04-06T18:34:44.375Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-06T18:34:44.375Z
Learning: Applies to **/*.{spec,main.spec}.ts : Only mock Linux-only APIs (process.getuid(), process.getgid(), process.geteuid(), process.getegid()) when defensive coding with optional chaining isn't possible
Applied to files:
src/ipc/main/validateSender.spec.ts
📚 Learning: 2026-04-06T18:34:44.375Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-06T18:34:44.375Z
Learning: Applies to **/*.spec.ts : Use *.spec.ts file naming convention for Renderer process tests
Applied to files:
src/ipc/main/validateSender.spec.ts
🔇 Additional comments (5)
src/ipc/validateSender.ts (1)
41-89: Good centralized trust gate with deny-by-default behavior.The split between
server-webvieworigin checks and local-window identity checks is clean and security-oriented.src/logViewerWindow/ipc.ts (1)
163-163: Window getter registration is correctly wired.This ensures sender validation can resolve the active log viewer
webContentsat call time.src/videoCallWindow/ipc.ts (1)
266-266:video-callgetter registration looks correct.This provides the expected trusted
webContentsidentity source for validation.src/ipc/main/validateSender.spec.ts (1)
45-201: Great coverage for the new sender trust model.The suite exercises class-based trust, origin matching, hosted webview behavior, and mixed allowlists with useful negative cases.
src/logging/index.ts (1)
272-280: No action required—the allowlist is correct.The
['log-viewer']restriction on Line 273 correctly matches the only caller of this IPC. The log-viewer window is the only process that (1) imports the preload, (2) hasprocessContext === 'renderer:webview', and (3) triggers the IPC call. Other webviews do not import the preload and rely on theexecuteJavaScriptinjection fallback instead (see the TODO at line 347), so they never attempt this IPC route and are unaffected by the allowlist.
| ipcMain.handle('refresh-supported-versions', async (event, serverUrl) => { | ||
| if (!isTrustedSender(event.sender, ['server-webview'])) { | ||
| console.warn( | ||
| '[ipc] refresh-supported-versions: rejected untrusted sender', | ||
| event.sender.getURL() | ||
| ); | ||
| return; | ||
| } | ||
| updateSupportedVersionsData(serverUrl); |
There was a problem hiding this comment.
Bind serverUrl to the authenticated sender origin.
Right now, Line 510 only checks sender class. A trusted webview can still trigger refreshes for other servers by passing a different serverUrl. Add origin binding before calling updateSupportedVersionsData.
🔒 Proposed fix
ipcMain.handle('refresh-supported-versions', async (event, serverUrl) => {
if (!isTrustedSender(event.sender, ['server-webview'])) {
console.warn(
'[ipc] refresh-supported-versions: rejected untrusted sender',
event.sender.getURL()
);
return;
}
+
+ try {
+ const senderOrigin = new URL(event.sender.getURL()).origin;
+ const requestedOrigin = new URL(serverUrl).origin;
+ if (senderOrigin !== requestedOrigin) {
+ console.warn(
+ '[ipc] refresh-supported-versions: rejected origin mismatch',
+ senderOrigin,
+ requestedOrigin
+ );
+ return;
+ }
+ } catch {
+ return;
+ }
+
updateSupportedVersionsData(serverUrl);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ipcMain.handle('refresh-supported-versions', async (event, serverUrl) => { | |
| if (!isTrustedSender(event.sender, ['server-webview'])) { | |
| console.warn( | |
| '[ipc] refresh-supported-versions: rejected untrusted sender', | |
| event.sender.getURL() | |
| ); | |
| return; | |
| } | |
| updateSupportedVersionsData(serverUrl); | |
| ipcMain.handle('refresh-supported-versions', async (event, serverUrl) => { | |
| if (!isTrustedSender(event.sender, ['server-webview'])) { | |
| console.warn( | |
| '[ipc] refresh-supported-versions: rejected untrusted sender', | |
| event.sender.getURL() | |
| ); | |
| return; | |
| } | |
| try { | |
| const senderOrigin = new URL(event.sender.getURL()).origin; | |
| const requestedOrigin = new URL(serverUrl).origin; | |
| if (senderOrigin !== requestedOrigin) { | |
| console.warn( | |
| '[ipc] refresh-supported-versions: rejected origin mismatch', | |
| senderOrigin, | |
| requestedOrigin | |
| ); | |
| return; | |
| } | |
| } catch { | |
| return; | |
| } | |
| updateSupportedVersionsData(serverUrl); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/servers/supportedVersions/main.ts` around lines 509 - 517, The handler
for 'refresh-supported-versions' only verifies sender type but doesn't bind the
serverUrl to the sender origin; modify the ipcMain.handle callback so after
isTrustedSender succeeds you derive the origin from event.sender.getURL() (or
parse event.sender.getURL() into an origin) and either validate that the
provided serverUrl has the same origin or replace serverUrl with the sender
origin before calling updateSupportedVersionsData; update the logic around
isTrustedSender, event.sender.getURL(), and updateSupportedVersionsData to
enforce origin binding.
| if (!isTrustedSender(onceEvent.sender, ['video-call'])) { | ||
| console.warn( | ||
| '[ipc] video-call-window/screen-sharing-source-responded: rejected untrusted sender', | ||
| onceEvent.sender.getURL() | ||
| ); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Always complete the screen-sharing response path on rejection.
When Line 311 fails trust, the code returns without notifying callerWebContents. The caller-side listener can hang waiting for 'video-call-window/screen-sharing-source-responded'.
🛠️ Proposed fix
if (!isTrustedSender(onceEvent.sender, ['video-call'])) {
console.warn(
'[ipc] video-call-window/screen-sharing-source-responded: rejected untrusted sender',
onceEvent.sender.getURL()
);
+ if (!callerWebContents.isDestroyed()) {
+ callerWebContents.send(
+ 'video-call-window/screen-sharing-source-responded',
+ null
+ );
+ }
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!isTrustedSender(onceEvent.sender, ['video-call'])) { | |
| console.warn( | |
| '[ipc] video-call-window/screen-sharing-source-responded: rejected untrusted sender', | |
| onceEvent.sender.getURL() | |
| ); | |
| return; | |
| } | |
| if (!isTrustedSender(onceEvent.sender, ['video-call'])) { | |
| console.warn( | |
| '[ipc] video-call-window/screen-sharing-source-responded: rejected untrusted sender', | |
| onceEvent.sender.getURL() | |
| ); | |
| if (!callerWebContents.isDestroyed()) { | |
| callerWebContents.send( | |
| 'video-call-window/screen-sharing-source-responded', | |
| null | |
| ); | |
| } | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/videoCallWindow/ipc.ts` around lines 311 - 317, The early return when
isTrustedSender(onceEvent.sender, ['video-call']) fails leaves the caller
waiting; before returning, notify the original caller (callerWebContents) with
the 'video-call-window/screen-sharing-source-responded' channel indicating
rejection/failure and include any error reason, then safely return. Locate the
check around isTrustedSender and onceEvent.sender and ensure you call
callerWebContents.send('video-call-window/screen-sharing-source-responded', {
success: false, error: 'untrusted-sender' }) (or equivalent payload) before the
existing return so the caller-side listener is always completed.
…ons and log tag refresh-supported-versions is invoked from ServerPane (root renderer / main window), not from a server webview — change allowlist from ['server-webview'] to ['main-window']. log-viewer-window/get-server-tag is sent by logging/preload.ts only when processContext === 'renderer:webview' (server webviews hosted in the main window), not by the log-viewer window — change allowlist from ['log-viewer'] to ['server-webview']. Add test suites for both corrected allowlists (6 new cases, 20 total).
|
Codex review findings addressed (commit 954be66) Two P2 allowlist regressions identified in Codex review have been corrected: 1.
2.
6 new test cases added to |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/logging/index.ts`:
- Around line 274-277: The current rejection log prints the full sender URL via
event.sender.getURL(), which may leak sensitive query/fragment data; change the
console.warn in the 'ipc' log-viewer-window/get-server-tag rejection path to log
a normalized origin or the sender id instead: obtain the URL string from
event.sender.getURL(), parse it with the URL constructor to extract .origin
(wrapped in a try/catch to handle invalid URLs), and fall back to
event.sender.id when parsing fails; then pass that sanitized value to
console.warn rather than the raw URL.
🪄 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: 3cd0e062-ac98-4540-a774-963914450e3e
📒 Files selected for processing (3)
src/ipc/main/validateSender.spec.tssrc/logging/index.tssrc/servers/supportedVersions/main.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/servers/supportedVersions/main.ts
- src/ipc/main/validateSender.spec.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: check (ubuntu-latest)
- GitHub Check: check (windows-latest)
- GitHub Check: Analyze (javascript)
🧰 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/logging/index.ts
🔇 Additional comments (1)
src/logging/index.ts (1)
269-280: Good hardening on sync IPC sender validation.The early trust check plus immediate
event.returnValue = ''fallback is a solid safeguard for this channel and keepssendSyncbehavior safe.
| console.warn( | ||
| '[ipc] log-viewer-window/get-server-tag: rejected untrusted sender', | ||
| event.sender.getURL() | ||
| ); |
There was a problem hiding this comment.
Avoid logging full rejected sender URLs.
Logging event.sender.getURL() in this rejection path can persist sensitive query/fragment data into logs. Prefer logging a normalized origin (or just event.sender.id) instead.
Proposed fix
- console.warn(
- '[ipc] log-viewer-window/get-server-tag: rejected untrusted sender',
- event.sender.getURL()
- );
+ const rejectedSender = (() => {
+ try {
+ const senderUrl = event.sender.getURL();
+ return senderUrl ? new URL(senderUrl).origin : '<empty-url>';
+ } catch {
+ return '<invalid-url>';
+ }
+ })();
+ console.warn(
+ '[ipc] log-viewer-window/get-server-tag: rejected untrusted sender',
+ rejectedSender
+ );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/logging/index.ts` around lines 274 - 277, The current rejection log
prints the full sender URL via event.sender.getURL(), which may leak sensitive
query/fragment data; change the console.warn in the 'ipc'
log-viewer-window/get-server-tag rejection path to log a normalized origin or
the sender id instead: obtain the URL string from event.sender.getURL(), parse
it with the URL constructor to extract .origin (wrapped in a try/catch to handle
invalid URLs), and fall back to event.sender.id when parsing fails; then pass
that sanitized value to console.warn rather than the raw URL.
…lidation
registerWindowGetter('main-window', ...) was never called in production
code, causing isTrustedSender to always return false for the main window
and breaking the refresh-supported-versions IPC channel.
P1 fix: main-window getter now registered in production codeRoot cause confirmed: Fix (commit 2a2aa9b): In ```ts The getter is a closure over the module-level Other windows: both Tests added to
Verification: |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/ui/main/rootWindow.spec.ts (1)
371-377: Tighten the catch blocks to avoid masking unrelated failures.The empty
catchwill also swallow unexpected errors fromcreateRootWindow(). Assert the expected error shape/message so the test still fails on unrelated regressions.♻️ Suggested test hardening
try { rootWindowModule.createRootWindow(); - } catch { - // expected: tempWindow is undefined in this test context + } catch (error) { + expect(error).toBeInstanceOf(TypeError); + expect((error as Error).message).toMatch(/destroy|undefined/i); } @@ try { rootWindowModule.createRootWindow(); - } catch { - // expected: tempWindow is undefined in this test context + } catch (error) { + expect(error).toBeInstanceOf(TypeError); + expect((error as Error).message).toMatch(/destroy|undefined/i); }Also applies to: 391-395
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/main/rootWindow.spec.ts` around lines 371 - 377, The test currently swallows all errors from rootWindowModule.createRootWindow() via a bare catch; change the test to assert the specific expected failure instead of silencing everything: call createRootWindow inside an assertion that checks the thrown error shape/message (or use expect(() => rootWindowModule.createRootWindow()).toThrow(/tempWindow.*undefined|expected message/) ), or in the catch inspect the error (e.g., error.message or a specific error class) and rethrow if it doesn't match the expected "tempWindow is undefined" condition; target the createRootWindow invocation and the tempWindow-related error check so unrelated exceptions still fail the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/ui/main/rootWindow.spec.ts`:
- Around line 371-377: The test currently swallows all errors from
rootWindowModule.createRootWindow() via a bare catch; change the test to assert
the specific expected failure instead of silencing everything: call
createRootWindow inside an assertion that checks the thrown error shape/message
(or use expect(() =>
rootWindowModule.createRootWindow()).toThrow(/tempWindow.*undefined|expected
message/) ), or in the catch inspect the error (e.g., error.message or a
specific error class) and rethrow if it doesn't match the expected "tempWindow
is undefined" condition; target the createRootWindow invocation and the
tempWindow-related error check so unrelated exceptions still fail the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b621fd41-9972-4d40-ac79-0d21bc1eb752
📒 Files selected for processing (2)
src/ui/main/rootWindow.spec.tssrc/ui/main/rootWindow.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: check (windows-latest)
- GitHub Check: check (ubuntu-latest)
- GitHub Check: Analyze (javascript)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{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/ui/main/rootWindow.tssrc/ui/main/rootWindow.spec.ts
**/*.spec.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use *.spec.ts file naming convention for Renderer process tests
Files:
src/ui/main/rootWindow.spec.ts
**/*.{spec,main.spec}.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{spec,main.spec}.ts: Tests must run and pass on Windows, macOS, and Linux CI environments — always verify cross-platform compatibility
Only mock Linux-only APIs (process.getuid(), process.getgid(), process.geteuid(), process.getegid()) when defensive coding with optional chaining isn't possible
Files:
src/ui/main/rootWindow.spec.ts
🧠 Learnings (5)
📚 Learning: 2026-04-06T18:34:44.375Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-06T18:34:44.375Z
Learning: Applies to **/*.main.spec.ts : Use *.main.spec.ts file naming convention for Main process tests
Applied to files:
src/ui/main/rootWindow.spec.ts
📚 Learning: 2026-04-06T18:34:44.375Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-06T18:34:44.375Z
Learning: Applies to **/*.{spec,main.spec}.ts : Tests must run and pass on Windows, macOS, and Linux CI environments — always verify cross-platform compatibility
Applied to files:
src/ui/main/rootWindow.spec.ts
📚 Learning: 2026-04-06T18:34:44.375Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-06T18:34:44.375Z
Learning: Applies to **/*.spec.ts : Use *.spec.ts file naming convention for Renderer process tests
Applied to files:
src/ui/main/rootWindow.spec.ts
📚 Learning: 2026-04-06T18:34:44.375Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-06T18:34:44.375Z
Learning: Applies to **/*.{spec,main.spec}.ts : Only mock Linux-only APIs (process.getuid(), process.getgid(), process.geteuid(), process.getegid()) when defensive coding with optional chaining isn't possible
Applied to files:
src/ui/main/rootWindow.spec.ts
📚 Learning: 2026-04-06T18:34:44.375Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-06T18:34:44.375Z
Learning: Applies to **/*.{ts,tsx} : 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
Applied to files:
src/ui/main/rootWindow.spec.ts
🔇 Additional comments (3)
src/ui/main/rootWindow.ts (1)
26-26: Solid integration of main-window sender registration.Registering
'main-window'right afterBrowserWindowcreation with a null-safe getter is the right hook point for IPC sender trust checks.Also applies to: 113-114
src/ui/main/rootWindow.spec.ts (2)
41-43: Nice targeted mock for sender validation dependency.Mocking
registerWindowGetterdirectly keeps these tests focused on root window behavior and registration semantics.
365-370: Good coverage for getter registration contract.These assertions validate both registration arguments and the returned getter behavior, which is exactly what this security hook needs.
Also applies to: 397-404
…plies Replace ipcMain.once with ipcMain.on + manual removeListener so that an untrusted renderer firing the channel first does not consume the handler. With once(), a malicious sender could permanently hang the screen-picker request by removing the listener before the legitimate reply arrives. Also adds two tests asserting: untrusted sender leaves listener intact, trusted sender removes it and invokes handler exactly once.
Fix: screen-picker listener no longer consumable by untrusted rendererCommit: 786e69c Problem
FixReplaced Other
|
| File | Line | Pattern | Verdict |
|---|---|---|---|
src/ipc/main.ts:14 |
ipcMain.once(\${channel}@${id}`, ...)` |
Internal correlation channel with unique per-request suffix — not exposed to untrusted renderers | Safe, no change needed |
src/screenSharing/ScreenSharingRequestTracker.ts:135 |
ipcMain.once(this.responseChannel, listener) |
Has its own cleanup/tracker mechanism; sender validation happens inside tracker | Follow-up review recommended but out of scope for this PR |
Tests added (src/ipc/main/validateSender.spec.ts)
untrusted sender does not consume the listener— fires channel with untrusted event, asserts listener count remains 1 and no value receivedtrusted sender consumes the listener and invokes handler exactly once— fires trusted event, asserts value received and listener removed; second trusted emit is a no-op
Result: 22 tests pass, lint clean, typecheck clean.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/videoCallWindow/ipc.ts (1)
291-333:⚠️ Potential issue | 🟠 MajorValidate
callerWebContentsbefore relaying the picker result to match the authorization check inget-credentials.The
pickerResponseHandlervalidates onlyresponseEvent.sender, butcallerWebContentsbecomes the relay target without a local ownership check. Since the generichandle()wrapper performs no upstream sender validation, any renderer that can invoke this channel can receive the trusted picker result. Thevideo-call-window/get-credentialshandler already demonstrates the correct pattern—validate that the caller is either the video-call window itself or its hosted webview using:const isAuthorizedCaller = !!videoCallWindow && !videoCallWindow.isDestroyed() && (callerWebContents.id === videoCallWindow.webContents.id || callerWebContents.hostWebContents?.id === videoCallWindow.webContents.id);Apply the same check before sending the screen-sharing response on line 329.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/videoCallWindow/ipc.ts` around lines 291 - 333, The pickerResponseHandler in the handle('video-call-window/open-screen-picker') flow forwards the trusted picker result to callerWebContents without verifying the caller is authorized; mirror the get-credentials pattern by computing an isAuthorizedCaller check using videoCallWindow and callerWebContents (ensure videoCallWindow exists and is not destroyed, and that callerWebContents.id === videoCallWindow.webContents.id or callerWebContents.hostWebContents?.id === videoCallWindow.webContents.id) and only send the 'video-call-window/screen-sharing-source-responded' message when isAuthorizedCaller is true (otherwise skip sending and optionally log a warning) before removing the listener.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/videoCallWindow/ipc.ts`:
- Around line 291-333: The pickerResponseHandler in the
handle('video-call-window/open-screen-picker') flow forwards the trusted picker
result to callerWebContents without verifying the caller is authorized; mirror
the get-credentials pattern by computing an isAuthorizedCaller check using
videoCallWindow and callerWebContents (ensure videoCallWindow exists and is not
destroyed, and that callerWebContents.id === videoCallWindow.webContents.id or
callerWebContents.hostWebContents?.id === videoCallWindow.webContents.id) and
only send the 'video-call-window/screen-sharing-source-responded' message when
isAuthorizedCaller is true (otherwise skip sending and optionally log a warning)
before removing the listener.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 49501120-9ae6-4e5a-a796-3e73ca145b1f
📒 Files selected for processing (2)
src/ipc/main/validateSender.spec.tssrc/videoCallWindow/ipc.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/ipc/main/validateSender.spec.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: check (windows-latest)
🧰 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/videoCallWindow/ipc.ts
… to webContents identity Gate hostWebContents-match in isLocalWindowSender to 'video-call' only. 'main-window' and 'log-viewer' now require exact WebContents identity, preventing a server webview (which shares hostWebContents with the main window) from bypassing the allowlist on channels like refresh-supported-versions. Adds regression test: 'rejects a server webview whose hostWebContents is the main window (CORE-1129)'.
|
Codex v4 P2 finding addressed (commit 90a3b3d) The Codex v4 audit identified that `isLocalWindowSender` accepted both exact WebContents identity and `hostWebContents` match for all local window classes, including `main-window`. Since server webviews are hosted by the main window, a malicious server could call `refresh-supported-versions` with an arbitrary `serverUrl` by exploiting the `hostWebContents` path. Fix: `hostWebContents`-match is now gated on `cls === 'video-call'` only — the only class with a legitimate hosted-webview use case (Jitsi). `main-window` and `log-viewer` now require exact WebContents identity match. New regression test: "rejects a server webview whose hostWebContents is the main window (CORE-1129)" in `src/ipc/main/validateSender.spec.ts`. Test suite: 23/23 passing. Lint and typecheck clean. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/ipc/main/validateSender.spec.ts (1)
41-44: Consider trimming section-banner comments.The suite names already communicate intent; reducing divider comments would improve signal-to-noise.
As per coding guidelines, "Write self-documenting code through clear naming — avoid unnecessary comments."
Also applies to: 96-99, 125-128, 169-172, 215-218, 250-256, 334-337
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ipc/main/validateSender.spec.ts` around lines 41 - 44, Remove or trim the large divider banner comments (e.g., the "// ---------------------------------------------------------------------------" blocks that wrap titles like "server-webview") and similar banner blocks at the other locations (lines around 96-99, 125-128, 169-172, 215-218, 250-256, 334-337), keeping only concise, single-line comments or relying on the existing describe/it suite names (e.g., the validateSender.spec.ts test suite names) for context so the test file is self-documenting and less noisy.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/ipc/main/validateSender.spec.ts`:
- Around line 41-44: Remove or trim the large divider banner comments (e.g., the
"// ---------------------------------------------------------------------------"
blocks that wrap titles like "server-webview") and similar banner blocks at the
other locations (lines around 96-99, 125-128, 169-172, 215-218, 250-256,
334-337), keeping only concise, single-line comments or relying on the existing
describe/it suite names (e.g., the validateSender.spec.ts test suite names) for
context so the test file is self-documenting and less noisy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9de45762-697d-40a7-9585-6e4eb2547368
📒 Files selected for processing (2)
src/ipc/main/validateSender.spec.tssrc/ipc/validateSender.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/ipc/validateSender.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: check (windows-latest)
- GitHub Check: check (ubuntu-latest)
- GitHub Check: Analyze (javascript)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{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/ipc/main/validateSender.spec.ts
**/*.spec.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use *.spec.ts file naming convention for Renderer process tests
Files:
src/ipc/main/validateSender.spec.ts
**/*.{spec,main.spec}.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{spec,main.spec}.ts: Tests must run and pass on Windows, macOS, and Linux CI environments — always verify cross-platform compatibility
Only mock Linux-only APIs (process.getuid(), process.getgid(), process.geteuid(), process.getegid()) when defensive coding with optional chaining isn't possible
Files:
src/ipc/main/validateSender.spec.ts
🔇 Additional comments (4)
src/ipc/main/validateSender.spec.ts (4)
17-34: SolidWebContentstest-double helper.
makeWebContentskeeps test setup compact while still covering the fields needed by trust checks.
45-94: Good coverage forserver-webviewtrust boundaries.These cases exercise positive/negative origin matching, type checks, empty server state, and invalid sender URL handling.
100-213: Strong local-window trust regression coverage.The suites correctly lock
main-window/log-viewerto identity-only matching and keep hosted-webview matching scoped tovideo-call.
257-332: Nice robustness test for listener consumption semantics.The
ipcMain.on+ manualremoveListenersimulation clearly validates that untrusted events do not consume the handler.
1 participant
Summary
src/ipc/validateSender.tswithisTrustedSender(sender, allow)helper andregisterWindowGetterfor local windows. Server-webview channels validate sender origin againststate.serversin Redux; local-window channels (log-viewer,video-call) validate by WebContents instance identity via registered getters, covering both the window's own renderer and webviews it hosts.console.warnand an early return / null returnValue.log-viewer-window/get-server-tagnow derives the server origin fromevent.sender.getURL()and ignores the renderer-suppliedoriginparameter entirely.Channel allowlist table
refresh-supported-versionssrc/servers/supportedVersions/main.tsserver-webviewvideo-call-window/get-provider-syncsrc/videoCallWindow/ipc.tsvideo-callvideo-call-window/screen-sharing-source-respondedsrc/videoCallWindow/ipc.tsvideo-calllog-viewer-window/get-server-tagsrc/logging/index.tslog-viewerHow the wrapper enforces defaults
The generic
handlewrapper insrc/ipc/main.tsis not modified in this PR — only the four directly-audited call sites are hardened. The wrapper default (server-webviewfor all wrapped channels) is out of scope per the brief: mass-migrating all 143+ channels is a separate tracked item.What is NOT in scope
secureKeyboardEntry(separate PR)nodeIntegration/contextIsolationtogglesTest plan
yarn lint— passes cleannpx tsc --noEmit— no errorsyarn test— 227 tests, 18 suites, all passsrc/ipc/main/validateSender.spec.ts— 14 new tests covering all SenderClass variants (positive + negative), hosted-webview case, multi-class allow list, null getter, and invalid URLSummary by CodeRabbit
Security Improvements
Bug Fixes
Tests