Skip to content

Fix canvas WebView2 surface auth via plugin-node cap URL#588

Draft
christineyan4 wants to merge 1 commit into
openclaw:mainfrom
christineyan4:canvas-bug
Draft

Fix canvas WebView2 surface auth via plugin-node cap URL#588
christineyan4 wants to merge 1 commit into
openclaw:mainfrom
christineyan4:canvas-bug

Conversation

@christineyan4
Copy link
Copy Markdown
Contributor

Problem

The Windows tray's Canvas WebView2 hits 401 Unauthorized when navigating to /__openclaw__/canvas/* paths because it builds URLs against the gateway origin only, omitting the plugin-node capability token the gateway requires for cross-node plugin surface routes.

Fix

The gateway already advertises a cap-scoped surface URL in its hello-ok response under pluginSurfaceUrls.canvas (e.g. http://<gateway>/__openclaw__/cap/<token>). This PR plumbs that URL through to CanvasWindow and uses it as the base for /__openclaw__/canvas/* rewrites.

Additionally:

  • Treats the cap URL as a trusted origin prefix in IsUrlSafe so a cap URL whose host doesn't match _trustedGatewayOrigin (e.g. 127.0.0.1 cap vs localhost origin) isn't rejected by the safety check.
  • Clears the cached cap URL when a subsequent hello-ok/health update omits the canvas key, so a rotated/revoked token doesn't leave the node using a stale URL.

Files changed

File Change
src/OpenClaw.Shared/Models.cs Parse pluginSurfaceUrls.canvas from hello-ok
src/OpenClaw.Shared/WindowsNodeClient.cs Expose CanvasSurfaceUrl; clear when canvas key absent
src/OpenClaw.Tray.WinUI/Services/NodeService.cs Push URL into CanvasWindow on connect + on update
src/OpenClaw.Tray.WinUI/Windows/CanvasWindow.xaml.cs Rewrite /__openclaw__/canvas/* via cap base; trust as IsUrlSafe prefix

Net: +128 / -14 across 4 files. No new files, no API changes outside the tray.

Testing

  • ✅ Existing tray test suite passes (859 tests)
  • ✅ Build clean on net10.0-windows10.0.22621.0 / win-arm64 (0 warnings, 0 errors)
  • ✅ Verified E2E: canvas.present with /__openclaw__/canvas/generative-art.html now renders the real content instead of the dashboard SPA shell / 401 page.

Known limitations (deferred)

  • TOCTOU race between canvas.present and the dispatcher-enqueued SetTrustedGatewayOrigin — theoretical, never observed.
  • Absolute-form canvas URLs (https://gw/__openclaw__/canvas/*) still go through the gateway origin path; gateway doesn't currently emit those.

The Windows tray's Canvas WebView2 was hitting 401 when navigating
to /__openclaw__/canvas/* paths because it built URLs against the
gateway origin only, omitting the plugin-node capability token the
gateway requires for cross-node plugin surface routes.

The gateway already advertises a cap-scoped surface URL in its
hello-ok response (pluginSurfaceUrls.canvas), e.g.
  http://<gateway>/__openclaw__/cap/<token>

This change plumbs that URL through to CanvasWindow and uses it as
the base for /__openclaw__/canvas/* rewrites, and as an additional
trusted origin prefix in IsUrlSafe so a cap URL whose host doesn't
match _trustedGatewayOrigin (e.g. 127.0.0.1 cap vs localhost origin)
is not rejected by the safety check.

Also clears the cached cap URL when a subsequent hello-ok/health
update omits the canvas key, so a rotated/revoked token doesn't
leave the node using a stale URL.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@clawsweeper
Copy link
Copy Markdown

clawsweeper Bot commented May 29, 2026

Codex review: needs real behavior proof before merge. Reviewed May 29, 2026, 5:24 PM ET / 21:24 UTC.

Summary
The branch parses pluginSurfaceUrls.canvas from the gateway handshake, exposes it through WindowsNodeClient, and uses it in NodeService/CanvasWindow to rewrite canvas WebView2 routes through the cap-scoped URL.

Reproducibility: no. high-confidence live reproduction is included. Source inspection shows current master rewrites relative canvas paths to the bare gateway origin and does not read pluginSurfaceUrls, which matches the reported 401 failure mode if the gateway requires a cap URL.

Review metrics: 2 noteworthy metrics.

  • Changed surface: 4 files, +128/-14. The patch crosses shared handshake parsing, node-client cache state, tray service propagation, and WebView URL safety.
  • WebView trust change: 1 URL-safety path broadened. CanvasWindow now treats a gateway-advertised cap prefix as safe before applying the dangerous URL regex.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🧂 unranked krab
Result: blocked until 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] Validate cap URLs as absolute http(s) cap routes and keep dangerous schemes blocked.
  • Propagate pluginSurfaceUrls removal or empty maps to existing CanvasWindow instances and cover it with focused tests.
  • [P1] Add redacted proof showing canvas.present renders through the cap URL without a 401; screenshots, recordings, terminal output, or logs are acceptable when they show the behavior.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body states E2E verification, but it does not include inspectable after-fix screenshot, recording, terminal output, live output, linked artifact, or redacted log; the contributor should add proof and update the PR body to trigger re-review.

Mantis proof suggestion
A real desktop proof would materially help review because the change is visible WebView2 canvas routing with auth diagnostics. A maintainer can ask Mantis to capture proof by posting a new PR comment that starts with the OpenClaw Mantis account mention, followed by:

visual task: verify Windows tray canvas.present for /__openclaw__/canvas/generative-art.html renders through the cap-scoped URL and captures diagnostics showing no 401.

Risk before merge

  • [P1] Merging as-is broadens WebView navigation trust to an unvalidated gateway-provided string before the existing dangerous URL checks run.
  • [P1] Cap rotation or removal can leave an already-open canvas window using and trusting a stale cap URL.
  • [P1] The PR body reports E2E verification, but no screenshot, recording, terminal output, redacted log, or linked artifact is available to inspect the after-fix behavior.

Maintainer options:

  1. Validate cap routing before merge (recommended)
    Require the branch to parse and validate pluginSurfaceUrls.canvas, keep dangerous schemes blocked, refresh existing windows on removal, and prove cap rotation/removal behavior before landing.
  2. Accept gateway-expanded trust explicitly
    Maintainers could intentionally treat hello-ok surface URLs as a WebView trust boundary, but that decision should be documented and paired with tests for malformed and revoked cap values.

Next step before merge

  • [P1] This PR needs contributor-visible fixes and real behavior proof before normal maintainer review; automation should not take over while proof is missing.

Security
Needs attention: The PR widens WebView URL trust using an unvalidated cap URL from the gateway handshake.

Review findings

  • [P1] Validate cap URLs before trusting them — src/OpenClaw.Tray.WinUI/Windows/CanvasWindow.xaml.cs:95-96
  • [P2] Propagate cap removal to existing canvas windows — src/OpenClaw.Tray.WinUI/Services/NodeService.cs:832-834
  • [P2] Keep empty surface maps as removal signals — src/OpenClaw.Shared/Models.cs:706-707
Review details

Best possible solution:

Validate the advertised cap URL as an absolute http(s) cap route, propagate surface removals to existing canvas windows, add focused regression coverage for happy-path and removal cases, then attach redacted real behavior proof.

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

No high-confidence live reproduction is included. Source inspection shows current master rewrites relative canvas paths to the bare gateway origin and does not read pluginSurfaceUrls, which matches the reported 401 failure mode if the gateway requires a cap URL.

Is this the best way to solve the issue?

No for the current branch. The cap-scoped routing approach is likely the right narrow fix, but it needs URL validation and complete stale-cap clearing before it is safe to merge.

Full review comments:

  • [P1] Validate cap URLs before trusting them — src/OpenClaw.Tray.WinUI/Windows/CanvasWindow.xaml.cs:95-96
    This accepts any URL that starts with _canvasSurfaceBaseUrl before DangerousUrlPattern runs, but _canvasSurfaceBaseUrl is copied directly from hello-ok. A malformed surface value such as a javascript:, file:, or non-cap URL would bypass the node's existing canvas navigation guard; parse the advertised value and only trust absolute http(s) cap routes.
    Confidence: 0.86
  • [P2] Propagate cap removal to existing canvas windows — src/OpenClaw.Tray.WinUI/Services/NodeService.cs:832-834
    This refreshes an open CanvasWindow only when the update includes a non-empty canvas URL. If a later pluginSurfaceUrls update removes canvas during rotation or revocation, the client cache can be cleared while the already-open window keeps its old _canvasSurfaceBaseUrl and keeps rewriting/allowing stale cap URLs; enqueue a refresh with null as well.
    Confidence: 0.9
  • [P2] Keep empty surface maps as removal signals — src/OpenClaw.Shared/Models.cs:706-707
    FromHelloOk discards an empty pluginSurfaceUrls object by leaving PluginSurfaceUrls null. Downstream cache-clearing only happens when the property is non-null, so a gateway update of {} cannot revoke the cached canvas surface; preserve the empty map or another explicit removal signal.
    Confidence: 0.88

Overall correctness: patch is incorrect
Overall confidence: 0.88

AGENTS.md: found and applied where relevant.

Codex review notes: model gpt-5.5, reasoning high; reviewed against 64b650cb3313.

Label changes

Label changes:

  • add P2: This is a normal-priority canvas auth bug fix with limited blast radius, but it needs review fixes before merge.
  • add merge-risk: 🚨 auth-provider: The diff changes token-scoped canvas routing and can leave stale cap credentials in an open window after removal or rotation.
  • add merge-risk: 🚨 security-boundary: The diff changes the WebView URL safety boundary by trusting a cap URL string from the gateway handshake.
  • add rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🧂 unranked krab.
  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body states E2E verification, but it does not include inspectable after-fix screenshot, recording, terminal output, live output, linked artifact, or redacted log; the contributor should add proof and update the PR body to trigger re-review.

Label justifications:

  • P2: This is a normal-priority canvas auth bug fix with limited blast radius, but it needs review fixes before merge.
  • merge-risk: 🚨 security-boundary: The diff changes the WebView URL safety boundary by trusting a cap URL string from the gateway handshake.
  • merge-risk: 🚨 auth-provider: The diff changes token-scoped canvas routing and can leave stale cap credentials in an open window after removal or rotation.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🧂 unranked krab.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body states E2E verification, but it does not include inspectable after-fix screenshot, recording, terminal output, live output, linked artifact, or redacted log; the contributor should add proof and update the PR body to trigger re-review.
Evidence reviewed

Security concerns:

  • [medium] Unvalidated cap URL bypasses canvas URL guards — src/OpenClaw.Tray.WinUI/Windows/CanvasWindow.xaml.cs:95
    IsUrlSafe returns true for _canvasSurfaceBaseUrl before the dangerous-scheme regex, and the base URL is assigned from hello-ok without checking that it is an absolute http(s) cap route. That weakens the existing navigation boundary for canvas content.
    Confidence: 0.86

What I checked:

Likely related people:

  • Scott Hanselman: Auth header injection and related CanvasWindow/NodeService hardening were added in recent commit 0d4fcbd, directly adjacent to this PR's WebView auth and trust changes. (role: recent area contributor; confidence: high; commits: 0d4fcbd50ad5; files: src/OpenClaw.Tray.WinUI/Windows/CanvasWindow.xaml.cs, src/OpenClaw.Tray.WinUI/Services/NodeService.cs)
  • Christine Yan: Before this PR, Christine authored merged canvas window behavior in NodeService, including the existing Canvas window activation path this PR modifies. (role: prior canvas contributor; confidence: medium; commits: 82408b8d7fa0; files: src/OpenClaw.Tray.WinUI/Services/NodeService.cs)
  • github-actions[bot]: The available shallow history blames the initial checked-in versions of GatewaySelfInfo, WindowsNodeClient, NodeService, and CanvasWindow to the grafted 5a87c99 commit, so it is provenance but weak routing signal. (role: bulk source introducer; confidence: low; commits: 5a87c9921c31; files: src/OpenClaw.Shared/Models.cs, src/OpenClaw.Shared/WindowsNodeClient.cs, src/OpenClaw.Tray.WinUI/Services/NodeService.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: 🧂 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. P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 auth-provider 🚨 Merging this PR could break OAuth, tokens, provider routing, model choice, or credentials. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. labels May 29, 2026
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: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. P2 Normal priority bug or improvement with limited blast radius. 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