From 554fd38785d3a35ce881c7ee011675796a2473cc Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Thu, 26 Feb 2026 17:27:01 -0800 Subject: [PATCH] fix: use AbsolutePathBuf for permission profile file roots --- ...CommandExecutionRequestApprovalParams.json | 8 +- .../schema/json/EventMsg.json | 4 +- .../schema/json/ServerRequest.json | 8 +- .../codex_app_server_protocol.schemas.json | 8 +- .../typescript/FileSystemPermissions.ts | 3 +- .../v2/AdditionalFileSystemPermissions.ts | 3 +- .../src/protocol/common.rs | 7 +- .../app-server-protocol/src/protocol/v2.rs | 35 +++++++- codex-rs/app-server/README.md | 2 +- codex-rs/app-server/src/transport.rs | 9 +- codex-rs/core/src/exec.rs | 3 - codex-rs/core/src/sandboxing/mod.rs | 78 ++++------------- codex-rs/core/src/skills/loader.rs | 32 ++++--- codex-rs/core/src/skills/permissions.rs | 84 ++++--------------- codex-rs/core/src/tools/handlers/mod.rs | 4 +- .../runtimes/shell/unix_escalation_tests.rs | 5 +- .../core/tests/suite/request_permissions.rs | 24 ++++-- codex-rs/core/tests/suite/skill_approval.rs | 17 +++- codex-rs/protocol/src/models.rs | 6 +- .../tui/src/bottom_pane/approval_overlay.rs | 17 ++-- 20 files changed, 173 insertions(+), 184 deletions(-) diff --git a/codex-rs/app-server-protocol/schema/json/CommandExecutionRequestApprovalParams.json b/codex-rs/app-server-protocol/schema/json/CommandExecutionRequestApprovalParams.json index 4e2106bbbc..5a19a39281 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 c7bf908748..745979df4e 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 ff810643d4..9138440548 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 62850442fc..9fa85afdf2 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 9e03768df3..aedf84de80 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 fb45464f99..0ed8a1b12f 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 3af2976106..9a96aefc2e 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 f7c4eec7a7..0602dc0e60 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2.rs @@ -811,8 +811,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 { @@ -4142,6 +4142,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 20b2dcf2c7..d1254b9f7a 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 6ac3e0d466..6da7a23443 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, }, ), diff --git a/codex-rs/core/src/exec.rs b/codex-rs/core/src/exec.rs index 5a805a3b8b..01c559c1c5 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 da03f00a62..bbd73749e4 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 a1aa3c630e..83db7a9078 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 89c49709e3..38d02ee0b6 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 23dc979f2c..dc2d61177e 100644 --- a/codex-rs/core/src/tools/handlers/mod.rs +++ b/codex-rs/core/src/tools/handlers/mod.rs @@ -63,7 +63,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 +91,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/runtimes/shell/unix_escalation_tests.rs b/codex-rs/core/src/tools/runtimes/shell/unix_escalation_tests.rs index d2fd612670..519608a43b 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 43ed8941d1..b57865e280 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, @@ -187,7 +193,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() }; @@ -272,7 +278,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 +369,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 +460,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 +556,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 a51c69bcfa..66a80d6439 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,10 @@ use std::fs; use std::path::Path; use std::path::PathBuf; +fn absolute_path(path: &Path) -> AbsolutePathBuf { + AbsolutePathBuf::try_from(path).expect("absolute path") +} + 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 +335,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 +601,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 d74401c609..ee58d46e68 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 2ea3c10e9d..9e392f3edc 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)); @@ -851,8 +856,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 +889,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 +928,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() }),