Skip to content

Fix canvas navigation URL handling#711

Open
liorb-mountapps wants to merge 1 commit into
openclaw:mainfrom
liorb-mountapps:codex/fix-canvas-navigate
Open

Fix canvas navigation URL handling#711
liorb-mountapps wants to merge 1 commit into
openclaw:mainfrom
liorb-mountapps:codex/fix-canvas-navigate

Conversation

@liorb-mountapps

@liorb-mountapps liorb-mountapps commented Jun 7, 2026

Copy link
Copy Markdown

Summary

  • route canvas.navigate into the WebView canvas again
  • stop canvas.present from rewriting external URLs to the gateway origin
  • preserve gateway/tunnel URL rewriting by tracking both configured and effective gateway origins
  • add behavior tests for external URLs, tunnel gateway rewrites, and relative gateway paths

Why

  • canvas.navigate is a canvas command, so it should change the canvas URL. That matches the other clients and lets agents reuse one canvas surface.
  • Opening the OS browser from canvas.navigate made Windows the odd one out and overlapped with browser-specific capabilities.
  • Gateway URL rewriting is still needed for gateway/tunnel URLs, but external URLs must stay external.

Review Fix

  • CanvasWindow now receives the effective node URL plus the configured gateway URL when SSH tunnel mode is enabled.
  • The effective origin remains the trusted WebView/auth origin.
  • The configured origin is only used as an extra rewrite match.
  • External origins are not rewritten.

Proof

Live Windows/WebView proof via local MCP against this branch, using the source-dev profile:

canvas.present https://example.com/ -> { presented: true }
canvas.eval location.href -> https://example.com/
canvas.navigate https://www.iana.org/domains/reserved -> { navigated: true, opener: canvas, url: https://www.iana.org/domains/reserved }
canvas.eval location.href -> https://www.iana.org/domains/reserved

Redacted app log proof:

[Canvas] Trusted gateway origin: [redacted-gateway]; configured gateway origin: [redacted-gateway]
Canvas presented: 900x650
Canvas navigate -> canvas: https://www.iana.org/domains/...

Tunnel/gateway rewrite proof is covered by CanvasGatewayUrlRewriterTests:

  • external URL stays unchanged
  • configured gateway origin rewrites to effective localhost tunnel origin
  • relative gateway path resolves against effective origin

Validation

  • ./build.ps1
  • dotnet test ./tests/OpenClaw.Shared.Tests/OpenClaw.Shared.Tests.csproj --no-restore
  • dotnet test ./tests/OpenClaw.Tray.Tests/OpenClaw.Tray.Tests.csproj --no-restore

Results:

  • Shared tests: 2049 passed, 29 skipped
  • Tray tests: 964 passed

@clawsweeper

clawsweeper Bot commented Jun 7, 2026

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge. Reviewed June 7, 2026, 6:53 AM ET / 10:53 UTC.

Summary
The PR routes canvas.navigate through the CanvasWindow WebView, narrows CanvasWindow gateway URL rewriting, and adds tray source-scan regression tests.

Reproducibility: yes. for the blocking review finding from source inspection: tunnel node connections use a localhost URL, CanvasWindow stores both rewrite origins from that same URL, and the PR condition therefore cannot match remote gateway URLs that still need rewriting.

Review metrics: 1 noteworthy metric.

  • Changed surface: 2 runtime files changed, 1 test file changed. The PR changes both command routing and URL rewriting, so source-scan tests alone do not settle runtime behavior.

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] Fix gateway URL rewriting so tunnel/local gateway URLs still rewrite while external URLs stay external.
  • [P1] Add redacted real behavior proof showing canvas.navigate and canvas.present with external and gateway/tunnel URLs.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body lists validation commands but no screenshot, recording, terminal output, logs, or live output proving the changed canvas behavior in a real Windows/WebView setup; add redacted proof and update the PR body to trigger re-review.

Mantis proof suggestion
A real Windows/WebView visual proof would materially help verify the changed canvas navigation and URL rewrite behavior. 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 canvas.navigate loads the requested URL inside the WebView, canvas.present preserves external URLs, and gateway/tunnel URLs still rewrite correctly.

Risk before merge

  • [P1] Merging as-is can break existing SSH-tunnel or remote-gateway canvas URLs that previously relied on rewriting gateway-origin URLs to the effective localhost tunnel origin.
  • [P1] The PR has no real Windows/WebView behavior proof, so reviewers cannot confirm the changed canvas navigation path, external URL preservation, or tunnel rewrite behavior outside source inspection.

Maintainer options:

  1. Preserve dual-origin rewriting (recommended)
    Track both the configured gateway origin and the effective node/tunnel origin so only gateway URLs rewrite while external URLs remain untouched.
  2. Accept direct remote gateway loads
    Maintainers could intentionally drop tunnel rewrite compatibility, but that should be an explicit product decision with upgrade notes and proof.
  3. Pause for navigation policy
    If Windows canvas.navigate is meant to keep using the OS browser, close or redesign this PR around the narrower external URL rewrite bug.

Next step before merge

  • [P1] Contributor action is needed to repair the tunnel rewrite compatibility bug and provide real behavior proof; ClawSweeper repair should not be requested while the external proof gate is still missing.

Security
Cleared: No dependency, secret, or supply-chain regression was found; the remaining blocker is compatibility of the URL rewrite behavior.

Review findings

  • [P1] Preserve tunnel gateway URL rewrites — src/OpenClaw.Tray.WinUI/Windows/CanvasWindow.xaml.cs:185-187
Review details

Best possible solution:

Route canvas.navigate to the WebView only after preserving original-gateway-to-effective-gateway rewriting for tunnel/local-origin cases, then add real redacted Windows canvas proof for both external and gateway URL paths.

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

Yes for the blocking review finding from source inspection: tunnel node connections use a localhost URL, CanvasWindow stores both rewrite origins from that same URL, and the PR condition therefore cannot match remote gateway URLs that still need rewriting.

Is this the best way to solve the issue?

No as-is. The WebView routing may be the right direction, but the rewrite logic needs a dual-origin model and real behavior proof before this is the narrow maintainable fix.

Full review comments:

  • [P1] Preserve tunnel gateway URL rewrites — src/OpenClaw.Tray.WinUI/Windows/CanvasWindow.xaml.cs:185-187
    This condition only rewrites URLs whose origin equals _trustedGatewayOrigin, but SetTrustedGatewayOrigin currently sets _trustedGatewayOrigin and _gatewayOriginForRewrite from the same effective node URL. In SSH-tunnel mode that effective URL is ws://localhost:<port>, so gateway-served URLs using the configured remote origin no longer rewrite to the local tunnel and can fail to load. Please keep external URLs untouched while still tracking both the original gateway origin and the effective tunnel origin for gateway URL rewrites.
    Confidence: 0.89

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 4be005707f44.

Label changes

Label changes:

  • add P2: This is a normal-priority canvas navigation fix with limited blast radius but real user-visible behavior impact.
  • add merge-risk: 🚨 compatibility: The diff can break existing tunnel/gateway canvas URLs by no longer rewriting remote gateway origins to the effective local origin.
  • 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 lists validation commands but no screenshot, recording, terminal output, logs, or live output proving the changed canvas behavior in a real Windows/WebView setup; add redacted proof and update the PR body to trigger re-review.

Label justifications:

  • P2: This is a normal-priority canvas navigation fix with limited blast radius but real user-visible behavior impact.
  • merge-risk: 🚨 compatibility: The diff can break existing tunnel/gateway canvas URLs by no longer rewriting remote gateway origins to the effective local origin.
  • 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 lists validation commands but no screenshot, recording, terminal output, logs, or live output proving the changed canvas behavior in a real Windows/WebView setup; add redacted proof and update the PR body to trigger re-review.
Evidence reviewed

What I checked:

  • Repository policy read: AGENTS.md was read fully; its tray/node architecture and validation guidance is relevant to this canvas/node PR, though the review itself stayed read-only. (AGENTS.md:1, 4be005707f44)
  • Current source passes effective node URL to CanvasWindow: Current main creates CanvasWindow and calls SetTrustedGatewayOrigin(GatewayUrl, _token), where GatewayUrl comes from the active WindowsNodeClient. (src/OpenClaw.Tray.WinUI/Services/NodeService.cs:862, 4be005707f44)
  • Tunnel node connections use localhost as the effective URL: GatewayConnectionManager connects the node to ws://localhost: when an SSH tunnel is configured, so CanvasWindow currently receives the effective local URL in tunnel mode. (src/OpenClaw.Connection/GatewayConnectionManager.cs:940, 4be005707f44)
  • Current CanvasWindow stores both origins from one URL: SetTrustedGatewayOrigin currently assigns both _trustedGatewayOrigin and _gatewayOriginForRewrite from the same normalized gatewayUrl, so they do not represent original-vs-effective origins. (src/OpenClaw.Tray.WinUI/Windows/CanvasWindow.xaml.cs:141, 4be005707f44)
  • PR rewrite guard cannot match remote gateway origins: The PR only rewrites when urlOrigin equals _trustedGatewayOrigin and differs from _gatewayOriginForRewrite; with current storage those origins are identical, so remote configured gateway URLs no longer rewrite to the tunnel URL. (src/OpenClaw.Tray.WinUI/Windows/CanvasWindow.xaml.cs:185, 48685d439cb5)
  • Proof is absent: The PR body lists build/test commands but does not include screenshot, recording, logs, terminal output, or live output showing the changed canvas behavior in a real setup. (48685d439cb5)

Likely related people:

  • shanselman: Commit 0d4fcbd touched both NodeService and CanvasWindow in a recent reliability/hardening pass for this surface. (role: recent area contributor; confidence: medium; commits: 0d4fcbd50ad5; files: src/OpenClaw.Tray.WinUI/Services/NodeService.cs, src/OpenClaw.Tray.WinUI/Windows/CanvasWindow.xaml.cs)
  • codemonkeychris: Commit 9fa43f3 introduced the HttpUrlRiskEvaluator and touched NodeService and CanvasCapability, which are adjacent to this navigation behavior. (role: related hardening introducer; confidence: medium; commits: 9fa43f347703; files: src/OpenClaw.Shared/HttpUrlRiskEvaluator.cs, src/OpenClaw.Shared/Capabilities/CanvasCapability.cs, src/OpenClaw.Tray.WinUI/Services/NodeService.cs)
  • AlexAlves87: Commit e832229 wired the WebView2 native bridge in CanvasWindow and added related tray source-scan tests. (role: CanvasWindow feature contributor; confidence: medium; commits: e832229a9e4b; files: src/OpenClaw.Tray.WinUI/Windows/CanvasWindow.xaml.cs, tests/OpenClaw.Tray.Tests/TrayMenuWindowMarkupTests.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: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. labels Jun 7, 2026
@liorb-mountapps liorb-mountapps force-pushed the codex/fix-canvas-navigate branch from 48685d4 to 091d7aa Compare June 7, 2026 11:43
@liorb-mountapps

Copy link
Copy Markdown
Author

@clawsweeper re-review

Updated after review: preserved configured-gateway-to-effective-tunnel rewrites, added behavior tests for external/tunnel/relative URL rewriting, and added live Windows/WebView MCP proof to the PR body.

@clawsweeper

clawsweeper Bot commented Jun 7, 2026

Copy link
Copy Markdown

🦞🧹
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.

@liorb-mountapps

Copy link
Copy Markdown
Author

@clawsweeper re-review

Retrying because the previous ClawSweeper run failed during target repository checkout before reviewing the updated PR.

@clawsweeper

clawsweeper Bot commented Jun 7, 2026

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

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

Re-review progress:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. 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