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

Open
#1,042 0 comments 0 reactions 0 assignees View on GitHub
bug
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.