From 5c4b4f0c246f4b83368afa63c67e65630936c7b5 Mon Sep 17 00:00:00 2001 From: ABHAY PANDEY Date: Thu, 12 Nov 2026 09:34:38 +0530 Subject: [PATCH 1/2] fix(admin): hold the settings write lock for every writer A settings PATCH read settings.yaml before saving it, while the moderation writer read and rewrote the file under the settings write lock. The two could interleave, so an ordinary admin save silently reverted a ban written by the other request. - read, validate and save inside withSettingsWriteLock in the settings, notifications, restore and CLI writers - keep audit and outbox work outside the lock so it is not held across I/O - add regression tests showing the read happens inside the lock and a ban written before acquisition survives the save Fixes #1039 --- .changeset/settings-write-lock-1039.md | 5 + src/cli/commands/config.ts | 42 +++-- .../admin/patch-notifications-controller.ts | 50 ++++-- .../admin/patch-settings-controller.ts | 47 ++++-- .../admin/post-settings-restore-controller.ts | 2 +- src/utils/settings-config.ts | 26 +-- .../patch-notifications-controller.spec.ts | 25 +++ .../admin/patch-settings-controller.spec.ts | 150 ++++++++++++++++++ test/unit/utils/settings-config.spec.ts | 22 +++ 9 files changed, 311 insertions(+), 58 deletions(-) create mode 100644 .changeset/settings-write-lock-1039.md create mode 100644 test/unit/controllers/admin/patch-settings-controller.spec.ts 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..8d80479cf 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, @@ -1048,4 +1052,22 @@ 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 restored = await restoreSettingsBackup(filename) + + expect(restored.info?.name).to.equal('restored-from-backup') + expect(loadUserSettings().info?.name).to.equal('restored-from-backup') + expect(fs.existsSync(getSettingsWriteLockPath())).to.equal(false) + }) }) From 4e0c06e7be1c2f27f4e81d89828201dd14c0f6e0 Mon Sep 17 00:00:00 2001 From: ABHAY PANDEY Date: Thu, 12 Nov 2026 10:47:34 +0530 Subject: [PATCH 2/2] test(admin): prove restore holds the settings write lock Greptile pointed out that the restore test only checked the result and the absence of the lock file, so it would still pass if restoreSettingsBackup stopped taking the lock. Assert that the lock file exists during the rename that writes the restored settings. Removing withSettingsWriteLock from restoreSettingsBackup now fails the test. --- test/unit/utils/settings-config.spec.ts | 32 +++++++++++++++++++++---- 1 file changed, 27 insertions(+), 5 deletions(-) diff --git a/test/unit/utils/settings-config.spec.ts b/test/unit/utils/settings-config.spec.ts index 8d80479cf..4bb110525 100644 --- a/test/unit/utils/settings-config.spec.ts +++ b/test/unit/utils/settings-config.spec.ts @@ -22,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, @@ -1064,10 +1064,32 @@ describe('settings write lock', () => { yaml.dump({ ...defaults, info: { ...defaults.info, name: 'restored-from-backup' } }), ) - const restored = await restoreSettingsBackup(filename) + 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') - expect(fs.existsSync(getSettingsWriteLockPath())).to.equal(false) + 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) }) })