From a4eca5010a76267de1e7194116d896207b18bcbe Mon Sep 17 00:00:00 2001 From: JohnMcPMS Date: Fri, 18 Sep 2026 11:25:41 -0700 Subject: [PATCH 01/14] first draft of enforcing path fields --- .../ContextOrchestrator.cpp | 9 ++- .../Workflows/DownloadFlow.cpp | 8 ++- .../ShellExecuteInstallerHandler.cpp | 6 +- src/AppInstallerCLITests/Filesystem.cpp | 12 ++++ src/AppInstallerCLITests/YamlManifest.cpp | 72 +++++++++++++++++++ src/AppInstallerCommonCore/Fonts.cpp | 13 ++-- .../Manifest/ManifestValidation.cpp | 59 +++++++++++++++ .../Manifest/YamlParser.cpp | 10 +++ .../Public/winget/ManifestValidation.h | 10 +++ .../Json/SearchResponseDeserializer_1_0.cpp | 29 ++++++++ src/AppInstallerSharedLib/Filesystem.cpp | 10 +++ .../Public/winget/Filesystem.h | 5 ++ .../Common/ManifestErrorId.cs | 9 +++ 13 files changed, 244 insertions(+), 8 deletions(-) diff --git a/src/AppInstallerCLICore/ContextOrchestrator.cpp b/src/AppInstallerCLICore/ContextOrchestrator.cpp index c2f48d2765..23e3d3e90e 100644 --- a/src/AppInstallerCLICore/ContextOrchestrator.cpp +++ b/src/AppInstallerCLICore/ContextOrchestrator.cpp @@ -7,6 +7,7 @@ #include "Commands/COMCommand.h" #include "Public/ShutdownMonitoring.h" #include "winget/UserSettings.h" +#include #include namespace AppInstaller::CLI::Execution @@ -144,7 +145,9 @@ namespace AppInstaller::CLI::Execution if (queueItem.IsApplicableForInstallingSource()) { const auto& manifest = queueItem.GetContext().Get(); - m_installingWriteableSource.AddPackageVersion(manifest, std::filesystem::path{ manifest.Id + '.' + manifest.Version }); + std::string relativePath = manifest.Id + '.' + manifest.Version; + Filesystem::ThrowIfPathEscapesBaseDirectory(relativePath); + m_installingWriteableSource.AddPackageVersion(manifest, std::filesystem::path{ relativePath }); } } @@ -153,7 +156,9 @@ namespace AppInstaller::CLI::Execution if (queueItem.IsApplicableForInstallingSource()) { const auto& manifest = queueItem.GetContext().Get(); - m_installingWriteableSource.RemovePackageVersion(manifest, std::filesystem::path{ manifest.Id + '.' + manifest.Version }); + std::string relativePath = manifest.Id + '.' + manifest.Version; + Filesystem::ThrowIfPathEscapesBaseDirectory(relativePath); + m_installingWriteableSource.RemovePackageVersion(manifest, std::filesystem::path{ relativePath }); } } diff --git a/src/AppInstallerCLICore/Workflows/DownloadFlow.cpp b/src/AppInstallerCLICore/Workflows/DownloadFlow.cpp index cd24892c44..27e8c0d7ed 100644 --- a/src/AppInstallerCLICore/Workflows/DownloadFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/DownloadFlow.cpp @@ -35,8 +35,11 @@ namespace AppInstaller::CLI::Workflow { const auto& manifest = context.Get(); + std::string pathPart = manifest.Id + '.' + manifest.Version; + Filesystem::ThrowIfPathEscapesBaseDirectory(pathPart); + std::filesystem::path tempInstallerPath = Runtime::GetPathTo(Runtime::PathName::Temp); - tempInstallerPath /= Utility::ConvertToUTF16(manifest.Id + '.' + manifest.Version); + tempInstallerPath /= Utility::ConvertToUTF16(pathPart); std::filesystem::create_directories(tempInstallerPath); @@ -754,6 +757,9 @@ namespace AppInstaller::CLI::Workflow { packageDownloadFolderName += '_' + manifest.Version; } + + Filesystem::ThrowIfPathEscapesBaseDirectory(packageDownloadFolderName); + context.Add(downloadsDirectory / Utility::ConvertToUTF16(packageDownloadFolderName)); } } diff --git a/src/AppInstallerCLICore/Workflows/ShellExecuteInstallerHandler.cpp b/src/AppInstallerCLICore/Workflows/ShellExecuteInstallerHandler.cpp index 7686310ef6..b2fca6ef35 100644 --- a/src/AppInstallerCLICore/Workflows/ShellExecuteInstallerHandler.cpp +++ b/src/AppInstallerCLICore/Workflows/ShellExecuteInstallerHandler.cpp @@ -179,7 +179,11 @@ namespace AppInstaller::CLI::Workflow case Logging::LogNameStrategy::Manifest: // Use manifest ID and version for log file name // Results in \.-.log - path /= Utility::ConvertToUTF16(manifest.Id + '.' + manifest.Version); + { + std::string logFileNamePart = manifest.Id + '.' + manifest.Version; + Filesystem::ThrowIfPathEscapesBaseDirectory(logFileNamePart); + path /= Utility::ConvertToUTF16(logFileNamePart); + } path += '-'; path += Utility::GetCurrentTimeForFilename(true); break; diff --git a/src/AppInstallerCLITests/Filesystem.cpp b/src/AppInstallerCLITests/Filesystem.cpp index 95ea62c493..c1d8a2d987 100644 --- a/src/AppInstallerCLITests/Filesystem.cpp +++ b/src/AppInstallerCLITests/Filesystem.cpp @@ -2,6 +2,7 @@ // Licensed under the MIT License. #include "pch.h" #include "TestCommon.h" +#include #include #include #include @@ -77,6 +78,17 @@ TEST_CASE("PathEscapesDirectory", "[filesystem]") } } +TEST_CASE("ThrowIfPathEscapesDirectory", "[filesystem]") +{ + REQUIRE_NOTHROW(ThrowIfPathEscapesBaseDirectory("target.exe")); + REQUIRE_NOTHROW(ThrowIfPathEscapesBaseDirectory("test\\subdir\\target.exe")); + REQUIRE_NOTHROW(ThrowIfPathEscapesBaseDirectory("")); + + REQUIRE_THROWS_HR(ThrowIfPathEscapesBaseDirectory(".."), APPINSTALLER_CLI_ERROR_INVALID_MANIFEST); + REQUIRE_THROWS_HR(ThrowIfPathEscapesBaseDirectory("..\\..\\target.exe"), APPINSTALLER_CLI_ERROR_INVALID_MANIFEST); + REQUIRE_THROWS_HR(ThrowIfPathEscapesBaseDirectory("C:\\Windows\\target.exe"), APPINSTALLER_CLI_ERROR_INVALID_MANIFEST); +} + TEST_CASE("VerifySymlink", "[filesystem]") { TestCommon::TempDirectory tempDirectory("TempDirectory"); diff --git a/src/AppInstallerCLITests/YamlManifest.cpp b/src/AppInstallerCLITests/YamlManifest.cpp index 796c4d15fa..ca150889d7 100644 --- a/src/AppInstallerCLITests/YamlManifest.cpp +++ b/src/AppInstallerCLITests/YamlManifest.cpp @@ -1398,6 +1398,78 @@ TEST_CASE("ManifestLocalizationValidation", "[ManifestValidation]") REQUIRE(errors.at(0).ErrorLevel == ValidationError::Level::Warning); } +TEST_CASE("PathFieldValueValidation", "[ManifestValidation]") +{ + auto RequireSingleError = [](const std::vector& errors, AppInstaller::StringResource::StringId message) + { + REQUIRE(errors.size() == 1); + REQUIRE(ValidationError::Level::Error == errors[0].ErrorLevel); + REQUIRE(message == errors[0].Message); + }; + + auto ContainsError = [](const std::vector& errors, AppInstaller::StringResource::StringId message) + { + return std::any_of(errors.begin(), errors.end(), [&](const ValidationError& error) + { + return error.Message == message && error.ErrorLevel == ValidationError::Level::Error; + }); + }; + + // Valid values produce no errors. + REQUIRE(ValidatePathFieldValue("PackageVersion", "1.0.0").empty()); + REQUIRE(ValidatePathFieldValue("PackageVersion", "1.0 beta").empty()); + REQUIRE(ValidatePathFieldValue("PackageIdentifier", "Foo.Bar", true).empty()); + REQUIRE(ValidatePathFieldValue("PackageIdentifier", "Foo.Bar.Baz.Qux", true).empty()); + + // Empty values are covered by the required field validation. + REQUIRE(ValidatePathFieldValue("PackageVersion", "").empty()); + + // Characters excluded by the schema because the values are used to construct paths. + for (const auto& value : { "ab\\c", "ab/c", "ab:c", "ab*c", "ab?c", "ab\"c", "abc", "ab|c", "ab\tc" }) + { + REQUIRE(ContainsError(ValidatePathFieldValue("PackageVersion", value), ManifestError::InvalidPathCharacters)); + } + + // Whitespace is only excluded for the fields that require it. + auto errors = ValidatePathFieldValue("PackageIdentifier", "Foo Bar", true); + REQUIRE(errors.size() == 1); + ValidateError(errors[0], ValidationError::Level::Error, ManifestError::InvalidPathCharacters, "PackageIdentifier", "Foo Bar"); + + // Values that exceed the maximum length declared by the schema. + RequireSingleError(ValidatePathFieldValue("PackageVersion", std::string(129, '1')), ManifestError::FieldExceedsMaxLength); + + // Values consisting solely of relative path specifiers. + RequireSingleError(ValidatePathFieldValue("PackageVersion", ".."), ManifestError::FieldEscapesDirectory); + REQUIRE(ContainsError(ValidatePathFieldValue("PackageVersion", "..\\.."), ManifestError::FieldEscapesDirectory)); +} + +TEST_CASE("PackageIdentifierAndVersionPathValidation", "[ManifestValidation]") +{ + Manifest manifest = YamlParser::CreateFromPath(TestDataFile("Manifest-Good-InstallerTypeZip-PortableExeUppercase.yaml")); + + // A valid manifest has no path related errors. + REQUIRE(ValidateManifest(manifest, false).size() == 0); + + // These are enforced regardless of the full validation option, as manifests that are not validated + // against the schema (for example, those from a REST source) are only checked here. + manifest.Id = "Foo\\Bar"; + auto errors = ValidateManifest(manifest, false); + REQUIRE(errors.size() == 1); + ValidateError(errors[0], ValidationError::Level::Error, ManifestError::InvalidPathCharacters, "PackageIdentifier", manifest.Id); + + manifest = YamlParser::CreateFromPath(TestDataFile("Manifest-Good-InstallerTypeZip-PortableExeUppercase.yaml")); + manifest.Version = "1.0:0"; + errors = ValidateManifest(manifest, false); + REQUIRE(errors.size() == 1); + ValidateError(errors[0], ValidationError::Level::Error, ManifestError::InvalidPathCharacters, "PackageVersion", manifest.Version); + + manifest = YamlParser::CreateFromPath(TestDataFile("Manifest-Good-InstallerTypeZip-PortableExeUppercase.yaml")); + manifest.Version = ".."; + errors = ValidateManifest(manifest, false); + REQUIRE(errors.size() == 1); + ValidateError(errors[0], ValidationError::Level::Error, ManifestError::FieldEscapesDirectory, "PackageVersion", manifest.Version); +} + TEST_CASE("PortableFileTypeValidation", "[ManifestValidation]") { Manifest installerManifest = YamlParser::CreateFromPath(TestDataFile("Manifest-Bad-InstallerTypeZip-PortableNotExe.yaml")); diff --git a/src/AppInstallerCommonCore/Fonts.cpp b/src/AppInstallerCommonCore/Fonts.cpp index c28cf136c7..bda0bb0a61 100644 --- a/src/AppInstallerCommonCore/Fonts.cpp +++ b/src/AppInstallerCommonCore/Fonts.cpp @@ -54,13 +54,17 @@ namespace AppInstaller::Fonts void AssertPackageInformation(const FontContext& context) { - if (!context.PackageId.empty() && !context.PackageVersion.empty()) + if (context.PackageId.empty() || context.PackageVersion.empty()) { - return; + // This is a programming error if we reach this point where the package identifer cannot be created or derived. + THROW_HR_MSG(E_UNEXPECTED, "Package Id and Version must be provided and non-empty."); } - // This is a programming error if we reach this point where the package identifer cannot be created or derived. - THROW_HR_MSG(E_UNEXPECTED, "Package Id and Version must be provided and non-empty."); + // Defense in depth; the package id and version originate from a manifest and are used to + // construct both file system and registry paths. Manifest validation rejects these values, + // but verify again here as this is the point of use. + Filesystem::ThrowIfPathEscapesBaseDirectory(ConvertToUTF8(context.PackageId)); + Filesystem::ThrowIfPathEscapesBaseDirectory(ConvertToUTF8(context.PackageVersion)); } std::wstring GetFontRegistryPath(const FontContext& context) @@ -76,6 +80,7 @@ namespace AppInstaller::Fonts break; case InstallerSource::WinGet: // WinGet path adds the WinGet prefix + package id + version. + AssertPackageInformation(context); path << s_Separator << s_FontsWinGetPrefix << s_Separator << context.PackageId << s_Separator << context.PackageVersion; break; diff --git a/src/AppInstallerCommonCore/Manifest/ManifestValidation.cpp b/src/AppInstallerCommonCore/Manifest/ManifestValidation.cpp index bfe866f0ed..900d4fc807 100644 --- a/src/AppInstallerCommonCore/Manifest/ManifestValidation.cpp +++ b/src/AppInstallerCommonCore/Manifest/ManifestValidation.cpp @@ -94,6 +94,9 @@ namespace AppInstaller::Manifest { AppInstaller::Manifest::ManifestError::BlockedMsiProperty, "Contains a blocked MSI property."sv }, { AppInstaller::Manifest::ManifestError::InvalidMsiSwitches, "Contains invalid MSI switches."sv }, { AppInstaller::Manifest::ManifestError::ContainsNetworkAddress, "Installer switch contains network address."sv }, + { AppInstaller::Manifest::ManifestError::InvalidPathCharacters, "The field value contains characters that are not allowed because the value is used to construct a file system path."sv }, + { AppInstaller::Manifest::ManifestError::FieldExceedsMaxLength, "The field value exceeds the maximum allowed length."sv }, + { AppInstaller::Manifest::ManifestError::FieldEscapesDirectory, "The field value must not point to a location outside of its base directory."sv }, }; return ErrorIdToMessageMap; @@ -110,12 +113,26 @@ namespace AppInstaller::Manifest Utility::CaseInsensitiveContainsSubstring(input, "https://") || Utility::CaseInsensitiveContainsSubstring(input, "ftp://"); } + + // The characters excluded by the manifest schema from fields that are used to construct file system paths. + constexpr std::string_view s_InvalidPathFieldCharacters = "\\/:*?\"<>|"sv; + + // The maximum length declared by the manifest schema for fields that are used to construct file system paths. + constexpr size_t s_MaxPathFieldLength = 128; } std::vector ValidateManifest(const Manifest& manifest, const ManifestValidateOption& options) { std::vector resultErrors; + // PackageIdentifier and PackageVersion are used to construct file system paths, so the schema + // restrictions on them must be enforced at runtime for all manifest sources. + auto idErrors = ValidatePathFieldValue("PackageIdentifier", manifest.Id, /* disallowWhitespace */ true); + std::move(idErrors.begin(), idErrors.end(), std::inserter(resultErrors, resultErrors.end())); + + auto versionErrors = ValidatePathFieldValue("PackageVersion", manifest.Version); + std::move(versionErrors.begin(), versionErrors.end(), std::inserter(resultErrors, resultErrors.end())); + // Channel is not supported currently if (!manifest.Channel.empty()) { @@ -591,6 +608,48 @@ namespace AppInstaller::Manifest return errors; } + std::vector ValidatePathFieldValue(std::string_view fieldName, std::string_view value, bool disallowWhitespace) + { + std::vector resultErrors; + + if (value.empty()) + { + return resultErrors; + } + + std::string fieldNameString{ fieldName }; + std::string valueString{ value }; + + if (value.length() > s_MaxPathFieldLength) + { + resultErrors.emplace_back(ManifestError::FieldExceedsMaxLength, fieldNameString, valueString); + } + + for (char character : value) + { + auto rawCharacter = static_cast(character); + + // Control characters, characters that are not valid in a file system path, and (optionally) whitespace. + if ((rawCharacter >= 0x01 && rawCharacter <= 0x1f) || + rawCharacter == 0x7f || + s_InvalidPathFieldCharacters.find(character) != std::string_view::npos || + (disallowWhitespace && rawCharacter == ' ')) + { + resultErrors.emplace_back(ManifestError::InvalidPathCharacters, fieldNameString, valueString); + break; + } + } + + // The character restrictions above prevent traversal using path separators, but the value can still + // consist solely of relative path specifiers (for instance, ".."). Reject those as well. + if (AppInstaller::Filesystem::PathEscapesBaseDirectory(value)) + { + resultErrors.emplace_back(ManifestError::FieldEscapesDirectory, fieldNameString, valueString); + } + + return resultErrors; + } + std::string ValidationError::GetErrorMessage() const { const auto& ErrorIdToMessageMap = GetErrorIdToMessageMap(); diff --git a/src/AppInstallerCommonCore/Manifest/YamlParser.cpp b/src/AppInstallerCommonCore/Manifest/YamlParser.cpp index afedeb17e2..7625a2eeaa 100644 --- a/src/AppInstallerCommonCore/Manifest/YamlParser.cpp +++ b/src/AppInstallerCommonCore/Manifest/YamlParser.cpp @@ -488,6 +488,16 @@ namespace AppInstaller::Manifest::YamlParser std::move(errors.begin(), errors.end(), std::inserter(resultErrors, resultErrors.end())); } } + else + { + // PackageIdentifier and PackageVersion are used to construct file system paths, so the schema + // restrictions on them are enforced even when the full semantic validation is not requested. + errors = ValidatePathFieldValue("PackageIdentifier", manifest.Id, /* disallowWhitespace */ true); + std::move(errors.begin(), errors.end(), std::inserter(resultErrors, resultErrors.end())); + + errors = ValidatePathFieldValue("PackageVersion", manifest.Version); + std::move(errors.begin(), errors.end(), std::inserter(resultErrors, resultErrors.end())); + } if (validateOption.InstallerValidation) { diff --git a/src/AppInstallerCommonCore/Public/winget/ManifestValidation.h b/src/AppInstallerCommonCore/Public/winget/ManifestValidation.h index e530cbef78..b5256798d8 100644 --- a/src/AppInstallerCommonCore/Public/winget/ManifestValidation.h +++ b/src/AppInstallerCommonCore/Public/winget/ManifestValidation.h @@ -41,6 +41,8 @@ namespace AppInstaller::Manifest WINGET_DEFINE_RESOURCE_STRINGID(ExceededNestedInstallerFilesLimit); WINGET_DEFINE_RESOURCE_STRINGID(ExeInstallerMissingSilentSwitches); WINGET_DEFINE_RESOURCE_STRINGID(FieldDuplicate); + WINGET_DEFINE_RESOURCE_STRINGID(FieldEscapesDirectory); + WINGET_DEFINE_RESOURCE_STRINGID(FieldExceedsMaxLength); WINGET_DEFINE_RESOURCE_STRINGID(FieldFailedToProcess); WINGET_DEFINE_RESOURCE_STRINGID(FieldIsNotPascalCase); WINGET_DEFINE_RESOURCE_STRINGID(FieldNotSupported); @@ -60,6 +62,7 @@ namespace AppInstaller::Manifest WINGET_DEFINE_RESOURCE_STRINGID(InvalidBcp47Value); WINGET_DEFINE_RESOURCE_STRINGID(InvalidFieldValue); WINGET_DEFINE_RESOURCE_STRINGID(InvalidMsiSwitches); + WINGET_DEFINE_RESOURCE_STRINGID(InvalidPathCharacters); WINGET_DEFINE_RESOURCE_STRINGID(InvalidRootNode); WINGET_DEFINE_RESOURCE_STRINGID(InvalidWindowsFeatureName); WINGET_DEFINE_RESOURCE_STRINGID(MissingManifestDependenciesNode); @@ -223,4 +226,11 @@ namespace AppInstaller::Manifest std::vector ValidateManifest(const Manifest& manifest, const ManifestValidateOption& options); std::vector ValidateManifestLocalization(const ManifestLocalization& localization, bool treatErrorAsWarning = false); std::vector ValidateManifestInstallers(const Manifest& manifest, bool treatErrorAsWarning = false); + + // Validates a manifest field value that is used to construct file system paths. + // The manifest schemas restrict these fields to values that are safe to use as a path part, but the + // schema is not applied at runtime for all manifest sources (for example, REST sources), so the + // restrictions are enforced here as well. + // disallowWhitespace: set for fields whose schema definition excludes whitespace (for example, PackageIdentifier). + std::vector ValidatePathFieldValue(std::string_view fieldName, std::string_view value, bool disallowWhitespace = false); } diff --git a/src/AppInstallerRepositoryCore/Rest/Schema/1_0/Json/SearchResponseDeserializer_1_0.cpp b/src/AppInstallerRepositoryCore/Rest/Schema/1_0/Json/SearchResponseDeserializer_1_0.cpp index 378464f628..10d6f126f4 100644 --- a/src/AppInstallerRepositoryCore/Rest/Schema/1_0/Json/SearchResponseDeserializer_1_0.cpp +++ b/src/AppInstallerRepositoryCore/Rest/Schema/1_0/Json/SearchResponseDeserializer_1_0.cpp @@ -5,6 +5,7 @@ #include "Rest/Schema/IRestClient.h" #include "SearchResponseDeserializer.h" #include +#include #include namespace AppInstaller::Repository::Rest::Schema::V1_0::Json @@ -20,6 +21,24 @@ namespace AppInstaller::Repository::Rest::Schema::V1_0::Json constexpr std::string_view Versions = "Versions"sv; constexpr std::string_view PackageVersion = "PackageVersion"sv; constexpr std::string_view Channel = "Channel"sv; + + // The package identifier and version flow into file system paths, so the manifest schema restrictions + // on them are enforced here as well; the schema itself is not applied to REST responses. + bool IsValidPathFieldValue(std::string_view fieldName, std::string_view value, bool disallowWhitespace = false) + { + bool result = true; + + for (const auto& error : AppInstaller::Manifest::ValidatePathFieldValue(fieldName, value, disallowWhitespace)) + { + if (error.ErrorLevel == AppInstaller::Manifest::ValidationError::Level::Error) + { + AICLI_LOG(Repo, Error, << "Invalid " << fieldName << " received from rest source: " << error.GetErrorMessage()); + result = false; + } + } + + return result; + } } IRestClient::SearchResult SearchResponseDeserializer::Deserialize(const web::json::value& searchResponseObject) const @@ -62,6 +81,11 @@ namespace AppInstaller::Repository::Rest::Schema::V1_0::Json return {}; } + if (!IsValidPathFieldValue(PackageIdentifier, packageId.value(), /* disallowWhitespace */ true)) + { + return {}; + } + std::optional> versionValue = JSON::GetRawJsonArrayFromJsonNode(manifestItem, JSON::GetUtilityString(Versions)); std::vector versionList; @@ -115,6 +139,11 @@ namespace AppInstaller::Repository::Rest::Schema::V1_0::Json return {}; } + if (!IsValidPathFieldValue(PackageVersion, version.value())) + { + return {}; + } + std::string channel = JSON::GetRawStringValueFromJsonNode(versionInfoJsonObject, JSON::GetUtilityString(Channel)).value_or(""); std::vector packageFamilyNames = AppInstaller::Rest::GetUniqueItems(JSON::GetRawStringArrayFromJsonNode(versionInfoJsonObject, JSON::GetUtilityString(PackageFamilyNames))); std::vector productCodes = AppInstaller::Rest::GetUniqueItems(JSON::GetRawStringArrayFromJsonNode(versionInfoJsonObject, JSON::GetUtilityString(ProductCodes))); diff --git a/src/AppInstallerSharedLib/Filesystem.cpp b/src/AppInstallerSharedLib/Filesystem.cpp index 90394c6824..9a5135b545 100644 --- a/src/AppInstallerSharedLib/Filesystem.cpp +++ b/src/AppInstallerSharedLib/Filesystem.cpp @@ -2,6 +2,7 @@ // Licensed under the MIT License. #include "pch.h" #include "Public/winget/Filesystem.h" +#include "Public/AppInstallerErrors.h" #include "Public/AppInstallerStrings.h" #include "Public/AppInstallerLogging.h" #include "Public/winget/Runtime.h" @@ -393,6 +394,15 @@ namespace AppInstaller::Filesystem return false; } + void ThrowIfPathEscapesBaseDirectory(std::string_view relativePath) + { + if (PathEscapesBaseDirectory(relativePath)) + { + AICLI_LOG(Core, Error, << "Path part points to a location outside of its base directory: " << relativePath); + THROW_HR(APPINSTALLER_CLI_ERROR_INVALID_MANIFEST); + } + } + // Complicated rename algorithm due to somewhat arbitrary failures. // 1. First, try to rename. // 2. Then, create an empty file for the target, and attempt to rename. diff --git a/src/AppInstallerSharedLib/Public/winget/Filesystem.h b/src/AppInstallerSharedLib/Public/winget/Filesystem.h index f450444234..bd643626f7 100644 --- a/src/AppInstallerSharedLib/Public/winget/Filesystem.h +++ b/src/AppInstallerSharedLib/Public/winget/Filesystem.h @@ -24,6 +24,11 @@ namespace AppInstaller::Filesystem // Checks if a relative paths points to a location outside of the base path. bool PathEscapesBaseDirectory(std::string_view relativePath); + // Throws if a relative path points to a location outside of the base path. + // This is intended as a defense in depth check for path parts that originate from external data, + // such as manifest fields, that should already have been rejected by validation. + void ThrowIfPathEscapesBaseDirectory(std::string_view relativePath); + // Renames the file to a new path. void RenameFile(const std::filesystem::path& from, const std::filesystem::path& to); diff --git a/src/WinGetUtilInterop/Common/ManifestErrorId.cs b/src/WinGetUtilInterop/Common/ManifestErrorId.cs index 433e35f698..1b3b0a0844 100644 --- a/src/WinGetUtilInterop/Common/ManifestErrorId.cs +++ b/src/WinGetUtilInterop/Common/ManifestErrorId.cs @@ -78,6 +78,12 @@ public enum ManifestErrorId /// Duplicate field found in the manifest. FieldDuplicate, + /// The field value must not point to a location outside of its base directory. + FieldEscapesDirectory, + + /// The field value exceeds the maximum allowed length. + FieldExceedsMaxLength, + /// Failed to process field. FieldFailedToProcess, @@ -135,6 +141,9 @@ public enum ManifestErrorId /// Contains invalid MSI switches. InvalidMsiSwitches, + /// The field value contains characters that are not allowed because the value is used to construct a file system path. + InvalidPathCharacters, + /// Encountered unexpected root node. InvalidRootNode, From 09f0c90f940c55046cd042f5a306f636f96d48fd Mon Sep 17 00:00:00 2001 From: JohnMcPMS Date: Fri, 18 Sep 2026 11:35:38 -0700 Subject: [PATCH 02/14] more reuse --- src/AppInstallerCLITests/Filesystem.cpp | 4 + src/AppInstallerCLITests/YamlManifest.cpp | 20 ++-- .../Manifest/ManifestValidation.cpp | 113 ++++++++++-------- .../Manifest/YamlParser.cpp | 5 +- .../Public/winget/ManifestValidation.h | 11 +- .../Json/SearchResponseDeserializer_1_0.cpp | 8 +- src/AppInstallerSharedLib/Filesystem.cpp | 4 +- .../Public/winget/Filesystem.h | 5 +- 8 files changed, 97 insertions(+), 73 deletions(-) diff --git a/src/AppInstallerCLITests/Filesystem.cpp b/src/AppInstallerCLITests/Filesystem.cpp index c1d8a2d987..8d3c14a7cb 100644 --- a/src/AppInstallerCLITests/Filesystem.cpp +++ b/src/AppInstallerCLITests/Filesystem.cpp @@ -87,6 +87,10 @@ TEST_CASE("ThrowIfPathEscapesDirectory", "[filesystem]") REQUIRE_THROWS_HR(ThrowIfPathEscapesBaseDirectory(".."), APPINSTALLER_CLI_ERROR_INVALID_MANIFEST); REQUIRE_THROWS_HR(ThrowIfPathEscapesBaseDirectory("..\\..\\target.exe"), APPINSTALLER_CLI_ERROR_INVALID_MANIFEST); REQUIRE_THROWS_HR(ThrowIfPathEscapesBaseDirectory("C:\\Windows\\target.exe"), APPINSTALLER_CLI_ERROR_INVALID_MANIFEST); + + // The error to throw can be overridden by the caller. + REQUIRE_THROWS_HR(ThrowIfPathEscapesBaseDirectory("..", E_INVALIDARG), E_INVALIDARG); + REQUIRE_NOTHROW(ThrowIfPathEscapesBaseDirectory("target.exe", E_INVALIDARG)); } TEST_CASE("VerifySymlink", "[filesystem]") diff --git a/src/AppInstallerCLITests/YamlManifest.cpp b/src/AppInstallerCLITests/YamlManifest.cpp index ca150889d7..8886738b00 100644 --- a/src/AppInstallerCLITests/YamlManifest.cpp +++ b/src/AppInstallerCLITests/YamlManifest.cpp @@ -1416,31 +1416,31 @@ TEST_CASE("PathFieldValueValidation", "[ManifestValidation]") }; // Valid values produce no errors. - REQUIRE(ValidatePathFieldValue("PackageVersion", "1.0.0").empty()); - REQUIRE(ValidatePathFieldValue("PackageVersion", "1.0 beta").empty()); - REQUIRE(ValidatePathFieldValue("PackageIdentifier", "Foo.Bar", true).empty()); - REQUIRE(ValidatePathFieldValue("PackageIdentifier", "Foo.Bar.Baz.Qux", true).empty()); + REQUIRE(ValidatePackageVersion("1.0.0").empty()); + REQUIRE(ValidatePackageVersion("1.0 beta").empty()); + REQUIRE(ValidatePackageIdentifier("Foo.Bar").empty()); + REQUIRE(ValidatePackageIdentifier("Foo.Bar.Baz.Qux").empty()); // Empty values are covered by the required field validation. - REQUIRE(ValidatePathFieldValue("PackageVersion", "").empty()); + REQUIRE(ValidatePackageVersion("").empty()); // Characters excluded by the schema because the values are used to construct paths. for (const auto& value : { "ab\\c", "ab/c", "ab:c", "ab*c", "ab?c", "ab\"c", "abc", "ab|c", "ab\tc" }) { - REQUIRE(ContainsError(ValidatePathFieldValue("PackageVersion", value), ManifestError::InvalidPathCharacters)); + REQUIRE(ContainsError(ValidatePackageVersion(value), ManifestError::InvalidPathCharacters)); } // Whitespace is only excluded for the fields that require it. - auto errors = ValidatePathFieldValue("PackageIdentifier", "Foo Bar", true); + auto errors = ValidatePackageIdentifier("Foo Bar"); REQUIRE(errors.size() == 1); ValidateError(errors[0], ValidationError::Level::Error, ManifestError::InvalidPathCharacters, "PackageIdentifier", "Foo Bar"); // Values that exceed the maximum length declared by the schema. - RequireSingleError(ValidatePathFieldValue("PackageVersion", std::string(129, '1')), ManifestError::FieldExceedsMaxLength); + RequireSingleError(ValidatePackageVersion(std::string(129, '1')), ManifestError::FieldExceedsMaxLength); // Values consisting solely of relative path specifiers. - RequireSingleError(ValidatePathFieldValue("PackageVersion", ".."), ManifestError::FieldEscapesDirectory); - REQUIRE(ContainsError(ValidatePathFieldValue("PackageVersion", "..\\.."), ManifestError::FieldEscapesDirectory)); + RequireSingleError(ValidatePackageVersion(".."), ManifestError::FieldEscapesDirectory); + REQUIRE(ContainsError(ValidatePackageVersion("..\\.."), ManifestError::FieldEscapesDirectory)); } TEST_CASE("PackageIdentifierAndVersionPathValidation", "[ManifestValidation]") diff --git a/src/AppInstallerCommonCore/Manifest/ManifestValidation.cpp b/src/AppInstallerCommonCore/Manifest/ManifestValidation.cpp index 900d4fc807..7cf5e6acac 100644 --- a/src/AppInstallerCommonCore/Manifest/ManifestValidation.cpp +++ b/src/AppInstallerCommonCore/Manifest/ManifestValidation.cpp @@ -119,6 +119,70 @@ namespace AppInstaller::Manifest // The maximum length declared by the manifest schema for fields that are used to construct file system paths. constexpr size_t s_MaxPathFieldLength = 128; + + // Validates a manifest field value that is used to construct file system paths. + // disallowWhitespace: set for fields whose schema definition excludes whitespace (for example, PackageIdentifier). + std::vector ValidatePathFieldValue(std::string_view fieldName, std::string_view value, bool disallowWhitespace) + { + std::vector resultErrors; + + if (value.empty()) + { + return resultErrors; + } + + std::string fieldNameString{ fieldName }; + std::string valueString{ value }; + + if (value.length() > s_MaxPathFieldLength) + { + resultErrors.emplace_back(ManifestError::FieldExceedsMaxLength, fieldNameString, valueString); + } + + for (char character : value) + { + auto rawCharacter = static_cast(character); + + // Control characters, characters that are not valid in a file system path, and (optionally) whitespace. + if ((rawCharacter >= 0x01 && rawCharacter <= 0x1f) || + rawCharacter == 0x7f || + s_InvalidPathFieldCharacters.find(character) != std::string_view::npos || + (disallowWhitespace && rawCharacter == ' ')) + { + resultErrors.emplace_back(ManifestError::InvalidPathCharacters, fieldNameString, valueString); + break; + } + } + + // The character restrictions above prevent traversal using path separators, but the value can still + // consist solely of relative path specifiers (for instance, ".."). Reject those as well. + if (AppInstaller::Filesystem::PathEscapesBaseDirectory(value)) + { + resultErrors.emplace_back(ManifestError::FieldEscapesDirectory, fieldNameString, valueString); + } + + return resultErrors; + } + } + + std::vector ValidatePackageIdentifier(std::string_view value) + { + return ValidatePathFieldValue("PackageIdentifier", value, /* disallowWhitespace */ true); + } + + std::vector ValidatePackageVersion(std::string_view value) + { + return ValidatePathFieldValue("PackageVersion", value, /* disallowWhitespace */ false); + } + + std::vector ValidatePathFields(const Manifest& manifest) + { + std::vector resultErrors = ValidatePackageIdentifier(manifest.Id); + + auto versionErrors = ValidatePackageVersion(manifest.Version); + std::move(versionErrors.begin(), versionErrors.end(), std::inserter(resultErrors, resultErrors.end())); + + return resultErrors; } std::vector ValidateManifest(const Manifest& manifest, const ManifestValidateOption& options) @@ -127,11 +191,8 @@ namespace AppInstaller::Manifest // PackageIdentifier and PackageVersion are used to construct file system paths, so the schema // restrictions on them must be enforced at runtime for all manifest sources. - auto idErrors = ValidatePathFieldValue("PackageIdentifier", manifest.Id, /* disallowWhitespace */ true); - std::move(idErrors.begin(), idErrors.end(), std::inserter(resultErrors, resultErrors.end())); - - auto versionErrors = ValidatePathFieldValue("PackageVersion", manifest.Version); - std::move(versionErrors.begin(), versionErrors.end(), std::inserter(resultErrors, resultErrors.end())); + auto pathFieldErrors = ValidatePathFields(manifest); + std::move(pathFieldErrors.begin(), pathFieldErrors.end(), std::inserter(resultErrors, resultErrors.end())); // Channel is not supported currently if (!manifest.Channel.empty()) @@ -608,48 +669,6 @@ namespace AppInstaller::Manifest return errors; } - std::vector ValidatePathFieldValue(std::string_view fieldName, std::string_view value, bool disallowWhitespace) - { - std::vector resultErrors; - - if (value.empty()) - { - return resultErrors; - } - - std::string fieldNameString{ fieldName }; - std::string valueString{ value }; - - if (value.length() > s_MaxPathFieldLength) - { - resultErrors.emplace_back(ManifestError::FieldExceedsMaxLength, fieldNameString, valueString); - } - - for (char character : value) - { - auto rawCharacter = static_cast(character); - - // Control characters, characters that are not valid in a file system path, and (optionally) whitespace. - if ((rawCharacter >= 0x01 && rawCharacter <= 0x1f) || - rawCharacter == 0x7f || - s_InvalidPathFieldCharacters.find(character) != std::string_view::npos || - (disallowWhitespace && rawCharacter == ' ')) - { - resultErrors.emplace_back(ManifestError::InvalidPathCharacters, fieldNameString, valueString); - break; - } - } - - // The character restrictions above prevent traversal using path separators, but the value can still - // consist solely of relative path specifiers (for instance, ".."). Reject those as well. - if (AppInstaller::Filesystem::PathEscapesBaseDirectory(value)) - { - resultErrors.emplace_back(ManifestError::FieldEscapesDirectory, fieldNameString, valueString); - } - - return resultErrors; - } - std::string ValidationError::GetErrorMessage() const { const auto& ErrorIdToMessageMap = GetErrorIdToMessageMap(); diff --git a/src/AppInstallerCommonCore/Manifest/YamlParser.cpp b/src/AppInstallerCommonCore/Manifest/YamlParser.cpp index 7625a2eeaa..e651c14da9 100644 --- a/src/AppInstallerCommonCore/Manifest/YamlParser.cpp +++ b/src/AppInstallerCommonCore/Manifest/YamlParser.cpp @@ -492,10 +492,7 @@ namespace AppInstaller::Manifest::YamlParser { // PackageIdentifier and PackageVersion are used to construct file system paths, so the schema // restrictions on them are enforced even when the full semantic validation is not requested. - errors = ValidatePathFieldValue("PackageIdentifier", manifest.Id, /* disallowWhitespace */ true); - std::move(errors.begin(), errors.end(), std::inserter(resultErrors, resultErrors.end())); - - errors = ValidatePathFieldValue("PackageVersion", manifest.Version); + errors = ValidatePathFields(manifest); std::move(errors.begin(), errors.end(), std::inserter(resultErrors, resultErrors.end())); } diff --git a/src/AppInstallerCommonCore/Public/winget/ManifestValidation.h b/src/AppInstallerCommonCore/Public/winget/ManifestValidation.h index b5256798d8..334730c69e 100644 --- a/src/AppInstallerCommonCore/Public/winget/ManifestValidation.h +++ b/src/AppInstallerCommonCore/Public/winget/ManifestValidation.h @@ -227,10 +227,15 @@ namespace AppInstaller::Manifest std::vector ValidateManifestLocalization(const ManifestLocalization& localization, bool treatErrorAsWarning = false); std::vector ValidateManifestInstallers(const Manifest& manifest, bool treatErrorAsWarning = false); - // Validates a manifest field value that is used to construct file system paths. + // Validates the manifest fields that are used to construct file system paths. // The manifest schemas restrict these fields to values that are safe to use as a path part, but the // schema is not applied at runtime for all manifest sources (for example, REST sources), so the // restrictions are enforced here as well. - // disallowWhitespace: set for fields whose schema definition excludes whitespace (for example, PackageIdentifier). - std::vector ValidatePathFieldValue(std::string_view fieldName, std::string_view value, bool disallowWhitespace = false); + std::vector ValidatePathFields(const Manifest& manifest); + + // Validates an individual PackageIdentifier value, for sources that do not produce a full manifest. + std::vector ValidatePackageIdentifier(std::string_view value); + + // Validates an individual PackageVersion value, for sources that do not produce a full manifest. + std::vector ValidatePackageVersion(std::string_view value); } diff --git a/src/AppInstallerRepositoryCore/Rest/Schema/1_0/Json/SearchResponseDeserializer_1_0.cpp b/src/AppInstallerRepositoryCore/Rest/Schema/1_0/Json/SearchResponseDeserializer_1_0.cpp index 10d6f126f4..240ca2f301 100644 --- a/src/AppInstallerRepositoryCore/Rest/Schema/1_0/Json/SearchResponseDeserializer_1_0.cpp +++ b/src/AppInstallerRepositoryCore/Rest/Schema/1_0/Json/SearchResponseDeserializer_1_0.cpp @@ -24,11 +24,11 @@ namespace AppInstaller::Repository::Rest::Schema::V1_0::Json // The package identifier and version flow into file system paths, so the manifest schema restrictions // on them are enforced here as well; the schema itself is not applied to REST responses. - bool IsValidPathFieldValue(std::string_view fieldName, std::string_view value, bool disallowWhitespace = false) + bool IsValidPathFieldValue(std::string_view fieldName, const std::vector& validationErrors) { bool result = true; - for (const auto& error : AppInstaller::Manifest::ValidatePathFieldValue(fieldName, value, disallowWhitespace)) + for (const auto& error : validationErrors) { if (error.ErrorLevel == AppInstaller::Manifest::ValidationError::Level::Error) { @@ -81,7 +81,7 @@ namespace AppInstaller::Repository::Rest::Schema::V1_0::Json return {}; } - if (!IsValidPathFieldValue(PackageIdentifier, packageId.value(), /* disallowWhitespace */ true)) + if (!IsValidPathFieldValue(PackageIdentifier, AppInstaller::Manifest::ValidatePackageIdentifier(packageId.value()))) { return {}; } @@ -139,7 +139,7 @@ namespace AppInstaller::Repository::Rest::Schema::V1_0::Json return {}; } - if (!IsValidPathFieldValue(PackageVersion, version.value())) + if (!IsValidPathFieldValue(PackageVersion, AppInstaller::Manifest::ValidatePackageVersion(version.value()))) { return {}; } diff --git a/src/AppInstallerSharedLib/Filesystem.cpp b/src/AppInstallerSharedLib/Filesystem.cpp index 9a5135b545..2af4fc0665 100644 --- a/src/AppInstallerSharedLib/Filesystem.cpp +++ b/src/AppInstallerSharedLib/Filesystem.cpp @@ -394,12 +394,12 @@ namespace AppInstaller::Filesystem return false; } - void ThrowIfPathEscapesBaseDirectory(std::string_view relativePath) + void ThrowIfPathEscapesBaseDirectory(std::string_view relativePath, HRESULT hr) { if (PathEscapesBaseDirectory(relativePath)) { AICLI_LOG(Core, Error, << "Path part points to a location outside of its base directory: " << relativePath); - THROW_HR(APPINSTALLER_CLI_ERROR_INVALID_MANIFEST); + THROW_HR(hr); } } diff --git a/src/AppInstallerSharedLib/Public/winget/Filesystem.h b/src/AppInstallerSharedLib/Public/winget/Filesystem.h index bd643626f7..802520a4d5 100644 --- a/src/AppInstallerSharedLib/Public/winget/Filesystem.h +++ b/src/AppInstallerSharedLib/Public/winget/Filesystem.h @@ -1,6 +1,7 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. #pragma once +#include #include #include #include @@ -25,9 +26,7 @@ namespace AppInstaller::Filesystem bool PathEscapesBaseDirectory(std::string_view relativePath); // Throws if a relative path points to a location outside of the base path. - // This is intended as a defense in depth check for path parts that originate from external data, - // such as manifest fields, that should already have been rejected by validation. - void ThrowIfPathEscapesBaseDirectory(std::string_view relativePath); + void ThrowIfPathEscapesBaseDirectory(std::string_view relativePath, HRESULT hr = APPINSTALLER_CLI_ERROR_INVALID_MANIFEST); // Renames the file to a new path. void RenameFile(const std::filesystem::path& from, const std::filesystem::path& to); From 05d4df4d57dd42835347fa42076b99edad5da11f Mon Sep 17 00:00:00 2001 From: JohnMcPMS Date: Fri, 18 Sep 2026 14:14:56 -0700 Subject: [PATCH 03/14] Convert helper to macros to preserve error sourcing in logs --- src/AppInstallerCLICore/ContextOrchestrator.cpp | 4 ++-- .../Workflows/DownloadFlow.cpp | 4 ++-- .../Workflows/ShellExecuteInstallerHandler.cpp | 2 +- src/AppInstallerCLITests/Filesystem.cpp | 16 ---------------- src/AppInstallerCommonCore/Fonts.cpp | 7 +++++-- src/AppInstallerSharedLib/Filesystem.cpp | 10 ---------- .../Public/winget/Filesystem.h | 4 ---- 7 files changed, 10 insertions(+), 37 deletions(-) diff --git a/src/AppInstallerCLICore/ContextOrchestrator.cpp b/src/AppInstallerCLICore/ContextOrchestrator.cpp index 23e3d3e90e..90077af5ac 100644 --- a/src/AppInstallerCLICore/ContextOrchestrator.cpp +++ b/src/AppInstallerCLICore/ContextOrchestrator.cpp @@ -146,7 +146,7 @@ namespace AppInstaller::CLI::Execution { const auto& manifest = queueItem.GetContext().Get(); std::string relativePath = manifest.Id + '.' + manifest.Version; - Filesystem::ThrowIfPathEscapesBaseDirectory(relativePath); + THROW_HR_IF_MSG(APPINSTALLER_CLI_ERROR_INVALID_MANIFEST, Filesystem::PathEscapesBaseDirectory(relativePath), "Path part points to a location outside of its base directory: %hs", relativePath.c_str()); m_installingWriteableSource.AddPackageVersion(manifest, std::filesystem::path{ relativePath }); } } @@ -157,7 +157,7 @@ namespace AppInstaller::CLI::Execution { const auto& manifest = queueItem.GetContext().Get(); std::string relativePath = manifest.Id + '.' + manifest.Version; - Filesystem::ThrowIfPathEscapesBaseDirectory(relativePath); + THROW_HR_IF_MSG(APPINSTALLER_CLI_ERROR_INVALID_MANIFEST, Filesystem::PathEscapesBaseDirectory(relativePath), "Path part points to a location outside of its base directory: %hs", relativePath.c_str()); m_installingWriteableSource.RemovePackageVersion(manifest, std::filesystem::path{ relativePath }); } } diff --git a/src/AppInstallerCLICore/Workflows/DownloadFlow.cpp b/src/AppInstallerCLICore/Workflows/DownloadFlow.cpp index 27e8c0d7ed..d9579c5f1f 100644 --- a/src/AppInstallerCLICore/Workflows/DownloadFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/DownloadFlow.cpp @@ -36,7 +36,7 @@ namespace AppInstaller::CLI::Workflow const auto& manifest = context.Get(); std::string pathPart = manifest.Id + '.' + manifest.Version; - Filesystem::ThrowIfPathEscapesBaseDirectory(pathPart); + THROW_HR_IF_MSG(APPINSTALLER_CLI_ERROR_INVALID_MANIFEST, Filesystem::PathEscapesBaseDirectory(pathPart), "Path part points to a location outside of its base directory: %hs", pathPart.c_str()); std::filesystem::path tempInstallerPath = Runtime::GetPathTo(Runtime::PathName::Temp); tempInstallerPath /= Utility::ConvertToUTF16(pathPart); @@ -758,7 +758,7 @@ namespace AppInstaller::CLI::Workflow packageDownloadFolderName += '_' + manifest.Version; } - Filesystem::ThrowIfPathEscapesBaseDirectory(packageDownloadFolderName); + THROW_HR_IF_MSG(APPINSTALLER_CLI_ERROR_INVALID_MANIFEST, Filesystem::PathEscapesBaseDirectory(packageDownloadFolderName), "Path part points to a location outside of its base directory: %hs", packageDownloadFolderName.c_str()); context.Add(downloadsDirectory / Utility::ConvertToUTF16(packageDownloadFolderName)); } diff --git a/src/AppInstallerCLICore/Workflows/ShellExecuteInstallerHandler.cpp b/src/AppInstallerCLICore/Workflows/ShellExecuteInstallerHandler.cpp index b2fca6ef35..2e019c1c64 100644 --- a/src/AppInstallerCLICore/Workflows/ShellExecuteInstallerHandler.cpp +++ b/src/AppInstallerCLICore/Workflows/ShellExecuteInstallerHandler.cpp @@ -181,7 +181,7 @@ namespace AppInstaller::CLI::Workflow // Results in \.-.log { std::string logFileNamePart = manifest.Id + '.' + manifest.Version; - Filesystem::ThrowIfPathEscapesBaseDirectory(logFileNamePart); + THROW_HR_IF_MSG(APPINSTALLER_CLI_ERROR_INVALID_MANIFEST, Filesystem::PathEscapesBaseDirectory(logFileNamePart), "Path part points to a location outside of its base directory: %hs", logFileNamePart.c_str()); path /= Utility::ConvertToUTF16(logFileNamePart); } path += '-'; diff --git a/src/AppInstallerCLITests/Filesystem.cpp b/src/AppInstallerCLITests/Filesystem.cpp index 8d3c14a7cb..95ea62c493 100644 --- a/src/AppInstallerCLITests/Filesystem.cpp +++ b/src/AppInstallerCLITests/Filesystem.cpp @@ -2,7 +2,6 @@ // Licensed under the MIT License. #include "pch.h" #include "TestCommon.h" -#include #include #include #include @@ -78,21 +77,6 @@ TEST_CASE("PathEscapesDirectory", "[filesystem]") } } -TEST_CASE("ThrowIfPathEscapesDirectory", "[filesystem]") -{ - REQUIRE_NOTHROW(ThrowIfPathEscapesBaseDirectory("target.exe")); - REQUIRE_NOTHROW(ThrowIfPathEscapesBaseDirectory("test\\subdir\\target.exe")); - REQUIRE_NOTHROW(ThrowIfPathEscapesBaseDirectory("")); - - REQUIRE_THROWS_HR(ThrowIfPathEscapesBaseDirectory(".."), APPINSTALLER_CLI_ERROR_INVALID_MANIFEST); - REQUIRE_THROWS_HR(ThrowIfPathEscapesBaseDirectory("..\\..\\target.exe"), APPINSTALLER_CLI_ERROR_INVALID_MANIFEST); - REQUIRE_THROWS_HR(ThrowIfPathEscapesBaseDirectory("C:\\Windows\\target.exe"), APPINSTALLER_CLI_ERROR_INVALID_MANIFEST); - - // The error to throw can be overridden by the caller. - REQUIRE_THROWS_HR(ThrowIfPathEscapesBaseDirectory("..", E_INVALIDARG), E_INVALIDARG); - REQUIRE_NOTHROW(ThrowIfPathEscapesBaseDirectory("target.exe", E_INVALIDARG)); -} - TEST_CASE("VerifySymlink", "[filesystem]") { TestCommon::TempDirectory tempDirectory("TempDirectory"); diff --git a/src/AppInstallerCommonCore/Fonts.cpp b/src/AppInstallerCommonCore/Fonts.cpp index bda0bb0a61..48879a1973 100644 --- a/src/AppInstallerCommonCore/Fonts.cpp +++ b/src/AppInstallerCommonCore/Fonts.cpp @@ -63,8 +63,11 @@ namespace AppInstaller::Fonts // Defense in depth; the package id and version originate from a manifest and are used to // construct both file system and registry paths. Manifest validation rejects these values, // but verify again here as this is the point of use. - Filesystem::ThrowIfPathEscapesBaseDirectory(ConvertToUTF8(context.PackageId)); - Filesystem::ThrowIfPathEscapesBaseDirectory(ConvertToUTF8(context.PackageVersion)); + std::string packageId = ConvertToUTF8(context.PackageId); + THROW_HR_IF_MSG(APPINSTALLER_CLI_ERROR_INVALID_MANIFEST, Filesystem::PathEscapesBaseDirectory(packageId), "Path part points to a location outside of its base directory: %hs", packageId.c_str()); + + std::string packageVersion = ConvertToUTF8(context.PackageVersion); + THROW_HR_IF_MSG(APPINSTALLER_CLI_ERROR_INVALID_MANIFEST, Filesystem::PathEscapesBaseDirectory(packageVersion), "Path part points to a location outside of its base directory: %hs", packageVersion.c_str()); } std::wstring GetFontRegistryPath(const FontContext& context) diff --git a/src/AppInstallerSharedLib/Filesystem.cpp b/src/AppInstallerSharedLib/Filesystem.cpp index 2af4fc0665..90394c6824 100644 --- a/src/AppInstallerSharedLib/Filesystem.cpp +++ b/src/AppInstallerSharedLib/Filesystem.cpp @@ -2,7 +2,6 @@ // Licensed under the MIT License. #include "pch.h" #include "Public/winget/Filesystem.h" -#include "Public/AppInstallerErrors.h" #include "Public/AppInstallerStrings.h" #include "Public/AppInstallerLogging.h" #include "Public/winget/Runtime.h" @@ -394,15 +393,6 @@ namespace AppInstaller::Filesystem return false; } - void ThrowIfPathEscapesBaseDirectory(std::string_view relativePath, HRESULT hr) - { - if (PathEscapesBaseDirectory(relativePath)) - { - AICLI_LOG(Core, Error, << "Path part points to a location outside of its base directory: " << relativePath); - THROW_HR(hr); - } - } - // Complicated rename algorithm due to somewhat arbitrary failures. // 1. First, try to rename. // 2. Then, create an empty file for the target, and attempt to rename. diff --git a/src/AppInstallerSharedLib/Public/winget/Filesystem.h b/src/AppInstallerSharedLib/Public/winget/Filesystem.h index 802520a4d5..f450444234 100644 --- a/src/AppInstallerSharedLib/Public/winget/Filesystem.h +++ b/src/AppInstallerSharedLib/Public/winget/Filesystem.h @@ -1,7 +1,6 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. #pragma once -#include #include #include #include @@ -25,9 +24,6 @@ namespace AppInstaller::Filesystem // Checks if a relative paths points to a location outside of the base path. bool PathEscapesBaseDirectory(std::string_view relativePath); - // Throws if a relative path points to a location outside of the base path. - void ThrowIfPathEscapesBaseDirectory(std::string_view relativePath, HRESULT hr = APPINSTALLER_CLI_ERROR_INVALID_MANIFEST); - // Renames the file to a new path. void RenameFile(const std::filesystem::path& from, const std::filesystem::path& to); From 07d7a3dc19609d0230ac7d288d4cb56bdc22d912 Mon Sep 17 00:00:00 2001 From: JohnMcPMS Date: Fri, 18 Sep 2026 14:49:25 -0700 Subject: [PATCH 04/14] move manifest that we now reject out of test source --- .../TestInvalidManifest.yaml | 0 src/AppInstallerCLIE2ETests/ValidateCommand.cs | 2 +- src/AppInstallerCLITests/YamlManifest.cpp | 11 ++++++----- 3 files changed, 7 insertions(+), 6 deletions(-) rename src/AppInstallerCLIE2ETests/TestData/{Manifests => InvalidManifests}/TestInvalidManifest.yaml (100%) diff --git a/src/AppInstallerCLIE2ETests/TestData/Manifests/TestInvalidManifest.yaml b/src/AppInstallerCLIE2ETests/TestData/InvalidManifests/TestInvalidManifest.yaml similarity index 100% rename from src/AppInstallerCLIE2ETests/TestData/Manifests/TestInvalidManifest.yaml rename to src/AppInstallerCLIE2ETests/TestData/InvalidManifests/TestInvalidManifest.yaml diff --git a/src/AppInstallerCLIE2ETests/ValidateCommand.cs b/src/AppInstallerCLIE2ETests/ValidateCommand.cs index eeaef8cd19..45e626a3ab 100644 --- a/src/AppInstallerCLIE2ETests/ValidateCommand.cs +++ b/src/AppInstallerCLIE2ETests/ValidateCommand.cs @@ -42,7 +42,7 @@ public void ValidateManifestWithExtendedCharacter() [Test] public void ValidateInvalidManifest() { - var result = TestCommon.RunAICLICommand("validate", TestCommon.GetTestDataFile("Manifests\\TestInvalidManifest.yaml")); + var result = TestCommon.RunAICLICommand("validate", TestCommon.GetTestDataFile("InvalidManifests\\TestInvalidManifest.yaml")); Assert.That(result.ExitCode, Is.EqualTo(Constants.ErrorCode.ERROR_MANIFEST_VALIDATION_FAILURE)); Assert.That(result.StdOut, Does.Contain("Manifest validation failed.")); } diff --git a/src/AppInstallerCLITests/YamlManifest.cpp b/src/AppInstallerCLITests/YamlManifest.cpp index 8886738b00..a1e9533f40 100644 --- a/src/AppInstallerCLITests/YamlManifest.cpp +++ b/src/AppInstallerCLITests/YamlManifest.cpp @@ -1421,6 +1421,11 @@ TEST_CASE("PathFieldValueValidation", "[ManifestValidation]") REQUIRE(ValidatePackageIdentifier("Foo.Bar").empty()); REQUIRE(ValidatePackageIdentifier("Foo.Bar.Baz.Qux").empty()); + // Whitespace is only excluded for the fields that require it. + auto errors = ValidatePackageIdentifier("Foo Bar"); + REQUIRE(errors.size() == 1); + ValidateError(errors[0], ValidationError::Level::Error, ManifestError::InvalidPathCharacters, "PackageIdentifier", "Foo Bar"); + // Empty values are covered by the required field validation. REQUIRE(ValidatePackageVersion("").empty()); @@ -1428,13 +1433,9 @@ TEST_CASE("PathFieldValueValidation", "[ManifestValidation]") for (const auto& value : { "ab\\c", "ab/c", "ab:c", "ab*c", "ab?c", "ab\"c", "abc", "ab|c", "ab\tc" }) { REQUIRE(ContainsError(ValidatePackageVersion(value), ManifestError::InvalidPathCharacters)); + REQUIRE(ContainsError(ValidatePackageIdentifier(value), ManifestError::InvalidPathCharacters)); } - // Whitespace is only excluded for the fields that require it. - auto errors = ValidatePackageIdentifier("Foo Bar"); - REQUIRE(errors.size() == 1); - ValidateError(errors[0], ValidationError::Level::Error, ManifestError::InvalidPathCharacters, "PackageIdentifier", "Foo Bar"); - // Values that exceed the maximum length declared by the schema. RequireSingleError(ValidatePackageVersion(std::string(129, '1')), ManifestError::FieldExceedsMaxLength); From d346f1e14b2480a896136ddbc13df2dbdf5c5c4d Mon Sep 17 00:00:00 2001 From: JohnMcPMS Date: Mon, 21 Sep 2026 10:11:04 -0700 Subject: [PATCH 05/14] use suitable path function and reject nulls --- .../ContextOrchestrator.cpp | 9 ++--- .../Workflows/DownloadFlow.cpp | 17 +++------- .../ShellExecuteInstallerHandler.cpp | 6 +--- src/AppInstallerCLITests/YamlManifest.cpp | 27 +++++++++++++++ .../Manifest/Manifest.cpp | 33 +++++++++++++++++++ .../Manifest/ManifestValidation.cpp | 6 ++-- .../Public/winget/Manifest.h | 10 ++++++ 7 files changed, 82 insertions(+), 26 deletions(-) diff --git a/src/AppInstallerCLICore/ContextOrchestrator.cpp b/src/AppInstallerCLICore/ContextOrchestrator.cpp index 90077af5ac..be52f29837 100644 --- a/src/AppInstallerCLICore/ContextOrchestrator.cpp +++ b/src/AppInstallerCLICore/ContextOrchestrator.cpp @@ -7,7 +7,6 @@ #include "Commands/COMCommand.h" #include "Public/ShutdownMonitoring.h" #include "winget/UserSettings.h" -#include #include namespace AppInstaller::CLI::Execution @@ -145,9 +144,7 @@ namespace AppInstaller::CLI::Execution if (queueItem.IsApplicableForInstallingSource()) { const auto& manifest = queueItem.GetContext().Get(); - std::string relativePath = manifest.Id + '.' + manifest.Version; - THROW_HR_IF_MSG(APPINSTALLER_CLI_ERROR_INVALID_MANIFEST, Filesystem::PathEscapesBaseDirectory(relativePath), "Path part points to a location outside of its base directory: %hs", relativePath.c_str()); - m_installingWriteableSource.AddPackageVersion(manifest, std::filesystem::path{ relativePath }); + m_installingWriteableSource.AddPackageVersion(manifest, Manifest::GetPathPart(manifest)); } } @@ -156,9 +153,7 @@ namespace AppInstaller::CLI::Execution if (queueItem.IsApplicableForInstallingSource()) { const auto& manifest = queueItem.GetContext().Get(); - std::string relativePath = manifest.Id + '.' + manifest.Version; - THROW_HR_IF_MSG(APPINSTALLER_CLI_ERROR_INVALID_MANIFEST, Filesystem::PathEscapesBaseDirectory(relativePath), "Path part points to a location outside of its base directory: %hs", relativePath.c_str()); - m_installingWriteableSource.RemovePackageVersion(manifest, std::filesystem::path{ relativePath }); + m_installingWriteableSource.RemovePackageVersion(manifest, Manifest::GetPathPart(manifest)); } } diff --git a/src/AppInstallerCLICore/Workflows/DownloadFlow.cpp b/src/AppInstallerCLICore/Workflows/DownloadFlow.cpp index d9579c5f1f..3dbcede5ee 100644 --- a/src/AppInstallerCLICore/Workflows/DownloadFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/DownloadFlow.cpp @@ -35,11 +35,8 @@ namespace AppInstaller::CLI::Workflow { const auto& manifest = context.Get(); - std::string pathPart = manifest.Id + '.' + manifest.Version; - THROW_HR_IF_MSG(APPINSTALLER_CLI_ERROR_INVALID_MANIFEST, Filesystem::PathEscapesBaseDirectory(pathPart), "Path part points to a location outside of its base directory: %hs", pathPart.c_str()); - std::filesystem::path tempInstallerPath = Runtime::GetPathTo(Runtime::PathName::Temp); - tempInstallerPath /= Utility::ConvertToUTF16(pathPart); + tempInstallerPath /= GetPathPart(manifest); std::filesystem::create_directories(tempInstallerPath); @@ -752,15 +749,11 @@ namespace AppInstaller::CLI::Workflow } const auto& manifest = context.Get(); - std::string packageDownloadFolderName = manifest.Id; - if (!Utility::Version{ manifest.Version }.IsUnknown()) - { - packageDownloadFolderName += '_' + manifest.Version; - } - - THROW_HR_IF_MSG(APPINSTALLER_CLI_ERROR_INVALID_MANIFEST, Filesystem::PathEscapesBaseDirectory(packageDownloadFolderName), "Path part points to a location outside of its base directory: %hs", packageDownloadFolderName.c_str()); + std::filesystem::path packageDownloadFolderName = Utility::Version{ manifest.Version }.IsUnknown() ? + GetPathPart(manifest.Id) : + GetPathPart(manifest, '_'); - context.Add(downloadsDirectory / Utility::ConvertToUTF16(packageDownloadFolderName)); + context.Add(downloadsDirectory / packageDownloadFolderName); } } diff --git a/src/AppInstallerCLICore/Workflows/ShellExecuteInstallerHandler.cpp b/src/AppInstallerCLICore/Workflows/ShellExecuteInstallerHandler.cpp index 2e019c1c64..b4674f7f2f 100644 --- a/src/AppInstallerCLICore/Workflows/ShellExecuteInstallerHandler.cpp +++ b/src/AppInstallerCLICore/Workflows/ShellExecuteInstallerHandler.cpp @@ -179,11 +179,7 @@ namespace AppInstaller::CLI::Workflow case Logging::LogNameStrategy::Manifest: // Use manifest ID and version for log file name // Results in \.-.log - { - std::string logFileNamePart = manifest.Id + '.' + manifest.Version; - THROW_HR_IF_MSG(APPINSTALLER_CLI_ERROR_INVALID_MANIFEST, Filesystem::PathEscapesBaseDirectory(logFileNamePart), "Path part points to a location outside of its base directory: %hs", logFileNamePart.c_str()); - path /= Utility::ConvertToUTF16(logFileNamePart); - } + path /= GetPathPart(manifest); path += '-'; path += Utility::GetCurrentTimeForFilename(true); break; diff --git a/src/AppInstallerCLITests/YamlManifest.cpp b/src/AppInstallerCLITests/YamlManifest.cpp index a1e9533f40..fbb2575a46 100644 --- a/src/AppInstallerCLITests/YamlManifest.cpp +++ b/src/AppInstallerCLITests/YamlManifest.cpp @@ -1436,6 +1436,10 @@ TEST_CASE("PathFieldValueValidation", "[ManifestValidation]") REQUIRE(ContainsError(ValidatePackageIdentifier(value), ManifestError::InvalidPathCharacters)); } + // An embedded null would truncate any path that the value is used in. + RequireSingleError(ValidatePackageVersion("ab\0c"sv), ManifestError::InvalidPathCharacters); + RequireSingleError(ValidatePackageIdentifier("ab\0c"sv), ManifestError::InvalidPathCharacters); + // Values that exceed the maximum length declared by the schema. RequireSingleError(ValidatePackageVersion(std::string(129, '1')), ManifestError::FieldExceedsMaxLength); @@ -1471,6 +1475,29 @@ TEST_CASE("PackageIdentifierAndVersionPathValidation", "[ManifestValidation]") ValidateError(errors[0], ValidationError::Level::Error, ManifestError::FieldEscapesDirectory, "PackageVersion", manifest.Version); } +TEST_CASE("ManifestGetPathPart", "[ManifestValidation]") +{ + Manifest manifest; + manifest.Id = "Foo.Bar"; + manifest.Version = "1.0.0"; + + // The common case must not alter the value, as the resulting paths are persisted. + REQUIRE(GetPathPart(manifest) == std::filesystem::path{ L"Foo.Bar.1.0.0" }); + REQUIRE(GetPathPart(manifest, '_') == std::filesystem::path{ L"Foo.Bar_1.0.0" }); + REQUIRE(GetPathPart(manifest.Id) == std::filesystem::path{ L"Foo.Bar" }); + + // Values that validation would have rejected are sanitized rather than used as given. + REQUIRE(GetPathPart("a\\b") == std::filesystem::path{ L"a_b" }); + REQUIRE(GetPathPart("a/b") == std::filesystem::path{ L"a_b" }); + REQUIRE(GetPathPart("C:") == std::filesystem::path{ L"C_" }); + REQUIRE(GetPathPart("\\\\server\\share") == std::filesystem::path{ L"__server_share" }); + + // Relative path specifiers can never produce a path part that escapes its base directory. + REQUIRE(GetPathPart("..") == std::filesystem::path{ L"._" }); + REQUIRE_THROWS(GetPathPart("..\\..")); + REQUIRE_THROWS(GetPathPart("../../foo")); +} + TEST_CASE("PortableFileTypeValidation", "[ManifestValidation]") { Manifest installerManifest = YamlParser::CreateFromPath(TestDataFile("Manifest-Bad-InstallerTypeZip-PortableNotExe.yaml")); diff --git a/src/AppInstallerCommonCore/Manifest/Manifest.cpp b/src/AppInstallerCommonCore/Manifest/Manifest.cpp index 32612b1147..97be981086 100644 --- a/src/AppInstallerCommonCore/Manifest/Manifest.cpp +++ b/src/AppInstallerCommonCore/Manifest/Manifest.cpp @@ -2,8 +2,10 @@ // Licensed under the MIT License. #include "pch.h" #include "winget/Manifest.h" +#include "winget/Filesystem.h" #include "winget/Locale.h" #include "winget/UserSettings.h" +#include namespace AppInstaller::Manifest { @@ -240,4 +242,35 @@ namespace AppInstaller::Manifest return result; } + + std::filesystem::path GetPathPart(std::string_view value) + { + std::string result; + + try + { + result = Utility::MakeSuitablePathPart(value); + } + catch (...) + { + // MakeSuitablePathPart throws for values that cannot be made into a usable path part, + // such as reserved device names. Surface that as a manifest problem rather than E_INVALIDARG. + THROW_HR_MSG(APPINSTALLER_CLI_ERROR_INVALID_MANIFEST, "Value cannot be used as a path part: %.*hs", static_cast(value.length()), value.data()); + } + + // MakeSuitablePathPart removes all path separators, so this can only fire if it stops doing so. + // It is kept as a final check because the values that reach here originate outside of the client. + THROW_HR_IF_MSG(APPINSTALLER_CLI_ERROR_INVALID_MANIFEST, Filesystem::PathEscapesBaseDirectory(result), "Path part points to a location outside of its base directory: %hs", result.c_str()); + + return { Utility::ConvertToUTF16(result) }; + } + + std::filesystem::path GetPathPart(const Manifest& manifest, char separator) + { + std::string value = manifest.Id; + value += separator; + value += manifest.Version; + + return GetPathPart(value); + } } \ No newline at end of file diff --git a/src/AppInstallerCommonCore/Manifest/ManifestValidation.cpp b/src/AppInstallerCommonCore/Manifest/ManifestValidation.cpp index 7cf5e6acac..255b63ee9a 100644 --- a/src/AppInstallerCommonCore/Manifest/ManifestValidation.cpp +++ b/src/AppInstallerCommonCore/Manifest/ManifestValidation.cpp @@ -143,8 +143,10 @@ namespace AppInstaller::Manifest { auto rawCharacter = static_cast(character); - // Control characters, characters that are not valid in a file system path, and (optionally) whitespace. - if ((rawCharacter >= 0x01 && rawCharacter <= 0x1f) || + // Nulls, control characters, characters that are not valid in a file system path, and (optionally) whitespace. + // Note that a null is rejected even though the schema pattern does not exclude it; it cannot appear in a + // YAML manifest, but a REST source can produce one and it would truncate any path that it is used in. + if (rawCharacter <= 0x1f || rawCharacter == 0x7f || s_InvalidPathFieldCharacters.find(character) != std::string_view::npos || (disallowWhitespace && rawCharacter == ' ')) diff --git a/src/AppInstallerCommonCore/Public/winget/Manifest.h b/src/AppInstallerCommonCore/Public/winget/Manifest.h index 725b4dd56c..7b69f42617 100644 --- a/src/AppInstallerCommonCore/Public/winget/Manifest.h +++ b/src/AppInstallerCommonCore/Public/winget/Manifest.h @@ -7,6 +7,7 @@ #include #include +#include #include namespace AppInstaller::Manifest @@ -72,4 +73,13 @@ namespace AppInstaller::Manifest std::function extractStringFromInstaller = {}, std::function extractStringFromAppsAndFeaturesEntry = {}) const; }; + + // Creates a file system path part from a value that originated in manifest data. + // Manifest validation rejects values that are not safe to use in a path, but the values can also come from + // sources that do not go through it (for instance, installed package data), so they are sanitized here as + // well. Throws if the result would point outside of the directory that it is used in. + std::filesystem::path GetPathPart(std::string_view value); + + // Creates a file system path part for the manifest in the form ``. + std::filesystem::path GetPathPart(const Manifest& manifest, char separator = '.'); } \ No newline at end of file From 8834c305daa406c8735324e92ab6c3477c75084f Mon Sep 17 00:00:00 2001 From: JohnMcPMS Date: Mon, 21 Sep 2026 10:19:01 -0700 Subject: [PATCH 06/14] check for suitable path part conversion in manifest validation --- src/AppInstallerCLITests/YamlManifest.cpp | 11 +++++++++++ .../Manifest/ManifestValidation.cpp | 13 +++++++++++++ .../Public/winget/ManifestValidation.h | 1 + src/WinGetUtilInterop/Common/ManifestErrorId.cs | 3 +++ 4 files changed, 28 insertions(+) diff --git a/src/AppInstallerCLITests/YamlManifest.cpp b/src/AppInstallerCLITests/YamlManifest.cpp index fbb2575a46..aed6c9c111 100644 --- a/src/AppInstallerCLITests/YamlManifest.cpp +++ b/src/AppInstallerCLITests/YamlManifest.cpp @@ -1446,6 +1446,17 @@ TEST_CASE("PathFieldValueValidation", "[ManifestValidation]") // Values consisting solely of relative path specifiers. RequireSingleError(ValidatePackageVersion(".."), ManifestError::FieldEscapesDirectory); REQUIRE(ContainsError(ValidatePackageVersion("..\\.."), ManifestError::FieldEscapesDirectory)); + + // Reserved names cannot be used to construct a path part, so they must fail here rather than at the point of use. + for (const auto& value : { "CON", "con", "NUL.txt", "COM1", "LPT9.1.0" }) + { + RequireSingleError(ValidatePackageVersion(value), ManifestError::ReservedPathName); + RequireSingleError(ValidatePackageIdentifier(value), ManifestError::ReservedPathName); + } + + // Values that merely contain a reserved name are fine. + REQUIRE(ValidatePackageIdentifier("Contoso.NULL").empty()); + REQUIRE(ValidatePackageVersion("1.0-com1").empty()); } TEST_CASE("PackageIdentifierAndVersionPathValidation", "[ManifestValidation]") diff --git a/src/AppInstallerCommonCore/Manifest/ManifestValidation.cpp b/src/AppInstallerCommonCore/Manifest/ManifestValidation.cpp index 255b63ee9a..78ba43bba2 100644 --- a/src/AppInstallerCommonCore/Manifest/ManifestValidation.cpp +++ b/src/AppInstallerCommonCore/Manifest/ManifestValidation.cpp @@ -97,6 +97,7 @@ namespace AppInstaller::Manifest { AppInstaller::Manifest::ManifestError::InvalidPathCharacters, "The field value contains characters that are not allowed because the value is used to construct a file system path."sv }, { AppInstaller::Manifest::ManifestError::FieldExceedsMaxLength, "The field value exceeds the maximum allowed length."sv }, { AppInstaller::Manifest::ManifestError::FieldEscapesDirectory, "The field value must not point to a location outside of its base directory."sv }, + { AppInstaller::Manifest::ManifestError::ReservedPathName, "The field value cannot be used to construct a file system path because it is a reserved name."sv }, }; return ErrorIdToMessageMap; @@ -163,6 +164,18 @@ namespace AppInstaller::Manifest resultErrors.emplace_back(ManifestError::FieldEscapesDirectory, fieldNameString, valueString); } + // Finally, run the value through the same conversion that the consumers of these fields use so that + // values which cannot be turned into a usable path part, such as reserved device names, fail here + // rather than at the point of use. + try + { + std::ignore = Utility::MakeSuitablePathPart(value); + } + catch (...) + { + resultErrors.emplace_back(ManifestError::ReservedPathName, fieldNameString, valueString); + } + return resultErrors; } } diff --git a/src/AppInstallerCommonCore/Public/winget/ManifestValidation.h b/src/AppInstallerCommonCore/Public/winget/ManifestValidation.h index 334730c69e..e389a7e3e6 100644 --- a/src/AppInstallerCommonCore/Public/winget/ManifestValidation.h +++ b/src/AppInstallerCommonCore/Public/winget/ManifestValidation.h @@ -73,6 +73,7 @@ namespace AppInstaller::Manifest WINGET_DEFINE_RESOURCE_STRINGID(OptionalFieldMissing); WINGET_DEFINE_RESOURCE_STRINGID(PortableCommandAliasEscapesDirectory); WINGET_DEFINE_RESOURCE_STRINGID(RelativeFilePathEscapesDirectory); + WINGET_DEFINE_RESOURCE_STRINGID(ReservedPathName); WINGET_DEFINE_RESOURCE_STRINGID(RequiredFieldEmpty); WINGET_DEFINE_RESOURCE_STRINGID(RequiredFieldMissing); WINGET_DEFINE_RESOURCE_STRINGID(SchemaError); diff --git a/src/WinGetUtilInterop/Common/ManifestErrorId.cs b/src/WinGetUtilInterop/Common/ManifestErrorId.cs index 1b3b0a0844..5485f76a06 100644 --- a/src/WinGetUtilInterop/Common/ManifestErrorId.cs +++ b/src/WinGetUtilInterop/Common/ManifestErrorId.cs @@ -174,6 +174,9 @@ public enum ManifestErrorId /// Relative file path must not point to a location outside of archive directory. RelativeFilePathEscapesDirectory, + /// The field value cannot be used to construct a file system path because it is a reserved name. + ReservedPathName, + /// Required field with empty value. RequiredFieldEmpty, From 235145da127e4569f8df8bde1201de9e70cf8ffa Mon Sep 17 00:00:00 2001 From: JohnMcPMS Date: Wed, 23 Sep 2026 10:21:56 -0700 Subject: [PATCH 07/14] trim id and version in REST during parsing --- .../RestInterface_1_0.cpp | 67 +++++++++++++++++++ src/AppInstallerCLITests/YamlManifest.cpp | 7 ++ .../1_0/Json/ManifestDeserializer_1_0.cpp | 7 +- .../Json/SearchResponseDeserializer_1_0.cpp | 10 +++ 4 files changed, 90 insertions(+), 1 deletion(-) diff --git a/src/AppInstallerCLITests/RestInterface_1_0.cpp b/src/AppInstallerCLITests/RestInterface_1_0.cpp index 374f61e568..5282dd9985 100644 --- a/src/AppInstallerCLITests/RestInterface_1_0.cpp +++ b/src/AppInstallerCLITests/RestInterface_1_0.cpp @@ -450,6 +450,34 @@ TEST_CASE("Search_BadResponse_NoVersions", "[RestSource][Interface_1_0]") REQUIRE_THROWS_HR(v1.Search({}), APPINSTALLER_CLI_ERROR_RESTSOURCE_INVALID_DATA); } +TEST_CASE("Search_GoodResponse_PathFieldWhitespaceTrimmed", "[RestSource][Interface_1_0]") +{ + // The schema allows surrounding whitespace in PackageVersion, so the response is not an error. The value is + // used to construct file system paths, where Windows strips trailing spaces, and version comparison trims; + // trimming at parse time keeps those in agreement so that "1.0.0 " cannot conflict with "1.0.0". + std::string version = GENERATE("1.0.0 ", " 1.0.0", " 1.0.0 "); + + utility::string_t sample = ConvertToUTF16( + R"delimiter({ + "Data" : [ + { + "PackageIdentifier": " git.package ", + "PackageName": "package", + "Publisher": "git", + "Versions": [ + { "PackageVersion": ")delimiter" + version + R"delimiter(" }] + }] + })delimiter"); + + HttpClientHelper helper{ GetTestRestRequestHandler(web::http::status_codes::OK, std::move(sample)) }; + Interface v1{ TestRestUriString, std::move(helper) }; + Schema::IRestClient::SearchResult searchResponse = v1.Search({}); + REQUIRE(searchResponse.Matches.size() == 1); + REQUIRE(searchResponse.Matches.at(0).PackageInformation.PackageIdentifier == "git.package"); + REQUIRE(searchResponse.Matches.at(0).Versions.size() == 1); + REQUIRE(searchResponse.Matches.at(0).Versions.at(0).VersionAndChannel.GetVersion().ToString() == "1.0.0"); +} + TEST_CASE("Search_BadResponse_NotFoundCode", "[RestSource][Interface_1_0]") { HttpClientHelper helper{ GetTestRestRequestHandler(web::http::status_codes::NotFound) }; @@ -622,6 +650,45 @@ TEST_CASE("GetManifests_GoodResponse", "[RestSource][Interface_1_0]") sampleManifest.VerifyInstallers_AllFields(manifest); } +TEST_CASE("GetManifests_GoodResponse_PathFieldWhitespaceTrimmed", "[RestSource][Interface_1_0]") +{ + // The schema permits surrounding whitespace in these values, so this is a valid response. Trimming at parse + // time keeps the stored value consistent with version comparison and with the file system path built from it. + utility::string_t sample = _XPLATSTR( + R"delimiter({ + "Data": { + "PackageIdentifier": " Foo.Bar ", + "Versions": [ + { + "PackageVersion": " 5.0.0 ", + "DefaultLocale": { + "PackageLocale": "en-us", + "Publisher": "Foo", + "PackageName": "Bar", + "License": "Foo bar license", + "ShortDescription": "Foo bar description" + }, + "Installers": [ + { + "Architecture": "x64", + "InstallerSha256": "011048877dfaef109801b3f3ab2b60afc74f3fc4f7b3430e0c897f5da1df84b6", + "InstallerType": "exe", + "InstallerUrl": "https://installer.example.com/foobar.exe" + } + ] + } + ] + } + })delimiter"); + + HttpClientHelper helper{ GetTestRestRequestHandler(web::http::status_codes::OK, std::move(sample)) }; + Interface v1{ TestRestUriString, std::move(helper) }; + std::vector manifests = v1.GetManifests("Foo.Bar"); + REQUIRE(manifests.size() == 1); + REQUIRE(manifests[0].Id == "Foo.Bar"); + REQUIRE(manifests[0].Version == "5.0.0"); +} + TEST_CASE("GetManifests_GoodResponse_404AsEmpty", "[RestSource][Interface_1_0]") { utility::string_t notFoundResponse = _XPLATSTR( diff --git a/src/AppInstallerCLITests/YamlManifest.cpp b/src/AppInstallerCLITests/YamlManifest.cpp index aed6c9c111..a8d8b8d691 100644 --- a/src/AppInstallerCLITests/YamlManifest.cpp +++ b/src/AppInstallerCLITests/YamlManifest.cpp @@ -1457,6 +1457,13 @@ TEST_CASE("PathFieldValueValidation", "[ManifestValidation]") // Values that merely contain a reserved name are fine. REQUIRE(ValidatePackageIdentifier("Contoso.NULL").empty()); REQUIRE(ValidatePackageVersion("1.0-com1").empty()); + + // The schema permits whitespace anywhere in PackageVersion, so it is not an error here; parsing trims the + // surrounding whitespace instead. PackageIdentifier excludes whitespace entirely. + REQUIRE(ValidatePackageVersion("1.0.0 ").empty()); + REQUIRE(ValidatePackageVersion(" 1.0.0").empty()); + REQUIRE(ValidatePackageVersion("1.0.0 beta").empty()); + REQUIRE(ContainsError(ValidatePackageIdentifier("1.0.0 "), ManifestError::InvalidPathCharacters)); } TEST_CASE("PackageIdentifierAndVersionPathValidation", "[ManifestValidation]") diff --git a/src/AppInstallerRepositoryCore/Rest/Schema/1_0/Json/ManifestDeserializer_1_0.cpp b/src/AppInstallerRepositoryCore/Rest/Schema/1_0/Json/ManifestDeserializer_1_0.cpp index 081f2f50e0..5c33048107 100644 --- a/src/AppInstallerRepositoryCore/Rest/Schema/1_0/Json/ManifestDeserializer_1_0.cpp +++ b/src/AppInstallerRepositoryCore/Rest/Schema/1_0/Json/ManifestDeserializer_1_0.cpp @@ -5,6 +5,7 @@ #include "Rest/Schema/CommonRestConstants.h" #include "Rest/Schema/IRestClient.h" #include "ManifestDeserializer.h" +#include #include #include @@ -150,6 +151,10 @@ namespace AppInstaller::Repository::Rest::Schema::V1_0::Json THROW_HR(APPINSTALLER_CLI_ERROR_RESTSOURCE_INVALID_DATA); } + // The schema allows surrounding whitespace in these values, but they are used to construct file system + // paths and version comparison trims, so trim them here as the YAML parser does. + Utility::Trim(id.value()); + std::optional> versions = JSON::GetRawJsonArrayFromJsonNode(dataJsonObject, JSON::GetUtilityString(Versions)); if (!versions || versions.value().get().size() == 0) { @@ -171,7 +176,7 @@ namespace AppInstaller::Repository::Rest::Schema::V1_0::Json AICLI_LOG(Repo, Error, << "Missing package version in package: " << manifest.Id); THROW_HR(APPINSTALLER_CLI_ERROR_RESTSOURCE_INVALID_DATA); } - manifest.Version = std::move(packageVersion.value()); + manifest.Version = Utility::Trim(std::move(packageVersion.value())); manifest.Channel = JSON::GetRawStringValueFromJsonNode(versionItem, JSON::GetUtilityString(Channel)).value_or(""); diff --git a/src/AppInstallerRepositoryCore/Rest/Schema/1_0/Json/SearchResponseDeserializer_1_0.cpp b/src/AppInstallerRepositoryCore/Rest/Schema/1_0/Json/SearchResponseDeserializer_1_0.cpp index 240ca2f301..a849526700 100644 --- a/src/AppInstallerRepositoryCore/Rest/Schema/1_0/Json/SearchResponseDeserializer_1_0.cpp +++ b/src/AppInstallerRepositoryCore/Rest/Schema/1_0/Json/SearchResponseDeserializer_1_0.cpp @@ -4,6 +4,7 @@ #include "Rest/Schema/CommonRestConstants.h" #include "Rest/Schema/IRestClient.h" #include "SearchResponseDeserializer.h" +#include #include #include #include @@ -81,6 +82,10 @@ namespace AppInstaller::Repository::Rest::Schema::V1_0::Json return {}; } + // The YAML parser trims these values, so do the same here before validating; the schema excludes + // whitespace from the identifier, but surrounding whitespace should be tolerated identically. + Utility::Trim(packageId.value()); + if (!IsValidPathFieldValue(PackageIdentifier, AppInstaller::Manifest::ValidatePackageIdentifier(packageId.value()))) { return {}; @@ -139,6 +144,11 @@ namespace AppInstaller::Repository::Rest::Schema::V1_0::Json return {}; } + // The schema allows surrounding whitespace in the version, but the value is used to construct file system + // paths and version comparison trims, so trim it here as the YAML parser does. Trim before validating so + // that a value which is only path unsafe because of its surrounding whitespace is still accepted. + Utility::Trim(version.value()); + if (!IsValidPathFieldValue(PackageVersion, AppInstaller::Manifest::ValidatePackageVersion(version.value()))) { return {}; From 552e05d64dcd4b46648a09b17c1499d068fa3497 Mon Sep 17 00:00:00 2001 From: JohnMcPMS Date: Thu, 24 Sep 2026 16:23:00 -0700 Subject: [PATCH 08/14] pr feedbacks --- .../Workflows/DownloadFlow.cpp | 6 +- .../RestInterface_1_0.cpp | 4 +- src/AppInstallerCLITests/YamlManifest.cpp | 37 +++++++++---- src/AppInstallerCommonCore/Fonts.cpp | 2 +- .../Manifest/Manifest.cpp | 55 ++++++++++--------- .../Manifest/ManifestValidation.cpp | 10 ++-- .../Manifest/YamlParser.cpp | 2 +- .../Public/winget/Manifest.h | 10 ++-- .../Public/winget/ManifestValidation.h | 2 +- .../1_0/Json/ManifestDeserializer_1_0.cpp | 2 +- .../Json/SearchResponseDeserializer_1_0.cpp | 6 +- 11 files changed, 76 insertions(+), 60 deletions(-) diff --git a/src/AppInstallerCLICore/Workflows/DownloadFlow.cpp b/src/AppInstallerCLICore/Workflows/DownloadFlow.cpp index 3dbcede5ee..888198be1c 100644 --- a/src/AppInstallerCLICore/Workflows/DownloadFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/DownloadFlow.cpp @@ -749,11 +749,7 @@ namespace AppInstaller::CLI::Workflow } const auto& manifest = context.Get(); - std::filesystem::path packageDownloadFolderName = Utility::Version{ manifest.Version }.IsUnknown() ? - GetPathPart(manifest.Id) : - GetPathPart(manifest, '_'); - - context.Add(downloadsDirectory / packageDownloadFolderName); + context.Add(downloadsDirectory / GetPathPart(manifest, '_', true)); } } diff --git a/src/AppInstallerCLITests/RestInterface_1_0.cpp b/src/AppInstallerCLITests/RestInterface_1_0.cpp index 5282dd9985..8998685a39 100644 --- a/src/AppInstallerCLITests/RestInterface_1_0.cpp +++ b/src/AppInstallerCLITests/RestInterface_1_0.cpp @@ -652,8 +652,8 @@ TEST_CASE("GetManifests_GoodResponse", "[RestSource][Interface_1_0]") TEST_CASE("GetManifests_GoodResponse_PathFieldWhitespaceTrimmed", "[RestSource][Interface_1_0]") { - // The schema permits surrounding whitespace in these values, so this is a valid response. Trimming at parse - // time keeps the stored value consistent with version comparison and with the file system path built from it. + // The schema permits surrounding whitespace in the PackageIdentifier and PackageVersion values, so this is a valid response. + // Trimming at parse time keeps the stored value consistent with version comparison and with the file system path built from it. utility::string_t sample = _XPLATSTR( R"delimiter({ "Data": { diff --git a/src/AppInstallerCLITests/YamlManifest.cpp b/src/AppInstallerCLITests/YamlManifest.cpp index a8d8b8d691..93c862d1e4 100644 --- a/src/AppInstallerCLITests/YamlManifest.cpp +++ b/src/AppInstallerCLITests/YamlManifest.cpp @@ -4,6 +4,7 @@ #include "TestCommon.h" #include "TestSettings.h" #include +#include #include #include #include @@ -1502,18 +1503,34 @@ TEST_CASE("ManifestGetPathPart", "[ManifestValidation]") // The common case must not alter the value, as the resulting paths are persisted. REQUIRE(GetPathPart(manifest) == std::filesystem::path{ L"Foo.Bar.1.0.0" }); REQUIRE(GetPathPart(manifest, '_') == std::filesystem::path{ L"Foo.Bar_1.0.0" }); - REQUIRE(GetPathPart(manifest.Id) == std::filesystem::path{ L"Foo.Bar" }); + + // An unknown version carries no information, so it is left out entirely. + manifest.Version = "Unknown"; + REQUIRE(GetPathPart(manifest) == std::filesystem::path{ L"Foo.Bar" }); + REQUIRE(GetPathPart(manifest, '_') == std::filesystem::path{ L"Foo.Bar" }); // Values that validation would have rejected are sanitized rather than used as given. - REQUIRE(GetPathPart("a\\b") == std::filesystem::path{ L"a_b" }); - REQUIRE(GetPathPart("a/b") == std::filesystem::path{ L"a_b" }); - REQUIRE(GetPathPart("C:") == std::filesystem::path{ L"C_" }); - REQUIRE(GetPathPart("\\\\server\\share") == std::filesystem::path{ L"__server_share" }); - - // Relative path specifiers can never produce a path part that escapes its base directory. - REQUIRE(GetPathPart("..") == std::filesystem::path{ L"._" }); - REQUIRE_THROWS(GetPathPart("..\\..")); - REQUIRE_THROWS(GetPathPart("../../foo")); + manifest.Id = "a\\b"; + manifest.Version = "c/d"; + REQUIRE(GetPathPart(manifest) == std::filesystem::path{ L"a_b.c_d" }); + + manifest.Id = "Foo.Bar"; + manifest.Version = "C:"; + REQUIRE(GetPathPart(manifest) == std::filesystem::path{ L"Foo.Bar.C_" }); + + // A trailing dot is not allowed at the end of a path part. + manifest.Version = "1.0."; + REQUIRE(GetPathPart(manifest) == std::filesystem::path{ L"Foo.Bar.1.0_" }); + + // Values that cannot be made into a usable path part are reported as a manifest problem rather than + // surfacing the raw E_INVALIDARG from the conversion. + manifest.Version = "1.0.0"; + + for (const std::string_view id : { "..", "..\\..", "../../foo", "CON", "NUL" }) + { + manifest.Id = id; + REQUIRE_THROWS_HR(GetPathPart(manifest), APPINSTALLER_CLI_ERROR_INVALID_MANIFEST); + } } TEST_CASE("PortableFileTypeValidation", "[ManifestValidation]") diff --git a/src/AppInstallerCommonCore/Fonts.cpp b/src/AppInstallerCommonCore/Fonts.cpp index 48879a1973..43298c0099 100644 --- a/src/AppInstallerCommonCore/Fonts.cpp +++ b/src/AppInstallerCommonCore/Fonts.cpp @@ -56,7 +56,7 @@ namespace AppInstaller::Fonts { if (context.PackageId.empty() || context.PackageVersion.empty()) { - // This is a programming error if we reach this point where the package identifer cannot be created or derived. + // This is a programming error if we reach this point where the package identifier cannot be created or derived. THROW_HR_MSG(E_UNEXPECTED, "Package Id and Version must be provided and non-empty."); } diff --git a/src/AppInstallerCommonCore/Manifest/Manifest.cpp b/src/AppInstallerCommonCore/Manifest/Manifest.cpp index 97be981086..8e5dee34d4 100644 --- a/src/AppInstallerCommonCore/Manifest/Manifest.cpp +++ b/src/AppInstallerCommonCore/Manifest/Manifest.cpp @@ -18,6 +18,29 @@ namespace AppInstaller::Manifest set.emplace(Utility::FoldCase(value)); } } + + // Creates a file system path part from a value that originated in manifest data. + std::filesystem::path GetPathPartFromValue(std::string_view value) + { + std::string result; + + try + { + result = Utility::MakeSuitablePathPart(value); + } + catch (...) + { + // MakeSuitablePathPart throws for values that cannot be made into a usable path part, + // such as reserved device names. Surface that as a manifest problem rather than E_INVALIDARG. + THROW_HR_MSG(APPINSTALLER_CLI_ERROR_INVALID_MANIFEST, "Value cannot be used as a path part: %.*hs", static_cast(value.length()), value.data()); + } + + // MakeSuitablePathPart removes all path separators, so this can only fire if it stops doing so. + // It is kept as a final check because the values that reach here originate outside of the client. + THROW_HR_IF_MSG(APPINSTALLER_CLI_ERROR_INVALID_MANIFEST, Filesystem::PathEscapesBaseDirectory(result), "Path part points to a location outside of its base directory: %hs", result.c_str()); + + return { Utility::ConvertToUTF16(result) }; + } } void Manifest::ApplyLocale(const std::string& locale) @@ -243,34 +266,16 @@ namespace AppInstaller::Manifest return result; } - std::filesystem::path GetPathPart(std::string_view value) + std::filesystem::path GetPathPart(const Manifest& manifest, char separator, bool dropUnknownVersion) { - std::string result; + std::string value = manifest.Id; - try - { - result = Utility::MakeSuitablePathPart(value); - } - catch (...) + if (!dropUnknownVersion || !Utility::Version{ manifest.Version }.IsUnknown()) { - // MakeSuitablePathPart throws for values that cannot be made into a usable path part, - // such as reserved device names. Surface that as a manifest problem rather than E_INVALIDARG. - THROW_HR_MSG(APPINSTALLER_CLI_ERROR_INVALID_MANIFEST, "Value cannot be used as a path part: %.*hs", static_cast(value.length()), value.data()); + value += separator; + value += manifest.Version; } - // MakeSuitablePathPart removes all path separators, so this can only fire if it stops doing so. - // It is kept as a final check because the values that reach here originate outside of the client. - THROW_HR_IF_MSG(APPINSTALLER_CLI_ERROR_INVALID_MANIFEST, Filesystem::PathEscapesBaseDirectory(result), "Path part points to a location outside of its base directory: %hs", result.c_str()); - - return { Utility::ConvertToUTF16(result) }; - } - - std::filesystem::path GetPathPart(const Manifest& manifest, char separator) - { - std::string value = manifest.Id; - value += separator; - value += manifest.Version; - - return GetPathPart(value); + return GetPathPartFromValue(value); } -} \ No newline at end of file +} diff --git a/src/AppInstallerCommonCore/Manifest/ManifestValidation.cpp b/src/AppInstallerCommonCore/Manifest/ManifestValidation.cpp index 78ba43bba2..6496f3a885 100644 --- a/src/AppInstallerCommonCore/Manifest/ManifestValidation.cpp +++ b/src/AppInstallerCommonCore/Manifest/ManifestValidation.cpp @@ -123,7 +123,7 @@ namespace AppInstaller::Manifest // Validates a manifest field value that is used to construct file system paths. // disallowWhitespace: set for fields whose schema definition excludes whitespace (for example, PackageIdentifier). - std::vector ValidatePathFieldValue(std::string_view fieldName, std::string_view value, bool disallowWhitespace) + std::vector ValidateFieldValueUsedInPathConstruction(std::string_view fieldName, std::string_view value, bool disallowWhitespace) { std::vector resultErrors; @@ -182,15 +182,15 @@ namespace AppInstaller::Manifest std::vector ValidatePackageIdentifier(std::string_view value) { - return ValidatePathFieldValue("PackageIdentifier", value, /* disallowWhitespace */ true); + return ValidateFieldValueUsedInPathConstruction("PackageIdentifier", value, /* disallowWhitespace */ true); } std::vector ValidatePackageVersion(std::string_view value) { - return ValidatePathFieldValue("PackageVersion", value, /* disallowWhitespace */ false); + return ValidateFieldValueUsedInPathConstruction("PackageVersion", value, /* disallowWhitespace */ false); } - std::vector ValidatePathFields(const Manifest& manifest) + std::vector ValidateFieldsUsedInPathConstruction(const Manifest& manifest) { std::vector resultErrors = ValidatePackageIdentifier(manifest.Id); @@ -206,7 +206,7 @@ namespace AppInstaller::Manifest // PackageIdentifier and PackageVersion are used to construct file system paths, so the schema // restrictions on them must be enforced at runtime for all manifest sources. - auto pathFieldErrors = ValidatePathFields(manifest); + auto pathFieldErrors = ValidateFieldsUsedInPathConstruction(manifest); std::move(pathFieldErrors.begin(), pathFieldErrors.end(), std::inserter(resultErrors, resultErrors.end())); // Channel is not supported currently diff --git a/src/AppInstallerCommonCore/Manifest/YamlParser.cpp b/src/AppInstallerCommonCore/Manifest/YamlParser.cpp index e651c14da9..83392c71cb 100644 --- a/src/AppInstallerCommonCore/Manifest/YamlParser.cpp +++ b/src/AppInstallerCommonCore/Manifest/YamlParser.cpp @@ -492,7 +492,7 @@ namespace AppInstaller::Manifest::YamlParser { // PackageIdentifier and PackageVersion are used to construct file system paths, so the schema // restrictions on them are enforced even when the full semantic validation is not requested. - errors = ValidatePathFields(manifest); + errors = ValidateFieldsUsedInPathConstruction(manifest); std::move(errors.begin(), errors.end(), std::inserter(resultErrors, resultErrors.end())); } diff --git a/src/AppInstallerCommonCore/Public/winget/Manifest.h b/src/AppInstallerCommonCore/Public/winget/Manifest.h index 7b69f42617..0c3826558d 100644 --- a/src/AppInstallerCommonCore/Public/winget/Manifest.h +++ b/src/AppInstallerCommonCore/Public/winget/Manifest.h @@ -74,12 +74,10 @@ namespace AppInstaller::Manifest std::function extractStringFromAppsAndFeaturesEntry = {}) const; }; - // Creates a file system path part from a value that originated in manifest data. + // Creates a file system path part for the manifest in the form ``, + // or just `` when the version is unknown. // Manifest validation rejects values that are not safe to use in a path, but the values can also come from // sources that do not go through it (for instance, installed package data), so they are sanitized here as // well. Throws if the result would point outside of the directory that it is used in. - std::filesystem::path GetPathPart(std::string_view value); - - // Creates a file system path part for the manifest in the form ``. - std::filesystem::path GetPathPart(const Manifest& manifest, char separator = '.'); -} \ No newline at end of file + std::filesystem::path GetPathPart(const Manifest& manifest, char separator = '.', bool dropUnknownVersion = false); +} diff --git a/src/AppInstallerCommonCore/Public/winget/ManifestValidation.h b/src/AppInstallerCommonCore/Public/winget/ManifestValidation.h index e389a7e3e6..98cf663800 100644 --- a/src/AppInstallerCommonCore/Public/winget/ManifestValidation.h +++ b/src/AppInstallerCommonCore/Public/winget/ManifestValidation.h @@ -232,7 +232,7 @@ namespace AppInstaller::Manifest // The manifest schemas restrict these fields to values that are safe to use as a path part, but the // schema is not applied at runtime for all manifest sources (for example, REST sources), so the // restrictions are enforced here as well. - std::vector ValidatePathFields(const Manifest& manifest); + std::vector ValidateFieldsUsedInPathConstruction(const Manifest& manifest); // Validates an individual PackageIdentifier value, for sources that do not produce a full manifest. std::vector ValidatePackageIdentifier(std::string_view value); diff --git a/src/AppInstallerRepositoryCore/Rest/Schema/1_0/Json/ManifestDeserializer_1_0.cpp b/src/AppInstallerRepositoryCore/Rest/Schema/1_0/Json/ManifestDeserializer_1_0.cpp index 5c33048107..6b9e843cb6 100644 --- a/src/AppInstallerRepositoryCore/Rest/Schema/1_0/Json/ManifestDeserializer_1_0.cpp +++ b/src/AppInstallerRepositoryCore/Rest/Schema/1_0/Json/ManifestDeserializer_1_0.cpp @@ -151,7 +151,7 @@ namespace AppInstaller::Repository::Rest::Schema::V1_0::Json THROW_HR(APPINSTALLER_CLI_ERROR_RESTSOURCE_INVALID_DATA); } - // The schema allows surrounding whitespace in these values, but they are used to construct file system + // The schema allows surrounding whitespace in the id and version values, but they are used to construct file system // paths and version comparison trims, so trim them here as the YAML parser does. Utility::Trim(id.value()); diff --git a/src/AppInstallerRepositoryCore/Rest/Schema/1_0/Json/SearchResponseDeserializer_1_0.cpp b/src/AppInstallerRepositoryCore/Rest/Schema/1_0/Json/SearchResponseDeserializer_1_0.cpp index a849526700..5c2da28762 100644 --- a/src/AppInstallerRepositoryCore/Rest/Schema/1_0/Json/SearchResponseDeserializer_1_0.cpp +++ b/src/AppInstallerRepositoryCore/Rest/Schema/1_0/Json/SearchResponseDeserializer_1_0.cpp @@ -25,7 +25,7 @@ namespace AppInstaller::Repository::Rest::Schema::V1_0::Json // The package identifier and version flow into file system paths, so the manifest schema restrictions // on them are enforced here as well; the schema itself is not applied to REST responses. - bool IsValidPathFieldValue(std::string_view fieldName, const std::vector& validationErrors) + bool CheckPathFieldValueValidation(std::string_view fieldName, const std::vector& validationErrors) { bool result = true; @@ -86,7 +86,7 @@ namespace AppInstaller::Repository::Rest::Schema::V1_0::Json // whitespace from the identifier, but surrounding whitespace should be tolerated identically. Utility::Trim(packageId.value()); - if (!IsValidPathFieldValue(PackageIdentifier, AppInstaller::Manifest::ValidatePackageIdentifier(packageId.value()))) + if (!CheckPathFieldValueValidation(PackageIdentifier, AppInstaller::Manifest::ValidatePackageIdentifier(packageId.value()))) { return {}; } @@ -149,7 +149,7 @@ namespace AppInstaller::Repository::Rest::Schema::V1_0::Json // that a value which is only path unsafe because of its surrounding whitespace is still accepted. Utility::Trim(version.value()); - if (!IsValidPathFieldValue(PackageVersion, AppInstaller::Manifest::ValidatePackageVersion(version.value()))) + if (!CheckPathFieldValueValidation(PackageVersion, AppInstaller::Manifest::ValidatePackageVersion(version.value()))) { return {}; } From 1d631c94d1c75c1aab974396ce475200e6412955 Mon Sep 17 00:00:00 2001 From: JohnMcPMS Date: Thu, 24 Sep 2026 16:25:39 -0700 Subject: [PATCH 09/14] fixes --- src/AppInstallerCLITests/YamlManifest.cpp | 12 +++++++++--- src/AppInstallerCommonCore/Public/winget/Manifest.h | 2 +- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/src/AppInstallerCLITests/YamlManifest.cpp b/src/AppInstallerCLITests/YamlManifest.cpp index 93c862d1e4..4749dc0aa1 100644 --- a/src/AppInstallerCLITests/YamlManifest.cpp +++ b/src/AppInstallerCLITests/YamlManifest.cpp @@ -1504,10 +1504,16 @@ TEST_CASE("ManifestGetPathPart", "[ManifestValidation]") REQUIRE(GetPathPart(manifest) == std::filesystem::path{ L"Foo.Bar.1.0.0" }); REQUIRE(GetPathPart(manifest, '_') == std::filesystem::path{ L"Foo.Bar_1.0.0" }); - // An unknown version carries no information, so it is left out entirely. + // An unknown version is only dropped when the caller asks for it. manifest.Version = "Unknown"; - REQUIRE(GetPathPart(manifest) == std::filesystem::path{ L"Foo.Bar" }); - REQUIRE(GetPathPart(manifest, '_') == std::filesystem::path{ L"Foo.Bar" }); + REQUIRE(GetPathPart(manifest) == std::filesystem::path{ L"Foo.Bar.Unknown" }); + REQUIRE(GetPathPart(manifest, '_') == std::filesystem::path{ L"Foo.Bar_Unknown" }); + REQUIRE(GetPathPart(manifest, '.', true) == std::filesystem::path{ L"Foo.Bar" }); + REQUIRE(GetPathPart(manifest, '_', true) == std::filesystem::path{ L"Foo.Bar" }); + + // A known version is kept regardless of the drop request. + manifest.Version = "1.0.0"; + REQUIRE(GetPathPart(manifest, '.', true) == std::filesystem::path{ L"Foo.Bar.1.0.0" }); // Values that validation would have rejected are sanitized rather than used as given. manifest.Id = "a\\b"; diff --git a/src/AppInstallerCommonCore/Public/winget/Manifest.h b/src/AppInstallerCommonCore/Public/winget/Manifest.h index 0c3826558d..ea4f9f23cd 100644 --- a/src/AppInstallerCommonCore/Public/winget/Manifest.h +++ b/src/AppInstallerCommonCore/Public/winget/Manifest.h @@ -75,7 +75,7 @@ namespace AppInstaller::Manifest }; // Creates a file system path part for the manifest in the form ``, - // or just `` when the version is unknown. + // or just `` when the version is unknown and the drop is requested. // Manifest validation rejects values that are not safe to use in a path, but the values can also come from // sources that do not go through it (for instance, installed package data), so they are sanitized here as // well. Throws if the result would point outside of the directory that it is used in. From ef2a1f3c5bcbfde368b1be239e222233cdc51fce Mon Sep 17 00:00:00 2001 From: JohnMcPMS Date: Thu, 24 Sep 2026 16:30:50 -0700 Subject: [PATCH 10/14] reserved names with superscripts --- src/AppInstallerCLITests/Strings.cpp | 13 +++++++++++++ src/AppInstallerCLITests/YamlManifest.cpp | 2 +- src/AppInstallerSharedLib/AppInstallerStrings.cpp | 6 +++++- 3 files changed, 19 insertions(+), 2 deletions(-) diff --git a/src/AppInstallerCLITests/Strings.cpp b/src/AppInstallerCLITests/Strings.cpp index 86f052cb31..a8785d2181 100644 --- a/src/AppInstallerCLITests/Strings.cpp +++ b/src/AppInstallerCLITests/Strings.cpp @@ -194,6 +194,19 @@ TEST_CASE("MakeSuitablePathPart", "[strings]") REQUIRE(MakeSuitablePathPart(std::string(300, ' ')) == SHA256::ConvertToString(SHA256::ComputeHash(std::string(300, ' ')))); REQUIRE_THROWS_HR(MakeSuitablePathPart("COM1"), E_INVALIDARG); REQUIRE_THROWS_HR(MakeSuitablePathPart("NUL.txt"), E_INVALIDARG); + + // The superscript digit forms of COM and LPT are reserved as well. + REQUIRE_THROWS_HR(MakeSuitablePathPart("COM\xC2\xB9"), E_INVALIDARG); + REQUIRE_THROWS_HR(MakeSuitablePathPart("COM\xC2\xB2"), E_INVALIDARG); + REQUIRE_THROWS_HR(MakeSuitablePathPart("COM\xC2\xB3"), E_INVALIDARG); + REQUIRE_THROWS_HR(MakeSuitablePathPart("LPT\xC2\xB9"), E_INVALIDARG); + REQUIRE_THROWS_HR(MakeSuitablePathPart("LPT\xC2\xB2"), E_INVALIDARG); + REQUIRE_THROWS_HR(MakeSuitablePathPart("LPT\xC2\xB3"), E_INVALIDARG); + REQUIRE_THROWS_HR(MakeSuitablePathPart("lpt\xC2\xB3.txt"), E_INVALIDARG); + + // Only the exact superscript digits are reserved; other trailing values are not. + REQUIRE(MakeSuitablePathPart("COM\xC2\xB4") == "COM\xC2\xB4"); + REQUIRE(MakeSuitablePathPart("COM\xC2\xB9" "0") == "COM\xC2\xB9" "0"); } TEST_CASE("GetFileNameFromURI", "[strings]") diff --git a/src/AppInstallerCLITests/YamlManifest.cpp b/src/AppInstallerCLITests/YamlManifest.cpp index 4749dc0aa1..ec8a9b9d17 100644 --- a/src/AppInstallerCLITests/YamlManifest.cpp +++ b/src/AppInstallerCLITests/YamlManifest.cpp @@ -1449,7 +1449,7 @@ TEST_CASE("PathFieldValueValidation", "[ManifestValidation]") REQUIRE(ContainsError(ValidatePackageVersion("..\\.."), ManifestError::FieldEscapesDirectory)); // Reserved names cannot be used to construct a path part, so they must fail here rather than at the point of use. - for (const auto& value : { "CON", "con", "NUL.txt", "COM1", "LPT9.1.0" }) + for (const auto& value : { "CON", "con", "NUL.txt", "COM1", "LPT9.1.0", "COM\xC2\xB9", "com\xC2\xB2", "LPT\xC2\xB3.txt" }) { RequireSingleError(ValidatePackageVersion(value), ManifestError::ReservedPathName); RequireSingleError(ValidatePackageIdentifier(value), ManifestError::ReservedPathName); diff --git a/src/AppInstallerSharedLib/AppInstallerStrings.cpp b/src/AppInstallerSharedLib/AppInstallerStrings.cpp index f1ee1b359b..930cc02c19 100644 --- a/src/AppInstallerSharedLib/AppInstallerStrings.cpp +++ b/src/AppInstallerSharedLib/AppInstallerStrings.cpp @@ -763,9 +763,13 @@ namespace AppInstaller::Utility // Second, look for any newly formed illegal names. // For now just error on these cases; they should not happen often. + // The COM/LPT names using the superscript digits (U+00B9, U+00B2, U+00B3) are reserved as well; they are + // written here as explicit UTF-8 byte sequences so that the encoding of this file cannot alter them. for (const auto& illegalName : { "."sv, "CON"sv, "PRN"sv, "AUX"sv, "NUL"sv, "COM1"sv, "COM2"sv, "COM3"sv, "COM4"sv, "COM5"sv, "COM6"sv, "COM7"sv, "COM8"sv, "COM9"sv, - "LPT1"sv, "LPT2"sv, "LPT3"sv, "LPT4"sv, "LPT5"sv, "LPT6"sv, "LPT7"sv, "LPT8"sv, "LPT9"sv }) + "COM\xC2\xB9"sv, "COM\xC2\xB2"sv, "COM\xC2\xB3"sv, + "LPT1"sv, "LPT2"sv, "LPT3"sv, "LPT4"sv, "LPT5"sv, "LPT6"sv, "LPT7"sv, "LPT8"sv, "LPT9"sv, + "LPT\xC2\xB9"sv, "LPT\xC2\xB2"sv, "LPT\xC2\xB3"sv }) { // Either equals the illegal name (starts with and same length) or starts with and the first character after is a . if (CaseInsensitiveStartsWith(result, illegalName) && (result.size() == illegalName.size() || result[illegalName.size()] == '.')) From 843e5bb49298fd5e5d27cf52a8890b5c886145fc Mon Sep 17 00:00:00 2001 From: JohnMcPMS Date: Fri, 25 Sep 2026 10:51:32 -0700 Subject: [PATCH 11/14] font values validated --- src/AppInstallerCLITests/Fonts.cpp | 61 +++++++++++++++++++ src/AppInstallerCommonCore/Fonts.cpp | 16 +++-- .../Manifest/ManifestValidation.cpp | 7 +++ .../Public/winget/ManifestValidation.h | 9 +++ 4 files changed, 88 insertions(+), 5 deletions(-) diff --git a/src/AppInstallerCLITests/Fonts.cpp b/src/AppInstallerCLITests/Fonts.cpp index 425178bbb1..a86731dab4 100644 --- a/src/AppInstallerCLITests/Fonts.cpp +++ b/src/AppInstallerCLITests/Fonts.cpp @@ -2,6 +2,7 @@ // Licensed under the MIT License. #include "pch.h" #include "TestCommon.h" +#include #include #include @@ -113,6 +114,66 @@ TEST_CASE("ValidateValidFontPackage", "[fonts]") REQUIRE(fontValidationResult.Status == FontStatus::Absent); } +TEST_CASE("PackageInformationUsedInPathsIsValidated", "[fonts]") +{ + TestDataFile testFont(s_FontFile); + + // The package id and version are used to construct both file system and registry paths. They are not + // always supplied by a validated manifest; an uninstall without a manifest uses the moniker and version + // of the installed package, which are read back from the font registry keys. Those keys are writable by + // the user for a per-user scope, so the values are checked at their point of use. + auto makeContext = [&](std::wstring_view packageId, std::wstring_view packageVersion) + { + auto context = FontContext(); + context.Scope = ScopeEnum::User; + context.InstallerSource = InstallerSource::WinGet; + context.PackageId = packageId; + context.PackageVersion = packageVersion; + context.AddPackageFile(testFont.GetPath()); + return context; + }; + + // A well formed package is unaffected. + auto goodContext = makeContext(L"TestPackage", L"1.0.0.0"); + REQUIRE(ValidateFontPackage(goodContext).Result == FontResult::Success); + + // Values that escape the directory they are used to construct. + for (const auto& value : { L"..", L"..\\..", L"C:" }) + { + auto idContext = makeContext(value, L"1.0.0.0"); + REQUIRE_THROWS_HR(ValidateFontPackage(idContext), APPINSTALLER_CLI_ERROR_INVALID_MANIFEST); + + auto versionContext = makeContext(L"TestPackage", value); + REQUIRE_THROWS_HR(ValidateFontPackage(versionContext), APPINSTALLER_CLI_ERROR_INVALID_MANIFEST); + } + + // Values containing a path separator or another character that is not valid in a path. A registry key + // name cannot contain a separator, but the value can reach here from other sources. + for (const auto& value : { L"Test\\Package", L"Test/Package", L"Test*Package", L"Test|Package" }) + { + auto idContext = makeContext(value, L"1.0.0.0"); + REQUIRE_THROWS_HR(ValidateFontPackage(idContext), APPINSTALLER_CLI_ERROR_INVALID_MANIFEST); + + auto versionContext = makeContext(L"TestPackage", value); + REQUIRE_THROWS_HR(ValidateFontPackage(versionContext), APPINSTALLER_CLI_ERROR_INVALID_MANIFEST); + } + + // Reserved device names, which a registry key name is free to use. + for (const auto& value : { L"CON", L"NUL.txt", L"COM1" }) + { + auto idContext = makeContext(value, L"1.0.0.0"); + REQUIRE_THROWS_HR(ValidateFontPackage(idContext), APPINSTALLER_CLI_ERROR_INVALID_MANIFEST); + + auto versionContext = makeContext(L"TestPackage", value); + REQUIRE_THROWS_HR(ValidateFontPackage(versionContext), APPINSTALLER_CLI_ERROR_INVALID_MANIFEST); + } + + // Whitespace is not a path safety concern, so it must not be rejected here. The values have to keep + // matching the paths and registry keys that were persisted at install time, so they are never altered. + auto whitespaceContext = makeContext(L"Test Package", L"1.0 beta"); + REQUIRE(ValidateFontPackage(whitespaceContext).Result == FontResult::Success); +} + TEST_CASE("InstallInvalidFontPackageUser", "[fonts]") { TestDataFile testFont(s_InvalidFontFile); diff --git a/src/AppInstallerCommonCore/Fonts.cpp b/src/AppInstallerCommonCore/Fonts.cpp index 43298c0099..2cd3ee4d6d 100644 --- a/src/AppInstallerCommonCore/Fonts.cpp +++ b/src/AppInstallerCommonCore/Fonts.cpp @@ -10,6 +10,7 @@ #include #include #include +#include #include #include #include @@ -60,14 +61,19 @@ namespace AppInstaller::Fonts THROW_HR_MSG(E_UNEXPECTED, "Package Id and Version must be provided and non-empty."); } - // Defense in depth; the package id and version originate from a manifest and are used to - // construct both file system and registry paths. Manifest validation rejects these values, - // but verify again here as this is the point of use. + // Defense in depth; the package id and version are used to construct both file system and + // registry paths. These values do not always originate from a manifest that has been validated: + // an uninstall without a manifest uses the moniker and version of the installed package, and + // those are read back from the font registry keys by GetInstalledFontPackages. For a per-user + // scope those keys are writable by the user, so the values are checked here with the same + // restrictions that manifest validation applies rather than only checking for directory escapes. + // These values are only ever checked and never altered, as they must continue to match the paths + // and registry keys that were persisted at install time. std::string packageId = ConvertToUTF8(context.PackageId); - THROW_HR_IF_MSG(APPINSTALLER_CLI_ERROR_INVALID_MANIFEST, Filesystem::PathEscapesBaseDirectory(packageId), "Path part points to a location outside of its base directory: %hs", packageId.c_str()); + THROW_HR_IF_MSG(APPINSTALLER_CLI_ERROR_INVALID_MANIFEST, !Manifest::IsValueSafeForPathConstruction(packageId), "Package id cannot be used to construct a path: %hs", packageId.c_str()); std::string packageVersion = ConvertToUTF8(context.PackageVersion); - THROW_HR_IF_MSG(APPINSTALLER_CLI_ERROR_INVALID_MANIFEST, Filesystem::PathEscapesBaseDirectory(packageVersion), "Path part points to a location outside of its base directory: %hs", packageVersion.c_str()); + THROW_HR_IF_MSG(APPINSTALLER_CLI_ERROR_INVALID_MANIFEST, !Manifest::IsValueSafeForPathConstruction(packageVersion), "Package version cannot be used to construct a path: %hs", packageVersion.c_str()); } std::wstring GetFontRegistryPath(const FontContext& context) diff --git a/src/AppInstallerCommonCore/Manifest/ManifestValidation.cpp b/src/AppInstallerCommonCore/Manifest/ManifestValidation.cpp index 6496f3a885..ef181c4963 100644 --- a/src/AppInstallerCommonCore/Manifest/ManifestValidation.cpp +++ b/src/AppInstallerCommonCore/Manifest/ManifestValidation.cpp @@ -190,6 +190,13 @@ namespace AppInstaller::Manifest return ValidateFieldValueUsedInPathConstruction("PackageVersion", value, /* disallowWhitespace */ false); } + bool IsValueSafeForPathConstruction(std::string_view value) + { + // Whitespace is excluded here because it is not a path safety concern; it is only excluded from + // the fields whose schema definition happens to exclude it. + return ValidateFieldValueUsedInPathConstruction({}, value, /* disallowWhitespace */ false).empty(); + } + std::vector ValidateFieldsUsedInPathConstruction(const Manifest& manifest) { std::vector resultErrors = ValidatePackageIdentifier(manifest.Id); diff --git a/src/AppInstallerCommonCore/Public/winget/ManifestValidation.h b/src/AppInstallerCommonCore/Public/winget/ManifestValidation.h index 98cf663800..df5eb2c203 100644 --- a/src/AppInstallerCommonCore/Public/winget/ManifestValidation.h +++ b/src/AppInstallerCommonCore/Public/winget/ManifestValidation.h @@ -239,4 +239,13 @@ namespace AppInstaller::Manifest // Validates an individual PackageVersion value, for sources that do not produce a full manifest. std::vector ValidatePackageVersion(std::string_view value); + + // Determines whether a value can be used to construct a file system or registry path. + // This applies the same restrictions that manifest validation applies to the fields used in path + // construction, except for whitespace, which only some of those fields exclude. It is intended for + // values at their point of use, including values that did not come directly from a manifest; for + // example, values read back from existing install information that a manifest originally produced. + // An empty value is not considered a failure here, as emptiness is reported by the required field + // validation and is not meaningful to callers that only need to know whether a value is path safe. + bool IsValueSafeForPathConstruction(std::string_view value); } From 5aa6d9ae582127fd92fd72d2b0bf95e570041483 Mon Sep 17 00:00:00 2001 From: JohnMcPMS Date: Fri, 25 Sep 2026 10:58:19 -0700 Subject: [PATCH 12/14] icu length --- src/AppInstallerCLITests/YamlManifest.cpp | 27 +++++++++++++++++++ .../Manifest/ManifestValidation.cpp | 8 +++++- 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/src/AppInstallerCLITests/YamlManifest.cpp b/src/AppInstallerCLITests/YamlManifest.cpp index ec8a9b9d17..6a27eeb2c6 100644 --- a/src/AppInstallerCLITests/YamlManifest.cpp +++ b/src/AppInstallerCLITests/YamlManifest.cpp @@ -1442,8 +1442,35 @@ TEST_CASE("PathFieldValueValidation", "[ManifestValidation]") RequireSingleError(ValidatePackageIdentifier("ab\0c"sv), ManifestError::InvalidPathCharacters); // Values that exceed the maximum length declared by the schema. + REQUIRE(ValidatePackageVersion(std::string(128, '1')).empty()); RequireSingleError(ValidatePackageVersion(std::string(129, '1')), ManifestError::FieldExceedsMaxLength); + // The schema limit is expressed in characters, so the length is measured in grapheme clusters rather than + // in UTF-8 code units. Each of these characters encodes to more than one byte. + { + // U+00E9, two bytes each. + std::string twoByteCharacters; + for (size_t i = 0; i < 128; ++i) + { + twoByteCharacters += "\xC3\xA9"; + } + + REQUIRE(twoByteCharacters.size() == 256); + REQUIRE(ValidatePackageVersion(twoByteCharacters).empty()); + RequireSingleError(ValidatePackageVersion(twoByteCharacters + "\xC3\xA9"), ManifestError::FieldExceedsMaxLength); + + // U+1F600, four bytes each. + std::string fourByteCharacters; + for (size_t i = 0; i < 128; ++i) + { + fourByteCharacters += "\xF0\x9F\x98\x80"; + } + + REQUIRE(fourByteCharacters.size() == 512); + REQUIRE(ValidatePackageVersion(fourByteCharacters).empty()); + RequireSingleError(ValidatePackageVersion(fourByteCharacters + "\xF0\x9F\x98\x80"), ManifestError::FieldExceedsMaxLength); + } + // Values consisting solely of relative path specifiers. RequireSingleError(ValidatePackageVersion(".."), ManifestError::FieldEscapesDirectory); REQUIRE(ContainsError(ValidatePackageVersion("..\\.."), ManifestError::FieldEscapesDirectory)); diff --git a/src/AppInstallerCommonCore/Manifest/ManifestValidation.cpp b/src/AppInstallerCommonCore/Manifest/ManifestValidation.cpp index ef181c4963..fc96f3543a 100644 --- a/src/AppInstallerCommonCore/Manifest/ManifestValidation.cpp +++ b/src/AppInstallerCommonCore/Manifest/ManifestValidation.cpp @@ -3,6 +3,7 @@ #include "pch.h" #include "AppInstallerLogging.h" #include "AppInstallerMsixInfo.h" +#include "AppInstallerStrings.h" #include "winget/MsixManifest.h" #include "winget/ManifestValidation.h" #include "winget/MsixManifestValidation.h" @@ -119,6 +120,9 @@ namespace AppInstaller::Manifest constexpr std::string_view s_InvalidPathFieldCharacters = "\\/:*?\"<>|"sv; // The maximum length declared by the manifest schema for fields that are used to construct file system paths. + // The schema limit is expressed in characters rather than bytes, so the value is measured with the ICU + // helpers instead of by the size of its UTF-8 encoding. This also matches how MakeSuitablePathPart measures + // the values that these fields are converted into. constexpr size_t s_MaxPathFieldLength = 128; // Validates a manifest field value that is used to construct file system paths. @@ -135,11 +139,13 @@ namespace AppInstaller::Manifest std::string fieldNameString{ fieldName }; std::string valueString{ value }; - if (value.length() > s_MaxPathFieldLength) + if (Utility::UTF8Length(value) > s_MaxPathFieldLength) { resultErrors.emplace_back(ManifestError::FieldExceedsMaxLength, fieldNameString, valueString); } + // Every character excluded below is in the ASCII range, and the bytes of a multi byte UTF-8 sequence + // are all 0x80 or greater, so iterating the encoded bytes cannot produce a false match here. for (char character : value) { auto rawCharacter = static_cast(character); From 64e69d01c6f3ae201c0ab517b862165bf8898f8f Mon Sep 17 00:00:00 2001 From: JohnMcPMS Date: Fri, 25 Sep 2026 11:35:29 -0700 Subject: [PATCH 13/14] remove spaces in suitable path --- src/AppInstallerCLITests/Strings.cpp | 24 +++++++++++++++++++ .../AppInstallerStrings.cpp | 22 ++++++++++++++++- .../Public/AppInstallerStrings.h | 5 +++- 3 files changed, 49 insertions(+), 2 deletions(-) diff --git a/src/AppInstallerCLITests/Strings.cpp b/src/AppInstallerCLITests/Strings.cpp index a8785d2181..249926dada 100644 --- a/src/AppInstallerCLITests/Strings.cpp +++ b/src/AppInstallerCLITests/Strings.cpp @@ -207,6 +207,30 @@ TEST_CASE("MakeSuitablePathPart", "[strings]") // Only the exact superscript digits are reserved; other trailing values are not. REQUIRE(MakeSuitablePathPart("COM\xC2\xB4") == "COM\xC2\xB4"); REQUIRE(MakeSuitablePathPart("COM\xC2\xB9" "0") == "COM\xC2\xB9" "0"); + + // Win32 removes trailing spaces when normalizing a path, so they are removed here too. Otherwise two + // values that differ only by trailing spaces would collide on disk while appearing distinct. + REQUIRE(MakeSuitablePathPart("AB ") == "AB"); + REQUIRE(MakeSuitablePathPart("AB ") == "AB"); + REQUIRE(MakeSuitablePathPart("A B") == "A B"); + REQUIRE(MakeSuitablePathPart(" AB") == " AB"); + + // Removing the trailing spaces can expose a . at the end of the name, which is also not allowed. + REQUIRE(MakeSuitablePathPart("AB. ") == "AB_"); + REQUIRE(MakeSuitablePathPart("AB. ") == "AB_"); + REQUIRE(MakeSuitablePathPart("AB. . ") == "AB. _"); + + // A reserved name is still reserved once the trailing spaces are removed, as Win32 would remove them + // before resolving the name. + REQUIRE_THROWS_HR(MakeSuitablePathPart("CON "), E_INVALIDARG); + REQUIRE_THROWS_HR(MakeSuitablePathPart("NUL.txt "), E_INVALIDARG); + + // A candidate that is left with nothing cannot be used as a path part. + REQUIRE_THROWS_HR(MakeSuitablePathPart(" "), E_INVALIDARG); + REQUIRE_THROWS_HR(MakeSuitablePathPart(" "), E_INVALIDARG); + + // An empty candidate is unchanged; emptiness is the caller's concern. + REQUIRE(MakeSuitablePathPart("") == ""); } TEST_CASE("GetFileNameFromURI", "[strings]") diff --git a/src/AppInstallerSharedLib/AppInstallerStrings.cpp b/src/AppInstallerSharedLib/AppInstallerStrings.cpp index 930cc02c19..3261470157 100644 --- a/src/AppInstallerSharedLib/AppInstallerStrings.cpp +++ b/src/AppInstallerSharedLib/AppInstallerStrings.cpp @@ -711,6 +711,8 @@ namespace AppInstaller::Utility // invalid characters in a candidate path part. // Additionally, based on https://docs.microsoft.com/en-us/windows/win32/fileio/filesystem-functionality-comparison#limits // limit the number of characters to 255. + // Trailing spaces and dots are also handled, as Win32 removes those when it normalizes a path and the + // result of this function is expected to be the name that the file system actually uses. std::string MakeSuitablePathPart(std::string_view candidate) { constexpr char replaceChar = '_'; @@ -761,7 +763,25 @@ namespace AppInstaller::Utility return SHA256::ConvertToString(SHA256::ComputeHash(candidate)); } - // Second, look for any newly formed illegal names. + // Second, remove any trailing spaces. Win32 removes these when it normalizes a path, so leaving them + // would mean that the value we hand out is not the one that the file system actually uses. Two values + // that differ only by trailing spaces would then collide on disk while appearing distinct here. + // Only the space needs to be considered, as every other whitespace character is a control character + // that was already replaced above. + size_t lastKeptCharacter = result.find_last_not_of(' '); + result.erase(lastKeptCharacter + 1); + + // Removing the trailing spaces can expose a . at the end of the name, which Win32 also removes. + // The loop above only sees the final character of the candidate, so handle that here. + if (!result.empty() && result.back() == '.') + { + result.back() = replaceChar; + } + + // A candidate that consists only of characters that are removed here cannot be used as a path part. + THROW_HR_IF(E_INVALIDARG, result.empty() && !candidate.empty()); + + // Third, look for any newly formed illegal names. // For now just error on these cases; they should not happen often. // The COM/LPT names using the superscript digits (U+00B9, U+00B2, U+00B3) are reserved as well; they are // written here as explicit UTF-8 byte sequences so that the encoding of this file cannot alter them. diff --git a/src/AppInstallerSharedLib/Public/AppInstallerStrings.h b/src/AppInstallerSharedLib/Public/AppInstallerStrings.h index a04b057c43..809f76b8fd 100644 --- a/src/AppInstallerSharedLib/Public/AppInstallerStrings.h +++ b/src/AppInstallerSharedLib/Public/AppInstallerStrings.h @@ -201,7 +201,10 @@ namespace AppInstaller::Utility // Expands environment variables within the input. std::wstring ExpandEnvironmentVariables(const std::wstring& input); - // Converts the candidate path part into one suitable for the actual file system + // Converts the candidate path part into one suitable for the actual file system. + // Illegal characters are replaced, and trailing spaces and dots are removed or replaced so that the + // result matches what Win32 would normalize the path to. Throws E_INVALIDARG for a candidate that + // cannot be represented, such as a reserved device name or a value that is entirely trailing spaces. std::string MakeSuitablePathPart(std::string_view candidate); // Splits the file name part off of the given URI. From 80e0317ed780aa504eabebb20ceedd3fe4ccde0c Mon Sep 17 00:00:00 2001 From: JohnMcPMS Date: Wed, 30 Sep 2026 15:25:49 -0700 Subject: [PATCH 14/14] pr feedback --- src/AppInstallerCLITests/YamlManifest.cpp | 105 ++++++++---------- .../Manifest/ManifestValidation.cpp | 2 - 2 files changed, 49 insertions(+), 58 deletions(-) diff --git a/src/AppInstallerCLITests/YamlManifest.cpp b/src/AppInstallerCLITests/YamlManifest.cpp index 6a27eeb2c6..c9d353861e 100644 --- a/src/AppInstallerCLITests/YamlManifest.cpp +++ b/src/AppInstallerCLITests/YamlManifest.cpp @@ -41,14 +41,23 @@ namespace void ValidateError( const ValidationError& error, ValidationError::Level level, - AppInstaller::StringResource::StringId message, - std::string field, - std::string value) + std::optional message, + std::optional field, + std::optional value) { REQUIRE(level == error.ErrorLevel); - REQUIRE(message == error.Message); - REQUIRE(field == error.Context); - REQUIRE(value == error.Value); + if (message) + { + REQUIRE(message.value() == error.Message); + } + if (field) + { + REQUIRE(field.value() == error.Context); + } + if (value) + { + REQUIRE(value.value() == error.Value); + } } void ValidateError(const ValidationError& error, ValidationError::Level level, AppInstaller::StringResource::StringId message) @@ -56,6 +65,24 @@ namespace ValidateError(error, level, message, std::string(), std::string()); } + void RequireSingleError( + const std::vector& errors, + std::optional message = std::nullopt, + std::optional field = std::nullopt, + std::optional value = std::nullopt) + { + REQUIRE(errors.size() == 1); + ValidateError(errors[0], ValidationError::Level::Error, message, field, value); + } + + bool ContainsError(const std::vector& errors, AppInstaller::StringResource::StringId message) + { + return std::any_of(errors.begin(), errors.end(), [&](const ValidationError& error) + { + return error.Message == message && error.ErrorLevel == ValidationError::Level::Error; + }); + } + std::vector ValidateManifest(const Manifest& manifest, bool fullValidation) { return ValidateManifest(manifest, ManifestValidateOption{ fullValidation }); @@ -1389,33 +1416,16 @@ TEST_CASE("ManifestLocalizationValidation", "[ManifestValidation]") manifest.Localizations.at(0).Locale = "Invalid"; // Full validation should detect as error - auto errors = ValidateManifest(manifest, true); - REQUIRE(errors.size() == 1); - REQUIRE(errors.at(0).ErrorLevel == ValidationError::Level::Error); + RequireSingleError(ValidateManifest(manifest, true)); // Not full validation should detect as warning - errors = ValidateManifest(manifest, false); + auto errors = ValidateManifest(manifest, false); REQUIRE(errors.size() == 1); REQUIRE(errors.at(0).ErrorLevel == ValidationError::Level::Warning); } TEST_CASE("PathFieldValueValidation", "[ManifestValidation]") { - auto RequireSingleError = [](const std::vector& errors, AppInstaller::StringResource::StringId message) - { - REQUIRE(errors.size() == 1); - REQUIRE(ValidationError::Level::Error == errors[0].ErrorLevel); - REQUIRE(message == errors[0].Message); - }; - - auto ContainsError = [](const std::vector& errors, AppInstaller::StringResource::StringId message) - { - return std::any_of(errors.begin(), errors.end(), [&](const ValidationError& error) - { - return error.Message == message && error.ErrorLevel == ValidationError::Level::Error; - }); - }; - // Valid values produce no errors. REQUIRE(ValidatePackageVersion("1.0.0").empty()); REQUIRE(ValidatePackageVersion("1.0 beta").empty()); @@ -1424,8 +1434,7 @@ TEST_CASE("PathFieldValueValidation", "[ManifestValidation]") // Whitespace is only excluded for the fields that require it. auto errors = ValidatePackageIdentifier("Foo Bar"); - REQUIRE(errors.size() == 1); - ValidateError(errors[0], ValidationError::Level::Error, ManifestError::InvalidPathCharacters, "PackageIdentifier", "Foo Bar"); + RequireSingleError(errors, ManifestError::InvalidPathCharacters, "PackageIdentifier", "Foo Bar"); // Empty values are covered by the required field validation. REQUIRE(ValidatePackageVersion("").empty()); @@ -1505,20 +1514,17 @@ TEST_CASE("PackageIdentifierAndVersionPathValidation", "[ManifestValidation]") // against the schema (for example, those from a REST source) are only checked here. manifest.Id = "Foo\\Bar"; auto errors = ValidateManifest(manifest, false); - REQUIRE(errors.size() == 1); - ValidateError(errors[0], ValidationError::Level::Error, ManifestError::InvalidPathCharacters, "PackageIdentifier", manifest.Id); + RequireSingleError(errors, ManifestError::InvalidPathCharacters, "PackageIdentifier", manifest.Id); manifest = YamlParser::CreateFromPath(TestDataFile("Manifest-Good-InstallerTypeZip-PortableExeUppercase.yaml")); manifest.Version = "1.0:0"; errors = ValidateManifest(manifest, false); - REQUIRE(errors.size() == 1); - ValidateError(errors[0], ValidationError::Level::Error, ManifestError::InvalidPathCharacters, "PackageVersion", manifest.Version); + RequireSingleError(errors, ManifestError::InvalidPathCharacters, "PackageVersion", manifest.Version); manifest = YamlParser::CreateFromPath(TestDataFile("Manifest-Good-InstallerTypeZip-PortableExeUppercase.yaml")); manifest.Version = ".."; errors = ValidateManifest(manifest, false); - REQUIRE(errors.size() == 1); - ValidateError(errors[0], ValidationError::Level::Error, ManifestError::FieldEscapesDirectory, "PackageVersion", manifest.Version); + RequireSingleError(errors, ManifestError::FieldEscapesDirectory, "PackageVersion", manifest.Version); } TEST_CASE("ManifestGetPathPart", "[ManifestValidation]") @@ -1573,16 +1579,12 @@ TEST_CASE("PortableFileTypeValidation", "[ManifestValidation]") Manifest uppercaseManifest = YamlParser::CreateFromPath(TestDataFile("Manifest-Good-InstallerTypeZip-PortableExeUppercase.yaml")); // Regular validation should detect as error - auto errors = ValidateManifest(installerManifest, true); - REQUIRE(errors.size() == 1); - REQUIRE(errors.at(0).ErrorLevel == ValidationError::Level::Error); + RequireSingleError(ValidateManifest(installerManifest, true)); - errors = ValidateManifest(rootManifest, true); - REQUIRE(errors.size() == 1); - REQUIRE(errors.at(0).ErrorLevel == ValidationError::Level::Error); + RequireSingleError(ValidateManifest(rootManifest, true)); // Should not error when full validation is set to false - errors = ValidateManifest(installerManifest, false); + auto errors = ValidateManifest(installerManifest, false); REQUIRE(errors.size() == 0); errors = ValidateManifest(rootManifest, false); @@ -1599,12 +1601,10 @@ TEST_CASE("WindowsFeatureNameValidation", "[ManifestValidation][111981]") Manifest invalidManifest = YamlParser::CreateFromPath(TestDataFile("Manifest-Bad-InvalidWindowsFeatureName.yaml")); auto errors = ValidateManifest(invalidManifest, true); - REQUIRE(errors.size() == 1); - ValidateError(errors[0], ValidationError::Level::Error, ManifestError::InvalidWindowsFeatureName, "Invalid@Feature", ""); + RequireSingleError(errors, ManifestError::InvalidWindowsFeatureName, "Invalid@Feature", ""); errors = ValidateManifest(invalidManifest, false); - REQUIRE(errors.size() == 1); - ValidateError(errors[0], ValidationError::Level::Error, ManifestError::InvalidWindowsFeatureName, "Invalid@Feature", ""); + RequireSingleError(errors, ManifestError::InvalidWindowsFeatureName, "Invalid@Feature", ""); } TEST_CASE("NetworkAddressInSwitchesValidation", "[ManifestValidation][111981]") @@ -1618,8 +1618,7 @@ TEST_CASE("NetworkAddressInSwitchesValidation", "[ManifestValidation][111981]") ManifestValidateOption options{ true }; options.ErrorOnNetworkAddressInSwitches = true; errors = ValidateManifest(manifest, options); - REQUIRE(errors.size() == 1); - ValidateError(errors[0], ValidationError::Level::Error, ManifestError::ContainsNetworkAddress, "http://evil.example.com", ""); + RequireSingleError(errors, ManifestError::ContainsNetworkAddress, "http://evil.example.com", ""); errors = ValidateManifest(manifest, false); REQUIRE(errors.size() == 0); @@ -1632,8 +1631,7 @@ TEST_CASE("BlockedMsiPropertyValidation", "[ManifestValidation][111981]") Manifest manifest = YamlParser::CreateFromPath(TestDataFile("Manifest-Bad-BlockedMsiProperty.yaml")); auto errors = ValidateManifest(manifest, true); - REQUIRE(errors.size() == 1); - ValidateError(errors[0], ValidationError::Level::Error, ManifestError::BlockedMsiProperty, "TRANSFORMS", ""); + RequireSingleError(errors, ManifestError::BlockedMsiProperty, "TRANSFORMS", ""); // Not checked when fullValidation is false errors = ValidateManifest(manifest, false); @@ -1645,8 +1643,7 @@ TEST_CASE("BlockedMsiPropertyValidation", "[ManifestValidation][111981]") Manifest manifest = YamlParser::CreateFromPath(TestDataFile("Manifest-Bad-InvalidMsiSwitches.yaml")); auto errors = ValidateManifest(manifest, true); - REQUIRE(errors.size() == 1); - ValidateError(errors[0], ValidationError::Level::Error, ManifestError::InvalidMsiSwitches); + RequireSingleError(errors, ManifestError::InvalidMsiSwitches, "", ""); // Not checked when fullValidation is false errors = ValidateManifest(manifest, false); @@ -1699,9 +1696,7 @@ TEST_CASE("ReadManifestAndValidateMsixInstallers_NoSupportedPlatforms", "[Manife manifest.Installers[0].Url = msixFile.GetPath().u8string(); auto errors = ValidateManifestInstallers(manifest); - REQUIRE(1 == errors.size()); - - ValidateError(errors[0], ValidationError::Level::Error, ManifestError::NoSupportedPlatforms, "InstallerUrl", manifest.Installers.front().Url); + RequireSingleError(errors, ManifestError::NoSupportedPlatforms, "InstallerUrl", manifest.Installers.front().Url); } TEST_CASE("ReadManifestAndValidateMsixInstallers_PackageVersionNotUINT64", "[ManifestValidation]") @@ -1714,9 +1709,7 @@ TEST_CASE("ReadManifestAndValidateMsixInstallers_PackageVersionNotUINT64", "[Man manifest.Installers[0].Url = msixFile.GetPath().u8string(); auto errors = ValidateManifestInstallers(manifest); - REQUIRE(1 == errors.size()); - - ValidateError(errors[0], ValidationError::Level::Error, ManifestError::InstallerMsixInconsistencies, "PackageVersion", "43690.48059.52428.56797"); + RequireSingleError(errors, ManifestError::InstallerMsixInconsistencies, "PackageVersion", "43690.48059.52428.56797"); } TEST_CASE("ReadManifestAndValidateMsixInstallers_MissingFields", "[ManifestValidation]") diff --git a/src/AppInstallerCommonCore/Manifest/ManifestValidation.cpp b/src/AppInstallerCommonCore/Manifest/ManifestValidation.cpp index fc96f3543a..71f946871a 100644 --- a/src/AppInstallerCommonCore/Manifest/ManifestValidation.cpp +++ b/src/AppInstallerCommonCore/Manifest/ManifestValidation.cpp @@ -198,8 +198,6 @@ namespace AppInstaller::Manifest bool IsValueSafeForPathConstruction(std::string_view value) { - // Whitespace is excluded here because it is not a path safety concern; it is only excluded from - // the fields whose schema definition happens to exclude it. return ValidateFieldValueUsedInPathConstruction({}, value, /* disallowWhitespace */ false).empty(); }