diff --git a/codex-rs/config/src/config_requirements.rs b/codex-rs/config/src/config_requirements.rs index c415959ede..60aaa455b5 100644 --- a/codex-rs/config/src/config_requirements.rs +++ b/codex-rs/config/src/config_requirements.rs @@ -12,8 +12,6 @@ use codex_utils_absolute_path::AbsolutePathBuf; use serde::Deserialize; use serde::Serialize; use serde::de::Error as _; -use serde::de::value::Error as ValueDeserializerError; -use serde::de::value::StrDeserializer; use std::collections::BTreeMap; use std::collections::BTreeSet; use std::collections::HashMap; @@ -711,25 +709,45 @@ impl FilesystemDenyReadPattern { } pub fn from_input(input: &str) -> Result { + codex_utils_path_uri::PathUri::validate_config_path_text( + input, + crate::path_context::convention(), + ) + .map_err(|error| error.to_string())?; if !input.chars().any(is_glob_metacharacter) { let path = deserialize_absolute_path(input)?; - return Ok(Self(path.to_string_lossy().into_owned())); + return Ok(Self(path)); } let (directory_prefix, suffix) = split_glob_pattern(input); + if crate::path_context::convention() == codex_utils_path_uri::PathConvention::Windows + && matches!(input.as_bytes(), [b'/' | b'\\', b'/' | b'\\', ..]) + && !matches!( + directory_prefix.as_bytes(), + [b'/' | b'\\', b'/' | b'\\', ..] + ) + { + return Err( + "filesystem denial glob requires a literal UNC server and share".to_string(), + ); + } let normalized_prefix = if directory_prefix.is_empty() { deserialize_absolute_path(".")? } else { deserialize_absolute_path(directory_prefix)? }; - let normalized_prefix = normalized_prefix.to_string_lossy(); let normalized = if suffix.is_empty() { - normalized_prefix.into_owned() + normalized_prefix } else if normalized_prefix == "/" { format!("/{suffix}") } else { format!("{normalized_prefix}/{suffix}") }; + codex_utils_path_uri::PathUri::validate_config_path_text( + &normalized, + crate::path_context::convention(), + ) + .map_err(|error| error.to_string())?; Ok(Self(normalized)) } } @@ -750,9 +768,8 @@ impl<'de> Deserialize<'de> for FilesystemDenyReadPattern { } } -fn deserialize_absolute_path(input: &str) -> Result { - AbsolutePathBuf::deserialize(StrDeserializer::::new(input)) - .map_err(|err| err.to_string()) +fn deserialize_absolute_path(input: &str) -> Result { + crate::path_context::resolve(input) } fn split_glob_pattern(input: &str) -> (&str, &str) { @@ -768,7 +785,8 @@ fn split_glob_pattern(input: &str) -> (&str, &str) { match separator_index { Some(0) => ("/", &input[1..]), Some(index) - if cfg!(windows) + if crate::path_context::convention() + == codex_utils_path_uri::PathConvention::Windows && index == 2 && input.as_bytes().get(1) == Some(&b':') && input.as_bytes().get(2).is_some() => @@ -781,7 +799,7 @@ fn split_glob_pattern(input: &str) -> (&str, &str) { } fn is_path_separator(ch: char) -> bool { - if cfg!(windows) { + if crate::path_context::convention() == codex_utils_path_uri::PathConvention::Windows { ch == '/' || ch == '\\' } else { ch == '/' diff --git a/codex-rs/config/src/filesystem_constraints.rs b/codex-rs/config/src/filesystem_constraints.rs new file mode 100644 index 0000000000..0eebd85565 --- /dev/null +++ b/codex-rs/config/src/filesystem_constraints.rs @@ -0,0 +1,62 @@ +//! Convert resolved filesystem denials into portable policy entries. + +use crate::FilesystemConstraints; +use codex_protocol::permissions::FileSystemAccessMode; +use codex_protocol::permissions::FileSystemPath; +use codex_protocol::permissions::FileSystemSandboxEntry; +use codex_protocol::permissions::FileSystemSandboxPolicy; +use codex_utils_path_uri::LegacyAppPathString; +use codex_utils_path_uri::LegacyAppPathStringError; +use codex_utils_path_uri::PathConvention; +use codex_utils_path_uri::PathUri; +use std::io; + +impl FilesystemConstraints { + /// Adds denials without changing existing entries or other policy settings. + /// `convention` must match the executor facts used to resolve these requirements. + /// Literal paths become URIs; glob spellings and entry order are preserved. + /// Invalid denials return an error before the policy is changed. + pub fn apply_to_policy( + &self, + policy: &mut FileSystemSandboxPolicy, + convention: PathConvention, + ) -> io::Result<()> { + let entries = self + .deny_read + .iter() + .map(|deny_read| { + PathUri::validate_config_path_text(deny_read.as_str(), convention)?; + let path = if deny_read.contains_glob() { + FileSystemPath::GlobPattern { + pattern: deny_read.as_str().to_string(), + } + } else { + let mut path = LegacyAppPathString::from_string(deny_read.as_str()) + .to_path_uri(convention)?; + path.validate_config_path(convention)?; + // Match Core's existing URI spelling for UNC share roots. + if path.to_url().host_str().is_some() { + path = path.join(".")?; + } + path.into() + }; + Ok(FileSystemSandboxEntry::new( + path, + FileSystemAccessMode::Deny, + )) + }) + .collect::, LegacyAppPathStringError>>() + .map_err(|error| { + io::Error::new( + io::ErrorKind::InvalidInput, + format!("invalid permissions.filesystem.deny_read path: {error}"), + ) + })?; + for entry in entries { + if !policy.entries.contains(&entry) { + policy.entries.push(entry); + } + } + Ok(()) + } +} diff --git a/codex-rs/config/src/lib.rs b/codex-rs/config/src/lib.rs index 704964d294..06fb796c80 100644 --- a/codex-rs/config/src/lib.rs +++ b/codex-rs/config/src/lib.rs @@ -12,6 +12,7 @@ mod config_requirements; pub mod config_toml; mod constraint; mod diagnostics; +mod filesystem_constraints; mod fingerprint; mod hook_config; mod host_name; @@ -25,6 +26,7 @@ mod mcp_types; mod merge; mod model_provider_requirements; mod overrides; +mod path_context; pub mod permissions_toml; mod plugin_edit; pub mod profile_toml; @@ -169,6 +171,7 @@ pub use merge::is_structured_feature_path; pub use merge::merge_toml_values; pub use merge::shell_environment_filter_entry; pub use overrides::build_cli_overrides_layer; +pub use path_context::ConfigPathContext; pub use plugin_edit::PluginConfigEdit; pub use plugin_edit::apply_user_plugin_config_edits; pub use plugin_edit::clear_user_plugin; diff --git a/codex-rs/config/src/path_context.rs b/codex-rs/config/src/path_context.rs new file mode 100644 index 0000000000..e30477f081 --- /dev/null +++ b/codex-rs/config/src/path_context.rs @@ -0,0 +1,88 @@ +//! Path facts for filesystem denial parsing. Native callers and callers resolving +//! another platform use the same resolver after fact capture. + +use codex_utils_absolute_path::AbsolutePathBufGuard; +use codex_utils_path_uri::PathConvention; +use codex_utils_path_uri::PathUri; +use std::cell::RefCell; + +/// The owning environment's path convention, base directory, and home for denials. +#[derive(Clone, Debug)] +pub struct ConfigPathContext { + convention: PathConvention, + base_dir: Option, + user_home_dir: Option, +} + +impl ConfigPathContext { + /// Supplies the grammar and directories of the environment owning the layer. + /// Relative paths require a base; home-relative paths require a home directory. + pub fn new( + convention: PathConvention, + base_dir: Option, + user_home_dir: Option, + ) -> Self { + Self { + convention, + base_dir, + user_home_dir, + } + } + + pub(crate) fn enter(&self) -> PathContextGuard { + PathContextGuard(PATH_CONTEXT.with(|current| current.replace(Some(self.clone())))) + } +} + +thread_local! { + static PATH_CONTEXT: RefCell> = const { RefCell::new(None) }; +} + +pub(crate) struct PathContextGuard(Option); + +impl Drop for PathContextGuard { + fn drop(&mut self) { + PATH_CONTEXT.with(|current| *current.borrow_mut() = self.0.take()); + } +} + +pub(crate) fn convention() -> PathConvention { + PATH_CONTEXT.with(|current| { + current + .borrow() + .as_ref() + .map_or_else(PathConvention::native, |context| context.convention) + }) +} + +pub(crate) fn resolve(input: &str) -> Result { + let context = match PATH_CONTEXT.with(|current| current.borrow().clone()) { + Some(context) => context, + None => { + let convention = PathConvention::native(); + let base = AbsolutePathBufGuard::deserialization_base(std::path::Path::new(input)) + .map_err(str::to_owned)? + .map(PathUri::from_host_native_path) + .transpose() + .map_err(|error| error.to_string())?; + let home = convention + .home_relative_suffix(input) + .and_then(|_| AbsolutePathBufGuard::home_directory()) + .map(PathUri::from_host_native_path) + .transpose() + .map_err(|error| error.to_string())?; + ConfigPathContext::new(convention, base, home) + } + }; + PathUri::resolve_config_path( + input, + context.convention, + context.base_dir.as_ref(), + context.user_home_dir.as_ref(), + ) + .map_err(|error| error.to_string()) +} + +#[cfg(test)] +#[path = "path_context_tests.rs"] +mod tests; diff --git a/codex-rs/config/src/path_context_tests.rs b/codex-rs/config/src/path_context_tests.rs new file mode 100644 index 0000000000..ec82ab89d9 --- /dev/null +++ b/codex-rs/config/src/path_context_tests.rs @@ -0,0 +1,204 @@ +//! Contracts for layer-owned denial facts, parser scoping, and policy conversion. + +use super::ConfigPathContext; +use crate::FilesystemConstraints; +use crate::FilesystemDenyReadPattern; +use crate::RequirementSource; +use crate::RequirementsLayerEntry; +use crate::compose_requirements_for_hostname; +use codex_protocol::permissions::FileSystemAccessMode; +use codex_protocol::permissions::FileSystemPath; +use codex_protocol::permissions::FileSystemSandboxEntry; +use codex_protocol::permissions::FileSystemSandboxKind; +use codex_protocol::permissions::FileSystemSandboxPolicy; +use codex_utils_absolute_path::AbsolutePathBufGuard; +use codex_utils_path_uri::PathConvention; +use codex_utils_path_uri::PathUri; +use pretty_assertions::assert_eq; +use tempfile::tempdir; + +#[test] +fn composition_applies_each_layers_base_and_home_to_policy() { + let native_home = tempdir().expect("native home"); + let first = RequirementsLayerEntry::from_toml( + RequirementSource::Unknown, + "[permissions.filesystem]\ndeny_read = ['private', '~/secret', 'private', 'C:\\', '\\\\server\\share\\', 'private/*.key']", + ) + .with_path_context(ConfigPathContext::new( + PathConvention::Windows, + Some(PathUri::parse("file:///C:/first").expect("first base")), + Some(PathUri::parse("file:///C:/Users/first").expect("first home")), + )); + let second = RequirementsLayerEntry::from_toml_value( + RequirementSource::Unknown, + toml::toml! { [permissions.filesystem] deny_read = ["private", "~/secret"] }.into(), + ) + .with_path_context(ConfigPathContext::new( + PathConvention::Windows, + Some(PathUri::parse("file:///D:/second").expect("second base")), + Some(PathUri::parse("file:///D:/Users/second").expect("second home")), + )); + let composed = AbsolutePathBufGuard::with_home_directory(native_home.path(), || { + compose_requirements_for_hostname([first, second], /*hostname*/ None) + .expect("compose layers") + .expect("requirements present") + }); + let constraints = FilesystemConstraints::from(composed.permissions.expect("permissions").value); + let mut expected = FileSystemSandboxPolicy { + kind: FileSystemSandboxKind::Restricted, + glob_scan_max_depth: Some(7), + entries: vec![FileSystemSandboxEntry::new( + PathUri::parse("file:///C:/allowed") + .expect("existing path") + .into(), + FileSystemAccessMode::Write, + )], + }; + let mut policy = expected.clone(); + expected.entries.extend( + [ + "file:///D:/second/private", + "file:///D:/Users/second/secret", + "file:///C:/first/private", + "file:///C:/Users/first/secret", + "file:///C:/", + "file://server/share", + ] + .map(|path| { + FileSystemSandboxEntry::new( + PathUri::parse(path).expect("expected denial").into(), + FileSystemAccessMode::Deny, + ) + }), + ); + expected.entries.push(FileSystemSandboxEntry::new( + FileSystemPath::GlobPattern { + pattern: r"C:\first\private/*.key".to_string(), + }, + FileSystemAccessMode::Deny, + )); + constraints + .apply_to_policy(&mut policy, PathConvention::Windows) + .expect("apply denials"); + constraints + .apply_to_policy(&mut policy, PathConvention::Windows) + .expect("deduplicate existing denials"); + assert_eq!(policy, expected); + assert!( + constraints + .apply_to_policy(&mut policy, PathConvention::Posix) + .is_err(), + "a foreign denial cannot be reinterpreted using the host convention", + ); + assert_eq!(policy, expected); +} + +#[test] +fn common_parser_uses_supplied_windows_drive_and_glob_separators() { + let context = ConfigPathContext::new( + PathConvention::Windows, + Some(PathUri::parse("file:///C:/base/cwd").expect("base")), + /*user_home_dir*/ None, + ); + let _guard = context.enter(); + for (input, expected) in [ + (r"C:private\*.key", r"C:\base\cwd\private/*.key"), + (r"D:\*.key", r"D:\/*.key"), + (r"\private\*.key", r"C:\private/*.key"), + ] { + assert_eq!( + FilesystemDenyReadPattern::from_input(input) + .expect("resolve pattern") + .as_str(), + expected, + ); + } +} + +#[test] +fn windows_denial_globs_reject_ambiguous_roots_and_streams() { + let context = ConfigPathContext::new( + PathConvention::Windows, + Some(PathUri::parse("file://server/share/base").expect("UNC base")), + /*user_home_dir*/ None, + ); + let _guard = context.enter(); + for input in [ + r"\\?\C:\private", + r"\\*\share\private", + r"D:*.key", + r"private\*\file:stream", + ] { + assert!( + FilesystemDenyReadPattern::from_input(input).is_err(), + "{input}" + ); + } +} + +#[test] +fn failed_nested_composition_restores_outer_context_and_then_native_resolution() { + let base = tempdir().expect("native base"); + let _native_guard = AbsolutePathBufGuard::new(base.path()); + let native = FilesystemDenyReadPattern::from_input("private"); + { + let outer = ConfigPathContext::new( + PathConvention::Posix, + Some(PathUri::parse("file:///outer").expect("outer base")), + /*user_home_dir*/ None, + ); + let _outer_guard = outer.enter(); + let inner = RequirementsLayerEntry::from_toml( + RequirementSource::Unknown, + "[permissions.filesystem]\ndeny_read = ['relative']", + ) + .with_path_context(ConfigPathContext::new( + PathConvention::Windows, + /*base_dir*/ None, + /*user_home_dir*/ None, + )); + assert!(compose_requirements_for_hostname([inner], /*hostname*/ None).is_err()); + assert_eq!( + FilesystemDenyReadPattern::from_input("private") + .expect("outer context restored") + .as_str(), + "/outer/private", + ); + } + assert_eq!(FilesystemDenyReadPattern::from_input("private"), native); +} + +#[test] +fn native_guard_and_supplied_native_facts_use_the_same_parser() { + let base = tempdir().expect("base"); + let home = tempdir().expect("home"); + let _base_guard = AbsolutePathBufGuard::new(base.path()); + AbsolutePathBufGuard::with_home_directory(home.path(), || { + let inputs = ["./private", "~/secret/*.txt"]; + let native = inputs.map(FilesystemDenyReadPattern::from_input); + let context = ConfigPathContext::new( + PathConvention::native(), + Some(PathUri::from_host_native_path(base.path()).expect("native base URI")), + Some(PathUri::from_host_native_path(home.path()).expect("native home URI")), + ); + let _context_guard = context.enter(); + assert_eq!(inputs.map(FilesystemDenyReadPattern::from_input), native); + }); +} + +#[test] +fn missing_supplied_home_does_not_fall_back_to_native_home() { + let home = tempdir().expect("native home"); + let layer = RequirementsLayerEntry::from_toml( + RequirementSource::Unknown, + "[permissions.filesystem]\ndeny_read = ['~/secret']", + ) + .with_path_context(ConfigPathContext::new( + PathConvention::Windows, + Some(PathUri::parse("file:///C:/base").expect("base")), + /*user_home_dir*/ None, + )); + AbsolutePathBufGuard::with_home_directory(home.path(), || { + assert!(compose_requirements_for_hostname([layer], /*hostname*/ None).is_err()); + }); +} diff --git a/codex-rs/config/src/requirements_layers/layer.rs b/codex-rs/config/src/requirements_layers/layer.rs index 8df3146ca1..57baaf50a8 100644 --- a/codex-rs/config/src/requirements_layers/layer.rs +++ b/codex-rs/config/src/requirements_layers/layer.rs @@ -1,3 +1,4 @@ +use crate::ConfigPathContext; use crate::ConfigRequirementsToml; use crate::ManagedHooksRequirementsToml; use crate::RequirementSource; @@ -21,6 +22,7 @@ pub struct RequirementsLayerEntry { pub(super) source: RequirementSource, toml: RequirementsLayerToml, base_dir: Option, + path_context: Option, } impl RequirementsLayerEntry { @@ -29,6 +31,7 @@ impl RequirementsLayerEntry { source, toml: RequirementsLayerToml::String(contents.into()), base_dir: None, + path_context: None, } } @@ -37,6 +40,7 @@ impl RequirementsLayerEntry { source, toml: RequirementsLayerToml::Value(value), base_dir: None, + path_context: None, } } @@ -45,6 +49,13 @@ impl RequirementsLayerEntry { self } + /// Supplies the owning environment's facts for filesystem denial paths. + /// Other path fields keep their existing native deserialization behavior. + pub fn with_path_context(mut self, context: ConfigPathContext) -> Self { + self.path_context = Some(context); + self + } + pub(crate) fn into_raw_parts( self, ) -> Result<(RequirementSource, TomlValue, Option), RequirementsCompositionError> @@ -53,6 +64,7 @@ impl RequirementsLayerEntry { source, toml, base_dir, + path_context: _, } = self; let toml = parse_layer_toml(&toml, &source)?; Ok((source, toml, base_dir)) @@ -81,6 +93,7 @@ impl ComposableRequirementsLayer { source, toml, base_dir, + path_context, } = layer; let (mut regular_toml, mut requirements) = { let _guard = base_dir @@ -120,6 +133,7 @@ impl ComposableRequirementsLayer { } let mut layer_requirements_toml = regular_toml.clone(); remove_top_level_field(&mut layer_requirements_toml, "model_providers"); + let _path_context = path_context.as_ref().map(ConfigPathContext::enter); let requirements = parse_layer_requirements( &RequirementsLayerToml::Value(layer_requirements_toml), &source, diff --git a/codex-rs/core/src/config/config_loader_tests.rs b/codex-rs/core/src/config/config_loader_tests.rs index 0c11f7ff27..4bdbfa8a07 100644 --- a/codex-rs/core/src/config/config_loader_tests.rs +++ b/codex-rs/core/src/config/config_loader_tests.rs @@ -2887,6 +2887,32 @@ deny_read = ["secrets/**"] Ok(()) } +#[tokio::test] +async fn load_config_rejects_nul_in_required_deny_read_glob() -> anyhow::Result<()> { + let tmp = tempdir()?; + let err = ConfigBuilder::default() + .codex_home(tmp.path().to_path_buf()) + .fallback_cwd(Some(tmp.path().to_path_buf())) + .loader_overrides(LoaderOverrides::without_managed_config_for_tests()) + .cloud_config_bundle( + CloudConfigBundleFixture::loader_with_enterprise_requirement( + r#" +[permissions.filesystem] +deny_read = ["secrets/**\u0000"] +"#, + ), + ) + .build() + .await + .expect_err("an unsupported required denial must fail configuration loading"); + + assert!( + err.to_string().contains("unsupported configuration path"), + "{err}" + ); + Ok(()) +} + #[tokio::test] async fn strict_config_rejects_unknown_cloud_config_key() { let tmp = tempdir().expect("tempdir"); diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index 64790ecb04..71dcff72b7 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -2550,39 +2550,6 @@ fn resolve_permission_config_syntax( }) } -fn apply_managed_filesystem_constraints( - file_system_sandbox_policy: &mut FileSystemSandboxPolicy, - filesystem_constraints: &codex_config::FilesystemConstraints, -) { - for deny_read in &filesystem_constraints.deny_read { - let deny_entry = if deny_read.contains_glob() { - codex_protocol::permissions::FileSystemSandboxEntry { - path: codex_protocol::permissions::FileSystemPath::GlobPattern { - pattern: deny_read.as_str().to_string(), - }, - access: codex_protocol::permissions::FileSystemAccessMode::Deny, - missing_path_behavior: None, - } - } else { - let Ok(path) = AbsolutePathBuf::try_from(deny_read.as_str()) else { - continue; - }; - codex_protocol::permissions::FileSystemSandboxEntry { - path: path.into(), - access: codex_protocol::permissions::FileSystemAccessMode::Deny, - missing_path_behavior: None, - } - }; - if !file_system_sandbox_policy - .entries - .iter() - .any(|existing| existing == &deny_entry) - { - file_system_sandbox_policy.entries.push(deny_entry); - } - } -} - /// Optional overrides for user configuration (e.g., from CLI flags). #[derive(Default, Debug, Clone)] pub struct ConfigOverrides { @@ -4090,11 +4057,12 @@ impl Config { let managed_deny_read_policy = filesystem_requirements .as_ref() .filter(|Sourced { value, .. }| !value.deny_read.is_empty()) - .map(|Sourced { value, .. }| { + .map(|Sourced { value, .. }| -> std::io::Result<_> { let mut policy = FileSystemSandboxPolicy::restricted(Vec::new()); - apply_managed_filesystem_constraints(&mut policy, value); - Arc::new(policy) - }); + value.apply_to_policy(&mut policy, codex_utils_path_uri::PathConvention::native())?; + Ok(Arc::new(policy)) + }) + .transpose()?; if let Some(managed_deny_read_policy) = managed_deny_read_policy.as_ref() { effective_file_system_sandbox_policy .preserve_deny_read_restrictions_from(managed_deny_read_policy); diff --git a/codex-rs/utils/absolute-path/src/lib.rs b/codex-rs/utils/absolute-path/src/lib.rs index fc26cc4f56..5918990fdb 100644 --- a/codex-rs/utils/absolute-path/src/lib.rs +++ b/codex-rs/utils/absolute-path/src/lib.rs @@ -27,9 +27,7 @@ impl AbsolutePathBuf { fn maybe_expand_home_directory(path: &Path) -> PathBuf { if let Some(path_str) = path.to_str() && let Some(rest) = path_str.strip_prefix('~') - && let Some(home) = ABSOLUTE_PATH_HOME - .with(|cell| cell.borrow().clone()) - .or_else(home_dir) + && let Some(home) = AbsolutePathBufGuard::home_directory() { if rest.is_empty() { return home; @@ -338,6 +336,23 @@ thread_local! { pub struct AbsolutePathBufGuard; impl AbsolutePathBufGuard { + /// Reads the native deserialization base and validates the guard requirement + /// before home expansion or namespace normalization. Does not look up cwd. + pub fn deserialization_base(path: &Path) -> Result, &'static str> { + let base = ABSOLUTE_PATH_BASE.with(|cell| cell.borrow().clone()); + if base.is_none() && !path.is_absolute() { + return Err("AbsolutePathBuf deserialized without a base path"); + } + Ok(base) + } + + /// Reads the effective native home, including the thread-local override. + pub fn home_directory() -> Option { + ABSOLUTE_PATH_HOME + .with(|cell| cell.borrow().clone()) + .or_else(home_dir) + } + pub fn new(base_path: &Path) -> Self { ABSOLUTE_PATH_BASE.with(|cell| { *cell.borrow_mut() = Some(base_path.to_path_buf()); @@ -379,15 +394,13 @@ impl<'de> Deserialize<'de> for AbsolutePathBuf { D: Deserializer<'de>, { let path = PathBuf::deserialize(deserializer)?; - ABSOLUTE_PATH_BASE.with(|cell| match cell.borrow().as_deref() { + match AbsolutePathBufGuard::deserialization_base(&path) + .map_err(SerdeError::custom)? + .as_deref() + { Some(base) => Ok(Self::resolve_path_against_base(path, base)), - None if path.is_absolute() => { - Self::from_absolute_path(path).map_err(SerdeError::custom) - } - None => Err(SerdeError::custom( - "AbsolutePathBuf deserialized without a base path", - )), - }) + None => Self::from_absolute_path(path).map_err(SerdeError::custom), + } } } diff --git a/codex-rs/utils/path-uri/src/api_path_string.rs b/codex-rs/utils/path-uri/src/api_path_string.rs index f4a42afbbc..3b9def4ba4 100644 --- a/codex-rs/utils/path-uri/src/api_path_string.rs +++ b/codex-rs/utils/path-uri/src/api_path_string.rs @@ -111,10 +111,7 @@ impl LegacyAppPathString { })?; let is_windows = convention == PathConvention::Windows; let path = self.as_str(); - let home_relative = path - .strip_prefix("~/") - .or_else(|| (path == "~").then_some("")) - .or_else(|| is_windows.then(|| path.strip_prefix(r"~\")).flatten()); + let home_relative = convention.home_relative_suffix(path); if let Some(suffix) = home_relative { let home = user_home_dir.ok_or_else(|| LegacyAppPathStringError::MissingHomeDirectory { @@ -413,6 +410,11 @@ pub enum LegacyAppPathStringError { path: String, convention: Option, }, + #[error("unsupported configuration path {path:?} using {convention} path syntax")] + UnsupportedConfigPath { + path: String, + convention: PathConvention, + }, #[error("path URI `{cwd}` has no path convention")] MissingBaseConvention { cwd: String }, #[error("cannot resolve home-relative path `{path}` without an executor home")] diff --git a/codex-rs/utils/path-uri/src/config_path.rs b/codex-rs/utils/path-uri/src/config_path.rs new file mode 100644 index 0000000000..518c52d1c1 --- /dev/null +++ b/codex-rs/utils/path-uri/src/config_path.rs @@ -0,0 +1,129 @@ +//! Resolve filesystem denial paths with the existing URI parser and lexical join. +//! Reject ambiguous or lossy representations before rendering security constraints. + +use crate::LegacyAppPathString; +use crate::LegacyAppPathStringError; +use crate::PathConvention; +use crate::PathUri; +use crate::PathUriParseError; + +impl PathUri { + /// Resolves configuration text using only the supplied convention, base, and home. + /// Home-relative inputs require a home; other relative inputs require a base. + /// Used facts and results must have an unambiguous, lossless native spelling. + pub fn resolve_config_path( + input: &str, + convention: PathConvention, + base: Option<&Self>, + user_home_dir: Option<&Self>, + ) -> Result { + Self::validate_config_path_text(input, convention)?; + let path = LegacyAppPathString::from_string(input); + let resolved = if convention.home_relative_suffix(input).is_some() { + let home = + user_home_dir.ok_or_else(|| LegacyAppPathStringError::MissingHomeDirectory { + path: input.to_string(), + })?; + home.validate_config_path(convention)?; + path.resolve_against(home, Some(home))? + } else { + match path.to_path_uri(convention) { + Ok(absolute) => absolute, + Err(error) => { + let base = base.ok_or(error)?; + base.validate_config_path(convention)?; + path.resolve_against(base, /*user_home_dir*/ None)? + } + } + }; + resolved.validate_config_path(convention)?; + // Config paths historically omit trailing separators except at a root. + let resolved = resolved.join(".")?; + let mut rendered = LegacyAppPathString::from_path_uri(&resolved, convention)?.into_string(); + if resolved.0.host_str().is_some() && resolved.parent().is_none() { + rendered.push('\\'); + } + Ok(rendered) + } + + /// Checks that a resolved configuration URI has a lossless native spelling + /// in the owning executor's convention. Opaque and ambiguous paths fail. + pub fn validate_config_path( + &self, + convention: PathConvention, + ) -> Result<(), LegacyAppPathStringError> { + if self.infer_path_convention() != Some(convention) { + return Err(LegacyAppPathStringError::IncompatibleConvention { + path: self.to_string(), + convention, + }); + } + let bytes = self.decoded_path_bytes(); + if self.lexical_depth().is_none() + || bytes.contains(&0) + || std::str::from_utf8(&bytes).is_err() + { + return Err(PathUriParseError::InvalidFileUriPath { + path: self.to_string(), + } + .into()); + } + Self::validate_config_path_text( + LegacyAppPathString::from_path_uri(self, convention)?.as_str(), + convention, + ) + } + + /// Validates native configuration text without resolving paths or glob syntax. + /// Rejects spellings that could change targets during later native conversion. + pub fn validate_config_path_text( + input: &str, + convention: PathConvention, + ) -> Result<(), LegacyAppPathStringError> { + let namespace_alias = codex_utils_absolute_path::normalize_windows_device_path(input); + let native_input = namespace_alias.as_deref().unwrap_or(input); + let has_windows_component_colon = convention == PathConvention::Windows + && convention + .path_segments(native_input) + .enumerate() + .any(|(index, segment)| { + let segment = if index == 0 + && segment + .as_bytes() + .first() + .is_some_and(u8::is_ascii_alphabetic) + && segment.as_bytes().get(/*index*/ 1) == Some(&b':') + { + &segment[2..] + } else { + segment + }; + segment.contains(':') + }); + let mixed_home_separators = convention == PathConvention::Windows + && convention + .home_relative_suffix(input) + .is_some_and(|suffix| { + suffix.starts_with('/') && suffix.trim_start_matches('/').starts_with('\\') + || suffix.starts_with('\\') + && suffix.trim_start_matches('\\').starts_with('/') + }); + let bare_windows_drive = convention == PathConvention::Windows + && matches!(native_input.as_bytes(), [drive, b':'] if drive.is_ascii_alphabetic()); + if input.contains('\0') + || has_windows_component_colon + || mixed_home_separators + || bare_windows_drive + { + return Err(LegacyAppPathStringError::UnsupportedConfigPath { + path: input.to_string(), + convention, + }); + } + Ok(()) + } +} + +#[cfg(test)] +#[path = "config_path_tests.rs"] +mod tests; diff --git a/codex-rs/utils/path-uri/src/config_path_tests.rs b/codex-rs/utils/path-uri/src/config_path_tests.rs new file mode 100644 index 0000000000..934d1dd096 --- /dev/null +++ b/codex-rs/utils/path-uri/src/config_path_tests.rs @@ -0,0 +1,130 @@ +//! The configuration boundary shares URI resolution and rejects lossy denials. + +use super::*; +use pretty_assertions::assert_eq; + +#[test] +fn configuration_paths_resolve_with_the_supplied_platform_and_home() { + for (convention, base, home, input, expected) in [ + ( + PathConvention::Posix, + "file:///work/project", + "file:///home/user", + "~/a b/雪%/", + "/home/user/a b/雪%", + ), + ( + PathConvention::Windows, + "file:///C:/work/project", + "file:///C:/Users/user", + r"..\private\", + r"C:\work\private", + ), + ( + PathConvention::Windows, + "file:///C:/work/project", + "file:///C:/Users/user", + r"~\a b\雪%\", + r"C:\Users\user\a b\雪%", + ), + ( + PathConvention::Windows, + "file:///C:/work", + "file:///C:/Users/user", + r"\\server\share", + r"\\server\share\", + ), + ] { + let base = PathUri::parse(base).unwrap(); + let home = PathUri::parse(home).unwrap(); + assert_eq!( + PathUri::resolve_config_path(input, convention, Some(&base), Some(&home)), + Ok(expected.to_string()), + "{input:?}", + ); + if !input.starts_with('.') && !input.starts_with('\\') || input.starts_with(r"\\") { + assert_eq!( + PathUri::resolve_config_path(input, convention, /*base*/ None, Some(&home)), + Ok(expected.to_string()), + "without base: {input:?}", + ); + } + } +} + +#[test] +fn configuration_paths_reject_ambiguous_or_lossy_denials() { + for (convention, base) in [ + (PathConvention::Posix, "file:///base/%FF"), + (PathConvention::Posix, "file:///base/encoded%2Fname"), + (PathConvention::Posix, "file:///C:/base"), + (PathConvention::Windows, "file:///base"), + (PathConvention::Windows, "file:///C:/encoded%5Cname"), + (PathConvention::Windows, "file:///C:/%FF"), + (PathConvention::Windows, "file:///C:/base/a:b"), + ] { + let base = PathUri::parse(base).unwrap(); + assert!(base.validate_config_path(convention).is_err()); + } + let base = PathUri::parse("file:///base").unwrap(); + let opaque = PathUri::from_opaque_path_bytes(b"/base"); + for input in ["nul\0", "nul\0/../private", "/nul\0", "~/private"] { + assert!( + PathUri::resolve_config_path( + input, + PathConvention::Posix, + Some(&base), + /*user_home_dir*/ None + ) + .is_err() + ); + } + assert!( + PathUri::resolve_config_path( + "child", + PathConvention::Posix, + Some(&opaque), + /*user_home_dir*/ None + ) + .is_err() + ); + for input in [ + r"\\server\.\private", + r"\\server\..\private", + r"C:\base\a:b\private", + r"C:\base\a:b\..\private", + ] { + assert!( + PathUri::resolve_config_path( + input, + PathConvention::Windows, + /*base*/ None, + /*user_home_dir*/ None + ) + .is_err() + ); + } + let home = PathUri::parse("file:///C:/Users/user").unwrap(); + for input in [ + "C:", + r"~/\private", + r"~\/private", + r"\\0x7f000001\share\private", + r"\\bücher\share\private", + ] { + assert!( + PathUri::resolve_config_path(input, PathConvention::Windows, Some(&home), Some(&home)) + .is_err() + ); + } + // Unused facts cannot change an already absolute path. + assert_eq!( + PathUri::resolve_config_path( + "/private", + PathConvention::Posix, + Some(&opaque), + Some(&opaque) + ), + Ok("/private".to_string()), + ); +} diff --git a/codex-rs/utils/path-uri/src/lib.rs b/codex-rs/utils/path-uri/src/lib.rs index 3eb754bf6e..18158ffd4d 100644 --- a/codex-rs/utils/path-uri/src/lib.rs +++ b/codex-rs/utils/path-uri/src/lib.rs @@ -24,6 +24,7 @@ use url::Url; mod absolute_path_normalization; mod api_path_string; +mod config_path; mod native_path_bytes; use absolute_path_normalization::path_uri_from_segments; @@ -891,11 +892,19 @@ fn parse_unnormalized_windows_path(path: &str) -> Option { let mut components = path[2..].split(is_windows_separator_char); let host = components.next().filter(|host| !host.is_empty())?; let share = components.next().filter(|share| !share.is_empty())?; + if matches!(host, "." | "..") || matches!(share, "." | "..") { + return Some(windows_opaque_path_uri(path)); + } return path_uri_from_segments( PathConvention::Windows, Some(host), std::iter::once(share).chain(components), ) + .filter(|uri| { + uri.0 + .host_str() + .is_some_and(|parsed| parsed.eq_ignore_ascii_case(host)) + }) .or_else(|| Some(windows_opaque_path_uri(path))); } @@ -995,6 +1004,16 @@ impl PathConvention { Self::Posix } + /// Returns the suffix of `~` paths using this grammar's separators. + /// Named-user paths such as `~someone/private` are not home-relative. + pub fn home_relative_suffix(self, path: &str) -> Option<&str> { + let suffix = path.strip_prefix('~')?; + (suffix.is_empty() + || suffix.starts_with('/') + || self == Self::Windows && suffix.starts_with('\\')) + .then_some(suffix) + } + /// Splits absolute or relative native path text into lexical segments. /// /// This does not validate the path or require it to be absolute. POSIX paths split on `/`,