From 34e74fda0eb05ce918b6ce8778857807bc9b4a6a Mon Sep 17 00:00:00 2001 From: iceweasel-oai Date: Thu, 27 Aug 2026 19:40:40 +0000 Subject: [PATCH] Align deny-read matching with executor path semantics (#41209) ## Why Read-deny policies must use the target executor's path convention so URI-based policy checks and native filesystem enumeration enforce the same rules. ## What changed - Prepare deny roots and glob matchers from `PathUri` policy context, including executor-relative working directories and home-relative patterns. - Match Windows globs case-insensitively with normalized separators, while preserving byte-oriented POSIX matching for non-UTF-8 paths. - Fail closed for malformed paths, incompatible path conventions, unresolved home-relative patterns, and invalid globs. - Make Windows deny-read discovery use case-insensitive ripgrep glob matching. ## Testing Added coverage for Windows URI conventions, executor home expansion, case-insensitive `.env` discovery, malformed paths, non-UTF-8 names, and canonical directory-link targets. GitOrigin-RevId: 36001219a2e9b36acfce8972dc1d3bbc304271c5 --- codex-rs/protocol/src/permissions.rs | 404 ++++++++++++++---- codex-rs/utils/path-uri/src/lib.rs | 7 + .../src/deny_read_resolver.rs | 17 +- .../src/deny_read_resolver_access_tests.rs | 10 +- 4 files changed, 342 insertions(+), 96 deletions(-) diff --git a/codex-rs/protocol/src/permissions.rs b/codex-rs/protocol/src/permissions.rs index 19a42b80dc..4f6303a709 100644 --- a/codex-rs/protocol/src/permissions.rs +++ b/codex-rs/protocol/src/permissions.rs @@ -1,3 +1,4 @@ +use std::borrow::Cow; use std::collections::HashSet; use std::ffi::OsStr; use std::io; @@ -9,6 +10,7 @@ use codex_utils_absolute_path::canonicalize_preserving_symlinks; use codex_utils_path_uri::LegacyAppPathString; use codex_utils_path_uri::PathConvention; use codex_utils_path_uri::PathUri; +use globset::Candidate; use globset::GlobBuilder; use globset::GlobMatcher; use schemars::JsonSchema; @@ -286,7 +288,10 @@ struct FileSystemSemanticSignature { /// Runtime matcher for read-deny entries in a filesystem sandbox policy. pub struct ReadDenyMatcher { - denied_candidates: Vec>, + cwd: AbsolutePathBuf, + user_home_dir: Option, + temporary_directories: Vec, + denied_roots: Vec, deny_read_matchers: Vec, invalid_pattern: bool, } @@ -332,32 +337,24 @@ impl ReadDenyMatcher { if !file_system_sandbox_policy.has_denied_read_restrictions() { return Ok(None); } - - // Exact roots are stored as all meaningful path spellings we can derive - // cheaply. This lets direct tool checks catch both a symlink path and - // its canonical target without changing the policy entries themselves. - let denied_candidates = file_system_sandbox_policy - .get_unreadable_roots_with_cwd(cwd) - .into_iter() - .map(|path| normalized_and_canonical_candidates(path.as_path())) - .collect(); - // Pattern entries stay as policy-level globs. They are matched at read - // time here instead of being snapshotted to startup filesystem state. - let mut invalid_pattern = false; - let mut deny_read_matchers = Vec::new(); - for pattern in file_system_sandbox_policy.get_unreadable_globs_with_cwd(cwd) { - match build_glob_matcher(&pattern) { - Ok(matcher) => deny_read_matchers.push(matcher), - Err(err) => match invalid_glob_behavior { - InvalidDenyReadGlobBehavior::FailClosed => invalid_pattern = true, - InvalidDenyReadGlobBehavior::ReturnError => { - return Err(format!("invalid deny-read glob pattern `{pattern}`: {err}")); - } - }, - } - } + let cwd = AbsolutePathBuf::from_absolute_path(cwd) + .map_err(|err| format!("invalid read-deny cwd: {err}"))?; + let cwd_uri = PathUri::from_abs_path(&cwd); + let user_home_dir = PathUri::from_host_native_path("~").ok(); + let temporary_directories = local_temporary_directories(); + let context = FileSystemSandboxPolicyContext { + cwd: &cwd_uri, + workspace_roots: std::slice::from_ref(&cwd_uri), + user_home_dir: user_home_dir.as_ref(), + temporary_directories: Some(&temporary_directories), + }; + let (denied_roots, deny_read_matchers, invalid_pattern) = file_system_sandbox_policy + .prepare_deny_read_matcher(&context, invalid_glob_behavior)?; Ok(Some(Self { - denied_candidates, + cwd, + user_home_dir, + temporary_directories, + denied_roots, deny_read_matchers, invalid_pattern, })) @@ -365,10 +362,24 @@ impl ReadDenyMatcher { /// Returns whether `path` is denied by the policy used to build this matcher. pub fn is_read_denied(&self, path: &Path) -> bool { - if self.invalid_pattern { + let Some(path) = resolve_candidate_path(path, self.cwd.as_path()) else { return true; - } - self.is_read_denied_candidates(&normalized_and_canonical_candidates(path)) + }; + let path = PathUri::from(path); + let cwd = PathUri::from_abs_path(&self.cwd); + let context = FileSystemSandboxPolicyContext { + cwd: &cwd, + workspace_roots: std::slice::from_ref(&cwd), + user_home_dir: self.user_home_dir.as_ref(), + temporary_directories: Some(&self.temporary_directories), + }; + FileSystemSandboxPolicy::matches_prepared_read_deny( + &path, + &context, + &self.denied_roots, + &self.deny_read_matchers, + self.invalid_pattern, + ) } /// Checks an enumerated path using a canonical location already resolved by @@ -379,39 +390,7 @@ impl ReadDenyMatcher { /// must be resolved separately. Do not reuse these locations across walks: /// a later operation must observe newly created files and changed links. pub fn is_read_denied_with_canonical_path(&self, path: &Path, canonical_path: &Path) -> bool { - let candidates = [path, canonical_path].map(|candidate| { - AbsolutePathBuf::from_absolute_path(candidate) - .map(AbsolutePathBuf::into_path_buf) - .unwrap_or_else(|_| candidate.to_path_buf()) - }); - self.is_read_denied_candidates(&candidates) - } - - fn is_read_denied_candidates(&self, path_candidates: &[PathBuf]) -> bool { - if self.invalid_pattern { - // Direct tool reads fail closed on malformed deny patterns. Silent - // allow would turn a config typo into a policy bypass. - return true; - } - - // Check exact roots against each candidate spelling before evaluating - // glob matchers. Exact entries are subtree denies; glob entries match - // according to the pattern compiler's path-separator rules. - if self.denied_candidates.iter().any(|denied_candidates| { - path_candidates.iter().any(|candidate| { - denied_candidates.iter().any(|denied_candidate| { - candidate == denied_candidate || candidate.starts_with(denied_candidate) - }) - }) - }) { - return true; - } - - self.deny_read_matchers.iter().any(|matcher| { - path_candidates - .iter() - .any(|candidate| matcher.is_match(candidate)) - }) + self.is_read_denied(path) || self.is_read_denied(canonical_path) } } @@ -990,6 +969,128 @@ impl FileSystemSandboxPolicy { .then_some(metadata_name) } + fn prepare_deny_read_matcher( + &self, + context: &FileSystemSandboxPolicyContext<'_>, + invalid_glob_behavior: InvalidDenyReadGlobBehavior, + ) -> Result<(Vec, Vec, bool), String> { + let file_system_root = file_system_root(context); + let denied_roots = self + .resolved_entries(context) + .into_iter() + .filter(|(_, access)| *access == FileSystemAccessMode::Deny) + .filter(|(root, _)| { + !file_system_root.as_ref().is_some_and(|file_system_root| { + root.starts_with(file_system_root) && file_system_root.starts_with(root) + }) + }) + .map(|(root, _)| root) + .collect(); + let Some(convention) = context.cwd.infer_path_convention() else { + return Ok((denied_roots, Vec::new(), true)); + }; + let mut deny_read_matchers = Vec::new(); + let mut invalid_pattern = false; + let patterns = match self.deny_read_globs(context) { + Ok(patterns) => patterns, + Err(err) => match invalid_glob_behavior { + InvalidDenyReadGlobBehavior::FailClosed => { + return Ok((denied_roots, Vec::new(), true)); + } + InvalidDenyReadGlobBehavior::ReturnError => return Err(err), + }, + }; + for pattern in patterns { + match build_glob_matcher(&pattern, convention) { + Ok(matcher) => deny_read_matchers.push(matcher), + Err(err) => match invalid_glob_behavior { + InvalidDenyReadGlobBehavior::FailClosed => invalid_pattern = true, + InvalidDenyReadGlobBehavior::ReturnError => { + return Err(format!("invalid deny-read glob pattern `{pattern}`: {err}")); + } + }, + } + } + Ok((denied_roots, deny_read_matchers, invalid_pattern)) + } + + fn matches_prepared_read_deny( + path: &PathUri, + context: &FileSystemSandboxPolicyContext<'_>, + denied_roots: &[PathUri], + deny_read_matchers: &[GlobMatcher], + invalid_pattern: bool, + ) -> bool { + let Some(convention) = context.cwd.infer_path_convention() else { + return true; + }; + if path.infer_path_convention() != Some(convention) + || path.lexical_depth().is_none() + || context.cwd.lexical_depth().is_none() + { + return true; + } + if invalid_pattern { + return true; + } + denied_roots.iter().any(|root| path.starts_with(root)) + || deny_read_matchers.iter().any(|matcher| { + let path = match convention { + PathConvention::Posix => path.decoded_path_bytes(), + PathConvention::Windows => Cow::Owned( + path.inferred_native_path_string() + .replace('\\', "/") + .into_bytes(), + ), + }; + matcher.is_match_candidate(&Candidate::from_bytes(path.as_ref())) + }) + } + + fn deny_read_globs( + &self, + context: &FileSystemSandboxPolicyContext<'_>, + ) -> Result, String> { + self.entries + .iter() + .filter(|entry| entry.access == FileSystemAccessMode::Deny) + .filter_map(|entry| { + let FileSystemPath::GlobPattern { pattern } = &entry.path else { + return None; + }; + let is_windows = + context.cwd.infer_path_convention() == Some(PathConvention::Windows); + let home_relative = pattern.strip_prefix("~/").or_else(|| { + is_windows.then(|| pattern.strip_prefix("~\\")).flatten() + }); + let (root, pattern) = match home_relative { + Some(suffix) => match context.user_home_dir { + Some(home) => ( + home, + suffix.trim_start_matches(|separator| { + separator == '/' || is_windows && separator == '\\' + }), + ), + None => { + return Some(Err(format!( + "unable to resolve deny-read glob pattern `{pattern}` without executor home" + ))); + } + }, + None => (context.cwd, pattern.as_str()), + }; + Some( + root + .join(pattern) + .map(|path| path.inferred_native_path_string()) + .map_err(|_| { + format!("unable to resolve deny-read glob pattern `{pattern}`") + }), + ) + }) + .collect() + } + /// Replaces symbolic `:workspace_roots` entries with absolute paths resolved /// against `cwd`. /// @@ -1890,39 +1991,19 @@ fn absolute_root_path_for_cwd(cwd: &AbsolutePathBuf) -> AbsolutePathBuf { .unwrap_or_else(|err| panic!("cwd root must be an absolute path: {err}")) } -fn normalized_and_canonical_candidates(path: &Path) -> Vec { - // Compare the lexical absolute form plus the canonical target when it - // exists. Missing paths still need the lexical candidate so future-created - // denied paths remain blocked by direct tool checks. - let mut candidates = Vec::new(); - - if let Ok(normalized) = AbsolutePathBuf::from_absolute_path(path) { - push_unique(&mut candidates, normalized.to_path_buf()); - } else { - push_unique(&mut candidates, path.to_path_buf()); - } - - if let Ok(canonical) = path.canonicalize() - && let Ok(canonical_absolute) = AbsolutePathBuf::from_absolute_path(canonical) - { - push_unique(&mut candidates, canonical_absolute.to_path_buf()); - } - - candidates -} - -fn push_unique(candidates: &mut Vec, candidate: PathBuf) { - if !candidates.iter().any(|existing| existing == &candidate) { - candidates.push(candidate); - } -} - -fn build_glob_matcher(pattern: &str) -> Result { +fn build_glob_matcher(pattern: &str, convention: PathConvention) -> Result { // Keep `*` and `?` within a single path component and preserve an unclosed // `[` as a literal so matcher behavior stays aligned with config parsing. - GlobBuilder::new(pattern) + let pattern = if convention == PathConvention::Windows { + pattern.replace('\\', "/") + } else { + pattern.to_string() + }; + GlobBuilder::new(&pattern) .literal_separator(true) .allow_unclosed_class(true) + .backslash_escape(convention == PathConvention::Posix) + .case_insensitive(convention == PathConvention::Windows) .build() .map(|glob| glob.compile_matcher()) .map_err(|err| err.to_string()) @@ -2624,6 +2705,121 @@ mod tests { ); } + #[test] + fn uri_deny_and_metadata_matcher_use_executor_paths() { + let path = |path| PathUri::parse(path).expect("valid path URI"); + let cwd = path("file:///C:/workspace"); + let roots = [cwd.clone()]; + let context = FileSystemSandboxPolicyContext { + cwd: &cwd, + workspace_roots: &roots, + user_home_dir: None, + temporary_directories: None, + }; + let policy = FileSystemSandboxPolicy::restricted(vec![ + FileSystemSandboxEntry::new(cwd.clone().into(), FileSystemAccessMode::Write), + FileSystemSandboxEntry::new(path("file:///C:/").into(), FileSystemAccessMode::Deny), + FileSystemSandboxEntry::new( + path("file:///C:/workspace/.codex").into(), + FileSystemAccessMode::Read, + ), + unreadable_glob_entry(r"C:\workspace\**\*.env".to_string()), + ]); + let (roots, globs, invalid) = policy + .prepare_deny_read_matcher(&context, InvalidDenyReadGlobBehavior::ReturnError) + .expect("remote deny matcher"); + + assert!(roots.is_empty()); + assert!(FileSystemSandboxPolicy::matches_prepared_read_deny( + &path("file:///c:/WORKSPACE/app/.ENV"), + &context, + &roots, + &globs, + invalid, + )); + for candidate in ["file:///%00/bad/path/YQ", "file:///C:/workspace/%2Fsecret"] { + assert!(FileSystemSandboxPolicy::matches_prepared_read_deny( + &path(candidate), + &context, + &roots, + &globs, + invalid, + )); + } + assert!(!policy.can_write_path(&path("file:///C:/workspace/.codex/config"), &context)); + assert_eq!( + policy.metadata_write_denial(&path("file:///C:/workspace/.codex/config"), &context), + Some(".codex"), + ); + } + + #[test] + fn uri_deny_globs_use_executor_home() { + for (cwd, home, pattern, candidate) in [ + ( + "file:///workspace", + "file:///home/executor", + "~/private/*.env", + "file:///home/executor/private/secret.env", + ), + ( + "file:///workspace", + "file:///home/executor", + "~//private/*.env", + "file:///home/executor/private/secret.env", + ), + ( + "file:///C:/workspace", + "file:///C:/Users/executor", + r"~\private\*.env", + "file:///C:/Users/executor/private/secret.env", + ), + ( + "file:///C:/workspace", + "file:///C:/Users/executor", + r"~\\private\*.env", + "file:///C:/Users/executor/private/secret.env", + ), + ] { + let cwd = PathUri::parse(cwd).expect("executor cwd"); + let home = PathUri::parse(home).expect("executor home"); + let candidate = PathUri::parse(candidate).expect("denied path"); + let context = FileSystemSandboxPolicyContext { + cwd: &cwd, + workspace_roots: std::slice::from_ref(&cwd), + user_home_dir: Some(&home), + temporary_directories: None, + }; + let policy = FileSystemSandboxPolicy::restricted(vec![unreadable_glob_entry( + pattern.to_string(), + )]); + let (roots, globs, invalid) = policy + .prepare_deny_read_matcher(&context, InvalidDenyReadGlobBehavior::ReturnError) + .expect("home-relative deny glob"); + + assert!(FileSystemSandboxPolicy::matches_prepared_read_deny( + &candidate, &context, &roots, &globs, invalid, + )); + + let without_home = FileSystemSandboxPolicyContext { + user_home_dir: None, + ..context + }; + assert!( + policy + .prepare_deny_read_matcher( + &without_home, + InvalidDenyReadGlobBehavior::ReturnError, + ) + .is_err() + ); + let (_, _, invalid) = policy + .prepare_deny_read_matcher(&without_home, InvalidDenyReadGlobBehavior::FailClosed) + .expect("missing executor home fails closed"); + assert!(invalid); + } + } + #[test] fn unknown_special_paths_are_ignored_by_legacy_bridge() -> std::io::Result<()> { let policy = FileSystemSandboxPolicy::restricted(vec![ @@ -4091,7 +4287,7 @@ mod tests { #[cfg(unix)] #[test] - fn canonical_target_matches_denied_symlink_alias() { + fn native_read_deny_matching_uses_uri_matcher_semantics() { let temp = TempDir::new().expect("tempdir"); let real_dir = temp.path().join("real"); let alias_dir = temp.path().join("alias"); @@ -4102,8 +4298,17 @@ mod tests { std::fs::write(&secret, "secret").expect("write secret"); let alias_secret = alias_dir.join("secret.txt"); - let policy = deny_policy(&real_dir); - assert!(is_read_denied(&alias_secret, &policy, temp.path())); + for (denied_root, candidate) in [(&real_dir, &alias_secret), (&alias_dir, &secret)] { + let policy = deny_policy(denied_root); + let denied_root_uri = + PathUri::from_host_native_path(denied_root).expect("deny root URI"); + let candidate_uri = PathUri::from_host_native_path(candidate).expect("candidate URI"); + + assert_eq!( + is_read_denied(candidate, &policy, temp.path()), + candidate_uri.starts_with(&denied_root_uri) + ); + } } #[test] @@ -4137,6 +4342,17 @@ mod tests { )); assert!(is_read_denied(&denied, &policy, temp.path())); + + #[cfg(unix)] + { + use std::os::unix::ffi::OsStrExt; + + let non_utf8 = denied + .parent() + .expect("parent") + .join(OsStr::from_bytes(b"secret\xff.txt")); + assert!(is_read_denied(&non_utf8, &policy, temp.path())); + } } #[test] diff --git a/codex-rs/utils/path-uri/src/lib.rs b/codex-rs/utils/path-uri/src/lib.rs index 0633aa4b88..3eb754bf6e 100644 --- a/codex-rs/utils/path-uri/src/lib.rs +++ b/codex-rs/utils/path-uri/src/lib.rs @@ -186,6 +186,13 @@ impl PathUri { self.0.path() } + /// Returns the percent-decoded URI path without requiring valid UTF-8. + /// + /// The URL authority is not included. + pub fn decoded_path_bytes(&self) -> Cow<'_, [u8]> { + urlencoding::decode_binary(self.encoded_path().as_bytes()) + } + fn windows_identity_path_bytes(&self) -> Option> { if self.infer_path_convention() != Some(PathConvention::Windows) || self.opaque_fallback_bytes().is_some() diff --git a/codex-rs/windows-sandbox-rs/src/deny_read_resolver.rs b/codex-rs/windows-sandbox-rs/src/deny_read_resolver.rs index 57a19d0720..e5f8a2099e 100644 --- a/codex-rs/windows-sandbox-rs/src/deny_read_resolver.rs +++ b/codex-rs/windows-sandbox-rs/src/deny_read_resolver.rs @@ -103,6 +103,7 @@ fn ripgrep_files(scan_plan: &GlobScanPlan) -> Result>, Strin .arg("--files") .arg("--hidden") .arg("--no-ignore") + .arg("--glob-case-insensitive") .arg("--null"); if let Some(max_depth) = scan_plan.max_depth { command.arg("--max-depth").arg(max_depth.to_string()); @@ -541,22 +542,36 @@ mod tests { let cwd = AbsolutePathBuf::from_absolute_path(tmp.path()).expect("absolute cwd"); let root_env = tmp.path().join(".env"); let nested_env = tmp.path().join("app").join(".env"); + let uppercase_env = tmp.path().join("app").join("SECRET.ENV"); let notes = tmp.path().join("app").join("notes.txt"); std::fs::create_dir_all(notes.parent().expect("parent")).expect("create parent"); std::fs::write(&root_env, "secret").expect("write root env"); std::fs::write(&nested_env, "secret").expect("write nested env"); + std::fs::write(&uppercase_env, "secret").expect("write uppercase env"); std::fs::write(¬es, "notes").expect("write notes"); let policy = FileSystemSandboxPolicy::restricted(vec![unreadable_glob_entry(format!( "{}/**/*.env", tmp.path().display() ))]); + if let Some(paths) = super::ripgrep_files(&GlobScanPlan { + root: tmp.path().to_path_buf(), + max_depth: None, + globs: vec!["**/*.env".to_string()], + }) + .expect("case-insensitive ripgrep scan") + { + assert!(paths.contains(&uppercase_env)); + } let actual: HashSet = resolve_windows_deny_read_paths(&policy, &cwd) .expect("resolve") .into_iter() .map(AbsolutePathBuf::into_path_buf) .collect(); - let expected = [root_env, nested_env].into_iter().collect(); + let mut expected = [root_env, nested_env].into_iter().collect::>(); + if cfg!(windows) { + expected.insert(uppercase_env); + } assert_eq!(actual, expected); } diff --git a/codex-rs/windows-sandbox-rs/src/deny_read_resolver_access_tests.rs b/codex-rs/windows-sandbox-rs/src/deny_read_resolver_access_tests.rs index f473c9f96e..7a40902001 100644 --- a/codex-rs/windows-sandbox-rs/src/deny_read_resolver_access_tests.rs +++ b/codex-rs/windows-sandbox-rs/src/deny_read_resolver_access_tests.rs @@ -222,7 +222,15 @@ fn native_walk_preserves_canonical_matches_through_directory_links_and_cycles() let matcher = ReadDenyMatcher::try_new(&policy, cwd.as_path()) .expect("valid matcher") .expect("deny restriction"); - assert!(matcher.is_read_denied(&alias.join("secret.env"))); + assert!( + matcher.is_read_denied_with_canonical_path( + &alias.join("secret.env"), + &target + .canonicalize() + .expect("canonical target") + .join("secret.env"), + ) + ); let mut paths = Vec::new(); collect_existing_glob_directory_matches( &walk,