From f04c0f388467203f826924f54a2051075434c8ea Mon Sep 17 00:00:00 2001 From: David Bennett Date: Fri, 23 Oct 2026 15:12:14 -0700 Subject: [PATCH 1/2] Fix user mode fonts to install the same way as machine fonts and add repair support --- .../Workflows/FontFlow.cpp | 25 + src/AppInstallerCLICore/Workflows/FontFlow.h | 6 + .../Workflows/RepairFlow.cpp | 15 + src/AppInstallerCLIE2ETests/FontCommand.cs | 66 +- .../Helpers/TestCommon.cs | 65 +- src/AppInstallerCLITests/Fonts.cpp | 326 ++++++++ src/AppInstallerCLITests/RepairFlow.cpp | 67 ++ src/AppInstallerCLITests/TestHooks.h | 7 + src/AppInstallerCommonCore/Fonts.cpp | 737 +++++++++++------- .../Public/winget/Fonts.h | 2 + 10 files changed, 978 insertions(+), 338 deletions(-) diff --git a/src/AppInstallerCLICore/Workflows/FontFlow.cpp b/src/AppInstallerCLICore/Workflows/FontFlow.cpp index 87fb26682b..650a41561d 100644 --- a/src/AppInstallerCLICore/Workflows/FontFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/FontFlow.cpp @@ -284,6 +284,31 @@ namespace AppInstaller::CLI::Workflow } } + void FontRepairImpl(Execution::Context& context) + { + const auto& installedPackage = context.Get(); + Fonts::FontContext fontContext; + fontContext.InstallerSource = InstallerSource::WinGet; + fontContext.Scope = Manifest::ConvertToScopeEnum(installedPackage->GetMetadata()[Repository::PackageVersionMetadata::InstalledScope]); + fontContext.PackageId = ConvertToUTF16(installedPackage->GetProperty(Repository::PackageVersionProperty::Moniker)); + fontContext.PackageVersion = ConvertToUTF16(installedPackage->GetProperty(Repository::PackageVersionProperty::Version)); + + if (fontContext.Scope == Manifest::ScopeEnum::User && Runtime::IsRunningWithNonDefaultFullToken()) + { + context.Reporter.Error() << Resource::String::NoAdminRepairForUserScopePackage << std::endl; + AICLI_TERMINATE_CONTEXT(APPINSTALLER_CLI_ERROR_ADMIN_CONTEXT_ACTION_PROHIBITED); + } + if (fontContext.Scope == Manifest::ScopeEnum::Machine && !Runtime::IsRunningAsAdmin()) + { + context.Reporter.Error() << Resource::String::CommandRequiresAdmin << std::endl; + AICLI_TERMINATE_CONTEXT(APPINSTALLER_CLI_ERROR_COMMAND_REQUIRES_ADMIN); + } + + context.Reporter.Info() << Resource::String::RepairFlowStartingPackageRepair << std::endl; + const auto result = Fonts::RepairFontPackage(fontContext); + context.Add(result.HResult); + } + void FontUninstallImpl(Execution::Context& context) { context.Reporter.Info() << Resource::String::UninstallFlowStartingPackageUninstall << std::endl; diff --git a/src/AppInstallerCLICore/Workflows/FontFlow.h b/src/AppInstallerCLICore/Workflows/FontFlow.h index 6f38c3294c..eb852b5fb4 100644 --- a/src/AppInstallerCLICore/Workflows/FontFlow.h +++ b/src/AppInstallerCLICore/Workflows/FontFlow.h @@ -17,6 +17,12 @@ namespace AppInstaller::CLI::Workflow // Outputs: None void FontInstallImpl(Execution::Context& context); + // Repairs the installed font package's files and registrations. + // Required Args: None + // Inputs: InstalledPackageVersion + // Outputs: OperationReturnCode + void FontRepairImpl(Execution::Context& context); + // Uninstalls the font package. // Required Args: None // Inputs: Manifest, Scope, Rename, Location diff --git a/src/AppInstallerCLICore/Workflows/RepairFlow.cpp b/src/AppInstallerCLICore/Workflows/RepairFlow.cpp index 404c0e3c79..980370e0b9 100644 --- a/src/AppInstallerCLICore/Workflows/RepairFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/RepairFlow.cpp @@ -7,6 +7,7 @@ #include "Workflows/DownloadFlow.h" #include "Workflows/ArchiveFlow.h" #include "Workflows/InstallFlow.h" +#include "Workflows/FontFlow.h" #include "Workflows/PromptFlow.h" #include "winget/ManifestCommon.h" #include "AppInstallerDeployment.h" @@ -242,6 +243,7 @@ namespace AppInstaller::CLI::Workflow switch (installerTypeEnum) { case InstallerTypeEnum::Msi: + case InstallerTypeEnum::Font: return false; case InstallerTypeEnum::Msix: // For MSIX packages that are from the Microsoft Store, selecting an installer is required. @@ -362,6 +364,13 @@ namespace AppInstaller::CLI::Workflow MSStoreRepair; } break; + case InstallerTypeEnum::Font: + { + context << + FontRepairImpl << + ReportRepairResult("Font", APPINSTALLER_CLI_ERROR_EXEC_REPAIR_FAILED, true); + } + break; case InstallerTypeEnum::Portable: default: THROW_HR(HRESULT_FROM_WIN32(ERROR_NOT_SUPPORTED)); @@ -400,6 +409,12 @@ namespace AppInstaller::CLI::Workflow } break; case InstallerTypeEnum::MSStore: + case InstallerTypeEnum::Font: + break; + case InstallerTypeEnum::Zip: + THROW_HR_IF(HRESULT_FROM_WIN32(ERROR_NOT_SUPPORTED), + !context.Contains(Execution::Data::Installer) || + context.Get()->EffectiveInstallerType() != InstallerTypeEnum::Font); break; case InstallerTypeEnum::Portable: default: diff --git a/src/AppInstallerCLIE2ETests/FontCommand.cs b/src/AppInstallerCLIE2ETests/FontCommand.cs index bbf97013d8..fdc8ed3a1e 100644 --- a/src/AppInstallerCLIE2ETests/FontCommand.cs +++ b/src/AppInstallerCLIE2ETests/FontCommand.cs @@ -6,7 +6,9 @@ namespace AppInstallerCLIE2ETests { + using System.IO; using AppInstallerCLIE2ETests.Helpers; + using Microsoft.Win32; using NUnit.Framework; /// @@ -31,14 +33,19 @@ public void InstallFont_UserScope() { var fontPackageName = "AppInstallerTest.TestFont"; var fontPackageVersion = "1.0.0.0"; - var installResult = TestCommon.RunAICLICommand("install", fontPackageName); + var installResult = TestCommon.RunAICLICommand("install", $"{fontPackageName} --scope User"); Assert.That(installResult.ExitCode, Is.EqualTo(Constants.ErrorCode.S_OK)); - Assert.That(installResult.StdOut, Does.Contain("Successfully installed")); - TestCommon.VerifyFontPackage(fontPackageName, fontPackageVersion, TestCommon.Scope.User); - - var uninstallResult = TestCommon.RunAICLICommand("uninstall", fontPackageName); - Assert.That(uninstallResult.ExitCode, Is.EqualTo(Constants.ErrorCode.S_OK)); - TestCommon.VerifyFontPackage(fontPackageName, fontPackageVersion, TestCommon.Scope.User, false); + try + { + Assert.That(installResult.StdOut, Does.Contain("Successfully installed")); + TestCommon.VerifyFontPackage(fontPackageName, fontPackageVersion, new[] { Constants.FontFileName }, TestCommon.Scope.User); + } + finally + { + var uninstallResult = TestCommon.RunAICLICommand("uninstall", $"{fontPackageName} --scope User"); + Assert.That(uninstallResult.ExitCode, Is.EqualTo(Constants.ErrorCode.S_OK)); + TestCommon.VerifyFontPackage(fontPackageName, fontPackageVersion, new[] { Constants.FontFileName }, TestCommon.Scope.User, false); + } } /// @@ -52,11 +59,52 @@ public void InstallFont_MachineScope() var result = TestCommon.RunAICLICommand("install", $"{fontPackageName} --scope Machine"); Assert.That(result.ExitCode, Is.EqualTo(Constants.ErrorCode.S_OK)); Assert.That(result.StdOut, Does.Contain("Successfully installed")); - TestCommon.VerifyFontPackage(fontPackageName, fontPackageVersion, TestCommon.Scope.Machine); + TestCommon.VerifyFontPackage(fontPackageName, fontPackageVersion, new[] { Constants.FontFileName }, TestCommon.Scope.Machine); var uninstallResult = TestCommon.RunAICLICommand("uninstall", fontPackageName); Assert.That(uninstallResult.ExitCode, Is.EqualTo(Constants.ErrorCode.S_OK)); - TestCommon.VerifyFontPackage(fontPackageName, fontPackageVersion, TestCommon.Scope.Machine, false); + TestCommon.VerifyFontPackage(fontPackageName, fontPackageVersion, new[] { Constants.FontFileName }, TestCommon.Scope.Machine, false); + } + + /// + /// Test repair of a user font's discovery registration. + /// + [Test] + public void RepairFont_UserScope() + { + var fontPackageName = "AppInstallerTest.TestFont"; + var fontPackageVersion = "1.0.0.0"; + var installResult = TestCommon.RunAICLICommand("install", $"{fontPackageName} --scope User"); + Assert.That(installResult.ExitCode, Is.EqualTo(Constants.ErrorCode.S_OK)); + try + { + using (var fontsKey = Registry.CurrentUser.OpenSubKey(Constants.FontsSubKey, true)) + using (var packageKey = fontsKey.OpenSubKey( + $"Microsoft.DesktopAppInstaller_8wekyb3d8bbwe\\{fontPackageName}\\{fontPackageVersion}")) + { + Assert.That(packageKey, Is.Not.Null); + var names = packageKey.GetValueNames(); + Assert.That(names, Is.Not.Empty); + foreach (var name in names) + { + var installedPath = packageKey.GetValue(name).ToString(); + var registrationName = Path.GetFileName(installedPath); + Assert.That(fontsKey.GetValue(registrationName), Is.EqualTo(installedPath)); + fontsKey.DeleteValue(registrationName); + } + } + + var repairResult = TestCommon.RunAICLICommand("repair", $"{fontPackageName} --scope User"); + Assert.That(repairResult.ExitCode, Is.EqualTo(Constants.ErrorCode.S_OK)); + Assert.That(repairResult.StdOut, Does.Contain("Repair operation completed successfully.")); + TestCommon.VerifyFontPackage(fontPackageName, fontPackageVersion, new[] { Constants.FontFileName }, TestCommon.Scope.User); + } + finally + { + var uninstallResult = TestCommon.RunAICLICommand("uninstall", $"{fontPackageName} --scope User"); + Assert.That(uninstallResult.ExitCode, Is.EqualTo(Constants.ErrorCode.S_OK)); + TestCommon.VerifyFontPackage(fontPackageName, fontPackageVersion, new[] { Constants.FontFileName }, TestCommon.Scope.User, false); + } } /// diff --git a/src/AppInstallerCLIE2ETests/Helpers/TestCommon.cs b/src/AppInstallerCLIE2ETests/Helpers/TestCommon.cs index e6e9d4c78b..cc323371b1 100644 --- a/src/AppInstallerCLIE2ETests/Helpers/TestCommon.cs +++ b/src/AppInstallerCLIE2ETests/Helpers/TestCommon.cs @@ -376,58 +376,57 @@ public static string GetFontsDirectory(Scope scope) /// /// Name of the package. /// Name of the package version. + /// Original filenames of the package's fonts. /// Scope. /// If package should exist. public static void VerifyFontPackage( string packageName, string packageVersion, + IEnumerable expectedFileNames, Scope scope = Scope.User, bool shouldExist = true) { RegistryKey baseKey = (scope == Scope.Machine) ? Registry.LocalMachine : Registry.CurrentUser; var fileList = new List(); - using (RegistryKey fontsRegistryKey = baseKey.OpenSubKey(Constants.FontsSubKey, true)) + using (RegistryKey fontsRegistryKey = baseKey.OpenSubKey(Constants.FontsSubKey)) { - using var winGetRootKey = fontsRegistryKey.OpenSubKey("Microsoft.DesktopAppInstaller_8wekyb3d8bbwe"); - if (shouldExist) - { - Assert.That(winGetRootKey, Is.Not.Null); - } - else + if (!shouldExist) { + using var installedVersion = fontsRegistryKey?.OpenSubKey( + $"Microsoft.DesktopAppInstaller_8wekyb3d8bbwe\\{packageName}\\{packageVersion}"); + Assert.That(installedVersion, Is.Null); return; } - using var packageNameSubkey = winGetRootKey.OpenSubKey(packageName); - if (shouldExist) - { - Assert.That(packageNameSubkey, Is.Not.Null); - } - - if (packageNameSubkey is not null) - { - using var versionSubkey = packageNameSubkey.OpenSubKey(packageVersion); + Assert.That(fontsRegistryKey, Is.Not.Null); + using var winGetRootKey = fontsRegistryKey.OpenSubKey("Microsoft.DesktopAppInstaller_8wekyb3d8bbwe"); + Assert.That(winGetRootKey, Is.Not.Null); - if (shouldExist) - { - Assert.That(versionSubkey, Is.Not.Null); - } - else - { - Assert.That(versionSubkey, Is.Null); - } + using var packageNameSubkey = winGetRootKey.OpenSubKey(packageName); + Assert.That(packageNameSubkey, Is.Not.Null); - if (versionSubkey is not null) - { - var valueNames = versionSubkey.GetValueNames(); - foreach (var valueName in valueNames) - { - fileList.Add(versionSubkey.GetValue(valueName).ToString()); - } + using var versionSubkey = packageNameSubkey.OpenSubKey(packageVersion); + Assert.That(versionSubkey, Is.Not.Null); - Assert.That(fileList.Count, Is.EqualTo(valueNames.Length)); - } + Assert.That(expectedFileNames, Is.Not.Empty); + var valueNames = versionSubkey.GetValueNames(); + Assert.That(valueNames, Is.EquivalentTo(expectedFileNames)); + foreach (var valueName in valueNames) + { + Assert.That(versionSubkey.GetValueKind(valueName), Is.EqualTo(RegistryValueKind.String)); + var installedPath = (string)versionSubkey.GetValue(valueName, null, RegistryValueOptions.DoNotExpandEnvironmentNames); + Assert.That(installedPath, Is.Not.Null.And.Not.Empty); + Assert.That(Path.IsPathFullyQualified(installedPath), Is.True); + Assert.That(Path.GetDirectoryName(installedPath), Is.EqualTo(GetFontsDirectory(scope)).IgnoreCase); + fileList.Add(installedPath); + + var registrationName = Path.GetFileName(installedPath); + var expectedRegistration = scope == Scope.Machine ? registrationName : installedPath; + Assert.That( + fontsRegistryKey.GetValue(registrationName, null, RegistryValueOptions.DoNotExpandEnvironmentNames), + Is.EqualTo(expectedRegistration)); + Assert.That(fontsRegistryKey.GetValueKind(registrationName), Is.EqualTo(RegistryValueKind.String)); } } diff --git a/src/AppInstallerCLITests/Fonts.cpp b/src/AppInstallerCLITests/Fonts.cpp index a86731dab4..2b53198d2f 100644 --- a/src/AppInstallerCLITests/Fonts.cpp +++ b/src/AppInstallerCLITests/Fonts.cpp @@ -2,6 +2,7 @@ // Licensed under the MIT License. #include "pch.h" #include "TestCommon.h" +#include "TestHooks.h" #include #include #include @@ -15,6 +16,331 @@ constexpr std::string_view s_FontFile = "TestFont.ttf"; constexpr std::string_view s_FontFileTemp = "TestFont_temp.ttf"; constexpr std::string_view s_InvalidFontFile = "Installer-Good.msix"; +namespace +{ + constexpr std::wstring_view s_WinGetFontPrefix = L"Microsoft.DesktopAppInstaller_8wekyb3d8bbwe"; + + struct FontLayoutTest + { + FontLayoutTest() : + RegistryPath(L"Software\\Microsoft\\WinGet\\FontLayoutTests\\" + Directory.GetPath().filename().wstring()), + RegistryHive(AppInstaller::Registry::Key::Create(HKEY_CURRENT_USER, RegistryPath)), + UserPath(AppInstaller::Runtime::GetPathDetailsFor(AppInstaller::Runtime::PathName::FontsUserInstallLocation)), + MachinePath(AppInstaller::Runtime::GetPathDetailsFor(AppInstaller::Runtime::PathName::FontsMachineInstallLocation)) + { + AppInstaller::Runtime::TestHook_SetPathOverride(AppInstaller::Runtime::PathName::FontsUserInstallLocation, RootPath(ScopeEnum::User)); + AppInstaller::Runtime::TestHook_SetPathOverride(AppInstaller::Runtime::PathName::FontsMachineInstallLocation, RootPath(ScopeEnum::Machine)); + TestHook_SetGetFontRegistryHive_Override([this](ScopeEnum scope) { return Hive(scope); }); + TestHook_SetFontSessionChangesDisabled(true); + } + + ~FontLayoutTest() + { + TestHook_SetGetFontRegistryHive_Override({}); + TestHook_SetFontSessionChangesDisabled(false); + AppInstaller::Runtime::TestHook_SetPathOverride(AppInstaller::Runtime::PathName::FontsUserInstallLocation, UserPath); + AppInstaller::Runtime::TestHook_SetPathOverride(AppInstaller::Runtime::PathName::FontsMachineInstallLocation, MachinePath); + AppInstaller::Registry::Key::DeleteTree(HKEY_CURRENT_USER, RegistryPath); + } + + std::filesystem::path RootPath(ScopeEnum scope) const + { + return Directory.GetPath() / AppInstaller::Manifest::ScopeToString(scope); + } + + AppInstaller::Registry::Key Hive(ScopeEnum scope) const + { + return AppInstaller::Registry::Key::Create(RegistryHive, AppInstaller::Utility::ConvertToUTF16(AppInstaller::Manifest::ScopeToString(scope))); + } + + AppInstaller::Registry::Key Root(ScopeEnum scope) const + { + return AppInstaller::Registry::Key::Create(Hive(scope), GetFontRegistryRoot()); + } + + FontContext Context(ScopeEnum scope) const + { + FontContext result; + result.Scope = scope; + result.InstallerSource = InstallerSource::WinGet; + result.PackageId = L"TestPackage"; + result.PackageVersion = L"1.0.0.0"; + return result; + } + + std::wstring PackagePath(const FontContext& context) const + { + return GetFontRegistryRoot() + L"\\" + std::wstring(s_WinGetFontPrefix) + L"\\" + context.PackageId + L"\\" + context.PackageVersion; + } + + AppInstaller::Registry::Key Package(const FontContext& context) const + { + return AppInstaller::Registry::Key::Create(Hive(context.Scope), PackagePath(context)); + } + + std::filesystem::path AddPackageDirectoryFont(const FontContext& context) + { + const auto path = RootPath(context.Scope) / s_WinGetFontPrefix / context.PackageId / context.PackageVersion / s_FontFile; + CopyFont(path); + Package(context).SetValue(L"Font title", path.wstring()); + return path; + } + + void CopyFont(const std::filesystem::path& path) const + { + std::filesystem::create_directories(path.parent_path()); + std::filesystem::copy_file(TestDataFile(s_FontFile).GetPath(), path); + } + + TestCommon::TempDirectory Directory{ "FontLayout" }; + std::wstring RegistryPath; + AppInstaller::Registry::Key RegistryHive; + AppInstaller::Filesystem::PathDetails UserPath; + AppInstaller::Filesystem::PathDetails MachinePath; + }; + + std::wstring RegistryString(const AppInstaller::Registry::Key& key, const std::wstring& name) + { + auto value = key[name]; + REQUIRE(value.has_value()); + REQUIRE(value->GetType() == AppInstaller::Registry::Value::Type::String); + return value->GetValue(); + } + + std::vector RegistryValueNames(const AppInstaller::Registry::Key& key) + { + std::vector result; + for (const auto& value : key.Values()) + { + result.push_back(value.Name()); + } + return result; + } +} + +TEST_CASE_METHOD(FontLayoutTest, "FontLayout_InstallRepairAndUninstall", "[fonts][fontlayout]") +{ + const auto scope = GENERATE(ScopeEnum::User, ScopeEnum::Machine); + const auto otherScope = scope == ScopeEnum::User ? ScopeEnum::Machine : ScopeEnum::User; + auto context = Context(scope); + const auto source = Directory.GetPath() / "Source" / s_FontFile; + const auto destination = RootPath(scope) / s_FontFile; + const std::vector expectedNames{ std::string{ s_FontFile } }; + CopyFont(source); + context.AddPackageFile(source); + + REQUIRE(InstallFontPackage(context).HResult == S_OK); + REQUIRE(GetFontRegistryRoot() == L"SOFTWARE\\Microsoft\\Windows NT\\CurrentVersion\\Fonts"); + REQUIRE(std::filesystem::exists(destination)); + REQUIRE_FALSE(std::filesystem::exists(source)); + REQUIRE_FALSE(std::filesystem::exists(RootPath(scope) / s_WinGetFontPrefix)); + REQUIRE(RegistryValueNames(Package(context)) == expectedNames); + REQUIRE(RegistryString(Package(context), L"TestFont.ttf") == destination.wstring()); + REQUIRE(RegistryValueNames(Root(scope)) == expectedNames); + REQUIRE(RegistryString(Root(scope), L"TestFont.ttf") == + (scope == ScopeEnum::Machine ? destination.filename().wstring() : destination.wstring())); + REQUIRE_FALSE(AppInstaller::Registry::Key::OpenIfExists(Hive(otherScope), GetFontRegistryRoot())); + REQUIRE(ValidateFontPackage(context).Status == FontStatus::OK); + auto inventory = GetInstalledFontFiles(); + REQUIRE(inventory.size() == 1); + REQUIRE(inventory[0].InstallerSource == InstallerSource::WinGet); + REQUIRE(inventory[0].Scope == scope); + + Root(scope).DeleteValue(L"TestFont.ttf"); + REQUIRE(ValidateFontPackage(context).Status == FontStatus::Corrupt); + REQUIRE(RepairFontPackage(context).HResult == S_OK); + REQUIRE(RepairFontPackage(context).HResult == S_OK); + REQUIRE(RegistryValueNames(Package(context)) == expectedNames); + REQUIRE(RegistryString(Package(context), L"TestFont.ttf") == destination.wstring()); + REQUIRE(RegistryValueNames(Root(scope)) == expectedNames); + REQUIRE(RegistryString(Root(scope), L"TestFont.ttf") == + (scope == ScopeEnum::Machine ? destination.filename().wstring() : destination.wstring())); + REQUIRE(ValidateFontPackage(context).Status == FontStatus::OK); + REQUIRE(GetInstalledFontFiles().size() == 1); + + const auto unrelated = RootPath(scope) / "Unrelated.ttf"; + CopyFont(unrelated); + Root(scope).SetValue(L"Unrelated.ttf", unrelated.wstring()); + REQUIRE(UninstallFontPackage(context).HResult == S_OK); + REQUIRE_FALSE(std::filesystem::exists(destination)); + REQUIRE_FALSE(Root(scope)[std::wstring{ L"TestFont.ttf" }].has_value()); + REQUIRE_FALSE(AppInstaller::Registry::Key::OpenIfExists(Hive(scope), PackagePath(context))); + REQUIRE(std::filesystem::exists(unrelated)); + REQUIRE(RegistryString(Root(scope), L"Unrelated.ttf") == unrelated.wstring()); +} + +TEST_CASE_METHOD(FontLayoutTest, "FontLayout_FileAndRegistryCollisions", "[fonts][fontlayout]") +{ + const auto scope = GENERATE(ScopeEnum::User, ScopeEnum::Machine); + auto first = Context(scope); + const auto source = Directory.GetPath() / "Source" / s_FontFile; + CopyFont(source); + CopyFont(RootPath(scope) / s_FontFile); + Root(scope).SetValue(L"TestFont0.ttf", L"Unrelated.ttf"); + first.AddPackageFile(source); + REQUIRE(InstallFontPackage(first).HResult == S_OK); + const auto firstDestination = RootPath(scope) / "TestFont1.ttf"; + const std::vector expectedNames{ std::string{ s_FontFile } }; + REQUIRE(RegistryValueNames(Package(first)) == expectedNames); + REQUIRE(RegistryString(Package(first), L"TestFont.ttf") == firstDestination.wstring()); + REQUIRE(RegistryString(Root(scope), L"TestFont1.ttf") == + (scope == ScopeEnum::Machine ? firstDestination.filename().wstring() : firstDestination.wstring())); + REQUIRE(ValidateFontPackage(first).Status == FontStatus::OK); + + auto second = Context(scope); + second.PackageId = L"OtherPackage"; + const auto otherSource = Directory.GetPath() / "OtherSource" / s_FontFile; + CopyFont(otherSource); + second.AddPackageFile(otherSource); + REQUIRE(InstallFontPackage(second).HResult == S_OK); + const auto secondDestination = RootPath(scope) / "TestFont2.ttf"; + REQUIRE(RegistryValueNames(Package(second)) == expectedNames); + REQUIRE(RegistryString(Package(second), L"TestFont.ttf") == secondDestination.wstring()); + REQUIRE(RegistryString(Root(scope), L"TestFont2.ttf") == + (scope == ScopeEnum::Machine ? secondDestination.filename().wstring() : secondDestination.wstring())); + REQUIRE(UninstallFontPackage(first).HResult == S_OK); + REQUIRE_FALSE(Root(scope)[std::wstring{ L"TestFont1.ttf" }].has_value()); + REQUIRE_FALSE(AppInstaller::Registry::Key::OpenIfExists(Hive(scope), PackagePath(first))); + REQUIRE(std::filesystem::exists(secondDestination)); + REQUIRE(RegistryString(Root(scope), L"TestFont2.ttf") == + (scope == ScopeEnum::Machine ? secondDestination.filename().wstring() : secondDestination.wstring())); + REQUIRE(ValidateFontPackage(second).Status == FontStatus::OK); + REQUIRE(RegistryString(Root(scope), L"TestFont0.ttf") == L"Unrelated.ttf"); + REQUIRE(std::filesystem::exists(RootPath(scope) / s_FontFile)); + REQUIRE(UninstallFontPackage(second).HResult == S_OK); +} + +TEST_CASE_METHOD(FontLayoutTest, "FontLayout_RepairMigratesPackageDirectory", "[fonts][fontlayout]") +{ + const bool keepDirectory = GENERATE(false, true); + auto context = Context(ScopeEnum::User); + const auto source = AddPackageDirectoryFont(context); + const auto sentinel = source.parent_path() / "keep.txt"; + if (keepDirectory) + { + std::ofstream{ sentinel } << "Keep this file"; + } + CopyFont(RootPath(context.Scope) / s_FontFile); + context.AddPackageFile(source); + REQUIRE(ValidateFontPackage(context).Status == FontStatus::Corrupt); + const auto packages = GetInstalledFontPackages(context.Scope); + REQUIRE(packages.size() == 1); + REQUIRE(packages[0].Status == FontStatus::Corrupt); + + REQUIRE(RepairFontPackage(context).HResult == S_OK); + const auto destination = RootPath(context.Scope) / "TestFont0.ttf"; + REQUIRE(std::filesystem::exists(destination)); + REQUIRE_FALSE(std::filesystem::exists(source)); + REQUIRE(std::filesystem::exists(source.parent_path()) == keepDirectory); + REQUIRE_FALSE(Package(context)[std::wstring{ L"Font title" }].has_value()); + REQUIRE(RegistryString(Package(context), L"TestFont.ttf") == destination.wstring()); + REQUIRE(RegistryString(Root(context.Scope), L"TestFont0.ttf") == destination.wstring()); + REQUIRE(RepairFontPackage(context).HResult == S_OK); + REQUIRE(RegistryString(Package(context), L"TestFont.ttf") == destination.wstring()); + REQUIRE(GetInstalledFontFiles().size() == 1); + REQUIRE(UninstallFontPackage(context).HResult == S_OK); + REQUIRE(std::filesystem::exists(RootPath(context.Scope) / s_FontFile)); + REQUIRE(std::filesystem::exists(sentinel) == keepDirectory); +} + +TEST_CASE_METHOD(FontLayoutTest, "FontLayout_InstalledPathTakesPrecedenceOverSourceName", "[fonts][fontlayout]") +{ + const auto scope = GENERATE(ScopeEnum::User, ScopeEnum::Machine); + auto context = Context(scope); + const auto first = RootPath(scope) / "TestFont0.ttf"; + const auto second = RootPath(scope) / "TestFont00.ttf"; + CopyFont(first); + CopyFont(second); + Package(context).SetValue(L"TestFont0.ttf", second.wstring()); + Package(context).SetValue(L"TestFont.ttf", first.wstring()); + REQUIRE(CreateFontFileInfo(context, first).InstallPath == first); + REQUIRE(CreateFontFileInfo(context, second).InstallPath == second); + REQUIRE(UninstallFontPackage(context).HResult == S_OK); +} + +TEST_CASE_METHOD(FontLayoutTest, "FontLayout_RejectsDuplicateSourceFilenames", "[fonts][fontlayout]") +{ + const auto scope = GENERATE(ScopeEnum::User, ScopeEnum::Machine); + auto context = Context(scope); + const auto first = Directory.GetPath() / "First" / s_FontFile; + const auto second = Directory.GetPath() / "Second" / s_FontFile; + CopyFont(first); + CopyFont(second); + context.AddPackageFile(first); + context.AddPackageFile(second); + const auto validation = ValidateFontPackage(context); + REQUIRE(validation.Result == FontResult::Error); + REQUIRE(validation.HResult == APPINSTALLER_CLI_ERROR_FONT_VALIDATION_FAILED); + REQUIRE(std::filesystem::exists(first)); + REQUIRE(std::filesystem::exists(second)); + REQUIRE_FALSE(std::filesystem::exists(RootPath(scope) / s_FontFile)); +} + +TEST_CASE_METHOD(FontLayoutTest, "FontLayout_RepairMoveFailurePreservesMappings", "[fonts][fontlayout]") +{ + auto context = Context(ScopeEnum::User); + const auto source = AddPackageDirectoryFont(context); + Root(context.Scope).SetValue(L"Font title", source.wstring()); + wil::unique_hfile lock{ CreateFileW(source.c_str(), GENERIC_READ, FILE_SHARE_READ, nullptr, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, nullptr) }; + REQUIRE(lock); + REQUIRE(RepairFontPackage(context).Result() == FontResult::Error); + REQUIRE(std::filesystem::exists(source)); + REQUIRE_FALSE(std::filesystem::exists(RootPath(context.Scope) / s_FontFile)); + REQUIRE(RegistryString(Package(context), L"Font title") == source.wstring()); + REQUIRE_FALSE(Package(context)[std::wstring{ L"TestFont.ttf" }].has_value()); + REQUIRE(RegistryString(Root(context.Scope), L"Font title") == source.wstring()); + REQUIRE_FALSE(Root(context.Scope)[std::wstring{ L"TestFont.ttf" }].has_value()); + lock.reset(); + REQUIRE(RepairFontPackage(context).HResult == S_OK); + REQUIRE(UninstallFontPackage(context).HResult == S_OK); +} + +TEST_CASE_METHOD(FontLayoutTest, "FontLayout_MissingFileRepairAndUninstall", "[fonts][fontlayout]") +{ + const auto scope = GENERATE(ScopeEnum::User, ScopeEnum::Machine); + auto context = Context(scope); + const auto missing = RootPath(scope) / s_FontFile; + Package(context).SetValue(L"TestFont.ttf", missing.wstring()); + Root(scope).SetValue(L"TestFont.ttf", missing.wstring()); + REQUIRE(RepairFontPackage(context).HResult == APPINSTALLER_CLI_ERROR_FONT_FILE_NOT_FOUND); + REQUIRE(RegistryString(Package(context), L"TestFont.ttf") == missing.wstring()); + REQUIRE(UninstallFontPackage(context).HResult == S_OK); + REQUIRE_FALSE(Root(scope)[std::wstring{ L"TestFont.ttf" }].has_value()); + REQUIRE_FALSE(AppInstaller::Registry::Key::OpenIfExists(Hive(scope), PackagePath(context))); +} + +TEST_CASE_METHOD(FontLayoutTest, "FontLayout_RejectsFilesOutsideFontLocations", "[fonts][fontlayout]") +{ + auto context = Context(ScopeEnum::User); + const auto unrelated = Directory.GetPath() / "Other" / s_FontFile; + CopyFont(unrelated); + Package(context).SetValue(L"TestFont.ttf", unrelated.wstring()); + REQUIRE(RepairFontPackage(context).HResult == APPINSTALLER_CLI_ERROR_FONT_VALIDATION_FAILED); + REQUIRE(UninstallFontPackage(context).HResult == APPINSTALLER_CLI_ERROR_FONT_UNINSTALL_FAILED); + REQUIRE(std::filesystem::exists(unrelated)); + REQUIRE(RegistryString(Package(context), L"TestFont.ttf") == unrelated.wstring()); +} + +TEST_CASE_METHOD(FontLayoutTest, "FontLayout_ReinstallUsesFlatDirectory", "[fonts][fontlayout]") +{ + auto context = Context(ScopeEnum::User); + context.Force = GENERATE(false, true); + const auto installed = AddPackageDirectoryFont(context); + const bool missingFile = GENERATE(false, true); + if (missingFile) + { + std::filesystem::remove(installed); + } + const auto source = Directory.GetPath() / "Source" / s_FontFile; + CopyFont(source); + context.AddPackageFile(source); + REQUIRE(InstallFontPackage(context).HResult == S_OK); + REQUIRE_FALSE(std::filesystem::exists(installed)); + REQUIRE(RegistryString(Package(context), L"TestFont.ttf") == (RootPath(context.Scope) / s_FontFile).wstring()); + REQUIRE(ValidateFontPackage(context).Status == FontStatus::OK); + REQUIRE(UninstallFontPackage(context).HResult == S_OK); +} + TEST_CASE("GetInstalledFonts", "[fonts]") { FontCatalog fontCatalog; diff --git a/src/AppInstallerCLITests/RepairFlow.cpp b/src/AppInstallerCLITests/RepairFlow.cpp index 40a8c6a73e..b7a5a77fd0 100644 --- a/src/AppInstallerCLITests/RepairFlow.cpp +++ b/src/AppInstallerCLITests/RepairFlow.cpp @@ -7,6 +7,8 @@ #include #include #include +#include +#include using namespace TestCommon; using namespace AppInstaller::CLI::Execution; @@ -45,3 +47,68 @@ TEST_CASE("RepairFlow_AdminContextWithUserScopeInstall", "[RepairFlow][workflow] INFO(repairOutput.str()); REQUIRE_TERMINATED_WITH(context, APPINSTALLER_CLI_ERROR_ADMIN_CONTEXT_ACTION_PROHIBITED); } + +TEST_CASE("RepairFlow_FontUsesInstalledMetadata", "[RepairFlow][workflow]") +{ + const HRESULT repairResult = GENERATE(S_OK, APPINSTALLER_CLI_ERROR_FONT_FILE_NOT_FOUND); + std::ostringstream output; + TestContext context{ output, std::cin }; + auto previousThreadGlobals = context.SetForCurrentThread(); + auto manifest = YamlParser::CreateFromPath(TestDataFile("InstallFlowTest_Exe.yaml")); + TestPackageVersion::MetadataMap metadata + { + { PackageVersionMetadata::InstalledType, "Font" }, + { PackageVersionMetadata::InstalledScope, "User" }, + }; + context.Add(TestPackageVersion::Make(manifest, metadata)); + context.Override({ AppInstaller::CLI::Workflow::FontRepairImpl, [repairResult](TestContext& repairContext) + { + repairContext.Add(static_cast(repairResult)); + }, 1 }); + + context << + AppInstaller::CLI::Workflow::SelectApplicableInstallerIfNecessary << + AppInstaller::CLI::Workflow::RepairSinglePackage; + + REQUIRE_FALSE(context.Contains(Data::Installer)); + INFO(output.str()); + if (SUCCEEDED(repairResult)) + { + REQUIRE_FALSE(context.IsTerminated()); + REQUIRE(output.str().find("Repair operation completed successfully.") != std::string::npos); + } + else + { + REQUIRE_TERMINATED_WITH(context, APPINSTALLER_CLI_ERROR_EXEC_REPAIR_FAILED); + } +} + +TEST_CASE("RepairFlow_FontBlocksElevatedUserRepair", "[RepairFlow][workflow]") +{ + TestHook::SetIsRunningWithNonDefaultFullToken_Override tokenOverride(true); + std::ostringstream output; + TestContext context{ output, std::cin }; + auto previousThreadGlobals = context.SetForCurrentThread(); + auto manifest = YamlParser::CreateFromPath(TestDataFile("InstallFlowTest_Exe.yaml")); + TestPackageVersion::MetadataMap metadata + { + { PackageVersionMetadata::InstalledType, "Font" }, + { PackageVersionMetadata::InstalledScope, "User" }, + }; + context.Add(TestPackageVersion::Make(manifest, metadata)); + context << AppInstaller::CLI::Workflow::FontRepairImpl; + REQUIRE_TERMINATED_WITH(context, APPINSTALLER_CLI_ERROR_ADMIN_CONTEXT_ACTION_PROHIBITED); +} + +TEST_CASE("RepairFlow_ArchiveFontNeedsNoExternalRepairInfo", "[RepairFlow][workflow]") +{ + std::ostringstream output; + TestContext context{ output, std::cin }; + auto previousThreadGlobals = context.SetForCurrentThread(); + auto manifest = YamlParser::CreateFromPath(TestDataFile("InstallFlowTest_Exe.yaml")); + manifest.Installers[0].BaseInstallerType = InstallerTypeEnum::Zip; + manifest.Installers[0].NestedInstallerType = InstallerTypeEnum::Font; + context.Add(manifest.Installers[0]); + context << AppInstaller::CLI::Workflow::GetRepairInfo; + REQUIRE_FALSE(context.IsTerminated()); +} diff --git a/src/AppInstallerCLITests/TestHooks.h b/src/AppInstallerCLITests/TestHooks.h index 396eafc9e1..3c205db96a 100644 --- a/src/AppInstallerCLITests/TestHooks.h +++ b/src/AppInstallerCLITests/TestHooks.h @@ -55,6 +55,13 @@ namespace AppInstaller void TestHook_SetGetFontRegistryRootFunc(GetFontRegistryRootFunc value); } + namespace Fonts + { + using GetFontRegistryHiveFunc = std::function; + void TestHook_SetGetFontRegistryHive_Override(GetFontRegistryHiveFunc value); + void TestHook_SetFontSessionChangesDisabled(bool value); + } + namespace Logging { void TestHook_SetTelemetryOverride(std::shared_ptr ttl); diff --git a/src/AppInstallerCommonCore/Fonts.cpp b/src/AppInstallerCommonCore/Fonts.cpp index 2cd3ee4d6d..56b98fa170 100644 --- a/src/AppInstallerCommonCore/Fonts.cpp +++ b/src/AppInstallerCommonCore/Fonts.cpp @@ -15,11 +15,28 @@ #include #include #include +#include using namespace AppInstaller::Utility; namespace AppInstaller::Fonts { +#ifndef AICLI_DISABLE_TEST_HOOKS + using GetFontRegistryHiveFunc = std::function; + static GetFontRegistryHiveFunc s_GetFontRegistryHiveOverride; + static bool s_FontSessionChangesDisabled = false; + + void TestHook_SetGetFontRegistryHive_Override(GetFontRegistryHiveFunc value) + { + s_GetFontRegistryHiveOverride = std::move(value); + } + + void TestHook_SetFontSessionChangesDisabled(bool value) + { + s_FontSessionChangesDisabled = value; + } +#endif + namespace { constexpr std::wstring_view s_FontsPathSubkey = L"SOFTWARE\\Microsoft\\Windows NT\\CurrentVersion\\Fonts"; @@ -30,6 +47,32 @@ namespace AppInstaller::Fonts constexpr std::wstring_view s_FontsWinGetPrefix = L"Microsoft.DesktopAppInstaller_8wekyb3d8bbwe"; const int s_RemoveFontResourceMaxTries = 1000; + Registry::Key GetFontRegistryHive(Manifest::ScopeEnum scope) + { +#ifndef AICLI_DISABLE_TEST_HOOKS + if (s_GetFontRegistryHiveOverride) + { + return s_GetFontRegistryHiveOverride(scope); + } +#endif + + switch (scope) + { + case Manifest::ScopeEnum::User: + return Registry::Key(HKEY_CURRENT_USER, L"", 0UL, MAXIMUM_ALLOWED); + case Manifest::ScopeEnum::Machine: + return Registry::Key(HKEY_LOCAL_MACHINE, L"", 0UL, MAXIMUM_ALLOWED); + case Manifest::ScopeEnum::Unknown: + default: + THROW_HR(E_INVALIDARG); + } + } + + bool FontPathsEqual(const std::filesystem::path& left, const std::filesystem::path& right) + { + return CaseInsensitiveEquals(left.lexically_normal().wstring(), right.lexically_normal().wstring()); + } + std::wstring GetFontFileTitle(const std::filesystem::path& fontFilePath) { // This code can fail in a number of ways, including if the file isn't actually @@ -102,58 +145,83 @@ namespace AppInstaller::Fonts std::filesystem::path GetRootFontPath(Manifest::ScopeEnum scope) { - if (scope == Manifest::ScopeEnum::Machine) + switch (scope) { + case Manifest::ScopeEnum::Machine: return Runtime::GetPathTo(Runtime::PathName::FontsMachineInstallLocation); - } - else - { + case Manifest::ScopeEnum::User: return Runtime::GetPathTo(Runtime::PathName::FontsUserInstallLocation); + case Manifest::ScopeEnum::Unknown: + default: + THROW_HR(E_INVALIDARG); } } - std::filesystem::path GetFontFileInstallPath(const FontContext& context) + std::filesystem::path GetUserFontPackagePath(const FontContext& context) { - auto path = GetRootFontPath(context.Scope); + AssertPackageInformation(context); + return GetRootFontPath(Manifest::ScopeEnum::User) / s_FontsWinGetPrefix / context.PackageId / context.PackageVersion; + } - switch (context.InstallerSource) - { - case InstallerSource::Unknown: - // Unknown installer is assumed to be installed in the default location for machine. - // For a user installed font it could be anywhere. - break; - case InstallerSource::WinGet: - // WinGet installed packages have two formats depending on the scope. - // For Per-user we use subfolders for better robustness and cleaner format. - // For Per-machine we must use a single folder due to a system requirement, - // so the root path is the machine install path. - if (context.Scope == Manifest::ScopeEnum::User) - { - AssertPackageInformation(context); - path /= s_FontsWinGetPrefix; - path /= context.PackageId; - path /= context.PackageVersion; - } + void AssertFontFilePath(const FontContext& context, const std::filesystem::path& path) + { + const bool inRoot = FontPathsEqual(path.parent_path(), GetRootFontPath(context.Scope)); + const bool inPackage = context.Scope == Manifest::ScopeEnum::User && + FontPathsEqual(path.parent_path(), GetUserFontPackagePath(context)); + THROW_HR_IF_MSG(APPINSTALLER_CLI_ERROR_FONT_VALIDATION_FAILED, + !path.is_absolute() || !Manifest::IsValueSafeForPathConstruction(path.filename().u8string()) || (!inRoot && !inPackage), + "Font path is outside the package's font locations: %ls", path.c_str()); + } - break; - default: - THROW_HR_MSG(E_UNEXPECTED, "Undefined InstallerSource."); - } + struct PackageFontFile + { + std::wstring RegistryName; + std::filesystem::path OriginalFileName; + std::filesystem::path FilePath; + }; - return path; + std::vector GetPackageFontFiles(const FontContext& context) + { + auto hive = GetFontRegistryHive(context.Scope); + auto key = Registry::Key::OpenIfExists(hive, GetFontRegistryPath(context)); + std::vector result; + if (!key) + { + return result; + } + for (const auto& entry : key.Values()) + { + auto value = entry.Value(); + THROW_HR_IF(APPINSTALLER_CLI_ERROR_FONT_VALIDATION_FAILED, + !value || value->GetType() != Registry::Value::Type::String); + + PackageFontFile file; + file.RegistryName = ConvertToUTF16(entry.Name()); + file.FilePath = value->GetValue(); + AssertFontFilePath(context, file.FilePath); + file.OriginalFileName = context.Scope == Manifest::ScopeEnum::User && + FontPathsEqual(file.FilePath.parent_path(), GetUserFontPackagePath(context)) ? + file.FilePath.filename() : std::filesystem::path(file.RegistryName); + THROW_HR_IF(APPINSTALLER_CLI_ERROR_FONT_VALIDATION_FAILED, + file.OriginalFileName.empty() || file.OriginalFileName != file.OriginalFileName.filename() || + !Manifest::IsValueSafeForPathConstruction(file.OriginalFileName.u8string())); + result.push_back(std::move(file)); + } + return result; } - // For machine-installed fonts the files all reside in the same folder. We need a unique name that is reasonably human - // readable. The way system fonts normally do this is by appending a number to the end of the file stem. We will follow - // the same pattern by using the original file and adding a number until we arrive at a unique filename. - std::filesystem::path GetUniquePathForDestination(const std::filesystem::path& source, const std::filesystem::path& destination) + // Files and registry value names share the scope's font namespace. + std::filesystem::path GetUniquePathForDestination(const std::filesystem::path& source, Manifest::ScopeEnum scope) { + const auto destination = GetRootFontPath(scope); + auto hive = GetFontRegistryHive(scope); + auto root = Registry::Key::OpenIfExists(hive, std::wstring{ s_FontsPathSubkey }); const auto stem = source.stem(); const auto ext = source.extension(); auto uniqueName = source.filename(); auto candidatePath = destination / uniqueName; int index = 0; - while (std::filesystem::exists(candidatePath)) + while (std::filesystem::exists(candidatePath) || (root && root[uniqueName.wstring()].has_value())) { std::filesystem::path appendId = { std::to_string(index) }; uniqueName = stem; @@ -166,6 +234,146 @@ namespace AppInstaller::Fonts return candidatePath; } + std::optional GetRegisteredFontInstallPath(const FontContext& context, const std::filesystem::path& filePath) + { + const auto files = GetPackageFontFiles(context); + for (const auto& file : files) + { + if (FontPathsEqual(file.FilePath, filePath)) + { + return file.FilePath; + } + } + for (const auto& file : files) + { + if (FontPathsEqual(file.OriginalFileName, filePath.filename())) + { + return file.FilePath; + } + } + return std::nullopt; + } + + using FontRegistryEntry = std::pair; + + std::vector GetFontRegistrations(Manifest::ScopeEnum scope, const std::filesystem::path& filePath) + { + auto hive = GetFontRegistryHive(scope); + auto root = Registry::Key::OpenIfExists(hive, std::wstring{ s_FontsPathSubkey }); + std::vector result; + if (!root) + { + return result; + } + const auto rootPath = GetRootFontPath(scope); + for (const auto& entry : root.Values()) + { + auto value = entry.Value(); + if (!value || value->GetType() != Registry::Value::Type::String) + { + continue; + } + + auto data = value->GetValue(); + std::filesystem::path registeredPath = data; + if (registeredPath.is_relative()) + { + registeredPath = rootPath / registeredPath; + } + if (FontPathsEqual(registeredPath, filePath)) + { + result.emplace_back(ConvertToUTF16(entry.Name()), std::move(data)); + } + } + return result; + } + + void UnregisterFontFile(Manifest::ScopeEnum scope, const std::filesystem::path& filePath) + { + auto hive = GetFontRegistryHive(scope); + auto root = Registry::Key::OpenIfExists(hive, std::wstring{ s_FontsPathSubkey }, 0UL, KEY_ALL_ACCESS); + for (const auto& entry : GetFontRegistrations(scope, filePath)) + { + root.DeleteValue(entry.first); + } + } + + void RegisterFontFile(const FontContext& context, const std::filesystem::path& originalFileName, const std::filesystem::path& installPath) + { + AssertFontFilePath(context, installPath); + THROW_HR_IF(APPINSTALLER_CLI_ERROR_FONT_VALIDATION_FAILED, + !FontPathsEqual(installPath.parent_path(), GetRootFontPath(context.Scope))); + auto hive = GetFontRegistryHive(context.Scope); + auto root = Registry::Key::Create(hive, std::wstring{ s_FontsPathSubkey }); + const auto valueName = installPath.filename().wstring(); + if (root[valueName]) + { + const auto registrations = GetFontRegistrations(context.Scope, installPath); + THROW_HR_IF_MSG(APPINSTALLER_CLI_ERROR_FONT_INSTALL_FAILED, + std::none_of(registrations.begin(), registrations.end(), [&](const auto& entry) + { return CaseInsensitiveEquals(entry.first, valueName); }), + "Font registration is owned by another file: %ls", valueName.c_str()); + } + + auto package = Registry::Key::Create(hive, GetFontRegistryPath(context)); + const auto originalName = originalFileName.wstring(); + if (const auto existing = package[originalName]) + { + THROW_HR_IF(APPINSTALLER_CLI_ERROR_FONT_VALIDATION_FAILED, existing->GetType() != Registry::Value::Type::String); + const std::filesystem::path existingPath = existing->GetValue(); + const bool packageFile = context.Scope == Manifest::ScopeEnum::User && + FontPathsEqual(existingPath.parent_path(), GetUserFontPackagePath(context)) && + FontPathsEqual(existingPath.filename(), originalFileName); + THROW_HR_IF_MSG(APPINSTALLER_CLI_ERROR_FONT_VALIDATION_FAILED, + !FontPathsEqual(existingPath, installPath) && !packageFile, + "Font package mapping is owned by another file: %ls", originalName.c_str()); + } + package.SetValue(originalName, installPath.wstring()); + root.SetValue(valueName, context.Scope == Manifest::ScopeEnum::Machine ? + installPath.filename().wstring() : installPath.wstring()); + } + + void AddFontToSession(const std::filesystem::path& path) + { +#ifndef AICLI_DISABLE_TEST_HOOKS + if (s_FontSessionChangesDisabled) + { + return; + } +#endif + const auto fontsAdded = ::AddFontResource(path.c_str()); + if (fontsAdded == 0) + { + AICLI_LOG(Core, Warning, << "Failed to add font resource: " << path); + } + else + { + AICLI_LOG(Core, Info, << "Added " << fontsAdded << " fonts to the session."); + } + } + + void RemoveEmptyUserFontPackageDirectories(const FontContext& context) + { + if (context.Scope != Manifest::ScopeEnum::User) + { + return; + } + + auto path = GetUserFontPackagePath(context); + for (size_t i = 0; i < 3; ++i) + { + if (std::filesystem::exists(path)) + { + if (!std::filesystem::is_empty(path)) + { + break; + } + std::filesystem::remove(path); + } + path = path.parent_path(); + } + } + std::vector GetFontFilePaths(const wil::com_ptr& fontFace) { UINT32 fileCount = 0; @@ -204,6 +412,12 @@ namespace AppInstaller::Fonts void RemoveAllFontResources(const std::filesystem::path& filePath) { +#ifndef AICLI_DISABLE_TEST_HOOKS + if (s_FontSessionChangesDisabled) + { + return; + } +#endif // The recommended uninstall method of a font is to call RemoveFontResource until it fails, // This is not guaranteed to remove the file from use, but it is the best we have. // https://learn.microsoft.com/en-us/windows/win32/api/wingdi/nf-wingdi-removefontresourcea @@ -220,6 +434,12 @@ namespace AppInstaller::Fonts void NotifyFontChange() { +#ifndef AICLI_DISABLE_TEST_HOOKS + if (s_FontSessionChangesDisabled) + { + return; + } +#endif // Send the WM_FONTCHANGE message so the system and apps know that there has been a font change. // Sometimes this does not have an error when it returns non-zero. To avoid random assert failures // we will not try to log failures for this call. If it fails it does not affect install state. @@ -227,79 +447,78 @@ namespace AppInstaller::Fonts AICLI_LOG(Core, Info, << "Notified system of font change: " << sendResult); } - // Checks the registry path for the file specified, and returns the key value if it finds one. - std::optional CheckRegistryForFontFileReference(const std::wstring& registryPath, const std::filesystem::path& filePath, Manifest::ScopeEnum scope) + void MigrateFontFile(const FontContext& context, const PackageFontFile& file) { - try - { - const auto& hive = scope == Manifest::ScopeEnum::Machine ? HKEY_LOCAL_MACHINE : HKEY_CURRENT_USER; - const auto& key = Registry::Key::OpenIfExists(hive, registryPath, 0UL, KEY_READ); - if (key) + auto hive = GetFontRegistryHive(context.Scope); + auto package = Registry::Key::OpenIfExists(hive, GetFontRegistryPath(context), 0UL, KEY_ALL_ACCESS); + const auto destination = GetUniquePathForDestination(file.OriginalFileName, context.Scope); + const auto registrations = GetFontRegistrations(context.Scope, file.FilePath); + const auto originalName = file.OriginalFileName.wstring(); + const auto originalMapping = package[originalName]; + bool sourceRemoved = false; + bool sourceUnloaded = false; + bool mappingChanged = false; + bool destinationCreated = false; + auto rollback = wil::scope_exit([&]() { - // Check for the file being present in this key. - for (const auto& valueRef : key.Values()) + if (!destinationCreated) { - const auto& value = valueRef.Value(); - if (!value.has_value() || (value.value().GetType() != Registry::Value::Type::String)) + return; + } + try + { + if (sourceRemoved) { - continue; + std::filesystem::copy_file(destination, file.FilePath); } - const auto& valueName = ConvertToUTF16(valueRef.Name()); - const std::filesystem::path& valuePath = { value->GetValue() }; - if (valuePath.is_relative()) + if (mappingChanged) { - // Relative value is OK, just compare the filenames. - // We will assume this is in the standard font paths. - if (valuePath.filename() == filePath.filename()) + if (originalMapping) { - return valueName; + package.SetValue(originalName, originalMapping->GetValue()); } - } - else - { - if (valuePath == filePath) + else { - return valueName; + package.DeleteValue(originalName); } + package.SetValue(file.RegistryName, file.FilePath.wstring()); } - } - } - } - CATCH_LOG(); - - return std::nullopt; - } - - // The package registry for machine installed fonts stores the package filename as a registry value. - // This is so we can map the original package filename to the installed filename since they may be different. - std::optional GetRegisteredMachineFontInstallPath(const std::wstring& registryPath, const std::filesystem::path& filePath) - { - try - { - const auto& key = Registry::Key::OpenIfExists(HKEY_LOCAL_MACHINE, registryPath, 0UL, KEY_READ); - if (key) - { - for (const auto& valueRef : key.Values()) - { - const auto& value = valueRef.Value(); - if (!value.has_value() || (value.value().GetType() != Registry::Value::Type::String)) + UnregisterFontFile(context.Scope, destination); + auto root = Registry::Key::Create(hive, std::wstring{ s_FontsPathSubkey }); + for (const auto& registration : registrations) { - continue; + root.SetValue(registration.first, registration.second); } - - const auto& valueName = ConvertToUTF16(valueRef.Name()); - if (valueName == filePath.filename()) + std::filesystem::remove(destination); + if (sourceUnloaded) { - const std::filesystem::path& valuePath = { value->GetValue() }; - return valuePath; + AddFontToSession(file.FilePath); } } - } + catch (...) + { + LOG_CAUGHT_EXCEPTION(); + AICLI_LOG(Core, Error, << "Font repair rollback failed for " << file.FilePath); + } + }); + + std::filesystem::copy_file(file.FilePath, destination); + destinationCreated = true; + mappingChanged = true; + RegisterFontFile(context, file.OriginalFileName, destination); + UnregisterFontFile(context.Scope, file.FilePath); + RemoveAllFontResources(file.FilePath); + sourceUnloaded = true; + THROW_HR_IF(APPINSTALLER_CLI_ERROR_EXEC_REPAIR_FAILED, !std::filesystem::remove(file.FilePath)); + sourceRemoved = true; + if (!CaseInsensitiveEquals(file.RegistryName, originalName)) + { + package.DeleteValue(file.RegistryName); } - CATCH_LOG(); - - return std::nullopt; + rollback.release(); + AddFontToSession(destination); } + } FontResult FontOperationResult::Result() const @@ -379,70 +598,25 @@ namespace AppInstaller::Fonts } fileInfo.RegistryPackagePath = GetFontRegistryPath(context); - if (fileInfo.Scope == Manifest::ScopeEnum::Machine && fileInfo.InstallerSource == InstallerSource::WinGet) - { - // Machine fonts from WinGet still install into the Font folder but we track package source separately - // due to limited information in the Font registry and all font files being in the same key and folder. - fileInfo.RegistryInstallPath = s_FontsPathSubkey; - } - else + fileInfo.RegistryInstallPath = s_FontsPathSubkey; + + const auto registeredPath = context.InstallerSource == InstallerSource::WinGet ? + GetRegisteredFontInstallPath(context, fileInfo.FilePath) : std::nullopt; + if (registeredPath) { - // User-installed and non-Winget fonts have same registry package path and install path. - fileInfo.RegistryInstallPath = fileInfo.RegistryPackagePath; + fileInfo.InstallPath = registeredPath.value(); } - - // Check if the file is already in the install path, in which case this is a query. - auto const& installPath = GetFontFileInstallPath(context); - if (installPath == fileInfo.FilePath.parent_path()) + else if (context.InstallerSource == InstallerSource::Unknown) { fileInfo.InstallPath = fileInfo.FilePath; } else { - if (fileInfo.InstallerSource == InstallerSource::WinGet && fileInfo.Scope == Manifest::ScopeEnum::Machine) - { - // Machine installed WinGet fonts must have a non-conflicting filename in the root font folder. - // To ensure a unique font name and allow different versions to exist side by side, the install file - // name may be a different name from the original package install. To account for this, the Package - // registry location uses the original filename as the registry value name and the value as the - // resulting path. If we are installing a machine font that already exists, we must check to see - // if this file already has a mapping defined. - const auto& installedFilePath = GetRegisteredMachineFontInstallPath(fileInfo.RegistryPackagePath, fileInfo.FilePath.filename()); - if (installedFilePath.has_value()) - { - fileInfo.InstallPath = installedFilePath.value(); - } - else - { - // File not already defined, we need to create a non-conflicting filename. - // Machine-installed WinGet must provide a non-conflicting filename to the root install location. - fileInfo.InstallPath = GetUniquePathForDestination(fileInfo.FilePath, installPath); - } - } - else - { - fileInfo.InstallPath = installPath / fileInfo.FilePath.filename(); - } - } - - // If our font file is registered it will exist in the install path and have a registry value. - const auto& registryTitle = CheckRegistryForFontFileReference(fileInfo.RegistryInstallPath, fileInfo.InstallPath, fileInfo.Scope); - - if (fileInfo.Title.empty()) - { - // Use the registry title or filename if we still dont' know. - if (registryTitle.has_value()) - { - fileInfo.Title = registryTitle.value(); - } - else - { - fileInfo.Title = fileInfo.FilePath.filename().wstring(); - } + fileInfo.InstallPath = GetUniquePathForDestination(fileInfo.FilePath, context.Scope); } - fileInfo.IsFontFileInstalled = std::filesystem::exists(fileInfo.InstallPath) ? true : false; - fileInfo.IsFontFileRegistered = registryTitle.has_value() ? true : false; + fileInfo.IsFontFileInstalled = std::filesystem::exists(fileInfo.InstallPath); + fileInfo.IsFontFileRegistered = !GetFontRegistrations(fileInfo.Scope, fileInfo.InstallPath).empty(); if (fileInfo.IsFontFileInstalled && fileInfo.IsFontFileRegistered) { fileInfo.Status = FontStatus::OK; @@ -472,6 +646,10 @@ namespace AppInstaller::Fonts // Create font file info for each package file for (const auto& file : context.PackageFiles) { + THROW_HR_IF_MSG(APPINSTALLER_CLI_ERROR_FONT_VALIDATION_FAILED, + std::any_of(result.FontFileInfos.begin(), result.FontFileInfos.end(), [&](const auto& info) + { return FontPathsEqual(info.FilePath.filename(), file.filename()); }), + "Font package contains duplicate filenames: %ls", file.filename().c_str()); result.FontFileInfos.push_back(CreateFontFileInfo(context, file)); } @@ -534,7 +712,7 @@ namespace AppInstaller::Fonts } // Validate the package data was all processed successfully. - const auto& validationResult = ValidateFontPackage(context); + auto validationResult = ValidateFontPackage(context); if (validationResult.Result != FontResult::Success) { result.HResult = winrt::hresult(APPINSTALLER_CLI_ERROR_FONT_FILE_NOT_SUPPORTED); @@ -558,13 +736,15 @@ namespace AppInstaller::Fonts return result; } - // We will attempt a cleanup / force install if either this is a forced install or the package was not in a good state. - if (validationResult.Status == FontStatus::Corrupt || context.Force) + const auto installRoot = GetRootFontPath(context.Scope); + const bool usesPackageDirectory = std::any_of(validationResult.FontFileInfos.begin(), validationResult.FontFileInfos.end(), + [&](const auto& info) { return !FontPathsEqual(info.InstallPath.parent_path(), installRoot); }); + if (validationResult.Status == FontStatus::Corrupt || context.Force || usesPackageDirectory) { // Force install of a font can have problems because if the font is already there then it // may be in use. The scenarios for using force is if a prior install attempt failed, so // we will try to clean up the existing font registration so it may be successful this time. - AICLI_LOG(Core, Info, << "Package is corrupt or forced install, attempting to remove any existing registration."); + AICLI_LOG(Core, Info, << "Removing existing font package mappings before installation."); auto uninstallResult = UninstallFontPackage(context); if (uninstallResult.Result() != FontResult::Success) { @@ -572,6 +752,14 @@ namespace AppInstaller::Fonts AICLI_LOG(Core, Error, << "Font cleanup uninstall failed: " << context.PackageId.c_str() << " - " << AppInstaller::Logging::SetHRFormat << uninstallResult.HResult); return result; } + + validationResult = ValidateFontPackage(context); + if (validationResult.Result != FontResult::Success || validationResult.HasUnsupportedFonts) + { + result.HResult = APPINSTALLER_CLI_ERROR_FONT_VALIDATION_FAILED; + AICLI_LOG(Core, Error, << "Font validation failed after cleanup: " << context.PackageId.c_str()); + return result; + } } AICLI_LOG(Core, Info, << "Starting install of " << context.PackageId.c_str()); @@ -581,70 +769,32 @@ namespace AppInstaller::Fonts // Install each file from the file info. for (const auto& fontFileInfo : validationResult.FontFileInfos) { - // Font install step 1: Copy file to winget package identifiable location. AICLI_LOG(Core, Info, << "Moving " << fontFileInfo.FilePath << " to " << fontFileInfo.InstallPath); if (!std::filesystem::exists(fontFileInfo.InstallPath.parent_path())) { std::filesystem::create_directories(fontFileInfo.InstallPath.parent_path()); } - // The file should not be present, but if it is, do another attempt to remove it. - if (std::filesystem::exists(fontFileInfo.InstallPath)) - { - // Try to remove font resource to avoid file-in-use. - RemoveAllFontResources(fontFileInfo.InstallPath); - if (!std::filesystem::remove(fontFileInfo.InstallPath)) + // Copying into the Fonts directory makes the file visible to system font discovery. + std::filesystem::copy_file(fontFileInfo.FilePath, fontFileInfo.InstallPath); + auto removeUnregisteredFile = wil::scope_exit([&]() { - AICLI_LOG(Core, Error, << "Font file already exists and was unable to be removed."); - THROW_HR(APPINSTALLER_CLI_ERROR_FONT_INSTALL_FAILED); - } - } - - if (context.Scope == Manifest::ScopeEnum::Machine) - { - // Using std::filesystem::rename does not work when installing into the system Fonts - // folder; the system does not recognize the font files. std::filesystem::copy_file does work. - std::filesystem::copy_file(fontFileInfo.FilePath, fontFileInfo.InstallPath); - std::filesystem::remove(fontFileInfo.FilePath); - } - else - { - // We prefer to use rename where it works. - AppInstaller::Filesystem::RenameFile(fontFileInfo.FilePath, fontFileInfo.InstallPath); - } + try + { + std::filesystem::remove(fontFileInfo.InstallPath); + } + catch (...) + { + LOG_CAUGHT_EXCEPTION(); + AICLI_LOG(Core, Error, << "Failed removing font file after installation failure: " << fontFileInfo.InstallPath); + } + }); AICLI_LOG(Core, Info, << "Adding " << fontFileInfo.Title.c_str() << " to " << fontFileInfo.InstallPath); - if (context.Scope == Manifest::ScopeEnum::User) - { - auto key = Registry::Key::Create(HKEY_CURRENT_USER, fontFileInfo.RegistryInstallPath); - key.SetValue(fontFileInfo.Title, fontFileInfo.InstallPath, REG_SZ); - } - else - { - // Machine install we set two keys, one for source information, and one for the actual install. - // The value name is the source filename so we can map this later for validation. - auto sourceKey = Registry::Key::Create(HKEY_LOCAL_MACHINE, fontFileInfo.RegistryPackagePath, 0UL, KEY_ALL_ACCESS); - sourceKey.SetValue(fontFileInfo.FilePath.filename(), fontFileInfo.InstallPath, REG_SZ); - - // Machine font install uses relative filename. Use filename as value to avoid collisions in the key. - auto installKey = Registry::Key::OpenIfExists(HKEY_LOCAL_MACHINE, fontFileInfo.RegistryInstallPath, 0UL, KEY_ALL_ACCESS); - installKey.SetValue(fontFileInfo.InstallPath.filename(), fontFileInfo.InstallPath.filename(), REG_SZ); - } - - // Add Font Resource to the session. - const auto& fontsAdded = ::AddFontResource(fontFileInfo.InstallPath.c_str()); - if (fontsAdded == 0) - { - // AddFontResource does not add additional information for us, so we dont know why they were not added, - // only that they were not added. At this point the "install" is successful, but we were not able to - // add the font to the system, which means subsequent adds on session start may also fail, but is not - // guaranteed to fail. We will note it in the log and carry on. - AICLI_LOG(Core, Warning, << "Failed to add font resource: " << fontFileInfo.InstallPath); - } - else - { - AICLI_LOG(Core, Info, << "Added " << fontsAdded << " fonts to the session."); - } + RegisterFontFile(context, fontFileInfo.FilePath.filename(), fontFileInfo.InstallPath); + std::filesystem::remove(fontFileInfo.FilePath); + removeUnregisteredFile.release(); + AddFontToSession(fontFileInfo.InstallPath); } result.HResult = S_OK; @@ -678,120 +828,114 @@ namespace AppInstaller::Fonts return result; } - FontOperationResult UninstallFontPackage(FontContext& context) + FontOperationResult RepairFontPackage(FontContext& context) { - FontOperationResult result; - - if (context.InstallerSource != InstallerSource::WinGet) - { - throw std::logic_error("Only WinGet format of font package uninstall is supported."); - } - AssertPackageInformation(context); - - const auto& installFolderPath = GetFontFileInstallPath(context); - const auto& installPackagePath = GetFontRegistryPath(context); - std::vector filesToRemove; + THROW_HR_IF(E_INVALIDARG, context.InstallerSource != InstallerSource::WinGet); + FontOperationResult result; try { - auto hive = context.Scope == Manifest::ScopeEnum::Machine ? HKEY_LOCAL_MACHINE : HKEY_CURRENT_USER; - auto key = Registry::Key::OpenIfExists(hive, installPackagePath, 0ul, KEY_ALL_ACCESS); + auto files = GetPackageFontFiles(context); + THROW_HR_IF(APPINSTALLER_CLI_ERROR_NO_REPAIR_INFO_FOUND, files.empty()); - if (key) + FontCatalog catalog; + for (size_t i = 0; i < files.size(); ++i) { - // Assume all values in this key are fonts related to this package. - for (const auto& fontEntry : key.Values()) + THROW_HR_IF(APPINSTALLER_CLI_ERROR_FONT_FILE_NOT_FOUND, !std::filesystem::is_regular_file(files[i].FilePath)); + DWRITE_FONT_FILE_TYPE fileType; + THROW_HR_IF(APPINSTALLER_CLI_ERROR_FONT_FILE_NOT_SUPPORTED, !catalog.IsFontFileSupported(files[i].FilePath, fileType)); + for (size_t j = 0; j < i; ++j) { - auto value = fontEntry.Value(); - if (!value.has_value() || (value.value().GetType() != Registry::Value::Type::String)) - { - // Not a valid entry, nothing to do here. - continue; - } - - std::filesystem::path filePath = { value->GetValue() }; - if (!std::filesystem::exists(filePath)) - { - // File doesn't exist, nothing to do here. - continue; - } - - // The font may be in use by the system or other apps, it needs to be removed from - // from use or at least attempted to be removed from use. - RemoveAllFontResources(filePath); - filesToRemove.push_back(filePath); + THROW_HR_IF(APPINSTALLER_CLI_ERROR_FONT_VALIDATION_FAILED, + FontPathsEqual(files[i].FilePath, files[j].FilePath) || + FontPathsEqual(files[i].OriginalFileName, files[j].OriginalFileName)); } + } - // Delete the key - if (!Registry::Key::DeleteTree(hive, installPackagePath)) + const auto rootPath = GetRootFontPath(context.Scope); + for (const auto& file : files) + { + if (FontPathsEqual(file.FilePath.parent_path(), rootPath)) { - AICLI_LOG(Core, Warning, << "Failed removing registry tree " << installPackagePath.c_str()); + RegisterFontFile(context, file.OriginalFileName, file.FilePath); + RemoveAllFontResources(file.FilePath); + AddFontToSession(file.FilePath); + continue; } - if (context.Scope == Manifest::ScopeEnum::Machine) - { - // Machine installed fonts also have an entry in the System font registry. - // We have to find the values that have the filename as data. - auto machineFontRoot = Registry::Key::OpenIfExists(HKEY_LOCAL_MACHINE, s_FontsPathSubkey.data(), 0ul, KEY_ALL_ACCESS); - - for (const auto& file : filesToRemove) - { - machineFontRoot.DeleteValue(file.filename()); - } - } + MigrateFontFile(context, file); } + + RemoveEmptyUserFontPackageDirectories(context); } catch (...) { + result.HResult = wil::ResultFromCaughtException(); LOG_CAUGHT_EXCEPTION(); + AICLI_LOG(Core, Error, << "Font repair failed for " << context.PackageId.c_str()); + } + + NotifyFontChange(); + return result; + } - // For uninstall we will log the error and continue trying to remove it, since we have partial remove and are in an unknown state. - AICLI_LOG(Core, Error, << "Failed removing fonts in the registry."); + FontOperationResult UninstallFontPackage(FontContext& context) + { + AssertPackageInformation(context); + THROW_HR_IF(E_INVALIDARG, context.InstallerSource != InstallerSource::WinGet); + FontOperationResult result; + + std::vector files; + try + { + files = GetPackageFontFiles(context); + } + catch (...) + { + LOG_CAUGHT_EXCEPTION(); + AICLI_LOG(Core, Error, << "Failed reading font package mappings."); + result.HResult = APPINSTALLER_CLI_ERROR_FONT_UNINSTALL_FAILED; + return result; } - // Remove any font files. - for (const auto& file : filesToRemove) + auto hive = GetFontRegistryHive(context.Scope); + auto package = Registry::Key::OpenIfExists(hive, GetFontRegistryPath(context), 0UL, KEY_ALL_ACCESS); + for (const auto& file : files) { try { - if (std::filesystem::exists(file)) + RemoveAllFontResources(file.FilePath); + if (std::filesystem::exists(file.FilePath)) { - // TODO: Add robustness for files-in-use scenarios. - std::filesystem::remove(file); + THROW_HR_IF(APPINSTALLER_CLI_ERROR_FONT_UNINSTALL_FAILED, !std::filesystem::remove(file.FilePath)); } + UnregisterFontFile(context.Scope, file.FilePath); + package.DeleteValue(file.RegistryName); } catch (...) { LOG_CAUGHT_EXCEPTION(); - AICLI_LOG(Core, Error, << "Failed removing font files."); - result.HResult = winrt::hresult(APPINSTALLER_CLI_ERROR_FONT_UNINSTALL_FAILED); + AICLI_LOG(Core, Error, << "Failed removing font file or registration: " << file.FilePath); + result.HResult = APPINSTALLER_CLI_ERROR_FONT_UNINSTALL_FAILED; } } - if (context.Scope == Manifest::ScopeEnum::User) + if (result.Result() == FontResult::Success) { - // User installed fonts also have an install folder, we will clean that up as well. - if (std::filesystem::exists(installFolderPath)) + try { - try + if (package) { - const auto& parent = installFolderPath.parent_path(); - std::filesystem::remove_all(installFolderPath); - - // This may have been the only version installed, so remove parent if empty. - if (std::filesystem::is_empty(parent)) - { - std::filesystem::remove(parent); - } - } - catch (...) - { - // Font files have already been removed at this point, so failure to delete - // this folder is not a failure of removing the fonts. - LOG_CAUGHT_EXCEPTION(); - AICLI_LOG(Core, Error, << "Failed removing font folders."); + Registry::Key::DeleteTree(hive, GetFontRegistryPath(context)); } + RemoveEmptyUserFontPackageDirectories(context); + } + catch (...) + { + LOG_CAUGHT_EXCEPTION(); + AICLI_LOG(Core, Error, << "Failed removing font package metadata or directories."); + result.HResult = APPINSTALLER_CLI_ERROR_FONT_UNINSTALL_FAILED; } } @@ -957,13 +1101,16 @@ namespace AppInstaller::Fonts try { - auto wingetMachineFonts = std::unordered_map(); - // Iterate through scopes for machine and user for (const auto& scope : { Manifest::ScopeEnum::Machine, Manifest::ScopeEnum::User }) { - auto hive = scope == Manifest::ScopeEnum::Machine ? HKEY_LOCAL_MACHINE : HKEY_CURRENT_USER; + std::vector wingetFonts; + auto hive = GetFontRegistryHive(scope); auto root = Registry::Key::OpenIfExists(hive, std::wstring{ s_FontsPathSubkey }); + if (!root) + { + continue; + } // There are two supported scenarios for tracking sub-keys. // 1) We created the subkey, therefore it is a winget installed font. @@ -996,18 +1143,14 @@ namespace AppInstaller::Fonts } std::filesystem::path filePath = { value->GetValue() }; - if (scope == Manifest::ScopeEnum::Machine) - { - // Capture the filename for winget machine fonts so we can avoid duplicates. - wingetMachineFonts[filePath.filename()] = true; - } + wingetFonts.push_back(filePath); auto context = FontContext(); context.Scope = scope; context.InstallerSource = InstallerSource::WinGet; context.PackageId = packageId; context.PackageVersion = version; - auto fontFile = CreateFontFileInfo(context, filePath, ConvertToUTF16(versionValue.Name())); + auto fontFile = CreateFontFileInfo(context, filePath); fontFiles.push_back(std::move(fontFile)); } } @@ -1025,13 +1168,11 @@ namespace AppInstaller::Fonts } std::filesystem::path filePath = { value->GetValue() }; - if (scope == Manifest::ScopeEnum::Machine) + auto registeredPath = filePath.is_relative() ? GetRootFontPath(scope) / filePath : filePath; + if (std::any_of(wingetFonts.begin(), wingetFonts.end(), + [&](const auto& path) { return FontPathsEqual(path, registeredPath); })) { - // Skip it if this is already identified in the WinGet machine fonts. - if (wingetMachineFonts[filePath.filename()]) - { - continue; - } + continue; } auto context = FontContext(); @@ -1058,8 +1199,12 @@ namespace AppInstaller::Fonts try { - auto hive = scope == Manifest::ScopeEnum::Machine ? HKEY_LOCAL_MACHINE : HKEY_CURRENT_USER; + auto hive = GetFontRegistryHive(scope); auto root = Registry::Key::OpenIfExists(hive, s_FontsPathSubkey.data(), 0UL, KEY_READ); + if (!root) + { + return fontPackages; + } for (const auto& subkey : root) { diff --git a/src/AppInstallerCommonCore/Public/winget/Fonts.h b/src/AppInstallerCommonCore/Public/winget/Fonts.h index abc2bf90d5..66bf286941 100644 --- a/src/AppInstallerCommonCore/Public/winget/Fonts.h +++ b/src/AppInstallerCommonCore/Public/winget/Fonts.h @@ -125,6 +125,8 @@ namespace AppInstaller::Fonts FontOperationResult InstallFontPackage(FontContext& context); + FontOperationResult RepairFontPackage(FontContext& context); + FontOperationResult UninstallFontPackage(FontContext& context); std::wstring GetFontRegistryRoot(); From 67898a5f21e1aa9e577e373a748d254c9dfff740 Mon Sep 17 00:00:00 2001 From: David Bennett Date: Mon, 26 Oct 2026 13:11:53 -0700 Subject: [PATCH 2/2] Fix font layout test registry isolation Add a regression for volatile shared test keys and acknowledge fontlayout in the spelling dictionary. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a9e21d38-9ab1-4358-963d-9aadad897ad0 --- .github/actions/spelling/expect.txt | 1 + src/AppInstallerCLITests/Fonts.cpp | 42 ++++++++++++++++++++++++++--- 2 files changed, 40 insertions(+), 3 deletions(-) diff --git a/.github/actions/spelling/expect.txt b/.github/actions/spelling/expect.txt index 42fb16ffcb..02b00f28f7 100644 --- a/.github/actions/spelling/expect.txt +++ b/.github/actions/spelling/expect.txt @@ -196,6 +196,7 @@ FOF FOLDERID FONTCHANGE FONTHASH +fontlayout FORPARSING foundfr fsanitize diff --git a/src/AppInstallerCLITests/Fonts.cpp b/src/AppInstallerCLITests/Fonts.cpp index 2b53198d2f..eecdc410e0 100644 --- a/src/AppInstallerCLITests/Fonts.cpp +++ b/src/AppInstallerCLITests/Fonts.cpp @@ -23,8 +23,15 @@ namespace struct FontLayoutTest { FontLayoutTest() : - RegistryPath(L"Software\\Microsoft\\WinGet\\FontLayoutTests\\" + Directory.GetPath().filename().wstring()), - RegistryHive(AppInstaller::Registry::Key::Create(HKEY_CURRENT_USER, RegistryPath)), + FontLayoutTest(AppInstaller::Registry::Key::OpenIfExists(HKEY_CURRENT_USER, std::wstring{}, 0, KEY_ALL_ACCESS)) + { + } + + explicit FontLayoutTest(AppInstaller::Registry::Key registryRoot) : + RegistryRoot(std::move(registryRoot)), + // Font registration keys require non-volatile ancestors. + RegistryPath(L"WinGetFontLayoutTests_" + Directory.GetPath().filename().wstring()), + RegistryHive(AppInstaller::Registry::Key::Create(RegistryRoot, RegistryPath)), UserPath(AppInstaller::Runtime::GetPathDetailsFor(AppInstaller::Runtime::PathName::FontsUserInstallLocation)), MachinePath(AppInstaller::Runtime::GetPathDetailsFor(AppInstaller::Runtime::PathName::FontsMachineInstallLocation)) { @@ -40,7 +47,7 @@ namespace TestHook_SetFontSessionChangesDisabled(false); AppInstaller::Runtime::TestHook_SetPathOverride(AppInstaller::Runtime::PathName::FontsUserInstallLocation, UserPath); AppInstaller::Runtime::TestHook_SetPathOverride(AppInstaller::Runtime::PathName::FontsMachineInstallLocation, MachinePath); - AppInstaller::Registry::Key::DeleteTree(HKEY_CURRENT_USER, RegistryPath); + AppInstaller::Registry::Key::DeleteTree(RegistryRoot, RegistryPath); } std::filesystem::path RootPath(ScopeEnum scope) const @@ -93,6 +100,7 @@ namespace } TestCommon::TempDirectory Directory{ "FontLayout" }; + AppInstaller::Registry::Key RegistryRoot; std::wstring RegistryPath; AppInstaller::Registry::Key RegistryHive; AppInstaller::Filesystem::PathDetails UserPath; @@ -118,6 +126,34 @@ namespace } } +TEST_CASE("FontLayout_InstallWithVolatileSharedTestTree", "[fonts][fontlayout]") +{ + const auto scope = GENERATE(ScopeEnum::User, ScopeEnum::Machine); + const TempDirectory directory{ "FontRegistry" }; + const auto registryPath = directory.GetPath().filename().wstring(); + auto cleanup = wil::scope_exit([&]() + { + AppInstaller::Registry::Key::DeleteTree(HKEY_CURRENT_USER, registryPath); + }); + auto registryRoot = AppInstaller::Registry::Key::Create(HKEY_CURRENT_USER, registryPath); + AppInstaller::Registry::Key::Create(registryRoot, L"Software\\Microsoft\\WinGet", REG_OPTION_VOLATILE); + REQUIRE_THROWS_HR( + AppInstaller::Registry::Key::Create(registryRoot, L"Software\\Microsoft\\WinGet\\FontLayoutTests"), + HRESULT_FROM_WIN32(ERROR_CHILD_MUST_BE_VOLATILE)); + + FontLayoutTest layout{ std::move(registryRoot) }; + auto context = layout.Context(scope); + const auto source = layout.Directory.GetPath() / "Source" / s_FontFile; + const auto destination = layout.RootPath(scope) / s_FontFile; + layout.CopyFont(source); + context.AddPackageFile(source); + REQUIRE(InstallFontPackage(context).HResult == S_OK); + REQUIRE(RegistryString(layout.Package(context), L"TestFont.ttf") == destination.wstring()); + REQUIRE(RegistryString(layout.Root(scope), L"TestFont.ttf") == + (scope == ScopeEnum::Machine ? destination.filename().wstring() : destination.wstring())); + REQUIRE(UninstallFontPackage(context).HResult == S_OK); +} + TEST_CASE_METHOD(FontLayoutTest, "FontLayout_InstallRepairAndUninstall", "[fonts][fontlayout]") { const auto scope = GENERATE(ScopeEnum::User, ScopeEnum::Machine);