browser: route existing-session user profile through browser nodes#68891
browser: route existing-session user profile through browser nodes#68891
Conversation
Greptile SummaryThis PR fixes a bug where the browser tool unconditionally hard-blocked Confidence Score: 5/5Safe to merge — the change is narrowly scoped, the fallback-to-host logic is correct, sandbox rejection is preserved, and focused tests cover all three relevant routing branches. All findings are P2 or lower. The routing logic is correct, the sandbox guard and explicit host-pin are preserved, and the new tests close the coverage gap that originally allowed this bug to exist. No files require special attention. Reviews (1): Last reviewed commit: "browser: route user profile through brow..." | Re-trigger Greptile |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 06b641efb1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d6ada08a5c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
Follow-up on the remaining security-bot concern: I do not think it blocks this PR. This patch is aligning
The sandbox-policy point is broader than this PR: today |
🔒 Aisle Security AnalysisWe found 1 potential security issue(s) in this PR:
1. 🟠 Automatic routing of existing-session (profile=user) to browser nodes may expose node-local browser data without explicit opt-in
DescriptionThe browser tool now allows This introduces a security risk in deployments where:
Because existing-session attachment uses Chrome DevTools MCP and may be configured with Key issue:
Vulnerable behavior (new/changed logic): nodeTarget = await resolveBrowserNodeTarget({ requestedNode, target, sandboxBridgeUrl });
...
const proxyRequest = nodeTarget ? async (...) => callBrowserProxy({ nodeId: nodeTarget.nodeId, ... }) : null;This can unexpectedly move an existing-session attachment from the local host to a node, expanding the attack surface to the node's filesystem/profile context. RecommendationRequire explicit opt-in before routing Options:
Example (option 1): if (isUserBrowserProfile && !requestedNode && target !== "node") {
target = "host";
}This ensures node-hosted existing-session attachments cannot occur implicitly and prevents accidental exposure of node-local browser profiles. Analyzed PR: #68891 at commit Last updated on: 2026-04-19T10:20:55Z |
9def88f to
23fa1d1
Compare
* test: stabilize standalone Parallels smoke lanes * fix: strip orphaned OpenAI reasoning blocks before responses API call (openclaw#55787) Merged via squash. Prepared head SHA: 263b952 Co-authored-by: suboss87 <11032439+suboss87@users.noreply.github.com> Co-authored-by: jalehman <550978+jalehman@users.noreply.github.com> Reviewed-by: @jalehman * fix: stabilize release smoke reruns * test: tolerate empty fireworks live responses * test: make OpenWebUI smoke deterministic * tasks: extract detached task lifecycle runtime (openclaw#68886) * tasks: extract detached task lifecycle runtime * tests: relax gateway seam expectation --------- Co-authored-by: Mariano Belinky <mariano@mb-server-643.local> * test: complete workspace setup in update smokes * fix: parse PowerShell cron tools allow-list (openclaw#68858) (thanks @chen-zhang-cs-code) * fix(cron): parse PowerShell tools allow list * fix(cron): clarify tools allow-list help * fix: parse PowerShell cron tools allow-list (openclaw#68858) (thanks @chen-zhang-cs-code) --------- Co-authored-by: Ayaan Zaidi <hi@obviy.us> * fix(browser): discover CDP websocket from bare ws:// URL before attach (openclaw#68715) * fix(browser): discover CDP websocket from bare ws:// URL before attach When browser.cdpUrl is set to a bare ws://host:port (no /devtools/ path), ensureBrowserAvailable would call isChromeReachable -> canOpenWebSocket against the URL verbatim. Chrome only accepts WebSocket upgrades at the specific path returned by /json/version, so the handshake failed immediately with HTTP 400. With attachOnly: true, that surfaced as: Browser attachOnly is enabled and profile "openclaw" is not running. even though the CDP endpoint was reachable and the profile was healthy. Reproduced by the new tests in chrome.test.ts and cdp.test.ts (openclaw#68027). Fix: introduce isDirectCdpWebSocketEndpoint(url) — true only when a ws/wss URL has a /devtools/<kind>/<id> handshake path. Route any other ws/wss cdpUrl (including the bare ws://host:port shape) through HTTP /json/version discovery by normalising the scheme via the existing normalizeCdpHttpBaseForJsonEndpoints helper. Apply this in isChromeReachable, getChromeWebSocketUrl, and createTargetViaCdp. Direct WS endpoints with a /devtools/ path are still opened without an extra discovery round-trip. Fixes openclaw#68027 * test(browser): add seeded fuzz coverage for CDP URL helpers Adds property-based / seeded-fuzz tests for the URL helpers the attachOnly CDP fix depends on (openclaw#68027): - isWebSocketUrl - isDirectCdpWebSocketEndpoint - normalizeCdpHttpBaseForJsonEndpoints - parseBrowserHttpUrl - redactCdpUrl - appendCdpPath - getHeadersWithAuth Follows the existing repo convention (see src/gateway/http-common.fuzz.test.ts): no fast-check dep, small mulberry32 PRNG + hand-rolled generators, deterministic per-describe seeds so failures are reproducible. Lifts cdp.helpers.ts coverage from 77.77% -> 89.54% statements, 67.9% -> 80.24% branches, 78% -> 90% lines. Remaining uncovered lines are inside the WS sender internals (createCdpSender, withCdpSocket, fetchCdpChecked rate-limit branch), which require integration-style mocks and are unrelated to the attachOnly fix. * test(browser): drive cdp.helpers/cdp/chrome to 100% coverage Lifts the three files touched by the openclaw#68027 attachOnly fix to 100% statements/branches/functions/lines across the extensions test suite. Adds cdp.helpers.internal.test.ts, cdp.internal.test.ts, and chrome.internal.test.ts covering error paths, branch matrices, CDP session helpers, Chrome spawn/launch/stop flows, and canRunCdpHealthCommand. Defensively unreachable guards are annotated with c8 ignore + inline justifications. * fix(browser): restore WS fallback for non-/devtools ws:// CDP URLs When /json/version discovery is unavailable (or returns no webSocketDebuggerUrl), fall back to treating the original bare ws/wss URL as a direct WebSocket endpoint. This preserves the openclaw#68027 fix for Chrome's debug port while restoring compatibility with Browserless/ Browserbase-style providers that expose a direct WebSocket root without a /json/version endpoint. Priority order for bare ws/wss cdpUrl inputs: 1. /devtools/<kind>/<id> URL \u2192 direct handshake, no discovery (unchanged) 2. bare ws/wss root \u2192 try HTTP discovery first; if discovery returns a webSocketDebuggerUrl use it; otherwise fall back to the original URL as a direct WS endpoint 3. HTTP/HTTPS URL \u2192 HTTP discovery only, no fallback (unchanged) Affected call sites: isChromeReachable, getChromeWebSocketUrl, createTargetViaCdp. Also renames a misleading test ('still enforces SSRF policy for direct WebSocket URLs') to accurately describe what it tests: SSRF enforcement on the navigation target URL, not on the CDP endpoint. New tests added for all three fallback paths. Coverage remains 100% on all three touched files (238 tests). * fix: browser attachOnly bare ws CDP follow-ups (openclaw#68715) (thanks @visionik) * test(tasks): align detached runtime mock return types * browser: route existing-session user profile through browser nodes (openclaw#68891) * browser: route user profile through browser nodes * browser: align existing-session node docs * browser: preserve host fallback on node discovery errors * browser: preserve configured node pin errors * browser: widen config mock in node pin test * fix: default kimi thinking to off (openclaw#68907) Co-authored-by: termtek <termtek@ubuntu.tail2b72cd.ts.net> * docs(changelog): note kimi thinking default fix * docs(changelog): add thanks for kimi fix * tasks: add detached runtime plugin registration contract (openclaw#68915) * tasks: register detached runtime plugins * tasks: harden detached runtime ownership * tasks: extract detached runtime contract types * changelog: note detached runtime contract * changelog: attribute detached runtime contract * feat(ui): add a2a operator dashboard shell * fix(ui): align a2a baseline with current read contract * feat(ui): add A2A case inspector panels * CI: route small workflows to ubuntu-latest * CI: route fork-blocked workflows to GitHub-hosted runners * CI: fall back to GITHUB_TOKEN for fork automation * CI: skip app-token steps when fork secrets are absent * CI: gate fork app-token steps through env guards * CI: fix stale fallback env guard * fix(ui): sync A2A locale snapshots * fix(ui): sync A2A locale metadata --------- Co-authored-by: Peter Steinberger <steipete@gmail.com> Co-authored-by: Subash Natarajan <suboss87@gmail.com> Co-authored-by: suboss87 <11032439+suboss87@users.noreply.github.com> Co-authored-by: jalehman <550978+jalehman@users.noreply.github.com> Co-authored-by: Mariano <132747814+mbelinky@users.noreply.github.com> Co-authored-by: Mariano Belinky <mariano@mb-server-643.local> Co-authored-by: ZC <chenzhangcode@163.com> Co-authored-by: Ayaan Zaidi <hi@obviy.us> Co-authored-by: Viz <visionik@pobox.com> Co-authored-by: Frank Yang <frank.ekn@gmail.com> Co-authored-by: termtek <termtek@ubuntu.tail2b72cd.ts.net> Co-authored-by: bangtong-ai <bangtong-ai@users.noreply.github.com> Co-authored-by: OpenClaw Agent <openclaw-agent@users.noreply.github.com> Co-authored-by: OpenClaw Bot <bot@openclaw.local>
…penclaw#68891) * browser: route user profile through browser nodes * browser: align existing-session node docs * browser: preserve host fallback on node discovery errors * browser: preserve configured node pin errors * browser: widen config mock in node pin test
…penclaw#68891) * browser: route user profile through browser nodes * browser: align existing-session node docs * browser: preserve host fallback on node discovery errors * browser: preserve configured node pin errors * browser: widen config mock in node pin test
…penclaw#68891) * browser: route user profile through browser nodes * browser: align existing-session node docs * browser: preserve host fallback on node discovery errors * browser: preserve configured node pin errors * browser: widen config mock in node pin test
…penclaw#68891) * browser: route user profile through browser nodes * browser: align existing-session node docs * browser: preserve host fallback on node discovery errors * browser: preserve configured node pin errors * browser: widen config mock in node pin test
Summary
profile="user"/existing-sessionwhen targeting a browser node, even though the docs describe node-host browser proxy as the default remote-gateway path.openclawbrowser, but they could not reach the real signed-in Chrome session on the browser machine.existing-sessionnode hard-block, kept sandbox rejection, preserved explicittarget="host"pinning, and added focused regression coverage for omitted target, explicittarget="node", and explicittarget="host".target="host"semantics, and does not touch Chrome MCP session management beyond allowing the browser-tool routing layer to use the existing node proxy path.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
Root Cause (if applicable)
extensions/browser/src/browser-tool.tstreated Chrome MCPexisting-sessionprofiles as host-local only and threw before node routing could run.profile="user"with node routing semantics.Regression Test Plan (if applicable)
extensions/browser/src/browser-tool.test.tsprofile="user"should auto-route through a connected browser node when target is omitted, should work with explicittarget="node", and should still stay on host whentarget="host"is explicit.User-visible / Behavior Changes
profile="user"through a connected browser node instead of being hard-blocked.target="host"still pins the host and prevents node auto-routing.Diagram (if applicable)
Security Impact (required)
No)No)No)No)No)Yes, explain risk + mitigation:Repro + Verification
Environment
pnpmbrowser.profiles.user.driver = "existing-session"Steps
profile="user"and either no target ortarget="node".Expected
profile="user"can route through the connected browser node.target="host"still stays on host.Actual
Evidence
Human Verification (required)
What you personally verified (not just CI), and how:
target="node"both route through the node proxy; explicittarget="host"remains host-pinned.profile="user"still rejectstarget="sandbox"and still falls back to host when no browser node is available.Review Conversations
Compatibility / Migration
Yes)No)No)Risks and Mitigations
target="host"pinning still preserves the old host-only intent when needed.