fix(control-ui): keep chat UI mounted across transient reconnects#72522
fix(control-ui): keep chat UI mounted across transient reconnects#72522Sanjays2402 wants to merge 3 commits into
Conversation
🔒 Aisle Security AnalysisWe found 2 potential security issue(s) in this PR:
1. 🟡 Sensitive UI data remains visible after disconnect due to sticky `hasEverConnected` gate
DescriptionThe dashboard/chat UI is now gated only when the client is both disconnected and has never connected ( This creates an information disclosure risk in scenarios where disconnects correlate with session invalidation or where an attacker gains brief physical access during a transient disconnect:
Vulnerable logic: if (!state.connected && !state.hasEverConnected) {
return html` ${renderLoginGate(state)} ${renderGatewayUrlConfirmation(state)} `;
}RecommendationIf the login gate is intended to act as a lock screen (hide sensitive UI whenever disconnected), restore that behavior or replace it with a dedicated disconnect/lock overlay. Options:
if (!state.connected) {
return html`${renderLoginGate(state)} ${renderGatewayUrlConfirmation(state)}`;
}
const showLockOverlay = !state.connected;
return html`
${renderDashboard(state)}
${showLockOverlay ? renderDisconnectedOverlay({ onReconnect: state.connect }) : ""}
${renderGatewayUrlConfirmation(state)}
`;Additionally consider clearing or redacting sensitive in-memory state (chat messages, approvals) on disconnects that may indicate session loss, and/or providing a user setting like “Hide content when disconnected”. 2. 🟡 UI auth gate bypass via sticky `hasEverConnected` not cleared on auth-related WebSocket closes without detail code
DescriptionThe UI login/credentials gate is now bypassed whenever In
Impact depends on what the mounted UI reveals when disconnected, but it can result in stale privileged UI/state remaining visible after credentials are no longer valid (auth revocation/expiry), potentially exposing cached sensitive data to anyone with access to the tab. RecommendationTreat auth-related WebSocket close codes/reasons as sufficient to clear Options:
const AUTH_FAILURE_CLOSE_CODES = new Set([4001, 4003]);
if (
(host.lastErrorCode && isAuthFailureDetailCode(host.lastErrorCode)) ||
AUTH_FAILURE_CLOSE_CODES.has(code)
) {
host.hasEverConnected = false;
}
Also add tests that simulate server closes with Analyzed PR: #72522 at commit Last updated on: 2026-04-27T03:28:21Z |
Greptile SummaryThis PR introduces a sticky Confidence Score: 4/5Safe to merge; one P2 concern about the breadth of the auth-code prefix check is non-blocking. Only a single P2 finding: ui/src/ui/app-gateway.ts — the
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } | ||
| return ( | ||
| code.startsWith("AUTH_") || | ||
| code.startsWith("DEVICE_AUTH_") || | ||
| code === ConnectErrorDetailCodes.DEVICE_IDENTITY_REQUIRED || | ||
| code === ConnectErrorDetailCodes.CONTROL_UI_ORIGIN_NOT_ALLOWED || | ||
| code === ConnectErrorDetailCodes.CONTROL_UI_DEVICE_IDENTITY_REQUIRED | ||
| ); |
There was a problem hiding this comment.
startsWith("AUTH_") is broader than "genuine re-auth" failures
The catch-all code.startsWith("AUTH_") clears hasEverConnected for codes that represent transient infrastructure issues, not just credential failures. Specifically AUTH_RATE_LIMITED (temporary server-side block, credentials are valid), AUTH_TAILSCALE_WHOIS_FAILED, AUTH_TAILSCALE_PROXY_MISSING, and AUTH_TAILSCALE_IDENTITY_MISSING can all appear on a reconnect after a brief network blip — they map to conditions the user cannot fix by re-entering credentials. On a Tailscale-auth setup, a momentary WhoIs lookup failure would clear the flag and flash the full credentials gate, reproducing the exact UX regression this PR aims to prevent.
Consider using an explicit allowlist matching the set in gateway.ts isAuthOrPairingError (lines 84–91), which already excludes transient infrastructure codes, rather than the prefix wildcard.
Prompt To Fix With AI
This is a comment left during a code review.
Path: ui/src/ui/app-gateway.ts
Line: 86-93
Comment:
**`startsWith("AUTH_")` is broader than "genuine re-auth" failures**
The catch-all `code.startsWith("AUTH_")` clears `hasEverConnected` for codes that represent transient infrastructure issues, not just credential failures. Specifically `AUTH_RATE_LIMITED` (temporary server-side block, credentials are valid), `AUTH_TAILSCALE_WHOIS_FAILED`, `AUTH_TAILSCALE_PROXY_MISSING`, and `AUTH_TAILSCALE_IDENTITY_MISSING` can all appear on a reconnect after a brief network blip — they map to conditions the user cannot fix by re-entering credentials. On a Tailscale-auth setup, a momentary WhoIs lookup failure would clear the flag and flash the full credentials gate, reproducing the exact UX regression this PR aims to prevent.
Consider using an explicit allowlist matching the set in `gateway.ts` `isAuthOrPairingError` (lines 84–91), which already excludes transient infrastructure codes, rather than the prefix wildcard.
How can I resolve this? If you propose a fix, please make it concise.
|
Good catch on Replaced the prefix wildcards in Added a parameterized test covering all four excluded codes —
Pushed in ff7cee4. |
|
Codex review: needs real behavior proof before merge. Latest ClawSweeper review: 2026-05-20 19:35 UTC / May 20, 2026, 3:35 PM ET. Workflow note: Future ClawSweeper reviews update this same comment in place. How this review workflow works
Summary Reproducibility: yes. at source level. Current main clears PR rating Rank-up moves:
What the crustacean ranks mean
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. Real behavior proof Mantis proof suggestion Risk before merge
Maintainer options:
Next step before merge Security Review findings
Review detailsBest possible solution: Keep the sticky prior-hello approach only after it resets for every current nonrecoverable auth/scope failure, has regression coverage, is rebased, and has redacted browser proof. Do we have a high-confidence way to reproduce the issue? Yes, at source level. Current main clears Is this the best way to solve the issue? No, not as-is. The sticky prior-hello state is a narrow maintainable direction, but the auth reset contract, branch conflicts, and real behavior proof must be fixed before merge. Label justifications:
Full review comments:
Overall correctness: patch is incorrect Security concerns:
Acceptance criteria:
What I checked:
Likely related people:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 1a7669bc63a0. |
Webchat was flashing the credentials gate on every browser tab refocus or gateway restart because app-render renders the login gate unconditionally on `!state.connected`, and app-gateway sets `connected = false` immediately on any WebSocket close — including expected transient closes. Add a sticky `hasEverConnected` flag on the host that latches to true on the first successful hello handshake and only resets on explicit auth/pairing failures. The renderer now keeps the chat UI mounted during transient reconnects; the existing sidebar status indicator already surfaces the offline state, so users see a brief 'offline' dot instead of the full credentials form. Auth-failure reset uses a small helper that catches AUTH_*, DEVICE_AUTH_*, PAIRING_REQUIRED, DEVICE_IDENTITY_REQUIRED, and the CONTROL_UI_* identity errors so genuine re-auth still surfaces the gate. Adds three tests covering the sticky flag across reconnects, AUTH_* resets, and PAIRING_REQUIRED resets. Fixes openclaw#72500
Replaces broad startsWith("AUTH_") / startsWith("DEVICE_AUTH_") checks in
isAuthFailureDetailCode with an explicit allowlist of detail codes that
represent genuine credential/identity failures.
Transient infra codes (AUTH_RATE_LIMITED, AUTH_TAILSCALE_WHOIS_FAILED,
AUTH_TAILSCALE_PROXY_MISSING, AUTH_TAILSCALE_IDENTITY_MISSING) no longer
clear hasEverConnected — those represent server-side conditions the user
can't fix by re-entering credentials, and treating them as auth failures
would re-flash the credentials gate on a brief WhoIs blip (the exact
regression this PR prevents).
Adds parameterized test covering all four transient infra codes.
Tests: pnpm test ui/src/ui/app-gateway.node.test.ts → 42/42 pass.
Address codex P2 review on openclaw#72522: the previous reset condition only fired when the close carried a structured detail code via isAuthFailureDetailCode. Server-initiated revocation paths (shared-auth rotation, device removal, session revocation) close active sockets with code 4001 + a reason string, and the browser client forwards only {code, reason} for post-hello closes (error is set only for the pending connect error). Without classifying these directly, cached Control UI content survived explicit revocation until the next reconnect outcome. Add isAuthRevocationClose() that matches code 4001 + a lowercased reason substring against an allowlist (gateway auth changed/rotated/revoked, shared auth changed/rotated/revoked, device removed/revoked, session revoked, pairing revoked). Wire it into onClose so hasEverConnected is also dropped on these code-only revocation closes, preserving the existing transient-infra and structured-error paths. Tests cover all 8 revocation reasons (parametrized), plus two defensive cases: (a) a non-revocation 4001 reason stays sticky, (b) a non-4001 close that happens to mention 'device removed' in its transient reason stays sticky. 61 tests pass (was 51; +10 = 8 it.each + 2 single).
ff7cee4 to
aafc245
Compare
|
Thanks for the detailed security review — addressed in aafc245. Fix: Added
Reproduction (codex): post-hello Why substring match (not exact equality): the gateway emits reason variants ( Regression tests added in
Acceptance criteria run locally:
Updated changelog entry to mention the revocation-close handling. Open question for maintainers: if there are other |
clawsweeper
Bot
added
rating: 🦪 silver shellfish
openclaw-barnacle
Bot
added
the
triage: needs-real-behavior-proof
|
ClawSweeper PR egg 🎁 Pass real behavior proof to wake the egg and unlock a hatchable treat. Where did the egg go?
|
|
Closing — stale (15+ days). Will reopen / resubmit if still relevant. |
1 participant
Fixes #72500.
Problem
Webchat flashes the full credentials gate every time the browser tab loses and regains focus. On slower hardware (Pi-class) the flash lasts long enough that it looks like a forced logout. Same flash happens on any transient WebSocket close — gateway restart, network blip, brief sleep.
Root cause is exactly what @brandco diagnosed:
app-render.tsrendersrenderLoginGate(state)unconditionally on!state.connected.app-gateway.tsflipsconnected = falseimmediately on every WebSocket close, including expected transient closes when the tab backgrounds.Fix
Add a sticky
hasEverConnectedflag on the gateway host (and onAppViewState/ the Lit element). It latches totrueon the first successful hello handshake. The renderer now only shows the credentials gate when the user has both never connected AND is currently disconnected:For genuine re-auth (token rotated, password changed, pairing revoked) the flag is cleared in the
onClosehandler when the close detail code matches an auth/pairing failure. New helperisAuthFailureDetailCode()catchesAUTH_*,DEVICE_AUTH_*,PAIRING_REQUIRED,DEVICE_IDENTITY_REQUIRED, and theCONTROL_UI_*identity errors fromconnect-error-details.ts.Plain transport closes (1006, 1012 service restart, network blips) keep the flag set, so the chat UI stays mounted. The existing sidebar
renderSidebarConnectionStatusalready surfaces the offline state via the status dot, satisfying the "non-blocking reconnecting indicator" expectation in the issue without adding a new banner.Tests
Three new tests in
app-gateway.node.test.ts:1006closes and reconnect cycles.AUTH_TOKEN_MISMATCHclose clears the flag → gate reappears.PAIRING_REQUIREDclose clears the flag.Diff
5 files, +115/-1.
ui/src/ui/app-gateway.tshasEverConnectedtoGatewayHost; set inonHello; conditionally clear inonClose; newisAuthFailureDetailCodehelper.ui/src/ui/app-render.ts!connected && !hasEverConnected.ui/src/ui/app-view-state.tshasEverConnectedto view state.ui/src/ui/app.ts@state() hasEverConnected = falseto the Lit element.ui/src/ui/app-gateway.node.test.tsOut of scope
The issue also suggested a 1–2s debounce on the
connected = falsetransition. That's not needed here — the sticky flag already absorbs the transition without any timing dependency, which is more robust on Pi-class hardware where reconnect latency varies wildly.cc @brandco — would appreciate confirmation this fixes the flash on your Pi 5 setup.