mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
3c76081876
## Why Filesystem permission profiles used `none` for deny-read entries, which is less direct than the action the entry actually represents. This change makes `deny` the canonical filesystem permission spelling while preserving compatibility for older configs that still send `none`. ## What changed - rename `FileSystemAccessMode::None` to `Deny` - serialize and generate schemas with `deny` as the canonical value - retain `none` only as a legacy input alias for temporary config compatibility - update filesystem glob diagnostics and regression coverage to use the canonical spelling - refresh config and app-server schema fixtures to match the new wire shape ## Validation - `cargo test -p codex-protocol` - `cargo test -p codex-app-server-protocol` - `cargo test -p codex-core config_toml_deserializes_permission_profiles --lib` - `cargo test -p codex-core read_write_glob_patterns_still_reject_non_subpath_globs --lib` Earlier in the session, a broad `cargo test -p codex-core` run reached unrelated pre-existing failures in timing/snapshot/git-info tests under this environment; the targeted surfaces touched by this PR passed cleanly.
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::Deny,
|
||
},
|
||
]);
|
||
|
||
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,
|
||
);
|
||
}
|