From 8f21b13a1ea861f29b5c766521d83bbf3569fe22 Mon Sep 17 00:00:00 2001 From: Richie Gomez Date: Mon, 27 Jul 2026 13:32:31 -0700 Subject: [PATCH 1/2] Seatbelt: fall back to policy-allowed cwd to avoid getcwd warnings When an explicit process.cwd (or the inherited host cwd) is not readable under the deny-by-default Seatbelt profile, the child shell's startup getcwd() walk fails and leaks noisy "cannot access parent directories" warnings from bash's shell-init / job-working-directory onto stderr. resolve_working_directory now honors an explicit working directory only when it is readable under the filesystem policy (within a readwrite/readonly path and not within a denied path, matched component-wise). Otherwise it launches from a policy-allowed directory (first readwrite, else first readonly, else /) and logs an informational diagnostic to the mxc log. The command still runs unchanged; only the launch directory changes, never the granted filesystem access. Adds unit tests for the allow/deny/fallback/tilde/component-boundary cases and an end-to-end characterization test asserting an out-of-policy cwd produces no getcwd noise while the command still succeeds. Updates the Seatbelt backend doc. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e4e66e58-7a38-4be3-ab78-32fa15b726ca --- docs/macos-support/seatbelt-backend.md | 11 +- .../seatbelt/common/src/seatbelt_runner.rs | 192 +++++++++++++++++- .../tests/e2e_seatbelt_characterization.rs | 54 +++++ 3 files changed, 246 insertions(+), 11 deletions(-) diff --git a/docs/macos-support/seatbelt-backend.md b/docs/macos-support/seatbelt-backend.md index 59d9f50d2..89c9e9112 100644 --- a/docs/macos-support/seatbelt-backend.md +++ b/docs/macos-support/seatbelt-backend.md @@ -230,9 +230,16 @@ unconditional; it applies whether or not `process.env` is provided.) ### Working directory -If `process.cwd` is omitted it resolves to `readwritePaths[0]`, else +If `process.cwd` is provided it is honored **only when it is readable under the +filesystem policy** — i.e. within a `readwritePaths`/`readonlyPaths` entry and +not within a `deniedPaths` entry. If `process.cwd` is omitted, or points at a +directory the policy does not allow, it resolves to `readwritePaths[0]`, else `readonlyPaths[0]`, else `/`; a `~`/`~/…` default is tilde-expanded the same way -the sandbox profile expands policy paths. `PWD` is exported to the resolved +the sandbox profile expands policy paths. Launching from a policy-allowed +directory (rather than an inaccessible one) keeps the child shell's startup +`getcwd()` from emitting noisy "cannot access parent directories" warnings under +the deny-by-default profile; the fallback changes only the launch directory and +never grants additional filesystem access. `PWD` is exported to the resolved directory so the child's `getcwd()` takes its fast `$PWD` path. ## Usage diff --git a/src/backends/seatbelt/common/src/seatbelt_runner.rs b/src/backends/seatbelt/common/src/seatbelt_runner.rs index 71a510d88..e7a18400f 100644 --- a/src/backends/seatbelt/common/src/seatbelt_runner.rs +++ b/src/backends/seatbelt/common/src/seatbelt_runner.rs @@ -222,7 +222,7 @@ fn spawn_exec( // fast `$PWD` path (a single stat) instead of walking parent directories // the sandbox may not let it read — which otherwise leaks // "getcwd: ... Operation not permitted" to stderr. - let cwd = resolve_working_directory(request); + let cwd = resolve_working_directory(request, logger); command.current_dir(&cwd); command.env("PWD", &cwd); @@ -685,16 +685,36 @@ fn spawn_error(error: &std::io::Error) -> String { /// Resolve the working directory for the sandboxed child. /// -/// An explicit `working_directory` always wins. Otherwise — rather than -/// inheriting the host process's cwd, which under the deny-by-default Seatbelt -/// profile may be inaccessible and make `getcwd()` fail (leaking a -/// "getcwd: ... Operation not permitted" line on the child's stderr) — we pick -/// a directory the profile is guaranteed to allow: the first readwrite path, -/// else the first readonly path, else `/` (always readable per the baseline). -fn resolve_working_directory(request: &ExecutionRequest) -> String { +/// An explicit `working_directory` wins **only when it is readable under the +/// sandbox policy**. Otherwise — whether it is empty (so we would inherit the +/// host process's cwd) or an explicit directory outside the policy — the +/// deny-by-default Seatbelt profile makes `getcwd()`'s parent-directory walk +/// fail, leaking noisy "getcwd: ... Operation not permitted" lines from +/// `bash`'s `shell-init` / `job-working-directory` onto the child's stderr. In +/// that case we fall back to a directory the profile is guaranteed to allow: +/// the first readwrite path, else the first readonly path, else `/` (always +/// readable per the baseline). Falling back only changes the launch directory; +/// it never grants additional filesystem access. +fn resolve_working_directory(request: &ExecutionRequest, logger: &mut Logger) -> String { if !request.working_directory.is_empty() { - return request.working_directory.clone(); + if is_working_directory_allowed(request, &request.working_directory) { + return request.working_directory.clone(); + } + let fallback = policy_fallback_directory(request); + logger.log_line(&format!( + "Seatbelt: requested working directory '{}' is not readable under the \ + sandbox policy; launching from policy-allowed directory '{}' to avoid \ + getcwd startup warnings", + request.working_directory, fallback + )); + return fallback; } + policy_fallback_directory(request) +} + +/// Pick a directory the Seatbelt profile is guaranteed to allow: the first +/// readwrite path, else the first readonly path, else `/`. +fn policy_fallback_directory(request: &ExecutionRequest) -> String { let default = request .policy .readwrite_paths @@ -708,6 +728,52 @@ fn resolve_working_directory(request: &ExecutionRequest) -> String { crate::profile_builder::expand_tilde(&default).unwrap_or(default) } +/// Whether `dir` is readable under the sandbox's filesystem policy — i.e. a +/// process launched there can `getcwd()` without the profile denying the walk. +/// +/// A directory is considered readable when it is within (a subpath of, or +/// equal to) some `readwritePaths` / `readonlyPaths` entry and is not within +/// any `deniedPaths` entry (deny overrides allow, matching the profile's rule +/// ordering). Policy paths are tilde-expanded exactly as the profile builder +/// expands them so the comparison sees the same absolute paths the profile +/// grants. Matching is component-wise, so `/data` never matches `/database`. +fn is_working_directory_allowed(request: &ExecutionRequest, dir: &str) -> bool { + let dir = crate::profile_builder::expand_tilde(dir).unwrap_or_else(|_| dir.to_string()); + let policy = &request.policy; + + // Deny wins: if the directory is within any denied subpath it is unreadable. + for denied in &policy.denied_paths { + if let Ok(denied) = crate::profile_builder::expand_tilde(denied) { + if path_within(&dir, &denied) { + return false; + } + } + } + + // Allowed only when within a readwrite or readonly policy subpath. + policy + .readwrite_paths + .iter() + .chain(policy.readonly_paths.iter()) + .filter_map(|p| crate::profile_builder::expand_tilde(p).ok()) + .any(|allowed| path_within(&dir, &allowed)) +} + +/// Whether `child` is equal to, or nested within, `ancestor`, comparing whole +/// path components so `/data` does not match `/database`. Both paths are +/// treated as already absolute/expanded; trailing slashes are ignored. +fn path_within(child: &str, ancestor: &str) -> bool { + let child_components: Vec<&str> = child.trim_end_matches('/').split('/').collect(); + let ancestor_components: Vec<&str> = ancestor.trim_end_matches('/').split('/').collect(); + if ancestor_components.len() > child_components.len() { + return false; + } + child_components + .iter() + .zip(ancestor_components.iter()) + .all(|(c, a)| c == a) +} + /// Baseline `PATH` for the sandboxed child. We always start from a cleared /// environment (so the host process's env — cloud creds, API tokens — never /// leaks into untrusted sandboxed code), which means we must supply a default @@ -952,4 +1018,112 @@ mod tests { let _ = fs::remove_file(p); } } + + // --- working-directory resolution ------------------------------------- + + use wxc_common::logger::{Logger, Mode}; + + fn discard_logger() -> Logger { + Logger::new(Mode::Buffer) + } + + #[test] + fn path_within_matches_whole_components_only() { + assert!(path_within("/data", "/data")); + assert!(path_within("/data/sub/dir", "/data")); + assert!(path_within("/data/", "/data")); + // Component boundary: /database must not match the /data root. + assert!(!path_within("/database", "/data")); + assert!(!path_within("/data", "/data/sub")); + } + + #[test] + fn resolve_working_directory_keeps_allowed_subpath() { + let mut request = base_request(); + request.policy.readwrite_paths = vec!["/work".into()]; + request.working_directory = "/work/project".into(); + assert_eq!( + resolve_working_directory(&request, &mut discard_logger()), + "/work/project" + ); + } + + #[test] + fn resolve_working_directory_falls_back_when_outside_policy() { + let mut request = base_request(); + request.policy.readwrite_paths = vec!["/work".into()]; + request.policy.readonly_paths = vec!["/data".into()]; + request.working_directory = "/somewhere/else".into(); + // Falls back to the first readwrite path. + assert_eq!( + resolve_working_directory(&request, &mut discard_logger()), + "/work" + ); + } + + #[test] + fn resolve_working_directory_falls_back_when_denied() { + let mut request = base_request(); + request.policy.readwrite_paths = vec!["/work".into()]; + request.policy.denied_paths = vec!["/work/secret".into()]; + request.working_directory = "/work/secret/inner".into(); + // Deny overrides the broader readwrite allow, so we fall back. + assert_eq!( + resolve_working_directory(&request, &mut discard_logger()), + "/work" + ); + } + + #[test] + fn resolve_working_directory_falls_back_to_readonly_then_root() { + let mut request = base_request(); + request.policy.readonly_paths = vec!["/ro".into()]; + request.working_directory = "/nope".into(); + assert_eq!( + resolve_working_directory(&request, &mut discard_logger()), + "/ro" + ); + + let mut bare = base_request(); + bare.working_directory = "/nope".into(); + // No policy paths at all → `/`. + assert_eq!(resolve_working_directory(&bare, &mut discard_logger()), "/"); + } + + #[test] + fn resolve_working_directory_matches_tilde_expanded_policy_path() { + // A tilde policy path allows a matching absolute requested cwd. + let home = std::env::var("HOME").expect("HOME set in test env"); + let mut request = base_request(); + request.policy.readwrite_paths = vec!["~/projects".into()]; + request.working_directory = format!("{home}/projects/app"); + assert_eq!( + resolve_working_directory(&request, &mut discard_logger()), + format!("{home}/projects/app") + ); + } + + #[test] + fn resolve_working_directory_empty_uses_first_readwrite() { + let mut request = base_request(); + request.policy.readwrite_paths = vec!["/work".into()]; + request.working_directory = String::new(); + assert_eq!( + resolve_working_directory(&request, &mut discard_logger()), + "/work" + ); + } + + #[test] + fn resolve_working_directory_component_boundary_falls_back() { + // Policy allows /data; a requested /database cwd must not be treated as + // allowed and should fall back. + let mut request = base_request(); + request.policy.readwrite_paths = vec!["/data".into()]; + request.working_directory = "/database".into(); + assert_eq!( + resolve_working_directory(&request, &mut discard_logger()), + "/data" + ); + } } diff --git a/src/testing/wxc_e2e_tests/tests/e2e_seatbelt_characterization.rs b/src/testing/wxc_e2e_tests/tests/e2e_seatbelt_characterization.rs index eafda1312..f3e59679f 100644 --- a/src/testing/wxc_e2e_tests/tests/e2e_seatbelt_characterization.rs +++ b/src/testing/wxc_e2e_tests/tests/e2e_seatbelt_characterization.rs @@ -267,3 +267,57 @@ fn seatbelt_timeout_kills_before_completion() { result.wall_time_ms ); } + +/// Regression guard: when the requested `process.cwd` is not readable under the +/// sandbox policy, the executor launches from a policy-allowed directory so the +/// child shell's startup `getcwd()` does not emit noisy "cannot access parent +/// directories" warnings. The command (reading an allowed absolute path) must +/// still succeed and stderr must be free of the getcwd noise. +#[test] +fn seatbelt_out_of_policy_cwd_has_no_getcwd_noise() { + if !has_platform_exec() { + return; + } + // Allowed read-only location holding the file the command reads. + let ro_dir = fs::canonicalize(unique_tempdir("noise-ro")).expect("canonicalize"); + let protected = ro_dir.join("protected.txt"); + fs::write(&protected, "original read-only content").expect("write protected file"); + // Requested cwd that is deliberately outside every policy path. + let out_of_policy = fs::canonicalize(unique_tempdir("noise-cwd")).expect("canonicalize"); + + let mut cfg = config("cwd-noise", &format!("cat {}", protected.to_string_lossy())); + cfg["process"]["cwd"] = json!(out_of_policy.to_string_lossy()); + cfg["filesystem"] = json!({ "readonlyPaths": [ro_dir.to_string_lossy()] }); + + let result = run_platform_config_value("seatbelt cwd noise", &cfg, &[], None); + + let _ = fs::remove_dir_all(&ro_dir); + let _ = fs::remove_dir_all(&out_of_policy); + + assert_eq!( + result.code, + Some(0), + "reading an allowed absolute path should still succeed:\n{}", + result.combined_output() + ); + assert!( + result.stdout.contains("original read-only content"), + "expected the file contents on stdout:\n{}", + result.combined_output() + ); + // The core of the fix: no getcwd startup noise leaks onto stderr. + for needle in [ + "getcwd", + "error retrieving current directory", + "shell-init", + "job-working-directory", + ] { + assert!( + !result.stderr.contains(needle), + "stderr should not contain getcwd noise ({needle:?}); \ + out-of-policy cwd should fall back to a policy-allowed directory.\n\ + --- stderr ---\n{}", + result.stderr + ); + } +} From 9fd7696bc26a7cc57c17081d44674a3cb6d1401e Mon Sep 17 00:00:00 2001 From: Richie Gomez Date: Tue, 28 Jul 2026 09:44:17 -0700 Subject: [PATCH 2/2] Seatbelt cwd: honor profileOverride and fold ./.. before containment check Addresses PR review feedback: - A raw `profileOverride` replaces the generated profile, so the readwrite/readonly/denied fields are not applied. resolve_working_directory now preserves an explicit cwd unchanged when an override is present instead of consulting the ignored policy fields (which could wrongly rewrite the cwd to the fallback and break relative commands). - is_working_directory_allowed now lexically normalizes (folds `.`/`..`, collapses repeated slashes) the requested cwd and the policy roots before the containment test, so `/work/../private` is no longer classified as within an allowed `/work`. Adds unit tests for both cases plus normalize_path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e4e66e58-7a38-4be3-ab78-32fa15b726ca --- .../seatbelt/common/src/seatbelt_runner.rs | 116 +++++++++++++++++- 1 file changed, 112 insertions(+), 4 deletions(-) diff --git a/src/backends/seatbelt/common/src/seatbelt_runner.rs b/src/backends/seatbelt/common/src/seatbelt_runner.rs index e7a18400f..2d8e73aeb 100644 --- a/src/backends/seatbelt/common/src/seatbelt_runner.rs +++ b/src/backends/seatbelt/common/src/seatbelt_runner.rs @@ -697,7 +697,13 @@ fn spawn_error(error: &std::io::Error) -> String { /// it never grants additional filesystem access. fn resolve_working_directory(request: &ExecutionRequest, logger: &mut Logger) -> String { if !request.working_directory.is_empty() { - if is_working_directory_allowed(request, &request.working_directory) { + // A raw `profileOverride` replaces the generated profile entirely, so the + // readwrite/readonly/denied policy fields are not applied and cannot be + // used to predict readability. Honor the explicit cwd unchanged in that + // case rather than second-guessing an opaque profile. + if has_profile_override(request) + || is_working_directory_allowed(request, &request.working_directory) + { return request.working_directory.clone(); } let fallback = policy_fallback_directory(request); @@ -728,6 +734,16 @@ fn policy_fallback_directory(request: &ExecutionRequest) -> String { crate::profile_builder::expand_tilde(&default).unwrap_or(default) } +/// Whether a raw Seatbelt `profileOverride` is set, in which case the generated +/// filesystem policy fields are ignored by the profile builder. +fn has_profile_override(request: &ExecutionRequest) -> bool { + request + .seatbelt + .as_ref() + .and_then(|c| c.profile_override.as_ref()) + .is_some() +} + /// Whether `dir` is readable under the sandbox's filesystem policy — i.e. a /// process launched there can `getcwd()` without the profile denying the walk. /// @@ -738,13 +754,19 @@ fn policy_fallback_directory(request: &ExecutionRequest) -> String { /// expands them so the comparison sees the same absolute paths the profile /// grants. Matching is component-wise, so `/data` never matches `/database`. fn is_working_directory_allowed(request: &ExecutionRequest, dir: &str) -> bool { - let dir = crate::profile_builder::expand_tilde(dir).unwrap_or_else(|_| dir.to_string()); + // Expand `~` exactly as the profile builder does, then lexically fold `.` / + // `..` so the containment test compares the path the kernel actually + // resolves. Without this, `/work/../private` would spuriously match an + // allowed `/work` even though it resolves outside it. + let dir = normalize_path( + &crate::profile_builder::expand_tilde(dir).unwrap_or_else(|_| dir.to_string()), + ); let policy = &request.policy; // Deny wins: if the directory is within any denied subpath it is unreadable. for denied in &policy.denied_paths { if let Ok(denied) = crate::profile_builder::expand_tilde(denied) { - if path_within(&dir, &denied) { + if path_within(&dir, &normalize_path(&denied)) { return false; } } @@ -756,7 +778,42 @@ fn is_working_directory_allowed(request: &ExecutionRequest, dir: &str) -> bool { .iter() .chain(policy.readonly_paths.iter()) .filter_map(|p| crate::profile_builder::expand_tilde(p).ok()) - .any(|allowed| path_within(&dir, &allowed)) + .any(|allowed| path_within(&dir, &normalize_path(&allowed))) +} + +/// Lexically normalize an absolute path: collapse repeated slashes, drop `.` +/// components, and resolve `..` by popping the previous component (never past +/// the root). Purely lexical — it does not resolve symlinks or touch the +/// filesystem — which is sufficient to fold the `..` a caller may embed in a +/// requested cwd before the containment check. +fn normalize_path(path: &str) -> String { + let is_absolute = path.starts_with('/'); + let mut components: Vec<&str> = Vec::new(); + for part in path.split('/') { + match part { + "" | "." => {} + ".." => { + // Pop a real component; keep `..` for relative paths that walk + // above their start, but for absolute paths never go past root. + match components.last() { + Some(&last) if last != ".." => { + components.pop(); + } + _ if is_absolute => {} + _ => components.push(".."), + } + } + other => components.push(other), + } + } + let joined = components.join("/"); + if is_absolute { + format!("/{joined}") + } else if joined.is_empty() { + ".".to_string() + } else { + joined + } } /// Whether `child` is equal to, or nested within, `ancestor`, comparing whole @@ -1126,4 +1183,55 @@ mod tests { "/data" ); } + + #[test] + fn resolve_working_directory_folds_dotdot_before_containment() { + // `/work/../private` resolves outside the allowed `/work`, so it must + // not be treated as allowed and should fall back. + let mut request = base_request(); + request.policy.readwrite_paths = vec!["/work".into()]; + request.working_directory = "/work/../private".into(); + assert_eq!( + resolve_working_directory(&request, &mut discard_logger()), + "/work" + ); + } + + #[test] + fn resolve_working_directory_allows_dot_segments_within_policy() { + // `.` and redundant slashes inside an allowed root stay allowed. + let mut request = base_request(); + request.policy.readwrite_paths = vec!["/work".into()]; + request.working_directory = "/work/./sub//dir".into(); + assert_eq!( + resolve_working_directory(&request, &mut discard_logger()), + "/work/./sub//dir" + ); + } + + #[test] + fn resolve_working_directory_preserves_explicit_cwd_with_profile_override() { + // With a raw profileOverride the generated filesystem policy is ignored, + // so an explicit out-of-policy cwd must be honored, not replaced. + let mut request = base_request(); + request.policy.readwrite_paths = vec!["/work".into()]; + request.working_directory = "/anywhere".into(); + request.seatbelt = Some(SeatbeltConfig { + profile_override: Some("(version 1)(allow default)".into()), + ..SeatbeltConfig::default() + }); + assert_eq!( + resolve_working_directory(&request, &mut discard_logger()), + "/anywhere" + ); + } + + #[test] + fn normalize_path_folds_dot_and_dotdot() { + assert_eq!(normalize_path("/work/../private"), "/private"); + assert_eq!(normalize_path("/work/./sub//dir"), "/work/sub/dir"); + assert_eq!(normalize_path("/a/b/../../c"), "/c"); + assert_eq!(normalize_path("/.."), "/"); + assert_eq!(normalize_path("/work/"), "/work"); + } }