Upstream names in the idle-gateway warning are sanitized and capped like every other status-file label (#680)

src/core/commands/status.js

@@ -2,6 +2,7 @@
 
 import { Attr, withSpan } from '../observability/index.js'
 import { collectHypAwareStatus } from '../daemon/status.js'
+import { sanitizeLabel } from '../util/json_util.js'
 import { formatFirstSyncDeadline } from '../usage-policy/first_sync_hold.js'
 
 /**
@@ -286,6 +287,42 @@ export function renderStatusJson({ report, clientNames, datasets, cacheRoot }) {
   }
 }
 
+/**
+ * How much of the daemon line's `error=` a hostile status file may spend.
+ * Wider than a label's 120, because unlike a name this carries a real error
+ * message - typically an fs error naming a full path - and the clamp exists
+ * to stop the line being bloated, not to bound an identifier. Cutting a path
+ * short is the one way this cleaning could cost a reader an answer.
+ */
+const MAX_DAEMON_ERROR_CHARS = 400
+
+/**
+ * A string on its way into the text surface, made safe to print.
+ *
+ * `renderStatusText` is the last point before a terminal, and several of the
+ * strings it interpolates were read back out of `status.json`
+ * (`daemon.state`, `daemon.mode`, and - with no runtime attached - every
+ * `sources[]` / `sinks[]` entry). That file is a *file*: core cannot assume
+ * the daemon that wrote it was this version, this build, or well behaved, so
+ * a raw value from it can carry an escape sequence that repaints the
+ * operator's screen or a newline that forges a plausible extra status line.
+ *
+ * Cleaning happens *here* rather than in the collector because these values
+ * are not display-only: `sources[].name` and `sinks[].instance` are identity
+ * keys and part of the `--json` contract, which a consumer escapes for
+ * itself. Cleaning at the interpolation closes the terminal path and leaves
+ * both intact - including the raw values the provenance lookups below match
+ * on.
+ *
+ * @param {string | undefined} value
+ * @param {number} [max]
+ * @returns {string}
+ * @ref LLP 0164#status-reads-it-from-the-status-file [constrained-by]: what core reads back out of status.json is cleaned at the last point before render, whichever field it came from
+ */
+function printable(value, max) {
+  return sanitizeLabel(value, max) ?? ''
+}
+
 /**
  * Render the V1 status report as human-friendly text. Mirrors the
  * JSON shape but groups sections and surfaces diagnostics + repair
@@ -324,7 +361,7 @@ export function renderStatusText({ report, clientNames, datasets, cacheRoot, std
     stdout.write('    (none)\n')
   } else {
     for (const s of report.sources) {
-      stdout.write(`    - ${s.name}  (${s.plugin})  [${s.state}]${provenanceTag(report.layered, isCentralPlugin(report.layered, s.plugin))}\n`)
+      stdout.write(`    - ${printable(s.name)}  (${printable(s.plugin)})  [${printable(s.state)}]${provenanceTag(report.layered, isCentralPlugin(report.layered, s.plugin))}\n`)
     }
   }
 
@@ -333,7 +370,7 @@ export function renderStatusText({ report, clientNames, datasets, cacheRoot, std
     stdout.write('    (none - keeping captured data local only)\n')
   } else {
     for (const s of report.sinks) {
-      stdout.write(`    - ${s.instance}  (${s.plugin}, ${s.kind})${provenanceTag(report.layered, isCentralSink(report.layered, s.instance))}\n`)
+      stdout.write(`    - ${printable(s.instance)}  (${printable(s.plugin)}, ${printable(s.kind)})${provenanceTag(report.layered, isCentralSink(report.layered, s.instance))}\n`)
     }
   }
 
@@ -564,17 +601,25 @@ function isCentralSink(layered, instance) {
 }
 
 /**
+ * The `daemon:` line. `state` and `mode` are read straight out of
+ * `status.json` (`collectHypAwareStatus` takes them from the snapshot when
+ * the pid file did not already supply them), and `error` can quote the file's
+ * own bytes back: a `status.json` that is not valid JSON surfaces here as
+ * `JSON.parse`'s message, which echoes an excerpt of the input verbatim. All
+ * three are cleaned on the way into the line. `pid` needs no cleaning -
+ * `readPidFile` rejects a non-number.
+ *
  * @param {ServiceState} daemon
  */
 function describeDaemon(daemon) {
   const parts = []
   parts.push(daemon.installed ? 'installed' : 'not installed')
   if (daemon.installed) parts.push(daemon.loaded ? 'loaded' : 'not loaded')
   parts.push(daemon.running ? 'running' : 'not running')
-  if (daemon.state) parts.push(`state=${daemon.state}`)
+  if (daemon.state) parts.push(`state=${printable(daemon.state)}`)
   if (daemon.pid) parts.push(`pid=${daemon.pid}`)
-  if (daemon.mode) parts.push(`mode=${daemon.mode}`)
-  if (daemon.error) parts.push(`error=${daemon.error}`)
+  if (daemon.mode) parts.push(`mode=${printable(daemon.mode)}`)
+  if (daemon.error) parts.push(`error=${printable(daemon.error, MAX_DAEMON_ERROR_CHARS)}`)
   return parts.join(', ')
 }
 

src/core/daemon/status.js

@@ -119,10 +119,15 @@ export function gatewaySourceDetails(sources) {
   const host = typeof details.host === 'string' && details.host.length > 0 ? details.host : '127.0.0.1'
   // @ref LLP 0114#fallback-is-visible [implements]: the gateway records whether this bind came through the default-port fallback
   const listenFallback = details.listen_fallback === true
-  const listenFallbackFrom =
-    typeof details.listen_fallback_from === 'string' && details.listen_fallback_from.length > 0
-      ? details.listen_fallback_from
-      : undefined
+  // Display-only, and read out of a file: `listen_fallback_from` is the
+  // configured listen address the gateway could not take, and it is printed
+  // verbatim into `gateway_port_fallback`'s message and its repair line. That
+  // makes it the same kind of value as an upstream `name` or an `entrypoint`,
+  // so it is cleaned at the same last point before render. `host` above is
+  // deliberately left alone: it is not display-only (it composes the endpoint
+  // attach writes into client settings), so bounding it is a separate change.
+  // @ref LLP 0164#status-reads-it-from-the-status-file [constrained-by]: a string read back out of status.json is cleaned before it is printed, whichever detail it came from
+  const listenFallbackFrom = sanitizeLabel(details.listen_fallback_from)
   return { host, port, listenFallback, ...(listenFallbackFrom ? { listenFallbackFrom } : {}) }
 }
 
@@ -145,6 +150,57 @@ function gatewaySourceRawDetails(sources) {
   return /** @type {Record<string, unknown>} */ (rawDetails)
 }
 
+/**
+ * How many upstream names a single warning line will spell out before it stops
+ * naming them and counts the remainder.
+ *
+ * `sanitizeLabel` bounds each name; nothing bounds how many of them the file
+ * holds. These names are read inside one sentence rather than down a block of
+ * lines, so the cap sits well under `recent clients`' 32: the count leads that
+ * sentence and is the number that actually matters, which leaves the list free
+ * to be a sample.
+ */
+const MAX_PRINTED_UPSTREAM_NAMES = 8
+
+/**
+ * The upstream names a status file offers, made safe to print: each one
+ * cleaned through `sanitizeLabel`, the list capped, and the number of names
+ * being withheld returned alongside so the message can account for them.
+ *
+ * `total` counts the raw non-empty strings, before either filter, because that
+ * is what an older status file's missing `upstreams_configured` falls back to.
+ * Cleaning bounds what is *printed*; it must never revise how many upstreams
+ * the config is reported to have asked for.
+ *
+ * These names arrive in the same file as `recent_entrypoints`, so the reason
+ * that list is sanitized on read applies here unchanged: `status.json` is a
+ * *file*, and core cannot assume the daemon that wrote it was this version,
+ * this build, or well behaved, while everything read here is about to be
+ * printed to a terminal. An upstream `name` is config-authored rather than
+ * client-authored, which lowers the odds but not the reachability, and two
+ * paths reading one file should not disagree about whether it is trusted.
+ *
+ * @param {unknown} value
+ * @returns {{ names: string[], total: number, hidden: number }}
+ * @ref LLP 0164#status-reads-it-from-the-status-file [constrained-by]: the sanitize-and-cap on read is a property of reading status.json, not of the entrypoint list that first needed it
+ */
+function printableUpstreamNames(value) {
+  const raw = Array.isArray(value)
+    ? /** @type {string[]} */ (value.filter((u) => typeof u === 'string' && u.length > 0))
+    : []
+  /** @type {string[]} */
+  const names = []
+  for (const name of raw) {
+    if (names.length === MAX_PRINTED_UPSTREAM_NAMES) break
+    const label = sanitizeLabel(name)
+    // A name that sanitizes away entirely is withheld rather than printed
+    // empty; `hidden` counts it with the ones the cap dropped, since from the
+    // reader's side both are names the file holds and the line does not show.
+    if (label !== undefined) names.push(label)
+  }
+  return { names, total: raw.length, hidden: raw.length - names.length }
+}
+
 /**
  * How many upstreams a *deliberately idle* gateway was nonetheless configured
  * with, and which of them it can name, or `undefined` when the gateway is
@@ -171,24 +227,21 @@ function gatewaySourceRawDetails(sources) {
  *
  * A status file written before `upstreams_configured` existed carries names
  * only; those still count for themselves, so an older daemon's dropped
- * `base_url` stays visible.
+ * `base_url` stays visible. The count comes off the raw list for that reason,
+ * while `printableUpstreamNames` bounds only the names handed back for
+ * display (`hidden` is how many of them it kept back).
  *
  * @param {SourceSnapshot[] | undefined} sources
- * @returns {{ count: number, names: string[] } | undefined}
+ * @returns {{ count: number, names: string[], hidden: number } | undefined}
  */
 function gatewayIdleWithConfiguredUpstreams(sources) {
   const details = gatewaySourceRawDetails(sources)
   if (!details || details.listening !== false) return undefined
-  const upstreams = details.upstreams
-  const names = Array.isArray(upstreams)
-    ? /** @type {string[]} */ (upstreams.filter((u) => typeof u === 'string' && u.length > 0))
-    : []
+  const { names, total, hidden } = printableUpstreamNames(details.upstreams)
   const rawCount = details.upstreams_configured
   const count =
-    typeof rawCount === 'number' && Number.isInteger(rawCount) && rawCount >= 0
-      ? rawCount
-      : names.length
-  return count > 0 ? { count, names } : undefined
+    typeof rawCount === 'number' && Number.isInteger(rawCount) && rawCount >= 0 ? rawCount : total
+  return count > 0 ? { count, names, hidden } : undefined
 }
 
 /**
@@ -671,11 +724,14 @@ export async function collectHypAwareStatus(opts = {}) {
     // *wanted* no upstream (hermes-only) reports no configured upstreams here
     // and never reaches this branch, so it stays healthy and silent.
     // @ref LLP 0114#fallback-is-visible [implements]: an exception to "the gateway is listening" is readable from status.json steadily, not only from a boot-time log line
-    const { count, names } = idleGatewayUpstreams
+    const { count, names, hidden } = idleGatewayUpstreams
     // Count first, names in parentheses when there are any: `name` is itself
     // one of the two keys that drops an entry, so the config that most needs
-    // this warning is exactly the one that can supply no name to print.
-    const named = names.length > 0 ? ` (${names.join(', ')})` : ''
+    // this warning is exactly the one that can supply no name to print. The
+    // names the reader held back are counted rather than dropped silently, so
+    // a truncated list never reads as a complete one.
+    const withheld = hidden > 0 ? `, +${hidden} more` : ''
+    const named = names.length > 0 ? ` (${names.join(', ')}${withheld})` : ''
     diagnostics.push({
       severity: 'warning',
       kind: 'gateway_idle_no_upstreams',

test/core/status-gateway-fallback.test.js

@@ -94,3 +94,68 @@ test('a default-port boot emits no gateway_port_fallback diagnostic', async () =
   const report = await collectHypAwareStatus(collectOpts(hypHome))
   assert.equal(report.diagnostics.find((d) => d.kind === 'gateway_port_fallback'), undefined)
 })
+
+// `listen_fallback_from` is read back out of `status.json` and printed
+// verbatim, into this diagnostic's message and into its repair line. That is
+// the same last point before render that `recent_entrypoints` and the idle
+// gateway's upstream names are cleaned at (LLP 0164), and the value has the
+// same provenance as an upstream `name`: config-authored, but reaching core
+// through a file this build did not necessarily write.
+// @ref LLP 0164#status-reads-it-from-the-status-file [tests]: a display string read out of status.json is cleaned before it reaches the terminal
+test('a hostile fallback address cannot drive the terminal from the warning', async () => {
+  const { hypHome, stateRoot } = await makeHome()
+  writeRunningDaemon(stateRoot, {
+    host: '127.0.0.1',
+    port: 54321,
+    listen_fallback: true,
+    // An erase-line sequence and a newline, which together forge a plausible
+    // extra status line out of a value the operator never chose to trust.
+    listen_fallback_from: '127.0.0.1:18521\u001b[2K\nhyp: all good',
+  })
+
+  const report = await collectHypAwareStatus(collectOpts(hypHome))
+  const diag = report.diagnostics.find((d) => d.kind === 'gateway_port_fallback')
+  assert.ok(diag)
+  assert.ok(!/[\u0000-\u001f\u007f-\u009f]/.test(diag.message), 'no control byte reaches the message')
+  assert.ok(
+    !diag.repair.some((r) => /[\u0000-\u001f\u007f-\u009f]/.test(r)),
+    'and none reaches the repair line, which is printed too',
+  )
+  assert.match(diag.message, /127\.0\.0\.1:18521/, 'the printable part still names the address')
+})
+
+test('an unbounded fallback address is clamped in the warning', async () => {
+  const { hypHome, stateRoot } = await makeHome()
+  const long = 'a'.repeat(5000)
+  writeRunningDaemon(stateRoot, {
+    host: '127.0.0.1',
+    port: 54321,
+    listen_fallback: true,
+    listen_fallback_from: long,
+  })
+
+  const report = await collectHypAwareStatus(collectOpts(hypHome))
+  const diag = report.diagnostics.find((d) => d.kind === 'gateway_port_fallback')
+  assert.ok(diag)
+  assert.ok(!diag.message.includes(long), 'the raw value is not printed whole')
+  assert.ok(diag.message.includes('a'.repeat(117) + '...'), 'it is clamped, and marked truncated')
+})
+
+// A `listen_fallback_from` that sanitizes away entirely falls back to the
+// generic phrasing, exactly as an absent one does: the warning is about the
+// bind, and it stays readable with no address to name.
+test('a fallback address that sanitizes away leaves the generic phrasing', async () => {
+  const { hypHome, stateRoot } = await makeHome()
+  writeRunningDaemon(stateRoot, {
+    host: '127.0.0.1',
+    port: 54321,
+    listen_fallback: true,
+    listen_fallback_from: '\u200b\u200b',
+  })
+
+  const report = await collectHypAwareStatus(collectOpts(hypHome))
+  const diag = report.diagnostics.find((d) => d.kind === 'gateway_port_fallback')
+  assert.ok(diag)
+  assert.match(diag.message, /its default listen address/, 'the generic antecedent stands in')
+  assert.ok(!diag.message.includes('\u200b'), 'and the empty run does not ride along')
+})

test/core/status-gateway-idle.test.js

@@ -228,3 +228,85 @@ test('a stopped daemon does not warn off a stale status snapshot', async () => {
   const report = await collectHypAwareStatus(collectOpts(hypHome))
   assert.equal(report.diagnostics.find((d) => d.kind === 'gateway_idle_no_upstreams'), undefined)
 })
+
+// The names in this warning are read out of `status.json`, which is a *file*:
+// core cannot assume the daemon that wrote it was this version, this build, or
+// well behaved, and the value is about to be printed to a terminal. The
+// `recent clients` list is read back out of the same file through
+// `sanitizeLabel` and a count cap for exactly that reason (LLP 0164); these
+// names were going to the terminal raw. All three ways a name can be hostile
+// are answered below - control and invisible bytes, unbounded length, and
+// unbounded count.
+// @ref LLP 0164#status-reads-it-from-the-status-file [tests]: what core reads back out of status.json is cleaned at the last point before render, whichever list it came from
+test('a hostile upstream name cannot drive the terminal from the warning', async () => {
+  const { hypHome, stateRoot } = await makeHome()
+  writeRunningDaemon(stateRoot, {
+    listening: false,
+    // An escape sequence that erases the line and forges a plausible second
+    // status line, and a zero-width run that hides inside a name on screen.
+    upstreams: ['anthropic\u001b[2K\nhyp: all good', 'open\u200b\u200bai'],
+    upstreams_configured: 2,
+  })
+
+  const report = await collectHypAwareStatus(collectOpts(hypHome))
+  const diag = report.diagnostics.find((d) => d.kind === 'gateway_idle_no_upstreams')
+  assert.ok(diag)
+  assert.ok(!/[\u0000-\u001f\u007f-\u009f]/.test(diag.message), 'no control byte reaches the message')
+  assert.ok(!diag.message.includes('\u200b'), 'and no zero-width run does either')
+  assert.match(diag.message, /anthropic/, 'the printable part of a name still names it')
+  assert.match(diag.message, /openai/, 'a hidden run is closed up, not made to drop the name')
+})
+
+test('an unbounded upstream name is clamped in the warning', async () => {
+  const { hypHome, stateRoot } = await makeHome()
+  const long = 'a'.repeat(5000)
+  writeRunningDaemon(stateRoot, { listening: false, upstreams: [long], upstreams_configured: 1 })
+
+  const report = await collectHypAwareStatus(collectOpts(hypHome))
+  const diag = report.diagnostics.find((d) => d.kind === 'gateway_idle_no_upstreams')
+  assert.ok(diag)
+  assert.ok(!diag.message.includes(long), 'the raw name is not printed whole')
+  // `sanitizeLabel`'s 120-character clamp, truncation marker included.
+  assert.ok(diag.message.includes('a'.repeat(117) + '...'), 'it is clamped, and marked truncated')
+})
+
+test('an unbounded number of upstream names is capped, and the rest counted', async () => {
+  const { hypHome, stateRoot } = await makeHome()
+  const many = Array.from({ length: 50 }, (_, i) => `up-${i}`)
+  writeRunningDaemon(stateRoot, { listening: false, upstreams: many, upstreams_configured: 50 })
+
+  const report = await collectHypAwareStatus(collectOpts(hypHome))
+  const diag = report.diagnostics.find((d) => d.kind === 'gateway_idle_no_upstreams')
+  assert.ok(diag)
+  assert.match(diag.message, /nothing: 50 upstreams \(/, 'the count is still the true one')
+  assert.equal(
+    many.filter((name) => diag.message.includes(`${name},`) || diag.message.includes(`${name})`)).length,
+    8,
+    'only the capped number of names is spelled out',
+  )
+  // A truncated list that reads as a complete one would be worse than no list.
+  assert.match(diag.message, /\+42 more/, 'and the names held back are counted, not dropped')
+})
+
+// The sanitizer and the cap bound what is *printed*. Neither may revise the
+// count, which is the whole signal separating a dropped upstream from a
+// legitimately upstream-less gateway - including on a status file too old to
+// carry `upstreams_configured`, where the raw name list is the only count
+// there is.
+test('an older status file counts every name it holds, capped or not', async () => {
+  const { hypHome, stateRoot } = await makeHome()
+  // 20 names, two of which sanitize away to nothing: past the cap, so the cap
+  // cannot be what makes the count 20, and holding names the printer refuses,
+  // so the sanitizer cannot be either. Counting either filter's leavings would
+  // report 18 or 8 upstreams for a config that asked for 20.
+  const older = Array.from({ length: 20 }, (_, i) => (i === 3 || i === 11 ? '\u200b\u200b' : `up-${i}`))
+  writeRunningDaemon(stateRoot, { listening: false, upstreams: older })
+
+  const report = await collectHypAwareStatus(collectOpts(hypHome))
+  const diag = report.diagnostics.find((d) => d.kind === 'gateway_idle_no_upstreams')
+  assert.ok(diag)
+  assert.match(diag.message, /nothing: 20 upstreams \(/, 'the fallback count is the raw one')
+  // 20 held, 8 printed: the 12 the line does not show are all accounted for,
+  // whichever filter withheld them.
+  assert.match(diag.message, /\+12 more/, 'and every name it does not print is counted back')
+})