Repository navigation
Conversation
Signed-off-by: Sebastien Tardif <SebTardif@ncf.ca>
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
|
Codex review: needs real behavior proof before merge. What this changesThe branch routes SSH dashboard launches through one-use local links and an HTTP and WebSocket proxy that checks tunnel ownership. Example: Open the dashboard for wss://gateway.example/mount/?view=compact#section through local SSH port 45678
Review scores
ProductKind: Bug fix · Worth it: Yes Merge readiness⛔ Blocked before merge - 9 items remain This PR addresses a real dashboard-routing defect, but all six previously reported proxy defects remain on the unchanged source tree. Current main does not implement the requested repair. Priority: P1 Before merge
Findings
Tests
Agent review detailsHow this fits togetherWindows Companion opens Gateway dashboards from tray actions, saved gateway rows, and local MCP commands. The changed path selects browser credentials and forwards dashboard traffic through the SSH tunnel. flowchart TD
A[Tray, saved gateway, or MCP action] --> B[Resolve gateway and shared credential]
B --> C[Check owned SSH listener]
C --> D[One-use local link]
D --> E[Browser dashboard]
E --> F[HTTP and WebSocket proxy]
F --> G[SSH forward]
G --> H[Gateway]
Technical reviewBest possible solution: Use one guarded forwarding owner that preserves per-Gateway browser identity, existing preferences, validated TLS and accepted-socket ownership through final I/O. Do we have a high-confidence way to reproduce the issue? Yes, current source establishes the saved-address routing defect and concrete failures in the introduced proxy. No runtime reproduction was executed during this read-only review. Is this the best way to solve the issue? No, the current proxy breaks browser delivery and Gateway isolation; the related guarded-forward implementation offers a narrower direction, subject to its separate port-reuse choice. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 037c17dcb581. Provenance checked
TestingProof path: in-process harness. SecurityNeeds attention: The proxy changes credential isolation and weakens existing transport and browser safeguards; no unrelated supply-chain changes were found. EvidenceSecurity concerns:
What I checked:
Review metrics
LabelsLabel changes: No label changes. Label justifications:
Rating scale6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior. WorkflowClawSweeper edits this one comment on every review. Comment HistoryReview history (18 earlier review cycles; latest 8 shown)
Reviewed October 8, 2026, 6:10 PM ET / 22:10 UTC (Revision 19). |
Require the SSH server port and a verified listener before a dashboard URL can carry a shared token, and read the token from the same gateway record as the tunnel. Signed-off-by: Sebastien Tardif <SebTardif@ncf.ca>
|
Global triage: HOLD_FOR_AUTHOR. Take confidence 25%; recommendation confidence 93%; effort moderate; risk high. Reviewed exact head |
…ecord An SSH dashboard was always appending the shared token and labeling that as the MCP source. The URL now follows the pinned record's credential source, and a pin mismatch returns no token URL. Signed-off-by: Sebastien Tardif <SebTardif@ncf.ca>
|
Thanks, Seb. The new pin checks and truthful credential-source reporting are useful. We independently reviewed the One correction to the earlier precedence feedback: operator WebSocket authentication and browser dashboard authentication use different resolvers. Keeping a device token ahead of a shared token for an operator connection does not mean a browser can authenticate with that device token. The new The small correction is already available in this delta: use Please add a regression that actually invokes the resolver with both a stored device token and a shared token, rather than supplying a preselected source to This is not a request for a new authentication framework or a general token-link redesign. The separate active-record-switch observation already exists at the prior reviewed head, and no specific closed architecture-ledger responsibility was established as newly violated; neither is being added as a new delta blocker. |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5ec217af-d09a-475f-8c7c-1153efc34eae
Resolve the pinned dashboard credential with the existing shared-first HTTP resolver while preserving authorization, pin checks, and the final token gate. Add stored-identity regression coverage and tray, saved-row, and MCP wiring guards. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5ec217af-d09a-475f-8c7c-1153efc34eae
…nnel Signed-off-by: Sebastien Tardif <SebTardif@ncf.ca> # Conflicts: # src/OpenClaw.Connection/InteractiveGatewayCredentialResolver.cs # src/OpenClaw.Tray.WinUI/App.xaml.cs # src/OpenClaw.Tray.WinUI/Pages/ConnectionPage.xaml.cs # tests/OpenClaw.Tray.Tests/AppRefactorContractTests.cs
…the shared authorizer The merge left a direct managed-port check on the pinned dashboard path. HTTP surfaces now ask InteractiveGatewayEndpointAuthorizer, which still applies that check for a non-native gateway and adds native ownership inspection. Signed-off-by: Sebastien Tardif <SebTardif@ncf.ca>
The UI job aborted after five minutes of host inactivity during markdown and workspace layout tests. Those tests are not part of this dashboard change. Signed-off-by: Sebastien Tardif <SebTardif@ncf.ca>
An SSH dashboard address such as wss://host/mount/?view=compact#section now opens on localhost at the forwarded port and keeps the path, query, and fragment. Browser launch goes through GatewayDashboardLauncher, so a failure shows the existing retry dialog and the token URL is not logged. The mounted-address resolver case passed. build.ps1 exit 0. Shared 4263 passed, 33 skipped. Tray 4000 passed. Signed-off-by: Sebastien Tardif <SebTardif@ncf.ca>
The listener check that builds a dashboard link can be stale by the time the browser starts. The tray, the saved-row action, and local MCP check the owned listener again immediately before launch or before returning the link. A failed recheck does not open the browser and does not return the URL. ./build.ps1 passed. Shared 4263 passed, 33 skipped, 4296 total. Tray 4001 passed. Signed-off-by: Sebastien Tardif <SebTardif@ncf.ca>
The ownership recheck still returned a reusable token fragment aimed at the tunnel port. SSH dashboard opens and local MCP now issue a loopback page with no credential. That page writes the token only after a fresh ownership check, and a second request does not. ./build.ps1 passed. Shared 4263 passed, 33 skipped, 4296 total. Tray 4002 passed. Signed-off-by: Sebastien Tardif <SebTardif@ncf.ca>
…d it The one-use page checked whichever gateway was active and treated a gateway with no SSH tunnel as ready. Switching away from the SSH gateway before the browser requested the page could release the first gateway's token. The check now requires the active record to still be the issued gateway and its tunnel. ./build.ps1 passed. Shared 4263 passed, 33 skipped, 4296 total. Tray 4003 passed. Signed-off-by: Sebastien Tardif <SebTardif@ncf.ca>
The one-use page sent the browser to the SSH forward with the token in the address. A process that binds that port after the response can read the fragment. The page now stays on the handoff listener and proxies to the forward only while ownership still holds. ./build.ps1 passed. Shared 4263 passed, 33 skipped, 4296 total. Tray 4003 passed. Signed-off-by: Sebastien Tardif <SebTardif@ncf.ca>
The handoff listener used a new port and closed after two minutes, so a reload lost the Control UI profile and freed that origin. It also sent bytes to the forward before checking that the SSH process still owned the port, and it downgraded loopback TLS to plain HTTP. Launches now share 127.0.0.1:47831 for the life of the tray. Unused links expire. Connected sockets are checked again before any dashboard bytes move, and the upstream scheme follows the saved destination. ./build.ps1 passed. Shared 4263 passed, 33 skipped, 4296 total. Tray 4003 passed. Signed-off-by: Sebastien Tardif <SebTardif@ncf.ca>
The UI job on run 37845784723 aborted because the test host exited. The tests named at the crash were markdown list layout and workspace session layout. Tray, Setup and connect E2E, and the other product checks passed. Signed-off-by: Sebastien Tardif <SebTardif@ncf.ca>
ConcurrentCreationConvergesOnOneCredential failed on run 37848102774 because api-credential.dpapi was in use by another process. UI passed on this tree. The dashboard handoff does not touch that store. Signed-off-by: Sebastien Tardif <SebTardif@ncf.ca>
What Problem This Solves
Open dashboard for an SSH gateway built the browser URL from the saved gateway URL and appended the shared token. The node socket is ws://localhost on the tunnel. A cleartext saved URL carried the token to the remote host.
Why
IsStrongCredentialAllowed treated every SSH record as allowed, so the managed-gateway check was skipped.
User Impact
The dashboard opens on localhost at the forwarded port and keeps the saved path, query, and fragment. The token is appended only to that tunnel URL. A down or different tunnel is refused. A browser failure uses the existing retry dialog and does not log the address.
Evidence
Current head:
18bebb666e67e0edbe236f18fe339bfdd3357f72. It is the same tree ase8f4d2c10ff8f7c74aae24225b4f8d3d242e95c6. UI passed ona6f5e2ed. This tip retriggers CI afterConcurrentCreationConvergesOnOneCredentialhit a file lock on run 37848102774.Dashboard launches share
http://127.0.0.1:47831for the life of the tray. An unused link expires after two minutes. The listener stays up. After the socket to the forward connects, ownership is checked again before any dashboard bytes are copied. The upstream scheme follows the saved destination, including HTTPS and WSS. The live tray trace below is parent60b1cc0e. It was not repeated on this head.The one-use page no longer sends the browser to the SSH forward. The follow-up address stays on the handoff listener, and that listener proxies to the forward only while ownership still holds. The live tray trace below is parent
60b1cc0ee03d04c2389ce820ce10922b4025e2e0, whose first response still named the tunnel address. That live tray was not repeated on this head.The one-use page now keeps the gateway and tunnel that issued the link. If the active gateway changes before the browser requests the page, the token is not released.
An SSH dashboard link issued by the tray, the saved-row action, or local MCP is now
http://127.0.0.1:<port>/d/<nonce>and does not contain the token. The first request writes the real dashboard address only when the owned listener still matches. A second request is HTTP 404 and does not contain the token. After the browser follows that page, its address bar can still show the tunnel URL.Current-head tray process 18376.
ssh.exe21220 was its child and listened on127.0.0.1:45678.winnode --list-toolsincludedapp.dashboard.url. The call returnedhttp://127.0.0.1:63604/d/<nonce>withhasTokenQueryfalse,usesSharedGatewayTokentrue,credentialSourcerecord.SharedGatewayToken, and no token in that URL. The first request returned HTTP 200 and alocation.replacepage. The second request to the same URL returned HTTP 404 with an empty body and no token. After thatssh.exestopped, nothing listened on 45678, and the next call returnedDashboard blocked because the SSH tunnel is not upwith no URL. A foreign process then listened on 45678, and the next call returned the same error with no URL. The browser was not opened. Switching the active gateway was not done on this live tray. The first 200 response body contains the tunnel address, so a browser that follows it can still open that port.The tray, the saved-row action, and local MCP check the owned listener again immediately before the browser launch or before the link is returned. If that recheck fails, the browser is not opened and the link is not returned. A link that was already returned can still be opened later. The live trace below is parent
1d9bbac0e6d18288ecafe6ce4a50552185904778.Isolated current-head tray, process 18504. The active saved gateway URL was
wss://gateway.example/mount/?view=compact#section. Its SSH forward was local port 45678 to the Ubuntu-24.04 gateway port 18789.ssh.exeprocess 30580 was a child of that tray.winnode --list-toolsincludedapp.dashboard.url.winnode --command app.dashboard.urlreturned:curl.exetohttp://127.0.0.1:45678/mount/?view=compactreturned HTTP 200. The same path over https returned no status. The tray log line wasSSH tunnel startedforrootat local port 45678 to remote port 18789. Stoppingssh.exe30580 left nothing listening on 45678. The nextapp.dashboard.urlcall returnedDashboard blocked because the SSH tunnel is not up.and no URL.On head
1d9bbac0e6d18288ecafe6ce4a50552185904778,wss://gateway.example/mount/?view=compact#sectionwith local port 45678 resolves towss://localhost:45678/mount/?view=compact#section. A plainws://gateway.example:18789still resolves tows://localhost:45678../build.ps1exit 0 ond94a9f770c0f1dbf2d5d757aeeae5b3b0e6e4de0. Shared 4263 passed, 33 skipped, 4296 total. Tray 4001 passed, 0 failed. The saved-row menu and a browser window were not opened on this head.Maintainer repair
Normal integration of main
42c562f91a42ae0251ac971c44f6ce0f8028e88dpreserves the original author commits and branch. Repair99ea745923d5d6edf113a8d2bf979f5db85615b1changes only the pinned dashboard credential helper (10 added / 9 removed production lines) and two focused test files.Paired SSH dashboard HTTP authentication now uses the existing shared-first
InteractiveGatewayCredentialResolver.TryResolveRecord, not the device-first WebSocket resolver. Before/after pin checks, existing authorization semantics, owned-listener and endpoint binding, and the final shared-token equality gate remain. Device/bootstrap credentials never enter dashboard URLs. WebSocket precedence is unchanged. The HTTP browser URL builder uses a#tokenfragment, not a token query.Required proof pools
windows-winui-interactive: local MCPapp.dashboard.urlon head1d9bbac0, with the owned SSH listener up and then stopped. Headd94a9f77rechecks that listener immediately before launch or before returning the link. The saved-row menu and a browser window were not opened.Validation
Validated source tree:
beae2a9b7790c4819df441690c6ba6e489dec1a6, repair head99ea745923d5d6edf113a8d2bf979f5db85615b1.Native Windows ARM64, SDK 10.0.400. Isolated tray data, worktree-specific
OPENCLAW_REPO_ROOT, E2E flags off. Each test project was built first so--no-restorecould not silently no-op..\build.ps1dotnet test .\tests\OpenClaw.Shared.Tests\OpenClaw.Shared.Tests.csproj --no-restoredotnet test .\tests\OpenClaw.Tray.Tests\OpenClaw.Tray.Tests.csproj --no-restoredotnet test .\tests\OpenClaw.Connection.Tests\OpenClaw.Connection.Tests.csproj --no-restoredotnet test .\tests\OpenClaw.WinNode.Cli.Tests\OpenClaw.WinNode.Cli.Tests.csproj --no-restoreTRX result entries confirm these counts. Focused Connection resolver/gate/endpoint/credential tests: 48 passed. New tray/saved-row/MCP wiring guards: red 1 failed / 3 passed before the repair; green 4 passed after it. The behavioral cases use a real isolated persisted
DeviceIdentityand production credential resolvers with fabricated tokens, including paired shared-token selection, device/bootstrap exclusion, authorization rejection, and changed-pin/shared-token mismatch controls.Focused full-source rubber-duck review and parent inspection of the exact repair passed with no blockers. Structured autoreview could not start:
python .\.agents\skills\autoreview\scripts\autoreview --mode commit --commit 99ea745923d5d6edf113a8d2bf979f5db85615b1 --engine codex(using the approved Codex 0.156.1 executable) exited 1 before engine invocation. Its bundle scanner rejected plainly synthetic token fixtures and source-field expressions withrefusing to include secret-like content in review bundle; clean or redact commit diff before running autoreview. No scanner was weakened, no source-altering review mirror or alternate engine was used, and no clean autoreview result is claimed. After inspecting these false positives and the full repair, the parent authorized publication as a reviewable maintainer update, not merge/ship approval. The outstanding structured-review and live-proof limitations remain visible.Real behavior proof
1d9bbac0e6d18288ecafe6ce4a50552185904778, process 18504, isolated data directory, Ubuntu-24.04 gateway on port 18789,ssh.exe30580 parented by the tray. The recheck is headd94a9f770c0f1dbf2d5d757aeeae5b3b0e6e4de0.winnode --list-tools. Runwinnode --command app.dashboard.url --params {}. Requesthttp://127.0.0.1:45678/mount/?view=compact. Stop the tray'sssh.exe. Callapp.dashboard.urlagain./mount, queryview=compact, and fragment fieldssectionandtoken. The saved hostgateway.examplewas absent. The token length was 48 and it was a fragment field.usesSharedGatewayTokenwas true. The plain HTTP forward returned 200. After the ownedssh.exeexited, the port was closed and the next call returned the tunnel-down error with no URL. On the new head, a failed ownership recheck returns before the browser delegate runs.