mirror of
https://github.com/openai/codex.git
synced 2026-11-02 19:43:24 +00:00
fix(permissions): preserve managed deny_read across mode changes
This commit is contained in:
@@ -148,6 +148,30 @@ impl<T: Send + Sync> Constrained<T> {
|
||||
self.value = value;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
pub fn add_normalizer(
|
||||
&mut self,
|
||||
normalizer: impl Fn(T) -> T + Send + Sync + 'static,
|
||||
) -> ConstraintResult<()>
|
||||
where
|
||||
T: Clone + 'static,
|
||||
{
|
||||
let existing_normalizer = self.normalizer.clone();
|
||||
let combined_normalizer: Arc<ConstraintNormalizer<T>> = Arc::new(move |value| {
|
||||
let value = if let Some(existing) = &existing_normalizer {
|
||||
existing(value)
|
||||
} else {
|
||||
value
|
||||
};
|
||||
normalizer(value)
|
||||
});
|
||||
|
||||
let normalized = combined_normalizer(self.value.clone());
|
||||
(self.validator)(&normalized)?;
|
||||
self.value = normalized;
|
||||
self.normalizer = Some(combined_normalizer);
|
||||
Ok(())
|
||||
}
|
||||
}
|
||||
|
||||
impl<T> std::ops::Deref for Constrained<T> {
|
||||
@@ -224,6 +248,22 @@ mod tests {
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn constrained_add_normalizer_composes_with_existing_normalizer() -> anyhow::Result<()> {
|
||||
let mut constrained = Constrained::normalized(-1, |value| value.max(0))?;
|
||||
constrained.add_normalizer(|value| value.min(5))?;
|
||||
|
||||
assert_eq!(constrained.value(), 0);
|
||||
|
||||
constrained.set(10)?;
|
||||
assert_eq!(constrained.value(), 5);
|
||||
|
||||
constrained.set(-10)?;
|
||||
assert_eq!(constrained.value(), 0);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn constrained_new_rejects_invalid_initial_value() {
|
||||
let result = Constrained::new(0, |value| {
|
||||
|
||||
@@ -12,6 +12,7 @@ pub use config_requirements::ConfigRequirements;
|
||||
pub use config_requirements::ConfigRequirementsToml;
|
||||
pub use config_requirements::ConfigRequirementsWithSources;
|
||||
pub use config_requirements::ConstrainedWithSource;
|
||||
pub use config_requirements::FilesystemConstraints;
|
||||
pub use config_requirements::McpServerIdentity;
|
||||
pub use config_requirements::McpServerRequirement;
|
||||
pub use config_requirements::NetworkConstraints;
|
||||
|
||||
@@ -1910,18 +1910,20 @@ impl Config {
|
||||
}) = filesystem_requirements
|
||||
&& !filesystem_requirements.deny_read.is_empty()
|
||||
{
|
||||
let mut sandbox_policy_with_deny_read = constrained_sandbox_policy.get().clone();
|
||||
let deny_read_paths = filesystem_requirements.deny_read;
|
||||
constrained_sandbox_policy
|
||||
.value
|
||||
.add_normalizer(move |mut policy| {
|
||||
policy.append_deny_read_paths(&deny_read_paths);
|
||||
policy
|
||||
})
|
||||
.map_err(std::io::Error::from)?;
|
||||
|
||||
let supports_deny_read = matches!(
|
||||
sandbox_policy_with_deny_read,
|
||||
constrained_sandbox_policy.get(),
|
||||
SandboxPolicy::ReadOnly { .. } | SandboxPolicy::WorkspaceWrite { .. }
|
||||
);
|
||||
sandbox_policy_with_deny_read
|
||||
.append_deny_read_paths(&filesystem_requirements.deny_read);
|
||||
if supports_deny_read {
|
||||
constrained_sandbox_policy
|
||||
.set(sandbox_policy_with_deny_read)
|
||||
.map_err(std::io::Error::from)?;
|
||||
|
||||
if cfg!(target_os = "windows") {
|
||||
startup_warnings.push(format!(
|
||||
"managed filesystem deny_read from {filesystem_requirements_source} is only enforced for direct file tools on Windows; shell subprocess reads are not sandboxed"
|
||||
@@ -5637,6 +5639,49 @@ mcp_oauth_callback_url = "https://example.com/callback"
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn requirements_filesystem_deny_read_persists_when_sandbox_mode_changes()
|
||||
-> std::io::Result<()> {
|
||||
let codex_home = TempDir::new()?;
|
||||
let denied_path = codex_home.path().join("sensitive").join("secret.txt");
|
||||
let denied_path =
|
||||
AbsolutePathBuf::try_from(denied_path).expect("deny_read test path should be absolute");
|
||||
|
||||
let mut config = ConfigBuilder::default()
|
||||
.codex_home(codex_home.path().to_path_buf())
|
||||
.fallback_cwd(Some(codex_home.path().to_path_buf()))
|
||||
.cloud_requirements(CloudRequirementsLoader::new({
|
||||
let denied_path = denied_path.clone();
|
||||
async move {
|
||||
Some(
|
||||
toml::from_str::<crate::config_loader::ConfigRequirementsToml>(&format!(
|
||||
r#"
|
||||
[permissions.filesystem]
|
||||
deny_read = [{:?}]
|
||||
"#,
|
||||
denied_path.as_path().display().to_string()
|
||||
))
|
||||
.expect("parse requirements toml"),
|
||||
)
|
||||
}
|
||||
}))
|
||||
.build()
|
||||
.await?;
|
||||
|
||||
config
|
||||
.permissions
|
||||
.sandbox_policy
|
||||
.set(SandboxPolicy::new_workspace_write_policy())
|
||||
.map_err(std::io::Error::from)?;
|
||||
|
||||
assert_eq!(
|
||||
config.permissions.sandbox_policy.get().denied_read_paths(),
|
||||
vec![denied_path]
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn requirements_web_search_mode_overrides_danger_full_access_default()
|
||||
-> std::io::Result<()> {
|
||||
|
||||
@@ -33,6 +33,7 @@ pub use codex_config::ConfigLayerStackOrdering;
|
||||
pub use codex_config::ConfigRequirements;
|
||||
pub use codex_config::ConfigRequirementsToml;
|
||||
pub use codex_config::ConstrainedWithSource;
|
||||
pub use codex_config::FilesystemConstraints;
|
||||
pub use codex_config::LoaderOverrides;
|
||||
pub use codex_config::McpServerIdentity;
|
||||
pub use codex_config::McpServerRequirement;
|
||||
|
||||
@@ -5114,6 +5114,7 @@ async fn preset_matching_requires_exact_workspace_write_settings() {
|
||||
let current_sandbox = SandboxPolicy::WorkspaceWrite {
|
||||
writable_roots: vec![AbsolutePathBuf::try_from("C:\\extra").unwrap()],
|
||||
read_only_access: Default::default(),
|
||||
deny_read_paths: Vec::new(),
|
||||
network_access: false,
|
||||
exclude_tmpdir_env_var: false,
|
||||
exclude_slash_tmp: false,
|
||||
|
||||
@@ -164,6 +164,22 @@ fn render_debug_config_lines(stack: &ConfigLayerStack) -> Vec<Line<'static>> {
|
||||
));
|
||||
}
|
||||
|
||||
if let Some(filesystem) = requirements.filesystem.as_ref() {
|
||||
let deny_read = join_or_empty(
|
||||
filesystem
|
||||
.value
|
||||
.deny_read
|
||||
.iter()
|
||||
.map(|path| path.as_path().display().to_string())
|
||||
.collect::<Vec<_>>(),
|
||||
);
|
||||
requirement_lines.push(requirement_line(
|
||||
"permissions.filesystem.deny_read",
|
||||
deny_read,
|
||||
Some(&filesystem.source),
|
||||
));
|
||||
}
|
||||
|
||||
if requirement_lines.is_empty() {
|
||||
lines.push(" <none>".dim().into());
|
||||
} else {
|
||||
@@ -396,6 +412,7 @@ mod tests {
|
||||
use codex_core::config_loader::ConfigRequirements;
|
||||
use codex_core::config_loader::ConfigRequirementsToml;
|
||||
use codex_core::config_loader::ConstrainedWithSource;
|
||||
use codex_core::config_loader::FilesystemConstraints;
|
||||
use codex_core::config_loader::McpServerIdentity;
|
||||
use codex_core::config_loader::McpServerRequirement;
|
||||
use codex_core::config_loader::NetworkConstraints;
|
||||
@@ -481,6 +498,11 @@ mod tests {
|
||||
} else {
|
||||
absolute_path("/etc/codex/requirements.toml")
|
||||
};
|
||||
let denied_path = if cfg!(windows) {
|
||||
absolute_path("C:\\Users\\alice\\.gitconfig")
|
||||
} else {
|
||||
absolute_path("/home/alice/.gitconfig")
|
||||
};
|
||||
|
||||
let requirements = ConfigRequirements {
|
||||
approval_policy: ConstrainedWithSource::new(
|
||||
@@ -520,6 +542,14 @@ mod tests {
|
||||
},
|
||||
RequirementSource::CloudRequirements,
|
||||
)),
|
||||
filesystem: Some(Sourced::new(
|
||||
FilesystemConstraints {
|
||||
deny_read: vec![denied_path.clone()],
|
||||
},
|
||||
RequirementSource::SystemRequirementsToml {
|
||||
file: requirements_file.clone(),
|
||||
},
|
||||
)),
|
||||
..ConfigRequirements::default()
|
||||
};
|
||||
|
||||
@@ -538,6 +568,7 @@ mod tests {
|
||||
rules: None,
|
||||
enforce_residency: Some(ResidencyRequirement::Us),
|
||||
network: None,
|
||||
permissions: None,
|
||||
};
|
||||
|
||||
let user_file = if cfg!(windows) {
|
||||
@@ -578,6 +609,15 @@ mod tests {
|
||||
assert!(rendered.contains(
|
||||
"experimental_network: enabled=true, allowed_domains=[example.com] (source: cloud requirements)"
|
||||
));
|
||||
assert!(
|
||||
rendered.contains(
|
||||
format!(
|
||||
"permissions.filesystem.deny_read: {}",
|
||||
denied_path.as_path().display()
|
||||
)
|
||||
.as_str()
|
||||
)
|
||||
);
|
||||
assert!(!rendered.contains(" - rules:"));
|
||||
}
|
||||
#[test]
|
||||
@@ -656,6 +696,7 @@ approval_policy = "never"
|
||||
rules: None,
|
||||
enforce_residency: None,
|
||||
network: None,
|
||||
permissions: None,
|
||||
};
|
||||
|
||||
let stack = ConfigLayerStack::new(Vec::new(), requirements, requirements_toml)
|
||||
|
||||
@@ -103,6 +103,7 @@ async fn status_snapshot_includes_reasoning_details() {
|
||||
.set(SandboxPolicy::WorkspaceWrite {
|
||||
writable_roots: Vec::new(),
|
||||
read_only_access: Default::default(),
|
||||
deny_read_paths: Vec::new(),
|
||||
network_access: false,
|
||||
exclude_tmpdir_env_var: false,
|
||||
exclude_slash_tmp: false,
|
||||
@@ -188,6 +189,7 @@ async fn status_permissions_non_default_workspace_write_is_custom() {
|
||||
.set(SandboxPolicy::WorkspaceWrite {
|
||||
writable_roots: Vec::new(),
|
||||
read_only_access: Default::default(),
|
||||
deny_read_paths: Vec::new(),
|
||||
network_access: true,
|
||||
exclude_tmpdir_env_var: false,
|
||||
exclude_slash_tmp: false,
|
||||
|
||||
Reference in New Issue
Block a user