More data from download, more telemetry on hash mismatch

JohnMcPMS · Oct 7, 2024 · bff2803 · bff2803
1 parent fdb6e60
commit bff2803
Show file tree

Hide file tree

Showing 28 changed files with 267 additions and 115 deletions.
diff --git 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
@@ -1,6 +1,7 @@
 // Copyright (c) Microsoft Corporation.
 // Licensed under the MIT License.
 #pragma once
+#include <AppInstallerDownloader.h>
 #include <winget/RepositorySource.h>
 #include <winget/Manifest.h>
 #include <winget/ARPCorrelation.h>
@@ -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<Data::HashPair>
+        struct DataMapping<Data::DownloadHashInfo>
         {
-            using value_t = std::pair<std::vector<uint8_t>, std::vector<uint8_t>>;
+            using value_t = std::pair<std::vector<uint8_t>, Utility::DownloadResult>;
         };
 
         template <>

diff --git 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
@@ -293,7 +293,7 @@ namespace AppInstaller::CLI::Workflow
 
         AICLI_LOG(CLI, Info, << "Existing installer file hash matches. Will use existing installer.");
         context.Add<Execution::Data::InstallerPath>(installerPath / installerFilename);
-        context.Add<Execution::Data::HashPair>(std::make_pair(installer.Sha256, fileHash));
+        context.Add<Execution::Data::DownloadHashInfo>(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<std::vector<BYTE>> 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<Execution::Data::HashPair>(std::make_pair(installer.Sha256, hash.value()));
+        context.Add<Execution::Data::DownloadHashInfo>(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<Execution::Data::HashPair>(std::make_pair(installer.SignatureSha256, signatureHash));
+            context.Add<Execution::Data::DownloadHashInfo>(std::make_pair(installer.SignatureSha256, DownloadResult{ signatureHash }));
             context.Add<Execution::Data::MsixDigests>({ 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<Execution::Data::HashPair>();
+        const auto& [expectedHash, downloadResult] = context.Get<Execution::Data::DownloadHashInfo>();
 
         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<Execution::Data::Manifest>();
-            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<Execution::Data::InstallerPath>();
             std::ifstream inStream{ installerPath, std::ifstream::binary };
             auto existingFileHash = SHA256::ComputeHash(inStream);
-            context.Add<Execution::Data::HashPair>(std::make_pair(installer.Sha256, existingFileHash));
+            context.Add<Execution::Data::DownloadHashInfo>(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
@@ -101,8 +101,8 @@ namespace AppInstaller::CLI::Workflow
                 }
 
                 // Verify hash
-                const auto& hashPair = subContext.Get<Execution::Data::HashPair>();
-                if (std::equal(hashPair.first.begin(), hashPair.first.end(), hashPair.second.begin()))
+                const auto& hashPair = subContext.Get<Execution::Data::DownloadHashInfo>();
+                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
@@ -55,6 +55,7 @@
   <ItemGroup>
     <None Remove="TestData\Configuration\ShowDetails_TestRepo_0_3.yml" />
     <None Remove="TestData\Configuration\WithParameters_0_3.yml" />
+    <None Remove="TestData\empty" />
     <None Remove="TestData\Manifests\TestUpgradeAddsDependency.1.0.yaml" />
     <None Remove="TestData\Manifests\TestUpgradeAddsDependency.2.0.yaml" />
     <None Remove="TestData\Manifests\TestUpgradeAddsDependencyDependent.1.0.yaml" />

diff --git 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
@@ -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);
         }
+
+        /// <summary>
+        /// Downloads the zero byte test installer with a hash mismatch.
+        /// </summary>
+        [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);
+        }
+
+        /// <summary>
+        /// Downloads the zero byte test installer.
+        /// </summary>
+        [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));
+        }
     }
-}
+}
diff --git a/src/AppInstallerCLIE2ETests/TestData/Manifests/ZeroByteFile_CorrectHash.yaml 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
@@ -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
diff --git 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.</value>
   <data name="APPINSTALLER_CLI_ERROR_LICENSING_API_FAILED_FORBIDDEN" xml:space="preserve">
     <value>Failed to retrieve Microsoft Store package license. The Microsoft Entra Id account does not have required privilege.</value>
   </data>
-</root>
+  <data name="APPINSTALLER_CLI_ERROR_INSTALLER_ZERO_BYTE_FILE" xml:space="preserve">
+    <value>Downloaded zero byte installer; ensure that your network connection is working properly.</value>
+  </data>
+  <data name="InstallerZeroByteFile" xml:space="preserve">
+    <value>Downloaded zero byte installer; ensure that your network connection is working properly.</value>
+  </data>
+</root>
diff --git a/src/AppInstallerCLITests/Downloader.cpp b/src/AppInstallerCLITests/Downloader.cpp
@@ -17,18 +17,20 @@ 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(
         expectedHash.begin(),
         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<std::vector<BYTE>> 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
@@ -34,7 +34,7 @@ void OverrideForDirectMsi(TestContext& context)
 
     context.Override({ DownloadInstallerFile, [](TestContext& context)
     {
-        context.Add<Data::HashPair>({ {}, {} });
+        context.Add<Data::DownloadHashInfo>({ {}, {} });
         // We don't have an msi installer for tests, but we won't execute it anyway
         context.Add<Data::InstallerPath>(TestDataFile("AppInstallerTestExeInstaller.exe"));
     } });
@@ -92,7 +92,7 @@ TEST_CASE("InstallFlow_RenameFromEncodedUrl", "[InstallFlow][workflow]")
     OverrideForCheckExistingInstaller(context);
     context.Override({ DownloadInstallerFile, [](TestContext& context)
     {
-        context.Add<Data::HashPair>({ {}, {} });
+        context.Add<Data::DownloadHashInfo>({ {}, {} });
         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<Data::HashPair>({ {}, {} });
+        context.Add<Data::DownloadHashInfo>({ {}, {} });
         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
@@ -45,7 +45,7 @@ TEST_CASE("VerifyInstallerTrustLevelAndUpdateInstallerFileMotw", "[DownloadInsta
     std::ostringstream updateMotwOutput;
     TestContext context{ updateMotwOutput, std::cin };
     auto previousThreadGlobals = context.SetForCurrentThread();
-    context.Add<Data::HashPair>({ {}, {} });
+    context.Add<Data::DownloadHashInfo>({ {}, {} });
     context.Add<Data::InstallerPath>(testInstallerPath);
     auto packageVersion = std::make_shared<TestPackageVersion>(Manifest{});
     auto testSource = std::make_shared<TestSource>();

diff --git a/src/AppInstallerCLITests/WorkflowCommon.cpp b/src/AppInstallerCLITests/WorkflowCommon.cpp
@@ -558,7 +558,7 @@ namespace TestCommon
 
         context.Override({ DownloadInstallerFile, [](TestContext& context)
         {
-            context.Add<Data::HashPair>({ {}, {} });
+            context.Add<Data::DownloadHashInfo>({ {}, {} });
             context.Add<Data::InstallerPath>(TestDataFile("AppInstallerTestExeInstaller.exe"));
         }, expectedUseCount });
 
@@ -573,7 +573,7 @@ namespace TestCommon
     {
         context.Override({ DownloadInstallerFile, [&installationLog](TestContext& context)
         {
-            context.Add<Data::HashPair>({ {}, {} });
+            context.Add<Data::DownloadHashInfo>({ {}, {} });
             context.Add<Data::InstallerPath>(TestDataFile("AppInstallerTestExeInstaller.exe"));
 
             auto dependency = Dependency(DependencyType::Package, context.Get<Execution::Data::Manifest>().Id, context.Get<Execution::Data::Manifest>().Version);
@@ -602,7 +602,7 @@ namespace TestCommon
     {
         context.Override({ DownloadInstallerFile, [](TestContext& context)
         {
-            context.Add<Data::HashPair>({ {}, {} });
+            context.Add<Data::DownloadHashInfo>({ {}, {} });
             context.Add<Data::InstallerPath>(TestDataFile("AppInstallerTestExeInstaller.exe"));
         } });
 
@@ -626,7 +626,7 @@ namespace TestCommon
 
             std::ifstream inStream{ tempInstallerPath, std::ifstream::binary };
             SHA256::HashBuffer fileHash = SHA256::ComputeHash(inStream);
-            context.Add<Data::HashPair>({ fileHash, fileHash });
+            context.Add<Data::DownloadHashInfo>({ 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<Data::HashPair>({ {}, {} });
+            context.Add<Data::DownloadHashInfo>({ {}, {} });
         } });
     }
 }