From 350cc75cb24381582595a33ed0ea8d9be50223cf Mon Sep 17 00:00:00 2001 From: Day Dreamer <87640074+HugoXK@users.noreply.github.com> Date: Thu, 5 Nov 2026 02:15:07 +0800 Subject: [PATCH] host: a team file linking to a file it cannot read is not reported as read through the link MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit stateNotPlain() followed a symbolic link with stat() and asked only isFile(), so a target this host may not read still earned the link sentence: 'the store reads the team file through the link and the next save replaces the name'. The next row of the same report then said that read failed with EACCES, and no save ever renames over the name, because readFile() throws before writable() can become true. The target's read permission is now asked with access(path, R_OK) — no open, no byte — so a link to an unreadable file keeps the leaf sentence, which matches the Read-only row below it. Covered by one case that skips when the run is root. patches/ds-bot/0001-core-fixes.patch and 0007-version-and-tooling.patch are what generate writes. --- .../2428-linked-state-unreadable.fixed.md | 1 + core/src/host/diagnostics.js | 12 +++++-- core/test/diagnostics.test.js | 32 +++++++++++++++++ docs/subsystems/core-host.md | 2 +- patches/ds-bot/0001-core-fixes.patch | 18 +++++++--- patches/ds-bot/0007-version-and-tooling.patch | 34 ++++++++++++++++++- 6 files changed, 89 insertions(+), 10 deletions(-) create mode 100644 changelog.d/2428-linked-state-unreadable.fixed.md diff --git a/changelog.d/2428-linked-state-unreadable.fixed.md b/changelog.d/2428-linked-state-unreadable.fixed.md new file mode 100644 index 000000000..241f57e5d --- /dev/null +++ b/changelog.d/2428-linked-state-unreadable.fixed.md @@ -0,0 +1 @@ +The diagnostics report's state line no longer promises a read it could not make: a `state.json` that is a link to a plain file this host cannot read now keeps the same sentence as a link to a folder (`not a plain file (nothing is read or written)`), instead of saying the store reads the team file through the link directly above a `Read-only` row that reports that read failing with `EACCES`. The target's read permission is asked with `access()`, which opens nothing and reads no byte ([#2428](https://github.com/ThinkFlowLab/FlyBot/issues/2428)). diff --git a/core/src/host/diagnostics.js b/core/src/host/diagnostics.js index 970cd6c88..350bc1e8e 100644 --- a/core/src/host/diagnostics.js +++ b/core/src/host/diagnostics.js @@ -4,8 +4,8 @@ // a report written after a restart still has them. Every text is cleaned of saved keys, // credential-like strings and the home folder before it is kept and again before it is // shown, because the report is meant for a public issue. -import { readFileSync } from 'node:fs' -import { appendFile, lstat, mkdir, readFile, rename, rm, stat, writeFile } from 'node:fs/promises' +import { constants, readFileSync } from 'node:fs' +import { access, appendFile, lstat, mkdir, readFile, rename, rm, stat, writeFile } from 'node:fs/promises' import { arch, homedir, platform, release } from 'node:os' import { dirname, join } from 'node:path' import { fileURLToPath } from 'node:url' @@ -261,7 +261,13 @@ export function install(rt) { try { const info = await lstat(path) if (!info.isSymbolicLink()) return info.isFile() && info.nlink > 1 ? LINKED_STATE : NOT_PLAIN - return (await stat(path)).isFile() ? LINKED_STATE : NOT_PLAIN + if (!(await stat(path)).isFile()) return NOT_PLAIN + // A target can be a plain file this host still cannot read: `readFile(statePath)` then fails + // EACCES, the store lands in `stuck`, `writable()` stays false and the save the sentence above + // promises never happens — the leaf sentence is the true one there too. `access` asks the + // permission without opening the file and without reading a byte of it. + await access(path, constants.R_OK) + return LINKED_STATE } catch { return NOT_PLAIN } diff --git a/core/test/diagnostics.test.js b/core/test/diagnostics.test.js index eda770117..297efe811 100644 --- a/core/test/diagnostics.test.js +++ b/core/test/diagnostics.test.js @@ -791,6 +791,38 @@ test('a team file linking to a folder keeps the leaf sentence, not the read-thro assert.equal(existsSync(join(elsewhere, 'not-there.json')), false, 'asking the link creates nothing') }) +// #2428: a link can name a plain file this host cannot read at all. `stat` alone answered "a plain +// file", so the state line promised a read and a save that the row below it already contradicted: +// `readFile(statePath)` fails EACCES, the store lands in `stuck`, `writable()` stays false and no +// save ever renames over the name. The permission is asked with `access(path, R_OK)`, which opens +// nothing and reads no byte, and only a target this host may read earns the link sentence. +test('a team file linking to a file it cannot read keeps the leaf sentence, not the read-through-link one', { + // Root reads a file without its read bits, so the case cannot be built there; the runner marks + // it skipped instead of asserting something the kernel never enforces. + skip: process.getuid?.() === 0 ? 'root reads a file without its read bits' : undefined, +}, async (t) => { + const elsewhere = mkdtempSync(join(tmpdir(), 'ds-bot-diag-target-')) + const target = join(elsewhere, 'unreadable.json') + const kept = '{"version":2,"bots":{}}\n' + writeFileSync(target, kept) + chmodSync(target, 0o000) + // The restore is registered before the folder's removal, and the body restores it as well, so a + // failed assertion never leaves a `000` file behind the symlink for a later hook to trip over. + t.after(() => chmodSync(target, 0o600)) + t.after(() => rmSync(elsewhere, { recursive: true, force: true, maxRetries: 5 })) + + const linked = await realHost(t, { prepare: statePath => symlinkSync(target, statePath) }) + const line = linked.report.markdown.match(/^state file: (.*)$/m)?.[1] + assert.equal(linked.report.facts.stateFile, 'not a plain file (nothing is read or written)', + 'a link the store cannot read through is not described as read through') + assert.equal(line, 'not a plain file (nothing is read or written)') + assert.match(linked.report.markdown, /^\| Read-only \| The Bot team file .* could not be read \(EACCES\)/m, + 'the row below it says that read happened and failed') + + chmodSync(target, 0o600) + assert.equal(readFileSync(target, 'utf8'), kept, 'the target keeps every byte while it is unreadable') +}) + // The line is read information, not a write: a window another dsh keeps read-only still names the // team file, the way it still counts the team it shows. test('a read-only window names the team file too (#2394)', async (t) => { diff --git a/docs/subsystems/core-host.md b/docs/subsystems/core-host.md index b713c94d2..b8a173083 100644 --- a/docs/subsystems/core-host.md +++ b/docs/subsystems/core-host.md @@ -407,7 +407,7 @@ Work a member hands a Bot with `message_bot` from its group Session runs in a hi - Host diagnostics never depend on `ctx.logger`: DSH may export no logger, and a test or plugin-local host may carry one without `warn`. `rt.warn` still writes stderr and the diagnostics log in that case (`src/host/diagnostics.js`). Plugins reach the same log through `flybot.warn` (#68); see [plugin-runtime](plugin-runtime.md). - The `Read-only` row of a report repeats this host's own answer rather than one constant: `rt.readOnly()` is `undefined` while the window writes, a sentence about a live `state.lock` when another dsh holds the team, and a sentence about the team file when it is `stuck` (a newer version, a file that will not parse, an unreadable path). The report shows that sentence — `clean(cut(answer, 200))`, so saved keys, the home folder and `|` are handled like any other report text — the same one the sidebar shows, so a newer team file reads `update FlyBot` and an unreadable one reads `could not be read`, never `another dsh holds the team`; a writable window still adds no row ([#2167](https://github.com/ThinkFlowLab/FlyBot/issues/2167)). - The log itself is one leaf file under the home, and a write asks it before it appends ([#2089](https://github.com/ThinkFlowLab/FlyBot/issues/2089)): when `logs/diagnostics.jsonl` is a link, a hard link or anything other than a plain file of its own — the line is dropped with one line on stderr that carries no home folder, the entries stay readable in memory, and `clear` leaves a file behind such a link untouched rather than truncating it. Past 1 MB the leaf moves aside to `diagnostics.jsonl.1`, and `clear` removes that name as well — in the same write chain, behind the same leaf guard — so a cleared log leaves no rotated lines behind. The truncate runs first and the removal only after it: a leaf that cannot be emptied keeps the rotated copy where it is, because those lines are then the only copy left of them. The removal keeps a `try` of its own: a copy the host cannot remove (a directory under that name, a folder without its write bit) keeps its bytes, is named on one stderr line that carries no folder, and never turns a Clear that emptied the leaf into one that leaves every line on disk. The truncate keeps a `try` of its own for the same reason ([#2364](https://github.com/ThinkFlowLab/FlyBot/issues/2364)): the leaf guard asks the file's kind, not its mode, so a plain `0444` leaf passes it and the write fails with `EACCES` — that failure is named on one stderr line, `[ds-bot] diagnostics log not truncated: `, which carries no folder, the same way `append` names its own; the answer it leaves behind is `leaf: 'kept'`. `clear` answers with what it did — `{ leaf: 'truncated' | 'kept' | 'absent', rotated: 'removed' | 'kept' | 'none' }` — and `diagnostics-clear` passes that result through instead of a constant, so the Feedback page's sentence matches it: the unchanged one, one that names the leftover `logs/diagnostics.jsonl.1`, or one that does not claim a cleared log ([#2360](https://github.com/ThinkFlowLab/FlyBot/issues/2360)). `rotated` says what this call did to that name: `removed` when it deleted one, `kept` when one was there and still is — the removal failed, or the truncate failed first and nothing was removed — and `none` when there was none; a leaf the guard refuses never reaches the probe, so it answers `none`. What the window reads moves only with an emptied leaf: `entries()` answers the old lines through `cleared` plus the live ones in `fresh`, and `logText()` behind the Download reads the same, so a Clear that leaves the leaf full changes neither of them — the page can no longer show "not cleared" next to "0 recent errors" while every byte is still on disk. Only a leaf that really was truncated clears both — plus a host that keeps no log file at all (`diagnosticsFile: false`), which has no file and no list to keep; an `ENOENT` write leaves both alone like every other refusal ([#2364](https://github.com/ThinkFlowLab/FlyBot/issues/2364)). That is the answer, one stderr line and nothing else that moves: the truncate, the `fileKind` guard, the removal rule and the `try` keep the behaviour they had, and a leaf that is not there yet still gets the empty file the write always landed there. `absent` therefore means nothing was on disk to clear — this host keeps no log file (`diagnosticsFile: false`), or the folder is not there so the write found nothing to truncate — not a leaf the call refused. The report says whether that same file is being written, in the shape the journal line already has: `diagnostics log: recording` while the appends land, `not written ()` after an append the host could not make, and `not written (not a plain file of its own)` when the leaf guard refused the write, so a bundle that lists recent errors also says when the file holding them is missing or short and why. The value follows the last append attempt, not the first refusal: a host that puts a plain file of its own back writes the next entry into it and says `recording` again, and a leaf that has become a plain `0444` file says `not written (EACCES)` on the next attempt, while the guard's own stderr line stays one per host ([#2089](https://github.com/ThinkFlowLab/FlyBot/issues/2089)); a host with the log off (`diagnosticsFile: false`) adds no line at all, the rule the journal line keeps ([#2383](https://github.com/ThinkFlowLab/FlyBot/issues/2383)). -- The report also names the team file itself, right after the table that counts the team ([#2394](https://github.com/ThinkFlowLab/FlyBot/issues/2394)): `state file: ( bytes, last written )` for a plain file with this one name, `state file: none yet (this computer has not saved the team)` before this computer has one, and `state file: not a plain file (described only; the store reads the team file through the link and the next save replaces the name)` when the path is a link to a plain file or a file with a second name (`nlink > 1`) — the store's own shape, which reads the team file through the name and renames a temp name over it on the next save, so the line must not claim nothing is read or written ([#2404](https://github.com/ThinkFlowLab/FlyBot/issues/2404)). A directory, a link to a directory, a link to nothing or a check that cannot run keeps the journal leaf's `state file: not a plain file (nothing is read or written)`, which is true of them: no read of those lands, and for a folder behind the link the store's read fails as one (`EISDIR`), so it lands in `stuck`, `writable()` stays false and no save ever renames over the name. Both sentences are metadata only — `lstat` for the name, plus `stat` on a link to ask what it points at, never open, read or stream the target; that is the point: `state.json` holds the members, the messages and the saved keys, so the report may describe the file and must never open it — a report can say how large the team file is and when it was last written without reading a byte of it. The path goes through the same home redaction, the line is read information rather than a write action so a read-only window draws it too, and the wording of `state journal:`, `journal file:` and the `Read-only` row is unchanged. +- The report also names the team file itself, right after the table that counts the team ([#2394](https://github.com/ThinkFlowLab/FlyBot/issues/2394)): `state file: ( bytes, last written )` for a plain file with this one name, `state file: none yet (this computer has not saved the team)` before this computer has one, and `state file: not a plain file (described only; the store reads the team file through the link and the next save replaces the name)` when the path is a link to a plain file or a file with a second name (`nlink > 1`) — the store's own shape, which reads the team file through the name and renames a temp name over it on the next save, so the line must not claim nothing is read or written ([#2404](https://github.com/ThinkFlowLab/FlyBot/issues/2404)). A directory, a link to a directory, a link to nothing or a check that cannot run keeps the journal leaf's `state file: not a plain file (nothing is read or written)`, which is true of them: no read of those lands, and for a folder behind the link the store's read fails as one (`EISDIR`), so it lands in `stuck`, `writable()` stays false and no save ever renames over the name. A link to a plain file this host may not read keeps that leaf sentence too ([#2428](https://github.com/ThinkFlowLab/FlyBot/issues/2428)): `stat` alone answers `isFile()`, so the permission is asked as well with `access(path, R_OK)` — no open, no byte — because otherwise the line promised the read above the `Read-only` row that reports the same read failing with `EACCES`. Both sentences are metadata only — `lstat` for the name, plus `stat` on a link to ask what it points at, never open, read or stream the target; that is the point: `state.json` holds the members, the messages and the saved keys, so the report may describe the file and must never open it — a report can say how large the team file is and when it was last written without reading a byte of it. The path goes through the same home redaction, the line is read information rather than a write action so a read-only window draws it too, and the wording of `state journal:`, `journal file:` and the `Read-only` row is unchanged. - The report names the file the journal records into, in the journal's own shape and without opening it ([#2388](https://github.com/ThinkFlowLab/FlyBot/issues/2388)): `journal file: ( bytes, last written )` for a plain file with this one name, `journal file: none yet` before the first save creates it, and `journal file: not a plain file (nothing is read or written)` when the path is a link, a file with a second name (`nlink > 1`) or the check cannot run. `lstat` is the only call — never `open`, `readFile` or `createReadStream` — so the line needs no read permission on the file and touches nothing another name holds; the path goes through the same home redaction the rest of the report uses, and a host without a journal adds no line, like the `state journal:` sentence it follows. The line says what the file is, not that a write happened: `state journal:` keeps answering whether the host is recording. ### Untrusted fences in every shared room diff --git a/patches/ds-bot/0001-core-fixes.patch b/patches/ds-bot/0001-core-fixes.patch index 518080991..07cba9c59 100644 --- a/patches/ds-bot/0001-core-fixes.patch +++ b/patches/ds-bot/0001-core-fixes.patch @@ -5027,12 +5027,14 @@ diff --git a/src/host/connectors.js b/src/host/connectors.js diff --git a/src/host/diagnostics.js b/src/host/diagnostics.js --- a/src/host/diagnostics.js +++ b/src/host/diagnostics.js -@@ -5,18 +5,20 @@ +@@ -4,19 +4,21 @@ + // a report written after a restart still has them. Every text is cleaned of saved keys, // credential-like strings and the home folder before it is kept and again before it is // shown, because the report is meant for a public issue. - import { readFileSync } from 'node:fs' +-import { readFileSync } from 'node:fs' -import { appendFile, mkdir, readFile, rename, stat, writeFile } from 'node:fs/promises' -+import { appendFile, lstat, mkdir, readFile, rename, rm, stat, writeFile } from 'node:fs/promises' ++import { constants, readFileSync } from 'node:fs' ++import { access, appendFile, lstat, mkdir, readFile, rename, rm, stat, writeFile } from 'node:fs/promises' import { arch, homedir, platform, release } from 'node:os' import { dirname, join } from 'node:path' import { fileURLToPath } from 'node:url' @@ -5187,7 +5189,7 @@ diff --git a/src/host/diagnostics.js b/src/host/diagnostics.js console.error(`[ds-bot] ${line}${frames.length > 0 ? `\n${clean(frames.join('\n'))}` : ''}`) note({ level: 'warn', -@@ -187,24 +187,98 @@ export function install(rt) { +@@ -187,24 +187,104 @@ export function install(rt) { })) } @@ -5213,7 +5215,13 @@ diff --git a/src/host/diagnostics.js b/src/host/diagnostics.js + try { + const info = await lstat(path) + if (!info.isSymbolicLink()) return info.isFile() && info.nlink > 1 ? LINKED_STATE : NOT_PLAIN -+ return (await stat(path)).isFile() ? LINKED_STATE : NOT_PLAIN ++ if (!(await stat(path)).isFile()) return NOT_PLAIN ++ // A target can be a plain file this host still cannot read: `readFile(statePath)` then fails ++ // EACCES, the store lands in `stuck`, `writable()` stays false and the save the sentence above ++ // promises never happens — the leaf sentence is the true one there too. `access` asks the ++ // permission without opening the file and without reading a byte of it. ++ await access(path, constants.R_OK) ++ return LINKED_STATE + } catch { + return NOT_PLAIN + } diff --git a/patches/ds-bot/0007-version-and-tooling.patch b/patches/ds-bot/0007-version-and-tooling.patch index 31e8edf3d..2f276687b 100644 --- a/patches/ds-bot/0007-version-and-tooling.patch +++ b/patches/ds-bot/0007-version-and-tooling.patch @@ -1005,7 +1005,7 @@ diff --git a/test/diagnostics.test.js b/test/diagnostics.test.js test('diagnostics append fallback redacts an error message', async (t) => { const home = mkdtempSync(join(tmpdir(), 'ds-bot-diag-')) t.after(() => rmSync(home, { recursive: true, force: true, maxRetries: 5 })) -@@ -133,6 +133,567 @@ test('diagnostics append fallback redacts an error message', async (t) => { +@@ -133,6 +133,599 @@ test('diagnostics append fallback redacts an error message', async (t) => { assert.match(stderr.join('\n'), /api_key=\[hidden\]/) }) @@ -1525,6 +1525,38 @@ diff --git a/test/diagnostics.test.js b/test/diagnostics.test.js + assert.equal(existsSync(join(elsewhere, 'not-there.json')), false, 'asking the link creates nothing') +}) + ++// #2428: a link can name a plain file this host cannot read at all. `stat` alone answered "a plain ++// file", so the state line promised a read and a save that the row below it already contradicted: ++// `readFile(statePath)` fails EACCES, the store lands in `stuck`, `writable()` stays false and no ++// save ever renames over the name. The permission is asked with `access(path, R_OK)`, which opens ++// nothing and reads no byte, and only a target this host may read earns the link sentence. ++test('a team file linking to a file it cannot read keeps the leaf sentence, not the read-through-link one', { ++ // Root reads a file without its read bits, so the case cannot be built there; the runner marks ++ // it skipped instead of asserting something the kernel never enforces. ++ skip: process.getuid?.() === 0 ? 'root reads a file without its read bits' : undefined, ++}, async (t) => { ++ const elsewhere = mkdtempSync(join(tmpdir(), 'ds-bot-diag-target-')) ++ const target = join(elsewhere, 'unreadable.json') ++ const kept = '{"version":2,"bots":{}}\n' ++ writeFileSync(target, kept) ++ chmodSync(target, 0o000) ++ // The restore is registered before the folder's removal, and the body restores it as well, so a ++ // failed assertion never leaves a `000` file behind the symlink for a later hook to trip over. ++ t.after(() => chmodSync(target, 0o600)) ++ t.after(() => rmSync(elsewhere, { recursive: true, force: true, maxRetries: 5 })) ++ ++ const linked = await realHost(t, { prepare: statePath => symlinkSync(target, statePath) }) ++ const line = linked.report.markdown.match(/^state file: (.*)$/m)?.[1] ++ assert.equal(linked.report.facts.stateFile, 'not a plain file (nothing is read or written)', ++ 'a link the store cannot read through is not described as read through') ++ assert.equal(line, 'not a plain file (nothing is read or written)') ++ assert.match(linked.report.markdown, /^\| Read-only \| The Bot team file .* could not be read \(EACCES\)/m, ++ 'the row below it says that read happened and failed') ++ ++ chmodSync(target, 0o600) ++ assert.equal(readFileSync(target, 'utf8'), kept, 'the target keeps every byte while it is unreadable') ++}) ++ +// The line is read information, not a write: a window another dsh keeps read-only still names the +// team file, the way it still counts the team it shows. +test('a read-only window names the team file too (#2394)', async (t) => {