diff --git a/codex-rs/protocol/src/permissions.rs b/codex-rs/protocol/src/permissions.rs index 38d9c4b78f..511d84557d 100644 --- a/codex-rs/protocol/src/permissions.rs +++ b/codex-rs/protocol/src/permissions.rs @@ -54,23 +54,11 @@ pub fn forbidden_agent_metadata_write( return None; } - let target = resolve_candidate_path(path, cwd)?; - let (protected_metadata_path, metadata_name) = - metadata_child_of_writable_root(file_system_sandbox_policy, target.as_path(), cwd)?; - if has_explicit_write_entry_for_metadata_path( - file_system_sandbox_policy, - &protected_metadata_path, - target.as_path(), - cwd, - ) { - return None; - } - - if !file_system_sandbox_policy.can_write_path_with_cwd(target.as_path(), cwd) { - return Some(metadata_name); - } - - None + with_local_policy_context(path, cwd, |path, context| { + file_system_sandbox_policy + .metadata_write_denial(path, context) + .filter(|_| !file_system_sandbox_policy.can_write_path(path, context)) + })? } #[derive( @@ -884,23 +872,10 @@ impl FileSystemSandboxPolicy { } pub fn resolve_access_with_cwd(&self, path: &Path, cwd: &Path) -> FileSystemAccessMode { - match self.kind { - FileSystemSandboxKind::Unrestricted | FileSystemSandboxKind::ExternalSandbox => { - return FileSystemAccessMode::Write; - } - FileSystemSandboxKind::Restricted => {} - } - - let Some(path) = resolve_candidate_path(path, cwd) else { - return FileSystemAccessMode::Deny; - }; - - self.resolved_entries_with_cwd(cwd) - .into_iter() - .filter(|entry| path.as_path().starts_with(entry.path.as_path())) - .max_by_key(resolved_entry_precedence) - .map(|entry| entry.access) - .unwrap_or(FileSystemAccessMode::Deny) + with_local_policy_context(path, cwd, |path, context| { + self.resolve_access(path, context) + }) + .unwrap_or(FileSystemAccessMode::Deny) } pub fn can_read_path_with_cwd(&self, path: &Path, cwd: &Path) -> bool { @@ -908,35 +883,88 @@ impl FileSystemSandboxPolicy { } pub fn can_write_path_with_cwd(&self, path: &Path, cwd: &Path) -> bool { - if !self.resolve_access_with_cwd(path, cwd).can_write() { - return false; - } - if self.has_full_disk_write_access() { - return true; - } - !self.is_metadata_write_denied(path, cwd) + with_local_policy_context(path, cwd, |path, context| { + self.can_write_path(path, context) + }) + .unwrap_or(false) } - fn is_metadata_write_denied(&self, path: &Path, cwd: &Path) -> bool { - if !matches!(self.kind, FileSystemSandboxKind::Restricted) { - return false; + fn resolve_access( + &self, + path: &PathUri, + context: &FileSystemSandboxPolicyContext<'_>, + ) -> FileSystemAccessMode { + match self.kind { + FileSystemSandboxKind::Unrestricted | FileSystemSandboxKind::ExternalSandbox => { + return FileSystemAccessMode::Write; + } + FileSystemSandboxKind::Restricted => {} } - let Some(target) = resolve_candidate_path(path, cwd) else { - return true; + let Some(convention) = context.cwd.infer_path_convention() else { + return FileSystemAccessMode::Deny; }; - let Some((protected_metadata_path, _)) = - metadata_child_of_writable_root(self, target.as_path(), cwd) - else { - return false; + if path.infer_path_convention() != Some(convention) + || path.lexical_depth().is_none() + || context.cwd.lexical_depth().is_none() + { + return FileSystemAccessMode::Deny; + } + + let Some(entries) = self.resolved_entries(context).into_iter().try_fold( + Vec::new(), + |mut entries, (root, access)| { + match root.lexical_depth() { + Some(depth) => entries.push((root, access, depth)), + None if root.is_opaque() => {} + None => return None, + } + Some(entries) + }, + ) else { + return FileSystemAccessMode::Deny; }; - !has_explicit_write_entry_for_metadata_path( - self, - &protected_metadata_path, - target.as_path(), - cwd, - ) + entries + .into_iter() + .filter(|(root, _, _)| path.starts_with(root)) + .max_by_key(|(_, access, depth)| (*depth, *access)) + .map(|(_, access, _)| access) + .unwrap_or(FileSystemAccessMode::Deny) + } + + fn can_write_path(&self, path: &PathUri, context: &FileSystemSandboxPolicyContext<'_>) -> bool { + if !self.resolve_access(path, context).can_write() { + return false; + } + self.has_full_disk_write_access() || self.metadata_write_denial(path, context).is_none() + } + + fn metadata_write_denial( + &self, + path: &PathUri, + context: &FileSystemSandboxPolicyContext<'_>, + ) -> Option<&'static str> { + if !matches!(self.kind, FileSystemSandboxKind::Restricted) { + return None; + } + let entries = self.resolved_entries(context); + let (protected, metadata_name) = entries + .iter() + .filter(|(_, access)| access.can_write()) + .find_map(|(root, _)| { + PROTECTED_METADATA_PATH_NAMES + .iter() + .find_map(|metadata_name| { + let protected = root.join_descendant(metadata_name).ok()?; + path.starts_with(&protected) + .then_some((protected, *metadata_name)) + }) + })?; + (!entries.iter().any(|(root, access)| { + access.can_write() && path.starts_with(root) && root.starts_with(&protected) + })) + .then_some(metadata_name) } /// Replaces symbolic `:workspace_roots` entries with absolute paths resolved @@ -1567,6 +1595,50 @@ impl FileSystemSandboxPolicy { .collect() } + fn resolved_entries( + &self, + context: &FileSystemSandboxPolicyContext<'_>, + ) -> Vec<(PathUri, FileSystemAccessMode)> { + let convention = context.cwd.infer_path_convention(); + self.entries + .iter() + .flat_map(|entry| { + let paths = match &entry.path { + FileSystemPath::Path { path } => vec![path.clone()], + FileSystemPath::GlobPattern { .. } => Vec::new(), + FileSystemPath::Special { value } => match value { + FileSystemSpecialPath::Root => { + file_system_root(context).into_iter().collect() + } + FileSystemSpecialPath::ProjectRoots { subpath } => context + .workspace_roots + .iter() + .filter_map(|root| match subpath { + Some(subpath) => root.join(subpath).ok(), + None => Some(root.clone()), + }) + .collect(), + FileSystemSpecialPath::Tmpdir => { + context.temporary_directories.unwrap_or_default().to_vec() + } + FileSystemSpecialPath::SlashTmp + if convention == Some(PathConvention::Posix) => + { + context.cwd.join("/tmp").into_iter().collect() + } + FileSystemSpecialPath::SlashTmp + | FileSystemSpecialPath::Minimal + | FileSystemSpecialPath::Unknown { .. } => Vec::new(), + }, + }; + paths + .into_iter() + .filter(move |path| path.infer_path_convention() == convention) + .map(move |path| (path, entry.access)) + }) + .collect() + } + fn semantic_signature(&self, cwd: &Path) -> FileSystemSemanticSignature { FileSystemSemanticSignature { has_full_disk_read_access: self.has_full_disk_read_access(), @@ -1682,6 +1754,39 @@ fn resolve_scoped_workspace_path(root: &PathUri, subpath: &str) -> Option( + path: &Path, + cwd: &Path, + evaluate: impl FnOnce(&PathUri, &FileSystemSandboxPolicyContext<'_>) -> T, +) -> Option { + let cwd = AbsolutePathBuf::from_absolute_path(cwd).ok()?; + let path = PathUri::from(resolve_candidate_path(path, cwd.as_path())?); + let cwd = PathUri::from(cwd); + let temporary_directories = local_temporary_directories(); + let context = FileSystemSandboxPolicyContext { + cwd: &cwd, + workspace_roots: std::slice::from_ref(&cwd), + temporary_directories: Some(&temporary_directories), + }; + Some(evaluate(&path, &context)) +} + +fn file_system_root(context: &FileSystemSandboxPolicyContext<'_>) -> Option { + context.cwd.lexical_depth()?; + context.cwd.ancestors().last() +} + +fn local_temporary_directories() -> Vec { + let Some(tmpdir) = std::env::var_os("TMPDIR").filter(|path| !path.is_empty()) else { + return Vec::new(); + }; + AbsolutePathBuf::from_absolute_path(PathBuf::from(tmpdir)) + .ok() + .map(PathUri::from) + .into_iter() + .collect() +} + /// Returns true when two config paths refer to the same exact target before /// any prefix matching is applied. /// @@ -1750,13 +1855,6 @@ fn special_path_matches_absolute_path( } } -/// Orders resolved entries so the most specific path wins first, then applies -/// the access tie-breaker from [`FileSystemAccessMode`]. -fn resolved_entry_precedence(entry: &ResolvedFileSystemEntry) -> (usize, FileSystemAccessMode) { - let specificity = entry.path.as_path().components().count(); - (specificity, entry.access) -} - fn absolute_root_path_for_cwd(cwd: &AbsolutePathBuf) -> AbsolutePathBuf { let root = cwd .as_path() @@ -2083,31 +2181,6 @@ fn has_explicit_resolved_path_entry( entries.iter().any(|entry| &entry.path == path) } -fn metadata_path_name(name: &OsStr) -> Option<&'static str> { - PROTECTED_METADATA_PATH_NAMES - .iter() - .copied() - .find(|metadata_name| name == OsStr::new(metadata_name)) -} - -fn metadata_child_of_writable_root( - policy: &FileSystemSandboxPolicy, - target: &Path, - cwd: &Path, -) -> Option<(AbsolutePathBuf, &'static str)> { - policy - .resolved_entries_with_cwd(cwd) - .iter() - .filter(|entry| entry.access.can_write()) - .filter_map(|entry| { - let relative_path = target.strip_prefix(entry.path.as_path()).ok()?; - let first_component = relative_path.components().next()?; - let metadata_name = metadata_path_name(first_component.as_os_str())?; - Some((entry.path.join(metadata_name), metadata_name)) - }) - .next() -} - fn protected_metadata_names_for_writable_root( policy: &FileSystemSandboxPolicy, root: &AbsolutePathBuf, @@ -2163,22 +2236,6 @@ fn protected_metadata_names_need_direct_runtime_enforcement( }) } -fn has_explicit_write_entry_for_metadata_path( - policy: &FileSystemSandboxPolicy, - protected_metadata_path: &AbsolutePathBuf, - target: &Path, - cwd: &Path, -) -> bool { - policy.resolved_entries_with_cwd(cwd).iter().any(|entry| { - entry.access.can_write() - && target.starts_with(entry.path.as_path()) - && entry - .path - .as_path() - .starts_with(protected_metadata_path.as_path()) - }) -} - fn is_git_pointer_file(path: &AbsolutePathBuf) -> bool { path.as_path().is_file() && path.as_path().file_name() == Some(OsStr::new(PROTECTED_METADATA_GIT_PATH_NAME)) @@ -2356,9 +2413,188 @@ mod tests { !policy.can_read_path_with_cwd(Path::new(path), cwd.path()), "deny should apply to {path}" ); + assert!( + policy.can_read_path_with_cwd(&cwd.path().join("ordinary"), cwd.path()), + "opaque deny for {path} should not poison ordinary paths" + ); } } + #[test] + fn uri_matcher_resolves_selected_executor_paths() { + let path = |path| PathUri::parse(path).expect("valid path URI"); + let cwd = path("file:///C:/workspace"); + let workspace_roots = [cwd.clone()]; + let temporary_directories = [path("file:///C:/Temp")]; + let context = FileSystemSandboxPolicyContext { + cwd: &cwd, + workspace_roots: &workspace_roots, + temporary_directories: Some(&temporary_directories), + }; + let policy = FileSystemSandboxPolicy::restricted(vec![ + FileSystemSandboxEntry::new( + FileSystemPath::Special { + value: FileSystemSpecialPath::Root, + }, + FileSystemAccessMode::Read, + ), + FileSystemSandboxEntry::new( + FileSystemPath::Special { + value: FileSystemSpecialPath::project_roots(/*subpath*/ None), + }, + FileSystemAccessMode::Write, + ), + FileSystemSandboxEntry::new( + path("file:///C:/workspace/private").into(), + FileSystemAccessMode::Deny, + ), + FileSystemSandboxEntry::new( + path("file:///C:/workspace/private/public").into(), + FileSystemAccessMode::Write, + ), + FileSystemSandboxEntry::new( + FileSystemPath::Special { + value: FileSystemSpecialPath::Tmpdir, + }, + FileSystemAccessMode::Write, + ), + ]); + + for (candidate, expected) in [ + ( + "file:///C:/workspace/src/main.rs", + FileSystemAccessMode::Write, + ), + ( + "file:///c:/WORKSPACE/private/key", + FileSystemAccessMode::Deny, + ), + ( + "file:///C:/workspace/private/public/ok", + FileSystemAccessMode::Write, + ), + ("file:///C:/Temp/cache", FileSystemAccessMode::Write), + ("file:///C:/outside", FileSystemAccessMode::Read), + ("file:///tmp/cache", FileSystemAccessMode::Deny), + ] { + assert_eq!( + policy.resolve_access(&path(candidate), &context), + expected, + "resolving {candidate}" + ); + } + assert!(!policy.can_write_path(&path("file:///c:/WORKSPACE/.git/config"), &context,)); + + let scoped = FileSystemSandboxPolicy::restricted(vec![FileSystemSandboxEntry::new( + FileSystemPath::Special { + value: FileSystemSpecialPath::project_roots(Some("docs/../public".to_string())), + }, + FileSystemAccessMode::Write, + )]); + assert_eq!( + scoped.resolve_access(&path("file:///C:/workspace/public/file"), &context), + FileSystemAccessMode::Write + ); + assert_eq!( + scoped.resolve_access(&path("file:///C:/outside/file"), &context), + FileSystemAccessMode::Deny + ); + } + + #[test] + fn uri_matcher_uses_validated_native_components_for_precedence() { + let path = |path| PathUri::parse(path).expect("valid path URI"); + let cwd = path("file:///workspace"); + let context = FileSystemSandboxPolicyContext { + cwd: &cwd, + workspace_roots: std::slice::from_ref(&cwd), + temporary_directories: None, + }; + let candidate = path("file:///workspace/private/secret/key"); + + for (case, writable_path, denied_path) in [ + ( + "encoded component", + FileSystemPath::Special { + value: FileSystemSpecialPath::Root, + }, + path("file:///workspace/%70rivate").into(), + ), + ( + "encoded separator", + FileSystemPath::Special { + value: FileSystemSpecialPath::Root, + }, + path("file:///workspace/private%2Fsecret").into(), + ), + ( + "repeated separators", + path("file:///workspace////").into(), + path("file:///workspace/private").into(), + ), + ] { + let policy = FileSystemSandboxPolicy::restricted(vec![ + FileSystemSandboxEntry::new(writable_path, FileSystemAccessMode::Write), + FileSystemSandboxEntry::new(denied_path, FileSystemAccessMode::Deny), + ]); + + assert_eq!( + policy.resolve_access(&candidate, &context), + FileSystemAccessMode::Deny, + "resolving {case}" + ); + } + } + + #[test] + fn uri_matcher_fails_closed_without_executor_roots_or_lexical_paths() { + let path = |path| PathUri::parse(path).expect("valid path URI"); + let cwd = path("file://server/share/workspace"); + let context = FileSystemSandboxPolicyContext { + cwd: &cwd, + workspace_roots: &[], + temporary_directories: None, + }; + let policy = FileSystemSandboxPolicy::restricted(vec![ + FileSystemSandboxEntry::new( + FileSystemPath::Special { + value: FileSystemSpecialPath::project_roots(/*subpath*/ None), + }, + FileSystemAccessMode::Write, + ), + FileSystemSandboxEntry::new( + FileSystemPath::Special { + value: FileSystemSpecialPath::Tmpdir, + }, + FileSystemAccessMode::Write, + ), + ]); + + assert_eq!( + policy.resolve_access(&cwd, &context), + FileSystemAccessMode::Deny + ); + assert_eq!( + policy.resolve_access(&path("file:///tmp/cache"), &context), + FileSystemAccessMode::Deny + ); + + let opaque = path("file:///%00/bad/path/YQ"); + let opaque_context = FileSystemSandboxPolicyContext { + cwd: &opaque, + workspace_roots: std::slice::from_ref(&opaque), + temporary_directories: None, + }; + let opaque_policy = FileSystemSandboxPolicy::restricted(vec![FileSystemSandboxEntry::new( + opaque.clone().into(), + FileSystemAccessMode::Write, + )]); + assert_eq!( + opaque_policy.resolve_access(&opaque, &opaque_context), + FileSystemAccessMode::Deny + ); + } + #[test] fn unknown_special_paths_are_ignored_by_legacy_bridge() -> std::io::Result<()> { let policy = FileSystemSandboxPolicy::restricted(vec![ @@ -2811,6 +3047,9 @@ mod tests { ), Some(".git") ); + assert!( + file_system_policy.can_write_path_with_cwd(Path::new("src/main.rs"), relative_cwd,) + ); assert!( !file_system_policy .can_write_path_with_cwd(Path::new(".codex/config.toml"), relative_cwd,) diff --git a/codex-rs/sandboxing/src/policy_transforms.rs b/codex-rs/sandboxing/src/policy_transforms.rs index d79f8a0df2..8012564f3a 100644 --- a/codex-rs/sandboxing/src/policy_transforms.rs +++ b/codex-rs/sandboxing/src/policy_transforms.rs @@ -12,6 +12,7 @@ use codex_protocol::permissions::NetworkSandboxPolicy; use codex_protocol::permissions::ReadDenyMatcher; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_absolute_path::canonicalize_preserving_symlinks; +use codex_utils_path_uri::PathUri; use std::num::NonZeroUsize; use std::path::Path; use std::path::PathBuf; @@ -327,11 +328,10 @@ fn deny_entry_constrains_accepted_grant( }; match &deny_entry.path { FileSystemPath::GlobPattern { pattern } => glob_static_prefix_path(pattern, cwd) - .is_some_and(|prefix| paths_overlap(prefix.as_path(), grant_path.as_path())), + .is_some_and(|prefix| paths_may_overlap(&prefix, &grant_path)), FileSystemPath::Path { .. } | FileSystemPath::Special { .. } => { - resolve_permission_path(&deny_entry.path, cwd).is_some_and(|deny_path| { - paths_overlap(deny_path.as_path(), grant_path.as_path()) - }) + resolve_permission_path(&deny_entry.path, cwd) + .is_some_and(|deny_path| paths_may_overlap(&deny_path, &grant_path)) } } }) @@ -358,8 +358,10 @@ fn glob_static_prefix_path(pattern: &str, cwd: &Path) -> Option AbsolutePathBuf::from_absolute_path(prefix).ok() } -fn paths_overlap(left: &Path, right: &Path) -> bool { - left.starts_with(right) || right.starts_with(left) +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 access_covers(requested: FileSystemAccessMode, granted: FileSystemAccessMode) -> bool { diff --git a/codex-rs/sandboxing/src/policy_transforms_tests.rs b/codex-rs/sandboxing/src/policy_transforms_tests.rs index b43863d8a2..ae7fe9ae41 100644 --- a/codex-rs/sandboxing/src/policy_transforms_tests.rs +++ b/codex-rs/sandboxing/src/policy_transforms_tests.rs @@ -343,6 +343,46 @@ fn intersect_permission_profiles_drops_explicit_empty_reads_without_grant() { ); } +#[test] +fn intersect_permission_profiles_preserves_parent_relative_project_root_restriction() { + let temp_dir = TempDir::new().expect("create temp dir"); + let root = AbsolutePathBuf::from_absolute_path(temp_dir.path()).expect("absolute temp dir"); + let cwd = root.join("project"); + let outside_child = root.join("outside/child"); + let requested = PermissionProfile { + file_system: Some(FileSystemPermissions { + entries: vec![ + FileSystemSandboxEntry::new( + FileSystemPath::Special { + value: FileSystemSpecialPath::Root, + }, + FileSystemAccessMode::Write, + ), + FileSystemSandboxEntry::new( + FileSystemPath::Special { + value: FileSystemSpecialPath::project_roots(Some("../outside".to_string())), + }, + FileSystemAccessMode::Read, + ), + ], + glob_scan_max_depth: None, + }), + ..Default::default() + }; + let granted = PermissionProfile { + file_system: Some(FileSystemPermissions::from_read_write_roots( + /*read*/ None, + Some(vec![outside_child]), + )), + ..Default::default() + }; + + assert_eq!( + intersect_permission_profiles(requested, granted, cwd.as_path()), + PermissionProfile::default() + ); +} + #[cfg(windows)] #[test] fn intersect_permission_profiles_rejects_symbolic_slash_tmp_grants() { @@ -378,6 +418,89 @@ fn intersect_permission_profiles_rejects_symbolic_slash_tmp_grants() { ); } +#[cfg(windows)] +#[test] +fn intersect_permission_profiles_preserves_deny_across_case_variant_grant() { + use FileSystemAccessMode::Deny; + use FileSystemAccessMode::Write; + + let requested_root = + AbsolutePathBuf::from_absolute_path(r"C:\workspace").expect("absolute requested root"); + let granted_root = + AbsolutePathBuf::from_absolute_path(r"C:\WORKSPACE").expect("absolute granted root"); + let requested_write = FileSystemSandboxEntry::new(requested_root.clone().into(), Write); + let requested_deny = FileSystemSandboxEntry::new(requested_root.join("secret").into(), Deny); + let granted_write = FileSystemSandboxEntry::new(granted_root.into(), Write); + let profile = |entries| PermissionProfile { + file_system: Some(FileSystemPermissions { + entries, + glob_scan_max_depth: None, + }), + ..Default::default() + }; + + assert_eq!( + intersect_permission_profiles( + profile(vec![requested_write, requested_deny.clone()]), + profile(vec![granted_write.clone()]), + requested_root.as_path(), + ), + profile(vec![granted_write, requested_deny]) + ); +} + +#[test] +fn intersect_permission_profiles_preserves_opaque_child_deny() { + use std::ffi::OsString; + #[cfg(unix)] + use std::os::unix::ffi::OsStringExt; + #[cfg(windows)] + use std::os::windows::ffi::OsStringExt; + + use FileSystemAccessMode::Deny; + use FileSystemAccessMode::Write; + + let temp_dir = TempDir::new().expect("create temp dir"); + let root = AbsolutePathBuf::from_absolute_path( + canonicalize(temp_dir.path()).expect("canonicalize temp dir"), + ) + .expect("absolute temp dir"); + #[cfg(unix)] + let opaque_name = OsString::from_vec(b"private-\xff".to_vec()); + #[cfg(windows)] + let opaque_name = OsString::from_wide(&[ + u16::from(b'p'), + u16::from(b'r'), + u16::from(b'i'), + u16::from(b'v'), + u16::from(b'a'), + u16::from(b't'), + u16::from(b'e'), + u16::from(b'-'), + 0xD800, + ]); + let deny_path = root.join(opaque_name); + let requested_write = FileSystemSandboxEntry::new(root.clone().into(), Write); + let requested_deny = FileSystemSandboxEntry::new(deny_path.into(), Deny); + let granted_write = FileSystemSandboxEntry::new(root.clone().into(), Write); + let profile = |entries| PermissionProfile { + file_system: Some(FileSystemPermissions { + entries, + glob_scan_max_depth: None, + }), + ..Default::default() + }; + + assert_eq!( + intersect_permission_profiles( + profile(vec![requested_write, requested_deny.clone()]), + profile(vec![granted_write.clone()]), + root.as_path(), + ), + profile(vec![granted_write, requested_deny]) + ); +} + #[test] fn intersect_permission_profiles_accepts_child_path_granted_for_requested_cwd() { let temp_dir = TempDir::new().expect("create temp dir"); diff --git a/codex-rs/utils/path-uri/src/lib.rs b/codex-rs/utils/path-uri/src/lib.rs index cf0160cb97..0633aa4b88 100644 --- a/codex-rs/utils/path-uri/src/lib.rs +++ b/codex-rs/utils/path-uri/src/lib.rs @@ -347,6 +347,37 @@ impl PathUri { native_path_segments_start_with(&path_segments, &base_segments, convention) } + /// Returns whether the lexical subtrees rooted at these URIs overlap. + /// + /// Returns `None` when either URI does not expose unambiguous lexical + /// components. Equal URIs are known to overlap even when they are opaque. + pub fn overlaps(&self, other: &Self) -> Option { + if self == other { + return Some(true); + } + self.lexical_depth()?; + other.lexical_depth()?; + Some(self.starts_with(other) || other.starts_with(self)) + } + + /// Returns true for a fallback URI that losslessly stores native path bytes. + pub fn is_opaque(&self) -> bool { + self.opaque_fallback_bytes().is_some() + } + + /// Returns the number of non-empty path segments when this URI is safe for + /// lexical containment. + /// + /// Opaque fallback URIs and segments containing encoded native separators + /// return `None` because they do not expose unambiguous component boundaries. + pub fn lexical_depth(&self) -> Option { + if decode_bad_path_uri(&self.0).is_some() { + return None; + } + let convention = self.infer_path_convention()?; + containment_path_segments(&self.0, convention).map(|segments| segments.len()) + } + /// Returns the decoded relative path from `base` to this URI. /// /// The result uses the separators of the inferred path convention, @@ -493,6 +524,27 @@ impl PathUri { Self::try_from(url) } + /// Lexically resolves a relative native path that remains at or below this URI. + /// + /// Absolute, Windows root- or drive-relative, and escaping paths are rejected. + pub fn join_descendant(&self, path: &str) -> Result { + let descendant = self.join(path)?; + let windows = self.infer_path_convention() == Some(PathConvention::Windows); + if path.starts_with('/') + || windows + && (path.starts_with('\\') + || PathConvention::Windows + .path_segments(path) + .any(|segment| segment.contains(':'))) + || !descendant.starts_with(self) + { + return Err(PathUriParseError::JoinPathMustBeDescendant( + path.to_string(), + )); + } + Ok(descendant) + } + /// Converts this file URI to a path using the current host's path rules. /// /// The URI's inferred path convention must match the current host. Conversion should succeed @@ -730,7 +782,13 @@ fn native_path_segments_start_with( convention: PathConvention, ) -> bool { match convention { - PathConvention::Posix => path_segments.starts_with(base_segments), + PathConvention::Posix => { + path_segments.len() >= base_segments.len() + && path_segments.iter().zip(base_segments).all(|(path, base)| { + urlencoding::decode_binary(path.as_bytes()) + == urlencoding::decode_binary(base.as_bytes()) + }) + } PathConvention::Windows => { path_segments.len() >= base_segments.len() && path_segments.iter().zip(base_segments).all(|(path, base)| { @@ -901,8 +959,8 @@ pub enum PathUriParseError { QueryNotAllowed, #[error("fragments are not allowed in path URIs")] FragmentNotAllowed, - #[error("path `{0}` must be relative when joining a path URI")] - JoinPathMustBeRelative(String), + #[error("path `{0}` must resolve to a relative descendant when joining a path URI")] + JoinPathMustBeDescendant(String), } /// Path syntax used to render a [`PathUri`] as an operating-system path. diff --git a/codex-rs/utils/path-uri/src/tests.rs b/codex-rs/utils/path-uri/src/tests.rs index ecbd87d1e5..fab23b0c46 100644 --- a/codex-rs/utils/path-uri/src/tests.rs +++ b/codex-rs/utils/path-uri/src/tests.rs @@ -800,6 +800,57 @@ fn join_normalizes_relative_uri_segments() { } } +#[test] +fn join_descendant_uses_the_base_path_convention() { + for (base, relative, expected) in [ + ( + "file:///workspace", + "docs/../public", + "file:///workspace/public", + ), + ( + "file:///C:/workspace", + r"docs\..\public", + "file:///C:/workspace/public", + ), + ( + "file://server/share/workspace", + r"docs\..\public", + "file://server/share/workspace/public", + ), + ] { + let base = PathUri::parse(base).expect("valid base URI"); + let expected = PathUri::parse(expected).expect("valid expected URI"); + assert_eq!( + base.join_descendant(relative), + Ok(expected), + "joining {relative}" + ); + } +} + +#[test] +fn join_descendant_rejects_non_descendant_paths() { + for (base, path) in [ + ("file:///workspace", "/workspace/docs"), + ("file:///workspace", "../outside"), + ("file:///C:/workspace", r"\workspace\docs"), + ("file:///C:/workspace", r"C:\workspace\docs"), + ("file:///C:/workspace", r"C:docs"), + ("file:///C:/workspace", r"docs\file:stream"), + ("file://server/share/workspace", r"..\outside"), + ] { + let base = PathUri::parse(base).expect("valid base URI"); + assert_eq!( + base.join_descendant(path), + Err(PathUriParseError::JoinPathMustBeDescendant( + path.to_string() + )), + "joining {path}" + ); + } +} + #[test] fn join_replaces_posix_absolute_path() { let base = PathUri::parse("file:///workspace").expect("valid base URI"); @@ -1157,6 +1208,12 @@ fn starts_with_uses_uri_segment_boundaries() { "file:///workspace/plugin", false, ), + ( + "file:///workspace/pri%76ate/file", + "file:///workspace/%70rivate", + true, + ), + ("file:///workspace/%ff/file", "file:///workspace/%FF", true), ( "file:///workspace/plugin/%5C..%5Coutside", "file:///workspace/plugin", @@ -1174,6 +1231,54 @@ fn starts_with_uses_uri_segment_boundaries() { } } +#[test] +fn overlaps_uses_lexical_containment() { + for (left, right, expected) in [ + ("file:///workspace", "file:///workspace/src", Some(true)), + ( + "file:///C:/WORKSPACE", + "file:///c:/workspace/src", + Some(true), + ), + ( + "file:///workspace/src", + "file:///workspace/tests", + Some(false), + ), + ("file:///WORKSPACE", "file:///workspace/src", Some(false)), + ] { + let left = PathUri::parse(left).expect("valid left URI"); + let right = PathUri::parse(right).expect("valid right URI"); + + assert_eq!(left.overlaps(&right), expected, "{left} and {right}"); + assert_eq!(right.overlaps(&left), expected, "{right} and {left}"); + } + + let opaque = PathUri::from_opaque_path_bytes(b"/workspace/private"); + let lexical = PathUri::parse("file:///workspace").expect("valid lexical URI"); + assert_eq!(opaque.overlaps(&opaque), Some(true)); + assert_eq!(opaque.overlaps(&lexical), None); + assert_eq!(lexical.overlaps(&opaque), None); +} + +#[test] +fn lexical_depth_counts_validated_nonempty_segments() { + for (path, expected) in [ + ("file:///", Some(0)), + ("file:///workspace////", Some(1)), + ("file:///workspace/%70rivate", Some(2)), + ("file:///workspace/private%2Fsecret", None), + ] { + let path = PathUri::parse(path).expect("valid path URI"); + assert_eq!(path.lexical_depth(), expected, "lexical depth for {path}"); + } + + assert_eq!( + PathUri::from_opaque_path_bytes(b"/workspace").lexical_depth(), + None + ); +} + #[test] fn to_url_returns_the_validated_url() { let uri = PathUri::parse("file://localhost/workspace/a%20file.rs").expect("valid file URI");