clone/codex
1
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-06-02 19:31:59 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(core): harden workspace mutation approvals

Browse Source
This commit is contained in:
Felipe Coury
2026-05-30 20:22:43 -03:00
parent 91b30d8116
commit af663f18ff
2 changed files with 173 additions and 34 deletions
Download Patch File Download Diff File Expand all files Collapse all files

193
codex-rs/core/src/tools/handlers/workspace_mutation.rs
View File

@@ -10,18 +10,24 @@ use crate::tools::handlers::workspace_mutation_spec::create_add_workspace_root_t
use crate::tools::handlers::workspace_mutation_spec::create_set_working_directory_tool;
use crate::tools::registry::CoreToolRuntime;
use crate::tools::registry::ToolExecutor;
use codex_protocol::models::AdditionalPermissionProfile;
use codex_protocol::models::FileSystemPermissions;
use codex_protocol::permissions::FileSystemSandboxPolicy;
use codex_protocol::request_permissions::PermissionGrantScope;
use codex_protocol::request_permissions::RequestPermissionProfile;
use codex_protocol::request_permissions::RequestPermissionsArgs;
use codex_protocol::request_permissions::WorkspaceMutationApprovalRequest;
use codex_protocol::request_permissions::WorkspaceMutationOperation;
use codex_sandboxing::policy_transforms::intersect_permission_profiles;
use codex_tools::ToolName;
use codex_tools::ToolSpec;
use codex_utils_absolute_path::AbsolutePathBuf;
use serde::Deserialize;
use serde::Serialize;
use std::io;
use std::path::Path;
const MAX_MODEL_VISIBLE_WORKSPACE_ROOTS: usize = 16;
#[derive(Clone, Copy)]
enum WorkspaceMutation {
@@ -57,6 +63,8 @@ struct WorkspaceMutationResult {
changed: bool,
cwd: AbsolutePathBuf,
workspace_roots: Vec<AbsolutePathBuf>,
#[serde(skip_serializing_if = "is_zero")]
omitted_workspace_roots: usize,
}
#[derive(Serialize)]
@@ -65,6 +73,8 @@ struct WorkspaceMutationError {
message: String,
cwd: AbsolutePathBuf,
workspace_roots: Vec<AbsolutePathBuf>,
#[serde(skip_serializing_if = "is_zero")]
omitted_workspace_roots: usize,
}
#[async_trait::async_trait]
@@ -106,10 +116,20 @@ impl ToolExecutor<ToolInvocation> for WorkspaceMutationHandler {
let args: WorkspaceMutationArgs = parse_arguments(&arguments)?;
let current = turn.runtime_workspace.snapshot().await;
let requested = current.cwd.join(args.path);
let Some(environment) = turn.environments.primary() else {
return Err(FunctionCallError::RespondToModel(
"workspace mutation is unavailable without an execution environment".to_string(),
));
let environment = match turn.environments.turn_environments.as_slice() {
[environment] => environment,
[] => {
return Err(FunctionCallError::RespondToModel(
"workspace mutation is unavailable without an execution environment"
.to_string(),
));
}
_ => {
return Err(FunctionCallError::RespondToModel(
"workspace mutation is unavailable with multiple execution environments"
.to_string(),
));
}
};
let fs = environment.environment.get_filesystem();
let canonical = match fs.canonicalize(&requested, /*sandbox*/ None).await {
@@ -186,25 +206,17 @@ impl ToolExecutor<ToolInvocation> for WorkspaceMutationHandler {
current.workspace_roots,
);
}
let requested_permissions = if preview_policy
.can_write_path_with_cwd(canonical.as_path(), cwd.as_path())
&& !current_policy.can_write_path_with_cwd(canonical.as_path(), current.cwd.as_path())
{
Some(FileSystemPermissions::from_read_write_roots(
/*read*/ None,
Some(vec![canonical.clone()]),
))
} else if preview_policy.can_read_path_with_cwd(canonical.as_path(), cwd.as_path())
&& !current_policy.can_read_path_with_cwd(canonical.as_path(), current.cwd.as_path())
{
Some(FileSystemPermissions::from_read_write_roots(
Some(vec![canonical.clone()]),
/*write*/ None,
))
} else {
None
};
let requested_permissions = newly_accessible_roots(
&current_policy,
current.cwd.as_path(),
&preview_policy,
cwd.as_path(),
);
if let Some(file_system) = requested_permissions {
let requested_permissions = RequestPermissionProfile {
file_system: Some(file_system),
network: None,
};
let response = session
.request_workspace_permissions_for_cwd(
&turn,
@@ -220,10 +232,7 @@ impl ToolExecutor<ToolInvocation> for WorkspaceMutationHandler {
canonical.as_path().display()
),
}),
permissions: RequestPermissionProfile {
file_system: Some(file_system),
network: None,
},
permissions: requested_permissions.clone(),
},
current.cwd.clone(),
WorkspaceMutationApprovalRequest {
@@ -249,12 +258,17 @@ impl ToolExecutor<ToolInvocation> for WorkspaceMutationHandler {
current.workspace_roots,
);
};
if response.permissions.is_empty()
|| !matches!(response.scope, PermissionGrantScope::Session)
if !matches!(response.scope, PermissionGrantScope::Session)
|| !permissions_are_approved(
requested_permissions,
response.permissions,
current.cwd.as_path(),
)
{
return workspace_error(
"approval_denied",
"workspace mutation requires session-scoped approval".to_string(),
"workspace mutation requires session-scoped approval with the requested filesystem access"
.to_string(),
current.cwd,
current.workspace_roots,
);
@@ -295,11 +309,13 @@ fn workspace_success(
cwd: AbsolutePathBuf,
workspace_roots: Vec<AbsolutePathBuf>,
) -> Result<Box<dyn crate::tools::context::ToolOutput>, FunctionCallError> {
let (workspace_roots, omitted_workspace_roots) = bounded_workspace_roots(workspace_roots);
workspace_output(
WorkspaceMutationResult {
changed,
cwd,
workspace_roots,
omitted_workspace_roots,
},
/*success*/ true,
)
@@ -311,12 +327,14 @@ fn workspace_error(
cwd: AbsolutePathBuf,
workspace_roots: Vec<AbsolutePathBuf>,
) -> Result<Box<dyn crate::tools::context::ToolOutput>, FunctionCallError> {
let (workspace_roots, omitted_workspace_roots) = bounded_workspace_roots(workspace_roots);
workspace_output(
WorkspaceMutationError {
code,
message,
cwd,
workspace_roots,
omitted_workspace_roots,
},
/*success*/ false,
)
@@ -333,10 +351,71 @@ fn workspace_output(
})?;
Ok(boxed_tool_output(FunctionToolOutput::from_text(
content,
Some(success),
/*success*/ Some(success),
)))
}
fn newly_accessible_roots(
current_policy: &FileSystemSandboxPolicy,
current_cwd: &Path,
preview_policy: &FileSystemSandboxPolicy,
preview_cwd: &Path,
) -> Option<FileSystemPermissions> {
let write = preview_policy
.get_writable_roots_with_cwd(preview_cwd)
.into_iter()
.map(|root| root.root)
.filter(|root| !current_policy.can_write_path_with_cwd(root.as_path(), current_cwd))
.collect::<Vec<_>>();
let read = preview_policy
.get_readable_roots_with_cwd(preview_cwd)
.into_iter()
.filter(|root| !current_policy.can_read_path_with_cwd(root.as_path(), current_cwd))
.filter(|root| {
!write
.iter()
.any(|writable_root| root.as_path().starts_with(writable_root.as_path()))
})
.collect::<Vec<_>>();
if read.is_empty() && write.is_empty() {
None
} else {
Some(FileSystemPermissions::from_read_write_roots(
/*read*/ (!read.is_empty()).then_some(read),
/*write*/ (!write.is_empty()).then_some(write),
))
}
}
fn permissions_are_approved(
requested: RequestPermissionProfile,
granted: RequestPermissionProfile,
cwd: &Path,
) -> bool {
let requested: AdditionalPermissionProfile = requested.into();
let granted: AdditionalPermissionProfile = granted.into();
intersect_permission_profiles(requested.clone(), granted, cwd) == requested
}
fn bounded_workspace_roots(workspace_roots: Vec<AbsolutePathBuf>) -> (Vec<AbsolutePathBuf>, usize) {
let omitted_workspace_roots = workspace_roots
.len()
.saturating_sub(MAX_MODEL_VISIBLE_WORKSPACE_ROOTS);
(
workspace_roots
.into_iter()
.take(MAX_MODEL_VISIBLE_WORKSPACE_ROOTS)
.collect(),
omitted_workspace_roots,
)
}
// Serde passes `skip_serializing_if` predicates a reference.
#[allow(clippy::trivially_copy_pass_by_ref)]
fn is_zero(value: &usize) -> bool {
*value == 0
}
fn io_error_code(err: &io::Error) -> &'static str {
match err.kind() {
io::ErrorKind::NotFound => "path_not_found",
@@ -344,3 +423,57 @@ fn io_error_code(err: &io::Error) -> &'static str {
_ => "resolution_failed",
}
}
#[cfg(test)]
mod tests {
use super::*;
use codex_protocol::permissions::FileSystemAccessMode;
use codex_protocol::permissions::FileSystemPath;
use codex_protocol::permissions::FileSystemSandboxEntry;
use codex_protocol::permissions::FileSystemSpecialPath;
#[test]
fn newly_accessible_roots_include_materialized_workspace_subpaths() {
let temp_dir = tempfile::tempdir().expect("tempdir");
let workspace_root = AbsolutePathBuf::try_from(
std::fs::canonicalize(temp_dir.path()).expect("canonical tempdir"),
)
.expect("absolute tempdir");
let current_policy = FileSystemSandboxPolicy::restricted(Vec::new());
let preview_policy = FileSystemSandboxPolicy::restricted(vec![FileSystemSandboxEntry {
path: FileSystemPath::Special {
value: FileSystemSpecialPath::project_roots(Some(".codex".into())),
},
access: FileSystemAccessMode::Write,
}])
.materialize_project_roots_with_workspace_roots(std::slice::from_ref(&workspace_root));
assert_eq!(
newly_accessible_roots(
&current_policy,
workspace_root.as_path(),
&preview_policy,
workspace_root.as_path(),
),
Some(FileSystemPermissions::from_read_write_roots(
/*read*/ None,
/*write*/ Some(vec![workspace_root.join(".codex")]),
))
);
}
#[test]
fn bounded_workspace_roots_reports_omitted_count() {
let roots = (0..MAX_MODEL_VISIBLE_WORKSPACE_ROOTS + 2)
.map(|index| {
AbsolutePathBuf::from_absolute_path(format!("/root-{index}"))
.expect("absolute test root")
})
.collect();
let (visible_roots, omitted_workspace_roots) = bounded_workspace_roots(roots);
assert_eq!(visible_roots.len(), MAX_MODEL_VISIBLE_WORKSPACE_ROOTS);
assert_eq!(omitted_workspace_roots, 2);
}
}

14
codex-rs/core/tests/suite/tools.rs
View File

@@ -432,7 +432,7 @@ async fn add_workspace_root_under_existing_root_is_noop() -> Result<()> {
}
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn workspace_mutation_requires_session_scoped_approval_and_cancels_suffix() -> Result<()> {
async fn workspace_mutation_rejects_reduced_approval_and_cancels_suffix() -> Result<()> {
skip_if_no_network!(Ok(()));
let server = start_mock_server().await;
@@ -503,8 +503,14 @@ async fn workspace_mutation_requires_session_scoped_approval_and_cancels_suffix(
.submit(Op::RequestPermissionsResponse {
id: mutation_call_id.to_string(),
response: RequestPermissionsResponse {
permissions: expected_permissions,
scope: PermissionGrantScope::Turn,
permissions: RequestPermissionProfile {
file_system: Some(FileSystemPermissions::from_read_write_roots(
/*read*/ Some(vec![external_root.clone()]),
/*write*/ None,
)),
..Default::default()
},
scope: PermissionGrantScope::Session,
strict_auto_review: false,
},
})
@@ -518,7 +524,7 @@ async fn workspace_mutation_requires_session_scoped_approval_and_cancels_suffix(
serde_json::from_str::<Value>(&mutation_output)?,
json!({
"code": "approval_denied",
"message": "workspace mutation requires session-scoped approval",
"message": "workspace mutation requires session-scoped approval with the requested filesystem access",
"cwd": test.config.cwd,
"workspace_roots": test.config.workspace_roots,
})