[JohnMcPMS]

📖 Description

Enforce that fields used for paths cannot have invalid characters or sequences that would be used to traverse the filesystem.

🔍 Validation

Added tests for enforcement.

Microsoft Reviewers: Open in CodeFlow

[Kaleb Luedtke (Trenly)]

It might be a non-issue, but this introduces a change to the string between time of check and time of use. If the UTF8 representation passes the check, is it guaranteed that the UTF16 represenation will too?

[ranm-msft]

Looked at this from the "what can still reach the filesystem" angle rather than the diff shape. Enforcing the schema rules on Id/Version at runtime is the right call - REST sources never went through the schema, so that was a real hole - and the point-of-use PathEscapesBaseDirectory checks land where the values actually get concatenated (ContextOrchestrator, the temp/download dirs, the log name, Fonts).

One gap in ValidatePathFieldValue: the character set covers the Win32 invalid characters and relative specifiers, but not the other two things that make a name unusable as a path component.

• Reserved DOS device names are matched on the base name even when followed by an extension - NUL.txt and NUL.tar.gz are documented as equivalent to NUL - so a PackageIdentifier like NUL.foo still produces the component NUL.foo.1.0. The dotted-segment shape of an identifier doesn't save us here.
• PackageVersion terminates the constructed component, so a trailing dot or space ends up at the end of the directory/file name, where Win32 may normalize, alias or reject it depending on the API and the namespace form.

Separately, the control-character check starts at 0x01, so an embedded NUL passes - a REST source can send \u0000 and everything downstream is null-terminated.

Is the intent to cover those in the validator too, or to let them fail at the point of use?

[ranm-msft]

Re-reviewed at head against my earlier comment. Reserved DOS device names, embedded NUL/control characters (<= 0x1f), and the trailing-dot case are all addressed now - ValidatePathFieldValue routing through MakeSuitablePathPart and mapping to ReservedPathName covers those cleanly. Thanks for picking those up.

One case from that list looks like it is still open, and narrower than I originally framed it: PackageVersion values ending in a space, arriving over REST.

YAML manifests are safe - population trims PackageVersion, so a trailing space from the community repo does not survive. The REST path is different: DeserializeVersionInfo preserves the raw value, ValidatePackageVersion passes disallowWhitespace: false so spaces are legal, and MakeSuitablePathPart only treats a final . as illegal, not a final space. Because ValidatePathFieldValue only catches exceptions rather than comparing the sanitized output back to the input, a sanitization that is a silent no-op passes.

GetPathPart puts Version last, so the trailing space lands at the end of the component. On Windows "v1 " aliases to "v1" for both directories and files, which means two distinct REST versions can collide on one path.

Worth rejecting a trailing space alongside the trailing-dot case, or having the validator reject whenever MakeSuitablePathPart would alter the value? The latter closes this class of gap generally rather than one character at a time.

[Flor Chacón (florelis)]

Is ' ' the only whitespace we care about?

[JohnMcPMS]

Other whitespace are already excluded by the control character block at 150.

[Flor Chacón (florelis)]

Aren't there other fancy Unicode space characters? Or is this ASCII only?

[JohnMcPMS]

There are almost certainly other codepoints that have no glyph but consume space (and some that neither have a glyph nor consume space). We are enforcing the more common thing here not for security, but simply to error earlier.

[Copilot]

Copilot review overview

🟡 Changes recommended

Length validation conflicts with supported schemas, and some reserved-name and path-component cases remain unhandled.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 3 · 1

Open (4)

• Validate package path components beyond traversal checks · New
• Use schema-aware Unicode length validation · New
• Recognize superscript Windows reserved device names · New
• Correct the typo in identifier · New

What changed in this PR

Adds path-safety enforcement for manifest identifiers and versions across parsing, REST sources, and filesystem consumers.

Changes:

• Adds path-field validation and diagnostics.
• Sanitizes path construction in install/download workflows.
• Adds validation and REST tests.

File	Description
src/​WinGetUtilInterop/​Common/​ManifestErrorId.cs	Adds interop error IDs.
src/​AppInstallerRepositoryCore/​Rest/​Schema/​1_0/​Json/​SearchResponseDeserializer_1_0.cpp	Validates REST search fields.
src/​AppInstallerRepositoryCore/​Rest/​Schema/​1_0/​Json/​ManifestDeserializer_1_0.cpp	Trims REST manifest fields.
src/​AppInstallerCommonCore/​Public/​winget/​ManifestValidation.h	Exposes path validators.
src/​AppInstallerCommonCore/​Public/​winget/​Manifest.h	Declares path-part helpers.
src/​AppInstallerCommonCore/​Manifest/​YamlParser.cpp	Enforces path validation during parsing.
src/​AppInstallerCommonCore/​Manifest/​ManifestValidation.cpp	Implements path-field validation.
src/​AppInstallerCommonCore/​Manifest/​Manifest.cpp	Implements safe path construction.
src/​AppInstallerCommonCore/​Fonts.cpp	Adds font path checks.
src/​AppInstallerCLITests/​YamlManifest.cpp	Tests validation and sanitization.
src/​AppInstallerCLITests/​RestInterface_1_0.cpp	Tests REST field trimming.
src/​AppInstallerCLIE2ETests/​ValidateCommand.cs	Updates invalid-manifest fixture path.
src/​AppInstallerCLIE2ETests/​TestData/​InvalidManifests/​TestInvalidManifest.yaml	Adds invalid test manifest.
src/​AppInstallerCLICore/​Workflows/​ShellExecuteInstallerHandler.cpp	Sanitizes log filenames.
src/​AppInstallerCLICore/​Workflows/​DownloadFlow.cpp	Sanitizes download directories.
src/​AppInstallerCLICore/​ContextOrchestrator.cpp	Sanitizes source-relative paths.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Copilot review overview

🟡 Changes recommended

Length validation rejects schema-valid manifests, while trailing-dot and trailing-space components can still alias filesystem paths.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 · 2

Open (3)

• Sanitize trailing spaces in path components · New
• Use schema-aware Unicode length validation
• Validate package path components beyond traversal checks

Resolved since last review (2)

• Recognize superscript Windows reserved device names
• Correct the typo in identifier

[ranm-msft]

I re-read this at the current head against my two earlier review rounds, and all four of the points I raised have landed:

• Reserved DOS device names including an extension (NUL.txt) are now rejected, routed through MakeSuitablePathPart and surfaced as ReservedPathName.
• The trailing-dot case that only becomes visible after trailing spaces are trimmed is handled in the same helper.
• Embedded NUL and control characters are rejected, with explicit test coverage.
• The REST trailing-space case is fixed at the source: both search and manifest deserialization trim Id and Version before validation and storage.

I also spot-checked that the current filesystem construction sites go through GetPathPart / MakeSuitablePathPart rather than rebuilding the component ad hoc, which was the underlying concern behind my comments. I have no remaining path-safety findings.

I am holding my approval only because the last domain-owner review predates the most recent commits on this path-hardening change - not because of anything I found. Thanks for working through all of it.