From 12e42db1ba11186939a6cdb46edc88c509dfca71 Mon Sep 17 00:00:00 2001 From: Dev-3 Date: Mon, 2 Nov 2026 08:43:09 +0000 Subject: [PATCH] fix(host): refuse a hard link at the journal's own file name `journal/state.jsonl` must be a plain file this host owns, but a hard link is a plain file to `lstat` while its bytes are also another name's. The start-up read then took that file as the team's history, `chmod`-ed it and the append wrote the next lines into it, all without a warning. Both places now require the file to carry only this one name (`nlink === 1`), so a shared inode is refused exactly like a symbolic link and a journal with one name records as before. --- changelog.d/2131-journal-hardlink.fixed.md | 1 + core/src/host/journal.js | 7 +- core/test/state-journal.test.js | 70 ++++++++++++++++++- docs/subsystems/core-host.md | 2 +- patches/ds-bot/files/src/host/journal.js | 7 +- .../ds-bot/files/test/state-journal.test.js | 70 ++++++++++++++++++- 6 files changed, 148 insertions(+), 9 deletions(-) create mode 100644 changelog.d/2131-journal-hardlink.fixed.md diff --git a/changelog.d/2131-journal-hardlink.fixed.md b/changelog.d/2131-journal-hardlink.fixed.md new file mode 100644 index 000000000..ed83c8068 --- /dev/null +++ b/changelog.d/2131-journal-hardlink.fixed.md @@ -0,0 +1 @@ +The state journal now refuses a `journal/state.jsonl` that is a hard link: its bytes are also a name outside this home, so the start-up read and the append both require the file to carry only this one name (`nlink === 1`). A shared inode is refused exactly like a symbolic link — one warning, nothing read, chmod-ed or appended — while a journal with one name records as before. diff --git a/core/src/host/journal.js b/core/src/host/journal.js index d9a467d84..128c2bdf4 100644 --- a/core/src/host/journal.js +++ b/core/src/host/journal.js @@ -256,7 +256,7 @@ export function openJournal({ path, warn, now = Date.now, maxBytes = JOURNAL_MAX if (error?.code === 'ENOENT') return throw error } - if (!info.isFile()) throw new JournalError('the journal path is not a plain file') + if (!info.isFile() || info.nlink !== 1) throw new JournalError('the journal path is not a plain file') await chmod(path, 0o600) const text = await readFile(path, 'utf8') const entries = parseJournal(text) @@ -302,14 +302,15 @@ export function openJournal({ path, warn, now = Date.now, maxBytes = JOURNAL_MAX oldest = kept[1]?.t } else { // #2074: an append goes to a plain file this host owns and nothing else — a link standing in - // for the file would have the snapshot appended to whatever file it names. + // for the file would have the snapshot appended to whatever file it names, and a hard link's + // other name holds the same bytes, so only a file with this one name is this host's own. let info try { info = await lstat(path) } catch (error) { if (error?.code !== 'ENOENT') throw error } - if (info !== undefined && (info.isSymbolicLink() || !info.isFile())) throw new JournalError('the journal path is not a plain file') + if (info !== undefined && (info.isSymbolicLink() || !info.isFile() || info.nlink !== 1)) throw new JournalError('the journal path is not a plain file') await appendFile(path, text, { mode: 0o600 }) bytes += added oldest = firstChange diff --git a/core/test/state-journal.test.js b/core/test/state-journal.test.js index aabb12ec4..d527a87a3 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 { chmodSync, existsSync, lstatSync, mkdirSync, mkdtempSync, readdirSync, readFileSync, rmSync, statSync, symlinkSync, writeFileSync } from 'node:fs' +import { chmodSync, existsSync, linkSync, 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' @@ -389,6 +389,74 @@ test('a journal file swapped for a link after start-up is left byte-for-byte unc rmSync(outside, { recursive: true, force: true }) }) +// A hard link is `lstat`'s idea of a plain file and not a link, so both checks let it through +// (#2074 asked whether the name is this host's; #2115 answered it for the other name a file can +// have): the journal took another file's bytes as the team's history, chmod-ed that file and +// appended to it. One name is this host's, two names are not. +test('a journal file that is a hard link is not the team’s history at start-up (#2074 shape)', async () => { + const home = tempHome() + const outside = mkdtempSync(join(tmpdir(), 'ds-bot-journal-outside-')) + const target = join(outside, 'someone-else.jsonl') + // A legal journal file: one base line, so the start-up read would take it as this team's history. + const line = lines([{ seq: 1, t: 0, op: 'base', next: { version: 2, revision: 7 }, actor: 'host' }]) + writeFileSync(target, line) + if (posix) chmodSync(target, 0o644) + mkdirSync(join(home, 'journal')) + const path = join(home, 'journal', 'state.jsonl') + linkSync(target, path) + assert.equal(lstatSync(path).nlink, 2, 'the journal name shares its file with another name') + const warnings = [] + const journal = openJournal({ path, warn: (...args) => warnings.push(args.join(' ')) }) + await journal.record(stateText({ version: 2, revision: 8 })) + assert.equal(readFileSync(target, 'utf8'), line, 'the file the other name holds is not read as history or appended to') + if (posix) assert.equal(statSync(target).mode & 0o777, 0o644, 'it keeps its mode') + assert.equal(warnings.length, 1, 'the name is refused once, not once per save') + assert.match(warnings[0], /not recording/) + assert.equal(lstatSync(path).nlink, 2, 'the refused name is left as it was') + rmSync(home, { recursive: true, force: true }) + rmSync(outside, { recursive: true, force: true }) +}) + +test('a journal file swapped for a hard link after start-up is not appended to (#2074 shape)', async () => { + const home = tempHome() + const outside = mkdtempSync(join(tmpdir(), 'ds-bot-journal-outside-')) + const target = join(outside, 'someone-else.txt') + writeFileSync(target, 'PRIVATE LINE\n') + if (posix) chmodSync(target, 0o644) + const folder = join(home, 'journal') + mkdirSync(folder) + const path = join(folder, 'state.jsonl') + const warnings = [] + const journal = openJournal({ path, warn: (...args) => warnings.push(args.join(' ')) }) + await journal.record(stateText({ version: 2, revision: 1 })) + assert.deepEqual(warnings, []) + rmSync(path) + linkSync(target, path) + assert.equal(lstatSync(path).nlink, 2, 'the journal name shares its file with another name now') + await journal.record(stateText({ version: 2, revision: 2 })) + assert.equal(readFileSync(target, 'utf8'), 'PRIVATE LINE\n', 'the file the other name holds is not appended to') + if (posix) assert.equal(statSync(target).mode & 0o777, 0o644, 'it keeps its mode') + assert.equal(warnings.length, 1, 'the file that has a second name now is refused when the write runs') + assert.match(warnings[0], /not recording/) + rmSync(home, { recursive: true, force: true }) + rmSync(outside, { recursive: true, force: true }) +}) + +test('a journal file that is still a plain file of its own keeps recording (#2074 control)', 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, revision: 1 })) + await journal.record(stateText({ version: 2, revision: 2 })) + assert.deepEqual(warnings, []) + assert.equal(lstatSync(path).nlink, 1, 'the journal file carries only the name this host gave it') + if (posix) assert.equal(statSync(path).mode & 0o777, 0o600) + assert.equal(stateText(replayTo(readFileSync(path, 'utf8'))), stateText({ version: 2, revision: 2 })) + rmSync(home, { recursive: true, force: true }) +}) + test('a journal that is still its own folder and file keeps recording (#2074 control)', async () => { const home = tempHome() mkdirSync(join(home, 'journal')) diff --git a/docs/subsystems/core-host.md b/docs/subsystems/core-host.md index ed52e66ad..c479c2fae 100644 --- a/docs/subsystems/core-host.md +++ b/docs/subsystems/core-host.md @@ -556,7 +556,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. 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. Since [#2074](https://github.com/ThinkFlowLab/FlyBot/issues/2074) the same question is asked again when the write runs: a journal folder swapped for a link is refused with one warning and recording stops, and a `state.jsonl` swapped for a link is never written through. The append path refuses the write; the prune path reads the current file and replaces the link with a fresh file in the verified folder, so nothing is written through it. +- **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. Since [#2074](https://github.com/ThinkFlowLab/FlyBot/issues/2074) the same question is asked again when the write runs: a journal folder swapped for a link is refused with one warning and recording stops, and a `state.jsonl` swapped for a link is never written through. A hard link reads as a plain file to `lstat` while its bytes are also a name outside this home, so both the start-up read and the append also require the file to carry only this one name (`nlink === 1`): a shared inode is refused exactly like a link — one warning, nothing read, chmod-ed or appended — and a journal with one name records as before. The append path refuses the write; the prune path reads the current file and replaces the link with a fresh file in the verified folder, so nothing is written through it. - **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 d9a467d84..128c2bdf4 100644 --- a/patches/ds-bot/files/src/host/journal.js +++ b/patches/ds-bot/files/src/host/journal.js @@ -256,7 +256,7 @@ export function openJournal({ path, warn, now = Date.now, maxBytes = JOURNAL_MAX if (error?.code === 'ENOENT') return throw error } - if (!info.isFile()) throw new JournalError('the journal path is not a plain file') + if (!info.isFile() || info.nlink !== 1) throw new JournalError('the journal path is not a plain file') await chmod(path, 0o600) const text = await readFile(path, 'utf8') const entries = parseJournal(text) @@ -302,14 +302,15 @@ export function openJournal({ path, warn, now = Date.now, maxBytes = JOURNAL_MAX oldest = kept[1]?.t } else { // #2074: an append goes to a plain file this host owns and nothing else — a link standing in - // for the file would have the snapshot appended to whatever file it names. + // for the file would have the snapshot appended to whatever file it names, and a hard link's + // other name holds the same bytes, so only a file with this one name is this host's own. let info try { info = await lstat(path) } catch (error) { if (error?.code !== 'ENOENT') throw error } - if (info !== undefined && (info.isSymbolicLink() || !info.isFile())) throw new JournalError('the journal path is not a plain file') + if (info !== undefined && (info.isSymbolicLink() || !info.isFile() || info.nlink !== 1)) throw new JournalError('the journal path is not a plain file') await appendFile(path, text, { mode: 0o600 }) bytes += added oldest = firstChange diff --git a/patches/ds-bot/files/test/state-journal.test.js b/patches/ds-bot/files/test/state-journal.test.js index aabb12ec4..d527a87a3 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 { chmodSync, existsSync, lstatSync, mkdirSync, mkdtempSync, readdirSync, readFileSync, rmSync, statSync, symlinkSync, writeFileSync } from 'node:fs' +import { chmodSync, existsSync, linkSync, 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' @@ -389,6 +389,74 @@ test('a journal file swapped for a link after start-up is left byte-for-byte unc rmSync(outside, { recursive: true, force: true }) }) +// A hard link is `lstat`'s idea of a plain file and not a link, so both checks let it through +// (#2074 asked whether the name is this host's; #2115 answered it for the other name a file can +// have): the journal took another file's bytes as the team's history, chmod-ed that file and +// appended to it. One name is this host's, two names are not. +test('a journal file that is a hard link is not the team’s history at start-up (#2074 shape)', async () => { + const home = tempHome() + const outside = mkdtempSync(join(tmpdir(), 'ds-bot-journal-outside-')) + const target = join(outside, 'someone-else.jsonl') + // A legal journal file: one base line, so the start-up read would take it as this team's history. + const line = lines([{ seq: 1, t: 0, op: 'base', next: { version: 2, revision: 7 }, actor: 'host' }]) + writeFileSync(target, line) + if (posix) chmodSync(target, 0o644) + mkdirSync(join(home, 'journal')) + const path = join(home, 'journal', 'state.jsonl') + linkSync(target, path) + assert.equal(lstatSync(path).nlink, 2, 'the journal name shares its file with another name') + const warnings = [] + const journal = openJournal({ path, warn: (...args) => warnings.push(args.join(' ')) }) + await journal.record(stateText({ version: 2, revision: 8 })) + assert.equal(readFileSync(target, 'utf8'), line, 'the file the other name holds is not read as history or appended to') + if (posix) assert.equal(statSync(target).mode & 0o777, 0o644, 'it keeps its mode') + assert.equal(warnings.length, 1, 'the name is refused once, not once per save') + assert.match(warnings[0], /not recording/) + assert.equal(lstatSync(path).nlink, 2, 'the refused name is left as it was') + rmSync(home, { recursive: true, force: true }) + rmSync(outside, { recursive: true, force: true }) +}) + +test('a journal file swapped for a hard link after start-up is not appended to (#2074 shape)', async () => { + const home = tempHome() + const outside = mkdtempSync(join(tmpdir(), 'ds-bot-journal-outside-')) + const target = join(outside, 'someone-else.txt') + writeFileSync(target, 'PRIVATE LINE\n') + if (posix) chmodSync(target, 0o644) + const folder = join(home, 'journal') + mkdirSync(folder) + const path = join(folder, 'state.jsonl') + const warnings = [] + const journal = openJournal({ path, warn: (...args) => warnings.push(args.join(' ')) }) + await journal.record(stateText({ version: 2, revision: 1 })) + assert.deepEqual(warnings, []) + rmSync(path) + linkSync(target, path) + assert.equal(lstatSync(path).nlink, 2, 'the journal name shares its file with another name now') + await journal.record(stateText({ version: 2, revision: 2 })) + assert.equal(readFileSync(target, 'utf8'), 'PRIVATE LINE\n', 'the file the other name holds is not appended to') + if (posix) assert.equal(statSync(target).mode & 0o777, 0o644, 'it keeps its mode') + assert.equal(warnings.length, 1, 'the file that has a second name now is refused when the write runs') + assert.match(warnings[0], /not recording/) + rmSync(home, { recursive: true, force: true }) + rmSync(outside, { recursive: true, force: true }) +}) + +test('a journal file that is still a plain file of its own keeps recording (#2074 control)', 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, revision: 1 })) + await journal.record(stateText({ version: 2, revision: 2 })) + assert.deepEqual(warnings, []) + assert.equal(lstatSync(path).nlink, 1, 'the journal file carries only the name this host gave it') + if (posix) assert.equal(statSync(path).mode & 0o777, 0o600) + assert.equal(stateText(replayTo(readFileSync(path, 'utf8'))), stateText({ version: 2, revision: 2 })) + rmSync(home, { recursive: true, force: true }) +}) + test('a journal that is still its own folder and file keeps recording (#2074 control)', async () => { const home = tempHome() mkdirSync(join(home, 'journal'))