Refuse a decision name that leaves decisions/ and stop project_state following a symlink

changelog.d/2037-decision-name-guard.fixed.md

@@ -0,0 +1 @@
+A decision file name can no longer reach outside the project, and a symlinked Project State file is no longer read ([#2037](https://github.com/ThinkFlowLab/FlyBot/issues/2037)): the board accepted any name that started with `decisions/` and ended in `.md`, so a hand-edited `decisionFile` or a crafted folder name such as `decisions/zz/../../../outside.md` put a file outside the folder in a row — with its `path` and its first line as `title` — and the same name could come back through the `known` fallback past the 200-row cut. One rule now decides a decision name at the point the listing is produced: exactly one segment under `decisions/`, ending in `.md`, used by the listing, the reader and the fallback alike; a symlink is refused too, and `project_state` answers `absent` for a symlinked file instead of handing over the text it points at.

core/src/host/project-board.js

@@ -16,10 +16,10 @@
 // Read-only by construction: it reads `state.tasks`, `state.meetings` and the Project bindings,
 // asks each registered provider for the workflow refs of tasks that still exist, and returns the
 // sections. It writes nothing, starts nothing and is not called unless someone asks for the board.
-import { open, readFile, stat } from 'node:fs/promises'
+import { open } from 'node:fs/promises'
 import { join } from 'node:path'
 import { columnOf } from '../shared/task-board.js'
-import { DECISIONS_DIR, listProjectState, sha256 } from './project-state.js'
+import { DECISIONS_DIR, describeFile, isDecisionPath, listProjectState } from './project-state.js'
 import { cut } from './text.js'
 import { advanceRefusal } from './meetings.js'
 import { readMetrics } from './project-metrics.js'
@@ -247,11 +247,12 @@ export function install(rt) {
   const archivedOf = async (meeting) => {
     const dir = text(meeting?.decisionDir)
     const path = text(meeting?.decisionFile)
-    if (dir === '' || !path.startsWith(`${DECISIONS_DIR}/`) || !path.endsWith('.md')) return undefined
+    // The listing accepts one shape only, and this read is part of it: a name that is not
+    // `decisions/<name>.md`, or a symlink standing in for that file, is refused here too. Reading
+    // it would put a file outside the folder in the rows, and the past-cut fallback reads this set.
+    if (dir === '' || !isDecisionPath(path)) return undefined
     try {
-      const bytes = await readFile(join(dir, path))
-      const info = await stat(join(dir, path))
-      return { dir, path, bytes: bytes.length, sha256: sha256(bytes), mtime: new Date(info.mtimeMs).toISOString() }
+      return { dir, ...await describeFile(dir, path) }
     } catch {
       // Absent or unreadable is not a row: the meeting keeps its own record, with no path.
       return undefined
@@ -274,7 +275,7 @@ export function install(rt) {
       }
       if (state.ok !== true) return { dir, files: [], total: 0, reason: state.reason }
       found.push(...state.files
-        .filter(file => file.path.startsWith(`${DECISIONS_DIR}/`) && file.path.endsWith('.md'))
+        .filter(file => isDecisionPath(file.path))
         .map(file => ({ ...file, dir })))
     }
     // The other source: the folder each decided meeting of this Project recorded, one file each.

core/src/host/project-state.js

@@ -9,7 +9,7 @@
 // file and a file too large to hand over each answer with the reason, never with an empty
 // string that would read as an empty document.
 import { createHash } from 'node:crypto'
-import { readdir, readFile, stat } from 'node:fs/promises'
+import { lstat, readdir, readFile, stat } from 'node:fs/promises'
 import { join } from 'node:path'
 import { defineTool } from './tool-schema.js'
 import { output } from './tools.js'
@@ -22,7 +22,9 @@ export const DECISIONS_DIR = 'decisions'
 // the context it was meant to save. Over the bound the answer is the size, not the text.
 export const MAX_BYTES = 200_000
 
-const isDecisionPath = path => /^decisions\/[^/]+\.md$/.test(path)
+// The one name shape a decision has: exactly one segment under `decisions/`, ending in `.md`.
+// The lister, the reader and the board all take a name only when it matches this.
+export const isDecisionPath = path => /^decisions\/[^/]+\.md$/.test(path)
 const allowed = path => PROJECT_STATE_PATHS.includes(path) || isDecisionPath(path)
 export const sha256 = bytes => createHash('sha256').update(bytes).digest('hex')
 
@@ -48,9 +50,22 @@ async function decisionFiles(dir) {
     .sort()
 }
 
-async function describe(dir, path) {
+// One file sitting directly in the folder, never through a link. `readdir` already reports a
+// symlink as "not a plain file", and reading a name — here, in the reader, and on the board —
+// answers the same way: a symlink is an absent file, not its target. The `lstat` is what makes
+// that one rule hold for a name that was never listed, a name from a record, or `PROJECT.md`.
+async function fileAt(dir, path) {
+  const info = await lstat(join(dir, path))
+  if (!info.isFile()) throw new Error(`not a plain file: ${path}`)
+  return info
+}
+
+// One Project State file with its byte size, content hash and last-modified time. Exported
+// because the board reads the file a decided meeting archived, and it must refuse exactly what
+// this reader refuses — one rule, one place.
+export async function describeFile(dir, path) {
+  const info = await fileAt(dir, path)
   const bytes = await readFile(join(dir, path))
-  const info = await stat(join(dir, path))
   return { path, bytes: bytes.length, sha256: sha256(bytes), mtime: new Date(info.mtimeMs).toISOString() }
 }
 
@@ -66,12 +81,12 @@ export async function listProjectState(dir, { repository = true } = {}) {
   const found = []
   for (const path of PROJECT_STATE_PATHS) {
     try {
-      found.push(await describe(dir, path))
+      found.push(await describeFile(dir, path))
     } catch { /* an absent Project State file is simply not listed */ }
   }
   for (const path of await decisionFiles(dir)) {
     try {
-      found.push(await describe(dir, path))
+      found.push(await describeFile(dir, path))
     } catch { /* a file that vanished between readdir and read is not listed */ }
   }
   return { ok: true, dir, files: found }
@@ -88,11 +103,11 @@ export async function readProjectState(dir, file) {
   if (!await repositoryAt(dir)) return { ok: false, reason: `not a repository: ${dir}` }
   let info
   try {
-    info = await stat(join(dir, path))
+    // A symlink answers like a file that is not there — never with the text it points at.
+    info = await fileAt(dir, path)
   } catch {
     return { ok: false, reason: `absent: ${path}` }
   }
-  if (!info.isFile()) return { ok: false, reason: `absent: ${path}` }
   if (info.size > MAX_BYTES) return { ok: false, reason: `too large: ${path} is ${info.size} bytes (bound ${MAX_BYTES})` }
   const bytes = await readFile(join(dir, path))
   return { ok: true, path, bytes: bytes.length, sha256: sha256(bytes), mtime: new Date(info.mtimeMs).toISOString(), text: bytes.toString('utf8') }

core/test/project-board.test.js

@@ -2,7 +2,7 @@
 // that same id through `run.task.id`. The aggregate reads both views as one row, never twice,
 // and writes nothing back.
 import assert from 'node:assert/strict'
-import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, statSync, writeFileSync } from 'node:fs'
+import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, statSync, symlinkSync, writeFileSync } from 'node:fs'
 import { tmpdir } from 'node:os'
 import { join, resolve } from 'node:path'
 import { test } from 'node:test'
@@ -557,6 +557,127 @@ test('#2033: a decision name that points outside the folder never becomes a path
   }
 })
 
+// A hand-edited record, or a name crafted into a decisions folder, can point `decisionFile`
+// anywhere. The board lists one shape only — a regular `decisions/<name>.md` — and that same rule
+// has to refuse every other name where the listing is produced, so neither a row nor the past-cut
+// fallback reads outside the folder. Each class below is a refusal, never a silent skip.
+const traversalFolder = () => {
+  const base = mkdtempSync(join(tmpdir(), 'ds-bot-decisions-escape-'))
+  const dir = join(base, 'project')
+  mkdirSync(join(dir, '.git'), { recursive: true })
+  mkdirSync(join(dir, 'decisions'))
+  mkdirSync(join(dir, 'decisions', 'zz'))
+  mkdirSync(join(dir, 'decisions', 'sub'))
+  writeFileSync(join(dir, 'decisions', '2026-01-02-legit.md'), '# The real decision\n')
+  writeFileSync(join(dir, 'decisions', 'sub', '2026-01-02-deep.md'), '# Left out of the listing\n')
+  writeFileSync(join(dir, 'decisions', 'handwritten-notes.md'), '# Written by hand\n')
+  writeFileSync(join(base, 'outside.md'), '# Outside the folder\n')
+  symlinkSync(join(base, 'outside.md'), join(dir, 'decisions', '2026-01-02-link.md'))
+  return { base, dir }
+}
+
+const traversalSeed = (dir, base, ids) => {
+  const team = seed()
+  team.projects['project-p1'].workDir = dir
+  const at = (id, decisionFile) => ({
+    id, status: 'decided', question: `Q ${id}`, owner: 'Chief', project: 'project-p1',
+    decisionDir: dir, decisionFile, createdAt: 1, updatedAt: 5, decidedAt: 6,
+  })
+  const all = {
+    'm-dotdot': 'decisions/zz/../../../outside.md',
+    'm-absolute-out': join(base, 'outside.md'),
+    'm-absolute-root': '/etc/passwd',
+    'm-symlink': 'decisions/2026-01-02-link.md',
+    'm-deep': 'decisions/sub/2026-01-02-deep.md',
+    'm-legit': 'decisions/2026-01-02-legit.md',
+  }
+  team.meetings = Object.fromEntries(ids.map(id => [id, at(id, all[id])]))
+  return team
+}
+
+// Each case gets only its own meetings, so a leak in one class cannot make another class red.
+async function startTraversal(ids) {
+  const home = mkdtempSync(join(tmpdir(), 'ds-bot-board-'))
+  const { base, dir } = traversalFolder()
+  const { rt, stop } = await start(home, traversalSeed(dir, base, ids))
+  return { rt, base, dir, stop }
+}
+
+const decidedRow = (board, id) => board.meetings.decided.find(meeting => meeting.id === id)
+
+test('a decision name with a .. segment never becomes a row or a past-cut path', async () => {
+  const { rt, base, stop } = await startTraversal(['m-dotdot'])
+  try {
+    const board = await rt.projectBoard('project-p1')
+    assert.equal(decidedRow(board, 'm-dotdot').path, '', 'a name with a .. segment is refused, not projected')
+    assert.equal(board.decisions.files.some(file => file.path.includes('..')), false, 'no row names a .. path')
+    assert.equal(board.decisions.files.some(file => file.title === 'Outside the folder'), false, 'no row carries the outside title')
+  } finally {
+    await stop()
+    rmSync(base, { recursive: true, force: true })
+  }
+})
+
+test('an absolute decision name is refused, never read', async () => {
+  const { rt, base, stop } = await startTraversal(['m-absolute-out', 'm-absolute-root'])
+  try {
+    const board = await rt.projectBoard('project-p1')
+    assert.equal(decidedRow(board, 'm-absolute-out').path, '', 'an absolute path is refused')
+    assert.equal(decidedRow(board, 'm-absolute-root').path, '', 'and so is a root absolute path')
+    assert.equal('sha256' in decidedRow(board, 'm-absolute-out'), false)
+    assert.equal(board.decisions.files.some(file => file.path.startsWith('/')), false, 'no row names an absolute path')
+  } finally {
+    await stop()
+    rmSync(base, { recursive: true, force: true })
+  }
+})
+
+test('a symlinked decision name is refused, never followed', async () => {
+  const { rt, base, stop } = await startTraversal(['m-symlink'])
+  try {
+    const board = await rt.projectBoard('project-p1')
+    const row = decidedRow(board, 'm-symlink')
+    assert.equal(row.path, '', 'a symlink is not a decision file this listing found')
+    assert.equal('sha256' in row, false, 'and it carries no hash of the file it points at')
+    assert.equal(board.decisions.files.some(file => file.path === 'decisions/2026-01-02-link.md'), false, 'the link is not a row')
+    assert.equal(board.decisions.files.some(file => file.title === 'Outside the folder'), false, 'and its target is never titled')
+  } finally {
+    await stop()
+    rmSync(base, { recursive: true, force: true })
+  }
+})
+
+test('a decision name this listing did not read never becomes a past-cut path', async () => {
+  const { rt, base, stop } = await startTraversal(['m-deep'])
+  try {
+    const board = await rt.projectBoard('project-p1')
+    const row = decidedRow(board, 'm-deep')
+    assert.equal(row.path, '', 'the fallback names only what this read listed, so a name it never read is not a path')
+    assert.equal('sha256' in row, false)
+    assert.equal(board.decisions.files.some(file => file.path === 'decisions/sub/2026-01-02-deep.md'), false, 'the listing reads one level only')
+  } finally {
+    await stop()
+    rmSync(base, { recursive: true, force: true })
+  }
+})
+
+test('a regular decision name is still listed, titled and hashed', async () => {
+  const { rt, base, stop } = await startTraversal(['m-legit'])
+  try {
+    const board = await rt.projectBoard('project-p1')
+    const file = board.decisions.files.find(row => row.path === 'decisions/2026-01-02-legit.md')
+    assert.equal(file.title, 'The real decision', 'a legal name is still listed with its own title')
+    const handwritten = board.decisions.files.find(row => row.path === 'decisions/handwritten-notes.md')
+    assert.equal(handwritten.title, 'Written by hand', 'a legal name with no date is listed too: the rule is the shape, not the date')
+    const row = decidedRow(board, 'm-legit')
+    assert.equal(row.path, 'decisions/2026-01-02-legit.md', 'and the meeting still finds it')
+    assert.match(row.sha256, /^[0-9a-f]{64}$/)
+  } finally {
+    await stop()
+    rmSync(base, { recursive: true, force: true })
+  }
+})
+
 // #2005: a decision file's title is its first heading, bounded at 200 units. A heading whose 200th
 // unit is the first half of an emoji used to reach the board as an unpaired surrogate, so the
 // Decisions section carried half a character.

core/test/project-state.test.js

@@ -1,5 +1,5 @@
 import assert from 'node:assert/strict'
-import { mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'
+import { mkdirSync, mkdtempSync, readFileSync, rmSync, symlinkSync, writeFileSync } from 'node:fs'
 import { tmpdir } from 'node:os'
 import { join } from 'node:path'
 import { test } from 'node:test'
@@ -93,6 +93,32 @@ test('#1702: read one returns its bytes, bounded, and reports a missing file', a
   assert.match(bigRow, /\t200001 bytes\t/, 'a file over the read bound still lists with its size')
 })
 
+// A symlink is a name for a file somewhere else, and this reader answers for one folder. A name
+// that stands for a link is therefore absent — the same answer as a file that is not there — and
+// never the target's text. `readdir` already leaves a link out of the listing, and the read has to
+// agree with it; a name from a record can reach the same file without ever being listed.
+test('#2037: a symlinked Project State file is not followed, it answers as absent', async (t) => {
+  const dir = projectStateFolder()
+  const outside = mkdtempSync(join(tmpdir(), 'ds-bot-project-state-outside-'))
+  writeFileSync(join(outside, 'secret.md'), '# Outside secret\n')
+  const { tool, stop } = await boot({ dir })
+  t.after(async () => {
+    await stop()
+    rmSync(dir, { recursive: true, force: true, maxRetries: 5 })
+    rmSync(outside, { recursive: true, force: true, maxRetries: 5 })
+  })
+
+  symlinkSync(join(outside, 'secret.md'), join(dir, 'decisions/0002-link.md'))
+  assert.equal(await tool({ file: 'decisions/0002-link.md' }), 'absent: decisions/0002-link.md', 'a link answers like a missing file, never with the text it points at')
+
+  rmSync(join(dir, 'PROJECT.md'))
+  symlinkSync(join(outside, 'secret.md'), join(dir, 'PROJECT.md'))
+  assert.equal(await tool({ file: 'PROJECT.md' }), 'absent: PROJECT.md', 'the same answer for a named Project State file')
+  const lines = (await tool({})).split('\n')
+  assert.equal(lines.some(line => line.startsWith('PROJECT.md\t')), false, 'a link is not listed either')
+  assert.equal(lines.some(line => line.startsWith('decisions/0002-link.md\t')), false, 'and neither is a linked decision')
+})
+
 test('#1702: a folder with no repository answers not a repository', async (t) => {
   const dir = mkdtempSync(join(tmpdir(), 'ds-bot-project-state-nogit-'))
   writeFileSync(join(dir, 'PROJECT.md'), '# Project\n')