mirror of
https://github.com/openai/codex.git
synced 2026-05-05 11:57:33 +00:00
fix: use AbsolutePathBuf for permission profile file roots (#12970)
## Why `PermissionProfile` should describe filesystem roots as absolute paths at the type level. Using `PathBuf` in `FileSystemPermissions` made the shared type too permissive and blurred together three different deserialization cases: - skill metadata in `agents/openai.yaml`, where relative paths should resolve against the skill directory - app-server API payloads, where callers should have to send absolute paths - local tool-call payloads for commands like `shell_command` and `exec_command`, where `additional_permissions.file_system` may legitimately be relative to the command `workdir` This change tightens the shared model without regressing the existing local command flow. ## What Changed - changed `protocol::models::FileSystemPermissions` and the app-server `AdditionalFileSystemPermissions` mirror to use `AbsolutePathBuf` - wrapped skill metadata deserialization in `AbsolutePathBufGuard`, so relative permission roots in `agents/openai.yaml` resolve against the containing skill directory - kept app-server/API deserialization strict, so relative `additionalPermissions.fileSystem.*` paths are rejected at the boundary - restored cwd/workdir-relative deserialization for local tool-call payloads by parsing `shell`, `shell_command`, and `exec_command` arguments under an `AbsolutePathBufGuard` rooted at the resolved command working directory - simplified runtime additional-permission normalization so it only canonicalizes and deduplicates absolute roots instead of trying to recover relative ones later - updated the app-server schema fixtures, `app-server/README.md`, and the affected transport/TUI tests to match the final behavior
This commit is contained in:
@@ -16,9 +16,12 @@ mod test_sync;
|
||||
pub(crate) mod unified_exec;
|
||||
mod view_image;
|
||||
|
||||
use codex_utils_absolute_path::AbsolutePathBufGuard;
|
||||
pub use plan::PLAN_TOOL;
|
||||
use serde::Deserialize;
|
||||
use serde_json::Value;
|
||||
use std::path::Path;
|
||||
use std::path::PathBuf;
|
||||
|
||||
use crate::function_tool::FunctionCallError;
|
||||
use crate::sandboxing::SandboxPermissions;
|
||||
@@ -56,6 +59,33 @@ where
|
||||
})
|
||||
}
|
||||
|
||||
fn parse_arguments_with_base_path<T>(
|
||||
arguments: &str,
|
||||
base_path: &Path,
|
||||
) -> Result<T, FunctionCallError>
|
||||
where
|
||||
T: for<'de> Deserialize<'de>,
|
||||
{
|
||||
let _guard = AbsolutePathBufGuard::new(base_path);
|
||||
parse_arguments(arguments)
|
||||
}
|
||||
|
||||
fn resolve_workdir_base_path(
|
||||
arguments: &str,
|
||||
default_cwd: &Path,
|
||||
) -> Result<PathBuf, FunctionCallError> {
|
||||
let arguments: Value = parse_arguments(arguments)?;
|
||||
Ok(arguments
|
||||
.get("workdir")
|
||||
.and_then(Value::as_str)
|
||||
.filter(|workdir| !workdir.is_empty())
|
||||
.map(PathBuf::from)
|
||||
.map_or_else(
|
||||
|| default_cwd.to_path_buf(),
|
||||
|workdir| crate::util::resolve_path(default_cwd, &workdir),
|
||||
))
|
||||
}
|
||||
|
||||
/// Validates feature/policy constraints for `with_additional_permissions` and
|
||||
/// returns normalized absolute paths. Errors if paths are invalid.
|
||||
pub(super) fn normalize_and_validate_additional_permissions(
|
||||
@@ -63,7 +93,7 @@ pub(super) fn normalize_and_validate_additional_permissions(
|
||||
approval_policy: AskForApproval,
|
||||
sandbox_permissions: SandboxPermissions,
|
||||
additional_permissions: Option<PermissionProfile>,
|
||||
cwd: &Path,
|
||||
_cwd: &Path,
|
||||
) -> Result<Option<PermissionProfile>, String> {
|
||||
let uses_additional_permissions = matches!(
|
||||
sandbox_permissions,
|
||||
@@ -91,7 +121,7 @@ pub(super) fn normalize_and_validate_additional_permissions(
|
||||
.to_string(),
|
||||
);
|
||||
};
|
||||
let normalized = normalize_additional_permissions(additional_permissions, cwd)?;
|
||||
let normalized = normalize_additional_permissions(additional_permissions)?;
|
||||
if normalized.is_empty() {
|
||||
return Err(
|
||||
"`additional_permissions` must include at least one path in `file_system.read` or `file_system.write`"
|
||||
|
||||
@@ -22,7 +22,8 @@ use crate::tools::events::ToolEmitter;
|
||||
use crate::tools::events::ToolEventCtx;
|
||||
use crate::tools::handlers::apply_patch::intercept_apply_patch;
|
||||
use crate::tools::handlers::normalize_and_validate_additional_permissions;
|
||||
use crate::tools::handlers::parse_arguments;
|
||||
use crate::tools::handlers::parse_arguments_with_base_path;
|
||||
use crate::tools::handlers::resolve_workdir_base_path;
|
||||
use crate::tools::orchestrator::ToolOrchestrator;
|
||||
use crate::tools::registry::ToolHandler;
|
||||
use crate::tools::registry::ToolKind;
|
||||
@@ -176,7 +177,9 @@ impl ToolHandler for ShellHandler {
|
||||
|
||||
match payload {
|
||||
ToolPayload::Function { arguments } => {
|
||||
let params: ShellToolCallParams = parse_arguments(&arguments)?;
|
||||
let cwd = resolve_workdir_base_path(&arguments, turn.cwd.as_path())?;
|
||||
let params: ShellToolCallParams =
|
||||
parse_arguments_with_base_path(&arguments, cwd.as_path())?;
|
||||
let prefix_rule = params.prefix_rule.clone();
|
||||
let exec_params =
|
||||
Self::to_exec_params(¶ms, turn.as_ref(), session.conversation_id);
|
||||
@@ -266,7 +269,9 @@ impl ToolHandler for ShellCommandHandler {
|
||||
)));
|
||||
};
|
||||
|
||||
let params: ShellCommandToolCallParams = parse_arguments(&arguments)?;
|
||||
let cwd = resolve_workdir_base_path(&arguments, turn.cwd.as_path())?;
|
||||
let params: ShellCommandToolCallParams =
|
||||
parse_arguments_with_base_path(&arguments, cwd.as_path())?;
|
||||
maybe_emit_implicit_skill_invocation(
|
||||
session.as_ref(),
|
||||
turn.as_ref(),
|
||||
|
||||
@@ -13,6 +13,8 @@ use crate::tools::context::ToolPayload;
|
||||
use crate::tools::handlers::apply_patch::intercept_apply_patch;
|
||||
use crate::tools::handlers::normalize_and_validate_additional_permissions;
|
||||
use crate::tools::handlers::parse_arguments;
|
||||
use crate::tools::handlers::parse_arguments_with_base_path;
|
||||
use crate::tools::handlers::resolve_workdir_base_path;
|
||||
use crate::tools::registry::ToolHandler;
|
||||
use crate::tools::registry::ToolKind;
|
||||
use crate::unified_exec::ExecCommandRequest;
|
||||
@@ -136,7 +138,9 @@ impl ToolHandler for UnifiedExecHandler {
|
||||
|
||||
let response = match tool_name.as_str() {
|
||||
"exec_command" => {
|
||||
let args: ExecCommandArgs = parse_arguments(&arguments)?;
|
||||
let cwd = resolve_workdir_base_path(&arguments, context.turn.cwd.as_path())?;
|
||||
let args: ExecCommandArgs =
|
||||
parse_arguments_with_base_path(&arguments, cwd.as_path())?;
|
||||
maybe_emit_implicit_skill_invocation(
|
||||
session.as_ref(),
|
||||
turn.as_ref(),
|
||||
@@ -183,7 +187,7 @@ impl ToolHandler for UnifiedExecHandler {
|
||||
let workdir = workdir.filter(|value| !value.is_empty());
|
||||
|
||||
let workdir = workdir.map(|dir| context.turn.resolve_path(Some(dir)));
|
||||
let cwd = workdir.clone().unwrap_or_else(|| context.turn.cwd.clone());
|
||||
let cwd = workdir.clone().unwrap_or(cwd);
|
||||
let normalized_additional_permissions =
|
||||
match normalize_and_validate_additional_permissions(
|
||||
request_permission_enabled,
|
||||
@@ -336,8 +340,15 @@ fn format_response(response: &UnifiedExecResponse) -> String {
|
||||
mod tests {
|
||||
use super::*;
|
||||
use crate::shell::default_user_shell;
|
||||
use crate::tools::handlers::parse_arguments_with_base_path;
|
||||
use crate::tools::handlers::resolve_workdir_base_path;
|
||||
use codex_protocol::models::FileSystemPermissions;
|
||||
use codex_protocol::models::PermissionProfile;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use pretty_assertions::assert_eq;
|
||||
use std::fs;
|
||||
use std::sync::Arc;
|
||||
use tempfile::tempdir;
|
||||
|
||||
#[test]
|
||||
fn test_get_command_uses_default_shell_when_unspecified() -> anyhow::Result<()> {
|
||||
@@ -420,4 +431,37 @@ mod tests {
|
||||
);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn exec_command_args_resolve_relative_additional_permissions_against_workdir()
|
||||
-> anyhow::Result<()> {
|
||||
let cwd = tempdir()?;
|
||||
let workdir = cwd.path().join("nested");
|
||||
fs::create_dir_all(&workdir)?;
|
||||
let expected_write = workdir.join("relative-write.txt");
|
||||
let json = r#"{
|
||||
"cmd": "echo hello",
|
||||
"workdir": "nested",
|
||||
"additional_permissions": {
|
||||
"file_system": {
|
||||
"write": ["./relative-write.txt"]
|
||||
}
|
||||
}
|
||||
}"#;
|
||||
|
||||
let base_path = resolve_workdir_base_path(json, cwd.path())?;
|
||||
let args: ExecCommandArgs = parse_arguments_with_base_path(json, base_path.as_path())?;
|
||||
|
||||
assert_eq!(
|
||||
args.additional_permissions,
|
||||
Some(PermissionProfile {
|
||||
file_system: Some(FileSystemPermissions {
|
||||
read: None,
|
||||
write: Some(vec![AbsolutePathBuf::try_from(expected_write)?]),
|
||||
}),
|
||||
..Default::default()
|
||||
})
|
||||
);
|
||||
Ok(())
|
||||
}
|
||||
}
|
||||
|
||||
@@ -36,7 +36,6 @@ use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use pretty_assertions::assert_eq;
|
||||
#[cfg(target_os = "macos")]
|
||||
use std::collections::HashMap;
|
||||
use std::path::PathBuf;
|
||||
use std::time::Duration;
|
||||
|
||||
#[test]
|
||||
@@ -152,7 +151,9 @@ fn shell_request_escalation_execution_is_explicit() {
|
||||
let requested_permissions = PermissionProfile {
|
||||
file_system: Some(FileSystemPermissions {
|
||||
read: None,
|
||||
write: Some(vec![PathBuf::from("./output")]),
|
||||
write: Some(vec![
|
||||
AbsolutePathBuf::from_absolute_path("/tmp/output").unwrap(),
|
||||
]),
|
||||
}),
|
||||
..Default::default()
|
||||
};
|
||||
|
||||
Reference in New Issue
Block a user