Skip to content

Commit

Permalink
Make sure titles always sanitized before passing over conpty (#12211)
Browse files Browse the repository at this point in the history
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 3804f26)
  • Loading branch information
j4james authored and DHowett committed Jan 31, 2022
1 parent 2c3bf18 commit 0d1e4d3
Show file tree
Hide file tree
Showing 3 changed files with 18 additions and 21 deletions.
4 changes: 3 additions & 1 deletion src/host/ConsoleArguments.cpp
Expand Up @@ -667,13 +667,15 @@ 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:
// - <none>
// Return Value:
// - <none>
void ConsoleArguments::EnableConptyModeForTests()
{
_headless = true;
_vtInHandle = GetStdHandle(STD_INPUT_HANDLE);
_vtOutHandle = GetStdHandle(STD_OUTPUT_HANDLE);
}
#endif
14 changes: 14 additions & 0 deletions src/host/consoleInformation.cpp
Expand Up @@ -277,6 +277,20 @@ std::pair<COLORREF, COLORREF> 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;
Expand Down
21 changes: 1 addition & 20 deletions src/host/getset.cpp
Expand Up @@ -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;
}

Expand Down

0 comments on commit 0d1e4d3

Please sign in to comment.