Use a "virtual CWD" for each terminal window (#15280)

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.
This commit is contained in:
Mike Griese 2023-05-12 13:20:27 -05:00 committed by GitHub
parent 99abb2a6b5
commit 5c08a86c49
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
12 changed files with 138 additions and 23 deletions

View File

@ -10,7 +10,7 @@ namespace Microsoft.Terminal.Remoting
CommandlineArgs(String[] args, String cwd, UInt32 showWindowCommand); CommandlineArgs(String[] args, String cwd, UInt32 showWindowCommand);
String[] Commandline { get; set; }; String[] Commandline { get; set; };
String CurrentDirectory(); String CurrentDirectory { get; };
UInt32 ShowWindowCommand { get; }; UInt32 ShowWindowCommand { get; };
}; };

View File

@ -557,27 +557,28 @@ namespace winrt::TerminalApp::implementation
// Handle it on a subsequent pass of the UI thread. // Handle it on a subsequent pass of the UI thread.
co_await wil::resume_foreground(Dispatcher(), CoreDispatcherPriority::Normal); 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 // 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 // up the scope_exit _first_. We'll release the scope_exit if we don't
// actually need it. // actually need it.
auto originalCwd{ wil::GetCurrentDirectoryW<std::wstring>() };
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 // ignore errors, we'll just power on through. We'd rather do
// something rather than fail silently if the directory doesn't // something rather than fail silently if the directory doesn't
// actually exist. // actually exist.
LOG_IF_WIN32_BOOL_FALSE(SetCurrentDirectory(originalCwd.c_str())); _WindowProperties.VirtualWorkingDirectory(originalVirtualCwd);
}); });
if (cwd.empty()) if (cwd.empty())
{ {
// We didn't actually need to change the virtual CWD, so we don't
// need to restore it
restoreCwd.release(); restoreCwd.release();
} }
else else
{ {
// ignore errors, we'll just power on through. We'd rather do _WindowProperties.VirtualWorkingDirectory(cwd);
// something rather than fail silently if the directory doesn't
// actually exist.
LOG_IF_WIN32_BOOL_FALSE(SetCurrentDirectory(cwd.c_str()));
} }
if (auto page{ weakThis.get() }) if (auto page{ weakThis.get() })
@ -1232,15 +1233,12 @@ namespace winrt::TerminalApp::implementation
// construction, because the connection might not spawn the child // construction, because the connection might not spawn the child
// process until later, on another thread, after we've already // process until later, on another thread, after we've already
// restored the CWD to its original value. // restored the CWD to its original value.
auto newWorkingDirectory{ settings.StartingDirectory() }; const auto currentVirtualDir{ _WindowProperties.VirtualWorkingDirectory() };
if (newWorkingDirectory.size() == 0 || newWorkingDirectory.size() == 1 && const auto cwdString{ std::wstring_view{ currentVirtualDir } };
!(newWorkingDirectory[0] == L'~' || newWorkingDirectory[0] == L'/')) const auto requestedStartingDir{ settings.StartingDirectory() };
{ // We only want to resolve the new WD against the CWD if it doesn't look like a Linux path (see GH#592) auto newWorkingDirectory = winrt::hstring{
auto cwdString{ wil::GetCurrentDirectoryW<std::wstring>() }; Utils::EvaluateStartingDirectory(cwdString, std::wstring_view{ requestedStartingDir })
std::filesystem::path cwd{ cwdString }; };
cwd /= settings.StartingDirectory().c_str();
newWorkingDirectory = winrt::hstring{ cwd.wstring() };
}
auto conhostConn = TerminalConnection::ConptyConnection(); auto conhostConn = TerminalConnection::ConptyConnection();
auto valueSet = TerminalConnection::ConptyConnection::CreateSettings(settings.Commandline(), auto valueSet = TerminalConnection::ConptyConnection::CreateSettings(settings.Commandline(),

View File

@ -51,6 +51,8 @@ namespace TerminalApp
String WindowNameForDisplay { get; }; String WindowNameForDisplay { get; };
String WindowIdForDisplay { get; }; String WindowIdForDisplay { get; };
String VirtualWorkingDirectory { get; set; };
Boolean IsQuakeWindow(); Boolean IsQuakeWindow();
}; };

View File

@ -1019,11 +1019,14 @@ namespace winrt::TerminalApp::implementation
// returned. // returned.
// Arguments: // Arguments:
// - args: an array of strings to process as a commandline. These args can contain spaces // - 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: // Return Value:
// - the result of the first command who's parsing returned a non-zero code, // - the result of the first command who's parsing returned a non-zero code,
// or 0. (see TerminalWindow::_ParseArgs) // or 0. (see TerminalWindow::_ParseArgs)
int32_t TerminalWindow::SetStartupCommandline(array_view<const winrt::hstring> args) int32_t TerminalWindow::SetStartupCommandline(array_view<const winrt::hstring> args, winrt::hstring cwd)
{ {
_WindowProperties->SetInitialCwd(std::move(cwd));
// This is called in AppHost::ctor(), before we've created the window // This is called in AppHost::ctor(), before we've created the window
// (or called TerminalWindow::Initialize) // (or called TerminalWindow::Initialize)
const auto result = _appArgs.ParseArgs(args); const auto result = _appArgs.ParseArgs(args);
@ -1345,6 +1348,7 @@ namespace winrt::TerminalApp::implementation
CATCH_LOG(); CATCH_LOG();
} }
} }
uint64_t WindowProperties::WindowId() const noexcept uint64_t WindowProperties::WindowId() const noexcept
{ {
return _WindowId; return _WindowId;

View File

@ -48,8 +48,14 @@ namespace winrt::TerminalApp::implementation
winrt::hstring WindowNameForDisplay() const noexcept; winrt::hstring WindowNameForDisplay() const noexcept;
bool IsQuakeWindow() const noexcept; bool IsQuakeWindow() const noexcept;
WINRT_OBSERVABLE_PROPERTY(winrt::hstring, VirtualWorkingDirectory, _PropertyChangedHandlers, L"");
WINRT_CALLBACK(PropertyChanged, Windows::UI::Xaml::Data::PropertyChangedEventHandler); 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: private:
winrt::hstring _WindowName{}; winrt::hstring _WindowName{};
uint64_t _WindowId{ 0 }; uint64_t _WindowId{ 0 };
@ -71,7 +77,7 @@ namespace winrt::TerminalApp::implementation
bool HasCommandlineArguments() const noexcept; bool HasCommandlineArguments() const noexcept;
int32_t SetStartupCommandline(array_view<const winrt::hstring> actions); int32_t SetStartupCommandline(array_view<const winrt::hstring> actions, winrt::hstring cwd);
void SetStartupContent(const winrt::hstring& content, const Windows::Foundation::IReference<Windows::Foundation::Rect>& contentBounds); void SetStartupContent(const winrt::hstring& content, const Windows::Foundation::IReference<Windows::Foundation::Rect>& contentBounds);
int32_t ExecuteCommandline(array_view<const winrt::hstring> actions, const winrt::hstring& cwd); int32_t ExecuteCommandline(array_view<const winrt::hstring> actions, const winrt::hstring& cwd);
void SetSettingsStartupArgs(const std::vector<winrt::Microsoft::Terminal::Settings::Model::ActionAndArgs>& actions); void SetSettingsStartupArgs(const std::vector<winrt::Microsoft::Terminal::Settings::Model::ActionAndArgs>& actions);

View File

@ -53,7 +53,7 @@ namespace TerminalApp
Boolean HasCommandlineArguments(); Boolean HasCommandlineArguments();
Int32 SetStartupCommandline(String[] commands); Int32 SetStartupCommandline(String[] commands, String cwd);
void SetStartupContent(String json, Windows.Foundation.IReference<Windows.Foundation.Rect> bounds); void SetStartupContent(String json, Windows.Foundation.IReference<Windows.Foundation.Rect> bounds);
Int32 ExecuteCommandline(String[] commands, String cwd); Int32 ExecuteCommandline(String[] commands, String cwd);
String ParseCommandlineMessage { get; }; String ParseCommandlineMessage { get; };

View File

@ -177,7 +177,7 @@ void AppHost::_HandleCommandlineArgs(const Remoting::WindowRequestedArgs& window
} }
else if (args) else if (args)
{ {
const auto result = _windowLogic.SetStartupCommandline(args.Commandline()); const auto result = _windowLogic.SetStartupCommandline(args.Commandline(), args.CurrentDirectory());
const auto message = _windowLogic.ParseCommandlineMessage(); const auto message = _windowLogic.ParseCommandlineMessage();
if (!message.empty()) if (!message.empty())
{ {

View File

@ -71,7 +71,17 @@ bool WindowEmperor::HandleCommandlineArgs()
{ {
std::vector<winrt::hstring> args; std::vector<winrt::hstring> args;
_buildArgsFromCommandline(args); _buildArgsFromCommandline(args);
auto cwd{ wil::GetCurrentDirectoryW<std::wstring>() }; const auto cwd{ wil::GetCurrentDirectoryW<std::wstring>() };
{
// 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<std::wstring>(system32)))
{
LOG_IF_WIN32_BOOL_FALSE(SetCurrentDirectoryW(system32.c_str()));
}
}
// Get the requested initial state of the window from our startup info. For // Get the requested initial state of the window from our startup info. For
// something like `start /min`, this will set the wShowWindow member to // something like `start /min`, this will set the wShowWindow member to

View File

@ -112,4 +112,7 @@ namespace Microsoft::Console::Utils
// testing easier. // testing easier.
std::wstring_view TrimPaste(std::wstring_view textView) noexcept; 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);
} }

View File

@ -33,6 +33,8 @@ class UtilsTests
TEST_METHOD(TestTrimTrailingWhitespace); TEST_METHOD(TestTrimTrailingWhitespace);
TEST_METHOD(TestDontTrimTrailingWhitespace); TEST_METHOD(TestDontTrimTrailingWhitespace);
TEST_METHOD(TestEvaluateStartingDirectory);
void _VerifyXTermColorResult(const std::wstring_view wstr, DWORD colorValue); void _VerifyXTermColorResult(const std::wstring_view wstr, DWORD colorValue);
void _VerifyXTermColorInvalid(const std::wstring_view wstr); void _VerifyXTermColorInvalid(const std::wstring_view wstr);
}; };
@ -546,3 +548,65 @@ void UtilsTests::TestDontTrimTrailingWhitespace()
// * trim when there's a tab followed by only whitespace // * 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 // * 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");
}
}

View File

@ -828,3 +828,27 @@ std::wstring_view Utils::TrimPaste(std::wstring_view textView) noexcept
return textView.substr(0, lastNonSpace + 1); 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;
}

View File

@ -32,6 +32,10 @@ if (%1) == (rel) (
echo Manually building release echo Manually building release
set _LAST_BUILD_CONF=Release set _LAST_BUILD_CONF=Release
) )
if (%1) == (audit) (
echo Manually building audit mode
set _LAST_BUILD_CONF=AuditMode
)
if (%1) == (no_clean) ( if (%1) == (no_clean) (
set _MSBUILD_TARGET=Build set _MSBUILD_TARGET=Build
) )