From 2d82b5f261eef2a7a72e3e5e1b92133765b75d96 Mon Sep 17 00:00:00 2001 From: JohnMcPMS Date: Tue, 27 Oct 2026 16:49:13 -0700 Subject: [PATCH 1/2] Hash requirements --- .../1.31/require-rest-installer-hash.md | 5 ++ .../Workflows/DownloadFlow.cpp | 7 +- src/AppInstallerCLITests/InstallFlow.cpp | 9 ++- .../RestInterface_1_0.cpp | 64 +++++++++++++++++++ src/AppInstallerCLITests/WorkFlow.cpp | 52 ++++++++++++++- src/AppInstallerCLITests/WorkflowCommon.cpp | 21 ++++-- src/AppInstallerCLITests/WorkflowCommon.h | 3 + .../AppInstallerTelemetry.cpp | 4 +- .../1_0/Json/ManifestDeserializer_1_0.cpp | 18 ++++-- 9 files changed, 162 insertions(+), 21 deletions(-) create mode 100644 doc/release-notes/1.31/require-rest-installer-hash.md diff --git a/doc/release-notes/1.31/require-rest-installer-hash.md b/doc/release-notes/1.31/require-rest-installer-hash.md new file mode 100644 index 0000000000..fbbcc6b454 --- /dev/null +++ b/doc/release-notes/1.31/require-rest-installer-hash.md @@ -0,0 +1,5 @@ +## Bug Fixes + +### Minor Bug Fixes + +* Ignore REST installers missing an installer hash, except for Microsoft Store installers, and reject missing or incomplete hashes during installer verification. diff --git a/src/AppInstallerCLICore/Workflows/DownloadFlow.cpp b/src/AppInstallerCLICore/Workflows/DownloadFlow.cpp index 18ea847de2..0a0db10b27 100644 --- a/src/AppInstallerCLICore/Workflows/DownloadFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/DownloadFlow.cpp @@ -543,10 +543,11 @@ namespace AppInstaller::CLI::Workflow { const auto& [expectedHash, downloadResult] = context.Get(); - if (!std::equal( + if (downloadResult.Sha256Hash.empty() || !std::equal( + downloadResult.Sha256Hash.begin(), + downloadResult.Sha256Hash.end(), expectedHash.begin(), - expectedHash.end(), - downloadResult.Sha256Hash.begin())) + expectedHash.end())) { bool overrideHashMismatch = context.Args.Contains(Execution::Args::Type::HashOverride); diff --git a/src/AppInstallerCLITests/InstallFlow.cpp b/src/AppInstallerCLITests/InstallFlow.cpp index 30bf3da050..1456884fc8 100644 --- a/src/AppInstallerCLITests/InstallFlow.cpp +++ b/src/AppInstallerCLITests/InstallFlow.cpp @@ -39,7 +39,8 @@ void OverrideForDirectMsi(TestContext& context) context.Override({ DownloadInstallerFile, [](TestContext& context) { - context.Add({ {}, {} }); + const auto& fileHash = GetTestExeInstallerHash(); + context.Add({ fileHash, DownloadResult{ fileHash } }); // We don't have an msi installer for tests, but we won't execute it anyway context.Add(TestDataFile("AppInstallerTestExeInstaller.exe")); } }); @@ -97,7 +98,8 @@ TEST_CASE("InstallFlow_RenameFromEncodedUrl", "[InstallFlow][workflow]") OverrideForCheckExistingInstaller(context); context.Override({ DownloadInstallerFile, [](TestContext& context) { - context.Add({ {}, {} }); + const auto& fileHash = GetTestExeInstallerHash(); + context.Add({ fileHash, DownloadResult{ fileHash } }); auto installerPath = std::filesystem::temp_directory_path(); installerPath /= "EncodedUrlTest.exe"; std::filesystem::copy(TestDataFile("AppInstallerTestExeInstaller.exe"), installerPath, std::filesystem::copy_options::overwrite_existing); @@ -129,7 +131,8 @@ TEST_CASE("InstallFlow_RenameFromInvalidFileCharacterUrl", "[InstallFlow][workfl OverrideForCheckExistingInstaller(context); context.Override({ DownloadInstallerFile, [](TestContext& context) { - context.Add({ {}, {} }); + const auto& fileHash = GetTestExeInstallerHash(); + context.Add({ fileHash, DownloadResult{ fileHash } }); auto installerPath = std::filesystem::temp_directory_path(); installerPath /= "InvalidFileCharacterUrlTest.exe"; std::filesystem::copy(TestDataFile("AppInstallerTestExeInstaller.exe"), installerPath, std::filesystem::copy_options::overwrite_existing); diff --git a/src/AppInstallerCLITests/RestInterface_1_0.cpp b/src/AppInstallerCLITests/RestInterface_1_0.cpp index 25390594d0..49b2c55f96 100644 --- a/src/AppInstallerCLITests/RestInterface_1_0.cpp +++ b/src/AppInstallerCLITests/RestInterface_1_0.cpp @@ -6,6 +6,7 @@ #include #include #include +#include #include #include #include @@ -2270,6 +2271,68 @@ TEST_CASE("GetManifests_BadResponse_SuccessCode", "[RestSource][Interface_1_0]") REQUIRE_THROWS_HR(v1.GetManifests("Foo.Bar"), APPINSTALLER_CLI_ERROR_RESTSOURCE_INVALID_DATA); } +TEST_CASE("GetManifests_BadResponse_InstallerHash", "[RestSource][Interface_1_0]") +{ + auto response = web::json::value::parse(GetGoodManifest_RequiredFields()); + auto& installers = response[L"Data"][L"Versions"][0][L"Installers"]; + auto& installer = installers[0]; + + SECTION("Missing hash") + { + installer.as_object().erase(L"InstallerSha256"); + } + SECTION("Empty hash") + { + installer[L"InstallerSha256"] = web::json::value::string(L""); + } + SECTION("Null hash") + { + installer[L"InstallerSha256"] = web::json::value::null(); + } + SECTION("Non-string hash") + { + installer[L"InstallerSha256"] = web::json::value::number(42); + } + HttpClientHelper helper{ GetTestRestRequestHandler(web::http::status_codes::OK, response.serialize()) }; + Interface v1{ TestRestUriString, std::move(helper) }; + REQUIRE_THROWS_HR(v1.GetManifests("Foo.Bar"), APPINSTALLER_CLI_ERROR_RESTSOURCE_INVALID_DATA); + REQUIRE_THROWS_HR(V1_28::Json::ManifestDeserializer{}.Deserialize(response), APPINSTALLER_CLI_ERROR_RESTSOURCE_INVALID_DATA); +} + +TEST_CASE("GetManifests_SkipsInstallerWithoutHash", "[RestSource][Interface_1_0][Interface_1_28]") +{ + auto response = web::json::value::parse(GetGoodManifest_RequiredFields()); + auto& installers = response[L"Data"][L"Versions"][0][L"Installers"]; + auto validInstaller = installers[0]; + installers[0].as_object().erase(L"InstallerSha256"); + installers[1] = std::move(validInstaller); + + HttpClientHelper helper{ GetTestRestRequestHandler(web::http::status_codes::OK, response.serialize()) }; + Interface v1{ TestRestUriString, std::move(helper) }; + for (const auto& manifests : { v1.GetManifests("Foo.Bar"), V1_28::Json::ManifestDeserializer{}.Deserialize(response) }) + { + REQUIRE(manifests.size() == 1); + REQUIRE(manifests[0].Installers.size() == 1); + REQUIRE(manifests[0].Installers[0].Sha256 == SHA256::ConvertToBytes("011048877dfaef109801b3f3ab2b60afc74f3fc4f7b3430e0c897f5da1df84b6")); + } +} + +TEST_CASE("ManifestDeserializer_MSStoreWithoutInstallerHash", "[RestSource][Interface_1_28]") +{ + auto response = web::json::value::parse(GetGoodManifest_RequiredFields()); + auto& installer = response[L"Data"][L"Versions"][0][L"Installers"][0]; + installer[L"InstallerType"] = web::json::value::string(L"msstore"); + installer[L"MSStoreProductIdentifier"] = web::json::value::string(L"9nblggh4nns1"); + installer.as_object().erase(L"InstallerUrl"); + installer.as_object().erase(L"InstallerSha256"); + + auto manifests = V1_28::Json::ManifestDeserializer{}.Deserialize(response); + REQUIRE(manifests.size() == 1); + REQUIRE(manifests[0].Installers.size() == 1); + REQUIRE(manifests[0].Installers[0].BaseInstallerType == InstallerTypeEnum::MSStore); + REQUIRE(manifests[0].Installers[0].Sha256.empty()); +} + TEST_CASE("GetManifests_NotFoundCode", "[RestSource][Interface_1_0]") { HttpClientHelper helper{ GetTestRestRequestHandler(web::http::status_codes::NotFound) }; @@ -2296,6 +2359,7 @@ TEST_CASE("GetManifests_GoodResponse_UnknownInstaller", "[RestSource][Interface_ "Installers": [ { "Architecture": "x64", + "InstallerSha256": "011048877dfaef109801b3f3ab2b60afc74f3fc4f7b3430e0c897f5da1df84b6", "InstallerType": "msstore", "MSStoreProductIdentifier": "9nblggh4nns1" } diff --git a/src/AppInstallerCLITests/WorkFlow.cpp b/src/AppInstallerCLITests/WorkFlow.cpp index 0a6795cb23..2f56924ed1 100644 --- a/src/AppInstallerCLITests/WorkFlow.cpp +++ b/src/AppInstallerCLITests/WorkFlow.cpp @@ -45,7 +45,8 @@ TEST_CASE("VerifyInstallerTrustLevelAndUpdateInstallerFileMotw", "[DownloadInsta std::ostringstream updateMotwOutput; TestContext context{ updateMotwOutput, std::cin }; auto previousThreadGlobals = context.SetForCurrentThread(); - context.Add({ {}, {} }); + auto installerHash = SHA256::ComputeHashFromFile(testInstallerPath); + context.Add({ installerHash, DownloadResult{ installerHash } }); context.Add(testInstallerPath); auto packageVersion = std::make_shared(Manifest{}); auto testSource = std::make_shared(); @@ -69,6 +70,55 @@ TEST_CASE("VerifyInstallerTrustLevelAndUpdateInstallerFileMotw", "[DownloadInsta INFO(updateMotwOutput.str()); } +TEST_CASE("VerifyInstallerHash_RejectsMissingOrMismatchedHash", "[DownloadInstaller][workflow]") +{ + auto expectedHash = SHA256::ComputeHash("test"sv); + DownloadResult downloadResult{ expectedHash }; + downloadResult.SizeInBytes = 4; + + SECTION("Missing expected hash") + { + expectedHash.clear(); + } + SECTION("Truncated expected hash") + { + expectedHash.pop_back(); + } + SECTION("Overlong expected hash") + { + expectedHash.push_back(0); + } + SECTION("Mismatched hash") + { + expectedHash[0] ^= 0xFF; + } + SECTION("Missing downloaded hash") + { + downloadResult.Sha256Hash.clear(); + } + SECTION("Truncated downloaded hash") + { + downloadResult.Sha256Hash.pop_back(); + } + SECTION("Both hashes missing") + { + expectedHash.clear(); + downloadResult.Sha256Hash.clear(); + } + + std::ostringstream output; + TestContext context{ output, std::cin }; + auto previousThreadGlobals = context.SetForCurrentThread(); + context.Add(Manifest{}); + context.Add({ expectedHash, downloadResult }); + + context << VerifyInstallerHash; + + REQUIRE_TERMINATED_WITH(context, APPINSTALLER_CLI_ERROR_INSTALLER_HASH_MISMATCH); + REQUIRE_FALSE(WI_IsFlagSet(context.GetFlags(), ContextFlag::InstallerHashMatched)); + REQUIRE_FALSE(WI_IsFlagSet(context.GetFlags(), ContextFlag::InstallerTrusted)); +} + TEST_CASE("ValidateCommand_Dependencies", "[workflow][dependencies]") { std::ostringstream validateOutput; diff --git a/src/AppInstallerCLITests/WorkflowCommon.cpp b/src/AppInstallerCLITests/WorkflowCommon.cpp index 93a42282a4..40863f0d54 100644 --- a/src/AppInstallerCLITests/WorkflowCommon.cpp +++ b/src/AppInstallerCLITests/WorkflowCommon.cpp @@ -552,13 +552,20 @@ namespace TestCommon } }); } + const SHA256::HashBuffer& GetTestExeInstallerHash() + { + static const auto hash = SHA256::ComputeHashFromFile(TestDataFile("AppInstallerTestExeInstaller.exe")); + return hash; + } + void OverrideForShellExecute(TestContext& context, int expectedUseCount) { OverrideForCheckExistingInstaller(context); context.Override({ DownloadInstallerFile, [](TestContext& context) { - context.Add({ {}, {} }); + const auto& fileHash = GetTestExeInstallerHash(); + context.Add({ fileHash, DownloadResult{ fileHash } }); context.Add(TestDataFile("AppInstallerTestExeInstaller.exe")); }, expectedUseCount }); @@ -573,7 +580,8 @@ namespace TestCommon { context.Override({ DownloadInstallerFile, [&installationLog](TestContext& context) { - context.Add({ {}, {} }); + const auto& fileHash = GetTestExeInstallerHash(); + context.Add({ fileHash, DownloadResult{ fileHash } }); context.Add(TestDataFile("AppInstallerTestExeInstaller.exe")); auto dependency = Dependency(DependencyType::Package, context.Get().Id, context.Get().Version); @@ -602,7 +610,8 @@ namespace TestCommon { context.Override({ DownloadInstallerFile, [](TestContext& context) { - context.Add({ {}, {} }); + const auto& fileHash = GetTestExeInstallerHash(); + context.Add({ fileHash, DownloadResult{ fileHash } }); context.Add(TestDataFile("AppInstallerTestExeInstaller.exe")); } }); @@ -624,8 +633,7 @@ namespace TestCommon std::filesystem::copy(installerPath, tempInstallerPath, std::filesystem::copy_options::overwrite_existing); context.Add(tempInstallerPath); - std::ifstream inStream{ tempInstallerPath, std::ifstream::binary }; - SHA256::HashBuffer fileHash = SHA256::ComputeHash(inStream); + const auto& fileHash = GetTestExeInstallerHash(); context.Add({ fileHash, DownloadResult{ fileHash } }); } }); @@ -732,7 +740,8 @@ namespace TestCommon std::ofstream file(installerPath, std::ofstream::out | std::ofstream::trunc); file << installer.Url; file.close(); - context.Add({ {}, {} }); + auto fileHash = SHA256::ComputeHashFromFile(installerPath); + context.Add({ fileHash, DownloadResult{ fileHash } }); } }); } } diff --git a/src/AppInstallerCLITests/WorkflowCommon.h b/src/AppInstallerCLITests/WorkflowCommon.h index e4f6333780..64fccef531 100644 --- a/src/AppInstallerCLITests/WorkflowCommon.h +++ b/src/AppInstallerCLITests/WorkflowCommon.h @@ -3,6 +3,7 @@ #include "pch.h" #include "TestCommon.h" #include "TestSource.h" +#include #include #include #include @@ -116,6 +117,8 @@ namespace TestCommon void OverrideForCheckExistingInstaller(TestContext& context); + const AppInstaller::Utility::SHA256::HashBuffer& GetTestExeInstallerHash(); + void OverrideForShellExecute(TestContext& context, int expectedUseCount = -1); void OverrideForShellExecute(TestContext& context, std::vector& installationLog); diff --git a/src/AppInstallerCommonCore/AppInstallerTelemetry.cpp b/src/AppInstallerCommonCore/AppInstallerTelemetry.cpp index 43680f5316..3120f6c374 100644 --- a/src/AppInstallerCommonCore/AppInstallerTelemetry.cpp +++ b/src/AppInstallerCommonCore/AppInstallerTelemetry.cpp @@ -599,9 +599,9 @@ namespace AppInstaller::Logging AICLI_LOG(CLI, Error, << "Package hash verification failed. SHA256 in manifest [" - << Utility::SHA256::ConvertToString(expected) + << Utility::ConvertToHexString(expected) << "] does not match download [" - << Utility::SHA256::ConvertToString(actual) + << Utility::ConvertToHexString(actual) << "] with file size [" << downloadSizeInBytes << "] and content type [" << actualContentType << "]"); } 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 6b9e843cb6..f514e83fb0 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 @@ -324,12 +324,6 @@ namespace AppInstaller::Repository::Rest::Schema::V1_0::Json installer.Url = JSON::GetRawStringValueFromJsonNode(installerJsonObject, JSON::GetUtilityString(InstallerUrl)).value_or(""); - std::optional sha256 = JSON::GetRawStringValueFromJsonNode(installerJsonObject, JSON::GetUtilityString(InstallerSha256)); - if (JSON::IsValidNonEmptyStringValue(sha256)) - { - installer.Sha256 = Utility::SHA256::ConvertToBytes(sha256.value()); - } - std::optional arch = JSON::GetRawStringValueFromJsonNode(installerJsonObject, JSON::GetUtilityString(Architecture)); if (!JSON::IsValidNonEmptyStringValue(arch)) { @@ -345,6 +339,18 @@ namespace AppInstaller::Repository::Rest::Schema::V1_0::Json return {}; } installer.BaseInstallerType = ConvertToInstallerType(installerType.value()); + + std::optional sha256 = JSON::GetRawStringValueFromJsonNode(installerJsonObject, JSON::GetUtilityString(InstallerSha256)); + if (JSON::IsValidNonEmptyStringValue(sha256)) + { + installer.Sha256 = Utility::SHA256::ConvertToBytes(sha256.value()); + } + else if (installer.BaseInstallerType != InstallerTypeEnum::MSStore) + { + AICLI_LOG(Repo, Error, << "Missing installer SHA256."); + return {}; + } + installer.Locale = JSON::GetRawStringValueFromJsonNode(installerJsonObject, JSON::GetUtilityString(InstallerLocale)).value_or(""); // platform From 8f1072a88fc3219f9bfd3d2d52fbe04f081eae85 Mon Sep 17 00:00:00 2001 From: JohnMcPMS Date: Tue, 27 Oct 2026 16:59:12 -0700 Subject: [PATCH 2/2] semantic function --- src/AppInstallerCommonCore/Manifest/ManifestCommon.cpp | 5 +++++ src/AppInstallerCommonCore/Manifest/ManifestValidation.cpp | 2 +- src/AppInstallerCommonCore/Public/winget/ManifestCommon.h | 3 +++ .../Rest/Schema/1_0/Json/ManifestDeserializer_1_0.cpp | 2 +- 4 files changed, 10 insertions(+), 2 deletions(-) diff --git a/src/AppInstallerCommonCore/Manifest/ManifestCommon.cpp b/src/AppInstallerCommonCore/Manifest/ManifestCommon.cpp index 18fb24e5fa..ac78dfe303 100644 --- a/src/AppInstallerCommonCore/Manifest/ManifestCommon.cpp +++ b/src/AppInstallerCommonCore/Manifest/ManifestCommon.cpp @@ -964,6 +964,11 @@ namespace AppInstaller::Manifest installerType == InstallerTypeEnum::Exe; } + bool DoesInstallerTypeRequireHash(InstallerTypeEnum installerType) + { + return installerType != InstallerTypeEnum::MSStore; + } + bool DoesInstallerTypeUseMsiProperties(InstallerTypeEnum installerType) { return diff --git a/src/AppInstallerCommonCore/Manifest/ManifestValidation.cpp b/src/AppInstallerCommonCore/Manifest/ManifestValidation.cpp index 71f946871a..5de5d54cf5 100644 --- a/src/AppInstallerCommonCore/Manifest/ManifestValidation.cpp +++ b/src/AppInstallerCommonCore/Manifest/ManifestValidation.cpp @@ -360,7 +360,7 @@ namespace AppInstaller::Manifest { resultErrors.emplace_back(ManifestError::RequiredFieldMissing, "InstallerUrl"); } - if (installer.Sha256.empty()) + if (DoesInstallerTypeRequireHash(installer.EffectiveInstallerType()) && installer.Sha256.empty()) { resultErrors.emplace_back(ManifestError::RequiredFieldMissing, "InstallerSha256"); } diff --git a/src/AppInstallerCommonCore/Public/winget/ManifestCommon.h b/src/AppInstallerCommonCore/Public/winget/ManifestCommon.h index 8831d7cab8..33e28e7d58 100644 --- a/src/AppInstallerCommonCore/Public/winget/ManifestCommon.h +++ b/src/AppInstallerCommonCore/Public/winget/ManifestCommon.h @@ -503,6 +503,9 @@ namespace AppInstaller::Manifest // Gets a value indicating whether the given installer requires RepairBehavior for repair. bool DoesInstallerTypeRequireRepairBehaviorForRepair(InstallerTypeEnum installerType); + // Gets a value indicating whether the given installer type requires an installer hash. + bool DoesInstallerTypeRequireHash(InstallerTypeEnum installerType); + // Gets a value indicating whether the given installer type uses MSI properties in its command line. bool DoesInstallerTypeUseMsiProperties(InstallerTypeEnum installerType); 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 f514e83fb0..82f94c9cd2 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 @@ -345,7 +345,7 @@ namespace AppInstaller::Repository::Rest::Schema::V1_0::Json { installer.Sha256 = Utility::SHA256::ConvertToBytes(sha256.value()); } - else if (installer.BaseInstallerType != InstallerTypeEnum::MSStore) + else if (DoesInstallerTypeRequireHash(installer.BaseInstallerType)) { AICLI_LOG(Repo, Error, << "Missing installer SHA256."); return {};