Skip to content

Improve MOTW error handling in download flow #6127

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
JohnMcPMS merged 5 commits into from
Apr 13, 2026
+36 −13
Merged

Improve MOTW error handling in download flow #6127

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5814994
Surface errors instead of swallowing
Trenly Apr 6, 2026
9c47b70
Ensure extension for Shell32
Trenly Apr 6, 2026
484a898
Revert to WIL logging
Trenly Apr 6, 2026
4d6d414
Empty commit for spell check
Trenly Apr 6, 2026
478e8e4
Merge remote-tracking branch 'upstream/master' into Downloader
Trenly Apr 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/AppInstallerCLICore/Workflows/DownloadFlow.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -338,8 +338,8 @@ namespace AppInstaller::CLI::Workflow

context <<
VerifyInstallerHash <<
UpdateInstallerFileMotwIfApplicable <<
RenameDownloadedInstaller;
RenameDownloadedInstaller <<
UpdateInstallerFileMotwIfApplicable;

if (installerDownloadOnly)
{
Expand Down
45 changes: 34 additions & 11 deletions src/AppInstallerCommonCore/Downloader.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -481,16 +481,20 @@ 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));

AICLI_LOG(Core, Info, << "File does not contain motw. Skipped removing motw");
return;
}

THROW_IF_FAILED(hr);

THROW_IF_FAILED(zoneIdentifier->Remove());
THROW_IF_FAILED(persistFile->Save(NULL, TRUE));

Expand All @@ -516,8 +520,17 @@ namespace AppInstaller::Utility
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).
// 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();

Expand All @@ -536,21 +549,31 @@ namespace AppInstaller::Utility

std::thread aesThread([&]()
Copy link
Copy Markdown
Member

@JohnMcPMS JohnMcPMS Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just realized that the failures being missing from the logs is probably due to this new thread. If you want this change to actually make the logs show up and not just fix the error handling, you would need to:

// outside lambda
ThreadGlobals* globals = ThreadGlobals::GetForCurrentThread();
// capture into lambda
... = [..., globals] (...)
{
if (globals)
{
  auto globalsCleanup = globals->SetForCurrentThread();
}
...
}

Copy link
Copy Markdown
Contributor Author

@Trenly Trenly Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that the error handling is the main thing causing the issue, my preference is to get this error handling merged so that some portion of the local manifests start working again for users who are having issues, and I can raise a second PR for the logging globals

{
hr = CoInitializeEx(nullptr, COINIT_APARTMENTTHREADED);
if (FAILED(hr))
try
{
return;
}
hr = LOG_IF_FAILED(CoInitializeEx(nullptr, COINIT_APARTMENTTHREADED));
if (FAILED(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<IStream> GetReadOnlyStreamFromURI(std::string_view uriStr)
Expand Down