From 5814994d1849252a7280c6f5ed0943f389dc6dcb Mon Sep 17 00:00:00 2001 From: Kaleb Luedtke Date: Mon, 6 Apr 2026 15:49:08 -0500 Subject: [PATCH 1/4] Surface errors instead of swallowing --- src/AppInstallerCommonCore/Downloader.cpp | 67 +++++++++++++---------- 1 file changed, 39 insertions(+), 28 deletions(-) diff --git a/src/AppInstallerCommonCore/Downloader.cpp b/src/AppInstallerCommonCore/Downloader.cpp index a073f59861..a16bcfd1c2 100644 --- a/src/AppInstallerCommonCore/Downloader.cpp +++ b/src/AppInstallerCommonCore/Downloader.cpp @@ -131,8 +131,8 @@ namespace AppInstaller::Utility std::optional info) { // For AICLI_LOG usages with string literals. - #pragma warning(push) - #pragma warning(disable:26449) +#pragma warning(push) +#pragma warning(disable:26449) AICLI_LOG(Core, Info, << "WinINet downloading from url: " << url); @@ -277,7 +277,7 @@ namespace AppInstaller::Utility AICLI_LOG(Core, Info, << "Download completed."); - #pragma warning(pop) +#pragma warning(pop) return result; } @@ -437,7 +437,7 @@ namespace AppInstaller::Utility return false; } - + static inline bool FileSupportsMotw(const std::filesystem::path& path) { return SupportsNamedStreams(path); @@ -510,47 +510,58 @@ namespace AppInstaller::Utility // 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())); + { + 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())); - // IAttachmentExecute::Save() expects the local file to be clean(i.e. it won't clear existing motw if it thinks the source url is trusted) - RemoveMotwIfApplicable(filePath); + // IAttachmentExecute::Save() expects the local file to be clean(i.e. it won't clear existing motw if it thinks the source url is trusted) + RemoveMotwIfApplicable(filePath); - aesSaveResult = attachmentExecute->Save(); + aesSaveResult = attachmentExecute->Save(); - // Reapply desired zone upon scan failure. - // Not using SUCCEEDED(hr) to check since there are cases file is missing after a successful scan - if (aesSaveResult != S_OK && std::filesystem::exists(filePath)) - { - ApplyMotwIfApplicable(filePath, zoneIfScanFailure); - } + // Reapply desired zone upon scan failure. + // Not using SUCCEEDED(hr) to check since there are cases file is missing after a successful scan + if (aesSaveResult != S_OK && std::filesystem::exists(filePath)) + { + ApplyMotwIfApplicable(filePath, zoneIfScanFailure); + } - RETURN_IF_FAILED(aesSaveResult); - return S_OK; - }; + RETURN_IF_FAILED(aesSaveResult); + return S_OK; + }; HRESULT hr = S_OK; std::thread aesThread([&]() { - hr = CoInitializeEx(nullptr, COINIT_APARTMENTTHREADED); - if (FAILED(hr)) + try { - return; - } + hr = CoInitializeEx(nullptr, COINIT_APARTMENTTHREADED); + if (FAILED(hr)) + { + AICLI_LOG(Core, Error, << "CoInitializeEx failed in IAttachmentExecute thread. Result: " << hr); + return; + } - hr = updateMotw(); - CoUninitialize(); + hr = updateMotw(); + CoUninitialize(); + } + catch (...) + { + hr = wil::ResultFromCaughtException(); + AICLI_LOG(Core, Error, << "Exception in IAttachmentExecute thread. Result: " << hr); + } }); aesThread.join(); AICLI_LOG(Core, Info, << "Finished applying motw using IAttachmentExecute. Result: " << hr << " IAttachmentExecute::Save() result: " << aesSaveResult); - return aesSaveResult; + // Return the thread's hr when aesSaveResult was never updated (e.g. CoInitializeEx failure or exception + // before IAttachmentExecute::Save() was called), so the caller sees the real failure instead of S_OK. + return (FAILED(hr) && aesSaveResult == S_OK) ? hr : aesSaveResult; } Microsoft::WRL::ComPtr GetReadOnlyStreamFromURI(std::string_view uriStr) From 9c47b703f97d08774d9f0a9f009a9b4803fd0c43 Mon Sep 17 00:00:00 2001 From: Kaleb Luedtke Date: Mon, 6 Apr 2026 16:12:29 -0500 Subject: [PATCH 2/4] Ensure extension for Shell32 --- .../Workflows/DownloadFlow.cpp | 4 +- src/AppInstallerCommonCore/Downloader.cpp | 87 ++++++++++++++----- 2 files changed, 66 insertions(+), 25 deletions(-) diff --git a/src/AppInstallerCLICore/Workflows/DownloadFlow.cpp b/src/AppInstallerCLICore/Workflows/DownloadFlow.cpp index 1745ab7539..3339ace469 100644 --- a/src/AppInstallerCLICore/Workflows/DownloadFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/DownloadFlow.cpp @@ -338,8 +338,8 @@ namespace AppInstaller::CLI::Workflow context << VerifyInstallerHash << - UpdateInstallerFileMotwIfApplicable << - RenameDownloadedInstaller; + RenameDownloadedInstaller << + UpdateInstallerFileMotwIfApplicable; if (installerDownloadOnly) { diff --git a/src/AppInstallerCommonCore/Downloader.cpp b/src/AppInstallerCommonCore/Downloader.cpp index a16bcfd1c2..b11691b86e 100644 --- a/src/AppInstallerCommonCore/Downloader.cpp +++ b/src/AppInstallerCommonCore/Downloader.cpp @@ -131,8 +131,8 @@ namespace AppInstaller::Utility std::optional info) { // For AICLI_LOG usages with string literals. -#pragma warning(push) -#pragma warning(disable:26449) + #pragma warning(push) + #pragma warning(disable:26449) AICLI_LOG(Core, Info, << "WinINet downloading from url: " << url); @@ -277,7 +277,7 @@ namespace AppInstaller::Utility AICLI_LOG(Core, Info, << "Download completed."); -#pragma warning(pop) + #pragma warning(pop) return result; } @@ -437,7 +437,7 @@ namespace AppInstaller::Utility return false; } - + static inline bool FileSupportsMotw(const std::filesystem::path& path) { return SupportsNamedStreams(path); @@ -481,9 +481,11 @@ namespace AppInstaller::Utility THROW_IF_FAILED(zoneIdentifier.As(&persistFile)); auto hr = persistFile->Load(filePath.c_str(), STGM_READ); - if (hr == HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND)) + if (hr == HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND) || + hr == HRESULT_FROM_WIN32(ERROR_PATH_NOT_FOUND)) { - // IPersistFile::Load returns same error for "file not found" and "motw not found". + // IPersistFile::Load can return ERROR_FILE_NOT_FOUND or ERROR_PATH_NOT_FOUND for + // both "file not found" and "motw not found" depending on the Windows build. // Check if the file exists to be sure we are on the "motw not found" case. THROW_HR_IF(HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND), !std::filesystem::exists(filePath)); @@ -491,8 +493,21 @@ namespace AppInstaller::Utility return; } - THROW_IF_FAILED(zoneIdentifier->Remove()); - THROW_IF_FAILED(persistFile->Save(NULL, TRUE)); + THROW_IF_FAILED(hr); + + auto hrRemove = zoneIdentifier->Remove(); + if (FAILED(hrRemove)) + { + AICLI_LOG(Core, Error, << "IZoneIdentifier::Remove failed. Result: " << hrRemove); + THROW_IF_FAILED(hrRemove); + } + + auto hrSave = persistFile->Save(NULL, TRUE); + if (FAILED(hrSave)) + { + AICLI_LOG(Core, Error, << "IPersistFile::Save failed after removing motw. Result: " << hrSave); + THROW_IF_FAILED(hrSave); + } AICLI_LOG(Core, Info, << "Finished removing motw"); } @@ -510,27 +525,53 @@ namespace AppInstaller::Utility // 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; + auto hrCreate = CoCreateInstance(CLSID_AttachmentServices, nullptr, CLSCTX_INPROC_SERVER, IID_PPV_ARGS(&attachmentExecute)); + if (FAILED(hrCreate)) + { + AICLI_LOG(Core, Error, << "CoCreateInstance(CLSID_AttachmentServices) failed. Result: " << hrCreate); + return hrCreate; + } + + auto hrSetLocalPath = attachmentExecute->SetLocalPath(filePath.c_str()); + if (FAILED(hrSetLocalPath)) { - 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())); + AICLI_LOG(Core, Error, << "IAttachmentExecute::SetLocalPath failed for path: " << filePath << " Result: " << hrSetLocalPath); + return hrSetLocalPath; + } - // IAttachmentExecute::Save() expects the local file to be clean(i.e. it won't clear existing motw if it thinks the source url is trusted) + auto hrSetSource = attachmentExecute->SetSource(Utility::ConvertToUTF16(source).c_str()); + if (FAILED(hrSetSource)) + { + AICLI_LOG(Core, Error, << "IAttachmentExecute::SetSource failed for source: " << source << " Result: " << hrSetSource); + return hrSetSource; + } + + // IAttachmentExecute::Save() expects the local file to be clean (i.e. it won't clear existing motw if it thinks the source url is trusted). + // If removal fails for any reason, log a warning and proceed — a removal failure should not abort the security check. + try + { RemoveMotwIfApplicable(filePath); + } + catch (...) + { + HRESULT hrRemoveMotw = wil::ResultFromCaughtException(); + AICLI_LOG(Core, Warning, << "RemoveMotwIfApplicable failed before IAttachmentExecute::Save(). Result: " << hrRemoveMotw << ". Proceeding with Save()"); + } - aesSaveResult = attachmentExecute->Save(); + aesSaveResult = attachmentExecute->Save(); - // Reapply desired zone upon scan failure. - // Not using SUCCEEDED(hr) to check since there are cases file is missing after a successful scan - if (aesSaveResult != S_OK && std::filesystem::exists(filePath)) - { - ApplyMotwIfApplicable(filePath, zoneIfScanFailure); - } + // Reapply desired zone upon scan failure. + // Not using SUCCEEDED(hr) to check since there are cases file is missing after a successful scan + if (aesSaveResult != S_OK && std::filesystem::exists(filePath)) + { + ApplyMotwIfApplicable(filePath, zoneIfScanFailure); + } - RETURN_IF_FAILED(aesSaveResult); - return S_OK; - }; + RETURN_IF_FAILED(aesSaveResult); + return S_OK; + }; HRESULT hr = S_OK; From 484a898cd60bdda5763c19e4c880184632793493 Mon Sep 17 00:00:00 2001 From: Kaleb Luedtke Date: Mon, 6 Apr 2026 16:50:56 -0500 Subject: [PATCH 3/4] Revert to WIL logging --- src/AppInstallerCommonCore/Downloader.cpp | 41 ++++------------------- 1 file changed, 6 insertions(+), 35 deletions(-) diff --git a/src/AppInstallerCommonCore/Downloader.cpp b/src/AppInstallerCommonCore/Downloader.cpp index b11691b86e..6b73cc06cb 100644 --- a/src/AppInstallerCommonCore/Downloader.cpp +++ b/src/AppInstallerCommonCore/Downloader.cpp @@ -495,19 +495,8 @@ namespace AppInstaller::Utility THROW_IF_FAILED(hr); - auto hrRemove = zoneIdentifier->Remove(); - if (FAILED(hrRemove)) - { - AICLI_LOG(Core, Error, << "IZoneIdentifier::Remove failed. Result: " << hrRemove); - THROW_IF_FAILED(hrRemove); - } - - auto hrSave = persistFile->Save(NULL, TRUE); - if (FAILED(hrSave)) - { - AICLI_LOG(Core, Error, << "IPersistFile::Save failed after removing motw. Result: " << hrSave); - THROW_IF_FAILED(hrSave); - } + THROW_IF_FAILED(zoneIdentifier->Remove()); + THROW_IF_FAILED(persistFile->Save(NULL, TRUE)); AICLI_LOG(Core, Info, << "Finished removing motw"); } @@ -527,26 +516,9 @@ namespace AppInstaller::Utility auto updateMotw = [&]() -> HRESULT { Microsoft::WRL::ComPtr attachmentExecute; - auto hrCreate = CoCreateInstance(CLSID_AttachmentServices, nullptr, CLSCTX_INPROC_SERVER, IID_PPV_ARGS(&attachmentExecute)); - if (FAILED(hrCreate)) - { - AICLI_LOG(Core, Error, << "CoCreateInstance(CLSID_AttachmentServices) failed. Result: " << hrCreate); - return hrCreate; - } - - auto hrSetLocalPath = attachmentExecute->SetLocalPath(filePath.c_str()); - if (FAILED(hrSetLocalPath)) - { - AICLI_LOG(Core, Error, << "IAttachmentExecute::SetLocalPath failed for path: " << filePath << " Result: " << hrSetLocalPath); - return hrSetLocalPath; - } - - auto hrSetSource = attachmentExecute->SetSource(Utility::ConvertToUTF16(source).c_str()); - if (FAILED(hrSetSource)) - { - AICLI_LOG(Core, Error, << "IAttachmentExecute::SetSource failed for source: " << source << " Result: " << hrSetSource); - return hrSetSource; - } + 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())); // IAttachmentExecute::Save() expects the local file to be clean (i.e. it won't clear existing motw if it thinks the source url is trusted). // If removal fails for any reason, log a warning and proceed — a removal failure should not abort the security check. @@ -579,10 +551,9 @@ namespace AppInstaller::Utility { try { - hr = CoInitializeEx(nullptr, COINIT_APARTMENTTHREADED); + hr = LOG_IF_FAILED(CoInitializeEx(nullptr, COINIT_APARTMENTTHREADED)); if (FAILED(hr)) { - AICLI_LOG(Core, Error, << "CoInitializeEx failed in IAttachmentExecute thread. Result: " << hr); return; } From 4d6d41459462f47d81fe861b761587e248e64722 Mon Sep 17 00:00:00 2001 From: Kaleb Luedtke Date: Mon, 6 Apr 2026 16:53:32 -0500 Subject: [PATCH 4/4] Empty commit for spell check