From 5df2729e426e96ec424e06f0ff11d4024d1c76ef Mon Sep 17 00:00:00 2001 From: Troy Hoffman Date: Sun, 26 Jul 2026 04:49:08 +0200 Subject: [PATCH] fix(desktop): make backend providers runnable on Windows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `discover_provider_candidates` stripped only the literal `buzz-backend-` prefix and used the remainder as the provider id, so on Windows `buzz-backend-ssh.exe` yielded the id `ssh.exe`. The `.` fails the `[a-z0-9][a-z0-9_-]*` check in `resolve_provider_binary`, which made every provider undeployable on Windows. `is_executable` compounded it: the `cfg(not(unix))` arm returned `true` unconditionally, so any file — a directory entry, a stray text file — was "discovered" as a provider. Introduce an `ExecNaming` seam describing how the platform names an executable: `NoExtension` on unix (the execute bit is the contract) and `Extensions(..)` on Windows. Id derivation and the executable check both read it, so the rule lives in one place and — because the rule is passed in rather than read from `cfg!` at the point of use — the Windows behaviour is unit-testable from a linux host. The Windows allowlist is `PATHEXT` ∩ {exe, com}. `.cmd`/`.bat` are excluded deliberately: `std::process::Command` routes those through `cmd.exe`, which would insert a shell-quoting surface in front of a code path that pipes an agent's nsec over stdin. Intersecting rather than substituting means a user-configured `PATHEXT` can narrow the set but can never re-admit a shell script. An empty Windows allowlist therefore discovers nothing, which is the opposite of the unix arm — hence a two-variant enum rather than a possibly-empty list. Also dedupe on the derived id instead of the file name (on Windows two extensions collapse to one id, and the earlier PATH entry should win) and drop ids that `resolve_provider_binary` would reject, so the catalog never advertises a provider that can never be executed. Same-directory candidates are ordered by their extension's position in `PATHEXT` before deduping, so a directory holding both `buzz-backend-foo.com` and `buzz-backend-foo.exe` selects the file Windows command lookup would run rather than whichever `read_dir` happened to list first. `resolve_command` already appends `EXE_SUFFIX` via `executable_basename`, so PATH lookup of `buzz-backend-ssh` finds the `.exe` unchanged. Secret-key validation in `validate_provider_config` is untouched. Co-Authored-By: Claude Fable 5 Signed-off-by: Troy Hoffman --- .../src-tauri/src/managed_agents/backend.rs | 413 +++++++++++++++++- 1 file changed, 394 insertions(+), 19 deletions(-) diff --git a/desktop/src-tauri/src/managed_agents/backend.rs b/desktop/src-tauri/src/managed_agents/backend.rs index 2a72af92d7..56ca390d27 100644 --- a/desktop/src-tauri/src/managed_agents/backend.rs +++ b/desktop/src-tauri/src/managed_agents/backend.rs @@ -409,6 +409,129 @@ pub fn validate_provider_config(config: &serde_json::Value) -> Result<(), String Ok(()) } +const PROVIDER_PREFIX: &str = "buzz-backend-"; + +/// Executable extensions a provider binary may carry on Windows. +/// +/// `.cmd` and `.bat` are deliberately absent. `std::process::Command` runs +/// those through `cmd.exe`, which would put a shell-quoting surface in front of +/// a code path that pipes an agent's private key over stdin. `.exe` and `.com` +/// are launched directly by `CreateProcess` with no interpreter in between. +/// This is a security boundary, not a style preference. +#[cfg(any(windows, test))] +const SAFE_EXEC_EXTENSIONS: &[&str] = &["exe", "com"]; + +/// Windows' documented `PATHEXT` default, used when the variable is unset or empty. +#[cfg(any(windows, test))] +const DEFAULT_PATHEXT: &str = ".COM;.EXE;.BAT;.CMD"; + +/// Parse `PATHEXT` into this machine's provider-executable allowlist: +/// `PATHEXT` ∩ `SAFE_EXEC_EXTENSIONS`, lowercased and without the leading dot. +/// +/// Taking the intersection means a machine whose `PATHEXT` drops an extension +/// drops it here too, while `.cmd`/`.bat` can never re-enter through a +/// user-configured `PATHEXT`. +#[cfg(any(windows, test))] +fn allowed_exec_extensions_from(pathext: Option<&str>) -> Vec { + let raw = pathext + .map(str::trim) + .filter(|s| !s.is_empty()) + .unwrap_or(DEFAULT_PATHEXT); + raw.split(';') + .map(|ext| ext.trim().trim_start_matches('.').to_ascii_lowercase()) + .filter(|ext| SAFE_EXEC_EXTENSIONS.contains(&ext.as_str())) + .collect() +} + +/// How this platform names an executable file. +/// +/// An explicit two-variant seam rather than "an allowlist that may be empty": +/// an empty Windows allowlist (a `PATHEXT` of only `.CMD`/`.BAT`) means +/// *nothing* is executable, which is the opposite of the unix meaning. +#[derive(Debug, Clone, PartialEq, Eq)] +enum ExecNaming { + /// Unix: no extension convention; the execute permission bit is the contract. + NoExtension, + /// Windows: the file name must end in one of these extensions (lowercased, + /// no leading dot). See `SAFE_EXEC_EXTENSIONS`. + /// + /// Only constructed on Windows (and in tests, which exercise the Windows + /// rule from any host) — the variant still has to exist everywhere so the + /// derivation logic is compiled and tested once rather than per-platform. + #[cfg_attr(not(windows), allow(dead_code))] + Extensions(Vec), +} + +impl ExecNaming { + /// Where `ext` (no leading dot) sits in this platform's extension + /// precedence, or `None` when it does not name an executable at all. + /// + /// The order is `PATHEXT`'s, which `allowed_exec_extensions_from` + /// preserves: when one directory holds both `foo.com` and `foo.exe`, + /// Windows command lookup runs whichever extension `PATHEXT` lists first. + fn rank(&self, ext: &str) -> Option { + match self { + ExecNaming::NoExtension => None, + ExecNaming::Extensions(allowed) => { + allowed.iter().position(|a| a.eq_ignore_ascii_case(ext)) + } + } + } + + /// Does `ext` (no leading dot) name an executable on this platform? + fn allows(&self, ext: &str) -> bool { + self.rank(ext).is_some() + } +} + +/// The executable-naming rule for the running platform. +fn provider_exec_naming() -> ExecNaming { + #[cfg(windows)] + { + ExecNaming::Extensions(allowed_exec_extensions_from( + std::env::var("PATHEXT").ok().as_deref(), + )) + } + #[cfg(not(windows))] + { + ExecNaming::NoExtension + } +} + +/// Derive a provider id from a directory entry's file name, or `None` when the +/// entry is not a usable provider. +/// +/// Under [`ExecNaming::NoExtension`] the name after the prefix is the id +/// verbatim. Under [`ExecNaming::Extensions`] the name MUST end in an allowed +/// extension and that extension is stripped: `buzz-backend-ssh.exe` yields +/// `ssh`, because `ssh.exe` fails [`provider_id_is_valid`] and would leave +/// every Windows provider undeployable. `buzz-backend-ssh.cmd` yields `None` +/// — see `SAFE_EXEC_EXTENSIONS`. +fn provider_id_from_file_name(name: &str, naming: &ExecNaming) -> Option { + let stem = name.strip_prefix(PROVIDER_PREFIX)?; + if stem.is_empty() { + return None; + } + if *naming == ExecNaming::NoExtension { + return Some(stem.to_string()); + } + let (base, ext) = stem.rsplit_once('.')?; + if base.is_empty() || !naming.allows(ext) { + return None; + } + Some(base.to_string()) +} + +/// Provider ids must match `^[a-z0-9][a-z0-9_-]*$`: no path components, no +/// shell metacharacters, and no file extension. +fn provider_id_is_valid(id: &str) -> bool { + !id.is_empty() + && id.starts_with(|c: char| c.is_ascii_lowercase() || c.is_ascii_digit()) + && id + .chars() + .all(|c| c.is_ascii_lowercase() || c.is_ascii_digit() || c == '_' || c == '-') +} + /// Enumerate PATH for buzz-backend-* executables. Returns (id, path) pairs. /// Only includes files that are executable. Does NOT execute any binaries. /// @@ -416,8 +539,11 @@ pub fn validate_provider_config(config: &serde_json::Value) -> Result<(), String /// which excludes both the app bundle's `Contents/MacOS/` dir and `~/.local/bin`. /// We augment the search with those directories so bundled and user-installed providers /// are always discovered regardless of how the desktop was launched. +/// +/// On Windows the executable extension is stripped from the id and constrained +/// to `SAFE_EXEC_EXTENSIONS` — see [`provider_id_from_file_name`]. pub fn discover_provider_candidates() -> Vec<(String, PathBuf)> { - let prefix = "buzz-backend-"; + let naming = provider_exec_naming(); let mut seen = std::collections::HashSet::new(); let mut results = Vec::new(); @@ -448,19 +574,62 @@ pub fn discover_provider_candidates() -> Vec<(String, PathBuf)> { let Ok(entries) = std::fs::read_dir(&dir) else { continue; }; - for entry in entries.flatten() { + let names = entries.flatten().filter_map(|entry| { let name = entry.file_name().to_string_lossy().to_string(); - if let Some(id) = name.strip_prefix(prefix) { - if !id.is_empty() && !seen.contains(&name) && is_executable(&entry.path()) { - seen.insert(name.clone()); - results.push((id.to_string(), entry.path())); - } + // Cheap name filter before the `is_executable` stat, so an + // unrelated directory entry never costs a metadata call. + (name.starts_with(PROVIDER_PREFIX) && is_executable(&entry.path())).then_some(name) + }); + // Dedupe on the derived id, not the file name: on Windows two + // extensions map to one id, and the earlier PATH entry must win as it + // would for any other command lookup. + for (id, name) in provider_candidates_in_dir(names, &naming) { + if seen.insert(id.clone()) { + results.push((id, dir.join(name))); } } } results } +/// Reduce one directory's file names to its `(id, file name)` providers, +/// ordered so a same-id conflict resolves the way the platform's command +/// lookup would. +/// +/// `read_dir` has no defined iteration order, so a directory holding both +/// `buzz-backend-foo.com` and `buzz-backend-foo.exe` — one id, two files — +/// would otherwise yield whichever the filesystem happened to list first, and +/// discovery could deploy a different binary than Windows would execute. +/// Candidates are ranked by their extension's position in `PATHEXT` (which +/// [`allowed_exec_extensions_from`] preserves) and only the winner per id is +/// kept. Ids that `resolve_provider_binary` would reject are dropped here so +/// the catalog never advertises a provider that can never be executed. +fn provider_candidates_in_dir( + names: impl IntoIterator, + naming: &ExecNaming, +) -> Vec<(String, String)> { + let mut ranked: Vec<(String, usize, String)> = names + .into_iter() + .filter_map(|name| { + let id = provider_id_from_file_name(&name, naming)?; + if !provider_id_is_valid(&id) { + return None; + } + let rank = Path::new(&name) + .extension() + .and_then(|e| e.to_str()) + .and_then(|ext| naming.rank(ext)) + .unwrap_or(0); + Some((id, rank, name)) + }) + .collect(); + // Sort by id so duplicates are adjacent, then by precedence; the file name + // breaks any remaining tie so the result never depends on `read_dir` order. + ranked.sort(); + ranked.dedup_by(|a, b| a.0 == b.0); + ranked.into_iter().map(|(id, _, name)| (id, name)).collect() +} + /// Resolve a provider ID to a discovered, executable binary path. /// /// This is the ONLY way to resolve provider binaries for execution. It: @@ -473,12 +642,7 @@ pub fn discover_provider_candidates() -> Vec<(String, PathBuf)> { /// frontend/IPC caller from steering execution to an arbitrary binary. pub fn resolve_provider_binary(provider_id: &str) -> Result { // Reject IDs that could be path components or shell metacharacters. - let valid_id = provider_id - .chars() - .all(|c| c.is_ascii_lowercase() || c.is_ascii_digit() || c == '_' || c == '-') - && !provider_id.is_empty() - && provider_id.starts_with(|c: char| c.is_ascii_lowercase() || c.is_ascii_digit()); - if !valid_id { + if !provider_id_is_valid(provider_id) { return Err(format!( "invalid provider ID '{provider_id}': must match [a-z0-9][a-z0-9_-]*" )); @@ -500,18 +664,36 @@ pub fn resolve_provider_binary(provider_id: &str) -> Result { } } -/// Check if a file is executable (Unix: mode bits; other platforms: always true). +/// Check if a file is executable. +/// +/// Unix: a regular file with at least one execute mode bit. +/// Windows: a regular file whose extension is allowed by [`provider_exec_naming`] +/// — Windows has no execute bit, so the extension *is* the contract. +/// Other platforms: regular file only. fn is_executable(path: &Path) -> bool { + let Ok(metadata) = path.metadata() else { + return false; + }; + if !metadata.is_file() { + return false; + } + #[cfg(unix)] { use std::os::unix::fs::PermissionsExt; - path.metadata() - .map(|m| m.permissions().mode() & 0o111 != 0) - .unwrap_or(false) + metadata.permissions().mode() & 0o111 != 0 + } + + #[cfg(windows)] + { + let naming = provider_exec_naming(); + path.extension() + .and_then(|e| e.to_str()) + .is_some_and(|ext| naming.allows(ext)) } - #[cfg(not(unix))] + + #[cfg(not(any(unix, windows)))] { - let _ = path; true } } @@ -671,6 +853,199 @@ mod tests { assert_eq!(split_config_key("MyAPIKey"), vec!["my", "api", "key"]); } + /// The naming rule a Windows host with a default `PATHEXT` produces. Lets + /// the id-derivation tests exercise the Windows branch from any host — + /// `provider_id_from_file_name` takes the rule as an argument precisely so + /// this logic is testable without a Windows runner. + fn windows_naming() -> ExecNaming { + ExecNaming::Extensions(allowed_exec_extensions_from(None)) + } + + #[test] + fn provider_id_strips_windows_executable_extension() { + // The W1 bug: `buzz-backend-ssh.exe` yielded id "ssh.exe", which + // `provider_id_is_valid` rejects on the `.`, so no provider was ever + // runnable on Windows. + let id = provider_id_from_file_name("buzz-backend-ssh.exe", &windows_naming()); + assert_eq!(id.as_deref(), Some("ssh")); + assert!(provider_id_is_valid(id.as_deref().unwrap())); + // Case-insensitive: Windows file names are routinely upper-cased. + assert_eq!( + provider_id_from_file_name("buzz-backend-ssh.EXE", &windows_naming()).as_deref(), + Some("ssh") + ); + assert_eq!( + provider_id_from_file_name("buzz-backend-ssh.com", &windows_naming()).as_deref(), + Some("ssh") + ); + // Only the final extension is stripped — a dotted id stays invalid + // rather than collapsing two files onto one id. + assert_eq!( + provider_id_from_file_name("buzz-backend-ssh.v2.exe", &windows_naming()).as_deref(), + Some("ssh.v2") + ); + assert!(!provider_id_is_valid("ssh.v2")); + } + + #[test] + fn provider_id_rejects_shell_script_extensions_on_windows() { + // `.cmd`/`.bat` route through cmd.exe, which would add shell quoting + // in front of a stdin channel that carries an nsec. Security boundary. + for name in [ + "buzz-backend-ssh.cmd", + "buzz-backend-ssh.bat", + "buzz-backend-ssh.CMD", + "buzz-backend-ssh.ps1", + ] { + assert!( + provider_id_from_file_name(name, &windows_naming()).is_none(), + "{name} must not yield a provider id" + ); + } + } + + #[test] + fn provider_id_rejects_extensionless_and_empty_names_on_windows() { + // Windows requires an extension to execute, so a bare name is not a + // provider there even though it is the normal case on unix. + assert!(provider_id_from_file_name("buzz-backend-ssh", &windows_naming()).is_none()); + assert!(provider_id_from_file_name("buzz-backend-.exe", &windows_naming()).is_none()); + assert!(provider_id_from_file_name("buzz-backend-", &windows_naming()).is_none()); + assert!(provider_id_from_file_name("other-tool.exe", &windows_naming()).is_none()); + } + + #[test] + fn provider_id_on_unix_keeps_the_name_verbatim() { + // The unix arm must behave exactly like the old bare `strip_prefix`: + // no extension handling at all. + let unix = ExecNaming::NoExtension; + assert_eq!( + provider_id_from_file_name("buzz-backend-ssh", &unix).as_deref(), + Some("ssh") + ); + assert_eq!( + provider_id_from_file_name("buzz-backend-my_provider-2", &unix).as_deref(), + Some("my_provider-2") + ); + assert!(provider_id_from_file_name("buzz-backend-", &unix).is_none()); + assert!(provider_id_from_file_name("buzz-agent", &unix).is_none()); + // A dotted name on unix stays dotted and is then rejected as an id — + // it is not silently normalized into a different provider. + let dotted = provider_id_from_file_name("buzz-backend-ssh.exe", &unix); + assert_eq!(dotted.as_deref(), Some("ssh.exe")); + assert!(!provider_id_is_valid(dotted.as_deref().unwrap())); + } + + #[test] + fn allowed_exec_extensions_honors_pathext_and_never_admits_scripts() { + // Unset/empty → documented Windows default, minus the script types. + assert_eq!(allowed_exec_extensions_from(None), vec!["com", "exe"]); + assert_eq!(allowed_exec_extensions_from(Some(" ")), vec!["com", "exe"]); + // Intersection: a PATHEXT that omits .COM omits it here too. + assert_eq!( + allowed_exec_extensions_from(Some(".EXE;.BAT;.CMD")), + vec!["exe"] + ); + // A user-configured PATHEXT cannot re-admit shell scripts. + let scripts_only = allowed_exec_extensions_from(Some(".CMD;.BAT;.PS1;.VBS")); + assert!(scripts_only.is_empty()); + } + + #[test] + fn empty_windows_allowlist_discovers_nothing_rather_than_everything() { + // Regression guard for the seam itself: an empty Windows allowlist and + // the unix rule are NOT the same thing. If `PATHEXT` admits no safe + // extension, no file is a provider — the opposite of the unix arm, + // where the absence of extensions means the name is the id. + let empty = ExecNaming::Extensions(Vec::new()); + assert!(provider_id_from_file_name("buzz-backend-ssh.cmd", &empty).is_none()); + assert!(provider_id_from_file_name("buzz-backend-ssh.exe", &empty).is_none()); + assert!(provider_id_from_file_name("buzz-backend-ssh", &empty).is_none()); + assert_eq!( + provider_id_from_file_name("buzz-backend-ssh", &ExecNaming::NoExtension).as_deref(), + Some("ssh") + ); + } + + #[test] + fn same_directory_conflicts_resolve_by_pathext_precedence() { + // `read_dir` order is undefined, so both listings of the same + // directory must select the same file — the one Windows command + // lookup would run, i.e. the extension `PATHEXT` lists first. + let both = ["buzz-backend-foo.exe", "buzz-backend-foo.com"]; + for order in [both, [both[1], both[0]]] { + let names = || order.iter().map(|s| s.to_string()); + + let com_first = ExecNaming::Extensions(allowed_exec_extensions_from(Some(".COM;.EXE"))); + assert_eq!( + provider_candidates_in_dir(names(), &com_first), + vec![("foo".to_string(), "buzz-backend-foo.com".to_string())] + ); + + let exe_first = ExecNaming::Extensions(allowed_exec_extensions_from(Some(".EXE;.COM"))); + assert_eq!( + provider_candidates_in_dir(names(), &exe_first), + vec![("foo".to_string(), "buzz-backend-foo.exe".to_string())] + ); + } + } + + #[test] + fn distinct_ids_in_a_directory_all_survive_dedup() { + // Dedup collapses same-id conflicts only — unrelated providers in the + // same directory must all be discovered, and non-providers dropped. + let names = [ + "buzz-backend-zed.exe", + "buzz-backend-ssh.com", + "buzz-backend-ssh.exe", + "buzz-backend-ssh.cmd", // script extension: never a provider + "buzz-backend-Bad.exe", // uppercase id: resolve would reject it + "unrelated.exe", + ]; + let found = provider_candidates_in_dir( + names.iter().map(|s| s.to_string()), + &windows_naming(), // default PATHEXT → .COM before .EXE + ); + assert_eq!( + found, + vec![ + ("ssh".to_string(), "buzz-backend-ssh.com".to_string()), + ("zed".to_string(), "buzz-backend-zed.exe".to_string()), + ] + ); + } + + #[test] + fn unix_directory_listing_needs_no_extension_ranking() { + // The unix arm has no extension precedence: every name is its own id, + // and ranking must not reorder or drop anything. + let found = provider_candidates_in_dir( + ["buzz-backend-ssh", "buzz-backend-zed"] + .iter() + .map(|s| s.to_string()), + &ExecNaming::NoExtension, + ); + assert_eq!( + found, + vec![ + ("ssh".to_string(), "buzz-backend-ssh".to_string()), + ("zed".to_string(), "buzz-backend-zed".to_string()), + ] + ); + } + + #[test] + fn provider_id_is_valid_matches_resolve_provider_binary_rules() { + assert!(provider_id_is_valid("ssh")); + assert!(provider_id_is_valid("my_provider-2")); + assert!(!provider_id_is_valid("")); + assert!(!provider_id_is_valid("ssh.exe")); + assert!(!provider_id_is_valid("MyProvider")); + assert!(!provider_id_is_valid("-leading-dash")); + assert!(!provider_id_is_valid("_leading_underscore")); + assert!(!provider_id_is_valid("foo;rm -rf /")); + } + #[test] fn resolve_provider_binary_rejects_invalid_ids() { // Path traversal