diff --git a/changelog.d/2631.fixed.md b/changelog.d/2631.fixed.md new file mode 100644 index 000000000..9ee061e6f --- /dev/null +++ b/changelog.d/2631.fixed.md @@ -0,0 +1 @@ +A Bot Social credentials, backup or settings file that exists but no longer parses is now refused with a message naming it instead of being read as "not configured", so a torn file no longer makes a restart register a new home identity and overwrite the old one. Writes land through a temp file and a rename, so a failed write leaves the previous file whole ([#2631](https://github.com/ThinkFlowLab/FlyBot/issues/2631)). diff --git a/docs/subsystems/social.md b/docs/subsystems/social.md index 49a1c5f46..3014f2257 100644 --- a/docs/subsystems/social.md +++ b/docs/subsystems/social.md @@ -152,7 +152,7 @@ Creating a group opens its Session with zero turns, and the host draws no sessio ### The server in Settings (#337) - **Where it comes from.** `config.server` in the profile patch is the default. A server picked in Settings → **Bot Social** is kept in `flybot-social-server.json` (mode 600, next to the credentials file) and wins while it exists; choosing the default again removes the file. It is written through `flybot-social-server.json..tmp`, and that temp name is asked as a file first (temp-name sweep): a symbolic or hard link left there refuses the save instead of putting the choice outside the folder and renaming the link into place — `lstat` reports a hard link as a plain file while its bytes belong to another name too, so the guard also requires `nlink === 1`, and the plain leftover of a crashed host stays replaceable. Nothing configured anywhere behaves as before (the start fails with `server URL is required`, `Social is not running: …`; the page says **No server set**). The page reads `social/server` (a read; `{ server, source: settings|config|none, defaultServer, state: connected|connecting|not-configured|error, handle, error, canChange, known[], platform? }`: never a token) and calls `social/server-check` (it makes the host fetch an address the caller names, so it follows the write rules like `social/server-set`). -- **What may be written where.** `flybot-social.json`, the `.bak` copy `startSocial` keeps for another server (`flybot-social.json.`) and `flybot-social-settings.json` are written by one function, and it asks the leaf with `lstat` before the first byte: a missing file is created (mode 600), a plain file of its own (`nlink === 1`) is rewritten, and a symbolic link, a hard link, a directory or any other kind refuses the write — nothing is written there, the file behind a link keeps its bytes, one line names the path and carries no value from the file, and `startSocial` fails with that refusal instead of running on a link. The refusal is asked **before** onboarding: the onboarding is a real home on the server and a round-trip, so a start whose credentials path cannot be written refuses first and never spends one on a token it would drop. A refused `.bak` leaves the credentials in use untouched, and its name is asked in that same pre-check: the backup name follows from the server in the credentials file already on disk (`flybot-social.json.`), a local read, so a link left there refuses the start before the onboarding rather than after it, and a free name still keeps the old credentials under their server. `flybot-social-settings.json` is a different shape and stays as it is: it is read (a link at that name is followed, like any read) and written only when a setting changes, where a refusal is caught and warned as `settings not saved` — no onboarding depends on it, so it never fails a start. A read-only window (`flybot.readOnly?.()`) writes it not at all (#2231): its read endpoints (`social/remote-bots`, `social/friends`) still derive the one-shot inbound backfill, the hidden-remote aging and the accept-notice cleanup in memory and answer the same, but the save path returns without writing, so it cannot create the file, cannot age a hide on it, and cannot flush an older copy over what the writable window saved. +- **What may be written where.** `flybot-social.json`, the `.bak` copy `startSocial` keeps for another server (`flybot-social.json.`) and `flybot-social-settings.json` are written by one function, and it asks the leaf with `lstat` before the first byte: a missing file is created (mode 600), a plain file of its own (`nlink === 1`) is rewritten through a same-directory `..tmp` that is fsynced and renamed over the file (the temp name is a leaf of its own too, so a link left there refuses the save as well), and a symbolic link, a hard link, a directory or any other kind refuses the write — nothing is written there, the file behind a link keeps its bytes, one line names the path and carries no value from the file, and `startSocial` fails with that refusal instead of running on a link. The temp plus rename is what keeps a write that stops half way (ENOSPC, EFBIG, a kill) from taking the previous file with it: the old bytes stay, and a failed write removes its temp (#2631). The refusal is asked **before** onboarding: the onboarding is a real home on the server and a round-trip, so a start whose credentials path cannot be written refuses first and never spends one on a token it would drop. A refused `.bak` leaves the credentials in use untouched, and its name is asked in that same pre-check: the backup name follows from the server in the credentials file already on disk (`flybot-social.json.`), a local read, so a link left there refuses the start before the onboarding rather than after it, and a free name still keeps the old credentials under their server. The read side tells "nothing saved yet" from "damaged": `ENOENT` is a first start, while a file that exists and does not parse refuses with a message naming the path and is left where it is — a torn credentials file therefore never reads as "not configured", and `startSocial` never onboards a fresh home over an identity it could not read (#2631); `flybot-social-settings.json` is read with that same rule (a link at that name is followed, like any read) and written only when a setting changes, where a write refusal is caught and warned as `settings not saved` — no onboarding depends on that write, but a damaged settings file refuses the start instead of reading as empty. A read-only window (`flybot.readOnly?.()`) writes it not at all (#2231): its read endpoints (`social/remote-bots`, `social/friends`) still derive the one-shot inbound backfill, the hidden-remote aging and the accept-notice cleanup in memory and answer the same, but the save path returns without writing, so it cannot create the file, cannot age a hide on it, and cannot flush an older copy over what the writable window saved. - **Validation.** `parseServerUrl`: `https://` only, `http://` just for `localhost`, `127.0.0.1` and `[::1]`; no user name or password, query or fragment; at most 200 characters; an address without a scheme gets `https://` (`http://` for those local hosts). `probeServer` GETs `/v1/platform` without credentials, 8 s deadline, `redirect: 'manual'` (a redirect is refused, so the saved address is the checked one); the answer must look like Bot Social (a `name`, a `platform_key` or a `features` list) and its `features` are shown (cleaned, at most 40) in words (`featureLine` in `client.js`: `typed_by_owner` → typing as you, `mentions` → @mentions, `presence` → online status; an unknown id is only counted, and every raw id is in the line's tooltip, #421). `social/server-set` repeats both before it touches anything, so a UI that skipped **Check** gains nothing. - **A server is an identity.** The default server keeps its files. Any other server gets `flybot-social.json.` (credentials; the same suffix `startSocial` already used for backups, `tag` = first 8 hex of the sha-256 of the normalized address) and `flybot-social-settings..json`; a different server never reads another's file, and no credentials file is ever deleted or rewritten by a switch. The per-Bot parts that live in host state (the `member` identity, the `autoApprove` marks and the person/room grants, which are Bot Social ids and handles) are parked in `flybot-social-bots..json` (no token) and cleared before the new server starts, so a stale grant to `@bob.home` can never apply to a different Bob on the new server; switching back restores them and the saved credentials, without a second onboarding. The old server's friends' Bots leave the list when the new server's first roster fetch replaces it (`remote.publish`); face-to-face rooms already hosted stay as local transcripts, but nothing polls them. - **Applied live, with a way back.** `social/server-set` (needs `confirm: true`; a write, so a read-only host and a multi-member team refuse it) stops the social layer, parks the old server's Bot identities, saves the choice, restores what the new server had, and starts again (first start onboards). If that start fails or takes over 45 s, the previous server's files, identities and choice are put back, it is started again, and the error says so; a start that finishes late is disposed. A disposed instance never re-arms its poll: a face poll already in flight when `dispose()` runs cannot install a new interval on the discarded layer, so it never keeps polling (or delivering from) the server it was built for. When the switch succeeds and the address really changed, the previous server's hosted face-to-face rooms — open or still forming (candidate) — are marked `left` here — a read-only past room whose composer is gone, since the new server cannot reach them. Renaming this home on the same server is not a switch: that server still reaches those rooms, so only a new address ends them (the room and its composer stay as they were). Switching back does not reopen them: a room this home left stays a past room (`faceSnapshotPast`, #182), and a line to it is refused (`bot/room-ended`). The Settings page says plainly that this is a new identity, that friends and sharing stay on the old server, and that the old sign-in is kept as a backup and never deleted. Tests: `plugins/social/test/social.test.js` (`Settings server`, `server controller`, `parseServerUrl`, `probeServer`). diff --git a/plugins/social/src/social.js b/plugins/social/src/social.js index acd5643fb..40c074bda 100644 --- a/plugins/social/src/social.js +++ b/plugins/social/src/social.js @@ -27,7 +27,7 @@ // - Backward compat: without a home, config.botId + token use the per-bot identity // (start_conversation / send_message) as before. import { createHash } from 'node:crypto' -import { chmodSync, lstatSync, mkdirSync, readFileSync, realpathSync, rmSync, statSync, writeFileSync } from 'node:fs' +import { chmodSync, closeSync, fsyncSync, lstatSync, mkdirSync, openSync, readFileSync, realpathSync, renameSync, rmSync, statSync, writeFileSync } from 'node:fs' import { readFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { basename, dirname, extname, isAbsolute, join, sep } from 'node:path' @@ -477,9 +477,21 @@ export function registerUnavailable(service, reason) { return () => { for (const off of offs.splice(0).reverse()) off?.() } } -export const readCredentials = (path) => { - try { return JSON.parse(readFileSync(path, 'utf8')) } catch { return undefined } +// A state file is read for its bytes only. `ENOENT` is the one answer that means "nothing saved +// yet"; a file that exists and cannot be read or parsed is not that answer. Reading it as absent is +// how a start onboarded a fresh home over a torn credentials file (#2631), so it refuses, names the +// path, and leaves the file where an operator can move it aside on purpose. +const readStateFile = (path, what) => { + let text + try { text = readFileSync(path, 'utf8') } catch (error) { + if (error?.code === 'ENOENT') return undefined + throw Object.assign(new Error(`Bot Social ${what} at ${path} cannot be read: ${error?.message ?? error}`), { code: 'bot/refused', cause: error }) + } + try { return JSON.parse(text) } catch { + throw Object.assign(new Error(`Bot Social ${what} at ${path} is damaged: the file exists but does not parse as JSON. Move it aside to start over; nothing here overwrote it.`), { code: 'bot/refused' }) + } } +export const readCredentials = path => readStateFile(path, 'credentials') // The credentials file is one leaf of its own, and so are the `.bak` copy kept for another server // and `flybot-social-settings.json`: all three go through this one function. `lstat` does not follow // the last component, so a link left at the path reports itself and is refused before the first @@ -506,7 +518,24 @@ const refuseLeaf = (path) => { export const writeCredentials = (path, value) => { if (!leafWritable(path)) throw refuseLeaf(path) mkdirSync(dirname(path), { recursive: true }) - writeFileSync(path, JSON.stringify(value, null, 2), { mode: 0o600 }) + // Same-directory temp, fsync, rename — the shape `server-config.js` writes with. Writing in place + // truncated the file first, so a write that stopped half way (ENOSPC, EFBIG, a kill) took the + // previous credentials with it (#2631). The temp name is a leaf of its own (temp-name sweep): a + // link left there refuses the write instead of taking the bytes outside the folder and renaming + // the link into place. + const temp = `${path}.${process.pid}.tmp` + if (!leafWritable(temp)) throw refuseLeaf(temp) + try { + const fd = openSync(temp, 'w', 0o600) + try { + writeFileSync(fd, JSON.stringify(value, null, 2)) + fsyncSync(fd) + } finally { closeSync(fd) } + renameSync(temp, path) + } catch (error) { + try { rmSync(temp, { force: true }) } catch { /* already gone */ } + throw error + } try { chmodSync(path, 0o600) } catch { console.warn('flybot-social:', 'credentials mode not tightened:', path) } } @@ -684,7 +713,9 @@ export function createSocial(service, config = {}, { client: given, api: givenAp // identity carries the user's own chat with that friend's Bot. const settingsPath = config.settingsPath ?? (config.credentialsPath ? join(dirname(config.credentialsPath), 'flybot-social-settings.json') : undefined) let settings = { autoAccept: {}, via: {}, hiddenRemotes: {}, inboundBots: {}, roomBots: {} } - try { if (settingsPath) settings = { autoAccept: {}, via: {}, hiddenRemotes: {}, inboundBots: {}, roomBots: {}, ...JSON.parse(readFileSync(settingsPath, 'utf8')) } } catch { /* none yet */ } + // Absent is a normal first start; a file that exists and does not parse refuses the start + // (#2631) instead of silently dropping the owner's Auto-send, hides and cursors for an empty set. + if (settingsPath) settings = { ...settings, ...readStateFile(settingsPath, 'settings') } settings.hiddenRemotes ??= {} settings.inboundBots ??= {} settings.roomBots ??= {} diff --git a/plugins/social/test/credentials-atomic.test.js b/plugins/social/test/credentials-atomic.test.js new file mode 100644 index 000000000..9a3243519 --- /dev/null +++ b/plugins/social/test/credentials-atomic.test.js @@ -0,0 +1,102 @@ +// A credentials write that dies half way must not take the previous identity with it (#2631). +// +// `writeCredentials` writes the credentials file, the `.bak` copy and +// `flybot-social-settings.json`; writing in place truncates first, so a torn file is what is +// left when the write stops early. The write lands through `..tmp` and a rename +// instead, and a start refuses a file that exists but does not parse rather than reading it as +// "nothing configured" — the difference is a fresh Bot Social home onboarded over the old one. +import assert from 'node:assert/strict' +import { chmodSync, mkdtempSync, readFileSync, readdirSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { test } from 'node:test' +import { createSocial, readCredentials, startSocial, writeCredentials } from '../src/social.js' + +const homeOf = (t) => { + const home = mkdtempSync(join(tmpdir(), 'flybot-social-atomic-')) + t.after(() => rmSync(home, { recursive: true, force: true })) + return home +} + +const settle = async (run) => { + try { return { value: await run() } } catch (failure) { return { failure } } +} + +const quiet = (t) => { + const real = console.warn + console.warn = () => {} + t.after(() => { console.warn = real }) +} + +// The bytes the 16th audit round left behind: the first write was cut inside the token. +const TORN = '{\n "token": "tok-OLD-12' + +test('a torn credentials file does not read as "not configured", and the start does not onboard over it', async (t) => { + const home = homeOf(t) + const path = join(home, 'flybot-social.json') + writeFileSync(path, TORN) + quiet(t) + assert.throws(() => readCredentials(path), (error) => { + assert.match(String(error?.message), /damaged/, 'the read says the file is damaged') + assert.ok(String(error?.message).includes(path), 'and names the file a reader has to look at') + return true + }, 'a file that exists and does not parse is not "nothing saved yet"') + + // The start used to onboard a new home here and overwrite the torn file (issue steps D and E). + const asked = [] + const onboarded = async (url) => { + asked.push(String(url)) + return new Response(JSON.stringify({ connector: { token: 'tok-NEW-9999' }, home: { bot_id: 'h9', handle: '@me.home' }, owner_id: 'owner-9' }), { status: 200, headers: { 'content-type': 'application/json' } }) + } + const { failure } = await settle(() => startSocial({ scope() { throw new Error('the plugin scope is not built in this test') } }, { server: 'https://social.test', credentialsPath: path, onboard: true, fetch: onboarded })) + assert.equal(asked.length, 0, 'the server was not asked for a new home') + assert.match(String(failure?.message ?? 'no failure: the start went on'), /damaged/, 'the start refuses instead of onboarding') + assert.equal(readFileSync(path, 'utf8'), TORN, 'the torn bytes are left where the operator can see them, not overwritten') + assert.deepEqual(readdirSync(home).sort(), ['flybot-social.json'], 'and no new identity was written beside them') +}) + +test('a failed credentials write leaves the previous file byte for byte', (t) => { + const home = homeOf(t) + const path = join(home, 'flybot-social.json') + const old = { token: 'tok-OLD-12', home: { id: 'h1', handle: '@me.home' }, server: 'https://social.test' } + writeCredentials(path, old) + const before = readFileSync(path, 'utf8') + // The temp file cannot be created while the directory is read-only; the file in it is still + // writable, so the truncate-then-write shape would replace the credentials without failing. + chmodSync(home, 0o500) + try { + assert.throws(() => writeCredentials(path, { token: 'tok-NEW-9999' }), (error) => error?.code === 'EACCES' || error?.code === 'EPERM') + assert.equal(readFileSync(path, 'utf8'), before, 'the credentials are byte for byte what they were') + assert.deepEqual(readdirSync(home).sort(), ['flybot-social.json'], 'a failed write leaves no intermediate file behind') + } finally { + chmodSync(home, 0o700) + } +}) + +test('a successful write lands in one rename and leaves no intermediate file', (t) => { + const home = homeOf(t) + const path = join(home, 'flybot-social.json') + writeCredentials(path, { token: 'tok-OLD-12' }) + writeCredentials(path, { token: 'tok-NEW-9999' }) + assert.deepEqual(JSON.parse(readFileSync(path, 'utf8')), { token: 'tok-NEW-9999' }) + assert.deepEqual(readdirSync(home).sort(), ['flybot-social.json'], 'no *.tmp is left beside the file') +}) + +test('a torn settings file is refused too, instead of being read as empty', (t) => { + const home = homeOf(t) + const credentialsPath = join(home, 'flybot-social.json') + const settingsPath = join(home, 'flybot-social-settings.json') + writeFileSync(credentialsPath, JSON.stringify({ token: 'tok-OLD-12', home: { id: 'h1', handle: '@me.home' }, server: 'https://social.test' })) + writeFileSync(settingsPath, '{"autoAccept": {') + quiet(t) + // Every host service is unused before the settings load: the start refuses there, so one + // stand-in for all of them keeps the test on the file and not on the host shape. + const anything = new Proxy(function () {}, { get: (target, key) => (key === 'then' ? undefined : anything), apply: () => undefined, construct: () => ({}) }) + const flybot = new Proxy({}, { get: (target, key) => (key === 'then' ? undefined : anything) }) + assert.throws(() => createSocial({ scope: () => flybot }, { server: 'https://social.test', token: 'tok-OLD-12', credentialsPath, settingsPath, fetch: async () => { throw new Error('no network in this test') } }, { setInterval: () => 0, clearInterval: () => {} }), (error) => { + assert.match(String(error?.message), /settings/, 'the start says which file is damaged') + assert.ok(String(error?.message).includes(settingsPath), 'and names it') + return true + }, 'a settings file that exists and does not parse is not "nothing saved yet"') + assert.equal(readFileSync(settingsPath, 'utf8'), '{"autoAccept": {', 'the damaged file is left as it was found') +}) diff --git a/plugins/social/test/credentials-leaf.test.js b/plugins/social/test/credentials-leaf.test.js index 7c37a2510..faaf7b7ee 100644 --- a/plugins/social/test/credentials-leaf.test.js +++ b/plugins/social/test/credentials-leaf.test.js @@ -76,14 +76,14 @@ test('a directory at the credentials file is refused, and stays a directory', (t assert.deepEqual(readdirSync(path), [], 'and nothing lands inside it') }) -test('a missing credentials file is created, mode 600, and a plain one is rewritten in place', (t) => { +test('a missing credentials file is created, mode 600, and a plain one is replaced', (t) => { const home = homeOf(t) const path = join(home, 'bot', 'flybot-social.json') writeCredentials(path, { token: 'first' }) assert.deepEqual(JSON.parse(readFileSync(path, 'utf8')), { token: 'first' }, 'a missing leaf is ours to create') if (process.platform !== 'win32') assert.equal(statSync(path).mode & 0o777, 0o600) writeCredentials(path, { token: 'second' }) - assert.deepEqual(JSON.parse(readFileSync(path, 'utf8')), { token: 'second' }, 'a plain leaf is written in place') + assert.deepEqual(JSON.parse(readFileSync(path, 'utf8')), { token: 'second' }, 'a plain leaf is replaced') if (process.platform !== 'win32') assert.equal(statSync(path).mode & 0o777, 0o600, 'the rewrite keeps mode 600') }) @@ -154,7 +154,7 @@ test('a linked credentials path is refused before onboarding: no server call, an expected = ['flybot-social.json'] } else { const outside = join(home, 'notes.txt') - writeFileSync(outside, 'not a credentials file') + writeFileSync(outside, CREDENTIALS) symlinkSync(outside, path) expected = ['flybot-social.json', 'notes.txt'] } @@ -224,26 +224,26 @@ test('a free .bak name still onboards once, and the old credentials are kept und assert.match(String(failure?.message ?? 'no failure: the start went on'), /plugin scope is not built in this test/, 'the start gets past the write') }) -test('an absent or plain credentials path still onboards exactly once and writes the file', async (t) => { - for (const shape of ['absent', 'plain']) { - const home = homeOf(t) - const path = join(home, 'flybot-social.json') - if (shape === 'plain') writeFileSync(path, 'not a credentials file') - quiet(t) - const asked = [] - const onboarded = async (url) => { - asked.push(String(url)) - return new Response(JSON.stringify({ connector: { token: 'new-token' }, home: { bot_id: 'h9', handle: '@me.home' }, owner_id: 'owner-9' }), { status: 200, headers: { 'content-type': 'application/json' } }) - } - // The layer below `startSocial` is not built here: the write happens before it, so this stub - // stops the start there and leaves the two facts under test intact. - const service = { scope() { throw new Error('the plugin scope is not built in this test') } } - const { failure } = await settle(() => startSocial(service, { server: 'https://social.test', credentialsPath: path, onboard: true, fetch: onboarded })) - assert.equal(asked.length, 1, `${shape}: exactly one home is asked for`) - assert.match(String(failure?.message ?? 'no failure: the start went on'), /plugin scope is not built in this test/, `${shape}: the start gets past the write`) - assert.equal(JSON.parse(readFileSync(path, 'utf8')).token, 'new-token', `${shape}: the token that was just minted is kept`) - assert.equal(lstatSync(path).isFile() && lstatSync(path).isSymbolicLink() === false, true, `${shape}: the credentials file is a plain file of its own`) +// A file that exists but does not parse is no longer "not configured" (#2631): it refuses the start +// instead of onboarding over it, and `credentials-atomic.test.js` covers that side. A missing file is +// still a first start, which this test pins down. +test('an absent credentials path still onboards exactly once and writes the file', async (t) => { + const home = homeOf(t) + const path = join(home, 'flybot-social.json') + quiet(t) + const asked = [] + const onboarded = async (url) => { + asked.push(String(url)) + return new Response(JSON.stringify({ connector: { token: 'new-token' }, home: { bot_id: 'h9', handle: '@me.home' }, owner_id: 'owner-9' }), { status: 200, headers: { 'content-type': 'application/json' } }) } + // The layer below `startSocial` is not built here: the write happens before it, so this stub + // stops the start there and leaves the two facts under test intact. + const service = { scope() { throw new Error('the plugin scope is not built in this test') } } + const { failure } = await settle(() => startSocial(service, { server: 'https://social.test', credentialsPath: path, onboard: true, fetch: onboarded })) + assert.equal(asked.length, 1, 'exactly one home is asked for') + assert.match(String(failure?.message ?? 'no failure: the start went on'), /plugin scope is not built in this test/, 'the start gets past the write') + assert.equal(JSON.parse(readFileSync(path, 'utf8')).token, 'new-token', 'the token that was just minted is kept') + assert.equal(lstatSync(path).isFile() && lstatSync(path).isSymbolicLink() === false, true, 'the credentials file is a plain file of its own') }) test('the settings file goes through the same leaf check, and the credentials beside it stay readable', (t) => {