agent/codex
1
0
Fork 0
mirror of https://github.com/pchuan98/codex.git synced 2026-07-01 00:31:56 +08:00
Code Issues Packages Projects Releases Wiki Activity

sandboxing: materialize cwd-relative permission globs (#18867)

Browse Source 
## Why

#18275 anchors session-scoped `:cwd` and `:project_roots` grants to the
request cwd before recording them for reuse. Relative deny glob entries
need the same treatment. Without anchoring, a stored session permission
can keep a pattern such as `**/*.env` relative, then reinterpret that
deny against a later turn cwd. That makes the persisted profile depend
on the cwd at reuse time instead of the cwd that was reviewed and
approved.

## What changed

`intersect_permission_profiles` now materializes retained
`FileSystemPath::GlobPattern` entries against the request cwd, matching
the existing materialization for cwd-sensitive special paths.

Materialized accepted grants are now deduplicated before deny retention
runs. This keeps the sticky-grant preapproval shape stable when a
repeated request is merged with the stored grant and both `:cwd = write`
and the materialized absolute cwd write are present.

The preapproval check compares against the same materialized form, so a
later request for the same cwd-relative deny glob still matches the
stored anchored grant instead of re-prompting or rejecting.

Tests cover both the storage path and the preapproval path: a
session-scoped `:cwd = write` grant with `**/*.env = none` is stored
with both the cwd write and deny glob anchored to the original request
cwd, cannot be reused from a later cwd, and remains preapproved when
re-requested from the original cwd after merging with the stored grant.

## Verification

- `cargo test -p codex-sandboxing policy_transforms`
- `cargo test -p codex-core --lib
relative_deny_glob_grants_remain_preapproved_after_materialization`
- `cargo clippy -p codex-sandboxing --tests -- -D
clippy::redundant_clone`
- `cargo clippy -p codex-core --lib -- -D clippy::redundant_clone`

---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/openai/codex/pull/18867).
* #18288
* #18287
* #18286
* #18285
* #18284
* #18283
* #18282
* #18281
* #18280
* #18279
* #18278
* #18277
* #18276
* __->__ #18867
This commit is contained in:
Michael Bolin
2026-04-21 17:28:58 -07:00
committed by GitHub
Unverified
parent 37701d4654
commit 799e50412e
3 changed files with 220 additions and 23 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/core/src/tools/handlers/mod.rs
+61 -2
View File
@@ -193,8 +193,7 @@ pub(super) async fn apply_granted_turn_permissions(
);
let permissions_preapproved = match (effective_permissions.as_ref(), granted_permissions) {
(Some(effective_permissions), Some(granted_permissions)) => {
intersect_permission_profiles(effective_permissions.clone(), granted_permissions, cwd)
== *effective_permissions
permissions_are_preapproved(effective_permissions, granted_permissions, cwd)
}
_ => false,
};
@@ -213,17 +212,38 @@ pub(super) async fn apply_granted_turn_permissions(
}
}
fn permissions_are_preapproved(
effective_permissions: &PermissionProfile,
granted_permissions: PermissionProfile,
cwd: &Path,
) -> bool {
let materialized_effective_permissions = intersect_permission_profiles(
effective_permissions.clone(),
effective_permissions.clone(),
cwd,
);
intersect_permission_profiles(effective_permissions.clone(), granted_permissions, cwd)
== materialized_effective_permissions
}
#[cfg(test)]
mod tests {
use super::EffectiveAdditionalPermissions;
use super::implicit_granted_permissions;
use super::normalize_and_validate_additional_permissions;
use super::permissions_are_preapproved;
use crate::sandboxing::SandboxPermissions;
use codex_protocol::models::FileSystemPermissions;
use codex_protocol::models::NetworkPermissions;
use codex_protocol::models::PermissionProfile;
use codex_protocol::permissions::FileSystemAccessMode;
use codex_protocol::permissions::FileSystemPath;
use codex_protocol::permissions::FileSystemSandboxEntry;
use codex_protocol::permissions::FileSystemSpecialPath;
use codex_protocol::protocol::AskForApproval;
use codex_protocol::protocol::GranularApprovalConfig;
use codex_sandboxing::policy_transforms::intersect_permission_profiles;
use codex_sandboxing::policy_transforms::merge_permission_profiles;
use codex_utils_absolute_path::AbsolutePathBuf;
use pretty_assertions::assert_eq;
use tempfile::tempdir;
@@ -326,4 +346,43 @@ mod tests {
assert_eq!(implicit_permissions, None);
}
#[test]
fn relative_deny_glob_grants_remain_preapproved_after_materialization() {
let cwd = tempdir().expect("tempdir");
let requested_permissions = PermissionProfile {
file_system: Some(FileSystemPermissions {
entries: vec![
FileSystemSandboxEntry {
path: FileSystemPath::Special {
value: FileSystemSpecialPath::CurrentWorkingDirectory,
},
access: FileSystemAccessMode::Write,
},
FileSystemSandboxEntry {
path: FileSystemPath::GlobPattern {
pattern: "**/*.env".to_string(),
},
access: FileSystemAccessMode::None,
},
],
glob_scan_max_depth: None,
}),
..Default::default()
};
let stored_grant = intersect_permission_profiles(
requested_permissions.clone(),
requested_permissions.clone(),
cwd.path(),
);
let effective_permissions =
merge_permission_profiles(Some(&requested_permissions), Some(&stored_grant))
.expect("merged permissions");
assert!(permissions_are_preapproved(
&effective_permissions,
stored_grant,
cwd.path(),
));
}
}
codex-rs/sandboxing/src/policy_transforms.rs
+24 -17
View File
@@ -159,20 +159,21 @@ pub fn intersect_permission_profiles(
let requested_policy =
FileSystemSandboxPolicy::restricted(requested_file_system.entries.clone());
let requested_read_deny_matcher = ReadDenyMatcher::new(&requested_policy, cwd);
let accepted_entries: Vec<_> = granted_file_system
.entries
.iter()
.filter(|entry| {
granted_file_system_entry_within_request(
&requested_file_system,
&requested_policy,
requested_read_deny_matcher.as_ref(),
entry,
cwd,
)
})
.map(|entry| materialize_cwd_dependent_entry(entry, cwd))
.collect();
let mut accepted_entries = Vec::new();
for entry in granted_file_system.entries.iter().filter(|entry| {
granted_file_system_entry_within_request(
&requested_file_system,
&requested_policy,
requested_read_deny_matcher.as_ref(),
entry,
cwd,
)
}) {
let entry = materialize_cwd_dependent_entry(entry, cwd);
if !accepted_entries.contains(&entry) {
accepted_entries.push(entry);
}
}
let mut entries = accepted_entries.clone();
let requested_retained_deny_entries = retain_constraining_deny_entries(
&requested_file_system.entries,
@@ -383,9 +384,15 @@ fn materialize_cwd_dependent_entry(
access: entry.access,
})
.unwrap_or_else(|| entry.clone()),
FileSystemPath::Path { .. }
| FileSystemPath::GlobPattern { .. }
| FileSystemPath::Special { .. } => entry.clone(),
FileSystemPath::GlobPattern { pattern } => FileSystemSandboxEntry {
path: FileSystemPath::GlobPattern {
pattern: AbsolutePathBuf::resolve_path_against_base(pattern, cwd)
.to_string_lossy()
.into_owned(),
},
access: entry.access,
},
FileSystemPath::Path { .. } | FileSystemPath::Special { .. } => entry.clone(),
}
}
codex-rs/sandboxing/src/policy_transforms_tests.rs
+135 -4
View File
@@ -377,6 +377,42 @@ fn intersect_permission_profiles_materializes_cwd_grant_for_reuse() {
);
}
#[test]
fn intersect_permission_profiles_deduplicates_materialized_grants() {
let temp_dir = TempDir::new().expect("create temp dir");
let cwd =
AbsolutePathBuf::from_absolute_path(temp_dir.path().join("cwd")).expect("absolute cwd");
let permissions = PermissionProfile {
file_system: Some(FileSystemPermissions {
entries: vec![
FileSystemSandboxEntry {
path: FileSystemPath::Special {
value: FileSystemSpecialPath::CurrentWorkingDirectory,
},
access: FileSystemAccessMode::Write,
},
FileSystemSandboxEntry {
path: FileSystemPath::Path { path: cwd.clone() },
access: FileSystemAccessMode::Write,
},
],
glob_scan_max_depth: None,
}),
..Default::default()
};
assert_eq!(
intersect_permission_profiles(permissions.clone(), permissions, cwd.as_path()),
PermissionProfile {
file_system: Some(FileSystemPermissions::from_read_write_roots(
/*read*/ None,
Some(vec![cwd]),
)),
..Default::default()
}
);
}
#[test]
fn intersect_permission_profiles_materializes_cwd_deny_entries() {
let temp_dir = TempDir::new().expect("create temp dir");
@@ -509,6 +545,75 @@ fn intersect_permission_profiles_rejects_concrete_grants_matched_by_requested_de
);
}
#[test]
fn intersect_permission_profiles_materializes_relative_deny_globs_for_reuse() {
let temp_dir = TempDir::new().expect("create temp dir");
let request_cwd = AbsolutePathBuf::from_absolute_path(temp_dir.path().join("request-cwd"))
.expect("absolute request cwd");
let later_cwd = AbsolutePathBuf::from_absolute_path(temp_dir.path().join("later-cwd"))
.expect("absolute later cwd");
let cwd_write = FileSystemSandboxEntry {
path: FileSystemPath::Special {
value: FileSystemSpecialPath::CurrentWorkingDirectory,
},
access: FileSystemAccessMode::Write,
};
let deny_env_files = FileSystemSandboxEntry {
path: FileSystemPath::GlobPattern {
pattern: "**/*.env".to_string(),
},
access: FileSystemAccessMode::None,
};
let permissions = PermissionProfile {
file_system: Some(FileSystemPermissions {
entries: vec![cwd_write, deny_env_files],
glob_scan_max_depth: std::num::NonZeroUsize::new(2),
}),
..Default::default()
};
let intersected =
intersect_permission_profiles(permissions.clone(), permissions, request_cwd.as_path());
assert_eq!(
intersected,
PermissionProfile {
file_system: Some(FileSystemPermissions {
entries: vec![
FileSystemSandboxEntry {
path: FileSystemPath::Path {
path: request_cwd.clone(),
},
access: FileSystemAccessMode::Write,
},
FileSystemSandboxEntry {
path: FileSystemPath::GlobPattern {
pattern: request_cwd.join("**/*.env").to_string_lossy().into_owned(),
},
access: FileSystemAccessMode::None,
},
],
glob_scan_max_depth: std::num::NonZeroUsize::new(2),
}),
..Default::default()
}
);
assert_eq!(
intersect_permission_profiles(
PermissionProfile {
file_system: Some(FileSystemPermissions::from_read_write_roots(
/*read*/ None,
Some(vec![later_cwd.join("token.env")]),
)),
..Default::default()
},
intersected,
later_cwd.as_path(),
),
PermissionProfile::default()
);
}
#[test]
fn intersect_permission_profiles_drops_broader_cwd_grant_for_requested_child_path() {
let temp_dir = TempDir::new().expect("create temp dir");
@@ -567,7 +672,7 @@ fn intersect_permission_profiles_uses_granted_bounded_glob_scan_depth() {
};
let granted = PermissionProfile {
file_system: Some(FileSystemPermissions {
entries: vec![root_write.clone(), deny_env_files.clone()],
entries: vec![root_write.clone(), deny_env_files],
glob_scan_max_depth: std::num::NonZeroUsize::new(4),
}),
..Default::default()
@@ -577,7 +682,20 @@ fn intersect_permission_profiles_uses_granted_bounded_glob_scan_depth() {
intersect_permission_profiles(requested, granted, cwd.as_path()),
PermissionProfile {
file_system: Some(FileSystemPermissions {
entries: vec![root_write, deny_env_files],
entries: vec![
root_write,
FileSystemSandboxEntry {
path: FileSystemPath::GlobPattern {
pattern: AbsolutePathBuf::resolve_path_against_base(
"**/*.env",
cwd.as_path()
)
.to_string_lossy()
.into_owned(),
},
access: FileSystemAccessMode::None,
},
],
glob_scan_max_depth: std::num::NonZeroUsize::new(4),
}),
..Default::default()
@@ -609,7 +727,7 @@ fn intersect_permission_profiles_uses_granted_unbounded_glob_scan_depth() {
};
let granted = PermissionProfile {
file_system: Some(FileSystemPermissions {
entries: vec![root_write.clone(), deny_env_files.clone()],
entries: vec![root_write.clone(), deny_env_files],
glob_scan_max_depth: None,
}),
..Default::default()
@@ -619,7 +737,20 @@ fn intersect_permission_profiles_uses_granted_unbounded_glob_scan_depth() {
intersect_permission_profiles(requested, granted, cwd.as_path()),
PermissionProfile {
file_system: Some(FileSystemPermissions {
entries: vec![root_write, deny_env_files],
entries: vec![
root_write,
FileSystemSandboxEntry {
path: FileSystemPath::GlobPattern {
pattern: AbsolutePathBuf::resolve_path_against_base(
"**/*.env",
cwd.as_path()
)
.to_string_lossy()
.into_owned(),
},
access: FileSystemAccessMode::None,
},
],
glob_scan_max_depth: None,
}),
..Default::default()