Skip to content

fix(runtime): guard and attribute remote destructive closes#8888

Open
bbingz wants to merge 4 commits into
stablyai:mainfrom
bbingz:fix/session-close-kill-attribution
Open

fix(runtime): guard and attribute remote destructive closes#8888
bbingz wants to merge 4 commits into
stablyai:mainfrom
bbingz:fix/session-close-kill-attribution

Conversation

@bbingz

@bbingz bbingz commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Summary

Refs #8871. This is the host-side defense-in-depth companion to #8872.

  • soft-deny session.tabs.close, terminal.close, and terminal.closeTab when a paired desktop/runtime client omits explicit close intent; the RPC succeeds but no host tab or PTY is destroyed
  • validate the intent source and target, deduplicate request ids across reconnects, rate-limit destructive actions per paired device, and reject lifecycle/mirror-only sources
  • permit client-created-rollback only for a terminal created by the same connection within a short ownership window
  • preserve legacy mobile, local CLI, automation, SSH, and in-process close behavior
  • retain recent subscription evidence and persist allowed/blocked close decisions with device, connection, target, source, and reason
  • include the requesting local daemon-control client id in session-killed logs
  • never log the paired device bearer token

Screenshots

No visual change.

Testing

  • pnpm lint — exact changed-file oxlint and repository pre-commit lint/React Doctor checks pass; max-lines ratchet passes; remaining full verify chain deferred to CI
  • pnpm typecheck — node, CLI, and web tsconfigs pass
  • pnpm test — focused RPC/runtime/daemon/CLI suites pass (7 files / 112 tests); final policy, attribution, and daemon rerun passes (3 files / 42 tests)
  • pnpm build — not run locally; deferred to CI
  • Added or updated high-quality tests that would catch regressions

The failing-first tests proved that a legacy runtime client still invoked all three destructive methods before this patch. Coverage now includes explicit user closes, target mismatch, schema rejection, duplicate ids across reconnects, same-connection rollback ownership, forged rollback, per-device rate limiting, recent-attach evidence, lifecycle-only intent, mobile compatibility, attribution success/failure, token exclusion, and daemon requester logging.

AI Review Report

The review traced the destructive call chain from authenticated WebSocket context through dispatcher schemas, runtime tab/terminal handlers, PTY controller shutdown, and daemon logging. It rejected the initial attribution-only design because it diagnosed the next incident but did not stop it. It also rejected using client wall-clock age as an authorization gate: macOS, Linux, Windows, WSL, and SSH hosts can have clock skew, so replay protection uses device-scoped request-id dedupe instead.

The final policy is default-deny only for paired desktop/runtime clients. Mobile and in-process callers retain the established contract. The client-created rollback exception is connection-scoped and consumed once. Bulk closes have a high enough rate ceiling for normal UI use, while repeated destructive RPCs remain bounded.

Cross-platform review found no shortcut, label, shell, path-separator, Git, or platform-specific API changes. The policy and intent are transport-neutral JSON and apply equally to macOS, Linux, Windows, WSL, and SSH-backed runtime hosts.

Security Audit

Close intent is not an authentication credential and does not elevate a client; the existing paired-device scope remains the trust boundary. The intent only distinguishes an explicit destructive action from lifecycle/reconciliation noise. Untrusted fields are bounded and parsed with Zod, targets must match the RPC, duplicate ids are suppressed per device, and rollback ownership is tied to the creating connection.

RpcContext.clientId contains the E2EE bearer token and is never read by the attribution path. Only the non-sensitive deviceId is persisted, with a regression test searching the complete serialized trace. The daemon-side client id names a local control connection, not a paired-device credential. No command execution, filesystem path, dependency, secret, or auth mechanism changed.

Notes

Older runtime clients will receive a successful no-op for destructive close requests until upgraded to a client that sends intent (#8872). This intentionally favors preserving host processes over honoring ambiguous legacy close requests. Mobile and local CLI behavior is unchanged.

@coderabbitai

coderabbitai Bot commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 5a99f6e3-4d7c-4fcf-9e6e-35c48e76269a

📥 Commits

Reviewing files that changed from the base of the PR and between d01308b and b132352.

📒 Files selected for processing (2)
  • src/main/runtime/rpc/methods/terminal.ts
  • src/main/runtime/rpc/terminal-multiplex.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/main/runtime/rpc/terminal-multiplex.test.ts
  • src/main/runtime/rpc/methods/terminal.ts

📝 Walkthrough

Walkthrough

Adds runtime close intent schemas, an in-memory authorization policy, decision-based terminal and session-tab teardown, attachment and creation tracking, and persisted close attribution spans. RPC dispatch now forwards device identity and the shared policy to handlers. Tests cover allowed and blocked closes, malformed or mismatched intents, replay detection, rollback ownership, rate limiting, lifecycle sources, tracing outcomes, token redaction, and local identity fallbacks. Daemon session-killed logs now include the requesting control client identifier.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 11.76% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed It clearly summarizes the main change: guarding and attributing destructive runtime closes.
Description check ✅ Passed All required template sections are present and substantively filled, including testing, review, security, and notes.
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.

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.

@bbingz
bbingz force-pushed the fix/session-close-kill-attribution branch from 774cd39 to 811c765 Compare July 16, 2026 14:47
@bbingz bbingz changed the title fix(runtime): record client identity on session.tabs.close and daemon kills fix(runtime): guard and attribute remote destructive closes Jul 16, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 93b0880d-936a-4472-8bb6-1b8f6e3a019b

📥 Commits

Reviewing files that changed from the base of the PR and between 774cd39 and 811c765.

📒 Files selected for processing (13)
  • src/main/daemon/daemon-server.test.ts
  • src/main/daemon/daemon-server.ts
  • src/main/runtime/rpc/core.ts
  • src/main/runtime/rpc/dispatcher.ts
  • src/main/runtime/rpc/methods/session-tabs-schemas.ts
  • src/main/runtime/rpc/methods/session-tabs.ts
  • src/main/runtime/rpc/methods/terminal.ts
  • src/main/runtime/rpc/runtime-close-attribution.ts
  • src/main/runtime/rpc/runtime-close-intent-schema.ts
  • src/main/runtime/rpc/runtime-close-policy.ts
  • src/main/runtime/rpc/runtime-terminal-close-policy.test.ts
  • src/main/runtime/rpc/session-close-attribution.test.ts
  • src/main/runtime/runtime-rpc.ts
🚧 Files skipped from review as they are similar to previous changes (3)
  • src/main/daemon/daemon-server.test.ts
  • src/main/runtime/runtime-rpc.ts
  • src/main/runtime/rpc/core.ts

Comment thread src/main/runtime/rpc/methods/session-tabs.ts
Comment thread src/main/runtime/rpc/methods/terminal.ts Outdated
Comment thread src/main/runtime/rpc/runtime-close-policy.ts Outdated
@bbingz

bbingz commented Jul 16, 2026

Copy link
Copy Markdown
Contributor Author

Follow-up d01308b3a addresses all three actionable review findings:

  • records worktrees first observed through later session.tabs.subscribeAll updates
  • records terminal attachment evidence only after a usable live stream is established, with negative abort/scrollback-only coverage and positive multiplex coverage
  • replaces global live-entry eviction with actor-partitioned TTL maps that fail closed at per-actor capacity without weakening another device's replay or rollback protection

Verification after the follow-up: 10 related test files / 161 tests passed; full node/CLI/web typecheck passed; exact changed-file oxlint passed; max-lines ratchet passed; git diff --check passed.

The remaining CodeRabbit docstring-coverage item is a non-blocking style warning rather than a functional finding. I kept the repository convention of a concise Why: comment on the non-obvious actor-capacity boundary instead of adding boilerplate docstrings to private/internal helpers.

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.

2 participants