From b617c434a171f0349395038a532d1f6630fb441d Mon Sep 17 00:00:00 2001
From: Leonard Hecker <leonard@hecker.io>
Date: Thu, 6 Aug 2020 03:03:58 +0200
Subject: [PATCH] Fix #7064: Ignore key events without scan code (#7145)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Up until #4999 we deferred all key events to the character event handler
for which `ToUnicodeEx` returned a valid character and alternatively
those who aren't a special key combination as listed in
`TerminalInput`'s implementation.

Since #4999 we started acknowledging/handling all key events no matter
whether they're actually a known key combination. Given non-ASCII inputs
the Win32 `SendInput()` method generates certain sequences that aren't
recognizable combinations though and if they're handled by the key event
handler no follow up character event is sent containing the unicode
character.

This PR adds another condition and defers all key events without scan
code (i.e. those not representable by the current keyboard layout) to
the character event handler.

I'm absolutely not certain that this PR doesn't have a negative effect
on other kinds of inputs.

Is it common for key events to not contain a scan code? I personally
haven't seen it happen before AutoHotKey/SendInput.

Before this PR is merged it'd be nice to have a good testing plan in
place in order to ensure nothing breaks.

## Validation Steps Performed

Remapped `AltGr+8` to `»` using AutoHotKey using `<^>!8::SendInput {Raw}»`.
Ensured `»` is printed if `AltGr+8` is pressed.

Closes #7064
Closes #7120
---
 src/cascadia/TerminalCore/Terminal.cpp            | 13 +++++++++++++
 src/cascadia/UnitTests_TerminalCore/InputTest.cpp | 11 +++++++++++
 2 files changed, 24 insertions(+)

diff --git a/src/cascadia/TerminalCore/Terminal.cpp b/src/cascadia/TerminalCore/Terminal.cpp
index b71471fc7ef..7f106ba1f90 100644
--- a/src/cascadia/TerminalCore/Terminal.cpp
+++ b/src/cascadia/TerminalCore/Terminal.cpp
@@ -430,6 +430,19 @@ bool Terminal::SendKeyEvent(const WORD vkey,
 
     _StoreKeyEvent(vkey, scanCode);
 
+    // As a Terminal we're mostly interested in getting key events from physical hardware (mouse & keyboard).
+    // We're thus ignoring events whose values are outside the valid range and unlikely to be generated by the current keyboard.
+    // It's very likely that a proper followup character event will be sent to us.
+    // This prominently happens using AutoHotKey's keyboard remapping feature,
+    // which sends input events whose vkey is 0xff and scanCode is 0.
+    // We need to check for this early, as _CharacterFromKeyEvent() always returns 0 for such invalid values,
+    // making us believe that this is an actual non-character input, while it usually isn't.
+    // GH#7064
+    if (vkey == 0 || vkey >= 0xff || scanCode == 0)
+    {
+        return false;
+    }
+
     const auto isAltOnlyPressed = states.IsAltPressed() && !states.IsCtrlPressed();
     const auto isSuppressedAltGrAlias = !_altGrAliasing && states.IsAltPressed() && states.IsCtrlPressed();
 
diff --git a/src/cascadia/UnitTests_TerminalCore/InputTest.cpp b/src/cascadia/UnitTests_TerminalCore/InputTest.cpp
index 037c81db45b..9745b0e6b2d 100644
--- a/src/cascadia/UnitTests_TerminalCore/InputTest.cpp
+++ b/src/cascadia/UnitTests_TerminalCore/InputTest.cpp
@@ -30,6 +30,7 @@ namespace TerminalCoreUnitTests
 
         TEST_METHOD(AltShiftKey);
         TEST_METHOD(AltSpace);
+        TEST_METHOD(InvalidKeyEvent);
 
         void _VerifyExpectedInput(std::wstring& actualInput)
         {
@@ -65,4 +66,14 @@ namespace TerminalCoreUnitTests
         VERIFY_IS_FALSE(term.SendKeyEvent(L' ', 0, ControlKeyStates::LeftAltPressed, false));
         VERIFY_IS_FALSE(term.SendCharEvent(L' ', 0, ControlKeyStates::LeftAltPressed));
     }
+
+    void InputTest::InvalidKeyEvent()
+    {
+        // Certain applications like AutoHotKey and its keyboard remapping feature,
+        // send us key events using SendInput() whose values are outside of the valid range.
+        // We don't want to handle those events as we're probably going to get a proper followup character event.
+        VERIFY_IS_FALSE(term.SendKeyEvent(0, 123, {}, true));
+        VERIFY_IS_FALSE(term.SendKeyEvent(255, 123, {}, true));
+        VERIFY_IS_FALSE(term.SendKeyEvent(123, 0, {}, true));
+    }
 }