diff --git a/.changeset/settings-write-lock-1039.md b/.changeset/settings-write-lock-1039.md new file mode 100644 index 000000000..6933b07a8 --- /dev/null +++ b/.changeset/settings-write-lock-1039.md @@ -0,0 +1,5 @@ +--- +"nostream": patch +--- + +fix(admin): serialize every settings-file writer with the settings write lock diff --git a/src/cli/commands/config.ts b/src/cli/commands/config.ts index c851ff7ce..6391de710 100644 --- a/src/cli/commands/config.ts +++ b/src/cli/commands/config.ts @@ -11,6 +11,7 @@ import { toCategoryLabel, validatePathAgainstDefaults, validateSettings, + withSettingsWriteLock, } from '../../utils/settings-config' import { isSecretEnvKey, @@ -128,25 +129,38 @@ export const runConfigSet = async ( return 1 } - const settings = loadUserSettings() as unknown as Record - const next = setByPath(settings, path, parseTypedValue(rawValue, valueType)) + // Read and write under the settings-file lock so a CLI update cannot clobber + // a moderation ban written by a running relay in between. + const outcome = await withSettingsWriteLock< + | { ok: true } + | { ok: false; validationIssues: Array<{ path: string; message: string }> } + >(() => { + const settings = loadUserSettings() as unknown as Record + const next = setByPath(settings, path, parseTypedValue(rawValue, valueType)) + + if (options.validate !== false) { + const merged = loadMergedSettings() as unknown as Record + const mergedNext = setByPath(merged, path, getByPath(next, path)) + const validationIssues = validateSettings(mergedNext as any) + + if (validationIssues.length > 0) { + return { ok: false as const, validationIssues } + } + } - if (options.validate !== false) { - const merged = loadMergedSettings() as unknown as Record - const mergedNext = setByPath(merged, path, getByPath(next, path)) - const validationIssues = validateSettings(mergedNext as any) + saveSettings(next as any) - if (validationIssues.length > 0) { - logError('Config update rejected by validation:') - for (const issue of validationIssues) { - logError(`- ${issue.path}: ${issue.message}`) - } + return { ok: true as const } + }) - return 1 + if ('validationIssues' in outcome) { + logError('Config update rejected by validation:') + for (const issue of outcome.validationIssues) { + logError(`- ${issue.path}: ${issue.message}`) } - } - saveSettings(next as any) + return 1 + } logInfo(`Updated ${path}`) diff --git a/src/controllers/admin/patch-notifications-controller.ts b/src/controllers/admin/patch-notifications-controller.ts index 2e3545d49..d64e1dabb 100644 --- a/src/controllers/admin/patch-notifications-controller.ts +++ b/src/controllers/admin/patch-notifications-controller.ts @@ -19,6 +19,7 @@ import { loadUserSettings, saveSettings, validateSettings, + withSettingsWriteLock, } from '../../utils/settings-config' import { validateSchema } from '../../utils/validation' import { recordAdminAuditFromRequest } from '../../utils/record-admin-audit' @@ -35,27 +36,44 @@ export class PatchAdminNotificationsController implements IController { return } - const merged = loadMergedSettings() - const current = getMergedAdminNotifications(merged) - const nextNotifications = mergeAdminNotificationsPatch(current, validation.value) + // Read and write under the same settings-file lock the moderation writer + // uses, so a notification save cannot clobber a ban written meanwhile. + const outcome = await withSettingsWriteLock< + | { ok: true; redacted: ReturnType } + | { ok: false; issues: Array<{ path: string; message: string }> } + >(() => { + const merged = loadMergedSettings() + const current = getMergedAdminNotifications(merged) + const nextNotifications = mergeAdminNotificationsPatch(current, validation.value) - const userSettings = loadUserSettings() as Settings - const nextUserSettings = mergeDeepRight(userSettings, { - admin: { - ...userSettings.admin, - notifications: nextNotifications, - }, - }) as Settings + const userSettings = loadUserSettings() as Settings + const nextUserSettings = mergeDeepRight(userSettings, { + admin: { + ...userSettings.admin, + notifications: nextNotifications, + }, + }) as Settings - const mergedNext = mergeDeepRight(loadDefaults(), nextUserSettings) as Settings + const mergedNext = mergeDeepRight(loadDefaults(), nextUserSettings) as Settings - const issues = validateSettings(mergedNext) - if (issues.length > 0) { - response.status(400).setHeader('content-type', 'application/json').send({ error: 'Validation failed', issues }) + const issues = validateSettings(mergedNext) + if (issues.length > 0) { + return { ok: false as const, issues } + } + + saveSettings(nextUserSettings) + + return { ok: true as const, redacted: getRedactedAdminNotifications() } + }) + + if ('issues' in outcome) { + response + .status(400) + .setHeader('content-type', 'application/json') + .send({ error: 'Validation failed', issues: outcome.issues }) return } - saveSettings(nextUserSettings) appendSettingsAuditLog({ action: 'settings.updated', changes: [{ path: 'admin.notifications', reload: 'hot-reload' }], @@ -74,7 +92,7 @@ export class PatchAdminNotificationsController implements IController { response.status(200).setHeader('content-type', 'application/json').send({ ok: true, - notifications: getRedactedAdminNotifications(), + notifications: outcome.redacted, }) } } diff --git a/src/controllers/admin/patch-settings-controller.ts b/src/controllers/admin/patch-settings-controller.ts index f67eb099e..ac624c6e7 100644 --- a/src/controllers/admin/patch-settings-controller.ts +++ b/src/controllers/admin/patch-settings-controller.ts @@ -12,6 +12,7 @@ import { setByPath, validatePathAgainstDefaults, validateSettings, + withSettingsWriteLock, } from '../../utils/settings-config' import { isWriteProtectedSettingsPath, @@ -50,27 +51,43 @@ export class PatchAdminSettingsController implements IController { return } - const userSettings = loadUserSettings() as unknown as Record - const nextUserSettings = changes.reduce( - (settings, change) => setByPath(settings, change.path, change.value), - userSettings, - ) - const merged = loadMergedSettings() as unknown as Record - const mergedNext = changes.reduce( - (settings, change) => setByPath(settings, change.path, getByPath(nextUserSettings, change.path)), - merged, - ) - const validationIssues = validateSettings(mergedNext as unknown as Settings) - - if (validationIssues.length > 0) { + // Every writer of settings.yaml takes this lock. Reading inside it is what + // stops an ordinary settings save from writing a document read before a + // moderation ban was applied, which would silently undo the ban. + const outcome = await withSettingsWriteLock< + | { ok: true; nextUserSettings: Record } + | { ok: false; issues: Array<{ path: string; message: string }> } + >(() => { + const userSettings = loadUserSettings() as unknown as Record + const nextUserSettings = changes.reduce( + (settings, change) => setByPath(settings, change.path, change.value), + userSettings, + ) + const merged = loadMergedSettings() as unknown as Record + const mergedNext = changes.reduce( + (settings, change) => setByPath(settings, change.path, getByPath(nextUserSettings, change.path)), + merged, + ) + const validationIssues = validateSettings(mergedNext as unknown as Settings) + + if (validationIssues.length > 0) { + return { ok: false as const, issues: validationIssues } + } + + saveSettings(nextUserSettings as unknown as Settings) + + return { ok: true as const, nextUserSettings } + }) + + if ('issues' in outcome) { response .status(400) .setHeader('content-type', 'application/json') - .send({ error: 'Validation failed', issues: validationIssues }) + .send({ error: 'Validation failed', issues: outcome.issues }) return } - saveSettings(nextUserSettings as unknown as Settings) + const nextUserSettings = outcome.nextUserSettings const updatedChanges = changes.map(({ path }) => ({ path, value: redactSettingsValue(path, getByPath(nextUserSettings, path)), diff --git a/src/controllers/admin/post-settings-restore-controller.ts b/src/controllers/admin/post-settings-restore-controller.ts index aded18e40..1bb3c65e9 100644 --- a/src/controllers/admin/post-settings-restore-controller.ts +++ b/src/controllers/admin/post-settings-restore-controller.ts @@ -15,7 +15,7 @@ export class PostAdminSettingsRestoreController implements IController { } try { - restoreSettingsBackup(validation.value.filename) + await restoreSettingsBackup(validation.value.filename) appendSettingsAuditLog({ action: 'settings.restored', filename: validation.value.filename, diff --git a/src/utils/settings-config.ts b/src/utils/settings-config.ts index 7f326f4e4..65679c223 100644 --- a/src/utils/settings-config.ts +++ b/src/utils/settings-config.ts @@ -585,24 +585,26 @@ export const listSettingsBackups = (): SettingsBackupInfo[] => { .sort((left, right) => right.createdAt.localeCompare(left.createdAt)) } -export const restoreSettingsBackup = (filename: string): Settings => { +export const restoreSettingsBackup = async (filename: string): Promise => { if (!isSafeBackupFilename(filename)) { throw new Error('Invalid backup filename') } - const backupPath = join(getSettingsBackupDir(), filename) - if (!fs.existsSync(backupPath)) { - throw new Error('Backup not found') - } + return withSettingsWriteLock(() => { + const backupPath = join(getSettingsBackupDir(), filename) + if (!fs.existsSync(backupPath)) { + throw new Error('Backup not found') + } - const restored = yaml.load(fs.readFileSync(backupPath, 'utf-8')) as Settings - const issues = validateSettings(mergeDeepRight(loadDefaults(), restored) as Settings) - if (issues.length > 0) { - throw new Error(`Backup failed validation: ${issues.map((issue) => issue.path).join(', ')}`) - } + const restored = yaml.load(fs.readFileSync(backupPath, 'utf-8')) as Settings + const issues = validateSettings(mergeDeepRight(loadDefaults(), restored) as Settings) + if (issues.length > 0) { + throw new Error(`Backup failed validation: ${issues.map((issue) => issue.path).join(', ')}`) + } - saveSettings(restored) - return restored + saveSettings(restored) + return restored + }) } export const getTopLevelSettingCategories = (): string[] => { diff --git a/test/unit/controllers/admin/patch-notifications-controller.spec.ts b/test/unit/controllers/admin/patch-notifications-controller.spec.ts index 9698bcfae..a7b649fe7 100644 --- a/test/unit/controllers/admin/patch-notifications-controller.spec.ts +++ b/test/unit/controllers/admin/patch-notifications-controller.spec.ts @@ -47,6 +47,7 @@ describe('PatchAdminNotificationsController', () => { send: sandbox.stub().returnsThis(), } + sandbox.stub(settingsConfig, 'withSettingsWriteLock').callsFake(async (operation: () => unknown) => operation()) sandbox.stub(settingsConfig, 'loadMergedSettings').returns(baseMergedSettings as any) sandbox.stub(settingsConfig, 'loadUserSettings').returns({ admin: { notifications: baseNotifications } } as any) sandbox.stub(settingsConfig, 'saveSettings') @@ -130,6 +131,30 @@ describe('PatchAdminNotificationsController', () => { expect(response.send.firstCall.args[0].ok).to.equal(true) }) + it('reads and saves inside the settings write lock', async () => { + const order: string[] = [] + ;(settingsConfig.withSettingsWriteLock as Sinon.SinonStub).callsFake(async (operation: () => unknown) => { + order.push('lock:enter') + try { + return operation() + } finally { + order.push('lock:exit') + } + }) + ;(settingsConfig.loadUserSettings as Sinon.SinonStub).callsFake(() => { + order.push('read') + return { admin: { notifications: baseNotifications } } + }) + ;(settingsConfig.saveSettings as Sinon.SinonStub).callsFake(() => { + order.push('save') + }) + + const controller = new PatchAdminNotificationsController(outboxRepository as any) + await controller.handleRequest({ body: { enabled: true }, ip: '127.0.0.1' } as any, response as any) + + expect(order).to.deep.equal(['lock:enter', 'read', 'save', 'lock:exit']) + }) + it('still returns 200 when outbox enqueue fails', async () => { outboxRepository.enqueue.rejects(new Error('db unavailable')) const controller = new PatchAdminNotificationsController(outboxRepository as any) diff --git a/test/unit/controllers/admin/patch-settings-controller.spec.ts b/test/unit/controllers/admin/patch-settings-controller.spec.ts new file mode 100644 index 000000000..70dad7f3f --- /dev/null +++ b/test/unit/controllers/admin/patch-settings-controller.spec.ts @@ -0,0 +1,150 @@ +import chai from 'chai' +import Sinon from 'sinon' +import sinonChai from 'sinon-chai' + +import * as recordAdminAudit from '../../../../src/utils/record-admin-audit' +import * as settingsConfig from '../../../../src/utils/settings-config' + +// eslint-disable-next-line @typescript-eslint/no-var-requires +const { PatchAdminSettingsController } = require('../../../../src/controllers/admin/patch-settings-controller') + +chai.use(sinonChai) + +const { expect } = chai + +const baseSettings = { + payments: { enabled: false }, + info: { relay_url: 'wss://relay.example', name: 'test' }, +} + +describe('PatchAdminSettingsController settings write lock', () => { + let sandbox: Sinon.SinonSandbox + let outboxRepository: { enqueue: Sinon.SinonStub } + let response: { status: Sinon.SinonStub; setHeader: Sinon.SinonStub; send: Sinon.SinonStub } + let request: Record + + beforeEach(() => { + sandbox = Sinon.createSandbox() + outboxRepository = { enqueue: sandbox.stub().resolves() } + response = { + status: sandbox.stub().returnsThis(), + setHeader: sandbox.stub().returnsThis(), + send: sandbox.stub().returnsThis(), + } + request = { body: { path: 'payments.enabled', value: true }, ip: '127.0.0.1' } + + sandbox.stub(settingsConfig, 'withSettingsWriteLock').callsFake(async (operation: () => unknown) => operation()) + sandbox.stub(settingsConfig, 'loadUserSettings').returns(baseSettings as any) + sandbox.stub(settingsConfig, 'loadMergedSettings').returns(baseSettings as any) + sandbox.stub(settingsConfig, 'saveSettings') + sandbox.stub(settingsConfig, 'appendSettingsAuditLog') + sandbox.stub(settingsConfig, 'validatePathAgainstDefaults').returns([]) + sandbox.stub(settingsConfig, 'validateSettings').returns([]) + sandbox.stub(recordAdminAudit, 'recordAdminAuditFromRequest').resolves() + }) + + afterEach(() => { + sandbox.restore() + }) + + it('re-reads and saves inside the settings write lock', async () => { + const order: string[] = [] + ;(settingsConfig.withSettingsWriteLock as Sinon.SinonStub).callsFake(async (operation: () => unknown) => { + order.push('lock:enter') + try { + return operation() + } finally { + order.push('lock:exit') + } + }) + ;(settingsConfig.loadUserSettings as Sinon.SinonStub).callsFake(() => { + order.push('read') + return baseSettings + }) + ;(settingsConfig.loadMergedSettings as Sinon.SinonStub).callsFake(() => { + order.push('merged') + return baseSettings + }) + ;(settingsConfig.saveSettings as Sinon.SinonStub).callsFake(() => { + order.push('save') + }) + + const controller = new PatchAdminSettingsController(outboxRepository as any) + await controller.handleRequest(request as any, response as any) + + // The read has to happen after the lock is acquired. Otherwise a ban written + // by another worker between the read and the save would be overwritten. + expect(order).to.deep.equal(['lock:enter', 'read', 'merged', 'save', 'lock:exit']) + expect(response.status).to.have.been.calledOnceWithExactly(200) + }) + + it('keeps a moderation ban written before the lock is acquired', async () => { + let stored = { + payments: { enabled: false }, + limits: { event: { pubkey: { blacklist: [] as string[] } } }, + info: { relay_url: 'wss://relay.example', name: 'test' }, + } + ;(settingsConfig.withSettingsWriteLock as Sinon.SinonStub).callsFake(async (operation: () => unknown) => { + // Another worker bans a pubkey just before this writer gets the lock. + stored = { + ...stored, + limits: { event: { pubkey: { blacklist: ['banned-pubkey'] } } }, + } + return operation() + }) + ;(settingsConfig.loadUserSettings as Sinon.SinonStub).callsFake(() => stored) + ;(settingsConfig.loadMergedSettings as Sinon.SinonStub).callsFake(() => stored) + ;(settingsConfig.saveSettings as Sinon.SinonStub).callsFake((next: typeof stored) => { + stored = next + }) + + const controller = new PatchAdminSettingsController(outboxRepository as any) + await controller.handleRequest(request as any, response as any) + + // The ordinary save must not revert the ban; it also applies its own change. + expect(stored.limits.event.pubkey.blacklist).to.deep.equal(['banned-pubkey']) + expect(stored.payments.enabled).to.equal(true) + }) + + it('never reads settings outside the lock', async () => { + let locked = false + const readsWhileLocked: boolean[] = [] + ;(settingsConfig.withSettingsWriteLock as Sinon.SinonStub).callsFake(async (operation: () => unknown) => { + locked = true + try { + return operation() + } finally { + locked = false + } + }) + ;(settingsConfig.loadUserSettings as Sinon.SinonStub).callsFake(() => { + readsWhileLocked.push(locked) + return baseSettings + }) + ;(settingsConfig.loadMergedSettings as Sinon.SinonStub).callsFake(() => { + readsWhileLocked.push(locked) + return baseSettings + }) + + const controller = new PatchAdminSettingsController(outboxRepository as any) + await controller.handleRequest(request as any, response as any) + + expect(readsWhileLocked).to.deep.equal([true, true]) + }) + + it('does not save when merged validation fails', async () => { + ;(settingsConfig.validateSettings as Sinon.SinonStub).returns([{ path: 'payments', message: 'bad' }]) + + const controller = new PatchAdminSettingsController(outboxRepository as any) + await controller.handleRequest(request as any, response as any) + + expect(response.status).to.have.been.calledOnceWithExactly(400) + expect(response.send.firstCall.args[0]).to.deep.equal({ + error: 'Validation failed', + issues: [{ path: 'payments', message: 'bad' }], + }) + expect(settingsConfig.saveSettings).not.to.have.been.called + expect(settingsConfig.appendSettingsAuditLog).not.to.have.been.called + expect(outboxRepository.enqueue).not.to.have.been.called + }) +}) diff --git a/test/unit/utils/settings-config.spec.ts b/test/unit/utils/settings-config.spec.ts index 8bd0cf5e3..4bb110525 100644 --- a/test/unit/utils/settings-config.spec.ts +++ b/test/unit/utils/settings-config.spec.ts @@ -3,10 +3,14 @@ import fs from 'fs' import os from 'os' import path from 'path' import Sinon from 'sinon' +import yaml from 'js-yaml' import { ensureSettingsExists, + getSettingsBackupDir, loadUserSettings, + restoreSettingsBackup, + saveSettings, toCategoryLabel, getByPath, getTopLevelSettingCategories, @@ -18,7 +22,7 @@ import { validateSettings, withSettingsWriteLock, } from '../../../src/utils/settings-config' -import { getSettingsWriteLockPath } from '../../../src/utils/settings-paths' +import { getSettingsFilePath, getSettingsWriteLockPath } from '../../../src/utils/settings-paths' import { toBech32 } from '../../../src/utils/transform' import { getGuidedSettingCategory, @@ -1048,4 +1052,44 @@ describe('settings write lock', () => { expect(error?.message).to.equal('Timed out waiting for settings write lock') expect(fs.existsSync(lockPath)).to.equal(true) }) + + it('restores a backup while holding the settings write lock', async () => { + const backupDir = getSettingsBackupDir() + fs.mkdirSync(backupDir, { recursive: true }) + const filename = 'settings.2026-11-02T12-00-00-000Z.yaml' + const defaults = loadDefaults() + + fs.writeFileSync( + path.join(backupDir, filename), + yaml.dump({ ...defaults, info: { ...defaults.info, name: 'restored-from-backup' } }), + ) + + const lockPath = getSettingsWriteLockPath() + const settingsPath = getSettingsFilePath() + const renameSync = fs.renameSync.bind(fs) + let lockHeldDuringWrite = false + + // saveSettings() swaps the new document into place with renameSync(). Seeing + // the lock during that call proves restoreSettingsBackup() still holds it; + // the test would fail if the lock were removed from the restore path. + const renameStub = Sinon.stub(fs, 'renameSync').callsFake((from, to) => { + if (to === settingsPath) { + lockHeldDuringWrite = fs.existsSync(lockPath) + } + + return renameSync(from, to) + }) + + try { + const restored = await restoreSettingsBackup(filename) + + expect(restored.info?.name).to.equal('restored-from-backup') + expect(loadUserSettings().info?.name).to.equal('restored-from-backup') + } finally { + renameStub.restore() + } + + expect(lockHeldDuringWrite).to.equal(true) + expect(fs.existsSync(lockPath)).to.equal(false) + }) })