Merge pull request #277 from ProverCoderAI/issue-273

fix(web): block DECRQM/DECRQSS replies and focus/mouse event leaks in xterm.js

Author: skulidropek

experiments/terminal-query-suppression.md

@@ -1,56 +1,103 @@
 # Terminal query suppression experiment
 
-## Issue (GitHub #271)
-
-The web terminal renders raw escape sequences such as
-`^[]10;rgb:f4f4/f7f7/fbfb^[\` and `^[[?1;2c` inside Claude Code's
-prompt area, which makes navigation and rendering look broken.
-
-## Root cause
-
-TUI applications (Claude Code, Ultraplan, etc.) probe the terminal with
-queries like:
-
-- `\x1b]10;?\x1b\\` – ask for the foreground color (OSC 10).
-- `\x1b]11;?\x1b\\` – ask for the background color (OSC 11).
-- `\x1b]12;?\x1b\\` – ask for the cursor color (OSC 12).
-- `\x1b]4;<n>;?\x1b\\` – ask for an indexed palette color (OSC 4).
-- `\x1b[c` – primary device attributes query (DA1).
-- `\x1b[>c` – secondary device attributes (DA2).
-- `\x1b[=c` – tertiary device attributes (DA3).
-- `\x1b[6n` – cursor position report (CPR).
-
-`xterm.js@5.3.0` responds to all of those out-of-the-box. Because the
-web terminal is fronted by `xterm.js`, the responses are emitted via
-`Terminal.onData` and we forward them to the host PTY as user input.
-Claude Code receives these bytes as keystrokes inside its prompt loop
-and renders them verbatim, which is exactly what the screenshot in the
-issue shows.
-
-## Fix
-
-We install a small parser shim immediately after instantiating the
-`Terminal` (see `terminal-query-suppression.ts`):
-
-- For `OSC 4/10/11/12` we intercept the handler chain. If the payload
-  contains a `?` segment (query), we return `true` to consume the
-  sequence without invoking xterm's default handler that would
-  otherwise reply. Plain "set color" payloads return `false` so the
-  default handler still applies the requested theme change.
-- For DA1/DA2/DA3 (`CSI ... c`) and CPR (`CSI ... n`) we always
-  return `true` so xterm never reports back to the PTY. None of the
-  features that depend on those responses are useful for our headless
-  web frontend.
-
-The handlers are returned as disposables so callers (and the unit
-tests) can roll the registration back without touching `xterm`'s
-internal parser state.
-
-## Manual reproduction notes
-
-1. Start the web build (`bun run docker-git -- browser`) and open the
-   web terminal.
-2. Inside the container run a TUI that probes color (for example
-   `bash -c 'printf "\\033]10;?\\033\\\\"'`).
-3. Without the fix the printed escape sequence is echoed back into the
-   prompt as garbage. With the fix nothing is echoed.
+## Issues addressed
+
+- **#271** — raw `^[]10;rgb:f4f4/f7f7/fbfb^[\` and `^[[?1;2c` echoed into the
+  Claude Code prompt area.
+- **#273** — Claude Code TUI in the web terminal: input rendered on the wrong
+  row ("asd" below the plan banner), plan-approval menu does not accept
+  arrow keys, and `─` divider lines bleed through the typing area.
+
+Both have the same root cause: xterm.js answers terminal capability probes
+through `Terminal.onData`, and the web bridge forwards every `onData` byte
+to the PTY as if the user had typed it. Claude Code (Ink) then renders
+those reply bytes verbatim, scrambles its frame buffer, or misroutes
+keystrokes through its input parser.
+
+## Probes that leaked in xterm.js@5.3.0
+
+All sources cite xterm.js@5.3.0 `src/common/InputHandler.ts`.
+
+### Active leaks closed by #271
+
+| Probe | Reply |
+|---|---|
+| `OSC 4;<n>;? ST`, `OSC 10;?`, `OSC 11;?`, `OSC 12;?` color queries | indexed/foreground/background/cursor color |
+| `CSI c`, `CSI > c`, `CSI = c` device attributes (DA1/DA2/DA3) | `CSI ?...c` |
+| `CSI n`, `CSI ? n` device status / cursor position report (DSR/CPR) | `CSI ...n` |
+
+### Additional active leaks closed by this change (for #273)
+
+| Probe | Source in xterm.js@5.3.0 | Reply |
+|---|---|---|
+| `CSI Pm $ p` — DECRQM ANSI form | `InputHandler.ts:267` (`requestMode`) | `CSI Ps;Pm$y` |
+| `CSI ? Pm $ p` — DECRQM private form (includes Ink's `?2026$p` synchronized-output probe) | `InputHandler.ts:268` (`requestMode`) | `CSI ?Ps;Pm$y` |
+| `DCS $ q ... ST` — DECRQSS | `InputHandler.ts:381` (`requestStatusString`) | `DCS P{0,1}$r... ST` |
+
+### Indirect leaks (state-triggered output) closed by this change
+
+| Mode | Reason | Symptom in #273 |
+|---|---|---|
+| `CSI ? 1004 h` (focus reporting) | every DOM focus/blur pumps `CSI I` / `CSI O` into the PTY | cursor jumps, "asd" rendered one row below the plan banner, plan-approval menu mis-decodes input |
+| `CSI ? 1000 h` / `1002 h` / `1003 h` / `1006 h` / `1015 h` / `1016 h` (mouse tracking) | every mouse event pumps coordinate bytes into the PTY | Ink frame buffer corruption, e.g. `─` divider bleeds through typing line |
+
+### Defense-in-depth (no leak today, prevents regressions)
+
+| Probe | Notes |
+|---|---|
+| `CSI > q` — XTVERSION | not in 5.3.0; xterm.js master always replies `DCS > \| xterm.js(version) ST` |
+| `CSI Pm t` — window manipulation | gated by `windowOptions` (off by default in our build); explicit suppressor prevents accidental future enable |
+| `DCS + q ... ST` — XTGETTCAP | not registered in 5.3.0; cheap to pre-empt |
+
+## Modes intentionally left to fall through
+
+These DEC private modes are useful and must continue to reach xterm's
+built-in `h`/`l` setters:
+
+- `25` — cursor visibility
+- `1007` — alternate scroll (changes wheel semantics; no reply)
+- `1049` — alternate screen buffer (Claude Code does not use it today but
+  other TUIs do)
+- `2004` — bracketed paste (xterm.js wraps actual paste bytes; we want this)
+- `2026` — synchronized output (Ink wraps every frame in BSU/ESU; xterm.js
+  ≥5.3 batches the writes)
+
+The selective `CSI ? h` / `CSI ? l` handler returns `true` (suppress) only
+when one of the seven suppressed mouse/focus modes is in the parameter
+list, and `false` (fall through) for every other parameter.
+
+## Implementation
+
+`packages/app/src/web/terminal-query-suppression.ts` registers handlers via
+`Terminal.parser.registerOscHandler` / `registerCsiHandler` /
+`registerDcsHandler` directly after constructing the `Terminal`. Each
+returns a disposable; the install function aggregates them so callers can
+roll back the registrations cleanly (used by the unit tests in
+`packages/app/tests/docker-git/terminal-query-suppression.test.ts`).
+
+The selective DEC-private-mode handler iterates the params list (handling
+both flat `(number | number[])[]` shapes that xterm.js emits) and short-
+circuits as soon as a suppressed mode number is found. Sub-parameters
+(`number[]`) are reduced to their head, which matches DECSET/DECRST
+semantics (sub-params on these modes are vendor-specific extensions).
+
+## Manual reproduction (unchanged from #271 notes)
+
+1. `bun run docker-git -- browser` and open the web terminal.
+2. Run a TUI that probes capabilities, e.g. `claude` or
+   `bash -c 'printf "\\033[?2026\$p"'` (synchronized-output DECRQM).
+3. Without the fix, reply bytes leak into the prompt and the TUI either
+   echoes them or mis-decodes them as keystrokes. With the fix, nothing
+   leaks and Ink rendering stays stable across browser focus/blur.
+
+## Verification
+
+- `bun x vitest run tests/docker-git/terminal-query-suppression.test.ts`
+  — 15/15 tests pass (color query detection, every probe identifier
+  registered, all suppressed modes consumed, all benign DEC private modes
+  pass through, disposal closes every handler).
+- `bun run typecheck` — clean.
+- `vibecode-linter src/web/terminal-query-suppression.ts tests/docker-git/terminal-query-suppression.test.ts`
+  — 0 errors, 0 warnings, 0 duplicates.
+- `eslint --config eslint.effect-ts-check.config.mjs ...` — clean on both
+  files.

packages/app/src/web/panel-tasks.tsx

@@ -50,9 +50,11 @@ const TaskSystemToggle = (
     onIncludeDefaultChange
   }: Pick<TaskPanelProps, "includeDefault" | "onIncludeDefaultChange">
 ): JSX.Element => (
-  <label style={systemToggleStyle}>
+  <label htmlFor="task-panel-show-system" style={systemToggleStyle}>
     <input
       checked={includeDefault}
+      id="task-panel-show-system"
+      name="task-panel-show-system"
       onChange={(event) => {
         onIncludeDefaultChange(event.currentTarget.checked)
       }}

packages/app/src/web/terminal-image-paths.ts

@@ -8,8 +8,7 @@ const imagePathPattern = new RegExp(
   "giu"
 )
 
-const treePointerImagePathSource =
-  String.raw`[^\s"'(<>\[\]{}|\\/][^\s"'(<>\[\]{}|\\]*\.(?:${extensionAlternation})`
+const treePointerImagePathSource = String.raw`[^\s"'(<>\[\]{}|\\/][^\s"'(<>\[\]{}|\\]*\.(?:${extensionAlternation})`
 const treePointerImagePathPattern = new RegExp(
   String.raw`(?:^|\s)[└├]\s+(${treePointerImagePathSource})(?=$|[\s"')<>\[\]{}|.,;:?!])`,
   "giu"

packages/app/src/web/terminal-query-suppression.ts

@@ -2,21 +2,55 @@ export type TerminalQuerySuppression = { readonly dispose: () => void }
 
 type Disposable = { readonly dispose: () => void }
 
-type CsiIdentifier = { readonly final: string; readonly prefix?: string }
+type FunctionIdentifier = {
+  readonly final: string
+  readonly intermediates?: string
+  readonly prefix?: string
+}
+
+type CsiParam = number | ReadonlyArray<number>
+
+type CsiParams = ReadonlyArray<CsiParam>
 
 export type TerminalQuerySuppressionTarget = {
   readonly parser: {
+    readonly registerCsiHandler: (
+      id: FunctionIdentifier,
+      callback: (params: CsiParams) => boolean
+    ) => Disposable
+    readonly registerDcsHandler: (
+      id: FunctionIdentifier,
+      callback: (data: string, params: CsiParams) => boolean
+    ) => Disposable
     readonly registerOscHandler: (
       ident: number,
       callback: (data: string) => boolean
     ) => Disposable
-    readonly registerCsiHandler: (
-      id: CsiIdentifier,
-      callback: () => boolean
-    ) => Disposable
   }
 }
 
+// DEC private modes whose `h`/`l` setter causes xterm.js to start emitting
+// unsolicited reply bytes back through `onData` on later DOM events:
+//   1000/1002/1003/1006/1015/1016 — mouse tracking (mouse events -> bytes)
+//   1004                          — focus reporting (focus/blur -> CSI I/O)
+// Suppressing the SET (`h`) leaves xterm.js in the default state (no event
+// emission); suppressing the RESET (`l`) is harmless and kept for symmetry.
+// Modes intentionally left to fall through to xterm's built-in handlers:
+//   25   — cursor visibility
+//   1049 — alternate screen buffer
+//   2004 — bracketed paste
+//   2026 — synchronized output (Ink uses BSU/ESU around every frame)
+//   1007 — alternate scroll (only changes wheel semantics, no leak)
+const SUPPRESSED_PRIVATE_MODES: ReadonlySet<number> = new Set([
+  1000,
+  1002,
+  1003,
+  1004,
+  1006,
+  1015,
+  1016
+])
+
 const isColorQuery = (data: string): boolean => {
   for (const segment of data.split(";")) {
     if (segment === "?") {
@@ -26,29 +60,83 @@ const isColorQuery = (data: string): boolean => {
   return false
 }
 
+const extractParam = (param: CsiParam): number | null => {
+  if (typeof param === "number") {
+    return param
+  }
+  const head = param[0]
+  return typeof head === "number" ? head : null
+}
+
+const containsSuppressedPrivateMode = (params: CsiParams): boolean => {
+  for (const param of params) {
+    const value = extractParam(param)
+    if (value !== null && SUPPRESSED_PRIVATE_MODES.has(value)) {
+      return true
+    }
+  }
+  return false
+}
+
 const registerOscColorQuerySuppressor = (
   terminal: TerminalQuerySuppressionTarget,
   identifier: number
 ): Disposable => terminal.parser.registerOscHandler(identifier, (data) => isColorQuery(data))
 
 const registerCsiSuppressor = (
   terminal: TerminalQuerySuppressionTarget,
-  identifier: CsiIdentifier
+  identifier: FunctionIdentifier
 ): Disposable => terminal.parser.registerCsiHandler(identifier, () => true)
 
+const registerDcsSuppressor = (
+  terminal: TerminalQuerySuppressionTarget,
+  identifier: FunctionIdentifier
+): Disposable => terminal.parser.registerDcsHandler(identifier, () => true)
+
+const registerSelectivePrivateModeSuppressor = (
+  terminal: TerminalQuerySuppressionTarget,
+  final: "h" | "l"
+): Disposable =>
+  terminal.parser.registerCsiHandler(
+    { final, prefix: "?" },
+    (params) => containsSuppressedPrivateMode(params)
+  )
+
 export const installTerminalQuerySuppression = (
   terminal: TerminalQuerySuppressionTarget
 ): TerminalQuerySuppression => {
   const disposables: ReadonlyArray<Disposable> = [
+    // OSC 4/10/11/12 — color queries (`?` payload). Set-color payloads fall through.
     registerOscColorQuerySuppressor(terminal, 4),
     registerOscColorQuerySuppressor(terminal, 10),
     registerOscColorQuerySuppressor(terminal, 11),
     registerOscColorQuerySuppressor(terminal, 12),
+    // CSI c / > c / = c — primary/secondary/tertiary device attributes (DA1/DA2/DA3).
     registerCsiSuppressor(terminal, { final: "c" }),
     registerCsiSuppressor(terminal, { final: "c", prefix: ">" }),
     registerCsiSuppressor(terminal, { final: "c", prefix: "=" }),
+    // CSI n / ? n — device status report and cursor position report.
     registerCsiSuppressor(terminal, { final: "n" }),
-    registerCsiSuppressor(terminal, { final: "n", prefix: "?" })
+    registerCsiSuppressor(terminal, { final: "n", prefix: "?" }),
+    // CSI $ p / CSI ? $ p — DECRQM (ANSI and DEC private forms). xterm.js always
+    // replies via `requestMode`, including for the `?2026 $p` synchronized-output
+    // probe that Ink emits during startup.
+    registerCsiSuppressor(terminal, { final: "p", intermediates: "$" }),
+    registerCsiSuppressor(terminal, { final: "p", intermediates: "$", prefix: "?" }),
+    // DCS $ q ... ST — DECRQSS. xterm.js always replies via `requestStatusString`.
+    registerDcsSuppressor(terminal, { final: "q", intermediates: "$" }),
+    // DCS + q ... ST — XTGETTCAP. No reply in 5.3.0; suppressed for defense-in-depth.
+    registerDcsSuppressor(terminal, { final: "q", intermediates: "+" }),
+    // CSI > q — XTVERSION. Not in 5.3.0 but auto-replies in xterm.js master.
+    registerCsiSuppressor(terminal, { final: "q", prefix: ">" }),
+    // CSI Pm t — window manipulation. Gated by `windowOptions` (off by default);
+    // suppressed so an accidental future enable does not leak size reports.
+    registerCsiSuppressor(terminal, { final: "t" }),
+    // CSI ? h / CSI ? l — block xterm from enabling focus reporting and mouse
+    // tracking modes that would later pump unsolicited bytes back through onData.
+    // Other DEC private modes fall through to xterm's built-in setters.
+    registerSelectivePrivateModeSuppressor(terminal, "h"),
+    registerSelectivePrivateModeSuppressor(terminal, "l")
   ]
   return {
     dispose: () => {
@@ -60,3 +148,5 @@ export const installTerminalQuerySuppression = (
 }
 
 export const isTerminalColorQuery = isColorQuery
+
+export const isSuppressedDecPrivateMode = (mode: number): boolean => SUPPRESSED_PRIVATE_MODES.has(mode)