From 0d1e4d3461c6f5dac19df890dd43ed9f43e818e1 Mon Sep 17 00:00:00 2001 From: James Holderness Date: Mon, 24 Jan 2022 16:13:50 +0000 Subject: [PATCH] Make sure titles always sanitized before passing over conpty (#12211) When title updates are forwarded from the host to the client terminal, they're passed over conpty in an `OSC 0` sequence. If there are any control characters embedded in that title text it's essential they be filtered out, otherwise they are likely to be misinterpreted by the VT parser on the other side. This PR fixes a case where that sanitization step was missed for titles initialized at startup. Originally the sanitization step was handled in `DoSrvSetConsoleTitleW`, which catches title changes made via VT escape sequences, or through the console API, but missed the title initialization at startup. I've now moved that sanitization code into the `CONSOLE_INFORMATION::SetTitle` method, which should cover all cases. This sanitization is only meant to occur when in "pty mode", though, which we were originally establishing with an `IsInVtIoMode` call. However, `IsInVtIoMode` does not return the correct result when the title is set at startup, since the VT I/O thread is not initialized at that point. So I've instead had to change that to an `InConptyMode` call, which determines the conpty state from the launch args. ## Validation Steps Performed I've manually confirmed the test case described in issue #12206 is now working correctly. However, the change to using `InConptyMode` caused some of the unit tests to fail, because there isn't a real conpty connection when testing. Fortunately there are some `EnableConptyModeForTests` methods used by the unit tests to fake the appearance of a conpty connection, and I just needed to add some additional state in one of those methods to trigger the correct `InConptyMode` response. Closes #12206 (cherry picked from commit 3804f2672ea89f72c623846e5155621e6878eefd) --- src/host/ConsoleArguments.cpp | 4 +++- src/host/consoleInformation.cpp | 14 ++++++++++++++ src/host/getset.cpp | 21 +-------------------- 3 files changed, 18 insertions(+), 21 deletions(-) diff --git a/src/host/ConsoleArguments.cpp b/src/host/ConsoleArguments.cpp index 148a6d321d2..333f9e1d859 100644 --- a/src/host/ConsoleArguments.cpp +++ b/src/host/ConsoleArguments.cpp @@ -667,7 +667,7 @@ bool ConsoleArguments::IsWin32InputModeEnabled() const #ifdef UNIT_TESTING // Method Description: // - This is a test helper method. It can be used to trick us into thinking -// we're headless (in conpty mode), even without parsing any arguments. +// we're in conpty mode, even without parsing any arguments. // Arguments: // - // Return Value: @@ -675,5 +675,7 @@ bool ConsoleArguments::IsWin32InputModeEnabled() const void ConsoleArguments::EnableConptyModeForTests() { _headless = true; + _vtInHandle = GetStdHandle(STD_INPUT_HANDLE); + _vtOutHandle = GetStdHandle(STD_OUTPUT_HANDLE); } #endif diff --git a/src/host/consoleInformation.cpp b/src/host/consoleInformation.cpp index 1129059219b..9445fb303e5 100644 --- a/src/host/consoleInformation.cpp +++ b/src/host/consoleInformation.cpp @@ -277,6 +277,20 @@ std::pair CONSOLE_INFORMATION::LookupAttributeColors(const T void CONSOLE_INFORMATION::SetTitle(const std::wstring_view newTitle) { _Title = std::wstring{ newTitle.begin(), newTitle.end() }; + + // Sanitize the input if we're in pty mode. No control chars - this string + // will get emitted back to the TTY in a VT sequence, and we don't want + // to embed control characters in that string. Note that we can't use + // IsInVtIoMode for this test, because the VT I/O thread won't have + // been created when the title is first set during startup. + if (ServiceLocator::LocateGlobals().launchArgs.InConptyMode()) + { + _Title.erase(std::remove_if(_Title.begin(), _Title.end(), [](auto ch) { + return ch < UNICODE_SPACE || (ch > UNICODE_DEL && ch < UNICODE_NBSP); + }), + _Title.end()); + } + _TitleAndPrefix = _Prefix + _Title; auto* const pRender = ServiceLocator::LocateGlobals().pRender; diff --git a/src/host/getset.cpp b/src/host/getset.cpp index c4e32b015d9..e80089cd761 100644 --- a/src/host/getset.cpp +++ b/src/host/getset.cpp @@ -1955,26 +1955,7 @@ void DoSrvPrivateRefreshWindow(_In_ const SCREEN_INFORMATION& screenInfo) [[nodiscard]] HRESULT DoSrvSetConsoleTitleW(const std::wstring_view title) noexcept { CONSOLE_INFORMATION& gci = ServiceLocator::LocateGlobals().getConsoleInformation(); - - // Sanitize the input if we're in pty mode. No control chars - this string - // will get emitted back to the TTY in a VT sequence, and we don't want - // to embed control characters in that string. - if (gci.IsInVtIoMode()) - { - std::wstring sanitized{ title }; - sanitized.erase(std::remove_if(sanitized.begin(), sanitized.end(), [](auto ch) { - return ch < UNICODE_SPACE || (ch > UNICODE_DEL && ch < UNICODE_NBSP); - }), - sanitized.end()); - - gci.SetTitle({ sanitized }); - } - else - { - // SetTitle will trigger the renderer to update the titlebar for us. - gci.SetTitle(title); - } - + gci.SetTitle(title); return S_OK; }