From 8d2447bce42251190f784c534579dce6ac8da5ee Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Fri, 12 May 2023 13:20:27 -0500 Subject: [PATCH] Use a "virtual CWD" for each terminal window (#15280) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Before process model v3, each Terminal window was running in its own process, each with its own CWD. This allowed `startingDirectory: .` to work relative to the terminal's own CWD. However, now all windows share the same process, so there can only be one CWD. That's not great - folks who right-click "open in terminal", then "Use parent process directory" are only ever going to be able to use the CWD of the _first_ terminal opened. This PR remedies this issue, with a theory we had for another issue. Essentially, we'll give each Terminal window a "virtual" CWD. The Terminal isn't actually in that CWD, the terminal is in `system32`. This also will prevent the Terminal from locking the directory it was originally opened in. * Closes #5506 * There wasn't a 1.18 issue for "Use parent process directory is broken" yet, presumably selfhosters aren't using that feature * Related to #14957 Many more notes on this topic in https://github.com/microsoft/terminal/issues/4637#issuecomment-1531979200 > **Warning** > ## Breaking change‼️ This will break a profile like ```json { "commandline": "media-test.exe", "name": "Use CWD for media-test", "startingDirectory": "." }, ``` if the user right-clicks "open in terminal", then attempts to open that profile. There's some theoretical work we could do in a follow up to fix this, but I'm inclined to say that relative paths for `commandline`s were already dangerous and should have been avoided. (cherry picked from commit 5c08a86c49ba6fc12d792e2d078a09f5e3ae3aef) Service-Card-Id: 89180224 Service-Version: 1.18 --- src/cascadia/Remoting/Peasant.idl | 2 +- src/cascadia/TerminalApp/TerminalPage.cpp | 32 +++++----- src/cascadia/TerminalApp/TerminalPage.idl | 2 + src/cascadia/TerminalApp/TerminalWindow.cpp | 6 +- src/cascadia/TerminalApp/TerminalWindow.h | 8 ++- src/cascadia/TerminalApp/TerminalWindow.idl | 2 +- src/cascadia/WindowsTerminal/AppHost.cpp | 2 +- .../WindowsTerminal/WindowEmperor.cpp | 12 +++- src/types/inc/utils.hpp | 3 + src/types/ut_types/UtilsTests.cpp | 64 +++++++++++++++++++ src/types/utils.cpp | 24 +++++++ tools/bcz.cmd | 4 ++ 12 files changed, 138 insertions(+), 23 deletions(-) diff --git a/src/cascadia/Remoting/Peasant.idl b/src/cascadia/Remoting/Peasant.idl index 89e2b52db2c..64ff512a1b4 100644 --- a/src/cascadia/Remoting/Peasant.idl +++ b/src/cascadia/Remoting/Peasant.idl @@ -10,7 +10,7 @@ namespace Microsoft.Terminal.Remoting CommandlineArgs(String[] args, String cwd, UInt32 showWindowCommand); String[] Commandline { get; set; }; - String CurrentDirectory(); + String CurrentDirectory { get; }; UInt32 ShowWindowCommand { get; }; }; diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index a3d475ced8f..2af6a23f2a2 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -557,27 +557,28 @@ namespace winrt::TerminalApp::implementation // Handle it on a subsequent pass of the UI thread. co_await wil::resume_foreground(Dispatcher(), CoreDispatcherPriority::Normal); - // If the caller provided a CWD, switch to that directory, then switch + // If the caller provided a CWD, "switch" to that directory, then switch // back once we're done. This looks weird though, because we have to set // up the scope_exit _first_. We'll release the scope_exit if we don't // actually need it. - auto originalCwd{ wil::GetCurrentDirectoryW() }; - auto restoreCwd = wil::scope_exit([&originalCwd]() { + + auto originalVirtualCwd{ _WindowProperties.VirtualWorkingDirectory() }; + auto restoreCwd = wil::scope_exit([&originalVirtualCwd, this]() { // ignore errors, we'll just power on through. We'd rather do // something rather than fail silently if the directory doesn't // actually exist. - LOG_IF_WIN32_BOOL_FALSE(SetCurrentDirectory(originalCwd.c_str())); + _WindowProperties.VirtualWorkingDirectory(originalVirtualCwd); }); + if (cwd.empty()) { + // We didn't actually need to change the virtual CWD, so we don't + // need to restore it restoreCwd.release(); } else { - // ignore errors, we'll just power on through. We'd rather do - // something rather than fail silently if the directory doesn't - // actually exist. - LOG_IF_WIN32_BOOL_FALSE(SetCurrentDirectory(cwd.c_str())); + _WindowProperties.VirtualWorkingDirectory(cwd); } if (auto page{ weakThis.get() }) @@ -1232,15 +1233,12 @@ namespace winrt::TerminalApp::implementation // construction, because the connection might not spawn the child // process until later, on another thread, after we've already // restored the CWD to its original value. - auto newWorkingDirectory{ settings.StartingDirectory() }; - if (newWorkingDirectory.size() == 0 || newWorkingDirectory.size() == 1 && - !(newWorkingDirectory[0] == L'~' || newWorkingDirectory[0] == L'/')) - { // We only want to resolve the new WD against the CWD if it doesn't look like a Linux path (see GH#592) - auto cwdString{ wil::GetCurrentDirectoryW() }; - std::filesystem::path cwd{ cwdString }; - cwd /= settings.StartingDirectory().c_str(); - newWorkingDirectory = winrt::hstring{ cwd.wstring() }; - } + const auto currentVirtualDir{ _WindowProperties.VirtualWorkingDirectory() }; + const auto cwdString{ std::wstring_view{ currentVirtualDir } }; + const auto requestedStartingDir{ settings.StartingDirectory() }; + auto newWorkingDirectory = winrt::hstring{ + Utils::EvaluateStartingDirectory(cwdString, std::wstring_view{ requestedStartingDir }) + }; auto conhostConn = TerminalConnection::ConptyConnection(); auto valueSet = TerminalConnection::ConptyConnection::CreateSettings(settings.Commandline(), diff --git a/src/cascadia/TerminalApp/TerminalPage.idl b/src/cascadia/TerminalApp/TerminalPage.idl index 92052ed1677..4375c000738 100644 --- a/src/cascadia/TerminalApp/TerminalPage.idl +++ b/src/cascadia/TerminalApp/TerminalPage.idl @@ -51,6 +51,8 @@ namespace TerminalApp String WindowNameForDisplay { get; }; String WindowIdForDisplay { get; }; + String VirtualWorkingDirectory { get; set; }; + Boolean IsQuakeWindow(); }; diff --git a/src/cascadia/TerminalApp/TerminalWindow.cpp b/src/cascadia/TerminalApp/TerminalWindow.cpp index 6cb20121784..c95cf789315 100644 --- a/src/cascadia/TerminalApp/TerminalWindow.cpp +++ b/src/cascadia/TerminalApp/TerminalWindow.cpp @@ -1019,11 +1019,14 @@ namespace winrt::TerminalApp::implementation // returned. // Arguments: // - args: an array of strings to process as a commandline. These args can contain spaces + // - cwd: The CWD that this window should treat as its own "virtual" CWD // Return Value: // - the result of the first command who's parsing returned a non-zero code, // or 0. (see TerminalWindow::_ParseArgs) - int32_t TerminalWindow::SetStartupCommandline(array_view args) + int32_t TerminalWindow::SetStartupCommandline(array_view args, winrt::hstring cwd) { + _WindowProperties->SetInitialCwd(std::move(cwd)); + // This is called in AppHost::ctor(), before we've created the window // (or called TerminalWindow::Initialize) const auto result = _appArgs.ParseArgs(args); @@ -1345,6 +1348,7 @@ namespace winrt::TerminalApp::implementation CATCH_LOG(); } } + uint64_t WindowProperties::WindowId() const noexcept { return _WindowId; diff --git a/src/cascadia/TerminalApp/TerminalWindow.h b/src/cascadia/TerminalApp/TerminalWindow.h index 2d288ccfddf..a59c139a95c 100644 --- a/src/cascadia/TerminalApp/TerminalWindow.h +++ b/src/cascadia/TerminalApp/TerminalWindow.h @@ -48,8 +48,14 @@ namespace winrt::TerminalApp::implementation winrt::hstring WindowNameForDisplay() const noexcept; bool IsQuakeWindow() const noexcept; + WINRT_OBSERVABLE_PROPERTY(winrt::hstring, VirtualWorkingDirectory, _PropertyChangedHandlers, L""); + WINRT_CALLBACK(PropertyChanged, Windows::UI::Xaml::Data::PropertyChangedEventHandler); + public: + // Used for setting the initial CWD, before we have XAML set up for property change notifications. + void SetInitialCwd(winrt::hstring cwd) { _VirtualWorkingDirectory = std::move(cwd); }; + private: winrt::hstring _WindowName{}; uint64_t _WindowId{ 0 }; @@ -71,7 +77,7 @@ namespace winrt::TerminalApp::implementation bool HasCommandlineArguments() const noexcept; - int32_t SetStartupCommandline(array_view actions); + int32_t SetStartupCommandline(array_view actions, winrt::hstring cwd); void SetStartupContent(const winrt::hstring& content, const Windows::Foundation::IReference& contentBounds); int32_t ExecuteCommandline(array_view actions, const winrt::hstring& cwd); void SetSettingsStartupArgs(const std::vector& actions); diff --git a/src/cascadia/TerminalApp/TerminalWindow.idl b/src/cascadia/TerminalApp/TerminalWindow.idl index 912663e4a0b..d91de2dce88 100644 --- a/src/cascadia/TerminalApp/TerminalWindow.idl +++ b/src/cascadia/TerminalApp/TerminalWindow.idl @@ -53,7 +53,7 @@ namespace TerminalApp Boolean HasCommandlineArguments(); - Int32 SetStartupCommandline(String[] commands); + Int32 SetStartupCommandline(String[] commands, String cwd); void SetStartupContent(String json, Windows.Foundation.IReference bounds); Int32 ExecuteCommandline(String[] commands, String cwd); String ParseCommandlineMessage { get; }; diff --git a/src/cascadia/WindowsTerminal/AppHost.cpp b/src/cascadia/WindowsTerminal/AppHost.cpp index 16be33a3db3..07f4c3e114d 100644 --- a/src/cascadia/WindowsTerminal/AppHost.cpp +++ b/src/cascadia/WindowsTerminal/AppHost.cpp @@ -177,7 +177,7 @@ void AppHost::_HandleCommandlineArgs(const Remoting::WindowRequestedArgs& window } else if (args) { - const auto result = _windowLogic.SetStartupCommandline(args.Commandline()); + const auto result = _windowLogic.SetStartupCommandline(args.Commandline(), args.CurrentDirectory()); const auto message = _windowLogic.ParseCommandlineMessage(); if (!message.empty()) { diff --git a/src/cascadia/WindowsTerminal/WindowEmperor.cpp b/src/cascadia/WindowsTerminal/WindowEmperor.cpp index e847547c01e..882f8d097ae 100644 --- a/src/cascadia/WindowsTerminal/WindowEmperor.cpp +++ b/src/cascadia/WindowsTerminal/WindowEmperor.cpp @@ -71,7 +71,17 @@ bool WindowEmperor::HandleCommandlineArgs() { std::vector args; _buildArgsFromCommandline(args); - auto cwd{ wil::GetCurrentDirectoryW() }; + const auto cwd{ wil::GetCurrentDirectoryW() }; + + { + // ALWAYS change the _real_ CWD of the Terminal to system32, so that we + // don't lock the directory we were spawned in. + std::wstring system32{}; + if (SUCCEEDED_LOG(wil::GetSystemDirectoryW(system32))) + { + LOG_IF_WIN32_BOOL_FALSE(SetCurrentDirectoryW(system32.c_str())); + } + } // Get the requested initial state of the window from our startup info. For // something like `start /min`, this will set the wShowWindow member to diff --git a/src/types/inc/utils.hpp b/src/types/inc/utils.hpp index 45e16a049de..b8ce375f1ea 100644 --- a/src/types/inc/utils.hpp +++ b/src/types/inc/utils.hpp @@ -112,4 +112,7 @@ namespace Microsoft::Console::Utils // testing easier. std::wstring_view TrimPaste(std::wstring_view textView) noexcept; + // Same deal, but in TerminalPage::_evaluatePathForCwd + std::wstring EvaluateStartingDirectory(std::wstring_view cwd, std::wstring_view startingDirectory); + } diff --git a/src/types/ut_types/UtilsTests.cpp b/src/types/ut_types/UtilsTests.cpp index f6246df87df..f83b1b4cad7 100644 --- a/src/types/ut_types/UtilsTests.cpp +++ b/src/types/ut_types/UtilsTests.cpp @@ -33,6 +33,8 @@ class UtilsTests TEST_METHOD(TestTrimTrailingWhitespace); TEST_METHOD(TestDontTrimTrailingWhitespace); + TEST_METHOD(TestEvaluateStartingDirectory); + void _VerifyXTermColorResult(const std::wstring_view wstr, DWORD colorValue); void _VerifyXTermColorInvalid(const std::wstring_view wstr); }; @@ -546,3 +548,65 @@ void UtilsTests::TestDontTrimTrailingWhitespace() // * trim when there's a tab followed by only whitespace // * not trim then there's a tab in the middle, and the string ends in whitespace } + +void UtilsTests::TestEvaluateStartingDirectory() +{ + // Continue on failures + const WEX::TestExecution::DisableVerifyExceptions disableExceptionsScope; + + auto test = [](auto& expected, auto& cwd, auto& startingDir) { + VERIFY_ARE_EQUAL(expected, EvaluateStartingDirectory(cwd, startingDir)); + }; + + // A NOTE: EvaluateStartingDirectory makes no attempt to cannonicalize the + // path. So if you do any sort of relative paths, it'll literally just + // append. + + { + std::wstring cwd = L"C:\\Windows\\System32"; + + // Literally blank + test(L"C:\\Windows\\System32\\", cwd, L""); + + // Absolute Windows path + test(L"C:\\Windows", cwd, L"C:\\Windows"); + test(L"C:/Users/migrie", cwd, L"C:/Users/migrie"); + + // Relative Windows path + test(L"C:\\Windows\\System32\\.", cwd, L"."); // ? + test(L"C:\\Windows\\System32\\.\\System32", cwd, L".\\System32"); // ? + test(L"C:\\Windows\\System32\\./dev", cwd, L"./dev"); + + // WSL '~' path + test(L"~", cwd, L"~"); + test(L"~/dev", cwd, L"~/dev"); + + // WSL or Windows / path - this will ultimately be evaluated by the connection + test(L"/", cwd, L"/"); + test(L"/dev", cwd, L"/dev"); + } + + { + std::wstring cwd = L"C:/Users/migrie"; + + // Literally blank + test(L"C:/Users/migrie\\", cwd, L""); + + // Absolute Windows path + test(L"C:\\Windows", cwd, L"C:\\Windows"); + test(L"C:/Users/migrie", cwd, L"C:/Users/migrie"); + + // Relative Windows path + test(L"C:/Users/migrie\\.", cwd, L"."); // ? + test(L"C:/Users/migrie\\.\\System32", cwd, L".\\System32"); // ? + test(L"C:/Users/migrie\\./dev", cwd, L"./dev"); + + // WSL '~' path + test(L"~", cwd, L"~"); + test(L"~/dev", cwd, L"~/dev"); + + // WSL or Windows / path - this will ultimately be evaluated by the connection + test(L"/", cwd, L"/"); + test(L"/dev", cwd, L"/dev"); + } +} diff --git a/src/types/utils.cpp b/src/types/utils.cpp index 94813bfc967..2386998e793 100644 --- a/src/types/utils.cpp +++ b/src/types/utils.cpp @@ -828,3 +828,27 @@ std::wstring_view Utils::TrimPaste(std::wstring_view textView) noexcept return textView.substr(0, lastNonSpace + 1); } + +std::wstring Utils::EvaluateStartingDirectory( + std::wstring_view currentDirectory, + std::wstring_view startingDirectory) +{ + std::wstring resultPath{ startingDirectory }; + + // We only want to resolve the new WD against the CWD if it doesn't look + // like a Linux path (see GH#592) + + // Append only if it DOESN'T look like a linux-y path. A linux-y path starts + // with `~` or `/`. + const bool looksLikeLinux = + resultPath.size() >= 1 && + (til::at(resultPath, 0) == L'~' || til::at(resultPath, 0) == L'/'); + + if (!looksLikeLinux) + { + std::filesystem::path cwd{ currentDirectory }; + cwd /= startingDirectory; + resultPath = cwd.wstring(); + } + return resultPath; +} diff --git a/tools/bcz.cmd b/tools/bcz.cmd index 46640a725f4..72071fa7cca 100644 --- a/tools/bcz.cmd +++ b/tools/bcz.cmd @@ -32,6 +32,10 @@ if (%1) == (rel) ( echo Manually building release set _LAST_BUILD_CONF=Release ) +if (%1) == (audit) ( + echo Manually building audit mode + set _LAST_BUILD_CONF=AuditMode +) if (%1) == (no_clean) ( set _MSBUILD_TARGET=Build )