diff --git a/codex-rs/app-server-protocol/schema/json/CommandExecutionRequestApprovalParams.json b/codex-rs/app-server-protocol/schema/json/CommandExecutionRequestApprovalParams.json index 4e2106bbb..5a19a3928 100644 --- a/codex-rs/app-server-protocol/schema/json/CommandExecutionRequestApprovalParams.json +++ b/codex-rs/app-server-protocol/schema/json/CommandExecutionRequestApprovalParams.json @@ -1,11 +1,15 @@ { "$schema": "http://json-schema.org/draft-07/schema#", "definitions": { + "AbsolutePathBuf": { + "description": "A path that is guaranteed to be absolute and normalized (though it is not guaranteed to be canonicalized or exist on the filesystem).\n\nIMPORTANT: When deserializing an `AbsolutePathBuf`, a base path must be set using [AbsolutePathBufGuard::new]. If no base path is set, the deserialization will fail unless the path being deserialized is already absolute.", + "type": "string" + }, "AdditionalFileSystemPermissions": { "properties": { "read": { "items": { - "type": "string" + "$ref": "#/definitions/AbsolutePathBuf" }, "type": [ "array", @@ -14,7 +18,7 @@ }, "write": { "items": { - "type": "string" + "$ref": "#/definitions/AbsolutePathBuf" }, "type": [ "array", diff --git a/codex-rs/app-server-protocol/schema/json/EventMsg.json b/codex-rs/app-server-protocol/schema/json/EventMsg.json index 9f442ece9..b3e431706 100644 --- a/codex-rs/app-server-protocol/schema/json/EventMsg.json +++ b/codex-rs/app-server-protocol/schema/json/EventMsg.json @@ -3411,7 +3411,7 @@ "properties": { "read": { "items": { - "type": "string" + "$ref": "#/definitions/AbsolutePathBuf" }, "type": [ "array", @@ -3420,7 +3420,7 @@ }, "write": { "items": { - "type": "string" + "$ref": "#/definitions/AbsolutePathBuf" }, "type": [ "array", diff --git a/codex-rs/app-server-protocol/schema/json/ServerRequest.json b/codex-rs/app-server-protocol/schema/json/ServerRequest.json index ff810643d..913844054 100644 --- a/codex-rs/app-server-protocol/schema/json/ServerRequest.json +++ b/codex-rs/app-server-protocol/schema/json/ServerRequest.json @@ -1,11 +1,15 @@ { "$schema": "http://json-schema.org/draft-07/schema#", "definitions": { + "AbsolutePathBuf": { + "description": "A path that is guaranteed to be absolute and normalized (though it is not guaranteed to be canonicalized or exist on the filesystem).\n\nIMPORTANT: When deserializing an `AbsolutePathBuf`, a base path must be set using [AbsolutePathBufGuard::new]. If no base path is set, the deserialization will fail unless the path being deserialized is already absolute.", + "type": "string" + }, "AdditionalFileSystemPermissions": { "properties": { "read": { "items": { - "type": "string" + "$ref": "#/definitions/AbsolutePathBuf" }, "type": [ "array", @@ -14,7 +18,7 @@ }, "write": { "items": { - "type": "string" + "$ref": "#/definitions/AbsolutePathBuf" }, "type": [ "array", diff --git a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json index a10734046..a0863ad61 100644 --- a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json +++ b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json @@ -5,7 +5,7 @@ "properties": { "read": { "items": { - "type": "string" + "$ref": "#/definitions/AbsolutePathBuf" }, "type": [ "array", @@ -14,7 +14,7 @@ }, "write": { "items": { - "type": "string" + "$ref": "#/definitions/AbsolutePathBuf" }, "type": [ "array", @@ -4722,7 +4722,7 @@ "properties": { "read": { "items": { - "type": "string" + "$ref": "#/definitions/AbsolutePathBuf" }, "type": [ "array", @@ -4731,7 +4731,7 @@ }, "write": { "items": { - "type": "string" + "$ref": "#/definitions/AbsolutePathBuf" }, "type": [ "array", diff --git a/codex-rs/app-server-protocol/schema/typescript/FileSystemPermissions.ts b/codex-rs/app-server-protocol/schema/typescript/FileSystemPermissions.ts index 9e03768df..aedf84de8 100644 --- a/codex-rs/app-server-protocol/schema/typescript/FileSystemPermissions.ts +++ b/codex-rs/app-server-protocol/schema/typescript/FileSystemPermissions.ts @@ -1,5 +1,6 @@ // GENERATED CODE! DO NOT MODIFY BY HAND! // This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually. +import type { AbsolutePathBuf } from "./AbsolutePathBuf"; -export type FileSystemPermissions = { read: Array | null, write: Array | null, }; +export type FileSystemPermissions = { read: Array | null, write: Array | null, }; diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/AdditionalFileSystemPermissions.ts b/codex-rs/app-server-protocol/schema/typescript/v2/AdditionalFileSystemPermissions.ts index fb45464f9..0ed8a1b12 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/AdditionalFileSystemPermissions.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/AdditionalFileSystemPermissions.ts @@ -1,5 +1,6 @@ // GENERATED CODE! DO NOT MODIFY BY HAND! // This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually. +import type { AbsolutePathBuf } from "../AbsolutePathBuf"; -export type AdditionalFileSystemPermissions = { read: Array | null, write: Array | null, }; +export type AdditionalFileSystemPermissions = { read: Array | null, write: Array | null, }; diff --git a/codex-rs/app-server-protocol/src/protocol/common.rs b/codex-rs/app-server-protocol/src/protocol/common.rs index 3af297610..9a96aefc2 100644 --- a/codex-rs/app-server-protocol/src/protocol/common.rs +++ b/codex-rs/app-server-protocol/src/protocol/common.rs @@ -893,10 +893,15 @@ mod tests { use codex_protocol::account::PlanType; use codex_protocol::parse_command::ParsedCommand; use codex_protocol::protocol::AskForApproval; + use codex_utils_absolute_path::AbsolutePathBuf; use pretty_assertions::assert_eq; use serde_json::json; use std::path::PathBuf; + fn absolute_path(path: &str) -> AbsolutePathBuf { + AbsolutePathBuf::from_absolute_path(path).expect("absolute path") + } + #[test] fn serialize_new_conversation() -> Result<()> { let request = ClientRequest::NewConversation { @@ -1533,7 +1538,7 @@ mod tests { additional_permissions: Some(v2::AdditionalPermissionProfile { network: None, file_system: Some(v2::AdditionalFileSystemPermissions { - read: Some(vec![std::path::PathBuf::from("/tmp/allowed")]), + read: Some(vec![absolute_path("/tmp/allowed")]), write: None, }), macos: None, diff --git a/codex-rs/app-server-protocol/src/protocol/v2.rs b/codex-rs/app-server-protocol/src/protocol/v2.rs index 32bc120f6..8052eb080 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2.rs @@ -812,8 +812,8 @@ impl From for NetworkApprovalContext { #[serde(rename_all = "camelCase")] #[ts(export_to = "v2/")] pub struct AdditionalFileSystemPermissions { - pub read: Option>, - pub write: Option>, + pub read: Option>, + pub write: Option>, } impl From for AdditionalFileSystemPermissions { @@ -4170,6 +4170,37 @@ mod tests { AbsolutePathBuf::from_absolute_path(path).expect("path must be absolute") } + #[test] + fn command_execution_request_approval_rejects_relative_additional_permission_paths() { + let err = serde_json::from_value::(json!({ + "threadId": "thr_123", + "turnId": "turn_123", + "itemId": "call_123", + "command": "cat file", + "cwd": "/tmp", + "commandActions": null, + "reason": null, + "networkApprovalContext": null, + "additionalPermissions": { + "network": null, + "fileSystem": { + "read": ["relative/path"], + "write": null + }, + "macos": null + }, + "proposedExecpolicyAmendment": null, + "proposedNetworkPolicyAmendments": null, + "availableDecisions": null + })) + .expect_err("relative additional permission paths should fail"); + assert!( + err.to_string() + .contains("AbsolutePathBuf deserialized without a base path"), + "unexpected error: {err}" + ); + } + #[test] fn sandbox_policy_round_trips_external_sandbox_network_access() { let v2_policy = SandboxPolicy::ExternalSandbox { diff --git a/codex-rs/app-server/README.md b/codex-rs/app-server/README.md index 60cf6d610..1e5c37d28 100644 --- a/codex-rs/app-server/README.md +++ b/codex-rs/app-server/README.md @@ -710,7 +710,7 @@ Certain actions (shell commands or modifying files) may require explicit user ap Order of messages: 1. `item/started` — shows the pending `commandExecution` item with `command`, `cwd`, and other fields so you can render the proposed action. -2. `item/commandExecution/requestApproval` (request) — carries the same `itemId`, `threadId`, `turnId`, optionally `approvalId` (for subcommand callbacks), and `reason`. For normal command approvals, it also includes `command`, `cwd`, and `commandActions` for friendly display. When `initialize.params.capabilities.experimentalApi = true`, it may also include experimental `additionalPermissions` describing requested per-command sandbox access. For network-only approvals, those command fields may be omitted and `networkApprovalContext` is provided instead. Optional persistence hints may also be included via `proposedExecpolicyAmendment` and `proposedNetworkPolicyAmendments`. Clients can prefer `availableDecisions` when present to render the exact set of choices the server wants to expose, while still falling back to the older heuristics if it is omitted. +2. `item/commandExecution/requestApproval` (request) — carries the same `itemId`, `threadId`, `turnId`, optionally `approvalId` (for subcommand callbacks), and `reason`. For normal command approvals, it also includes `command`, `cwd`, and `commandActions` for friendly display. When `initialize.params.capabilities.experimentalApi = true`, it may also include experimental `additionalPermissions` describing requested per-command sandbox access; any filesystem paths in that payload are absolute on the wire. For network-only approvals, those command fields may be omitted and `networkApprovalContext` is provided instead. Optional persistence hints may also be included via `proposedExecpolicyAmendment` and `proposedNetworkPolicyAmendments`. Clients can prefer `availableDecisions` when present to render the exact set of choices the server wants to expose, while still falling back to the older heuristics if it is omitted. 3. Client response — for example `{ "decision": "accept" }`, `{ "decision": "acceptForSession" }`, `{ "decision": { "acceptWithExecpolicyAmendment": { "execpolicy_amendment": [...] } } }`, `{ "decision": { "applyNetworkPolicyAmendment": { "network_policy_amendment": { "host": "example.com", "action": "allow" } } } }`, `{ "decision": "decline" }`, or `{ "decision": "cancel" }`. 4. `item/completed` — final `commandExecution` item with `status: "completed" | "failed" | "declined"` and execution output. Render this as the authoritative result. diff --git a/codex-rs/app-server/src/transport.rs b/codex-rs/app-server/src/transport.rs index 6ac3e0d46..2bae5b44e 100644 --- a/codex-rs/app-server/src/transport.rs +++ b/codex-rs/app-server/src/transport.rs @@ -671,12 +671,17 @@ pub(crate) async fn route_outgoing_envelope( mod tests { use super::*; use crate::error_code::OVERLOADED_ERROR_CODE; + use codex_utils_absolute_path::AbsolutePathBuf; use pretty_assertions::assert_eq; use serde_json::json; use std::path::PathBuf; use tokio::time::Duration; use tokio::time::timeout; + fn absolute_path(path: &str) -> AbsolutePathBuf { + AbsolutePathBuf::from_absolute_path(path).expect("absolute path") + } + #[test] fn app_server_transport_parses_stdio_listen_url() { let transport = AppServerTransport::from_listen_url(AppServerTransport::DEFAULT_LISTEN_URL) @@ -977,7 +982,7 @@ mod tests { network: None, file_system: Some( codex_app_server_protocol::AdditionalFileSystemPermissions { - read: Some(vec![PathBuf::from("/tmp/allowed")]), + read: Some(vec![absolute_path("/tmp/allowed")]), write: None, }, ), @@ -1039,7 +1044,7 @@ mod tests { network: None, file_system: Some( codex_app_server_protocol::AdditionalFileSystemPermissions { - read: Some(vec![PathBuf::from("/tmp/allowed")]), + read: Some(vec![absolute_path("/tmp/allowed")]), write: None, }, ), @@ -1060,12 +1065,13 @@ mod tests { .await .expect("request should be delivered to the connection"); let json = serde_json::to_value(message).expect("request should serialize"); + let allowed_path = absolute_path("/tmp/allowed").to_string_lossy().into_owned(); assert_eq!( json["params"]["additionalPermissions"], json!({ "network": null, "fileSystem": { - "read": ["/tmp/allowed"], + "read": [allowed_path], "write": null, }, "macos": null, diff --git a/codex-rs/core/src/exec.rs b/codex-rs/core/src/exec.rs index 5a805a3b8..01c559c1c 100644 --- a/codex-rs/core/src/exec.rs +++ b/codex-rs/core/src/exec.rs @@ -521,9 +521,6 @@ pub(crate) mod errors { SandboxTransformError::SeatbeltUnavailable => CodexErr::UnsupportedOperation( "seatbelt sandbox is only available on macOS".to_string(), ), - SandboxTransformError::InvalidAdditionalPermissionsPath(path) => { - CodexErr::InvalidRequest(format!("invalid additional_permissions path: {path}")) - } } } } diff --git a/codex-rs/core/src/sandboxing/mod.rs b/codex-rs/core/src/sandboxing/mod.rs index da03f00a6..bbd73749e 100644 --- a/codex-rs/core/src/sandboxing/mod.rs +++ b/codex-rs/core/src/sandboxing/mod.rs @@ -92,8 +92,6 @@ pub enum SandboxPreference { pub(crate) enum SandboxTransformError { #[error("missing codex-linux-sandbox executable path")] MissingLinuxSandboxExecutable, - #[error("invalid additional permissions path: {0}")] - InvalidAdditionalPermissionsPath(String), #[cfg(not(target_os = "macos"))] #[error("seatbelt sandbox is only available on macOS")] SeatbeltUnavailable, @@ -101,19 +99,16 @@ pub(crate) enum SandboxTransformError { pub(crate) fn normalize_additional_permissions( additional_permissions: PermissionProfile, - command_cwd: &Path, ) -> Result { let Some(file_system) = additional_permissions.file_system else { return Ok(PermissionProfile::default()); }; let read = file_system .read - .map(|paths| normalize_permission_paths(paths, command_cwd, "file_system.read")) - .transpose()?; + .map(|paths| normalize_permission_paths(paths, "file_system.read")); let write = file_system .write - .map(|paths| normalize_permission_paths(paths, command_cwd, "file_system.write")) - .transpose()?; + .map(|paths| normalize_permission_paths(paths, "file_system.write")); Ok(PermissionProfile { file_system: Some(FileSystemPermissions { read, write }), ..Default::default() @@ -121,48 +116,25 @@ pub(crate) fn normalize_additional_permissions( } fn normalize_permission_paths( - paths: Vec, - command_cwd: &Path, - permission_kind: &str, -) -> Result, String> { + paths: Vec, + _permission_kind: &str, +) -> Vec { let mut out = Vec::with_capacity(paths.len()); let mut seen = HashSet::new(); for path in paths { - if path.as_os_str().is_empty() { - return Err(format!("{permission_kind} contains an empty path")); - } - - let resolved = if path.is_absolute() { - AbsolutePathBuf::from_absolute_path(path.clone()).map_err(|err| { - format!( - "{permission_kind} path `{}` is invalid: {err}", - path.display() - ) - })? - } else { - AbsolutePathBuf::resolve_path_against_base(&path, command_cwd).map_err(|err| { - format!( - "{permission_kind} path `{}` cannot be resolved against cwd `{}`: {err}", - path.display(), - command_cwd.display() - ) - })? - }; - - let canonicalized = resolved + let canonicalized = path .as_path() .canonicalize() .ok() .and_then(|path| AbsolutePathBuf::from_absolute_path(path).ok()) - .unwrap_or(resolved); - let canonicalized = canonicalized.to_path_buf(); + .unwrap_or(path); if seen.insert(canonicalized.clone()) { out.push(canonicalized); } } - Ok(out) + out } fn dedup_absolute_paths(paths: Vec) -> Vec { @@ -178,37 +150,23 @@ fn dedup_absolute_paths(paths: Vec) -> Vec { fn additional_permission_roots( additional_permissions: &PermissionProfile, -) -> Result<(Vec, Vec), SandboxTransformError> { - let to_abs = |paths: &[PathBuf]| { - let mut out = Vec::with_capacity(paths.len()); - for path in paths { - let absolute = AbsolutePathBuf::from_absolute_path(path.clone()).map_err(|err| { - SandboxTransformError::InvalidAdditionalPermissionsPath(format!( - "`{}`: {err}", - path.display() - )) - })?; - out.push(absolute); - } - Ok(dedup_absolute_paths(out)) - }; - - Ok(( - to_abs( +) -> (Vec, Vec) { + ( + dedup_absolute_paths( additional_permissions .file_system .as_ref() - .and_then(|file_system| file_system.read.as_deref()) + .and_then(|file_system| file_system.read.clone()) .unwrap_or_default(), - )?, - to_abs( + ), + dedup_absolute_paths( additional_permissions .file_system .as_ref() - .and_then(|file_system| file_system.write.as_deref()) + .and_then(|file_system| file_system.write.clone()) .unwrap_or_default(), - )?, - )) + ), + ) } fn merge_read_only_access_with_additional_reads( @@ -239,7 +197,7 @@ fn sandbox_policy_with_additional_permissions( return Ok(sandbox_policy.clone()); } - let (extra_reads, extra_writes) = additional_permission_roots(additional_permissions)?; + let (extra_reads, extra_writes) = additional_permission_roots(additional_permissions); let policy = match sandbox_policy { SandboxPolicy::DangerFullAccess | SandboxPolicy::ExternalSandbox { .. } => { diff --git a/codex-rs/core/src/skills/loader.rs b/codex-rs/core/src/skills/loader.rs index a1aa3c630..83db7a907 100644 --- a/codex-rs/core/src/skills/loader.rs +++ b/codex-rs/core/src/skills/loader.rs @@ -17,6 +17,7 @@ use crate::skills::system::system_cache_root_dir; use codex_app_server_protocol::ConfigLayerSource; use codex_protocol::models::PermissionProfile; use codex_protocol::protocol::SkillScope; +use codex_utils_absolute_path::AbsolutePathBufGuard; use dirs::home_dir; use dunce::canonicalize as canonicalize_path; use serde::Deserialize; @@ -573,15 +574,18 @@ fn load_skill_metadata(skill_path: &Path) -> LoadedSkillMetadata { } }; - let parsed: SkillMetadataFile = match serde_yaml::from_str(&contents) { - Ok(parsed) => parsed, - Err(error) => { - tracing::warn!( - "ignoring {path}: invalid {label}: {error}", - path = metadata_path.display(), - label = SKILLS_METADATA_FILENAME - ); - return LoadedSkillMetadata::default(); + let parsed: SkillMetadataFile = { + let _guard = AbsolutePathBufGuard::new(skill_dir); + match serde_yaml::from_str(&contents) { + Ok(parsed) => parsed, + Err(error) => { + tracing::warn!( + "ignoring {path}: invalid {label}: {error}", + path = metadata_path.display(), + label = SKILLS_METADATA_FILENAME + ); + return LoadedSkillMetadata::default(); + } } }; @@ -1376,8 +1380,14 @@ permissions: Some(PermissionProfile { network: Some(true), file_system: Some(FileSystemPermissions { - read: Some(vec![PathBuf::from("./data")]), - write: Some(vec![PathBuf::from("./output")]), + read: Some(vec![ + AbsolutePathBuf::try_from(normalized(skill_dir.join("data").as_path())) + .expect("absolute data path"), + ]), + write: Some(vec![ + AbsolutePathBuf::try_from(normalized(skill_dir.join("output").as_path())) + .expect("absolute output path"), + ]), }), macos: None, }) diff --git a/codex-rs/core/src/skills/permissions.rs b/codex-rs/core/src/skills/permissions.rs index 89c49709e..38d02ee0b 100644 --- a/codex-rs/core/src/skills/permissions.rs +++ b/codex-rs/core/src/skills/permissions.rs @@ -1,7 +1,5 @@ use std::collections::HashSet; -use std::path::Component; use std::path::Path; -use std::path::PathBuf; #[cfg(target_os = "macos")] use codex_protocol::models::MacOsAutomationValue; @@ -11,7 +9,6 @@ use codex_protocol::models::MacOsPreferencesValue; use codex_protocol::models::MacOsSeatbeltProfileExtensions; use codex_protocol::models::PermissionProfile; use codex_utils_absolute_path::AbsolutePathBuf; -use dirs::home_dir; use dunce::canonicalize as canonicalize_path; use tracing::warn; @@ -23,7 +20,7 @@ use crate::protocol::ReadOnlyAccess; use crate::protocol::SandboxPolicy; pub(crate) fn compile_permission_profile( - skill_dir: &Path, + _skill_dir: &Path, permissions: Option, ) -> Option { let PermissionProfile { @@ -33,12 +30,10 @@ pub(crate) fn compile_permission_profile( } = permissions?; let file_system = file_system.unwrap_or_default(); let fs_read = normalize_permission_paths( - skill_dir, file_system.read.as_deref().unwrap_or_default(), "permissions.file_system.read", ); let fs_write = normalize_permission_paths( - skill_dir, file_system.write.as_deref().unwrap_or_default(), "permissions.file_system.write", ); @@ -83,16 +78,12 @@ pub(crate) fn compile_permission_profile( }) } -fn normalize_permission_paths( - skill_dir: &Path, - values: &[PathBuf], - field: &str, -) -> Vec { +fn normalize_permission_paths(values: &[AbsolutePathBuf], field: &str) -> Vec { let mut paths = Vec::new(); let mut seen = HashSet::new(); for value in values { - let Some(path) = normalize_permission_path(skill_dir, value, field) else { + let Some(path) = normalize_permission_path(value, field) else { continue; }; if seen.insert(path.clone()) { @@ -103,26 +94,8 @@ fn normalize_permission_paths( paths } -fn normalize_permission_path( - skill_dir: &Path, - value: &Path, - field: &str, -) -> Option { - let value = value.to_string_lossy(); - let trimmed = value.trim(); - if trimmed.is_empty() { - warn!("ignoring {field}: value is empty"); - return None; - } - - let expanded = expand_home(trimmed); - let absolute = if expanded.is_absolute() { - expanded - } else { - skill_dir.join(expanded) - }; - let normalized = normalize_lexically(&absolute); - let canonicalized = canonicalize_path(&normalized).unwrap_or(normalized); +fn normalize_permission_path(value: &AbsolutePathBuf, field: &str) -> Option { + let canonicalized = canonicalize_path(value.as_path()).unwrap_or_else(|_| value.to_path_buf()); match AbsolutePathBuf::from_absolute_path(&canonicalized) { Ok(path) => Some(path), Err(error) => { @@ -132,21 +105,6 @@ fn normalize_permission_path( } } -fn expand_home(path: &str) -> PathBuf { - if path == "~" { - if let Some(home) = home_dir() { - return home; - } - return PathBuf::from(path); - } - if let Some(rest) = path.strip_prefix("~/") - && let Some(home) = home_dir() - { - return home.join(rest); - } - PathBuf::from(path) -} - #[cfg(target_os = "macos")] fn build_macos_seatbelt_profile_extensions( permissions: &MacOsPermissions, @@ -233,22 +191,6 @@ fn build_macos_seatbelt_profile_extensions( None } -fn normalize_lexically(path: &Path) -> PathBuf { - let mut normalized = PathBuf::new(); - for component in path.components() { - match component { - Component::CurDir => {} - Component::ParentDir => { - normalized.pop(); - } - Component::RootDir | Component::Prefix(_) | Component::Normal(_) => { - normalized.push(component.as_os_str()); - } - } - } - normalized -} - #[cfg(test)] mod tests { use super::compile_permission_profile; @@ -269,7 +211,11 @@ mod tests { use codex_utils_absolute_path::AbsolutePathBuf; use pretty_assertions::assert_eq; use std::fs; - use std::path::PathBuf; + use std::path::Path; + + fn absolute_path(path: &Path) -> AbsolutePathBuf { + AbsolutePathBuf::try_from(path).expect("absolute path") + } #[test] fn compile_permission_profile_normalizes_paths() { @@ -285,11 +231,11 @@ mod tests { network: Some(true), file_system: Some(FileSystemPermissions { read: Some(vec![ - PathBuf::from("./data"), - PathBuf::from("./data"), - PathBuf::from("scripts/../data"), + absolute_path(&skill_dir.join("data")), + absolute_path(&skill_dir.join("data")), + absolute_path(&skill_dir.join("scripts/../data")), ]), - write: Some(vec![PathBuf::from("./output")]), + write: Some(vec![absolute_path(&skill_dir.join("output"))]), }), ..Default::default() }), @@ -389,7 +335,7 @@ mod tests { Some(PermissionProfile { network: Some(true), file_system: Some(FileSystemPermissions { - read: Some(vec![PathBuf::from("./data")]), + read: Some(vec![absolute_path(&skill_dir.join("data"))]), write: Some(Vec::new()), }), ..Default::default() diff --git a/codex-rs/core/src/tools/handlers/mod.rs b/codex-rs/core/src/tools/handlers/mod.rs index 23dc979f2..89e35fa22 100644 --- a/codex-rs/core/src/tools/handlers/mod.rs +++ b/codex-rs/core/src/tools/handlers/mod.rs @@ -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( + arguments: &str, + base_path: &Path, +) -> Result +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 { + 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, - cwd: &Path, + _cwd: &Path, ) -> Result, 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`" diff --git a/codex-rs/core/src/tools/handlers/shell.rs b/codex-rs/core/src/tools/handlers/shell.rs index 9a1ff0052..efc596ea3 100644 --- a/codex-rs/core/src/tools/handlers/shell.rs +++ b/codex-rs/core/src/tools/handlers/shell.rs @@ -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(), diff --git a/codex-rs/core/src/tools/handlers/unified_exec.rs b/codex-rs/core/src/tools/handlers/unified_exec.rs index 57a109dd4..a04c5883c 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec.rs @@ -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(()) + } } diff --git a/codex-rs/core/src/tools/runtimes/shell/unix_escalation_tests.rs b/codex-rs/core/src/tools/runtimes/shell/unix_escalation_tests.rs index d2fd61267..519608a43 100644 --- a/codex-rs/core/src/tools/runtimes/shell/unix_escalation_tests.rs +++ b/codex-rs/core/src/tools/runtimes/shell/unix_escalation_tests.rs @@ -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() }; diff --git a/codex-rs/core/tests/suite/request_permissions.rs b/codex-rs/core/tests/suite/request_permissions.rs index 43ed8941d..a6381de2d 100644 --- a/codex-rs/core/tests/suite/request_permissions.rs +++ b/codex-rs/core/tests/suite/request_permissions.rs @@ -13,6 +13,7 @@ use codex_protocol::protocol::Op; use codex_protocol::protocol::ReviewDecision; use codex_protocol::protocol::SandboxPolicy; use codex_protocol::user_input::UserInput; +use codex_utils_absolute_path::AbsolutePathBuf; use core_test_support::responses::ev_assistant_message; use core_test_support::responses::ev_completed; use core_test_support::responses::ev_function_call; @@ -31,6 +32,11 @@ use regex_lite::Regex; use serde_json::Value; use serde_json::json; use std::fs; +use std::path::Path; + +fn absolute_path(path: &Path) -> AbsolutePathBuf { + AbsolutePathBuf::try_from(path).expect("absolute path") +} struct CommandResult { exit_code: Option, @@ -91,6 +97,24 @@ fn shell_event_with_request_permissions( Ok(ev_function_call(call_id, "shell_command", &args_str)) } +#[cfg(target_os = "macos")] +fn shell_event_with_raw_request_permissions( + call_id: &str, + command: &str, + workdir: Option<&str>, + additional_permissions: Value, +) -> Result { + let args = json!({ + "command": command, + "workdir": workdir, + "timeout_ms": 1_000_u64, + "sandbox_permissions": SandboxPermissions::WithAdditionalPermissions, + "additional_permissions": additional_permissions, + }); + let args_str = serde_json::to_string(&args)?; + Ok(ev_function_call(call_id, "shell_command", &args_str)) +} + async fn submit_turn( test: &TestCodex, prompt: &str, @@ -187,7 +211,7 @@ async fn with_additional_permissions_requires_approval_under_on_request() -> Res let requested_permissions = PermissionProfile { file_system: Some(FileSystemPermissions { read: Some(vec![]), - write: Some(vec![requested_write.clone()]), + write: Some(vec![absolute_path(&requested_write)]), }), ..Default::default() }; @@ -241,6 +265,98 @@ async fn with_additional_permissions_requires_approval_under_on_request() -> Res Ok(()) } +#[tokio::test(flavor = "current_thread")] +#[cfg(target_os = "macos")] +async fn relative_additional_permissions_resolve_against_tool_workdir() -> Result<()> { + skip_if_no_network!(Ok(())); + skip_if_sandbox!(Ok(())); + + let server = start_mock_server().await; + let approval_policy = AskForApproval::OnRequest; + let sandbox_policy = SandboxPolicy::new_read_only_policy(); + let sandbox_policy_for_config = sandbox_policy.clone(); + + let mut builder = test_codex().with_config(move |config| { + config.permissions.approval_policy = Constrained::allow_any(approval_policy); + config.permissions.sandbox_policy = Constrained::allow_any(sandbox_policy_for_config); + config.features.enable(Feature::RequestPermissions); + }); + let test = builder.build(&server).await?; + + let nested_dir = test.workspace_path("nested"); + fs::create_dir_all(&nested_dir)?; + let requested_write = nested_dir.join("relative-write.txt"); + let _ = fs::remove_file(&requested_write); + + 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(&requested_write)]), + }), + ..Default::default() + }; + let event = shell_event_with_raw_request_permissions( + call_id, + command, + Some("nested"), + json!({ + "file_system": { + "write": ["./relative-write.txt"], + }, + }), + )?; + + let _ = mount_sse_once( + &server, + sse(vec![ + ev_response_created("resp-relative-1"), + event, + ev_completed("resp-relative-1"), + ]), + ) + .await; + let results = mount_sse_once( + &server, + sse(vec![ + ev_assistant_message("msg-relative-1", "done"), + ev_completed("resp-relative-2"), + ]), + ) + .await; + + submit_turn(&test, call_id, approval_policy, sandbox_policy.clone()).await?; + + let approval = expect_exec_approval(&test, command).await; + assert_eq!( + approval.additional_permissions, + Some(expected_permissions.clone()) + ); + test.codex + .submit(Op::ExecApproval { + id: approval.effective_approval_id(), + turn_id: None, + decision: ReviewDecision::Approved, + }) + .await?; + wait_for_completion(&test).await; + + let result = parse_result(&results.single_request().function_call_output(call_id)); + assert!( + result.exit_code.is_none() || result.exit_code == Some(0), + "unexpected exit code/output: {:?} {}", + result.exit_code, + result.stdout + ); + assert!( + requested_write.exists(), + "touch command should create requested path" + ); + + Ok(()) +} + #[tokio::test(flavor = "current_thread")] #[cfg(target_os = "macos")] async fn read_only_with_additional_permissions_widens_to_unrequested_cwd_write() -> Result<()> { @@ -272,7 +388,7 @@ async fn read_only_with_additional_permissions_widens_to_unrequested_cwd_write() let requested_permissions = PermissionProfile { file_system: Some(FileSystemPermissions { read: Some(vec![]), - write: Some(vec![requested_write.clone()]), + write: Some(vec![absolute_path(&requested_write)]), }), ..Default::default() }; @@ -363,7 +479,7 @@ async fn read_only_with_additional_permissions_widens_to_unrequested_tmp_write() let requested_permissions = PermissionProfile { file_system: Some(FileSystemPermissions { read: Some(vec![]), - write: Some(vec![requested_write.clone()]), + write: Some(vec![absolute_path(&requested_write)]), }), ..Default::default() }; @@ -454,14 +570,16 @@ async fn workspace_write_with_additional_permissions_can_write_outside_cwd() -> let requested_permissions = PermissionProfile { file_system: Some(FileSystemPermissions { read: Some(vec![]), - write: Some(vec![outside_dir.path().to_path_buf()]), + write: Some(vec![absolute_path(outside_dir.path())]), }), ..Default::default() }; let normalized_requested_permissions = PermissionProfile { file_system: Some(FileSystemPermissions { read: Some(vec![]), - write: Some(vec![outside_dir.path().canonicalize()?]), + write: Some(vec![AbsolutePathBuf::try_from( + outside_dir.path().canonicalize()?, + )?]), }), ..Default::default() }; @@ -548,14 +666,16 @@ async fn with_additional_permissions_denied_approval_blocks_execution() -> Resul let requested_permissions = PermissionProfile { file_system: Some(FileSystemPermissions { read: Some(vec![]), - write: Some(vec![outside_dir.path().to_path_buf()]), + write: Some(vec![absolute_path(outside_dir.path())]), }), ..Default::default() }; let normalized_requested_permissions = PermissionProfile { file_system: Some(FileSystemPermissions { read: Some(vec![]), - write: Some(vec![outside_dir.path().canonicalize()?]), + write: Some(vec![AbsolutePathBuf::try_from( + outside_dir.path().canonicalize()?, + )?]), }), ..Default::default() }; diff --git a/codex-rs/core/tests/suite/skill_approval.rs b/codex-rs/core/tests/suite/skill_approval.rs index a51c69bcf..846152a9e 100644 --- a/codex-rs/core/tests/suite/skill_approval.rs +++ b/codex-rs/core/tests/suite/skill_approval.rs @@ -13,6 +13,7 @@ use codex_protocol::protocol::Op; use codex_protocol::protocol::ReviewDecision; use codex_protocol::protocol::SandboxPolicy; use codex_protocol::user_input::UserInput; +use codex_utils_absolute_path::AbsolutePathBuf; use core_test_support::responses::mount_function_call_agent_response; use core_test_support::responses::start_mock_server; use core_test_support::skip_if_no_network; @@ -26,6 +27,13 @@ use std::fs; use std::path::Path; use std::path::PathBuf; +fn absolute_path(path: &Path) -> AbsolutePathBuf { + match AbsolutePathBuf::try_from(path) { + Ok(path) => path, + Err(err) => panic!("absolute path: {err}"), + } +} + fn write_skill_metadata(home: &Path, name: &str, contents: &str) -> Result<()> { let metadata_dir = home.join("skills").join(name).join("agents"); fs::create_dir_all(&metadata_dir)?; @@ -330,8 +338,14 @@ permissions: approval.additional_permissions, Some(PermissionProfile { file_system: Some(FileSystemPermissions { - read: Some(vec![PathBuf::from("./data")]), - write: Some(vec![PathBuf::from("./output")]), + read: Some(vec![absolute_path( + &test.codex_home_path().join("skills/mbolin-test-skill/data"), + )]), + write: Some(vec![absolute_path( + &test + .codex_home_path() + .join("skills/mbolin-test-skill/output"), + )]), }), ..Default::default() }) @@ -590,7 +604,7 @@ async fn shell_zsh_fork_skill_session_approval_enforces_skill_permissions() -> R Some(PermissionProfile { file_system: Some(FileSystemPermissions { read: None, - write: Some(vec![allowed_dir.clone()]), + write: Some(vec![absolute_path(&allowed_dir)]), }), ..Default::default() }) diff --git a/codex-rs/protocol/src/models.rs b/codex-rs/protocol/src/models.rs index 14cb96828..cf06636ad 100644 --- a/codex-rs/protocol/src/models.rs +++ b/codex-rs/protocol/src/models.rs @@ -1,6 +1,5 @@ use std::collections::HashMap; use std::path::Path; -use std::path::PathBuf; use codex_utils_image::load_and_resize_to_fit; use serde::Deserialize; @@ -20,6 +19,7 @@ use crate::protocol::WritableRoot; use crate::user_input::UserInput; use codex_execpolicy::Policy; use codex_git::GhostCommit; +use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_image::error::ImageProcessingError; use schemars::JsonSchema; @@ -54,8 +54,8 @@ impl SandboxPermissions { #[derive(Debug, Clone, Default, Eq, Hash, PartialEq, Serialize, Deserialize, JsonSchema, TS)] pub struct FileSystemPermissions { - pub read: Option>, - pub write: Option>, + pub read: Option>, + pub write: Option>, } impl FileSystemPermissions { diff --git a/codex-rs/tui/src/bottom_pane/approval_overlay.rs b/codex-rs/tui/src/bottom_pane/approval_overlay.rs index 2ea3c10e9..b4a2de333 100644 --- a/codex-rs/tui/src/bottom_pane/approval_overlay.rs +++ b/codex-rs/tui/src/bottom_pane/approval_overlay.rs @@ -614,10 +614,15 @@ mod tests { use codex_protocol::protocol::ExecPolicyAmendment; use codex_protocol::protocol::NetworkApprovalProtocol; use codex_protocol::protocol::NetworkPolicyAmendment; + use codex_utils_absolute_path::AbsolutePathBuf; use insta::assert_snapshot; use pretty_assertions::assert_eq; use tokio::sync::mpsc::unbounded_channel; + fn absolute_path(path: &str) -> AbsolutePathBuf { + AbsolutePathBuf::from_absolute_path(path).expect("absolute path") + } + fn render_overlay_lines(view: &ApprovalOverlay, width: u16) -> String { let height = view.desired_height(width); let mut buf = Buffer::empty(Rect::new(0, 0, width, height)); @@ -634,6 +639,17 @@ mod tests { .join("\n") } + fn normalize_snapshot_paths(rendered: String) -> String { + [ + (absolute_path("/tmp/readme.txt"), "/tmp/readme.txt"), + (absolute_path("/tmp/out.txt"), "/tmp/out.txt"), + ] + .into_iter() + .fold(rendered, |rendered, (path, normalized)| { + rendered.replace(&path.display().to_string(), normalized) + }) + } + fn make_exec_request() -> ApprovalRequest { ApprovalRequest::Exec { id: "test".to_string(), @@ -851,8 +867,8 @@ mod tests { fn additional_permissions_exec_options_hide_execpolicy_amendment() { let additional_permissions = PermissionProfile { file_system: Some(FileSystemPermissions { - read: Some(vec![PathBuf::from("/tmp/readme.txt")]), - write: Some(vec![PathBuf::from("/tmp/out.txt")]), + read: Some(vec![absolute_path("/tmp/readme.txt")]), + write: Some(vec![absolute_path("/tmp/out.txt")]), }), ..Default::default() }; @@ -884,8 +900,8 @@ mod tests { network_approval_context: None, additional_permissions: Some(PermissionProfile { file_system: Some(FileSystemPermissions { - read: Some(vec![PathBuf::from("/tmp/readme.txt")]), - write: Some(vec![PathBuf::from("/tmp/out.txt")]), + read: Some(vec![absolute_path("/tmp/readme.txt")]), + write: Some(vec![absolute_path("/tmp/out.txt")]), }), ..Default::default() }), @@ -923,8 +939,8 @@ mod tests { network_approval_context: None, additional_permissions: Some(PermissionProfile { file_system: Some(FileSystemPermissions { - read: Some(vec![PathBuf::from("/tmp/readme.txt")]), - write: Some(vec![PathBuf::from("/tmp/out.txt")]), + read: Some(vec![absolute_path("/tmp/readme.txt")]), + write: Some(vec![absolute_path("/tmp/out.txt")]), }), ..Default::default() }), @@ -933,7 +949,7 @@ mod tests { let view = ApprovalOverlay::new(exec_request, tx, Features::with_defaults()); assert_snapshot!( "approval_overlay_additional_permissions_prompt", - render_overlay_lines(&view, 120) + normalize_snapshot_paths(render_overlay_lines(&view, 120)) ); }