Keep this host's paths and keys out of room lines

changelog.d/2699-room-line-leak.fixed.md

@@ -0,0 +1 @@
+A room line no longer tells the other homes in the room about this machine: a picture that could not be read is named by its file name and size instead of its absolute path (a Bot's own chat still names the path for itself), and a crashed external Bot's line reports only how it ended (`exit 1`, `signal SIGKILL`) while the child's last `stderr` words stay in this host's log. Every stored room line also passes this host's secret redaction before it is kept or sent, so a pasted key cannot ride a line to a friend's Bot ([#2699](https://github.com/ThinkFlowLab/FlyBot/issues/2699)).

core/src/host/groups.js

@@ -30,7 +30,7 @@ import { FACE_LEAD_NOTE, FACE_LEAD_ROUTE, faceLeadIsRemote, faceMemberNote, face
 import { faceMentions } from './face-mentions.js'
 import { cleanName, stripSessionChips, unwrapUntrusted } from './delivery.js'
 import { createRoomParticipants } from './room-participant.js'
-import { ROOM_CONTEXT_BUDGET, boardCoversCue, fitRoomLines, formatRoomBoard, formatRoomPresence, prepareRoomLine } from './room-context.js'
+import { ROOM_CONTEXT_BUDGET, boardCoversCue, fitRoomLines, formatRoomBoard, formatRoomPresence, prepareRoomLine, setRoomLineRedactor } from './room-context.js'
 import { workDispatch } from './work-dispatch.js'
 import { retiredBot } from './store.js'
 
@@ -236,6 +236,9 @@ export function install(rt) {
   const participant = createRoomParticipants(rt)
   fanoutRoomLine = (room, line) => rt.fanoutRoomLine?.(room, line)
   stampRoomLine = (room, line) => rt.stampHop?.(room, line) ?? line
+  // A stored room line leaves for the room's other homes, so the store gate redacts with this host's
+  // own redactor (#2699).
+  setRoomLineRedactor(text => rt.redactSecrets?.(text) ?? text)
   rt.notePostedLine = (roomId, text) => postedLines.set(roomId, [...(postedLines.get(roomId) ?? []), text].slice(-20))
   // What an external participant's process does is a room line (#198), so a crash is seen where
   // the Bot speaks and not only in a log nobody reads.

core/src/host/images.js

@@ -72,9 +72,11 @@ const imageFacts = attachment => {
 }
 
 // One placeholder for both places an unread image becomes text: a Bot chat (`imageAsText`) and the
-// group relay (`imagesForGroup`). It names the bytes only when `path` is a file this Bot can open.
+// group relay (`imagesForGroup`). It names the bytes only when `path` is a file this Bot can open, and
+// never in the group form: a group line is fanned out to the room's other homes (#2699), where a path
+// of this host says nothing the far side can open and names this machine's folders to it.
 export function imagePlaceholder(attachment, { path, group = false } = {}) {
-  const where = path === undefined ? '' : ` The file is at "${path}"; your own file tools can open it.`
+  const where = group || path === undefined ? '' : ` The file is at "${path}"; your own file tools can open it.`
   const cannot = group ? '' : 'you cannot see images and '
   return `[Image ${imageLabel(attachment)}${imageFacts(attachment)}: ${cannot}it could not be read right now.${where} Ask the user what it shows if it matters.]`
 }
@@ -207,7 +209,7 @@ export function install(rt) {
       const reading = await imageReading(attachment, sessionId)
       const label = imageLabel(attachment)
       return reading === null
-        ? imagePlaceholder(attachment, { path: await attachmentFilePath(attachment, attachmentServices()), group: true })
+        ? imagePlaceholder(attachment, { group: true })
         : `[Image ${label}, read by ${reading.model}:]\n${reading.text}\n[End of image ${label}]`
     }))
   }

core/src/host/room-context.js

@@ -105,9 +105,20 @@ const FENCED_LINE = /^<untrusted-message from="([^"]*)">\n?([\s\S]*?)\n?<\/untru
 
 // Bot speech only. A person's line, and a short line, stay as stored. The log
 // object is not modified: a collapsed line is a copy.
-export function prepareRoomLine(line) {
+//
+// Every writer of `room.log` passes through here — `groups.js` `appendRoom` (a group round) and
+// `room-membership.js` `append` (a person's line, a seat's `line` op, a face-room event) — and the log
+// is fanned out to the room's other homes (`fanoutRoomLine`), so the redaction this host trusts runs
+// once, on this gate, instead of in each builder of a line (#2699). `redact` is that host's
+// `rt.redactSecrets`, wired once at install; a line the redactor leaves alone comes back unchanged.
+let redactRoomLine = text => text
+export function setRoomLineRedactor(redact) { redactRoomLine = redact }
+
+export function prepareRoomLine(line, { redact = redactRoomLine } = {}) {
   if (line == null || typeof line !== 'object') return line
   if (typeof line.text !== 'string') return line
+  const clean = typeof redact === 'function' ? redact(line.text) : line.text
+  if (clean !== line.text) line = { ...line, text: clean }
   // A line a writer already fenced for another home's Bot keeps its fence, and while it is fenced
   // neither `kind` nor `authorKind` is an exemption: both travel from the other home
   // (room-membership.js copies them), so a peer could otherwise mark an essay `notice` or type it as a

core/src/host/room-membership.js

@@ -413,7 +413,7 @@ export function install(rt) {
   // one (postRoomMessage, a seat's `line` op, a face-room event), so the same rule runs here. The rule
   // itself knows a fence now (`room-context.js` `prepareRoomLine`): it collapses the inside and wraps it
   // again, so a guest's essay is never stored as this home's own text, and both writers share one copy.
-  const storeLine = (line) => prepareRoomLine(line)
+  const storeLine = (line) => prepareRoomLine(line, { redact: rt.redactSecrets })
 
   const append = (room, line) => {
     ensureLog(room)

core/src/host/room-participant-external.js

@@ -81,20 +81,22 @@ export function createExternalRoomParticipant(rt) {
     return `${member.name}'s external process ${state === 'timeout' ? 'timed out' : state}${detail ? ` (${detail})` : ''}.`
   }
 
-  // A crash line carries the child's last words, flattened and capped, so `exit 1` is diagnosable
-  // from the room itself (#1164).
+  // The child's last words, flattened and capped, are this host's diagnosis (#1164).
   const tailOf = (entry) => String(entry.stderr ?? '').slice(-STDERR_TAIL).replace(/[\r\n]+/g, ' ').trim().slice(0, STDERR_TAIL)
   // The room sees what happened to its participant, not just a turn that never came back (#198).
-  async function report(room, member, state, detail) {
-    rt.warn?.('ds-bot: external Bot %s in %s: %s%s', member.name, room.name, state, detail ? ` (${detail})` : '')
+  // `roomDetail` is the part the room may read — an exit code, a signal name, a timeout reason — and
+  // `logDetail` is this host's diagnosis (the stderr tail, a spawn error naming a file): a room line is
+  // fanned out to the room's other homes, so only the log keeps the detail (#2699).
+  async function report(room, member, state, roomDetail, logDetail = roomDetail) {
+    rt.warn?.('ds-bot: external Bot %s in %s: %s%s', member.name, room.name, state, logDetail ? ` (${logDetail})` : '')
     try {
       await rt.externalRoomLine?.(room, {
         who: member.name,
         botId: member.id,
         authorId: member.id,
         authorKind: 'bot',
         kind: 'external',
-        text: stateLine(member, state, detail),
+        text: stateLine(member, state, roomDetail),
       })
     } catch (error) {
       rt.warn?.('ds-bot: the state of %s in %s was not posted: %s', member.name, room.name, error)
@@ -148,7 +150,7 @@ export function createExternalRoomParticipant(rt) {
       })
     } catch (error) {
       settle(entry, 'crashed', oneLine(error?.message ?? error))
-      void report(room, member, 'crashed', entry.detail)
+      void report(room, member, 'crashed', undefined, entry.detail)
       return entry
     }
     entry.child = child
@@ -192,7 +194,7 @@ export function createExternalRoomParticipant(rt) {
       entry.child = undefined
       entry.reported = true
       settle(entry, 'crashed', oneLine(error?.message ?? error))
-      void report(room, member, 'crashed', entry.detail)
+      void report(room, member, 'crashed', undefined, entry.detail)
     })
     const reportExit = (code, signal) => {
       // One line either way: whichever of 'exit' and 'close' reports first wins (#1164).
@@ -202,11 +204,12 @@ export function createExternalRoomParticipant(rt) {
       // room should hear about: it stopped when asked, so no 'crashed' line (#198).
       if (entry.state === 'stopped' || entry.stopping === true) return
       const clean = code === 0
-      const detail = signal !== null ? `signal ${signal}` : `exit ${code}`
+      const ending = signal !== null ? `signal ${signal}` : `exit ${code}`
       const tail = clean ? '' : tailOf(entry)
-      const said = tail === '' ? detail : `${detail}: ${tail}`
-      settle(entry, clean ? 'exited' : 'crashed', said)
-      void report(room, member, entry.state, said)
+      // The room hears how the process ended; its last words stay in the log, where the owner reads
+      // them and a seat on another home cannot (#2699).
+      settle(entry, clean ? 'exited' : 'crashed', tail === '' ? ending : `${ending}: ${tail}`)
+      void report(room, member, entry.state, ending, entry.detail)
       entry.stopping = false
     }
     // 'exit' must free the seat at once — otherwise the next turn writes into a dead child and the
@@ -339,7 +342,7 @@ export function createExternalRoomParticipant(rt) {
       entry.child.stdin.write(`${JSON.stringify({ type: 'turn', text, note, ...(by ? { from: by } : {}), exchangeId })}\n`)
     } catch (error) {
       settle(entry, 'crashed', oneLine(error?.message ?? error))
-      void report(room, member, 'crashed', entry.detail)
+      void report(room, member, 'crashed', undefined, entry.detail)
     }
     return done
   }

core/test/images.test.js

@@ -112,6 +112,9 @@ test('#779: the placeholder names the file, its size and its type; without a pat
   const group = imagePlaceholder(attachment, { path: '/p/x', group: true })
   assert.match(group, /\[Image "IMG_2431\.jpeg" \(image\/jpeg, 1080x1596, 213569 bytes\): it could not be read right now\./)
   assert.doesNotMatch(group, /you cannot see images/)
+  // #2699: the group line is fanned out to the room's other homes, so it never names a path of this
+  // host — not even when one is passed in.
+  assert.doesNotMatch(group, /The file is at/, 'a group line is read on the far side: no path of this host')
 
   // An unnamed image keeps the sha prefix the host has always shown.
   assert.match(imagePlaceholder({ attachmentId: id }), /\[Image sha256:bdc77c70:/)

core/test/room-context.test.js

@@ -36,6 +36,25 @@ test('prepare collapses bot status and leaves the stored line and a person line'
   assert.equal(prepareRoomLine(short), short)
 })
 
+test('the store gate runs the host redactor on every line, and leaves a clean one alone (#2699)', () => {
+  // A room line leaves for the room's other homes (`fanoutRoomLine`), so the one gate both writers of
+  // `room.log` pass through is where the host's redactor runs (`rt.redactSecrets`, a stub here).
+  const redact = text => text.replace(/ghp_[A-Za-z0-9]+/g, '[secret:GITHUB_TOKEN]')
+  const person = { seq: 1, who: 'User', text: 'the key is ghp_FAKE2699, keep it here' }
+  const stored = prepareRoomLine(person, { redact })
+  assert.match(stored.text, /\[secret:GITHUB_TOKEN\]/)
+  assert.equal(person.text.includes('ghp_FAKE2699'), true, 'the log line is not rewritten')
+  const bot = { seq: 2, who: 'Dev', botId: 'dev', text: 'ghp_FAKE2699 is what I pasted' }
+  assert.match(prepareRoomLine(bot, { redact }).text, /\[secret:GITHUB_TOKEN\]/)
+  // A bot essay is collapsed after the redaction, so a secret in it is gone whichever end it was on.
+  const essayBot = { seq: 3, who: 'Dev', botId: 'dev', text: essay('ghp_FAKE2699') }
+  assert.doesNotMatch(prepareRoomLine(essayBot, { redact }).text, /ghp_FAKE/)
+  const clean = { seq: 4, who: 'User', text: 'nothing to redact here' }
+  assert.equal(prepareRoomLine(clean, { redact }), clean, 'a line with no secret keeps its identity')
+  // Without the host's redactor (a caller that has none) the line is prepared as before.
+  assert.equal(prepareRoomLine(person), person)
+})
+
 test('the transcript stays inside the budget and prefers a newer short line over an older essay', () => {
   const lines = []
   for (let seq = 1; seq <= 8; seq += 1) lines.push({ seq, who: 'Dev', botId: 'dev', text: essay(`marker-${seq}`) })

core/test/room-external.test.js

@@ -11,10 +11,11 @@ const DIE = 'process.exit(3)'
 
 function fixture(t) {
   const lines = []
+  const warnings = []
   const rt = {
     live: new Map(), groupOwners: new Map(), roomWaiters: new Map(),
     workDirOf: () => undefined,
-    warn: () => {},
+    warn: (...args) => warnings.push(args.join(' ')),
     externalRoomLine: async (room, line) => { lines.push({ roomId: room.id, ...line }); return line },
   }
   const driver = createExternalRoomParticipant(rt)
@@ -25,7 +26,7 @@ function fixture(t) {
     room, member, sessionId: `external:ops:${member.id}`, text, note: 'your turn',
     signal: controller.signal, started: [], by: 'chief', exchangeId: `x-${member.id}`,
   })
-  return { driver, rt, room, lines, run }
+  return { driver, rt, room, lines, warnings, run }
 }
 
 test('an external Bot whose process dies answers with a failure and lands in the room, not in silence (#198)', async (t) => {
@@ -233,20 +234,27 @@ test('the timeout state line says timed out (#1164)', async (t) => {
   assert.doesNotMatch(entry.text, /process timeout/)
 })
 
-test('a crash line carries the last words from stderr (#1164)', async (t) => {
+test('a crash line reports the ending, and the last words stay in this host\'s log (#1164, #2699)', async (t) => {
   const BOOM = "process.stderr.write('boom: missing config\\n',()=>process.exit(3))"
-  const { lines, run } = fixture(t)
+  const { lines, warnings, run } = fixture(t)
   const member = { id: 'boom', name: 'Boom', external: { driver: 'subprocess', command: [process.execPath, '-e', BOOM] } }
   assert.equal((await run(member))?.failure?.reason, 'crashed')
   const entry = await waitForLine(lines, 'boom')
-  assert.match(entry.text, /crashed \(exit 3: boom: missing config\)/)
+  assert.match(entry.text, /crashed \(exit 3\)/)
+  // The room reads how the process ended; its last words name this host's files, and the room's line
+  // is read by the other homes in it (#2699).
+  assert.doesNotMatch(entry.text, /missing config/)
+  assert.ok(warnings.some(line => line.includes('boom: missing config')), 'the log keeps what the child said')
   // The tail is capped: 400 characters of stderr keep only the last 200.
   const NOISE = "process.stderr.write('x'.repeat(400),()=>process.exit(3))"
   const noisy = { id: 'noise', name: 'Noise', external: { driver: 'subprocess', command: [process.execPath, '-e', NOISE] } }
   assert.equal((await run(noisy))?.failure?.reason, 'crashed')
   const noise = await waitForLine(lines, 'noise')
-  assert.match(noise.text, /x{200}/)
-  assert.doesNotMatch(noise.text, /x{201}/)
+  assert.match(noise.text, /crashed \(exit 3\)/)
+  assert.doesNotMatch(noise.text, /x{2,}/)
+  const logged = warnings.find(line => line.includes('xxxxxxxx')) ?? ''
+  assert.match(logged, /x{200}/)
+  assert.doesNotMatch(logged, /x{201}/)
 })
 test('a crash is reported when a grandchild holds the stdio open (#1164)', async (t) => {
   // On the first launch the child leaves a grandchild on its inherited stdio and exits 1, so
@@ -256,14 +264,16 @@ test('a crash is reported when a grandchild holds the stdio open (#1164)', async
   const HELD = "const fs=require('node:fs');const cp=require('node:child_process');if(!fs.existsSync(process.env.MARK)){fs.writeFileSync(process.env.MARK,'1');fs.writeSync(2,'boom: helper holds the pipe\\n');cp.spawn(process.execPath,['-e','setTimeout(()=>{},1200)'],{stdio:'inherit'});process.exit(1)}let b='';process.stdin.setEncoding('utf8');process.stdin.on('data',c=>{b+=c;let i;while((i=b.indexOf('\\n'))!==-1){const l=b.slice(0,i);b=b.slice(i+1);const m=JSON.parse(l);if(m.type==='turn')process.stdout.write(JSON.stringify({text:'again: '+m.text})+'\\n')}})"
   const dir = mkdtempSync(join(tmpdir(), 'flybot-external-'))
   const mark = join(dir, 'mark')
-  const { lines, run } = fixture(t)
+  const { lines, warnings, run } = fixture(t)
   const member = { id: 'held', name: 'Held', external: { driver: 'subprocess', command: [process.execPath, '-e', HELD], env: { MARK: mark } } }
   const started = Date.now()
   const first = run(member)
   // 900 ms is before the grandchild's 1.2 s: only reporting from 'exit' can meet this.
   const entry = await waitForLine(lines, 'held', 900)
   const elapsed = Date.now() - started
-  assert.match(entry.text, /crashed \(exit 1: boom: helper holds the pipe\)/)
+  assert.match(entry.text, /crashed \(exit 1\)/)
+  assert.doesNotMatch(entry.text, /helper holds the pipe/)
+  assert.ok(warnings.some(line => line.includes('helper holds the pipe')), 'the log keeps what the child said')
   assert.ok(elapsed < 900, `the crash line waited for close (${elapsed} ms)`)
   assert.equal((await first)?.failure?.reason, 'crashed')
   // The seat was cleared on 'exit', so the next turn reaches a fresh process, not a dead pipe.