From 81f340436c6ecd7cf7ed2fdc5199c5e3597f5cf9 Mon Sep 17 00:00:00 2001 From: "Adam Perry @ OpenAI" Date: Wed, 24 Jun 2026 15:33:18 -0700 Subject: [PATCH] path-uri: normalize parent segments in absolute joins (#29903) ## Why `PathUri::join` normalized `..` for relative paths, but its absolute-path branch rebuilt URIs through `url::PathSegmentsMut::push`, which skips dot segments. `/tmp/a/../b` therefore resolved to `/tmp/a/b` instead of `/tmp/b`. ## What changed Normalize absolute native path segments before constructing the file URI. Parent traversal now clamps at POSIX roots, Windows drive roots, and UNC share roots, including paths with repeated separators. Add platform-independent coverage for POSIX, drive, UNC, root-clamping, and repeated-separator cases. ## Manual validation - `just test -p codex-utils-path-uri` --- .../src/absolute_path_normalization.rs | 46 +++++++ codex-rs/utils/path-uri/src/lib.rs | 32 ++--- codex-rs/utils/path-uri/src/tests.rs | 127 ++++++++++++++++++ 3 files changed, 184 insertions(+), 21 deletions(-) create mode 100644 codex-rs/utils/path-uri/src/absolute_path_normalization.rs diff --git a/codex-rs/utils/path-uri/src/absolute_path_normalization.rs b/codex-rs/utils/path-uri/src/absolute_path_normalization.rs new file mode 100644 index 000000000..d391c2e5c --- /dev/null +++ b/codex-rs/utils/path-uri/src/absolute_path_normalization.rs @@ -0,0 +1,46 @@ +use crate::PathConvention; +use crate::PathUri; +use url::Url; + +pub(super) fn path_uri_from_segments<'a>( + convention: PathConvention, + host: Option<&str>, + segments: impl Iterator, +) -> Option { + let mut url = Url::parse("file:///").ok()?; + if let Some(host) = host { + url.set_host(Some(host)).ok()?; + } + let anchor_depth = usize::from(convention == PathConvention::Windows); + let mut depth = 0; + let mut normalized_segments = Vec::new(); + let mut has_trailing_separator = false; + for segment in segments { + match segment { + "" => has_trailing_separator = true, + "." => has_trailing_separator = false, + ".." => { + has_trailing_separator = false; + if depth > anchor_depth { + normalized_segments.pop(); + depth -= 1; + } + } + segment => { + normalized_segments.push(segment); + depth += 1; + has_trailing_separator = false; + } + } + } + if has_trailing_separator + || (convention == PathConvention::Windows && host.is_none() && depth == anchor_depth) + { + normalized_segments.push(""); + } + { + let mut url_segments = url.path_segments_mut().ok()?; + url_segments.clear().extend(normalized_segments); + } + PathUri::try_from(url).ok() +} diff --git a/codex-rs/utils/path-uri/src/lib.rs b/codex-rs/utils/path-uri/src/lib.rs index 7b84beccb..ccc9cfd79 100644 --- a/codex-rs/utils/path-uri/src/lib.rs +++ b/codex-rs/utils/path-uri/src/lib.rs @@ -18,8 +18,11 @@ use thiserror::Error; use ts_rs::TS; use url::Url; +mod absolute_path_normalization; mod api_path_string; +use absolute_path_normalization::path_uri_from_segments; + pub use api_path_string::LegacyAppPathString; pub use api_path_string::LegacyAppPathStringError; @@ -589,7 +592,7 @@ fn parse_posix_path(path: &str) -> Option { format!("/{path}").as_bytes(), )); } - path_uri_from_segments(/*host*/ None, path.split('/')) + path_uri_from_segments(PathConvention::Posix, /*host*/ None, path.split('/')) } fn parse_windows_path(path: &str) -> Option { @@ -612,6 +615,7 @@ fn parse_windows_path(path: &str) -> Option { if drive.is_ascii_alphabetic() && is_windows_separator_byte(*separator) ) { return path_uri_from_segments( + PathConvention::Windows, /*host*/ None, std::iter::once(&path[..2]).chain(path[3..].split(is_windows_separator_char)), ); @@ -623,31 +627,17 @@ fn parse_windows_path(path: &str) -> Option { let mut components = path[2..].split(is_windows_separator_char); let host = components.next().filter(|host| !host.is_empty())?; let share = components.next().filter(|share| !share.is_empty())?; - return path_uri_from_segments(Some(host), std::iter::once(share).chain(components)) - .or_else(|| Some(windows_opaque_path_uri(path))); + return path_uri_from_segments( + PathConvention::Windows, + Some(host), + std::iter::once(share).chain(components), + ) + .or_else(|| Some(windows_opaque_path_uri(path))); } None } -fn path_uri_from_segments<'a>( - host: Option<&str>, - segments: impl Iterator, -) -> Option { - let mut url = Url::parse("file:///").ok()?; - if let Some(host) = host { - url.set_host(Some(host)).ok()?; - } - { - let mut url_segments = url.path_segments_mut().ok()?; - url_segments.clear(); - for segment in segments { - url_segments.push(segment); - } - } - PathUri::try_from(url).ok() -} - fn windows_opaque_path_uri(path: &str) -> PathUri { let path_bytes = path .encode_utf16() diff --git a/codex-rs/utils/path-uri/src/tests.rs b/codex-rs/utils/path-uri/src/tests.rs index 8954b7ef4..68113c8f6 100644 --- a/codex-rs/utils/path-uri/src/tests.rs +++ b/codex-rs/utils/path-uri/src/tests.rs @@ -617,6 +617,133 @@ fn join_replaces_posix_absolute_path() { ); } +#[test] +fn join_keeps_canonicalized_posix_double_slash_paths_hierarchical() { + let base = PathUri::parse("file:///workspace").expect("valid base URI"); + let cwd = base + .join("//server/share/project") + .expect("valid absolute path"); + + assert_eq!( + cwd, + PathUri::parse("file:///server/share/project").expect("valid canonical URI") + ); + assert_eq!( + cwd.parent(), + Some(PathUri::parse("file:///server/share").expect("valid parent URI")) + ); + assert_eq!( + cwd.join("AGENTS.md"), + Ok(PathUri::parse("file:///server/share/project/AGENTS.md").expect("valid child URI")) + ); + #[cfg(unix)] + assert_eq!( + cwd.to_abs_path() + .expect("cwd should convert to a native path"), + AbsolutePathBuf::try_from("/server/share/project") + .expect("expected native path should be absolute") + ); +} + +#[test] +fn join_normalizes_absolute_parent_segments() { + for (base, path, expected) in [ + ("file:///workspace", "/tmp/a/../b", "file:///tmp/b"), + ("file:///C:/workspace", r"D:\tmp\a\..\b", "file:///D:/tmp/b"), + ( + "file:///C:/workspace", + r"\\server\share\a\..\b", + "file://server/share/b", + ), + ("file:///workspace", "/tmp//a/../b", "file:///tmp/b"), + ("file:///workspace", "/tmp/a/..//b", "file:///tmp/b"), + ("file:///workspace", "/tmp/a///../b", "file:///tmp/b"), + ( + "file:///C:/workspace", + r"D:\tmp\a\\\..\b", + "file:///D:/tmp/b", + ), + ( + "file:///C:/workspace", + r"\\server\share\a\\\..\b", + "file://server/share/b", + ), + ("file:///workspace", "/tmp/a///b/../..", "file:///tmp"), + ( + "file:///C:/workspace", + r"D:\tmp\a\\\b\..\..", + "file:///D:/tmp", + ), + ( + "file:///C:/workspace", + r"\\server\share\a\\\b\..\..", + "file://server/share", + ), + ] { + let base = PathUri::parse(base).expect("valid base URI"); + let expected = PathUri::parse(expected).expect("valid expected URI"); + assert_eq!(base.join(path), Ok(expected), "joining {path}"); + } +} + +#[test] +fn join_absolute_parent_segments_stop_at_native_path_roots() { + for (base, path, expected) in [ + ("file:///workspace", "/a/..", "file:///"), + ("file:///C:/workspace", r"D:\a\..", "file:///D:/"), + ( + "file:///C:/workspace", + r"\\server\share\a\..", + "file://server/share", + ), + ("file:///workspace", "/../../b", "file:///b"), + ("file:///C:/workspace", r"D:\..\..\b", "file:///D:/b"), + ( + "file:///C:/workspace", + r"\\server\share\..\..\b", + "file://server/share/b", + ), + ( + "file:///C:/workspace", + r"\\server\share\\\..\b", + "file://server/share/b", + ), + ] { + let base = PathUri::parse(base).expect("valid base URI"); + let expected = PathUri::parse(expected).expect("valid expected URI"); + assert_eq!(base.join(path), Ok(expected), "joining {path}"); + } +} + +#[test] +fn join_collapses_redundant_absolute_separators() { + for (base, path, expected) in [ + ("file:///workspace", "/tmp///", "file:///tmp/"), + ("file:///workspace", "///", "file:///"), + ( + "file:///workspace", + "///server/share///", + "file:///server/share/", + ), + ("file:///C:/workspace", r"D:\tmp\\\", "file:///D:/tmp/"), + ("file:///C:/workspace", r"D:\\\", "file:///D:/"), + ( + "file:///C:/workspace", + r"\\server\share\tmp\\\", + "file://server/share/tmp/", + ), + ( + "file:///C:/workspace", + r"\\server\share\\\", + "file://server/share/", + ), + ] { + let base = PathUri::parse(base).expect("valid base URI"); + let expected = PathUri::parse(expected).expect("valid expected URI"); + assert_eq!(base.join(path), Ok(expected), "joining {path}"); + } +} + #[test] fn join_replaces_windows_absolute_path() { let base = PathUri::parse("file:///C:/workspace/src").expect("valid base URI");