mirror of
https://github.com/openai/codex.git
synced 2026-11-13 06:45:07 +00:00
Improve sandboxing, MCP errors, and cached approvals (#41196)
Expand Windows deny-read globs robustly across protected directories while preserving filesystem enforcement. Preserve structured MCP tool and resource errors through app-server responses. Bind cached Guardian classifications to current local and root authorization state so stale scores cannot approve actions. Add regression coverage for the sandbox resolver, structured protocol errors, and authorization changes. GitOrigin-RevId: 4b80ed724d869afeca79204222d8465fa99d3a24
This commit is contained in:
1 parent
0182ff3480
commit
035295b46e
18 files changed
+1285
-81
No files matched your search
@@ -5,12 +5,20 @@ use codex_protocol::permissions::FileSystemSandboxPolicy;
|
||||
use codex_protocol::permissions::ReadDenyMatcher;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use std::collections::HashSet;
|
||||
use std::path::Path;
|
||||
use std::path::PathBuf;
|
||||
use std::process::Command;
|
||||
|
||||
#[path = "deny_read_walker.rs"]
|
||||
mod walker;
|
||||
|
||||
use walker::DirectoryScanMode;
|
||||
use walker::collect_existing_glob_directory_matches;
|
||||
|
||||
#[derive(Debug, Eq, PartialEq)]
|
||||
struct GlobScanPlan {
|
||||
root: PathBuf,
|
||||
max_depth: Option<usize>,
|
||||
globs: Vec<String>,
|
||||
}
|
||||
|
||||
/// Resolve split filesystem `None` read entries into concrete Windows ACL targets.
|
||||
@@ -52,87 +60,107 @@ pub fn resolve_windows_deny_read_paths(
|
||||
return Ok(paths);
|
||||
};
|
||||
|
||||
let scan_plans = unreadable_globs
|
||||
.iter()
|
||||
.map(|pattern| {
|
||||
let scan_plan = glob_scan_plan(pattern, file_system_sandbox_policy.glob_scan_max_depth);
|
||||
if scan_plan.max_depth.is_none() && scan_plan.root.parent().is_none() {
|
||||
return Err(format!(
|
||||
"unreadable glob `{pattern}` cannot be safely expanded from a filesystem root without `glob_scan_max_depth`; configure `glob_scan_max_depth` or use a non-root directory prefix"
|
||||
));
|
||||
}
|
||||
Ok(scan_plan)
|
||||
})
|
||||
.collect::<Result<Vec<_>, String>>()?;
|
||||
let scan_plans = glob_scan_plans(
|
||||
&unreadable_globs,
|
||||
file_system_sandbox_policy.glob_scan_max_depth,
|
||||
)?;
|
||||
|
||||
for scan_plan in scan_plans {
|
||||
let mut seen_scan_dirs = HashSet::new();
|
||||
collect_existing_glob_matches(
|
||||
if !scan_plan.root.exists() {
|
||||
continue;
|
||||
}
|
||||
|
||||
let directory_scan_mode = if let Some(file_paths) = ripgrep_files(&scan_plan)? {
|
||||
for path in file_paths {
|
||||
if matcher.is_read_denied(&path) {
|
||||
push_absolute_path(&mut paths, &mut seen, path)?;
|
||||
}
|
||||
}
|
||||
DirectoryScanMode::DirectoriesOnly
|
||||
} else {
|
||||
// Rebuild the complete accessible snapshot with the policy matcher
|
||||
// when ripgrep is missing or reports an incomplete traversal. Never
|
||||
// apply ACLs from a failed scan's partial stdout.
|
||||
DirectoryScanMode::IncludeAllFiles
|
||||
};
|
||||
|
||||
collect_existing_glob_directory_matches(
|
||||
&scan_plan.root,
|
||||
&matcher,
|
||||
&mut paths,
|
||||
&mut seen,
|
||||
&mut seen_scan_dirs,
|
||||
scan_plan.max_depth,
|
||||
/*depth*/ 0,
|
||||
directory_scan_mode,
|
||||
)?;
|
||||
}
|
||||
|
||||
Ok(paths)
|
||||
}
|
||||
|
||||
fn collect_existing_glob_matches(
|
||||
path: &Path,
|
||||
matcher: &ReadDenyMatcher,
|
||||
paths: &mut Vec<AbsolutePathBuf>,
|
||||
seen_paths: &mut HashSet<PathBuf>,
|
||||
seen_scan_dirs: &mut HashSet<PathBuf>,
|
||||
max_depth: Option<usize>,
|
||||
depth: usize,
|
||||
) -> Result<(), String> {
|
||||
if !path.exists() {
|
||||
return Ok(());
|
||||
fn ripgrep_files(scan_plan: &GlobScanPlan) -> Result<Option<Vec<PathBuf>>, String> {
|
||||
let mut command = Command::new("rg");
|
||||
command
|
||||
.arg("--files")
|
||||
.arg("--hidden")
|
||||
.arg("--no-ignore")
|
||||
.arg("--null");
|
||||
if let Some(max_depth) = scan_plan.max_depth {
|
||||
command.arg("--max-depth").arg(max_depth.to_string());
|
||||
}
|
||||
|
||||
if matcher.is_read_denied(path) {
|
||||
push_absolute_path(paths, seen_paths, path.to_path_buf())?;
|
||||
for glob in &scan_plan.globs {
|
||||
command.arg("--glob").arg(glob);
|
||||
}
|
||||
command.arg("--").arg(&scan_plan.root);
|
||||
|
||||
let Ok(metadata) = path.metadata() else {
|
||||
return Ok(());
|
||||
let output = match command.output() {
|
||||
Ok(output) => output,
|
||||
Err(err) if err.kind() == std::io::ErrorKind::NotFound => return Ok(None),
|
||||
Err(err) => {
|
||||
return Err(format!(
|
||||
"failed to run bundled ripgrep for unreadable glob scan under {}: {err}",
|
||||
scan_plan.root.display()
|
||||
));
|
||||
}
|
||||
};
|
||||
if !metadata.is_dir() {
|
||||
return Ok(());
|
||||
if !output.status.success() {
|
||||
if output.status.code() == Some(1) && output.stderr.is_empty() {
|
||||
return Ok(Some(Vec::new()));
|
||||
}
|
||||
if output.status.code() == Some(2) {
|
||||
// Ripgrep uses exit 2 for traversal errors, including protected
|
||||
// Windows directories. Its output may be incomplete. The caller
|
||||
// must enumerate again using the matcher-backed walker; policy
|
||||
// syntax has already been validated by ReadDenyMatcher::try_new.
|
||||
return Ok(None);
|
||||
}
|
||||
|
||||
let stderr = String::from_utf8_lossy(&output.stderr);
|
||||
return Err(format!(
|
||||
"ripgrep unreadable glob scan failed under {}: {stderr}",
|
||||
scan_plan.root.display()
|
||||
));
|
||||
}
|
||||
|
||||
// Canonical directory keys keep recursive scans from following a symlink or
|
||||
// junction cycle forever while preserving the original matched path for the
|
||||
// ACL layer.
|
||||
let scan_key = dunce::canonicalize(path).unwrap_or_else(|_| path.to_path_buf());
|
||||
if !seen_scan_dirs.insert(scan_key) {
|
||||
return Ok(());
|
||||
}
|
||||
|
||||
if max_depth.is_some_and(|max_depth| depth >= max_depth) {
|
||||
return Ok(());
|
||||
}
|
||||
|
||||
let Ok(entries) = std::fs::read_dir(path) else {
|
||||
return Ok(());
|
||||
};
|
||||
for entry in entries.flatten() {
|
||||
collect_existing_glob_matches(
|
||||
&entry.path(),
|
||||
matcher,
|
||||
paths,
|
||||
seen_paths,
|
||||
seen_scan_dirs,
|
||||
max_depth,
|
||||
depth + 1,
|
||||
)?;
|
||||
}
|
||||
|
||||
Ok(())
|
||||
output
|
||||
.stdout
|
||||
.split(|byte| *byte == b'\0')
|
||||
.filter(|path| !path.is_empty())
|
||||
.map(|path| {
|
||||
let path = std::str::from_utf8(path).map_err(|err| {
|
||||
format!(
|
||||
"ripgrep returned a non-UTF-8 path under {}: {err}",
|
||||
scan_plan.root.display()
|
||||
)
|
||||
})?;
|
||||
let path = PathBuf::from(path);
|
||||
Ok(if path.is_absolute() {
|
||||
path
|
||||
} else {
|
||||
scan_plan.root.join(path)
|
||||
})
|
||||
})
|
||||
.collect::<Result<Vec<_>, _>>()
|
||||
.map(Some)
|
||||
}
|
||||
|
||||
fn push_absolute_path(
|
||||
@@ -148,6 +176,37 @@ fn push_absolute_path(
|
||||
Ok(())
|
||||
}
|
||||
|
||||
fn glob_scan_plans(
|
||||
patterns: &[String],
|
||||
configured_max_depth: Option<usize>,
|
||||
) -> Result<Vec<GlobScanPlan>, String> {
|
||||
let mut scan_plans: Vec<GlobScanPlan> = Vec::new();
|
||||
|
||||
for pattern in patterns {
|
||||
let mut scan_plan = glob_scan_plan(pattern, configured_max_depth);
|
||||
if scan_plan.max_depth.is_none() && scan_plan.root.parent().is_none() {
|
||||
return Err(format!(
|
||||
"unreadable glob `{pattern}` cannot be safely expanded from a filesystem root without `glob_scan_max_depth`; configure `glob_scan_max_depth` or use a non-root directory prefix"
|
||||
));
|
||||
}
|
||||
|
||||
if let Some(existing) = scan_plans
|
||||
.iter_mut()
|
||||
.find(|existing| existing.root == scan_plan.root)
|
||||
{
|
||||
existing.max_depth = match (existing.max_depth, scan_plan.max_depth) {
|
||||
(Some(existing_depth), Some(new_depth)) => Some(existing_depth.max(new_depth)),
|
||||
_ => None,
|
||||
};
|
||||
existing.globs.append(&mut scan_plan.globs);
|
||||
} else {
|
||||
scan_plans.push(scan_plan);
|
||||
}
|
||||
}
|
||||
|
||||
Ok(scan_plans)
|
||||
}
|
||||
|
||||
fn glob_scan_plan(pattern: &str, configured_max_depth: Option<usize>) -> GlobScanPlan {
|
||||
// Start scanning at the deepest literal directory prefix before the first
|
||||
// glob metacharacter. For example, `C:\repo\**\*.env` only scans `C:\repo`
|
||||
@@ -162,6 +221,7 @@ fn glob_scan_plan(pattern: &str, configured_max_depth: Option<usize>) -> GlobSca
|
||||
return GlobScanPlan {
|
||||
root: PathBuf::from("."),
|
||||
max_depth: effective_glob_scan_max_depth(pattern, configured_max_depth),
|
||||
globs: vec![ripgrep_glob(pattern)],
|
||||
};
|
||||
};
|
||||
let pattern_suffix = &pattern[separator_index + 1..];
|
||||
@@ -174,11 +234,51 @@ fn glob_scan_plan(pattern: &str, configured_max_depth: Option<usize>) -> GlobSca
|
||||
return GlobScanPlan {
|
||||
root: PathBuf::from(&literal_prefix[..=separator_index]),
|
||||
max_depth: effective_glob_scan_max_depth(pattern_suffix, configured_max_depth),
|
||||
globs: vec![ripgrep_glob(pattern_suffix)],
|
||||
};
|
||||
}
|
||||
GlobScanPlan {
|
||||
root: PathBuf::from(literal_prefix[..separator_index].to_string()),
|
||||
max_depth: effective_glob_scan_max_depth(pattern_suffix, configured_max_depth),
|
||||
globs: vec![ripgrep_glob(pattern_suffix)],
|
||||
}
|
||||
}
|
||||
|
||||
fn ripgrep_glob(pattern: &str) -> String {
|
||||
let pattern = pattern.replace('\\', "/");
|
||||
let mut escaped = String::with_capacity(pattern.len());
|
||||
let mut chars = pattern.chars();
|
||||
|
||||
while let Some(ch) = chars.next() {
|
||||
if ch != '[' {
|
||||
escaped.push(ch);
|
||||
continue;
|
||||
}
|
||||
|
||||
let mut class = String::new();
|
||||
let mut closed = false;
|
||||
for class_ch in chars.by_ref() {
|
||||
if class_ch == ']' {
|
||||
closed = true;
|
||||
break;
|
||||
}
|
||||
class.push(class_ch);
|
||||
}
|
||||
|
||||
if closed {
|
||||
escaped.push('[');
|
||||
escaped.push_str(&class);
|
||||
escaped.push(']');
|
||||
} else {
|
||||
escaped.push_str(r"\[");
|
||||
escaped.push_str(&class);
|
||||
}
|
||||
}
|
||||
|
||||
if escaped.starts_with("**/") {
|
||||
escaped
|
||||
} else {
|
||||
format!("**/{escaped}")
|
||||
}
|
||||
}
|
||||
|
||||
@@ -198,10 +298,17 @@ fn effective_glob_scan_max_depth(
|
||||
}))
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
#[path = "deny_read_resolver_access_tests.rs"]
|
||||
mod access_tests;
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::GlobScanPlan;
|
||||
use super::glob_scan_plan;
|
||||
use super::glob_scan_plans;
|
||||
use super::resolve_windows_deny_read_paths;
|
||||
use super::ripgrep_glob;
|
||||
use codex_protocol::permissions::FileSystemAccessMode;
|
||||
use codex_protocol::permissions::FileSystemPath;
|
||||
use codex_protocol::permissions::FileSystemSandboxEntry;
|
||||
@@ -283,6 +390,102 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn glob_patterns_with_the_same_root_and_depth_share_one_scan() {
|
||||
assert_eq!(
|
||||
glob_scan_plans(
|
||||
&[
|
||||
"/tmp/work/**/*.env".to_string(),
|
||||
"/tmp/work/**/*.pem".to_string(),
|
||||
"/tmp/work/**/*.secret".to_string(),
|
||||
],
|
||||
/*configured_max_depth*/ Some(3),
|
||||
)
|
||||
.expect("combined scan plans"),
|
||||
vec![GlobScanPlan {
|
||||
root: PathBuf::from("/tmp/work"),
|
||||
max_depth: Some(3),
|
||||
globs: vec![
|
||||
"**/*.env".to_string(),
|
||||
"**/*.pem".to_string(),
|
||||
"**/*.secret".to_string(),
|
||||
],
|
||||
}]
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn bounded_globs_with_the_same_root_share_the_deepest_scan() {
|
||||
assert_eq!(
|
||||
glob_scan_plans(
|
||||
&[
|
||||
"/tmp/work/*.env".to_string(),
|
||||
"/tmp/work/*/*.pem".to_string(),
|
||||
],
|
||||
/*configured_max_depth*/ None,
|
||||
)
|
||||
.expect("combined scan plans"),
|
||||
vec![GlobScanPlan {
|
||||
root: PathBuf::from("/tmp/work"),
|
||||
max_depth: Some(2),
|
||||
globs: vec!["**/*.env".to_string(), "**/*/*.pem".to_string()],
|
||||
}]
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn recursive_and_bounded_globs_with_the_same_root_share_one_scan() {
|
||||
assert_eq!(
|
||||
glob_scan_plans(
|
||||
&[
|
||||
"/tmp/work/*.env".to_string(),
|
||||
"/tmp/work/**/*.pem".to_string(),
|
||||
],
|
||||
/*configured_max_depth*/ None,
|
||||
)
|
||||
.expect("combined scan plans"),
|
||||
vec![GlobScanPlan {
|
||||
root: PathBuf::from("/tmp/work"),
|
||||
max_depth: None,
|
||||
globs: vec!["**/*.env".to_string(), "**/*.pem".to_string()],
|
||||
}]
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn project_recursive_globs_without_depth_expand_existing_matches() {
|
||||
let tmp = TempDir::new().expect("tempdir");
|
||||
let cwd = AbsolutePathBuf::from_absolute_path(tmp.path()).expect("absolute cwd");
|
||||
let secret = tmp.path().join("nested").join("secret.env");
|
||||
std::fs::create_dir_all(secret.parent().expect("secret parent")).expect("create parent");
|
||||
std::fs::write(&secret, "secret").expect("write secret");
|
||||
let pattern = tmp.path().join("**").join("*.env").display().to_string();
|
||||
let policy = FileSystemSandboxPolicy::restricted(vec![unreadable_glob_entry(pattern)]);
|
||||
|
||||
assert_eq!(
|
||||
resolve_windows_deny_read_paths(&policy, &cwd).expect("unbounded project glob"),
|
||||
vec![AbsolutePathBuf::from_absolute_path(secret).expect("absolute secret")]
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn relative_recursive_globs_without_depth_expand_existing_matches() {
|
||||
let tmp = TempDir::new().expect("tempdir");
|
||||
let cwd = AbsolutePathBuf::from_absolute_path(tmp.path()).expect("absolute cwd");
|
||||
let secret = tmp.path().join("nested").join("secret.env");
|
||||
std::fs::create_dir_all(secret.parent().expect("secret parent")).expect("create parent");
|
||||
std::fs::write(&secret, "secret").expect("write secret");
|
||||
let policy = FileSystemSandboxPolicy::restricted(vec![unreadable_glob_entry(
|
||||
"**/*.env".to_string(),
|
||||
)]);
|
||||
|
||||
assert_eq!(
|
||||
resolve_windows_deny_read_paths(&policy, &cwd)
|
||||
.expect("unbounded relative project glob"),
|
||||
vec![AbsolutePathBuf::from_absolute_path(secret).expect("absolute secret")]
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn root_recursive_globs_without_depth_fail_before_expansion() {
|
||||
let tmp = TempDir::new().expect("tempdir");
|
||||
@@ -358,6 +561,127 @@ mod tests {
|
||||
assert_eq!(actual, expected);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn recursive_globs_include_hidden_and_ignored_matches() {
|
||||
let tmp = TempDir::new().expect("tempdir");
|
||||
let cwd = AbsolutePathBuf::from_absolute_path(tmp.path()).expect("absolute cwd");
|
||||
let hidden_secret = tmp.path().join("nested").join(".env");
|
||||
let ignored_secret = tmp.path().join("nested").join("ignored.env");
|
||||
std::fs::create_dir_all(hidden_secret.parent().expect("secret parent"))
|
||||
.expect("create parent");
|
||||
std::fs::write(tmp.path().join(".ignore"), "ignored.env\n").expect("write ignore file");
|
||||
std::fs::write(&hidden_secret, "secret").expect("write hidden secret");
|
||||
std::fs::write(&ignored_secret, "secret").expect("write ignored secret");
|
||||
let policy = FileSystemSandboxPolicy::restricted(vec![unreadable_glob_entry(
|
||||
"**/*.env".to_string(),
|
||||
)]);
|
||||
|
||||
let actual: HashSet<PathBuf> = resolve_windows_deny_read_paths(&policy, &cwd)
|
||||
.expect("resolve hidden and ignored matches")
|
||||
.into_iter()
|
||||
.map(AbsolutePathBuf::into_path_buf)
|
||||
.collect();
|
||||
|
||||
assert_eq!(
|
||||
actual,
|
||||
[hidden_secret, ignored_secret].into_iter().collect()
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn recursive_globs_preserve_matching_empty_directories() {
|
||||
let tmp = TempDir::new().expect("tempdir");
|
||||
let cwd = AbsolutePathBuf::from_absolute_path(tmp.path()).expect("absolute cwd");
|
||||
let root_secret = tmp.path().join("root.env");
|
||||
let nested_secret = tmp.path().join("nested").join("empty.env");
|
||||
std::fs::create_dir_all(&root_secret).expect("create root secret directory");
|
||||
std::fs::create_dir_all(&nested_secret).expect("create nested secret directory");
|
||||
let policy = FileSystemSandboxPolicy::restricted(vec![unreadable_glob_entry(
|
||||
"**/*.env".to_string(),
|
||||
)]);
|
||||
|
||||
let actual: HashSet<PathBuf> = resolve_windows_deny_read_paths(&policy, &cwd)
|
||||
.expect("resolve matching directories")
|
||||
.into_iter()
|
||||
.map(AbsolutePathBuf::into_path_buf)
|
||||
.collect();
|
||||
|
||||
assert_eq!(actual, [root_secret, nested_secret].into_iter().collect());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn configured_depth_excludes_deeper_recursive_matches() {
|
||||
let tmp = TempDir::new().expect("tempdir");
|
||||
let cwd = AbsolutePathBuf::from_absolute_path(tmp.path()).expect("absolute cwd");
|
||||
let shallow_secret = tmp.path().join("shallow.env");
|
||||
let deep_secret = tmp.path().join("nested").join("deep.env");
|
||||
std::fs::create_dir_all(deep_secret.parent().expect("secret parent"))
|
||||
.expect("create parent");
|
||||
std::fs::write(&shallow_secret, "secret").expect("write shallow secret");
|
||||
std::fs::write(&deep_secret, "secret").expect("write deep secret");
|
||||
let mut policy = FileSystemSandboxPolicy::restricted(vec![unreadable_glob_entry(
|
||||
"**/*.env".to_string(),
|
||||
)]);
|
||||
policy.glob_scan_max_depth = Some(1);
|
||||
|
||||
assert_eq!(
|
||||
resolve_windows_deny_read_paths(&policy, &cwd).expect("resolve depth-limited glob"),
|
||||
vec![AbsolutePathBuf::from_absolute_path(shallow_secret).expect("absolute secret")]
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn recursive_globs_rescan_for_new_matches() {
|
||||
let tmp = TempDir::new().expect("tempdir");
|
||||
let cwd = AbsolutePathBuf::from_absolute_path(tmp.path()).expect("absolute cwd");
|
||||
std::fs::write(tmp.path().join("notes.txt"), "notes").expect("write notes");
|
||||
let policy = FileSystemSandboxPolicy::restricted(vec![unreadable_glob_entry(
|
||||
"**/*.env".to_string(),
|
||||
)]);
|
||||
|
||||
assert_eq!(
|
||||
resolve_windows_deny_read_paths(&policy, &cwd).expect("resolve unmatched glob"),
|
||||
Vec::<AbsolutePathBuf>::new()
|
||||
);
|
||||
let secret = tmp.path().join("new.env");
|
||||
std::fs::write(&secret, "new secret").expect("create matching file");
|
||||
assert_eq!(
|
||||
resolve_windows_deny_read_paths(&policy, &cwd).expect("fresh resolution"),
|
||||
vec![AbsolutePathBuf::from_absolute_path(secret).expect("secret path")]
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn ripgrep_globs_normalize_windows_separators_and_unclosed_classes() {
|
||||
assert_eq!(ripgrep_glob(r"**\*.env"), "**/*.env");
|
||||
assert_eq!(ripgrep_glob("nested/*.env"), "**/nested/*.env");
|
||||
assert_eq!(ripgrep_glob("[*.env"), r"**/\[*.env");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn shared_scan_roots_preserve_matches_at_each_required_depth() {
|
||||
let tmp = TempDir::new().expect("tempdir");
|
||||
let cwd = AbsolutePathBuf::from_absolute_path(tmp.path()).expect("absolute cwd");
|
||||
let env = tmp.path().join(".env");
|
||||
let pem = tmp.path().join("nested").join("credentials.pem");
|
||||
std::fs::create_dir_all(pem.parent().expect("pem parent")).expect("create pem parent");
|
||||
std::fs::write(&env, "secret").expect("write env");
|
||||
std::fs::write(&pem, "secret").expect("write pem");
|
||||
let policy = FileSystemSandboxPolicy::restricted(vec![
|
||||
unreadable_glob_entry(format!("{}/*.env", tmp.path().display())),
|
||||
unreadable_glob_entry(format!("{}/*/*.pem", tmp.path().display())),
|
||||
]);
|
||||
|
||||
let actual: HashSet<PathBuf> = resolve_windows_deny_read_paths(&policy, &cwd)
|
||||
.expect("resolve shared root")
|
||||
.into_iter()
|
||||
.map(AbsolutePathBuf::into_path_buf)
|
||||
.collect();
|
||||
let expected = [env, pem].into_iter().collect();
|
||||
|
||||
assert_eq!(actual, expected);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn invalid_glob_patterns_fail_before_expansion() {
|
||||
let tmp = TempDir::new().expect("tempdir");
|
||||
@@ -424,4 +748,23 @@ mod tests {
|
||||
|
||||
assert_eq!(actual, expected);
|
||||
}
|
||||
|
||||
#[cfg(unix)]
|
||||
#[test]
|
||||
fn recursive_globs_preserve_symlinked_file_matches() {
|
||||
let tmp = TempDir::new().expect("tempdir");
|
||||
let cwd = AbsolutePathBuf::from_absolute_path(tmp.path()).expect("absolute cwd");
|
||||
let target = tmp.path().join("target.txt");
|
||||
let alias = tmp.path().join("linked.env");
|
||||
std::fs::write(&target, "secret").expect("write symlink target");
|
||||
symlink(&target, &alias).expect("create symlink");
|
||||
let policy = FileSystemSandboxPolicy::restricted(vec![unreadable_glob_entry(
|
||||
"**/*.env".to_string(),
|
||||
)]);
|
||||
|
||||
assert_eq!(
|
||||
resolve_windows_deny_read_paths(&policy, &cwd).expect("resolve symlinked secret"),
|
||||
vec![AbsolutePathBuf::from_absolute_path(alias).expect("absolute symlink")]
|
||||
);
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,242 @@
|
||||
use super::resolve_windows_deny_read_paths;
|
||||
use super::walker::DirectoryScanMode;
|
||||
use super::walker::collect_existing_glob_directory_matches;
|
||||
use codex_protocol::permissions::FileSystemAccessMode;
|
||||
use codex_protocol::permissions::FileSystemPath;
|
||||
use codex_protocol::permissions::FileSystemSandboxEntry;
|
||||
use codex_protocol::permissions::FileSystemSandboxPolicy;
|
||||
use codex_protocol::permissions::ReadDenyMatcher;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use pretty_assertions::assert_eq;
|
||||
use std::collections::HashSet;
|
||||
use std::path::Path;
|
||||
use std::path::PathBuf;
|
||||
use tempfile::TempDir;
|
||||
|
||||
fn policy() -> FileSystemSandboxPolicy {
|
||||
FileSystemSandboxPolicy::restricted(vec![FileSystemSandboxEntry {
|
||||
path: FileSystemPath::GlobPattern {
|
||||
pattern: "**/*.env".to_string(),
|
||||
},
|
||||
access: FileSystemAccessMode::Deny,
|
||||
missing_path_behavior: None,
|
||||
}])
|
||||
}
|
||||
|
||||
struct ProtectedDirectory {
|
||||
path: PathBuf,
|
||||
#[cfg(unix)]
|
||||
permissions: std::fs::Permissions,
|
||||
}
|
||||
|
||||
impl ProtectedDirectory {
|
||||
fn new(path: &Path) -> Self {
|
||||
#[cfg(unix)]
|
||||
let permissions = {
|
||||
use std::os::unix::fs::PermissionsExt;
|
||||
|
||||
let permissions = path.metadata().expect("directory metadata").permissions();
|
||||
std::fs::set_permissions(path, std::fs::Permissions::from_mode(/*mode*/ 0o000))
|
||||
.expect("protect directory");
|
||||
permissions
|
||||
};
|
||||
#[cfg(windows)]
|
||||
assert!(
|
||||
std::process::Command::new("icacls.exe")
|
||||
.arg(path)
|
||||
.args(["/deny", "*S-1-1-0:(OI)(CI)R"])
|
||||
.output()
|
||||
.expect("protect directory for every account")
|
||||
.status
|
||||
.success()
|
||||
);
|
||||
Self {
|
||||
path: path.to_path_buf(),
|
||||
#[cfg(unix)]
|
||||
permissions,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
impl Drop for ProtectedDirectory {
|
||||
fn drop(&mut self) {
|
||||
#[cfg(unix)]
|
||||
std::fs::set_permissions(&self.path, self.permissions.clone())
|
||||
.expect("restore directory permissions");
|
||||
#[cfg(windows)]
|
||||
assert!(
|
||||
std::process::Command::new("icacls.exe")
|
||||
.arg(&self.path)
|
||||
.args(["/remove:d", "*S-1-1-0"])
|
||||
.output()
|
||||
.expect("restore directory permissions")
|
||||
.status
|
||||
.success()
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn protected_directory_does_not_discard_accessible_deny_matches() {
|
||||
#[cfg(windows)]
|
||||
if let Some(root) = std::env::var_os("CODEX_TEST_DENY_READ_RESTRICTED_ROOT") {
|
||||
assert_accessible_deny_matches(Path::new(&root));
|
||||
return;
|
||||
}
|
||||
|
||||
let temp = TempDir::new().expect("tempdir");
|
||||
let protected = temp.path().join("protected");
|
||||
let readable = temp.path().join("readable");
|
||||
std::fs::create_dir(&protected).expect("protected directory");
|
||||
std::fs::create_dir(&readable).expect("readable directory");
|
||||
std::fs::write(protected.join("hidden.env"), "protected").expect("protected file");
|
||||
std::fs::write(readable.join("secret.env"), "secret").expect("readable secret");
|
||||
std::fs::create_dir(readable.join("empty.env")).expect("matching empty directory");
|
||||
std::fs::write(readable.join("public.txt"), "public").expect("public file");
|
||||
let guard = ProtectedDirectory::new(&protected);
|
||||
|
||||
#[cfg(windows)]
|
||||
{
|
||||
// CI enables backup privileges, which let directory enumeration bypass
|
||||
// this fixture's ACL. Restrict a separate process, including its rg child,
|
||||
// rather than changing the shared test process's privileges. The parent
|
||||
// owns ACL setup and cleanup, which the restricted child cannot perform.
|
||||
let codex_home = TempDir::new().expect("restricted codex home");
|
||||
let cwd = AbsolutePathBuf::from_absolute_path(temp.path()).expect("workspace");
|
||||
let mut env = std::collections::HashMap::from([
|
||||
(
|
||||
"CODEX_TEST_DENY_READ_RESTRICTED_ROOT".to_string(),
|
||||
cwd.display().to_string(),
|
||||
),
|
||||
("TEMP".to_string(), cwd.display().to_string()),
|
||||
("TMP".to_string(), cwd.display().to_string()),
|
||||
]);
|
||||
for name in ["PATH", "SystemRoot", "COMSPEC", "USERPROFILE"] {
|
||||
if let Ok(value) = std::env::var(name) {
|
||||
env.insert(name.to_string(), value);
|
||||
}
|
||||
}
|
||||
let result = crate::run_windows_sandbox_capture(
|
||||
&codex_protocol::models::PermissionProfile::workspace_write(),
|
||||
std::slice::from_ref(&cwd),
|
||||
codex_home.path(),
|
||||
vec![
|
||||
std::env::current_exe()
|
||||
.expect("test binary")
|
||||
.display()
|
||||
.to_string(),
|
||||
"--exact".to_string(),
|
||||
std::thread::current()
|
||||
.name()
|
||||
.expect("test name")
|
||||
.to_string(),
|
||||
],
|
||||
cwd.as_path(),
|
||||
env,
|
||||
/*timeout_ms*/ Some(30_000),
|
||||
/*cancellation*/ None,
|
||||
/*use_private_desktop*/ true,
|
||||
)
|
||||
.expect("run protected-directory regression without backup privileges");
|
||||
let stdout = String::from_utf8_lossy(&result.stdout);
|
||||
let stderr = String::from_utf8_lossy(&result.stderr);
|
||||
assert_eq!(
|
||||
(result.exit_code, result.timed_out),
|
||||
(0, false),
|
||||
"{stdout}\n{stderr}"
|
||||
);
|
||||
assert!(stdout.contains("test result: ok. 1 passed"), "{stdout}");
|
||||
}
|
||||
|
||||
#[cfg(not(windows))]
|
||||
assert_accessible_deny_matches(temp.path());
|
||||
drop(guard);
|
||||
}
|
||||
|
||||
fn assert_accessible_deny_matches(root: &Path) {
|
||||
let cwd = AbsolutePathBuf::from_absolute_path(root).expect("cwd");
|
||||
let protected = root.join("protected");
|
||||
let enumeration = std::fs::read_dir(&protected)
|
||||
.and_then(std::iter::Iterator::collect::<std::io::Result<Vec<_>>>);
|
||||
assert_eq!(
|
||||
enumeration
|
||||
.expect_err("fixture must deny directory enumeration")
|
||||
.kind(),
|
||||
std::io::ErrorKind::PermissionDenied
|
||||
);
|
||||
|
||||
let actual = resolve_windows_deny_read_paths(&policy(), &cwd)
|
||||
.expect("protected branches must not abort expansion of accessible paths")
|
||||
.into_iter()
|
||||
.map(AbsolutePathBuf::into_path_buf)
|
||||
.collect::<HashSet<_>>();
|
||||
assert_eq!(
|
||||
actual,
|
||||
HashSet::from([
|
||||
root.join("readable/secret.env"),
|
||||
root.join("readable/empty.env"),
|
||||
])
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn native_walk_preserves_canonical_matches_through_directory_links_and_cycles() {
|
||||
let temp = TempDir::new().expect("tempdir");
|
||||
let cwd = AbsolutePathBuf::from_absolute_path(temp.path()).expect("cwd");
|
||||
let walk = temp.path().join("walk");
|
||||
let target = temp.path().join("target");
|
||||
std::fs::create_dir(&walk).expect("walk root");
|
||||
std::fs::create_dir(&target).expect("link target");
|
||||
std::fs::write(target.join("secret.env"), "secret").expect("secret");
|
||||
std::fs::write(target.join("public.txt"), "public").expect("public");
|
||||
let alias = walk.join("alias");
|
||||
let cycle = target.join("cycle");
|
||||
for (link, destination) in [(&alias, &target), (&cycle, &walk)] {
|
||||
#[cfg(unix)]
|
||||
std::os::unix::fs::symlink(destination, link).expect("directory symlink");
|
||||
#[cfg(windows)]
|
||||
assert!(
|
||||
std::process::Command::new("cmd.exe")
|
||||
.args(["/d", "/c", "mklink", "/J"])
|
||||
.arg(link)
|
||||
.arg(destination)
|
||||
.output()
|
||||
.expect("directory junction")
|
||||
.status
|
||||
.success()
|
||||
);
|
||||
}
|
||||
let policy = FileSystemSandboxPolicy::restricted(vec![FileSystemSandboxEntry {
|
||||
path: FileSystemPath::GlobPattern {
|
||||
pattern: target
|
||||
.canonicalize()
|
||||
.expect("canonical target")
|
||||
.join("**")
|
||||
.join("*.env")
|
||||
.display()
|
||||
.to_string(),
|
||||
},
|
||||
access: FileSystemAccessMode::Deny,
|
||||
missing_path_behavior: None,
|
||||
}]);
|
||||
let matcher = ReadDenyMatcher::try_new(&policy, cwd.as_path())
|
||||
.expect("valid matcher")
|
||||
.expect("deny restriction");
|
||||
assert!(matcher.is_read_denied(&alias.join("secret.env")));
|
||||
let mut paths = Vec::new();
|
||||
collect_existing_glob_directory_matches(
|
||||
&walk,
|
||||
&matcher,
|
||||
&mut paths,
|
||||
&mut HashSet::new(),
|
||||
/*max_depth*/ None,
|
||||
DirectoryScanMode::IncludeAllFiles,
|
||||
)
|
||||
.expect("walk through directory alias and cycle");
|
||||
assert_eq!(
|
||||
paths,
|
||||
vec![
|
||||
AbsolutePathBuf::from_absolute_path(alias.join("secret.env")).expect("lexical target")
|
||||
]
|
||||
);
|
||||
}
|
||||
@@ -0,0 +1,151 @@
|
||||
//! Matcher-backed snapshot traversal for Windows deny-read ACL targets.
|
||||
//!
|
||||
//! The accessible-directory behavior is the same as the original native
|
||||
//! resolver: disappearing and inaccessible branches cannot be enumerated, but
|
||||
//! a matching directory is retained before reading its children. This does not
|
||||
//! grant access to a skipped branch. Unexpected I/O errors abort the snapshot.
|
||||
//! Canonical parents are reused only within this walk; links are resolved anew.
|
||||
|
||||
use super::push_absolute_path;
|
||||
use codex_protocol::permissions::ReadDenyMatcher;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use std::collections::HashSet;
|
||||
use std::fs::FileType;
|
||||
use std::io;
|
||||
use std::path::Path;
|
||||
use std::path::PathBuf;
|
||||
|
||||
#[derive(Clone, Copy)]
|
||||
pub(super) enum DirectoryScanMode {
|
||||
DirectoriesOnly,
|
||||
IncludeAllFiles,
|
||||
}
|
||||
|
||||
pub(super) fn collect_existing_glob_directory_matches(
|
||||
root: &Path,
|
||||
matcher: &ReadDenyMatcher,
|
||||
paths: &mut Vec<AbsolutePathBuf>,
|
||||
seen_paths: &mut HashSet<PathBuf>,
|
||||
max_depth: Option<usize>,
|
||||
mode: DirectoryScanMode,
|
||||
) -> Result<(), String> {
|
||||
let Some(metadata) = accessible_entry(root.metadata(), root)? else {
|
||||
return Ok(());
|
||||
};
|
||||
let canonical = accessible_entry(root.canonicalize(), root)?;
|
||||
let mut walker = GlobWalker {
|
||||
matcher,
|
||||
paths,
|
||||
seen_paths,
|
||||
seen_directories: HashSet::new(),
|
||||
max_depth,
|
||||
};
|
||||
walker.visit(
|
||||
root,
|
||||
canonical.as_deref(),
|
||||
metadata.file_type(),
|
||||
/*depth*/ 0,
|
||||
mode,
|
||||
)
|
||||
}
|
||||
|
||||
struct GlobWalker<'a> {
|
||||
matcher: &'a ReadDenyMatcher,
|
||||
paths: &'a mut Vec<AbsolutePathBuf>,
|
||||
seen_paths: &'a mut HashSet<PathBuf>,
|
||||
seen_directories: HashSet<PathBuf>,
|
||||
max_depth: Option<usize>,
|
||||
}
|
||||
|
||||
impl GlobWalker<'_> {
|
||||
fn visit(
|
||||
&mut self,
|
||||
path: &Path,
|
||||
canonical: Option<&Path>,
|
||||
file_type: FileType,
|
||||
depth: usize,
|
||||
mode: DirectoryScanMode,
|
||||
) -> Result<(), String> {
|
||||
let denied = match canonical {
|
||||
Some(canonical) => self
|
||||
.matcher
|
||||
.is_read_denied_with_canonical_path(path, canonical),
|
||||
None => self.matcher.is_read_denied(path),
|
||||
};
|
||||
if denied {
|
||||
push_absolute_path(self.paths, self.seen_paths, path.to_path_buf())?;
|
||||
}
|
||||
if !file_type.is_dir()
|
||||
|| self.max_depth.is_some_and(|limit| depth >= limit)
|
||||
|| !self
|
||||
.seen_directories
|
||||
.insert(canonical.unwrap_or(path).to_path_buf())
|
||||
{
|
||||
return Ok(());
|
||||
}
|
||||
let Some(entries) = accessible_entry(std::fs::read_dir(path), path)? else {
|
||||
return Ok(());
|
||||
};
|
||||
for entry in entries {
|
||||
let Some(entry) = accessible_entry(entry, path)? else {
|
||||
continue;
|
||||
};
|
||||
let entry_path = entry.path();
|
||||
let Some(file_type) = accessible_entry(entry.file_type(), &entry_path)? else {
|
||||
continue;
|
||||
};
|
||||
if file_type.is_symlink() {
|
||||
// Name-surrogate reparse points (including Windows junctions)
|
||||
// cannot inherit the parent's canonical location. Resolve the
|
||||
// target, then retain lexical matches and detect target cycles.
|
||||
let target = accessible_entry(entry_path.canonicalize(), &entry_path)?;
|
||||
let Some(metadata) = accessible_entry(entry_path.metadata(), &entry_path)? else {
|
||||
continue;
|
||||
};
|
||||
self.visit(
|
||||
&entry_path,
|
||||
target.as_deref(),
|
||||
metadata.file_type(),
|
||||
depth + 1,
|
||||
DirectoryScanMode::IncludeAllFiles,
|
||||
)?;
|
||||
} else if file_type.is_dir() || matches!(mode, DirectoryScanMode::IncludeAllFiles) {
|
||||
// Directory entries already carry their type on Windows. A
|
||||
// normal child has the resolved parent's location plus its
|
||||
// enumerated name; do not stat/canonicalize every ordinary file.
|
||||
self.visit(
|
||||
&entry_path,
|
||||
canonical
|
||||
.map(|parent| parent.join(entry.file_name()))
|
||||
.as_deref(),
|
||||
file_type,
|
||||
depth + 1,
|
||||
mode,
|
||||
)?;
|
||||
}
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
}
|
||||
|
||||
fn accessible_entry<T>(result: io::Result<T>, path: &Path) -> Result<Option<T>, String> {
|
||||
match result {
|
||||
Ok(value) => Ok(Some(value)),
|
||||
Err(err)
|
||||
if matches!(
|
||||
err.kind(),
|
||||
io::ErrorKind::NotFound | io::ErrorKind::PermissionDenied
|
||||
) =>
|
||||
{
|
||||
Ok(None)
|
||||
}
|
||||
Err(err) => Err(format!(
|
||||
"failed to enumerate unreadable glob paths under {}: {err}",
|
||||
path.display()
|
||||
)),
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
#[path = "deny_read_walker_tests.rs"]
|
||||
mod tests;
|
||||
@@ -0,0 +1,17 @@
|
||||
use super::accessible_entry;
|
||||
use pretty_assertions::assert_eq;
|
||||
use std::io;
|
||||
use std::path::Path;
|
||||
|
||||
#[test]
|
||||
fn unexpected_io_errors_abort_the_snapshot() {
|
||||
let error = io::Error::other("directory read interrupted");
|
||||
|
||||
assert_eq!(
|
||||
accessible_entry::<()>(Err(error), Path::new("scan-root")),
|
||||
Err(
|
||||
"failed to enumerate unreadable glob paths under scan-root: directory read interrupted"
|
||||
.to_string()
|
||||
)
|
||||
);
|
||||
}
|
||||
Reference in new issue
Block a user