Ensure relative paths are rooted#6357
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes AppInstaller::Filesystem::PathEscapesBaseDirectory() so that it correctly treats any path with a root component (absolute, root-relative, drive-relative, UNC, extended-length) as escaping, instead of only checking for leading .. after lexical normalization. This hardens validation of manifest-provided nested installer paths and portable command aliases against incorrectly-accepted rooted paths.
Changes:
- Reject rooted paths up front via
std::filesystem::path::has_root_path()before lexical./..normalization. - Keep the existing lexical traversal check by rejecting normalized paths whose first component is
... - Expand unit tests to cover rooted path forms (absolute, UNC,
\\?\), root-relative, drive-relative, plus additional relative edge cases.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/AppInstallerSharedLib/Filesystem.cpp | Updates PathEscapesBaseDirectory() to reject any rooted path and then apply lexical ..-based escape detection. |
| src/AppInstallerCLITests/Filesystem.cpp | Adds broader test coverage for relative/escaping cases, including absolute/UNC/root-relative/drive-relative paths. |
Trenly
left a comment
Trenly
left a comment
There was a problem hiding this comment.
I know this is a minor change, but should there be an entry in ReleaseNotes under the bug fix section?
* Fixed relative path handling for rooted paths
Sure. I forgot we had a section for bug fixes, and it didn't seem big enough to list it otherwise. |
📖 Description
The way we were checking that a relative path was inside a directory was wrong because it didn't check if the path was rooted. This PR fixes that.
🔗 References
🔍 Validation
✅ Checklist
📋 Issue Type
Microsoft Reviewers: Open in CodeFlow