diff --git a/codex-rs/app-server-protocol/schema/json/ClientRequest.json b/codex-rs/app-server-protocol/schema/json/ClientRequest.json index fd9203ce1..dad64418d 100644 --- a/codex-rs/app-server-protocol/schema/json/ClientRequest.json +++ b/codex-rs/app-server-protocol/schema/json/ClientRequest.json @@ -1465,10 +1465,10 @@ "type": "object" }, "FsRemoveParams": { - "description": "Remove a file or directory tree from the host filesystem.", + "description": "Remove a file or directory tree from the host filesystem.\n\nThe target must resolve inside the session's authorized workspace scope; the server rejects paths (including symlink escapes) outside every workspace root.", "properties": { "force": { - "description": "Whether missing paths should be ignored. Defaults to `true`.", + "description": "Whether missing paths should be ignored. Defaults to `false`; ignoring missing targets must be requested explicitly.", "type": [ "boolean", "null" @@ -1480,10 +1480,10 @@ "$ref": "#/definitions/AbsolutePathBuf" } ], - "description": "Absolute path to remove." + "description": "Absolute path to remove. Must be inside an authorized workspace root." }, "recursive": { - "description": "Whether directory removal should recurse. Defaults to `true`.", + "description": "Whether directory removal should recurse. Defaults to `false`; recursive tree deletion must be requested explicitly.", "type": [ "boolean", "null" 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 3e83d4045..a582b9381 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 @@ -11376,10 +11376,10 @@ }, "FsRemoveParams": { "$schema": "http://json-schema.org/draft-07/schema#", - "description": "Remove a file or directory tree from the host filesystem.", + "description": "Remove a file or directory tree from the host filesystem.\n\nThe target must resolve inside the session's authorized workspace scope; the server rejects paths (including symlink escapes) outside every workspace root.", "properties": { "force": { - "description": "Whether missing paths should be ignored. Defaults to `true`.", + "description": "Whether missing paths should be ignored. Defaults to `false`; ignoring missing targets must be requested explicitly.", "type": [ "boolean", "null" @@ -11391,10 +11391,10 @@ "$ref": "#/definitions/v2/AbsolutePathBuf" } ], - "description": "Absolute path to remove." + "description": "Absolute path to remove. Must be inside an authorized workspace root." }, "recursive": { - "description": "Whether directory removal should recurse. Defaults to `true`.", + "description": "Whether directory removal should recurse. Defaults to `false`; recursive tree deletion must be requested explicitly.", "type": [ "boolean", "null" diff --git a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json index 6bb9270fb..0989bac0f 100644 --- a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json +++ b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json @@ -7746,10 +7746,10 @@ }, "FsRemoveParams": { "$schema": "http://json-schema.org/draft-07/schema#", - "description": "Remove a file or directory tree from the host filesystem.", + "description": "Remove a file or directory tree from the host filesystem.\n\nThe target must resolve inside the session's authorized workspace scope; the server rejects paths (including symlink escapes) outside every workspace root.", "properties": { "force": { - "description": "Whether missing paths should be ignored. Defaults to `true`.", + "description": "Whether missing paths should be ignored. Defaults to `false`; ignoring missing targets must be requested explicitly.", "type": [ "boolean", "null" @@ -7761,10 +7761,10 @@ "$ref": "#/definitions/AbsolutePathBuf" } ], - "description": "Absolute path to remove." + "description": "Absolute path to remove. Must be inside an authorized workspace root." }, "recursive": { - "description": "Whether directory removal should recurse. Defaults to `true`.", + "description": "Whether directory removal should recurse. Defaults to `false`; recursive tree deletion must be requested explicitly.", "type": [ "boolean", "null" diff --git a/codex-rs/app-server-protocol/schema/json/v2/FsRemoveParams.json b/codex-rs/app-server-protocol/schema/json/v2/FsRemoveParams.json index d6289d46d..eada7c299 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/FsRemoveParams.json +++ b/codex-rs/app-server-protocol/schema/json/v2/FsRemoveParams.json @@ -6,10 +6,10 @@ "type": "string" } }, - "description": "Remove a file or directory tree from the host filesystem.", + "description": "Remove a file or directory tree from the host filesystem.\n\nThe target must resolve inside the session's authorized workspace scope; the server rejects paths (including symlink escapes) outside every workspace root.", "properties": { "force": { - "description": "Whether missing paths should be ignored. Defaults to `true`.", + "description": "Whether missing paths should be ignored. Defaults to `false`; ignoring missing targets must be requested explicitly.", "type": [ "boolean", "null" @@ -21,10 +21,10 @@ "$ref": "#/definitions/AbsolutePathBuf" } ], - "description": "Absolute path to remove." + "description": "Absolute path to remove. Must be inside an authorized workspace root." }, "recursive": { - "description": "Whether directory removal should recurse. Defaults to `true`.", + "description": "Whether directory removal should recurse. Defaults to `false`; recursive tree deletion must be requested explicitly.", "type": [ "boolean", "null" diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/FsRemoveParams.ts b/codex-rs/app-server-protocol/schema/typescript/v2/FsRemoveParams.ts index c95b860ac..ffc677fff 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/FsRemoveParams.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/FsRemoveParams.ts @@ -5,17 +5,22 @@ import type { AbsolutePathBuf } from "../AbsolutePathBuf"; /** * Remove a file or directory tree from the host filesystem. + * + * The target must resolve inside the session's authorized workspace scope; the + * server rejects paths (including symlink escapes) outside every workspace root. */ export type FsRemoveParams = { /** - * Absolute path to remove. + * Absolute path to remove. Must be inside an authorized workspace root. */ path: AbsolutePathBuf, /** - * Whether directory removal should recurse. Defaults to `true`. + * Whether directory removal should recurse. Defaults to `false`; recursive + * tree deletion must be requested explicitly. */ recursive?: boolean | null, /** - * Whether missing paths should be ignored. Defaults to `true`. + * Whether missing paths should be ignored. Defaults to `false`; ignoring + * missing targets must be requested explicitly. */ force?: boolean | null, }; diff --git a/codex-rs/app-server-protocol/src/protocol/common.rs b/codex-rs/app-server-protocol/src/protocol/common.rs index 41b1869a1..84c7980c6 100644 --- a/codex-rs/app-server-protocol/src/protocol/common.rs +++ b/codex-rs/app-server-protocol/src/protocol/common.rs @@ -1200,6 +1200,12 @@ client_request_definitions! { }, // File system requests are intentionally concurrent. Desktop already treats local // file system operations as concurrent, and app-server remote fs mirrors that model. + // + // Although these methods accept absolute host paths, the app-server does not trust + // the client's path choice: every fs/* request is authorized server-side against the + // session's workspace scope (active cwd plus configured workspace roots) with symlink + // escapes rejected, before any filesystem access. See app-server/src/fs_scope.rs and + // the "Filesystem" section of app-server/README.md for the trust and scope semantics. FsReadFile => "fs/readFile" { params: v2::FsReadFileParams, serialization: None, diff --git a/codex-rs/app-server-protocol/src/protocol/v2/fs.rs b/codex-rs/app-server-protocol/src/protocol/v2/fs.rs index 0132c6b28..8e8ce3c5c 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2/fs.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2/fs.rs @@ -117,16 +117,21 @@ pub struct FsReadDirectoryResponse { } /// Remove a file or directory tree from the host filesystem. +/// +/// The target must resolve inside the session's authorized workspace scope; the +/// server rejects paths (including symlink escapes) outside every workspace root. #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, JsonSchema, TS)] #[serde(rename_all = "camelCase")] #[ts(export_to = "v2/")] pub struct FsRemoveParams { - /// Absolute path to remove. + /// Absolute path to remove. Must be inside an authorized workspace root. pub path: AbsolutePathBuf, - /// Whether directory removal should recurse. Defaults to `true`. + /// Whether directory removal should recurse. Defaults to `false`; recursive + /// tree deletion must be requested explicitly. #[ts(optional = nullable)] pub recursive: Option, - /// Whether missing paths should be ignored. Defaults to `true`. + /// Whether missing paths should be ignored. Defaults to `false`; ignoring + /// missing targets must be requested explicitly. #[ts(optional = nullable)] pub force: Option, } diff --git a/codex-rs/app-server/README.md b/codex-rs/app-server/README.md index a341bddd5..3223ca726 100644 --- a/codex-rs/app-server/README.md +++ b/codex-rs/app-server/README.md @@ -199,15 +199,17 @@ Example with notification opt-out: - `process/kill` — experimental; terminate a running `process/spawn` session by `processHandle`; returns `{}`. - `process/outputDelta` — experimental; notification emitted for base64-encoded stdout/stderr chunks from a streaming `process/spawn` session. - `process/exited` — experimental; notification emitted when a `process/spawn` session exits. -- `fs/readFile` — read an absolute file path and return `{ dataBase64 }`. -- `fs/writeFile` — write an absolute file path from base64-encoded `{ dataBase64 }`; returns `{}`. -- `fs/createDirectory` — create an absolute directory path; `recursive` defaults to `true`. -- `fs/getMetadata` — return metadata for an absolute path: `isDirectory`, `isFile`, `isSymlink`, `createdAtMs`, and `modifiedAtMs`. -- `fs/readDirectory` — list direct child entries for an absolute directory path; each entry contains `fileName`, `isDirectory`, and `isFile`, and `fileName` is just the child name, not a path. -- `fs/remove` — remove an absolute file or directory tree; `recursive` and `force` default to `true`. -- `fs/copy` — copy between absolute paths; directory copies require `recursive: true`. -- `fs/watch` — subscribe this connection to filesystem change notifications for an absolute file or directory path and caller-provided `watchId`; returns the canonicalized `path`. -- `fs/unwatch` — stop sending notifications for a prior `fs/watch`; returns `{}`. +The `fs/*` methods take absolute host paths, but the app-server does not trust the client's choice of path. Every `fs/*` request is authorized server-side against the session's workspace scope — the active working directory plus the configured workspace roots — before any filesystem access occurs. A path is rejected with `path is outside the authorized workspace roots` when its fully canonicalized location (with all symlinks resolved) falls outside every authorized root; this holds for reads, writes, directory creation, metadata, directory listing, copy (both source and destination), remove, and watch, and it cannot be bypassed by client-side UI validation, absolute-path traversal, or a symlink inside a root that resolves outside it. Write/mkdir/copy-destination targets that do not exist yet are authorized by resolving their deepest existing ancestor, so a symlinked ancestor cannot escape the scope either. When no workspace root is established the scope is default-deny. + +- `fs/readFile` — read an absolute file path inside the workspace scope and return `{ dataBase64 }`. +- `fs/writeFile` — write an absolute file path inside the workspace scope from base64-encoded `{ dataBase64 }`; returns `{}`. +- `fs/createDirectory` — create an absolute directory path inside the workspace scope; `recursive` defaults to `true`. +- `fs/getMetadata` — return metadata for an absolute path inside the workspace scope: `isDirectory`, `isFile`, `isSymlink`, `createdAtMs`, and `modifiedAtMs`. +- `fs/readDirectory` — list direct child entries for an absolute directory path inside the workspace scope; each entry contains `fileName`, `isDirectory`, and `isFile`, and `fileName` is just the child name, not a path. +- `fs/remove` — remove an absolute file or directory path inside the workspace scope. Destructive behavior is opt-in: `recursive` and `force` both default to `false`, so removing a non-empty directory requires `recursive: true` and removing a missing path requires `force: true`. Combined with the scope check, a client cannot recursively delete outside an authorized root. +- `fs/copy` — copy between absolute paths that are both inside the workspace scope; directory copies require `recursive: true`. +- `fs/watch` — subscribe this connection to filesystem change notifications for an absolute file or directory path inside the workspace scope and caller-provided `watchId`; returns the canonicalized `path`. Emitted `changedPaths` are always descendants of the authorized watch path. +- `fs/unwatch` — stop sending notifications for a prior `fs/watch`; returns `{}`. `fs/unwatch` is connection-scoped and takes no path. - `fs/changed` — notification emitted when watched paths change, including the `watchId` and `changedPaths`. - `model/list` — list available models (set `includeHidden: true` to include entries with `hidden: true`; set `modelProvider` to a configured provider id to browse that provider without changing the active thread; set `modelGateway` to scope through a gateway such as `hasna` or `openrouter`; set `upstreamProvider` to filter aggregator slugs such as OpenRouter models), with route metadata, reasoning effort options, `additionalSpeedTiers`, `serviceTiers`, optional `defaultServiceTier`, optional legacy `upgrade` model ids, optional `upgradeInfo` metadata (`model`, `upgradeCopy`, `modelLink`, `migrationMarkdown`), and optional `availabilityNux` metadata. - `modelGateway/list` — list model gateways. `hasna` is the default direct gateway over configured providers; `openrouter` is the built-in aggregator gateway for OpenRouter-backed model routes. diff --git a/codex-rs/app-server/src/fs_scope.rs b/codex-rs/app-server/src/fs_scope.rs new file mode 100644 index 000000000..38b2f17d8 --- /dev/null +++ b/codex-rs/app-server/src/fs_scope.rs @@ -0,0 +1,249 @@ +//! Server-side authorization scope for app-server `fs/*` filesystem RPCs. +//! +//! The `fs/*` methods accept absolute host paths chosen by the client. Without a +//! server-side scope check, a generic initialized client could read, write, +//! copy, watch, or delete arbitrary host paths outside the session's workspace +//! (CWE-862 Missing Authorization, CWE-22 Path Traversal, CWE-284 Improper +//! Access Control). [`FsScope`] pins those operations to an explicit set of +//! authorized workspace roots derived from server-held state (the session cwd +//! plus configured workspace roots) rather than trusting the request path. +//! +//! Every target is fully canonicalized before the containment check so symlink +//! escapes (a link inside a root whose real target is outside it) are rejected. +//! For write/mkdir/copy-destination targets that do not exist yet, the deepest +//! existing ancestor is canonicalized and the missing suffix re-attached, so a +//! symlinked ancestor cannot be used to escape the scope. + +use crate::error_code::internal_error; +use crate::error_code::invalid_request; +use codex_app_server_protocol::JSONRPCErrorError; +use codex_core::config::Config; +use codex_utils_absolute_path::AbsolutePathBuf; +use std::path::Path; +use std::path::PathBuf; +use std::sync::Arc; + +/// Authorized filesystem roots for app-server `fs/*` operations. +/// +/// Cloning is cheap; the canonicalized roots are shared behind an `Arc`. +#[derive(Clone)] +pub(crate) struct FsScope { + /// Canonicalized allowed roots. Empty means default-deny. + roots: Arc>, +} + +impl FsScope { + /// Builds a scope from an explicit set of authorized roots. Roots that can + /// be canonicalized are stored canonicalized so containment checks compare + /// resolved paths on both sides; roots that cannot be canonicalized (for + /// example a configured directory that does not exist yet) fall back to + /// their normalized logical path. + pub(crate) fn new(roots: impl IntoIterator) -> Self { + let mut canonical_roots: Vec = Vec::new(); + for root in roots { + let canonical = match root.canonicalize() { + Ok(canonical) => canonical.into_path_buf(), + Err(_) => root.into_path_buf(), + }; + if !canonical_roots.contains(&canonical) { + canonical_roots.push(canonical); + } + } + Self { + roots: Arc::new(canonical_roots), + } + } + + /// Derives the authorized roots from server-held session state: the active + /// working directory plus the effective workspace roots. This is the trust + /// boundary for `fs/*`; it never trusts a client-supplied root. + pub(crate) fn from_config(config: &Config) -> Self { + let mut roots = Vec::new(); + roots.push(config.cwd.clone()); + roots.extend(config.effective_workspace_roots()); + Self::new(roots) + } + + /// Authorizes a target path that is expected to exist (read, metadata, + /// readdir, remove, copy source, watch). Rejects paths whose canonical + /// location is outside every authorized root, including symlink escapes. + pub(crate) fn authorize_existing( + &self, + path: &AbsolutePathBuf, + ) -> Result<(), JSONRPCErrorError> { + self.authorize(path) + } + + /// Authorizes a target path that may not exist yet (write, mkdir, copy + /// destination). The deepest existing ancestor is canonicalized so a + /// symlinked ancestor cannot escape the scope. + pub(crate) fn authorize_target(&self, path: &AbsolutePathBuf) -> Result<(), JSONRPCErrorError> { + self.authorize(path) + } + + fn authorize(&self, path: &AbsolutePathBuf) -> Result<(), JSONRPCErrorError> { + if self.roots.is_empty() { + return Err(denied_error()); + } + let resolved = canonicalize_allowing_missing(path).map_err(|err| { + internal_error(format!( + "failed to resolve fs path for authorization: {err}" + )) + })?; + if self + .roots + .iter() + .any(|root| path_is_within(&resolved, root)) + { + Ok(()) + } else { + Err(denied_error()) + } + } +} + +fn denied_error() -> JSONRPCErrorError { + invalid_request("path is outside the authorized workspace roots") +} + +fn path_is_within(resolved: &Path, root: &Path) -> bool { + resolved == root || resolved.starts_with(root) +} + +/// Canonicalizes `path`, tolerating a non-existent tail. The deepest existing +/// ancestor is canonicalized (fully resolving symlinks) and the missing suffix +/// is re-attached. `AbsolutePathBuf` is already normalized, so the suffix +/// contains no `..`/`.` components that could climb back out of the resolved +/// ancestor. +fn canonicalize_allowing_missing(path: &AbsolutePathBuf) -> std::io::Result { + let mut suffix: Vec = Vec::new(); + let mut current = path.clone(); + loop { + match current.canonicalize() { + Ok(resolved) => { + let mut resolved = resolved.into_path_buf(); + for component in suffix.iter().rev() { + resolved.push(component); + } + return Ok(resolved); + } + Err(err) if err.kind() == std::io::ErrorKind::NotFound => { + let Some(file_name) = current.as_path().file_name() else { + return Err(err); + }; + suffix.push(file_name.to_os_string()); + let Some(parent) = current.parent() else { + return Err(err); + }; + current = parent; + } + Err(err) => return Err(err), + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use tempfile::TempDir; + + fn abs(path: impl AsRef) -> AbsolutePathBuf { + AbsolutePathBuf::from_absolute_path(path.as_ref()).expect("absolute path") + } + + #[test] + fn allows_paths_inside_root() { + let root = TempDir::new().expect("temp dir"); + let scope = FsScope::new([abs(root.path())]); + let nested = root.path().join("sub").join("file.txt"); + std::fs::create_dir_all(nested.parent().expect("parent")).expect("mkdir"); + std::fs::write(&nested, "hi").expect("write"); + + scope + .authorize_existing(&abs(&nested)) + .expect("existing path inside root should be allowed"); + } + + #[test] + fn allows_missing_target_inside_root() { + let root = TempDir::new().expect("temp dir"); + let scope = FsScope::new([abs(root.path())]); + let target = root.path().join("does").join("not").join("exist.txt"); + + scope + .authorize_target(&abs(&target)) + .expect("missing target inside root should be allowed"); + } + + #[test] + fn rejects_paths_outside_root() { + let root = TempDir::new().expect("temp dir"); + let outside = TempDir::new().expect("temp dir"); + let scope = FsScope::new([abs(root.path())]); + let target = outside.path().join("secret.txt"); + std::fs::write(&target, "secret").expect("write"); + + let err = scope + .authorize_existing(&abs(&target)) + .expect_err("path outside root should be denied"); + assert_eq!( + err.message, + "path is outside the authorized workspace roots" + ); + } + + #[test] + fn rejects_sibling_prefix_paths() { + // `/root/../root-evil` must not be considered inside `/root`. + let base = TempDir::new().expect("temp dir"); + let root = base.path().join("root"); + let sibling = base.path().join("root-evil"); + std::fs::create_dir_all(&root).expect("mkdir root"); + std::fs::create_dir_all(&sibling).expect("mkdir sibling"); + let scope = FsScope::new([abs(&root)]); + + let err = scope + .authorize_existing(&abs(sibling.join("file.txt"))) + .expect_err("sibling with shared prefix should be denied"); + assert_eq!( + err.message, + "path is outside the authorized workspace roots" + ); + } + + #[cfg(unix)] + #[test] + fn rejects_symlink_escape() { + use std::os::unix::fs::symlink; + + let root = TempDir::new().expect("temp dir"); + let outside = TempDir::new().expect("temp dir"); + let secret = outside.path().join("secret.txt"); + std::fs::write(&secret, "secret").expect("write secret"); + let link = root.path().join("escape"); + symlink(outside.path(), &link).expect("symlink"); + let scope = FsScope::new([abs(root.path())]); + + // The link lives inside the root but resolves outside of it. + let err = scope + .authorize_existing(&abs(link.join("secret.txt"))) + .expect_err("symlink escape should be denied"); + assert_eq!( + err.message, + "path is outside the authorized workspace roots" + ); + } + + #[test] + fn empty_scope_denies_everything() { + let scope = FsScope::new(std::iter::empty()); + let root = TempDir::new().expect("temp dir"); + let err = scope + .authorize_existing(&abs(root.path().join("file.txt"))) + .expect_err("empty scope should default-deny"); + assert_eq!( + err.message, + "path is outside the authorized workspace roots" + ); + } +} diff --git a/codex-rs/app-server/src/lib.rs b/codex-rs/app-server/src/lib.rs index 74f5c5c8d..535b2b170 100644 --- a/codex-rs/app-server/src/lib.rs +++ b/codex-rs/app-server/src/lib.rs @@ -88,6 +88,7 @@ mod dynamic_tools; mod error_code; mod extensions; mod filters; +mod fs_scope; mod fs_watch; mod fuzzy_file_search; pub mod in_process; diff --git a/codex-rs/app-server/src/message_processor.rs b/codex-rs/app-server/src/message_processor.rs index 6823bdbc5..4bc17fbea 100644 --- a/codex-rs/app-server/src/message_processor.rs +++ b/codex-rs/app-server/src/message_processor.rs @@ -584,6 +584,7 @@ impl MessageProcessor { let fs_processor = FsRequestProcessor::new( Arc::clone(&environment_manager_for_requests), FsWatchManager::new(outgoing.clone()), + crate::fs_scope::FsScope::from_config(config.as_ref()), ); let windows_sandbox_processor = WindowsSandboxRequestProcessor::new( outgoing.clone(), diff --git a/codex-rs/app-server/src/request_processors/fs_processor.rs b/codex-rs/app-server/src/request_processors/fs_processor.rs index 99a8620b4..413fc6b7f 100644 --- a/codex-rs/app-server/src/request_processors/fs_processor.rs +++ b/codex-rs/app-server/src/request_processors/fs_processor.rs @@ -1,5 +1,6 @@ use crate::error_code::internal_error; use crate::error_code::invalid_request; +use crate::fs_scope::FsScope; use crate::fs_watch::FsWatchManager; use crate::outgoing_message::ConnectionId; use base64::Engine; @@ -36,16 +37,19 @@ use std::sync::Arc; pub(crate) struct FsRequestProcessor { environment_manager: Arc, fs_watch_manager: FsWatchManager, + scope: FsScope, } impl FsRequestProcessor { pub(crate) fn new( environment_manager: Arc, fs_watch_manager: FsWatchManager, + scope: FsScope, ) -> Self { Self { environment_manager, fs_watch_manager, + scope, } } @@ -64,6 +68,7 @@ impl FsRequestProcessor { &self, params: FsReadFileParams, ) -> Result { + self.scope.authorize_existing(¶ms.path)?; let bytes = self .file_system()? .read_file(¶ms.path, /*sandbox*/ None) @@ -78,6 +83,7 @@ impl FsRequestProcessor { &self, params: FsWriteFileParams, ) -> Result { + self.scope.authorize_target(¶ms.path)?; let bytes = STANDARD.decode(params.data_base64).map_err(|err| { invalid_request(format!( "fs/writeFile requires valid base64 dataBase64: {err}" @@ -94,6 +100,7 @@ impl FsRequestProcessor { &self, params: FsCreateDirectoryParams, ) -> Result { + self.scope.authorize_target(¶ms.path)?; self.file_system()? .create_directory( ¶ms.path, @@ -111,6 +118,7 @@ impl FsRequestProcessor { &self, params: FsGetMetadataParams, ) -> Result { + self.scope.authorize_existing(¶ms.path)?; let metadata = self .file_system()? .get_metadata(¶ms.path, /*sandbox*/ None) @@ -129,6 +137,7 @@ impl FsRequestProcessor { &self, params: FsReadDirectoryParams, ) -> Result { + self.scope.authorize_existing(¶ms.path)?; let entries = self .file_system()? .read_directory(¶ms.path, /*sandbox*/ None) @@ -150,12 +159,17 @@ impl FsRequestProcessor { &self, params: FsRemoveParams, ) -> Result { + self.scope.authorize_existing(¶ms.path)?; + // Destructive defaults are opt-in: recursive tree deletion and ignoring + // missing targets both require the client to request them explicitly. + // Combined with the scope check above, a client cannot recursively + // delete outside an authorized root. self.file_system()? .remove( ¶ms.path, RemoveOptions { - recursive: params.recursive.unwrap_or(true), - force: params.force.unwrap_or(true), + recursive: params.recursive.unwrap_or(false), + force: params.force.unwrap_or(false), }, /*sandbox*/ None, ) @@ -168,6 +182,8 @@ impl FsRequestProcessor { &self, params: FsCopyParams, ) -> Result { + self.scope.authorize_existing(¶ms.source_path)?; + self.scope.authorize_target(¶ms.destination_path)?; self.file_system()? .copy( ¶ms.source_path, @@ -187,6 +203,7 @@ impl FsRequestProcessor { connection_id: ConnectionId, params: FsWatchParams, ) -> Result { + self.scope.authorize_existing(¶ms.path)?; self.file_system()?; self.fs_watch_manager.watch(connection_id, params).await } diff --git a/codex-rs/app-server/tests/suite/v2/fs.rs b/codex-rs/app-server/tests/suite/v2/fs.rs index 1f04c847d..7f8d21b03 100644 --- a/codex-rs/app-server/tests/suite/v2/fs.rs +++ b/codex-rs/app-server/tests/suite/v2/fs.rs @@ -305,8 +305,8 @@ async fn fs_methods_cover_current_fs_utils_surface() -> Result<()> { let remove_request_id = mcp .send_fs_remove_request(codex_app_server_protocol::FsRemoveParams { path: absolute_path(copied_dir.clone()), - recursive: None, - force: None, + recursive: Some(true), + force: Some(true), }) .await?; timeout( @@ -316,7 +316,7 @@ async fn fs_methods_cover_current_fs_utils_surface() -> Result<()> { .await??; assert!( !copied_dir.exists(), - "fs/remove should default to recursive+force for directory trees" + "fs/remove should recurse when recursive: true is requested" ); Ok(()) @@ -845,6 +845,229 @@ async fn fs_watch_rejects_relative_paths() -> Result<()> { Ok(()) } +const OUT_OF_SCOPE_ERROR: &str = "path is outside the authorized workspace roots"; + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn fs_methods_reject_paths_outside_workspace_roots() -> Result<()> { + // The app-server subprocess runs with `codex_home` as its cwd, so + // `codex_home` is the only authorized workspace root. A sibling temp dir is + // outside that scope and every fs/* method must refuse to touch it. + let codex_home = TempDir::new()?; + let outside = TempDir::new()?; + let outside_file = outside.path().join("secret.txt"); + std::fs::write(&outside_file, "secret")?; + let inside_file = codex_home.path().join("inside.txt"); + std::fs::write(&inside_file, "hello")?; + + let mut mcp = initialized_mcp(&codex_home).await?; + + let read_id = mcp + .send_fs_read_file_request(codex_app_server_protocol::FsReadFileParams { + path: absolute_path(outside_file.clone()), + }) + .await?; + expect_error_message(&mut mcp, read_id, OUT_OF_SCOPE_ERROR).await?; + + let write_id = mcp + .send_fs_write_file_request(FsWriteFileParams { + path: absolute_path(outside.path().join("written.txt")), + data_base64: STANDARD.encode("nope"), + }) + .await?; + expect_error_message(&mut mcp, write_id, OUT_OF_SCOPE_ERROR).await?; + + let mkdir_id = mcp + .send_fs_create_directory_request(codex_app_server_protocol::FsCreateDirectoryParams { + path: absolute_path(outside.path().join("newdir")), + recursive: None, + }) + .await?; + expect_error_message(&mut mcp, mkdir_id, OUT_OF_SCOPE_ERROR).await?; + + let metadata_id = mcp + .send_fs_get_metadata_request(codex_app_server_protocol::FsGetMetadataParams { + path: absolute_path(outside_file.clone()), + }) + .await?; + expect_error_message(&mut mcp, metadata_id, OUT_OF_SCOPE_ERROR).await?; + + let readdir_id = mcp + .send_fs_read_directory_request(codex_app_server_protocol::FsReadDirectoryParams { + path: absolute_path(outside.path().to_path_buf()), + }) + .await?; + expect_error_message(&mut mcp, readdir_id, OUT_OF_SCOPE_ERROR).await?; + + let remove_id = mcp + .send_fs_remove_request(codex_app_server_protocol::FsRemoveParams { + path: absolute_path(outside_file.clone()), + recursive: None, + force: None, + }) + .await?; + expect_error_message(&mut mcp, remove_id, OUT_OF_SCOPE_ERROR).await?; + + let copy_source_id = mcp + .send_fs_copy_request(FsCopyParams { + source_path: absolute_path(outside_file.clone()), + destination_path: absolute_path(codex_home.path().join("copied.txt")), + recursive: false, + }) + .await?; + expect_error_message(&mut mcp, copy_source_id, OUT_OF_SCOPE_ERROR).await?; + + let copy_destination_id = mcp + .send_fs_copy_request(FsCopyParams { + source_path: absolute_path(inside_file.clone()), + destination_path: absolute_path(outside.path().join("copied.txt")), + recursive: false, + }) + .await?; + expect_error_message(&mut mcp, copy_destination_id, OUT_OF_SCOPE_ERROR).await?; + + let watch_id = mcp + .send_fs_watch_request(codex_app_server_protocol::FsWatchParams { + watch_id: "watch-outside".to_string(), + path: absolute_path(outside_file.clone()), + }) + .await?; + expect_error_message(&mut mcp, watch_id, OUT_OF_SCOPE_ERROR).await?; + + // None of the denied operations may have touched the out-of-scope tree. + assert_eq!(std::fs::read_to_string(&outside_file)?, "secret"); + assert!(!outside.path().join("written.txt").exists()); + assert!(!outside.path().join("newdir").exists()); + assert!(!outside.path().join("copied.txt").exists()); + assert!(!codex_home.path().join("copied.txt").exists()); + + Ok(()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn fs_methods_allow_authorized_paths_inside_workspace_root() -> Result<()> { + // A positive control for the scope check: reads and writes inside the + // authorized workspace root continue to work through the scoped path. + let codex_home = TempDir::new()?; + let nested = codex_home.path().join("nested"); + std::fs::create_dir_all(&nested)?; + let file_path = nested.join("note.txt"); + + let mut mcp = initialized_mcp(&codex_home).await?; + let write_id = mcp + .send_fs_write_file_request(FsWriteFileParams { + path: absolute_path(file_path.clone()), + data_base64: STANDARD.encode("scoped write"), + }) + .await?; + timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(write_id)), + ) + .await??; + assert_eq!(std::fs::read_to_string(&file_path)?, "scoped write"); + + let read_id = mcp + .send_fs_read_file_request(codex_app_server_protocol::FsReadFileParams { + path: absolute_path(file_path), + }) + .await?; + let read_response: FsReadFileResponse = to_response( + timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(read_id)), + ) + .await??, + )?; + assert_eq!( + read_response, + FsReadFileResponse { + data_base64: STANDARD.encode("scoped write"), + } + ); + + Ok(()) +} + +#[cfg(unix)] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn fs_methods_reject_symlink_escape_from_workspace_root() -> Result<()> { + // A symlink that lives inside the workspace root but resolves outside of it + // must not become a read/remove escape hatch. + let codex_home = TempDir::new()?; + let outside = TempDir::new()?; + let secret = outside.path().join("secret.txt"); + std::fs::write(&secret, "top secret")?; + let link = codex_home.path().join("escape"); + symlink(outside.path(), &link)?; + + let mut mcp = initialized_mcp(&codex_home).await?; + + let read_id = mcp + .send_fs_read_file_request(codex_app_server_protocol::FsReadFileParams { + path: absolute_path(link.join("secret.txt")), + }) + .await?; + expect_error_message(&mut mcp, read_id, OUT_OF_SCOPE_ERROR).await?; + + let remove_id = mcp + .send_fs_remove_request(codex_app_server_protocol::FsRemoveParams { + path: absolute_path(link.join("secret.txt")), + recursive: None, + force: None, + }) + .await?; + expect_error_message(&mut mcp, remove_id, OUT_OF_SCOPE_ERROR).await?; + + assert_eq!(std::fs::read_to_string(&secret)?, "top secret"); + + Ok(()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn fs_remove_no_longer_defaults_to_recursive_force() -> Result<()> { + let codex_home = TempDir::new()?; + let dir = codex_home.path().join("keep"); + std::fs::create_dir_all(&dir)?; + std::fs::write(dir.join("child.txt"), "data")?; + + let mut mcp = initialized_mcp(&codex_home).await?; + + // A non-empty directory must not be deleted unless recursive is requested. + let remove_dir_id = mcp + .send_fs_remove_request(codex_app_server_protocol::FsRemoveParams { + path: absolute_path(dir.clone()), + recursive: None, + force: None, + }) + .await?; + timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_error_message(RequestId::Integer(remove_dir_id)), + ) + .await??; + assert!( + dir.join("child.txt").exists(), + "fs/remove must not recurse into directory trees by default" + ); + + // A missing path must error rather than being silently ignored, since force + // no longer defaults to true. + let remove_missing_id = mcp + .send_fs_remove_request(codex_app_server_protocol::FsRemoveParams { + path: absolute_path(codex_home.path().join("missing.txt")), + recursive: None, + force: None, + }) + .await?; + timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_error_message(RequestId::Integer(remove_missing_id)), + ) + .await??; + + Ok(()) +} + fn fs_changed_notification(notification: JSONRPCNotification) -> Result { let params = notification .params