fix(runtime): guard and attribute remote destructive closes#8888
fix(runtime): guard and attribute remote destructive closes#8888bbingz wants to merge 4 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds 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)
✅ Passed checks (4 passed)
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. Comment |
774cd39 to
811c765
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 93b0880d-936a-4472-8bb6-1b8f6e3a019b
📒 Files selected for processing (13)
src/main/daemon/daemon-server.test.tssrc/main/daemon/daemon-server.tssrc/main/runtime/rpc/core.tssrc/main/runtime/rpc/dispatcher.tssrc/main/runtime/rpc/methods/session-tabs-schemas.tssrc/main/runtime/rpc/methods/session-tabs.tssrc/main/runtime/rpc/methods/terminal.tssrc/main/runtime/rpc/runtime-close-attribution.tssrc/main/runtime/rpc/runtime-close-intent-schema.tssrc/main/runtime/rpc/runtime-close-policy.tssrc/main/runtime/rpc/runtime-terminal-close-policy.test.tssrc/main/runtime/rpc/session-close-attribution.test.tssrc/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
|
Follow-up
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; 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 |
Summary
Refs #8871. This is the host-side defense-in-depth companion to #8872.
session.tabs.close,terminal.close, andterminal.closeTabwhen a paired desktop/runtime client omits explicit close intent; the RPC succeeds but no host tab or PTY is destroyedclient-created-rollbackonly for a terminal created by the same connection within a short ownership windowsession-killedlogsScreenshots
No visual change.
Testing
pnpm lint— exact changed-fileoxlintand repository pre-commit lint/React Doctor checks pass; max-lines ratchet passes; remaining full verify chain deferred to CIpnpm typecheck— node, CLI, and web tsconfigs passpnpm 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 CIThe 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.clientIdcontains the E2EE bearer token and is never read by the attribution path. Only the non-sensitivedeviceIdis 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.