Menu ▾ ▴

#72 fix(lan,settings): cut in-flight sockets on device revoke or repo change, always show revocable device list

closed
nobody
bug (36)
2026-07-18
2026-07-18
Anonymous
No

Originally created by: theBGuy

This change hardens the LAN companion device security model by actively terminating any in-flight WebSocket monitor connections whenever sharing is disabled, the shared repo is changed, or a device is revoked. It also ensures that the list of revocable devices is always visible in the settings, even when sharing is off, so users can review and revoke access at any time.

LAN server/cut-socket lifecycle

  • Adds monitor_cut broadcast channel to src-tauri/src/lan/mod.rs, src-tauri/src/lan/auth.rs, and src-tauri/src/lan/server.rs. This channel is fired on:
  • Disabling LAN sharing
  • Actual changes to the shared repo path
  • Revoking a device
  • Mode switch rebinds
  • Tunes lifecycle methods to fire monitor_cut.send(()) only on actual state changes (set_active_repo), so healthy sockets aren't severed on repeats.
  • Refactors device revocation in src-tauri/src/lan/mod.rs so that on successful device revoke, all in-flight monitor connections are forcibly closed.
  • The cut is coarse (all connections); per-device cuts are left for later.
  • Updates pair submission in src-tauri/src/lan/auth.rs to atomically verify then consume pairing sessions, closing a rare double-mint race on concurrent submissions.
  • Expands unit tests and comments in src-tauri/src/lan/mod.rs to verify and document these lifecycle events.

WebSocket monitor handling

  • In src-tauri/src/lan/routes/reviews.rs, updates the monitor stream upgrade logic so each socket also listens for the monitor_cut signal, closing itself when triggered (on repo switch, sharing off, or revoke).
  • Detailed comments clarify why and how in-flight monitors are actively terminated.

Companion device settings UI

  • Changes src/features/settings/CompanionSection.tsx so the list of paired devices is always visible and up to date, regardless of whether sharing is currently enabled (useLanDevices({ enabled: true })).
  • UI copy and explanations are added to make it clear that tokens persist while sharing is off, and existing device tokens must remain revocable at all times.
  • Fixes focus handling and roving keyboard navigation for the device list to support the always-on view.

Related

Tickets: #70
Tickets: #72
Tickets: #75

Discussion

  • Anonymous

    Anonymous - 2026-07-18

    Originally posted by: theBGuy

    🤖 GitDesktop AI security audit · opus · automated

    No security issues found. The changes are defensive hardening — atomic verify-and-consume of the pairing session (closing a double-mint token race, fail-closed on persist failure), a broadcast cut signal that severs in-flight WebSocket monitors on disable/repo-change/device-revoke (closing the "auth only at upgrade time" gap), and a local-only frontend change to always show the revocable device list. No new attacker-controlled path to a sensitive sink is introduced.

    Posted by GitDesktop — AI output, verify before acting on it.

     

  • Anonymous

    Anonymous - 2026-07-18

    Originally posted by: theBGuy

    🤖 GitDesktop AI review · opus · automated

    This is a well-reasoned security-hardening change: the monitor_cut broadcast cleanly severs hijacked WebSockets on every lifecycle transition (disable, mode-switch rebind, real repo change, revoke), the change-detection in set_active_repo correctly avoids severing healthy streams on no-op re-pushes, and the pair_submit atomic verify-then-consume genuinely closes the double-mint race. The reasoning that a reconnecting phone won't be re-cut by a stale signal (a fresh subscribe() only sees sends after it) holds. No blockers — the design is sound and the new unit tests are on point.

    Tests

    • should-fix — src-tauri/src/lan/routes/reviews.rs, forward_stream cut branch. The central behavior of this PR — an in-flight socket actually closing on a cut — is verified only "by inspection" (per the NOTE at mod.rs:885). The tests cover that the desktop fires the cut (set_active_repo_cuts_monitors_only_on_change, revoke_device_cuts_monitors_on_success), but not that the pump reacts. The documented reason (the WebSocketUpgrade extractor rejects a non-upgrade GET under tower::oneshot) is real, but it only blocks the in-memory router path, not an end-to-end one: server::start binds a real listener, so a #[tokio::test] could enable the server, pair, open a real WS to a stream via tokio-tungstenite, fire a revoke/disable, and assert the client receives a Close. For a change whose whole point is severing sockets, that end-to-end assertion is worth adding.

    Edge cases

    • nit — src-tauri/src/lan/auth.rs, pair_submit. Consuming the session inside the lock before persist_device changes the failure semantics: previously a persist_device error (e.g. a transient disk-write failure on lan-devices.json) left the session live so the phone could resubmit the same proof and succeed; now the session is already take()n, so that same disk error mints nothing and kills the session — the user must generate a fresh PIN via "Start again". The security rationale is sound for the concurrent case, but on a single-submission persist failure there's no racer, so this is stricter than the race strictly requires. The comment documents the choice, so if that's intended, fine — flagging only so it's a conscious call.

    • nit — src-tauri/src/lan/mod.rs, revoke_device. The cut is global, so revoking device A also severs device B's healthy live monitor, forcing B to reconnect. This is explicitly documented as deferred ("per-device cut tracking is deferred") and is acceptable for v1; noting it only because with several paired phones it's a visible blip for innocent devices.

    Posted by GitDesktop — AI output, verify before acting on it.

     

  • Anonymous

    Anonymous - 2026-07-18

    Originally posted by: theBGuy

    Context: this PR carries the round-6 review fixes from [#70] (slice 1 was squash-merged into epic/lan-companion mid-review-round, so the remaining fixes land here instead of on that thread). Dispositions for the round-6 findings, verified against the source before fixing:

    1. Live WebSocket monitors survived Stop-sharing (should-fix, accepted + fixed here). Confirmed: forward_stream held only the broadcast receiver — an accepted socket outlived lan_disable (upgraded sockets are hijacked from the serve loop) and any repo switch. Fix: a monitor_cut broadcast on LanState, subscribed per-socket before the upgrade and selected on in the pump — any recv result severs the socket with a Close frame. It fires on disable, the enable mode-switch rebind, an actual active-repo change (compared before overwrite; the App effect re-pushes the same value freely), and — one deliberate addition beyond the finding — device revoke: bearer auth runs at upgrade time only, so a revoked device's live socket was the same hole class. The revoke cut is coarse (severs all monitors; still-valid phones reconnect and re-authorize) — per-device connection tracking is deliberately deferred. Tests pin the fire points (change fires / same-value doesn't / revoke-success fires / revoke-error doesn't); the select branch itself isn't reachable via tower::oneshot (WebSocketUpgrade rejects non-upgrade requests during extraction — same constraint documented for the round-5 scoping tests).

    2. pair_submit double-mint race (should-fix, accepted + fixed here). Confirmed the structure matched the finding: proof verified after the pairing lock released, session cleared in a second acquisition after minting. Now the session check, proof computation, constant_time_eq, and guard.take() (consumption) are one atomic locked block, with mint + persist outside the lock — a racing duplicate submission gets pairingInactive. Fail-closed on the rare persist error: the consumed session is not restored (the phone re-pairs via "Start again"); a comment states that's deliberate. Rate-limit bookkeeping unchanged on every path. Test: replaying the same correct proof after a successful pair now 403s and device count stays 1.

    3. Paired devices invisible while sharing is off (nit, accepted + fixed here). The device list was gated on sharing being enabled, while tokens persist — standing access the user couldn't see or revoke, contradicting the help guide's "revoke any of them at any time." The list and revoke flow now render regardless of sharing state (the device store is a local read; no server needed), with a muted note when sharing is off. The guide's claim is now true as written.

    4. README/site docs-sync — the recorded decision stands (fifth raise, see [#70] rounds 1/3/4/5): public docs land with the final epic→master PR; docs written against this slice would describe a bundle-less preview and be rewritten when the actual companion app ships in a later slice.

    Gates on this PR's content: cargo test 754/754 (three new tests), clippy --all-targets -- -D warnings clean, tsc -b clean, touched frontend file format-verified as CI sees it. The changelog fragment amendment in this PR reflects behavior this PR made true (device management while sharing is off; live disconnect on stop/switch/revoke).

    Posted by GitDesktop — automated agent comment, verify before acting on it.

     

    Related

    Tickets: #70

  • Anonymous

    Anonymous - 2026-07-18

    Originally posted by: theBGuy

    Round-1 triage — one accept (fixed), two design choices confirmed on the record. Fixes land in the next push.

    1. End-to-end cut test — accepted, added. The critique was fair: the PR's central behavior was inspection-verified only, and the suggested technique works. New live_websocket_monitor_is_severed_by_the_cut_signal (src-tauri/src/lan/mod.rs): binds a real listener via server::start (loopback; the port scan absorbs a busy default port), seeds a paired device and connects a real WebSocket with its bearer (tokio-tungstenite, added as a dev-dependency, version cargo-resolved), first proves the pump is live (one ReviewEvent through the registry → one Text frame with its JSON), then fires monitor_cut and asserts the next thing the client sees is a Close/stream-end — never another Text frame (the tolerance covers stacks that surface a server close as stream-end; it cannot mask a still-forwarding socket). Every receive is bounded by a 5s timeout so the test fails rather than hangs. Ran twice — no flakiness. The NOTE block claiming inspection-only coverage is updated.

    2. pair_submit fail-closed on persist failure — intended, confirmed. Consuming the session before persist was the deliberate call (as the in-code comment records): never leave a session live after its proof has been accepted once. The cost — a rare transient disk error forcing one "Start again" — is the right trade against a session that has already had a correct proof observed remaining redeemable. No change.

    3. Global revoke cut — intended for v1, confirmed. Per-device connection tracking is deliberately deferred (documented at the fire point); an innocent phone's monitor reconnects and re-authorizes in one round-trip. If multi-device usage materializes in the companion-app slice, per-device tracking rides along there.

    Gates after the round: cargo test 755/755 (the E2E test included, run twice), clippy --all-targets -- -D warnings clean, tsc -b clean, the touched frontend file format-verified as CI sees it.

    Posted by GitDesktop — automated agent comment, verify before acting on it.

     

  • Anonymous

    Anonymous - 2026-07-18

    Originally posted by: theBGuy

    🤖 GitDesktop AI review · opus · automated

    This is a well-targeted security-hardening change: monitor_cut is subscribed per-socket before upgrade and selected on in the pump, so in-flight WebSocket monitors are severed on disable, mode-switch rebind, a real active-repo change, and device revoke; pair_submit now verifies-and-consumes the session under one lock to close the double-mint race; and the settings panel always shows the revocable device list with a skeleton to avoid an empty-state flash. The design is sound, the change detection correctly avoids cutting healthy streams on no-op re-pushes, and reconnecting phones won't be re-cut by a stale signal (a fresh subscribe() only sees later sends). No blockers.

    Resolved since last review

    • The previous should-fix (the socket-actually-closing behavior was verified only "by inspection") is resolved: live_websocket_monitor_is_severed_by_the_cut_signal in mod.rs binds a real listener via server::start, connects a real tokio-tungstenite WebSocket with a paired-device bearer, proves the pump forwards one event, then fires monitor_cut and asserts the client sees a Close/stream-end and never another Text. The cut half of forward_stream's select! is now genuinely exercised end-to-end, and the NOTE block is updated to match.
    • Copilot flagged the empty-state flash at CompanionSection.tsx:169 (pending query → deviceList [] → misleading "No devices paired" copy). Resolved: the new devices.isPending branch renders a Skeleton ahead of the empty-state ternary.

    Confirmed-deliberate (not re-raising)

    • auth.rs pair_submit — consuming the session before persist_device means a transient lan-devices.json write failure now kills the session (forcing "Start again") even with no racer present. This is documented in the code comment as an intentional fail-closed choice; noting only that the previous nit still describes real behavior, and the call is now explicit.
    • mod.rs revoke_device — the cut is global, so revoking device A also severs device B's healthy monitor. Documented as deliberately coarse ("per-device cut tracking is deferred"); acceptable for v1.

    Nits

    • nit — src-tauri/Cargo.toml, [dev-dependencies]. tokio-tungstenite = "0.30.0" resolves to a second copy alongside the 0.29.0 that axum already pulls in (visible in the Cargo.lock diff — two tokio-tungstenite, tungstenite, and sha1 entries), so the test build compiles both trees. It's dev-only and doesn't touch the release binary, but pinning the dev-dep to 0.29 would reuse axum's copy; the test's APIs (IntoClientRequest, Message, connect_async, StreamExt) exist in both. Purely optional.

    Posted by GitDesktop — AI output, verify before acting on it.

     

  • Anonymous

    Anonymous - 2026-07-18

    Originally posted by: theBGuy

    Round-2 triage — the single nit is confirmed and applied; everything else in the round closed prior items.

    Duplicate tungstenite tree — accepted, fixed in the next push (applied directly as an orchestrator trivial fix, disclosed): verified in Cargo.lock that tokio-tungstenite 0.30 had landed alongside the 0.29 axum's ws feature already pulls (duplicating tungstenite and sha1 too). The dev-dependency is now pinned "0.29" with a comment explaining the deliberate not-latest choice; the lock resolves to a single tokio-tungstenite/tungstenite/sha1 tree, and the full suite (755/755, E2E cut test included) plus clippy --all-targets -- -D warnings pass against 0.29's APIs — exactly as the finding predicted.

    With that, every item from both rounds is fixed, confirmed-deliberate, or closed on the record.

    Posted by GitDesktop — automated agent comment, verify before acting on it.

     

  • Anonymous

    Anonymous - 2026-07-18

    Originally posted by: theBGuy

    🤖 GitDesktop AI review · opus · automated

    This is a sound, well-scoped security-hardening change: monitor_cut is subscribed per-socket before upgrade and selected on in forward_stream (which I confirmed breaks the loop and sends a Close frame on any cut result), so in-flight monitors are severed on disable, mode-switch rebind, a real active-repo change, and device revoke; pair_submit now verifies-and-consumes the session under one lock; and the settings panel always renders the revocable device list with a skeleton to avoid an empty-state flash. No blockers.

    Resolved since last review

    • The duplicate tokio-tungstenite dev-dep tree (previous nit) is fixed: Cargo.toml now pins tokio-tungstenite = "0.29" with an explanatory comment, and the Cargo.lock diff collapses to a single tokio-tungstenite/tungstenite/sha1 tree (the 0.30/0.11.0 entries are removed). Confirmed in the current diff.

    Documentation / conventions

    • should-fix — src/features/help/content.ts, "Managing paired devices" section (~lines 1559–1564). This PR meaningfully alters a user-facing surface — the paired-device list is now visible and revocable while sharing is off (previously the query was gated on enabled), and connected phones are now actively disconnected on stop/switch/revoke — but the in-app guide wasn't updated to match. The section still reads as if managing devices happens in the context of an active session and doesn't mention that the list persists while sharing is off or that live connections are cut immediately. The changelog fragment was updated but the guide (a hard "same-change" convention here) was not. Concrete fix: add a sentence to "Managing paired devices" noting the list stays visible/revocable even when sharing is off (mirroring the new in-panel "These devices can connect the next time sharing is on." copy), and that stopping sharing / switching repos / revoking severs any live phone connection right away.

    Edge cases

    • nit — src/features/settings/CompanionSection.tsx, device-list render (lines 166–175). The devices query is now always enabled, so its error path is reachable in more states than before, yet there's no error branch: if lanDevicesList rejects, devices.isPending is false and deviceList is [], so the empty-state copy ("Turn on sharing to pair a device." / "No devices paired yet…") renders even though paired devices may exist and the fetch simply failed. The component already tracks an error state, but only for the enable/disable/revoke mutations. Impact is low (the list reads a local JSON file that rarely errors), so optional — but a devices.isError branch showing a short "Couldn't load paired devices." would avoid the misleading copy.

    Copilot's only remaining flag (the :169 empty-state flash) is genuinely resolved by the new devices.isPending → Skeleton branch; not re-raising it.

    Posted by GitDesktop — AI output, verify before acting on it.

     

  • Anonymous

    Anonymous - 2026-07-18

    Originally posted by: theBGuy

    Round-3 triage — both items accepted and applied (orchestrator-applied small fixes, disclosed; they land in the next push).

    1. Help guide out of sync with the new device-management behavior — accepted, fixed. Fair catch, and correctly distinguished from the README/site epic deferral: the in-app guide is a same-change convention, and this PR did alter the user-facing surface. "Managing paired devices" now states that the list stays visible even while sharing is off (pairings persist across sharing sessions; mirrors the in-panel copy) and that stopping sharing, switching repos, or revoking a device disconnects any live phone immediately. The guide, the in-panel note, and the changelog fragment now describe the same behavior.

    2. devices.isError branch — accepted, fixed. A failed read is not "no devices": an isError arm now renders "Couldn't load paired devices." ahead of the empty-state ternary, so the misleading copy can't appear on a fetch failure. (Kept as muted text like the sibling states; the pending-Skeleton and empty-state branches are unchanged.)

    Verification: tsc -b clean; both touched files format-verified as CI sees them (LF-normalized biome check, byte-identical).

    Posted by GitDesktop — automated agent comment, verify before acting on it.

     

  • Anonymous

    Anonymous - 2026-07-18

    Originally posted by: theBGuy

    🤖 GitDesktop AI review · opus · automated

    Both files check out against the current diff. Here's my assessment.

    This is a sound, well-scoped security-hardening change: a monitor_cut broadcast is subscribed per-socket before upgrade and selected on in forward_stream (any recv result breaks the loop and sends a Close), so in-flight monitors are severed on disable, mode-switch rebind, a real active-repo change, and device revoke; pair_submit verifies-and-consumes the session under one lock (closing the double-mint race, fail-closed on persist error); and the settings panel now always renders the revocable device list with isPending/isError/empty branches. No blockers, and the change is well tested (fire-point unit tests plus a real-socket end-to-end cut test).

    Resolved since last review

    • Help guide out of sync (previous should-fix) — resolved. src/features/help/content.ts "Managing paired devices" now states the list stays visible/revocable while sharing is off and that stopping sharing / switching repos / revoking disconnects a live phone immediately, matching the in-panel copy and changelog fragment.
    • devices.isError branch missing (previous nit) — resolved. CompanionSection.tsx now renders "Couldn't load paired devices." on devices.isError ahead of the empty-state ternary, so a failed read no longer masquerades as "no devices."
    • Copilot's empty-state-flash flag at :169 is genuinely resolved by the new devices.isPending → Skeleton branch; not re-raising.

    Concurrency

    • nit — src-tauri/src/lan/routes/reviews.rs, forward_stream tokio::select! (line 116). The select! has no biased;, so when a monitor_cut recv and a ReviewEvent recv are both ready at the top of a loop iteration, tokio picks a branch at random — meaning a just-revoked/disabled socket can forward one more Text frame before it honors the cut. Impact is negligible (at most one already-buffered event to a device that's being torn down anyway, and the test doesn't exercise a simultaneously-pending event so it stays green), but since the cut is framed as a security sever — especially for revoke — adding biased; and keeping the cut branch first makes the cut take priority on the common "both ready" case. Optional.

    Posted by GitDesktop — AI output, verify before acting on it.

     

  • Anonymous

    Anonymous - 2026-07-18

    Originally posted by: theBGuy

    Round-4 triage — the one optional nit is accepted and applied (orchestrator trivial fix, disclosed; lands in the next push).

    biased; in forward_stream's select! — accepted. The cut branch was already first in the block, so biased; gives the sever strict priority on the both-ready case — a just-revoked or just-disabled socket never forwards one more buffered frame before honoring the cut. Worth taking despite "optional": for the revoke path specifically, the cut is a security sever and one-frame leakage was the only remaining slack. No starvation concern — each loop iteration re-polls the cut first, and event forwarding proceeds normally when no cut is pending. Verified: cargo test 755/755 (E2E cut test included), clippy --all-targets -- -D warnings clean.

    With that, every finding across [#70]'s six rounds and [#72]'s four is fixed, confirmed-deliberate, or closed on the record.

    Posted by GitDesktop — automated agent comment, verify before acting on it.

     

    Related

    Tickets: #70
    Tickets: #72

  • Anonymous

    Anonymous - 2026-07-18

    Originally posted by: theBGuy

    🤖 GitDesktop AI review · opus · automated

    This PR forcibly severs already-upgraded WebSocket monitor sockets when the LAN companion's lifecycle state changes (disable, mode-switch rebind, real repo change, device revoke), makes pair_submit verify-and-consume the pairing session atomically, and keeps the paired-device list visible/revocable while sharing is off. Having read forward_stream, useLanDevices, and the settings panel against the diff, the change is sound and I see no blockers or should-fixes.

    Resolved since last review

    • biased; in forward_stream's select! (previous concurrency nit) — resolved. reviews.rs:120 now has biased; with the cut branch first, so on a both-ready iteration the sever wins over forwarding one more buffered frame. I confirmed the loop break falls through to let _ = socket.send(Message::Close(None)).await; at line 174, which grounds the E2E test's Close-frame expectation.
    • Help guide out of sync (previous should-fix) — resolved. content.ts "Managing paired devices" now states the list stays visible/revocable while sharing is off and that stop-sharing / repo-switch / revoke disconnects a live phone, matching the panel copy and changelog fragment.
    • devices.isError branch missing and empty-state flash on first fetch (Copilot :169, previous nit) — both resolved. CompanionSection.tsx now renders a Skeleton on devices.isPending and "Couldn't load paired devices." on devices.isError ahead of the empty-state ternary, so neither the initial pending fetch nor a read failure masquerades as "no devices." Note useLanDevices defaults enabled ?? true / refetchInterval ?? false, so { enabled: true } fetches once without introducing polling.

    Correctness

    The two substantive backend changes hold up:

    • auth.rs pair_submit — the compare and guard.take() now happen in one critical section, so a second concurrent submission with the same proof finds the session already consumed and hits pairingInactive rather than minting a second token from one PIN. The new sequential replay test asserts 403/pairingInactive and exactly one device, which pins the race closure. Fail-closed-on-persist-error is deliberate and documented.
    • mod.rs set_active_repo compares before overwrite and only fires the cut on a real change, so the App effect re-pushing the same repoPath each render won't sever a healthy stream; revoke_device fires only on Ok. Both are directly unit-tested for fire/no-fire, and the E2E test exercises the real upgraded-socket cut path the oneshot tests can't reach.

    No other issues found; the broadcast capacity (4), lagged-receiver-still-terminates handling, and the subscribe-before-upgrade ordering are all reasoned correctly.

    Posted by GitDesktop — AI output, verify before acting on it.

     

  • Anonymous

    Anonymous - 2026-07-18

    Ticket changed by: theBGuy

    • status: open --> closed
     

Log in to post a comment.

MongoDB Logo MongoDB