From bc73fa7c4ad8d71be1dbff171022b44e13e75106 Mon Sep 17 00:00:00 2001 From: ABHAY PANDEY Date: Sun, 25 Oct 2026 07:44:14 +0530 Subject: [PATCH 1/3] fix(admin): address Greptile moderation follow-ups from #941 Serialize settings updates across workers, count stats by first_seen, normalize IPv6 bypass addresses, reject reverd kind ranges, and return saved limits from PATCH/actions. --- .../patch-moderation-limits-controller.ts | 9 +-- .../post-moderation-actions-controller.ts | 24 +++++--- src/schemas/admin-moderation-schema.ts | 8 ++- src/utils/admin-moderation-persist.ts | 10 +++- src/utils/admin-moderation-stats.ts | 4 +- src/utils/admin-moderation.ts | 47 +++++++++++++--- src/utils/settings-config.ts | 56 +++++++++++++++++++ src/utils/settings-paths.ts | 2 + ...pply-moderation-actions-controller.spec.ts | 17 ++++++ ...patch-moderation-limits-controller.spec.ts | 15 +++++ .../utils/admin-moderation-persist.spec.ts | 26 +++++---- .../unit/utils/admin-moderation-stats.spec.ts | 9 ++- test/unit/utils/admin-moderation.spec.ts | 9 +++ 13 files changed, 197 insertions(+), 39 deletions(-) diff --git a/src/controllers/admin/patch-moderation-limits-controller.ts b/src/controllers/admin/patch-moderation-limits-controller.ts index 415991af5..8d0c9384a 100644 --- a/src/controllers/admin/patch-moderation-limits-controller.ts +++ b/src/controllers/admin/patch-moderation-limits-controller.ts @@ -21,11 +21,9 @@ export class PatchAdminModerationLimitsController implements IController { return } - const next = mergeAdminModerationLimitsPatch(readAdminModerationLimits(), validation.value) - try { await saveModerationLimitsFromRequest( - next, + (current) => mergeAdminModerationLimitsPatch(current, validation.value), request, this.notificationOutboxRepository, 'moderation.limits.updated', @@ -47,6 +45,9 @@ export class PatchAdminModerationLimitsController implements IController { throw error } - response.status(200).setHeader('content-type', 'application/json').send({ ok: true, limits: next }) + response + .status(200) + .setHeader('content-type', 'application/json') + .send({ ok: true, limits: readAdminModerationLimits() }) } } diff --git a/src/controllers/admin/post-moderation-actions-controller.ts b/src/controllers/admin/post-moderation-actions-controller.ts index f0af6d9c2..bee566dd2 100644 --- a/src/controllers/admin/post-moderation-actions-controller.ts +++ b/src/controllers/admin/post-moderation-actions-controller.ts @@ -22,10 +22,9 @@ export class PostAdminModerationActionsController implements IController { } const { action, pubkey, ip } = validation.value - let next try { - next = applyAdminModerationAction(readAdminModerationLimits(), action, { pubkey, ip }) + applyAdminModerationAction(readAdminModerationLimits(), action, { pubkey, ip }) } catch (error) { response .status(400) @@ -35,11 +34,17 @@ export class PostAdminModerationActionsController implements IController { } try { - await saveModerationLimitsFromRequest(next, request, this.notificationOutboxRepository, 'moderation.action', { - action, - pubkey, - ip, - }) + await saveModerationLimitsFromRequest( + (current) => applyAdminModerationAction(current, action, { pubkey, ip }), + request, + this.notificationOutboxRepository, + 'moderation.action', + { + action, + pubkey, + ip, + }, + ) } catch (error) { if (error instanceof ValidationError) { response @@ -57,6 +62,9 @@ export class PostAdminModerationActionsController implements IController { throw error } - response.status(200).setHeader('content-type', 'application/json').send({ ok: true, limits: next }) + response + .status(200) + .setHeader('content-type', 'application/json') + .send({ ok: true, limits: readAdminModerationLimits() }) } } diff --git a/src/schemas/admin-moderation-schema.ts b/src/schemas/admin-moderation-schema.ts index fe51155fd..81ff29d57 100644 --- a/src/schemas/admin-moderation-schema.ts +++ b/src/schemas/admin-moderation-schema.ts @@ -1,6 +1,12 @@ import { z } from 'zod' -const kindRuleSchema = z.union([z.number().int().min(0).max(65535), z.tuple([z.number().int(), z.number().int()])]) +const kindValueSchema = z.number().int().min(0).max(65535) +const kindRuleSchema = z.union([ + kindValueSchema, + z.tuple([kindValueSchema, kindValueSchema]).refine(([start, end]) => start <= end, { + message: 'Kind range start must not exceed its end', + }), +]) export const adminModerationLimitsPatchSchema = z .object({ diff --git a/src/utils/admin-moderation-persist.ts b/src/utils/admin-moderation-persist.ts index ddfcc3dc5..4d0106a8b 100644 --- a/src/utils/admin-moderation-persist.ts +++ b/src/utils/admin-moderation-persist.ts @@ -5,7 +5,11 @@ import { INotificationOutboxRepository } from '../@types/repositories' import { createLogger } from '../factories/logger-factory' import { appendSettingsAuditLog } from './settings-config' import { recordAdminAuditFromRequest } from './record-admin-audit' -import { AdminModerationLimits, persistAdminModerationLimits, ValidationError } from './admin-moderation' +import { + AdminModerationLimitsUpdater, + updateAdminModerationLimits, + ValidationError, +} from './admin-moderation' const logger = createLogger('admin-moderation-persist') @@ -19,14 +23,14 @@ const MODERATION_SETTINGS_PATHS = [ ] as const export const saveModerationLimitsFromRequest = async ( - limits: AdminModerationLimits, + update: AdminModerationLimitsUpdater, request: Request, notificationOutboxRepository: INotificationOutboxRepository, auditAction: 'moderation.limits.updated' | 'moderation.action', auditDetail?: Record, ): Promise => { try { - persistAdminModerationLimits(limits) + updateAdminModerationLimits(update) } catch (error) { if (error instanceof ValidationError) { throw error diff --git a/src/utils/admin-moderation-stats.ts b/src/utils/admin-moderation-stats.ts index 97cbed21e..0188fce71 100644 --- a/src/utils/admin-moderation-stats.ts +++ b/src/utils/admin-moderation-stats.ts @@ -51,7 +51,7 @@ export const queryTopPublishingPubkeys = async ( .select(db.raw(`encode(event_pubkey, 'hex') as pubkey`)) .count('* as eventCount') .whereNull('deleted_at') - .where('event_created_at', '>=', sinceUnixSeconds) + .where('first_seen', '>=', new Date(sinceUnixSeconds * 1000)) .groupBy('event_pubkey') .orderBy('eventCount', 'desc') .limit(limit) @@ -73,7 +73,7 @@ export const queryTopClientIps = async ( .count('* as eventCount') .whereNull('deleted_at') .whereNotNull('remote_address') - .where('event_created_at', '>=', sinceUnixSeconds) + .where('first_seen', '>=', new Date(sinceUnixSeconds * 1000)) .groupBy('remote_address') .orderBy('eventCount', 'desc') .limit(limit) diff --git a/src/utils/admin-moderation.ts b/src/utils/admin-moderation.ts index 9020cb8fd..b99597d72 100644 --- a/src/utils/admin-moderation.ts +++ b/src/utils/admin-moderation.ts @@ -1,4 +1,4 @@ -import { isIP } from 'node:net' +import { isIP, SocketAddress } from 'node:net' import { mergeDeepRight } from 'ramda' import { Settings } from '../@types/settings' @@ -9,6 +9,7 @@ import { loadUserSettings, saveSettings, validateSettings, + withSettingsWriteLock, } from './settings-config' export interface AdminModerationLimits { @@ -53,13 +54,28 @@ const normalizePubkeyList = (values: string[], label: string): string[] => { return normalized } +export const normalizeModerationIpAddress = (raw: string): string => { + const ip = raw.trim() + if (!ip || isIP(ip) === 0) { + throw new Error('ip must be a valid IPv4 or IPv6 address') + } + + const family = isIP(ip) + return new SocketAddress({ + address: ip, + family: family === 6 ? 'ipv6' : 'ipv4', + }).address +} + const normalizeIpList = (values: string[], label: string): string[] => { const normalized: string[] = [] const seen = new Set() for (const raw of values) { - const ip = raw.trim() - if (!ip || isIP(ip) === 0) { + let ip: string + try { + ip = normalizeModerationIpAddress(raw) + } catch { throw new Error(`${label} must contain valid IPv4 or IPv6 addresses`) } @@ -119,7 +135,9 @@ const applyLimitsToUserSettings = (userSettings: Settings, limits: AdminModerati } } -export const persistAdminModerationLimits = (next: AdminModerationLimits): void => { +export type AdminModerationLimitsUpdater = (current: AdminModerationLimits) => AdminModerationLimits + +const persistAdminModerationLimitsUnlocked = (next: AdminModerationLimits): void => { const normalized: AdminModerationLimits = { pubkey: { blacklist: normalizePubkeyList(next.pubkey.blacklist, 'pubkey blacklist'), @@ -147,6 +165,17 @@ export const persistAdminModerationLimits = (next: AdminModerationLimits): void saveSettings(nextUserSettings) } +export const updateAdminModerationLimits = (updater: AdminModerationLimitsUpdater): void => { + withSettingsWriteLock(() => { + const current = readAdminModerationLimits() + persistAdminModerationLimitsUnlocked(updater(current)) + }) +} + +export const persistAdminModerationLimits = (next: AdminModerationLimits): void => { + updateAdminModerationLimits(() => next) +} + export class ValidationError extends Error { public constructor( message: string, @@ -248,10 +277,11 @@ export const applyAdminModerationAction = ( } } case 'whitelist-ip': { - const ip = fields.ip?.trim() - if (!ip || isIP(ip) === 0) { + const rawIp = fields.ip?.trim() + if (!rawIp) { throw new Error('ip is required and must be a valid IPv4 or IPv6 address') } + const ip = normalizeModerationIpAddress(rawIp) return { ...current, rateLimitBypass: { @@ -261,10 +291,11 @@ export const applyAdminModerationAction = ( } } case 'unwhitelist-ip': { - const ip = fields.ip?.trim() - if (!ip || isIP(ip) === 0) { + const rawIp = fields.ip?.trim() + if (!rawIp) { throw new Error('ip is required and must be a valid IPv4 or IPv6 address') } + const ip = normalizeModerationIpAddress(rawIp) return { ...current, rateLimitBypass: { diff --git a/src/utils/settings-config.ts b/src/utils/settings-config.ts index de7e94a9a..e87854636 100644 --- a/src/utils/settings-config.ts +++ b/src/utils/settings-config.ts @@ -14,6 +14,7 @@ import { getSettingsAuditLogPath, getSettingsBackupDir, getSettingsFilePath, + getSettingsWriteLockPath, } from './settings-paths' export { @@ -23,8 +24,63 @@ export { getSettingsAuditLogPath, getSettingsBackupDir, getSettingsFilePath, + getSettingsWriteLockPath, } from './settings-paths' +const SETTINGS_WRITE_LOCK_STALE_MS = 60_000 +const SETTINGS_WRITE_LOCK_RETRY_MS = 25 +const SETTINGS_WRITE_LOCK_MAX_WAIT_MS = 30_000 + +const sleepSync = (milliseconds: number): void => { + const deadline = Date.now() + milliseconds + while (Date.now() < deadline) { + // Busy-wait so lock acquisition stays synchronous with saveSettings. + } +} + +export const withSettingsWriteLock = (operation: () => T): T => { + ensureSettingsExists() + const lockPath = getSettingsWriteLockPath() + const deadline = Date.now() + SETTINGS_WRITE_LOCK_MAX_WAIT_MS + let lockFd: number | undefined + + while (Date.now() < deadline) { + try { + lockFd = fs.openSync(lockPath, 'wx') + fs.writeFileSync(lockFd, `${process.pid}\n${Date.now()}\n`) + break + } catch (error) { + const errno = error as NodeJS.ErrnoException + if (errno.code !== 'EEXIST') { + throw error + } + + try { + const stat = fs.statSync(lockPath) + if (Date.now() - stat.mtimeMs > SETTINGS_WRITE_LOCK_STALE_MS) { + fs.rmSync(lockPath, { force: true }) + continue + } + } catch { + // Another worker may have released the lock. + } + + sleepSync(SETTINGS_WRITE_LOCK_RETRY_MS) + } + } + + if (lockFd === undefined) { + throw new Error('Timed out waiting for settings write lock') + } + + try { + return operation() + } finally { + fs.closeSync(lockFd) + fs.rmSync(lockPath, { force: true }) + } +} + const logger = createLogger('settings-config') export type ValidationIssue = { diff --git a/src/utils/settings-paths.ts b/src/utils/settings-paths.ts index d55743195..49290bb4f 100644 --- a/src/utils/settings-paths.ts +++ b/src/utils/settings-paths.ts @@ -12,3 +12,5 @@ export const getDefaultSettingsFilePath = (): string => join(process.cwd(), 'res export const getSettingsBackupDir = (): string => join(getConfigBaseDir(), 'backups') export const getSettingsAuditLogPath = (): string => join(getConfigBaseDir(), 'settings-audit.jsonl') + +export const getSettingsWriteLockPath = (): string => join(getConfigBaseDir(), '.settings-write.lock') diff --git a/test/unit/controllers/admin/apply-moderation-actions-controller.spec.ts b/test/unit/controllers/admin/apply-moderation-actions-controller.spec.ts index 0f925cbc8..058851caf 100644 --- a/test/unit/controllers/admin/apply-moderation-actions-controller.spec.ts +++ b/test/unit/controllers/admin/apply-moderation-actions-controller.spec.ts @@ -83,6 +83,23 @@ describe('PostAdminModerationActionsController', () => { it('applies ban-pubkey and persists limits', async () => { const controller = new PostAdminModerationActionsController(outboxRepository as any) const pubkey = 'c'.repeat(64) + const savedLimits = { + ...baseLimits, + pubkey: { blacklist: [pubkey], whitelist: [] }, + } + sandbox.restore() + sandbox = Sinon.createSandbox() + outboxRepository = { enqueue: sandbox.stub().resolves() } + response = { + status: sandbox.stub().returnsThis(), + setHeader: sandbox.stub().returnsThis(), + send: sandbox.stub().returnsThis(), + } + sandbox.stub(adminModeration, 'readAdminModerationLimits') + ;(adminModeration.readAdminModerationLimits as Sinon.SinonStub).onFirstCall().returns(baseLimits) + ;(adminModeration.readAdminModerationLimits as Sinon.SinonStub).onSecondCall().returns(savedLimits) + sandbox.stub(adminModerationPersist, 'saveModerationLimitsFromRequest').resolves() + await controller.handleRequest( { body: { action: 'ban-pubkey', pubkey }, ip: '127.0.0.1' } as any, response as any, diff --git a/test/unit/controllers/admin/patch-moderation-limits-controller.spec.ts b/test/unit/controllers/admin/patch-moderation-limits-controller.spec.ts index 654061799..4d7a2c48f 100644 --- a/test/unit/controllers/admin/patch-moderation-limits-controller.spec.ts +++ b/test/unit/controllers/admin/patch-moderation-limits-controller.spec.ts @@ -74,6 +74,21 @@ describe('PatchAdminModerationLimitsController', () => { it('merges patches and persists limits', async () => { const controller = new PatchAdminModerationLimitsController(outboxRepository as any) const pubkey = 'c'.repeat(64) + const savedLimits = { + ...baseLimits, + pubkey: { blacklist: [pubkey], whitelist: [] }, + } + sandbox.restore() + sandbox = Sinon.createSandbox() + outboxRepository = { enqueue: sandbox.stub().resolves() } + response = { + status: sandbox.stub().returnsThis(), + setHeader: sandbox.stub().returnsThis(), + send: sandbox.stub().returnsThis(), + } + sandbox.stub(adminModeration, 'readAdminModerationLimits').returns(savedLimits) + sandbox.stub(adminModerationPersist, 'saveModerationLimitsFromRequest').resolves() + await controller.handleRequest( { body: { pubkey: { blacklist: [pubkey] } }, ip: '127.0.0.1' } as any, response as any, diff --git a/test/unit/utils/admin-moderation-persist.spec.ts b/test/unit/utils/admin-moderation-persist.spec.ts index 9ebd88906..d90a612ab 100644 --- a/test/unit/utils/admin-moderation-persist.spec.ts +++ b/test/unit/utils/admin-moderation-persist.spec.ts @@ -1,11 +1,14 @@ +import { createRequire } from 'node:module' + import chai from 'chai' import Sinon from 'sinon' import sinonChai from 'sinon-chai' -import * as adminModeration from '../../../src/utils/admin-moderation' -import { saveModerationLimitsFromRequest } from '../../../src/utils/admin-moderation-persist' -import * as recordAdminAudit from '../../../src/utils/record-admin-audit' -import * as settingsConfig from '../../../src/utils/settings-config' +const require = createRequire(import.meta.url) +const adminModeration = require('../../../src/utils/admin-moderation') as typeof import('../../../src/utils/admin-moderation') +const adminModerationPersist = require('../../../src/utils/admin-moderation-persist') as typeof import('../../../src/utils/admin-moderation-persist') +const recordAdminAudit = require('../../../src/utils/record-admin-audit') as typeof import('../../../src/utils/record-admin-audit') +const settingsConfig = require('../../../src/utils/settings-config') as typeof import('../../../src/utils/settings-config') chai.use(sinonChai) @@ -22,7 +25,7 @@ describe('saveModerationLimitsFromRequest', () => { beforeEach(() => { sandbox = Sinon.createSandbox() - sandbox.stub(adminModeration, 'persistAdminModerationLimits') + sandbox.stub(adminModeration, 'updateAdminModerationLimits') sandbox.stub(settingsConfig, 'appendSettingsAuditLog') sandbox.stub(recordAdminAudit, 'recordAdminAuditFromRequest').resolves() }) @@ -33,14 +36,15 @@ describe('saveModerationLimitsFromRequest', () => { it('persists limits, audits, and enqueues notification', async () => { const enqueue = sandbox.stub().resolves() - await saveModerationLimitsFromRequest( - limits, + await adminModerationPersist.saveModerationLimitsFromRequest( + () => limits, { ip: '127.0.0.1' } as any, { enqueue } as any, 'moderation.limits.updated', ) - expect(adminModeration.persistAdminModerationLimits).to.have.been.calledOnceWithExactly(limits) + expect(adminModeration.updateAdminModerationLimits).to.have.been.calledOnce + expect(adminModeration.updateAdminModerationLimits.firstCall.args[0]).to.be.a('function') expect(settingsConfig.appendSettingsAuditLog).to.have.been.calledOnce expect(recordAdminAudit.recordAdminAuditFromRequest).to.have.been.calledOnce expect(enqueue).to.have.been.calledOnce @@ -48,14 +52,14 @@ describe('saveModerationLimitsFromRequest', () => { it('still completes when notification enqueue fails', async () => { const enqueue = sandbox.stub().rejects(new Error('db unavailable')) - await saveModerationLimitsFromRequest( - limits, + await adminModerationPersist.saveModerationLimitsFromRequest( + () => limits, { ip: '127.0.0.1' } as any, { enqueue } as any, 'moderation.action', { action: 'ban-pubkey' }, ) - expect(adminModeration.persistAdminModerationLimits).to.have.been.calledOnce + expect(adminModeration.updateAdminModerationLimits).to.have.been.calledOnce }) }) diff --git a/test/unit/utils/admin-moderation-stats.spec.ts b/test/unit/utils/admin-moderation-stats.spec.ts index 86410a387..59f184a6c 100644 --- a/test/unit/utils/admin-moderation-stats.spec.ts +++ b/test/unit/utils/admin-moderation-stats.spec.ts @@ -1,12 +1,15 @@ +import { createRequire } from 'node:module' + import { expect } from 'chai' import Sinon from 'sinon' -import { +const require = createRequire(import.meta.url) +const { parseModerationStatsLimit, parseModerationStatsWindowHours, queryTopClientIps, queryTopPublishingPubkeys, -} from '../../../src/utils/admin-moderation-stats' +} = require('../../../src/utils/admin-moderation-stats') as typeof import('../../../src/utils/admin-moderation-stats') describe('admin-moderation-stats parsing', () => { it('parses windowHours', () => { @@ -46,6 +49,7 @@ describe('admin-moderation-stats queries', () => { expect(rows).to.deep.equal([{ pubkey: 'e'.repeat(64), eventCount: 7 }]) expect(chain.limit.callCount).to.equal(1) + expect(chain.where.calledWith('first_seen', '>=', new Date(1_700_000_000 * 1000))).to.equal(true) }) it('maps top client IPs from the database', async () => { @@ -57,5 +61,6 @@ describe('admin-moderation-stats queries', () => { expect(rows).to.deep.equal([{ ip: '203.0.113.4', eventCount: 2 }]) expect(chain.whereNotNull.callCount).to.equal(1) + expect(chain.where.calledWith('first_seen', '>=', new Date(1_700_000_000 * 1000))).to.equal(true) }) }) diff --git a/test/unit/utils/admin-moderation.spec.ts b/test/unit/utils/admin-moderation.spec.ts index ee202b0f9..681d195d5 100644 --- a/test/unit/utils/admin-moderation.spec.ts +++ b/test/unit/utils/admin-moderation.spec.ts @@ -105,6 +105,15 @@ describe('admin-moderation', () => { expect(withoutIp.rateLimitBypass.ipAddresses).to.not.include('203.0.113.10') }) + it('normalizes IPv6 bypass addresses to match client socket spelling', () => { + const expanded = '2001:0db8:0:0:0:0:0:1' + const withIp = applyAdminModerationAction(baseLimits, 'whitelist-ip', { ip: expanded }) + expect(withIp.rateLimitBypass.ipAddresses).to.include('2001:db8::1') + + const withoutIp = applyAdminModerationAction(withIp, 'unwhitelist-ip', { ip: '2001:db8::1' }) + expect(withoutIp.rateLimitBypass.ipAddresses).to.not.include('2001:db8::1') + }) + it('persists normalized moderation limits to settings', () => { const pubkey = 'c'.repeat(64) persistAdminModerationLimits({ From 3696f3e418063418e027cd2a5756dfe19fe8ad58 Mon Sep 17 00:00:00 2001 From: ABHAY PANDEY Date: Sun, 25 Oct 2026 07:49:14 +0530 Subject: [PATCH 2/3] chore(changeset): admin moderation Greptile follow-ups (#941) --- .changeset/admin-moderation-greptile-941.md | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 .changeset/admin-moderation-greptile-941.md diff --git a/.changeset/admin-moderation-greptile-941.md b/.changeset/admin-moderation-greptile-941.md new file mode 100644 index 000000000..34c211a0e --- /dev/null +++ b/.changeset/admin-moderation-greptile-941.md @@ -0,0 +1,7 @@ +--- +"nostream": patch +--- + +fix(admin): Greptile follow-ups for moderation limits and activity stats + +Moderation limit PATCH and ban/unban actions could interleave read–update–save across workers and drop updates. Those paths now run under a settings write lock. Activity stats windows filter on `first_seen` (when the relay first saw the event), not `event_created_at`. IPv6 addresses on the rate-limit bypass list are normalized the same way as connection IPs so bypass rules cannot be skipped by formatting. PATCH rejects kind ranges with `start` greater than `end`. PATCH and moderation actions return the normalized limits read back from settings after save. From 527b1afade5c1ae67a323694a96be7911ab636f6 Mon Sep 17 00:00:00 2001 From: ABHAY PANDEY Date: Sun, 25 Oct 2026 07:54:54 +0530 Subject: [PATCH 3/3] fix(admin): address Greptile review on moderation follow-ups Yield while waiting on the settings write lock, reclaim stale locks safely, normalize IPv6 bypass removal, index first_seen for stats queries, and fix persist unit test loading under ts-node. --- .changeset/admin-moderation-greptile-941.md | 2 +- ...add_moderation_stats_first_seen_indexes.js | 31 ++++++++ src/utils/admin-moderation-persist.ts | 2 +- src/utils/admin-moderation.ts | 19 +++-- src/utils/settings-config.ts | 74 +++++++++++++++---- ...stats-first-seen-indexes-migration.spec.ts | 17 +++++ .../utils/admin-moderation-persist.spec.ts | 13 ++-- .../unit/utils/admin-moderation-stats.spec.ts | 7 +- test/unit/utils/admin-moderation.spec.ts | 28 ++++--- 9 files changed, 152 insertions(+), 41 deletions(-) create mode 100644 migrations/20261025_120000_add_moderation_stats_first_seen_indexes.js create mode 100644 test/unit/migrations/moderation-stats-first-seen-indexes-migration.spec.ts diff --git a/.changeset/admin-moderation-greptile-941.md b/.changeset/admin-moderation-greptile-941.md index 34c211a0e..1a37e8b40 100644 --- a/.changeset/admin-moderation-greptile-941.md +++ b/.changeset/admin-moderation-greptile-941.md @@ -4,4 +4,4 @@ fix(admin): Greptile follow-ups for moderation limits and activity stats -Moderation limit PATCH and ban/unban actions could interleave read–update–save across workers and drop updates. Those paths now run under a settings write lock. Activity stats windows filter on `first_seen` (when the relay first saw the event), not `event_created_at`. IPv6 addresses on the rate-limit bypass list are normalized the same way as connection IPs so bypass rules cannot be skipped by formatting. PATCH rejects kind ranges with `start` greater than `end`. PATCH and moderation actions return the normalized limits read back from settings after save. +Moderation limit PATCH and ban/unban actions could interleave read–update–save across workers and drop updates. Those paths now run under an async settings write lock that yields while waiting and reclaims stale locks without deleting a fresh holder’s file. Activity stats windows filter on `first_seen` (when the relay first saw the event), not `event_created_at`, with partial indexes on live rows for those queries. IPv6 addresses on the rate-limit bypass list are normalized the same way as connection IPs so bypass rules cannot be skipped by formatting, including when removing entries stored in another spelling. PATCH rejects kind ranges with `start` greater than `end`. PATCH and moderation actions return the normalized limits read back from settings after save. diff --git a/migrations/20261025_120000_add_moderation_stats_first_seen_indexes.js b/migrations/20261025_120000_add_moderation_stats_first_seen_indexes.js new file mode 100644 index 000000000..73f0bef47 --- /dev/null +++ b/migrations/20261025_120000_add_moderation_stats_first_seen_indexes.js @@ -0,0 +1,31 @@ +/** + * Supports admin moderation activity stats (queryTopPublishingPubkeys / + * queryTopClientIps), which filter live events by relay receipt time: + * + * WHERE deleted_at IS NULL AND first_seen >= ? + * + * Partial indexes keep the working set small on relays where most rows are + * soft-deleted history. CREATE INDEX CONCURRENTLY cannot run inside a + * transaction. + */ + +exports.config = { transaction: false } + +exports.up = async function (knex) { + await knex.raw(` + CREATE INDEX CONCURRENTLY IF NOT EXISTS events_live_first_seen_pubkey_idx + ON events (first_seen, event_pubkey) + WHERE deleted_at IS NULL + `) + + await knex.raw(` + CREATE INDEX CONCURRENTLY IF NOT EXISTS events_live_first_seen_remote_address_idx + ON events (first_seen, remote_address) + WHERE deleted_at IS NULL AND remote_address IS NOT NULL + `) +} + +exports.down = async function (knex) { + await knex.raw('DROP INDEX CONCURRENTLY IF EXISTS events_live_first_seen_remote_address_idx') + await knex.raw('DROP INDEX CONCURRENTLY IF EXISTS events_live_first_seen_pubkey_idx') +} diff --git a/src/utils/admin-moderation-persist.ts b/src/utils/admin-moderation-persist.ts index 4d0106a8b..625e86a4a 100644 --- a/src/utils/admin-moderation-persist.ts +++ b/src/utils/admin-moderation-persist.ts @@ -30,7 +30,7 @@ export const saveModerationLimitsFromRequest = async ( auditDetail?: Record, ): Promise => { try { - updateAdminModerationLimits(update) + await updateAdminModerationLimits(update) } catch (error) { if (error instanceof ValidationError) { throw error diff --git a/src/utils/admin-moderation.ts b/src/utils/admin-moderation.ts index b99597d72..d55c1be51 100644 --- a/src/utils/admin-moderation.ts +++ b/src/utils/admin-moderation.ts @@ -165,15 +165,15 @@ const persistAdminModerationLimitsUnlocked = (next: AdminModerationLimits): void saveSettings(nextUserSettings) } -export const updateAdminModerationLimits = (updater: AdminModerationLimitsUpdater): void => { - withSettingsWriteLock(() => { +export const updateAdminModerationLimits = async (updater: AdminModerationLimitsUpdater): Promise => { + await withSettingsWriteLock(() => { const current = readAdminModerationLimits() persistAdminModerationLimitsUnlocked(updater(current)) }) } -export const persistAdminModerationLimits = (next: AdminModerationLimits): void => { - updateAdminModerationLimits(() => next) +export const persistAdminModerationLimits = async (next: AdminModerationLimits): Promise => { + await updateAdminModerationLimits(() => next) } export class ValidationError extends Error { @@ -212,6 +212,15 @@ export const mergeAdminModerationLimitsPatch = ( const withoutValue = (list: T[], value: T): T[] => list.filter((entry) => entry !== value) +const withoutNormalizedModerationIp = (list: string[], normalizedTarget: string): string[] => + list.filter((stored) => { + try { + return normalizeModerationIpAddress(stored) !== normalizedTarget + } catch { + return stored.trim().toLowerCase() !== normalizedTarget.trim().toLowerCase() + } + }) + export const applyAdminModerationAction = ( current: AdminModerationLimits, action: AdminModerationAction, @@ -300,7 +309,7 @@ export const applyAdminModerationAction = ( ...current, rateLimitBypass: { ...current.rateLimitBypass, - ipAddresses: withoutValue(current.rateLimitBypass.ipAddresses, ip), + ipAddresses: withoutNormalizedModerationIp(current.rateLimitBypass.ipAddresses, ip), }, } } diff --git a/src/utils/settings-config.ts b/src/utils/settings-config.ts index e87854636..6ca808480 100644 --- a/src/utils/settings-config.ts +++ b/src/utils/settings-config.ts @@ -1,4 +1,5 @@ import fs from 'fs' +import { setTimeout as delay } from 'node:timers/promises' import { join } from 'path' import yaml from 'js-yaml' import { isNil, mergeDeepRight } from 'ramda' @@ -31,14 +32,64 @@ const SETTINGS_WRITE_LOCK_STALE_MS = 60_000 const SETTINGS_WRITE_LOCK_RETRY_MS = 25 const SETTINGS_WRITE_LOCK_MAX_WAIT_MS = 30_000 -const sleepSync = (milliseconds: number): void => { - const deadline = Date.now() + milliseconds - while (Date.now() < deadline) { - // Busy-wait so lock acquisition stays synchronous with saveSettings. +type SettingsWriteLockMetadata = { + pid: number + acquiredAt: number +} + +const readSettingsWriteLockMetadata = (lockPath: string): SettingsWriteLockMetadata | undefined => { + try { + const raw = fs.readFileSync(lockPath, 'utf-8') + const [pidLine, acquiredLine] = raw.split('\n') + const pid = Number(pidLine) + const acquiredAt = Number(acquiredLine) + if (!Number.isFinite(pid) || !Number.isFinite(acquiredAt)) { + return undefined + } + return { pid, acquiredAt } + } catch { + return undefined } } -export const withSettingsWriteLock = (operation: () => T): T => { +const isSettingsWriteLockStale = (metadata: SettingsWriteLockMetadata): boolean => { + if (Date.now() - metadata.acquiredAt > SETTINGS_WRITE_LOCK_STALE_MS) { + return true + } + + try { + process.kill(metadata.pid, 0) + return false + } catch { + return true + } +} + +const attemptStealStaleSettingsWriteLock = ( + lockPath: string, + expected: SettingsWriteLockMetadata, +): boolean => { + try { + const current = readSettingsWriteLockMetadata(lockPath) + if ( + !current || + current.pid !== expected.pid || + current.acquiredAt !== expected.acquiredAt || + !isSettingsWriteLockStale(current) + ) { + return false + } + + const reclaimPath = `${lockPath}.reclaim.${process.pid}.${Date.now()}` + fs.renameSync(lockPath, reclaimPath) + fs.rmSync(reclaimPath, { force: true }) + return true + } catch { + return false + } +} + +export const withSettingsWriteLock = async (operation: () => T): Promise => { ensureSettingsExists() const lockPath = getSettingsWriteLockPath() const deadline = Date.now() + SETTINGS_WRITE_LOCK_MAX_WAIT_MS @@ -55,17 +106,12 @@ export const withSettingsWriteLock = (operation: () => T): T => { throw error } - try { - const stat = fs.statSync(lockPath) - if (Date.now() - stat.mtimeMs > SETTINGS_WRITE_LOCK_STALE_MS) { - fs.rmSync(lockPath, { force: true }) - continue - } - } catch { - // Another worker may have released the lock. + const metadata = readSettingsWriteLockMetadata(lockPath) + if (metadata && isSettingsWriteLockStale(metadata)) { + attemptStealStaleSettingsWriteLock(lockPath, metadata) } - sleepSync(SETTINGS_WRITE_LOCK_RETRY_MS) + await delay(SETTINGS_WRITE_LOCK_RETRY_MS) } } diff --git a/test/unit/migrations/moderation-stats-first-seen-indexes-migration.spec.ts b/test/unit/migrations/moderation-stats-first-seen-indexes-migration.spec.ts new file mode 100644 index 000000000..03e9649d4 --- /dev/null +++ b/test/unit/migrations/moderation-stats-first-seen-indexes-migration.spec.ts @@ -0,0 +1,17 @@ +import { createRequire } from 'node:module' + +import { expect } from 'chai' + +const requireFromHere = createRequire(__filename) +const migration = requireFromHere('../../../migrations/20261025_120000_add_moderation_stats_first_seen_indexes.js') + +describe('migrations/20261025_120000_add_moderation_stats_first_seen_indexes', () => { + it('opts out of knex transaction so CREATE INDEX CONCURRENTLY can run', () => { + expect(migration.config).to.deep.equal({ transaction: false }) + }) + + it('exports up and down', () => { + expect(migration.up).to.be.a('function') + expect(migration.down).to.be.a('function') + }) +}) diff --git a/test/unit/utils/admin-moderation-persist.spec.ts b/test/unit/utils/admin-moderation-persist.spec.ts index d90a612ab..837aed42d 100644 --- a/test/unit/utils/admin-moderation-persist.spec.ts +++ b/test/unit/utils/admin-moderation-persist.spec.ts @@ -1,14 +1,15 @@ import { createRequire } from 'node:module' +import { join } from 'node:path' import chai from 'chai' import Sinon from 'sinon' import sinonChai from 'sinon-chai' -const require = createRequire(import.meta.url) -const adminModeration = require('../../../src/utils/admin-moderation') as typeof import('../../../src/utils/admin-moderation') -const adminModerationPersist = require('../../../src/utils/admin-moderation-persist') as typeof import('../../../src/utils/admin-moderation-persist') -const recordAdminAudit = require('../../../src/utils/record-admin-audit') as typeof import('../../../src/utils/record-admin-audit') -const settingsConfig = require('../../../src/utils/settings-config') as typeof import('../../../src/utils/settings-config') +const loadFromRepo = createRequire(join(process.cwd(), 'package.json')) +const adminModeration = loadFromRepo('./src/utils/admin-moderation') as typeof import('../../../src/utils/admin-moderation') +const adminModerationPersist = loadFromRepo('./src/utils/admin-moderation-persist') as typeof import('../../../src/utils/admin-moderation-persist') +const recordAdminAudit = loadFromRepo('./src/utils/record-admin-audit') as typeof import('../../../src/utils/record-admin-audit') +const settingsConfig = loadFromRepo('./src/utils/settings-config') as typeof import('../../../src/utils/settings-config') chai.use(sinonChai) @@ -25,7 +26,7 @@ describe('saveModerationLimitsFromRequest', () => { beforeEach(() => { sandbox = Sinon.createSandbox() - sandbox.stub(adminModeration, 'updateAdminModerationLimits') + sandbox.stub(adminModeration, 'updateAdminModerationLimits').resolves() sandbox.stub(settingsConfig, 'appendSettingsAuditLog') sandbox.stub(recordAdminAudit, 'recordAdminAuditFromRequest').resolves() }) diff --git a/test/unit/utils/admin-moderation-stats.spec.ts b/test/unit/utils/admin-moderation-stats.spec.ts index 59f184a6c..89c79fad9 100644 --- a/test/unit/utils/admin-moderation-stats.spec.ts +++ b/test/unit/utils/admin-moderation-stats.spec.ts @@ -1,15 +1,12 @@ -import { createRequire } from 'node:module' - import { expect } from 'chai' import Sinon from 'sinon' -const require = createRequire(import.meta.url) -const { +import { parseModerationStatsLimit, parseModerationStatsWindowHours, queryTopClientIps, queryTopPublishingPubkeys, -} = require('../../../src/utils/admin-moderation-stats') as typeof import('../../../src/utils/admin-moderation-stats') +} from '../../../src/utils/admin-moderation-stats' describe('admin-moderation-stats parsing', () => { it('parses windowHours', () => { diff --git a/test/unit/utils/admin-moderation.spec.ts b/test/unit/utils/admin-moderation.spec.ts index 681d195d5..84dde463c 100644 --- a/test/unit/utils/admin-moderation.spec.ts +++ b/test/unit/utils/admin-moderation.spec.ts @@ -114,9 +114,21 @@ describe('admin-moderation', () => { expect(withoutIp.rateLimitBypass.ipAddresses).to.not.include('2001:db8::1') }) - it('persists normalized moderation limits to settings', () => { + it('removes IPv6 bypass entries stored in alternate spelling', () => { + const limits = { + ...baseLimits, + rateLimitBypass: { + ...baseLimits.rateLimitBypass, + ipAddresses: ['2001:0db8:0:0:0:0:0:1'], + }, + } + const withoutIp = applyAdminModerationAction(limits, 'unwhitelist-ip', { ip: '2001:db8::1' }) + expect(withoutIp.rateLimitBypass.ipAddresses).to.be.empty + }) + + it('persists normalized moderation limits to settings', async () => { const pubkey = 'c'.repeat(64) - persistAdminModerationLimits({ + await persistAdminModerationLimits({ ...baseLimits, pubkey: { blacklist: [pubkey], whitelist: [] }, }) @@ -125,16 +137,14 @@ describe('admin-moderation', () => { expect(limits.pubkey.blacklist).to.include(pubkey) }) - it('persists when user settings only override part of info', () => { + it('persists when user settings only override part of info', async () => { settingsConfig.saveSettings({ info: { relay_url: 'wss://relay.example' } } as any) const pubkey = 'f'.repeat(64) - expect(() => - persistAdminModerationLimits({ - ...baseLimits, - pubkey: { blacklist: [pubkey], whitelist: [] }, - }), - ).to.not.throw() + await persistAdminModerationLimits({ + ...baseLimits, + pubkey: { blacklist: [pubkey], whitelist: [] }, + }) expect(readAdminModerationLimits().pubkey.blacklist).to.include(pubkey) })