mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
permissions: make legacy profile conversion cwd-free (#19414)
## Why The profile conversion path still required a `cwd` even when it was only translating a legacy `SandboxPolicy` into a `PermissionProfile`. That made profile producers invent an ambient `cwd`, which is exactly the anchoring we are trying to remove from permission-profile data. A legacy workspace-write policy can be represented symbolically instead: `:cwd = write` plus read-only `:project_roots` metadata subpaths. This PR creates that cwd-free base so the rest of the stack can stop threading cwd through profile construction. Callers that actually need a concrete runtime filesystem policy for a specific cwd still have an explicitly named cwd-bound conversion. ## What Changed - `PermissionProfile::from_legacy_sandbox_policy` now takes only `&SandboxPolicy`. - `FileSystemSandboxPolicy::from_legacy_sandbox_policy` is now the symbolic, cwd-free projection for profiles. - The old concrete projection is retained as `FileSystemSandboxPolicy::from_legacy_sandbox_policy_for_cwd` for runtime/boundary code that must materialize legacy cwd behavior. - Workspace-write profiles preserve `CurrentWorkingDirectory` and `ProjectRoots` special entries instead of materializing cwd into absolute paths. ## Verification - `cargo check -p codex-protocol -p codex-core -p codex-app-server-protocol -p codex-app-server -p codex-exec -p codex-exec-server -p codex-tui -p codex-sandboxing -p codex-linux-sandbox -p codex-analytics --tests` - `just fix -p codex-protocol -p codex-core -p codex-app-server-protocol -p codex-app-server -p codex-exec -p codex-exec-server -p codex-tui -p codex-sandboxing -p codex-linux-sandbox -p codex-analytics` --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/19414). * #19395 * #19394 * #19393 * #19392 * #19391 * __->__ #19414
This commit is contained in:
@@ -429,10 +429,10 @@ impl PermissionProfile {
|
||||
}
|
||||
}
|
||||
|
||||
pub fn from_legacy_sandbox_policy(sandbox_policy: &SandboxPolicy, cwd: &Path) -> Self {
|
||||
pub fn from_legacy_sandbox_policy(sandbox_policy: &SandboxPolicy) -> Self {
|
||||
Self::from_runtime_permissions_with_enforcement(
|
||||
SandboxEnforcement::from_legacy_sandbox_policy(sandbox_policy),
|
||||
&FileSystemSandboxPolicy::from_legacy_sandbox_policy(sandbox_policy, cwd),
|
||||
&FileSystemSandboxPolicy::from_legacy_sandbox_policy(sandbox_policy),
|
||||
NetworkSandboxPolicy::from(sandbox_policy),
|
||||
)
|
||||
}
|
||||
@@ -1765,10 +1765,8 @@ mod tests {
|
||||
#[test]
|
||||
fn permission_profile_round_trip_preserves_disabled_sandbox() -> Result<()> {
|
||||
let cwd = tempdir()?;
|
||||
let permission_profile = PermissionProfile::from_legacy_sandbox_policy(
|
||||
&SandboxPolicy::DangerFullAccess,
|
||||
cwd.path(),
|
||||
);
|
||||
let permission_profile =
|
||||
PermissionProfile::from_legacy_sandbox_policy(&SandboxPolicy::DangerFullAccess);
|
||||
|
||||
assert_eq!(permission_profile, PermissionProfile::Disabled);
|
||||
assert_eq!(
|
||||
@@ -1839,8 +1837,7 @@ mod tests {
|
||||
let sandbox_policy = SandboxPolicy::ExternalSandbox {
|
||||
network_access: crate::protocol::NetworkAccess::Restricted,
|
||||
};
|
||||
let permission_profile =
|
||||
PermissionProfile::from_legacy_sandbox_policy(&sandbox_policy, cwd.path());
|
||||
let permission_profile = PermissionProfile::from_legacy_sandbox_policy(&sandbox_policy);
|
||||
|
||||
assert_eq!(
|
||||
permission_profile,
|
||||
|
||||
@@ -321,7 +321,7 @@ impl FileSystemSandboxPolicy {
|
||||
cwd: &Path,
|
||||
existing: &Self,
|
||||
) -> Self {
|
||||
let mut rebuilt = Self::from_legacy_sandbox_policy(sandbox_policy, cwd);
|
||||
let mut rebuilt = Self::from_legacy_sandbox_policy_for_cwd(sandbox_policy, cwd);
|
||||
if !matches!(rebuilt.kind, FileSystemSandboxKind::Restricted) {
|
||||
return rebuilt;
|
||||
}
|
||||
@@ -413,30 +413,74 @@ impl FileSystemSandboxPolicy {
|
||||
})
|
||||
}
|
||||
|
||||
/// Converts a legacy sandbox policy into a cwd-independent filesystem policy.
|
||||
///
|
||||
/// `WorkspaceWrite` uses symbolic entries for cwd-scoped access so callers
|
||||
/// can preserve the active cwd binding until the policy is actually
|
||||
/// resolved for a turn or command.
|
||||
pub fn from_legacy_sandbox_policy(sandbox_policy: &SandboxPolicy) -> Self {
|
||||
let mut file_system_policy = Self::from(sandbox_policy);
|
||||
let SandboxPolicy::WorkspaceWrite {
|
||||
writable_roots,
|
||||
exclude_tmpdir_env_var,
|
||||
exclude_slash_tmp,
|
||||
..
|
||||
} = sandbox_policy
|
||||
else {
|
||||
return file_system_policy;
|
||||
};
|
||||
|
||||
prune_read_entries_under_writable_roots(
|
||||
&mut file_system_policy.entries,
|
||||
&legacy_non_cwd_writable_roots(
|
||||
writable_roots,
|
||||
*exclude_tmpdir_env_var,
|
||||
*exclude_slash_tmp,
|
||||
),
|
||||
);
|
||||
|
||||
append_default_read_only_project_root_subpath_if_no_explicit_rule(
|
||||
&mut file_system_policy.entries,
|
||||
".git",
|
||||
);
|
||||
append_default_read_only_project_root_subpath_if_no_explicit_rule(
|
||||
&mut file_system_policy.entries,
|
||||
".agents",
|
||||
);
|
||||
append_default_read_only_project_root_subpath_if_no_explicit_rule(
|
||||
&mut file_system_policy.entries,
|
||||
".codex",
|
||||
);
|
||||
for writable_root in writable_roots {
|
||||
for protected_path in default_read_only_subpaths_for_writable_root(
|
||||
writable_root,
|
||||
/*protect_missing_dot_codex*/ false,
|
||||
) {
|
||||
append_default_read_only_path_if_no_explicit_rule(
|
||||
&mut file_system_policy.entries,
|
||||
protected_path,
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
file_system_policy
|
||||
}
|
||||
|
||||
/// Converts a legacy sandbox policy into an equivalent filesystem policy
|
||||
/// for the provided cwd.
|
||||
/// after resolving cwd-sensitive legacy defaults for the provided cwd.
|
||||
///
|
||||
/// Legacy `WorkspaceWrite` policies may list readable roots that live
|
||||
/// under an already-writable root. Those paths were redundant in the
|
||||
/// legacy model and should not become read-only carveouts when projected
|
||||
/// into split filesystem policy.
|
||||
pub fn from_legacy_sandbox_policy(sandbox_policy: &SandboxPolicy, cwd: &Path) -> Self {
|
||||
pub fn from_legacy_sandbox_policy_for_cwd(sandbox_policy: &SandboxPolicy, cwd: &Path) -> Self {
|
||||
let mut file_system_policy = Self::from(sandbox_policy);
|
||||
if let SandboxPolicy::WorkspaceWrite { writable_roots, .. } = sandbox_policy {
|
||||
let legacy_writable_roots = sandbox_policy.get_writable_roots_with_cwd(cwd);
|
||||
file_system_policy.entries.retain(|entry| {
|
||||
if entry.access != FileSystemAccessMode::Read {
|
||||
return true;
|
||||
}
|
||||
|
||||
match &entry.path {
|
||||
FileSystemPath::Path { path } => !legacy_writable_roots
|
||||
.iter()
|
||||
.any(|root| root.is_path_writable(path.as_path())),
|
||||
FileSystemPath::GlobPattern { .. } => true,
|
||||
FileSystemPath::Special { .. } => true,
|
||||
}
|
||||
});
|
||||
prune_read_entries_under_writable_roots(
|
||||
&mut file_system_policy.entries,
|
||||
&legacy_writable_roots,
|
||||
);
|
||||
|
||||
if let Ok(cwd_root) = AbsolutePathBuf::from_absolute_path(cwd) {
|
||||
for protected_path in default_read_only_subpaths_for_writable_root(
|
||||
@@ -584,7 +628,7 @@ impl FileSystemSandboxPolicy {
|
||||
};
|
||||
|
||||
self.semantic_signature(cwd)
|
||||
!= FileSystemSandboxPolicy::from_legacy_sandbox_policy(&legacy_policy, cwd)
|
||||
!= FileSystemSandboxPolicy::from_legacy_sandbox_policy_for_cwd(&legacy_policy, cwd)
|
||||
.semantic_signature(cwd)
|
||||
}
|
||||
|
||||
@@ -1378,41 +1422,92 @@ fn default_read_only_subpaths_for_writable_root(
|
||||
dedup_absolute_paths(subpaths, /*normalize_effective_paths*/ false)
|
||||
}
|
||||
|
||||
fn append_path_entry_if_missing(
|
||||
fn append_default_read_only_project_root_subpath_if_no_explicit_rule(
|
||||
entries: &mut Vec<FileSystemSandboxEntry>,
|
||||
path: AbsolutePathBuf,
|
||||
access: FileSystemAccessMode,
|
||||
subpath: impl Into<PathBuf>,
|
||||
) {
|
||||
if entries.iter().any(|entry| {
|
||||
entry.access == access
|
||||
&& matches!(
|
||||
&entry.path,
|
||||
FileSystemPath::Path { path: existing } if existing == &path
|
||||
)
|
||||
}) {
|
||||
return;
|
||||
}
|
||||
|
||||
entries.push(FileSystemSandboxEntry {
|
||||
path: FileSystemPath::Path { path },
|
||||
access,
|
||||
});
|
||||
append_default_read_only_entry_if_no_explicit_rule(
|
||||
entries,
|
||||
FileSystemPath::Special {
|
||||
value: FileSystemSpecialPath::project_roots(Some(subpath.into())),
|
||||
},
|
||||
);
|
||||
}
|
||||
|
||||
fn append_default_read_only_path_if_no_explicit_rule(
|
||||
entries: &mut Vec<FileSystemSandboxEntry>,
|
||||
path: AbsolutePathBuf,
|
||||
) {
|
||||
if entries.iter().any(|entry| {
|
||||
matches!(
|
||||
&entry.path,
|
||||
FileSystemPath::Path { path: existing } if existing == &path
|
||||
)
|
||||
}) {
|
||||
append_default_read_only_entry_if_no_explicit_rule(entries, FileSystemPath::Path { path });
|
||||
}
|
||||
|
||||
fn append_default_read_only_entry_if_no_explicit_rule(
|
||||
entries: &mut Vec<FileSystemSandboxEntry>,
|
||||
path: FileSystemPath,
|
||||
) {
|
||||
if entries
|
||||
.iter()
|
||||
.any(|entry| file_system_paths_share_target(&entry.path, &path))
|
||||
{
|
||||
return;
|
||||
}
|
||||
|
||||
append_path_entry_if_missing(entries, path, FileSystemAccessMode::Read);
|
||||
entries.push(FileSystemSandboxEntry {
|
||||
path,
|
||||
access: FileSystemAccessMode::Read,
|
||||
});
|
||||
}
|
||||
|
||||
fn prune_read_entries_under_writable_roots(
|
||||
entries: &mut Vec<FileSystemSandboxEntry>,
|
||||
legacy_writable_roots: &[WritableRoot],
|
||||
) {
|
||||
entries.retain(|entry| {
|
||||
if entry.access != FileSystemAccessMode::Read {
|
||||
return true;
|
||||
}
|
||||
|
||||
match &entry.path {
|
||||
FileSystemPath::Path { path } => !legacy_writable_roots
|
||||
.iter()
|
||||
.any(|root| root.is_path_writable(path.as_path())),
|
||||
FileSystemPath::GlobPattern { .. } | FileSystemPath::Special { .. } => true,
|
||||
}
|
||||
});
|
||||
}
|
||||
|
||||
fn legacy_non_cwd_writable_roots(
|
||||
writable_roots: &[AbsolutePathBuf],
|
||||
exclude_tmpdir_env_var: bool,
|
||||
exclude_slash_tmp: bool,
|
||||
) -> Vec<WritableRoot> {
|
||||
let mut roots: Vec<AbsolutePathBuf> = writable_roots.to_vec();
|
||||
|
||||
if cfg!(unix)
|
||||
&& !exclude_slash_tmp
|
||||
&& let Ok(slash_tmp) = AbsolutePathBuf::from_absolute_path("/tmp")
|
||||
&& slash_tmp.as_path().is_dir()
|
||||
{
|
||||
roots.push(slash_tmp);
|
||||
}
|
||||
|
||||
if !exclude_tmpdir_env_var
|
||||
&& let Some(tmpdir) = std::env::var_os("TMPDIR")
|
||||
&& !tmpdir.is_empty()
|
||||
&& let Ok(tmpdir_path) = AbsolutePathBuf::from_absolute_path(PathBuf::from(tmpdir))
|
||||
{
|
||||
roots.push(tmpdir_path);
|
||||
}
|
||||
|
||||
dedup_absolute_paths(roots, /*normalize_effective_paths*/ true)
|
||||
.into_iter()
|
||||
.map(|root| WritableRoot {
|
||||
read_only_subpaths: default_read_only_subpaths_for_writable_root(
|
||||
&root, /*protect_missing_dot_codex*/ false,
|
||||
),
|
||||
root,
|
||||
})
|
||||
.collect()
|
||||
}
|
||||
|
||||
fn has_explicit_resolved_path_entry(
|
||||
@@ -1552,6 +1647,50 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn legacy_workspace_write_projection_preserves_symbolic_cwd() {
|
||||
let policy = SandboxPolicy::WorkspaceWrite {
|
||||
writable_roots: Vec::new(),
|
||||
read_only_access: ReadOnlyAccess::Restricted {
|
||||
include_platform_defaults: false,
|
||||
readable_roots: Vec::new(),
|
||||
},
|
||||
network_access: false,
|
||||
exclude_tmpdir_env_var: true,
|
||||
exclude_slash_tmp: true,
|
||||
};
|
||||
|
||||
assert_eq!(
|
||||
FileSystemSandboxPolicy::from_legacy_sandbox_policy(&policy),
|
||||
FileSystemSandboxPolicy::restricted(vec![
|
||||
FileSystemSandboxEntry {
|
||||
path: FileSystemPath::Special {
|
||||
value: FileSystemSpecialPath::CurrentWorkingDirectory,
|
||||
},
|
||||
access: FileSystemAccessMode::Write,
|
||||
},
|
||||
FileSystemSandboxEntry {
|
||||
path: FileSystemPath::Special {
|
||||
value: FileSystemSpecialPath::project_roots(Some(".git".into())),
|
||||
},
|
||||
access: FileSystemAccessMode::Read,
|
||||
},
|
||||
FileSystemSandboxEntry {
|
||||
path: FileSystemPath::Special {
|
||||
value: FileSystemSpecialPath::project_roots(Some(".agents".into())),
|
||||
},
|
||||
access: FileSystemAccessMode::Read,
|
||||
},
|
||||
FileSystemSandboxEntry {
|
||||
path: FileSystemPath::Special {
|
||||
value: FileSystemSpecialPath::project_roots(Some(".codex".into())),
|
||||
},
|
||||
access: FileSystemAccessMode::Read,
|
||||
},
|
||||
])
|
||||
);
|
||||
}
|
||||
|
||||
#[cfg(unix)]
|
||||
#[test]
|
||||
fn writable_roots_skip_default_dot_codex_when_explicit_user_rule_exists() {
|
||||
@@ -1612,7 +1751,7 @@ mod tests {
|
||||
};
|
||||
|
||||
let file_system_policy =
|
||||
FileSystemSandboxPolicy::from_legacy_sandbox_policy(&policy, cwd.path());
|
||||
FileSystemSandboxPolicy::from_legacy_sandbox_policy_for_cwd(&policy, cwd.path());
|
||||
|
||||
assert!(!file_system_policy.can_write_path_with_cwd(&dot_codex_config, cwd.path()));
|
||||
}
|
||||
@@ -1639,7 +1778,7 @@ mod tests {
|
||||
};
|
||||
|
||||
let file_system_policy =
|
||||
FileSystemSandboxPolicy::from_legacy_sandbox_policy(&policy, relative_cwd);
|
||||
FileSystemSandboxPolicy::from_legacy_sandbox_policy_for_cwd(&policy, relative_cwd);
|
||||
|
||||
assert_eq!(
|
||||
file_system_policy,
|
||||
@@ -2098,7 +2237,7 @@ mod tests {
|
||||
policy.needs_direct_runtime_enforcement(NetworkSandboxPolicy::Restricted, cwd.path(),)
|
||||
);
|
||||
|
||||
let legacy_workspace_write = FileSystemSandboxPolicy::from_legacy_sandbox_policy(
|
||||
let legacy_workspace_write = FileSystemSandboxPolicy::from_legacy_sandbox_policy_for_cwd(
|
||||
&SandboxPolicy::new_workspace_write_policy(),
|
||||
cwd.path(),
|
||||
);
|
||||
|
||||
@@ -3058,7 +3058,7 @@ impl TurnContextItem {
|
||||
self.permission_profile.clone().unwrap_or_else(|| {
|
||||
let file_system_sandbox_policy =
|
||||
self.file_system_sandbox_policy.clone().unwrap_or_else(|| {
|
||||
FileSystemSandboxPolicy::from_legacy_sandbox_policy(
|
||||
FileSystemSandboxPolicy::from_legacy_sandbox_policy_for_cwd(
|
||||
&self.sandbox_policy,
|
||||
&self.cwd,
|
||||
)
|
||||
@@ -4644,7 +4644,7 @@ mod tests {
|
||||
|
||||
assert_eq!(
|
||||
sorted_writable_roots(
|
||||
FileSystemSandboxPolicy::from_legacy_sandbox_policy(&policy, cwd.path())
|
||||
FileSystemSandboxPolicy::from_legacy_sandbox_policy_for_cwd(&policy, cwd.path())
|
||||
.get_writable_roots_with_cwd(cwd.path())
|
||||
),
|
||||
vec![(canonical_cwd, vec![expected_dot_codex.to_path_buf()])]
|
||||
@@ -4736,9 +4736,10 @@ mod tests {
|
||||
];
|
||||
|
||||
for expected in policies {
|
||||
let actual = FileSystemSandboxPolicy::from_legacy_sandbox_policy(&expected, cwd.path())
|
||||
.to_legacy_sandbox_policy(NetworkSandboxPolicy::from(&expected), cwd.path())
|
||||
.expect("legacy bridge should preserve legacy policy semantics");
|
||||
let actual =
|
||||
FileSystemSandboxPolicy::from_legacy_sandbox_policy_for_cwd(&expected, cwd.path())
|
||||
.to_legacy_sandbox_policy(NetworkSandboxPolicy::from(&expected), cwd.path())
|
||||
.expect("legacy bridge should preserve legacy policy semantics");
|
||||
|
||||
assert_same_sandbox_policy_semantics(&expected, &actual, cwd.path());
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user