Escape captured control sequences where a query result is rendered for a person (#752)

llp/0225-captured-text-is-escaped-for-display.decision.md

@@ -0,0 +1,175 @@
+# LLP 0225: captured text is escaped where it is rendered for a person, never where it is rendered for a program
+
+**Type:** Decision
+**Status:** Accepted
+**Systems:** CLI, Query, Observability
+**Author:** Claude
+**Date:** 2026-08-13
+**Related:** LLP 0189 (#choke-point: severity colour is applied where stderr is bound, the same one-place-per-stream shape), LLP 0164 (why `sanitizeLabel` strips, and why the zero-width group is in its class), LLP 0054 (the context controls that already clip every cell before it is rendered)
+
+> `hyp query sql` wrote captured bytes straight to `process.stdout`. Every
+> string column of every dataset is verbatim captured text, so an `ESC` in a
+> prompt, a log body, an HTTP header value or a filename reached the operator's
+> terminal and was interpreted: a row could hide itself, overwrite the row above
+> it, or reorder what was read. This settles that the *output format* decides
+> whether captured text is escaped, that the escape is visible rather than a
+> silent strip, and that one character vocabulary serves both this and the
+> existing label-stripping policy.
+
+## Context {#context}
+
+`formatCell` returned a string value unchanged, and the `query sql` verb wrote
+the rendered table to `process.stdout`, which is not wrapped the way `dispatch`
+wraps stderr ([LLP 0189](./0189-cli-severity-colour.decision.md) #choke-point).
+`--format markdown` escaped only `|` and newline. `json` and `jsonl`, and the
+MCP `query_sql` path, were safe only incidentally, through `JSON.stringify`
+([#752](https://github.com/hyparam/hypaware/issues/752)).
+
+Three facts make this worth a decision rather than a patch.
+
+**The data is captured, so there is no upstream to fix.** `content_text` is
+prose a model or a user wrote. A `logs` body is whatever the observed process
+emitted. `traces` carries HTTP header values and filenames. None of it passes
+through a regex or a parser that could have bounded it, and none of it is ours.
+The base64 strip marker in the gateway's message projector is how this surfaced
+([#748](https://github.com/hyparam/hypaware/pull/748) round 1), and fixing that
+regex would have fixed nothing: an `ESC` arrives in a cell with no regex
+involved at all.
+
+**The repo already treats this threat as real, one plane over.** `sanitizeLabel`
+strips control, bidi and zero-width characters out of status-file labels, and
+its doc comment names terminal repainting as the reason. The query plane is
+where far more attacker-influenced text reaches a terminal, and it was the plane
+with no policy at all.
+
+**The operator reading a doctored table is the person triaging hostile
+traffic.** That is exactly who must not be shown a row that lies about itself.
+
+## Decision {#decision}
+
+**A render escapes captured text if and only if a person reads it.**
+
+- `table` and `markdown` go through `formatCell`, which escapes.
+- `json` and `jsonl` stay byte-exact, so the data stays extractable and a
+  pipeline still receives what was captured.
+- The `--output` spill receipt escapes its preview even though the file it
+  describes may be `jsonl`: the receipt is a human render in its own right, and
+  the file is the machine copy.
+
+**The switch is the format, never `process.stdout.isTTY`.** {#format-not-tty}
+
+A TTY gate was the obvious alternative and is worse. It makes one command print
+different bytes depending on whether it is piped, so what an operator saw and
+what they saved to a file disagree, and a bug reproduces differently under
+redirection. That is a second, invisible mode in a command whose whole job is
+reporting what was captured. A format flag is already in the operator's hand,
+already documented, and already the thing that decides every other rendering
+question; the pipeline that wants exact bytes asks for `--format jsonl` and gets
+them on a TTY too.
+
+Doing nothing was the third option (declare `hyp query sql` output untrusted,
+like `cat` on a binary file). Rejected: `cat` has no idea what its bytes are,
+while this renderer knows every cell it is laying out and has already clipped
+each one for context budget. A renderer that structures output into aligned
+columns has already promised the columns mean something.
+
+**Escape, do not strip.** {#escape-not-strip}
+
+`sanitizeLabel` drops the bytes because a label *names* a surface and a stripped
+name is still a usable name. A rendered cell *is* the payload the operator asked
+to see, so dropping bytes silently turns a query into a lie about what was
+captured. The row stays honest: an `ESC` that was in the data renders as
+`\u001b`, visibly and greppably.
+
+Consequences accepted with this:
+
+- The escape is the familiar JavaScript spelling (`\n`, `\r`, `\t`, `\uXXXX`),
+  which is pure ASCII and one column per character, so column widths measured on
+  escaped text are still display widths.
+- A backslash already present in the value is **not** doubled. Captured data is
+  full of Windows paths, regexes and JSON blobs; mangling all of them to
+  disambiguate a literal `\n` from an escaped newline would cost far more
+  legibility than the ambiguity does. The ambiguity is cosmetic, and neither
+  spelling can move a cursor.
+- A newline inside a `table` cell now prints as `\n` instead of breaking the
+  row, and a newline in a `markdown` cell prints as `\n` rather than being
+  flattened to a space. That is a visible change to multi-line prose, and it
+  is the point: a newline in a cell is how a captured value forges a row.
+
+**One vocabulary, two policies.** {#one-vocabulary}
+
+The unsafe-character class is *not* duplicated. It is decomposed into three
+named groups (terminal control, bidi formatting, invisible formatting) in
+`src/core/util/json_util.js`, and both policies are built from those groups in
+that one file:
+
+- `sanitizeLabel` strips all three, exactly as before. The recomposed class is
+  the same set of code points as the literal it replaces, and a test asserts
+  that over the whole BMP so the refactor cannot have quietly widened or
+  narrowed it.
+- `escapeForDisplay` escapes the first two only.
+
+A second hand-written class somewhere else would be a second chance for the two
+to drift apart about what "unsafe" means, which is the failure mode this shape
+exists to prevent.
+
+**The display plane escapes control and bidi, not zero-width.** {#escape-class}
+
+| group | in a label | in a cell | why |
+| --- | --- | --- | --- |
+| C0, DEL, C1, U+2028/2029 | stripped | escaped | moves the cursor, erases lines, opens escape sequences, forges rows |
+| bidi marks, embeddings, overrides, isolates | stripped | escaped | reorders text that follows, past the end of the value |
+| zero-width and default-ignorable (ZWSP, ZWNJ, ZWJ, word joiner, BOM, soft hyphen, variation selectors) | stripped | **left alone** | see below |
+
+The zero-width group is stripped from a label because a label is a **map key**:
+two labels that render identically but compare unequal dilute the entrypoint
+tracker's eviction cap. A query cell is not a key, so nothing downstream is
+diluted. Against that, ZWJ and the variation selectors are load-bearing inside
+ordinary emoji (a family emoji *is* a ZWJ sequence, and U+FE0F is what makes a
+heart red), so escaping them would visibly corrupt legitimate captured prose on
+a large fraction of real rows, in exchange for defending against a character
+that cannot repaint anything.
+
+Confusables remain out of scope on both planes, for the reason already recorded
+against the label class: this bounds what a value *does*, not what it looks
+like.
+
+## Scope {#scope}
+
+The rule stated above is general. What is *implemented* with this document is
+the query plane, which is the whole of `src/core/query/`:
+
+- `format.js`, the renderer behind `hyp query sql`, the `--output` receipt, and
+  the `vector-search` plugin's result output.
+- `overview.js`, the block behind `hyp query overview` and the wizard's closing
+  first look, whose `provider`, `model`, `date`, `tool_name` and `repo_root`
+  columns are the same captured strings. Here the escape sits on each captured
+  value rather than on the assembled row, because this block paints its own
+  bars and headings: a sweep over the finished table would strip the colour
+  along with the attack.
+
+Every other CLI surface that prints a captured value is a separate change and
+is deliberately not swept in. `hyp graph neighbors` renders node labels lifted
+straight off `ai_gateway_messages` (`client_name`, `model`, `tool_name`, a
+`file_path` basename out of a `tool_use` block) with a 48-character clamp and no
+stripping. `hyp status`, `hyp daemon status`, `hyp purge`, `hyp policy show`,
+`hyp session`, `hyp backfill plan` and the client attach adapters each echo
+values read back out of files this product does not own. Those are label-plane
+surfaces where `sanitizeLabel` is already the established answer, so each needs
+its own argument about strip-versus-escape rather than an automatic import of
+this one.
+
+## Verification {#verification}
+
+Unit tests. Six were shown failing against the pre-fix source before being
+kept: `ESC` in a `table` cell, `ESC` and bidi in a `markdown` cell, a newline
+that cannot forge a row, tab/CR spellings, column alignment with an escaped
+cell, and the `--output` receipt. Two more pass pre-fix by construction and
+guard preserved behaviour rather than the fix: `json`/`jsonl` byte-exactness
+(fails if the escape is ever applied to a machine format) and non-ASCII and
+emoji survival (fails if the display class is widened to the zero-width
+group). Both were confirmed by mutating the code they guard.
+
+**Not verified:** no test here renders into a real terminal emulator. The claim
+that `\u001b` is inert, and that a raw `ESC` sequence is not, rests on the
+escape output being pure printable ASCII rather than on an observed terminal.

src/core/query/format.js

@@ -1,5 +1,7 @@
 // @ts-check
 
+import { escapeForDisplay } from '../util/json_util.js'
+
 /**
  * @import { ContextControls, ContextControlsResult, QueryFormat, QueryResultSet } from '../../../src/core/query/types.js'
  */
@@ -139,6 +141,14 @@ function rowBytes(row) {
  * - `markdown` GitHub-flavoured table
  * - `table`    fixed-width text table (default)
  *
+ * The format, and only the format, decides whether cell values are escaped
+ * for a terminal: `table` and `markdown` are read by a person and go through
+ * `formatCell`, which escapes; `json` and `jsonl` are read by a program and
+ * stay byte-exact. Deliberately not a TTY test, so a query redirected to a
+ * file holds exactly the bytes its terminal showed.
+ *
+ * @ref LLP 0225#format-not-tty [implements]: escaping is chosen by format, never by isTTY
+ *
  * @param {QueryResultSet} result
  * @param {QueryFormat} format
  * @returns {string}
@@ -182,22 +192,29 @@ export function jsonReplacer(_key, value) {
 function renderTable(result) {
   const columns = result.columns.length > 0 ? result.columns : inferColumns(result.rows)
   if (columns.length === 0) return '(no rows)\n'
-  if (result.rows.length === 0) return columns.join('  ') + '\n(no rows)\n'
+  // Headers are schema names rather than captured data, so escaping them is
+  // defence in depth, not the fix; it costs one map and means no part of the
+  // rendered table is exempt.
+  const headers = columns.map(escapeForDisplay)
+  if (result.rows.length === 0) return headers.join('  ') + '\n(no rows)\n'
 
   const cells = result.rows.map((row) => {
     /** @type {Record<string, string>} */
     const out = {}
     for (const column of columns) out[column] = formatCell(row[column])
     return out
   })
-  const widths = columns.map((column) => {
-    let width = column.length
+  // Widths are measured on the escaped text, which is what actually gets
+  // padded and printed. Every escape this file emits is ASCII, one column
+  // per character, so a measured length is still a display width.
+  const widths = columns.map((column, i) => {
+    let width = headers[i].length
     for (const row of cells) width = Math.max(width, row[column]?.length ?? 0)
     return Math.min(width, 80)
   })
 
   const lines = [
-    columns.map((column, i) => column.padEnd(widths[i])).join('  ').trimEnd(),
+    headers.map((header, i) => header.padEnd(widths[i])).join('  ').trimEnd(),
     columns.map((_, i) => '-'.repeat(widths[i])).join('  ').trimEnd(),
   ]
   for (const row of cells) {
@@ -213,7 +230,7 @@ function renderTable(result) {
 function renderMarkdown(result) {
   const columns = result.columns.length > 0 ? result.columns : inferColumns(result.rows)
   if (columns.length === 0) return '_(no rows)_\n'
-  const header = '| ' + columns.join(' | ') + ' |'
+  const header = '| ' + columns.map((c) => mdEscape(escapeForDisplay(c))).join(' | ') + ' |'
   const divider = '| ' + columns.map(() => '---').join(' | ') + ' |'
   if (result.rows.length === 0) return [header, divider, '_(no rows)_'].join('\n') + '\n'
   const body = result.rows.map((row) => {
@@ -232,10 +249,37 @@ function inferColumns(rows) {
 }
 
 /**
+ * One cell of a human-facing render, escaped so it cannot drive the
+ * terminal it is about to be written to.
+ *
+ * Every string column of every dataset is captured verbatim: `content_text`
+ * is prose a model or a user wrote, and a log body, an HTTP header value or
+ * a filename is whatever the observed process emitted. So an `ESC` reaches
+ * a cell with no parser or regex involved, and `hyp query sql` writes the
+ * rendered table straight to `process.stdout`, which (unlike stderr at
+ * `dispatch`) wraps nothing. A row could otherwise hide itself, overwrite
+ * the row above it, or reorder what the operator reads.
+ *
+ * The escape is applied to the finished text rather than only to the string
+ * branch, because the object branch is not safe either: `JSON.stringify`
+ * escapes C0 but passes C1 and every bidi override through untouched.
+ * Running it over already-stringified JSON is a no-op for the parts
+ * `JSON.stringify` already handled.
+ *
+ * @ref LLP 0225#decision [implements]: human-facing formats escape, machine formats do not
+ *
  * @param {unknown} value
  * @returns {string}
  */
 function formatCell(value) {
+  return escapeForDisplay(rawCell(value))
+}
+
+/**
+ * @param {unknown} value
+ * @returns {string}
+ */
+function rawCell(value) {
   if (value === null || value === undefined) return ''
   if (typeof value === 'string') return value
   if (typeof value === 'bigint') return value.toString()
@@ -245,12 +289,14 @@ function formatCell(value) {
 }
 
 /**
- * Pipe characters break Markdown tables; backslash-escape them.
+ * Pipe characters break Markdown tables; backslash-escape them. Newlines
+ * cannot reach here any more: every caller passes text that has already
+ * been through `escapeForDisplay`, which spells a newline `\n`.
  *
  * @param {string} value
  */
 function mdEscape(value) {
-  return value.replace(/\|/g, '\\|').replace(/\n/g, ' ')
+  return value.replace(/\|/g, '\\|')
 }
 
 /**
@@ -295,6 +341,13 @@ export function buildQuerySqlOutput(full, opts) {
  * result went, its shape, and a small truncated preview so the caller can
  * sanity-check without ingesting the file.
  *
+ * The receipt is a human-facing render in its own right, whatever
+ * `--format` the file got, so its preview is escaped too. It is escaped one
+ * rendered line at a time: the newlines between preview rows are structure
+ * this function produced, not captured bytes, and must survive.
+ *
+ * @ref LLP 0225#decision [constrained-by]: the receipt is a human render, so it escapes
+ *
  * @param {string} outputPath
  * @param {{ columns: string[], rows: Record<string, unknown>[] }} full
  * @param {string} content  the already-rendered file content (sized for the receipt)
@@ -306,15 +359,17 @@ function renderSpillReceipt(outputPath, full, content) {
   const lines = [
     `wrote ${full.rows.length} rows · ${cols.length} cols · ${bytes}B → ${outputPath}`,
   ]
-  if (cols.length > 0) lines.push(`schema: ${cols.join(', ')}`)
+  if (cols.length > 0) lines.push(`schema: ${cols.map(escapeForDisplay).join(', ')}`)
   const previewRows = full.rows.slice(0, 3)
   if (previewRows.length > 0) {
     const { result: preview } = applyContextControls(
       { columns: full.columns, rows: previewRows },
       { maxCell: 80, maxBytes: 0 }
     )
     lines.push(`preview (first ${previewRows.length}, cells clipped):`)
-    lines.push(renderResult(preview, 'jsonl').trimEnd())
+    for (const line of renderResult(preview, 'jsonl').trimEnd().split('\n')) {
+      lines.push(escapeForDisplay(line))
+    }
   }
   return lines.join('\n') + '\n'
 }

src/core/query/overview.js

@@ -15,6 +15,7 @@
  * @import { OverviewNotice, OverviewRows, OverviewQueryRunner, OverviewWindow } from '../../../src/core/query/types.js'
  */
 
+import { escapeForDisplay } from '../util/json_util.js'
 import { executeQuerySql } from './sql.js'
 import { renderLocalOnlyNotice } from './verb.js'
 // @ref LLP 0189#palette [implements]: one ANSI table for the whole CLI
@@ -702,7 +703,7 @@ export function renderProviderMix(rows, color, showSql = false, sql = '') {
     // An unlabelled row only reaches the table if it carried tokens, which
     // today it never does. If that changes, name it for what it is rather
     // than dropping measured tokens on the floor.
-    truncate(hasModelLabel(r) ? String(r.model).trim() : '(model not recorded)', MAX_MODEL_WIDTH),
+    escapeForDisplay(truncate(hasModelLabel(r) ? String(r.model).trim() : '(model not recorded)', MAX_MODEL_WIDTH)),
     formatCount(r.input_tokens),
     formatCount(r.cached_tokens),
     formatCount(r.output_tokens),
@@ -806,7 +807,7 @@ export function renderRepoMix(rows, color, showSql = false, sql = '') {
   // same thing here as in the models and daily tables.
   const max = Math.max(...shown.map((r) => toNumber(r.input_tokens) + toNumber(r.output_tokens)))
   const body = shown.map((r) => [
-    shortRepo(String(r.repo_root)),
+    escapeForDisplay(shortRepo(String(r.repo_root))),
     formatCount(r.sessions),
     formatCount(r.input_tokens),
     formatCount(r.cached_tokens),
@@ -840,7 +841,7 @@ export function renderRepoMix(rows, color, showSql = false, sql = '') {
 export function renderToolMix(rows, color, showSql = false, sql = '') {
   const max = Math.max(...rows.map((r) => toNumber(r.calls)))
   const body = rows.map((r) => [
-    truncate(cell(r.tool_name), MAX_MODEL_WIDTH),
+    cell(r.tool_name, MAX_MODEL_WIDTH),
     formatCount(r.calls),
     formatCount(r.sessions),
     paint(bar(toNumber(r.calls), max), ANSI.cyan, color),
@@ -1015,12 +1016,27 @@ function bar(value, max) {
  * (an unnamed model, an undated row), so render the absence explicitly
  * rather than emitting a blank column the reader has to interpret.
  *
+ * It is also where a captured group key (`provider`, `date`, `tool_name`)
+ * is escaped for the terminal. The escape belongs on the captured value and
+ * not on the assembled row, because this block emits `ESC` of its own: the
+ * bars and headings are painted, so a sweep over the finished table would
+ * erase the colour along with the attack.
+ *
+ * `width`, when given, clips the raw trimmed value before escaping rather
+ * than after: escaping first can cut a `\uXXXX` escape in half at the
+ * boundary, so the clip always runs on the raw text.
+ *
+ * @ref LLP 0225#decision [implements]: the same rule on `hyp query overview`, per captured cell
+ *
  * @param {unknown} value
+ * @param {number} [width]
  * @returns {string}
  */
-function cell(value) {
+function cell(value, width) {
   if (value === null || value === undefined) return '(none)'
-  const text = String(value).trim()
+  const trimmed = String(value).trim()
+  const clipped = width === undefined ? trimmed : truncate(trimmed, width)
+  const text = escapeForDisplay(clipped)
   return text.length === 0 ? '(none)' : text
 }