diff --git a/.github/actions/spelling/patterns.txt b/.github/actions/spelling/patterns.txt index 529cbc6a5b..280a58d2b4 100644 --- a/.github/actions/spelling/patterns.txt +++ b/.github/actions/spelling/patterns.txt @@ -9,6 +9,9 @@ REQUIRE(?:[_A-Z]*)\(.*\) 8wekyb3d8bbwe _tisf_sqliteReturnValue +# Compact installer log timestamp format +\bYYMMDDhhmmss\b + El proyecto .* diferentes # Package family names and package full names diff --git a/doc/Settings.md b/doc/Settings.md index 6aa29dc770..9967c0d1ac 100644 --- a/doc/Settings.md +++ b/doc/Settings.md @@ -320,6 +320,12 @@ In addition, there are special values that cover multiple channels. `default` i Sets the default strategy for naming log files for installers that support it. `manifest` is the default and uses the manifest name. `timestamp` uses the date and time. `guid` uses a generated GUID. `shortguid` uses the first 8 characters of a generated GUID. Invalid values will revert to `manifest`. +The `manifest` strategy targets a complete log path of at most 150 UTF-16 code units, including the directory and `.log` extension. Existing names are retained when they fit. Otherwise, WinGet omits the package version and uses a compact `YYMMDDhhmmss` timestamp. If that still exceeds the preferred length, it uses an `hhmmss` timestamp and retains as much of the start of the package ID as fits, preserving whole Unicode characters. If no package-ID prefix fits, it falls back to an eight-character random name. The complete package identity and selected path remain available in WinGet's diagnostic log. + +This leaves additional room for installers that append text to the supplied log name, but suffix compatibility is best effort, not guaranteed. The log directory is unchanged. Other configured naming strategies retain their formats when they fit within the Windows path limit. Any generated path that reaches `MAX_PATH` falls back to the eight-character random name. WinGet passes that fallback path to the installer even if it still reaches `MAX_PATH`; it does not omit arguments or display a warning because of the path length. + +Explicit log paths, such as those supplied with `--log`, are not shortened or omitted by this policy. For configuration runs, `WinGetUserSettings` can select `logging.fileNameStrategy` for the execution user's settings; `WinGetPackage` does not expose a per-package log-path property. + ```json "logging": { "fileNameStrategy": "manifest" | "timestamp" | "guid" | "shortguid" diff --git a/doc/release-notes/1.31/2036-shorter-installer-log-paths.md b/doc/release-notes/1.31/2036-shorter-installer-log-paths.md new file mode 100644 index 0000000000..b9cd0bd32a --- /dev/null +++ b/doc/release-notes/1.31/2036-shorter-installer-log-paths.md @@ -0,0 +1,7 @@ +## Bug Fixes + +### Shorter automatic installer log paths + +WinGet now shortens default installer log names as needed to target a complete path of 150 characters, leaving more room for installers that append their own log names. Names retain the package version and timestamp when they fit; otherwise, WinGet omits the version and uses a compact timestamp, then a package-ID prefix with a time-only timestamp, before falling back to a short random name. + +This is a best-effort mitigation, not a guarantee for installer-added suffixes. The log directory and explicit log paths are unchanged. WinGet still passes the shortest fallback path to the installer if it exceeds the Windows path limit, without removing installer arguments. diff --git a/src/AppInstallerCLICore/Workflows/DownloadFlow.cpp b/src/AppInstallerCLICore/Workflows/DownloadFlow.cpp index 0a0db10b27..172a551887 100644 --- a/src/AppInstallerCLICore/Workflows/DownloadFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/DownloadFlow.cpp @@ -751,7 +751,7 @@ namespace AppInstaller::CLI::Workflow } const auto& manifest = context.Get(); - context.Add(downloadsDirectory / GetPathPart(manifest, '_', true)); + context.Add(downloadsDirectory / GetPathPart(manifest, '_', PathPartVersionStrategy::DropUnknownVersion)); } } diff --git a/src/AppInstallerCLICore/Workflows/ShellExecuteInstallerHandler.cpp b/src/AppInstallerCLICore/Workflows/ShellExecuteInstallerHandler.cpp index b4674f7f2f..62dbc9b346 100644 --- a/src/AppInstallerCLICore/Workflows/ShellExecuteInstallerHandler.cpp +++ b/src/AppInstallerCLICore/Workflows/ShellExecuteInstallerHandler.cpp @@ -80,7 +80,7 @@ namespace AppInstaller::CLI::Workflow const auto& installer = context.Get(); const auto& installerSwitches = installer->Switches; - std::string installerArgs = {}; + std::string installerArgs; // Construct install experience arg. // SilentWithProgress is default, so look for it first. @@ -159,67 +159,121 @@ namespace AppInstaller::CLI::Workflow return installerArgs; } - // Applies values to the template. - void PopulateInstallerArgsTemplate(Execution::Context& context, std::string& installerArgs) + std::filesystem::path GetAutomaticInstallerLogPath(Execution::Context& context) { - // Populate with value from command line or temp path. - std::string logPath; - if (context.Args.Contains(Execution::Args::Type::Log)) + // Chosen to allow a reported 108 character suffix from installer. + constexpr size_t preferredPathLength = 150; + auto directory = Runtime::GetPathTo(Runtime::PathName::DefaultLogLocation); + auto makePath = [&](const std::filesystem::path& stem) { - logPath = context.Args.GetArg(Execution::Args::Type::Log); - } - else + auto result = directory / stem; + result += Logging::FileLogger::DefaultExt(); + return result; + }; + auto shortGuidPath = [&]() + { + return makePath(Utility::CreateNewGuidNameWString().substr(0, 8)); + }; + const auto time = std::chrono::system_clock::now(); + const auto timestamp = Utility::TimePointToString(time, Utility::TimeFacet::ShortYearSecondPrecision | Utility::TimeFacet::Filename); + std::filesystem::path result; + + switch (Settings::User().Get()) + { + case Logging::LogNameStrategy::Manifest: { const auto& manifest = context.Get(); - const Logging::LogNameStrategy logNameStrategy = Settings::User().Get(); - auto path = Runtime::GetPathTo(Runtime::PathName::DefaultLogLocation); + result = makePath(GetPathPart(manifest).native() + L'-' + Utility::ConvertToUTF16(timestamp)); - switch (logNameStrategy) + if (result.native().size() > preferredPathLength) { - case Logging::LogNameStrategy::Manifest: - // Use manifest ID and version for log file name - // Results in \.-.log - path /= GetPathPart(manifest); - path += '-'; - path += Utility::GetCurrentTimeForFilename(true); - break; - case Logging::LogNameStrategy::Timestamp: - // Use only timestamp for log file name - // Results in \.log - path /= Utility::GetCurrentTimeForFilename(true); - break; - case Logging::LogNameStrategy::Guid: - // Use a GUID for log file name - // Results in \.log - path /= Utility::CreateNewGuidNameWString(); - break; - case Logging::LogNameStrategy::ShortGuid: - // Use the first 8 characters of a GUID for log file name - // Results in \.log - path /= Utility::CreateNewGuidNameWString().substr(0, 8); - break; - default: - // This should never happen due to validation when reading settings, but handle it just in case. - AICLI_LOG(CLI, Error, << "Unknown log naming strategy."); - THROW_HR(E_UNEXPECTED); - break; + auto compactTimestamp = timestamp; + compactTimestamp.erase(std::remove(compactTimestamp.begin(), compactTimestamp.end(), '-'), compactTimestamp.end()); + const auto suffix = L"-" + Utility::ConvertToUTF16(compactTimestamp); + const auto id = GetPathPart(manifest, '.', PathPartVersionStrategy::NeverIncludeVersion); + result = makePath(id.native() + suffix); + + if (result.native().size() > preferredPathLength) + { + const auto timeSuffix = L"-" + Utility::ConvertToUTF16(compactTimestamp.substr(6)); + const auto fixedLength = makePath(timeSuffix).native().size(); + if (fixedLength < preferredPathLength && !id.empty()) + { + const auto idString = id.u8string(); + const auto idBudget = preferredPathLength - fixedLength; + auto prefixLength = idBudget; + auto prefix = Utility::ConvertToUTF16(Utility::UTF8Substring(idString, 0, prefixLength)); + // Substring counts graphemes, but the path budget counts UTF-16 code units. + while (prefix.size() > idBudget) + { + prefix = Utility::ConvertToUTF16(Utility::UTF8Substring(idString, 0, --prefixLength)); + } + if (!prefix.empty()) + { + result = makePath(prefix + timeSuffix); + } + } + } + + if (result.native().size() > preferredPathLength) + { + result = shortGuidPath(); + } } + AICLI_LOG(CLI, Info, << "Installer log for " << manifest.Id << " [" << manifest.Version << "]: " << result.u8string()); + break; + } + case Logging::LogNameStrategy::Timestamp: + result = makePath(timestamp); + break; + case Logging::LogNameStrategy::Guid: + result = makePath(Utility::CreateNewGuidNameWString()); + break; + case Logging::LogNameStrategy::ShortGuid: + result = shortGuidPath(); + break; + default: + AICLI_LOG(CLI, Error, << "Unknown log naming strategy."); + THROW_HR(E_UNEXPECTED); + } - // Add the extension to the log file regardless of naming strategy - path += Logging::FileLogger::DefaultExt(); - - logPath = path.u8string(); + if (result.native().size() >= MAX_PATH) + { + result = shortGuidPath(); + } + if (result.native().size() > preferredPathLength) + { + AICLI_LOG(CLI, Warning, << "Installer log path exceeds the preferred length of " << preferredPathLength << ": " << result.u8string()); } - if (Utility::FindAndReplace(installerArgs, std::string(ARG_TOKEN_LOGPATH), logPath)) + return result; + } + + // Applies values to the template. + void PopulateInstallerArgsTemplate(Execution::Context& context, std::string& installerArgs) + { + if (installerArgs.find(ARG_TOKEN_LOGPATH) != std::string::npos) { - context.Add(Utility::ConvertToUTF16(logPath)); + std::filesystem::path logPath; + if (context.Args.Contains(Execution::Args::Type::Log)) + { + logPath = Utility::ConvertToUTF16(context.Args.GetArg(Execution::Args::Type::Log)); + } + else + { + logPath = GetAutomaticInstallerLogPath(context); + } + + auto logPathString = logPath.u8string(); + Utility::FindAndReplace(installerArgs, ARG_TOKEN_LOGPATH, logPathString); + context.Add(std::move(logPath)); } // Populate with value from command line. if (context.Args.Contains(Execution::Args::Type::InstallLocation)) { - Utility::FindAndReplace(installerArgs, std::string(ARG_TOKEN_INSTALLPATH), context.Args.GetArg(Execution::Args::Type::InstallLocation)); + auto locationArg = context.Args.GetArg(Execution::Args::Type::InstallLocation); + Utility::FindAndReplace(installerArgs, ARG_TOKEN_INSTALLPATH, locationArg); } // Todo: language token support will be implemented later @@ -334,8 +388,7 @@ namespace AppInstaller::CLI::Workflow return; } - std::string installerArgs = GetInstallerArgsTemplate(context); - + auto installerArgs = GetInstallerArgsTemplate(context); PopulateInstallerArgsTemplate(context, installerArgs); AICLI_LOG(CLI, Info, << "Installer args: " << installerArgs); diff --git a/src/AppInstallerCLIE2ETests/Pinning.cs b/src/AppInstallerCLIE2ETests/Pinning.cs index 5711a0e911..81414ef515 100644 --- a/src/AppInstallerCLIE2ETests/Pinning.cs +++ b/src/AppInstallerCLIE2ETests/Pinning.cs @@ -25,7 +25,7 @@ public void Setup() // All tests use TestExeInstaller; try to clean it up for failure cases, // then install the base version for pinning TestCommon.RunAICLICommand("uninstall", "AppInstallerTest.TestExeInstaller"); - TestCommon.RunAICLICommand("install", "AppInstallerTest.TestExeInstaller -v 1.0.0.0"); + TestCommon.RunAICLICommand("install", "AppInstallerTest.TestExeInstaller -v 1.0.0.0 --custom \"/Version 1.0.0.0\""); TestCommon.RunAICLICommand("pin remove", "AppInstallerTest.TestExeInstaller"); } @@ -71,8 +71,7 @@ public void UpgradeWithPinningPin() RunCommandResult result; string installDir = Path.GetTempPath(); - // The base version of this app does not log /Version, but it still includes the version number in the log file name. - Assert.That(TestCommon.VerifyTestExeInstalled(installDir, "1.0.0.0"), Is.True, "Base version installed"); + Assert.That(TestCommon.VerifyTestExeInstalled(installDir, "/Version 1.0.0.0"), Is.True, "Base version installed"); result = TestCommon.RunAICLICommand("pin add", "AppInstallerTest.TestExeInstaller"); Assert.That(result.ExitCode, Is.EqualTo(Constants.ErrorCode.S_OK)); @@ -89,7 +88,7 @@ public void UpgradeWithPinningPin() result = TestCommon.RunAICLICommand("upgrade", "--all"); Assert.That(result.ExitCode, Is.EqualTo(Constants.ErrorCode.S_OK), "Upgrade succeeds with nothing to upgrade"); - Assert.That(TestCommon.VerifyTestExeInstalled(installDir, "1.0.0.0"), Is.True, "No newer version installed"); + Assert.That(TestCommon.VerifyTestExeInstalled(installDir, "/Version 1.0.0.0"), Is.True, "No newer version installed"); result = TestCommon.RunAICLICommand("upgrade", "--include-pinned"); Assert.That(result.ExitCode, Is.EqualTo(Constants.ErrorCode.S_OK)); diff --git a/src/AppInstallerCLITests/InstallFlow.cpp b/src/AppInstallerCLITests/InstallFlow.cpp index d424fac669..69a62968ff 100644 --- a/src/AppInstallerCLITests/InstallFlow.cpp +++ b/src/AppInstallerCLITests/InstallFlow.cpp @@ -33,6 +33,28 @@ using namespace AppInstaller::Settings; using namespace AppInstaller::Utility; using namespace AppInstaller::Utility::literals; +namespace +{ + struct InstallerLogDirectoryOverride + { + InstallerLogDirectoryOverride(size_t length) : + Original(AppInstaller::Runtime::GetPathDetailsFor(AppInstaller::Runtime::PathName::DefaultLogLocation)) + { + AppInstaller::Filesystem::PathDetails details; + details.Path = L"C:\\" + std::wstring(length - 3, L'a'); + details.Create = false; + AppInstaller::Runtime::TestHook_SetPathOverride(AppInstaller::Runtime::PathName::DefaultLogLocation, details); + } + + ~InstallerLogDirectoryOverride() + { + AppInstaller::Runtime::TestHook_SetPathOverride(AppInstaller::Runtime::PathName::DefaultLogLocation, Original); + } + + AppInstaller::Filesystem::PathDetails Original; + }; +} + void OverrideForDirectMsi(TestContext& context) { OverrideForCheckExistingInstaller(context); @@ -863,8 +885,12 @@ TEST_CASE("PortableInstallFlow_MachineScope", "[InstallFlow][workflow]") REQUIRE(std::filesystem::exists(portableInstallResultPath.GetPath())); } -TEST_CASE("ShellExecuteHandlerInstallerArgs", "[InstallFlow][workflow]") +TEST_CASE("ShellExecuteHandlerInstallerArgs", "[InstallFlow][workflow][InstallerLogPath]") { + InstallerLogDirectoryOverride logDirectory{ 7 }; + TestUserSettings testSettings; + testSettings.Set(LogNameStrategy::Manifest); + { std::ostringstream installOutput; TestContext context{ installOutput, std::cin }; @@ -877,7 +903,7 @@ TEST_CASE("ShellExecuteHandlerInstallerArgs", "[InstallFlow][workflow]") context << GetInstallerArgs; std::string installerArgs = context.Get(); REQUIRE(installerArgs.find("/passive") != std::string::npos); - REQUIRE(installerArgs.find(FileLogger::DefaultPrefix()) != std::string::npos); + REQUIRE(installerArgs.find(context.Get().u8string()) != std::string::npos); REQUIRE(installerArgs.find(manifest.Id) != std::string::npos); REQUIRE(installerArgs.find(manifest.Version) != std::string::npos); } @@ -931,7 +957,7 @@ TEST_CASE("ShellExecuteHandlerInstallerArgs", "[InstallFlow][workflow]") context << GetInstallerArgs; std::string installerArgs = context.Get(); REQUIRE(installerArgs.find("/SILENT") != std::string::npos); - REQUIRE(installerArgs.find(FileLogger::DefaultPrefix()) != std::string::npos); + REQUIRE(installerArgs.find(context.Get().u8string()) != std::string::npos); REQUIRE(installerArgs.find(manifest.Id) != std::string::npos); REQUIRE(installerArgs.find(manifest.Version) != std::string::npos); } @@ -1030,6 +1056,8 @@ TEST_CASE("ShellExecuteHandlerInstallerArgs", "[InstallFlow][workflow]") TEST_CASE("ShellExecuteHandlerInstallerArgs_LogNamingStrategy", "[InstallFlow][workflow]") { + InstallerLogDirectoryOverride logDirectory{ 7 }; + SECTION("Manifest") { TestUserSettings testSettings; @@ -1122,6 +1150,244 @@ TEST_CASE("ShellExecuteHandlerInstallerArgs_LogNamingStrategy", "[InstallFlow][w } } +TEST_CASE("ShellExecuteHandlerInstallerArgs_LogPathBudget", "[InstallFlow][workflow][InstallerLogPath]") +{ + TestUserSettings testSettings; + testSettings.Set(LogNameStrategy::Manifest); + auto manifest = YamlParser::CreateFromPath(TestDataFile("InstallerArgTest_Inno_WithSwitches.yaml")); + manifest.Id = "Microsoft.DotNet.SDK.10"; + manifest.Version = "10.0.404"; + size_t directoryLength = 95; + bool shortened = false; + bool timeOnly = false; + bool shortGuid = false; + std::wstring expectedPrefix; + + SECTION("ExactlyPreferredLength") + { + } + SECTION("JustAbovePreferredLength") + { + directoryLength = 96; + shortened = true; + } + SECTION("ExactlyPreferredCompactLength") + { + directoryLength = 109; + shortened = true; + } + SECTION("TimeOnlyWithoutTruncation") + { + directoryLength = 110; + shortened = timeOnly = true; + expectedPrefix = L"Microsoft.DotNet.SDK.10"; + } + SECTION("ReportedNestedSuffixes") + { + directoryLength = 119; + shortened = timeOnly = true; + expectedPrefix = L"Microsoft.DotNet.SD"; + } + SECTION("LongVersion") + { + manifest.Version = std::string(200, '1'); + shortened = true; + } + SECTION("UnicodePackageId") + { + std::string id = "Publisher."; + for (size_t i = 0; i < 30; ++i) + { + id += "\xF0\x9F\x98\x80"; + } + manifest.Id = id + ".SDK"; + directoryLength = 119; + shortened = timeOnly = true; + expectedPrefix = L"Publisher.\U0001F600\U0001F600\U0001F600\U0001F600"; + } + SECTION("LongPackageIdentity") + { + manifest.Id = std::string(200, 'a') + ".SDK"; + directoryLength = 119; + shortened = timeOnly = true; + expectedPrefix = std::wstring(19, L'a'); + } + SECTION("NormalizedPackageIdPrefix") + { + manifest.Id = "Publisher.e\xCC\x81.SDK"; + directoryLength = 127; + shortened = timeOnly = true; + expectedPrefix = L"Publisher.\u00E9"; + } + SECTION("CombiningCharacterBoundary") + { + // Unlike e + acute, x + acute remains two code units after normalization. + manifest.Id = "Publisher.x\xCC\x81.SDK"; + directoryLength = 127; + shortened = timeOnly = true; + expectedPrefix = L"Publisher."; + } + SECTION("OneCharacterPrefix") + { + directoryLength = 137; + shortened = timeOnly = true; + expectedPrefix = L"M"; + } + SECTION("NoRoomForFirstGrapheme") + { + manifest.Id = "\xF0\x9F\x98\x80.SDK"; + directoryLength = 137; + shortGuid = true; + } + + InstallerLogDirectoryOverride logDirectory{ directoryLength }; + std::ostringstream installOutput; + TestContext context{ installOutput, std::cin }; + auto previousThreadGlobals = context.SetForCurrentThread(); + context.SetFlags(ContextFlag::InstallerExecutionUseUpdate); + context.Add(manifest); + context.Add(manifest.Installers.at(0)); + context << GetInstallerArgs; + + REQUIRE_FALSE(context.IsTerminated()); + REQUIRE(installOutput.str().empty()); + REQUIRE(context.Contains(Data::LogPath)); + const auto& path = context.Get(); + INFO(path.u8string()); + REQUIRE(path.native().size() <= 150); + REQUIRE(path.extension() == L".log"); + REQUIRE(MakeSuitablePathPart(path.filename().u8string()) == path.filename().u8string()); + const auto stem = path.stem().u8string(); + REQUIRE(ConvertToUTF16(ConvertToUTF8(path.native())) == path.native()); + if (shortGuid) + { + REQUIRE(stem.size() == 8); + REQUIRE(stem.find('-') == std::string::npos); + } + else if (shortened) + { + const auto timestampStart = stem.rfind('-'); + REQUIRE(timestampStart != std::string::npos); + const auto timestamp = stem.substr(timestampStart + 1); + REQUIRE(timestamp.size() == (timeOnly ? 6u : 12u)); + REQUIRE(std::all_of(timestamp.begin(), timestamp.end(), [](char c) { return c >= '0' && c <= '9'; })); + REQUIRE(stem.find(manifest.Version) == std::string::npos); + if (timeOnly) + { + REQUIRE(stem.substr(0, timestampStart) == ConvertToUTF8(expectedPrefix)); + REQUIRE(path.native().size() == directoryLength + 1 + expectedPrefix.size() + 11); + } + else + { + REQUIRE(stem.substr(0, timestampStart) == GetPathPart(manifest, '.', PathPartVersionStrategy::NeverIncludeVersion).u8string()); + } + } + else + { + REQUIRE(path.native().size() == 150); + REQUIRE(stem.find(manifest.Id + '.' + manifest.Version) == 0); + } + const auto& args = context.Get(); + REQUIRE(args.find("/mylog=\"" + path.u8string() + '"') != std::string::npos); + REQUIRE(args.find("/mycustom") != std::string::npos); + for (const auto& suffix : { + L"_013_Microsoft.NET.Sdk.Android.Manifest_6.0.100.31.0.101_preview.9.16_x64.msi", + L"_018_{106C9C71-2390-4214-9E7E-7BA47FFB1FAB}_010_dotnet_sdk_internal_10.0.401_servicing.26423.113_win_x64.msi" }) + { + auto derivedPath = path.parent_path() / (path.stem().native() + suffix + path.extension().native()); + REQUIRE(derivedPath.native().size() < MAX_PATH); + } +} + +TEST_CASE("ShellExecuteHandlerInstallerArgs_LogPathFallback", "[InstallFlow][workflow][InstallerLogPath]") +{ + const auto strategy = GENERATE(LogNameStrategy::Manifest, LogNameStrategy::Timestamp, LogNameStrategy::Guid, LogNameStrategy::ShortGuid); + const size_t directoryLength = GENERATE(137, 138, 246, 247, 300); + CAPTURE(strategy, directoryLength); + InstallerLogDirectoryOverride logDirectory{ directoryLength }; + TestUserSettings testSettings; + testSettings.Set(LogNameStrategy{ strategy }); + std::ostringstream installOutput; + TestContext context{ installOutput, std::cin }; + auto previousThreadGlobals = context.SetForCurrentThread(); + auto manifest = YamlParser::CreateFromPath(TestDataFile("InstallerArgTest_Inno_WithSwitches.yaml")); + manifest.Installers.at(0).Switches[InstallerSwitchType::Log] = "/before /myLog=\"\" /after"; + manifest.Installers.at(0).Switches[InstallerSwitchType::Custom] = "/myCustom /log=\"\""; + context.Add(manifest); + context.Add(manifest.Installers.at(0)); + context.Args.AddArg(Execution::Args::Type::InstallLocation, "MyDir"sv); + context.Override({ ShellExecuteInstallImpl, [](TestContext& context) + { + REQUIRE(context.Get().find("/myCustom") != std::string::npos); + context.Add(0); + } }); + context << GetInstallerArgs << ShellExecuteInstallImpl; + + REQUIRE_FALSE(context.IsTerminated()); + REQUIRE(context.Get() == 0); + const auto& args = context.Get(); + REQUIRE(args.find("") == std::string::npos); + REQUIRE(args.find("/mysilentwithprogress") != std::string::npos); + REQUIRE(args.find("/myCustom") != std::string::npos); + REQUIRE(args.find("/myinstalldir=\"MyDir\"") != std::string::npos); + REQUIRE(context.Contains(Data::LogPath)); + const auto& path = context.Get(); + const size_t stemLength = strategy == LogNameStrategy::Guid ? 36 : (strategy == LogNameStrategy::Timestamp ? 17 : 8); + const size_t configuredLength = directoryLength + 1 + stemLength + 4; + REQUIRE(path.native().size() == (configuredLength < MAX_PATH ? configuredLength : directoryLength + 1 + 12)); + REQUIRE(args.find("/before /myLog=\"" + path.u8string() + "\" /after") != std::string::npos); + REQUIRE(args.find("/myCustom /log=\"" + path.u8string() + '"') != std::string::npos); + REQUIRE(installOutput.str().empty()); +} + +TEST_CASE("ShellExecuteHandlerInstallerArgs_LogPathBypass", "[InstallFlow][workflow][InstallerLogPath]") +{ + InstallerLogDirectoryOverride logDirectory{ 300 }; + std::ostringstream installOutput; + TestContext context{ installOutput, std::cin }; + auto previousThreadGlobals = context.SetForCurrentThread(); + auto manifest = YamlParser::CreateFromPath(TestDataFile("InstallerArgTest_Inno_WithSwitches.yaml")); + auto installer = manifest.Installers.at(0); + const std::string explicitPath = "C:\\" + std::string(270, 'a') + ".log"; + bool hasExplicitPath = false; + bool overridden = false; + SECTION("ExplicitLog") + { + context.Args.AddArg(Execution::Args::Type::Log, explicitPath); + hasExplicitPath = true; + } + SECTION("Override") + { + context.Args.AddArg(Execution::Args::Type::Override, "/override"sv); + overridden = true; + } + SECTION("NoLogToken") + { + installer.Switches[InstallerSwitchType::Log] = "/fixedLog"; + } + context.Add(manifest); + context.Add(installer); + context << GetInstallerArgs; + + REQUIRE_FALSE(context.IsTerminated()); + REQUIRE(context.Contains(Data::LogPath) == hasExplicitPath); + const auto& args = context.Get(); + if (hasExplicitPath) + { + REQUIRE(context.Get().u8string() == explicitPath); + REQUIRE(args.find("/mylog=\"" + explicitPath + '"') != std::string::npos); + } + else if (overridden) + { + REQUIRE(args == "/override"); + } + else + { + REQUIRE(args.find("/fixedLog") != std::string::npos); + } + REQUIRE(installOutput.str().empty()); +} + TEST_CASE("InstallFlow_SearchAndInstall", "[InstallFlow][workflow]") { TestCommon::TempFile installResultPath("TestExeInstalled.txt"); diff --git a/src/AppInstallerCLITests/YamlManifest.cpp b/src/AppInstallerCLITests/YamlManifest.cpp index 1454c85d10..5cc29fb88b 100644 --- a/src/AppInstallerCLITests/YamlManifest.cpp +++ b/src/AppInstallerCLITests/YamlManifest.cpp @@ -1641,7 +1641,7 @@ TEST_CASE("PackageIdentifierAndVersionPathValidation", "[ManifestValidation]") RequireSingleError(errors, ManifestError::FieldEscapesDirectory, "PackageVersion", manifest.Version); } -TEST_CASE("ManifestGetPathPart", "[ManifestValidation]") +TEST_CASE("ManifestGetPathPart", "[ManifestValidation][InstallerLogPath]") { Manifest manifest; manifest.Id = "Foo.Bar"; @@ -1650,22 +1650,28 @@ TEST_CASE("ManifestGetPathPart", "[ManifestValidation]") // The common case must not alter the value, as the resulting paths are persisted. REQUIRE(GetPathPart(manifest) == std::filesystem::path{ L"Foo.Bar.1.0.0" }); REQUIRE(GetPathPart(manifest, '_') == std::filesystem::path{ L"Foo.Bar_1.0.0" }); + REQUIRE(GetPathPart(manifest, '.', PathPartVersionStrategy::AlwaysIncludeVersion) == std::filesystem::path{ L"Foo.Bar.1.0.0" }); + REQUIRE(GetPathPart(manifest, '.', PathPartVersionStrategy::NeverIncludeVersion) == std::filesystem::path{ L"Foo.Bar" }); + REQUIRE(GetPathPart(manifest, '_', PathPartVersionStrategy::NeverIncludeVersion) == std::filesystem::path{ L"Foo.Bar" }); // An unknown version is only dropped when the caller asks for it. manifest.Version = "Unknown"; REQUIRE(GetPathPart(manifest) == std::filesystem::path{ L"Foo.Bar.Unknown" }); REQUIRE(GetPathPart(manifest, '_') == std::filesystem::path{ L"Foo.Bar_Unknown" }); - REQUIRE(GetPathPart(manifest, '.', true) == std::filesystem::path{ L"Foo.Bar" }); - REQUIRE(GetPathPart(manifest, '_', true) == std::filesystem::path{ L"Foo.Bar" }); + REQUIRE(GetPathPart(manifest, '.', PathPartVersionStrategy::AlwaysIncludeVersion) == std::filesystem::path{ L"Foo.Bar.Unknown" }); + REQUIRE(GetPathPart(manifest, '.', PathPartVersionStrategy::DropUnknownVersion) == std::filesystem::path{ L"Foo.Bar" }); + REQUIRE(GetPathPart(manifest, '_', PathPartVersionStrategy::DropUnknownVersion) == std::filesystem::path{ L"Foo.Bar" }); + REQUIRE(GetPathPart(manifest, '.', PathPartVersionStrategy::NeverIncludeVersion) == std::filesystem::path{ L"Foo.Bar" }); - // A known version is kept regardless of the drop request. + // Dropping only unknown versions keeps known versions. manifest.Version = "1.0.0"; - REQUIRE(GetPathPart(manifest, '.', true) == std::filesystem::path{ L"Foo.Bar.1.0.0" }); + REQUIRE(GetPathPart(manifest, '.', PathPartVersionStrategy::DropUnknownVersion) == std::filesystem::path{ L"Foo.Bar.1.0.0" }); // Values that validation would have rejected are sanitized rather than used as given. manifest.Id = "a\\b"; manifest.Version = "c/d"; REQUIRE(GetPathPart(manifest) == std::filesystem::path{ L"a_b.c_d" }); + REQUIRE(GetPathPart(manifest, '.', PathPartVersionStrategy::NeverIncludeVersion) == std::filesystem::path{ L"a_b" }); manifest.Id = "Foo.Bar"; manifest.Version = "C:"; @@ -1684,6 +1690,9 @@ TEST_CASE("ManifestGetPathPart", "[ManifestValidation]") manifest.Id = id; REQUIRE_THROWS_HR(GetPathPart(manifest), APPINSTALLER_CLI_ERROR_INVALID_MANIFEST); } + + manifest.Id = "CON"; + REQUIRE_THROWS_HR(GetPathPart(manifest, '.', PathPartVersionStrategy::NeverIncludeVersion), APPINSTALLER_CLI_ERROR_INVALID_MANIFEST); } TEST_CASE("PortableFileTypeValidation", "[ManifestValidation]") diff --git a/src/AppInstallerCommonCore/Manifest/Manifest.cpp b/src/AppInstallerCommonCore/Manifest/Manifest.cpp index 31eb41b26c..ac9d5dd401 100644 --- a/src/AppInstallerCommonCore/Manifest/Manifest.cpp +++ b/src/AppInstallerCommonCore/Manifest/Manifest.cpp @@ -326,11 +326,26 @@ namespace AppInstaller::Manifest return result; } - std::filesystem::path GetPathPart(const Manifest& manifest, char separator, bool dropUnknownVersion) + std::filesystem::path GetPathPart(const Manifest& manifest, char separator, PathPartVersionStrategy versionStrategy) { std::string value = manifest.Id; - if (!dropUnknownVersion || !Utility::Version{ manifest.Version }.IsUnknown()) + bool includeVersion = false; + switch (versionStrategy) + { + case PathPartVersionStrategy::AlwaysIncludeVersion: + includeVersion = true; + break; + case PathPartVersionStrategy::DropUnknownVersion: + includeVersion = !Utility::Version{ manifest.Version }.IsUnknown(); + break; + case PathPartVersionStrategy::NeverIncludeVersion: + break; + default: + THROW_HR(E_INVALIDARG); + } + + if (includeVersion) { value += separator; value += manifest.Version; diff --git a/src/AppInstallerCommonCore/Public/winget/Manifest.h b/src/AppInstallerCommonCore/Public/winget/Manifest.h index 2eb107d28d..63d8398576 100644 --- a/src/AppInstallerCommonCore/Public/winget/Manifest.h +++ b/src/AppInstallerCommonCore/Public/winget/Manifest.h @@ -81,10 +81,17 @@ namespace AppInstaller::Manifest std::function extractStringFromAppsAndFeaturesEntry = {}) const; }; + enum class PathPartVersionStrategy + { + AlwaysIncludeVersion, + DropUnknownVersion, + NeverIncludeVersion, + }; + // Creates a file system path part for the manifest in the form ``, - // or just `` when the version is unknown and the drop is requested. + // or just `` according to the version strategy. // Manifest validation rejects values that are not safe to use in a path, but the values can also come from // sources that do not go through it (for instance, installed package data), so they are sanitized here as // well. Throws if the result would point outside of the directory that it is used in. - std::filesystem::path GetPathPart(const Manifest& manifest, char separator = '.', bool dropUnknownVersion = false); + std::filesystem::path GetPathPart(const Manifest& manifest, char separator = '.', PathPartVersionStrategy versionStrategy = PathPartVersionStrategy::AlwaysIncludeVersion); }