From bff28036de553ce2bb97c7bba92a6bfd4b0fa456 Mon Sep 17 00:00:00 2001 From: John McPherson Date: Mon, 7 Oct 2024 15:17:21 -0700 Subject: [PATCH] More data from download, more telemetry on hash mismatch --- .github/actions/spelling/expect.txt | 3 + .../ExecutionContextData.h | 7 +- src/AppInstallerCLICore/Resources.h | 1 + .../Workflows/DownloadFlow.cpp | 41 ++++--- .../Workflows/MSStoreInstallerHandler.cpp | 4 +- .../AppInstallerCLIE2ETests.csproj | 1 + src/AppInstallerCLIE2ETests/Constants.cs | 2 + .../DownloadCommand.cs | 25 +++- .../Manifests/ZeroByteFile_CorrectHash.yaml | 14 +++ .../Manifests/ZeroByteFile_IncorrectHash.yaml | 14 +++ src/AppInstallerCLIE2ETests/TestData/empty | 0 .../Shared/Strings/en-us/winget.resw | 8 +- src/AppInstallerCLITests/Downloader.cpp | 18 +-- src/AppInstallerCLITests/InstallFlow.cpp | 6 +- src/AppInstallerCLITests/WorkFlow.cpp | 2 +- src/AppInstallerCLITests/WorkflowCommon.cpp | 10 +- .../AppInstallerTelemetry.cpp | 8 +- src/AppInstallerCommonCore/DODownloader.cpp | 46 +++++-- src/AppInstallerCommonCore/DODownloader.h | 3 +- src/AppInstallerCommonCore/Downloader.cpp | 112 +++++++++--------- src/AppInstallerCommonCore/FileCache.cpp | 6 +- .../Public/AppInstallerDownloader.h | 14 ++- .../Public/AppInstallerTelemetry.h | 4 +- src/AppInstallerSharedLib/Errors.cpp | 1 + .../Public/AppInstallerErrors.h | 1 + .../Public/AppInstallerSHA256.h | 9 ++ src/AppInstallerSharedLib/SHA256.cpp | 18 ++- src/WinGetUtil/Exports.cpp | 4 +- 28 files changed, 267 insertions(+), 115 deletions(-) create mode 100644 src/AppInstallerCLIE2ETests/TestData/Manifests/ZeroByteFile_CorrectHash.yaml create mode 100644 src/AppInstallerCLIE2ETests/TestData/Manifests/ZeroByteFile_IncorrectHash.yaml create mode 100644 src/AppInstallerCLIE2ETests/TestData/empty diff --git a/.github/actions/spelling/expect.txt b/.github/actions/spelling/expect.txt index c560fc664c..af16796812 100644 --- a/.github/actions/spelling/expect.txt +++ b/.github/actions/spelling/expect.txt @@ -126,6 +126,8 @@ decompressor dedupe deigh deleteifnotneeded +deliveryoptimization +deliveryoptimizationerrors DENYWR desktopappinstaller devhome @@ -404,6 +406,7 @@ PACL PARAMETERMAP pathparts Patil +pbstr pcb PCCERT PCs diff --git a/src/AppInstallerCLICore/ExecutionContextData.h b/src/AppInstallerCLICore/ExecutionContextData.h index 53d7c18406..4633b05d33 100644 --- a/src/AppInstallerCLICore/ExecutionContextData.h +++ b/src/AppInstallerCLICore/ExecutionContextData.h @@ -1,6 +1,7 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. #pragma once +#include #include #include #include @@ -34,7 +35,7 @@ namespace AppInstaller::CLI::Execution Manifest, PackageVersion, Installer, - HashPair, + DownloadHashInfo, InstallerPath, LogPath, InstallerArgs, @@ -128,9 +129,9 @@ namespace AppInstaller::CLI::Execution }; template <> - struct DataMapping + struct DataMapping { - using value_t = std::pair, std::vector>; + using value_t = std::pair, Utility::DownloadResult>; }; template <> diff --git a/src/AppInstallerCLICore/Resources.h b/src/AppInstallerCLICore/Resources.h index 04b932f07f..0e8407ab2e 100644 --- a/src/AppInstallerCLICore/Resources.h +++ b/src/AppInstallerCLICore/Resources.h @@ -308,6 +308,7 @@ namespace AppInstaller::CLI::Resource WINGET_DEFINE_RESOURCE_STRINGID(InstallersAbortTerminal); WINGET_DEFINE_RESOURCE_STRINGID(InstallersRequireInstallLocation); WINGET_DEFINE_RESOURCE_STRINGID(InstallerTypeArgumentDescription); + WINGET_DEFINE_RESOURCE_STRINGID(InstallerZeroByteFile); WINGET_DEFINE_RESOURCE_STRINGID(InstallFlowInstallSuccess); WINGET_DEFINE_RESOURCE_STRINGID(InstallFlowRegistrationDeferred); WINGET_DEFINE_RESOURCE_STRINGID(InstallFlowReturnCodeAlreadyInstalled); diff --git a/src/AppInstallerCLICore/Workflows/DownloadFlow.cpp b/src/AppInstallerCLICore/Workflows/DownloadFlow.cpp index 30a627f4a9..e056637600 100644 --- a/src/AppInstallerCLICore/Workflows/DownloadFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/DownloadFlow.cpp @@ -293,7 +293,7 @@ namespace AppInstaller::CLI::Workflow AICLI_LOG(CLI, Info, << "Existing installer file hash matches. Will use existing installer."); context.Add(installerPath / installerFilename); - context.Add(std::make_pair(installer.Sha256, fileHash)); + context.Add(std::make_pair(installer.Sha256, DownloadResult{ fileHash })); } void GetInstallerDownloadPath(Execution::Context& context) @@ -325,7 +325,7 @@ namespace AppInstaller::CLI::Workflow context.Reporter.Info() << Resource::String::Downloading << ' ' << Execution::UrlEmphasis << installer.Url << std::endl; - std::optional> hash; + DownloadResult downloadResult; constexpr int MaxRetryCount = 2; constexpr std::chrono::seconds maximumWaitTimeAllowed = 60s; @@ -334,15 +334,22 @@ namespace AppInstaller::CLI::Workflow bool success = false; try { - hash = context.Reporter.ExecuteWithProgress(std::bind(Utility::Download, + downloadResult = context.Reporter.ExecuteWithProgress(std::bind(Utility::Download, installer.Url, installerPath, Utility::DownloadType::Installer, std::placeholders::_1, - true, downloadInfo)); - success = true; + if (downloadResult.SizeInBytes == 0) + { + AICLI_LOG(CLI, Info, << "Got zero byte file; retrying download after a short wait..."); + std::this_thread::sleep_for(5s); + } + else + { + success = true; + } } catch (const ServiceUnavailableException& sue) { @@ -388,13 +395,13 @@ namespace AppInstaller::CLI::Workflow } } - if (!hash) + if (downloadResult.Sha256Hash.empty()) { context.Reporter.Info() << Resource::String::Cancelled << std::endl; AICLI_TERMINATE_CONTEXT(E_ABORT); } - context.Add(std::make_pair(installer.Sha256, hash.value())); + context.Add(std::make_pair(installer.Sha256, downloadResult)); } void GetMsixSignatureHash(Execution::Context& context) @@ -410,7 +417,7 @@ namespace AppInstaller::CLI::Workflow Msix::MsixInfo msixInfo(installer.Url); auto signatureHash = msixInfo.GetSignatureHash(); - context.Add(std::make_pair(installer.SignatureSha256, signatureHash)); + context.Add(std::make_pair(installer.SignatureSha256, DownloadResult{ signatureHash })); context.Add({ std::make_pair(installer.Url, msixInfo.GetDigest()) }); } catch (...) @@ -427,17 +434,23 @@ namespace AppInstaller::CLI::Workflow void VerifyInstallerHash(Execution::Context& context) { - const auto& hashPair = context.Get(); + const auto& [expectedHash, downloadResult] = context.Get(); if (!std::equal( - hashPair.first.begin(), - hashPair.first.end(), - hashPair.second.begin())) + expectedHash.begin(), + expectedHash.end(), + downloadResult.Sha256Hash.begin())) { bool overrideHashMismatch = context.Args.Contains(Execution::Args::Type::HashOverride); const auto& manifest = context.Get(); - Logging::Telemetry().LogInstallerHashMismatch(manifest.Id, manifest.Version, manifest.Channel, hashPair.first, hashPair.second, overrideHashMismatch); + Logging::Telemetry().LogInstallerHashMismatch(manifest.Id, manifest.Version, manifest.Channel, expectedHash, downloadResult.Sha256Hash, overrideHashMismatch, downloadResult.SizeInBytes, downloadResult.ContentType); + + if (downloadResult.SizeInBytes == 0) + { + context.Reporter.Error() << Resource::String::InstallerZeroByteFile << std::endl; + AICLI_TERMINATE_CONTEXT(APPINSTALLER_CLI_ERROR_INSTALLER_ZERO_BYTE_FILE); + } // If running as admin, do not allow the user to override the hash failure. if (Runtime::IsRunningAsAdmin()) @@ -527,7 +540,7 @@ namespace AppInstaller::CLI::Workflow const auto& installerPath = context.Get(); std::ifstream inStream{ installerPath, std::ifstream::binary }; auto existingFileHash = SHA256::ComputeHash(inStream); - context.Add(std::make_pair(installer.Sha256, existingFileHash)); + context.Add(std::make_pair(installer.Sha256, DownloadResult{ existingFileHash })); } else if (installer.EffectiveInstallerType() == InstallerTypeEnum::MSStore) { diff --git a/src/AppInstallerCLICore/Workflows/MSStoreInstallerHandler.cpp b/src/AppInstallerCLICore/Workflows/MSStoreInstallerHandler.cpp index 6a8147a086..9b36411334 100644 --- a/src/AppInstallerCLICore/Workflows/MSStoreInstallerHandler.cpp +++ b/src/AppInstallerCLICore/Workflows/MSStoreInstallerHandler.cpp @@ -101,8 +101,8 @@ namespace AppInstaller::CLI::Workflow } // Verify hash - const auto& hashPair = subContext.Get(); - if (std::equal(hashPair.first.begin(), hashPair.first.end(), hashPair.second.begin())) + const auto& hashPair = subContext.Get(); + if (std::equal(hashPair.first.begin(), hashPair.first.end(), hashPair.second.Sha256Hash.begin())) { AICLI_LOG(CLI, Info, << "Microsoft Store package hash verified"); subContext.Reporter.Info() << Resource::String::MSStoreDownloadPackageHashVerified << std::endl; diff --git a/src/AppInstallerCLIE2ETests/AppInstallerCLIE2ETests.csproj b/src/AppInstallerCLIE2ETests/AppInstallerCLIE2ETests.csproj index ddaa14aa3b..b39472efdf 100644 --- a/src/AppInstallerCLIE2ETests/AppInstallerCLIE2ETests.csproj +++ b/src/AppInstallerCLIE2ETests/AppInstallerCLIE2ETests.csproj @@ -55,6 +55,7 @@ + diff --git a/src/AppInstallerCLIE2ETests/Constants.cs b/src/AppInstallerCLIE2ETests/Constants.cs index da1510292c..57e02b6572 100644 --- a/src/AppInstallerCLIE2ETests/Constants.cs +++ b/src/AppInstallerCLIE2ETests/Constants.cs @@ -269,6 +269,8 @@ public class ErrorCode public const int ERROR_REPAIR_NOT_SUPPORTED = unchecked((int)0x8A15007C); public const int ERROR_ADMIN_CONTEXT_REPAIR_PROHIBITED = unchecked((int)0x8A15007D); + public const int ERROR_INSTALLER_ZERO_BYTE_FILE = unchecked((int)0x8A150086); + public const int ERROR_INSTALL_PACKAGE_IN_USE = unchecked((int)0x8A150101); public const int ERROR_INSTALL_INSTALL_IN_PROGRESS = unchecked((int)0x8A150102); public const int ERROR_INSTALL_FILE_IN_USE = unchecked((int)0x8A150103); diff --git a/src/AppInstallerCLIE2ETests/DownloadCommand.cs b/src/AppInstallerCLIE2ETests/DownloadCommand.cs index f8e0c0582f..6f5ae81ed2 100644 --- a/src/AppInstallerCLIE2ETests/DownloadCommand.cs +++ b/src/AppInstallerCLIE2ETests/DownloadCommand.cs @@ -152,5 +152,28 @@ public void DownloadWithHashMismatch() var errorResult = TestCommon.RunAICLICommand("download", $"AppInstallerTest.TestExeSha256Mismatch --download-directory {downloadDir}"); Assert.AreEqual(Constants.ErrorCode.ERROR_INSTALLER_HASH_MISMATCH, errorResult.ExitCode); } + + /// + /// Downloads the zero byte test installer with a hash mismatch. + /// + [Test] + public void DownloadZeroByteFileWithHashMismatch() + { + var downloadDir = TestCommon.GetRandomTestDir(); + var errorResult = TestCommon.RunAICLICommand("download", $"ZeroByteFile.IncorrectHash --download-directory {downloadDir}"); + Assert.AreEqual(Constants.ErrorCode.ERROR_INSTALLER_ZERO_BYTE_FILE, errorResult.ExitCode); + } + + /// + /// Downloads the zero byte test installer. + /// + [Test] + public void DownloadZeroByteFile() + { + var downloadDir = TestCommon.GetRandomTestDir(); + var result = TestCommon.RunAICLICommand("download", $"ZeroByteFile.CorrectHash --download-directory {downloadDir}"); + Assert.AreEqual(Constants.ErrorCode.S_OK, result.ExitCode); + Assert.True(TestCommon.VerifyInstallerDownload(downloadDir, "ZeroByteFile CorrectHash", "1.0.0.0", ProcessorArchitecture.X86, TestCommon.Scope.Unknown, PackageInstallerType.Exe)); + } } -} \ No newline at end of file +} diff --git a/src/AppInstallerCLIE2ETests/TestData/Manifests/ZeroByteFile_CorrectHash.yaml b/src/AppInstallerCLIE2ETests/TestData/Manifests/ZeroByteFile_CorrectHash.yaml new file mode 100644 index 0000000000..c96059fef9 --- /dev/null +++ b/src/AppInstallerCLIE2ETests/TestData/Manifests/ZeroByteFile_CorrectHash.yaml @@ -0,0 +1,14 @@ +PackageIdentifier: ZeroByteFile.CorrectHash +PackageVersion: 1.0.0.0 +PackageLocale: en-US +PackageName: ZeroByteFile CorrectHash +ShortDescription: Points to a zero byte installer file using the correct hash +Publisher: Microsoft Corporation +License: Test +Installers: + - Architecture: x86 + InstallerUrl: https://localhost:5001/TestKit/empty + InstallerType: exe + InstallerSha256: E3B0C44298FC1C149AFBF4C8996FB92427AE41E4649B934CA495991B7852B855 +ManifestType: singleton +ManifestVersion: 1.6.0 diff --git a/src/AppInstallerCLIE2ETests/TestData/Manifests/ZeroByteFile_IncorrectHash.yaml b/src/AppInstallerCLIE2ETests/TestData/Manifests/ZeroByteFile_IncorrectHash.yaml new file mode 100644 index 0000000000..adf8a5f615 --- /dev/null +++ b/src/AppInstallerCLIE2ETests/TestData/Manifests/ZeroByteFile_IncorrectHash.yaml @@ -0,0 +1,14 @@ +PackageIdentifier: ZeroByteFile.IncorrectHash +PackageVersion: 1.0.0.0 +PackageLocale: en-US +PackageName: ZeroByteFile IncorrectHash +ShortDescription: Points to a zero byte installer file using the incorrect hash +Publisher: Microsoft Corporation +License: Test +Installers: + - Architecture: x86 + InstallerUrl: https://localhost:5001/TestKit/empty + InstallerType: exe + InstallerSha256: BAD0C44298FC1C149AFBF4C8996FB92427AE41E4649B934CA495991B7852BBAD +ManifestType: singleton +ManifestVersion: 1.6.0 diff --git a/src/AppInstallerCLIE2ETests/TestData/empty b/src/AppInstallerCLIE2ETests/TestData/empty new file mode 100644 index 0000000000..e69de29bb2 diff --git a/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw b/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw index 9a6bff5c87..02882d0f56 100644 --- a/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw +++ b/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw @@ -3130,4 +3130,10 @@ Please specify one of them using the --source option to proceed. Failed to retrieve Microsoft Store package license. The Microsoft Entra Id account does not have required privilege. - + + Downloaded zero byte installer; ensure that your network connection is working properly. + + + Downloaded zero byte installer; ensure that your network connection is working properly. + + \ No newline at end of file diff --git a/src/AppInstallerCLITests/Downloader.cpp b/src/AppInstallerCLITests/Downloader.cpp index 95f8337818..7dc6731db0 100644 --- a/src/AppInstallerCLITests/Downloader.cpp +++ b/src/AppInstallerCLITests/Downloader.cpp @@ -17,10 +17,10 @@ TEST_CASE("DownloadValidFileAndVerifyHash", "[Downloader]") // Todo: point to files from our repo when the repo goes public ProgressCallback callback; - auto result = Download("https://raw.githubusercontent.com/microsoft/msix-packaging/master/LICENSE", tempFile.GetPath(), DownloadType::Manifest, callback, true); + auto result = Download("https://raw.githubusercontent.com/microsoft/msix-packaging/master/LICENSE", tempFile.GetPath(), DownloadType::Manifest, callback); - REQUIRE(result.has_value()); - auto resultHash = result.value(); + REQUIRE(!result.Sha256Hash.empty()); + auto resultHash = result.Sha256Hash; auto expectedHash = SHA256::ConvertToBytes("d2a45116709136462ee7a1c42f0e75f0efa258fe959b1504dc8ea4573451b759"); REQUIRE(std::equal( @@ -28,7 +28,9 @@ TEST_CASE("DownloadValidFileAndVerifyHash", "[Downloader]") expectedHash.end(), resultHash.begin())); - REQUIRE(std::filesystem::file_size(tempFile.GetPath()) > 0); + uint64_t expectedFileSize = 1119; + REQUIRE(result.SizeInBytes == expectedFileSize); + REQUIRE(std::filesystem::file_size(tempFile.GetPath()) == expectedFileSize); // Verify motw content std::filesystem::path motwFile(tempFile); @@ -47,17 +49,17 @@ TEST_CASE("DownloadValidFileAndCancel", "[Downloader]") ProgressCallback callback; - std::optional> waitResult; + DownloadResult waitResult; std::thread waitThread([&] { - waitResult = Download("https://aka.ms/win32-x64-user-stable", tempFile.GetPath(), DownloadType::Installer, callback, true); + waitResult = Download("https://aka.ms/win32-x64-user-stable", tempFile.GetPath(), DownloadType::Installer, callback); }); callback.Cancel(); waitThread.join(); - REQUIRE(!waitResult.has_value()); + REQUIRE(waitResult.Sha256Hash.empty()); } TEST_CASE("DownloadInvalidUrl", "[Downloader]") @@ -67,7 +69,7 @@ TEST_CASE("DownloadInvalidUrl", "[Downloader]") ProgressCallback callback; - REQUIRE_THROWS(Download("blargle-flargle-fluff", tempFile.GetPath(), DownloadType::Installer, callback, true)); + REQUIRE_THROWS(Download("blargle-flargle-fluff", tempFile.GetPath(), DownloadType::Installer, callback)); } TEST_CASE("HttpStream_ReadLastFullPage", "[HttpStream]") diff --git a/src/AppInstallerCLITests/InstallFlow.cpp b/src/AppInstallerCLITests/InstallFlow.cpp index 5cf6229805..f65408959d 100644 --- a/src/AppInstallerCLITests/InstallFlow.cpp +++ b/src/AppInstallerCLITests/InstallFlow.cpp @@ -34,7 +34,7 @@ void OverrideForDirectMsi(TestContext& context) context.Override({ DownloadInstallerFile, [](TestContext& context) { - context.Add({ {}, {} }); + context.Add({ {}, {} }); // We don't have an msi installer for tests, but we won't execute it anyway context.Add(TestDataFile("AppInstallerTestExeInstaller.exe")); } }); @@ -92,7 +92,7 @@ TEST_CASE("InstallFlow_RenameFromEncodedUrl", "[InstallFlow][workflow]") OverrideForCheckExistingInstaller(context); context.Override({ DownloadInstallerFile, [](TestContext& context) { - context.Add({ {}, {} }); + context.Add({ {}, {} }); auto installerPath = std::filesystem::temp_directory_path(); installerPath /= "EncodedUrlTest.exe"; std::filesystem::copy(TestDataFile("AppInstallerTestExeInstaller.exe"), installerPath, std::filesystem::copy_options::overwrite_existing); @@ -124,7 +124,7 @@ TEST_CASE("InstallFlow_RenameFromInvalidFileCharacterUrl", "[InstallFlow][workfl OverrideForCheckExistingInstaller(context); context.Override({ DownloadInstallerFile, [](TestContext& context) { - context.Add({ {}, {} }); + context.Add({ {}, {} }); 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/WorkFlow.cpp b/src/AppInstallerCLITests/WorkFlow.cpp index de7701264d..ce69a4d12d 100644 --- a/src/AppInstallerCLITests/WorkFlow.cpp +++ b/src/AppInstallerCLITests/WorkFlow.cpp @@ -45,7 +45,7 @@ TEST_CASE("VerifyInstallerTrustLevelAndUpdateInstallerFileMotw", "[DownloadInsta std::ostringstream updateMotwOutput; TestContext context{ updateMotwOutput, std::cin }; auto previousThreadGlobals = context.SetForCurrentThread(); - context.Add({ {}, {} }); + context.Add({ {}, {} }); context.Add(testInstallerPath); auto packageVersion = std::make_shared(Manifest{}); auto testSource = std::make_shared(); diff --git a/src/AppInstallerCLITests/WorkflowCommon.cpp b/src/AppInstallerCLITests/WorkflowCommon.cpp index c704bd55dd..51d004c583 100644 --- a/src/AppInstallerCLITests/WorkflowCommon.cpp +++ b/src/AppInstallerCLITests/WorkflowCommon.cpp @@ -558,7 +558,7 @@ namespace TestCommon context.Override({ DownloadInstallerFile, [](TestContext& context) { - context.Add({ {}, {} }); + context.Add({ {}, {} }); context.Add(TestDataFile("AppInstallerTestExeInstaller.exe")); }, expectedUseCount }); @@ -573,7 +573,7 @@ namespace TestCommon { context.Override({ DownloadInstallerFile, [&installationLog](TestContext& context) { - context.Add({ {}, {} }); + context.Add({ {}, {} }); context.Add(TestDataFile("AppInstallerTestExeInstaller.exe")); auto dependency = Dependency(DependencyType::Package, context.Get().Id, context.Get().Version); @@ -602,7 +602,7 @@ namespace TestCommon { context.Override({ DownloadInstallerFile, [](TestContext& context) { - context.Add({ {}, {} }); + context.Add({ {}, {} }); context.Add(TestDataFile("AppInstallerTestExeInstaller.exe")); } }); @@ -626,7 +626,7 @@ namespace TestCommon std::ifstream inStream{ tempInstallerPath, std::ifstream::binary }; SHA256::HashBuffer fileHash = SHA256::ComputeHash(inStream); - context.Add({ fileHash, fileHash }); + context.Add({ fileHash, DownloadResult{ fileHash } }); } }); context.Override({ RenameDownloadedInstaller, [](TestContext&) @@ -732,7 +732,7 @@ namespace TestCommon std::ofstream file(installerPath, std::ofstream::out | std::ofstream::trunc); file << installer.Url; file.close(); - context.Add({ {}, {} }); + context.Add({ {}, {} }); } }); } } diff --git a/src/AppInstallerCommonCore/AppInstallerTelemetry.cpp b/src/AppInstallerCommonCore/AppInstallerTelemetry.cpp index 9fb07722b2..416efa7265 100644 --- a/src/AppInstallerCommonCore/AppInstallerTelemetry.cpp +++ b/src/AppInstallerCommonCore/AppInstallerTelemetry.cpp @@ -546,10 +546,14 @@ namespace AppInstaller::Logging std::string_view channel, const std::vector& expected, const std::vector& actual, - bool overrideHashMismatch) const noexcept + bool overrideHashMismatch, + uint64_t downloadSizeInBytes, + const std::optional& contentType) const noexcept { if (IsTelemetryEnabled()) { + std::string actualContentType = contentType.value_or(std::string{}); + AICLI_TraceLoggingWriteActivity( "HashMismatch", TraceLoggingUInt32(m_subExecutionId, "SubExecutionId"), @@ -559,6 +563,8 @@ namespace AppInstaller::Logging TraceLoggingBinary(expected.data(), static_cast(expected.size()), "Expected"), TraceLoggingBinary(actual.data(), static_cast(actual.size()), "Actual"), TraceLoggingBool(overrideHashMismatch, "Override"), + TraceLoggingUInt64(downloadSizeInBytes, "FileSize"), + AICLI_TraceLoggingStringView(actualContentType, "ContentType"), TelemetryPrivacyDataTag(PDT_ProductAndServicePerformance), TraceLoggingKeyword(MICROSOFT_KEYWORD_CRITICAL_DATA)); diff --git a/src/AppInstallerCommonCore/DODownloader.cpp b/src/AppInstallerCommonCore/DODownloader.cpp index 84d12624e1..988a0f754e 100644 --- a/src/AppInstallerCommonCore/DODownloader.cpp +++ b/src/AppInstallerCommonCore/DODownloader.cpp @@ -15,6 +15,35 @@ namespace AppInstaller::Utility { + namespace + { + std::optional ExtractContentType(const std::optional& headers) + { + if (!headers) + { + return std::nullopt; + } + + static constexpr std::string_view s_ContentType = "content-type:"sv; + auto headerLines = Utility::SplitIntoLines(headers.value()); + + for (const auto& header : headerLines) + { + std::string_view headerView = header; + if (header.length() >= s_ContentType.length()) + { + std::string lowerFragment = ToLower(headerView.substr(0, s_ContentType.length())); + if (s_ContentType == lowerFragment) + { + return Trim(header.substr(s_ContentType.length())); + } + } + } + + return std::nullopt; + } + } + namespace DeliveryOptimization { // Represents a download work item for Delivery Optimization. @@ -369,11 +398,10 @@ namespace AppInstaller::Utility // Debugging tip: // From an elevated PowerShell, run: // > Get-DeliveryOptimizationLog | Set-Content doLogs.txt - std::optional> DODownload( + DownloadResult DODownload( const std::string& url, const std::filesystem::path& dest, IProgressCallback& progress, - bool computeHash, std::optional info) { AICLI_LOG(Core, Info, << "DeliveryOptimization downloading from url: " << url); @@ -432,11 +460,15 @@ namespace AppInstaller::Utility download.Finalize(); AICLI_LOG(Core, Info, << "Download completed."); - if (computeHash) - { - std::ifstream inStream{ dest, std::ifstream::binary }; - return SHA256::ComputeHash(inStream); - } + std::ifstream inStream{ dest, std::ifstream::binary }; + auto hashDetails = SHA256::ComputeHashDetails(inStream); + + DownloadResult result; + result.Sha256Hash = std::move(hashDetails.Hash); + result.SizeInBytes = hashDetails.SizeInBytes; + result.ContentType = ExtractContentType(responseHeaders); + + return result; } return {}; diff --git a/src/AppInstallerCommonCore/DODownloader.h b/src/AppInstallerCommonCore/DODownloader.h index 1950daeb46..a5186a4e55 100644 --- a/src/AppInstallerCommonCore/DODownloader.h +++ b/src/AppInstallerCommonCore/DODownloader.h @@ -15,11 +15,10 @@ namespace AppInstaller::Utility // url: The url to be downloaded from. http->https redirection is allowed. // dest: The stream to be downloaded to. // computeHash: Optional. Indicates if SHA256 hash should be calculated when downloading. - std::optional> DODownload( + DownloadResult DODownload( const std::string& url, const std::filesystem::path& dest, IProgressCallback& progress, - bool computeHash, std::optional info); // Returns true if the error from DODownload should be treated as fatal; diff --git a/src/AppInstallerCommonCore/Downloader.cpp b/src/AppInstallerCommonCore/Downloader.cpp index aa956f6de4..f96e0c0176 100644 --- a/src/AppInstallerCommonCore/Downloader.cpp +++ b/src/AppInstallerCommonCore/Downloader.cpp @@ -27,6 +27,47 @@ namespace AppInstaller::Utility { namespace { + std::wstring GetHttpQueryString(const wil::unique_hinternet& urlFile, DWORD queryProperty) + { + std::wstring result = {}; + DWORD length = 0; + if (!HttpQueryInfoW(urlFile.get(), + queryProperty, + &result[0], + &length, + nullptr)) + { + auto lastError = GetLastError(); + if (lastError == ERROR_INSUFFICIENT_BUFFER) + { + // lpdwBufferLength contains the size, in bytes, of a buffer large enough to receive the requested information + // without the nul char. not the exact buffer size. + auto size = static_cast(length) / sizeof(wchar_t); + result.resize(size + 1); + if (HttpQueryInfoW(urlFile.get(), + queryProperty, + &result[0], + &length, + nullptr)) + { + // because the buffer can be bigger remove possible null chars + result.erase(result.find(L'\0')); + } + else + { + AICLI_LOG(Core, Error, << "Error retrieving header value [" << queryProperty << "]: " << GetLastError()); + result.clear(); + } + } + else + { + AICLI_LOG(Core, Error, << "Error retrieving header [" << queryProperty << "]: " << GetLastError()); + } + } + + return result; + } + // Gets the retry after value in terms of a delay in seconds std::chrono::seconds GetRetryAfter(const HttpDateOrDeltaHeaderValue& retryAfter) { @@ -56,47 +97,15 @@ namespace AppInstaller::Utility std::chrono::seconds GetRetryAfter(const wil::unique_hinternet& urlFile) { - std::wstring retryAfter = {}; - DWORD length = 0; - if (!HttpQueryInfoW(urlFile.get(), - HTTP_QUERY_RETRY_AFTER, - &retryAfter, - &length, - nullptr)) - { - auto lastError = GetLastError(); - if (lastError == ERROR_INSUFFICIENT_BUFFER) - { - // lpdwBufferLength contains the size, in bytes, of a buffer large enough to receive the requested information - // without the nul char. not the exact buffer size. - auto size = static_cast(length) / sizeof(wchar_t); - retryAfter.resize(size + 1); - if (HttpQueryInfoW(urlFile.get(), - HTTP_QUERY_RETRY_AFTER, - &retryAfter[0], - &length, - nullptr)) - { - // because the buffer can be bigger remove possible null chars - retryAfter.erase(retryAfter.find(L'\0')); - return AppInstaller::Utility::GetRetryAfter(retryAfter); - } - } - else - { - AICLI_LOG(Core, Error, << "Error retrieving Retry-After header: " << GetLastError()); - } - } - - return 0s; + std::wstring retryAfter = GetHttpQueryString(urlFile, HTTP_QUERY_RETRY_AFTER); + return retryAfter.empty() ? 0s : AppInstaller::Utility::GetRetryAfter(retryAfter); } } - std::optional> WinINetDownloadToStream( + DownloadResult WinINetDownloadToStream( const std::string& url, std::ostream& dest, - IProgressCallback& progress, - bool computeHash) + IProgressCallback& progress) { // For AICLI_LOG usages with string literals. #pragma warning(push) @@ -181,6 +190,9 @@ namespace AppInstaller::Utility nullptr); AICLI_LOG(Core, Verbose, << "Download size: " << contentLength); + std::string contentType = Utility::ConvertToUTF8(GetHttpQueryString(urlFile, HTTP_QUERY_CONTENT_TYPE)); + AICLI_LOG(Core, Verbose, << "Content Type: " << contentType); + // Setup hash engine SHA256 hashEngine; @@ -203,10 +215,7 @@ namespace AppInstaller::Utility THROW_LAST_ERROR_IF_MSG(!readSuccess, "InternetReadFile() failed."); - if (computeHash) - { - hashEngine.Add(buffer.get(), bytesRead); - } + hashEngine.Add(buffer.get(), bytesRead); dest.write((char*)buffer.get(), bytesRead); @@ -227,12 +236,11 @@ namespace AppInstaller::Utility THROW_HR_IF(APPINSTALLER_CLI_ERROR_DOWNLOAD_SIZE_MISMATCH, bytesDownloaded != contentLength); } - std::vector result; - if (computeHash) - { - result = hashEngine.Get(); - AICLI_LOG(Core, Info, << "Download hash: " << SHA256::ConvertToString(result)); - } + DownloadResult result; + result.SizeInBytes = static_cast(bytesDownloaded); + result.ContentType = std::move(contentType); + result.Sha256Hash = hashEngine.Get(); + AICLI_LOG(Core, Info, << "Download hash: " << SHA256::ConvertToString(result.Sha256Hash)); AICLI_LOG(Core, Info, << "Download completed."); @@ -283,24 +291,22 @@ namespace AppInstaller::Utility return result; } - std::optional> DownloadToStream( + DownloadResult DownloadToStream( const std::string& url, std::ostream& dest, DownloadType, IProgressCallback& progress, - bool computeHash, std::optional) { THROW_HR_IF(E_INVALIDARG, url.empty()); - return WinINetDownloadToStream(url, dest, progress, computeHash); + return WinINetDownloadToStream(url, dest, progress); } - std::optional> Download( + DownloadResult Download( const std::string& url, const std::filesystem::path& dest, DownloadType type, IProgressCallback& progress, - bool computeHash, std::optional info) { THROW_HR_IF(E_INVALIDARG, url.empty()); @@ -320,7 +326,7 @@ namespace AppInstaller::Utility { try { - auto result = DODownload(url, dest, progress, computeHash, info); + auto result = DODownload(url, dest, progress, info); // Since we cannot pre-apply to the file with DO, post-apply the MotW to the file. // Only do so if the file exists, because cancellation will not throw here. if (std::filesystem::exists(dest)) @@ -362,7 +368,7 @@ namespace AppInstaller::Utility // Use std::ofstream::app to append to previous empty file so that it will not // create a new file and clear motw. std::ofstream outfile(dest, std::ofstream::binary | std::ofstream::app); - return WinINetDownloadToStream(url, outfile, progress, computeHash); + return WinINetDownloadToStream(url, outfile, progress); } using namespace std::string_view_literals; diff --git a/src/AppInstallerCommonCore/FileCache.cpp b/src/AppInstallerCommonCore/FileCache.cpp index 23a1ef98ea..331c44ed16 100644 --- a/src/AppInstallerCommonCore/FileCache.cpp +++ b/src/AppInstallerCommonCore/FileCache.cpp @@ -51,12 +51,12 @@ namespace AppInstaller::Caching { try { - auto downloadHash = Utility::DownloadToStream(fullPath, *result, Utility::DownloadType::Manifest, emptyCallback, !expectedHash.empty()); + auto downloadResult = Utility::DownloadToStream(fullPath, *result, Utility::DownloadType::Manifest, emptyCallback); if (!expectedHash.empty() && - (!downloadHash || !Utility::SHA256::AreEqual(expectedHash, downloadHash.value()))) + !Utility::SHA256::AreEqual(expectedHash, downloadResult.Sha256Hash)) { - AICLI_LOG(Core, Verbose, << "Invalid hash from [" << fullPath << "]: expected [" << Utility::SHA256::ConvertToString(expectedHash) << "], got [" << (downloadHash ? Utility::SHA256::ConvertToString(*downloadHash) : "null") << "]"); + AICLI_LOG(Core, Verbose, << "Invalid hash from [" << fullPath << "]: expected [" << Utility::SHA256::ConvertToString(expectedHash) << "], got [" << Utility::SHA256::ConvertToString(downloadResult.Sha256Hash) << "]"); THROW_HR(APPINSTALLER_CLI_ERROR_SOURCE_DATA_INTEGRITY_FAILURE); } diff --git a/src/AppInstallerCommonCore/Public/AppInstallerDownloader.h b/src/AppInstallerCommonCore/Public/AppInstallerDownloader.h index 1850a44be6..1c71b148dd 100644 --- a/src/AppInstallerCommonCore/Public/AppInstallerDownloader.h +++ b/src/AppInstallerCommonCore/Public/AppInstallerDownloader.h @@ -36,6 +36,14 @@ namespace AppInstaller::Utility std::string ContentId; }; + // Properties about the downloaded file. + struct DownloadResult + { + std::vector Sha256Hash; + uint64_t SizeInBytes = 0; + std::optional ContentType; + }; + // An exception that indicates that a remote service is too busy/unavailable and may contain data on when to try again. struct ServiceUnavailableException : public wil::ResultException { @@ -52,12 +60,11 @@ namespace AppInstaller::Utility // dest: The stream to be downloaded to. // computeHash: Optional. Indicates if SHA256 hash should be calculated when downloading. // downloadInfo: Optional. Currently only used by DO to identify the download. - std::optional> DownloadToStream( + DownloadResult DownloadToStream( const std::string& url, std::ostream& dest, DownloadType type, IProgressCallback& progress, - bool computeHash = false, std::optional downloadInfo = {}); // Downloads a file from the given URL and places it in the given location. @@ -65,12 +72,11 @@ namespace AppInstaller::Utility // dest: The path to local file to be downloaded to. // computeHash: Optional. Indicates if SHA256 hash should be calculated when downloading. // downloadInfo: Optional. Currently only used by DO to identify the download. - std::optional> Download( + DownloadResult Download( const std::string& url, const std::filesystem::path& dest, DownloadType type, IProgressCallback& progress, - bool computeHash = false, std::optional downloadInfo = {}); // Gets the headers for the given URL. diff --git a/src/AppInstallerCommonCore/Public/AppInstallerTelemetry.h b/src/AppInstallerCommonCore/Public/AppInstallerTelemetry.h index 3a39fb343d..f7af576255 100644 --- a/src/AppInstallerCommonCore/Public/AppInstallerTelemetry.h +++ b/src/AppInstallerCommonCore/Public/AppInstallerTelemetry.h @@ -232,7 +232,9 @@ namespace AppInstaller::Logging std::string_view channel, const std::vector& expected, const std::vector& actual, - bool overrideHashMismatch) const noexcept; + bool overrideHashMismatch, + uint64_t downloadSizeInBytes, + const std::optional& contentType) const noexcept; // Logs a failed installation attempt. void LogInstallerFailure(std::string_view id, std::string_view version, std::string_view channel, std::string_view type, uint32_t errorCode) const noexcept; diff --git a/src/AppInstallerSharedLib/Errors.cpp b/src/AppInstallerSharedLib/Errors.cpp index 46267e0a65..89df5f7bbb 100644 --- a/src/AppInstallerSharedLib/Errors.cpp +++ b/src/AppInstallerSharedLib/Errors.cpp @@ -221,6 +221,7 @@ namespace AppInstaller WINGET_HRESULT_INFO(APPINSTALLER_CLI_ERROR_LICENSING_API_FAILED, "Failed to retrieve Microsoft Store package license."), WINGET_HRESULT_INFO(APPINSTALLER_CLI_ERROR_SFSCLIENT_PACKAGE_NOT_SUPPORTED, "The Microsoft Store package does not support download."), WINGET_HRESULT_INFO(APPINSTALLER_CLI_ERROR_LICENSING_API_FAILED_FORBIDDEN, "Failed to retrieve Microsoft Store package license. The Microsoft Entra Id account does not have the required privilege."), + WINGET_HRESULT_INFO(APPINSTALLER_CLI_ERROR_INSTALLER_ZERO_BYTE_FILE, "Downloaded zero byte installer; ensure that your network connection is working properly."), // Install errors. WINGET_HRESULT_INFO(APPINSTALLER_CLI_ERROR_INSTALL_PACKAGE_IN_USE, "Application is currently running. Exit the application then try again."), diff --git a/src/AppInstallerSharedLib/Public/AppInstallerErrors.h b/src/AppInstallerSharedLib/Public/AppInstallerErrors.h index 821a76b815..7a7f38071c 100644 --- a/src/AppInstallerSharedLib/Public/AppInstallerErrors.h +++ b/src/AppInstallerSharedLib/Public/AppInstallerErrors.h @@ -151,6 +151,7 @@ #define APPINSTALLER_CLI_ERROR_LICENSING_API_FAILED ((HRESULT)0x8A150083) #define APPINSTALLER_CLI_ERROR_SFSCLIENT_PACKAGE_NOT_SUPPORTED ((HRESULT)0x8A150084) #define APPINSTALLER_CLI_ERROR_LICENSING_API_FAILED_FORBIDDEN ((HRESULT)0x8A150085) +#define APPINSTALLER_CLI_ERROR_INSTALLER_ZERO_BYTE_FILE ((HRESULT)0x8A150086) // Install errors. #define APPINSTALLER_CLI_ERROR_INSTALL_PACKAGE_IN_USE ((HRESULT)0x8A150101) diff --git a/src/AppInstallerSharedLib/Public/AppInstallerSHA256.h b/src/AppInstallerSharedLib/Public/AppInstallerSHA256.h index 0416c47dc4..bdd5b483d3 100644 --- a/src/AppInstallerSharedLib/Public/AppInstallerSHA256.h +++ b/src/AppInstallerSharedLib/Public/AppInstallerSHA256.h @@ -23,6 +23,12 @@ namespace AppInstaller::Utility { constexpr static size_t HashBufferSizeInBytes = 32; constexpr static size_t HashStringSizeInChars = 64; + struct HashDetails + { + HashBuffer Hash; + uint64_t SizeInBytes = 0; + }; + SHA256(); // Adds the next chunk of data to the hash. @@ -56,6 +62,9 @@ namespace AppInstaller::Utility { // Computes the hash from a given stream. static HashBuffer ComputeHash(std::istream& in); + // Computes the hash from a given stream. + static HashDetails ComputeHashDetails(std::istream& in); + // Computes the hash from a given file path. static HashBuffer ComputeHashFromFile(const std::filesystem::path& path); diff --git a/src/AppInstallerSharedLib/SHA256.cpp b/src/AppInstallerSharedLib/SHA256.cpp index dfaaf0daa3..8843a6d2d2 100644 --- a/src/AppInstallerSharedLib/SHA256.cpp +++ b/src/AppInstallerSharedLib/SHA256.cpp @@ -121,6 +121,11 @@ namespace AppInstaller::Utility { } SHA256::HashBuffer SHA256::ComputeHash(std::istream& in) + { + return ComputeHashDetails(in).Hash; + } + + SHA256::HashDetails SHA256::ComputeHashDetails(std::istream& in) { // Throw exceptions on badbit auto excState = in.exceptions(); @@ -131,19 +136,25 @@ namespace AppInstaller::Utility { auto buffer = std::make_unique(bufferSize); SHA256 hasher; + uint64_t totalSize = 0; while (in.good()) { in.read((char*)(buffer.get()), bufferSize); - if (in.gcount()) + std::streamsize bytesRead = in.gcount(); + if (bytesRead) { - hasher.Add(buffer.get(), static_cast(in.gcount())); + hasher.Add(buffer.get(), static_cast(bytesRead)); + totalSize += static_cast(bytesRead); } } if (in.eof()) { - return hasher.Get(); + HashDetails result; + result.Hash = hasher.Get(); + result.SizeInBytes = totalSize; + return result; } else { @@ -151,7 +162,6 @@ namespace AppInstaller::Utility { } } - SHA256::HashBuffer SHA256::ComputeHashFromFile(const std::filesystem::path& path) { std::ifstream inStream{ path, std::ifstream::binary }; diff --git a/src/WinGetUtil/Exports.cpp b/src/WinGetUtil/Exports.cpp index 4ed8dc9b9b..bef1c56b59 100644 --- a/src/WinGetUtil/Exports.cpp +++ b/src/WinGetUtil/Exports.cpp @@ -529,12 +529,12 @@ extern "C" THROW_HR_IF(E_INVALIDARG, computeHash && sha256HashLength != 32); AppInstaller::ProgressCallback callback; - auto hashValue = Download(ConvertToUTF8(url), filePath, DownloadType::WinGetUtil, callback, computeHash); + auto downloadResult = Download(ConvertToUTF8(url), filePath, DownloadType::WinGetUtil, callback); // At this point, if computeHash is set we have verified that the buffer is valid and 32 bytes. if (computeHash) { - const auto& hash = hashValue.value(); + const auto& hash = downloadResult.Sha256Hash; // The SHA 256 hash length should always be 32 bytes. THROW_HR_IF(E_UNEXPECTED, hash.size() != sha256HashLength);