Require fresh approval beneath denied permission paths (#39266)

## Why

A stored permission grant may allow access to a parent while explicitly denying
a child path. A later request for that child must not be treated as already
approved by the broader parent grant.

## What changed

- Compare materialized permission profiles without intersecting away denied or
  reopened paths before deciding that a request is preapproved.
- Execute preapproved commands with the stored grant itself so its denied paths
  remain enforced.
- Fail closed when permission profiles cannot be materialized.

## Testing

Added unit and integration coverage for turn and session grants across
`exec_command`, `shell_command`, and `apply_patch`, including approval-disabled
and `Never` approval modes.

GitOrigin-RevId: 5455880328a89c7958f859c7ce87805dff9704fb
This commit is contained in:
Dylan Hurd
2026-08-18 19:08:07 +00:00
committed by copyberry
parent 846a16852f
commit d68b85a097
5 changed files with 469 additions and 22 deletions

View File

@@ -35,7 +35,7 @@ mod view_image;
pub(crate) mod view_image_spec;
mod wait_for_environment;
use codex_sandboxing::policy_transforms::intersect_permission_profiles;
use codex_sandboxing::policy_transforms::materialize_additional_permissions;
use codex_sandboxing::policy_transforms::merge_permission_profiles;
use codex_sandboxing::policy_transforms::normalize_additional_permissions;
use codex_utils_absolute_path::AbsolutePathBuf;
@@ -291,12 +291,19 @@ pub(super) async fn apply_granted_turn_permissions(
additional_permissions.as_ref(),
granted_permissions.as_ref(),
);
let permissions_preapproved = match (effective_permissions.as_ref(), granted_permissions) {
(Some(effective_permissions), Some(granted_permissions)) => {
permissions_are_preapproved(effective_permissions, granted_permissions, cwd)
let preapproved_permissions = granted_permissions.as_ref().and_then(|granted| {
if additional_permissions.is_none() {
Some(granted.clone())
} else {
effective_permissions
.as_ref()
.and_then(|effective| preapproved_permission_profile(effective, granted, cwd))
}
_ => false,
};
});
let permissions_preapproved = preapproved_permissions.is_some();
// A preapproved command must execute with the stored authority, never an
// unchecked merge that could reopen one of the grant's denied paths.
let effective_permissions = preapproved_permissions.or(effective_permissions);
let sandbox_permissions =
if effective_permissions.is_some() && !sandbox_permissions.uses_additional_permissions() {
@@ -312,26 +319,45 @@ pub(super) async fn apply_granted_turn_permissions(
}
}
fn permissions_are_preapproved(
fn preapproved_permission_profile(
effective_permissions: &AdditionalPermissionProfile,
granted_permissions: AdditionalPermissionProfile,
granted_permissions: &AdditionalPermissionProfile,
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
) -> Option<AdditionalPermissionProfile> {
let (Ok(effective), Ok(granted)) = (
materialize_additional_permissions(effective_permissions.clone(), cwd),
materialize_additional_permissions(granted_permissions.clone(), cwd),
) else {
return None;
};
if effective.network != granted.network {
return None;
}
let unchanged = match (effective.file_system, granted.file_system) {
(Some(effective), Some(granted)) => {
effective.glob_scan_max_depth == granted.glob_scan_max_depth
&& effective.entries.len() == granted.entries.len()
&& effective
.entries
.iter()
.all(|entry| granted.entries.contains(entry))
}
(None, None) => true,
(Some(_), None) | (None, Some(_)) => false,
};
unchanged.then(|| granted_permissions.clone())
}
#[cfg(test)]
#[path = "permission_preapproval_tests.rs"]
mod permission_preapproval_tests;
#[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 super::preapproved_permission_profile;
use crate::sandboxing::SandboxPermissions;
use codex_protocol::models::AdditionalPermissionProfile;
use codex_protocol::models::FileSystemPermissions;
@@ -481,10 +507,9 @@ mod tests {
merge_permission_profiles(Some(&requested_permissions), Some(&stored_grant))
.expect("merged permissions");
assert!(permissions_are_preapproved(
&effective_permissions,
stored_grant,
cwd.path(),
));
assert_eq!(
preapproved_permission_profile(&effective_permissions, &stored_grant, cwd.path()),
Some(stored_grant)
);
}
}

View File

@@ -0,0 +1,82 @@
use super::preapproved_permission_profile;
use codex_protocol::models::AdditionalPermissionProfile;
use codex_protocol::models::FileSystemPermissions;
use codex_protocol::models::NetworkPermissions;
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;
use tempfile::tempdir;
fn file_system_permissions(entries: Vec<FileSystemSandboxEntry>) -> AdditionalPermissionProfile {
AdditionalPermissionProfile {
file_system: Some(FileSystemPermissions {
entries,
glob_scan_max_depth: None,
}),
..Default::default()
}
}
#[test]
fn preapproval_accepts_reordered_replay_of_one_accumulated_grant() {
let cwd = tempdir().expect("tempdir");
let root = AbsolutePathBuf::from_absolute_path(cwd.path()).expect("absolute cwd");
let write = FileSystemSandboxEntry::new(root.clone().into(), FileSystemAccessMode::Write);
let mut granted = file_system_permissions(vec![
FileSystemSandboxEntry::new(root.join("secrets").into(), FileSystemAccessMode::Deny),
write.clone(),
FileSystemSandboxEntry::new(root.join("readonly").into(), FileSystemAccessMode::Read),
]);
granted.network = Some(NetworkPermissions {
enabled: Some(true),
});
let replay = file_system_permissions(vec![write]);
let effective = merge_permission_profiles(Some(&replay), Some(&granted)).expect("permissions");
assert_eq!(
preapproved_permission_profile(&effective, &granted, cwd.path()),
Some(granted)
);
}
#[test]
fn preapproval_requires_fresh_read_and_write_beneath_a_deny() {
let cwd = tempdir().expect("tempdir");
let root = AbsolutePathBuf::from_absolute_path(cwd.path()).expect("absolute cwd");
let granted = file_system_permissions(vec![
FileSystemSandboxEntry::new(root.clone().into(), FileSystemAccessMode::Write),
FileSystemSandboxEntry::new(root.join("secrets").into(), FileSystemAccessMode::Deny),
]);
let preapproved = [FileSystemAccessMode::Read, FileSystemAccessMode::Write].map(|access| {
let requested = file_system_permissions(vec![FileSystemSandboxEntry::new(
root.join("secrets/token.txt").into(),
access,
)]);
let effective =
merge_permission_profiles(Some(&requested), Some(&granted)).expect("permissions");
preapproved_permission_profile(&effective, &granted, cwd.path())
});
assert_eq!(preapproved, [None, None]);
}
#[test]
fn preapproval_fails_closed_when_materialization_rejects_both_profiles() {
let cwd = tempdir().expect("tempdir");
let invalid = file_system_permissions(vec![FileSystemSandboxEntry::new(
FileSystemPath::GlobPattern {
pattern: "**/secrets".to_string(),
},
FileSystemAccessMode::Write,
)]);
assert_eq!(
preapproved_permission_profile(&invalid, &invalid, cwd.path()),
None
);
}

View File

@@ -13,6 +13,8 @@ use codex_protocol::config_types::Settings;
use codex_protocol::models::AdditionalPermissionProfile as PermissionProfile;
use codex_protocol::models::FileSystemPermissions;
use codex_protocol::models::PermissionProfile as CorePermissionProfile;
use codex_protocol::permissions::FileSystemAccessMode;
use codex_protocol::permissions::FileSystemSandboxEntry;
use codex_protocol::permissions::NetworkSandboxPolicy;
use codex_protocol::protocol::AskForApproval;
use codex_protocol::protocol::EventMsg;
@@ -27,6 +29,8 @@ use codex_protocol::request_permissions::RequestPermissionProfile;
use codex_protocol::request_permissions::RequestPermissionsResponse;
use codex_protocol::user_input::UserInput;
use codex_utils_absolute_path::AbsolutePathBuf;
use codex_utils_path_uri::PathUri;
use core_test_support::responses::ev_apply_patch_custom_tool_call;
use core_test_support::responses::ev_assistant_message;
use core_test_support::responses::ev_completed;
use core_test_support::responses::ev_function_call;
@@ -38,9 +42,12 @@ use core_test_support::responses::sse;
use core_test_support::responses::sse_response;
use core_test_support::responses::start_mock_server;
use core_test_support::skip_if_no_network;
use core_test_support::skip_if_remote;
use core_test_support::skip_if_sandbox;
use core_test_support::skip_if_target_windows;
use core_test_support::skip_if_wine_exec;
use core_test_support::test_codex::TestCodex;
use core_test_support::test_codex::TestCodexHarness;
use core_test_support::test_codex::local_selections;
use core_test_support::test_codex::test_codex;
use core_test_support::test_codex::turn_permission_fields;
@@ -2208,3 +2215,272 @@ async fn request_permissions_session_grants_carry_across_turns() -> Result<()> {
Ok(())
}
const SENTINEL_PATH: &str = "denied-child-permissions/secrets/nested/sentinel.txt";
const SENTINEL_CONTENT: &str = "untouched\n";
const PERMISSIONS_CALL_ID: &str = "denied-child-permissions-grant";
const WRITE_CALL_ID: &str = "denied-child-permissions-write";
#[derive(Clone, Copy, Debug)]
enum WriteTool {
ExecCommand,
ShellCommand,
ApplyPatch,
}
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
enum ApprovalMode {
Prompt,
Never,
InlineFeatureDisabled,
}
#[test_case(WriteTool::ExecCommand, PermissionGrantScope::Turn, ApprovalMode::Prompt; "exec_command_turn")]
#[test_case(WriteTool::ExecCommand, PermissionGrantScope::Session, ApprovalMode::Prompt; "exec_command_session")]
#[test_case(WriteTool::ShellCommand, PermissionGrantScope::Turn, ApprovalMode::Prompt; "shell_command_turn")]
#[test_case(WriteTool::ShellCommand, PermissionGrantScope::Session, ApprovalMode::Prompt; "shell_command_session")]
#[test_case(WriteTool::ApplyPatch, PermissionGrantScope::Turn, ApprovalMode::Prompt; "apply_patch_turn")]
#[test_case(WriteTool::ApplyPatch, PermissionGrantScope::Session, ApprovalMode::Prompt; "apply_patch_session")]
#[test_case(WriteTool::ExecCommand, PermissionGrantScope::Session, ApprovalMode::Never; "exec_command_session_never")]
#[test_case(WriteTool::ExecCommand, PermissionGrantScope::Session, ApprovalMode::InlineFeatureDisabled; "exec_command_inline_feature_disabled")]
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn denied_child_permissions_require_fresh_approval(
tool: WriteTool,
scope: PermissionGrantScope,
mode: ApprovalMode,
) -> Result<()> {
skip_if_no_network!(Ok(()));
skip_if_sandbox!(Ok(()));
skip_if_target_windows!(
Ok(()),
"this regression exercises POSIX split-policy enforcement; a disabled Windows sandbox can independently prompt for the command"
);
if matches!(tool, WriteTool::ShellCommand) {
skip_if_remote!(
Ok(()),
"the legacy shell_command tool is only registered for a single local environment"
);
}
let harness =
TestCodexHarness::with_auto_env_builder(test_codex().with_config(move |config| {
config.permissions.approval_policy = Constrained::allow_any(AskForApproval::OnRequest);
config.approvals_reviewer = ApprovalsReviewer::User;
config
.permissions
.set_permission_profile(CorePermissionProfile::read_only())
.expect("set permission profile");
config
.features
.enable(Feature::UnifiedExec)
.expect("enable unified exec");
let inline_permissions = if mode == ApprovalMode::InlineFeatureDisabled {
config.features.disable(Feature::ExecPermissionApprovals)
} else {
config.features.enable(Feature::ExecPermissionApprovals)
};
inline_permissions.expect("configure inline permissions");
config
.features
.enable(Feature::RequestPermissionsTool)
.expect("enable request_permissions");
}))
.await?;
harness.write_file(SENTINEL_PATH, SENTINEL_CONTENT).await?;
let test = harness.test();
let root = test
.fs()
.canonicalize(
&PathUri::from_abs_path(&harness.path_abs("denied-child-permissions")),
/*sandbox*/ None,
)
.await?
.to_abs_path()?;
let denied_root = root.join("secrets");
let target = denied_root.join("nested/sentinel.txt");
let requested_permissions = requested_directory_write_permissions(root.as_path());
let constrained_permissions = RequestPermissionProfile {
file_system: Some(FileSystemPermissions {
entries: vec![
FileSystemSandboxEntry::new(root.clone().into(), FileSystemAccessMode::Write),
FileSystemSandboxEntry::new(denied_root.clone().into(), FileSystemAccessMode::Deny),
],
glob_scan_max_depth: None,
}),
..Default::default()
};
let approved_response = RequestPermissionsResponse {
permissions: constrained_permissions.clone(),
scope,
strict_auto_review: false,
};
let fresh_permissions = requested_directory_write_permissions(target.as_path());
let command = format!("printf changed > {:?}", target.as_path());
let write_event = match tool {
WriteTool::ExecCommand => exec_command_event_with_request_permissions(
WRITE_CALL_ID,
&command,
&fresh_permissions,
)?,
WriteTool::ShellCommand => {
shell_event_with_request_permissions(WRITE_CALL_ID, &command, &fresh_permissions)?
}
WriteTool::ApplyPatch => ev_apply_patch_custom_tool_call(
WRITE_CALL_ID,
&format!(
"*** Begin Patch\n*** Update File: {}\n@@\n-untouched\n+changed\n*** End Patch\n",
target.display()
),
),
};
let response = |id, event| sse(vec![ev_response_created(id), event, ev_completed(id)]);
let mut response_sequence = vec![response(
"grant",
request_permissions_tool_event(
PERMISSIONS_CALL_ID,
"Allow writes except in secrets",
&requested_permissions,
)?,
)];
if scope == PermissionGrantScope::Session {
response_sequence.push(response(
"grant-complete",
ev_assistant_message("grant-message", "grant recorded"),
));
}
response_sequence.extend([
response("write", write_event),
response("complete", ev_assistant_message("done-message", "done")),
]);
let responses = mount_sse_sequence(harness.server(), response_sequence).await;
test.codex
.start_or_steer_turn(TurnInputRequest::user_input(vec![UserInput::Text {
text: "request constrained permissions, then try the denied child".into(),
text_elements: Vec::new(),
}]))
.await?;
assert_eq!(
expect_request_permissions_event(test, PERMISSIONS_CALL_ID).await,
requested_permissions
);
test.codex
.submit(Op::RequestPermissionsResponse {
id: PERMISSIONS_CALL_ID.to_string(),
response: approved_response.clone(),
})
.await?;
if scope == PermissionGrantScope::Session {
wait_for_completion(test).await;
let approval_policy = match mode {
ApprovalMode::Never => AskForApproval::Never,
ApprovalMode::Prompt | ApprovalMode::InlineFeatureDisabled => AskForApproval::OnRequest,
};
test.codex
.start_or_steer_turn(
TurnInputRequest::user_input(vec![UserInput::Text {
text: "try the denied child using the stored session grant".into(),
text_elements: Vec::new(),
}])
.with_thread_settings(ThreadSettingsOverrides {
approval_policy: Some(approval_policy),
..Default::default()
}),
)
.await?;
}
let expected_error = match mode {
ApprovalMode::Prompt => None,
ApprovalMode::Never => Some("approval policy is Never"),
ApprovalMode::InlineFeatureDisabled => Some("additional permissions are disabled"),
};
let event = wait_for_event(&test.codex, |event| {
matches!(
event,
EventMsg::ExecApprovalRequest(_)
| EventMsg::ApplyPatchApprovalRequest(_)
| EventMsg::TurnComplete(_)
) || (expected_error.is_some() && matches!(event, EventMsg::ExecCommandBegin(_)))
})
.await;
if let Some(expected_error) = expected_error {
assert!(
matches!(event, EventMsg::TurnComplete(_)),
"{mode:?} must reject before approval or execution: {event:?}"
);
let output = responses
.function_call_output_text(WRITE_CALL_ID)
.context("rejected exec output")?;
assert!(
output.contains(expected_error),
"unexpected rejection: {output}"
);
assert_eq!(
harness.read_file_text(SENTINEL_PATH).await?,
SENTINEL_CONTENT
);
return Ok(());
}
let (decision, reason) = match (tool, event) {
(
WriteTool::ExecCommand | WriteTool::ShellCommand,
EventMsg::ExecApprovalRequest(approval),
) => {
assert_eq!(approval.call_id, WRITE_CALL_ID);
let expected_permissions = PermissionProfile {
file_system: Some(FileSystemPermissions {
entries: vec![
FileSystemSandboxEntry::new(target.into(), FileSystemAccessMode::Write),
FileSystemSandboxEntry::new(root.into(), FileSystemAccessMode::Write),
FileSystemSandboxEntry::new(denied_root.into(), FileSystemAccessMode::Deny),
],
glob_scan_max_depth: None,
}),
..Default::default()
};
assert_eq!(approval.additional_permissions, Some(expected_permissions));
(
Op::ExecApproval {
id: approval.effective_approval_id(),
turn_id: None,
decision: ReviewDecision::denied("denied child is not approved"),
},
approval.reason,
)
}
(WriteTool::ApplyPatch, EventMsg::ApplyPatchApprovalRequest(approval)) => {
assert_eq!(approval.call_id, WRITE_CALL_ID);
(
Op::PatchApproval {
id: approval.call_id,
decision: ReviewDecision::denied("denied child is not approved"),
},
approval.reason,
)
}
(_, event) => panic!("expected fresh {tool:?} permission approval, got {event:?}"),
};
let content_before_denial = harness.read_file_text(SENTINEL_PATH).await?;
test.codex.submit(decision).await?;
wait_for_completion(test).await;
assert_eq!(
reason, None,
"the first attempt must require approval, not only a retry after sandbox denial"
);
assert_eq!(content_before_denial, SENTINEL_CONTENT);
assert_eq!(
harness.read_file_text(SENTINEL_PATH).await?,
SENTINEL_CONTENT
);
let recorded_grant: RequestPermissionsResponse = serde_json::from_str(
&responses
.function_call_output_text(PERMISSIONS_CALL_ID)
.context("request_permissions response")?,
)?;
assert_eq!(recorded_grant, approved_response);
Ok(())
}

View File

@@ -72,6 +72,21 @@ pub fn normalize_additional_permissions(
})
}
/// Resolves cwd-dependent permission entries without filtering their authority.
///
/// Unlike intersection, this preserves narrower grants beneath denied paths.
pub fn materialize_additional_permissions(
mut additional_permissions: AdditionalPermissionProfile,
cwd: &Path,
) -> Result<AdditionalPermissionProfile, String> {
if let Some(file_system) = additional_permissions.file_system.as_mut() {
for entry in &mut file_system.entries {
*entry = materialize_cwd_dependent_entry(entry, cwd);
}
}
normalize_additional_permissions(additional_permissions)
}
pub fn merge_permission_profiles(
base: Option<&AdditionalPermissionProfile>,
permissions: Option<&AdditionalPermissionProfile>,

View File

@@ -1,5 +1,6 @@
use super::effective_file_system_sandbox_policy;
use super::intersect_permission_profiles;
use super::materialize_additional_permissions;
use super::merge_file_system_policy_with_additional_permissions;
use super::normalize_additional_permissions;
use super::should_require_platform_sandbox;
@@ -230,6 +231,54 @@ fn normalize_additional_permissions_drops_empty_nested_profiles() {
assert_eq!(permissions, PermissionProfile::default());
}
#[test]
fn materialize_additional_permissions_preserves_authority_and_constraints() {
use FileSystemAccessMode::Deny;
use FileSystemAccessMode::Read;
use FileSystemAccessMode::Write;
let temp_dir = TempDir::new().expect("create temp dir");
let cwd = AbsolutePathBuf::from_absolute_path(
canonicalize(temp_dir.path()).expect("canonicalize temp dir"),
)
.expect("absolute temp dir");
let project_path = |subpath: &str| FileSystemPath::Special {
value: FileSystemSpecialPath::project_roots(Some(subpath.to_owned())),
};
let deny_glob = |pattern: String| {
FileSystemSandboxEntry::new(FileSystemPath::GlobPattern { pattern }, Deny)
};
let profile = |entries| PermissionProfile {
file_system: Some(FileSystemPermissions {
entries,
glob_scan_max_depth: std::num::NonZeroUsize::new(/*n*/ 3),
}),
..Default::default()
};
let reopened = FileSystemSandboxEntry::new(cwd.join("private/reopened").into(), Write);
let requested = profile(vec![
FileSystemSandboxEntry::new(project_path("."), Write),
FileSystemSandboxEntry::new(project_path("private"), Deny),
FileSystemSandboxEntry::skip_missing_path(project_path("readonly"), Read),
FileSystemSandboxEntry::new(project_path("private/reopened"), Write),
reopened.clone(),
deny_glob("**/*.env".to_owned()),
]);
let expected = profile(vec![
FileSystemSandboxEntry::new(cwd.clone().into(), Write),
FileSystemSandboxEntry::new(cwd.join("private").into(), Deny),
FileSystemSandboxEntry::skip_missing_path(cwd.join("readonly").into(), Read),
reopened,
deny_glob(cwd.join("**/*.env").to_string_lossy().into_owned()),
]);
assert_eq!(
materialize_additional_permissions(requested, cwd.as_path())
.expect("materialized permissions"),
expected
);
}
#[test]
fn intersect_permission_profiles_preserves_explicit_empty_requested_reads() {
let temp_dir = TempDir::new().expect("create temp dir");