From b88a23c2f858e50cd1b7ce0333e51267aca42ab5 Mon Sep 17 00:00:00 2001 From: Day Dreamer <87640074+HugoXK@users.noreply.github.com> Date: Mon, 2 Nov 2026 00:59:09 +0800 Subject: [PATCH] Refuse a journal folder that is a symlink The prologue of openJournal() created and chmod-ed the folder above /state.jsonl before looking at anything. When that folder was a symlink, all of it followed the link: the chmod changed a foreign directory's mode to 0700 and every later append, temp file and rename landed inside it. The leaf rule at the journal file was the only check. Ask #2055's shared real-dir helper about the folder first and refuse anything but a plain directory, so recording stops with one warning exactly like every other journal problem, before the mkdir, the chmod or any write. A folder that does not exist yet is still created, as before, and a real folder records unchanged; saving the team never depends on the journal. Fixes #2056 --- .../2056-journal-folder-symlink.fixed.md | 1 + core/src/host/journal.js | 12 +++- core/test/state-journal.test.js | 72 ++++++++++++++++++- docs/subsystems/core-host.md | 2 +- patches/ds-bot/files/src/host/journal.js | 12 +++- .../ds-bot/files/test/state-journal.test.js | 72 ++++++++++++++++++- 6 files changed, 164 insertions(+), 7 deletions(-) create mode 100644 changelog.d/2056-journal-folder-symlink.fixed.md diff --git a/changelog.d/2056-journal-folder-symlink.fixed.md b/changelog.d/2056-journal-folder-symlink.fixed.md new file mode 100644 index 000000000..413dd9209 --- /dev/null +++ b/changelog.d/2056-journal-folder-symlink.fixed.md @@ -0,0 +1 @@ +A journal folder that is a symlink now records nothing, instead of letting the host change the permissions of the folder it points at and write the team's state journal there ([#2056](https://github.com/ThinkFlowLab/FlyBot/issues/2056)): the folder that holds `/journal/state.jsonl` must be a real directory, checked before anything is created, permissions changed or written, so a link to a folder outside the home stops recording with one warning the same way any other journal problem does. A real journal folder records exactly as before. diff --git a/core/src/host/journal.js b/core/src/host/journal.js index b61406d26..8d22cbd26 100644 --- a/core/src/host/journal.js +++ b/core/src/host/journal.js @@ -16,6 +16,7 @@ // not match what the earlier lines built refuses the whole journal (no partial team). import { chmod, lstat, mkdir, readFile, rename, unlink, writeFile, appendFile } from 'node:fs/promises' import { dirname } from 'node:path' +import { dirKind } from './real-dir.js' export const JOURNAL_MAX_BYTES = 20 * 1024 * 1024 export const JOURNAL_MAX_AGE_MS = 7 * 24 * 60 * 60 * 1000 @@ -239,8 +240,15 @@ export function openJournal({ path, warn, now = Date.now, maxBytes = JOURNAL_MAX } const open = async () => { - await mkdir(dirname(path), { recursive: true, mode: 0o700 }) - await chmod(dirname(path), 0o700) + // The folder is this host's own bookkeeping too: a name that stands for a link would point + // the mkdir and chmod below, and every later write, at a folder the team never named. Only a + // real directory is this host's own -- a link, a plain file or a check that could not run + // refuses here, before anything is created, chmod-ed or written; no folder yet is still this + // writer's to create, as it was before (#2055's rule, #2056). + const folder = dirname(path) + if (await dirKind(folder) === 'other') throw new JournalError('the journal folder is not a plain directory') + await mkdir(folder, { recursive: true, mode: 0o700 }) + await chmod(folder, 0o700) let info try { info = await lstat(path) diff --git a/core/test/state-journal.test.js b/core/test/state-journal.test.js index 01e26ddc2..8ac46ad33 100644 --- a/core/test/state-journal.test.js +++ b/core/test/state-journal.test.js @@ -1,7 +1,7 @@ import assert from 'node:assert/strict' import { spawnSync } from 'node:child_process' import { randomUUID } from 'node:crypto' -import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, statSync, writeFileSync } from 'node:fs' +import { chmodSync, existsSync, lstatSync, mkdirSync, mkdtempSync, readdirSync, readFileSync, rmSync, statSync, symlinkSync, writeFileSync } from 'node:fs' import { tmpdir } from 'node:os' import { join } from 'node:path' import { test } from 'node:test' @@ -273,3 +273,73 @@ test('the dev replay script rebuilds a team to a seq, writes it 0600 and refuses assert.match(bad.stderr, /refused/) rmSync(home, { recursive: true, force: true }) }) + +// The folder above the journal is the host's own bookkeeping: a name that stands for a link +// points the journal at a folder the team never named, and the `mkdir`/`chmod` prologue would +// follow it. A folder that is not a real directory is therefore absent, like every other +// journal problem: one warning, recording stops, nothing is created or changed through the link. +test('a journal folder that is a symlink to a real folder outside the home records nothing and stays silent on disk (#2056)', async () => { + const home = tempHome() + const outside = mkdtempSync(join(tmpdir(), 'ds-bot-journal-outside-')) + chmodSync(outside, 0o755) + symlinkSync(outside, join(home, 'journal')) + const path = join(home, 'journal', 'state.jsonl') + const warnings = [] + const journal = openJournal({ path, warn: (...args) => warnings.push(args.join(' ')) }) + await journal.record(stateText({ version: 2, revision: 1 })) + await journal.record(stateText({ version: 2, revision: 2 })) + assert.equal(warnings.length, 1, 'the folder is refused once, not once per save') + assert.match(warnings[0], /not recording/) + if (posix) assert.equal(statSync(outside).mode & 0o777, 0o755, 'the folder the link points at keeps its mode') + assert.deepEqual(readdirSync(outside), [], 'nothing is written into the folder the link points at') + rmSync(home, { recursive: true, force: true }) + rmSync(outside, { recursive: true, force: true }) +}) + +test('a journal folder that is a symlink to a missing path records nothing (#2056)', async () => { + const home = tempHome() + const missing = join(home, 'not-created') + symlinkSync(missing, join(home, 'journal')) + const path = join(home, 'journal', 'state.jsonl') + const warnings = [] + const journal = openJournal({ path, warn: (...args) => warnings.push(args.join(' ')) }) + await journal.record(stateText({ version: 2, revision: 1 })) + assert.equal(warnings.length, 1) + assert.match(warnings[0], /not recording/) + assert.equal(existsSync(missing), false, 'the missing target is not created') + assert.equal(existsSync(path), false) + rmSync(home, { recursive: true, force: true }) +}) + +test('a journal folder that is a plain file records nothing (#2056)', async () => { + const home = tempHome() + writeFileSync(join(home, 'journal'), 'not a folder\n') + const path = join(home, 'journal', 'state.jsonl') + const warnings = [] + const journal = openJournal({ path, warn: (...args) => warnings.push(args.join(' ')) }) + await journal.record(stateText({ version: 2, revision: 1 })) + assert.equal(warnings.length, 1) + assert.match(warnings[0], /not recording/) + assert.equal(existsSync(path), false) + assert.equal(readFileSync(join(home, 'journal'), 'utf8'), 'not a folder\n', 'the plain file is left as it was') + rmSync(home, { recursive: true, force: true }) +}) + +test('a real journal folder still records (#2056)', async () => { + const home = tempHome() + mkdirSync(join(home, 'journal')) + const path = join(home, 'journal', 'state.jsonl') + const warnings = [] + const journal = openJournal({ path, warn: (...args) => warnings.push(args.join(' ')) }) + await journal.record(stateText({ version: 2, n: 1 })) + await journal.record(stateText({ version: 2, n: 2 })) + assert.deepEqual(warnings, []) + const text = readFileSync(path, 'utf8') + assert.equal(stateText(replayTo(text)), stateText({ version: 2, n: 2 })) + assert.equal(lstatSync(path).isSymbolicLink(), false) + if (posix) { + assert.equal(statSync(join(home, 'journal')).mode & 0o777, 0o700) + assert.equal(statSync(path).mode & 0o777, 0o600) + } + rmSync(home, { recursive: true, force: true }) +}) diff --git a/docs/subsystems/core-host.md b/docs/subsystems/core-host.md index d5e08af41..be650ca2f 100644 --- a/docs/subsystems/core-host.md +++ b/docs/subsystems/core-host.md @@ -554,7 +554,7 @@ A local, replayable record of every change to the saved team, for multi-Bot bugs - **On.** `stateJournal: true` in the `bot` config. Off (the default) nothing is opened or written, so every other test runs as before. - **What.** `src/host/journal.js`. After each successful `state.json` write, `store.js` hands the exact snapshot text to the journal, which appends the difference from the last recorded snapshot to `/journal/state.jsonl` (file mode 0600, folder 0700). Lines are `{ seq, t, op, key, prev, next, botId?, actor }`; `op` is `base` (the whole team: the first line, and any change only a key reorder explains), `set`, `delete`, or `append` (`prevLength`, `drop`, the new items: a group log or other list that grew or slid). Paths go at most three keys deep (`bots..`); `botId` is set on a `bots.` path; `actor` is `host` for a save and `retention` for a folded base (per-call attribution is not recorded). Only the host that holds the state lock writes, so a read-only window records nothing. - **Retention.** When a write would pass 20 MB, or the oldest change is older than 7 days, the oldest lines are dropped down to 90% of each cap and folded into the base line (atomic rewrite), so the kept lines still replay. A team that alone exceeds the cap stops recording. -- **Fail closed.** Replay (`replayTo(text, seq)`) refuses the whole journal on an unreadable or incomplete line, an unknown op, a gap in `seq`, a first line that is not a base, or a `prev` that does not match what the earlier lines built; a `seq` outside the file is refused, not approximated. On start the writer replays the existing file the same way; a journal that does not replay, a path that is not a plain file, or a failed write stops recording for that run with a warning and leaves the file as it is. Saving the team never depends on the journal. +- **Fail closed.** Replay (`replayTo(text, seq)`) refuses the whole journal on an unreadable or incomplete line, an unknown op, a gap in `seq`, a first line that is not a base, or a `prev` that does not match what the earlier lines built; a `seq` outside the file is refused, not approximated. On start the writer replays the existing file the same way; a journal that does not replay, a path that is not a plain file, or a failed write stops recording for that run with a warning and leaves the file as it is. Saving the team never depends on the journal. The folder above the journal must be a real directory too (#2056): a link, a plain file or a check that cannot run refuses before the `mkdir`/`chmod` that would follow it, so neither the permissions nor a write reach the folder a link names; a folder that does not exist yet is still this writer's to create, as before. - **Replay.** `core/scripts/replay.mjs` (dev-only, not in the package, see [install](install.md)) prints or writes (`--out`, new file only, 0600) the team after a given `--to `; it makes no network or process calls. Test: `core/test/state-journal.test.js` (a 3-Bot round replays byte-identical at every step). ### Standing tool approvals (#540) diff --git a/patches/ds-bot/files/src/host/journal.js b/patches/ds-bot/files/src/host/journal.js index b61406d26..8d22cbd26 100644 --- a/patches/ds-bot/files/src/host/journal.js +++ b/patches/ds-bot/files/src/host/journal.js @@ -16,6 +16,7 @@ // not match what the earlier lines built refuses the whole journal (no partial team). import { chmod, lstat, mkdir, readFile, rename, unlink, writeFile, appendFile } from 'node:fs/promises' import { dirname } from 'node:path' +import { dirKind } from './real-dir.js' export const JOURNAL_MAX_BYTES = 20 * 1024 * 1024 export const JOURNAL_MAX_AGE_MS = 7 * 24 * 60 * 60 * 1000 @@ -239,8 +240,15 @@ export function openJournal({ path, warn, now = Date.now, maxBytes = JOURNAL_MAX } const open = async () => { - await mkdir(dirname(path), { recursive: true, mode: 0o700 }) - await chmod(dirname(path), 0o700) + // The folder is this host's own bookkeeping too: a name that stands for a link would point + // the mkdir and chmod below, and every later write, at a folder the team never named. Only a + // real directory is this host's own -- a link, a plain file or a check that could not run + // refuses here, before anything is created, chmod-ed or written; no folder yet is still this + // writer's to create, as it was before (#2055's rule, #2056). + const folder = dirname(path) + if (await dirKind(folder) === 'other') throw new JournalError('the journal folder is not a plain directory') + await mkdir(folder, { recursive: true, mode: 0o700 }) + await chmod(folder, 0o700) let info try { info = await lstat(path) diff --git a/patches/ds-bot/files/test/state-journal.test.js b/patches/ds-bot/files/test/state-journal.test.js index 01e26ddc2..8ac46ad33 100644 --- a/patches/ds-bot/files/test/state-journal.test.js +++ b/patches/ds-bot/files/test/state-journal.test.js @@ -1,7 +1,7 @@ import assert from 'node:assert/strict' import { spawnSync } from 'node:child_process' import { randomUUID } from 'node:crypto' -import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, statSync, writeFileSync } from 'node:fs' +import { chmodSync, existsSync, lstatSync, mkdirSync, mkdtempSync, readdirSync, readFileSync, rmSync, statSync, symlinkSync, writeFileSync } from 'node:fs' import { tmpdir } from 'node:os' import { join } from 'node:path' import { test } from 'node:test' @@ -273,3 +273,73 @@ test('the dev replay script rebuilds a team to a seq, writes it 0600 and refuses assert.match(bad.stderr, /refused/) rmSync(home, { recursive: true, force: true }) }) + +// The folder above the journal is the host's own bookkeeping: a name that stands for a link +// points the journal at a folder the team never named, and the `mkdir`/`chmod` prologue would +// follow it. A folder that is not a real directory is therefore absent, like every other +// journal problem: one warning, recording stops, nothing is created or changed through the link. +test('a journal folder that is a symlink to a real folder outside the home records nothing and stays silent on disk (#2056)', async () => { + const home = tempHome() + const outside = mkdtempSync(join(tmpdir(), 'ds-bot-journal-outside-')) + chmodSync(outside, 0o755) + symlinkSync(outside, join(home, 'journal')) + const path = join(home, 'journal', 'state.jsonl') + const warnings = [] + const journal = openJournal({ path, warn: (...args) => warnings.push(args.join(' ')) }) + await journal.record(stateText({ version: 2, revision: 1 })) + await journal.record(stateText({ version: 2, revision: 2 })) + assert.equal(warnings.length, 1, 'the folder is refused once, not once per save') + assert.match(warnings[0], /not recording/) + if (posix) assert.equal(statSync(outside).mode & 0o777, 0o755, 'the folder the link points at keeps its mode') + assert.deepEqual(readdirSync(outside), [], 'nothing is written into the folder the link points at') + rmSync(home, { recursive: true, force: true }) + rmSync(outside, { recursive: true, force: true }) +}) + +test('a journal folder that is a symlink to a missing path records nothing (#2056)', async () => { + const home = tempHome() + const missing = join(home, 'not-created') + symlinkSync(missing, join(home, 'journal')) + const path = join(home, 'journal', 'state.jsonl') + const warnings = [] + const journal = openJournal({ path, warn: (...args) => warnings.push(args.join(' ')) }) + await journal.record(stateText({ version: 2, revision: 1 })) + assert.equal(warnings.length, 1) + assert.match(warnings[0], /not recording/) + assert.equal(existsSync(missing), false, 'the missing target is not created') + assert.equal(existsSync(path), false) + rmSync(home, { recursive: true, force: true }) +}) + +test('a journal folder that is a plain file records nothing (#2056)', async () => { + const home = tempHome() + writeFileSync(join(home, 'journal'), 'not a folder\n') + const path = join(home, 'journal', 'state.jsonl') + const warnings = [] + const journal = openJournal({ path, warn: (...args) => warnings.push(args.join(' ')) }) + await journal.record(stateText({ version: 2, revision: 1 })) + assert.equal(warnings.length, 1) + assert.match(warnings[0], /not recording/) + assert.equal(existsSync(path), false) + assert.equal(readFileSync(join(home, 'journal'), 'utf8'), 'not a folder\n', 'the plain file is left as it was') + rmSync(home, { recursive: true, force: true }) +}) + +test('a real journal folder still records (#2056)', async () => { + const home = tempHome() + mkdirSync(join(home, 'journal')) + const path = join(home, 'journal', 'state.jsonl') + const warnings = [] + const journal = openJournal({ path, warn: (...args) => warnings.push(args.join(' ')) }) + await journal.record(stateText({ version: 2, n: 1 })) + await journal.record(stateText({ version: 2, n: 2 })) + assert.deepEqual(warnings, []) + const text = readFileSync(path, 'utf8') + assert.equal(stateText(replayTo(text)), stateText({ version: 2, n: 2 })) + assert.equal(lstatSync(path).isSymbolicLink(), false) + if (posix) { + assert.equal(statSync(join(home, 'journal')).mode & 0o777, 0o700) + assert.equal(statSync(path).mode & 0o777, 0o600) + } + rmSync(home, { recursive: true, force: true }) +})