Skip to content

fix(ssh): a cancelled or revoked host's browser command stops on the desktop - #30630

Merged
nwparker merged 11 commits into
mainfrom
nwparker/browser-bridge-cancel
Oct 19, 2026
Merged

nwparker merged 11 commits into
mainfrom
nwparker/browser-bridge-cancel

Conversation

@nwparker

@nwparker nwparker commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 6 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​752 $\color{#cf222e}{\Huge{\mathbf{−}}}$​4 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​748
Prod 20 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​633 $\color{#cf222e}{\Huge{\mathbf{−}}}$​204 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​429

ELI5

When an SSH host's orca CLI drives a browser page on the desktop, the user (or orcad) can cancel the command, or the desktop can stop sharing its browser with that host. Before this change, neither one actually stopped the page action. A click waiting in the desktop's queue still ran after it was cancelled, and a click already running still finished after access was revoked. Now a cancelled or revoked host's page actions stop where they are.

What Changed

Before: cancelling an orca browser click on the host only made orcad stop waiting. The desktop never heard about it, so the queued click ran later anyway. Revoking a host's access was only checked when a command started. A command that was already running (its helper process, the CDP proxy, native key input) kept driving the page.

After:

  • Cancel reaches the desktop. orcad sends a new optional cancel event for that request. The desktop aborts that request's signal, so a queued command never starts and a running one is revoked.
  • Revocation is enforced where page actions are actually sent. The choke points are the guest CDP send, the CDP proxy's message intake and debugger channel, and native input and edit commands. A host's command holds the page while it runs, and once that host is revoked every later step is refused. That includes steps its helper sends through the proxy, and proxy input that was waiting on the focus guard when the revocation happened.
  • The revoked command's helper daemon is retired at once. It runs as a separate process that outlives its killed client and keeps retrying, so its page session is retired as soon as revocation fires, while the page is still held. Commands queued behind it still run, on a fresh session, and the page's network intercept routes are kept.
  • Nothing is left pressed. Every step after revocation is refused, including releases. If the revoked command still had a button, key or touch down, the desktop lets go of it itself once the command ends. The mouse is released off the page, so no click or drop completes. Keys are released through the normal frame routing, and touches are cancelled.
  • Desktop-local commands are never refused. A host's scope never leaks into long-lived state. The page's CDP proxy now starts outside any host's scope; before, a host that created the proxy left its scope on every later proxy connection.

Wire compatibility: cancel is gated on a new client capability, browser.desktopBridge.cancel.v1. orcad sends it only to desktops that declare that capability, because an older desktop drops its whole bridge attachment on an event type it can't parse. A new desktop paired with an old orcad never receives cancel. It still gets revocation enforcement locally, and timeouts and detach still abort as before.

Why

Cancellation has to reach the side that executes the action, and revocation has to be checked where actions are sent, not only when they are admitted. Checking at each command's call sites kept missing paths (helper steps through the proxy, deferred input, edit commands). Gating the few sinks every page action passes through covers all of them, including future commands. Releasing held input from the desktop after revocation was simpler than exempting releases at the gates. Exempting them would have let a revoked host keep sending mouseup and keyup events, and it broke frame routing.

ref-oss check: compared with how VS Code remote and Zed propagate cancellation over their remote protocols. The standard model is an optional per-request cancel message that the executing side honours (as in LSP's $/cancelRequest), with new message types negotiated as capabilities so older peers never receive them. This PR uses the same design: an optional per-request cancel event, sent only to desktops that declare browser.desktopBridge.cancel.v1, with revocation enforced at the choke points where page actions are sent. The approach didn't change after the check. The later review rounds hardened the choke points: the scope leak, held-input cleanup, retiring the helper daemon, and deferred proxy input.

Linked Issue

Fixes finding #6 of the SSH hardening Codex review 4 ("Canceled or revoked remote browser actions can still execute", introduced in #29384). That finding has no GitHub issue.

Visual Proof

N/A. No UI changes. The behaviour change is that a cancelled or revoked host's browser action no longer reaches the page.

Testing

  • New tests, each confirmed failing before its fix and passing after:

    • registry cancel emission, gated on the capability
    • the connection aborting a cancelled request
    • a revoked click not pressing, or not hit-testing during the focus wait
    • helper steps refused at the proxy
    • deferred proxy input refused after the hold ends
    • the proxy started outside the host's scope
    • held buttons and keys released off the page after revocation, on window and webview pages
    • a release that failed to land still gets cleaned up
    • the revoked helper daemon retired on the signal, while the page is still held
  • Ran the full static-analysis job locally (all steps of "static analysis and typecheck", including the ratchets), plus the src/main/browser and desktop-browser-bridge test folders single-worker.

  • Verified in unit tests only. Not exercised live against a real SSH host.

  • I manually tested these changes locally

  • Automated tests added/updated, or explained why not below

Review

  • Codex (gpt-6.1-sol, high) rounds 4 to 9, with every real finding fixed:
    • a press delivered before revocation left held
    • the native input guard refusing releases
    • an iframe keyUp misrouted
    • the cleanup needing a debugger lease and a pointer-state sync
    • a release recorded before it landed
    • deferred proxy input resuming without a gate
    • Round 9 was clean.
  • Opus review, every real finding fixed:
    • the host scope leaking into the CDP proxy
    • an unconditional release exemption, then repeated releases
    • a still-running helper daemon after the hold ended
    • the release taking focus without the focus guard
    • intercept routes lost on retire
    • a queue stall until the exec timeout
    • The last round was clean.

Agent skill upstream boundary

  • Not applicable, or this change copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.

Notes

  • Security: a revoked host loses all page access mid-command.
  • SSH and mixed versions: covered by the capability gate above.
  • Cross-platform: no platform-specific code.
  • Refactor: key-event planning moved out of guest-direct-key-input.ts into guest-key-event-plan.ts, unchanged, to stay under max-lines.

Checklist

  • This PR is small and focused
  • I explained what changed and why (ELI5, the user-facing before/after, the mechanism, and why over the alternatives)
  • Before/after screenshots or videos attached for UI changes, or N/A with reason
  • Self-reviewed for correctness, security, and performance
  • Cross-platform, SSH/remote, and path/shortcut impact considered (or N/A)
  • pnpm lint, pnpm typecheck, pnpm test, and pnpm build pass (or CI will cover; local preferred)

…desktop

orcad now tells a cancel-capable desktop when the CLI gave up on a forwarded browser command, and a running host command on the desktop checks revocation before every side-effecting step (each helper CDP message and each direct input dispatch).
… go of the page; release held input with a lease, synced pointer state and the focus guard
…e they land, and a revoked helper is retired at once with its routes kept
@coderabbitai

coderabbitai Bot commented Oct 19, 2026

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 0bf7c8f1-7e7c-499d-8339-d3334889b643
📥 Commits

Reviewing files that changed from the base of the PR and between 5f2bb93 and ed650ff.

📒 Files selected for processing: 26
  • src/main/browser/agent-browser-bridge-execution.ts
  • src/main/browser/agent-browser-bridge-host-revocation.test.ts
  • src/main/browser/agent-browser-bridge-lifecycle.ts
  • src/main/browser/agent-browser-bridge-mouse-commands.ts
  • src/main/browser/agent-browser-bridge-pointer-commands.ts
  • src/main/browser/agent-browser-bridge-queue.ts
  • src/main/browser/agent-browser-bridge-types.ts
  • src/main/browser/agent-input-injection.ts
  • src/main/browser/cdp-debugger-channel.ts
  • src/main/browser/cdp-page-input-commands.ts
  • src/main/browser/cdp-ws-proxy-host-admission.test.ts
  • src/main/browser/cdp-ws-proxy.ts
  • src/main/browser/desktop-browser-bridge/desktop-browser-bridge-connection.ts
  • src/main/browser/desktop-browser-bridge/desktop-browser-bridge-manager.test.ts
  • src/main/browser/guest-cdp-command.ts
  • src/main/browser/guest-direct-key-input.test.ts
  • src/main/browser/guest-direct-key-input.ts
  • src/main/browser/guest-key-event-plan.ts
  • src/main/browser/remote-cli-page-admission.ts
  • src/main/ipc/desktop-browser-bridge-wiring.ts
  • src/main/runtime/desktop-browser-bridge/desktop-browser-bridge-dispatch.test.ts
  • src/main/runtime/desktop-browser-bridge/desktop-browser-bridge-registry.test.ts
  • src/main/runtime/desktop-browser-bridge/desktop-browser-bridge-registry.ts
  • src/main/runtime/desktop-browser-bridge/remote-cli-browser-scope.ts
  • src/main/runtime/rpc/methods/desktop-browser-bridge.ts
  • src/shared/desktop-browser-bridge-protocol.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review.

📝 Walkthrough

The changes add remote browser scope and page-admission checks to browser commands and input dispatch. They track pressed input and attempt releases after host revocation. Queued commands can retain their place while a helper session retires. The desktop browser bridge adds a capability-gated cancel event, and the receiving connection aborts the matching request.

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to ed650

The reported type error and cancellation race do not occur on the supported paths. No actionable merge-blocking issue remains after normal checks.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 56.41% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 39 functions across 26 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Title check Passed The title clearly and concisely summarizes the primary change: cancelled or revoked SSH host browser commands stop on the desktop.
Description check Passed The description is comprehensive and follows the required template. It explains the user impact, implementation, rationale, compatibility behavior, testing, review findings, and checklist status. It r…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Description check

Explanation

The description is comprehensive and follows the required template. It explains the user impact, implementation, rationale, compatibility behavior, testing, review findings, and checklist status. It references a review finding instead of a GitHub issue and does not claim that the full lint, typecheck, test, and build commands passed locally, but these omissions are clearly disclosed.

  • Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 1
📝 Generate docstrings: 💡
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant