From 112546455e75aa5432ffb641f5b12d6388a38ef3 Mon Sep 17 00:00:00 2001 From: Yao Sun Date: Fri, 20 Nov 2020 15:42:14 -0800 Subject: [PATCH 01/10] Check aes result --- src/AppInstallerCLICore/Resources.h | 3 +++ .../Workflows/InstallFlow.cpp | 21 ++++++++++++++++++- .../Shared/Strings/en-us/winget.resw | 9 ++++++++ src/AppInstallerCommonCore/Downloader.cpp | 12 +++++++---- src/AppInstallerCommonCore/Errors.cpp | 2 ++ .../Public/AppInstallerDownloader.h | 3 ++- .../Public/AppInstallerErrors.h | 1 + 7 files changed, 45 insertions(+), 6 deletions(-) diff --git a/src/AppInstallerCLICore/Resources.h b/src/AppInstallerCLICore/Resources.h index 3258173277..8dc7449401 100644 --- a/src/AppInstallerCLICore/Resources.h +++ b/src/AppInstallerCLICore/Resources.h @@ -74,6 +74,9 @@ namespace AppInstaller::CLI::Resource WINGET_DEFINE_RESOURCE_STRINGID(InstallationRequiresHigherWindows); WINGET_DEFINE_RESOURCE_STRINGID(InstallCommandLongDescription); WINGET_DEFINE_RESOURCE_STRINGID(InstallCommandShortDescription); + WINGET_DEFINE_RESOURCE_STRINGID(InstallerBlockedByPolicy); + WINGET_DEFINE_RESOURCE_STRINGID(InstallerFailedSecurityCheck); + WINGET_DEFINE_RESOURCE_STRINGID(InstallerFailedVirusScan); WINGET_DEFINE_RESOURCE_STRINGID(InstallerHashMismatchAdminBlock); WINGET_DEFINE_RESOURCE_STRINGID(InstallerHashMismatchOverridden); WINGET_DEFINE_RESOURCE_STRINGID(InstallerHashMismatchOverrideRequired); diff --git a/src/AppInstallerCLICore/Workflows/InstallFlow.cpp b/src/AppInstallerCLICore/Workflows/InstallFlow.cpp index 6494d866d6..eeeff539a0 100644 --- a/src/AppInstallerCLICore/Workflows/InstallFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/InstallFlow.cpp @@ -210,7 +210,26 @@ namespace AppInstaller::CLI::Workflow else if (WI_IsFlagSet(context.GetFlags(), Execution::ContextFlag::InstallerHashMatched)) { const auto& installer = context.Get(); - Utility::ApplyMotwUsingIAttachmentExecuteIfApplicable(context.Get(), installer.value().Url); + HRESULT hr = Utility::ApplyMotwUsingIAttachmentExecuteIfApplicable(context.Get(), installer.value().Url); + + // Not using SUCCEEDED(hr) to check since there are cases file is missing after a successful scan + if (hr != S_OK) + { + switch (hr) + { + case INET_E_SECURITY_PROBLEM: + context.Reporter.Error() << Resource::String::InstallerBlockedByPolicy << std::endl; + break; + case E_FAIL: + context.Reporter.Error() << Resource::String::InstallerFailedVirusScan << std::endl; + break; + default: + context.Reporter.Error() << Resource::String::InstallerFailedSecurityCheck << std::endl; + } + + AICLI_LOG(Fail, Error, << "Installer failed security check. Url: " << installer.value().Url << " Result: " << WINGET_OSTREAM_FORMAT_HRESULT(hr)); + AICLI_TERMINATE_CONTEXT(APPINSTALLER_CLI_ERROR_INSTALLER_SECURITY_CHECK_FAILED); + } } } } diff --git a/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw b/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw index 64bd3f6ed3..1f178da2a1 100644 --- a/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw +++ b/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw @@ -722,4 +722,13 @@ They can be configured through the settings file 'winget settings'. Logs Diagnostic files containing information about application use. + + The installer is blocked by policy + + + The installer failed security check + + + An anti-virus product reports an infection in the installer + \ No newline at end of file diff --git a/src/AppInstallerCommonCore/Downloader.cpp b/src/AppInstallerCommonCore/Downloader.cpp index e1ad55432f..ea639411e6 100644 --- a/src/AppInstallerCommonCore/Downloader.cpp +++ b/src/AppInstallerCommonCore/Downloader.cpp @@ -192,24 +192,26 @@ namespace AppInstaller::Utility AICLI_LOG(Core, Info, << "Finished applying motw"); } - void ApplyMotwUsingIAttachmentExecuteIfApplicable(const std::filesystem::path& filePath, const std::string& source) + HRESULT ApplyMotwUsingIAttachmentExecuteIfApplicable(const std::filesystem::path& filePath, const std::string& source) { AICLI_LOG(Core, Info, << "Started applying motw using IAttachmentExecute to " << filePath); if (!IsNTFS(filePath)) { AICLI_LOG(Core, Info, << "File system is not NTFS. Skipped applying motw"); - return; + return S_OK; } // Attachment execution service needs STA to succeed, so we'll create a new thread and CoInitialize with STA. + HRESULT aesSaveResult = S_OK; auto updateMotw = [&]() -> HRESULT { Microsoft::WRL::ComPtr attachmentExecute; RETURN_IF_FAILED(CoCreateInstance(CLSID_AttachmentServices, nullptr, CLSCTX_INPROC_SERVER, IID_PPV_ARGS(&attachmentExecute))); RETURN_IF_FAILED(attachmentExecute->SetLocalPath(filePath.c_str())); RETURN_IF_FAILED(attachmentExecute->SetSource(Utility::ConvertToUTF16(source).c_str())); - RETURN_IF_FAILED(attachmentExecute->Save()); + aesSaveResult = attachmentExecute->Save(); + RETURN_IF_FAILED(aesSaveResult); return S_OK; }; @@ -229,6 +231,8 @@ namespace AppInstaller::Utility aesThread.join(); - AICLI_LOG(Core, Info, << "Finished applying motw using IAttachmentExecute. Result: " << hr); + AICLI_LOG(Core, Info, << "Finished applying motw using IAttachmentExecute. Result: " << hr << " AES Save result: " << aesSaveResult); + + return aesSaveResult; } } diff --git a/src/AppInstallerCommonCore/Errors.cpp b/src/AppInstallerCommonCore/Errors.cpp index 6f83155dca..c2f8a910e9 100644 --- a/src/AppInstallerCommonCore/Errors.cpp +++ b/src/AppInstallerCommonCore/Errors.cpp @@ -103,6 +103,8 @@ namespace AppInstaller return "No applicable update found"; case APPINSTALLER_CLI_ERROR_UPDATE_ALL_HAS_FAILURE: return "winget upgrade --all completed with failures"; + case APPINSTALLER_CLI_ERROR_INSTALLER_SECURITY_CHECK_FAILED: + return "Installer failed security check"; default: return "Unknown Error Code"; } diff --git a/src/AppInstallerCommonCore/Public/AppInstallerDownloader.h b/src/AppInstallerCommonCore/Public/AppInstallerDownloader.h index 10252c44d8..0d55061bd2 100644 --- a/src/AppInstallerCommonCore/Public/AppInstallerDownloader.h +++ b/src/AppInstallerCommonCore/Public/AppInstallerDownloader.h @@ -45,5 +45,6 @@ namespace AppInstaller::Utility // Apply Mark of the web using IAttachmentExecute::Save if the target file is on NTFS, otherwise does nothing. // This method only does a best effort since Attachment Execution Service may be disabled. - void ApplyMotwUsingIAttachmentExecuteIfApplicable(const std::filesystem::path& filePath, const std::string& source); + // If IAttachmentExecute::Save is successfully invoked and the scan failed, the failure HRESULT is returned. + HRESULT ApplyMotwUsingIAttachmentExecuteIfApplicable(const std::filesystem::path& filePath, const std::string& source); } diff --git a/src/AppInstallerCommonCore/Public/AppInstallerErrors.h b/src/AppInstallerCommonCore/Public/AppInstallerErrors.h index 66a8d7e25e..d8852d3d42 100644 --- a/src/AppInstallerCommonCore/Public/AppInstallerErrors.h +++ b/src/AppInstallerCommonCore/Public/AppInstallerErrors.h @@ -57,6 +57,7 @@ #define APPINSTALLER_CLI_ERROR_INVALID_MANIFEST ((HRESULT)0x8A15002A) #define APPINSTALLER_CLI_ERROR_UPDATE_NOT_APPLICABLE ((HRESULT)0x8A15002B) #define APPINSTALLER_CLI_ERROR_UPDATE_ALL_HAS_FAILURE ((HRESULT)0x8A15002C) +#define APPINSTALLER_CLI_ERROR_INSTALLER_SECURITY_CHECK_FAILED ((HRESULT)0x8A15002D) namespace AppInstaller { From e76069dd147260eb4e5ebb167f5db7a797f6a805 Mon Sep 17 00:00:00 2001 From: Yao Sun Date: Fri, 20 Nov 2020 15:55:07 -0800 Subject: [PATCH 02/10] Allow wait_abandoned --- .../Public/AppInstallerSynchronization.h | 3 +++ src/AppInstallerCommonCore/Synchronization.cpp | 3 ++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/AppInstallerCommonCore/Public/AppInstallerSynchronization.h b/src/AppInstallerCommonCore/Public/AppInstallerSynchronization.h index c3a328ff0c..3b5774cfe2 100644 --- a/src/AppInstallerCommonCore/Public/AppInstallerSynchronization.h +++ b/src/AppInstallerCommonCore/Public/AppInstallerSynchronization.h @@ -33,11 +33,14 @@ namespace AppInstaller::Synchronization static CrossProcessReaderWriteLock LockForWrite(std::string_view name); + bool WasAbandoned() { return m_wasAbandoned; } + private: CrossProcessReaderWriteLock(std::string_view name); wil::unique_mutex m_mutex; wil::unique_semaphore m_semaphore; ResetWhenMovedFrom m_semaphoreReleases{ 0 }; + bool m_wasAbandoned = false; }; } diff --git a/src/AppInstallerCommonCore/Synchronization.cpp b/src/AppInstallerCommonCore/Synchronization.cpp index 926fd41349..bb1bc865dc 100644 --- a/src/AppInstallerCommonCore/Synchronization.cpp +++ b/src/AppInstallerCommonCore/Synchronization.cpp @@ -46,7 +46,8 @@ namespace AppInstaller::Synchronization DWORD status = 0; auto lock = result.m_mutex.acquire(&status); - THROW_HR_IF(E_UNEXPECTED, status != WAIT_OBJECT_0); + THROW_HR_IF_NULL(E_UNEXPECTED, lock); + result.m_wasAbandoned = (status == WAIT_ABANDONED); for (LONG i = 0; i < s_CrossProcessReaderWriteLock_MaxReaders; ++i) { From 2b07135c9bf19ce9a641c3187afb240efcb7ea38 Mon Sep 17 00:00:00 2001 From: Yao Sun Date: Fri, 20 Nov 2020 16:03:16 -0800 Subject: [PATCH 03/10] Check download size --- src/AppInstallerCommonCore/Downloader.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/AppInstallerCommonCore/Downloader.cpp b/src/AppInstallerCommonCore/Downloader.cpp index ea639411e6..d509745339 100644 --- a/src/AppInstallerCommonCore/Downloader.cpp +++ b/src/AppInstallerCommonCore/Downloader.cpp @@ -109,6 +109,12 @@ namespace AppInstaller::Utility dest.flush(); + // Check download size matches if content length is provided in response header + if (contentLength > 0) + { + THROW_HR_IF(E_UNEXPECTED, bytesDownloaded != contentLength); + } + std::vector result; if (computeHash) { From 72ba75723da6f546e88f355c7b735733c8fdfa8d Mon Sep 17 00:00:00 2001 From: Yao Sun Date: Fri, 20 Nov 2020 16:40:38 -0800 Subject: [PATCH 04/10] range request error --- src/AppInstallerCommonCore/HttpStream/HttpClientWrapper.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/AppInstallerCommonCore/HttpStream/HttpClientWrapper.cpp b/src/AppInstallerCommonCore/HttpStream/HttpClientWrapper.cpp index a5ce68e188..cb5ea7843b 100644 --- a/src/AppInstallerCommonCore/HttpStream/HttpClientWrapper.cpp +++ b/src/AppInstallerCommonCore/HttpStream/HttpClientWrapper.cpp @@ -45,7 +45,9 @@ namespace AppInstaller::Utility::HttpStream HttpResponseMessage response = co_await m_httpClient.SendRequestAsync(request, HttpCompletionOption::ResponseHeadersRead); - THROW_HR_IF(HRESULT_FROM_WIN32(ERROR_NO_RANGES_PROCESSED), response.StatusCode() != HttpStatusCode::Ok); + THROW_HR_IF( + MAKE_HRESULT(SEVERITY_ERROR, FACILITY_HTTP, response.StatusCode()), + response.StatusCode() != HttpStatusCode::Ok); // Get the length from the response if (response.Content().Headers().HasKey(L"Content-Length")) From c8cbc3bebd635440a9f2208eabbddc3d8429ac1c Mon Sep 17 00:00:00 2001 From: Yao Sun Date: Fri, 20 Nov 2020 18:02:24 -0800 Subject: [PATCH 05/10] retry installer download and manifest download --- .../Workflows/InstallFlow.cpp | 89 ++++++++++++++----- .../Microsoft/SQLiteIndexSource.cpp | 29 +++++- 2 files changed, 94 insertions(+), 24 deletions(-) diff --git a/src/AppInstallerCLICore/Workflows/InstallFlow.cpp b/src/AppInstallerCLICore/Workflows/InstallFlow.cpp index eeeff539a0..f834f617d5 100644 --- a/src/AppInstallerCLICore/Workflows/InstallFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/InstallFlow.cpp @@ -101,11 +101,40 @@ namespace AppInstaller::CLI::Workflow context.Reporter.Info() << "Downloading " << Execution::UrlEmphasis << installer.Url << std::endl; - auto hash = context.Reporter.ExecuteWithProgress(std::bind(Utility::Download, - installer.Url, - tempInstallerPath, - std::placeholders::_1, - true)); + std::optional> hash; + + const int MaxRetryCount = 2; + for (int retryCount = 0; retryCount < MaxRetryCount; ++retryCount) + { + bool success = false; + try + { + hash = context.Reporter.ExecuteWithProgress(std::bind(Utility::Download, + installer.Url, + tempInstallerPath, + std::placeholders::_1, + true)); + + success = true; + } + catch (...) + { + if (retryCount < MaxRetryCount - 1) + { + AICLI_LOG(CLI, Info, << "Failed to download, waiting a bit and retry. Url: " << installer.Url); + Sleep(500); + } + else + { + throw; + } + } + + if (success) + { + break; + } + } if (!hash) { @@ -119,30 +148,44 @@ namespace AppInstaller::CLI::Workflow void GetMsixSignatureHash(Execution::Context& context) { + const auto& installer = context.Get().value(); + // We use this when the server won't support streaming install to swap to download. bool downloadInstead = false; - - try + const int MaxRetryCount = 2; + for (int retryCount = 0; retryCount < MaxRetryCount; ++retryCount) { - const auto& installer = context.Get().value(); - - Msix::MsixInfo msixInfo(installer.Url); - auto signature = msixInfo.GetSignature(); - - auto signatureHash = SHA256::ComputeHash(signature.data(), static_cast(signature.size())); - - context.Add(std::make_pair(installer.SignatureSha256, signatureHash)); - } - catch (const winrt::hresult_error& e) - { - if (e.code() == HRESULT_FROM_WIN32(ERROR_NO_RANGES_PROCESSED)) + bool success = false; + try { - // Server does not support range request, use download - downloadInstead = true; + Msix::MsixInfo msixInfo(installer.Url); + auto signature = msixInfo.GetSignature(); + auto signatureHash = SHA256::ComputeHash(signature.data(), static_cast(signature.size())); + context.Add(std::make_pair(installer.SignatureSha256, signatureHash)); + success = true; } - else + catch (const winrt::hresult_error& e) + { + if (e.code() == HRESULT_FROM_WIN32(ERROR_NO_RANGES_PROCESSED)) + { + // Server does not support range request, use download + downloadInstead = true; + break; + } + else if (retryCount < MaxRetryCount - 1) + { + AICLI_LOG(CLI, Info, << "Failed to get msix signature hash, waiting a bit and retry. Url: " << installer.Url); + Sleep(500); + } + else + { + throw; + } + } + + if (success) { - throw; + break; } } diff --git a/src/AppInstallerRepositoryCore/Microsoft/SQLiteIndexSource.cpp b/src/AppInstallerRepositoryCore/Microsoft/SQLiteIndexSource.cpp index f5d46d3794..4520ab48d9 100644 --- a/src/AppInstallerRepositoryCore/Microsoft/SQLiteIndexSource.cpp +++ b/src/AppInstallerRepositoryCore/Microsoft/SQLiteIndexSource.cpp @@ -106,7 +106,34 @@ namespace AppInstaller::Repository::Microsoft AICLI_LOG(Repo, Info, << "Downloading manifest"); ProgressCallback emptyCallback; - (void)Utility::DownloadToStream(fullPath, manifestStream, emptyCallback); + + const int MaxRetryCount = 2; + for (int retryCount = 0; retryCount < MaxRetryCount; ++retryCount) + { + bool success = false; + try + { + (void)Utility::DownloadToStream(fullPath, manifestStream, emptyCallback); + success = true; + } + catch (...) + { + if (retryCount < MaxRetryCount - 1) + { + AICLI_LOG(Repo, Info, << "Downloading manifest failed, waiting a bit and retrying: " << fullPath); + Sleep(500); + } + else + { + throw; + } + } + + if (success) + { + break; + } + } std::string manifestContents = manifestStream.str(); AICLI_LOG(Repo, Verbose, << "Manifest contents: " << manifestContents); From 99c959f6c3afbb1ccb6e508b72fa4dcdb91f8051 Mon Sep 17 00:00:00 2001 From: Yao Sun Date: Fri, 20 Nov 2020 21:40:39 -0800 Subject: [PATCH 06/10] Make source update optional --- src/AppInstallerCLICore/Resources.h | 1 + .../Workflows/InstallFlow.cpp | 3 +- .../Workflows/WorkflowBase.cpp | 8 +++- .../Shared/Strings/en-us/winget.resw | 3 ++ src/AppInstallerCLITests/Sources.cpp | 4 +- .../Public/AppInstallerRepositorySource.h | 11 ++++- .../RepositorySource.cpp | 40 ++++++++++++++----- 7 files changed, 56 insertions(+), 14 deletions(-) diff --git a/src/AppInstallerCLICore/Resources.h b/src/AppInstallerCLICore/Resources.h index 8dc7449401..b23aed7770 100644 --- a/src/AppInstallerCLICore/Resources.h +++ b/src/AppInstallerCLICore/Resources.h @@ -180,6 +180,7 @@ namespace AppInstaller::CLI::Resource WINGET_DEFINE_RESOURCE_STRINGID(SourceNameArgumentDescription); WINGET_DEFINE_RESOURCE_STRINGID(SourceOpenFailedSuggestion); WINGET_DEFINE_RESOURCE_STRINGID(SourceOpenPredefinedFailedSuggestion); + WINGET_DEFINE_RESOURCE_STRINGID(SourceOpenWithFailedUpdate); WINGET_DEFINE_RESOURCE_STRINGID(SourceRemoveAll); WINGET_DEFINE_RESOURCE_STRINGID(SourceRemoveCommandLongDescription); WINGET_DEFINE_RESOURCE_STRINGID(SourceRemoveCommandShortDescription); diff --git a/src/AppInstallerCLICore/Workflows/InstallFlow.cpp b/src/AppInstallerCLICore/Workflows/InstallFlow.cpp index f834f617d5..a10e51b66a 100644 --- a/src/AppInstallerCLICore/Workflows/InstallFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/InstallFlow.cpp @@ -166,7 +166,8 @@ namespace AppInstaller::CLI::Workflow } catch (const winrt::hresult_error& e) { - if (e.code() == HRESULT_FROM_WIN32(ERROR_NO_RANGES_PROCESSED)) + if (e.code() == HRESULT_FROM_WIN32(ERROR_NO_RANGES_PROCESSED) || + HRESULT_FACILITY(e.code()) == FACILITY_HTTP) { // Server does not support range request, use download downloadInstead = true; diff --git a/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp b/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp index 51d585df8d..d53f21b847 100644 --- a/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp +++ b/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp @@ -105,7 +105,13 @@ namespace AppInstaller::CLI::Workflow std::shared_ptr source; try { - source = context.Reporter.ExecuteWithProgress(std::bind(Repository::OpenSource, sourceName, std::placeholders::_1), true); + auto result = context.Reporter.ExecuteWithProgress(std::bind(Repository::OpenSource, sourceName, std::placeholders::_1), true); + source = result.Source; + + for (const auto& s : result.SourcesWithUpdateFailure) + { + context.Reporter.Warn() << Resource::String::SourceOpenWithFailedUpdate << ' ' << s.Name << std::endl; + } } catch (...) { diff --git a/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw b/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw index 1f178da2a1..7e685f343d 100644 --- a/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw +++ b/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw @@ -731,4 +731,7 @@ They can be configured through the settings file 'winget settings'. An anti-virus product reports an infection in the installer + + Failed in attempting to update the source: + \ No newline at end of file diff --git a/src/AppInstallerCLITests/Sources.cpp b/src/AppInstallerCLITests/Sources.cpp index ecd70cbbba..aecf5b5638 100644 --- a/src/AppInstallerCLITests/Sources.cpp +++ b/src/AppInstallerCLITests/Sources.cpp @@ -510,7 +510,7 @@ TEST_CASE("RepoSources_UpdateOnOpen", "[sources]") SetSetting(Streams::UserSources, s_SingleSource); ProgressCallback progress; - auto source = OpenSource(name, progress); + auto source = OpenSource(name, progress).Source; REQUIRE(updateCalledOnFactory); @@ -574,7 +574,7 @@ TEST_CASE("RepoSources_SearchAcrossMultipleSources", "[sources]") SetSetting(Streams::UserSources, s_TwoSource_AggregateSourceTest); ProgressCallback progress; - auto source = OpenSource("", progress); + auto source = OpenSource("", progress).Source; SearchRequest request; auto result = source->Search(request); diff --git a/src/AppInstallerRepositoryCore/Public/AppInstallerRepositorySource.h b/src/AppInstallerRepositoryCore/Public/AppInstallerRepositorySource.h index 77b08f1e81..86176a0220 100644 --- a/src/AppInstallerRepositoryCore/Public/AppInstallerRepositorySource.h +++ b/src/AppInstallerRepositoryCore/Public/AppInstallerRepositorySource.h @@ -97,9 +97,18 @@ namespace AppInstaller::Repository // Adds a new source for the user. void AddSource(std::string_view name, std::string_view type, std::string_view arg, IProgressCallback& progress); + struct OpenSourceResult + { + // The ISource returned by OpenSource + std::shared_ptr Source; + + // List of SourceDetails that failed to update + std::vector SourcesWithUpdateFailure; + }; + // Opens an existing source. // Passing an empty string as the name of the source will return a source that aggregates all others. - std::shared_ptr OpenSource(std::string_view name, IProgressCallback& progress); + OpenSourceResult OpenSource(std::string_view name, IProgressCallback& progress); // A predefined source. // These sources are not under the direct control of the user, such as packages installed on the system. diff --git a/src/AppInstallerRepositoryCore/RepositorySource.cpp b/src/AppInstallerRepositoryCore/RepositorySource.cpp index f9eb7693ca..5471d0653a 100644 --- a/src/AppInstallerRepositoryCore/RepositorySource.cpp +++ b/src/AppInstallerRepositoryCore/RepositorySource.cpp @@ -607,7 +607,7 @@ namespace AppInstaller::Repository sourceList.AddSource(details); } - std::shared_ptr OpenSource(std::string_view name, IProgressCallback& progress) + OpenSourceResult OpenSource(std::string_view name, IProgressCallback& progress) { SourceListInternal sourceList; auto currentSources = sourceList.GetCurrentSourceRefs(); @@ -628,6 +628,7 @@ namespace AppInstaller::Repository { AICLI_LOG(Repo, Info, << "Default source requested, multiple sources available, creating aggregated source."); auto aggregatedSource = std::make_shared("*DefaultSource"); + OpenSourceResult result; bool sourceUpdated = false; for (auto& source : currentSources) @@ -636,10 +637,18 @@ namespace AppInstaller::Repository if (ShouldUpdateBeforeOpen(source)) { - // TODO: Consider adding a context callback to indicate we are doing the same action - // to avoid the progress bar fill up multiple times. - UpdateSourceFromDetails(source, progress); - sourceUpdated = true; + try + { + // TODO: Consider adding a context callback to indicate we are doing the same action + // to avoid the progress bar fill up multiple times. + UpdateSourceFromDetails(source, progress); + sourceUpdated = true; + } + catch (...) + { + AICLI_LOG(Repo, Warning, << "Failed to update source: " << source.get().Name); + result.SourcesWithUpdateFailure.emplace_back(source); + } } aggregatedSource->AddAvailableSource(CreateSourceFromDetails(source, progress)); } @@ -649,7 +658,8 @@ namespace AppInstaller::Repository sourceList.SaveMetadata(); } - return aggregatedSource; + result.Source = aggregatedSource; + return result; } } else @@ -664,12 +674,24 @@ namespace AppInstaller::Repository { AICLI_LOG(Repo, Info, << "Named source requested, found: " << source->Name); + OpenSourceResult result; + if (ShouldUpdateBeforeOpen(*source)) { - UpdateSourceFromDetails(*source, progress); - sourceList.SaveMetadata(); + try + { + UpdateSourceFromDetails(*source, progress); + sourceList.SaveMetadata(); + } + catch (...) + { + AICLI_LOG(Repo, Warning, << "Failed to update source: " << (*source).Name); + result.SourcesWithUpdateFailure.emplace_back(*source); + } } - return CreateSourceFromDetails(*source, progress); + + result.Source = CreateSourceFromDetails(*source, progress); + return result; } } } From 5f7c96707cbc3aad055576849720a72d8fe562de Mon Sep 17 00:00:00 2001 From: Yao Sun Date: Fri, 20 Nov 2020 22:02:38 -0800 Subject: [PATCH 07/10] fix --- .../Workflows/InstallFlow.cpp | 52 +++++++------------ 1 file changed, 19 insertions(+), 33 deletions(-) diff --git a/src/AppInstallerCLICore/Workflows/InstallFlow.cpp b/src/AppInstallerCLICore/Workflows/InstallFlow.cpp index a10e51b66a..dad49d2bb8 100644 --- a/src/AppInstallerCLICore/Workflows/InstallFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/InstallFlow.cpp @@ -148,45 +148,31 @@ namespace AppInstaller::CLI::Workflow void GetMsixSignatureHash(Execution::Context& context) { - const auto& installer = context.Get().value(); - // We use this when the server won't support streaming install to swap to download. bool downloadInstead = false; - const int MaxRetryCount = 2; - for (int retryCount = 0; retryCount < MaxRetryCount; ++retryCount) + + try { - bool success = false; - try - { - Msix::MsixInfo msixInfo(installer.Url); - auto signature = msixInfo.GetSignature(); - auto signatureHash = SHA256::ComputeHash(signature.data(), static_cast(signature.size())); - context.Add(std::make_pair(installer.SignatureSha256, signatureHash)); - success = true; - } - catch (const winrt::hresult_error& e) + const auto& installer = context.Get().value(); + + Msix::MsixInfo msixInfo(installer.Url); + auto signature = msixInfo.GetSignature(); + + auto signatureHash = SHA256::ComputeHash(signature.data(), static_cast(signature.size())); + + context.Add(std::make_pair(installer.SignatureSha256, signatureHash)); + } + catch (const winrt::hresult_error& e) + { + if (e.code() == HRESULT_FROM_WIN32(ERROR_NO_RANGES_PROCESSED) || + HRESULT_FACILITY(e.code()) == FACILITY_HTTP) { - if (e.code() == HRESULT_FROM_WIN32(ERROR_NO_RANGES_PROCESSED) || - HRESULT_FACILITY(e.code()) == FACILITY_HTTP) - { - // Server does not support range request, use download - downloadInstead = true; - break; - } - else if (retryCount < MaxRetryCount - 1) - { - AICLI_LOG(CLI, Info, << "Failed to get msix signature hash, waiting a bit and retry. Url: " << installer.Url); - Sleep(500); - } - else - { - throw; - } + // Server does not support range request, use download + downloadInstead = true; } - - if (success) + else { - break; + throw; } } From f06c0b47403bb27f6fe43ef5150b30a80f7870f3 Mon Sep 17 00:00:00 2001 From: Yao Sun Date: Fri, 20 Nov 2020 22:14:19 -0800 Subject: [PATCH 08/10] Add INET to spelling check expected list --- .github/actions/spelling/expect.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/actions/spelling/expect.txt b/.github/actions/spelling/expect.txt index 722897a8f3..14473b620d 100644 --- a/.github/actions/spelling/expect.txt +++ b/.github/actions/spelling/expect.txt @@ -326,3 +326,4 @@ Wunused WZDNCRFJ xf xl +INET From b3c7b12fc7da16c06dd178577623fcbed7bf7762 Mon Sep 17 00:00:00 2001 From: Yao Sun Date: Mon, 23 Nov 2020 14:54:12 -0800 Subject: [PATCH 09/10] Fix e2e tests --- src/AppInstallerCLIE2ETests/Constants.cs | 1 + src/AppInstallerCLIE2ETests/SourceCommand.cs | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/AppInstallerCLIE2ETests/Constants.cs b/src/AppInstallerCLIE2ETests/Constants.cs index 91a4500154..21bd8ff245 100644 --- a/src/AppInstallerCLIE2ETests/Constants.cs +++ b/src/AppInstallerCLIE2ETests/Constants.cs @@ -52,6 +52,7 @@ public class ErrorCode public const int ERROR_NO_RANGES_PROCESSED = unchecked((int)0x80070138); public const int OPC_E_ZIP_MISSING_END_OF_CENTRAL_DIRECTORY = unchecked((int)0x8051100f); public const int ERROR_OLD_WIN_VERSION = unchecked((int)0x8007047e); + public const int HTTP_E_STATUS_NOT_FOUND = unchecked((int)0x80190194); // AICLI custom HRESULTs public const int ERROR_INTERNAL_ERROR = unchecked((int)0x8A150001); diff --git a/src/AppInstallerCLIE2ETests/SourceCommand.cs b/src/AppInstallerCLIE2ETests/SourceCommand.cs index 5d8e97b6f7..4837eb015f 100644 --- a/src/AppInstallerCLIE2ETests/SourceCommand.cs +++ b/src/AppInstallerCLIE2ETests/SourceCommand.cs @@ -30,7 +30,7 @@ public void SourceAddWithInvalidURL() { // Add source with invalid url should fail var result = TestCommon.RunAICLICommand("source add", "AnotherSource https://microsoft.com"); - Assert.AreEqual(Constants.ErrorCode.ERROR_NO_RANGES_PROCESSED, result.ExitCode); + Assert.AreEqual(Constants.ErrorCode.HTTP_E_STATUS_NOT_FOUND, result.ExitCode); Assert.True(result.StdOut.Contains("An unexpected error occurred while executing the command")); } From d667895e416e80cb3c5e874739f2e9140c356fda Mon Sep 17 00:00:00 2001 From: Yao Sun Date: Wed, 25 Nov 2020 18:44:25 -0800 Subject: [PATCH 10/10] PR feedback --- src/AppInstallerCLICore/Workflows/InstallFlow.cpp | 2 +- src/AppInstallerCLICore/Workflows/WorkflowBase.cpp | 1 + src/AppInstallerCommonCore/Downloader.cpp | 5 +++-- src/AppInstallerCommonCore/Errors.cpp | 2 ++ src/AppInstallerCommonCore/Public/AppInstallerErrors.h | 1 + 5 files changed, 8 insertions(+), 3 deletions(-) diff --git a/src/AppInstallerCLICore/Workflows/InstallFlow.cpp b/src/AppInstallerCLICore/Workflows/InstallFlow.cpp index dad49d2bb8..48fc577c89 100644 --- a/src/AppInstallerCLICore/Workflows/InstallFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/InstallFlow.cpp @@ -167,7 +167,7 @@ namespace AppInstaller::CLI::Workflow if (e.code() == HRESULT_FROM_WIN32(ERROR_NO_RANGES_PROCESSED) || HRESULT_FACILITY(e.code()) == FACILITY_HTTP) { - // Server does not support range request, use download + // Failed to get signature hash through HttpStream, use download downloadInstead = true; } else diff --git a/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp b/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp index d53f21b847..8018e1a044 100644 --- a/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp +++ b/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp @@ -108,6 +108,7 @@ namespace AppInstaller::CLI::Workflow auto result = context.Reporter.ExecuteWithProgress(std::bind(Repository::OpenSource, sourceName, std::placeholders::_1), true); source = result.Source; + // We'll only report the source update failure as warning and continue for (const auto& s : result.SourcesWithUpdateFailure) { context.Reporter.Warn() << Resource::String::SourceOpenWithFailedUpdate << ' ' << s.Name << std::endl; diff --git a/src/AppInstallerCommonCore/Downloader.cpp b/src/AppInstallerCommonCore/Downloader.cpp index d509745339..5593ff8998 100644 --- a/src/AppInstallerCommonCore/Downloader.cpp +++ b/src/AppInstallerCommonCore/Downloader.cpp @@ -1,6 +1,7 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. #include "pch.h" +#include "Public/AppInstallerErrors.h" #include "Public/AppInstallerRuntime.h" #include "Public/AppInstallerDownloader.h" #include "Public/AppInstallerSHA256.h" @@ -112,7 +113,7 @@ namespace AppInstaller::Utility // Check download size matches if content length is provided in response header if (contentLength > 0) { - THROW_HR_IF(E_UNEXPECTED, bytesDownloaded != contentLength); + THROW_HR_IF(APPINSTALLER_CLI_ERROR_DOWNLOAD_SIZE_MISMATCH, bytesDownloaded != contentLength); } std::vector result; @@ -237,7 +238,7 @@ namespace AppInstaller::Utility aesThread.join(); - AICLI_LOG(Core, Info, << "Finished applying motw using IAttachmentExecute. Result: " << hr << " AES Save result: " << aesSaveResult); + AICLI_LOG(Core, Info, << "Finished applying motw using IAttachmentExecute. Result: " << hr << " IAttachmentExecute::Save() result: " << aesSaveResult); return aesSaveResult; } diff --git a/src/AppInstallerCommonCore/Errors.cpp b/src/AppInstallerCommonCore/Errors.cpp index c2f8a910e9..d0a9db2495 100644 --- a/src/AppInstallerCommonCore/Errors.cpp +++ b/src/AppInstallerCommonCore/Errors.cpp @@ -105,6 +105,8 @@ namespace AppInstaller return "winget upgrade --all completed with failures"; case APPINSTALLER_CLI_ERROR_INSTALLER_SECURITY_CHECK_FAILED: return "Installer failed security check"; + case APPINSTALLER_CLI_ERROR_DOWNLOAD_SIZE_MISMATCH: + return "Download size does not match expected content length"; default: return "Unknown Error Code"; } diff --git a/src/AppInstallerCommonCore/Public/AppInstallerErrors.h b/src/AppInstallerCommonCore/Public/AppInstallerErrors.h index d8852d3d42..5931c2b573 100644 --- a/src/AppInstallerCommonCore/Public/AppInstallerErrors.h +++ b/src/AppInstallerCommonCore/Public/AppInstallerErrors.h @@ -58,6 +58,7 @@ #define APPINSTALLER_CLI_ERROR_UPDATE_NOT_APPLICABLE ((HRESULT)0x8A15002B) #define APPINSTALLER_CLI_ERROR_UPDATE_ALL_HAS_FAILURE ((HRESULT)0x8A15002C) #define APPINSTALLER_CLI_ERROR_INSTALLER_SECURITY_CHECK_FAILED ((HRESULT)0x8A15002D) +#define APPINSTALLER_CLI_ERROR_DOWNLOAD_SIZE_MISMATCH ((HRESULT)0x8A15002E) namespace AppInstaller {