diff --git a/codex-rs/core/src/tools/handlers/apply_patch_tests.rs b/codex-rs/core/src/tools/handlers/apply_patch_tests.rs index b78e62e2dad3..24d6f6ea4a98 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch_tests.rs +++ b/codex-rs/core/src/tools/handlers/apply_patch_tests.rs @@ -303,8 +303,7 @@ fn write_permissions_for_paths_keep_dirs_outside_workspace_root() { ); let permissions = write_permissions_for_paths(&[file_path], &sandbox_policy, &cwd_abs); - let expected_outside = - dunce::simplified(&outside.canonicalize().expect("canonicalize outside dir")).abs(); + let expected_outside = outside.abs(); assert_eq!( permissions diff --git a/codex-rs/core/tests/suite/request_permissions.rs b/codex-rs/core/tests/suite/request_permissions.rs index fa48e0144291..3cc15273c33b 100644 --- a/codex-rs/core/tests/suite/request_permissions.rs +++ b/codex-rs/core/tests/suite/request_permissions.rs @@ -287,16 +287,6 @@ fn requested_directory_write_permissions(path: &Path) -> RequestPermissionProfil } } -fn normalized_directory_write_permissions(path: &Path) -> Result { - Ok(RequestPermissionProfile { - file_system: Some(FileSystemPermissions::from_read_write_roots( - Some(vec![]), - Some(vec![AbsolutePathBuf::try_from(path.canonicalize()?)?]), - )), - ..RequestPermissionProfile::default() - }) -} - #[tokio::test(flavor = "current_thread")] async fn with_additional_permissions_requires_approval_under_on_request() -> Result<()> { skip_if_no_network!(Ok(())); @@ -508,7 +498,7 @@ async fn request_permissions_auto_review_applies_guardian_decision(outcome: &str let requested_dir = test.workspace_path("guardian-requested-permissions"); fs::create_dir_all(&requested_dir)?; let requested_permissions = requested_directory_write_permissions(&requested_dir); - let normalized_permissions = normalized_directory_write_permissions(&requested_dir)?; + let normalized_permissions = requested_directory_write_permissions(&requested_dir); let call_id = "guardian-request-permissions"; let reason = "Guardian should review access to the requested directory"; let responses = mount_sse_sequence( @@ -576,9 +566,7 @@ async fn request_permissions_auto_review_applies_guardian_decision(outcome: &str }) .context("expected Guardian review request")?; assert!(guardian_request.body_contains_text(reason)); - assert!( - guardian_request.body_contains_text(&requested_dir.canonicalize()?.display().to_string()) - ); + assert!(guardian_request.body_contains_text(&requested_dir.display().to_string())); let output = requests .iter() @@ -765,7 +753,6 @@ async fn relative_additional_permissions_resolve_against_tool_workdir() -> Resul let nested_dir = test.workspace_path("nested"); fs::create_dir_all(&nested_dir)?; - let nested_dir_canonical = nested_dir.canonicalize()?; let requested_write = nested_dir.join("relative-write.txt"); let _ = fs::remove_file(&requested_write); @@ -780,7 +767,7 @@ async fn relative_additional_permissions_resolve_against_tool_workdir() -> Resul let expected_permissions = PermissionProfile { file_system: Some(FileSystemPermissions::from_read_write_roots( /*read*/ None, - Some(vec![absolute_path(&nested_dir_canonical)]), + Some(vec![absolute_path(&nested_dir)]), )), ..Default::default() }; @@ -1096,15 +1083,7 @@ async fn workspace_write_with_additional_permissions_can_write_outside_cwd() -> )), ..RequestPermissionProfile::default() }; - let normalized_requested_permissions = RequestPermissionProfile { - file_system: Some(FileSystemPermissions::from_read_write_roots( - Some(vec![]), - Some(vec![AbsolutePathBuf::try_from( - outside_dir.path().canonicalize()?, - )?]), - )), - ..RequestPermissionProfile::default() - }; + let normalized_requested_permissions = requested_permissions.clone(); let event = exec_command_event_with_request_permissions(call_id, &command, &requested_permissions)?; @@ -1202,15 +1181,7 @@ async fn with_additional_permissions_denied_approval_blocks_execution() -> Resul )), ..Default::default() }; - let normalized_requested_permissions = PermissionProfile { - file_system: Some(FileSystemPermissions::from_read_write_roots( - Some(vec![]), - Some(vec![AbsolutePathBuf::try_from( - outside_dir.path().canonicalize()?, - )?]), - )), - ..Default::default() - }; + let normalized_requested_permissions = requested_permissions.clone(); let event = exec_command_event_with_request_permissions(call_id, &command, &requested_permissions)?; @@ -1308,15 +1279,7 @@ async fn request_permissions_grants_apply_to_later_exec_command_calls() -> Resul )), ..Default::default() }; - let normalized_requested_permissions = RequestPermissionProfile { - file_system: Some(FileSystemPermissions::from_read_write_roots( - Some(vec![]), - Some(vec![AbsolutePathBuf::try_from( - outside_dir.path().canonicalize()?, - )?]), - )), - ..Default::default() - }; + let normalized_requested_permissions = requested_permissions.clone(); let responses = mount_sse_sequence( &server, vec![ @@ -1430,7 +1393,7 @@ async fn request_permissions_preapprove_explicit_exec_permissions_outside_on_req ); let requested_permissions = requested_directory_write_permissions(outside_dir.path()); let normalized_requested_permissions = - normalized_directory_write_permissions(outside_dir.path())?; + requested_directory_write_permissions(outside_dir.path()); let responses = mount_sse_sequence( &server, vec![ @@ -1550,7 +1513,7 @@ async fn request_permissions_grants_apply_to_later_exec_command_calls_without_in ); let requested_permissions = requested_directory_write_permissions(outside_dir.path()); let normalized_requested_permissions = - normalized_directory_write_permissions(outside_dir.path())?; + requested_directory_write_permissions(outside_dir.path()); let responses = mount_sse_sequence( &server, vec![ @@ -1677,28 +1640,10 @@ async fn partial_request_permissions_grants_do_not_preapprove_new_permissions() )), ..RequestPermissionProfile::default() }; - let normalized_requested_permissions = RequestPermissionProfile { - file_system: Some(FileSystemPermissions::from_read_write_roots( - Some(vec![]), - Some(vec![ - AbsolutePathBuf::try_from(first_dir.path().canonicalize()?)?, - AbsolutePathBuf::try_from(second_dir.path().canonicalize()?)?, - ]), - )), - ..RequestPermissionProfile::default() - }; - let granted_permissions = normalized_directory_write_permissions(first_dir.path())?; + let normalized_requested_permissions = requested_permissions.clone(); + let granted_permissions = requested_directory_write_permissions(first_dir.path()); let second_dir_permissions = requested_directory_write_permissions(second_dir.path()); - let merged_permissions = PermissionProfile { - file_system: Some(FileSystemPermissions::from_read_write_roots( - Some(vec![]), - Some(vec![ - AbsolutePathBuf::try_from(first_dir.path().canonicalize()?)?, - AbsolutePathBuf::try_from(second_dir.path().canonicalize()?)?, - ]), - )), - ..Default::default() - }; + let merged_permissions = requested_permissions.clone(); let responses = mount_sse_sequence( &server, @@ -1835,7 +1780,7 @@ async fn request_permissions_grants_do_not_carry_across_turns() -> Result<()> { let outside_dir = tempfile::tempdir()?; let requested_permissions = requested_directory_write_permissions(outside_dir.path()); let normalized_requested_permissions = - normalized_directory_write_permissions(outside_dir.path())?; + requested_directory_write_permissions(outside_dir.path()); let _first_turn = mount_sse_sequence( &server, @@ -1952,7 +1897,7 @@ async fn request_permissions_session_grants_carry_across_turns() -> Result<()> { let outside_write = outside_dir.path().join("session-sticky-write.txt"); let requested_permissions = requested_directory_write_permissions(outside_dir.path()); let normalized_requested_permissions = - normalized_directory_write_permissions(outside_dir.path())?; + requested_directory_write_permissions(outside_dir.path()); let command = format!( "printf {:?} > {:?} && cat {:?}", "session-sticky-ok", outside_write, outside_write diff --git a/codex-rs/core/tests/suite/request_permissions_tool.rs b/codex-rs/core/tests/suite/request_permissions_tool.rs index 9538743eaead..e0adff8dedc4 100644 --- a/codex-rs/core/tests/suite/request_permissions_tool.rs +++ b/codex-rs/core/tests/suite/request_permissions_tool.rs @@ -98,16 +98,6 @@ fn requested_directory_write_permissions(path: &Path) -> RequestPermissionProfil } } -fn normalized_directory_write_permissions(path: &Path) -> Result { - Ok(RequestPermissionProfile { - file_system: Some(FileSystemPermissions::from_read_write_roots( - Some(vec![]), - Some(vec![AbsolutePathBuf::try_from(path.canonicalize()?)?]), - )), - ..RequestPermissionProfile::default() - }) -} - fn parse_result(item: &Value) -> (Option, String) { let output_str = item .get("output") @@ -241,7 +231,7 @@ async fn approved_folder_write_request_permissions_unblocks_later_exec_without_s ); let requested_permissions = requested_directory_write_permissions(requested_dir.path()); let normalized_requested_permissions = - normalized_directory_write_permissions(requested_dir.path())?; + requested_directory_write_permissions(requested_dir.path()); let responses = mount_sse_sequence( &server, @@ -378,13 +368,10 @@ async fn apply_patch_after_request_permissions(strict_auto_review: bool) -> Resu } else { "patched-via-request-permissions" }; - let requested_file = requested_dir - .path() - .canonicalize()? - .join(requested_file_name); + let requested_file = requested_dir.path().join(requested_file_name); let requested_permissions = requested_directory_write_permissions(requested_dir.path()); let normalized_requested_permissions = - normalized_directory_write_permissions(requested_dir.path())?; + requested_directory_write_permissions(requested_dir.path()); let patch = build_add_file_patch(&requested_file, patch_content); let response_prefix = if strict_auto_review { diff --git a/codex-rs/protocol/src/permissions.rs b/codex-rs/protocol/src/permissions.rs index 4f6303a7096a..dcdae60f287e 100644 --- a/codex-rs/protocol/src/permissions.rs +++ b/codex-rs/protocol/src/permissions.rs @@ -288,15 +288,43 @@ struct FileSystemSemanticSignature { /// Runtime matcher for read-deny entries in a filesystem sandbox policy. pub struct ReadDenyMatcher { - cwd: AbsolutePathBuf, + native_cwd: Option, user_home_dir: Option, temporary_directories: Vec, + prepared: PreparedReadDenyMatcher, +} + +/// Prepared PathUri deny roots and globs for repeated executor-owned read checks. +struct PreparedReadDenyMatcher { denied_roots: Vec, deny_read_matchers: Vec, invalid_pattern: bool, } impl ReadDenyMatcher { + /// Builds a matcher for executor-owned URI paths without host projection. + pub fn from_context( + file_system_sandbox_policy: &FileSystemSandboxPolicy, + context: &FileSystemSandboxPolicyContext<'_>, + ) -> Option { + file_system_sandbox_policy + .entries + .iter() + .any(|entry| entry.access == FileSystemAccessMode::Deny) + .then(|| { + file_system_sandbox_policy + .prepare_deny_read_matcher(context, InvalidDenyReadGlobBehavior::FailClosed) + .map(|prepared| Self { + native_cwd: None, + user_home_dir: None, + temporary_directories: Vec::new(), + prepared, + }) + .ok() + }) + .flatten() + } + /// Builds a matcher from exact deny-read roots and deny-read glob entries. /// /// Returns `None` when the policy has no deny-read restrictions, so callers @@ -348,38 +376,42 @@ impl ReadDenyMatcher { 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 + let prepared = file_system_sandbox_policy .prepare_deny_read_matcher(&context, invalid_glob_behavior)?; Ok(Some(Self { - cwd, + native_cwd: Some(cwd), user_home_dir, temporary_directories, - denied_roots, - deny_read_matchers, - invalid_pattern, + prepared, })) } /// Returns whether `path` is denied by the policy used to build this matcher. pub fn is_read_denied(&self, path: &Path) -> bool { - let Some(path) = resolve_candidate_path(path, self.cwd.as_path()) else { + let Some(cwd) = self.native_cwd.as_ref() else { + return true; + }; + let Some(path) = resolve_candidate_path(path, cwd.as_path()) else { return true; }; let path = PathUri::from(path); - let cwd = PathUri::from_abs_path(&self.cwd); + let cwd = PathUri::from_abs_path(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, - ) + self.is_read_denied_uri(&path, &context) + } + + /// Returns whether an executor-owned URI is denied under its matching path context. + pub fn is_read_denied_uri( + &self, + path: &PathUri, + context: &FileSystemSandboxPolicyContext<'_>, + ) -> bool { + FileSystemSandboxPolicy::matches_prepared_read_deny(path, context, &self.prepared) } /// Checks an enumerated path using a canonical location already resolved by @@ -891,7 +923,7 @@ impl FileSystemSandboxPolicy { .unwrap_or(false) } - fn resolve_access( + pub fn resolve_access( &self, path: &PathUri, context: &FileSystemSandboxPolicyContext<'_>, @@ -973,7 +1005,7 @@ impl FileSystemSandboxPolicy { &self, context: &FileSystemSandboxPolicyContext<'_>, invalid_glob_behavior: InvalidDenyReadGlobBehavior, - ) -> Result<(Vec, Vec, bool), String> { + ) -> Result { let file_system_root = file_system_root(context); let denied_roots = self .resolved_entries(context) @@ -987,7 +1019,11 @@ impl FileSystemSandboxPolicy { .map(|(root, _)| root) .collect(); let Some(convention) = context.cwd.infer_path_convention() else { - return Ok((denied_roots, Vec::new(), true)); + return Ok(PreparedReadDenyMatcher { + denied_roots, + deny_read_matchers: Vec::new(), + invalid_pattern: true, + }); }; let mut deny_read_matchers = Vec::new(); let mut invalid_pattern = false; @@ -995,7 +1031,11 @@ impl FileSystemSandboxPolicy { Ok(patterns) => patterns, Err(err) => match invalid_glob_behavior { InvalidDenyReadGlobBehavior::FailClosed => { - return Ok((denied_roots, Vec::new(), true)); + return Ok(PreparedReadDenyMatcher { + denied_roots, + deny_read_matchers: Vec::new(), + invalid_pattern: true, + }); } InvalidDenyReadGlobBehavior::ReturnError => return Err(err), }, @@ -1011,15 +1051,17 @@ impl FileSystemSandboxPolicy { }, } } - Ok((denied_roots, deny_read_matchers, invalid_pattern)) + Ok(PreparedReadDenyMatcher { + 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, + prepared: &PreparedReadDenyMatcher, ) -> bool { let Some(convention) = context.cwd.infer_path_convention() else { return true; @@ -1030,11 +1072,14 @@ impl FileSystemSandboxPolicy { { return true; } - if invalid_pattern { + if prepared.invalid_pattern { return true; } - denied_roots.iter().any(|root| path.starts_with(root)) - || deny_read_matchers.iter().any(|matcher| { + prepared + .denied_roots + .iter() + .any(|root| path.starts_with(root)) + || prepared.deny_read_matchers.iter().any(|matcher| { let path = match convention { PathConvention::Posix => path.decoded_path_bytes(), PathConvention::Windows => Cow::Owned( @@ -1897,7 +1942,7 @@ fn with_local_policy_context( Some(evaluate(&path, &context)) } -fn file_system_root(context: &FileSystemSandboxPolicyContext<'_>) -> Option { +pub fn file_system_root(context: &FileSystemSandboxPolicyContext<'_>) -> Option { context.cwd.lexical_depth()?; context.cwd.ancestors().last() } @@ -2725,25 +2770,21 @@ mod tests { ), unreadable_glob_entry(r"C:\workspace\**\*.env".to_string()), ]); - let (roots, globs, invalid) = policy + let prepared = policy .prepare_deny_read_matcher(&context, InvalidDenyReadGlobBehavior::ReturnError) .expect("remote deny matcher"); - assert!(roots.is_empty()); + assert!(prepared.denied_roots.is_empty()); assert!(FileSystemSandboxPolicy::matches_prepared_read_deny( &path("file:///c:/WORKSPACE/app/.ENV"), &context, - &roots, - &globs, - invalid, + &prepared, )); for candidate in ["file:///%00/bad/path/YQ", "file:///C:/workspace/%2Fsecret"] { assert!(FileSystemSandboxPolicy::matches_prepared_read_deny( &path(candidate), &context, - &roots, - &globs, - invalid, + &prepared, )); } assert!(!policy.can_write_path(&path("file:///C:/workspace/.codex/config"), &context)); @@ -2793,12 +2834,12 @@ mod tests { let policy = FileSystemSandboxPolicy::restricted(vec![unreadable_glob_entry( pattern.to_string(), )]); - let (roots, globs, invalid) = policy + let prepared = policy .prepare_deny_read_matcher(&context, InvalidDenyReadGlobBehavior::ReturnError) .expect("home-relative deny glob"); assert!(FileSystemSandboxPolicy::matches_prepared_read_deny( - &candidate, &context, &roots, &globs, invalid, + &candidate, &context, &prepared, )); let without_home = FileSystemSandboxPolicyContext { @@ -2813,10 +2854,10 @@ mod tests { ) .is_err() ); - let (_, _, invalid) = policy + let prepared = policy .prepare_deny_read_matcher(&without_home, InvalidDenyReadGlobBehavior::FailClosed) .expect("missing executor home fails closed"); - assert!(invalid); + assert!(prepared.invalid_pattern); } } diff --git a/codex-rs/sandboxing/src/policy_transforms.rs b/codex-rs/sandboxing/src/policy_transforms.rs index 8012564f3a2c..ad3573084363 100644 --- a/codex-rs/sandboxing/src/policy_transforms.rs +++ b/codex-rs/sandboxing/src/policy_transforms.rs @@ -7,15 +7,16 @@ use codex_protocol::permissions::FileSystemPath; use codex_protocol::permissions::FileSystemSandboxEntry; use codex_protocol::permissions::FileSystemSandboxKind; use codex_protocol::permissions::FileSystemSandboxPolicy; +use codex_protocol::permissions::FileSystemSandboxPolicyContext; use codex_protocol::permissions::FileSystemSpecialPath; use codex_protocol::permissions::NetworkSandboxPolicy; use codex_protocol::permissions::ReadDenyMatcher; +use codex_protocol::permissions::file_system_root; use codex_utils_absolute_path::AbsolutePathBuf; -use codex_utils_absolute_path::canonicalize_preserving_symlinks; +use codex_utils_path_uri::PathConvention; use codex_utils_path_uri::PathUri; use std::num::NonZeroUsize; use std::path::Path; -use std::path::PathBuf; pub fn normalize_additional_permissions( additional_permissions: AdditionalPermissionProfile, @@ -35,28 +36,8 @@ pub fn normalize_additional_permissions( "glob file system permissions only support deny-read entries".to_string(), ); } - let path = match entry.path { - FileSystemPath::Path { path } => FileSystemPath::Path { - path: path - .to_abs_path() - .ok() - .and_then(|path| canonicalize_preserving_symlinks(path.as_path()).ok()) - .and_then(|path| AbsolutePathBuf::from_absolute_path(path).ok()) - .map(Into::into) - .unwrap_or(path), - }, - FileSystemPath::GlobPattern { pattern } => { - FileSystemPath::GlobPattern { pattern } - } - FileSystemPath::Special { value } => FileSystemPath::Special { value }, - }; - let normalized_entry = FileSystemSandboxEntry { - path, - access: entry.access, - missing_path_behavior: entry.missing_path_behavior, - }; - if !entries.contains(&normalized_entry) { - entries.push(normalized_entry); + if !entries.contains(&entry) { + entries.push(entry); } } let file_system = FileSystemPermissions { @@ -73,19 +54,55 @@ pub fn normalize_additional_permissions( }) } +pub fn normalize_additional_permissions_with_context( + additional_permissions: AdditionalPermissionProfile, + context: &FileSystemSandboxPolicyContext<'_>, +) -> Result { + let normalized = normalize_additional_permissions(additional_permissions)?; + if let Some(file_system) = normalized.file_system.as_ref() { + for entry in &file_system.entries { + let FileSystemPath::Path { path } = &entry.path else { + continue; + }; + if path.infer_path_convention().is_none() + || path.infer_path_convention() != context.cwd.infer_path_convention() + || path.join(".").is_err() + || context.cwd.join(".").is_err() + { + return Err(format!( + "permission path `{path}` does not match executor cwd `{}`", + context.cwd + )); + } + } + } + Ok(normalized) +} + /// Resolves cwd-dependent permission entries without filtering their authority. /// /// Unlike intersection, this preserves narrower grants beneath denied paths. pub fn materialize_additional_permissions( - mut additional_permissions: AdditionalPermissionProfile, + additional_permissions: AdditionalPermissionProfile, cwd: &Path, +) -> Result { + let cwd = AbsolutePathBuf::from_absolute_path(cwd) + .map_err(|err| format!("invalid permission cwd: {err}"))?; + let cwd = PathUri::from(cwd); + with_local_context_for_cwd(&cwd, |context| { + materialize_additional_permissions_with_context(additional_permissions, context) + }) +} + +pub fn materialize_additional_permissions_with_context( + mut additional_permissions: AdditionalPermissionProfile, + context: &FileSystemSandboxPolicyContext<'_>, ) -> Result { if let Some(file_system) = additional_permissions.file_system.as_mut() { - for entry in &mut file_system.entries { - *entry = materialize_cwd_dependent_entry(entry, cwd); - } + file_system.entries = materialize_context_dependent_entries(&file_system.entries, context) + .ok_or_else(|| format!("unable to resolve permission path in `{}`", context.cwd))?; } - normalize_additional_permissions(additional_permissions) + normalize_additional_permissions_with_context(additional_permissions, context) } pub fn merge_permission_profiles( @@ -146,43 +163,61 @@ pub fn intersect_permission_profiles( requested: AdditionalPermissionProfile, granted: AdditionalPermissionProfile, cwd: &Path, +) -> AdditionalPermissionProfile { + AbsolutePathBuf::from_absolute_path(cwd) + .ok() + .map(PathUri::from) + .map_or_else(AdditionalPermissionProfile::default, |cwd| { + with_local_context_for_cwd(&cwd, |context| { + intersect_permission_profiles_with_context(requested, granted, context) + }) + }) +} + +pub fn intersect_permission_profiles_with_context( + requested: AdditionalPermissionProfile, + granted: AdditionalPermissionProfile, + context: &FileSystemSandboxPolicyContext<'_>, ) -> AdditionalPermissionProfile { let file_system = requested .file_system - .map(|requested_file_system| { + .and_then(|requested_file_system| { let granted_file_system = granted.file_system.unwrap_or_default(); - let requested_policy = - FileSystemSandboxPolicy::restricted(requested_file_system.entries.clone()); - let requested_read_deny_matcher = ReadDenyMatcher::new(&requested_policy, cwd); + let requested_entries = + materialize_context_dependent_entries(&requested_file_system.entries, context)?; + let granted_entries = + materialize_context_dependent_entries(&granted_file_system.entries, context)?; + let requested_policy = FileSystemSandboxPolicy::restricted(requested_entries.clone()); + let requested_read_deny_matcher = + ReadDenyMatcher::from_context(&requested_policy, context); let mut accepted_entries = Vec::new(); - for entry in granted_file_system.entries.iter().filter(|entry| { + for entry in granted_entries.iter().filter(|entry| { granted_file_system_entry_within_request( &requested_file_system, &requested_policy, requested_read_deny_matcher.as_ref(), entry, - cwd, + context, ) }) { - let entry = materialize_cwd_dependent_entry(entry, cwd); - if !accepted_entries.contains(&entry) { - accepted_entries.push(entry); + if !accepted_entries.contains(entry) { + accepted_entries.push(entry.clone()); } } let mut entries = accepted_entries.clone(); let requested_retained_deny_entries = retain_constraining_deny_entries( - &requested_file_system.entries, + &requested_entries, &accepted_entries, - cwd, + context, &mut entries, ); let granted_retained_deny_entries = retain_constraining_deny_entries( - &granted_file_system.entries, + &granted_entries, &accepted_entries, - cwd, + context, &mut entries, ); - FileSystemPermissions { + Some(FileSystemPermissions { glob_scan_max_depth: merge_glob_scan_max_depth( &requested_retained_deny_entries, requested_file_system.glob_scan_max_depth.map(usize::from), @@ -191,7 +226,7 @@ pub fn intersect_permission_profiles( ) .and_then(NonZeroUsize::new), entries, - } + }) }) .filter(|file_system| !file_system.is_empty()); let network = match (requested.network, granted.network) { @@ -214,6 +249,46 @@ pub fn intersect_permission_profiles( } } +fn with_local_context_for_cwd( + cwd: &PathUri, + evaluate: impl FnOnce(&FileSystemSandboxPolicyContext<'_>) -> T, +) -> T { + let user_home_dir = PathUri::from_host_native_path("~").ok(); + let local_cwd = std::env::current_dir().ok(); + let temporary_directory_env_vars: &[&str] = if cfg!(windows) { + &["TEMP", "TMP"] + } else { + &["TMPDIR"] + }; + let normalize_temp_path = |path: std::ffi::OsString| { + PathUri::from_host_native_path(&path).ok().or_else(|| { + if cfg!(unix) { + PathUri::from_host_native_path(local_cwd.as_ref()?.join(path)).ok() + } else { + None + } + }) + }; + let mut temporary_directories = Vec::new(); + for name in temporary_directory_env_vars { + if let Some(path) = std::env::var_os(name) + .filter(|path| !path.is_empty()) + .filter(|path| cfg!(unix) || Path::new(path).is_absolute()) + .and_then(&normalize_temp_path) + && !temporary_directories.contains(&path) + { + temporary_directories.push(path); + } + } + let context = FileSystemSandboxPolicyContext { + cwd, + workspace_roots: std::slice::from_ref(cwd), + user_home_dir: user_home_dir.as_ref(), + temporary_directories: Some(&temporary_directories), + }; + evaluate(&context) +} + fn merge_glob_scan_max_depth( left_entries: &[FileSystemSandboxEntry], left_depth: Option, @@ -261,26 +336,37 @@ fn granted_file_system_entry_within_request( requested_policy: &FileSystemSandboxPolicy, requested_read_deny_matcher: Option<&ReadDenyMatcher>, granted_entry: &FileSystemSandboxEntry, - cwd: &Path, + context: &FileSystemSandboxPolicyContext<'_>, ) -> bool { if !granted_entry.access.can_read() || matches!( &granted_entry.path, FileSystemPath::Special { value: FileSystemSpecialPath::SlashTmp, - } if !cfg!(unix) + } if context.cwd.infer_path_convention() != Some(PathConvention::Posix) ) { return false; } + if context.cwd.infer_path_convention() == Some(PathConvention::Windows) + && is_root_entry(granted_entry) + && !requested.entries.iter().any(|requested_entry| { + is_root_entry(requested_entry) + && access_covers(requested_entry.access, granted_entry.access) + }) + { + return false; + } - if let Some(path) = resolve_permission_path(&granted_entry.path, cwd) { - if requested_read_deny_matcher.is_some_and(|matcher| matcher.is_read_denied(path.as_path())) + if let Some(path) = resolve_permission_path(&granted_entry.path, context) { + if path.infer_path_convention() != context.cwd.infer_path_convention() + || requested_read_deny_matcher + .is_some_and(|matcher| matcher.is_read_denied_uri(&path, context)) { return false; } return access_covers( - requested_policy.resolve_access_with_cwd(path.as_path(), cwd), + requested_policy.resolve_access(&path, context), granted_entry.access, ); } @@ -294,7 +380,7 @@ fn granted_file_system_entry_within_request( fn retain_constraining_deny_entries( source_entries: &[FileSystemSandboxEntry], accepted_entries: &[FileSystemSandboxEntry], - cwd: &Path, + context: &FileSystemSandboxPolicyContext<'_>, output_entries: &mut Vec, ) -> Vec { let mut retained_entries = Vec::new(); @@ -302,14 +388,13 @@ fn retain_constraining_deny_entries( .iter() .filter(|entry| entry.access == FileSystemAccessMode::Deny) { - if !deny_entry_constrains_accepted_grant(entry, accepted_entries, cwd) { + if !deny_entry_constrains_accepted_grant(entry, accepted_entries, context) { continue; } - let entry = materialize_cwd_dependent_entry(entry, cwd); - if !output_entries.contains(&entry) { + if !output_entries.contains(entry) { output_entries.push(entry.clone()); } - retained_entries.push(entry); + retained_entries.push(entry.clone()); } retained_entries } @@ -317,51 +402,57 @@ fn retain_constraining_deny_entries( fn deny_entry_constrains_accepted_grant( deny_entry: &FileSystemSandboxEntry, accepted_entries: &[FileSystemSandboxEntry], - cwd: &Path, + context: &FileSystemSandboxPolicyContext<'_>, ) -> bool { accepted_entries .iter() .filter(|entry| entry.access.can_read()) .any(|entry| { - let Some(grant_path) = resolve_permission_path(&entry.path, cwd) else { + if is_root_entry(entry) { + return true; + } + let Some(grant_path) = resolve_permission_path(&entry.path, context) else { return false; }; match &deny_entry.path { - FileSystemPath::GlobPattern { pattern } => glob_static_prefix_path(pattern, cwd) - .is_some_and(|prefix| paths_may_overlap(&prefix, &grant_path)), + FileSystemPath::GlobPattern { pattern } => { + glob_static_prefix_path(pattern, context) + .is_none_or(|prefix| paths_overlap(&prefix, &grant_path)) + } FileSystemPath::Path { .. } | FileSystemPath::Special { .. } => { - resolve_permission_path(&deny_entry.path, cwd) - .is_some_and(|deny_path| paths_may_overlap(&deny_path, &grant_path)) + resolve_permission_path(&deny_entry.path, context) + .is_none_or(|deny_path| paths_overlap(&deny_path, &grant_path)) } } }) } -fn glob_static_prefix_path(pattern: &str, cwd: &Path) -> Option { - let resolved_pattern = AbsolutePathBuf::resolve_path_against_base(pattern, cwd); - let resolved_pattern = resolved_pattern.as_path().to_string_lossy(); - let prefix = match resolved_pattern.find(['*', '?', '[', ']']) { +fn glob_static_prefix_path( + pattern: &str, + context: &FileSystemSandboxPolicyContext<'_>, +) -> Option { + let is_windows = context.cwd.infer_path_convention() == Some(PathConvention::Windows); + let (prefix, wildcard_in_segment) = match pattern.find(['*', '?', '[', ']']) { Some(0) => return None, Some(index) => { - let prefix = &resolved_pattern[..index]; - if prefix.ends_with(std::path::MAIN_SEPARATOR) - || prefix.ends_with('/') - || prefix.ends_with('\\') - { - Path::new(prefix) - } else { - Path::new(prefix).parent()? - } + let prefix = &pattern[..index]; + ( + prefix, + !(prefix.ends_with('/') || is_windows && prefix.ends_with('\\')), + ) } - None => Path::new(resolved_pattern.as_ref()), + None => (pattern, false), }; - AbsolutePathBuf::from_absolute_path(prefix).ok() + let prefix = context.cwd.join(prefix).ok()?; + if wildcard_in_segment { + prefix.parent() + } else { + Some(prefix) + } } -fn paths_may_overlap(left: &AbsolutePathBuf, right: &AbsolutePathBuf) -> bool { - let left = PathUri::from_abs_path(left); - let right = PathUri::from_abs_path(right); - left.overlaps(&right).unwrap_or(true) +fn paths_overlap(left: &PathUri, right: &PathUri) -> bool { + left.overlaps(right).unwrap_or(true) } fn access_covers(requested: FileSystemAccessMode, granted: FileSystemAccessMode) -> bool { @@ -372,63 +463,70 @@ fn access_covers(requested: FileSystemAccessMode, granted: FileSystemAccessMode) } } +fn is_root_entry(entry: &FileSystemSandboxEntry) -> bool { + matches!( + &entry.path, + FileSystemPath::Special { + value: FileSystemSpecialPath::Root, + } + ) +} + fn materialize_cwd_dependent_entry( entry: &FileSystemSandboxEntry, - cwd: &Path, -) -> FileSystemSandboxEntry { + context: &FileSystemSandboxPolicyContext<'_>, +) -> Option { match &entry.path { - FileSystemPath::Special { - value: FileSystemSpecialPath::ProjectRoots { .. }, - } => resolve_permission_path(&entry.path, cwd) - .map(|path| FileSystemSandboxEntry { - path: path.into(), + FileSystemPath::GlobPattern { pattern } => { + let is_windows = context.cwd.infer_path_convention() == Some(PathConvention::Windows); + let home_relative = pattern + .strip_prefix("~/") + .or_else(|| (pattern == "~").then_some("")) + .or_else(|| is_windows.then(|| pattern.strip_prefix("~\\")).flatten()); + let (root, pattern) = match home_relative { + Some(suffix) => ( + context.user_home_dir?, + suffix.trim_start_matches(|separator| { + separator == '/' || is_windows && separator == '\\' + }), + ), + None => (context.cwd, pattern.as_str()), + }; + let path = root.join(pattern).ok()?; + let path = FileSystemPath::GlobPattern { + pattern: path.inferred_native_path_string(), + }; + Some(FileSystemSandboxEntry { + path, access: entry.access, missing_path_behavior: entry.missing_path_behavior, }) - .unwrap_or_else(|| entry.clone()), - FileSystemPath::GlobPattern { pattern } => FileSystemSandboxEntry { - path: FileSystemPath::GlobPattern { - pattern: AbsolutePathBuf::resolve_path_against_base(pattern, cwd) - .to_string_lossy() - .into_owned(), - }, - access: entry.access, - missing_path_behavior: entry.missing_path_behavior, - }, - FileSystemPath::Path { .. } | FileSystemPath::Special { .. } => entry.clone(), + } + FileSystemPath::Path { .. } | FileSystemPath::Special { .. } => Some(entry.clone()), } } -fn resolve_permission_path(path: &FileSystemPath, cwd: &Path) -> Option { +fn resolve_permission_path( + path: &FileSystemPath, + context: &FileSystemSandboxPolicyContext<'_>, +) -> Option { match path { - FileSystemPath::Path { path } => path.to_abs_path().ok(), + FileSystemPath::Path { path } => Some(path.clone()), FileSystemPath::GlobPattern { .. } => None, FileSystemPath::Special { value } => match value { - FileSystemSpecialPath::Root => { - let root = cwd.ancestors().last()?; - AbsolutePathBuf::from_absolute_path(root).ok() - } + FileSystemSpecialPath::Root => file_system_root(context), FileSystemSpecialPath::ProjectRoots { subpath } => { - let cwd = AbsolutePathBuf::from_absolute_path(cwd).ok()?; - Some(match subpath { - Some(subpath) => { - AbsolutePathBuf::resolve_path_against_base(subpath, cwd.as_path()) - } - None => cwd, - }) - } - FileSystemSpecialPath::Tmpdir => { - let tmpdir = std::env::var_os("TMPDIR")?; - if tmpdir.is_empty() { - None - } else { - AbsolutePathBuf::from_absolute_path(PathBuf::from(tmpdir)).ok() + let root = context.workspace_roots.first()?; + match subpath { + Some(subpath) => root.join(subpath).ok(), + None => Some(root.clone()), } } - FileSystemSpecialPath::SlashTmp if cfg!(unix) => { - AbsolutePathBuf::from_absolute_path("/tmp") - .ok() - .filter(|path| path.as_path().is_dir()) + FileSystemSpecialPath::Tmpdir => context.temporary_directories?.first().cloned(), + FileSystemSpecialPath::SlashTmp + if context.cwd.infer_path_convention() == Some(PathConvention::Posix) => + { + context.cwd.join("/tmp").ok() } FileSystemSpecialPath::SlashTmp | FileSystemSpecialPath::Minimal @@ -437,6 +535,81 @@ fn resolve_permission_path(path: &FileSystemPath, cwd: &Path) -> Option, +) -> Option> { + let mut materialized = Vec::new(); + for entry in entries { + match &entry.path { + FileSystemPath::Special { + value: FileSystemSpecialPath::ProjectRoots { .. }, + } => { + let mut resolved = Vec::new(); + for root in context.workspace_roots { + let mut root_context = *context; + root_context.workspace_roots = std::slice::from_ref(root); + let Some(path) = resolve_permission_path(&entry.path, &root_context) else { + if entry.access == FileSystemAccessMode::Deny { + return None; + } + continue; + }; + resolved.push(materialized_path_entry(entry, path)); + } + if entry.access == FileSystemAccessMode::Deny && resolved.is_empty() { + return None; + } + materialized.extend(resolved); + } + FileSystemPath::Special { + value: FileSystemSpecialPath::Tmpdir, + } => { + let Some(temporary_directories) = context.temporary_directories else { + if entry.access == FileSystemAccessMode::Deny { + return None; + } + materialized.push(entry.clone()); + continue; + }; + materialized.extend( + temporary_directories + .iter() + .cloned() + .map(|path| materialized_path_entry(entry, path)), + ); + } + FileSystemPath::Special { + value: FileSystemSpecialPath::Root, + } => { + resolve_permission_path(&entry.path, context)?; + materialized.push(entry.clone()); + } + _ => { + let Some(entry) = materialize_cwd_dependent_entry(entry, context) else { + if entry.access == FileSystemAccessMode::Deny { + return None; + } + continue; + }; + materialized.push(entry); + } + } + } + Some(materialized) +} + +fn materialized_path_entry( + entry: &FileSystemSandboxEntry, + path: PathUri, +) -> FileSystemSandboxEntry { + FileSystemSandboxEntry { + path: FileSystemPath::Path { path }, + access: entry.access, + missing_path_behavior: entry.missing_path_behavior, + } +} + fn merge_permission_entries( base: &[FileSystemSandboxEntry], permissions: &[FileSystemSandboxEntry], diff --git a/codex-rs/sandboxing/src/policy_transforms_tests.rs b/codex-rs/sandboxing/src/policy_transforms_tests.rs index ae7fe9ae4189..97334bfc404f 100644 --- a/codex-rs/sandboxing/src/policy_transforms_tests.rs +++ b/codex-rs/sandboxing/src/policy_transforms_tests.rs @@ -1,8 +1,11 @@ use super::effective_file_system_sandbox_policy; use super::intersect_permission_profiles; +use super::intersect_permission_profiles_with_context; use super::materialize_additional_permissions; +use super::materialize_additional_permissions_with_context; use super::merge_file_system_policy_with_additional_permissions; use super::normalize_additional_permissions; +use super::normalize_additional_permissions_with_context; use super::should_require_platform_sandbox; use codex_protocol::models::AdditionalPermissionProfile as PermissionProfile; use codex_protocol::models::FileSystemPermissions; @@ -11,9 +14,11 @@ use codex_protocol::permissions::FileSystemAccessMode; use codex_protocol::permissions::FileSystemPath; use codex_protocol::permissions::FileSystemSandboxEntry; use codex_protocol::permissions::FileSystemSandboxPolicy; +use codex_protocol::permissions::FileSystemSandboxPolicyContext; use codex_protocol::permissions::FileSystemSpecialPath; use codex_protocol::permissions::NetworkSandboxPolicy; use codex_utils_absolute_path::AbsolutePathBuf; +use codex_utils_path_uri::PathUri; use dunce::canonicalize; use pretty_assertions::assert_eq; #[cfg(unix)] @@ -129,6 +134,39 @@ fn normalize_additional_permissions_preserves_network() { ); } +#[test] +fn normalize_additional_permissions_only_checks_convention_with_context() { + let windows_path = PathUri::parse("file:///C:/workspace/out").expect("Windows path URI"); + let permissions = PermissionProfile { + file_system: Some(FileSystemPermissions { + entries: vec![FileSystemSandboxEntry::new( + FileSystemPath::Path { path: windows_path }, + FileSystemAccessMode::Write, + )], + glob_scan_max_depth: None, + }), + ..Default::default() + }; + + assert_eq!( + normalize_additional_permissions(permissions.clone()).expect("context-free permissions"), + permissions + ); + + let cwd = PathUri::parse("file:///workspace").expect("POSIX cwd URI"); + let workspace_roots = [cwd.clone()]; + let context = FileSystemSandboxPolicyContext { + cwd: &cwd, + workspace_roots: &workspace_roots, + user_home_dir: None, + temporary_directories: None, + }; + assert!( + normalize_additional_permissions_with_context(permissions, &context).is_err(), + "context-aware normalization should reject mismatched conventions" + ); +} + #[cfg(unix)] #[test] fn normalize_additional_permissions_preserves_symlinked_write_paths() { @@ -242,6 +280,7 @@ fn materialize_additional_permissions_preserves_authority_and_constraints() { canonicalize(temp_dir.path()).expect("canonicalize temp dir"), ) .expect("absolute temp dir"); + let home = PathUri::from_host_native_path("~").expect("host home"); let project_path = |subpath: &str| FileSystemPath::Special { value: FileSystemSpecialPath::project_roots(Some(subpath.to_owned())), }; @@ -263,6 +302,8 @@ fn materialize_additional_permissions_preserves_authority_and_constraints() { FileSystemSandboxEntry::new(project_path("private/reopened"), Write), reopened.clone(), deny_glob("**/*.env".to_owned()), + deny_glob("~".to_owned()), + deny_glob("~/private/*.env".to_owned()), ]); let expected = profile(vec![ FileSystemSandboxEntry::new(cwd.clone().into(), Write), @@ -270,6 +311,12 @@ fn materialize_additional_permissions_preserves_authority_and_constraints() { FileSystemSandboxEntry::skip_missing_path(cwd.join("readonly").into(), Read), reopened, deny_glob(cwd.join("**/*.env").to_string_lossy().into_owned()), + deny_glob(home.inferred_native_path_string()), + deny_glob( + home.join("private/*.env") + .expect("home-relative deny glob") + .inferred_native_path_string(), + ), ]); assert_eq!( @@ -279,6 +326,50 @@ fn materialize_additional_permissions_preserves_authority_and_constraints() { ); } +#[test] +fn materialize_additional_permissions_ignores_empty_tmpdir_deny() { + let cwd = PathUri::parse("file:///workspace").expect("cwd URI"); + let workspace_roots = [cwd.clone()]; + let temporary_directories = []; + let context = FileSystemSandboxPolicyContext { + cwd: &cwd, + workspace_roots: &workspace_roots, + user_home_dir: None, + temporary_directories: Some(&temporary_directories), + }; + let write = FileSystemSandboxEntry::new( + FileSystemPath::Path { path: cwd.clone() }, + FileSystemAccessMode::Write, + ); + let permissions = PermissionProfile { + file_system: Some(FileSystemPermissions { + entries: vec![ + write.clone(), + FileSystemSandboxEntry::new( + FileSystemPath::Special { + value: FileSystemSpecialPath::Tmpdir, + }, + FileSystemAccessMode::Deny, + ), + ], + glob_scan_max_depth: None, + }), + ..Default::default() + }; + + assert_eq!( + materialize_additional_permissions_with_context(permissions, &context) + .expect("permissions"), + PermissionProfile { + file_system: Some(FileSystemPermissions { + entries: vec![write], + glob_scan_max_depth: None, + }), + ..Default::default() + } + ); +} + #[test] fn intersect_permission_profiles_preserves_explicit_empty_requested_reads() { let temp_dir = TempDir::new().expect("create temp dir"); @@ -449,6 +540,58 @@ fn intersect_permission_profiles_preserves_deny_across_case_variant_grant() { ); } +#[test] +fn intersect_permission_profiles_preserves_rooted_first_segment_glob_deny() { + use FileSystemAccessMode::Deny; + use FileSystemAccessMode::Write; + + for (cwd_uri, grant_uri, pattern) in [ + ("file:///workspace", "file:///fooX", "/foo*/*"), + ("file:///workspace", "file:///foo%5Cbar", r"/foo\\*/*"), + ("file:///C:/workspace", "file:///C:/fooX", r"C:\foo*\*"), + ( + "file://server/share/workspace", + "file://server/share/fooX", + r"\\server\share\foo*\*", + ), + ] { + let cwd = PathUri::parse(cwd_uri).expect("cwd URI"); + let grant_path = PathUri::parse(grant_uri).expect("grant URI"); + let workspace_roots = [cwd.clone()]; + let context = FileSystemSandboxPolicyContext { + cwd: &cwd, + workspace_roots: &workspace_roots, + user_home_dir: None, + temporary_directories: None, + }; + let grant_entry = + FileSystemSandboxEntry::new(FileSystemPath::Path { path: grant_path }, Write); + let deny_entry = FileSystemSandboxEntry::new( + FileSystemPath::GlobPattern { + pattern: pattern.to_string(), + }, + Deny, + ); + let profile = |entries| PermissionProfile { + file_system: Some(FileSystemPermissions { + entries, + glob_scan_max_depth: None, + }), + ..Default::default() + }; + + assert_eq!( + intersect_permission_profiles_with_context( + profile(vec![grant_entry.clone(), deny_entry.clone()]), + profile(vec![grant_entry.clone()]), + &context, + ), + profile(vec![grant_entry, deny_entry]), + "deny glob should survive for {pattern}", + ); + } +} + #[test] fn intersect_permission_profiles_preserves_opaque_child_deny() { use std::ffi::OsString;