mirror of
https://github.com/openai/codex.git
synced 2026-10-20 08:19:50 +00:00
Resolve filesystem denials with explicit path context (#44669)
## Why Filesystem denial paths need to use the owning environment's path syntax, base directory, and home directory. Host-native resolution cannot supply those facts for another platform, and invalid denials must not be silently skipped when building the sandbox policy. ## What changed - Add `ConfigPathContext` to requirements layers so `permissions.filesystem.deny_read` can resolve using explicit POSIX or Windows path facts, with native defaults when no context is supplied. - Share URI-based resolution and validation for literal paths and glob prefixes. Reject ambiguous or lossy paths, including NUL bytes, Windows stream syntax, and unsupported UNC spellings. - Move denial conversion into `FilesystemConstraints::apply_to_policy`, preserving glob patterns and deduplicating entries. Validate all denials before modifying the policy and propagate failures through configuration loading. ## Testing Add tests for per-layer base and home resolution, Windows drives and globs, nested context restoration, missing home directories, and policy conversion without partial mutation. Add a configuration-loading regression test for a required denial glob containing a NUL byte. GitOrigin-RevId: fa0da0d149407958e39897a39fe7b87edd6f1644
This commit is contained in:
@@ -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<Self, String> {
|
||||
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, String> {
|
||||
AbsolutePathBuf::deserialize(StrDeserializer::<ValueDeserializerError>::new(input))
|
||||
.map_err(|err| err.to_string())
|
||||
fn deserialize_absolute_path(input: &str) -> Result<String, String> {
|
||||
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 == '/'
|
||||
|
||||
@@ -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::<Result<Vec<_>, 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(())
|
||||
}
|
||||
}
|
||||
@@ -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;
|
||||
|
||||
@@ -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<PathUri>,
|
||||
user_home_dir: Option<PathUri>,
|
||||
}
|
||||
|
||||
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<PathUri>,
|
||||
user_home_dir: Option<PathUri>,
|
||||
) -> 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<Option<ConfigPathContext>> = const { RefCell::new(None) };
|
||||
}
|
||||
|
||||
pub(crate) struct PathContextGuard(Option<ConfigPathContext>);
|
||||
|
||||
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<String, String> {
|
||||
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;
|
||||
@@ -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());
|
||||
});
|
||||
}
|
||||
@@ -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<AbsolutePathBuf>,
|
||||
path_context: Option<ConfigPathContext>,
|
||||
}
|
||||
|
||||
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<AbsolutePathBuf>), 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,
|
||||
|
||||
@@ -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");
|
||||
|
||||
@@ -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);
|
||||
|
||||
@@ -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<Option<PathBuf>, &'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<PathBuf> {
|
||||
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),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -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<PathConvention>,
|
||||
},
|
||||
#[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")]
|
||||
|
||||
@@ -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<String, LegacyAppPathStringError> {
|
||||
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;
|
||||
@@ -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()),
|
||||
);
|
||||
}
|
||||
@@ -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<PathUri> {
|
||||
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 `/`,
|
||||
|
||||
Reference in New Issue
Block a user