Allow windows created by console apps to appear above the Terminal #12799

zadjii-msft · 2022-03-31T15:24:55Z

Window shenanigans, part the third:

Hooks the Terminal's focus state up to the underlying ConPTY. This is LOAD BEARING for allowing windows created by console applications to bring themselves to the foreground.

We're using the FocusIn/FocusOut sequences to communicate to ConPTY when a control gains/loses focus. Theoretically, other terminals could do this as well.

References

#11682 tracks real support for this sequence in Console & conpty. When we do that, we should consider even if a client application disables this mode, the Terminal & conpty should always request this from the hosting terminal (and just ignore internally to ConPTY).

See also #12515, #12526, which are the other two parts of this effort. This was tested with all three merged together, and they worked beautifully for all our scenarios. They are kept separate for ease of review.

PR Checklist

This is prototype 3 for Other applications open behind the Terminal window - (ConPTY window handle z-order shenanigans with GetConsoleWindow) #2988
I work here
Tests added/passed
[n/a] Requires documentation to be updated

Detailed Description of the Pull Request / Additional comments

This allows windows spawned by console processes to bring themselves to the foreground when the console is focused. (Historically, this is also called in the WndProc, when focus changes).

Notably, before this, ConPTY was never focused, so windows could never bring themselves to the foreground when run from a ConPTY console. We're not blanket granting the SetForeground right to all console apps when run in ConPTY. It's the responsibility of the hosting terminal emulator to always tell ConPTY when a particular instance is focused.

Validation Steps Performed

(gif below)

…viously_ that's gonna blow this diff up.

zadjii-msft · 2022-03-31T17:52:28Z

malxau · 2022-04-01T20:18:28Z

Doesn't this introduce a fairly straightforward bypass for the SetForegroundWindow restrictions? Couldn't I write a (malicious/user-hostile) program that creates a PTY, tells ConPTY it has focus (when it doesn't), then use this mechanism to launch an instance of itself into the foreground?

zadjii-msft · 2022-04-04T14:29:54Z

Doesn't this introduce a fairly straightforward bypass for the SetForegroundWindow restrictions? Couldn't I write a (malicious/user-hostile) program that creates a PTY, tells ConPTY it has focus (when it doesn't), then use this mechanism to launch an instance of itself into the foreground?

.... yes. Yes it does. I'm clearly not up and up on all my Win32-isms, so I maybe wasn't totally familiar with that restriction.

Okay, thought: we link this up with #12526. When Terminal gets the focus notification, it checks if the owner hwnd's process is the foreground process, with GetForegroundWindow & GetWindowThreadProcessId? So long as the owner window is created on the same thread as the actual main terminal thread, then this should work, yea?

@malxau I actually would really appreciate your feedback here, cause clearly I'm not familiar with the intricacies of this bit of User32

src/terminal/parser/InputStateMachineEngine.cpp

src/cascadia/TerminalControl/ControlCore.cpp

## Window shenanigans, part the first: This PR enables terminals to tell ConPTY what the owning window for the pseudo window should be. This allows thigs like MessageBoxes created by console applications to work. It also enables console apps to use `GetAncestor(GetConsoleWindow(), GA_ROOT)` to get directly at the HWND of the Terminal (but _don't please_). This is tested with our internal partners and seems to work for their scenario. See #2988, #12799, #12515, #12570. ## PR Checklist This is 1/3 of #2988.

…us-foreground

…t....

lhecker · 2022-04-13T18:01:09Z

src/server/IoDispatchers.cpp

+    const bool hasFocus{ WI_IsFlagSet(gci.Flags, CONSOLE_HAS_FOCUS) };
+    const auto grantSetForeground{ hasFocus };
+    gci.ProcessHandleList.ModifyConsoleProcessFocus(grantSetForeground);


Why is this change needed? It's equivalent to the previous code?

Oh, I suppose the code change bits aren't needed anymore. They were artifacts of debugging.

should we revert it and leave the comment?

src/host/outputStream.cpp

…us-foreground

DHowett · 2022-04-18T22:35:57Z

src/host/CursorBlinker.cpp


-    if (!WI_IsFlagSet(gci.Flags, CONSOLE_HAS_FOCUS))
+    // GH#2988: ConPTY can now be focused, but it doesn't need to do any of this work either.
+    if (inConpty || !WI_IsFlagSet(gci.Flags, CONSOLE_HAS_FOCUS))


Plz file a followup to make sure that this timer doesn't even get scheduled? For every hidden OpenConsole window, we are burning a timer every 250-500ms to... check some booleans that will never be true.

CONSOLE_INFORMATION creates _blinker as a stack member

CursorBlinker ctor does CreateThreadpoolTimer

We don't start the timer till the call to SetCaretTimer

SetCaretTimer is called in two places:

CursorBlinker::FocusStart, which is only called in WM_SETFOCUS in wndproc.cpp, so that's never hit for conpty

CursorBlinker::SettingsChanged, which is conveniently only called WM_SETTINGCHANGE. So also never for conpty.

It's probably coincidence that the blinker was never started for conpty, but I'm not terribly afraid of this regressing any time soon

DHowett · 2022-04-18T22:36:54Z

src/server/IoDispatchers.cpp

+    const bool hasFocus{ WI_IsFlagSet(gci.Flags, CONSOLE_HAS_FOCUS) };
+    const auto grantSetForeground{ hasFocus };
+    gci.ProcessHandleList.ModifyConsoleProcessFocus(grantSetForeground);


should we revert it and leave the comment?

#### ⚠️ _Targets #12799_ ⚠️ This is an atomic bit of code that partners with #12799. It's separated as an individual PR to keep diffs more simple. This ensures that when a terminal tells ConPTY that it's focused, that ConPTY doesn't do the `ConsoleControl(CONSOLE_FOREGROUND` thing unless the terminal application is actually in the foreground. This prevents a trivial exploit whereby a `malicious.exe` could create a PTY, tell ConPTY it has focus (when it doesn't), then use this mechanism to launch an instance of itself into the foreground. When the terminal tells us it's in the foreground, we're gonna look at the owner of the ConPTY window handle. If that owner has focus, then cool, this is allowed. Otherwise, we won't grant them the FG right. For this to work, the terminal just have already called `ReparentPseudoConsole`. * built on top of #12799 and #12526 * [x] Part of #2988 * [x] Tested manually.

Further builds on #12799. #12799 assumes that the connection is prepared to receive FocusIn/FocusOut events as input. For ConPTY we can be relatively sure of that, but that's not _technically_ correct. In the hypothetical world where the connection is not a ConPTY connection, then the other side might not be expecting those sequences. This remedies the issue by * ConPTY will always request focus event mode (from the terminal) when it starts up * when a client tries to disable focus events in conpty, conpty is gonna note that internally, but never transmit that to the hosting terminal, to leave the terminal in focus event mode. * `TerminalDispatch` and `ControlCore` are hooked up now to only send focus events when the Terminal is in focus event mode (which will be always for conpty) * At this point, it was like, 4LOC in `terminalInput.cpp` to add support for focus events to conhost as well. ## checklist * [x] closes #11682 * This combined with #12515 will finally close out #2988 as well, but we can do that manually. * [x] I work here * [ ] There aren't tests for this. There probably should be.

ghost · 2022-05-24T17:08:14Z

🎉Windows Terminal Preview v1.14.143 has been released which incorporates this pull request.:tada:

Handy links:

See also: #12799, the origin of much of this. This change evolved over multiple phases. ### Part the first When we create a defterm connection in `TerminalPage::_OnNewConnection`, we don't have the hosting HWND yet, so the tab gets created without one. We'll later get called with the owner, in `Initialize`. To remedy this, we need to: * In `Initialize`, make sure to update any existing controls with the new owner. * In `ControlCore`, actually propogate the new owner down to the connection ### Part the second DefTerm launches don't actually request focus mode, so the Terminal never sends them focus events. We need those focus events so that the console can request foreground rights. To remedy this, we need to: * pass `--win32input` to the commandline used to initialize OpenConsole in ConPTY mode. We request focus events at the same time we request win32-input-mode. * I also added `--resizeQuirk`, because _by all accounts that should be there_. Resizing in defterm windows should be _wacky_ without it, and I'm a little surprised we haven't seen any bugs due to this yet. ### Part the third `ConsoleSetForeground` expects a `HANDLE` to the process we want to give foreground rights to. The problem is, the wire format we used _also_ decided that a HANDLE value was a good idea. It's not. If we pass the literal value of the HANDLE to the process from OpenConsole to conhost, so conhost can call that API, the value that conhost uses there will most likely be an invalid handle. The HANDLE's value is its value in _OpenConsole_, not in conhost. To remedy this, we need to: * Just not forward `ConsoleSetForeground`. Turns out, we _can_ just call that in OpenConsole safely. There's no validation. So just instantiate a static version of the Win32 version of ConsoleControl, just to use for SetForeground. (thanks Dustin) * [x] Tested manually - Win+R `powershell`, `notepad` spawns on top. Closes #13211

See also: #12799, the origin of much of this. This change evolved over multiple phases. When we create a defterm connection in `TerminalPage::_OnNewConnection`, we don't have the hosting HWND yet, so the tab gets created without one. We'll later get called with the owner, in `Initialize`. To remedy this, we need to: * In `Initialize`, make sure to update any existing controls with the new owner. * In `ControlCore`, actually propogate the new owner down to the connection DefTerm launches don't actually request focus mode, so the Terminal never sends them focus events. We need those focus events so that the console can request foreground rights. To remedy this, we need to: * pass `--win32input` to the commandline used to initialize OpenConsole in ConPTY mode. We request focus events at the same time we request win32-input-mode. * I also added `--resizeQuirk`, because _by all accounts that should be there_. Resizing in defterm windows should be _wacky_ without it, and I'm a little surprised we haven't seen any bugs due to this yet. `ConsoleSetForeground` expects a `HANDLE` to the process we want to give foreground rights to. The problem is, the wire format we used _also_ decided that a HANDLE value was a good idea. It's not. If we pass the literal value of the HANDLE to the process from OpenConsole to conhost, so conhost can call that API, the value that conhost uses there will most likely be an invalid handle. The HANDLE's value is its value in _OpenConsole_, not in conhost. To remedy this, we need to: * Just not forward `ConsoleSetForeground`. Turns out, we _can_ just call that in OpenConsole safely. There's no validation. So just instantiate a static version of the Win32 version of ConsoleControl, just to use for SetForeground. (thanks Dustin) * [x] Tested manually - Win+R `powershell`, `notepad` spawns on top. Closes #13211 (cherry picked from commit 2a7dd8b) Service-Card-Id: 83026847 Service-Version: 1.14

zadjii-msft added 5 commits March 31, 2022 09:00

Doesn't build. Want to merge in the ConGetSet change first, cause _ob…

9dd1649

…viously_ that's gonna blow this diff up.

cherry-pick 479c6c9

1a1caf9

comments comments comments

5be7f76

remove a todo

37adb94

you knew there'd be typos

172acd2

zadjii-msft mentioned this pull request Mar 31, 2022

Enable the Terminal to tell ConPTY who the owner is #12526

Merged

8 tasks

zadjii-msft added Product-Conpty For console issues specifically related to conpty Issue-Bug It either shouldn't be doing this or needs an investigation. Area-Windowing Window frame, quake mode, tearout labels Apr 4, 2022

DHowett reviewed Apr 4, 2022

View reviewed changes

src/terminal/parser/InputStateMachineEngine.cpp Show resolved Hide resolved

DHowett reviewed Apr 4, 2022

View reviewed changes

src/cascadia/TerminalControl/ControlCore.cpp Show resolved Hide resolved

Merge branch 'main' into dev/migrie/b/2988-focus-foreground

0d17cd7

This was referenced Apr 7, 2022

Terminal + Neovim: FocusLost/Gained events don't fire. #11682

Closed

Other applications open behind the Terminal window - (ConPTY window handle z-order shenanigans with GetConsoleWindow) #2988

Closed

Merge remote-tracking branch 'origin/main' into dev/migrie/b/2988-foc…

ac53c8a

…us-foreground

This was referenced Apr 13, 2022

Ensure a terminal requesting FG rights actually has them #12899

Merged

Plumb Focus events through VT Input #12900

Merged

what? I always run the tests locally i dunno what you're talking abou…

e6e5669

…t....

lhecker approved these changes Apr 13, 2022

View reviewed changes

zadjii-msft added 2 commits April 13, 2022 14:21

this should have always been in this branch

440323d

oh my god I've done it again

3b4675c

rkeithhill mentioned this pull request Apr 14, 2022

Pop "under" window behaviour PowerShell/PowerShell#17026

Closed

5 tasks

j4james reviewed Apr 15, 2022

View reviewed changes

src/host/outputStream.cpp Outdated Show resolved Hide resolved

zadjii-msft mentioned this pull request Apr 15, 2022

Make sure to do the window owner thing for WPF too #12911

Open

zadjii-msft added 2 commits April 15, 2022 06:00

Merge remote-tracking branch 'origin/main' into dev/migrie/b/2988-foc…

9706b59

…us-foreground

lucky day, these can get moved around now

f71af78

DHowett reviewed Apr 18, 2022

View reviewed changes

zadjii-msft merged commit a496af3 into main Apr 19, 2022

zadjii-msft deleted the dev/migrie/b/2988-focus-foreground branch April 19, 2022 19:20

zadjii-msft mentioned this pull request Jun 8, 2022

Make sure foreground access works for DefTerm #13247

Merged

1 task

zadjii-msft mentioned this pull request Aug 2, 2023

Cannot type in new pane or tab until focus is manually activated with mouse click #13388

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow windows created by console apps to appear above the Terminal #12799

Allow windows created by console apps to appear above the Terminal #12799

zadjii-msft commented Mar 31, 2022 •

edited

Loading

zadjii-msft commented Mar 31, 2022

malxau commented Apr 1, 2022

zadjii-msft commented Apr 4, 2022

lhecker Apr 13, 2022

zadjii-msft Apr 13, 2022

DHowett Apr 18, 2022

DHowett Apr 18, 2022

zadjii-msft Apr 19, 2022

DHowett Apr 18, 2022

ghost commented May 24, 2022

Allow windows created by console apps to appear above the Terminal #12799

Allow windows created by console apps to appear above the Terminal #12799

Conversation

zadjii-msft commented Mar 31, 2022 • edited Loading

Window shenanigans, part the third:

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

zadjii-msft commented Mar 31, 2022

malxau commented Apr 1, 2022

zadjii-msft commented Apr 4, 2022

lhecker Apr 13, 2022

Choose a reason for hiding this comment

zadjii-msft Apr 13, 2022

Choose a reason for hiding this comment

DHowett Apr 18, 2022

Choose a reason for hiding this comment

DHowett Apr 18, 2022

Choose a reason for hiding this comment

zadjii-msft Apr 19, 2022

Choose a reason for hiding this comment

DHowett Apr 18, 2022

Choose a reason for hiding this comment

ghost commented May 24, 2022

zadjii-msft commented Mar 31, 2022 •

edited

Loading