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
  • keep the existing high-risk URL approval gate before WebView navigation
  • return navigated: false when the approval prompt denies navigation
  • stop canvas.present from rewriting external URLs to the gateway origin
  • preserve gateway/tunnel URL rewriting by tracking both configured and effective gateway origins
  • read the configured gateway URL from the active saved gateway record, not stale settings
  • add behavior tests for external URLs, tunnel gateway rewrites, relative gateway paths, active-record wiring, and denied navigation responses

Why

  • canvas.navigate is a canvas command, so it should change the canvas URL.
  • Opening the OS browser from canvas.navigate made Windows different from the other clients.
  • Risky agent-supplied URLs still need user approval before they enter an inspectable WebView.
  • Gateway URL rewriting is still needed for gateway/tunnel URLs, but external URLs must stay external.
  • Saved gateway switches can leave SettingsManager stale, so canvas must use GatewayRegistry.GetActive() for the configured gateway URL.

Review Fix

  • CanvasWindow now receives the effective node URL plus the configured gateway URL.
  • App passes the configured URL from the active GatewayRecord.
  • canvas.navigate now runs HttpUrlRiskEvaluator, DNS enrichment, and ShouldLaunchAfterPromptAsync before WebView navigation.
  • If the prompt denies, WebView is not navigated and the MCP response says navigated: false, opener: denied.
  • 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
canvas.navigate http://127.0.0.1:9/ + deny prompt -> { navigated: false, opener: denied, url: http://127.0.0.1:9/ }
canvas.eval location.href -> https://www.iana.org/domains/reserved

Redacted app log proof:

[Canvas] Trusted gateway origin: [redacted-gateway]; configured gateway origin: [redacted-active-gateway]
Canvas presented: 900x650
Canvas navigate -> canvas: https://www.iana.org/domains/...
[NavigationApproval] Prompt decision: Deny for http://127.0.0.1:9/
Canvas navigate denied before WebView navigation: http://127.0.0.1:9/ (user denied)

Tunnel/gateway rewrite proof is covered by CanvasGatewayUrlRewriterTests:

  • external URL stays unchanged
  • configured saved 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: 2050 passed, 29 skipped
  • Tray tests: 965 passed

@clawsweeper

clawsweeper Bot commented Jun 7, 2026

Copy link
Copy Markdown

Codex review: found issues before merge. Reviewed June 11, 2026, 8:29 AM ET / 12:29 UTC.

Summary
The PR routes canvas.navigate into the WebView canvas, preserves gateway URL rewriting using configured and effective origins, reads the active GatewayRegistry URL, and adds navigation/rewrite tests.

Reproducibility: yes. from source rather than a Windows run: approve a localhost/private canvas.navigate request and NodeService will call _canvasWindow.Navigate, where CanvasWindow still rejects that URL class via DangerousUrlPattern.

Review metrics: 1 noteworthy metric.

  • Touched surface: 9 files, +222/-65. The diff changes the security-sensitive canvas navigation path, gateway URL rewriting, app wiring, and tests, so maintainers should review it as more than a narrow response-shape tweak.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🐚 platinum hermit
Patch quality: 🧂 unranked krab
Result: blocked by patch quality or review findings.

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

Rank-up moves:

  • [P2] Fix the approved high-risk URL path so consented localhost/private http(s) destinations either load in WebView or intentionally keep the browser fallback.
  • [P2] Add real Windows/WebView proof for both allow and deny prompt outcomes after the fix.
  • Get maintainer signoff on whether canvas.navigate should disclose prompt denial to the agent.

Mantis proof suggestion
A short visible Windows/WebView proof would materially help confirm the browser-to-canvas behavior and the high-risk allow/deny prompt flow. 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 an external URL in the WebView and that allowed and denied high-risk URLs report the correct result without bypassing the approval prompt.

Risk before merge

  • [P1] Approved localhost/private URLs can pass the new prompt and then fail in CanvasWindow.IsUrlSafe, returning a navigation error instead of loading the approved destination.
  • [P1] The diff intentionally changes the existing prompt disclosure model by awaiting and reporting denial to the agent; maintainers should explicitly accept that security-boundary change before merge.
  • [P1] The WebView-vs-browser behavior change affects existing canvas.navigate compatibility, especially for local dev, tunnel, and private-network destinations.

Maintainer options:

  1. Fix the approved high-risk path (recommended)
    Make the WebView navigation path honor an explicit approval for localhost/private http(s) URLs without weakening scheme, userinfo, or gateway-origin checks, then prove allow and deny behavior.
  2. Accept the prompt disclosure change
    Maintainers can choose to allow canvas.navigate to report denied/allowed prompt outcomes to the agent, but that should be an explicit security-boundary decision.
  3. Keep browser fallback semantics
    If approved risky URLs should remain outside the WebView boundary, pause this branch and keep or restore the browser fallback for that class of navigation.

Next step before merge

  • [P2] The PR needs a maintainer security/product decision plus a code fix for the approved high-risk navigation path before it is merge-ready.

Security
Needs attention: The diff changes the high-risk URL approval boundary and exposes prompt outcomes to the agent, so maintainer security review is needed before merge.

Review findings

  • [P1] Preserve the approved high-risk navigation path — src/OpenClaw.Tray.WinUI/Services/NodeService.cs:963
Review details

Best possible solution:

Align the approval result with CanvasWindow navigation so approved high-risk http(s) destinations can load only after explicit consent, denied destinations return false, gateway rewrites stay scoped, and maintainers explicitly accept the WebView-vs-browser boundary.

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

Yes, from source rather than a Windows run: approve a localhost/private canvas.navigate request and NodeService will call _canvasWindow.Navigate, where CanvasWindow still rejects that URL class via DangerousUrlPattern.

Is this the best way to solve the issue?

No. The direction is plausible, but the current patch does not align the approval gate with CanvasWindow's private-host block and changes prompt-disclosure semantics without clear maintainer signoff.

Full review comments:

  • [P1] Preserve the approved high-risk navigation path — src/OpenClaw.Tray.WinUI/Services/NodeService.cs:963
    After the user approves a high-risk URL such as http://127.0.0.1:9/, this new path calls _canvasWindow.Navigate(canonical!), but CanvasWindow still rejects localhost/private URLs in IsUrlSafe. That turns an approved navigation into Navigate failed, while current main launches the approved URL in the browser; route approved URLs through a WebView path that honors the approval or keep the browser fallback for this class.
    Confidence: 0.91

Overall correctness: patch is incorrect
Overall confidence: 0.86

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 913ba4e8f504.

Label changes

Label changes:

  • add proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes live MCP/WebView output and redacted app logs showing after-fix external navigation and denial behavior, though the approved high-risk path still needs proof after repair.
  • add rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🐚 platinum hermit and patch quality is 🧂 unranked krab.
  • add status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (live_output): The PR body includes live MCP/WebView output and redacted app logs showing after-fix external navigation and denial behavior, though the approved high-risk path still needs proof after repair.
  • remove rating: 🌊 off-meta tidepool: Current PR rating is rating: 🧂 unranked krab, so this older rating label is no longer current.

Label justifications:

  • P2: This is a normal-priority canvas navigation fix with limited blast radius, but it is not an emergency or proven live outage.
  • merge-risk: 🚨 compatibility: Merging changes canvas.navigate from default-browser launch to WebView navigation and can break existing approved localhost/private workflows.
  • merge-risk: 🚨 security-boundary: The PR changes how agent-supplied high-risk URLs cross the approval prompt and what prompt outcome is disclosed back to the agent.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🐚 platinum hermit and patch quality is 🧂 unranked krab.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (live_output): The PR body includes live MCP/WebView output and redacted app logs showing after-fix external navigation and denial behavior, though the approved high-risk path still needs proof after repair.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes live MCP/WebView output and redacted app logs showing after-fix external navigation and denial behavior, though the approved high-risk path still needs proof after repair.
Evidence reviewed

Security concerns:

  • [medium] High-risk URL boundary changes need signoff — src/OpenClaw.Tray.WinUI/Services/NodeService.cs:950
    Current main deliberately reports canvas.navigate before the prompt so prompt existence and decision timing are not observable by the agent; this PR awaits ShouldLaunchAfterPromptAsync and returns navigated: false on denial, which may be acceptable but should be an explicit security decision.
    Confidence: 0.82

What I checked:

  • Repository policy: Read the full AGENTS.md and applied its connection/canvas review guidance by inspecting the connection architecture, MCP mode, Windows node testing, and onboarding docs before reviewing the gateway/canvas changes. (AGENTS.md:1, 913ba4e8f504)
  • PR diff: The PR changes NodeService.OnCanvasNavigate to await DNS risk enrichment and the approval prompt, then dispatch _canvasWindow.Navigate(canonical!) on the UI thread. (src/OpenClaw.Tray.WinUI/Services/NodeService.cs:949, 764265df5e13)
  • Current WebView safety gate: CanvasWindow.IsUrlSafe still blocks localhost and private IPv4 URLs via DangerousUrlPattern unless they are the trusted gateway origin, so an approved local/private canvas.navigate request is rejected after the prompt. (src/OpenClaw.Tray.WinUI/Windows/CanvasWindow.xaml.cs:72, 913ba4e8f504)
  • Current main behavior: Current main launches approved high-risk canvas.navigate URLs in the default browser asynchronously and intentionally reports before the prompt to avoid leaking prompt timing or decision latency to the agent. (src/OpenClaw.Tray.WinUI/Services/NodeService.cs:970, 85445c78066b)
  • PR proof: The PR body includes live Windows/WebView MCP output for external navigation and denial, plus redacted logs showing the approval denial path; it does not show the approved high-risk local/private URL path that the source review found broken. (764265df5e13)
  • Maintainer notes: No matching .agents/maintainer-notes directory was present in this checkout, so no internal maintainer decision overrode the source review.

Likely related people:

  • Christine Yan: git blame attributes the current main NodeService, CanvasWindow, and CanvasCapability canvas navigation lines to the v0.6.3 localization/code-behind commit, so this is the strongest current-line routing signal. (role: current source owner; confidence: medium; commits: 85445c78066b; files: src/OpenClaw.Tray.WinUI/Services/NodeService.cs, src/OpenClaw.Tray.WinUI/Windows/CanvasWindow.xaml.cs, src/OpenClaw.Shared/Capabilities/CanvasCapability.cs)
  • Chris Anderson: git log -S traces the native WinUI A2UI renderer and MCP hardening work that introduced substantial CanvasCapability and NodeService behavior around the affected surface. (role: feature-history owner; confidence: medium; commits: 9fa43f347703; files: src/OpenClaw.Tray.WinUI/Services/NodeService.cs, src/OpenClaw.Shared/Capabilities/CanvasCapability.cs)
  • Scott Hanselman: recent history shows adjacent Slopwatch hardening changes in NodeService and CanvasWindow shortly before this PR, making this a useful security-adjacent review route. (role: recent adjacent hardening contributor; confidence: low; commits: d23f8ca50013; files: src/OpenClaw.Tray.WinUI/Services/NodeService.cs, src/OpenClaw.Tray.WinUI/Windows/CanvasWindow.xaml.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.

Re-review progress:

@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:

@liorb-mountapps

Copy link
Copy Markdown
Author

@clawsweeper re-review

Retrying now that the previous ClawSweeper runs failed before reviewing the updated commit. The PR body includes the review fix, behavior tests, validation, and live Windows/WebView proof.

@clawsweeper

clawsweeper Bot commented Jun 10, 2026

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

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

Re-review progress:

@clawsweeper clawsweeper Bot added the merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. label Jun 10, 2026
@liorb-mountapps liorb-mountapps force-pushed the codex/fix-canvas-navigate branch from 091d7aa to a6fa10a Compare June 10, 2026 10:24
@liorb-mountapps

Copy link
Copy Markdown
Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jun 10, 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.

Re-review progress:

@liorb-mountapps liorb-mountapps force-pushed the codex/fix-canvas-navigate branch from a6fa10a to 764265d Compare June 10, 2026 11:51
@liorb-mountapps

Copy link
Copy Markdown
Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jun 10, 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.

Re-review progress:

@clawsweeper clawsweeper Bot added rating: 🌊 off-meta tidepool PR readiness rating does not apply to this item. and removed 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. labels Jun 10, 2026
@liorb-mountapps

Copy link
Copy Markdown
Author

@clawsweeper re-review

@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed rating: 🌊 off-meta tidepool PR readiness rating does not apply to this item. labels Jun 11, 2026
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. 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. proof: sufficient Contributor real behavior proof is sufficient. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant