diff --git a/.pipelines/ESRPSigning_core.json b/.pipelines/ESRPSigning_core.json index 33631d6569..85eb370159 100644 --- a/.pipelines/ESRPSigning_core.json +++ b/.pipelines/ESRPSigning_core.json @@ -65,6 +65,8 @@ "FancyZonesCLI.exe", "FancyZonesCLI.dll", + "CliShim\\PowerToys.CliShim.exe", + "PowerToys.GcodePreviewHandler.dll", "PowerToys.GcodePreviewHandler.exe", "PowerToys.GcodePreviewHandlerCpp.dll", diff --git a/PowerToys.slnx b/PowerToys.slnx index 2d375dd426..ec28f3887e 100644 --- a/PowerToys.slnx +++ b/PowerToys.slnx @@ -1260,5 +1260,7 @@ + + diff --git a/doc/devdocs/cli-conventions.md b/doc/devdocs/cli-conventions.md index a5bc4ec04b..349a611c8f 100644 --- a/doc/devdocs/cli-conventions.md +++ b/doc/devdocs/cli-conventions.md @@ -2,6 +2,32 @@ This document describes the conventions for implementing command-line interfaces (CLI) in PowerToys modules. +## PATH-Visible Command Naming and Location + +- Name module CLI command shims `PowerToys..CLI.exe` (for example, `PowerToys.ImageResizer.CLI.exe`). +- Install these shims in the `bin` subfolder of the PowerToys installation directory, which the installer adds to `PATH`. + +Every command is the same `PowerToys.CliShim.exe` payload (`tools/CliShim/`) installed under a different name. The shim resolves which CLI to launch from its own file name, forwards the raw argument tail unchanged, shares the caller's console, and returns the CLI's exit code. The CLI runs in a job object owned by the shim, so killing the shim kills the CLI with it; processes the CLI itself starts (the Settings window, for example) break away and survive. + +On a per-machine install the `bin` folder is created with a protected DACL (`MachinePathFolderSddl` in `installer/PowerToysSetupVNext/Common.wxi`) so that a custom installation root cannot leave a machine-`PATH` folder writable by standard users. Author that `` on the same component as the folder's `` `PATH` entry, so the two cannot drift apart. + +### Adding a new shim + +1. Add a `` item to `tools/CliShim/CliShimManifest.props` with the command name and the target's path relative to `bin`. Write that path with `/` separators, and against the *installed* layout (see [Signing and Deployment](#signing-and-deployment)) - which is where the CLI ends up, not where it is built from. +2. Add the matching `` and `` to `installer/PowerToysSetupVNext/CliShims.wxs`, using the command name as the `File/@Name`. + +`CliShim.vcxproj` fails the build if the command names in those two drift apart, `build-installer.ps1` fails the build if a `RelativeTarget` does not resolve to a real executable, and `CliShim.UnitTests` generates its expectations from the same manifest, so there is no third list to update. + +### Shim exit codes + +The shim returns the target CLI's exit code unchanged. It substitutes one of its own codes only when the CLI never ran, using values outside the range the CLIs use themselves: + +| Code | Meaning | +| --- | --- | +| `9009` | No CLI is mapped to the invoked command name (matches `cmd.exe`'s "command not found"). | +| `9010` | The mapped target executable is missing from the installation. | +| `9011` | The shim could not start the target, including when it cannot resolve its own path. | + ## Library Use the **System.CommandLine** library for CLI argument parsing. This is already defined in `Directory.Packages.props`: @@ -89,5 +115,5 @@ Reference implementations: - CLI executables are signed automatically in CI/CD. - **New CLI tools**: Add your executable and dll to `.pipelines/ESRPSigning_core.json` in the signing list. -- CLI executables are deployed alongside their parent module (e.g., `C:\Program Files\PowerToys\modules\[ModuleName]\`). +- CLI executables are deployed either to the installation root (e.g., `C:\Program Files\PowerToys\FancyZonesCLI.exe`) or, for WinUI 3 modules, next to their module in `WinUI3Apps\` (e.g., `C:\Program Files\PowerToys\WinUI3Apps\PowerToys.ImageResizerCLI.exe`). PATH-visible shims are deployed to `C:\Program Files\PowerToys\bin\`, and a shim's `RelativeTarget` is resolved from that `bin` folder against the *installed* layout - not against the source tree. - Use self-contained deployment (import `Common.SelfContained.props`). diff --git a/installer/PowerToysSetupVNext/CliShims.wxs b/installer/PowerToysSetupVNext/CliShims.wxs new file mode 100644 index 0000000000..4c39ef2df1 --- /dev/null +++ b/installer/PowerToysSetupVNext/CliShims.wxs @@ -0,0 +1,85 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/installer/PowerToysSetupVNext/Common.wxi b/installer/PowerToysSetupVNext/Common.wxi index 21855a7936..f321875e40 100644 --- a/installer/PowerToysSetupVNext/Common.wxi +++ b/installer/PowerToysSetupVNext/Common.wxi @@ -54,5 +54,21 @@ + + + diff --git a/installer/PowerToysSetupVNext/DscResources.wxs b/installer/PowerToysSetupVNext/DscResources.wxs index 0566b13532..ff0b5b7cfe 100644 --- a/installer/PowerToysSetupVNext/DscResources.wxs +++ b/installer/PowerToysSetupVNext/DscResources.wxs @@ -28,7 +28,7 @@ - + diff --git a/installer/PowerToysSetupVNext/PowerToysInstallerVNext.wixproj b/installer/PowerToysSetupVNext/PowerToysInstallerVNext.wixproj index 45cfbdcbaa..4a5c40aca8 100644 --- a/installer/PowerToysSetupVNext/PowerToysInstallerVNext.wixproj +++ b/installer/PowerToysSetupVNext/PowerToysInstallerVNext.wixproj @@ -136,6 +136,7 @@ call powershell.exe -NonInteractive -executionpolicy Unrestricted -File $(MSBuil + diff --git a/installer/PowerToysSetupVNext/Product.wxs b/installer/PowerToysSetupVNext/Product.wxs index 8651a7d83d..0c2be9d701 100644 --- a/installer/PowerToysSetupVNext/Product.wxs +++ b/installer/PowerToysSetupVNext/Product.wxs @@ -67,6 +67,7 @@ + @@ -299,6 +300,7 @@ + diff --git a/tools/CliShim.UnitTests/CliShim.UnitTests.vcxproj b/tools/CliShim.UnitTests/CliShim.UnitTests.vcxproj new file mode 100644 index 0000000000..a5aa565306 --- /dev/null +++ b/tools/CliShim.UnitTests/CliShim.UnitTests.vcxproj @@ -0,0 +1,59 @@ + + + + + {D4B0ED68-867D-46B4-A03F-EC52B9FBEBE5} + Win32Proj + PowerToysCliShimUnitTests + CliShim.UnitTests + NativeUnitTestProject + false + $(RepoRoot)$(Platform)\$(Configuration)\tests\CliShim\ + + + DynamicLibrary + + + + + + false + false + + + + NotUsing + $(IntDir);$(MSBuildProjectDirectory)\..\CliShim;$(VCInstallDir)UnitTest\include;%(AdditionalIncludeDirectories) + + + $(VCInstallDir)UnitTest\lib;%(AdditionalLibraryDirectories) + + + + + + + + + + + + {8A7FB7FA-65EA-4004-BA73-1B237435A57B} + false + false + + + + + + <_CliShimUnderTest>$(RepoRoot)$(Platform)\$(Configuration)\CliShim\PowerToys.CliShim.exe + + + + + diff --git a/tools/CliShim.UnitTests/CommandLineTests.cpp b/tools/CliShim.UnitTests/CommandLineTests.cpp new file mode 100644 index 0000000000..b959652916 --- /dev/null +++ b/tools/CliShim.UnitTests/CommandLineTests.cpp @@ -0,0 +1,73 @@ +// Copyright (c) Microsoft Corporation +// The Microsoft Corporation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +#include + +#include + +#include "CommandLine.h" + +using namespace Microsoft::VisualStudio::CppUnitTestFramework; + +namespace CliShimUnitTests +{ + TEST_CLASS(CommandLineTests) + { + public: + TEST_METHOD(StripArgumentZero_ReturnsForwardedTail) + { + struct Case + { + const wchar_t* commandLine; + const wchar_t* expected; + }; + + const Case cases[] = { + { L"PowerToys.FancyZones.CLI arg", L"arg" }, + { L"PowerToys.FancyZones.CLI a b c", L"a b c" }, + { L"PowerToys.FancyZones.CLI", L"" }, + { L"PowerToys.FileLocksmith.CLI", L"" }, + + { LR"("C:\Program Files\PowerToys\bin\PowerToys.FancyZones.CLI.exe" arg)", L"arg" }, + { LR"("C:\Program Files\PowerToys\bin\PowerToys.FancyZones.CLI.exe")", L"" }, + + // Quotes only toggle, so a quoted argv[0] does not end at the closing quote: an + // argument glued to it is part of the program name. CommandLineToArgvW would + // forward `--help` here; the CRT - and so every target CLI - sees no arguments. + { LR"("C:\bin\PowerToys.FancyZones.CLI.exe"--help)", L"" }, + + // Partially quoted program names - the batch idiom of quoting only the variable, + // `"%ProgramFiles%"\PowerToys\bin\...`, which cmd.exe passes through verbatim - are + // resolved exactly as the CRT resolves them, so the target sees the same tail it + // would have seen had the caller invoked it directly rather than through the shim. + { LR"("C:\Program Files"\PowerToys\bin\PowerToys.FancyZones.CLI.exe arg)", L"arg" }, + { LR"(C:\Program" Files"\PowerToys\bin\PowerToys.FancyZones.CLI.exe arg)", L"arg" }, + { LR"("C:\Program Files"\PowerToys\bin\PowerToys.FancyZones.CLI.exe)", L"" }, + + { LR"("C:\bin\PowerToys.FancyZones.CLI.exe" "a b")", LR"("a b")" }, + { LR"(PowerToys.FancyZones.CLI --path "C:\a b\c.png")", LR"(--path "C:\a b\c.png")" }, + + { L"PowerToys.FancyZones.CLI\targ", L"arg" }, + { L"PowerToys.FancyZones.CLI \t arg", L"arg" }, + + // Non-shell CreateProcessW callers can prepend whitespace; argv[0] must not leak. + { L" PowerToys.FancyZones.CLI arg", L"arg" }, + { L" PowerToys.FancyZones.CLI", L"" }, + { LR"( "C:\bin\PowerToys.FancyZones.CLI.exe" arg)", L"arg" }, + + { L"", L"" }, + + // An unterminated argv[0] quote consumes the remaining command line. + { LR"("C:\Program Files\app)", L"" }, + }; + + for (const Case& testCase : cases) + { + const std::wstring actual = CommandLine::StripArgumentZero(testCase.commandLine); + const std::wstring message = std::wstring(L"input: <") + testCase.commandLine + L">"; + Assert::AreEqual(std::wstring(testCase.expected), actual, message.c_str()); + } + } + }; +} diff --git a/tools/CliShim.UnitTests/LauncherIntegrationTests.cpp b/tools/CliShim.UnitTests/LauncherIntegrationTests.cpp new file mode 100644 index 0000000000..55af5b2961 --- /dev/null +++ b/tools/CliShim.UnitTests/LauncherIntegrationTests.cpp @@ -0,0 +1,631 @@ +// Copyright (c) Microsoft Corporation +// The Microsoft Corporation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +#define WIN32_LEAN_AND_MEAN +#include + +#include + +#include +#include +#include +#include +#include +#include +#include +#include + +#include + +using namespace Microsoft::VisualStudio::CppUnitTestFramework; + +extern "C" IMAGE_DOS_HEADER __ImageBase; + +namespace +{ + constexpr DWORD ExitCommandNotMapped = 9009; + constexpr DWORD ExitTargetNotFound = 9010; + constexpr DWORD ForwardedExitCode = 37; + + // How often the process-tree helpers re-scan while waiting for a process to appear. + constexpr DWORD ProcessPollIntervalMilliseconds = 50; + + // How many times a temporary directory removal is retried before it is given up on. + constexpr int CleanupAttempts = 20; + + // Only used to unblock the test run if the shim ever hangs; the wait result is asserted. + constexpr DWORD TimeoutTerminationExitCode = 0xFFFFFFFF; + + struct ShimMapping + { + const wchar_t* command; + const wchar_t* relativeTarget; + }; + + // Generated from CliShimManifest.props, the same table the shim itself is built from, so a + // newly mapped command is covered here automatically instead of by hand. + constexpr ShimMapping ExpectedMappings[] = { +#include "CliShimTargets.g.inc" + }; + + constexpr const wchar_t* RejectedLegacyCommands[] = { + L"fancyzones", + L"imageresizer", + L"filelocksmith", + L"powerdisplay", + L"fancyzonescli", + L"imageresizercli", + L"filelocksmithcli", + L"powerdisplaycli", + }; + + class UniqueHandle + { + public: + UniqueHandle() = default; + + explicit UniqueHandle(HANDLE value) noexcept : + handle{ value } + { + } + + UniqueHandle(const UniqueHandle&) = delete; + UniqueHandle& operator=(const UniqueHandle&) = delete; + + UniqueHandle(UniqueHandle&& other) noexcept : + handle{ std::exchange(other.handle, nullptr) } + { + } + + UniqueHandle& operator=(UniqueHandle&& other) noexcept + { + if (this != &other) + { + Reset(); + handle = std::exchange(other.handle, nullptr); + } + + return *this; + } + + ~UniqueHandle() + { + Reset(); + } + + void Reset() noexcept + { + if (handle != nullptr && handle != INVALID_HANDLE_VALUE) + { + CloseHandle(handle); + } + + handle = nullptr; + } + + HANDLE Get() const noexcept + { + return handle; + } + + private: + HANDLE handle = nullptr; + }; + + std::filesystem::path GetTestBinaryDirectory() + { + wchar_t modulePath[MAX_PATH]{}; + const DWORD length = GetModuleFileNameW( + reinterpret_cast(&__ImageBase), + modulePath, + ARRAYSIZE(modulePath)); + + Assert::IsTrue(length > 0 && length < ARRAYSIZE(modulePath), L"Could not locate the test module."); + return std::filesystem::path{ modulePath }.parent_path(); + } + + std::filesystem::path GetShimUnderTest() + { + return GetTestBinaryDirectory() / L"PowerToys.CliShim.exe"; + } + + std::filesystem::path GetSystemDirectoryPath() + { + wchar_t systemDirectory[MAX_PATH]{}; + const UINT length = GetSystemDirectoryW(systemDirectory, ARRAYSIZE(systemDirectory)); + + Assert::IsTrue(length > 0 && length < ARRAYSIZE(systemDirectory), L"Could not locate the system directory."); + return systemDirectory; + } + + std::filesystem::path GetSystemCommandInterpreter() + { + return GetSystemDirectoryPath() / L"cmd.exe"; + } + + std::filesystem::path CreateTemporaryDirectory() + { + wchar_t temporaryRoot[MAX_PATH]{}; + const DWORD rootLength = GetTempPathW(ARRAYSIZE(temporaryRoot), temporaryRoot); + Assert::IsTrue(rootLength > 0 && rootLength < ARRAYSIZE(temporaryRoot), L"Could not locate the temporary directory."); + + wchar_t temporaryFile[MAX_PATH]{}; + Assert::IsTrue( + GetTempFileNameW(temporaryRoot, L"PTS", 0, temporaryFile) != 0, + L"Could not reserve a temporary path."); + Assert::IsTrue(DeleteFileW(temporaryFile), L"Could not remove the temporary placeholder file."); + Assert::IsTrue(CreateDirectoryW(temporaryFile, nullptr), L"Could not create the temporary directory."); + + return temporaryFile; + } + + class TemporaryDirectory + { + public: + TemporaryDirectory() : + path{ CreateTemporaryDirectory() } + { + } + + ~TemporaryDirectory() + { + // A process that has just been terminated can still have its image mapped for a moment, + // which keeps the copied executable open, so give the removal a few attempts. + for (int attempt = 0; attempt < CleanupAttempts; ++attempt) + { + std::error_code error; + std::filesystem::remove_all(path, error); + if (!error) + { + return; + } + + Sleep(ProcessPollIntervalMilliseconds); + } + } + + const std::filesystem::path& GetPath() const noexcept + { + return path; + } + + private: + std::filesystem::path path; + }; + + void CopyExecutable(const std::filesystem::path& source, const std::filesystem::path& destination) + { + std::error_code error; + std::filesystem::create_directories(destination.parent_path(), error); + Assert::AreEqual(0, error.value(), L"Could not create the destination directory."); + + std::filesystem::copy_file( + source, + destination, + std::filesystem::copy_options::overwrite_existing, + error); + Assert::AreEqual(0, error.value(), L"Could not copy the executable."); + } + + UniqueHandle OpenNul(const DWORD access) + { + SECURITY_ATTRIBUTES attributes{}; + attributes.nLength = sizeof(attributes); + attributes.bInheritHandle = TRUE; + + UniqueHandle handle{ CreateFileW( + L"NUL", + access, + FILE_SHARE_READ | FILE_SHARE_WRITE, + &attributes, + OPEN_EXISTING, + 0, + nullptr) }; + Assert::IsTrue(handle.Get() != INVALID_HANDLE_VALUE, L"Could not open NUL."); + + return handle; + } + + // Toolhelp is the whole story for observing the shim's process tree. A child has to be + // selected by image name rather than by "the first child", because the shim is also given a + // console host child; and a process that outlived its parent has to be found by image name + // alone, because by then its parent process id refers to a process that no longer exists. + UniqueHandle OpenProcessByImageName( + const std::wstring& imageName, + const std::optional parentProcessId, + const DWORD access) + { + UniqueHandle snapshot{ CreateToolhelp32Snapshot(TH32CS_SNAPPROCESS, 0) }; + if (snapshot.Get() == INVALID_HANDLE_VALUE) + { + return {}; + } + + PROCESSENTRY32W entry{}; + entry.dwSize = sizeof(entry); + + for (BOOL found = Process32FirstW(snapshot.Get(), &entry); found; found = Process32NextW(snapshot.Get(), &entry)) + { + if (parentProcessId.has_value() && entry.th32ParentProcessID != *parentProcessId) + { + continue; + } + + if (CompareStringOrdinal(entry.szExeFile, -1, imageName.c_str(), -1, TRUE) != CSTR_EQUAL) + { + continue; + } + + UniqueHandle process{ OpenProcess(access, FALSE, entry.th32ProcessID) }; + if (process.Get() != nullptr) + { + return process; + } + } + + return {}; + } + + UniqueHandle WaitForProcessByImageName( + const std::wstring& imageName, + const std::optional parentProcessId, + const DWORD access, + const DWORD timeoutMilliseconds) + { + for (DWORD elapsed = 0;; elapsed += ProcessPollIntervalMilliseconds) + { + UniqueHandle process = OpenProcessByImageName(imageName, parentProcessId, access); + if (process.Get() != nullptr || elapsed >= timeoutMilliseconds) + { + return process; + } + + Sleep(ProcessPollIntervalMilliseconds); + } + } + + struct LaunchedProcess + { + UniqueHandle process; + DWORD processId = 0; + }; + + // CppUnitTest assertions throw, so anything the job-object tests start has to be torn down by + // a destructor rather than by a line at the end of the test: both tests deliberately start a + // process that never exits on its own, and a failing assertion would otherwise leave it + // running - and its executable locked - for as long as the agent lives. + class ProcessKiller + { + public: + explicit ProcessKiller(UniqueHandle process) noexcept : + handle{ std::move(process) } + { + } + + ProcessKiller(const ProcessKiller&) = delete; + ProcessKiller& operator=(const ProcessKiller&) = delete; + + ~ProcessKiller() + { + if (handle.Get() != nullptr) + { + TerminateProcess(handle.Get(), 0); + WaitForSingleObject(handle.Get(), 5'000); + } + } + + HANDLE Get() const noexcept + { + return handle.Get(); + } + + private: + UniqueHandle handle; + }; + + // Starts the executable without waiting for it. When either standard handle is supplied the + // child is started with inherited handles so the shim's own redirection behaviour is + // exercised; the handles left null keep whatever the test host is using. + LaunchedProcess StartProcess( + const std::filesystem::path& executable, + const std::wstring& arguments, + HANDLE standardInput, + HANDLE standardOutput) + { + std::wstring commandLine = L"\"" + executable.wstring() + L"\""; + if (!arguments.empty()) + { + commandLine.push_back(L' '); + commandLine.append(arguments); + } + + const bool redirect = standardInput != nullptr || standardOutput != nullptr; + + STARTUPINFOW startupInfo{}; + startupInfo.cb = sizeof(startupInfo); + if (redirect) + { + startupInfo.dwFlags = STARTF_USESTDHANDLES; + startupInfo.hStdInput = standardInput != nullptr ? standardInput : GetStdHandle(STD_INPUT_HANDLE); + startupInfo.hStdOutput = standardOutput != nullptr ? standardOutput : GetStdHandle(STD_OUTPUT_HANDLE); + startupInfo.hStdError = standardOutput != nullptr ? standardOutput : GetStdHandle(STD_ERROR_HANDLE); + } + + PROCESS_INFORMATION processInfo{}; + + if (!CreateProcessW( + executable.c_str(), + commandLine.data(), + nullptr, + nullptr, + redirect ? TRUE : FALSE, + CREATE_NO_WINDOW, + nullptr, + nullptr, + &startupInfo, + &processInfo)) + { + const std::wstring message = L"CreateProcessW failed with error " + std::to_wstring(GetLastError()) + L"."; + Assert::Fail(message.c_str()); + } + + CloseHandle(processInfo.hThread); + return { UniqueHandle{ processInfo.hProcess }, processInfo.dwProcessId }; + } + + // Runs the executable to completion and returns its exit code. + DWORD RunProcess( + const std::filesystem::path& executable, + const std::wstring& arguments, + HANDLE standardInput, + HANDLE standardOutput) + { + const LaunchedProcess launched = StartProcess(executable, arguments, standardInput, standardOutput); + + const DWORD waitResult = WaitForSingleObject(launched.process.Get(), 30'000); + if (waitResult != WAIT_OBJECT_0) + { + TerminateProcess(launched.process.Get(), TimeoutTerminationExitCode); + WaitForSingleObject(launched.process.Get(), 5'000); + } + + DWORD exitCode = 0; + const BOOL gotExitCode = GetExitCodeProcess(launched.process.Get(), &exitCode); + + Assert::AreEqual(static_cast(WAIT_OBJECT_0), waitResult, L"The shim process did not exit."); + Assert::IsTrue(gotExitCode, L"Could not read the shim process exit code."); + return exitCode; + } + + DWORD RunAndGetExitCode(const std::filesystem::path& executable, const std::wstring& arguments = {}) + { + return RunProcess(executable, arguments, nullptr, nullptr); + } + + std::string RunAndCaptureStandardOutput( + const std::filesystem::path& executable, + const std::wstring& arguments, + const std::filesystem::path& capturePath, + DWORD& exitCode) + { + SECURITY_ATTRIBUTES attributes{}; + attributes.nLength = sizeof(attributes); + attributes.bInheritHandle = TRUE; + + UniqueHandle output{ CreateFileW( + capturePath.c_str(), + GENERIC_WRITE, + FILE_SHARE_READ | FILE_SHARE_WRITE, + &attributes, + CREATE_ALWAYS, + FILE_ATTRIBUTE_NORMAL, + nullptr) }; + Assert::IsTrue(output.Get() != INVALID_HANDLE_VALUE, L"Could not create the output capture file."); + + const UniqueHandle input = OpenNul(GENERIC_READ); + + exitCode = RunProcess(executable, arguments, input.Get(), output.Get()); + output.Reset(); + + std::ifstream stream{ capturePath, std::ios::binary }; + Assert::IsTrue(stream.is_open(), L"Could not read the captured output."); + + std::string captured{ std::istreambuf_iterator{ stream }, std::istreambuf_iterator{} }; + while (!captured.empty() && (captured.back() == '\r' || captured.back() == '\n')) + { + captured.pop_back(); + } + + return captured; + } + + // The forwarded payload is deliberately ASCII-only so it round-trips through cmd.exe's ECHO. + std::string NarrowAscii(std::wstring_view text) + { + std::string narrow; + narrow.reserve(text.size()); + + for (const wchar_t character : text) + { + narrow.push_back(static_cast(character)); + } + + return narrow; + } +} + +namespace CliShimUnitTests +{ + TEST_CLASS(LauncherIntegrationTests) + { + public: + TEST_METHOD(AllMappedCommandsLaunchExpectedRelativeTargets) + { + TemporaryDirectory installation; + const std::filesystem::path binDirectory = installation.GetPath() / L"bin"; + const std::filesystem::path targetSource = GetSystemCommandInterpreter(); + + for (const ShimMapping& mapping : ExpectedMappings) + { + const std::filesystem::path shimPath = binDirectory / (std::wstring{ mapping.command } + L".exe"); + const std::filesystem::path targetPath = (binDirectory / mapping.relativeTarget).lexically_normal(); + + CopyExecutable(GetShimUnderTest(), shimPath); + CopyExecutable(targetSource, targetPath); + + const DWORD exitCode = RunAndGetExitCode(shimPath, L"/d /c exit 37"); + const std::wstring message = L"Command failed: " + std::wstring{ mapping.command }; + Assert::AreEqual(ForwardedExitCode, exitCode, message.c_str()); + } + } + + // cmd.exe's ECHO writes the remainder of its command line out verbatim, so this pins down + // that the caller's quoting and spacing survive the hop through the shim unchanged. + TEST_METHOD(ArgumentTailIsForwardedVerbatim) + { + constexpr const wchar_t* argumentTail = LR"(--path "C:\a b\c.png" --size 100 -q)"; + + TemporaryDirectory installation; + const std::filesystem::path shimPath = installation.GetPath() / L"bin" / L"PowerToys.FancyZones.CLI.exe"; + + CopyExecutable(GetShimUnderTest(), shimPath); + CopyExecutable(GetSystemCommandInterpreter(), installation.GetPath() / L"FancyZonesCLI.exe"); + + DWORD exitCode = MAXDWORD; + const std::string captured = RunAndCaptureStandardOutput( + shimPath, + std::wstring{ L"/d /c echo " } + argumentTail, + installation.GetPath() / L"captured-output.txt", + exitCode); + + Assert::AreEqual(static_cast(0), exitCode, L"The forwarded command did not succeed."); + Assert::AreEqual(NarrowAscii(argumentTail), captured, L"The argument tail was not forwarded verbatim."); + } + + TEST_METHOD(UnknownCommandReturnsCommandNotMapped) + { + TemporaryDirectory installation; + const std::filesystem::path shimPath = installation.GetPath() / L"bin" / L"unknown.exe"; + + CopyExecutable(GetShimUnderTest(), shimPath); + + Assert::AreEqual(ExitCommandNotMapped, RunAndGetExitCode(shimPath)); + } + + TEST_METHOD(LegacyCommandsReturnCommandNotMapped) + { + TemporaryDirectory installation; + const std::filesystem::path binDirectory = installation.GetPath() / L"bin"; + + for (const wchar_t* command : RejectedLegacyCommands) + { + const std::filesystem::path shimPath = binDirectory / (std::wstring{ command } + L".exe"); + CopyExecutable(GetShimUnderTest(), shimPath); + + const std::wstring message = L"Legacy command was unexpectedly mapped: " + std::wstring{ command }; + Assert::AreEqual(ExitCommandNotMapped, RunAndGetExitCode(shimPath), message.c_str()); + } + } + + // A missing target must not be reported with an exit code the target CLI itself could + // return, so callers can tell the two apart. + TEST_METHOD(MissingTargetReturnsTargetNotFound) + { + TemporaryDirectory installation; + const std::filesystem::path shimPath = installation.GetPath() / L"bin" / L"PowerToys.FancyZones.CLI.exe"; + + CopyExecutable(GetShimUnderTest(), shimPath); + + Assert::AreEqual(ExitTargetNotFound, RunAndGetExitCode(shimPath)); + } + + // The shim is the only handle a caller holds on the CLI, so a single-process kill of the + // shim must not leave the CLI running - PowerToys.FileLocksmith.CLI --wait polls forever + // and prints nothing, which would make the orphan invisible and permanent. + TEST_METHOD(TerminatingTheShimTerminatesTheTargetCli) + { + TemporaryDirectory installation; + const std::filesystem::path shimPath = installation.GetPath() / L"bin" / L"PowerToys.FancyZones.CLI.exe"; + + CopyExecutable(GetShimUnderTest(), shimPath); + CopyExecutable(GetSystemCommandInterpreter(), installation.GetPath() / L"FancyZonesCLI.exe"); + + // PAUSE blocks inside the target itself, on a pipe that is never written, so the + // target needs no console and starts no worker that could outlive the test. + SECURITY_ATTRIBUTES attributes{}; + attributes.nLength = sizeof(attributes); + attributes.bInheritHandle = TRUE; + + HANDLE readEnd = nullptr; + HANDLE writeEnd = nullptr; + Assert::IsTrue(CreatePipe(&readEnd, &writeEnd, &attributes, 0) != FALSE, L"Could not create the standard input pipe."); + + const UniqueHandle standardInput{ readEnd }; + const UniqueHandle keepPipeOpen{ writeEnd }; + const UniqueHandle standardOutput = OpenNul(GENERIC_WRITE); + + LaunchedProcess launchedShim = StartProcess(shimPath, L"/d /c pause", standardInput.Get(), standardOutput.Get()); + const DWORD shimProcessId = launchedShim.processId; + const ProcessKiller shim{ std::move(launchedShim.process) }; + + const ProcessKiller target{ WaitForProcessByImageName( + L"FancyZonesCLI.exe", + shimProcessId, + SYNCHRONIZE | PROCESS_TERMINATE, + 10'000) }; + Assert::IsTrue(target.Get() != nullptr, L"The shim did not start the target CLI."); + Assert::AreEqual( + static_cast(WAIT_TIMEOUT), + WaitForSingleObject(target.Get(), 200), + L"The target CLI exited before the shim was terminated."); + + // taskkill without /T, Process.Kill() without entireProcessTree, a script's own + // timeout, stopping the debugger: all of these reach the shim and nothing else. + Assert::IsTrue(TerminateProcess(shim.Get(), 1) != FALSE, L"Could not terminate the shim."); + + Assert::AreEqual( + static_cast(WAIT_OBJECT_0), + WaitForSingleObject(target.Get(), 10'000), + L"The target CLI outlived the shim."); + + WaitForSingleObject(shim.Get(), 5'000); + } + + // The mirror of the test above: JOB_OBJECT_LIMIT_SILENT_BREAKAWAY_OK keeps the CLI's own + // children out of the job, so PowerToys.FancyZones.CLI open-settings can start a + // long-lived PowerToys.exe and return without that window dying with the shim. + TEST_METHOD(ProcessesStartedByTheTargetCliSurviveTheShim) + { + TemporaryDirectory installation; + const std::filesystem::path shimPath = installation.GetPath() / L"bin" / L"PowerToys.FancyZones.CLI.exe"; + + // Once the CLI that started it has exited, the survivor's parent process id refers to + // a process that no longer exists, so it can only be found by image name. Deriving + // that name from the temporary directory keeps concurrent test runs from colliding. + const std::wstring survivorName = installation.GetPath().stem().wstring() + L"-survivor.exe"; + const std::filesystem::path survivorPath = installation.GetPath() / survivorName; + + CopyExecutable(GetShimUnderTest(), shimPath); + CopyExecutable(GetSystemCommandInterpreter(), installation.GetPath() / L"FancyZonesCLI.exe"); + CopyExecutable(GetSystemDirectoryPath() / L"PING.EXE", survivorPath); + + // START returns as soon as the survivor is running, so the CLI - and with it the shim - + // exits while the survivor is still alive. + const std::wstring arguments = LR"(/d /c start "" /b ")" + survivorPath.wstring() + LR"(" -n 30 127.0.0.1 > nul)"; + Assert::AreEqual(static_cast(0), RunAndGetExitCode(shimPath, arguments), L"The target CLI did not succeed."); + + const ProcessKiller survivor{ WaitForProcessByImageName( + survivorName, + std::nullopt, + SYNCHRONIZE | PROCESS_TERMINATE, + 10'000) }; + Assert::IsTrue(survivor.Get() != nullptr, L"The process started by the target CLI was killed with the shim."); + + Assert::AreEqual( + static_cast(WAIT_TIMEOUT), + WaitForSingleObject(survivor.Get(), 1'000), + L"The process started by the target CLI did not outlive the shim."); + } + }; +} diff --git a/tools/CliShim/CliShim.rc b/tools/CliShim/CliShim.rc new file mode 100644 index 0000000000..f0c84bb1e8 --- /dev/null +++ b/tools/CliShim/CliShim.rc @@ -0,0 +1,40 @@ +// Copyright (c) Microsoft Corporation +// The Microsoft Corporation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +#include + +#include "../../src/common/version/version.h" + +1 VERSIONINFO +FILEVERSION FILE_VERSION +PRODUCTVERSION FILE_VERSION +FILEFLAGSMASK VS_FFI_FILEFLAGSMASK +#ifdef _DEBUG +FILEFLAGS VS_FF_DEBUG +#else +FILEFLAGS 0x0L +#endif +FILEOS VOS_NT_WINDOWS32 +FILETYPE VFT_APP +FILESUBTYPE VFT2_UNKNOWN +BEGIN + BLOCK "StringFileInfo" + BEGIN + BLOCK "040904b0" + BEGIN + VALUE "CompanyName", "Microsoft Corporation" + VALUE "FileDescription", "PowerToys CLI command shim" + VALUE "FileVersion", FILE_VERSION_STRING + VALUE "InternalName", "PowerToys.CliShim" + VALUE "LegalCopyright", "Copyright (C) Microsoft Corporation. All rights reserved." + VALUE "OriginalFilename", "PowerToys.CliShim.exe" + VALUE "ProductName", "PowerToys" + VALUE "ProductVersion", FILE_VERSION_STRING + END + END + BLOCK "VarFileInfo" + BEGIN + VALUE "Translation", 0x409, 1200 + END +END diff --git a/tools/CliShim/CliShim.vcxproj b/tools/CliShim/CliShim.vcxproj new file mode 100644 index 0000000000..c8e2eb63bf --- /dev/null +++ b/tools/CliShim/CliShim.vcxproj @@ -0,0 +1,83 @@ + + + + PackageReference + native,Version=v0.0 + Windows + $(WindowsTargetPlatformVersion) + + + + + + + {8A7FB7FA-65EA-4004-BA73-1B237435A57B} + Win32Proj + PowerToysCliShim + PowerToys.CliShim + false + $(RepoRoot)$(Platform)\$(Configuration)\CliShim\ + PowerToys.CliShim + + + Application + + + + + + false + false + + + + NotUsing + $(IntDir);%(AdditionalIncludeDirectories) + + + Console + + + + + + + + + + + + {CC6E41AC-8174-4E8A-8D22-85DD7F4851DF} + false + false + + + + + + + + + <_CliShimWxsPath>$(RepoRoot)installer\PowerToysSetupVNext\CliShims.wxs + <_CliShimWxsText>$([System.IO.File]::ReadAllText('$(_CliShimWxsPath)')) + + <_CliShimWxsCount>$([System.Text.RegularExpressions.Regex]::Matches('$(_CliShimWxsText)', '<File\b[^>]*\bName="[^"]+\.exe"').Count) + <_CliShimManifestCount>@(CliShim->Count()) + + + + $(_CliShimWxsText.Contains('Name="%(Identity).exe"')) + + + + + + diff --git a/tools/CliShim/CliShimManifest.props b/tools/CliShim/CliShimManifest.props new file mode 100644 index 0000000000..d0cec455bc --- /dev/null +++ b/tools/CliShim/CliShimManifest.props @@ -0,0 +1,47 @@ + + + + + + + + ../FancyZonesCLI.exe + + + ../WinUI3Apps/PowerToys.ImageResizerCLI.exe + + + ../FileLocksmithCLI.exe + + + ../WinUI3Apps/PowerToys.PowerDisplay.Cli.exe + + + + + + + + + <_CliShimTargetLine Include="{ L%22%(CliShim.Identity)%22, L%22%(CliShim.RelativeTarget)%22 }," /> + + + + + diff --git a/tools/CliShim/CommandLine.cpp b/tools/CliShim/CommandLine.cpp new file mode 100644 index 0000000000..88b47d3366 --- /dev/null +++ b/tools/CliShim/CommandLine.cpp @@ -0,0 +1,63 @@ +// Copyright (c) Microsoft Corporation +// The Microsoft Corporation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +#include "CommandLine.h" + +namespace CommandLine +{ + std::wstring StripArgumentZero(std::wstring_view commandLine) + { + const auto isWhitespace = [](const wchar_t character) { + return character == L' ' || character == L'\t'; + }; + + size_t index = 0; + + // A non-shell CreateProcessW caller can prepend whitespace; without this skip the scan + // below stalls at index 0 and leaks argv[0] into the forwarded tail. This is a deliberate + // departure from the CRT, which would report an empty argv[0] instead. + while (index < commandLine.size() && isWhitespace(commandLine[index])) + { + ++index; + } + + // argv[0] is tokenized differently from every later argument, and the rule that matters is + // the CRT's, because that is what every target ends up parsing: FileLocksmithCLI is a + // native wmain, and the .NET CLIs receive their string[] from the apphost's wmain. The CRT + // (ucrt\startup\argv_parsing.cpp, parse_command_line) toggles an in-quotes flag on every + // quote while scanning argv[0] and ends the name at the first whitespace found outside + // quotes; a quote never terminates the name and a backslash never escapes one. + // + // CommandLineToArgvW is the odd one out - it ends a quoted argv[0] at the closing quote, + // with no toggling - so following it instead would leak the rest of the program name into + // the tail of a partially quoted command line such as + // `"%ProgramFiles%"\PowerToys\bin\PowerToys.FancyZones.CLI.exe arg`, which cmd.exe passes + // through verbatim. Matching the CRT is what makes the shim transparent: the target sees + // the exact tail it would have seen had the caller invoked it directly. + bool inQuotes = false; + while (index < commandLine.size()) + { + const wchar_t character = commandLine[index]; + ++index; + + if (character == L'"') + { + inQuotes = !inQuotes; + continue; + } + + if (!inQuotes && isWhitespace(character)) + { + break; + } + } + + while (index < commandLine.size() && isWhitespace(commandLine[index])) + { + ++index; + } + + return std::wstring{ commandLine.substr(index) }; + } +} diff --git a/tools/CliShim/CommandLine.h b/tools/CliShim/CommandLine.h new file mode 100644 index 0000000000..2a26929c73 --- /dev/null +++ b/tools/CliShim/CommandLine.h @@ -0,0 +1,19 @@ +// Copyright (c) Microsoft Corporation +// The Microsoft Corporation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +#pragma once + +#include +#include + +namespace CommandLine +{ + // Removes argv[0] the way the CRT tokenizes it - every quote toggles an in-quotes flag and the + // name ends at the first whitespace outside quotes, so neither a quote nor a backslash-escape + // terminates it - then trims the separating spaces/tabs and preserves the remaining + // command-line text verbatim. Note that CommandLineToArgvW uses a different rule for argv[0]; + // the CRT's is the one the target CLIs actually parse. Leading whitespace is skipped first, + // which the CRT does not do; see CommandLine.cpp. + std::wstring StripArgumentZero(std::wstring_view commandLine); +} diff --git a/tools/CliShim/main.cpp b/tools/CliShim/main.cpp new file mode 100644 index 0000000000..96a9eaf5c6 --- /dev/null +++ b/tools/CliShim/main.cpp @@ -0,0 +1,166 @@ +// Copyright (c) Microsoft Corporation +// The Microsoft Corporation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +#define WIN32_LEAN_AND_MEAN +#include + +#include +#include +#include + +#include +#include +#include + +#include "CommandLine.h" + +namespace +{ + // Match cmd.exe's "command not found" exit code for unmapped shim names. + constexpr int ExitCommandNotMapped = 9009; + + // Shim failures use codes outside the range the target CLIs use for themselves (see + // doc/devdocs/cli-conventions.md: 0 success, 1 general error, 2 invalid arguments) so that a + // caller can tell "the shim could not run the CLI" apart from "the CLI ran and failed". + constexpr int ExitTargetNotFound = 9010; + constexpr int ExitLaunchFailed = 9011; + + struct ShimTarget + { + const wchar_t* name; + const wchar_t* target; + }; + + // Generated from CliShimManifest.props. + constexpr ShimTarget ShimTargets[] = { +#include "CliShimTargets.g.inc" + }; + + // The child receives Ctrl+C/Break; keep the shim alive to return its exit code. + BOOL WINAPI ConsoleCtrlHandler(DWORD /*controlType*/) + { + return TRUE; + } + + // The shim is the only handle a caller holds on the CLI, so killing the shim - taskkill without + // /T, Process.Kill() without entireProcessTree, a script's own timeout, stopping the debugger - + // must not leave the CLI running: PowerToys.FileLocksmith.CLI --wait polls until interrupted and + // prints nothing while it does. KILL_ON_JOB_CLOSE takes the CLI down with the shim, because the + // kernel closes the job handle however the shim dies. + // + // SILENT_BREAKAWAY_OK keeps the CLI's own children out of the job: PowerToys.FancyZones.CLI + // open-settings starts a long-lived PowerToys.exe and returns, and without this flag that window + // would be killed the moment the shim exits. Plain BREAKAWAY_OK cannot substitute - it requires + // the creator to pass CREATE_BREAKAWAY_FROM_JOB, which Process.Start cannot express. This is the + // one deliberate difference from src\runner\quick_access_host.cpp. + wil::unique_handle CreateShimJob() + { + wil::unique_handle job{ CreateJobObjectW(nullptr, nullptr) }; + if (!job) + { + return {}; + } + + JOBOBJECT_EXTENDED_LIMIT_INFORMATION limits{}; + limits.BasicLimitInformation.LimitFlags = JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE | JOB_OBJECT_LIMIT_SILENT_BREAKAWAY_OK; + if (!SetInformationJobObject(job.get(), JobObjectExtendedLimitInformation, &limits, sizeof(limits))) + { + // A job without the kill limit buys nothing, so drop it rather than assign to it. + return {}; + } + + return job; + } + + const wchar_t* ResolveTarget(const std::wstring& commandName) + { + for (const ShimTarget& entry : ShimTargets) + { + if (CompareStringOrdinal(commandName.c_str(), -1, entry.name, -1, TRUE) == CSTR_EQUAL) + { + return entry.target; + } + } + + return nullptr; + } +} + +int wmain() +{ + SetConsoleCtrlHandler(ConsoleCtrlHandler, TRUE); + + std::wstring modulePath; + if (FAILED(wil::GetModuleFileNameW(nullptr, modulePath))) + { + std::fwprintf(stderr, L"cli-shim: could not determine the shim's own path.\n"); + return ExitLaunchFailed; + } + + const std::filesystem::path selfPath{ modulePath }; + const std::wstring commandName = selfPath.stem().wstring(); + + const wchar_t* relativeTarget = ResolveTarget(commandName); + if (relativeTarget == nullptr) + { + std::fwprintf(stderr, L"cli-shim: no PowerToys CLI is mapped to the command '%s'.\n", commandName.c_str()); + return ExitCommandNotMapped; + } + + const std::filesystem::path targetPath = (selfPath.parent_path() / relativeTarget).lexically_normal(); + + std::error_code existsError; + if (!std::filesystem::exists(targetPath, existsError)) + { + std::fwprintf(stderr, L"cli-shim: target not found: \"%s\".\n", targetPath.c_str()); + return ExitTargetNotFound; + } + + // Forward the raw tail so the caller's argument quoting remains unchanged. + const std::wstring forwardedArguments = CommandLine::StripArgumentZero(GetCommandLineW()); + + // lpApplicationName selects the target; argv[0] in the command line is cosmetic. + std::wstring commandLine = L'"' + targetPath.wstring() + L'"'; + if (!forwardedArguments.empty()) + { + commandLine.push_back(L' '); + commandLine.append(forwardedArguments); + } + + STARTUPINFOW startupInfo{}; + startupInfo.cb = sizeof(startupInfo); + wil::unique_process_information processInfo; + + // Best effort, and silent on failure: an unprotected CLI beats a CLI that will not start, and + // this process's stderr belongs to the CLI's caller. + const wil::unique_handle shimJob = CreateShimJob(); + + if (!CreateProcessW( + targetPath.c_str(), + commandLine.data(), // Requires a mutable buffer; CreateProcessW may write to it. + nullptr, + nullptr, + TRUE, // Inherit handles: share stdin/stdout/stderr and stay in this console. + 0, + nullptr, + nullptr, + &startupInfo, + &processInfo)) + { + std::fwprintf(stderr, L"cli-shim: failed to launch \"%s\" (error %lu).\n", targetPath.c_str(), GetLastError()); + return ExitLaunchFailed; + } + + if (shimJob) + { + AssignProcessToJobObject(shimJob.get(), processInfo.hProcess); + } + + WaitForSingleObject(processInfo.hProcess, INFINITE); + + DWORD exitCode = static_cast(ExitLaunchFailed); + GetExitCodeProcess(processInfo.hProcess, &exitCode); + + return static_cast(exitCode); +} diff --git a/tools/build/build-installer.ps1 b/tools/build/build-installer.ps1 index 6c8ee237dd..e3da554b59 100644 --- a/tools/build/build-installer.ps1 +++ b/tools/build/build-installer.ps1 @@ -383,6 +383,29 @@ try { RestoreThenBuild 'tools\StylesReportTool\StylesReportTool.sln' $commonArgs $Platform $Configuration } + $cliShimPath = Join-Path $buildOutputPath 'CliShim\PowerToys.CliShim.exe' + if (-not (Test-Path -LiteralPath $cliShimPath -PathType Leaf)) { + Write-Error "CLI shim not found at '$cliShimPath'. Build PowerToys.slnx (tools\CliShim\CliShim.vcxproj) before building the installer." + exit 1 + } + + # Nothing else validates the target half of the shim manifest: CliShim.vcxproj only compares + # command names against CliShims.wxs, and the launcher tests fabricate their target at whatever + # path the manifest declares, so they cannot falsify it. The target CLIs are harvested into the + # MSI by directory glob rather than authored by hand, so moving or renaming one leaves every + # other gate green and ships a shim that exits 9010 for every user. This is the first point where + # the whole product tree exists, so resolve each mapping here exactly the way the shim will at + # run time - relative to its own bin folder - against the layout the build output mirrors. + $cliShimBinDir = Join-Path $buildOutputPath 'bin' + $cliShimManifest = [xml](Get-Content -LiteralPath (Join-Path $repoRoot 'tools\CliShim\CliShimManifest.props') -Raw) + foreach ($shim in $cliShimManifest.Project.ItemGroup.CliShim) { + $resolvedTarget = [System.IO.Path]::GetFullPath([System.IO.Path]::Combine($cliShimBinDir, $shim.RelativeTarget)) + if (-not (Test-Path -LiteralPath $resolvedTarget -PathType Leaf)) { + Write-Error "CLI shim '$($shim.Include)' maps to '$($shim.RelativeTarget)', which resolves to '$resolvedTarget' and does not exist. Update tools\CliShim\CliShimManifest.props to match where that CLI is built." + exit 1 + } + } + # Set NUGET_PACKAGES environment variable if not set, to help wixproj find heat.exe if (-not $env:NUGET_PACKAGES) { $env:NUGET_PACKAGES = Join-Path $env:USERPROFILE ".nuget\packages"