Browser: enforce network policy via session isolation - #333840
Browser: enforce network policy via session isolation#333840Kyle Cutler (kycutler) wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 2
New issues introduced by this change (2)
| Severity | Finding |
|---|---|
 |
src/vs/platform/browserView/electron-main/browserSession.ts — This subscription strongly retains every Agent-scoped BrowserSession: the singleton filter… |
|
src/vs/workbench/contrib/chat/browser/attachments/chatImplicitContext.ts — This guard prevents the lazy-resolution path from ever being observed: when an active browser input… |
What changed in this PR
Enforces agent network policy in the integrated browser through isolated Agent-scoped sessions and updated sharing controls.
Changes:
- Filters network requests from Agent browser sessions.
- Restricts sharing to policy-compatible sessions and updates related UI/context.
- Adds policy, unit, and extension API coverage.
| File | Description |
|---|---|
fetchPageTool.test.ts |
Updates network-filter mock. |
chatAttachmentResolveService.test.ts |
Tests isolated sharing and blocked pages. |
chat.shared.contribution.ts |
Makes network settings application-scoped. |
chatImplicitContext.ts |
Refreshes browser implicit context. |
chatAttachmentWidgets.ts |
Displays new sharing states. |
chatAttachmentResolveService.ts |
Resolves shareable browser copies. |
browserToolHelpers.test.ts |
Tests network-policy context. |
browserEditorInput.test.ts |
Tests sharing availability. |
screenshotBrowserTool.ts |
Restricts screenshots to shared pages. |
openBrowserTool.ts |
Supports isolated shared copies. |
browserToolHelpers.ts |
Reports blocked pages and active policy. |
browserEditorErrorFeatures.ts |
Explains policy-blocked requests. |
browserEditorChatFeatures.ts |
Updates sharing controls and errors. |
browserDataStorageFeatures.ts |
Documents Agent-session filtering. |
browserViewWorkbenchService.ts |
Reports revoked browser access. |
workbench/.../browserView.ts |
Implements expanded sharing state. |
sessionBrowsersControl.test.ts |
Updates default sharing state. |
webPageLoader.test.ts |
Updates network-filter mock. |
networkFilterService.ts |
Exposes filter enablement. |
browserView.test.ts |
Tests shareable storage scopes. |
browserViewMainService.ts |
Enforces session isolation in main. |
browserViewGroup.ts |
Validates agent-visible contexts. |
browserSession.ts |
Filters Agent-session requests. |
platform/.../browserView.ts |
Adds storage-scope policy helper. |
browser.tools.test.ts |
Tests filtering and storage isolation. |
policyData.jsonc |
Updates exported policy descriptions. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| return; | ||
| } | ||
| this._networkFilterEnabled = enabled; | ||
| this.electronSession.webRequest.onBeforeRequest(enabled ? (details, callback) => { |
There was a problem hiding this comment.
AI Review: This hook only intercepts Chromium traffic from the Electron session, but run_playwright_code still exposes process-side Playwright request contexts and host-realm values. Those requests use Node HTTP(S), never traverse this callback, and can reach denied domains despite the managed policy. Put the Playwright API behind a revocable realm-safe membrane that blocks request/browser/CDP capabilities and clones values across the VM boundary, or enforce the same policy on every process-side request path.
| return BrowserViewSharingState.Unavailable; | ||
| } | ||
| return this._sharedWithAgent ? BrowserViewSharingState.Shared : BrowserViewSharingState.NotShared; | ||
| if (this._url && !this.agentNetworkFilterService.isUriAllowed(URI.parse(this._url))) { |
There was a problem hiding this comment.
AI Review: This revalidates only the top-level URL. If an Agent-session page loaded a child frame before that frame's domain became denied, request interception affects only future loads, so the existing frame remains available through summaries, Playwright actions, and screenshots. Revalidate all live frame URLs before and after agent-visible output, or revoke/reload affected views when policy changes.
| callback({ cancel: true }); | ||
| return; | ||
| } | ||
| callback({ cancel: !this.agentNetworkFilterService.isUriAllowed(uri) }); |
There was a problem hiding this comment.
AI Review: Canceling a denied subresource is surfaced by Playwright as requestfailed, while the current summary path records request.url() verbatim and checks only the allowed top-level page URL. A blocked image, iframe, or fetch can therefore disclose its denied host, path, and query through browser summaries. Drop or redact policy-denied failure events and add a hidden-subresource regression test.
| // and also doesn't handle zooming well. | ||
| const browserViewModel = await this.browserViewWorkbenchService.getKnownBrowserViews().get(params.pageId)?.resolve(); | ||
| if (!browserViewModel) { | ||
| if (!browserViewModel || browserViewModel.sharingState !== BrowserViewSharingState.Shared) { |
There was a problem hiding this comment.
AI Review: This samples renderer sharing state only before asynchronous selector lookup and native capture. If the user unshares the page or a policy update revokes access while either operation is pending, capture still resolves the view by ID and returns the bytes without revalidation. Bind capture to authoritative agent access, or recheck after selector lookup and after capture and discard the image on revocation.
| if (isBrowserViewStorageScopeShareableWithAgent(view.session.storageScope, true)) { | ||
| continue; | ||
| } | ||
| view.setAudience({ type: 'agent' }, false); |
There was a problem hiding this comment.
AI Review: Removing the audience detaches CDP access but leaves user-owned WebContents alive in persistent sessions that never install the Agent request hook. Page-realm work scheduled before revocation, such as delayed fetches or navigations, can therefore run afterward against newly denied hosts. Retain filtering for previously agent-visible WebContents until destruction, or destroy/recreate them when policy activates.
| this.editor.browserContainer.classList.toggle('shared', isShared); | ||
|
|
||
| this._shareButtonContainer.style.display = isUnavailable ? 'none' : ''; | ||
| this._shareButton.enabled = !isBlockedByNetworkPolicy; |
There was a problem hiding this comment.
AI Review: When a shared Agent-storage tab becomes policy-blocked, sharingState hides the retained agent audience, and this disables the only Stop Sharing control. If navigation or policy later allows the URL, the retained audience restores agent access without another user action. Track policy blocking separately from the sharing grant so retained sharing stays visible and revocable, or clear the audience when blocking.
| if ( | ||
| e.affectsConfiguration(AgentNetworkDomainSettingId.NetworkFilter) | ||
| && this.configurationService.getValue<boolean>(AgentNetworkDomainSettingId.NetworkFilter) | ||
| && [...this._known.values()].some(input => input.model?.sharingState === BrowserViewSharingState.Shared && !input.model.isDirectlyShareable) |
There was a problem hiding this comment.
AI Review: This configuration listener can run before the delayed network-filter service refreshes its cached enabled state, so isDirectlyShareable still reports the old value and the affected-tabs notice is skipped even though main-process revocation proceeds. Derive this check from the new setting and storage scope directly, or react after the filter service emits its updated state.
No description provided.