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 diff --git a/src/AppInstallerCLICore/Resources.h b/src/AppInstallerCLICore/Resources.h index 3258173277..b23aed7770 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); @@ -177,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 6494d866d6..48fc577c89 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) { @@ -135,9 +164,10 @@ 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 + // Failed to get signature hash through HttpStream, use download downloadInstead = true; } else @@ -210,7 +240,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/AppInstallerCLICore/Workflows/WorkflowBase.cpp b/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp index 51d585df8d..8018e1a044 100644 --- a/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp +++ b/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp @@ -105,7 +105,14 @@ 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; + + // 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; + } } catch (...) { 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")); } diff --git a/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw b/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw index 64bd3f6ed3..7e685f343d 100644 --- a/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw +++ b/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw @@ -722,4 +722,16 @@ 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 + + + 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/AppInstallerCommonCore/Downloader.cpp b/src/AppInstallerCommonCore/Downloader.cpp index e1ad55432f..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" @@ -109,6 +110,12 @@ namespace AppInstaller::Utility dest.flush(); + // Check download size matches if content length is provided in response header + if (contentLength > 0) + { + THROW_HR_IF(APPINSTALLER_CLI_ERROR_DOWNLOAD_SIZE_MISMATCH, bytesDownloaded != contentLength); + } + std::vector result; if (computeHash) { @@ -192,24 +199,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 +238,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 << " IAttachmentExecute::Save() result: " << aesSaveResult); + + return aesSaveResult; } } diff --git a/src/AppInstallerCommonCore/Errors.cpp b/src/AppInstallerCommonCore/Errors.cpp index 6f83155dca..d0a9db2495 100644 --- a/src/AppInstallerCommonCore/Errors.cpp +++ b/src/AppInstallerCommonCore/Errors.cpp @@ -103,6 +103,10 @@ 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"; + 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/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")) 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..5931c2b573 100644 --- a/src/AppInstallerCommonCore/Public/AppInstallerErrors.h +++ b/src/AppInstallerCommonCore/Public/AppInstallerErrors.h @@ -57,6 +57,8 @@ #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) +#define APPINSTALLER_CLI_ERROR_DOWNLOAD_SIZE_MISMATCH ((HRESULT)0x8A15002E) namespace AppInstaller { 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) { 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); 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; } } }