Skip to content

fix(browser.proxy): only advertise capability when shared gateway token is available#602

Open
QQSHI13 wants to merge 3 commits into
openclaw:mainfrom
QQSHI13:master
Open

fix(browser.proxy): only advertise capability when shared gateway token is available#602
QQSHI13 wants to merge 3 commits into
openclaw:mainfrom
QQSHI13:master

Conversation

@QQSHI13
Copy link
Copy Markdown

@QQSHI13 QQSHI13 commented May 30, 2026

Problem

The Windows Node advertises browser.proxy capability even when it only has a node device token (from pairing/bootstrap). The BrowserProxyCapability forwards browser commands to the browser control host at gatewayPort + 2 and uses this token for Bearer auth.

The browser control host expects the shared gateway token, not the node device token. When the node intercepts browser.proxy commands with the wrong token, auth fails with:

"Browser control host rejected authentication"

This breaks browser functionality for all profiles (user, openclaw, etc.) because the gateway routes browser.proxy to any node advertising the capability.

Root cause

In NodeService.RegisterCapabilities():

_browserProxyCapability = new BrowserProxyCapability(
    _logger,
    _nodeClient.GatewayUrl,
    _token,  // node device token — WRONG
    ...);

_token comes from AttachClient() (node bearer token). NodeService had no access to the shared gateway token stored in GatewayRegistry.

Fix

  1. Add a Func<string?>? sharedGatewayTokenResolver to NodeService constructor
  2. In RegisterCapabilities(), only register BrowserProxyCapability when the resolver returns a non-empty shared token
  3. Pass the shared token (not _token) to BrowserProxyCapability
  4. Wire the resolver in App.xaml.cs to read from _gatewayRegistry?.GetActive()?.SharedGatewayToken

Behavior change

  • Before: Node always advertises browser.proxy (if settings toggle is on), auth fails silently
  • After: Node only advertises browser.proxy when a shared gateway token is saved in the registry. Falls back to not advertising the capability, letting the gateway handle browser commands via its own local path.

Validation

  • build.ps1
  • dotnet test tests/OpenClaw.Shared.Tests/OpenClaw.Shared.Tests.csproj --no-restore ✅ (2023 passed)
  • Tray tests: not applicable — NodeService.cs is not compiled into the Tray test project (it is not in the <Compile Include> list)

@clawsweeper
Copy link
Copy Markdown

clawsweeper Bot commented May 30, 2026

Codex review: needs real behavior proof before merge. Reviewed June 5, 2026, 8:53 PM ET / 00:53 UTC.

Summary
The PR changes browser.proxy registration to use SharedGatewayToken when present, suppresses the capability when it is absent, updates Command Center auth text, and also threads chat session keys through notifications and toast navigation.

Reproducibility: yes. for the core auth bug: current main passes the node bearer token into BrowserProxyCapability, and contributor terminal proof shows the auth rejection changing on the shared-token path. No high-confidence proof covers the no-shared-token skip/fallback path introduced by this PR.

Review metrics: 3 noteworthy metrics.

  • Diff scope: 11 files changed across 2 behavior areas. The branch combines browser.proxy auth-provider behavior with chat/toast session routing, so maintainers need to review more than the titled fix.
  • Required validation: 2 reported, 1 omitted. The PR body reports the repo build and shared tests but not the AGENTS-required tray test command for this tray-touching diff.
  • Proof coverage: 1 path shown, 1 path missing. Shared-token behavior has terminal proof, but the no-shared-token skip/fallback behavior has no real setup proof.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🦪 silver shellfish
Patch quality: 🧂 unranked krab
Result: blocked until stronger real behavior proof is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • [P1] Preserve browser.proxy when no shared gateway token is saved, or document an explicit maintainer-approved fail-closed contract.
  • [P1] Add redacted terminal or log proof for both shared-token and no-shared-token behavior; after updating the PR body, ClawSweeper should re-review automatically or a maintainer can comment @clawsweeper re-review.
  • [P1] Run and report the AGENTS-required tray test command, or give a concrete environment blocker.

Proof guidance:

  • [P1] Needs stronger real behavior proof before merge: Terminal proof supports the shared-token path, but contributor action is still needed for redacted terminal/log proof of the no-shared-token skip or fallback behavior before merge. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Risk before merge

  • [P1] Existing QR/bootstrap or no-secret-compatible browser-control host setups can lose browser.proxy entirely because the PR stops advertising the command when SharedGatewayToken is empty.
  • [P1] The Command Center auth warning is emitted only for nodes that declare browser.proxy, so the new skip path can leave affected users with a log line instead of visible Settings guidance.
  • [P2] The contributor proof covers the shared-token path but not the no-shared-token skip/fallback path, which is the upgrade-sensitive behavior changed by this PR.
  • [P1] The PR body reports the build and shared tests but omits the AGENTS-required tray test command even though the diff changes tray code and tray tests.
  • [P1] The branch now mixes the browser.proxy auth-provider change with chat notification/session routing changes, increasing the review surface before merge.

Maintainer options:

  1. Preserve the token-absent path (recommended)
    Before merge, keep browser.proxy registered when no shared token is saved, pass SharedGatewayToken when present, and retain missing-token guidance plus proof for both paths.
  2. Accept fail-closed with an explicit contract
    Maintainers can intentionally require SharedGatewayToken for browser.proxy, but the PR should document that contract and prove the gateway fallback and Command Center guidance for affected users.
  3. Split or pause the mixed branch
    If browser-control auth ownership is not settled, pause the browser.proxy change or split the unrelated chat-session routing work into a separate review.

Next step before merge

  • [P1] Human review is needed because the remaining blocker is a compatibility/auth-provider contract decision plus contributor-environment proof, not a narrow automation-safe repair.

Security
Cleared: The diff is auth-sensitive but adds no dependency, supply-chain, secret logging, broad permission, or non-loopback network change; the concrete concern is compatibility and auth-provider behavior.

Review findings

  • [P1] Preserve browser.proxy without a saved shared token — src/OpenClaw.Tray.WinUI/Services/NodeService.cs:370-386
  • [P2] Keep missing-token guidance visible when the command is skipped — src/OpenClaw.Tray.WinUI/Services/CommandCenterStateBuilder.cs:200
Review details

Best possible solution:

Use SharedGatewayToken for browser-control auth when available, preserve the existing token-absent browser.proxy path with visible Settings guidance, and validate both shared-token and no-shared-token behavior unless maintainers explicitly approve a documented fail-closed contract.

Do we have a high-confidence way to reproduce the issue?

Yes for the core auth bug: current main passes the node bearer token into BrowserProxyCapability, and contributor terminal proof shows the auth rejection changing on the shared-token path. No high-confidence proof covers the no-shared-token skip/fallback path introduced by this PR.

Is this the best way to solve the issue?

No, not as-is. Passing SharedGatewayToken when present is the narrow fix direction, but suppressing browser.proxy when the shared token is missing is broader than the tested and documented current behavior.

Full review comments:

  • [P1] Preserve browser.proxy without a saved shared token — src/OpenClaw.Tray.WinUI/Services/NodeService.cs:370-386
    This branch stops registering browser.proxy whenever the active gateway record has no SharedGatewayToken. Current main has explicit no-token diagnostics, regression coverage, and docs for compatible or no-secret browser-control host smokes, so token-absent users would lose the command instead of getting the existing actionable failure.
    Confidence: 0.92
  • [P2] Keep missing-token guidance visible when the command is skipped — src/OpenClaw.Tray.WinUI/Services/CommandCenterStateBuilder.cs:200
    The updated Command Center auth copy is still emitted only after a node declares browser.proxy. With the new skip path, QR/bootstrap-paired users get only a log entry and no visible route to Settings > Gateway Token, which conflicts with the repo guidance to surface missing credentials in the UI.
    Confidence: 0.86

Overall correctness: patch is incorrect
Overall confidence: 0.91

AGENTS.md: found and applied where relevant.

Codex review notes: model gpt-5.5, reasoning high; reviewed against 4be005707f44.

Label changes

Label justifications:

  • P1: The PR targets a broken browser.proxy auth workflow but currently introduces a blocking compatibility risk for token-absent setups.
  • merge-risk: 🚨 compatibility: Merging as-is can stop QR/bootstrap or no-secret-compatible setups from advertising browser.proxy at all.
  • merge-risk: 🚨 auth-provider: The diff changes which credential source is used for browser-control auth and when the browser.proxy capability is exposed.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🦪 silver shellfish and patch quality is 🧂 unranked krab.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: Terminal proof supports the shared-token path, but contributor action is still needed for redacted terminal/log proof of the no-shared-token skip or fallback behavior before merge. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed

What I checked:

  • Target AGENTS.md policy read: AGENTS.md was read in full; its connection credential rules and requirement that credential problems visibly route users to settings were relevant to the review. (AGENTS.md:1, 4be005707f44)
  • PR suppresses browser.proxy without SharedGatewayToken: At PR head, NodeService only constructs and registers BrowserProxyCapability when sharedGatewayTokenResolver returns a non-empty value; otherwise it logs that the capability was skipped. (src/OpenClaw.Tray.WinUI/Services/NodeService.cs:370, c319382650c2)
  • Current main preserves token-absent browser.proxy execution: Current main registers BrowserProxyCapability whenever the node client exists and the browser proxy setting is enabled, passing the existing node token into the capability instead of suppressing the command. (src/OpenClaw.Tray.WinUI/Services/NodeService.cs:364, 4be005707f44)
  • Current main has explicit missing shared-token guidance: BrowserProxyCapability returns a specific unauthenticated-request error telling users to save the matching gateway token or run a compatible auth host when no bearer token is available. (src/OpenClaw.Shared/Capabilities/BrowserProxyCapability.cs:149, 4be005707f44)
  • Current tests cover no-token auth guidance: CapabilityTests assert that an unauthorized browser.proxy request without a token explains the missing shared token and points to Settings. (tests/OpenClaw.Shared.Tests/CapabilityTests.cs:917, 4be005707f44)
  • Docs describe QR/bootstrap and no-secret-compatible browser-control smokes: Windows node testing docs say QR/bootstrap pairing can connect without a shared token and document a no-secret browser-control host smoke before full gateway invoke proof. (docs/WINDOWS_NODE_TESTING.md:99, 4be005707f44)

Likely related people:

  • shanselman: Introduced the local browser proxy bridge, added password-auth retry, and added the missing shared-token guidance used by the current behavior. (role: feature-history owner; confidence: high; commits: ff923514afa1, df700140927f, 10847ebd35a2; files: src/OpenClaw.Shared/Capabilities/BrowserProxyCapability.cs, tests/OpenClaw.Shared.Tests/CapabilityTests.cs, src/OpenClaw.Tray.WinUI/Services/NodeService.cs)
  • ranjeshj: Recent connection architecture work defines the device-token versus shared-token credential boundaries this PR changes around. (role: adjacent credential-architecture contributor; confidence: medium; commits: 03e560a05ce5, ffeff39c16c5; files: docs/CONNECTION_ARCHITECTURE.md, src/OpenClaw.Connection/CredentialResolver.cs, src/OpenClaw.Connection/InteractiveGatewayCredentialResolver.cs)
  • AlexAlves87: Extracted CommandCenterStateBuilder, the UI diagnostics path affected when browser.proxy is skipped. (role: adjacent Command Center contributor; confidence: medium; commits: 3d871f9b13c5; files: src/OpenClaw.Tray.WinUI/Services/CommandCenterStateBuilder.cs)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 auth-provider 🚨 Merging this PR could break OAuth, tokens, provider routing, model choice, or credentials. labels May 30, 2026
@QQSHI13 QQSHI13 force-pushed the master branch 2 times, most recently from 77a9f61 to 1f932c5 Compare May 30, 2026 11:23
@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels May 30, 2026
@QQSHI13
Copy link
Copy Markdown
Author

QQSHI13 commented May 30, 2026

Real behavior proof

browser.proxy is advertised when shared gateway token is available

❯ openclaw nodes describe --node 3f0faa8a2862fdb91b059ed902c2a0c73ea4cb2033200f07ffd518249222d691
...
│ Caps    │ browser, camera, canvas, device, location, screen, stt, system, tts                                                     │
...
Commands
- browser.proxy
- camera.list
...

browser.proxy appears in the command list, confirming the node advertises the capability when a shared gateway token is available.

Auth error is eliminated

Before fix (current master, node using device token):

nodes invoke failed: GatewayClientRequestError: Browser control host rejected authentication.
Verify the gateway token saved in Settings matches the browser-control host auth token or password.

After fix (this PR, node using shared gateway token):

nodes invoke failed: GatewayClientRequestError: {"error":"BrowserProfileUnavailableError:
Chrome MCP existing-session attach for profile \"user\" timed out after 300ms."}

The error changed from auth rejection (wrong token) to a downstream browser-host timeout (Chrome MCP session attach). This confirms the node now passes the correct shared gateway token to the browser control host. The remaining timeout is a gateway-side Chrome MCP issue unrelated to Windows Node auth.

@QQSHI13
Copy link
Copy Markdown
Author

QQSHI13 commented May 30, 2026

@clawsweeper re-review

@clawsweeper
Copy link
Copy Markdown

clawsweeper Bot commented May 30, 2026

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@QQSHI13
Copy link
Copy Markdown
Author

QQSHI13 commented May 31, 2026

@clawsweeper re-review

@clawsweeper
Copy link
Copy Markdown

clawsweeper Bot commented May 31, 2026

🦞👀
ClawSweeper picked this up.

Command router queued. I will update this comment with the next step.

Re-review progress:

@clawsweeper clawsweeper Bot added P1 Urgent regression or broken agent/channel workflow affecting real users now. and removed P2 Normal priority bug or improvement with limited blast radius. labels Jun 3, 2026
@clawsweeper
Copy link
Copy Markdown

clawsweeper Bot commented Jun 3, 2026

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@QQSHI13
Copy link
Copy Markdown
Author

QQSHI13 commented Jun 5, 2026

@clawsweeper re-review

@clawsweeper
Copy link
Copy Markdown

clawsweeper Bot commented Jun 5, 2026

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

QQ added 3 commits June 6, 2026 08:47
…able

The Windows Node was advertising browser.proxy with its node device token,
which cannot authenticate against the browser control host. The browser
control host expects the shared gateway token.

Changes:
- NodeService: add sharedGatewayTokenResolver callback
- Only register BrowserProxyCapability when a shared gateway token is available
- Pass the shared token (not the node bearer token) to BrowserProxyCapability
- App.xaml.cs: wire resolver to GatewayRegistry.GetActive().SharedGatewayToken

Fixes: the node no longer breaks browser commands by intercepting them with
wrong credentials.
Add SessionKey property to OpenClawNotification so toast activations
can route to the specific session that produced the assistant response.
Update EmitChatNotification to accept and populate the session key from
the gateway chat event payload.
Previously, clicking a session in the Sessions tab or a chat toast
notification always opened the default/main session instead of the
specific session.

Changes:
- Add App.PendingChatSessionKey as a fallback when HubWindow does not
  exist (background notifications, closed HubWindow).
- Thread sessionKey through ToastActivationRouter (Action<string?>).
- Embed sessionKey in chat toast arguments and route it on activation.
- ChatPage (functional): consume pending key from both HubWindow and
  App fallbacks; use !string.IsNullOrEmpty guard to treat empty string
  as null.
- ChatPage (WebView): consume pending key and append &session={key} to
  the navigation URL, or pass it through GatewayChatHelper on initial
  WebView init.
- SessionsPage: write key to CurrentApp.PendingChatSessionKey in
  addition to hub.PendingChatSessionKey.
- Update tests for the new OpenChat Action<string?> contract.

Fixes session routing for both native FunctionalUI and legacy WebView2
chat surfaces.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 auth-provider 🚨 Merging this PR could break OAuth, tokens, provider routing, model choice, or credentials. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P1 Urgent regression or broken agent/channel workflow affecting real users now. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant