Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions codex-rs/exec-server/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -340,9 +340,9 @@ Params:

## Filesystem RPCs

Filesystem methods use canonical `file:` URIs and return JSON-RPC errors for
invalid or unavailable paths. For compatibility, requests also accept native
absolute path strings and normalize them to `file:` URIs:
Filesystem methods require valid `file:` URI strings and return JSON-RPC errors
for invalid or unavailable paths. Native absolute path strings are rejected;
callers must convert them to `file:` URIs before sending requests:

- `fs/readFile`
- `fs/open`, `fs/readBlock`, and `fs/close` (internal transport for
Expand Down
51 changes: 23 additions & 28 deletions codex-rs/exec-server/src/protocol.rs
Original file line number Diff line number Diff line change
Expand Up @@ -591,38 +591,33 @@ mod tests {
}

#[test]
fn filesystem_protocol_accepts_legacy_absolute_paths_and_serializes_path_uris() {
let legacy_path = std::env::current_dir()
fn filesystem_protocol_rejects_native_absolute_paths() {
let native_path = std::env::current_dir()
.expect("current directory")
.join("legacy-file.txt");
let legacy_cwd = std::env::current_dir().expect("current directory");
let native_sandbox = FileSystemSandboxContext::from_permission_profile_with_cwd(
PermissionProfile::default(),
PathUri::from_host_native_path(&legacy_cwd).expect("cwd URI"),
);
let mut legacy_sandbox =
serde_json::to_value(&native_sandbox).expect("sandbox should serialize");
legacy_sandbox["cwd"] = serde_json::json!(legacy_cwd.to_string_lossy());
let params: FsReadFileParams = serde_json::from_value(serde_json::json!({
"path": legacy_path.to_string_lossy(),
"sandbox": legacy_sandbox,
.join("native-file.txt");
let native_cwd = std::env::current_dir().expect("current directory");

serde_json::from_value::<FsReadFileParams>(serde_json::json!({
"path": native_path.to_string_lossy(),
"sandbox": null,
}))
.expect("legacy absolute path should deserialize");
let expected_sandbox = native_sandbox;
let expected = FsReadFileParams {
path: PathUri::from_host_native_path(legacy_path).expect("path URI"),
sandbox: Some(expected_sandbox.clone()),
};
.expect_err("native absolute path should not deserialize as a URI");

assert_eq!(params, expected);
assert_eq!(
serde_json::to_value(params).expect("params should serialize"),
serde_json::json!({
"path": expected.path.to_string(),
"sandbox": serde_json::to_value(expected_sandbox)
.expect("sandbox should serialize"),
})
let sandbox = FileSystemSandboxContext::from_permission_profile_with_cwd(
PermissionProfile::default(),
PathUri::from_host_native_path(&native_cwd).expect("cwd URI"),
);
let mut native_path_sandbox =
serde_json::to_value(sandbox).expect("sandbox should serialize");
native_path_sandbox["cwd"] = serde_json::json!(native_cwd.to_string_lossy());

serde_json::from_value::<FsReadFileParams>(serde_json::json!({
"path": PathUri::from_host_native_path(native_path)
.expect("path URI")
.to_string(),
"sandbox": native_path_sandbox,
}))
.expect_err("native absolute sandbox cwd should not deserialize as a URI");
}

#[test]
Expand Down
9 changes: 5 additions & 4 deletions codex-rs/exec-server/tests/process.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ use codex_exec_server::ReadResponse;
use codex_exec_server::TerminateResponse;
use codex_exec_server::WriteResponse;
use codex_exec_server::WriteStatus;
use codex_utils_path_uri::PathUri;
use common::exec_server::exec_server;
use pretty_assertions::assert_eq;

Expand Down Expand Up @@ -46,7 +47,7 @@ async fn exec_server_starts_process_over_websocket() -> anyhow::Result<()> {
serde_json::json!({
"processId": "proc-1",
"argv": ["true"],
"cwd": std::env::current_dir()?,
"cwd": PathUri::from_host_native_path(std::env::current_dir()?)?,
"env": {},
"tty": false,
"pipeStdin": false,
Expand Down Expand Up @@ -113,7 +114,7 @@ async fn exec_server_defaults_omitted_pipe_stdin_to_closed_stdin() -> anyhow::Re
"-c",
"sleep 0.3; if IFS= read -r line; then printf 'read:%s\\n' \"$line\"; else printf 'eof\\n'; fi"
],
"cwd": std::env::current_dir()?,
"cwd": PathUri::from_host_native_path(std::env::current_dir()?)?,
"env": {},
"tty": false,
"arg0": null
Expand Down Expand Up @@ -207,7 +208,7 @@ async fn exec_server_dedupes_retried_process_write_ids() -> anyhow::Result<()> {
"-c",
"IFS= read -r first; printf 'line:%s\\n' \"$first\"; IFS= read -r second; printf 'line:%s\\n' \"$second\""
],
"cwd": std::env::current_dir()?,
"cwd": PathUri::from_host_native_path(std::env::current_dir()?)?,
"env": {},
"tty": false,
"pipeStdin": true,
Expand Down Expand Up @@ -341,7 +342,7 @@ async fn exec_server_resumes_detached_session_without_killing_processes() -> any
serde_json::json!({
"processId": "proc-resume",
"argv": ["/bin/sh", "-c", "sleep 5"],
"cwd": std::env::current_dir()?,
"cwd": PathUri::from_host_native_path(std::env::current_dir()?)?,
"env": {},
"tty": false,
"pipeStdin": false,
Expand Down
33 changes: 3 additions & 30 deletions codex-rs/utils/path-uri/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,13 +45,9 @@ const BAD_PATH_URI_PREFIX: &str = "file:///%00/bad/path/";
/// are not resolved.
///
/// Serde represents a `PathUri` as its canonical URI string. Deserialization
/// also accepts an absolute native path for compatibility with fields that
/// previously used [`AbsolutePathBuf`]; relative paths are rejected. Valid
/// `file:` strings round-trip through their canonical URL form, including
/// encoded non-UTF-8 path bytes, but conversion to a native path remains
/// host-dependent as described by [RFC 8089].
/// accepts only valid `file:` URI strings. These strings round-trip through
/// their canonical URL form, including encoded non-UTF-8 path bytes.
///
/// [RFC 8089]: https://www.rfc-editor.org/rfc/rfc8089.html
/// [VS Code resources]: https://github.com/microsoft/vscode/blob/main/src/vs/base/common/resources.ts
#[derive(Clone, Debug, PartialEq, Eq, Hash, TS)]
#[ts(type = "string")]
Expand Down Expand Up @@ -478,30 +474,7 @@ impl<'de> Deserialize<'de> for PathUri {
D: Deserializer<'de>,
{
let value = String::deserialize(deserializer)?;
let unsupported_scheme = match Url::parse(&value) {
Ok(url) => match Self::try_from(url) {
Ok(uri) => return Ok(uri),
// `Url` parses a Windows drive prefix such as `C:\` as the
// scheme `c`. Give any unsupported URI one chance to satisfy
// the native absolute-path invariant before reporting it.
Err(error @ PathUriParseError::UnsupportedScheme(_)) => Some(error),
Err(error) => return Err(serde::de::Error::custom(error)),
},
Err(url::ParseError::RelativeUrlWithoutBase) => None,
Err(error) => {
return Err(serde::de::Error::custom(PathUriParseError::InvalidUri(
error,
)));
}
};

let path = AbsolutePathBuf::from_absolute_path_checked(value).map_err(|path_error| {
serde::de::Error::custom(
unsupported_scheme
.map_or_else(|| path_error.to_string(), |error| error.to_string()),
)
})?;
Ok(Self::from_abs_path(&path))
Self::parse(&value).map_err(serde::de::Error::custom)
Comment thread
anp-oai marked this conversation as resolved.
Comment thread
anp-oai marked this conversation as resolved.
}
}

Expand Down
15 changes: 6 additions & 9 deletions codex-rs/utils/path-uri/src/tests.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
use super::*;
use codex_utils_absolute_path::AbsolutePathBufGuard;
use pretty_assertions::assert_eq;
#[cfg(windows)]
use std::ffi::OsString;
Expand Down Expand Up @@ -418,14 +417,14 @@ fn path_uri_serializes_as_a_string() {
}

#[test]
fn path_uri_deserializes_legacy_absolute_paths() {
fn path_uri_rejects_native_absolute_paths_during_deserialization() {
let path = AbsolutePathBuf::current_dir()
.expect("current directory")
.join("workspace/src");
let json = serde_json::to_string(&path).expect("absolute path should serialize");
let uri: PathUri = serde_json::from_str(&json).expect("legacy absolute path should parse");

assert_eq!(uri, PathUri::from_abs_path(&path));
serde_json::from_str::<PathUri>(&json)
.expect_err("native absolute path should not deserialize as a URI");
}

#[test]
Expand All @@ -437,13 +436,11 @@ fn path_uri_rejects_relative_native_paths() {
}

#[test]
fn path_uri_rejects_legacy_relative_paths_with_absolute_path_guard() {
let base = AbsolutePathBuf::current_dir().expect("current directory");
let _guard = AbsolutePathBufGuard::new(base.as_path());
fn path_uri_rejects_relative_strings_during_deserialization() {
let error = serde_json::from_str::<PathUri>(r#""src/lib.rs""#)
.expect_err("legacy relative path should be rejected");
.expect_err("relative path should be rejected");

assert!(error.to_string().contains("path is not absolute"));
assert!(error.to_string().contains("relative URL without a base"));
}

#[test]
Expand Down
Loading