From cd19c9588e4e840338db6f1462690f3cba88d556 Mon Sep 17 00:00:00 2001 From: Adam Perry Date: Sat, 13 Jun 2026 02:04:27 +0000 Subject: [PATCH] codex: expand native path rendering coverage (#27819) --- .../utils/path-uri/src/native_path_string.rs | 25 ++++++++-- .../path-uri/src/native_path_string_tests.rs | 46 ++++++++++++++++++- 2 files changed, 66 insertions(+), 5 deletions(-) diff --git a/codex-rs/utils/path-uri/src/native_path_string.rs b/codex-rs/utils/path-uri/src/native_path_string.rs index 97195ba62d..f6823a185e 100644 --- a/codex-rs/utils/path-uri/src/native_path_string.rs +++ b/codex-rs/utils/path-uri/src/native_path_string.rs @@ -33,9 +33,6 @@ impl PathConvention { } } -#[cfg(not(any(windows, unix)))] -compile_error!("PathConvention::native() requires a Windows or Unix target"); - impl fmt::Display for PathConvention { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { @@ -123,10 +120,15 @@ impl JsonSchema for NativePathString { fn render_posix_path(path: &PathUri) -> Result { let url = path.to_url(); + // POSIX file paths do not have a UNC authority, so `file://server/share` + // cannot be represented as `/share` without losing the server identity. if url.host_str().is_some() { return Err(incompatible_convention(path, PathConvention::Posix)); } + // URI segments are already separated with `/` on every host. Decode each + // one independently so `file:///a%20dir/file` becomes `/a dir/file` while + // an encoded separator cannot silently introduce another path component. let mut rendered = String::new(); for segment in path_segments(&url) { rendered.push('/'); @@ -144,6 +146,9 @@ fn render_windows_path(path: &PathUri) -> Result let mut segments = path_segments(&url); let mut rendered = String::new(); if let Some(host) = url.host_str() { + // A URI authority selects the UNC form: `file://server/share/file` + // becomes `\\server\share\file`. The first segment is the share name, + // which must be present and valid as a Windows path component. let Some(share) = segments.next() else { return Err(incompatible_convention(path, PathConvention::Windows)); }; @@ -157,6 +162,9 @@ fn render_windows_path(path: &PathUri) -> Result rendered.push('\\'); rendered.push_str(&share); } else { + // Without an authority, Windows requires a drive root. For example, + // `file:///C:/src/main.rs` begins with the `C:` URI segment and renders + // as `C:\src\main.rs`; a POSIX URI such as `file:///usr/bin` is rejected. let Some(drive) = segments.next() else { return Err(incompatible_convention(path, PathConvention::Windows)); }; @@ -169,6 +177,8 @@ fn render_windows_path(path: &PathUri) -> Result } for segment in segments { + // URL path separators become Windows separators after each component + // has been decoded and checked using Windows filename rules. let segment = decode_native_segment(path, segment, PathConvention::Windows)?; if !segment.is_empty() { validate_windows_component(path, &segment)?; @@ -176,6 +186,8 @@ fn render_windows_path(path: &PathUri) -> Result rendered.push('\\'); rendered.push_str(&segment); } + // `file:///C:` and `file:///C:/` both identify the drive root, never the + // drive-relative path `C:`. if rendered.len() == 2 && rendered.as_bytes()[1] == b':' { rendered.push('\\'); } @@ -192,7 +204,11 @@ fn decode_native_segment( segment: &str, convention: PathConvention, ) -> Result { + // Decode exactly once. Thus `%20` becomes a space and `%252F` becomes the + // literal text `%2F`, rather than being decoded a second time into `/`. let bytes = urlencoding::decode_binary(segment.as_bytes()); + // A separator encoded inside one URI segment would change path structure: + // reject `%2F` for both conventions and `%5C` for Windows. let contains_separator = bytes.contains(&b'/') || (convention == PathConvention::Windows && bytes.contains(&b'\\')); if contains_separator { @@ -212,6 +228,9 @@ fn validate_windows_component( path: &PathUri, component: &str, ) -> Result<(), NativePathStringError> { + // Windows forbids control characters and `<>:"/\|?*` in components. It + // also aliases trailing spaces and dots, so names such as `report?.txt`, + // `trailing.`, and `trailing ` are rejected instead of being rewritten. let contains_invalid_character = component .chars() .any(|character| character <= '\u{1f}' || r#"<>:"/\|?*"#.contains(character)); diff --git a/codex-rs/utils/path-uri/src/native_path_string_tests.rs b/codex-rs/utils/path-uri/src/native_path_string_tests.rs index 2e3ca90688..016840da17 100644 --- a/codex-rs/utils/path-uri/src/native_path_string_tests.rs +++ b/codex-rs/utils/path-uri/src/native_path_string_tests.rs @@ -6,11 +6,30 @@ use pretty_assertions::assert_eq; fn renders_posix_paths_on_every_host() { for (uri, expected) in [ ("file:///", "/"), + ("file:///home/alice/src/main.rs", "/home/alice/src/main.rs"), ("file:///home/alice/a%20file.rs", "/home/alice/a file.rs"), + ("file:///workspace/src/lib.rs", "/workspace/src/lib.rs"), + ( + "file:///workspace/tests/test.rs", + "/workspace/tests/test.rs", + ), + ("file:///etc", "/etc"), ("file:///tmp/", "/tmp/"), ("file:///C:/Project", "/C:/Project"), + ("file:///C:", "/C:"), ("file:///tmp/%E2%98%83", "/tmp/☃"), ("file:///tmp/a%5Cb", "/tmp/a\\b"), + ("file:///tmp/100%25/file", "/tmp/100%/file"), + ("file:///tmp/a%3Fb%23c%25d", "/tmp/a?b#c%d"), + ("file:///tmp/a%252Fb", "/tmp/a%2Fb"), + ( + "file:///bad/path/L3RtcC9udWxsLQAt_y1ieXRl", + "/bad/path/L3RtcC9udWxsLQAt_y1ieXRl", + ), + ("FILE:///workspace/src", "/workspace/src"), + ("file:/workspace/src", "/workspace/src"), + ("file://localhost/workspace/src", "/workspace/src"), + ("file://LOCALHOST/workspace/src", "/workspace/src"), ] { let path = PathUri::parse(uri).expect("valid file URI"); assert_eq!( @@ -31,6 +50,8 @@ fn renders_windows_drive_paths_on_every_host() { ), ("file:///C:/", "C:\\"), ("file:///C:", "C:\\"), + ("file:///C:/Users", r"C:\Users"), + ("file:///C:/Windows", r"C:\Windows"), ("file:///d:/snowman/%E2%98%83", r"d:\snowman\☃"), ("file:///C:/tmp/", "C:\\tmp\\"), ("file:///C:/test%20with%20%25/path", r"C:\test with %\path"), @@ -66,6 +87,7 @@ fn renders_windows_unc_paths_on_every_host() { "file://server/share/src/main.rs", r"\\server\share\src\main.rs", ), + ("file://server/share", r"\\server\share"), ("file://server/share/", "\\\\server\\share\\"), ("file://shares/files/c%23/p.cs", r"\\shares\files\c#\p.cs"), ( @@ -86,7 +108,9 @@ fn renders_windows_unc_paths_on_every_host() { #[test] fn rejects_paths_incompatible_with_the_convention() { for (uri, convention) in [ + ("file://server/share/file.txt", PathConvention::Posix), ("file://server/share/file.rs", PathConvention::Posix), + ("file:///usr/local/file.txt", PathConvention::Windows), ("file:///home/alice/file.rs", PathConvention::Windows), ("file://server/", PathConvention::Windows), ("file:///_:/path", PathConvention::Windows), @@ -117,14 +141,14 @@ fn renders_native_opaque_fallback_paths_lossily() { use std::os::unix::ffi::OsStringExt; let native_path = std::path::PathBuf::from(std::ffi::OsString::from_vec( - b"/tmp/null-\0-non-utf8-\xff".to_vec(), + b"/tmp/null-\0-\xff-byte".to_vec(), )); let path = PathUri::from_path(native_path).expect("absolute native path"); assert_eq!( NativePathString::from_path_uri(&path, PathConvention::Posix) .map(NativePathString::into_string), - Ok("/tmp/null-\0-non-utf8-�".to_string()) + Ok("/tmp/null-\0-�-byte".to_string()) ); assert_eq!( NativePathString::from_path_uri(&path, PathConvention::Windows), @@ -135,6 +159,24 @@ fn renders_native_opaque_fallback_paths_lossily() { ); } +#[cfg(windows)] +#[test] +fn renders_windows_namespace_fallback_paths() { + for native_path in [ + r"\\.\COM1", + r"\\?\Volume{00000000-0000-0000-0000-000000000000}\file.rs", + ] { + let path = PathUri::from_path(native_path).expect("absolute Windows namespace path"); + + assert_eq!( + NativePathString::from_path_uri(&path, PathConvention::Windows) + .map(NativePathString::into_string), + Ok(native_path.to_string()), + "rendering {native_path}" + ); + } +} + #[cfg(windows)] #[test] fn renders_native_opaque_fallback_paths_lossily() {