MM-69421 - Scope server view network requests by origin and handle subframe navigation - #3872
MM-69421 - Scope server view network requests by origin and handle subframe navigation#3872pvev wants to merge 6 commits into
Conversation
…bframe navigation
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR adds main-process local-network request filtering, subframe navigation allowlists, and end-to-end coverage using loopback servers and Electron policy tests. ChangesLocal network access policy
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Renderer
participant ElectronSession
participant NetworkPolicy
participant LocalServer
Renderer->>ElectronSession: fetch, frame, redirect, or service-worker request
ElectronSession->>NetworkPolicy: evaluate URL and webContents context
NetworkPolicy->>LocalServer: resolve or allow configured-origin request
LocalServer-->>NetworkPolicy: response or resolved address
NetworkPolicy-->>ElectronSession: cancel or allow
ElectronSession-->>Renderer: blocked request or response
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Documentation Impact Analysis — updates neededDocumentation Impact AnalysisOverall Assessment: Documentation Updates Recommended Changes SummaryPR #3872 adds a session-level Documentation Impact Details
Recommended Actions
ConfidenceMedium — The PR carries a |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/app/views/webContentEvents.test.js (1)
133-198: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd one regression case for the popup-specific subframe listener.
This suite only exercises
addWebContentsEventListeners(), butsrc/app/views/webContentEvents.tsalso wireswill-frame-navigatedirectly on popupwebContentsin the plugin/managed-resource branch. A small test for that branch would keep this security-sensitive path from regressing unnoticed.🤖 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 `@src/app/views/webContentEvents.test.js` around lines 133 - 198, Add a regression test for the popup-specific `will-frame-navigate` listener path, since `addWebContentsEventListeners()` is not the only place this guard is attached. Extend the `WebContentsEventManager` coverage to exercise the popup/plugin/managed-resource branch in `webContentEvents.ts`, verify the listener is registered on popup `webContents`, and assert the same main-frame bypass and protocol-blocking behavior so this security-sensitive path stays covered.src/main/security/localNetworkAccess.ts (1)
88-104: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCoalesce DNS checks on the request hot path.
This helper is awaited from the session
onBeforeRequesthandler, so every repeated request to the same public hostname currently pays for a freshdns.lookup()before Chromium can continue. That is an avoidable latency hit for CDN/embed-heavy pages. Cache or at least dedupe in-flight lookups by normalized hostname here.🤖 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 `@src/main/security/localNetworkAccess.ts` around lines 88 - 104, The isLocalOrPrivateHostname helper currently performs a fresh dns.lookup through lookup for every repeated public hostname on the onBeforeRequest hot path, adding avoidable latency. Add caching or in-flight deduplication keyed by the normalized hostname inside isLocalOrPrivateHostname so repeated calls reuse the same result. Keep the existing local/private fast-path checks in normalizeHostname, isLocalhostHostname, and isLocalOrPrivateIPAddress unchanged, and ensure lookup failures still fall back to false.
🤖 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 `@e2e/helpers/localNetworkServers.ts`:
- Around line 52-88: startLocalNetworkServers leaves secretService running if
listen(fakeServer) fails after the first server starts, which can leak a bound
loopback listener into later tests. Update startLocalNetworkServers to wrap the
startup sequence in try/catch and ensure any successfully started server is
closed on failure, reusing the existing close/cleanup pattern from
LocalNetworkServers. Make sure both secretService and fakeServer are tracked so
partial startup is always cleaned up before rethrowing.
In `@src/app/views/webContentEvents.ts`:
- Around line 38-41: The import block in webContentEvents should follow the
repo’s singleton and ordering conventions: rename the allowProtocolDialog import
to PascalCase and keep the relative imports grouped and alphabetized according
to the enforced ESLint order. Update the import section in webContentEvents to
use the correct symbol name for the singleton and preserve the existing grouping
around generateHandleConsoleMessage, isAllowedSubframeNavigation,
isCustomProtocol, isMattermostProtocol, composeUserAgent, and
allowProtocolDialog.
In `@src/main/app/initialize.test.js`:
- Around line 333-340: The helper in initialize.test.js is reading the first
onBeforeRequest registration instead of the one created by its own initialize()
call. Update getRegisteredHandler to return the most recently registered handler
from session.defaultSession.webRequest.onBeforeRequest.mock.calls so it always
uses the handler added by that helper, even when initialize() was called earlier
in the file.
In `@src/main/app/initialize.ts`:
- Around line 361-366: The warning in initialize.ts is logging the full
attacker-controlled URL via the shouldCancel block, which can leak query strings
and intranet paths. Update the log payload in the blocked-request path to redact
details.url before it is emitted, keeping only the origin/host or another
scrubbed form while preserving the other fields like details.resourceType and
details.webContentsId.
---
Nitpick comments:
In `@src/app/views/webContentEvents.test.js`:
- Around line 133-198: Add a regression test for the popup-specific
`will-frame-navigate` listener path, since `addWebContentsEventListeners()` is
not the only place this guard is attached. Extend the `WebContentsEventManager`
coverage to exercise the popup/plugin/managed-resource branch in
`webContentEvents.ts`, verify the listener is registered on popup `webContents`,
and assert the same main-frame bypass and protocol-blocking behavior so this
security-sensitive path stays covered.
In `@src/main/security/localNetworkAccess.ts`:
- Around line 88-104: The isLocalOrPrivateHostname helper currently performs a
fresh dns.lookup through lookup for every repeated public hostname on the
onBeforeRequest hot path, adding avoidable latency. Add caching or in-flight
deduplication keyed by the normalized hostname inside isLocalOrPrivateHostname
so repeated calls reuse the same result. Keep the existing local/private
fast-path checks in normalizeHostname, isLocalhostHostname, and
isLocalOrPrivateIPAddress unchanged, and ensure lookup failures still fall back
to false.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d9712224-8096-4610-aec8-25fed4e326cd
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (11)
e2e/helpers/config.tse2e/helpers/localNetworkServers.tse2e/specs/policy/local_network.test.tssrc/app/views/webContentEvents.test.jssrc/app/views/webContentEvents.tssrc/app/views/webContentEventsCommon.test.tssrc/app/views/webContentEventsCommon.tssrc/main/app/initialize.test.jssrc/main/app/initialize.tssrc/main/security/localNetworkAccess.test.tssrc/main/security/localNetworkAccess.ts
pvev
added
2: Dev Review
edgarbellot
left a comment
edgarbellot
left a comment
There was a problem hiding this comment.
LGTM, happy path + edge cases perfectly covered 🚀 thank you!
devinbinnie
left a comment
devinbinnie
left a comment
There was a problem hiding this comment.
Thanks for the work here @pvev, I think I have a few worthwhile improvements we can make to make this more correct. We'll probably want @edgarbellot to re-review afterwards.
| webContentsId: details.webContentsId, | ||
| webContents: details.webContents, | ||
| }, | ||
| ServerManager.getAllServers().map((server) => server.url), |
There was a problem hiding this comment.
The method that we're calling can pull these values in themselves, we don't need to pass them in from here.
There was a problem hiding this comment.
Updated this so the request details are passed through directly and the helper owns the extraction logic.
|
|
||
| const log = new Logger('WebContentsEventManager'); | ||
|
|
||
| type NavigationEvent = Event<WebContentsWillNavigateEventParams | WebContentsWillFrameNavigateEventParams>; |
There was a problem hiding this comment.
I don't think we need this type? The events are different and are shaped differently. Since we're creating different functions for each event type, we don't need to combine the types here.
There was a problem hiding this comment.
Agreed, removed the combined type and typed the handlers independently.
| return false; | ||
| } | ||
|
|
||
| function isLocalOrPrivateIPv4(address: string): boolean { |
There was a problem hiding this comment.
The implementations for checking specific IPv4 and IPv6 addresses here and below feel very janky and one thing I think we should be avoiding is bitwise operations in JS.
I checked with Claude and found a potential alternative that seems cleaner even if it's not perfect, it uses Node's net.BlockList object that allows us to specify CIDRs we want to block on and check those. Here's an example:
import {BlockList, isIP} from 'net';
// Local, loopback, and private ranges we refuse to let server content reach.
// Declarative CIDR data; matching is handled natively by BlockList.
const LOCAL_NETWORK_BLOCKLIST = new BlockList();
// IPv4
LOCAL_NETWORK_BLOCKLIST.addSubnet('0.0.0.0', 8, 'ipv4'); // unspecified / "this network"
LOCAL_NETWORK_BLOCKLIST.addSubnet('10.0.0.0', 8, 'ipv4'); // RFC 1918
LOCAL_NETWORK_BLOCKLIST.addSubnet('127.0.0.0', 8, 'ipv4'); // loopback
LOCAL_NETWORK_BLOCKLIST.addSubnet('169.254.0.0', 16, 'ipv4'); // link-local
LOCAL_NETWORK_BLOCKLIST.addSubnet('172.16.0.0', 12, 'ipv4'); // RFC 1918
LOCAL_NETWORK_BLOCKLIST.addSubnet('192.168.0.0', 16, 'ipv4'); // RFC 1918
// IPv6
LOCAL_NETWORK_BLOCKLIST.addSubnet('::1', 128, 'ipv6'); // loopback
LOCAL_NETWORK_BLOCKLIST.addSubnet('fc00::', 7, 'ipv6'); // unique local
LOCAL_NETWORK_BLOCKLIST.addSubnet('fe80::', 10, 'ipv6'); // link-local
export function isLocalOrPrivateIPAddress(address: string): boolean {
const family = isIP(address);
if (family === 4) {
return LOCAL_NETWORK_BLOCKLIST.check(address, 'ipv4');
}
if (family === 6) {
// Passing 'ipv6' lets BlockList match IPv4-mapped addresses
// (::ffff:127.0.0.1) against the IPv4 rules above automatically.
return LOCAL_NETWORK_BLOCKLIST.check(address, 'ipv6');
}
return false;
}
We should confirm with Prod Sec, but I think this would be a cleaner way to do this. Avoiding rolling our own IP detection here would be preferred.
There was a problem hiding this comment.
Agreed, replaced the custom IP range parsing with Node’s BlockList and kept the existing coverage for the expected ranges.
devinbinnie
added
the
CherryPick/Approved
|
FYI this needs to be cherry-picked to v6.3 as well. |
…nts handling while preserving request policy coverage
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/main/security/localNetworkAccess.ts`:
- Around line 32-34: Update the IPv6 initialization in LOCAL_NETWORK_BLOCKLIST
to add the omitted fec0::/10 site-local subnet, and add a regression assertion
verifying addresses in that range are blocked.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: db9cc64c-cf30-457c-86af-678122b224c5
📒 Files selected for processing (6)
e2e/specs/policy/local_network.test.tssrc/app/views/webContentEvents.tssrc/main/app/initialize.test.jssrc/main/app/initialize.tssrc/main/security/localNetworkAccess.test.tssrc/main/security/localNetworkAccess.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/main/app/initialize.test.js
- e2e/specs/policy/local_network.test.ts
- src/main/app/initialize.ts
- src/app/views/webContentEvents.ts
@DryRunSecurity nit drs_026233b0 This is a valid known limitation of doing hostname classification before Chromium performs the actual request resolution. It is not introduced by the |
devinbinnie
left a comment
devinbinnie
left a comment
There was a problem hiding this comment.
Thanks @pvev, huge improvement. I have two more concerns then we should be good to go.
| try { | ||
| const shouldCancel = await shouldCancelLocalNetworkRequest( | ||
| details, | ||
| ServerManager.getAllServers().map((server) => server.url), |
There was a problem hiding this comment.
Sorry, what I was saying before is that ServerManager.getAllServers().map((server) => server.url) can be called from shouldCancelLocalNetworkRequest. Actually, same with the function that gets the view by webcontentsid.
This call should just pass in the details (which I believe contains the webcontentsid as well)
|
|
||
| const defaultLookup: LookupFunction = (hostname: string) => dns.lookup(hostname, {all: true, verbatim: true}); | ||
|
|
||
| const LOCAL_NETWORK_BLOCKLIST = new BlockList(); |
There was a problem hiding this comment.
Nit: maybe a quick comment here explaining what this list is (ie. local IP addresses)
amyblais
added
the
3: Security Review
Claude
|
Removing myself from the reviewers list since I still see there are pending changes. Please add me again after Devin has approved everything and I'll re-review :) |
…hecks and document blocked address ranges
|
This pull request introduces a low-severity DNS rebinding vulnerability in the
DNS Rebinding in
|
| Vulnerability | DNS Rebinding |
|---|---|
| Description | The shouldCancelLocalNetworkRequest function performs a DNS lookup to check if a hostname resolves to a private IP address. This creates a Time-of-Check to Time-of-Use (TOCTOU) vulnerability because the DNS resolution performed in the check is independent of the actual network request made by the Electron network stack. An attacker could potentially use DNS rebinding to resolve the hostname to a public IP during the check and to a private IP during the actual request, bypassing the security filter. |
desktop/src/main/security/localNetworkAccess.ts
Lines 101 to 104 in 1ec92b1
Comment to provide feedback on these findings.
Report false positive: @dryrunsecurity fp [FINDING ID] [FEEDBACK]
Report low-impact: @dryrunsecurity nit [FINDING ID] [FEEDBACK]
Example: @dryrunsecurity fp drs_90eda195 This code is not user-facing
All finding details can be found in the DryRun Security Dashboard.
|
@DryRunSecurity nit drs_508aff52 . This is a partially valid low severity residual limitation of the pre-request DNS check, rather than a new vulnerability introduced by the PR. Fully addressing it requires connection level enforcement or Chromium’s native Local Network Access controls. I suggest accepting this as a residual very low risk for this fix and perhaps tracking native enforcement separately, this to avoid introducing a high regression custom networking layer completely out of scope of this change. |
|
Finding 508aff52 has been successfully dismissed and marked as Won't Fix / Nitpick. |
5 participants
Summary
Adds a session-level request filter that scopes network requests from loaded server views to their configured origins. It also extends navigation handling to cover subframes in addition to main frame navigation.
High-Level Architecture
The implementation uses two independent enforcement layers:
A session-level
onBeforeRequestfilter that:Separate navigation handlers for:
This ensures embedded content follows the same navigation policy as the top-level frame.
Ticket Link
https://mattermost.atlassian.net/browse/MM-69241
Checklist
npm run lint:jsfor proper code formattingE2E/RunDevice Information
Apple M4 MAX 64 GB Tahoe
Release Note
Change Impact: Medium 🟡
Regression Risk: Medium likelihood of security/policy regressions because the change introduces a session-wide
webRequest.onBeforeRequestcancellation filter and extends navigation gating towill-frame-navigate, impacting HTTP/HTTPS, WebSocket, redirects, and service-worker/worker-originated requests; coverage includes both unit tests and a dedicated local-network E2E spec, but behavior could still differ across Electron view ownership/webContents contexts.QA Recommendation: Limited manual QA recommended: verify in an Electron run that (1) configured-origin requests still load, (2) local/private targets are blocked (including subframe embeds, redirects, and service-worker fetch behavior), and (3) subframe navigation handling matches expectations for
about:blank/about:srcdocwhile disallowed protocols are prevented.Generated by CodeRabbitAI