microsoft / microsoft/microsoft-ui-reactor
[Bug] WinAppUi.SendKeys is fail-open: an unmapped key token exits 0 and types nothing, so every E2E inherits an instrument that cannot report failure
- Dominant language
- C#
- Stars
- 646
- Forks
- 54
- Avg merge
- 1d 3h
- Merged PRs (30d)
- 84
Description
## Summary
`WinAppUi.SendKeys` validates nothing about the key payload it forwards. Its entire correctness rests on one exit code, and the `winapp ui` CLI returns `0` both when it typed the requested keys **and** when it typed an unmapped glyph and did nothing.
**A no-op that reports success is the worst possible failure mode for an input primitive**, and this one is shared by the whole E2E tier.
## Evidence
`tests/Reactor.AppTests/Infrastructure/WinAppUi.cs:414-419`:
```csharp
var extra = new List { keys };
if (viaSendInput) { extra.Add("--via"); extra.Add("send-input"); }
if (!string.IsNullOrEmpty(target)) { extra.Add("--target"); extra.Add(target); }
var r = Run(15000, Args("send-keys", hwnd ?? HostHwnd, extra.ToArray()));
if (r.ExitCode != 0)
throw new WinAppException($"winapp ui send-keys '{keys}' failed: ...");
```
Measured on `main`:
```
WinAppUi.cs 555 lines
token validation / PUA guard / allow-list : 0 occurrences
ExitCode references (control) : 19 occurrences <- the read works
SendKeys( call sites in tests/Reactor.AppTests: 40
```
## Why it bites
`WinAppUi.SendKeys` takes the **winapp token grammar** (`"tab"`, `"enter"`, …) and forwards its argument verbatim. `Keys.*` are **WebDriver private-use code points** (`Keys.Tab == "\ue004"`), translated only by `UiElement.SendKeys` via its own table — see `tests/Reactor.AppTests/Infrastructure/UiElement.cs:235-247`:
```csharp
'\ue008' => "shift", // Keys.Shift
'\ue009' => "ctrl", // Keys.Control
'\ue004' => "tab", // Keys.Tab
'\ue006' or '\ue007' => "enter",
```
So `App.SendKeys(Keys.Tab, …)` passes an unmapped glyph to the CLI, which **exits 0 and moves nothing.**
## Why it cost a deadlock rather than a minute
The symptom is indistinguishable from a product bug: the assertion fails on a UI state that never changed, and it **relocates blame onto whatever the test was exercising.** One PR spent two CI cycles and a publicly retracted root-cause claim treating a dead instrument as a peer PR's regression.
**This also defeats a rule this repo has been propagating**: *"assert direction, not merely movement."* A direction oracle fails **identically** whether the arm walked the wrong way or the keystroke never arrived.
| failure | caught by |
|---|---|
| moved the **wrong way** | a direction oracle |
| moved **not at all** (dead instrument) | a state change **only the input can cause** |
Tests that gate on a commit only the chord can produce (e.g. `WaitForTextContaining("EditLog", "[1:Alicia,Smith]")`) time out on a silent no-op and are safe. Tests that assert only a focus index are not.
## Proposed fix — allow-list, not deny-list
A PUA **deny-list** is a second, partial copy of the CLI's token grammar: when the grammar gains a token the copy doesn't know, it silently reverts to exit-0-and-no-op. That's the same construction declined twice already in this repo (a hand-written key list duplicating `ShouldHandleKey`'s set; a duplicated `IsUnder` check).
**Validate against the known-good token alphabet and throw on anything unrecognised.** The alphabet already exists in-repo as `UiElement.cs`'s translation table, so the allow-list can be derived from the same source rather than maintained as a second grammar.
Every unmapped token then fails **loudly**, and no copy of a foreign grammar needs maintaining.
## Acceptance
- `WinAppUi.SendKeys` throws on any token outside the recognised alphabet, with the offending token in the message.
- A test asserting that a deliberately invalid token throws rather than exiting 0 — mutation-checked by removing the guard.
- The alphabet is derived from a single source shared with `UiElement`'s translation, not duplicated.
## Notes
Found by the sessions on #1010 and #1016 while resolving their merge. One of them has a PUA guard on its branch; this issue is for the shared-infrastructure fix, which no single PR owns.
Contributor guide
Research direction
Start with tests/Reactor.AppTests/Infrastructure/WinAppUi.cs:414-419 and compare its SendKeys path with the translation table in UiElement.cs:235-247. Trace the existing SendKeys call sites and determine how the shared alphabet can be used without duplication. Done means invalid tokens throw with the offending value, and a mutation-checked test proves the previous exit-0 no-op is rejected.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- csharp
- Domain
- desktop-dev, testing-qa
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 67/100