mirror of
https://github.com/openai/codex.git
synced 2026-05-01 01:47:18 +00:00
protocol: canonicalize file system permissions (#18274)
## Why `PermissionProfile` needs stable, canonical file-system semantics before it can become the primary runtime permissions abstraction. Without a canonical form, callers have to keep re-deriving legacy sandbox maps and profile comparisons remain lossy or order-dependent. ## What changed This adds canonicalization helpers for `FileSystemPermissions` and `PermissionProfile`, expands special paths into explicit sandbox entries, and updates permission request/conversion paths to consume those canonical entries. It also tightens the legacy bridge so root-wide write profiles with narrower carveouts are not silently projected as full-disk legacy access. ## Verification - `cargo test -p codex-protocol root_write_with_read_only_child_is_not_full_disk_write -- --nocapture` - `cargo test -p codex-sandboxing permission -- --nocapture` - `cargo test -p codex-tui permissions -- --nocapture`
This commit is contained in:
@@ -293,20 +293,20 @@ fn workspace_write_excluding_tmp() -> SandboxPolicy {
|
||||
|
||||
fn requested_directory_write_permissions(path: &Path) -> RequestPermissionProfile {
|
||||
RequestPermissionProfile {
|
||||
file_system: Some(FileSystemPermissions {
|
||||
read: Some(vec![]),
|
||||
write: Some(vec![absolute_path(path)]),
|
||||
}),
|
||||
file_system: Some(FileSystemPermissions::from_read_write_roots(
|
||||
Some(vec![]),
|
||||
Some(vec![absolute_path(path)]),
|
||||
)),
|
||||
..RequestPermissionProfile::default()
|
||||
}
|
||||
}
|
||||
|
||||
fn normalized_directory_write_permissions(path: &Path) -> Result<RequestPermissionProfile> {
|
||||
Ok(RequestPermissionProfile {
|
||||
file_system: Some(FileSystemPermissions {
|
||||
read: Some(vec![]),
|
||||
write: Some(vec![AbsolutePathBuf::try_from(path.canonicalize()?)?]),
|
||||
}),
|
||||
file_system: Some(FileSystemPermissions::from_read_write_roots(
|
||||
Some(vec![]),
|
||||
Some(vec![AbsolutePathBuf::try_from(path.canonicalize()?)?]),
|
||||
)),
|
||||
..RequestPermissionProfile::default()
|
||||
})
|
||||
}
|
||||
@@ -343,10 +343,10 @@ async fn with_additional_permissions_requires_approval_under_on_request() -> Res
|
||||
let call_id = "request_permissions_skip_approval";
|
||||
let command = "touch requested-dir/requested-but-unused.txt";
|
||||
let requested_permissions = PermissionProfile {
|
||||
file_system: Some(FileSystemPermissions {
|
||||
read: Some(vec![]),
|
||||
write: Some(vec![absolute_path(&requested_dir_canonical)]),
|
||||
}),
|
||||
file_system: Some(FileSystemPermissions::from_read_write_roots(
|
||||
Some(vec![]),
|
||||
Some(vec![absolute_path(&requested_dir_canonical)]),
|
||||
)),
|
||||
..Default::default()
|
||||
};
|
||||
let event = shell_event_with_request_permissions(call_id, command, &requested_permissions)?;
|
||||
@@ -521,10 +521,10 @@ async fn relative_additional_permissions_resolve_against_tool_workdir() -> Resul
|
||||
let call_id = "request_permissions_relative_workdir";
|
||||
let command = "touch relative-write.txt";
|
||||
let expected_permissions = PermissionProfile {
|
||||
file_system: Some(FileSystemPermissions {
|
||||
read: None,
|
||||
write: Some(vec![absolute_path(&nested_dir_canonical)]),
|
||||
}),
|
||||
file_system: Some(FileSystemPermissions::from_read_write_roots(
|
||||
/*read*/ None,
|
||||
Some(vec![absolute_path(&nested_dir_canonical)]),
|
||||
)),
|
||||
..Default::default()
|
||||
};
|
||||
let event = shell_event_with_raw_request_permissions(
|
||||
@@ -624,10 +624,10 @@ async fn read_only_with_additional_permissions_does_not_widen_to_unrequested_cwd
|
||||
"cwd-widened", unrequested_write, unrequested_write
|
||||
);
|
||||
let requested_permissions = PermissionProfile {
|
||||
file_system: Some(FileSystemPermissions {
|
||||
read: Some(vec![]),
|
||||
write: Some(vec![absolute_path(&requested_write)]),
|
||||
}),
|
||||
file_system: Some(FileSystemPermissions::from_read_write_roots(
|
||||
Some(vec![]),
|
||||
Some(vec![absolute_path(&requested_write)]),
|
||||
)),
|
||||
..Default::default()
|
||||
};
|
||||
let event = shell_event_with_request_permissions(call_id, &command, &requested_permissions)?;
|
||||
@@ -725,10 +725,10 @@ async fn read_only_with_additional_permissions_does_not_widen_to_unrequested_tmp
|
||||
"tmp-widened", tmp_write, tmp_write
|
||||
);
|
||||
let requested_permissions = PermissionProfile {
|
||||
file_system: Some(FileSystemPermissions {
|
||||
read: Some(vec![]),
|
||||
write: Some(vec![absolute_path(&requested_write)]),
|
||||
}),
|
||||
file_system: Some(FileSystemPermissions::from_read_write_roots(
|
||||
Some(vec![]),
|
||||
Some(vec![absolute_path(&requested_write)]),
|
||||
)),
|
||||
..Default::default()
|
||||
};
|
||||
let event = shell_event_with_request_permissions(call_id, &command, &requested_permissions)?;
|
||||
@@ -824,19 +824,19 @@ async fn workspace_write_with_additional_permissions_can_write_outside_cwd() ->
|
||||
"outside-cwd-ok", outside_write, outside_write
|
||||
);
|
||||
let requested_permissions = RequestPermissionProfile {
|
||||
file_system: Some(FileSystemPermissions {
|
||||
read: Some(vec![]),
|
||||
write: Some(vec![absolute_path(outside_dir.path())]),
|
||||
}),
|
||||
file_system: Some(FileSystemPermissions::from_read_write_roots(
|
||||
Some(vec![]),
|
||||
Some(vec![absolute_path(outside_dir.path())]),
|
||||
)),
|
||||
..RequestPermissionProfile::default()
|
||||
};
|
||||
let normalized_requested_permissions = RequestPermissionProfile {
|
||||
file_system: Some(FileSystemPermissions {
|
||||
read: Some(vec![]),
|
||||
write: Some(vec![AbsolutePathBuf::try_from(
|
||||
file_system: Some(FileSystemPermissions::from_read_write_roots(
|
||||
Some(vec![]),
|
||||
Some(vec![AbsolutePathBuf::try_from(
|
||||
outside_dir.path().canonicalize()?,
|
||||
)?]),
|
||||
}),
|
||||
)),
|
||||
..RequestPermissionProfile::default()
|
||||
};
|
||||
let event = shell_event_with_request_permissions(call_id, &command, &requested_permissions)?;
|
||||
@@ -926,19 +926,19 @@ async fn with_additional_permissions_denied_approval_blocks_execution() -> Resul
|
||||
"should-not-write", outside_write, outside_write
|
||||
);
|
||||
let requested_permissions = PermissionProfile {
|
||||
file_system: Some(FileSystemPermissions {
|
||||
read: Some(vec![]),
|
||||
write: Some(vec![absolute_path(outside_dir.path())]),
|
||||
}),
|
||||
file_system: Some(FileSystemPermissions::from_read_write_roots(
|
||||
Some(vec![]),
|
||||
Some(vec![absolute_path(outside_dir.path())]),
|
||||
)),
|
||||
..Default::default()
|
||||
};
|
||||
let normalized_requested_permissions = PermissionProfile {
|
||||
file_system: Some(FileSystemPermissions {
|
||||
read: Some(vec![]),
|
||||
write: Some(vec![AbsolutePathBuf::try_from(
|
||||
file_system: Some(FileSystemPermissions::from_read_write_roots(
|
||||
Some(vec![]),
|
||||
Some(vec![AbsolutePathBuf::try_from(
|
||||
outside_dir.path().canonicalize()?,
|
||||
)?]),
|
||||
}),
|
||||
)),
|
||||
..Default::default()
|
||||
};
|
||||
let event = shell_event_with_request_permissions(call_id, &command, &requested_permissions)?;
|
||||
@@ -1028,19 +1028,19 @@ async fn request_permissions_grants_apply_to_later_exec_command_calls() -> Resul
|
||||
"sticky-grant-ok", outside_write, outside_write
|
||||
);
|
||||
let requested_permissions = RequestPermissionProfile {
|
||||
file_system: Some(FileSystemPermissions {
|
||||
read: Some(vec![]),
|
||||
write: Some(vec![absolute_path(outside_dir.path())]),
|
||||
}),
|
||||
file_system: Some(FileSystemPermissions::from_read_write_roots(
|
||||
Some(vec![]),
|
||||
Some(vec![absolute_path(outside_dir.path())]),
|
||||
)),
|
||||
..Default::default()
|
||||
};
|
||||
let normalized_requested_permissions = RequestPermissionProfile {
|
||||
file_system: Some(FileSystemPermissions {
|
||||
read: Some(vec![]),
|
||||
write: Some(vec![AbsolutePathBuf::try_from(
|
||||
file_system: Some(FileSystemPermissions::from_read_write_roots(
|
||||
Some(vec![]),
|
||||
Some(vec![AbsolutePathBuf::try_from(
|
||||
outside_dir.path().canonicalize()?,
|
||||
)?]),
|
||||
}),
|
||||
)),
|
||||
..Default::default()
|
||||
};
|
||||
let responses = mount_sse_sequence(
|
||||
@@ -1492,35 +1492,35 @@ async fn partial_request_permissions_grants_do_not_preapprove_new_permissions()
|
||||
);
|
||||
|
||||
let requested_permissions = RequestPermissionProfile {
|
||||
file_system: Some(FileSystemPermissions {
|
||||
read: Some(vec![]),
|
||||
write: Some(vec![
|
||||
file_system: Some(FileSystemPermissions::from_read_write_roots(
|
||||
Some(vec![]),
|
||||
Some(vec![
|
||||
absolute_path(first_dir.path()),
|
||||
absolute_path(second_dir.path()),
|
||||
]),
|
||||
}),
|
||||
)),
|
||||
..RequestPermissionProfile::default()
|
||||
};
|
||||
let normalized_requested_permissions = RequestPermissionProfile {
|
||||
file_system: Some(FileSystemPermissions {
|
||||
read: Some(vec![]),
|
||||
write: Some(vec![
|
||||
file_system: Some(FileSystemPermissions::from_read_write_roots(
|
||||
Some(vec![]),
|
||||
Some(vec![
|
||||
AbsolutePathBuf::try_from(first_dir.path().canonicalize()?)?,
|
||||
AbsolutePathBuf::try_from(second_dir.path().canonicalize()?)?,
|
||||
]),
|
||||
}),
|
||||
)),
|
||||
..RequestPermissionProfile::default()
|
||||
};
|
||||
let granted_permissions = normalized_directory_write_permissions(first_dir.path())?;
|
||||
let second_dir_permissions = requested_directory_write_permissions(second_dir.path());
|
||||
let merged_permissions = PermissionProfile {
|
||||
file_system: Some(FileSystemPermissions {
|
||||
read: Some(vec![]),
|
||||
write: Some(vec![
|
||||
file_system: Some(FileSystemPermissions::from_read_write_roots(
|
||||
Some(vec![]),
|
||||
Some(vec![
|
||||
AbsolutePathBuf::try_from(first_dir.path().canonicalize()?)?,
|
||||
AbsolutePathBuf::try_from(second_dir.path().canonicalize()?)?,
|
||||
]),
|
||||
}),
|
||||
)),
|
||||
..Default::default()
|
||||
};
|
||||
|
||||
@@ -1584,16 +1584,20 @@ async fn partial_request_permissions_grants_do_not_preapprove_new_permissions()
|
||||
let approval_file_system = approval_permissions
|
||||
.file_system
|
||||
.unwrap_or_else(|| panic!("expected filesystem permissions"));
|
||||
assert!(approval_file_system.read.as_ref().is_none_or(Vec::is_empty));
|
||||
let (approval_reads, approval_writes) = approval_file_system
|
||||
.legacy_read_write_roots()
|
||||
.unwrap_or_default();
|
||||
assert!(approval_reads.as_ref().is_none_or(Vec::is_empty));
|
||||
|
||||
let mut approval_writes = approval_file_system.write.unwrap_or_default();
|
||||
let mut approval_writes = approval_writes.unwrap_or_default();
|
||||
approval_writes.sort_by_key(|path| path.display().to_string());
|
||||
|
||||
let mut expected_writes = merged_permissions
|
||||
let (_expected_reads, expected_writes) = merged_permissions
|
||||
.file_system
|
||||
.unwrap_or_else(|| panic!("expected merged filesystem permissions"))
|
||||
.write
|
||||
.legacy_read_write_roots()
|
||||
.unwrap_or_default();
|
||||
let mut expected_writes = expected_writes.unwrap_or_default();
|
||||
expected_writes.sort_by_key(|path| path.display().to_string());
|
||||
|
||||
assert_eq!(approval_writes, expected_writes);
|
||||
|
||||
Reference in New Issue
Block a user