From c5f1e990367d4da536258ee40f341d62d1768dbe Mon Sep 17 00:00:00 2001 From: Devvyn <22340871+opticon454@users.noreply.github.com> Date: Tue, 20 Oct 2026 16:47:49 +0800 Subject: [PATCH] fix(config-backup): ignore a web tab's lastOpenedAt, audit multi-user restores, refuse symlinked or loosened snapshots, keep the folder settings on restore --- docs/wiki/Config-Backups.md | 17 +++++-- src/config-backup.ts | 48 +++++++++++++++++++- src/web/routes/config-backup-routes.ts | 13 +++++- test/config-backup.test.ts | 58 ++++++++++++++++++++++++ test/routes/config-backup-routes.test.ts | 16 +++++++ 5 files changed, 146 insertions(+), 6 deletions(-) diff --git a/docs/wiki/Config-Backups.md b/docs/wiki/Config-Backups.md index a11be141..4ba6a999 100644 --- a/docs/wiki/Config-Backups.md +++ b/docs/wiki/Config-Backups.md @@ -42,7 +42,8 @@ cron jobs. were): `custom-model-hosts.json` `lastDiscoveredAt` (the model-discovery poll, every five minutes), `users.json` `lastLoginAt`, `settings.json` `lastUsedCase`, `intents.json` `recentPrompts` and `updatedAt` (every captured prompt) and `docker-cases.json` - `lastClaudeSessionId` (a Docker case's conversation id after a `/clear`). Without that, every + `lastClaudeSessionId` (a Docker case's conversation id after a `/clear`) and `webviews.json` + `lastOpenedAt` (opening a web tab). Without that, every tick looked like an edit and 20 copies covered only about 100 minutes. So Compare shows `intents.json` as unchanged when only prompts differ, yet a restore drops the prompts captured since. @@ -122,13 +123,19 @@ list and **Back up now** use the **saved** folder and retention: apply your edit 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. + 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). 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`. Only the tracked names are ever written, whatever a manifest lists. +Restoring `settings.json` keeps the four backup settings (on or off, folder, how many, how many +days) at their current values, so a restore from before a folder change does not point the list +at the old folder and hide the `pre-restore` copy it just made. + Settings are re-read on the next request, so reload the page. `.env` and `push-keys.json` are read once per process: **restart Codeman** for those to take effect (the response lists which). `clis.json` is reloaded by the restore itself and `users.json` is re-read within a second. Restoring `settings.json` also does not re-run the switches @@ -176,8 +183,12 @@ query and fragment. - **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. +- **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. - In [multi-user mode](Multi-User-Mode) all of it is admin only: the list, Back up now, - Restore and Compare. + Restore and Compare, and each restore is written to the admin audit log + (`config_backup_restore`). - Listing returns file names and sizes, never contents. A comparison returns changed values but never a credential's (see [Comparing backups](#comparing-backups)). diff --git a/src/config-backup.ts b/src/config-backup.ts index 4c6511e4..1189c2d7 100644 --- a/src/config-backup.ts +++ b/src/config-backup.ts @@ -206,11 +206,12 @@ const VOLATILE_KEYS: Readonly> = { 'custom-model-hosts.json': ['lastDiscoveredAt'], // Rewritten by activity, not by anything the user set: a Basic-auth login (at most once a minute // per user), a session started in another case, every captured prompt, and a Docker case's - // conversation id after a /clear. + // conversation id after a /clear, and a web tab being opened. 'users.json': ['lastLoginAt'], 'settings.json': ['lastUsedCase'], 'intents.json': ['recentPrompts', 'updatedAt'], 'docker-cases.json': ['lastClaudeSessionId'], + 'webviews.json': ['lastOpenedAt'], }; function withoutKeys(value: unknown, drop: ReadonlySet): unknown { @@ -412,6 +413,45 @@ export function pruneConfigBackups(dir: string, opts: PruneOptions): string[] { export class ConfigBackupNotFoundError extends Error {} export class ConfigBackupCorruptError extends Error {} +/** + * Restore refuses what this module never writes. Snapshots are 0700/0600 real files, and a restore + * 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`. + */ +function assertTrustedSnapshotEntry(path: string, label: string): 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`); + } +} + +/** The folder's own backup settings stay as they are now: restoring an old `configBackupDir` hides the `pre-restore` copy. */ +const BACKUP_SETTING_KEYS = ['configBackupEnabled', 'configBackupDir', 'configBackupKeepCount', 'configBackupKeepDays']; + +function keepCurrentBackupSettings(snapshot: Buffer, livePath: string): Buffer { + try { + const restored = JSON.parse(snapshot.toString('utf8')) as Record; + const live = JSON.parse(readFileSync(livePath, 'utf8')) as Record; + if (!restored || typeof restored !== 'object' || !live || typeof live !== 'object') return snapshot; + let changed = false; + for (const key of BACKUP_SETTING_KEYS) { + if (JSON.stringify(restored[key]) === JSON.stringify(live[key])) continue; + changed = true; + if (key in live) restored[key] = live[key]; + else delete restored[key]; + } + if (!changed) return snapshot; // byte-identical to the snapshot when the folder settings did not move + return Buffer.from(JSON.stringify(restored, null, 2) + '\n'); + } catch { + return snapshot; // not JSON, or no live file to take the values from + } +} + export interface RestoreResult { restored: string[]; /** Tracked files present now that the snapshot did not hold; a restore leaves them as they are. */ @@ -437,9 +477,11 @@ export function restoreConfigBackup(opts: { const allowed = new Set(CONFIG_BACKUP_FILES); const loaded: Array<{ name: string; data: Buffer }> = []; + assertTrustedSnapshotEntry(join(opts.backupDir, opts.id), `Backup ${opts.id}`); 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}`); try { data = readFileSync(join(opts.backupDir, opts.id, f.name)); } catch { @@ -466,7 +508,9 @@ export function restoreConfigBackup(opts: { const target = join(opts.dataDir, f.name); const tmp = `${target}.restore-${process.pid}`; try { - writeFileSync(tmp, f.data, { mode: 0o600 }); + writeFileSync(tmp, f.name === 'settings.json' ? keepCurrentBackupSettings(f.data, target) : f.data, { + mode: 0o600, + }); renameSync(tmp, target); } catch (err) { rmSync(tmp, { force: true }); diff --git a/src/web/routes/config-backup-routes.ts b/src/web/routes/config-backup-routes.ts index 1ecf9e03..b250437d 100644 --- a/src/web/routes/config-backup-routes.ts +++ b/src/web/routes/config-backup-routes.ts @@ -17,7 +17,8 @@ import { createRequire } from 'node:module'; import type { FastifyInstance, FastifyReply, FastifyRequest } from 'fastify'; import { ApiErrorCode, createErrorResponse, getErrorMessage, type ApiResponse } from '../../types.js'; -import { isAdmin, readJsonConfig, SETTINGS_PATH } from '../route-helpers.js'; +import { getAuthUser, isAdmin, readJsonConfig, SETTINGS_PATH } from '../route-helpers.js'; +import { appendAdminAudit } from '../admin-audit.js'; import { reloadCliRegistry } from '../../config/cli-registry/registry.js'; import { isMultiUserMode } from '../../config/multiuser.js'; import { getDataDir } from '../../config/instance.js'; @@ -207,6 +208,16 @@ export function registerConfigBackupRoutes(app: FastifyInstance): void { if (out.restored.includes('intents.json')) intentStore.reload(); // restoreConfigBackup writes clis.json directly (the one writer besides mutateRegistryFile). if (out.restored.includes('clis.json')) reloadCliRegistry(); + // A restore of users.json replaces accounts, password hashes, roles and bypass grants. + if (isMultiUserMode()) { + void appendAdminAudit({ + admin: getAuthUser(req).username, + action: 'config_backup_restore', + target: id, + ip: req.ip, + detail: { restored: out.restored }, + }); + } return { success: true, data: { diff --git a/test/config-backup.test.ts b/test/config-backup.test.ts index 5089e8c6..0a24ceea 100644 --- a/test/config-backup.test.ts +++ b/test/config-backup.test.ts @@ -11,6 +11,7 @@ import { readdirSync, readFileSync, rmSync, + symlinkSync, statSync, writeFileSync, } from 'node:fs'; @@ -148,6 +149,22 @@ describe('createConfigBackup', () => { }); it.each([ + [ + 'webviews.json', + (n: number) => + JSON.stringify([ + { + id: 'w1', + name: 'Grafana', + url: 'http://localhost:3001', + embedMode: 'proxy', + trusted: false, + createdAt: 1, + lastOpenedAt: n, + }, + ]), + 'name', + ], [ 'docker-cases.json', (n: number) => JSON.stringify([{ name: 'app', hostId: 'h', lastClaudeSessionId: `c${n}` }]), @@ -486,3 +503,44 @@ describe('instances sharing one backup folder', () => { expect(existsSync(stale)).toBe(false); }); }); + +describe('restore trusts only what this module wrote', () => { + const made = () => + createConfigBackup({ dataDir: data, backupDir: backups, reason: 'auto', now: T0 }) as { id: string }; + + it('refuses a symlinked snapshot file and a snapshot file with group or world bits', () => { + put('settings.json', '{"a":1}'); + put('clis.json', '{"clis":{}}'); + const { id } = made(); + const file = join(backups, id, 'clis.json'); + chmodSync(file, 0o644); + expect(() => restoreConfigBackup({ dataDir: data, backupDir: backups, id })).toThrow(ConfigBackupCorruptError); + + chmodSync(file, 0o600); + const real = join(root, 'elsewhere.json'); + writeFileSync(real, '{"clis":{}}'); + rmSync(file); + symlinkSync(real, file); + expect(() => restoreConfigBackup({ dataDir: data, backupDir: backups, id })).toThrow(/symlink/); + // nothing was written and no pre-restore copy was taken for a refused restore + expect(listConfigBackups(backups)).toHaveLength(1); + }); + + it('refuses a snapshot folder that others can write to', () => { + put('settings.json', '{"a":1}'); + const { id } = made(); + chmodSync(join(backups, id), 0o777); + expect(() => restoreConfigBackup({ dataDir: data, backupDir: backups, id })).toThrow(ConfigBackupCorruptError); + chmodSync(join(backups, id), 0o700); + expect(() => restoreConfigBackup({ dataDir: data, backupDir: backups, id })).not.toThrow(); + }); + + it('keeps the current folder and retention when settings.json is restored', () => { + put('settings.json', JSON.stringify({ displayName: 'old', configBackupDir: '/old', configBackupKeepCount: 5 })); + const { id } = made(); + put('settings.json', JSON.stringify({ displayName: 'new', configBackupDir: '/new', configBackupKeepDays: 7 })); + restoreConfigBackup({ dataDir: data, backupDir: backups, id }); + const after = JSON.parse(readFileSync(join(data, 'settings.json'), 'utf8')); + expect(after).toEqual({ displayName: 'old', configBackupDir: '/new', configBackupKeepDays: 7 }); + }); +}); diff --git a/test/routes/config-backup-routes.test.ts b/test/routes/config-backup-routes.test.ts index a0a2bc17..e5e7b79c 100644 --- a/test/routes/config-backup-routes.test.ts +++ b/test/routes/config-backup-routes.test.ts @@ -137,6 +137,22 @@ describe('/api/config-backups', () => { } }); + it('multi-user: a restore is written to the admin audit log', async () => { + process.env.CODEMAN_MULTIUSER = '1'; + const { app } = await createRouteTestHarness(registerConfigBackupRoutes, { + authUser: { username: 'root', role: 'admin' }, + }); + const id = (await app.inject({ method: 'POST', url: '/api/config-backups' })).json().data.id; + await app.inject({ method: 'POST', url: `/api/config-backups/${id}/restore` }); + const auditPath = join(getDataDir(), 'admin-audit.jsonl'); + const written = () => existsSync(auditPath) && readFileSync(auditPath, 'utf8').includes('config_backup_restore'); + for (let i = 0; i < 40 && !written(); i++) await new Promise((r) => setTimeout(r, 25)); + const last = JSON.parse(readFileSync(auditPath, 'utf8').trim().split('\n').pop()!); + expect(last).toMatchObject({ admin: 'root', action: 'config_backup_restore', target: id }); + expect(last.detail.restored).toContain('settings.json'); + rmSync(auditPath, { force: true }); + }); + it('multi-user: an admin is allowed', async () => { process.env.CODEMAN_MULTIUSER = '1'; const { app } = await createRouteTestHarness(registerConfigBackupRoutes, {