From 9708a75131f44eaf527e57c4193671841c2ed454 Mon Sep 17 00:00:00 2001 From: Ian O'Neill Date: Fri, 24 Sep 2021 17:17:16 +0100 Subject: [PATCH] Properly escape constructed `wt` command-lines (#11314) Ensures that command-lines constructed to invoke `wt` are escaped properly. ## PR Checklist * [x] Closes #11273 * [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [x] Tests added/passed ## Detailed Description of the Pull Request / Additional comments This was broken in two places - when constructing the command-line in the shell extension and in `NewTerminalArgs::ToCommandline()`. Both places now invoke a shared method to escape the command-line arguments that require it. ## Validation Steps Performed Added a test and additionally: * Invoked the shell extension from `D:\Downloads\With;Semicolon`. * Added a `newWindow` action to `settings.json` as below and ensured the new window opened without erroring. ```json { "command": { "action": "newWindow", "tabTitle": "\";foo\\" }, "keys": "ctrl+shift+s" } ``` --- .../LocalTests_SettingsModel/CommandTests.cpp | 21 ++++++++++- .../ShellExtension/OpenTerminalHere.cpp | 6 +-- .../TerminalSettingsModel/ActionArgs.cpp | 10 ++--- src/cascadia/WinRTUtils/inc/WtExeUtils.h | 37 +++++++++++++++++++ 4 files changed, 63 insertions(+), 11 deletions(-) diff --git a/src/cascadia/LocalTests_SettingsModel/CommandTests.cpp b/src/cascadia/LocalTests_SettingsModel/CommandTests.cpp index b25d476e7..da5e3916c 100644 --- a/src/cascadia/LocalTests_SettingsModel/CommandTests.cpp +++ b/src/cascadia/LocalTests_SettingsModel/CommandTests.cpp @@ -465,6 +465,10 @@ namespace SettingsModelLocalTests "name":"action7_startingDirectoryWithTrailingSlash", "command": { "action": "newWindow", "startingDirectory":"C:\\", "commandline": "bar.exe" } }, + { + "name":"action8_tabTitleEscaping", + "command": { "action": "newWindow", "tabTitle":"\\\";foo\\" } + } ])" }; const auto commands0Json = VerifyParseSucceeded(commands0String); @@ -473,7 +477,7 @@ namespace SettingsModelLocalTests VERIFY_ARE_EQUAL(0u, commands.Size()); auto warnings = implementation::Command::LayerJson(commands, commands0Json); VERIFY_ARE_EQUAL(0u, warnings.size()); - VERIFY_ARE_EQUAL(8u, commands.Size()); + VERIFY_ARE_EQUAL(9u, commands.Size()); { auto command = commands.Lookup(L"action0"); @@ -586,5 +590,20 @@ namespace SettingsModelLocalTests L"cmdline: \"%s\"", cmdline.c_str())); VERIFY_ARE_EQUAL(L"--startingDirectory \"C:\\\\\" -- \"bar.exe\"", terminalArgs.ToCommandline()); } + + { + auto command = commands.Lookup(L"action8_tabTitleEscaping"); + VERIFY_IS_NOT_NULL(command); + VERIFY_IS_NOT_NULL(command.ActionAndArgs()); + VERIFY_ARE_EQUAL(ShortcutAction::NewWindow, command.ActionAndArgs().Action()); + const auto& realArgs = command.ActionAndArgs().Args().try_as(); + VERIFY_IS_NOT_NULL(realArgs); + const auto& terminalArgs = realArgs.TerminalArgs(); + VERIFY_IS_NOT_NULL(terminalArgs); + auto cmdline = terminalArgs.ToCommandline(); + Log::Comment(NoThrowString().Format( + L"cmdline: \"%s\"", cmdline.c_str())); + VERIFY_ARE_EQUAL(LR"-(--title "\\\"\;foo\\")-", terminalArgs.ToCommandline()); + } } } diff --git a/src/cascadia/ShellExtension/OpenTerminalHere.cpp b/src/cascadia/ShellExtension/OpenTerminalHere.cpp index a92609e70..5f472d72f 100644 --- a/src/cascadia/ShellExtension/OpenTerminalHere.cpp +++ b/src/cascadia/ShellExtension/OpenTerminalHere.cpp @@ -55,9 +55,7 @@ HRESULT OpenTerminalHere::Invoke(IShellItemArray* psiItemArray, STARTUPINFOEX siEx{ 0 }; siEx.StartupInfo.cb = sizeof(STARTUPINFOEX); - // Append a "\." to the given path, so that this will work in "C:\" - auto path{ wil::str_printf(LR"-(%s\.)-", pszName.get()) }; - auto cmdline{ wil::str_printf(LR"-("%s" -d "%s")-", GetWtExePath().c_str(), path.c_str()) }; + auto cmdline{ wil::str_printf(LR"-("%s" -d %s)-", GetWtExePath().c_str(), QuoteAndEscapeCommandlineArg(pszName.get()).c_str()) }; RETURN_IF_WIN32_BOOL_FALSE(CreateProcessW( nullptr, // lpApplicationName cmdline.data(), @@ -66,7 +64,7 @@ HRESULT OpenTerminalHere::Invoke(IShellItemArray* psiItemArray, false, // bInheritHandles EXTENDED_STARTUPINFO_PRESENT | CREATE_UNICODE_ENVIRONMENT, // dwCreationFlags nullptr, // lpEnvironment - path.data(), + pszName.get(), &siEx.StartupInfo, // lpStartupInfo &_piClient // lpProcessInformation )); diff --git a/src/cascadia/TerminalSettingsModel/ActionArgs.cpp b/src/cascadia/TerminalSettingsModel/ActionArgs.cpp index dd30f3547..bf60db47e 100644 --- a/src/cascadia/TerminalSettingsModel/ActionArgs.cpp +++ b/src/cascadia/TerminalSettingsModel/ActionArgs.cpp @@ -38,6 +38,7 @@ #include "MultipleActionsArgs.g.cpp" #include +#include using namespace winrt::Microsoft::Terminal::Control; @@ -121,15 +122,12 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation if (!StartingDirectory().empty()) { - // If the directory ends in a '\', we need to add another one on so that the enclosing quote added - // afterwards isn't escaped - const auto trailingBackslashEscape = StartingDirectory().back() == L'\\' ? L"\\" : L""; - ss << fmt::format(L"--startingDirectory \"{}{}\" ", StartingDirectory(), trailingBackslashEscape); + ss << fmt::format(L"--startingDirectory {} ", QuoteAndEscapeCommandlineArg(StartingDirectory())); } if (!TabTitle().empty()) { - ss << fmt::format(L"--title \"{}\" ", TabTitle()); + ss << fmt::format(L"--title {} ", QuoteAndEscapeCommandlineArg(TabTitle())); } if (TabColor()) @@ -152,7 +150,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation if (!ColorScheme().empty()) { - ss << fmt::format(L"--colorScheme \"{}\" ", ColorScheme()); + ss << fmt::format(L"--colorScheme {} ", QuoteAndEscapeCommandlineArg(ColorScheme())); } if (!Commandline().empty()) diff --git a/src/cascadia/WinRTUtils/inc/WtExeUtils.h b/src/cascadia/WinRTUtils/inc/WtExeUtils.h index aed728db5..f4541594c 100644 --- a/src/cascadia/WinRTUtils/inc/WtExeUtils.h +++ b/src/cascadia/WinRTUtils/inc/WtExeUtils.h @@ -104,3 +104,40 @@ _TIL_INLINEPREFIX const std::wstring& GetWtExePath() }(); return exePath; } + +// Method Description: +// - Quotes and escapes the given string so that it can be used as a command-line arg. +// - e.g. given `\";foo\` will return `"\\\"\;foo\\"` so that the caller can construct a command-line +// using something such as `fmt::format(L"wt --title {}", QuoteAndQuoteAndEscapeCommandlineArg(TabTitle()))`. +// Arguments: +// - arg - the command-line argument to quote and escape. +// Return Value: +// - the quoted and escaped command-line argument. +_TIL_INLINEPREFIX std::wstring QuoteAndEscapeCommandlineArg(const std::wstring_view& arg) +{ + std::wstring out; + out.reserve(arg.size() + 2); + out.push_back(L'"'); + + size_t backslashes = 0; + for (const auto ch : arg) + { + if (ch == L'\\') + { + backslashes++; + } + else + { + if (ch == L';' || ch == L'"') + { + out.append(backslashes + 1, L'\\'); + } + backslashes = 0; + } + out.push_back(ch); + } + + out.append(backslashes, L'\\'); + out.push_back(L'"'); + return out; +}