mirror of
https://github.com/openai/codex.git
synced 2026-05-28 15:00:16 +00:00
## Why `SandboxPolicy` is now a legacy compatibility shape, but several tests still built a `SandboxPolicy` only to immediately convert it into `PermissionProfile` for APIs that already accept canonical runtime permissions. Those detours make it harder to audit where legacy sandbox policy is still required, because boundary-only usages are mixed together with ordinary test setup. ## What Changed - Updated tests in `codex-core`, `codex-exec`, `codex-analytics`, and `codex-config` to construct `PermissionProfile` values directly when the code under test takes a permission profile. - Changed exec-policy, request-permissions, session, and sandbox test helpers to pass `PermissionProfile` through instead of converting from `SandboxPolicy` internally. - Left `SandboxPolicy` in place where tests are explicitly exercising legacy compatibility or request/response boundaries. ## Test Plan - `cargo test -p codex-analytics -p codex-config` - `cargo test -p codex-core --lib safety::tests` - `cargo test -p codex-core --lib exec_policy::tests::` - `cargo test -p codex-core --lib exec::tests` - `cargo test -p codex-core --lib guardian_review_session_config` - `cargo test -p codex-core --lib tools::network_approval::tests` - `cargo test -p codex-core --lib tools::runtimes::shell::unix_escalation::tests` - `cargo test -p codex-core --lib managed_network` - `cargo test -p codex-core --test all request_permissions::` - `cargo test -p codex-exec sandbox` --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/23030). * #23036 * __->__ #23030
326 lines
10 KiB
Rust
326 lines
10 KiB
Rust
use super::*;
|
||
use codex_protocol::models::PermissionProfile;
|
||
use codex_protocol::permissions::NetworkSandboxPolicy;
|
||
use codex_protocol::protocol::FileSystemAccessMode;
|
||
use codex_protocol::protocol::FileSystemPath;
|
||
use codex_protocol::protocol::FileSystemSandboxEntry;
|
||
use codex_protocol::protocol::FileSystemSpecialPath;
|
||
use codex_protocol::protocol::GranularApprovalConfig;
|
||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||
use core_test_support::PathExt;
|
||
use pretty_assertions::assert_eq;
|
||
use tempfile::TempDir;
|
||
|
||
#[test]
|
||
fn test_writable_roots_constraint() {
|
||
// Use a temporary directory as our workspace to avoid touching
|
||
// the real current working directory.
|
||
let tmp = TempDir::new().unwrap();
|
||
let cwd = tmp.path().abs();
|
||
let parent = cwd.parent().unwrap();
|
||
|
||
// Helper to build a single‑entry patch that adds a file at `p`.
|
||
let make_add_change =
|
||
|p: AbsolutePathBuf| ApplyPatchAction::new_add_for_test(&p, "".to_string());
|
||
|
||
let add_inside = make_add_change(cwd.join("inner.txt"));
|
||
let add_outside = make_add_change(parent.join("outside.txt"));
|
||
|
||
// Policy limited to the workspace only; exclude system temp roots so
|
||
// only `cwd` is writable by default.
|
||
let workspace_only_file_system_policy = FileSystemSandboxPolicy::workspace_write(
|
||
&[],
|
||
/*exclude_tmpdir_env_var*/ true,
|
||
/*exclude_slash_tmp*/ true,
|
||
);
|
||
|
||
assert!(is_write_patch_constrained_to_writable_paths(
|
||
&add_inside,
|
||
&workspace_only_file_system_policy,
|
||
&cwd,
|
||
));
|
||
|
||
assert!(!is_write_patch_constrained_to_writable_paths(
|
||
&add_outside,
|
||
&workspace_only_file_system_policy,
|
||
&cwd,
|
||
));
|
||
|
||
// With the parent dir explicitly added as a writable root, the
|
||
// outside write should be permitted.
|
||
let file_system_policy_with_parent = FileSystemSandboxPolicy::workspace_write(
|
||
std::slice::from_ref(&parent),
|
||
/*exclude_tmpdir_env_var*/ true,
|
||
/*exclude_slash_tmp*/ true,
|
||
);
|
||
assert!(is_write_patch_constrained_to_writable_paths(
|
||
&add_outside,
|
||
&file_system_policy_with_parent,
|
||
&cwd,
|
||
));
|
||
}
|
||
|
||
#[test]
|
||
fn external_sandbox_auto_approves_in_on_request() {
|
||
let tmp = TempDir::new().unwrap();
|
||
let cwd = tmp.path().abs();
|
||
let add_inside_path = cwd.join("inner.txt");
|
||
let add_inside = ApplyPatchAction::new_add_for_test(&add_inside_path, "".to_string());
|
||
|
||
let permission_profile = PermissionProfile::External {
|
||
network: NetworkSandboxPolicy::Enabled,
|
||
};
|
||
let file_system_sandbox_policy = FileSystemSandboxPolicy::external_sandbox();
|
||
|
||
assert_eq!(
|
||
assess_patch_safety(
|
||
&add_inside,
|
||
AskForApproval::OnRequest,
|
||
&permission_profile,
|
||
&file_system_sandbox_policy,
|
||
&cwd,
|
||
WindowsSandboxLevel::Disabled
|
||
),
|
||
SafetyCheck::AutoApprove {
|
||
sandbox_type: SandboxType::None,
|
||
user_explicitly_approved: false,
|
||
}
|
||
);
|
||
}
|
||
|
||
#[test]
|
||
fn granular_with_all_flags_true_matches_on_request_for_out_of_root_patch() {
|
||
let tmp = TempDir::new().unwrap();
|
||
let cwd = tmp.path().abs();
|
||
let parent = cwd.parent().unwrap();
|
||
let outside_path = parent.join("outside.txt");
|
||
let add_outside = ApplyPatchAction::new_add_for_test(&outside_path, "".to_string());
|
||
let permission_profile = PermissionProfile::workspace_write_with(
|
||
&[],
|
||
NetworkSandboxPolicy::Restricted,
|
||
/*exclude_tmpdir_env_var*/ true,
|
||
/*exclude_slash_tmp*/ true,
|
||
);
|
||
let file_system_sandbox_policy = permission_profile.file_system_sandbox_policy();
|
||
|
||
assert_eq!(
|
||
assess_patch_safety(
|
||
&add_outside,
|
||
AskForApproval::OnRequest,
|
||
&permission_profile,
|
||
&file_system_sandbox_policy,
|
||
&cwd,
|
||
WindowsSandboxLevel::Disabled,
|
||
),
|
||
SafetyCheck::AskUser,
|
||
);
|
||
assert_eq!(
|
||
assess_patch_safety(
|
||
&add_outside,
|
||
AskForApproval::Granular(GranularApprovalConfig {
|
||
sandbox_approval: true,
|
||
rules: true,
|
||
skill_approval: true,
|
||
request_permissions: true,
|
||
mcp_elicitations: true,
|
||
}),
|
||
&permission_profile,
|
||
&file_system_sandbox_policy,
|
||
&cwd,
|
||
WindowsSandboxLevel::Disabled,
|
||
),
|
||
SafetyCheck::AskUser,
|
||
);
|
||
}
|
||
|
||
#[test]
|
||
fn granular_sandbox_approval_false_rejects_out_of_root_patch() {
|
||
let tmp = TempDir::new().unwrap();
|
||
let cwd = tmp.path().abs();
|
||
let parent = cwd.parent().unwrap();
|
||
let outside_path = parent.join("outside.txt");
|
||
let add_outside = ApplyPatchAction::new_add_for_test(&outside_path, "".to_string());
|
||
let permission_profile = PermissionProfile::workspace_write_with(
|
||
&[],
|
||
NetworkSandboxPolicy::Restricted,
|
||
/*exclude_tmpdir_env_var*/ true,
|
||
/*exclude_slash_tmp*/ true,
|
||
);
|
||
let file_system_sandbox_policy = permission_profile.file_system_sandbox_policy();
|
||
|
||
assert_eq!(
|
||
assess_patch_safety(
|
||
&add_outside,
|
||
AskForApproval::Granular(GranularApprovalConfig {
|
||
sandbox_approval: false,
|
||
rules: true,
|
||
skill_approval: true,
|
||
request_permissions: true,
|
||
mcp_elicitations: true,
|
||
}),
|
||
&permission_profile,
|
||
&file_system_sandbox_policy,
|
||
&cwd,
|
||
WindowsSandboxLevel::Disabled,
|
||
),
|
||
SafetyCheck::Reject {
|
||
reason: PATCH_REJECTED_OUTSIDE_PROJECT_REASON.to_string(),
|
||
},
|
||
);
|
||
}
|
||
|
||
#[test]
|
||
fn read_only_policy_rejects_patch_with_read_only_reason() {
|
||
let tmp = TempDir::new().unwrap();
|
||
let cwd = tmp.path().abs();
|
||
let inside_path = cwd.join("inside.txt");
|
||
let action = ApplyPatchAction::new_add_for_test(&inside_path, "".to_string());
|
||
let permission_profile = PermissionProfile::read_only();
|
||
let file_system_sandbox_policy = permission_profile.file_system_sandbox_policy();
|
||
|
||
assert!(!is_write_patch_constrained_to_writable_paths(
|
||
&action,
|
||
&file_system_sandbox_policy,
|
||
&cwd,
|
||
));
|
||
assert_eq!(
|
||
assess_patch_safety(
|
||
&action,
|
||
AskForApproval::Never,
|
||
&permission_profile,
|
||
&file_system_sandbox_policy,
|
||
&cwd,
|
||
WindowsSandboxLevel::Disabled,
|
||
),
|
||
SafetyCheck::Reject {
|
||
reason: PATCH_REJECTED_READ_ONLY_REASON.to_string(),
|
||
},
|
||
);
|
||
}
|
||
#[test]
|
||
fn explicit_unreadable_paths_prevent_auto_approval_for_external_sandbox() {
|
||
let tmp = TempDir::new().unwrap();
|
||
let cwd = tmp.path().abs();
|
||
let blocked_path = cwd.join("blocked.txt");
|
||
let blocked_absolute = blocked_path;
|
||
let action = ApplyPatchAction::new_add_for_test(&blocked_absolute, "".to_string());
|
||
let permission_profile = PermissionProfile::External {
|
||
network: NetworkSandboxPolicy::Restricted,
|
||
};
|
||
let file_system_sandbox_policy = FileSystemSandboxPolicy::restricted(vec![
|
||
FileSystemSandboxEntry {
|
||
path: FileSystemPath::Special {
|
||
value: FileSystemSpecialPath::Root,
|
||
},
|
||
access: FileSystemAccessMode::Write,
|
||
},
|
||
FileSystemSandboxEntry {
|
||
path: FileSystemPath::Path {
|
||
path: blocked_absolute,
|
||
},
|
||
access: FileSystemAccessMode::None,
|
||
},
|
||
]);
|
||
|
||
assert!(!is_write_patch_constrained_to_writable_paths(
|
||
&action,
|
||
&file_system_sandbox_policy,
|
||
&cwd,
|
||
));
|
||
assert_eq!(
|
||
assess_patch_safety(
|
||
&action,
|
||
AskForApproval::OnRequest,
|
||
&permission_profile,
|
||
&file_system_sandbox_policy,
|
||
&cwd,
|
||
WindowsSandboxLevel::Disabled,
|
||
),
|
||
SafetyCheck::AskUser,
|
||
);
|
||
}
|
||
|
||
#[test]
|
||
fn explicit_read_only_subpaths_prevent_auto_approval_for_external_sandbox() {
|
||
let tmp = TempDir::new().unwrap();
|
||
let cwd = tmp.path().abs();
|
||
let blocked_path = cwd.join("docs").join("blocked.txt");
|
||
let blocked_absolute = blocked_path;
|
||
let docs_absolute = AbsolutePathBuf::resolve_path_against_base("docs", &cwd);
|
||
let action = ApplyPatchAction::new_add_for_test(&blocked_absolute, "".to_string());
|
||
let permission_profile = PermissionProfile::External {
|
||
network: NetworkSandboxPolicy::Restricted,
|
||
};
|
||
let file_system_sandbox_policy = FileSystemSandboxPolicy::restricted(vec![
|
||
FileSystemSandboxEntry {
|
||
path: FileSystemPath::Special {
|
||
value: FileSystemSpecialPath::project_roots(/*subpath*/ None),
|
||
},
|
||
access: FileSystemAccessMode::Write,
|
||
},
|
||
FileSystemSandboxEntry {
|
||
path: FileSystemPath::Path {
|
||
path: docs_absolute,
|
||
},
|
||
access: FileSystemAccessMode::Read,
|
||
},
|
||
]);
|
||
|
||
assert!(!is_write_patch_constrained_to_writable_paths(
|
||
&action,
|
||
&file_system_sandbox_policy,
|
||
&cwd,
|
||
));
|
||
assert_eq!(
|
||
assess_patch_safety(
|
||
&action,
|
||
AskForApproval::OnRequest,
|
||
&permission_profile,
|
||
&file_system_sandbox_policy,
|
||
&cwd,
|
||
WindowsSandboxLevel::Disabled,
|
||
),
|
||
SafetyCheck::AskUser,
|
||
);
|
||
}
|
||
|
||
#[test]
|
||
fn missing_project_dot_codex_config_requires_approval() {
|
||
let tmp = TempDir::new().unwrap();
|
||
let cwd = tmp.path().abs();
|
||
let config_path = cwd.join(".codex").join("config.toml");
|
||
let action = ApplyPatchAction::new_add_for_test(&config_path, "".to_string());
|
||
let permission_profile = PermissionProfile::workspace_write_with(
|
||
&[],
|
||
NetworkSandboxPolicy::Restricted,
|
||
/*exclude_tmpdir_env_var*/ true,
|
||
/*exclude_slash_tmp*/ true,
|
||
);
|
||
let mut file_system_sandbox_policy = permission_profile.file_system_sandbox_policy();
|
||
file_system_sandbox_policy
|
||
.entries
|
||
.push(FileSystemSandboxEntry {
|
||
path: FileSystemPath::Path {
|
||
path: cwd.join(".codex"),
|
||
},
|
||
access: FileSystemAccessMode::Read,
|
||
});
|
||
|
||
assert!(!is_write_patch_constrained_to_writable_paths(
|
||
&action,
|
||
&file_system_sandbox_policy,
|
||
&cwd,
|
||
));
|
||
assert_eq!(
|
||
assess_patch_safety(
|
||
&action,
|
||
AskForApproval::OnRequest,
|
||
&permission_profile,
|
||
&file_system_sandbox_policy,
|
||
&cwd,
|
||
WindowsSandboxLevel::Disabled,
|
||
),
|
||
SafetyCheck::AskUser,
|
||
);
|
||
}
|