mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
exec-server: require explicit filesystem sandbox cwd (#19046)
## Why This is a cleanup PR for the `PermissionProfile` migration stack. #19016 fixed remote exec-server sandbox contexts so Docker-backed filesystem requests use a request/container `cwd` instead of leaking the local test runner `cwd`. That exposed the broader API problem: `FileSystemSandboxContext::new(SandboxPolicy)` could still reconstruct filesystem permissions by reading the exec-server process cwd with `AbsolutePathBuf::current_dir()`. That made `cwd`-dependent legacy entries, such as `:cwd`, `:project_roots`, and relative deny globs, depend on ambient process state instead of the request sandbox `cwd`. As later PRs make `PermissionProfile` the primary permissions abstraction, sandbox contexts should be explicit about whether they carry a request `cwd` or are profile-only. Removing the implicit constructor prevents new call sites from accidentally rebuilding permissions against the wrong `cwd`. ## What changed - Removed `FileSystemSandboxContext::new(SandboxPolicy)`. - Kept production callers on explicit constructors: `from_legacy_sandbox_policy(..., cwd)`, `from_permission_profile(...)`, and `from_permission_profile_with_cwd(...)`. - Updated exec-server test helpers to construct `PermissionProfile` values directly instead of routing through legacy `SandboxPolicy` projections. - Updated the environment regression test to use an explicit restricted profile with no synthetic `cwd`. ## Verification - `cargo test -p codex-exec-server` - `just fix -p codex-exec-server` --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/19046). * #18288 * #18287 * #18286 * #18285 * #18284 * #18283 * #18282 * #18281 * #18280 * __->__ #19046
This commit is contained in:
committed by
GitHub
Unverified
parent
46142c3cb0
commit
44dbd9e48a
@@ -443,8 +443,11 @@ mod tests {
|
||||
std::env::current_exe().expect("current exe").as_path(),
|
||||
)
|
||||
.expect("absolute current exe");
|
||||
let sandbox = crate::FileSystemSandboxContext::new(
|
||||
codex_protocol::protocol::SandboxPolicy::new_read_only_policy(),
|
||||
let sandbox = crate::FileSystemSandboxContext::from_permission_profile(
|
||||
codex_protocol::models::PermissionProfile::from_runtime_permissions(
|
||||
&codex_protocol::permissions::FileSystemSandboxPolicy::restricted(Vec::new()),
|
||||
codex_protocol::permissions::NetworkSandboxPolicy::Restricted,
|
||||
),
|
||||
);
|
||||
|
||||
let err = environment
|
||||
|
||||
@@ -57,18 +57,6 @@ pub struct FileSystemSandboxContext {
|
||||
}
|
||||
|
||||
impl FileSystemSandboxContext {
|
||||
pub fn new(sandbox_policy: SandboxPolicy) -> Self {
|
||||
if let Ok(cwd) = AbsolutePathBuf::current_dir() {
|
||||
Self::from_legacy_sandbox_policy(sandbox_policy, cwd)
|
||||
} else {
|
||||
let permissions = PermissionProfile::from_runtime_permissions(
|
||||
&FileSystemSandboxPolicy::from(&sandbox_policy),
|
||||
NetworkSandboxPolicy::from(&sandbox_policy),
|
||||
);
|
||||
Self::from_permission_profile(permissions)
|
||||
}
|
||||
}
|
||||
|
||||
pub fn from_legacy_sandbox_policy(sandbox_policy: SandboxPolicy, cwd: AbsolutePathBuf) -> Self {
|
||||
let permissions = PermissionProfile::from_runtime_permissions(
|
||||
&FileSystemSandboxPolicy::from_legacy_sandbox_policy(&sandbox_policy, cwd.as_path()),
|
||||
|
||||
@@ -350,8 +350,6 @@ mod tests {
|
||||
use codex_protocol::permissions::FileSystemSandboxPolicy;
|
||||
use codex_protocol::permissions::FileSystemSpecialPath;
|
||||
use codex_protocol::permissions::NetworkSandboxPolicy;
|
||||
use codex_protocol::protocol::ReadOnlyAccess;
|
||||
use codex_protocol::protocol::SandboxPolicy;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use pretty_assertions::assert_eq;
|
||||
|
||||
@@ -359,6 +357,7 @@ mod tests {
|
||||
|
||||
use super::FileSystemSandboxRunner;
|
||||
use super::add_helper_runtime_permissions;
|
||||
use super::compatibility_sandbox_policy;
|
||||
use super::helper_env;
|
||||
use super::helper_env_from_vars;
|
||||
use super::helper_env_key_is_allowed;
|
||||
@@ -366,18 +365,10 @@ mod tests {
|
||||
use super::sandbox_cwd;
|
||||
|
||||
#[test]
|
||||
fn helper_permissions_enable_minimal_reads_for_read_only_access() {
|
||||
fn helper_permissions_enable_minimal_reads_for_restricted_profile() {
|
||||
let cwd = AbsolutePathBuf::from_absolute_path(std::env::temp_dir().as_path())
|
||||
.expect("absolute cwd");
|
||||
let sandbox_policy = SandboxPolicy::ReadOnly {
|
||||
access: ReadOnlyAccess::Restricted {
|
||||
include_platform_defaults: false,
|
||||
readable_roots: Vec::new(),
|
||||
},
|
||||
network_access: false,
|
||||
};
|
||||
let mut policy =
|
||||
FileSystemSandboxPolicy::from_legacy_sandbox_policy(&sandbox_policy, cwd.as_path());
|
||||
let mut policy = restricted_policy(Vec::new());
|
||||
|
||||
add_helper_runtime_permissions(&mut policy, /*helper_read_roots*/ &[], cwd.as_path());
|
||||
|
||||
@@ -385,21 +376,13 @@ mod tests {
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn helper_permissions_enable_minimal_reads_for_workspace_read_access() {
|
||||
fn helper_permissions_enable_minimal_reads_for_restricted_profile_with_writes() {
|
||||
let cwd = AbsolutePathBuf::from_absolute_path(std::env::temp_dir().as_path())
|
||||
.expect("absolute cwd");
|
||||
let sandbox_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,
|
||||
};
|
||||
let mut policy =
|
||||
FileSystemSandboxPolicy::from_legacy_sandbox_policy(&sandbox_policy, cwd.as_path());
|
||||
let mut policy = restricted_policy(vec![path_entry(
|
||||
cwd.join("writable"),
|
||||
FileSystemAccessMode::Write,
|
||||
)]);
|
||||
|
||||
add_helper_runtime_permissions(&mut policy, /*helper_read_roots*/ &[], cwd.as_path());
|
||||
|
||||
@@ -415,21 +398,10 @@ mod tests {
|
||||
let cwd = AbsolutePathBuf::from_absolute_path(std::env::temp_dir().as_path())
|
||||
.expect("absolute cwd");
|
||||
let writable = cwd.join("writable");
|
||||
let sandbox_policy = SandboxPolicy::ReadOnly {
|
||||
access: ReadOnlyAccess::Restricted {
|
||||
include_platform_defaults: false,
|
||||
readable_roots: Vec::new(),
|
||||
},
|
||||
network_access: true,
|
||||
};
|
||||
let mut policy =
|
||||
FileSystemSandboxPolicy::from_legacy_sandbox_policy(&sandbox_policy, cwd.as_path());
|
||||
policy.entries.push(FileSystemSandboxEntry {
|
||||
path: FileSystemPath::Path {
|
||||
path: writable.clone(),
|
||||
},
|
||||
access: FileSystemAccessMode::Write,
|
||||
});
|
||||
let mut policy = restricted_policy(vec![path_entry(
|
||||
writable.clone(),
|
||||
FileSystemAccessMode::Write,
|
||||
)]);
|
||||
let readable = AbsolutePathBuf::from_absolute_path(
|
||||
runtime_paths
|
||||
.codex_self_exe
|
||||
@@ -523,16 +495,18 @@ mod tests {
|
||||
.expect("runtime paths");
|
||||
let runner = FileSystemSandboxRunner::new(runtime_paths);
|
||||
let cwd = AbsolutePathBuf::current_dir().expect("cwd");
|
||||
let sandbox_policy = SandboxPolicy::new_workspace_write_policy();
|
||||
let file_system_policy =
|
||||
FileSystemSandboxPolicy::from_legacy_sandbox_policy(&sandbox_policy, cwd.as_path());
|
||||
let sandbox_context = crate::FileSystemSandboxContext::new(sandbox_policy.clone());
|
||||
restricted_policy(vec![path_entry(cwd.clone(), FileSystemAccessMode::Write)]);
|
||||
let network_policy = NetworkSandboxPolicy::Restricted;
|
||||
let sandbox_policy =
|
||||
compatibility_sandbox_policy(&file_system_policy, network_policy, cwd.as_path());
|
||||
let sandbox_context = sandbox_context_with_cwd(&file_system_policy, cwd.clone());
|
||||
|
||||
let request = runner
|
||||
.sandbox_exec_request(
|
||||
&sandbox_policy,
|
||||
&file_system_policy,
|
||||
NetworkSandboxPolicy::Restricted,
|
||||
network_policy,
|
||||
&cwd,
|
||||
&sandbox_context,
|
||||
)
|
||||
@@ -545,10 +519,11 @@ mod tests {
|
||||
fn sandbox_cwd_uses_context_cwd() {
|
||||
let cwd = AbsolutePathBuf::from_absolute_path(std::env::temp_dir().as_path())
|
||||
.expect("absolute cwd");
|
||||
let sandbox_context = crate::FileSystemSandboxContext::from_legacy_sandbox_policy(
|
||||
SandboxPolicy::new_workspace_write_policy(),
|
||||
cwd.clone(),
|
||||
);
|
||||
let policy = restricted_policy(vec![special_entry(
|
||||
FileSystemSpecialPath::CurrentWorkingDirectory,
|
||||
FileSystemAccessMode::Write,
|
||||
)]);
|
||||
let sandbox_context = sandbox_context_with_cwd(&policy, cwd.clone());
|
||||
|
||||
assert_eq!(sandbox_cwd(&sandbox_context).expect("sandbox cwd"), cwd);
|
||||
}
|
||||
@@ -583,15 +558,7 @@ mod tests {
|
||||
.expect("runtime paths");
|
||||
let cwd = AbsolutePathBuf::from_absolute_path(std::env::temp_dir().as_path())
|
||||
.expect("absolute cwd");
|
||||
let sandbox_policy = SandboxPolicy::ReadOnly {
|
||||
access: ReadOnlyAccess::Restricted {
|
||||
include_platform_defaults: false,
|
||||
readable_roots: Vec::new(),
|
||||
},
|
||||
network_access: false,
|
||||
};
|
||||
let mut policy =
|
||||
FileSystemSandboxPolicy::from_legacy_sandbox_policy(&sandbox_policy, cwd.as_path());
|
||||
let mut policy = restricted_policy(Vec::new());
|
||||
let readable = AbsolutePathBuf::from_absolute_path(
|
||||
runtime_paths
|
||||
.codex_self_exe
|
||||
@@ -619,15 +586,7 @@ mod tests {
|
||||
.expect("runtime paths");
|
||||
let cwd = AbsolutePathBuf::from_absolute_path(std::env::temp_dir().as_path())
|
||||
.expect("absolute cwd");
|
||||
let sandbox_policy = SandboxPolicy::ReadOnly {
|
||||
access: ReadOnlyAccess::Restricted {
|
||||
include_platform_defaults: false,
|
||||
readable_roots: Vec::new(),
|
||||
},
|
||||
network_access: false,
|
||||
};
|
||||
let mut policy =
|
||||
FileSystemSandboxPolicy::from_legacy_sandbox_policy(&sandbox_policy, cwd.as_path());
|
||||
let mut policy = restricted_policy(Vec::new());
|
||||
let codex_parent = AbsolutePathBuf::from_absolute_path(root.path().join("bin"))
|
||||
.expect("absolute codex parent");
|
||||
let alias_parent = AbsolutePathBuf::from_absolute_path(root.path().join("aliases"))
|
||||
@@ -642,4 +601,35 @@ mod tests {
|
||||
assert!(policy.can_read_path_with_cwd(codex_parent.as_path(), cwd.as_path()));
|
||||
assert!(policy.can_read_path_with_cwd(alias_parent.as_path(), cwd.as_path()));
|
||||
}
|
||||
|
||||
fn restricted_policy(entries: Vec<FileSystemSandboxEntry>) -> FileSystemSandboxPolicy {
|
||||
FileSystemSandboxPolicy::restricted(entries)
|
||||
}
|
||||
|
||||
fn sandbox_context_with_cwd(
|
||||
policy: &FileSystemSandboxPolicy,
|
||||
cwd: AbsolutePathBuf,
|
||||
) -> crate::FileSystemSandboxContext {
|
||||
crate::FileSystemSandboxContext::from_permission_profile_with_cwd(
|
||||
PermissionProfile::from_runtime_permissions(policy, NetworkSandboxPolicy::Restricted),
|
||||
cwd,
|
||||
)
|
||||
}
|
||||
|
||||
fn path_entry(path: AbsolutePathBuf, access: FileSystemAccessMode) -> FileSystemSandboxEntry {
|
||||
FileSystemSandboxEntry {
|
||||
path: FileSystemPath::Path { path },
|
||||
access,
|
||||
}
|
||||
}
|
||||
|
||||
fn special_entry(
|
||||
value: FileSystemSpecialPath,
|
||||
access: FileSystemAccessMode,
|
||||
) -> FileSystemSandboxEntry {
|
||||
FileSystemSandboxEntry {
|
||||
path: FileSystemPath::Special { value },
|
||||
access,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -192,6 +192,8 @@ mod tests {
|
||||
)
|
||||
.expect("runtime paths");
|
||||
let handler = FileSystemHandler::new(runtime_paths);
|
||||
let sandbox_cwd =
|
||||
AbsolutePathBuf::from_absolute_path(temp_dir.path()).expect("absolute tempdir");
|
||||
|
||||
for (file_name, sandbox_policy) in [
|
||||
("danger.txt", SandboxPolicy::DangerFullAccess),
|
||||
@@ -210,7 +212,10 @@ mod tests {
|
||||
.write_file(FsWriteFileParams {
|
||||
path: path.clone(),
|
||||
data_base64: STANDARD.encode("ok"),
|
||||
sandbox: Some(FileSystemSandboxContext::new(sandbox_policy.clone())),
|
||||
sandbox: Some(FileSystemSandboxContext::from_legacy_sandbox_policy(
|
||||
sandbox_policy.clone(),
|
||||
sandbox_cwd.clone(),
|
||||
)),
|
||||
})
|
||||
.await
|
||||
.expect("write file");
|
||||
@@ -218,7 +223,10 @@ mod tests {
|
||||
let response = handler
|
||||
.read_file(FsReadFileParams {
|
||||
path,
|
||||
sandbox: Some(FileSystemSandboxContext::new(sandbox_policy)),
|
||||
sandbox: Some(FileSystemSandboxContext::from_legacy_sandbox_policy(
|
||||
sandbox_policy,
|
||||
sandbox_cwd.clone(),
|
||||
)),
|
||||
})
|
||||
.await
|
||||
.expect("read file");
|
||||
|
||||
@@ -22,9 +22,11 @@ use codex_exec_server::LocalFileSystem;
|
||||
use codex_exec_server::ReadDirectoryEntry;
|
||||
use codex_exec_server::RemoveOptions;
|
||||
use codex_protocol::models::FileSystemPermissions;
|
||||
use codex_protocol::models::NetworkPermissions;
|
||||
use codex_protocol::models::PermissionProfile;
|
||||
use codex_protocol::protocol::ReadOnlyAccess;
|
||||
use codex_protocol::protocol::SandboxPolicy;
|
||||
use codex_protocol::permissions::FileSystemAccessMode;
|
||||
use codex_protocol::permissions::FileSystemPath;
|
||||
use codex_protocol::permissions::FileSystemSandboxEntry;
|
||||
use codex_sandboxing::policy_transforms::merge_permission_profiles;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use pretty_assertions::assert_eq;
|
||||
@@ -80,38 +82,47 @@ fn absolute_path(path: std::path::PathBuf) -> AbsolutePathBuf {
|
||||
}
|
||||
|
||||
fn read_only_sandbox(readable_root: std::path::PathBuf) -> FileSystemSandboxContext {
|
||||
FileSystemSandboxContext::new(SandboxPolicy::ReadOnly {
|
||||
access: ReadOnlyAccess::Restricted {
|
||||
include_platform_defaults: false,
|
||||
readable_roots: vec![absolute_path(readable_root)],
|
||||
let readable_root = absolute_path(readable_root);
|
||||
sandbox_context(vec![FileSystemSandboxEntry {
|
||||
path: FileSystemPath::Path {
|
||||
path: readable_root,
|
||||
},
|
||||
network_access: false,
|
||||
})
|
||||
access: FileSystemAccessMode::Read,
|
||||
}])
|
||||
}
|
||||
|
||||
fn workspace_write_sandbox(writable_root: std::path::PathBuf) -> FileSystemSandboxContext {
|
||||
FileSystemSandboxContext::new(SandboxPolicy::WorkspaceWrite {
|
||||
writable_roots: vec![absolute_path(writable_root)],
|
||||
read_only_access: ReadOnlyAccess::Restricted {
|
||||
include_platform_defaults: false,
|
||||
readable_roots: vec![],
|
||||
let writable_root = absolute_path(writable_root);
|
||||
sandbox_context(vec![FileSystemSandboxEntry {
|
||||
path: FileSystemPath::Path {
|
||||
path: writable_root,
|
||||
},
|
||||
network_access: false,
|
||||
exclude_tmpdir_env_var: true,
|
||||
exclude_slash_tmp: true,
|
||||
access: FileSystemAccessMode::Write,
|
||||
}])
|
||||
}
|
||||
|
||||
fn sandbox_context(entries: Vec<FileSystemSandboxEntry>) -> FileSystemSandboxContext {
|
||||
FileSystemSandboxContext::from_permission_profile(PermissionProfile {
|
||||
network: Some(NetworkPermissions {
|
||||
enabled: Some(false),
|
||||
}),
|
||||
file_system: Some(FileSystemPermissions {
|
||||
entries,
|
||||
glob_scan_max_depth: None,
|
||||
}),
|
||||
})
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn sandbox_context_new_preserves_legacy_workspace_write_read_only_subpaths() -> Result<()> {
|
||||
fn sandbox_context_from_profile_preserves_workspace_write_read_only_subpaths() -> Result<()> {
|
||||
let tmp = TempDir::new()?;
|
||||
let writable_dir = tmp.path().join("writable");
|
||||
let git_dir = writable_dir.join(".git");
|
||||
std::fs::create_dir_all(&git_dir)?;
|
||||
|
||||
let sandbox = workspace_write_sandbox(writable_dir.clone());
|
||||
let cwd = sandbox.cwd.as_ref().expect("sandbox cwd");
|
||||
let policy = sandbox.permissions.file_system_sandbox_policy();
|
||||
let cwd = absolute_path(writable_dir.clone());
|
||||
let writable_roots = policy.get_writable_roots_with_cwd(cwd.as_path());
|
||||
let writable_dir = absolute_path(std::fs::canonicalize(writable_dir)?);
|
||||
let git_dir = absolute_path(std::fs::canonicalize(git_dir)?);
|
||||
|
||||
Reference in New Issue
Block a user