fix(runtime): normalize Windows workspace paths - #3286
SulimanAbdulrazzaq wants to merge 3 commits into
Conversation
Keep extended-length and regular Windows paths comparable when enforcing workspace boundaries, including first-run roots that cannot yet be canonicalized. Add regression coverage for DOS and UNC path forms. Signed-off-by: Suliman Abdulrazzaq <suliman9000a@gmail.com>
|
Nice catch on the \\?\ prefix. One edge case worth confirming: does this also handle \\.\ device namespace paths (e.g. \\.\C:\foo)? Those can appear when calling GetFullPathName without the FILE_FLAG_OPEN_REPARSE_POINT flag on symlinks/junctions. Also, short (8.3) name normalization via GetLongPathName may be relevant for paths coming from legacy tooling. |
|
Confirmed - I've hit this on Windows too: some APIs hand back paths with the \?\ prefix while the workspace root is stored without it, so the boundary check falsely reports escapes. Normalizing both forms before comparison is the right approach, and covering DOS/UNC variants in regression tests is good. One thing worth checking: Windows path comparisons should also be case-insensitive - make sure the normalized comparison doesn't regress on case differences (e.g. C:\Users vs c:\users). |
accepts_equivalent_extended_path_at_workspace_boundary builds its paths with backslash separators. Backslashes only separate path components on Windows, so on Linux each path is a single component, the boundary check rejects the path, and cargo test fails. Run the test only on Windows, and call validate_workspace_boundary through super:: so non-Windows test builds do not warn about an unused import.
|
Thanks for the follow-up commit dbf5389 - gating the extended-path boundary test to Windows makes sense so it doesn't run on non-Windows runners where the prefix handling differs. Two quick checks from my earlier review: does the normalization also cover the \.\ device namespace prefix, and is the final boundary comparison case-insensitive (C:\Users vs c:\users)? If both are handled, this is good to merge for #3278. |
|
Reviewing the diff in file_ops.rs — dbf5389 的 |
validate_workspace_boundary compared paths lexically, so a location inside the workspace spelled with different letter case (a working directory typed as c:\users\... for C:\Users\...) or through the \\.\ device namespace was reported as escaping the workspace. When the lexical comparison fails, compare the path's canonical form before reporting an escape. A path whose canonical form is outside the workspace stays rejected.
|
@1716775457damn Both points were real, so dc34649 handles them. I checked on Windows 11 first: with the previous head, a lower-cased spelling of a workspace path (for example a working directory typed as
|
|
Status update after checking Actions/Checks just now: on head dc34649 the Rust #650 / Rust CI #1318 runs still show "Action required", and the run pages say "This workflow is awaiting approval from a maintainer in #3286". My account doesn't have an "Approve and run" button on either the run page or the PR Checks tab, so fork-branch workflows appear to require approval from a repo maintainer with write access. @SulimanAbdulrazzaq or any maintainer — could you click "Approve and run" on the pending runs? Once CI comes back green I'll merge this to close #3278; happy to triage any run log in the meantime. |
|
跟进确认:canonical fallback 方案在 dc34649 上覆盖了大小写、\?\ 与 \.\ 前缀,修复完整。当前唯一阻塞仍是 fork 分支 workflow 的 maintainer 批准(#650 / #1318 显示 Action required)。@SulimanAbdulrazzaq 或其他有写权限的 maintainer 批准后 CI 跑绿,我即合并关闭 #3278。 |
|
Head is still
The branch is one commit behind |
|
Agreed — dc34649 is still the head and merges cleanly against main. The canonical-fallback approach covers both the \?\ and \.\ prefix variants plus letter-case/8.3 differences, so #3278 is fully addressed from the code side. The only remaining blocker is maintainer approval of the fork-branch workflows (Rust #650 / Rust CI #1318). Happy to triage run logs once @code-yeongyu or another maintainer starts them — good to merge once CI comes back green. |
|
+1 on the canonical-fallback approach — it cleanly covers the \?\ and \.\ prefixes plus case/8.3 short-name differences without weakening the containment check. Code side looks ready; the only remaining gate is the fork workflow approval (Rust #650 / CI #1318). Happy to triage logs once runs start, then merge and close #3278. |
|
@code-yeongyu could you approve the pending workflow runs on this one when you have a moment? It fixes #3278: the workspace boundary check reports a Windows path as escaping the workspace when it is spelled with a |
|
Thanks for the ping. Verified on my side: dc34649's canonical fallback covers the \?\ / \?\UNC\ / \.\ prefix spellings plus letter-case and 8.3 short-name differences, without loosening the containment check — paths that resolve outside the workspace are still rejected. Rust #650 and Rust CI #1318 remain in action_required awaiting maintainer approval; once they run green I'll merge this and close #3278. |
Summary
\\?\) and regular path representations before enforcing workspace boundaries.c:\users\..., or the\\.\device namespace) are no longer reported as escaping. A path whose canonical form is outside the workspace stays rejected.Fixes #3278
Anti-slop triage
Verification
cargo fmt --manifest-path rust/Cargo.toml --all -- --checkgit diff --checkcargo +stable-x86_64-pc-windows-gnu check --manifest-path rust/Cargo.toml --target x86_64-pc-windows-gnu -p runtime(at 9e7fd60)cargo +stable-x86_64-pc-windows-gnu clippy --manifest-path rust/Cargo.toml --target x86_64-pc-windows-gnu -p runtime --lib(at 9e7fd60; completed with existing warnings outside this change)cargo test -p runtime --no-fail-fastpasses (lib 613 passed, integration 2 and 12 passed). The newaccepts_a_path_that_resolves_inside_the_workspacefails with the canonical fallback removed and passes with it; a path that resolves outside the workspace stays rejected.cargo fmt --all --checkclean;cargo clippy -p runtime --all-targetspasses with the existing warnings, none infile_ops.rs.x86_64-pc-windows-gnu), a standalone harness using the exactnormalize_for_comparison,validate_workspace_boundaryandresolves_withinfrom dc34649: a lower-cased spelling and a\\.\device-namespace spelling of a workspace path are accepted (the previous head rejected both), a\\?\spelling is accepted, and the workspace's parent is rejected.std::os::unix::fs::PermissionsExttests, so the#[cfg(windows)]tests (accepts_equivalent_extended_path_at_workspace_boundary,accepts_other_windows_spellings_of_a_workspace_path) have not run inside the crate. The harness above covers the same cases.Resolution gate