fix(security): approval binding, chip authorship and task lifecycle follow-ups (#97 #106 #111)

changelog.d/106-chip-resolution.fixed.md

@@ -0,0 +1 @@
+Chat-reference chips are author-checked on answer-question and schedule-create as well as send and room-post, schedule-create authorization keys off the Bot or room the endpoint uses, and Bot-authored group posts drop chips so a member Bot cannot tunnel a private chat into a shared room. ([#106](https://github.com/ThinkFlowLab/FlyBot/issues/106))

changelog.d/111-task-lifecycle.fixed.md

@@ -0,0 +1 @@
+A held send blocks only the named working task (or that Bot's only working task), a task waiting for your acceptance is not pruned, and task channel names are an explicit allow-list rather than any `task-*` prefix. ([#111](https://github.com/ThinkFlowLab/FlyBot/issues/111))

changelog.d/97-approval-binding.fixed.md

@@ -0,0 +1 @@
+Held-send invalidation matches the contract: a hold addressed to a replaced Chief stays pending and decidable, unknown `invalidateFor` fields throw, and an in-flight approve is re-checked before dispatch so a concurrent Chief replacement cannot send on the old verdict. ([#97](https://github.com/ThinkFlowLab/FlyBot/issues/97))

core/src/host/authz.js

@@ -49,6 +49,12 @@ const ROOM_TARGETED = new Set(['update-room', 'delete-room', 'room-progress', 'c
 // Endpoints whose body names a Session.
 const SESSION_TARGETED = new Set(['earlier', 'send', 'answer-question', 'dismiss-question', 'approval-answer', 'exchange'])
 
+// Task channel names. Requester checks live on the record in tasks.js; listing them
+// here keeps a future `task-*` from auto-allowing for any signed-in actor (#111).
+const TASK_ENDPOINTS = new Set([
+  'tasks', 'task-create', 'task-accept', 'task-start', 'task-return', 'task-accept-result', 'task-cancel',
+])
+
 const deny = (code, message) => ({ ok: false, code, message })
 const ALLOW = { ok: true }
 
@@ -217,6 +223,16 @@ export function install(rt) {
 
     if (BOT_TARGETED.has(ep)) {
       if (ep === 'schedules' || ep === 'schedule-create' || ep === 'schedule-delete') {
+        // The create endpoint keys off `body.target` (Bot or room id); `sessionId` is
+        // the list/delete key. Checking only sessionId would allow a foreign target
+        // paired with an owned session (#106).
+        if (ep === 'schedule-create') {
+          const target = body?.target
+          if (typeof target === 'string' && target !== '') {
+            if (botOwnedBy(actor, target) || roomOwnedBy(actor, target) || roomMemberOf(actor, target)) return ALLOW
+            return deny('bot/forbidden', 'You do not own that Bot or group chat')
+          }
+        }
         if (ep === 'schedule-delete' && typeof body?.id === 'string' && !sessionOwnedBy(actor, body.id) && !sessionOwnedBy(actor, body.sessionId)) return deny('bot/forbidden', 'You do not own that schedule')
         return requireSession(body?.sessionId)
       }
@@ -327,8 +343,9 @@ export function install(rt) {
     if (ep === 'share-brief') return ALLOW
     // Task transitions are requester-checked on the record in tasks.js (#54) — the
     // endpoint only needs a signed-in actor, like held-send decisions. Create is
-    // grant-aware there (owner, matching grant, or the named room).
-    if (ep === 'tasks' || ep.startsWith('task-')) return ALLOW
+    // grant-aware there (owner, matching grant, or the named room). Explicit set:
+    // a future `task-*` name must be listed here, not auto-allowed (#111).
+    if (TASK_ENDPOINTS.has(ep)) return ALLOW
 
     // Anything not in the table and not a known auth/read endpoint is denied.
     if (NO_ACTOR_ALLOWED.has(ep) || ep.startsWith('auth-')) return ALLOW

core/src/host/channel.js

@@ -119,11 +119,18 @@ export function install(rt) {
   const READS = new Set(['state', 'models', 'efforts', 'earlier', 'room-progress', 'room-history', 'exchange', 'usage', 'schedules', 'memory-view', 'browser-wait', 'browser-result', 'diagnostics', 'diagnostics-log', 'diagnostics-clear', 'update-check', 'update-install', 'dsh-update-install', 'repo-stars', 'approval-answer', 'auth-whoami'])
 
   // A chat reference chip names a Session whose snapshot rides to every recipient of
-  // the message, so the author must be able to read what they share (#53).
+  // the message, so the author must be able to read what they share (#53, #106).
+  // Actor-authored text that becomes a session user message: send, room-post,
+  // answer-question, schedule-create. Bot-authored room posts strip chips in groups.js.
+  // Menu chips encode the session id as base64url(JSON); raw ids still appear in tests.
+  const decodeSessionChip = (payload) => {
+    try { return JSON.parse(Buffer.from(payload, 'base64url').toString('utf8')) } catch { return undefined }
+  }
   const unshareableReference = (actor, text) => {
     if (!actor || !rt.authzEnforced?.() || typeof text !== 'string') return undefined
     for (const match of text.matchAll(/dsh-session:([A-Za-z0-9_-]+)/g)) {
-      if (!rt.sessionOwnedBy?.(actor, match[1])) return match[1]
+      const id = decodeSessionChip(match[1]) ?? match[1]
+      if (!rt.sessionOwnedBy?.(actor, id)) return id
     }
     return undefined
   }
@@ -318,10 +325,15 @@ export function install(rt) {
           await rt.load()
           return ok(await rt.capabilityEndpoint(endpoint, body))
         case 'schedules':
-        case 'schedule-create':
         case 'schedule-delete':
           await rt.load()
           return ok(await rt.scheduleEndpoint(endpoint, body))
+        case 'schedule-create': {
+          await rt.load()
+          const leaked = unshareableReference(actor, [body.title, body.prompt].filter(part => typeof part === 'string').join('\n'))
+          if (leaked !== undefined) return fail('You do not own that chat', 'bot/forbidden')
+          return ok(await rt.scheduleEndpoint(endpoint, body))
+        }
         case 'memory-view':
         case 'memory-ask':
         case 'memory-forget':
@@ -336,6 +348,8 @@ export function install(rt) {
           if (question === undefined || question.id !== body.questionId) return fail('This question is no longer open', 'bot/stale')
           const reply = questionReply(question, body.answer)
           if (reply === undefined) return fail('Nothing to send', 'bot/invalid')
+          const leaked = unshareableReference(actor, reply.text)
+          if (leaked !== undefined) return fail('You do not own that chat', 'bot/forbidden')
           // Claim the question before queueing so a second answer is refused.
           delete state.questions[sessionId]
           try {

core/src/host/groups.js

@@ -28,6 +28,23 @@ const ROOM_LOG_MAX = 60
 const ROOM_FIRST_LOOK = 12
 const ROOM_DELTA_MAX = 24
 const ROOM_EARLIER_MAX = 8
+// Bot-authored group followups are createUserMessage; DSH may resolve `dsh-session:`
+// chips there and attach the snapshot to the last user post. Drop chips so a member
+// Bot cannot tunnel a private chat into the room (#106). Authorship stays the gate.
+function stripSessionChips(text) {
+  if (typeof text !== 'string' || !text.includes('dsh-session:')) return text
+  // Loop until stable: a nested marker like dsh-sessiondsh-session:x:ImQi must not
+  // reconstitute a valid chip after one pass (#106 Codex review).
+  let out = text
+  for (let previous; previous !== out;) {
+    previous = out
+    out = out
+      .replace(/@\[((?:\\.|[^\]\\])*)\]\(dsh-session:[A-Za-z0-9_-]+\)/g, (_, label) => `@${String(label).replace(/\\([\\\]])/g, '$1')}`)
+      .replace(/dsh-session:[A-Za-z0-9_-]+/g, '')
+  }
+  if (out.includes('dsh-session:')) out = out.replace(/dsh-session:/g, '')
+  return out
+}
 const ROOM_EXTRA_TURNS = 4
 const ROOM_ADMIN_EXTRA_TURNS = 4
 const ROOM_TURNS_EACH = 2
@@ -601,7 +618,7 @@ export function install(rt) {
   async function postToRoom(room, sender, text, extra = {}) {
     const agent = await rt.resolveAgent(room.id)
     agent.followup(createUserMessage({
-      content: [{ type: 'text', text: `[Group post from ${sender.name}]\n${text}` }],
+      content: [{ type: 'text', text: `[Group post from ${sender.name}]\n${stripSessionChips(text)}` }],
       source: { kind: SOURCE_KIND, form: 'relay', role: 'post', exchangeId: randomUUID(), senderName: sender.name, senderSessionId: sender.id, roomId: room.id, hop: 0, ...extra },
     }))
     await ctx.sessions.flush(agent.session)

core/src/host/hooks.js

@@ -328,10 +328,17 @@ export function install(rt) {
   // Queued work whose authority is gone does not wait forever: a hold naming one of
   // these Bots as sender or target is marked stale (still listed for the audit view).
   // `field` narrows the match ('from': sender-side only, e.g. a demoted Chief).
+  // Unknown values must throw: a silent no-op would skip invalidation (#97).
   const invalidateFor = async (botIds, reason, field) => {
+    if (field !== undefined && field !== 'from') {
+      throw new Error(`flybot: invalidateFor field must be 'from' or omitted, got ${JSON.stringify(field)}`)
+    }
     await rt.load()
     const ids = new Set(botIds)
-    const hit = Object.values(store()).filter(a => a.status === 'pending'
+    // Pending, and approved-but-not-yet-dispatched: otherwise a concurrent setMain /
+    // deleteBot during release() skips the record (status already `approved`) (#97).
+    const inFlight = a => a.status === 'pending' || (a.status === 'approved' && a.exchangeId === undefined)
+    const hit = Object.values(store()).filter(a => inFlight(a)
       && (ids.has(a.request?.from) && (field === undefined || field === 'from')
         || ids.has(a.request?.to) && field === undefined))
     if (hit.length === 0) return
@@ -415,6 +422,15 @@ export function install(rt) {
       }
       return
     }
+    // Re-check immediately before dispatch: evaluate() yielded, so a concurrent
+    // setMain/deleteBot may have marked this in-flight approve stale (#97).
+    if (approval.status === 'stale') return
+    const still = staleReason(approval)
+    if (still !== undefined) {
+      approval.status = 'stale'
+      approval.error = still
+      return
+    }
     try {
       approval.exchangeId = await dispatch(approval.request)
     } catch (error) {

core/src/host/tasks.js

@@ -17,6 +17,8 @@ import { randomBytes } from 'node:crypto'
 import { currentParticipants } from './room-membership.js'
 
 const ACTIVE = new Set(['open', 'claimed', 'working', 'blocked'])
+// result-ready still waits for the requester: prune must not evict it with terminals (#111).
+const PROTECTED = new Set([...ACTIVE, 'result-ready'])
 const TRANSITIONS = {
   open: ['claimed', 'cancelled', 'failed'],
   claimed: ['working', 'cancelled', 'failed'],
@@ -79,17 +81,25 @@ export function install(rt) {
   const view = task => ({ ...task, actors: [...task.actors ?? []] })
   const prune = () => {
     const all = Object.values(store())
-    const terminal = all.filter(task => !ACTIVE.has(task.status)).sort((a, b) => a.updatedAt - b.updatedAt)
-    for (const task of terminal.slice(0, Math.max(0, all.length - KEPT))) delete store()[task.id]
+    const droppable = all.filter(task => !PROTECTED.has(task.status)).sort((a, b) => a.updatedAt - b.updatedAt)
+    for (const task of droppable.slice(0, Math.max(0, all.length - KEPT))) delete store()[task.id]
   }
   const save = () => rt.save()
 
   // ---------------------------------------------------------------- host hooks
   // A consequential send from the responsible Bot was held (#52 card): the task
-  // blocks on that approval until the owner decides.
+  // blocks on that approval until the owner decides. Link one task, not every
+  // working task of that Bot: `meta.taskId` names it, else the Bot's only working
+  // task (several without a name stay working — they are not this send's) (#111).
   rt.taskHeld = (msg, approval) => {
-    for (const task of Object.values(store())) {
-      if (task.status !== 'working' || task.ownerBotId !== msg.from) continue
+    const namedId = typeof msg?.meta?.taskId === 'string' ? msg.meta.taskId : undefined
+    const named = namedId ? store()[namedId] : undefined
+    const ofSender = task => task.ownerBotId === msg.from || task.workBotId === msg.from
+    const working = Object.values(store()).filter(task => task.status === 'working' && ofSender(task))
+    const targets = named && named.status === 'working' && ofSender(named)
+      ? [named]
+      : working.length === 1 ? working : []
+    for (const task of targets) {
       task.approvalId = approval.id
       transition(task, 'blocked', null, { approvalId: approval.id })
     }
@@ -135,6 +145,7 @@ export function install(rt) {
           updatedAt: Date.now(),
           requesterId,
           ownerBotId: botId,
+          // workBotId is the specialist on the exchange; assigned with #55 wiring (#111).
           title: String(body.title ?? '').slice(0, 300),
           ...(typeof body.criteria === 'string' && body.criteria.trim() !== '' ? { criteria: body.criteria.slice(0, 2000) } : {}),
           origin: { ...(roomId ? { roomId } : {}), ...(body.sessionId ? { sessionId: String(body.sessionId) } : {}) },

core/test/approval-binding.test.js

@@ -251,6 +251,47 @@ test('replacing a Chief invalidates the held sends written under it', async (t)
   assert.equal(approval.status, 'stale', JSON.stringify(approval))
 })
 
+test('replacing a Chief leaves holds addressed to it pending and decidable', async (t) => {
+  const { fb, call, stop, received } = await start()
+  t.after(stop)
+  const alice = await signIn(call, 'Alice')
+  holdFlagged(fb)
+  // Sender-side only: a hold *to* the demoted Chief is still the sender's owner's to spend.
+  const id = await heldId(fb, { from: 'b', to: 'a', text: 'ask the chief', meta: { hold: true } })
+  const swapped = await call('set-main', { id: 'b', main: true, replace: 'a' }, alice.sessionToken)
+  assert.equal(swapped.ok, true, JSON.stringify(swapped))
+  const list = await call('flybot/approvals', { status: ['pending', 'failed', 'stale'] }, alice.sessionToken)
+  assert.equal(list.ok, true)
+  const approval = list.value.find(entry => entry.id === id)
+  assert.equal(approval.status, 'pending', JSON.stringify(approval))
+  const decided = await call('flybot/decide', { id, decision: 'approve' }, alice.sessionToken)
+  assert.equal(decided.ok, true, JSON.stringify(decided))
+  assert.equal(decided.value.status, 'approved', JSON.stringify(decided.value))
+  assert.deepEqual(received('a'), ['[Message from Writer]\nask the chief'])
+})
+
+test('an in-flight approve is invalidated if the Chief is replaced before dispatch', async (t) => {
+  const { fb, call, stop, received } = await start()
+  t.after(stop)
+  const alice = await signIn(call, 'Alice')
+  holdFlagged(fb)
+  const id = await heldId(fb, { from: 'a', to: 'c', text: 'still in flight', meta: { hold: true } })
+  // After the hold middleware, release re-evaluates the rest of the chain. Swap the
+  // Chief there so invalidateFor races the already-approved record (#97 TOCTOU).
+  fb.bot.send.use({
+    name: 'swap-chief',
+    handle: async () => {
+      const swapped = await call('set-main', { id: 'b', main: true, replace: 'a' }, alice.sessionToken)
+      assert.equal(swapped.ok, true, JSON.stringify(swapped))
+      return undefined
+    },
+  })
+  const decided = await call('flybot/decide', { id, decision: 'approve' }, alice.sessionToken)
+  assert.equal(decided.ok, true, JSON.stringify(decided))
+  assert.equal(decided.value.status, 'stale', JSON.stringify(decided.value))
+  assert.equal(received('c').length, 0)
+})
+
 test('a member whose sender Bot is gone cannot decide leftovers with an old session', async (t) => {
   const { fb, call, stop, statePath } = await start()
   t.after(stop)

core/test/authz.test.js

@@ -195,6 +195,35 @@ test('a member reaches their own Bots sessions, groups and schedules', async (t)
   // A group Session follows its room's rule; so does a schedule on it.
   assert.equal(rt.authorize(actor, 'send', { sessionId: 'gs-a1' }).ok, true)
   assert.equal(rt.authorize(actor, 'schedule-create', { sessionId: 'gs-a1' }).ok, true)
+  // The create endpoint keys off `target` (Bot or room id), not sessionId (#106).
+  assert.equal(rt.authorize(actor, 'schedule-create', { target: 'a1' }).ok, true)
+  assert.equal(rt.authorize(actor, 'schedule-create', { target: 'ra' }).ok, true)
+})
+
+test('schedule-create cannot hide a foreign target behind an owned sessionId', async (t) => {
+  const { rt } = runtime(t, TEAM)
+  await rt.load()
+  const sessionA = await redeemOwner(rt)
+  await redeemOwner(rt)
+  rt.state.bots.a1 = { id: 'a1', sessionId: 'chat-a1', name: 'A', instructions: '', createdAt: 2, ownerId: 'owner-1' }
+  rt.state.bots.b1 = { id: 'b1', sessionId: 'chat-b1', name: 'B', instructions: '', createdAt: 3, ownerId: 'owner-2' }
+  const actorA = await rt.authenticate(sessionA.sessionToken)
+  assert.equal(rt.authorize(actorA, 'schedule-create', { target: 'b1' }).ok, false)
+  assert.equal(rt.authorize(actorA, 'schedule-create', { target: 'b1', sessionId: 'chat-a1' }).code, 'bot/forbidden')
+  assert.equal(rt.authorize(actorA, 'schedule-create', { target: 'a1' }).ok, true)
+})
+
+test('task endpoints are an explicit allow-list, not a task-* prefix', async (t) => {
+  const { rt } = runtime(t, TEAM)
+  await rt.load()
+  const sessionA = await redeemOwner(rt)
+  const actorA = await rt.authenticate(sessionA.sessionToken)
+  for (const ep of ['tasks', 'task-create', 'task-accept', 'task-start', 'task-return', 'task-accept-result', 'task-cancel']) {
+    assert.equal(rt.authorize(actorA, ep, {}).ok, true, ep)
+  }
+  const unknown = rt.authorize(actorA, 'task-explode', {})
+  assert.equal(unknown.ok, false, JSON.stringify(unknown))
+  assert.equal(unknown.code, 'bot/not-found')
 })
 
 test('approval-answer resolves the approval, not caller-chosen fields', async (t) => {