From c982fa1ce525ce64e47eeb94346e603ba86f0052 Mon Sep 17 00:00:00 2001 From: Devvyn <22340871+opticon454@users.noreply.github.com> Date: Tue, 20 Oct 2026 18:03:55 +0800 Subject: [PATCH] fix(config-backup): retention survives an unreadable or reset settings.json, restore works on mode-less filesystems, compare checks snapshots too --- docs/wiki/Config-Backups.md | 17 +++-- docs/wiki/Troubleshooting.md | 5 ++ src/config-backup.ts | 85 +++++++++++++++++++++--- src/web/routes/config-backup-routes.ts | 44 ++++++++++-- test/config-backup.test.ts | 24 +++++++ test/routes/config-backup-routes.test.ts | 62 ++++++++++++++++- 6 files changed, 215 insertions(+), 22 deletions(-) diff --git a/docs/wiki/Config-Backups.md b/docs/wiki/Config-Backups.md index 4ba6a999..e743327c 100644 --- a/docs/wiki/Config-Backups.md +++ b/docs/wiki/Config-Backups.md @@ -90,7 +90,7 @@ Two limits, applied after every backup: | **Keep this many backups** (`configBackupKeepCount`) | 2 – 500 | 20 | Keep the newest N; older ones are removed. (1 is not offered: every change would delete the state before it.) | | **Delete backups older than (days)** (`configBackupKeepDays`) | 0 – 3650 | 30 | Also remove a backup once it has been replaced for longer than this. `0` means no age limit. | -Both apply together. Three guarantees: +Both apply together. Four guarantees: - **The newest backup is never removed**, whatever the numbers say, so there is always one to go back to. @@ -125,7 +125,11 @@ From the list, press **Restore** on a backup and confirm. Codeman then: 1. **Verifies** every file in the backup against the SHA-256 in its manifest. If any file is missing or does not match, nothing is changed and you get an error. It also refuses a backup folder or file that is a symlink or that group or others can read or write (Codeman never - makes one like that, and writing it back as `0600` would turn it into trusted config). + makes one like that, and writing it back as `0600` would turn it into trusted config). The + permission test is skipped when the backup folder's filesystem does not keep file modes + (FAT/exFAT drives, SMB shares, WSL's `/mnt/d` without the `metadata` option report the same + mode for every file, so it would refuse every backup there). The symlink test is not. Compare + applies the same checks to the backups it reads. 2. **Takes a `pre-restore` backup** of the files about to be replaced, so a restore can itself be undone. 3. **Replaces each file atomically** (written beside it, then renamed) with mode `0600`. @@ -147,7 +151,9 @@ When the backup folder is shared (a synced folder used by two machines, or prod each snapshot records a random id that the instance made once and keeps in its data directory (`config-backup-id`; not the hostname, which a Docker container changes on every recreate). Every instance only snapshots, lists, prunes, compares and restores its own. Deleting that file makes -the instance a new owner, and its earlier snapshots stop being listed (they stay on disk). +the instance a new owner, and its earlier snapshots stop being listed (they stay on disk). The +same happens after a reinstall of the data directory. To see them again, copy the `origin` value +from any of their `manifest.json` files into `/config-backup-id`. You can also restore by hand: the files in a backup folder are the originals, so copying one back over the file in the data directory is equivalent (stop Codeman first if you are replacing @@ -178,11 +184,14 @@ query and fragment. ## Security - Some tracked files carry credentials (`.env`, `users.json`, `custom-model-hosts.json`, - `push-keys.json`). Each backup folder is mode `0700` and its files `0600`, so the default + `push-keys.json`, `webhook.json`). Each backup folder is mode `0700` and its files `0600`, so the default location exposes nothing the data directory does not already. - **A custom folder is your decision.** Putting it on a shared drive, a synced folder or a network share copies those credentials there, with whatever permissions that location has. Choose a private location, or turn the feature off if you cannot. +- **Retention survives a bad `settings.json`.** If it does not parse, a pass still takes its + snapshot but deletes nothing. If the four backup settings are missing (a reset file), the pass + prunes with the retention the newest backup recorded, not the 20 copies and 30 days defaults. - **Anyone who can write to a custom backup folder can plant a backup that Restore applies.** Restore refuses symlinks and files others can write, but a file of your own user's making passes, so keep the folder private. diff --git a/docs/wiki/Troubleshooting.md b/docs/wiki/Troubleshooting.md index b101e45c..5c34528b 100644 --- a/docs/wiki/Troubleshooting.md +++ b/docs/wiki/Troubleshooting.md @@ -142,6 +142,11 @@ which is the default, you have copies: 2. **Restore** it, reload the page, and restart Codeman if the response's `restartRequired` list is not empty. +A reset `settings.json` also drops the backup folder setting. If you use a custom folder, enter +it again and save before looking for the list; until then the list shows the default folder. +After a reinstall of the data directory the old backups are unlisted because the instance has a +new id; see [Config Backups](Config-Backups) for how to make them visible again. + Without the UI: the backups are plain folders in `~/.codeman/backups/config/` (`cfg-YYYYMMDD-HHMMSS`); copy the file you need back over the one in `~/.codeman/`. diff --git a/src/config-backup.ts b/src/config-backup.ts index 1189c2d7..9912950e 100644 --- a/src/config-backup.ts +++ b/src/config-backup.ts @@ -100,6 +100,13 @@ export interface ConfigBackupManifest { hash: string; /** Id of the data dir that made it (see {@link configBackupOrigin}), so instances sharing one folder keep to their own snapshots. */ origin?: string; + /** + * The retention in force when it was made. If `settings.json` later fails to parse or is reset, + * the next pass prunes with these instead of the defaults, so one bad write cannot shorten a + * history the user asked to keep. + */ + keepCount?: number; + keepDays?: number; files: ConfigBackupFile[]; } @@ -191,6 +198,23 @@ export function configBackupOrigin(dataDir: string): string { return id; } +/** The id already made for this data dir, or null. Read-only: listing must not write a file. */ +export function readConfigBackupOrigin(dataDir: string): string | null { + try { + const existing = readFileSync(join(dataDir, ORIGIN_FILE), 'utf8').trim(); + return ORIGIN_RE.test(existing) ? existing : null; + } catch { + return null; + } +} + +/** True when any of the four `configBackup*` keys is present (the Settings form always sends all four). */ +export function hasConfigBackupSettings(raw: Record): boolean { + return ['configBackupEnabled', 'configBackupDir', 'configBackupKeepCount', 'configBackupKeepDays'].some( + (k) => k in raw + ); +} + const sha256 = (data: Buffer | string): string => createHash('sha256').update(data).digest('hex'); /** @@ -297,6 +321,9 @@ export interface CreateConfigBackupOptions { appVersion?: string; /** Snapshot even if nothing changed (a manual request, a pre-restore safety copy). */ force?: boolean; + /** Recorded in the manifest, see {@link ConfigBackupManifest.keepCount}. */ + keepCount?: number; + keepDays?: number; now?: number; } @@ -341,6 +368,8 @@ export function createConfigBackup(opts: CreateConfigBackupOptions): ConfigBacku appVersion: opts.appVersion ?? '', hash, origin, + ...(opts.keepCount !== undefined && { keepCount: opts.keepCount }), + ...(opts.keepDays !== undefined && { keepDays: opts.keepDays }), files: present.map((f) => ({ name: f.name, bytes: f.data.length, sha256: f.sha })), }; for (const f of present) writeFileSync(join(tmp, f.name), f.data, { mode: 0o600 }); @@ -373,6 +402,15 @@ export function sweepConfigBackupTemps(dir: string, now: number = Date.now()): v } } +/** The retention recorded in this instance's newest snapshot, or null when it recorded none. */ +export function latestBackupRetention(dir: string, origin: string): { keepCount: number; keepDays: number } | null { + const [newest] = listConfigBackups(dir, origin); + const m = newest ? readManifest(dir, newest.id, origin) : null; + return m && typeof m.keepCount === 'number' && typeof m.keepDays === 'number' + ? { keepCount: m.keepCount, keepDays: m.keepDays } + : null; +} + export interface PruneOptions { keepCount: number; keepDays: number; @@ -418,15 +456,36 @@ export class ConfigBackupCorruptError extends Error {} * writes the file as 0600: a symlink, or a file with a group/world bit, was planted or loosened * after the snapshot, and rewriting it as 0600 would turn it into trusted config (`clis.json` * selects the binaries Codeman spawns; `isUnsafePermissions` refuses the same bits there). - * POSIX only, like `isUnsafePermissions`. + * POSIX only, like `isUnsafePermissions`. The mode test is skipped (the symlink test is not) on a + * filesystem that does not keep modes, see {@link folderKeepsModes}. */ -function assertTrustedSnapshotEntry(path: string, label: string): void { +function assertTrustedSnapshotEntry(path: string, label: string, checkMode: boolean): void { if (process.platform === 'win32') return; const st = lstatSync(path, { throwIfNoEntry: false }); if (!st) return; // missing is reported by the read that follows - if (st.isSymbolicLink()) throw new ConfigBackupCorruptError(`${label} is a symlink, so it is not restored`); - if ((st.mode & 0o077) !== 0) { - throw new ConfigBackupCorruptError(`${label} is readable or writable by group or others, so it is not restored`); + if (st.isSymbolicLink()) throw new ConfigBackupCorruptError(`${label} is a symlink, so it is not used`); + if (checkMode && (st.mode & 0o077) !== 0) { + throw new ConfigBackupCorruptError(`${label} is readable or writable by group or others, so it is not used`); + } +} + +/** + * Whether the backup folder's filesystem keeps a file's mode: vfat/exFAT, CIFS shares and WSL's + * `/mnt/d` report 0755 or 0777 for everything, so the group/world test would refuse every backup + * there even though snapshots succeed (`createConfigBackup` already expects mode-less filesystems). + * Probed with a 0600 temp file; if the probe cannot run, the strict test stays on. + */ +function folderKeepsModes(dir: string): boolean { + if (process.platform === 'win32') return false; + const probe = join(dir, `.mode-probe-${process.pid}`); + try { + writeFileSync(probe, '', { mode: 0o600 }); + chmodSync(probe, 0o600); + return (lstatSync(probe).mode & 0o777 & 0o077) === 0; + } catch { + return true; + } finally { + rmSync(probe, { force: true }); } } @@ -471,17 +530,21 @@ export function restoreConfigBackup(opts: { id: string; appVersion?: string; now?: number; + /** Override the probe of whether the backup folder keeps file modes (tests; a mode-less filesystem). */ + keepsModes?: boolean; }): RestoreResult { - const manifest = readManifest(opts.backupDir, opts.id, configBackupOrigin(opts.dataDir)); + const origin = readConfigBackupOrigin(opts.dataDir); + const manifest = origin ? readManifest(opts.backupDir, opts.id, origin) : null; if (!manifest) throw new ConfigBackupNotFoundError(`No config backup named ${opts.id}`); const allowed = new Set(CONFIG_BACKUP_FILES); const loaded: Array<{ name: string; data: Buffer }> = []; - assertTrustedSnapshotEntry(join(opts.backupDir, opts.id), `Backup ${opts.id}`); + const checkMode = opts.keepsModes ?? folderKeepsModes(opts.backupDir); + assertTrustedSnapshotEntry(join(opts.backupDir, opts.id), `Backup ${opts.id}`, checkMode); for (const f of manifest.files) { if (!allowed.has(f.name)) continue; let data: Buffer; - assertTrustedSnapshotEntry(join(opts.backupDir, opts.id, f.name), `${f.name} in ${opts.id}`); + assertTrustedSnapshotEntry(join(opts.backupDir, opts.id, f.name), `${f.name} in ${opts.id}`, checkMode); try { data = readFileSync(join(opts.backupDir, opts.id, f.name)); } catch { @@ -661,12 +724,16 @@ function loadDiffSide(side: string, dataDir: string, backupDir: string): Map(CONFIG_BACKUP_FILES); + const checkMode = folderKeepsModes(backupDir); + assertTrustedSnapshotEntry(join(backupDir, side), `Backup ${side}`, checkMode); for (const f of manifest.files) { if (!allowed.has(f.name)) continue; let data: Buffer; + assertTrustedSnapshotEntry(join(backupDir, side, f.name), `${f.name} in ${side}`, checkMode); try { data = readFileSync(join(backupDir, side, f.name)); } catch { diff --git a/src/web/routes/config-backup-routes.ts b/src/web/routes/config-backup-routes.ts index b250437d..1a1cc3e4 100644 --- a/src/web/routes/config-backup-routes.ts +++ b/src/web/routes/config-backup-routes.ts @@ -14,6 +14,7 @@ * may have cached: `restartRequired` says which ones only apply after a restart. */ +import { readFile } from 'node:fs/promises'; import { createRequire } from 'node:module'; import type { FastifyInstance, FastifyReply, FastifyRequest } from 'fastify'; import { ApiErrorCode, createErrorResponse, getErrorMessage, type ApiResponse } from '../../types.js'; @@ -39,6 +40,9 @@ import { resolveConfigBackupDir, restoreConfigBackup, configBackupOrigin, + hasConfigBackupSettings, + latestBackupRetention, + readConfigBackupOrigin, sweepConfigBackupTemps, type ConfigBackupResult, type ConfigBackupSettings, @@ -96,27 +100,51 @@ export async function probeBackupDir(settings: ConfigBackupSettings, dir: string return false; } +async function readSettingsForBackup(): Promise | 'unreadable'> { + try { + const parsed: unknown = JSON.parse(await readFile(SETTINGS_PATH, 'utf8')); + return parsed && typeof parsed === 'object' && !Array.isArray(parsed) + ? (parsed as Record) + : 'unreadable'; + } catch (err) { + return (err as NodeJS.ErrnoException).code === 'ENOENT' ? {} : 'unreadable'; + } +} + /** Snapshot (when changed) and apply retention; shared by the timer and the manual route. */ export async function runConfigBackup( reason: string, opts: { force?: boolean } = {} ): Promise<{ result: ConfigBackupResult; removed: string[] } | null> { - const { settings, dir } = await readEffectiveBackupSettings(); - if (!settings.enabled && !opts.force) return null; - await probeBackupDir(settings, dir); + // Read settings.json directly: readJsonConfig answers {} for ANY failure, so a syntax error or a read + // that lands inside PUT /api/settings's non-atomic write used to prune with the defaults (20 / 30 + // days) and drop the longer history the user asked to keep. + const raw = await readSettingsForBackup(); + const unreadable = raw === 'unreadable'; + const fromFile = readConfigBackupSettings(unreadable ? {} : raw); + if (!fromFile.enabled && !opts.force) return null; + const dir = resolveConfigBackupDir(fromFile, getDataDir()); + await probeBackupDir(fromFile, dir); sweepConfigBackupTemps(dir); + const origin = configBackupOrigin(getDataDir()); + // No backup settings in the file (reset, or unreadable): keep the retention the newest snapshot recorded. + const settings = + unreadable || !hasConfigBackupSettings(raw) + ? { ...fromFile, ...(latestBackupRetention(dir, origin) ?? {}) } + : fromFile; const result = createConfigBackup({ dataDir: getDataDir(), backupDir: dir, reason, appVersion: APP_VERSION, force: opts.force, - }); - const removed = pruneConfigBackups(dir, { keepCount: settings.keepCount, keepDays: settings.keepDays, - origin: configBackupOrigin(getDataDir()), }); + // An unreadable settings.json still gets its snapshot, but no pass deletes anything on its say-so. + const removed = unreadable + ? [] + : pruneConfigBackups(dir, { keepCount: settings.keepCount, keepDays: settings.keepDays, origin }); return { result, removed }; } @@ -133,6 +161,8 @@ export function registerConfigBackupRoutes(app: FastifyInstance): void { const denied = deny(req, reply); if (denied) return denied; const { settings, dir } = await readEffectiveBackupSettings(); + // Read-only: a listing must not create config-backup-id, and without one nothing can be ours. + const origin = readConfigBackupOrigin(getDataDir()); let present: boolean; try { present = await probeBackupDir(settings, dir); @@ -147,7 +177,7 @@ export function registerConfigBackupRoutes(app: FastifyInstance): void { dir, defaultDir: defaultConfigBackupDir(getDataDir()), tracked: CONFIG_BACKUP_FILES, - backups: present ? listConfigBackups(dir, configBackupOrigin(getDataDir())) : [], + backups: present && origin ? listConfigBackups(dir, origin) : [], }, }; }); diff --git a/test/config-backup.test.ts b/test/config-backup.test.ts index 0a24ceea..fe56474f 100644 --- a/test/config-backup.test.ts +++ b/test/config-backup.test.ts @@ -526,6 +526,30 @@ describe('restore trusts only what this module wrote', () => { expect(listConfigBackups(backups)).toHaveLength(1); }); + it('skips the group/world bit test on a filesystem that does not keep modes, but never the symlink test', () => { + put('settings.json', '{"a":1}'); + put('clis.json', '{"clis":{}}'); + const { id } = made(); + const file = join(backups, id, 'clis.json'); + chmodSync(file, 0o644); // what vfat, CIFS and WSL /mnt/d report for every file + expect(() => restoreConfigBackup({ dataDir: data, backupDir: backups, id, keepsModes: false })).not.toThrow(); + + const real = join(root, 'elsewhere.json'); + writeFileSync(real, '{"clis":{}}'); + rmSync(file); + symlinkSync(real, file); + expect(() => restoreConfigBackup({ dataDir: data, backupDir: backups, id, keepsModes: false })).toThrow(/symlink/); + }); + + it('compare applies the same check to a snapshot file', () => { + put('settings.json', '{"a":1}'); + const { id } = made(); + chmodSync(join(backups, id, 'settings.json'), 0o644); + expect(() => diffConfigBackups({ dataDir: data, backupDir: backups, from: id, to: 'current' })).toThrow( + ConfigBackupCorruptError + ); + }); + it('refuses a snapshot folder that others can write to', () => { put('settings.json', '{"a":1}'); const { id } = made(); diff --git a/test/routes/config-backup-routes.test.ts b/test/routes/config-backup-routes.test.ts index e5e7b79c..1dc1704b 100644 --- a/test/routes/config-backup-routes.test.ts +++ b/test/routes/config-backup-routes.test.ts @@ -9,10 +9,10 @@ import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync import { tmpdir } from 'node:os'; import { dirname, join } from 'node:path'; import { createRouteTestHarness } from './_route-test-utils.js'; -import { registerConfigBackupRoutes } from '../../src/web/routes/config-backup-routes.js'; +import { registerConfigBackupRoutes, runConfigBackup } from '../../src/web/routes/config-backup-routes.js'; import { SETTINGS_PATH } from '../../src/web/route-helpers.js'; import { getDataDir } from '../../src/config/instance.js'; -import { defaultConfigBackupDir } from '../../src/config-backup.js'; +import { createConfigBackup, defaultConfigBackupDir, listConfigBackups } from '../../src/config-backup.js'; import { intentStore } from '../../src/intent-store.js'; let custom: string; @@ -183,3 +183,61 @@ describe('GET /api/config-backups/diff', () => { expect(missing.statusCode).toBe(404); }); }); + +describe('retention survives an unreadable or reset settings.json', () => { + const DAY = 86_400_000; + const BASE = Date.UTC(2025, 0, 1); + const dir = () => defaultConfigBackupDir(getDataDir()); + /** 30 snapshots a day apart, made under a long-history retention (100 copies, no age limit). */ + function seed() { + settings({ configBackupKeepCount: 100, configBackupKeepDays: 0 }); + for (let i = 0; i < 30; i++) { + writeFileSync( + SETTINGS_PATH, + JSON.stringify({ displayName: `v${i}`, configBackupKeepCount: 100, configBackupKeepDays: 0 }) + ); + createConfigBackup({ + dataDir: getDataDir(), + backupDir: dir(), + reason: 'auto', + now: BASE + i * DAY, + keepCount: 100, + keepDays: 0, + }); + } + expect(listConfigBackups(dir())).toHaveLength(30); + } + + it('a settings.json that does not parse still gets a snapshot but prunes nothing', async () => { + seed(); + writeFileSync(SETTINGS_PATH, '{ "displayName": "half-written", }'); + const out = await runConfigBackup('auto'); + expect(out?.result.status).toBe('created'); + expect(out?.removed).toEqual([]); + expect(listConfigBackups(dir())).toHaveLength(31); + }); + + it('a reset settings.json prunes with the retention the newest snapshot recorded, not the defaults', async () => { + seed(); + writeFileSync(SETTINGS_PATH, '{}'); + const out = await runConfigBackup('auto'); + expect(out?.result.status).toBe('created'); + // aged manifests (a year old) and keepDays 0: the defaults (20 copies, 30 days) would leave 20 + expect(listConfigBackups(dir())).toHaveLength(31); + }); + + it('a settings.json that sets the retention still wins', async () => { + seed(); + writeFileSync(SETTINGS_PATH, JSON.stringify({ configBackupKeepCount: 5, configBackupKeepDays: 0 })); + await runConfigBackup('auto'); + expect(listConfigBackups(dir())).toHaveLength(5); + }); + + it('listing never creates config-backup-id', async () => { + const { app } = await createRouteTestHarness(registerConfigBackupRoutes); + const idFile = join(getDataDir(), 'config-backup-id'); + rmSync(idFile, { force: true }); + expect((await app.inject({ method: 'GET', url: '/api/config-backups' })).json().data.backups).toEqual([]); + expect(existsSync(idFile)).toBe(false); + }); +});