Skip to content

fix(dashboard): Open Dashboard keeps secrets in the wrong part of the URL - #1505

Open
SebTardif wants to merge 5 commits into
openclaw:mainfrom
SebTardif:fix/f075-dashboard-url-join
Open

SebTardif wants to merge 5 commits into
openclaw:mainfrom
SebTardif:fix/f075-dashboard-url-join

Conversation

@SebTardif

@SebTardif SebTardif commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

What Problem This Solves

Open Dashboard previously appended a route to the whole gateway URL, so a base query or fragment swallowed the route. It could also retain userinfo or an old token fragment. A route with its own query later produced two ? delimiters instead of combining the queries.

User Impact

The opened URL has no userinfo or old fragment. The route stays in the path, route query parameters precede gateway query parameters, and only the active shared gateway token appears in the fragment. Literal and percent-encoded token query names from either source are removed, including when no shared token is appended.

Why This Change Was Made

The contributor's URI-based builder separates host, path, query, and fragment. The follow-up patch splits query and fragment from the optional relative route before joining it with the existing gateway path and query; it preserves the contributor's approach and original commits. Seb's c869ed59 commit rejects URI input whose scheme is not http, https, ws, or wss; it retains the query and fragment repair. Merge commit 8ea08dcd integrates then-current main (5a595352) without rewriting Seb's commits. The PR diff against that base is still only the builder and its tests.

Change Type

  • Bug fix
  • Security hardening
  • Tests or validation

Scope

  • Tray or WinUI UX
  • Gateway or connection
  • Permissions, privacy, or security

Required proof pools

  • windows-winui-interactive: Open Dashboard browser launch; the earlier live address-bar observation is from c869ed59. No current-merge-head screenshot or browser launch was collected. The current-head app/MCP proof below is not a substitute for visual browser proof.

Validation

At exact merge head 8ea08dcddb7ddb0a4a7ed92d30bfe260a3057f03 in the isolated worktree on Windows ARM64:

  • .\build.ps1: passed, including documentation validation and full Debug build.
  • dotnet test .\tests\OpenClaw.Shared.Tests\OpenClaw.Shared.Tests.csproj --no-restore: 4104 passed, 35 skipped, 0 failed.
  • dotnet test .\tests\OpenClaw.Tray.Tests\OpenClaw.Tray.Tests.csproj --no-restore: 3120 passed, 0 failed.
  • dotnet test .\tests\OpenClaw.Tray.Tests\OpenClaw.Tray.Tests.csproj --no-restore --filter FullyQualifiedName~GatewayDashboardUrlBuilderTests: 8 passed, 0 failed, including Build_RejectsSchemeLessGatewayInput.
  • git diff --check origin/main..HEAD: passed. Scoped rubber-duck review after merging main: no actionable findings in the builder, tests, or caller contracts.
  • Structured review command python .agents\skills\autoreview\scripts\autoreview --mode branch --base origin/main refused to bundle the synthetic userinfo/token test fixture as secret-like content before reviewer invocation. Its refusal is not a clean autoreview result; the helper was not bypassed or weakened.
  • Local build/tests do not replace required CI. See CI disposition below.

Real behavior proof

  • Current PR head: 8ea08dcddb7ddb0a4a7ed92d30bfe260a3057f03 (parents c869ed59, 5a595352).
  • Current-head isolated app/MCP reproduction: Launched the merged-head WinUI executable with a disposable, preseeded tray profile and dummy shared token. tools/list exposed app.dashboard.url; tools/call with path=config?tab=one&token=old-route#old-route returned the URL shape http://127.0.0.1:43179/ui/config?tab=one&x=1#token=[REDACTED], hasTokenQuery=false, and credentialSource=record.SharedGatewayToken. The input gateway URL also had an encoded token query key and an old fragment; neither appeared in the output. The isolated app process was stopped and the disposable profile removed.
  • Earlier browser launch at c869ed59: Dev-identity Debug tray, disposable profile, dummy shared token. Activated openclaw-dev://dashboard/config with a route query. Microsoft Edge opened. The live address bar read http://127.0.0.1:43179/ui/config?x=1#token=[REDACTED]. The route is on the path, saved gateway query remains, and old token query and fragment are absent. A screen copy was unusable, so none was attached. The protocol delivered only config, not tab=one; the route query composition is covered by the current-head MCP call and unit test.
  • Not verified / blocked: No browser screenshot or direct browser launch on the merge head. The interactive proof pool is outstanding.
  • Scheme-less input: localhost:18789/ui throws instead of parsing as a URI whose scheme is localhost.

Security Impact

No new permissions, capabilities, or network calls. Existing URL credentials are handled more narrowly: userinfo and previous fragments are discarded; literal and percent-encoded token query keys are filtered; the selected shared token is escaped into the single fragment.

Compatibility and Migration

No configuration changes or migration. Existing path-only callers and dashboard root URLs remain supported.

Exact-head CI disposition

Build and Test run 36178424792 at exact head 8ea08dcd finished red on both attempts. The full first attempt passed Setup/connect, Network recovery, Tray/setup/integration, and UI/functional/accessibility, but Revocation recovery E2E failed during wizard Gateway restart. Failed-jobs-only attempt 2 again passed Setup/connect (108222344541) and Network recovery (108222337744), while Revocation (108222289480) failed 1/1 with a slow SQLite coordinator lock wait on state.write, StateDatabaseCoordinatorContentionError: another OpenClaw process owns state-lifecycle, then GatewayRestartPreparationError: GATEWAY_RESTART_PREPARATION_REFUSED. CI Gate (108225835344) failed. This is not a dashboard URL assertion; it is a repeated Gateway state-lifecycle contention failure in the setup/recovery path. The PR is parked with required CI red. No further rerun, admin bypass, or merge is claimed. Resolution requires investigating/clearing that contention and obtaining a green required CI Gate on the current PR head.

- Drop userinfo from the opened dashboard URL
- Stop appending the shared token inside an existing fragment
- Keep the route off the query string

Signed-off-by: Sebastien Tardif <SebTardif@ncf.ca>
@clawsweeper

clawsweeper Bot commented Sep 24, 2026 •

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

Signed-off-by: Sebastien Tardif <SebTardif@ncf.ca>
@shanselman shanselman added the status: 🚢 actively landing A maintainer or agent is actively driving this item through implementation, validation, or merge. label Sep 24, 2026
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: c998b504-ca22-49c5-8712-15da55226686
@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Sep 24, 2026
@clawsweeper

clawsweeper Bot commented Sep 24, 2026 •

Copy link
Copy Markdown

Codex review: blocked before merge. Reviewed September 25, 2026, 3:56 PM ET / 19:56 UTC (Revision 7).

ClawSweeper review

What this changes

The tray now assembles dashboard links from URL components, removes old URL credentials, and tests path, query, fragment, and scheme handling.

Merge readiness

⛔ Blocked before merge - 5 items remain

This PR remains useful because current main still constructs dashboard URLs by appending a route to the whole address. The current patch fixes that behavior, but a previously identified invalid-address recovery defect remains at the unchanged PR head.

Priority: P2
Reviewed head: 8ea08dcddb7ddb0a4a7ed92d30bfe260a3057f03

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) Current-head production-path evidence supports the URL repair, while the invalid-address caller defect and outstanding interactive proof limit merge readiness.
Proof confidence 🐚 platinum hermit (4/6) Sufficient (live_output): At the current head, the contributor reports invoking the running WinUI app through its real app.dashboard.url MCP entry point with an isolated profile and observing a correctly composed URL with old token values removed. An earlier branch head also has a reported browser address-bar observation; direct browser launch on the current merge head remains the declared interactive proof-pool gap. No stored-data contract changes.
Patch quality 🦐 gold shrimp (3/6) 1 actionable review finding remain.

Verification

Check Result Evidence
Real behavior Verified Sufficient (live_output): At the current head, the contributor reports invoking the running WinUI app through its real app.dashboard.url MCP entry point with an isolated profile and observing a correctly composed URL with old token values removed. An earlier branch head also has a reported browser address-bar observation; direct browser launch on the current merge head remains the declared interactive proof-pool gap. No stored-data contract changes.
Evidence reviewed 9 items Introduced validator: The PR adds a scheme check that throws ArgumentException for a nonempty unsupported address.
Tray caller: OpenDashboard builds the URL before the try/catch that handles browser launch failures.
App capability caller: The dashboard URL handler calls the builder without handling its new validation exception.
Findings 1 actionable finding [P2] Handle rejected Gateway URLs at both dashboard callers
Security None None.

How this fits together

The tray takes a saved Gateway address, an optional dashboard route, and the selected credential, then builds a link for the browser or returns it through the local app capability. Those links determine which dashboard page opens and whether a shared token reaches it.

flowchart LR
  A[Saved Gateway address] --> D[Dashboard URL builder]
  B[Dashboard route] --> D
  C[Selected credential] --> D
  D --> E[Validate scheme and remove old credentials]
  E --> F[Browser launch]
  E --> G[Local app capability]
Loading

Before merge

  • Handle rejected Gateway URLs at both dashboard callers (P2) - The new scheme check throws for an unsupported saved address, but credential resolution can still return that nonempty address. OpenDashboard calls this builder before its try/catch, and the app-capability handler also lets the exception escape. Catch the invalid-address case and give the user a Connection settings path or the capability caller a clear error; add focused coverage.
  • Resolve merge risk (P1) - A nonempty legacy or saved Gateway address with an unsupported scheme can now interrupt dashboard opening without a visible recovery path.
  • Resolve merge risk (P1) - The declared interactive browser-launch proof pool has no direct observation from the current merge head.
  • Resolve merge risk (P1) - The required CI Gate remains red on the current head because revocation-recovery E2E fails during Gateway restart; its disposition is still needed before merge.
  • Complete next step (P2) - Handle invalid saved Gateway addresses at both dashboard callers, obtain current-head interactive browser proof, and resolve the required CI Gate before merge.

Findings

  • [P2] Handle rejected Gateway URLs at both dashboard callers — src/OpenClaw.Tray.WinUI/Helpers/GatewayDashboardUrlBuilder.cs:13-17
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production and test delta production +78 net lines, tests +64 lines The production growth implements component-wise URL construction; the new tests cover its principal route and credential cases.

Merge-risk options

Maintainer options:

  1. Make invalid addresses recoverable (recommended)
    Handle builder rejection at the tray and app-capability callers and cover a persisted invalid address in focused tests.
  2. Pause this landing
    Keep the PR open while the dashboard recovery path, interactive proof, and required CI Gate are resolved.
Copy recommended automerge instruction
@clawsweeper automerge

Special instructions:
Handle unsupported saved Gateway URLs at both dashboard callers with a visible Connection settings path or clear app-capability error, and add focused regression coverage.

Technical review

Best possible solution:

Keep the stricter URL construction, make rejected saved addresses lead to Connection settings or a clear app-capability error, and land with current-head browser proof and a passing required CI Gate.

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

Yes for the caller defect from source: a nonempty unsupported saved address passes credential resolution, then the new builder throws before tray recovery handling. I did not execute the Windows app in this read-only review.

Is this the best way to solve the issue?

Yes for the URL component repair, but not yet for caller recovery: handle validation failure at both entry points while retaining the strict scheme check.

Full review comments:

  • [P2] Handle rejected Gateway URLs at both dashboard callers — src/OpenClaw.Tray.WinUI/Helpers/GatewayDashboardUrlBuilder.cs:13-17
    The new scheme check throws for an unsupported saved address, but credential resolution can still return that nonempty address. OpenDashboard calls this builder before its try/catch, and the app-capability handler also lets the exception escape. Catch the invalid-address case and give the user a Connection settings path or the capability caller a clear error; add focused coverage.
    Confidence: 0.95

Overall correctness: patch is incorrect
Overall confidence: 0.94

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against 5a59535216ee.

Labels

Label changes:

No label changes.

Label justifications:

  • P2: The URL repair addresses a bounded dashboard-opening bug, with a recoverability defect still needing correction.
  • merge-risk: 🚨 compatibility: The introduced strict scheme check can reject a saved address that previously reached the dashboard caller.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🐚 platinum hermit and patch quality is 🦐 gold shrimp.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (live_output): At the current head, the contributor reports invoking the running WinUI app through its real app.dashboard.url MCP entry point with an isolated profile and observing a correctly composed URL with old token values removed. An earlier branch head also has a reported browser address-bar observation; direct browser launch on the current merge head remains the declared interactive proof-pool gap. No stored-data contract changes.
  • proof: sufficient: Contributor real behavior proof is sufficient. At the current head, the contributor reports invoking the running WinUI app through its real app.dashboard.url MCP entry point with an isolated profile and observing a correctly composed URL with old token values removed. An earlier branch head also has a reported browser address-bar observation; direct browser launch on the current merge head remains the declared interactive proof-pool gap. No stored-data contract changes.

Evidence

Acceptance criteria:

  • [P1] ./build.ps1.
  • [P1] dotnet test ./tests/OpenClaw.Shared.Tests/OpenClaw.Shared.Tests.csproj --no-restore.
  • [P1] dotnet test ./tests/OpenClaw.Tray.Tests/OpenClaw.Tray.Tests.csproj --no-restore.
  • [P1] dotnet test ./tests/OpenClaw.Tray.Tests/OpenClaw.Tray.Tests.csproj --no-restore --filter FullyQualifiedName~GatewayDashboardUrlBuilderTests.

What I checked:

Likely related people:

  • unknown: The claimed source-line change could not be verified from bounded local history. (role: source history unknown; confidence: low)
  • Scott Hanselman: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • Barbara Kudiess: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Make invalid saved Gateway URLs recoverable at both dashboard callers, with focused regression coverage.
  • Capture a direct Open Dashboard browser result from the current merge head.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
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.

Workflow

  • 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.

History

Review history (6 earlier review cycles)
  • reviewed 2026-09-24T11:18:30.167Z sha 06f243f :: blocked before merge. :: none
  • reviewed 2026-09-24T11:35:25.568Z sha 06f243f :: blocked before merge. :: none
  • reviewed 2026-09-24T21:44:07.614Z sha c869ed5 :: blocked before merge. :: [P2] Handle rejected Gateway URLs at the dashboard caller
  • reviewed 2026-09-25T18:00:33.599Z sha c869ed5 :: blocked before merge. :: [P2] Handle rejected Gateway URLs at both dashboard callers
  • reviewed 2026-09-25T19:06:51.828Z sha c869ed5 :: blocked before merge. :: [P2] Handle rejected Gateway URLs at both dashboard callers
  • reviewed 2026-09-25T19:20:06.592Z sha 8ea08dc :: blocked before merge. :: [P2] Handle rejected Gateway URLs at both dashboard callers

@shanselman shanselman removed the status: 🚢 actively landing A maintainer or agent is actively driving this item through implementation, validation, or merge. label Sep 24, 2026
@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. labels Sep 24, 2026
@karkarl

karkarl commented Sep 24, 2026

Copy link
Copy Markdown
Collaborator

Global triage: TAKE_AFTER_CHECKS. Take confidence 85%; recommendation confidence 90%; effort extra-small; risk low.

Reviewed exact head 06f243f3ab26. The builder keeps the token in the fragment, strips injected token query/fragment values including percent-encoded keys, prevents cross-host route injection, and preserves route queries. Focused and full Tray tests pass. The four red checks match the repository-wide issue #1498 failure, not this two-file change. One low-reach edge remains: a scheme-less gateway input can produce a wrong host, although stored gateway URLs are normalized and require ://.

Owner: maintainer. Complete the declared browser-launch windows-winui-interactive proof, optionally reject non-absolute inputs defensively, then take after the #1498 CI-gate disposition. This PR is independent of #1503 and #1504.

A scheme-less value such as localhost:18789 parses as an absolute URI
with scheme localhost, and the dashboard builder then opened the wrong
host. Accept only http, https, ws, and wss.

Test: GatewayDashboardUrlBuilderTests 8 passed.
Signed-off-by: Sebastien Tardif <SebTardif@ncf.ca>
@SebTardif

Copy link
Copy Markdown
Contributor Author

The scheme-less edge is closed on c869ed59. localhost:18789/ui parsed as a URI whose scheme was localhost, so the builder could open the wrong host. It now accepts only http, https, ws, and wss. GatewayDashboardUrlBuilderTests is 8 passed, including Build_RejectsSchemeLessGatewayInput.

Browser launch on that head: dev-identity Debug tray, disposable profile, dummy shared token, then openclaw-dev://dashboard/config with a route query. Microsoft Edge opened. The live address bar read http://127.0.0.1:43179/ui/config?x=1#token=[REDACTED]. The route is on the path, the saved gateway query x=1 remains, and the old token query and fragment are absent. The shared token is only in the fragment. A screen copy of that window was not usable, so it is not attached.

The protocol delivered the path config and did not include the route query tab=one. That join is covered by the unit test and by the earlier app.dashboard.url call on 06f243f3.

The three red setup E2E jobs on 06f243f3 match #1498. If this head fails the same way, I will not send an empty commit.

@clawsweeper clawsweeper Bot added merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. labels Sep 24, 2026
@shanselman shanselman added status: 🚢 actively landing A maintainer or agent is actively driving this item through implementation, validation, or merge. and removed status: 🚢 actively landing A maintainer or agent is actively driving this item through implementation, validation, or merge. labels Sep 25, 2026
@clawsweeper clawsweeper Bot added the merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. label Sep 25, 2026
@shanselman shanselman added the status: 🚢 actively landing A maintainer or agent is actively driving this item through implementation, validation, or merge. label Sep 25, 2026
Preserve SebTardif's dashboard commits while integrating the latest setup and migration fixes from main.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: c998b504-ca22-49c5-8712-15da55226686
@clawsweeper clawsweeper Bot removed the merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. label Sep 25, 2026
@shanselman shanselman removed the status: 🚢 actively landing A maintainer or agent is actively driving this item through implementation, validation, or merge. label Sep 25, 2026

This branch has not been deployed

No deployments
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. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. 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.

3 participants