From 585e10041e5abb230be961064dc2da7b3c04051d Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Sat, 31 Oct 2026 22:54:35 +0100 Subject: [PATCH 01/31] fix: refuse to save over stored settings or plugin credentials that no longer decrypt #1066 --- app/api/plugin-secrets/route.ts | 20 ++- app/api/settings/route.ts | 9 ++ lib/__tests__/stored-data-unreadable.test.ts | 162 +++++++++++++++++++ lib/plugin-secrets/proxy.ts | 17 +- lib/plugin-secrets/vault.ts | 11 +- lib/settings-sync.ts | 28 +++- lib/stored-data-errors.ts | 44 +++++ 7 files changed, 280 insertions(+), 11 deletions(-) create mode 100644 lib/__tests__/stored-data-unreadable.test.ts create mode 100644 lib/stored-data-errors.ts diff --git a/app/api/plugin-secrets/route.ts b/app/api/plugin-secrets/route.ts index e4080fcb5..e235b5a0c 100644 --- a/app/api/plugin-secrets/route.ts +++ b/app/api/plugin-secrets/route.ts @@ -12,6 +12,7 @@ import { putUserCredential, type VaultUser, } from '@/lib/plugin-secrets/vault'; +import { storedDataUnavailableResponse } from '@/lib/stored-data-errors'; const PLUGIN_ID_RE = /^[a-z0-9][a-z0-9-]*[a-z0-9]$/; const MAX_SECRET_LENGTH = 4096; @@ -35,7 +36,14 @@ export async function GET(request: NextRequest) { if (pluginId !== undefined && !PLUGIN_ID_RE.test(pluginId)) { return NextResponse.json({ error: 'Invalid plugin id' }, { status: 400 }); } - const credentials = await listUserCredentials(user, pluginId); + let credentials; + try { + credentials = await listUserCredentials(user, pluginId); + } catch (error) { + const unavailable = storedDataUnavailableResponse(error, NO_STORE); + if (unavailable) return unavailable; + throw error; + } // For one plugin, also say which of its admin credentials are set up, and // on which servers: the host page checks the links a plugin shows against // them (the plugin itself only gets yes/no, see proxyAccounts). @@ -93,6 +101,8 @@ export async function POST(request: NextRequest) { }); } catch (error) { if (error instanceof VaultFullError) return NextResponse.json({ error: error.message }, { status: 409 }); + const unavailable = storedDataUnavailableResponse(error, NO_STORE); + if (unavailable) return unavailable; throw error; } return NextResponse.json({ ok: true }, { headers: NO_STORE }); @@ -108,7 +118,13 @@ export async function DELETE(request: NextRequest) { const credentialId = request.nextUrl.searchParams.get('credential') ?? undefined; if (!PLUGIN_ID_RE.test(pluginId)) return bad('Invalid plugin id'); if (credentialId !== undefined && !CREDENTIAL_ID_RE.test(credentialId)) return bad('Invalid credential id'); - await deleteUserCredentials(user, pluginId, credentialId); + try { + await deleteUserCredentials(user, pluginId, credentialId); + } catch (error) { + const unavailable = storedDataUnavailableResponse(error, NO_STORE); + if (unavailable) return unavailable; + throw error; + } return NextResponse.json({ ok: true }, { headers: NO_STORE }); } diff --git a/app/api/settings/route.ts b/app/api/settings/route.ts index 18080c4b5..b1ecf91e6 100644 --- a/app/api/settings/route.ts +++ b/app/api/settings/route.ts @@ -12,6 +12,7 @@ import { hasSessionSecret } from '@/lib/auth/session-secret'; import { MAX_ACCOUNT_SLOTS } from '@/lib/account-utils'; import { mergeStoredTemplates } from '@/lib/template-merge'; import { mergeStoredSubscriptions } from '@/lib/calendar-subscription-sync'; +import { storedDataUnavailableResponse } from '@/lib/stored-data-errors'; /** Largest settings blob a client may store (templates and signatures included). */ const SETTINGS_MAX_BYTES = 2 * 1024 * 1024; @@ -141,6 +142,8 @@ export async function GET(request: NextRequest) { const message = error instanceof Error ? error.message : 'Unknown error'; const code = (error as NodeJS.ErrnoException).code; logger.error('Settings load error', { error: message, code }); + const unavailable = storedDataUnavailableResponse(error); + if (unavailable) return unavailable; const classified = classifyError(error); return NextResponse.json({ error: classified.message }, { status: classified.status }); } @@ -221,6 +224,10 @@ export async function POST(request: NextRequest) { const message = error instanceof Error ? error.message : 'Unknown error'; const code = (error as NodeJS.ErrnoException).code; logger.error('Settings save error', { error: message, code }); + // Stored settings that don't decrypt, or no store: write nothing, so + // the user's real settings survive and the client keeps its copy. + const unavailable = storedDataUnavailableResponse(error); + if (unavailable) return unavailable; const classified = classifyError(error); return NextResponse.json({ error: classified.message }, { status: classified.status }); } @@ -252,6 +259,8 @@ export async function DELETE(request: NextRequest) { const message = error instanceof Error ? error.message : 'Unknown error'; const code = (error as NodeJS.ErrnoException).code; logger.error('Settings delete error', { error: message, code }); + const unavailable = storedDataUnavailableResponse(error); + if (unavailable) return unavailable; const classified = classifyError(error); return NextResponse.json({ error: classified.message }, { status: classified.status }); } diff --git a/lib/__tests__/stored-data-unreadable.test.ts b/lib/__tests__/stored-data-unreadable.test.ts new file mode 100644 index 000000000..db9948f7a --- /dev/null +++ b/lib/__tests__/stored-data-unreadable.test.ts @@ -0,0 +1,162 @@ +// @vitest-environment node +import { mkdtempSync, readdirSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import path from 'node:path'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { NextRequest } from 'next/server'; + +// A settings file or plugin vault that exists but doesn't decrypt (written +// with another session secret, or cut short) used to read as "nothing +// stored". The next save then merged into nothing and overwrote the user's +// real data. Now the read throws, and the routes answer 503 and write nothing. + +let secret = 'a'.repeat(64); +vi.mock('@/lib/auth/session-secret', () => ({ + getSessionSecret: () => secret, + hasSessionSecret: () => true, +})); +vi.mock('@/lib/logger', () => ({ + logger: { warn: () => {}, error: () => {}, info: () => {}, debug: () => {} }, +})); +vi.mock('@/lib/admin/config-manager', () => ({ + configManager: { + ensureLoaded: async () => {}, + get: (key: string, fallback: unknown) => (key === 'settingsSyncEnabled' ? true : fallback), + getPolicy: () => ({ restrictions: {}, features: { pluginsEnabled: true } }), + }, +})); +vi.mock('@/lib/admin/third-party-mode', () => ({ isNoThirdPartyTrafficOn: async () => false, thirdPartyRefusal: () => null })); + +const jar = new Map(); +vi.mock('next/headers', () => ({ + cookies: async () => ({ + get: (name: string) => (jar.has(name) ? { name, value: jar.get(name)! } : undefined), + set: (name: string, value: string) => { jar.set(name, value); }, + delete: (name: string) => { jar.delete(name); }, + getAll: () => [...jar].map(([name, value]) => ({ name, value })), + }), +})); + +const SERVER = 'https://mail.example.org'; +const USER = 'alice@example.org'; +vi.mock('@/lib/stalwart/credentials', () => ({ + getStalwartCredentials: async () => ({ username: USER, accountName: USER, serverUrl: SERVER, authHeader: 'x' }), +})); +vi.mock('@/lib/plugin-secrets/known-plugins', () => ({ + declaredUserCredential: async () => ({ scope: 'user', label: 'Nextcloud', inject: { type: 'basic' } }), +})); + +let dataDir: string; + +beforeEach(async () => { + dataDir = mkdtempSync(path.join(tmpdir(), 'bw-unreadable-')); + vi.stubEnv('SETTINGS_DATA_DIR', path.join(dataDir, 'settings')); + vi.stubEnv('PLUGIN_SECRETS_DIR', path.join(dataDir, 'plugin-secrets')); + vi.stubEnv('SETTINGS_SYNC_ENABLED', 'true'); + secret = 'a'.repeat(64); + jar.clear(); +}); + +afterEach(() => { + vi.unstubAllEnvs(); + rmSync(dataDir, { recursive: true, force: true }); +}); + +/** Sign in after the secret change: the context cookie is sealed with the current secret. */ +async function signIn() { + const { setStalwartAuthContextInStore } = await import('@/lib/stalwart/auth-context'); + setStalwartAuthContextInStore({ + get: (n: string) => (jar.has(n) ? { name: n, value: jar.get(n)! } : undefined), + set: (n: string, v: string) => { jar.set(n, v); }, + delete: (n: string) => { jar.delete(n); }, + } as never, 0, { serverUrl: SERVER, authHeader: 'Bearer t', username: USER }); +} + +function onlyFile(dir: string): string { + const [file] = readdirSync(dir).filter((name) => name.endsWith('.enc')); + return path.join(dir, file); +} + +describe('settings that do not decrypt', () => { + it('reads a missing file as nothing stored and one written with another secret as an error', async () => { + const { loadUserSettings, saveUserSettings } = await import('@/lib/settings-sync'); + const { StoredDataUnreadableError } = await import('@/lib/stored-data-errors'); + expect(await loadUserSettings(USER, SERVER)).toBeNull(); + + await saveUserSettings(USER, SERVER, { theme: 'dark' }); + secret = 'b'.repeat(64); + await expect(loadUserSettings(USER, SERVER)).rejects.toBeInstanceOf(StoredDataUnreadableError); + + writeFileSync(onlyFile(path.join(dataDir, 'settings')), Buffer.alloc(10)); + await expect(loadUserSettings(USER, SERVER)).rejects.toBeInstanceOf(StoredDataUnreadableError); + }); + + it('answers 503 to GET and POST and leaves the stored file as it was', async () => { + const { saveUserSettings } = await import('@/lib/settings-sync'); + await saveUserSettings(USER, SERVER, { theme: 'dark', templates: [{ id: 'a' }] }); + const file = onlyFile(path.join(dataDir, 'settings')); + const before = readFileSync(file); + secret = 'b'.repeat(64); + await signIn(); + + const { GET, POST } = await import('@/app/api/settings/route'); + const got = await GET(new NextRequest('https://webmail.example/api/settings', { + headers: { 'x-settings-username': USER, 'x-settings-server': SERVER }, + })); + expect(got.status).toBe(503); + + const saved = await POST(new NextRequest('https://webmail.example/api/settings', { + method: 'POST', + headers: { 'sec-fetch-site': 'same-origin', 'content-type': 'application/json' }, + body: JSON.stringify({ username: USER, serverUrl: SERVER, settings: { theme: 'light' } }), + })); + expect(saved.status).toBe(503); + expect(readFileSync(file).equals(before)).toBe(true); + }); +}); + +describe('a plugin vault that does not decrypt', () => { + const user = { username: USER, serverUrl: SERVER }; + const input = { origin: 'https://cloud.example.org', inject: { type: 'basic' } as const, label: 'Nextcloud', secret: 'pw' }; + + it('reads a missing vault as empty and one written with another secret as an error', async () => { + const { listUserCredentials, putUserCredential } = await import('@/lib/plugin-secrets/vault'); + const { StoredDataUnreadableError } = await import('@/lib/stored-data-errors'); + expect(await listUserCredentials(user)).toEqual([]); + + await putUserCredential(user, 'nextcloud', 'cloud', input); + secret = 'b'.repeat(64); + await expect(listUserCredentials(user)).rejects.toBeInstanceOf(StoredDataUnreadableError); + // A write merges into what is stored, so it must not land either. + await expect(putUserCredential(user, 'other', 'x', input)).rejects.toBeInstanceOf(StoredDataUnreadableError); + }); + + it('answers 503 on the credential routes and the proxy and leaves the vault as it was', async () => { + const { putUserCredential } = await import('@/lib/plugin-secrets/vault'); + await putUserCredential(user, 'nextcloud', 'cloud', input); + const file = onlyFile(path.join(dataDir, 'plugin-secrets')); + const before = readFileSync(file); + secret = 'b'.repeat(64); + + const { GET, POST, DELETE } = await import('@/app/api/plugin-secrets/route'); + const { POST: proxy } = await import('@/app/api/plugin-proxy/route'); + const same = { 'sec-fetch-site': 'same-origin' }; + + expect((await GET(new NextRequest('http://localhost/api/plugin-secrets'))).status).toBe(503); + const post = await POST(new NextRequest('http://localhost/api/plugin-secrets', { + method: 'POST', + headers: same, + body: JSON.stringify({ pluginId: 'nextcloud', credentialId: 'other', label: 'x', origin: 'https://cloud.example.org', inject: { type: 'basic' }, secret: 's' }), + })); + expect(post.status).toBe(503); + const del = await DELETE(new NextRequest('http://localhost/api/plugin-secrets?plugin=nextcloud', { method: 'DELETE', headers: same })); + expect(del.status).toBe(503); + const proxied = await proxy(new NextRequest( + 'http://localhost/api/plugin-proxy?plugin=nextcloud&credential=cloud&url=https%3A%2F%2Fcloud.example.org%2F', + { method: 'POST', headers: same }, + )); + expect(proxied.status).toBe(503); + + expect(readFileSync(file).equals(before)).toBe(true); + }); +}); diff --git a/lib/plugin-secrets/proxy.ts b/lib/plugin-secrets/proxy.ts index 26de61aa9..f9d3653e4 100644 --- a/lib/plugin-secrets/proxy.ts +++ b/lib/plugin-secrets/proxy.ts @@ -12,6 +12,7 @@ import { DisallowedUrlError, fetchPublicUrl } from '@/lib/security/url-guard'; import { authHeaderFor, credentialOrigin, privateProxyOrigins, type CredentialInject, type CredentialSpec } from './credentials'; import { getUserCredential, setUserCredentialSecret, userKey, type VaultUser } from './vault'; import { declaredUserCredential } from './known-plugins'; +import { isStoredDataUnavailable } from '@/lib/stored-data-errors'; /** Largest request body the proxy forwards (one upload chunk). */ export const MAX_PROXY_REQUEST_BYTES = 100 * 1024 * 1024; @@ -269,7 +270,15 @@ export async function proxyRequest(input: ProxyInput): Promise { const method = input.method.toUpperCase(); if (!PROXY_METHODS.has(method)) throw new ProxyError(`Method ${method} is not allowed`, 405, 'bad_method'); - const credential = await resolveCredential(input.user, input.pluginId, input.credentialId); + let credential: ResolvedCredential | null; + try { + credential = await resolveCredential(input.user, input.pluginId, input.credentialId); + } catch (error) { + if (isStoredDataUnavailable(error)) { + throw Object.assign(new ProxyError('The connected accounts could not be read right now', 503, 'store_unavailable'), { retryAfter: 10 }); + } + throw error; + } if (!credential) throw new ProxyError('No such credential', 404, 'no_credential'); let target: URL; @@ -381,6 +390,12 @@ export async function proxyRequest(input: ProxyInput): Promise { audit(error.status, 0, error.code); throw error; } + // A captured secret that couldn't be stored: the vault doesn't read, + // or the shared store is down. Nothing was written. + if (isStoredDataUnavailable(error)) { + audit(503, 0, 'store unavailable'); + throw Object.assign(new ProxyError('The connected accounts could not be read or saved right now', 503, 'store_unavailable'), { retryAfter: 10 }); + } if (error instanceof DisallowedUrlError) { audit(403, 0, 'blocked address'); throw new ProxyError('This server address is not allowed', 403, 'blocked_address'); diff --git a/lib/plugin-secrets/vault.ts b/lib/plugin-secrets/vault.ts index 5a1eee25c..79459941d 100644 --- a/lib/plugin-secrets/vault.ts +++ b/lib/plugin-secrets/vault.ts @@ -7,6 +7,7 @@ import { createHash } from 'node:crypto'; import { readFile, writeFile, mkdir, rename, unlink } from 'node:fs/promises'; import path from 'node:path'; import { decryptPayload, encryptPayload } from '@/lib/auth/crypto'; +import { StoredDataUnreadableError } from '@/lib/stored-data-errors'; import type { CredentialInject } from './credentials'; export interface VaultUser { @@ -56,18 +57,24 @@ function vaultPath(user: VaultUser): string { return file; } +/** + * The user's credentials; {} when none are stored. A file that exists but + * can't be read or doesn't decrypt (another session secret) throws + * StoredDataUnreadableError, so no write lands on top of it. + */ async function load(user: VaultUser): Promise { let raw: string; try { raw = await readFile(vaultPath(user), 'utf8'); } catch (error) { if ((error as NodeJS.ErrnoException).code === 'ENOENT') return {}; - throw error; + throw new StoredDataUnreadableError('plugin credentials', { cause: error }); } const payload = decryptPayload(raw, 'plugin-secrets'); + if (!payload) throw new StoredDataUnreadableError('plugin credentials'); // Bound to this user: a file copied over from another account's name // decrypts, but carries the other account's key and is ignored. - if (!payload || payload.u !== userKey(user)) return {}; + if (payload.u !== userKey(user)) return {}; const data = payload.d; return data && typeof data === 'object' ? (data as VaultData) : {}; } diff --git a/lib/settings-sync.ts b/lib/settings-sync.ts index dc819046b..a7b8e0742 100644 --- a/lib/settings-sync.ts +++ b/lib/settings-sync.ts @@ -4,6 +4,7 @@ import { existsSync } from 'node:fs'; import path from 'node:path'; import { logger } from '@/lib/logger'; import { getSessionSecret } from '@/lib/auth/session-secret'; +import { StoredDataUnreadableError } from '@/lib/stored-data-errors'; const ALGORITHM = 'aes-256-gcm'; const IV_LENGTH = 12; @@ -52,14 +53,30 @@ export async function saveUserSettings(username: string, serverUrl: string, sett await rename(tmpPath, targetPath); } +/** + * The user's stored settings, or null when none are stored. A file that + * exists but can't be read or decrypted (another session secret, cut short) + * throws StoredDataUnreadableError: treating it as "nothing stored" would + * let the next save overwrite the user's real settings. + */ export async function loadUserSettings(username: string, serverUrl: string): Promise | null> { const filePath = getSettingsPath(username, serverUrl); + let data: Buffer; try { - const data = await readFile(filePath); - if (data.length < IV_LENGTH + TAG_LENGTH) return null; + data = await readFile(filePath); + } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'ENOENT') return null; + logger.warn('Failed to read user settings', { error: error instanceof Error ? error.message : 'Unknown error' }); + throw new StoredDataUnreadableError('settings', { cause: error }); + } + return decryptSettings(data); +} - const key = getKey(); +function decryptSettings(data: Buffer): Record { + if (data.length < IV_LENGTH + TAG_LENGTH) throw new StoredDataUnreadableError('settings'); + const key = getKey(); + try { const iv = data.subarray(0, IV_LENGTH); const tag = data.subarray(IV_LENGTH, IV_LENGTH + TAG_LENGTH); const encrypted = data.subarray(IV_LENGTH + TAG_LENGTH); @@ -70,9 +87,8 @@ export async function loadUserSettings(username: string, serverUrl: string): Pro const decrypted = Buffer.concat([decipher.update(encrypted), decipher.final()]); return JSON.parse(decrypted.toString('utf8')); } catch (error) { - if ((error as NodeJS.ErrnoException).code === 'ENOENT') return null; - logger.warn('Failed to load user settings', { error: error instanceof Error ? error.message : 'Unknown error' }); - return null; + logger.warn('Failed to decrypt user settings', { error: error instanceof Error ? error.message : 'Unknown error' }); + throw new StoredDataUnreadableError('settings', { cause: error }); } } diff --git a/lib/stored-data-errors.ts b/lib/stored-data-errors.ts new file mode 100644 index 000000000..a09534a1d --- /dev/null +++ b/lib/stored-data-errors.ts @@ -0,0 +1,44 @@ +import { NextResponse } from 'next/server'; +import { StoreUnavailableError } from '@/lib/shared-store/types'; + +/** + * A user's stored document exists but can't be read: it doesn't decrypt + * (another session secret), it is cut short, or the read failed. It is not + * the same as "nothing stored": a route that treated it as empty would merge + * into nothing and overwrite the user's real data on the next save. Routes + * answer 503 and write nothing. + */ +export class StoredDataUnreadableError extends Error { + constructor(what: string, options?: { cause?: unknown }) { + super(`Stored ${what} could not be read`, options); + this.name = 'StoredDataUnreadableError'; + } +} + +/** True for the errors that mean "try again later, nothing was written". */ +export function isStoredDataUnavailable(error: unknown): boolean { + return error instanceof StoredDataUnreadableError || error instanceof StoreUnavailableError; +} + +/** + * The 503 for an unreadable document or an unreachable shared store, or null + * for any other error. The client keeps its own copy and retries. + */ +export function storedDataUnavailableResponse( + error: unknown, + headers?: Record, +): NextResponse | null { + if (error instanceof StoredDataUnreadableError) { + return NextResponse.json( + { error: 'Your stored data could not be read on the server. Nothing was changed.', code: 'stored_data_unreadable' }, + { status: 503, headers: { 'Retry-After': '10', ...headers } }, + ); + } + if (error instanceof StoreUnavailableError) { + return NextResponse.json( + { error: 'Storage is temporarily unavailable. Nothing was changed.', code: 'store_unavailable' }, + { status: 503, headers: { 'Retry-After': '10', ...headers } }, + ); + } + return null; +} From 79d74d490053d118d705c570b87f922349e5a1a1 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Sat, 31 Oct 2026 22:56:56 +0100 Subject: [PATCH 02/31] feat: keep synced settings in the shared store so every instance serves the same ones #1066 --- lib/__tests__/helpers/separate-disk-store.ts | 69 +++++++++ lib/__tests__/settings-shared-store.test.ts | 143 +++++++++++++++++++ lib/settings-sync.ts | 120 +++++++++------- 3 files changed, 280 insertions(+), 52 deletions(-) create mode 100644 lib/__tests__/helpers/separate-disk-store.ts create mode 100644 lib/__tests__/settings-shared-store.test.ts diff --git a/lib/__tests__/helpers/separate-disk-store.ts b/lib/__tests__/helpers/separate-disk-store.ts new file mode 100644 index 000000000..d86428a98 --- /dev/null +++ b/lib/__tests__/helpers/separate-disk-store.ts @@ -0,0 +1,69 @@ +import { mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import path from 'node:path'; +import { localCollection } from '@/lib/shared-store/local'; +import { fakeSharedStore } from '@/lib/shared-store/__tests__/fake-shared'; +import { + StoreUnavailableError, + type DocCollection, + type KvStore, + type LeaseStore, + type SharedStore, +} from '@/lib/shared-store/types'; + +export type SeparateDiskStore = SharedStore & { down: boolean; root: string; cleanup(): void }; + +/** + * A fake shared store (mode 'redis') whose documents live in a folder of + * their own, not in the dirs a store names for local mode. Every replica has + * its own disk: a store that still wrote its local files would not find on + * "replica B" (another SETTINGS_DATA_DIR, …) what it saved on "replica A". + * `down` makes every document, kv and lease call fail as an outage would. + */ +export function separateDiskStore(): SeparateDiskStore { + const root = mkdtempSync(path.join(tmpdir(), 'bw-shared-store-')); + const base = fakeSharedStore(); + const guard = (run: () => Promise): Promise => + store.down ? Promise.reject(new StoreUnavailableError('down (test)')) : run(); + + const kv: KvStore = { + get: (key) => guard(() => base.kv.get(key)), + set: (key, value, options) => guard(() => base.kv.set(key, value, options)), + del: (key) => guard(() => base.kv.del(key)), + take: (key) => guard(() => base.kv.take(key)), + update: (key, fn, options) => guard(() => base.kv.update(key, fn, options)), + }; + const leases: LeaseStore = { + acquire: (name, holder, ttlMs) => guard(() => base.leases.acquire(name, holder, ttlMs)), + release: (name, holder) => guard(() => base.leases.release(name, holder)), + holder: (name) => guard(() => base.leases.holder(name)), + }; + + const store: SeparateDiskStore = { + ...base, + kv, + leases, + root, + get down() { + return base.down; + }, + set down(value: boolean) { + base.down = value; + }, + collection(name, options): DocCollection { + const inner = localCollection(name, { ...options, local: { ...options.local, dir: () => path.join(root, name) } }); + return { + name, + get: (id) => guard(() => inner.get(id)), + put: (id, data, putOptions) => guard(() => inner.put(id, data, putOptions)), + update: (id, fn, updateOptions) => guard(() => inner.update(id, fn, updateOptions)), + delete: (id, deleteOptions) => guard(() => inner.delete(id, deleteOptions)), + list: (prefix) => guard(() => inner.list(prefix)), + }; + }, + cleanup() { + rmSync(root, { recursive: true, force: true }); + }, + }; + return store; +} diff --git a/lib/__tests__/settings-shared-store.test.ts b/lib/__tests__/settings-shared-store.test.ts new file mode 100644 index 000000000..3a58e2a93 --- /dev/null +++ b/lib/__tests__/settings-shared-store.test.ts @@ -0,0 +1,143 @@ +// @vitest-environment node +import { mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import path from 'node:path'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { NextRequest } from 'next/server'; +import { setStoreForTests } from '@/lib/shared-store'; +import { resetLocalStoreForTests } from '@/lib/shared-store/local'; +import { SETTINGS_COLLECTION, settingsId } from '@/lib/settings-sync'; +import { separateDiskStore, type SeparateDiskStore } from './helpers/separate-disk-store'; + +// Synced settings with a shared store (#1066): every replica reads and +// writes the one store, never its own disk. + +vi.mock('@/lib/auth/session-secret', () => ({ + getSessionSecret: () => 's'.repeat(64), + hasSessionSecret: () => true, +})); +vi.mock('@/lib/logger', () => ({ + logger: { warn: () => {}, error: () => {}, info: () => {}, debug: () => {} }, +})); +vi.mock('@/lib/admin/config-manager', () => ({ + configManager: { + ensureLoaded: async () => {}, + get: (key: string, fallback: unknown) => (key === 'settingsSyncEnabled' ? true : fallback), + getPolicy: () => ({ restrictions: {} }), + }, +})); + +const jar = new Map(); +vi.mock('next/headers', () => ({ + cookies: async () => ({ + get: (name: string) => (jar.has(name) ? { name, value: jar.get(name)! } : undefined), + set: (name: string, value: string) => { jar.set(name, value); }, + delete: (name: string) => { jar.delete(name); }, + getAll: () => [...jar].map(([name, value]) => ({ name, value })), + }), +})); + +const SERVER = 'https://mail.example.org'; +const USER = 'alice@example.org'; + +let disks: string; +let store: SeparateDiskStore; + +/** Point this process at one replica's own data dir. */ +function onReplica(name: 'a' | 'b') { + vi.stubEnv('SETTINGS_DATA_DIR', path.join(disks, name, 'settings')); +} + +beforeEach(async () => { + disks = mkdtempSync(path.join(tmpdir(), 'bw-settings-replicas-')); + store = separateDiskStore(); + setStoreForTests(store); + vi.stubEnv('SETTINGS_SYNC_ENABLED', 'true'); + onReplica('a'); + jar.clear(); + const { setStalwartAuthContextInStore } = await import('@/lib/stalwart/auth-context'); + setStalwartAuthContextInStore({ + get: (n: string) => (jar.has(n) ? { name: n, value: jar.get(n)! } : undefined), + set: (n: string, v: string) => { jar.set(n, v); }, + delete: (n: string) => { jar.delete(n); }, + } as never, 0, { serverUrl: SERVER, authHeader: 'Bearer t', username: USER }); +}); + +afterEach(() => { + setStoreForTests(null); + resetLocalStoreForTests(); + vi.unstubAllEnvs(); + store.cleanup(); + rmSync(disks, { recursive: true, force: true }); +}); + +function template(id: string) { + const at = '2026-10-01T00:00:00.000Z'; + return { id, name: id, subject: '', body: '', isHTML: false, category: '', isFavorite: false, createdAt: at, updatedAt: at }; +} + +async function push(settings: Record) { + const { POST } = await import('@/app/api/settings/route'); + return POST(new NextRequest('https://webmail.example/api/settings', { + method: 'POST', + headers: { 'sec-fetch-site': 'same-origin', 'content-type': 'application/json' }, + body: JSON.stringify({ username: USER, serverUrl: SERVER, settings }), + })); +} + +async function fetchSettings() { + const { GET } = await import('@/app/api/settings/route'); + return GET(new NextRequest('https://webmail.example/api/settings', { + headers: { 'x-settings-username': USER, 'x-settings-server': SERVER }, + })); +} + +function storedDoc() { + return store.collection('settings', SETTINGS_COLLECTION).get(settingsId(USER, SERVER)); +} + +describe('settings with a shared store', () => { + it('loads on replica B what was saved through replica A', async () => { + expect((await push({ theme: 'dark' })).status).toBe(200); + + onReplica('b'); + const res = await fetchSettings(); + expect(res.status).toBe(200); + expect((await res.json()).settings).toEqual({ theme: 'dark' }); + }); + + it('deletes them for every replica', async () => { + await push({ theme: 'dark' }); + onReplica('b'); + const { DELETE } = await import('@/app/api/settings/route'); + const res = await DELETE(new NextRequest('https://webmail.example/api/settings', { + method: 'DELETE', + headers: { 'sec-fetch-site': 'same-origin', 'content-type': 'application/json' }, + body: JSON.stringify({ username: USER, serverUrl: SERVER }), + })); + expect(res.status).toBe(200); + onReplica('a'); + expect((await (await fetchSettings()).json()).settings).toBeNull(); + }); + + it('answers 503 to a save over a stored document that does not decrypt and leaves its bytes alone', async () => { + const garbage = Buffer.alloc(64, 7); + await store.collection('settings', SETTINGS_COLLECTION).put(settingsId(USER, SERVER), garbage); + + expect((await push({ theme: 'light' })).status).toBe(503); + expect((await fetchSettings()).status).toBe(503); + expect((await storedDoc())?.data.equals(garbage)).toBe(true); + }); + + it('answers 503 and writes nothing while the store is down', async () => { + await push({ theme: 'dark' }); + const before = (await storedDoc())!.data; + + store.down = true; + expect((await push({ theme: 'light' })).status).toBe(503); + expect((await fetchSettings()).status).toBe(503); + store.down = false; + + expect((await storedDoc())!.data.equals(before)).toBe(true); + }); +}); diff --git a/lib/settings-sync.ts b/lib/settings-sync.ts index a7b8e0742..dba37ea09 100644 --- a/lib/settings-sync.ts +++ b/lib/settings-sync.ts @@ -1,15 +1,16 @@ import { createHash, createCipheriv, createDecipheriv, randomBytes } from 'node:crypto'; -import { readFile, writeFile, unlink, mkdir, rename } from 'node:fs/promises'; -import { existsSync } from 'node:fs'; import path from 'node:path'; import { logger } from '@/lib/logger'; import { getSessionSecret } from '@/lib/auth/session-secret'; +import { getStore, StoreUnavailableError, type CollectionOptions } from '@/lib/shared-store'; import { StoredDataUnreadableError } from '@/lib/stored-data-errors'; const ALGORITHM = 'aes-256-gcm'; const IV_LENGTH = 12; const TAG_LENGTH = 16; +type Settings = Record; + function getKey(): Buffer { const secret = getSessionSecret(); if (!secret) throw new Error('SESSION_SECRET not configured'); @@ -20,60 +21,38 @@ export function getSettingsDir(): string { return process.env.SETTINGS_DATA_DIR || path.join(process.cwd(), 'data', 'settings'); } -function getSettingsPath(username: string, serverUrl: string): string { - const hash = createHash('sha256').update(`${username}:${serverUrl}`).digest('hex'); - const filePath = path.join(getSettingsDir(), `${hash}.enc`); - // Defense in depth: ensure path stays within the settings directory - const resolvedDir = path.resolve(getSettingsDir()); - const resolvedPath = path.resolve(filePath); - if (!resolvedPath.startsWith(resolvedDir + path.sep) && resolvedPath !== resolvedDir) { - throw new Error('Invalid settings path'); - } - return resolvedPath; +/** The document id of an account's settings: sha256 of `user:server`, hex. */ +export function settingsId(username: string, serverUrl: string): string { + return createHash('sha256').update(`${username}:${serverUrl}`).digest('hex'); } -export async function saveUserSettings(username: string, serverUrl: string, settings: Record): Promise { - const dir = getSettingsDir(); - if (!existsSync(dir)) { - await mkdir(dir, { recursive: true }); - } - - const key = getKey(); - const iv = randomBytes(IV_LENGTH); - const cipher = createCipheriv(ALGORITHM, key, iv); - - const payload = JSON.stringify(settings); - const encrypted = Buffer.concat([cipher.update(payload, 'utf8'), cipher.final()]); - const tag = cipher.getAuthTag(); - - const data = Buffer.concat([iv, tag, encrypted]); - const targetPath = getSettingsPath(username, serverUrl); - const tmpPath = targetPath + '.tmp'; - await writeFile(tmpPath, data); - await rename(tmpPath, targetPath); -} +const SETTINGS_FILE = /^[0-9a-f]{64}\.enc$/; /** - * The user's stored settings, or null when none are stored. A file that - * exists but can't be read or decrypted (another session secret, cut short) - * throws StoredDataUnreadableError: treating it as "nothing stored" would - * let the next save overwrite the user's real settings. + * One encrypted document per account (#1066). Local mode: today's + * `/.enc`, `iv‖tag‖ciphertext`. The account archives live + * in a subfolder of the same dir and are not part of this collection. */ -export async function loadUserSettings(username: string, serverUrl: string): Promise | null> { - const filePath = getSettingsPath(username, serverUrl); +export const SETTINGS_COLLECTION: CollectionOptions = { + local: { + dir: () => getSettingsDir(), + file: (id) => `${id}.enc`, + idOf: (relative) => (SETTINGS_FILE.test(relative) ? relative.slice(0, -'.enc'.length) : null), + }, +}; - let data: Buffer; - try { - data = await readFile(filePath); - } catch (error) { - if ((error as NodeJS.ErrnoException).code === 'ENOENT') return null; - logger.warn('Failed to read user settings', { error: error instanceof Error ? error.message : 'Unknown error' }); - throw new StoredDataUnreadableError('settings', { cause: error }); - } - return decryptSettings(data); +function collection() { + return getStore().collection('settings', SETTINGS_COLLECTION); } -function decryptSettings(data: Buffer): Record { +function encryptSettings(settings: Settings): Buffer { + const iv = randomBytes(IV_LENGTH); + const cipher = createCipheriv(ALGORITHM, getKey(), iv); + const encrypted = Buffer.concat([cipher.update(JSON.stringify(settings), 'utf8'), cipher.final()]); + return Buffer.concat([iv, cipher.getAuthTag(), encrypted]); +} + +function decryptSettings(data: Buffer): Settings { if (data.length < IV_LENGTH + TAG_LENGTH) throw new StoredDataUnreadableError('settings'); const key = getKey(); try { @@ -92,12 +71,49 @@ function decryptSettings(data: Buffer): Record { } } +export async function saveUserSettings(username: string, serverUrl: string, settings: Settings): Promise { + await collection().put(settingsId(username, serverUrl), encryptSettings(settings)); +} + +/** + * The user's stored settings, or null when none are stored. A document that + * exists but can't be read or decrypted (another session secret, cut short) + * throws StoredDataUnreadableError: treating it as "nothing stored" would + * let the next save overwrite the user's real settings. A shared store that + * can't be reached throws StoreUnavailableError. + */ +export async function loadUserSettings(username: string, serverUrl: string): Promise { + let stored; + try { + stored = await collection().get(settingsId(username, serverUrl)); + } catch (error) { + if (error instanceof StoreUnavailableError) throw error; + logger.warn('Failed to read user settings', { error: error instanceof Error ? error.message : 'Unknown error' }); + throw new StoredDataUnreadableError('settings', { cause: error }); + } + return stored ? decryptSettings(stored.data) : null; +} + +/** + * Read, change and write the user's settings in one step, so a save that + * lands at the same time (another tab, another replica) isn't lost: `change` + * gets what is stored now (null for nothing) and returns what to store. It + * may run more than once. Throws like loadUserSettings, writing nothing. + */ +export async function updateUserSettings( + username: string, + serverUrl: string, + change: (stored: Settings | null) => Settings, +): Promise { + await collection().update(settingsId(username, serverUrl), (current) => + encryptSettings(change(current ? decryptSettings(current) : null))); +} + export async function deleteUserSettings(username: string, serverUrl: string): Promise { try { - await unlink(getSettingsPath(username, serverUrl)); + await collection().delete(settingsId(username, serverUrl)); } catch (error) { - if ((error as NodeJS.ErrnoException).code !== 'ENOENT') { - logger.warn('Failed to delete user settings', { error: error instanceof Error ? error.message : 'Unknown error' }); - } + if (error instanceof StoreUnavailableError) throw error; + logger.warn('Failed to delete user settings', { error: error instanceof Error ? error.message : 'Unknown error' }); } } From 965109863497bf1ebb727e1e3c292a7dbb3d5a58 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Sat, 31 Oct 2026 22:58:14 +0100 Subject: [PATCH 03/31] fix: keep both templates when two settings saves arrive at once #1066 --- app/api/settings/route.ts | 9 ++++--- .../settings-route-account-key.test.ts | 3 +++ .../settings-route-template-merge.test.ts | 3 +++ lib/__tests__/settings-shared-store.test.ts | 26 +++++++++++++++++++ 4 files changed, 37 insertions(+), 4 deletions(-) diff --git a/app/api/settings/route.ts b/app/api/settings/route.ts index b1ecf91e6..b7223ddf2 100644 --- a/app/api/settings/route.ts +++ b/app/api/settings/route.ts @@ -5,7 +5,7 @@ import { logger } from '@/lib/logger'; import { decryptSession } from '@/lib/auth/crypto'; import { sessionCookieName } from '@/lib/auth/session-cookie'; import { readStalwartAuthContextFromStore } from '@/lib/stalwart/auth-context'; -import { saveUserSettings, loadUserSettings, deleteUserSettings } from '@/lib/settings-sync'; +import { updateUserSettings, loadUserSettings, deleteUserSettings } from '@/lib/settings-sync'; import { configManager } from '@/lib/admin/config-manager'; import { resolvePolicy } from '@/lib/admin/policy-scopes'; import { hasSessionSecret } from '@/lib/auth/session-secret'; @@ -213,11 +213,12 @@ export async function POST(request: NextRequest) { // Templates and iCal subscriptions are merged into the stored blob rather // than replaced, so a stale tab or device cannot drop ones another added. - const stored = await loadUserSettings(accountName, storageServerUrl(serverUrl)); - await saveUserSettings( + // In one step against what is stored now: two saves at once (two tabs, + // two replicas) each keep the other's additions. + await updateUserSettings( accountName, storageServerUrl(serverUrl), - mergeStoredSubscriptions(stored, mergeStoredTemplates(stored, filteredSettings)), + (stored) => mergeStoredSubscriptions(stored, mergeStoredTemplates(stored, filteredSettings)), ); return NextResponse.json({ ok: true }); } catch (error) { diff --git a/lib/__tests__/settings-route-account-key.test.ts b/lib/__tests__/settings-route-account-key.test.ts index e4ded638d..f6f99e7a6 100644 --- a/lib/__tests__/settings-route-account-key.test.ts +++ b/lib/__tests__/settings-route-account-key.test.ts @@ -39,6 +39,9 @@ vi.mock('@/lib/settings-sync', () => ({ saveUserSettings: async (username: string, serverUrl: string, settings: Record) => { store.set(`${username}|${serverUrl}`, settings); }, + updateUserSettings: async (username: string, serverUrl: string, change: (stored: Record | null) => Record) => { + store.set(`${username}|${serverUrl}`, change(store.get(`${username}|${serverUrl}`) ?? null)); + }, deleteUserSettings: async (username: string, serverUrl: string) => { store.delete(`${username}|${serverUrl}`); }, })); diff --git a/lib/__tests__/settings-route-template-merge.test.ts b/lib/__tests__/settings-route-template-merge.test.ts index 662edc1d6..befaf929c 100644 --- a/lib/__tests__/settings-route-template-merge.test.ts +++ b/lib/__tests__/settings-route-template-merge.test.ts @@ -38,6 +38,9 @@ vi.mock('@/lib/settings-sync', () => ({ saveUserSettings: async (username: string, serverUrl: string, settings: Record) => { store.set(`${username}|${serverUrl}`, settings); }, + updateUserSettings: async (username: string, serverUrl: string, change: (stored: Record | null) => Record) => { + store.set(`${username}|${serverUrl}`, change(store.get(`${username}|${serverUrl}`) ?? null)); + }, deleteUserSettings: async () => {}, })); diff --git a/lib/__tests__/settings-shared-store.test.ts b/lib/__tests__/settings-shared-store.test.ts index 3a58e2a93..970f006cd 100644 --- a/lib/__tests__/settings-shared-store.test.ts +++ b/lib/__tests__/settings-shared-store.test.ts @@ -141,3 +141,29 @@ describe('settings with a shared store', () => { expect((await storedDoc())!.data.equals(before)).toBe(true); }); }); + +// Two saves at once, each from a tab that added a template the other +// doesn't know yet. The route used to load, merge and save without a lock, +// so the second save put its merge over the first: one template was lost, +// with two tabs on one instance as well as across replicas. +async function twoConcurrentSaves() { + await push({ templates: [], deletedTemplateIds: {} }); + const [a, b] = await Promise.all([ + push({ templates: [template('from-a')], deletedTemplateIds: {} }), + push({ templates: [template('from-b')], deletedTemplateIds: {} }), + ]); + expect([a.status, b.status]).toEqual([200, 200]); + const saved = (await (await fetchSettings()).json()).settings as { templates: { id: string }[] }; + return saved.templates.map((t) => t.id).sort(); +} + +describe('two saves at once', () => { + it('keep both templates with a shared store', async () => { + expect(await twoConcurrentSaves()).toEqual(['from-a', 'from-b']); + }); + + it('keep both templates on a single instance', async () => { + setStoreForTests(null); + expect(await twoConcurrentSaves()).toEqual(['from-a', 'from-b']); + }); +}); From 741983c53cee3541bb819378fae29b0a9f4eae8a Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Sat, 31 Oct 2026 23:00:42 +0100 Subject: [PATCH 04/31] feat: keep users' plugin credentials in the shared store without losing a concurrent save #1066 --- .../plugin-vault-shared-store.test.ts | 99 ++++++++++++++++ lib/plugin-secrets/vault.ts | 112 ++++++++++-------- lib/settings-sync.ts | 8 +- lib/stored-data-errors.ts | 22 +++- 4 files changed, 181 insertions(+), 60 deletions(-) create mode 100644 lib/__tests__/plugin-vault-shared-store.test.ts diff --git a/lib/__tests__/plugin-vault-shared-store.test.ts b/lib/__tests__/plugin-vault-shared-store.test.ts new file mode 100644 index 000000000..a6171ff64 --- /dev/null +++ b/lib/__tests__/plugin-vault-shared-store.test.ts @@ -0,0 +1,99 @@ +// @vitest-environment node +import { mkdtempSync, readdirSync, rmSync, statSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import path from 'node:path'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { setStoreForTests } from '@/lib/shared-store'; +import { resetLocalStoreForTests } from '@/lib/shared-store/local'; +import { separateDiskStore, type SeparateDiskStore } from './helpers/separate-disk-store'; + +// Users' plugin credentials with a shared store (#1066). The vault used to +// serialise writes with a queue in its own module, which no other process +// (and no other module copy) sees: two writes on two replicas lost one. + +vi.mock('@/lib/auth/session-secret', () => ({ + getSessionSecret: () => 's'.repeat(64), + hasSessionSecret: () => true, +})); +vi.mock('@/lib/logger', () => ({ + logger: { warn: () => {}, error: () => {}, info: () => {}, debug: () => {} }, +})); + +const user = { username: 'alice@example.org', serverUrl: 'https://mail.example.org' }; +const input = (secret: string) => ({ origin: 'https://cloud.example.org', inject: { type: 'basic' } as const, label: 'Cloud', secret }); + +type Vault = typeof import('@/lib/plugin-secrets/vault'); + +/** A fresh copy of the vault module, as another process (or bundle) has. */ +async function replica(): Promise { + vi.resetModules(); + return import('@/lib/plugin-secrets/vault'); +} + +let disks: string; +let store: SeparateDiskStore; + +beforeEach(() => { + disks = mkdtempSync(path.join(tmpdir(), 'bw-vault-replicas-')); + vi.stubEnv('PLUGIN_SECRETS_DIR', path.join(disks, 'a')); + store = separateDiskStore(); + setStoreForTests(store); +}); + +afterEach(() => { + setStoreForTests(null); + resetLocalStoreForTests(); + vi.unstubAllEnvs(); + store.cleanup(); + rmSync(disks, { recursive: true, force: true }); +}); + +describe('plugin credentials with a shared store', () => { + it('reads on replica B what replica A stored', async () => { + const a = await replica(); + await a.putUserCredential(user, 'nextcloud', 'cloud', input('pw')); + + vi.stubEnv('PLUGIN_SECRETS_DIR', path.join(disks, 'b')); + const b = await replica(); + expect(await b.getUserCredential(user, 'nextcloud', 'cloud')).toMatchObject({ secret: 'pw' }); + }); + + it('keeps both of two credentials saved at once through two replicas', async () => { + const a = await replica(); + const b = await replica(); + await Promise.all([ + a.putUserCredential(user, 'nextcloud', 'cloud', input('one')), + b.putUserCredential(user, 'paperless', 'docs', input('two')), + a.putUserCredential(user, 'radicale', 'dav', input('three')), + b.putUserCredential(user, 'immich', 'photos', input('four')), + ]); + + const ids = (await a.listUserCredentials(user)).map((c) => c.pluginId).sort(); + expect(ids).toEqual(['immich', 'nextcloud', 'paperless', 'radicale']); + }); + + it('answers with StoreUnavailableError while the store is down and writes nothing', async () => { + const vault = await replica(); + await vault.putUserCredential(user, 'nextcloud', 'cloud', input('pw')); + store.down = true; + await expect(vault.putUserCredential(user, 'paperless', 'docs', input('x'))).rejects.toThrow('down (test)'); + await expect(vault.listUserCredentials(user)).rejects.toThrow('down (test)'); + store.down = false; + expect((await vault.listUserCredentials(user)).map((c) => c.pluginId)).toEqual(['nextcloud']); + }); +}); + +describe('plugin credentials on a single instance', () => { + it('keep today\'s file: .enc in the vault dir, mode 0600, gone when the last one goes', async () => { + setStoreForTests(null); + const vault = await replica(); + await vault.putUserCredential(user, 'nextcloud', 'cloud', input('pw')); + const dir = path.join(disks, 'a'); + const files = readdirSync(dir); + expect(files).toEqual([`${vault.userKey(user)}.enc`]); + if (process.platform !== 'win32') expect(statSync(path.join(dir, files[0])).mode & 0o777).toBe(0o600); + + await vault.deleteUserCredentials(user, 'nextcloud'); + expect(readdirSync(dir)).toEqual([]); + }); +}); diff --git a/lib/plugin-secrets/vault.ts b/lib/plugin-secrets/vault.ts index 79459941d..b497d6e31 100644 --- a/lib/plugin-secrets/vault.ts +++ b/lib/plugin-secrets/vault.ts @@ -1,13 +1,13 @@ // Per-user store of the credentials users connected for plugins. One -// encrypted file per mail account; the secrets never leave the server again -// except as an auth header on a request the proxy forwards to the +// encrypted document per mail account; the secrets never leave the server +// again except as an auth header on a request the proxy forwards to the // credential's own origin. import { createHash } from 'node:crypto'; -import { readFile, writeFile, mkdir, rename, unlink } from 'node:fs/promises'; import path from 'node:path'; import { decryptPayload, encryptPayload } from '@/lib/auth/crypto'; -import { StoredDataUnreadableError } from '@/lib/stored-data-errors'; +import { getStore, type CollectionOptions } from '@/lib/shared-store'; +import { isStoreUnavailable, StoredDataUnreadableError } from '@/lib/stored-data-errors'; import type { CredentialInject } from './credentials'; export interface VaultUser { @@ -50,27 +50,33 @@ export function userKey(user: VaultUser): string { return createHash('sha256').update(`${user.username}:${user.serverUrl}`).digest('hex'); } -function vaultPath(user: VaultUser): string { - const dir = path.resolve(vaultDir()); - const file = path.resolve(dir, `${userKey(user)}.enc`); - if (!file.startsWith(dir + path.sep)) throw new Error('Invalid vault path'); - return file; +const VAULT_FILE = /^[0-9a-f]{64}\.enc$/; + +/** + * One document per account, keyed by userKey (#1066). Local mode: today's + * `/.enc`, base64 `encryptPayload` text, mode 0600. + */ +export const VAULT_COLLECTION: CollectionOptions = { + local: { + dir: () => vaultDir(), + file: (id) => `${id}.enc`, + idOf: (relative) => (VAULT_FILE.test(relative) ? relative.slice(0, -'.enc'.length) : null), + mode: 0o600, + }, +}; + +function collection() { + return getStore().collection('plugin-secrets', VAULT_COLLECTION); } /** - * The user's credentials; {} when none are stored. A file that exists but - * can't be read or doesn't decrypt (another session secret) throws - * StoredDataUnreadableError, so no write lands on top of it. + * The user's credentials from a stored document; {} for none. One that + * doesn't decrypt (another session secret) throws StoredDataUnreadableError, + * so no write lands on top of it. */ -async function load(user: VaultUser): Promise { - let raw: string; - try { - raw = await readFile(vaultPath(user), 'utf8'); - } catch (error) { - if ((error as NodeJS.ErrnoException).code === 'ENOENT') return {}; - throw new StoredDataUnreadableError('plugin credentials', { cause: error }); - } - const payload = decryptPayload(raw, 'plugin-secrets'); +function decode(user: VaultUser, raw: Buffer | null): VaultData { + if (!raw) return {}; + const payload = decryptPayload(raw.toString('utf8'), 'plugin-secrets'); if (!payload) throw new StoredDataUnreadableError('plugin credentials'); // Bound to this user: a file copied over from another account's name // decrypts, but carries the other account's key and is ignored. @@ -79,28 +85,34 @@ async function load(user: VaultUser): Promise { return data && typeof data === 'object' ? (data as VaultData) : {}; } -async function save(user: VaultUser, data: VaultData): Promise { - const file = vaultPath(user); - if (Object.keys(data).length === 0) { - try { await unlink(file); } catch { /* nothing stored */ } - return; +async function load(user: VaultUser): Promise { + let stored; + try { + stored = await collection().get(userKey(user)); + } catch (error) { + if (isStoreUnavailable(error)) throw error; + throw new StoredDataUnreadableError('plugin credentials', { cause: error }); } - await mkdir(path.dirname(file), { recursive: true }); - const tmp = `${file}.${process.pid}.tmp`; - await writeFile(tmp, encryptPayload({ u: userKey(user), d: data }, 'plugin-secrets'), { encoding: 'utf8', mode: 0o600 }); - await rename(tmp, file); + return decode(user, stored?.data ?? null); } -// Writes for one user run one after another, so two saves can't drop each -// other's changes. -const queues = new Map>(); -function serialize(user: VaultUser, task: () => Promise): Promise { - const key = userKey(user); - const run = (queues.get(key) ?? Promise.resolve()).then(task, task); - const tail = run.catch(() => {}); - queues.set(key, tail); - void tail.then(() => { if (queues.get(key) === tail) queues.delete(key); }); - return run; +/** + * Change the user's credentials in one step against what is stored now, so + * two writes at once (two tabs, two replicas) both land. `change` edits the + * data it gets and says whether it changed anything; it may run more than + * once. An empty vault is deleted. + */ +async function modify(user: VaultUser, change: (data: VaultData) => { result: T; changed: boolean }): Promise { + let result!: T; + await collection().update(userKey(user), (current) => { + const data = decode(user, current); + const out = change(data); + result = out.result; + if (!out.changed) return undefined; + if (Object.keys(data).length === 0) return current ? null : undefined; + return encryptPayload({ u: userKey(user), d: data }, 'plugin-secrets'); + }); + return result; } export async function getUserCredential(user: VaultUser, pluginId: string, credentialId: string): Promise { @@ -141,8 +153,7 @@ export function putUserCredential( credentialId: string, input: { origin: string; inject: CredentialInject; label: string; username?: string; secret?: string }, ): Promise { - return serialize(user, async () => { - const data = await load(user); + return modify(user, (data) => { const existing = data[pluginId]?.[credentialId]; const count = Object.values(data).reduce((n, c) => n + Object.keys(c).length, 0); if (!existing && count >= MAX_USER_CREDENTIALS) throw new VaultFullError(); @@ -160,7 +171,7 @@ export function putUserCredential( updatedAt: now, }, }; - await save(user, data); + return { result: undefined, changed: true }; }); } @@ -172,32 +183,29 @@ export function setUserCredentialSecret( secret: string, username?: string, ): Promise { - return serialize(user, async () => { - const data = await load(user); + return modify(user, (data) => { const existing = data[pluginId]?.[credentialId]; - if (!existing) return false; + if (!existing) return { result: false, changed: false }; data[pluginId][credentialId] = { ...existing, secret, ...(username ? { username } : {}), updatedAt: new Date().toISOString(), }; - await save(user, data); - return true; + return { result: true, changed: true }; }); } /** Remove one credential, or all of a plugin's when credentialId is omitted. */ export function deleteUserCredentials(user: VaultUser, pluginId: string, credentialId?: string): Promise { - return serialize(user, async () => { - const data = await load(user); - if (!data[pluginId]) return; + return modify(user, (data) => { + if (!data[pluginId]) return { result: undefined, changed: false }; if (credentialId) { delete data[pluginId][credentialId]; if (Object.keys(data[pluginId]).length === 0) delete data[pluginId]; } else { delete data[pluginId]; } - await save(user, data); + return { result: undefined, changed: true }; }); } diff --git a/lib/settings-sync.ts b/lib/settings-sync.ts index dba37ea09..f459aebfc 100644 --- a/lib/settings-sync.ts +++ b/lib/settings-sync.ts @@ -2,8 +2,8 @@ import { createHash, createCipheriv, createDecipheriv, randomBytes } from 'node: import path from 'node:path'; import { logger } from '@/lib/logger'; import { getSessionSecret } from '@/lib/auth/session-secret'; -import { getStore, StoreUnavailableError, type CollectionOptions } from '@/lib/shared-store'; -import { StoredDataUnreadableError } from '@/lib/stored-data-errors'; +import { getStore, type CollectionOptions } from '@/lib/shared-store'; +import { isStoreUnavailable, StoredDataUnreadableError } from '@/lib/stored-data-errors'; const ALGORITHM = 'aes-256-gcm'; const IV_LENGTH = 12; @@ -87,7 +87,7 @@ export async function loadUserSettings(username: string, serverUrl: string): Pro try { stored = await collection().get(settingsId(username, serverUrl)); } catch (error) { - if (error instanceof StoreUnavailableError) throw error; + if (isStoreUnavailable(error)) throw error; logger.warn('Failed to read user settings', { error: error instanceof Error ? error.message : 'Unknown error' }); throw new StoredDataUnreadableError('settings', { cause: error }); } @@ -113,7 +113,7 @@ export async function deleteUserSettings(username: string, serverUrl: string): P try { await collection().delete(settingsId(username, serverUrl)); } catch (error) { - if (error instanceof StoreUnavailableError) throw error; + if (isStoreUnavailable(error)) throw error; logger.warn('Failed to delete user settings', { error: error instanceof Error ? error.message : 'Unknown error' }); } } diff --git a/lib/stored-data-errors.ts b/lib/stored-data-errors.ts index a09534a1d..453f93214 100644 --- a/lib/stored-data-errors.ts +++ b/lib/stored-data-errors.ts @@ -1,5 +1,4 @@ import { NextResponse } from 'next/server'; -import { StoreUnavailableError } from '@/lib/shared-store/types'; /** * A user's stored document exists but can't be read: it doesn't decrypt @@ -15,9 +14,24 @@ export class StoredDataUnreadableError extends Error { } } +// By name, not instanceof: Next bundles each entry with its own copy of a +// module, so the store's error class may not be the one this file imports. +function named(error: unknown, name: string): boolean { + return error instanceof Error && error.name === name; +} + +/** The shared store couldn't be reached (StoreUnavailableError from lib/shared-store). */ +export function isStoreUnavailable(error: unknown): boolean { + return named(error, 'StoreUnavailableError'); +} + +export function isStoredDataUnreadable(error: unknown): boolean { + return named(error, 'StoredDataUnreadableError'); +} + /** True for the errors that mean "try again later, nothing was written". */ export function isStoredDataUnavailable(error: unknown): boolean { - return error instanceof StoredDataUnreadableError || error instanceof StoreUnavailableError; + return isStoredDataUnreadable(error) || isStoreUnavailable(error); } /** @@ -28,13 +42,13 @@ export function storedDataUnavailableResponse( error: unknown, headers?: Record, ): NextResponse | null { - if (error instanceof StoredDataUnreadableError) { + if (isStoredDataUnreadable(error)) { return NextResponse.json( { error: 'Your stored data could not be read on the server. Nothing was changed.', code: 'stored_data_unreadable' }, { status: 503, headers: { 'Retry-After': '10', ...headers } }, ); } - if (error instanceof StoreUnavailableError) { + if (isStoreUnavailable(error)) { return NextResponse.json( { error: 'Storage is temporarily unavailable. Nothing was changed.', code: 'store_unavailable' }, { status: 503, headers: { 'Retry-After': '10', ...headers } }, From 7148dd2deda598d1317717180233012bb741af1b Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Sat, 31 Oct 2026 23:04:02 +0100 Subject: [PATCH 05/31] feat: keep account archives in the shared store #1066 --- app/api/account-archive/route.ts | 12 +- .../account-archive-shared-store.test.ts | 104 ++++++++++++++++++ lib/account-archive-storage.ts | 58 ++++++---- 3 files changed, 147 insertions(+), 27 deletions(-) create mode 100644 lib/__tests__/account-archive-shared-store.test.ts diff --git a/app/api/account-archive/route.ts b/app/api/account-archive/route.ts index 9c77f6ce8..fdbf79c8c 100644 --- a/app/api/account-archive/route.ts +++ b/app/api/account-archive/route.ts @@ -11,6 +11,7 @@ import { rejectCrossOriginRequest } from '@/lib/security/same-origin'; import { MAX_ACCOUNT_SLOTS } from '@/lib/account-utils'; import { archiveIdentity, parseArchiveEnvelope, ARCHIVE_MAX_BYTES, type ArchiveOwner } from '@/lib/account-archive'; import { deleteArchive, loadArchive, saveArchive } from '@/lib/account-archive-storage'; +import { isStoreUnavailable } from '@/lib/stored-data-errors'; export const runtime = 'nodejs'; @@ -18,6 +19,11 @@ const reply = (data: unknown, status = 200) => NextResponse.json(data, { status, headers: { 'Cache-Control': 'no-store' }, }); +/** A shared store that can't be reached is a 503 the client retries; nothing was written. */ +const storageFailed = (error: unknown) => isStoreUnavailable(error) + ? NextResponse.json({ error: 'storage_unavailable' }, { status: 503, headers: { 'Cache-Control': 'no-store', 'Retry-After': '10' } }) + : reply({ error: 'storage_failed' }, 500); + /** * The owner whose archive this request may touch, or the response refusing it. * @@ -67,7 +73,7 @@ export async function GET(request: NextRequest) { const owner = await verifiedOwner(request); if (owner instanceof NextResponse) return owner; try { return reply({ envelope: await loadArchive(owner) }); } - catch { return reply({ error: 'storage_failed' }, 500); } + catch (error) { return storageFailed(error); } } export async function PUT(request: NextRequest) { @@ -83,7 +89,7 @@ export async function PUT(request: NextRequest) { envelope = parseArchiveEnvelope((JSON.parse(text) as { envelope?: unknown })?.envelope); } catch { return reply({ error: 'invalid_archive' }, 400); } try { await saveArchive(owner, envelope); return reply({ ok: true }); } - catch { return reply({ error: 'storage_failed' }, 500); } + catch (error) { return storageFailed(error); } } export async function DELETE(request: NextRequest) { @@ -92,5 +98,5 @@ export async function DELETE(request: NextRequest) { const owner = await verifiedOwner(request); if (owner instanceof NextResponse) return owner; try { await deleteArchive(owner); return reply({ ok: true }); } - catch { return reply({ error: 'storage_failed' }, 500); } + catch (error) { return storageFailed(error); } } diff --git a/lib/__tests__/account-archive-shared-store.test.ts b/lib/__tests__/account-archive-shared-store.test.ts new file mode 100644 index 000000000..bb5aa7e26 --- /dev/null +++ b/lib/__tests__/account-archive-shared-store.test.ts @@ -0,0 +1,104 @@ +// @vitest-environment node +import { mkdtempSync, readdirSync, readFileSync, rmSync, statSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import path from 'node:path'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { NextRequest } from 'next/server'; +import { setStoreForTests } from '@/lib/shared-store'; +import { resetLocalStoreForTests } from '@/lib/shared-store/local'; +import { separateDiskStore, type SeparateDiskStore } from './helpers/separate-disk-store'; + +// Account archives with a shared store (#1066): one document per owner, +// replaced whole, the same on every replica. + +const mocks = vi.hoisted(() => ({ contexts: new Map(), verify: vi.fn() })); +vi.mock('@/lib/admin/config-manager', () => ({ configManager: { + ensureLoaded: async () => {}, + get: (_key: string, fallback: unknown) => fallback, + getPolicy: () => ({ features: { accountArchiveEnabled: true } }), +} })); +vi.mock('@/lib/auth/session-secret', () => ({ hasSessionSecret: () => true })); +vi.mock('next/headers', () => ({ cookies: async () => ({}) })); +vi.mock('@/lib/stalwart/auth-context', () => ({ + readStalwartAuthContextFromStore: (_: unknown, slot: number) => mocks.contexts.get(slot) ?? null, +})); +vi.mock('@/lib/auth/verify-jmap-auth', async importOriginal => ({ + ...await importOriginal(), verifyJmapAuth: mocks.verify, +})); + +import { deleteArchive, loadArchive, saveArchive } from '@/lib/account-archive-storage'; +import { GET, PUT } from '@/app/api/account-archive/route'; +import type { ArchiveEnvelope } from '@/lib/account-archive'; + +const owner = { username: 'owner@example.com', serverUrl: 'https://mail.example.com' }; +const envelope: ArchiveEnvelope = { version: 1, iterations: 600000, salt: 'A'.repeat(22) + '==', iv: 'A'.repeat(16), ciphertext: 'A'.repeat(43) + '=' }; +const other: ArchiveEnvelope = { ...envelope, ciphertext: 'B'.repeat(43) + '=' }; + +let disks: string; +let store: SeparateDiskStore; + +beforeEach(() => { + disks = mkdtempSync(path.join(tmpdir(), 'bw-archive-replicas-')); + vi.stubEnv('SETTINGS_DATA_DIR', path.join(disks, 'a')); + store = separateDiskStore(); + setStoreForTests(store); + mocks.contexts.clear(); + mocks.contexts.set(0, { username: owner.username, serverUrl: owner.serverUrl, authHeader: 'Basic ' + Buffer.from(`${owner.username}:pw`).toString('base64') }); + mocks.verify.mockReset().mockResolvedValue(owner.serverUrl); +}); + +afterEach(() => { + setStoreForTests(null); + resetLocalStoreForTests(); + vi.unstubAllEnvs(); + store.cleanup(); + rmSync(disks, { recursive: true, force: true }); +}); + +function request(method: string, body?: unknown) { + return new NextRequest('https://webmail.example.com/api/account-archive', { + method, + headers: { 'x-archive-username': owner.username, 'x-archive-server': owner.serverUrl, 'content-type': 'application/json', 'sec-fetch-site': 'same-origin' }, + ...(body === undefined ? {} : { body: JSON.stringify(body) }), + }); +} + +describe('account archives with a shared store', () => { + it('returns on replica B what replica A stored, and the latest save wins', async () => { + await saveArchive(owner, envelope); + vi.stubEnv('SETTINGS_DATA_DIR', path.join(disks, 'b')); + expect(await loadArchive(owner)).toEqual(envelope); + + await saveArchive(owner, other); + vi.stubEnv('SETTINGS_DATA_DIR', path.join(disks, 'a')); + expect(await loadArchive(owner)).toEqual(other); + + await deleteArchive(owner); + expect(await loadArchive(owner)).toBeNull(); + }); + + it('answers 503 while the store is down and keeps what is stored', async () => { + await saveArchive(owner, envelope); + store.down = true; + expect((await PUT(request('PUT', { envelope: other }))).status).toBe(503); + expect((await GET(request('GET'))).status).toBe(503); + store.down = false; + expect(await loadArchive(owner)).toEqual(envelope); + }); +}); + +describe('account archives on a single instance', () => { + it('keep today\'s file: .json under account-archives, mode 0600', async () => { + setStoreForTests(null); + await saveArchive(owner, envelope); + const dir = path.join(disks, 'a', 'account-archives'); + const files = readdirSync(dir); + expect(files).toHaveLength(1); + expect(JSON.parse(readFileSync(path.join(dir, files[0]), 'utf8'))).toEqual(envelope); + if (process.platform !== 'win32') { + expect(statSync(path.join(dir, files[0])).mode & 0o777).toBe(0o600); + expect(statSync(dir).mode & 0o777).toBe(0o700); + } + expect(files[0]).toMatch(/^[0-9a-f]{64}\.json$/); + }); +}); diff --git a/lib/account-archive-storage.ts b/lib/account-archive-storage.ts index c27c6d58a..80b71ac43 100644 --- a/lib/account-archive-storage.ts +++ b/lib/account-archive-storage.ts @@ -1,41 +1,51 @@ -import { createHash, randomBytes } from 'node:crypto'; -import { mkdir, open, readFile, rename, unlink } from 'node:fs/promises'; +import { createHash } from 'node:crypto'; +import { mkdir } from 'node:fs/promises'; import path from 'node:path'; +import { getStore, isSharedMode, type CollectionOptions } from '@/lib/shared-store'; import { archiveIdentity, parseArchiveEnvelope, type ArchiveEnvelope, type ArchiveOwner } from './account-archive'; /** One file per owner: `SETTINGS_DATA_DIR/account-archives/.json`. */ function archiveDir(): string { return path.join(process.env.SETTINGS_DATA_DIR || path.join(process.cwd(), 'data', 'settings'), 'account-archives'); } -function archivePath(owner: ArchiveOwner): string { - return path.join(archiveDir(), createHash('sha256').update(archiveIdentity(owner)).digest('hex') + '.json'); + +const ARCHIVE_FILE = /^[0-9a-f]{64}\.json$/; + +/** + * One document per owner (#1066), replaced whole on every save. Local mode: + * today's files, mode 0600. The archive is encrypted in the browser with + * the user's password; the server holds no key. + */ +export const ARCHIVE_COLLECTION: CollectionOptions = { + local: { + dir: archiveDir, + file: (id) => `${id}.json`, + idOf: (relative) => (ARCHIVE_FILE.test(relative) ? relative.slice(0, -'.json'.length) : null), + mode: 0o600, + }, +}; + +function collection() { + return getStore().collection('account-archives', ARCHIVE_COLLECTION); +} + +function archiveId(owner: ArchiveOwner): string { + return createHash('sha256').update(archiveIdentity(owner)).digest('hex'); } export async function loadArchive(owner: ArchiveOwner): Promise { - try { return parseArchiveEnvelope(JSON.parse(await readFile(archivePath(owner), 'utf8'))); } - catch (err) { - if ((err as NodeJS.ErrnoException).code === 'ENOENT') return null; - throw err; - } + const stored = await collection().get(archiveId(owner)); + return stored ? parseArchiveEnvelope(JSON.parse(stored.data.toString('utf8'))) : null; } -/** Replaces the owner's archive atomically: a reader sees the old file or the new one, never half of it. */ +/** Replaces the owner's archive atomically: a reader sees the old one or the new one, never half of it. */ export async function saveArchive(owner: ArchiveOwner, envelope: ArchiveEnvelope): Promise { - const target = archivePath(owner); - const temporary = `${target}.${randomBytes(8).toString('hex')}.tmp`; - await mkdir(archiveDir(), { recursive: true, mode: 0o700 }); - try { - const file = await open(temporary, 'wx', 0o600); - try { await file.writeFile(JSON.stringify(parseArchiveEnvelope(envelope))); await file.sync(); } - finally { await file.close(); } - await rename(temporary, target); - } finally { - await unlink(temporary).catch(() => {}); - } + const data = JSON.stringify(parseArchiveEnvelope(envelope)); + // The folder only its owner may list, as it has always been created. + if (!isSharedMode()) await mkdir(archiveDir(), { recursive: true, mode: 0o700 }); + await collection().put(archiveId(owner), data); } export async function deleteArchive(owner: ArchiveOwner): Promise { - await unlink(archivePath(owner)).catch(err => { - if ((err as NodeJS.ErrnoException).code !== 'ENOENT') throw err; - }); + await collection().delete(archiveId(owner)); } From a1ce0d7ec8e1bc656897cb61ee430f5baf67cd99 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Sat, 31 Oct 2026 23:05:14 +0100 Subject: [PATCH 06/31] fix: keep telemetry consent when its state file can't be read #1066 --- .../telemetry-state-read-error.test.ts | 48 +++++++++++++++++++ lib/telemetry/state.ts | 4 ++ 2 files changed, 52 insertions(+) create mode 100644 lib/__tests__/telemetry-state-read-error.test.ts diff --git a/lib/__tests__/telemetry-state-read-error.test.ts b/lib/__tests__/telemetry-state-read-error.test.ts new file mode 100644 index 000000000..8c11b79d0 --- /dev/null +++ b/lib/__tests__/telemetry-state-read-error.test.ts @@ -0,0 +1,48 @@ +// @vitest-environment node +import { mkdtempSync, readFileSync, rmSync, writeFileSync, mkdirSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import path from 'node:path'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +// A telemetry state file that couldn't be read (a torn write, a passing I/O +// error) used to be replaced with the defaults, which turned the admin's +// consent off for good. Now the read answers with the defaults and leaves +// the file alone. + +vi.mock('@/lib/admin/third-party-mode', () => ({ isNoThirdPartyTrafficOn: async () => false })); + +let dir: string; + +beforeEach(() => { + dir = mkdtempSync(path.join(tmpdir(), 'bw-telemetry-state-')); + vi.stubEnv('TELEMETRY_DATA_DIR', dir); + vi.stubEnv('BULWARK_TELEMETRY', ''); +}); + +afterEach(() => { + vi.unstubAllEnvs(); + rmSync(dir, { recursive: true, force: true }); +}); + +describe('telemetry state that cannot be read', () => { + it('keeps the stored consent', async () => { + const { effectiveConsent, loadState } = await import('@/lib/telemetry/state'); + const good = JSON.stringify({ consent: 'on', endpoint: 'https://collector.example.org/', consentedAt: '2026-10-01T00:00:00.000Z', lastSentAt: null, nextScheduledAt: null }); + mkdirSync(dir, { recursive: true }); + const file = path.join(dir, 'state.json'); + writeFileSync(file, good.slice(0, 20)); + + expect((await loadState()).consent).toBe('off'); + expect((await effectiveConsent()).consent).toBe('off'); + expect(readFileSync(file, 'utf8')).toBe(good.slice(0, 20)); + + writeFileSync(file, good); + expect((await effectiveConsent()).consent).toBe('on'); + }); + + it('still writes the default state on a fresh install', async () => { + const { loadState } = await import('@/lib/telemetry/state'); + expect((await loadState()).consent).toBe('off'); + expect(JSON.parse(readFileSync(path.join(dir, 'state.json'), 'utf8')).consent).toBe('off'); + }); +}); diff --git a/lib/telemetry/state.ts b/lib/telemetry/state.ts index 57dc68db5..cae5ccd96 100644 --- a/lib/telemetry/state.ts +++ b/lib/telemetry/state.ts @@ -64,9 +64,13 @@ export async function loadState(): Promise { return { ...DEFAULTS, ...parsed }; } catch (err) { if ((err as NodeJS.ErrnoException).code !== 'ENOENT') { + // Unreadable is not missing: answer with the defaults (so nothing is + // sent on a guess) but leave the file alone, or a passing read error + // would switch the admin's consent off for good. logger.warn('telemetry: state read failed', { error: err instanceof Error ? err.message : String(err), }); + return { ...DEFAULTS }; } // First-ever load on a fresh install: persist the default-off state so the // instance id and (lack of) consent are stable across restarts. The admin From eb0005f3efa65a19ff03af3c9df3d56554a47a1d Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Sat, 31 Oct 2026 23:10:30 +0100 Subject: [PATCH 07/31] feat: share telemetry and update-check state across instances, with one instance id and login count #1066 --- app/api/admin/telemetry/route.ts | 33 ++-- ...lemetry-version-check-shared-store.test.ts | 119 +++++++++++++ lib/telemetry/index.ts | 2 +- lib/telemetry/login-tracker.ts | 80 +++------ lib/telemetry/sender.ts | 11 +- lib/telemetry/state.ts | 166 +++++++++++++----- lib/version-check/index.ts | 2 +- lib/version-check/sender.ts | 32 ++-- lib/version-check/state.ts | 90 ++++++++-- lib/version-check/types.ts | 5 + 10 files changed, 393 insertions(+), 147 deletions(-) create mode 100644 lib/__tests__/telemetry-version-check-shared-store.test.ts diff --git a/app/api/admin/telemetry/route.ts b/app/api/admin/telemetry/route.ts index d0bb0f8c1..aac9d0263 100644 --- a/app/api/admin/telemetry/route.ts +++ b/app/api/admin/telemetry/route.ts @@ -4,8 +4,7 @@ import { auditLog } from '@/lib/admin/audit'; import { logger } from '@/lib/logger'; import { effectiveConsent, - loadState, - saveState, + updateState, buildPayload, sendOnce, reschedule, @@ -13,6 +12,7 @@ import { getLoginCounts, resolveEndpointAllowed, } from '@/lib/telemetry'; +import { storedDataUnavailableResponse } from '@/lib/stored-data-errors'; /** * GET /api/admin/telemetry @@ -48,6 +48,8 @@ export async function GET(request: NextRequest) { logger.error('telemetry GET error', { error: err instanceof Error ? err.message : 'unknown', }); + const unavailable = storedDataUnavailableResponse(err); + if (unavailable) return unavailable; return NextResponse.json({ error: 'Internal server error' }, { status: 500 }); } } @@ -90,13 +92,15 @@ export async function POST(request: NextRequest) { if (body.consent !== 'on' && body.consent !== 'off') { return NextResponse.json({ error: 'consent must be "on" or "off"' }, { status: 400 }); } - const state = await loadState(); - const before = state.consent; - state.consent = body.consent; - if (body.consent === 'on' && !state.consentedAt) { - state.consentedAt = new Date().toISOString(); - } - await saveState(state); + const consent = body.consent; + const now = new Date().toISOString(); + let before: string | undefined; + // Against the stored state, so a schedule written meanwhile stays. + await updateState((state) => { + before = state.consent; + state.consent = consent; + if (consent === 'on' && !state.consentedAt) state.consentedAt = now; + }); await reschedule(); await auditLog('telemetry.set-consent', { from: before, to: body.consent }, ip); return NextResponse.json({ ok: true }); @@ -113,11 +117,12 @@ export async function POST(request: NextRequest) { return NextResponse.json({ error: check.reason }, { status: 400 }); } } - const state = await loadState(); - const before = state.endpoint; + let before: string | undefined; // Empty means no collector: heartbeats stop until a URL is set again. - state.endpoint = trimmed; - await saveState(state); + const state = await updateState((current) => { + before = current.endpoint; + current.endpoint = trimmed; + }); await reschedule(); await auditLog('telemetry.set-endpoint', { from: before, to: state.endpoint }, ip); return NextResponse.json({ ok: true, endpoint: state.endpoint }); @@ -138,6 +143,8 @@ export async function POST(request: NextRequest) { logger.error('telemetry POST error', { error: err instanceof Error ? err.message : 'unknown', }); + const unavailable = storedDataUnavailableResponse(err); + if (unavailable) return unavailable; return NextResponse.json({ error: 'Internal server error' }, { status: 500 }); } } diff --git a/lib/__tests__/telemetry-version-check-shared-store.test.ts b/lib/__tests__/telemetry-version-check-shared-store.test.ts new file mode 100644 index 000000000..895a26d72 --- /dev/null +++ b/lib/__tests__/telemetry-version-check-shared-store.test.ts @@ -0,0 +1,119 @@ +// @vitest-environment node +import { mkdtempSync, readdirSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import path from 'node:path'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { setStoreForTests } from '@/lib/shared-store'; +import { resetLocalStoreForTests } from '@/lib/shared-store/local'; +import type { UpdateStatus } from '@/lib/version-check/types'; +import { separateDiskStore, type SeparateDiskStore } from './helpers/separate-disk-store'; + +// Telemetry and the version check with a shared store (#1066): one instance +// id, one login list and one state for the deployment. + +vi.mock('@/lib/admin/third-party-mode', () => ({ isNoThirdPartyTrafficOn: async () => false })); + +let disks: string; +let store: SeparateDiskStore; + +function onReplica(name: 'a' | 'b') { + vi.stubEnv('TELEMETRY_DATA_DIR', path.join(disks, name, 'telemetry')); + vi.stubEnv('VERSION_CHECK_DATA_DIR', path.join(disks, name, 'version-check')); +} + +beforeEach(() => { + disks = mkdtempSync(path.join(tmpdir(), 'bw-telemetry-replicas-')); + store = separateDiskStore(); + setStoreForTests(store); + vi.stubEnv('BULWARK_TELEMETRY', ''); + onReplica('a'); +}); + +afterEach(() => { + setStoreForTests(null); + resetLocalStoreForTests(); + vi.unstubAllEnvs(); + vi.unstubAllGlobals(); + store.cleanup(); + rmSync(disks, { recursive: true, force: true }); +}); + +describe('telemetry with a shared store', () => { + it('gives every replica one instance id, even when they ask at once', async () => { + const { getInstanceId } = await import('@/lib/telemetry/state'); + const [first, second] = await Promise.all([getInstanceId(), getInstanceId()]); + expect(second).toBe(first); + onReplica('b'); + expect(await getInstanceId()).toBe(first); + }); + + it('counts logins on every replica together and reports the same numbers everywhere', async () => { + // A fresh module copy per replica: nothing carried over in memory. + const tracker = async () => { + vi.resetModules(); + return import('@/lib/telemetry/login-tracker'); + }; + const a = await tracker(); + await Promise.all([ + a.recordLogin('alice@example.org', 'https://mail.example.org'), + a.recordLogin('bob@example.org', 'https://mail.example.org'), + ]); + onReplica('b'); + const b = await tracker(); + await b.recordLogin('carol@example.org', 'https://mail.example.org'); + expect(await b.getLoginCounts()).toEqual({ total: 3, active7d: 3 }); + expect(await a.getLoginCounts()).toEqual({ total: 3, active7d: 3 }); + }); + + it('shows on replica B the consent set on replica A', async () => { + const { updateState, effectiveConsent } = await import('@/lib/telemetry/state'); + await updateState((state) => { state.consent = 'on'; }); + onReplica('b'); + expect((await effectiveConsent()).consent).toBe('on'); + }); +}); + +function status(current: string, latest: string): UpdateStatus { + return { + schema: 1, current, latest, updateAvailable: current !== latest, severity: current !== latest ? 'normal' : 'none', + url: null, advisory: null, checkedAt: new Date().toISOString(), + }; +} + +describe('version check with a shared store', () => { + it('keeps each running version\'s status during a rolling update', async () => { + vi.stubEnv('BULWARK_UPDATE_CHECK_URL', 'https://version.example/'); + const answer = (current: string) => vi.stubGlobal('fetch', vi.fn(async () => new Response(JSON.stringify(status(current, '1.21.0'))))); + const { checkOnce, loadState, freshStatus } = await import('@/lib/version-check'); + + vi.stubEnv('NEXT_PUBLIC_APP_VERSION', '1.20.0'); + answer('1.20.0'); + expect((await checkOnce()).ok).toBe(true); + + onReplica('b'); + vi.stubEnv('NEXT_PUBLIC_APP_VERSION', '1.21.0'); + answer('1.21.0'); + expect((await checkOnce()).ok).toBe(true); + expect(freshStatus(await loadState())).toMatchObject({ current: '1.21.0', updateAvailable: false }); + + // A replica still on the old version keeps showing its banner. + onReplica('a'); + vi.stubEnv('NEXT_PUBLIC_APP_VERSION', '1.20.0'); + expect(freshStatus(await loadState())).toMatchObject({ current: '1.20.0', updateAvailable: true }); + }); +}); + +describe('telemetry and version check on a single instance', () => { + it('keep today\'s files', async () => { + setStoreForTests(null); + const { getInstanceId, loadState } = await import('@/lib/telemetry/state'); + const { recordLogin } = await import('@/lib/telemetry/login-tracker'); + const versionCheck = await import('@/lib/version-check'); + await getInstanceId(); + await loadState(); + await recordLogin('alice@example.org', 'https://mail.example.org'); + await versionCheck.saveState({ ...(await versionCheck.loadState()), lastCheckedAt: 'x' }); + expect(readdirSync(path.join(disks, 'a', 'telemetry')).sort()).toEqual(['.telemetry-id', 'logins.json', 'state.json']); + expect(readdirSync(path.join(disks, 'a', 'version-check'))).toEqual(['state.json']); + }); +}); diff --git a/lib/telemetry/index.ts b/lib/telemetry/index.ts index 1717c1bf7..2cc956d4c 100644 --- a/lib/telemetry/index.ts +++ b/lib/telemetry/index.ts @@ -1,7 +1,7 @@ export { startScheduler, stopScheduler, reschedule, sendOnce } from './sender'; export { buildPayload, markProcessStart } from './payload'; export { - loadState, saveState, getInstanceId, effectiveConsent, + loadState, saveState, updateState, getInstanceId, effectiveConsent, } from './state'; export { recordLogin, getLoginCounts } from './login-tracker'; export { diff --git a/lib/telemetry/login-tracker.ts b/lib/telemetry/login-tracker.ts index 3534ca365..96f776e81 100644 --- a/lib/telemetry/login-tracker.ts +++ b/lib/telemetry/login-tracker.ts @@ -1,9 +1,6 @@ -import { readFile, writeFile, mkdir, rename } from 'node:fs/promises'; -import { existsSync } from 'node:fs'; -import path from 'node:path'; import { createHmac } from 'node:crypto'; import { logger } from '@/lib/logger'; -import { getInstanceId } from './state'; +import { getInstanceId, telemetryDocs } from './state'; // We never store usernames or server URLs in the clear. Each login is // recorded as HMAC-SHA256(username + '@' + serverUrl, instance_id), so the @@ -22,39 +19,17 @@ interface LoginsFile { const SEVEN_DAYS_MS = 7 * 24 * 60 * 60 * 1000; const RETENTION_MS = 90 * 24 * 60 * 60 * 1000; -let cache: LoginsFile | null = null; +/** `logins.json` in the telemetry collection (#1066): one list for every replica. */ +const LOGINS_DOC = 'logins.json'; -function getDir(): string { - return process.env.TELEMETRY_DATA_DIR || path.join(process.cwd(), 'data', 'telemetry'); -} - -function loginsPath(): string { - return path.join(getDir(), 'logins.json'); -} - -async function ensureDir(): Promise { - const dir = getDir(); - if (!existsSync(dir)) await mkdir(dir, { recursive: true }); -} - -async function loadFile(): Promise { - if (cache) return cache; +function parse(data: Buffer | null): LoginsFile { + if (!data) return { records: [] }; try { - const raw = await readFile(loginsPath(), 'utf8'); - const parsed = JSON.parse(raw) as Partial; - cache = Array.isArray(parsed?.records) ? { records: parsed.records as LoginRecord[] } : { records: [] }; + const parsed = JSON.parse(data.toString('utf8')) as Partial; + return Array.isArray(parsed?.records) ? { records: parsed.records as LoginRecord[] } : { records: [] }; } catch { - cache = { records: [] }; + return { records: [] }; } - return cache; -} - -async function saveFile(file: LoginsFile): Promise { - await ensureDir(); - cache = file; - const tmp = loginsPath() + '.tmp'; - await writeFile(tmp, JSON.stringify(file), 'utf8'); - await rename(tmp, loginsPath()); } function normalizeServer(serverUrl: string): string { @@ -71,29 +46,31 @@ async function hashIdentity(username: string, serverUrl: string): Promise { if (!username || !serverUrl) return; try { const id = await hashIdentity(username, serverUrl); - const file = await loadFile(); const now = new Date().toISOString(); - const cutoff = Date.now() - RETENTION_MS; - const next: LoginRecord[] = []; - let updated = false; - for (const rec of file.records) { - const ts = new Date(rec.lastLoginAt).getTime(); - if (Number.isNaN(ts) || ts < cutoff) continue; - if (rec.id === id) { - next.push({ id, lastLoginAt: now }); - updated = true; - } else { - next.push(rec); + await telemetryDocs().update(LOGINS_DOC, (current) => { + const cutoff = Date.now() - RETENTION_MS; + const next: LoginRecord[] = []; + let updated = false; + for (const rec of parse(current).records) { + const ts = new Date(rec.lastLoginAt).getTime(); + if (Number.isNaN(ts) || ts < cutoff) continue; + if (rec.id === id) { + next.push({ id, lastLoginAt: now }); + updated = true; + } else { + next.push(rec); + } } - } - if (!updated) next.push({ id, lastLoginAt: now }); - await saveFile({ records: next }); + if (!updated) next.push({ id, lastLoginAt: now }); + return JSON.stringify({ records: next }); + }); } catch (err) { logger.debug?.('telemetry: recordLogin failed', { error: err instanceof Error ? err.message : String(err), @@ -103,11 +80,12 @@ export async function recordLogin(username: string, serverUrl: string): Promise< /** * Total distinct accounts seen in the 90-day retention window, plus those - * with a login in the last 7 days. + * with a login in the last 7 days. Read from the store each time, so every + * replica reports the same numbers. */ export async function getLoginCounts(): Promise<{ total: number; active7d: number }> { try { - const file = await loadFile(); + const file = parse((await telemetryDocs().get(LOGINS_DOC))?.data ?? null); const cutoff = Date.now() - RETENTION_MS; const sevenAgo = Date.now() - SEVEN_DAYS_MS; let total = 0; diff --git a/lib/telemetry/sender.ts b/lib/telemetry/sender.ts index f719f1ccc..f47b5a374 100644 --- a/lib/telemetry/sender.ts +++ b/lib/telemetry/sender.ts @@ -1,5 +1,5 @@ import { logger } from '@/lib/logger'; -import { effectiveConsent, endpointEnabled, loadState, saveState } from './state'; +import { effectiveConsent, endpointEnabled, loadState, updateState } from './state'; import { buildPayload } from './payload'; import { fetchTelemetryTarget, resolveEndpointAllowed } from './endpoint-guard'; import { DEFAULT_ENDPOINT } from './types'; @@ -49,9 +49,8 @@ export async function sendOnce(opts?: { reason?: string }): Promise<{ }); const ok = res.ok; if (ok) { - const next = await loadState(); - next.lastSentAt = new Date().toISOString(); - await saveState(next); + const at = new Date().toISOString(); + await updateState((next) => { next.lastSentAt = at; }); } logger.info('telemetry: heartbeat', { ok, status: res.status, reason: opts?.reason ?? 'scheduled', @@ -67,9 +66,7 @@ export async function sendOnce(opts?: { reason?: string }): Promise<{ async function scheduleNext(delayMs: number): Promise { if (currentTimer) clearTimeout(currentTimer); const at = new Date(Date.now() + delayMs).toISOString(); - const state = await loadState(); - state.nextScheduledAt = at; - await saveState(state); + await updateState((state) => { state.nextScheduledAt = at; }); currentTimer = setTimeout(() => { void tick(); }, delayMs); // Don't keep the process alive just for this. currentTimer.unref?.(); diff --git a/lib/telemetry/state.ts b/lib/telemetry/state.ts index cae5ccd96..cde8620b6 100644 --- a/lib/telemetry/state.ts +++ b/lib/telemetry/state.ts @@ -1,8 +1,8 @@ -import { readFile, writeFile, mkdir, rename } from 'node:fs/promises'; -import { existsSync } from 'node:fs'; import path from 'node:path'; import { randomUUID } from 'node:crypto'; import { logger } from '@/lib/logger'; +import { getStore, type CollectionOptions } from '@/lib/shared-store'; +import { isStoreUnavailable } from '@/lib/stored-data-errors'; import type { TelemetryStateFile, ConsentState } from './types'; import { DEFAULT_ENDPOINT } from './types'; import { isNoThirdPartyTrafficOn } from '@/lib/admin/third-party-mode'; @@ -12,8 +12,26 @@ export function getDir(): string { path.join(process.cwd(), 'data', 'telemetry'); } -function statePath(): string { return path.join(getDir(), 'state.json'); } -function idPath(): string { return path.join(getDir(), '.telemetry-id'); } +/** + * Telemetry's documents (#1066): `state.json`, `logins.json` and the + * instance id `.telemetry-id`. Local mode: today's files in the telemetry + * dir. Shared mode: one copy for the whole deployment, so every replica + * reports as the same instance with the same counts. + */ +export const TELEMETRY_COLLECTION: CollectionOptions = { + local: { + dir: () => getDir(), + idOf: (relative) => (['state.json', 'logins.json', '.telemetry-id'].includes(relative) ? relative : null), + }, +}; + +export const STATE_DOC = 'state.json'; +const ID_DOC = '.telemetry-id'; +const UUID = /^[0-9a-f-]{36}$/i; + +export function telemetryDocs() { + return getStore().collection('telemetry', TELEMETRY_COLLECTION); +} function envOverride(): ConsentState | null { const v = (process.env.BULWARK_TELEMETRY ?? '').toLowerCase(); @@ -26,21 +44,29 @@ function envOverride(): ConsentState | null { return null; } -export async function ensureDir(): Promise { - if (!existsSync(getDir())) await mkdir(getDir(), { recursive: true }); -} - +/** + * The instance id, created once: of several replicas (or module copies) + * asking at once, every one gets the id the first one stored. + */ export async function getInstanceId(): Promise { - await ensureDir(); - try { - const id = (await readFile(idPath(), 'utf8')).trim(); - if (/^[0-9a-f-]{36}$/i.test(id)) return id; - } catch { /* generate fresh */ } - const fresh = randomUUID(); - const tmp = idPath() + '.tmp'; - await writeFile(tmp, fresh, 'utf8'); - await rename(tmp, idPath()); - return fresh; + const docs = telemetryDocs(); + for (let attempt = 0; attempt < 3; attempt++) { + let current; + try { + current = await docs.get(ID_DOC); + } catch (error) { + if (isStoreUnavailable(error)) throw error; + current = null; // unreadable: make a fresh one, as before + } + const id = current?.data.toString('utf8').trim(); + if (id && UUID.test(id)) return id; + const fresh = randomUUID(); + const result = await docs.put(ID_DOC, fresh, { ifVersion: current ? current.version : null }); + if (result.ok) return fresh; + const won = result.current?.data.toString('utf8').trim(); + if (won && UUID.test(won)) return won; + } + throw new Error('telemetry: could not store an instance id'); } // Default consent is 'off' - telemetry is opt-in. Admins can enable it during @@ -56,36 +82,92 @@ const DEFAULTS: TelemetryStateFile = { nextScheduledAt: null, }; -export async function loadState(): Promise { - await ensureDir(); +function serialize(state: TelemetryStateFile): string { + return JSON.stringify(state, null, 2); +} + +function parse(data: Buffer): TelemetryStateFile { + return { ...DEFAULTS, ...(JSON.parse(data.toString('utf8')) as Partial) }; +} + +function readFailed(err: unknown): TelemetryStateFile { + // Unreadable is not missing: answer with the defaults (so nothing is + // sent on a guess) but leave the file alone, or a passing read error + // would switch the admin's consent off for good. + logger.warn('telemetry: state read failed', { + error: err instanceof Error ? err.message : String(err), + }); + return { ...DEFAULTS }; +} + +/** + * The state with its stored version (null when it couldn't be read), for a + * compare-and-set. Throws StoreUnavailableError when the shared store is down. + */ +export async function loadStateVersioned(): Promise<{ state: TelemetryStateFile; version: string | null }> { + const docs = telemetryDocs(); + let current; try { - const raw = await readFile(statePath(), 'utf8'); - const parsed = JSON.parse(raw) as Partial; - return { ...DEFAULTS, ...parsed }; + current = await docs.get(STATE_DOC); } catch (err) { - if ((err as NodeJS.ErrnoException).code !== 'ENOENT') { - // Unreadable is not missing: answer with the defaults (so nothing is - // sent on a guess) but leave the file alone, or a passing read error - // would switch the admin's consent off for good. - logger.warn('telemetry: state read failed', { - error: err instanceof Error ? err.message : String(err), - }); - return { ...DEFAULTS }; - } - // First-ever load on a fresh install: persist the default-off state so the - // instance id and (lack of) consent are stable across restarts. The admin - // can opt in later via the UI or the BULWARK_TELEMETRY env var. - const fresh: TelemetryStateFile = { ...DEFAULTS }; - await saveState(fresh); - return fresh; + if (isStoreUnavailable(err)) throw err; + return { state: readFailed(err), version: null }; } + if (current) { + try { + return { state: parse(current.data), version: current.version }; + } catch (err) { + return { state: readFailed(err), version: null }; + } + } + // First-ever load on a fresh install: persist the default-off state so the + // instance id and (lack of) consent are stable across restarts. The admin + // can opt in later via the UI or the BULWARK_TELEMETRY env var. + const fresh: TelemetryStateFile = { ...DEFAULTS }; + const written = await docs.put(STATE_DOC, serialize(fresh), { ifVersion: null }); + if (written.ok) return { state: fresh, version: written.version }; + // Another replica created it first. + return written.current + ? { state: parse(written.current.data), version: written.current.version } + : { state: fresh, version: null }; +} + +export async function loadState(): Promise { + return (await loadStateVersioned()).state; } export async function saveState(state: TelemetryStateFile): Promise { - await ensureDir(); - const tmp = statePath() + '.tmp'; - await writeFile(tmp, JSON.stringify(state, null, 2), 'utf8'); - await rename(tmp, statePath()); + await telemetryDocs().put(STATE_DOC, serialize(state)); +} + +/** + * Change some fields of the stored state without undoing a write that + * landed meanwhile (another replica scheduling, the admin changing consent). + * `change` may run more than once. Returns the state written. + */ +export async function updateState(change: (state: TelemetryStateFile) => void): Promise { + let next: TelemetryStateFile = { ...DEFAULTS }; + await telemetryDocs().update(STATE_DOC, (current) => { + let state: TelemetryStateFile; + try { + state = current ? parse(current) : { ...DEFAULTS }; + } catch { + state = { ...DEFAULTS }; // a corrupt file: this write replaces it + } + change(state); + next = state; + return serialize(state); + }); + return next; +} + +/** + * Write `state` only if the stored state is still at `version` (from + * loadStateVersioned). False when something else wrote first. + */ +export async function saveStateIfUnchanged(state: TelemetryStateFile, version: string | null): Promise { + if (version === null) return false; + return (await telemetryDocs().put(STATE_DOC, serialize(state), { ifVersion: version })).ok; } // Effective consent: the no-third-party mode wins over everything (#903), diff --git a/lib/version-check/index.ts b/lib/version-check/index.ts index efc5e213c..86d5d9a16 100644 --- a/lib/version-check/index.ts +++ b/lib/version-check/index.ts @@ -1,6 +1,6 @@ export { startScheduler, stopScheduler, checkOnce } from './sender'; export { - loadState, saveState, effectiveEndpoint, disabledByEnv, + loadState, saveState, updateState, effectiveEndpoint, disabledByEnv, getCurrentVersion, isStatusStale, freshStatus, } from './state'; export { fetchStatus, parseStatus } from './fetcher'; diff --git a/lib/version-check/sender.ts b/lib/version-check/sender.ts index ec05be376..34009c652 100644 --- a/lib/version-check/sender.ts +++ b/lib/version-check/sender.ts @@ -1,7 +1,9 @@ import { logger } from '@/lib/logger'; import { - disabledByEnv, effectiveEndpoint, getCurrentVersion, isStatusStale, loadState, saveState, + disabledByEnv, effectiveEndpoint, freshStatus, getCurrentVersion, isStatusStale, loadState, saveState, + updateState, withVersionStatus, } from './state'; +import { isSharedMode } from '@/lib/shared-store'; import { fetchStatus } from './fetcher'; import type { UpdateStatus } from './types'; import { isNoThirdPartyTrafficOn } from '@/lib/admin/third-party-mode'; @@ -39,13 +41,15 @@ export async function checkOnce(opts?: { reason?: string }): Promise<{ const now = new Date().toISOString(); const result = await fetchStatus(endpoint, current); - const next = await loadState(); - next.lastCheckedAt = now; - if (result.ok) { - next.lastSuccessAt = now; - next.status = result.status; - } - await saveState(next); + const shared = isSharedMode(); + await updateState((next) => { + next.lastCheckedAt = now; + if (result.ok) { + next.lastSuccessAt = now; + next.status = result.status; + if (shared) next.statuses = withVersionStatus(next.statuses, result.status); + } + }); logger.info('version-check: ran', { ok: result.ok, @@ -60,9 +64,7 @@ export async function checkOnce(opts?: { reason?: string }): Promise<{ async function scheduleNext(delayMs: number): Promise { if (currentTimer) clearTimeout(currentTimer); const at = new Date(Date.now() + delayMs).toISOString(); - const state = await loadState(); - state.nextScheduledAt = at; - await saveState(state); + await updateState((state) => { state.nextScheduledAt = at; }); currentTimer = setTimeout(() => { void tick(); }, delayMs); // Don't keep the process alive just for this. currentTimer.unref?.(); @@ -92,8 +94,12 @@ export async function startScheduler(): Promise { // version we no longer run, and its schedule would keep that stale answer // (e.g. "update available" for the version we just upgraded from) on screen // for up to an hour. Drop it and check promptly instead (#913). - const stale = isStatusStale(state.status); - if (stale) { + // With a shared store, replicas on another version may still be serving + // that status: keep it, and only check promptly when there is none for + // this version yet. + const shared = isSharedMode(); + const stale = shared ? !freshStatus(state) && state.status !== null : isStatusStale(state.status); + if (stale && !shared) { logger.info('version-check: discarding status from a previous version', { cached: state.status?.current ?? null, running: getCurrentVersion(), diff --git a/lib/version-check/state.ts b/lib/version-check/state.ts index 1797cb3e9..0bd5c19b6 100644 --- a/lib/version-check/state.ts +++ b/lib/version-check/state.ts @@ -1,7 +1,7 @@ -import { readFile, writeFile, mkdir, rename } from 'node:fs/promises'; -import { existsSync } from 'node:fs'; import path from 'node:path'; import { logger } from '@/lib/logger'; +import { getStore, type CollectionOptions } from '@/lib/shared-store'; +import { isStoreUnavailable } from '@/lib/stored-data-errors'; import type { UpdateStatus, VersionCheckStateFile } from './types'; import { DEFAULT_VERSION_ENDPOINT } from './types'; @@ -10,7 +10,23 @@ function getDir(): string { path.join(process.cwd(), 'data', 'version-check'); } -function statePath(): string { return path.join(getDir(), 'state.json'); } +/** + * `state.json` (#1066). Local mode: today's file in the version-check dir. + * Shared mode: one state for the deployment, written by the replica that + * runs the scheduled check and read by all. + */ +export const VERSION_CHECK_COLLECTION: CollectionOptions = { + local: { + dir: () => getDir(), + idOf: (relative) => (relative === 'state.json' ? relative : null), + }, +}; + +export const STATE_DOC = 'state.json'; + +function docs() { + return getStore().collection('version-check', VERSION_CHECK_COLLECTION); +} const DEFAULTS: VersionCheckStateFile = { endpoint: DEFAULT_VERSION_ENDPOINT, @@ -20,31 +36,44 @@ const DEFAULTS: VersionCheckStateFile = { status: null, }; -export async function ensureDir(): Promise { - if (!existsSync(getDir())) await mkdir(getDir(), { recursive: true }); +function parse(data: Buffer): VersionCheckStateFile { + return { ...DEFAULTS, ...(JSON.parse(data.toString('utf8')) as Partial) }; } +function serialize(state: VersionCheckStateFile): string { + return JSON.stringify(state, null, 2); +} + +/** The stored state, or the defaults. Throws StoreUnavailableError when the shared store is down. */ export async function loadState(): Promise { - await ensureDir(); try { - const raw = await readFile(statePath(), 'utf8'); - const parsed = JSON.parse(raw) as Partial; - return { ...DEFAULTS, ...parsed }; + const current = await docs().get(STATE_DOC); + return current ? parse(current.data) : { ...DEFAULTS }; } catch (err) { - if ((err as NodeJS.ErrnoException).code !== 'ENOENT') { - logger.warn('version-check: state read failed', { - error: err instanceof Error ? err.message : String(err), - }); - } + if (isStoreUnavailable(err)) throw err; + logger.warn('version-check: state read failed', { + error: err instanceof Error ? err.message : String(err), + }); return { ...DEFAULTS }; } } export async function saveState(state: VersionCheckStateFile): Promise { - await ensureDir(); - const tmp = statePath() + '.tmp'; - await writeFile(tmp, JSON.stringify(state, null, 2), 'utf8'); - await rename(tmp, statePath()); + await docs().put(STATE_DOC, serialize(state)); +} + +/** Change some fields against what is stored now. `change` may run more than once. */ +export async function updateState(change: (state: VersionCheckStateFile) => void): Promise { + await docs().update(STATE_DOC, (current) => { + let state: VersionCheckStateFile; + try { + state = current ? parse(current) : { ...DEFAULTS }; + } catch { + state = { ...DEFAULTS }; + } + change(state); + return serialize(state); + }); } export function disabledByEnv(): boolean { @@ -78,6 +107,29 @@ export function isStatusStale( return status.current !== current; } +/** How many running versions keep their own status (a rolling update runs two). */ +const KEPT_VERSIONS = 4; + +/** + * The status for the version this replica runs. With a shared store, + * replicas on two versions during a rolling update share one state; each + * reads the status checked for its own version from `statuses`, so the + * banner doesn't come and go as the other version's checks land. + */ export function freshStatus(state: VersionCheckStateFile): UpdateStatus | null { - return isStatusStale(state.status) ? null : state.status; + if (!isStatusStale(state.status)) return state.status; + const own = state.statuses?.[getCurrentVersion()]; + return own && !isStatusStale(own) ? own : null; +} + +/** `statuses` with `status` added for its version, keeping the newest few. */ +export function withVersionStatus( + statuses: Record | undefined, + status: UpdateStatus, +): Record { + const next = { ...(statuses ?? {}), [status.current]: status }; + const newest = Object.entries(next) + .sort(([, a], [, b]) => b.checkedAt.localeCompare(a.checkedAt)) + .slice(0, KEPT_VERSIONS); + return Object.fromEntries(newest); } diff --git a/lib/version-check/types.ts b/lib/version-check/types.ts index b624626cc..bf70e69d8 100644 --- a/lib/version-check/types.ts +++ b/lib/version-check/types.ts @@ -20,6 +20,11 @@ export interface VersionCheckStateFile { lastSuccessAt: string | null; nextScheduledAt: string | null; status: UpdateStatus | null; + /** + * With a shared store only: the last status per running version, so + * replicas on two versions during a rolling update each keep their own. + */ + statuses?: Record; } export const DEFAULT_VERSION_ENDPOINT = 'https://version.telemetry.bulwarkmail.org/'; From fd1d90dc3ad41e52c13f3b2ff2814000ff6e13ba Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Sat, 31 Oct 2026 23:11:48 +0100 Subject: [PATCH 08/31] fix: stop the old telemetry timer when an admin changes consent, so one heartbeat runs #1066 --- .../scheduler-timer-module-copies.test.ts | 62 +++++++++++++++++++ lib/telemetry/sender.ts | 22 +++++-- lib/version-check/sender.ts | 22 +++++-- 3 files changed, 94 insertions(+), 12 deletions(-) create mode 100644 lib/__tests__/scheduler-timer-module-copies.test.ts diff --git a/lib/__tests__/scheduler-timer-module-copies.test.ts b/lib/__tests__/scheduler-timer-module-copies.test.ts new file mode 100644 index 000000000..8beb553ee --- /dev/null +++ b/lib/__tests__/scheduler-timer-module-copies.test.ts @@ -0,0 +1,62 @@ +// @vitest-environment node +import { mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import path from 'node:path'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +// Next loads lib/telemetry and lib/version-check once per bundle. The timer +// sat in a module-level variable, so the admin route's reschedule() acted on +// its own copy: the timer instrumentation started kept running next to the +// new one (two heartbeats), and turning consent off didn't stop it. + +vi.mock('@/lib/admin/third-party-mode', () => ({ isNoThirdPartyTrafficOn: async () => false })); + +let dir: string; + +beforeEach(() => { + dir = mkdtempSync(path.join(tmpdir(), 'bw-scheduler-copies-')); + vi.stubEnv('TELEMETRY_DATA_DIR', path.join(dir, 'telemetry')); + vi.stubEnv('VERSION_CHECK_DATA_DIR', path.join(dir, 'version-check')); + vi.stubEnv('BULWARK_TELEMETRY', 'on'); + vi.stubEnv('BULWARK_UPDATE_CHECK', ''); + vi.stubEnv('BULWARK_UPDATE_CHECK_URL', 'https://version.example/'); + vi.useFakeTimers({ toFake: ['setTimeout', 'clearTimeout'] }); +}); + +afterEach(() => { + vi.useRealTimers(); + vi.unstubAllEnvs(); + vi.resetModules(); + rmSync(dir, { recursive: true, force: true }); +}); + +async function copy(load: () => Promise): Promise { + vi.resetModules(); + return load(); +} + +describe('scheduler timers across module copies', () => { + it('telemetry: a stop from another copy clears the running timer, a restart leaves one', async () => { + const instrumentation = await copy(() => import('@/lib/telemetry/sender')); + const route = await copy(() => import('@/lib/telemetry/sender')); + + await instrumentation.startScheduler(); + expect(vi.getTimerCount()).toBe(1); + await route.startScheduler(); + expect(vi.getTimerCount()).toBe(1); + await route.stopScheduler(); + expect(vi.getTimerCount()).toBe(0); + }); + + it('version check: the same', async () => { + const instrumentation = await copy(() => import('@/lib/version-check/sender')); + const route = await copy(() => import('@/lib/version-check/sender')); + + await instrumentation.startScheduler(); + expect(vi.getTimerCount()).toBe(1); + await route.startScheduler(); + expect(vi.getTimerCount()).toBe(1); + await route.stopScheduler(); + expect(vi.getTimerCount()).toBe(0); + }); +}); diff --git a/lib/telemetry/sender.ts b/lib/telemetry/sender.ts index f47b5a374..a06386565 100644 --- a/lib/telemetry/sender.ts +++ b/lib/telemetry/sender.ts @@ -8,7 +8,14 @@ const DAY_MS = 24 * 60 * 60 * 1000; const JITTER_MS = 2 * 60 * 60 * 1000; // ± 2 hours const FIRST_DELAY_MS = 60 * 60 * 1000; // 1 hour after consent -let currentTimer: NodeJS.Timeout | null = null; +// On globalThis: Next loads this module once per bundle (instrumentation, +// each route), and a reschedule from a route must clear the timer the +// instrumentation copy started, not a copy of its own. +const TIMER_KEY = Symbol.for('bulwark.telemetry.timer'); +function timer(): { current: NodeJS.Timeout | null } { + const g = globalThis as typeof globalThis & { [TIMER_KEY]?: { current: NodeJS.Timeout | null } }; + return (g[TIMER_KEY] ??= { current: null }); +} function jitteredDelay(base: number): number { const j = (Math.random() * 2 - 1) * JITTER_MS; @@ -64,12 +71,14 @@ export async function sendOnce(opts?: { reason?: string }): Promise<{ } async function scheduleNext(delayMs: number): Promise { - if (currentTimer) clearTimeout(currentTimer); + const t = timer(); + if (t.current) clearTimeout(t.current); const at = new Date(Date.now() + delayMs).toISOString(); await updateState((state) => { state.nextScheduledAt = at; }); - currentTimer = setTimeout(() => { void tick(); }, delayMs); + if (t.current) clearTimeout(t.current); + t.current = setTimeout(() => { void tick(); }, delayMs); // Don't keep the process alive just for this. - currentTimer.unref?.(); + t.current.unref?.(); } async function tick(): Promise { @@ -104,8 +113,9 @@ export async function startScheduler(): Promise { } export async function stopScheduler(): Promise { - if (currentTimer) clearTimeout(currentTimer); - currentTimer = null; + const t = timer(); + if (t.current) clearTimeout(t.current); + t.current = null; } // Called when consent flips on/off or the collector URL changes via the UI. diff --git a/lib/version-check/sender.ts b/lib/version-check/sender.ts index 34009c652..7904dad93 100644 --- a/lib/version-check/sender.ts +++ b/lib/version-check/sender.ts @@ -13,7 +13,14 @@ const JITTER_MS = 5 * 60 * 1000; // ± 5 min, keeps containers from syncin const FIRST_DELAY_MS = 30 * 1000; // first check ~30s after boot const FAILURE_BACKOFF_MS = 15 * 60 * 1000; // after a failed fetch, retry in 15 min -let currentTimer: NodeJS.Timeout | null = null; +// On globalThis: Next loads this module once per bundle (instrumentation, +// each route), and a reschedule from a route must clear the timer the +// instrumentation copy started, not a copy of its own. +const TIMER_KEY = Symbol.for('bulwark.versionCheck.timer'); +function timer(): { current: NodeJS.Timeout | null } { + const g = globalThis as typeof globalThis & { [TIMER_KEY]?: { current: NodeJS.Timeout | null } }; + return (g[TIMER_KEY] ??= { current: null }); +} /** What checkOnce reports while NO_THIRD_PARTY_TRAFFIC is on. */ export const NO_THIRD_PARTY_ERROR = 'off: no third-party traffic'; @@ -62,12 +69,14 @@ export async function checkOnce(opts?: { reason?: string }): Promise<{ } async function scheduleNext(delayMs: number): Promise { - if (currentTimer) clearTimeout(currentTimer); + const t = timer(); + if (t.current) clearTimeout(t.current); const at = new Date(Date.now() + delayMs).toISOString(); await updateState((state) => { state.nextScheduledAt = at; }); - currentTimer = setTimeout(() => { void tick(); }, delayMs); + if (t.current) clearTimeout(t.current); + t.current = setTimeout(() => { void tick(); }, delayMs); // Don't keep the process alive just for this. - currentTimer.unref?.(); + t.current.unref?.(); } async function tick(): Promise { @@ -122,6 +131,7 @@ export async function startScheduler(): Promise { } export async function stopScheduler(): Promise { - if (currentTimer) clearTimeout(currentTimer); - currentTimer = null; + const t = timer(); + if (t.current) clearTimeout(t.current); + t.current = null; } From a0af628a6a66d1bdc87fac4100824a4bd876b84d Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Sat, 31 Oct 2026 23:25:12 +0100 Subject: [PATCH 09/31] feat: run the telemetry heartbeat and update check on one instance of the deployment #1066 --- instrumentation.node.ts | 15 +- lib/__tests__/scheduler-leader.test.ts | 236 +++++++++++++++++++++++++ lib/scheduler-leader.ts | 176 ++++++++++++++++++ lib/telemetry/sender.ts | 83 +++++++-- lib/version-check/sender.ts | 16 +- 5 files changed, 505 insertions(+), 21 deletions(-) create mode 100644 lib/__tests__/scheduler-leader.test.ts create mode 100644 lib/scheduler-leader.ts diff --git a/instrumentation.node.ts b/instrumentation.node.ts index f73112096..3d5d5d4b5 100644 --- a/instrumentation.node.ts +++ b/instrumentation.node.ts @@ -80,16 +80,19 @@ migrateLegacyAdminLayout() // or with BULWARK_TELEMETRY=on; BULWARK_TELEMETRY=off or an empty // collector URL keeps it from sending. // See https://bulwarkmail.org/docs/legal/privacy/telemetry - const { startScheduler, markProcessStart } = await import("./lib/telemetry"); + const { startScheduler, stopScheduler, markProcessStart } = await import("./lib/telemetry"); markProcessStart(); - await startScheduler(); - }) - .then(async () => { // Hourly check against version.telemetry.bulwarkmail.org. Disable with // BULWARK_UPDATE_CHECK=off or override the endpoint with // BULWARK_UPDATE_CHECK_URL. - const { startScheduler } = await import("./lib/version-check"); - await startScheduler(); + const versionCheck = await import("./lib/version-check"); + // Both run on one replica of the deployment, the scheduler leader + // (#1066); a single instance always leads. + const { startSchedulerLeader } = await import("./lib/scheduler-leader"); + await startSchedulerLeader([ + { name: "telemetry", start: startScheduler, stop: stopScheduler }, + { name: "version-check", start: versionCheck.startScheduler, stop: versionCheck.stopScheduler }, + ]); }) .catch((err) => { console.warn("Admin dashboard init skipped:", err instanceof Error ? err.message : err); diff --git a/lib/__tests__/scheduler-leader.test.ts b/lib/__tests__/scheduler-leader.test.ts new file mode 100644 index 000000000..89641891b --- /dev/null +++ b/lib/__tests__/scheduler-leader.test.ts @@ -0,0 +1,236 @@ +// @vitest-environment node +import { mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import path from 'node:path'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { setTimeout as realSleep } from 'node:timers/promises'; +import { NextRequest } from 'next/server'; +import { replicaId, setStoreForTests } from '@/lib/shared-store'; +import { resetLocalStoreForTests } from '@/lib/shared-store/local'; +import { INIT_EPOCH, pollChanges, resetChangesForTests } from '@/lib/shared-store/changes'; +import { separateDiskStore, type SeparateDiskStore } from './helpers/separate-disk-store'; + +// The background jobs with several replicas (#1066): only the holder of the +// scheduler-leader lease runs them, another replica takes over when the +// holder's lease runs out and keeps the stored schedule, and a scheduled +// heartbeat is claimed with a compare-and-set so two leaders can't both send. + +const log = vi.hoisted(() => ({ info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() })); +vi.mock('@/lib/logger', () => ({ logger: log })); +vi.mock('@/lib/admin/third-party-mode', () => ({ isNoThirdPartyTrafficOn: async () => false })); +vi.mock('@/lib/telemetry/payload', () => ({ buildPayload: async () => ({ schema: '1' }), markProcessStart: () => {} })); +const sent = vi.hoisted(() => vi.fn(async (_url: string, _init?: unknown) => new Response('ok'))); +vi.mock('@/lib/telemetry/endpoint-guard', () => ({ + resolveEndpointAllowed: async () => ({ ok: true }), + fetchTelemetryTarget: (url: string, init?: unknown) => sent(url, init), +})); +vi.mock('@/lib/admin/session', () => ({ + requireAdminAuth: async () => ({ session: { sub: 'admin' } }), + getClientIP: () => '127.0.0.1', +})); +vi.mock('@/lib/admin/audit', () => ({ auditLog: async () => {} })); + +import { + LEADER_LEASE, isSchedulerLeader, resetSchedulerLeaderForTests, schedulerLeaderSettled, startSchedulerLeader, stopSchedulerLeader, +} from '@/lib/scheduler-leader'; +import { loadState, saveState, sendOnce, startScheduler, stopScheduler, updateState } from '@/lib/telemetry'; +import { POST as telemetryRoute } from '@/app/api/admin/telemetry/route'; + +const MIN = 60_000; +const COLLECTOR = 'https://collector.example.org/v1/heartbeat'; +const jobs = [{ name: 'telemetry', start: startScheduler, stop: stopScheduler }]; + +let dir: string; +let store: SeparateDiskStore; + +beforeEach(() => { + dir = mkdtempSync(path.join(tmpdir(), 'bw-leader-')); + vi.stubEnv('TELEMETRY_DATA_DIR', path.join(dir, 'telemetry')); + vi.stubEnv('BULWARK_TELEMETRY', ''); + vi.useFakeTimers({ toFake: ['setTimeout', 'clearTimeout', 'setInterval', 'clearInterval', 'Date'] }); + vi.setSystemTime(new Date('2026-10-31T12:00:00.000Z')); + store = separateDiskStore(); + setStoreForTests(store); + sent.mockClear(); + log.info.mockClear(); +}); + +afterEach(async () => { + await stopSchedulerLeader(); + resetSchedulerLeaderForTests(); + await stopScheduler(); + resetChangesForTests(); + vi.useRealTimers(); + setStoreForTests(null); + resetLocalStoreForTests(); + vi.unstubAllEnvs(); + store.cleanup(); + rmSync(dir, { recursive: true, force: true }); +}); + +async function consentOn(nextScheduledAt: string | null) { + await saveState({ ...(await loadState()), consent: 'on', endpoint: COLLECTOR, nextScheduledAt }); +} + +/** Let time pass; the leader's tries and job starts do real file I/O, so wait for them too. */ +async function pass(ms: number) { + for (let passed = 0; passed < ms; passed += 10_000) { + await vi.advanceTimersByTimeAsync(Math.min(10_000, ms - passed)); + await schedulerLeaderSettled(); + } +} + +/** Another replica holding (and renewing) the lease while `ms` pass here. */ +async function otherReplicaLeads(ms: number) { + for (let passed = 0; passed < ms; passed += 10_000) { + expect(await store.leases.acquire(LEADER_LEASE, 'replica-a', 30_000)).toBe(true); + await pass(10_000); + } +} + +/** Let a heartbeat a timer set off finish its real file I/O, so "not sent" means it. */ +async function ioSettles() { + await realSleep(200); +} + +/** The heartbeat the timer set off does real I/O before it sends. */ +async function sends(times: number) { + await vi.waitFor(() => expect(sent).toHaveBeenCalledTimes(times)); +} + +describe('scheduler leader with a shared store', () => { + it('only the holder sends; when its lease runs out another replica takes over and keeps the schedule', async () => { + const slot = new Date(Date.now() + 20 * MIN).toISOString(); + await consentOn(slot); + await store.leases.acquire(LEADER_LEASE, 'replica-a', 30_000); + + await startSchedulerLeader(jobs); + expect(isSchedulerLeader()).toBe(false); + await otherReplicaLeads(5 * MIN); + await ioSettles(); + expect(isSchedulerLeader()).toBe(false); + + // replica-a is gone: its lease expires after 30 s, the next try takes it. + await pass(40_000); + expect(isSchedulerLeader()).toBe(true); + expect(log.info).toHaveBeenCalledWith('scheduler: leading', expect.objectContaining({ replica: expect.any(String) })); + expect((await loadState()).nextScheduledAt).toBe(slot); + expect(sent).not.toHaveBeenCalled(); + + await vi.advanceTimersByTimeAsync(new Date(slot).getTime() - Date.now() + 1000); + await sends(1); + }); + + it('stops its timers when it can no longer reach the store', async () => { + const slot = new Date(Date.now() + 5 * MIN).toISOString(); + await consentOn(slot); + await startSchedulerLeader(jobs); + expect(isSchedulerLeader()).toBe(true); + + store.down = true; + await pass(10_000); + expect(isSchedulerLeader()).toBe(false); + expect(log.info).toHaveBeenCalledWith('scheduler: stopped leading', expect.objectContaining({ reason: expect.stringContaining('shared store') })); + // Past the slot with the store still down: no heartbeat from here. + await pass(6 * MIN); + await ioSettles(); + expect(sent).not.toHaveBeenCalled(); + expect(isSchedulerLeader()).toBe(false); + }); + + it('stops its timers when another replica has taken the lease', async () => { + const slot = new Date(Date.now() + 5 * MIN).toISOString(); + await consentOn(slot); + await startSchedulerLeader(jobs); + expect(isSchedulerLeader()).toBe(true); + + // This replica's lease is gone (it stalled past the TTL) and replica-a took it. + await store.leases.release(LEADER_LEASE, replicaId()); + await otherReplicaLeads(6 * MIN); + await ioSettles(); + expect(isSchedulerLeader()).toBe(false); + expect(sent).not.toHaveBeenCalled(); + }); + + it('sends once when two leaders fire for the same slot, and not again after it', async () => { + const slot = new Date(Date.now() - 1000).toISOString(); + await consentOn(slot); + + const results = await Promise.all([ + sendOnce({ reason: 'scheduled', slot }), + sendOnce({ reason: 'scheduled', slot }), + ]); + expect(results.filter((r) => r.ok)).toHaveLength(1); + expect(sent).toHaveBeenCalledTimes(1); + + // The paused leader wakes up an hour later, still holding that slot. + vi.advanceTimersByTime(60 * MIN); + expect((await sendOnce({ reason: 'scheduled', slot })).ok).toBe(false); + expect(sent).toHaveBeenCalledTimes(1); + }); + + it('gives the slot back when the send fails', async () => { + const slot = new Date(Date.now() - 1000).toISOString(); + await consentOn(slot); + sent.mockResolvedValueOnce(new Response('no', { status: 500 })); + expect((await sendOnce({ reason: 'scheduled', slot })).ok).toBe(false); + expect((await loadState()).lastSentAt).toBeNull(); + expect((await sendOnce({ reason: 'scheduled', slot })).ok).toBe(true); + }); + + it('sends on "send now" from a replica that does not lead, even right after a heartbeat', async () => { + await consentOn(null); + await store.leases.acquire(LEADER_LEASE, 'replica-a', 30_000); + await startSchedulerLeader(jobs); + expect(isSchedulerLeader()).toBe(false); + await updateState((state) => { state.lastSentAt = new Date().toISOString(); }); + + const res = await telemetryRoute(new NextRequest('http://localhost/api/admin/telemetry', { + method: 'POST', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify({ action: 'send-now' }), + })); + expect(res.status).toBe(200); + expect(sent).toHaveBeenCalledTimes(1); + }); + + it('starts the heartbeat on the leader when consent is turned on through another replica', async () => { + await store.epochs.bump(INIT_EPOCH); + await startSchedulerLeader(jobs); + expect(isSchedulerLeader()).toBe(true); + expect((await loadState()).nextScheduledAt).toBeNull(); + + // The other replica's admin route: write, then bump the epoch. + await updateState((state) => { state.consent = 'on'; state.endpoint = COLLECTOR; }); + await store.epochs.bump('telemetry'); + await pollChanges(); + + expect((await loadState()).nextScheduledAt).not.toBeNull(); + await vi.advanceTimersByTimeAsync(61 * MIN); + await sends(1); + }); +}); + +describe('scheduler leader on a single instance', () => { + it('leads at once and restarts the heartbeat when consent changes in this process', async () => { + setStoreForTests(null); + await startSchedulerLeader(jobs); + expect(isSchedulerLeader()).toBe(true); + expect(log.info).toHaveBeenCalledWith('scheduler: leading', expect.anything()); + + const res = await telemetryRoute(new NextRequest('http://localhost/api/admin/telemetry', { + method: 'POST', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify({ action: 'set-consent', consent: 'on' }), + })); + expect(res.status).toBe(200); + expect(vi.getTimerCount()).toBe(1); + + await telemetryRoute(new NextRequest('http://localhost/api/admin/telemetry', { + method: 'POST', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify({ action: 'set-consent', consent: 'off' }), + })); + expect(vi.getTimerCount()).toBe(0); + }); +}); diff --git a/lib/scheduler-leader.ts b/lib/scheduler-leader.ts new file mode 100644 index 000000000..ebbd10f56 --- /dev/null +++ b/lib/scheduler-leader.ts @@ -0,0 +1,176 @@ +import { logger } from '@/lib/logger'; +import { getStore, isSharedMode, replicaId } from '@/lib/shared-store'; +import { onSharedChange } from '@/lib/shared-store/changes'; + +/** + * One replica runs the background jobs (telemetry heartbeat, update check) + * for the whole deployment (#1066). It holds the `scheduler-leader` lease, + * renewing it every 10 s; every other replica tries to take it every 10 s, + * so when the leader dies or loses the store another one takes over within + * the 30 s lease plus one try. A leader that can't renew stops its jobs at + * once. The jobs keep their schedule in the shared state, so a new leader + * continues it instead of starting over. + * + * Local mode: the one process is always the leader. + * + * Everything lives on globalThis: instrumentation starts it, and the + * `telemetry` change listener may be run from a route's module copy. + */ + +export const LEADER_LEASE = 'scheduler-leader'; +export const LEADER_TTL_MS = 30_000; +export const LEADER_TRY_MS = 10_000; + +export interface ScheduledJob { + name: string; + /** Start the job's timer from its stored schedule. Must be idempotent. */ + start(): Promise; + stop(): Promise; +} + +interface LeaderGlobals { + jobs: ScheduledJob[]; + /** Jobs whose start went through while leading; the others are retried on each renewal. */ + started: Set; + leading: boolean; + interval?: NodeJS.Timeout; + trying?: Promise; + unlisten?: () => void; +} + +const KEY = Symbol.for('bulwark.schedulerLeader'); + +function globals(): LeaderGlobals { + const g = globalThis as typeof globalThis & { [KEY]?: LeaderGlobals }; + return (g[KEY] ??= { jobs: [], started: new Set(), leading: false }); +} + +function message(error: unknown): string { + return error instanceof Error ? error.message : String(error); +} + +/** True while this process runs the background jobs. */ +export function isSchedulerLeader(): boolean { + return globals().leading; +} + +async function startJobs(g: LeaderGlobals): Promise { + for (const job of g.jobs) { + if (g.started.has(job.name)) continue; + try { + await job.start(); + if (g.leading) g.started.add(job.name); + } catch (error) { + logger.warn('scheduler: could not start a job; retrying', { job: job.name, error: message(error) }); + } + } +} + +async function stopJobs(g: LeaderGlobals): Promise { + g.started.clear(); + for (const job of g.jobs) { + try { + await job.stop(); + } catch (error) { + logger.warn('scheduler: could not stop a job', { job: job.name, error: message(error) }); + } + } +} + +async function becomeLeader(g: LeaderGlobals): Promise { + g.leading = true; + logger.info('scheduler: leading', { replica: replicaId() }); + await startJobs(g); +} + +async function stepDown(g: LeaderGlobals, reason: string): Promise { + g.leading = false; + logger.info('scheduler: stopped leading', { replica: replicaId(), reason }); + await stopJobs(g); +} + +/** + * One try: take or renew the lease, start the jobs on taking it, stop them + * on losing it (or on not reaching the store). Returns whether this replica + * leads afterwards. + */ +export async function tryLead(): Promise { + const g = globals(); + if (g.trying) return g.trying; + g.trying = (async () => { + let holds: boolean; + let reason = 'another replica holds the lease'; + try { + holds = await getStore().leases.acquire(LEADER_LEASE, replicaId(), LEADER_TTL_MS); + } catch (error) { + holds = false; + reason = `could not reach the shared store: ${message(error)}`; + } + if (holds && !g.leading) await becomeLeader(g); + else if (holds) await startJobs(g); + else if (g.leading) await stepDown(g, reason); + return g.leading; + })().finally(() => { + g.trying = undefined; + }); + return g.trying; +} + +/** Restart one job on the leader (its settings changed), keeping its stored schedule. */ +async function restartJob(name: string): Promise { + const g = globals(); + if (!g.leading) return; + const job = g.jobs.find((j) => j.name === name); + if (!job) return; + g.started.delete(name); + await job.stop(); + await job.start(); + if (g.leading) g.started.add(name); +} + +/** + * Start leading or trying to (instrumentation, once per process). In shared + * mode every replica calls it; only the lease holder runs `jobs`. + */ +export async function startSchedulerLeader(jobs: ScheduledJob[]): Promise { + const g = globals(); + g.jobs = jobs; + // Consent or the collector changed on some replica: the leader picks it up. + g.unlisten ??= onSharedChange('telemetry', () => restartJob('telemetry')); + if (!isSharedMode()) { + if (!g.leading) await becomeLeader(g); + return; + } + if (!g.interval) { + g.interval = setInterval(() => void tryLead(), LEADER_TRY_MS); + g.interval.unref(); + } + await tryLead(); +} + +/** Stop the jobs and give the lease up (shutdown, tests). */ +export async function stopSchedulerLeader(): Promise { + const g = globals(); + if (g.interval) clearInterval(g.interval); + g.interval = undefined; + g.unlisten?.(); + g.unlisten = undefined; + if (g.leading) { + if (isSharedMode()) await getStore().leases.release(LEADER_LEASE, replicaId()).catch(() => {}); + await stepDown(g, 'stopped'); + } +} + +/** Resolves when the current try (and the job starts it set off) is done (tests). */ +export function schedulerLeaderSettled(): Promise { + return globals().trying ?? Promise.resolve(); +} + +/** Forget everything (tests). */ +export function resetSchedulerLeaderForTests(): void { + const g = globals(); + if (g.interval) clearInterval(g.interval); + g.unlisten?.(); + const holder = globalThis as typeof globalThis & { [KEY]?: LeaderGlobals }; + delete holder[KEY]; +} diff --git a/lib/telemetry/sender.ts b/lib/telemetry/sender.ts index a06386565..fe7bcd5ec 100644 --- a/lib/telemetry/sender.ts +++ b/lib/telemetry/sender.ts @@ -1,5 +1,8 @@ import { logger } from '@/lib/logger'; -import { effectiveConsent, endpointEnabled, loadState, updateState } from './state'; +import { announceChange } from '@/lib/shared-store/changes'; +import { + effectiveConsent, endpointEnabled, loadState, loadStateVersioned, saveStateIfUnchanged, updateState, +} from './state'; import { buildPayload } from './payload'; import { fetchTelemetryTarget, resolveEndpointAllowed } from './endpoint-guard'; import { DEFAULT_ENDPOINT } from './types'; @@ -7,6 +10,8 @@ import { DEFAULT_ENDPOINT } from './types'; const DAY_MS = 24 * 60 * 60 * 1000; const JITTER_MS = 2 * 60 * 60 * 1000; // ± 2 hours const FIRST_DELAY_MS = 60 * 60 * 1000; // 1 hour after consent +/** A send this long before a scheduled slot already counts for that slot. */ +const SLOT_SLACK_MS = 60 * 1000; // On globalThis: Next loads this module once per bundle (instrumentation, // each route), and a reschedule from a route must clear the timer the @@ -22,7 +27,42 @@ function jitteredDelay(base: number): number { return Math.max(60_000, base + j); } -export async function sendOnce(opts?: { reason?: string }): Promise<{ +type Claim = { claimed: true; previous: string | null; at: string } | { claimed: false }; + +/** + * Take the scheduled send for `slot` (its nextScheduledAt): set lastSentAt + * with a compare-and-set on the state, unless a send at or after the slot is + * already recorded. Of two leaders firing for one slot (a handover after a + * long pause), only one gets it. + */ +async function claimSlot(slot: string): Promise { + const slotMs = new Date(slot).getTime(); + for (let attempt = 0; attempt < 5; attempt++) { + const { state, version } = await loadStateVersioned(); + if (version === null) return { claimed: false }; + const last = state.lastSentAt ? new Date(state.lastSentAt).getTime() : NaN; + if (!Number.isNaN(last) && last >= slotMs - SLOT_SLACK_MS) return { claimed: false }; + const at = new Date().toISOString(); + if (await saveStateIfUnchanged({ ...state, lastSentAt: at }, version)) { + return { claimed: true, previous: state.lastSentAt, at }; + } + } + return { claimed: false }; +} + +/** Give a claimed slot back after a failed send, unless another send landed meanwhile. */ +async function releaseSlot(claim: Extract): Promise { + await updateState((state) => { + if (state.lastSentAt === claim.at) state.lastSentAt = claim.previous; + }).catch(() => {}); +} + +/** + * Send one heartbeat. A scheduled send passes its `slot` and is skipped + * when another replica already sent for it; the admin's "send now" passes + * none and always sends. + */ +export async function sendOnce(opts?: { reason?: string; slot?: string }): Promise<{ ok: boolean; status?: number; error?: string; @@ -44,6 +84,15 @@ export async function sendOnce(opts?: { reason?: string }): Promise<{ return { ok: false, error: `endpoint blocked: ${guard.reason}` }; } + let claim: Claim | null = null; + if (opts?.slot) { + claim = await claimSlot(opts.slot); + if (!claim.claimed) { + logger.info('telemetry: heartbeat skipped, already sent for this slot', { slot: opts.slot }); + return { ok: false, error: 'already sent' }; + } + } + const payload = await buildPayload(); try { // Never follows redirects: the endpoint guard only vetted the URL we @@ -55,10 +104,11 @@ export async function sendOnce(opts?: { reason?: string }): Promise<{ signal: AbortSignal.timeout(5000), }); const ok = res.ok; - if (ok) { + if (ok && !claim) { const at = new Date().toISOString(); await updateState((next) => { next.lastSentAt = at; }); } + if (!ok && claim?.claimed) await releaseSlot(claim); logger.info('telemetry: heartbeat', { ok, status: res.status, reason: opts?.reason ?? 'scheduled', }); @@ -66,6 +116,7 @@ export async function sendOnce(opts?: { reason?: string }): Promise<{ } catch (err) { const msg = err instanceof Error ? err.message : String(err); logger.warn('telemetry: heartbeat failed', { error: msg }); + if (claim?.claimed) await releaseSlot(claim); return { ok: false, error: msg }; } } @@ -74,19 +125,28 @@ async function scheduleNext(delayMs: number): Promise { const t = timer(); if (t.current) clearTimeout(t.current); const at = new Date(Date.now() + delayMs).toISOString(); - await updateState((state) => { state.nextScheduledAt = at; }); - if (t.current) clearTimeout(t.current); - t.current = setTimeout(() => { void tick(); }, delayMs); + // The timer first: a state write that fails (store down) must not end + // the chain of heartbeats. + t.current = setTimeout(() => { void tick(at); }, delayMs); // Don't keep the process alive just for this. t.current.unref?.(); + try { + await updateState((state) => { state.nextScheduledAt = at; }); + } catch (err) { + logger.warn('telemetry: could not store the schedule', { error: err instanceof Error ? err.message : String(err) }); + } } -async function tick(): Promise { - await sendOnce({ reason: 'scheduled' }); +async function tick(slot: string): Promise { + try { + await sendOnce({ reason: 'scheduled', slot }); + } catch (err) { + logger.warn('telemetry: heartbeat failed', { error: err instanceof Error ? err.message : String(err) }); + } await scheduleNext(jitteredDelay(DAY_MS)); } -// Called from instrumentation. Idempotent. +// Run by the scheduler leader (lib/scheduler-leader.ts). Idempotent. export async function startScheduler(): Promise { const { consent } = await effectiveConsent(); if (consent !== 'on') { @@ -119,7 +179,8 @@ export async function stopScheduler(): Promise { } // Called when consent flips on/off or the collector URL changes via the UI. +// The replica that runs the jobs (any replica's change reaches it) restarts +// the timer from the stored state; in local mode that is this process, at once. export async function reschedule(): Promise { - await stopScheduler(); - await startScheduler(); + await announceChange('telemetry'); } diff --git a/lib/version-check/sender.ts b/lib/version-check/sender.ts index 7904dad93..324aced79 100644 --- a/lib/version-check/sender.ts +++ b/lib/version-check/sender.ts @@ -72,15 +72,22 @@ async function scheduleNext(delayMs: number): Promise { const t = timer(); if (t.current) clearTimeout(t.current); const at = new Date(Date.now() + delayMs).toISOString(); - await updateState((state) => { state.nextScheduledAt = at; }); - if (t.current) clearTimeout(t.current); + // The timer first: a state write that fails (store down) must not end + // the chain of checks. t.current = setTimeout(() => { void tick(); }, delayMs); // Don't keep the process alive just for this. t.current.unref?.(); + try { + await updateState((state) => { state.nextScheduledAt = at; }); + } catch (err) { + logger.warn('version-check: could not store the schedule', { error: err instanceof Error ? err.message : String(err) }); + } } async function tick(): Promise { - const result = await checkOnce({ reason: 'scheduled' }); + const result = await checkOnce({ reason: 'scheduled' }).catch((err: unknown) => ({ + ok: false as const, error: err instanceof Error ? err.message : String(err), + })); // The scheduler keeps running while third-party traffic is off, so turning // the switch off at runtime resumes the checks; checkOnce skips meanwhile. const skipped = result.error === NO_THIRD_PARTY_ERROR; @@ -88,7 +95,8 @@ async function tick(): Promise { await scheduleNext(delay); } -// Idempotent - safe to call from instrumentation hot-reload in dev. +// Run by the scheduler leader (lib/scheduler-leader.ts). Idempotent - safe +// to call from instrumentation hot-reload in dev. export async function startScheduler(): Promise { if (disabledByEnv()) { logger.info('version-check: scheduler not started (disabled by env)'); From e8be9309840e35b899bdefcd82e8a0abbf5925be Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Sat, 31 Oct 2026 23:26:53 +0100 Subject: [PATCH 10/31] feat: import settings, plugin credentials, archives and telemetry state into the shared store #1066 --- .../__tests__/import-data.test.ts | 103 ++++++++++++++++++ lib/shared-store/import/data.ts | 42 +++++++ 2 files changed, 145 insertions(+) create mode 100644 lib/shared-store/__tests__/import-data.test.ts create mode 100644 lib/shared-store/import/data.ts diff --git a/lib/shared-store/__tests__/import-data.test.ts b/lib/shared-store/__tests__/import-data.test.ts new file mode 100644 index 000000000..80e1c589c --- /dev/null +++ b/lib/shared-store/__tests__/import-data.test.ts @@ -0,0 +1,103 @@ +/** + * @vitest-environment node + */ +import { mkdirSync, mkdtempSync, readFileSync, readdirSync, rmSync, statSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import path from 'node:path'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { setStoreForTests } from '../index'; +import { resetLocalStoreForTests } from '../local'; +import { separateDiskStore, type SeparateDiskStore } from '@/lib/__tests__/helpers/separate-disk-store'; +import { steps } from '../import/data'; +import { SETTINGS_COLLECTION } from '@/lib/settings-sync'; +import { VAULT_COLLECTION } from '@/lib/plugin-secrets/vault'; +import { ARCHIVE_COLLECTION } from '@/lib/account-archive-storage'; +import { TELEMETRY_COLLECTION } from '@/lib/telemetry/state'; +import { VERSION_CHECK_COLLECTION } from '@/lib/version-check/state'; + +// An existing single instance's settings, plugin credentials, archives, +// telemetry and update-check state go into an empty shared store once, as +// they are; the local files stay untouched. + +let data: string; +let store: SeparateDiskStore; + +const hash = (c: string) => c.repeat(64); + +function snapshot(dir: string): Record { + const out: Record = {}; + const walk = (d: string) => { + for (const name of readdirSync(d)) { + const full = path.join(d, name); + if (statSync(full).isDirectory()) walk(full); + else out[path.relative(dir, full)] = readFileSync(full).toString('base64'); + } + }; + walk(dir); + return out; +} + +beforeEach(() => { + data = mkdtempSync(path.join(tmpdir(), 'bw-import-data-')); + vi.stubEnv('SETTINGS_DATA_DIR', path.join(data, 'settings')); + vi.stubEnv('PLUGIN_SECRETS_DIR', path.join(data, 'plugin-secrets')); + vi.stubEnv('TELEMETRY_DATA_DIR', path.join(data, 'telemetry')); + vi.stubEnv('VERSION_CHECK_DATA_DIR', path.join(data, 'version-check')); + for (const dir of ['settings/account-archives', 'plugin-secrets', 'telemetry', 'version-check']) { + mkdirSync(path.join(data, dir), { recursive: true }); + } + writeFileSync(path.join(data, 'settings', `${hash('a')}.enc`), Buffer.from([1, 2, 3, 4])); + writeFileSync(path.join(data, 'settings', `${hash('b')}.enc`), Buffer.from([5, 6])); + writeFileSync(path.join(data, 'settings', `${hash('c')}.enc.tmp`), 'half written'); + writeFileSync(path.join(data, 'settings', 'account-archives', `${hash('d')}.json`), '{"version":1}'); + writeFileSync(path.join(data, 'plugin-secrets', `${hash('e')}.enc`), 'c2VhbGVk'); + writeFileSync(path.join(data, 'telemetry', 'state.json'), '{"consent":"on"}'); + writeFileSync(path.join(data, 'telemetry', '.telemetry-id'), '0b9f1f3e-8b8e-4c4b-9a35-3c1b8a7a1e11'); + writeFileSync(path.join(data, 'telemetry', 'logins.json'), '{"records":[]}'); + writeFileSync(path.join(data, 'version-check', 'state.json'), '{"status":null}'); + store = separateDiskStore(); + setStoreForTests(store); +}); + +afterEach(() => { + setStoreForTests(null); + resetLocalStoreForTests(); + vi.unstubAllEnvs(); + store.cleanup(); + rmSync(data, { recursive: true, force: true }); +}); + +async function runAll(): Promise> { + const counts: Record = {}; + for (const step of steps) counts[step.name] = await step.run(); + return counts; +} + +describe('importing part data', () => { + it('copies every document once and leaves the local files alone', async () => { + const before = snapshot(data); + + expect(await runAll()).toEqual({ + settings: 2, 'plugin-secrets': 1, 'account-archives': 1, telemetry: 3, 'version-check': 1, + }); + expect(await runAll()).toEqual({ + settings: 0, 'plugin-secrets': 0, 'account-archives': 0, telemetry: 0, 'version-check': 0, + }); + expect(snapshot(data)).toEqual(before); + + const settings = store.collection('settings', SETTINGS_COLLECTION); + expect(await settings.list()).toEqual([hash('a'), hash('b')]); + expect((await settings.get(hash('a')))?.data).toEqual(Buffer.from([1, 2, 3, 4])); + expect((await store.collection('plugin-secrets', VAULT_COLLECTION).get(hash('e')))?.data.toString()).toBe('c2VhbGVk'); + expect((await store.collection('account-archives', ARCHIVE_COLLECTION).get(hash('d')))?.data.toString()).toBe('{"version":1}'); + expect((await store.collection('telemetry', TELEMETRY_COLLECTION).list()).sort()).toEqual(['.telemetry-id', 'logins.json', 'state.json']); + expect((await store.collection('version-check', VERSION_CHECK_COLLECTION).get('state.json'))?.data.toString()).toBe('{"status":null}'); + }); + + it('keeps what the store already holds', async () => { + const settings = store.collection('settings', SETTINGS_COLLECTION); + await settings.put(hash('a'), Buffer.from([9])); + expect((await runAll()).settings).toBe(1); + expect((await settings.get(hash('a')))?.data).toEqual(Buffer.from([9])); + }); +}); diff --git a/lib/shared-store/import/data.ts b/lib/shared-store/import/data.ts new file mode 100644 index 000000000..9af9a1030 --- /dev/null +++ b/lib/shared-store/import/data.ts @@ -0,0 +1,42 @@ +import { getStore } from '@/lib/shared-store'; +import { localCollection } from '@/lib/shared-store/local'; +import type { CollectionOptions } from '@/lib/shared-store/types'; +import type { ImportStep } from '@/lib/shared-store/import'; +import { SETTINGS_COLLECTION } from '@/lib/settings-sync'; +import { VAULT_COLLECTION } from '@/lib/plugin-secrets/vault'; +import { ARCHIVE_COLLECTION } from '@/lib/account-archive-storage'; +import { TELEMETRY_COLLECTION } from '@/lib/telemetry/state'; +import { VERSION_CHECK_COLLECTION } from '@/lib/version-check/state'; + +/** + * Part `data` of the shared-store import (#1066): every document of these + * collections, read from the local files exactly as local mode reads them, + * written to the shared store only where it holds nothing yet. The bytes go + * over as they are: settings and plugin credentials stay encrypted with the + * session secret, which the boot imports first. + */ +function copyCollection(name: string, options: CollectionOptions): ImportStep { + return { + name, + async run() { + const local = localCollection(name, options); + const shared = getStore().collection(name, options); + let written = 0; + for (const id of await local.list()) { + const doc = await local.get(id); + if (!doc) continue; + if ((await shared.put(id, doc.data, { ifVersion: null })).ok) written++; + } + return written; + }, + }; +} + +export const steps: ImportStep[] = [ + copyCollection('settings', SETTINGS_COLLECTION), + copyCollection('plugin-secrets', VAULT_COLLECTION), + copyCollection('account-archives', ARCHIVE_COLLECTION), + // state.json, logins.json and the instance id .telemetry-id + copyCollection('telemetry', TELEMETRY_COLLECTION), + copyCollection('version-check', VERSION_CHECK_COLLECTION), +]; From ff6116e991c70bf845280fb0168180faa4bd2612 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Sat, 31 Oct 2026 23:44:26 +0100 Subject: [PATCH 11/31] refactor: recognise a store outage with the shared-store helper in user data routes --- lib/stored-data-errors.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/stored-data-errors.ts b/lib/stored-data-errors.ts index 453f93214..38b029548 100644 --- a/lib/stored-data-errors.ts +++ b/lib/stored-data-errors.ts @@ -1,4 +1,5 @@ import { NextResponse } from 'next/server'; +import { isStoreUnavailableError } from '@/lib/shared-store/types'; /** * A user's stored document exists but can't be read: it doesn't decrypt @@ -22,7 +23,7 @@ function named(error: unknown, name: string): boolean { /** The shared store couldn't be reached (StoreUnavailableError from lib/shared-store). */ export function isStoreUnavailable(error: unknown): boolean { - return named(error, 'StoreUnavailableError'); + return isStoreUnavailableError(error); } export function isStoredDataUnreadable(error: unknown): boolean { From 698b881f1f7180445283e88a364b1e20dc706571 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Sat, 31 Oct 2026 23:45:53 +0100 Subject: [PATCH 12/31] refactor: use the shared store's own outage check in the admin code #1066 --- lib/admin/admin-store.ts | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/lib/admin/admin-store.ts b/lib/admin/admin-store.ts index ef099076e..72e423ca4 100644 --- a/lib/admin/admin-store.ts +++ b/lib/admin/admin-store.ts @@ -1,4 +1,4 @@ -import { getStore, type CollectionOptions, type DocCollection } from '@/lib/shared-store'; +import { getStore, isStoreUnavailableError, type CollectionOptions, type DocCollection } from '@/lib/shared-store'; import { getConfigDir, getStateDir } from './paths'; /** @@ -32,12 +32,7 @@ export function adminStateDocs(): DocCollection { return getStore().collection(ADMIN_STATE_COLLECTION, adminStateOptions); } -/** - * Whether `error` is the shared store's StoreUnavailableError. By name, not - * instanceof: the store on globalThis may come from another bundle's copy - * of the shared-store module (proxy, instrumentation, routes), whose class - * is a different object. - */ +/** Whether `error` is the shared store's StoreUnavailableError, from any bundle's module copy. */ export function isStoreUnavailable(error: unknown): boolean { - return error instanceof Error && error.name === 'StoreUnavailableError'; + return isStoreUnavailableError(error); } From 7faed7e5f4739d2208ab645e5d979fd3debc3137 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Sat, 31 Oct 2026 23:46:01 +0100 Subject: [PATCH 13/31] fix: send a change notice again when the shared store dropped right after an admin save #1066 --- lib/shared-store/__tests__/boot.test.ts | 22 ++++++++++++++++++++++ lib/shared-store/changes.ts | 23 +++++++++++++++++++---- 2 files changed, 41 insertions(+), 4 deletions(-) diff --git a/lib/shared-store/__tests__/boot.test.ts b/lib/shared-store/__tests__/boot.test.ts index d6964070d..d6e9a44ff 100644 --- a/lib/shared-store/__tests__/boot.test.ts +++ b/lib/shared-store/__tests__/boot.test.ts @@ -87,6 +87,28 @@ describe('change notices', () => { expect(seen).toEqual(['local', 'local', 'remote']); }); + it('sends a change notice again when the store was unreachable right after the write', async () => { + const store = fakeSharedStore(); + setStoreForTests(store); + await store.epochs.bump(INIT_EPOCH); + await markEpochsSeen(); + const bump = store.epochs.bump.bind(store.epochs); + let fail = true; + store.epochs.bump = async (area: string) => { + if (fail) throw new Error('connection dropped'); + return bump(area); + }; + await announceChange('config'); + expect((await store.epochs.all()).config).toBeUndefined(); + await pollChanges(); // still down + expect((await store.epochs.all()).config).toBeUndefined(); + fail = false; + await pollChanges(); + expect((await store.epochs.all()).config).toBe(1); + await pollChanges(); + expect((await store.epochs.all()).config).toBe(1); // sent once, not every poll + }); + it('retries a listener that failed on the next poll', async () => { const store = fakeSharedStore(); setStoreForTests(store); diff --git a/lib/shared-store/changes.ts b/lib/shared-store/changes.ts index 060e8934f..c0bc3a79c 100644 --- a/lib/shared-store/changes.ts +++ b/lib/shared-store/changes.ts @@ -33,13 +33,15 @@ interface ChangeGlobals { polling?: Promise; lastPollOk: number; storeLost: boolean; + /** Areas whose epoch bump failed (store unreachable after the write). */ + pendingBumps: Set; } const KEY = Symbol.for('bulwark.sharedStore.changes'); function globals(): ChangeGlobals { const g = globalThis as typeof globalThis & { [KEY]?: ChangeGlobals }; - return (g[KEY] ??= { listeners: new Map(), seen: new Map(), lastPollOk: 0, storeLost: false }); + return (g[KEY] ??= { listeners: new Map(), seen: new Map(), lastPollOk: 0, storeLost: false, pendingBumps: new Set() }); } /** @@ -74,8 +76,8 @@ async function runListeners(area: string, origin: ChangeOrigin): Promise { const g = globals(); @@ -86,7 +88,8 @@ export async function announceChange(area: string): Promise { // otherwise the poller reloads to pick up the other replica's. if ((g.seen.get(area) ?? 0) === epoch - 1) g.seen.set(area, epoch); } catch (error) { - logger.warn('Could not announce a change to the other replicas', { + g.pendingBumps.add(area); + logger.warn('Could not announce a change to the other replicas; retrying with the next poll', { area, error: error instanceof Error ? error.message : 'Unknown error', }); @@ -95,11 +98,23 @@ export async function announceChange(area: string): Promise { await runListeners(area, 'local'); } +/** Send the bumps that failed earlier. A failure leaves them for the next poll. */ +async function retryPendingBumps(g: ChangeGlobals): Promise { + for (const area of [...g.pendingBumps]) { + await getStore().epochs.bump(area); + g.pendingBumps.delete(area); + logger.info('Announced a change to the other replicas after a retry', { area }); + } +} + /** One poll: read the epochs, run the listeners of every area that moved. */ export async function pollChanges(): Promise { const g = globals(); if (g.polling) return g.polling; g.polling = (async () => { + // Our own retried bump then shows up as a change on this replica too, + // which costs one reload and keeps the logic simple. + if (g.pendingBumps.size) await retryPendingBumps(g); const epochs = await getStore().epochs.all(); if (!epochs[INIT_EPOCH]) { if (!g.storeLost) { From d425a873e2670061ad58c63b25ea6cdbd78477e9 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Sat, 31 Oct 2026 23:47:31 +0100 Subject: [PATCH 14/31] fix: keep a replica that lost the scheduler lead from rearming the heartbeat or update check #1066 --- lib/__tests__/scheduler-leader.test.ts | 78 +++++++++++++++++++++++++- lib/scheduler-leader.ts | 3 + lib/telemetry/sender.ts | 26 ++++++--- lib/version-check/sender.ts | 26 ++++++--- 4 files changed, 116 insertions(+), 17 deletions(-) diff --git a/lib/__tests__/scheduler-leader.test.ts b/lib/__tests__/scheduler-leader.test.ts index 89641891b..6b41f059b 100644 --- a/lib/__tests__/scheduler-leader.test.ts +++ b/lib/__tests__/scheduler-leader.test.ts @@ -7,7 +7,7 @@ import { setTimeout as realSleep } from 'node:timers/promises'; import { NextRequest } from 'next/server'; import { replicaId, setStoreForTests } from '@/lib/shared-store'; import { resetLocalStoreForTests } from '@/lib/shared-store/local'; -import { INIT_EPOCH, pollChanges, resetChangesForTests } from '@/lib/shared-store/changes'; +import { INIT_EPOCH, announceChange, pollChanges, resetChangesForTests } from '@/lib/shared-store/changes'; import { separateDiskStore, type SeparateDiskStore } from './helpers/separate-disk-store'; // The background jobs with several replicas (#1066): only the holder of the @@ -152,6 +152,82 @@ describe('scheduler leader with a shared store', () => { expect(sent).not.toHaveBeenCalled(); }); + it('does not arm a new timer after a send that was in flight when the lead moved', async () => { + const slot = new Date(Date.now() + MIN).toISOString(); + await consentOn(slot); + let finish!: (res: Response) => void; + sent.mockImplementationOnce(() => new Promise((resolve) => { finish = resolve; })); + await startSchedulerLeader(jobs); + + await vi.advanceTimersByTimeAsync(MIN + 1000); + await sends(1); + // The lead moves to replica-a while the heartbeat hangs. + await store.leases.release(LEADER_LEASE, replicaId()); + expect(await store.leases.acquire(LEADER_LEASE, 'replica-a', 30_000)).toBe(true); + await pass(10_000); + expect(isSchedulerLeader()).toBe(false); + await stopSchedulerLeader(); // no more tries from this replica + + finish(new Response('ok')); + await ioSettles(); + await vi.advanceTimersByTimeAsync(27 * 60 * MIN); + await ioSettles(); + expect(sent).toHaveBeenCalledTimes(1); + expect(vi.getTimerCount()).toBe(0); + }); + + it('stops a job whose restart finished after the replica stepped down', async () => { + let running = false; + let starts = 0; + let finishStart: (() => void) | null = null; + const job = { + name: 'telemetry', + start: async () => { + if (++starts === 2) await new Promise((resolve) => { finishStart = resolve; }); + running = true; + }, + stop: async () => { running = false; }, + }; + await startSchedulerLeader([job]); + expect(running).toBe(true); + + // Consent changed: the restart's start hangs while the lead is lost. + const restarted = announceChange('telemetry'); + await vi.waitFor(() => expect(finishStart).not.toBeNull()); + store.down = true; + await pass(10_000); + expect(isSchedulerLeader()).toBe(false); + finishStart!(); + await restarted; + expect(running).toBe(false); + }); + + it('version check: does not arm a new timer after a check that was in flight when it stopped', async () => { + vi.stubEnv('VERSION_CHECK_DATA_DIR', path.join(dir, 'version-check')); + vi.stubEnv('BULWARK_UPDATE_CHECK', ''); + vi.stubEnv('BULWARK_UPDATE_CHECK_URL', 'https://version.example/'); + vi.stubEnv('NEXT_PUBLIC_APP_VERSION', '1.20.0'); + let finish!: (res: Response) => void; + const fetchMock = vi.fn(() => new Promise((resolve) => { finish = resolve; })); + vi.stubGlobal('fetch', fetchMock); + const versionCheck = await import('@/lib/version-check'); + try { + await versionCheck.startScheduler(); + await vi.advanceTimersByTimeAsync(31_000); + await vi.waitFor(() => expect(fetchMock).toHaveBeenCalledTimes(1)); + await versionCheck.stopScheduler(); + finish(new Response('{}', { status: 500 })); + await ioSettles(); + await vi.advanceTimersByTimeAsync(2 * 60 * MIN); + await ioSettles(); + expect(fetchMock).toHaveBeenCalledTimes(1); + expect(vi.getTimerCount()).toBe(0); + } finally { + await versionCheck.stopScheduler(); + vi.unstubAllGlobals(); + } + }); + it('sends once when two leaders fire for the same slot, and not again after it', async () => { const slot = new Date(Date.now() - 1000).toISOString(); await consentOn(slot); diff --git a/lib/scheduler-leader.ts b/lib/scheduler-leader.ts index ebbd10f56..8fbd29b12 100644 --- a/lib/scheduler-leader.ts +++ b/lib/scheduler-leader.ts @@ -59,7 +59,9 @@ async function startJobs(g: LeaderGlobals): Promise { if (g.started.has(job.name)) continue; try { await job.start(); + // Stepped down while it started: don't leave its timer running. if (g.leading) g.started.add(job.name); + else await job.stop(); } catch (error) { logger.warn('scheduler: could not start a job; retrying', { job: job.name, error: message(error) }); } @@ -126,6 +128,7 @@ async function restartJob(name: string): Promise { await job.stop(); await job.start(); if (g.leading) g.started.add(name); + else await job.stop(); } /** diff --git a/lib/telemetry/sender.ts b/lib/telemetry/sender.ts index fe7bcd5ec..fa05b55f8 100644 --- a/lib/telemetry/sender.ts +++ b/lib/telemetry/sender.ts @@ -17,9 +17,16 @@ const SLOT_SLACK_MS = 60 * 1000; // each route), and a reschedule from a route must clear the timer the // instrumentation copy started, not a copy of its own. const TIMER_KEY = Symbol.for('bulwark.telemetry.timer'); -function timer(): { current: NodeJS.Timeout | null } { - const g = globalThis as typeof globalThis & { [TIMER_KEY]?: { current: NodeJS.Timeout | null } }; - return (g[TIMER_KEY] ??= { current: null }); +/** + * `generation` goes up on every stop. A start or a tick captures it and + * arms a timer only while it is unchanged, so a check or send still in + * flight when the scheduler stops (this replica lost the lead) can't set a + * new timer afterwards. + */ +interface TimerState { current: NodeJS.Timeout | null; generation: number } +function timer(): TimerState { + const g = globalThis as typeof globalThis & { [TIMER_KEY]?: TimerState }; + return (g[TIMER_KEY] ??= { current: null, generation: 0 }); } function jitteredDelay(base: number): number { @@ -121,13 +128,14 @@ export async function sendOnce(opts?: { reason?: string; slot?: string }): Promi } } -async function scheduleNext(delayMs: number): Promise { +async function scheduleNext(delayMs: number, generation: number): Promise { const t = timer(); + if (t.generation !== generation) return; // stopped meanwhile if (t.current) clearTimeout(t.current); const at = new Date(Date.now() + delayMs).toISOString(); // The timer first: a state write that fails (store down) must not end // the chain of heartbeats. - t.current = setTimeout(() => { void tick(at); }, delayMs); + t.current = setTimeout(() => { void tick(at, generation); }, delayMs); // Don't keep the process alive just for this. t.current.unref?.(); try { @@ -137,17 +145,18 @@ async function scheduleNext(delayMs: number): Promise { } } -async function tick(slot: string): Promise { +async function tick(slot: string, generation: number): Promise { try { await sendOnce({ reason: 'scheduled', slot }); } catch (err) { logger.warn('telemetry: heartbeat failed', { error: err instanceof Error ? err.message : String(err) }); } - await scheduleNext(jitteredDelay(DAY_MS)); + await scheduleNext(jitteredDelay(DAY_MS), generation); } // Run by the scheduler leader (lib/scheduler-leader.ts). Idempotent. export async function startScheduler(): Promise { + const generation = timer().generation; const { consent } = await effectiveConsent(); if (consent !== 'on') { logger.info('telemetry: scheduler not started', { consent }); @@ -165,7 +174,7 @@ export async function startScheduler(): Promise { const remaining = new Date(state.nextScheduledAt).getTime() - Date.now(); if (remaining > 0) delay = Math.min(remaining, DAY_MS + JITTER_MS); } - await scheduleNext(delay); + await scheduleNext(delay, generation); logger.info('telemetry: scheduler started', { nextInMs: delay, endpoint: state.endpoint, @@ -174,6 +183,7 @@ export async function startScheduler(): Promise { export async function stopScheduler(): Promise { const t = timer(); + t.generation++; if (t.current) clearTimeout(t.current); t.current = null; } diff --git a/lib/version-check/sender.ts b/lib/version-check/sender.ts index 324aced79..9fb66db24 100644 --- a/lib/version-check/sender.ts +++ b/lib/version-check/sender.ts @@ -17,9 +17,16 @@ const FAILURE_BACKOFF_MS = 15 * 60 * 1000; // after a failed fetch, retry in 15 // each route), and a reschedule from a route must clear the timer the // instrumentation copy started, not a copy of its own. const TIMER_KEY = Symbol.for('bulwark.versionCheck.timer'); -function timer(): { current: NodeJS.Timeout | null } { - const g = globalThis as typeof globalThis & { [TIMER_KEY]?: { current: NodeJS.Timeout | null } }; - return (g[TIMER_KEY] ??= { current: null }); +/** + * `generation` goes up on every stop. A start or a tick captures it and + * arms a timer only while it is unchanged, so a check or send still in + * flight when the scheduler stops (this replica lost the lead) can't set a + * new timer afterwards. + */ +interface TimerState { current: NodeJS.Timeout | null; generation: number } +function timer(): TimerState { + const g = globalThis as typeof globalThis & { [TIMER_KEY]?: TimerState }; + return (g[TIMER_KEY] ??= { current: null, generation: 0 }); } /** What checkOnce reports while NO_THIRD_PARTY_TRAFFIC is on. */ @@ -68,13 +75,14 @@ export async function checkOnce(opts?: { reason?: string }): Promise<{ return { ok: false, error: result.error }; } -async function scheduleNext(delayMs: number): Promise { +async function scheduleNext(delayMs: number, generation: number): Promise { const t = timer(); + if (t.generation !== generation) return; // stopped meanwhile if (t.current) clearTimeout(t.current); const at = new Date(Date.now() + delayMs).toISOString(); // The timer first: a state write that fails (store down) must not end // the chain of checks. - t.current = setTimeout(() => { void tick(); }, delayMs); + t.current = setTimeout(() => { void tick(generation); }, delayMs); // Don't keep the process alive just for this. t.current.unref?.(); try { @@ -84,7 +92,7 @@ async function scheduleNext(delayMs: number): Promise { } } -async function tick(): Promise { +async function tick(generation: number): Promise { const result = await checkOnce({ reason: 'scheduled' }).catch((err: unknown) => ({ ok: false as const, error: err instanceof Error ? err.message : String(err), })); @@ -92,12 +100,13 @@ async function tick(): Promise { // the switch off at runtime resumes the checks; checkOnce skips meanwhile. const skipped = result.error === NO_THIRD_PARTY_ERROR; const delay = result.ok || skipped ? jitteredDelay(HOUR_MS) : FAILURE_BACKOFF_MS; - await scheduleNext(delay); + await scheduleNext(delay, generation); } // Run by the scheduler leader (lib/scheduler-leader.ts). Idempotent - safe // to call from instrumentation hot-reload in dev. export async function startScheduler(): Promise { + const generation = timer().generation; if (disabledByEnv()) { logger.info('version-check: scheduler not started (disabled by env)'); return; @@ -134,12 +143,13 @@ export async function startScheduler(): Promise { const remaining = new Date(state.nextScheduledAt).getTime() - Date.now(); if (remaining > 0) delay = Math.min(remaining, HOUR_MS + JITTER_MS); } - await scheduleNext(delay); + await scheduleNext(delay, generation); logger.info('version-check: scheduler started', { nextInMs: delay }); } export async function stopScheduler(): Promise { const t = timer(); + t.generation++; if (t.current) clearTimeout(t.current); t.current = null; } From 01acacc27b118708701212a5e7e39ccc5e719ac6 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Sat, 31 Oct 2026 23:48:40 +0100 Subject: [PATCH 15/31] fix: answer 503 instead of 500 when the shared store is full or too busy to take a write #1066 --- .../__tests__/store-errors.test.ts | 22 +++++++++++++++++++ lib/shared-store/redis.ts | 11 ++++++---- 2 files changed, 29 insertions(+), 4 deletions(-) diff --git a/lib/shared-store/__tests__/store-errors.test.ts b/lib/shared-store/__tests__/store-errors.test.ts index a957b63b6..b6f8564ee 100644 --- a/lib/shared-store/__tests__/store-errors.test.ts +++ b/lib/shared-store/__tests__/store-errors.test.ts @@ -12,3 +12,25 @@ describe('isStoreUnavailableError', () => { expect(second.isStoreUnavailableError(null)).toBe(false); }); }); + +describe('redis store error mapping', () => { + const reply = (message: string) => Object.assign(new Error(message), { name: 'ReplyError' }); + + it('treats a full server (OOM under noeviction) and connection errors as unavailable', async () => { + const { unavailable } = await import('../redis'); + const { isStoreUnavailableError } = await import('../types'); + for (const error of [reply('OOM command not allowed when used memory > \'maxmemory\'.'), reply('LOADING Redis is loading'), new Error('Stream isn\'t writeable')]) { + let thrown: unknown; + try { unavailable(error); } catch (e) { thrown = e; } + expect(isStoreUnavailableError(thrown)).toBe(true); + } + }); + + it('rethrows script and command errors as they are', async () => { + const { unavailable } = await import('../redis'); + const bug = reply('ERR Error running script (call to f_x): attempt to compare nil'); + let thrown: unknown; + try { unavailable(bug); } catch (e) { thrown = e; } + expect(thrown).toBe(bug); + }); +}); diff --git a/lib/shared-store/redis.ts b/lib/shared-store/redis.ts index 5483f4e5d..f698facec 100644 --- a/lib/shared-store/redis.ts +++ b/lib/shared-store/redis.ts @@ -128,9 +128,11 @@ type ScriptName = keyof typeof SCRIPTS; type Scripted = Redis & { [K in ScriptName]: (...args: Array) => Promise }; // Server replies that mean "not available right now", not "bug in our script". -const UNAVAILABLE_REPLY = /^(NOAUTH|WRONGPASS|LOADING|READONLY|MASTERDOWN|BUSY|CLUSTERDOWN|TRYAGAIN|ERR max number of clients)/; +// OOM is what a full server with the required noeviction policy answers to writes. +const UNAVAILABLE_REPLY = /^(NOAUTH|WRONGPASS|LOADING|READONLY|MASTERDOWN|BUSY|CLUSTERDOWN|TRYAGAIN|OOM|NOREPLICAS|ERR max number of clients)/; -function unavailable(error: unknown): never { +/** Throw `error` as a StoreUnavailableError when it means the store is unavailable; rethrow it as it is otherwise. */ +export function unavailable(error: unknown): never { if (isStoreUnavailableError(error)) throw error; const name = (error as Error)?.name; const message = (error as Error)?.message ?? ''; @@ -233,7 +235,8 @@ export function createRedisStore(options: RedisStoreOptions): SharedStore { if (ok === 1) return next; await backoff(attempt); } - throw new Error(`Too many concurrent writes to ${key}`); + // Fail closed like an outage: the callers then answer 503 instead of 500. + throw new StoreUnavailableError(`Too many concurrent writes to ${key}`); }, }; @@ -318,7 +321,7 @@ export function createRedisStore(options: RedisStoreOptions): SharedStore { } await backoff(attempt); } - throw new Error(`Too many concurrent writes to ${name}/${id}`); + throw new StoreUnavailableError(`Too many concurrent writes to ${name}/${id}`); }, async delete(id, putOpts: PutOptions = {}) { assertDocId(id); From 63f4af27f5b27cf7fbd060aec69579b0cbec0bd1 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Sat, 31 Oct 2026 23:50:07 +0100 Subject: [PATCH 16/31] fix: send one heartbeat when the scheduler lead moves after a stall or a crash #1066 --- lib/__tests__/scheduler-leader.test.ts | 44 ++++++++++++++++++++++++++ lib/telemetry/sender.ts | 22 +++++++++++-- lib/telemetry/types.ts | 5 +++ 3 files changed, 68 insertions(+), 3 deletions(-) diff --git a/lib/__tests__/scheduler-leader.test.ts b/lib/__tests__/scheduler-leader.test.ts index 6b41f059b..8cf22aa34 100644 --- a/lib/__tests__/scheduler-leader.test.ts +++ b/lib/__tests__/scheduler-leader.test.ts @@ -245,6 +245,40 @@ describe('scheduler leader with a shared store', () => { expect(sent).toHaveBeenCalledTimes(1); }); + it('sends once when the old leader stalled past its slot and the new one picked a later slot', async () => { + const oldSlot = new Date(Date.now() - 1000).toISOString(); + await consentOn(oldSlot); + // The new leader found the slot passed and scheduled an hour out. + const newSlot = new Date(Date.now() + 60 * MIN).toISOString(); + // The stalled leader wakes up and sends for its slot first. + expect((await sendOnce({ reason: 'scheduled', slot: oldSlot })).ok).toBe(true); + vi.advanceTimersByTime(61 * MIN); + expect((await sendOnce({ reason: 'scheduled', slot: newSlot })).ok).toBe(false); + expect(sent).toHaveBeenCalledTimes(1); + }); + + it('sends once when a leader died after sending but before storing the next slot', async () => { + const slot = new Date(Date.now() - 1000).toISOString(); + await consentOn(slot); + expect((await sendOnce({ reason: 'scheduled', slot })).ok).toBe(true); + // nextScheduledAt still names the slot just sent; the next leader + // finds it passed and fires an hour later. + vi.advanceTimersByTime(60 * MIN); + expect((await sendOnce({ reason: 'scheduled', slot: new Date().toISOString() })).ok).toBe(false); + expect(sent).toHaveBeenCalledTimes(1); + }); + + it('still sends the next scheduled heartbeat after a manual "send now"', async () => { + const slot = new Date(Date.now() - 1000).toISOString(); + await consentOn(slot); + expect((await sendOnce({ reason: 'scheduled', slot })).ok).toBe(true); + vi.advanceTimersByTime(20 * 60 * MIN); + expect((await sendOnce({ reason: 'admin-manual' })).ok).toBe(true); + vi.advanceTimersByTime(2 * 60 * MIN); + expect((await sendOnce({ reason: 'scheduled', slot: new Date().toISOString() })).ok).toBe(true); + expect(sent).toHaveBeenCalledTimes(3); + }); + it('gives the slot back when the send fails', async () => { const slot = new Date(Date.now() - 1000).toISOString(); await consentOn(slot); @@ -288,6 +322,16 @@ describe('scheduler leader with a shared store', () => { }); describe('scheduler leader on a single instance', () => { + it('sends a scheduled heartbeat an hour after the last one, as before', async () => { + setStoreForTests(null); + const slot = new Date(Date.now() - 1000).toISOString(); + await consentOn(slot); + expect((await sendOnce({ reason: 'scheduled', slot })).ok).toBe(true); + vi.advanceTimersByTime(60 * MIN); + expect((await sendOnce({ reason: 'scheduled', slot: new Date().toISOString() })).ok).toBe(true); + expect((await loadState()).lastScheduledSentAt).toBeUndefined(); + }); + it('leads at once and restarts the heartbeat when consent changes in this process', async () => { setStoreForTests(null); await startSchedulerLeader(jobs); diff --git a/lib/telemetry/sender.ts b/lib/telemetry/sender.ts index fa05b55f8..cb77372cb 100644 --- a/lib/telemetry/sender.ts +++ b/lib/telemetry/sender.ts @@ -1,4 +1,5 @@ import { logger } from '@/lib/logger'; +import { isSharedMode } from '@/lib/shared-store'; import { announceChange } from '@/lib/shared-store/changes'; import { effectiveConsent, endpointEnabled, loadState, loadStateVersioned, saveStateIfUnchanged, updateState, @@ -12,6 +13,12 @@ const JITTER_MS = 2 * 60 * 60 * 1000; // ± 2 hours const FIRST_DELAY_MS = 60 * 60 * 1000; // 1 hour after consent /** A send this long before a scheduled slot already counts for that slot. */ const SLOT_SLACK_MS = 60 * 1000; +/** + * With a shared store, a scheduled heartbeat this recent means the day's is + * done. A new leader computes its own slot when the old one stalled past its + * slot or died before storing the next one, so the slot alone can't tell. + */ +const SCHEDULED_GAP_MS = 20 * 60 * 60 * 1000; // On globalThis: Next loads this module once per bundle (instrumentation, // each route), and a reschedule from a route must clear the timer the @@ -34,7 +41,9 @@ function jitteredDelay(base: number): number { return Math.max(60_000, base + j); } -type Claim = { claimed: true; previous: string | null; at: string } | { claimed: false }; +type Claim = + | { claimed: true; previous: string | null; previousScheduled: string | null | undefined; at: string; shared: boolean } + | { claimed: false }; /** * Take the scheduled send for `slot` (its nextScheduledAt): set lastSentAt @@ -44,14 +53,20 @@ type Claim = { claimed: true; previous: string | null; at: string } | { claimed: */ async function claimSlot(slot: string): Promise { const slotMs = new Date(slot).getTime(); + const shared = isSharedMode(); for (let attempt = 0; attempt < 5; attempt++) { const { state, version } = await loadStateVersioned(); if (version === null) return { claimed: false }; const last = state.lastSentAt ? new Date(state.lastSentAt).getTime() : NaN; if (!Number.isNaN(last) && last >= slotMs - SLOT_SLACK_MS) return { claimed: false }; + if (shared && state.lastScheduledSentAt) { + const scheduled = new Date(state.lastScheduledSentAt).getTime(); + if (!Number.isNaN(scheduled) && Date.now() - scheduled < SCHEDULED_GAP_MS) return { claimed: false }; + } const at = new Date().toISOString(); - if (await saveStateIfUnchanged({ ...state, lastSentAt: at }, version)) { - return { claimed: true, previous: state.lastSentAt, at }; + const next = shared ? { ...state, lastSentAt: at, lastScheduledSentAt: at } : { ...state, lastSentAt: at }; + if (await saveStateIfUnchanged(next, version)) { + return { claimed: true, previous: state.lastSentAt, previousScheduled: state.lastScheduledSentAt, at, shared }; } } return { claimed: false }; @@ -61,6 +76,7 @@ async function claimSlot(slot: string): Promise { async function releaseSlot(claim: Extract): Promise { await updateState((state) => { if (state.lastSentAt === claim.at) state.lastSentAt = claim.previous; + if (claim.shared && state.lastScheduledSentAt === claim.at) state.lastScheduledSentAt = claim.previousScheduled ?? null; }).catch(() => {}); } diff --git a/lib/telemetry/types.ts b/lib/telemetry/types.ts index 12c0ba472..0e8fccfe2 100644 --- a/lib/telemetry/types.ts +++ b/lib/telemetry/types.ts @@ -41,6 +41,11 @@ export interface TelemetryStateFile { endpoint: string; consentedAt: string | null; lastSentAt: string | null; + /** + * With a shared store only: when the last scheduled heartbeat went out. + * Unlike lastSentAt, the admin's "send now" doesn't set it. + */ + lastScheduledSentAt?: string | null; nextScheduledAt: string | null; } From 2130716d83a8446201ea0c1c42934bcc8f6ce0e7 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Sat, 31 Oct 2026 23:51:19 +0100 Subject: [PATCH 17/31] fix: keep the day's heartbeat when its payload can't be built and never report zero accounts while the store is down #1066 --- lib/__tests__/scheduler-leader.test.ts | 13 ++++++++++++- .../telemetry-version-check-shared-store.test.ts | 9 +++++++++ lib/telemetry/login-tracker.ts | 7 +++++-- lib/telemetry/sender.ts | 4 +++- 4 files changed, 29 insertions(+), 4 deletions(-) diff --git a/lib/__tests__/scheduler-leader.test.ts b/lib/__tests__/scheduler-leader.test.ts index 8cf22aa34..3e0943516 100644 --- a/lib/__tests__/scheduler-leader.test.ts +++ b/lib/__tests__/scheduler-leader.test.ts @@ -18,7 +18,8 @@ import { separateDiskStore, type SeparateDiskStore } from './helpers/separate-di const log = vi.hoisted(() => ({ info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() })); vi.mock('@/lib/logger', () => ({ logger: log })); vi.mock('@/lib/admin/third-party-mode', () => ({ isNoThirdPartyTrafficOn: async () => false })); -vi.mock('@/lib/telemetry/payload', () => ({ buildPayload: async () => ({ schema: '1' }), markProcessStart: () => {} })); +const buildPayload = vi.hoisted(() => vi.fn(async () => ({ schema: '1' }))); +vi.mock('@/lib/telemetry/payload', () => ({ buildPayload, markProcessStart: () => {} })); const sent = vi.hoisted(() => vi.fn(async (_url: string, _init?: unknown) => new Response('ok'))); vi.mock('@/lib/telemetry/endpoint-guard', () => ({ resolveEndpointAllowed: async () => ({ ok: true }), @@ -279,6 +280,16 @@ describe('scheduler leader with a shared store', () => { expect(sent).toHaveBeenCalledTimes(3); }); + it('gives the slot back when the payload cannot be built', async () => { + const slot = new Date(Date.now() - 1000).toISOString(); + await consentOn(slot); + buildPayload.mockRejectedValueOnce(new Error('store down')); + expect((await sendOnce({ reason: 'scheduled', slot })).ok).toBe(false); + expect((await loadState()).lastSentAt).toBeNull(); + expect((await sendOnce({ reason: 'scheduled', slot })).ok).toBe(true); + expect(sent).toHaveBeenCalledTimes(1); + }); + it('gives the slot back when the send fails', async () => { const slot = new Date(Date.now() - 1000).toISOString(); await consentOn(slot); diff --git a/lib/__tests__/telemetry-version-check-shared-store.test.ts b/lib/__tests__/telemetry-version-check-shared-store.test.ts index 895a26d72..f9cef8755 100644 --- a/lib/__tests__/telemetry-version-check-shared-store.test.ts +++ b/lib/__tests__/telemetry-version-check-shared-store.test.ts @@ -65,6 +65,15 @@ describe('telemetry with a shared store', () => { expect(await a.getLoginCounts()).toEqual({ total: 3, active7d: 3 }); }); + it('does not report zero accounts while the store is down', async () => { + const { recordLogin, getLoginCounts } = await import('@/lib/telemetry/login-tracker'); + await recordLogin('alice@example.org', 'https://mail.example.org'); + store.down = true; + await expect(getLoginCounts()).rejects.toThrow('down (test)'); + store.down = false; + expect(await getLoginCounts()).toEqual({ total: 1, active7d: 1 }); + }); + it('shows on replica B the consent set on replica A', async () => { const { updateState, effectiveConsent } = await import('@/lib/telemetry/state'); await updateState((state) => { state.consent = 'on'; }); diff --git a/lib/telemetry/login-tracker.ts b/lib/telemetry/login-tracker.ts index 96f776e81..a8a79c0d9 100644 --- a/lib/telemetry/login-tracker.ts +++ b/lib/telemetry/login-tracker.ts @@ -1,5 +1,6 @@ import { createHmac } from 'node:crypto'; import { logger } from '@/lib/logger'; +import { isStoreUnavailableError } from '@/lib/shared-store/types'; import { getInstanceId, telemetryDocs } from './state'; // We never store usernames or server URLs in the clear. Each login is @@ -81,7 +82,8 @@ export async function recordLogin(username: string, serverUrl: string): Promise< /** * Total distinct accounts seen in the 90-day retention window, plus those * with a login in the last 7 days. Read from the store each time, so every - * replica reports the same numbers. + * replica reports the same numbers. Throws while the shared store is down + * rather than reporting zero accounts. */ export async function getLoginCounts(): Promise<{ total: number; active7d: number }> { try { @@ -97,7 +99,8 @@ export async function getLoginCounts(): Promise<{ total: number; active7d: numbe if (ts >= sevenAgo) active7d++; } return { total, active7d }; - } catch { + } catch (err) { + if (isStoreUnavailableError(err)) throw err; return { total: 0, active7d: 0 }; } } diff --git a/lib/telemetry/sender.ts b/lib/telemetry/sender.ts index cb77372cb..5eeab4366 100644 --- a/lib/telemetry/sender.ts +++ b/lib/telemetry/sender.ts @@ -116,8 +116,10 @@ export async function sendOnce(opts?: { reason?: string; slot?: string }): Promi } } - const payload = await buildPayload(); try { + // Inside the try: a payload that can't be built (store down) gives the + // claimed slot back instead of losing the day's heartbeat. + const payload = await buildPayload(); // Never follows redirects: the endpoint guard only vetted the URL we // were given, so a 3xx to an internal host would bypass it. const res = await fetchTelemetryTarget(endpoint, { From 68d64f5018a0daded9f2f1f6ee4ed799e2730e87 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Sat, 31 Oct 2026 23:55:17 +0100 Subject: [PATCH 18/31] fix: pair the policy with its own version and keep config reloads and saves from undoing each other #1066 Fixes from the review of the admin part: - GET /api/admin/policy reads the policy and its version together; an identity check waiting on the store let another replica's change pair the old policy with the new ETag, and a save with it then won. - A policy save answers with the stored policy and its ETag, and the policy, organisation, plugins and themes pages continue from that instead of reading it back from an instance that may not have it yet. - A stored policy that doesn't parse keeps its version, so a save can replace it instead of answering 409 forever. - A reload of another replica's change throws when this replica's writes kept landing while it read, so the change poller tries again; a write whose reload raced it reads the document again instead of applying its older result. - Dropping a deleted plugin from the policy rereads policy.json first in local mode, as every other write does. - New updateAdminConfig(key, change) builds a value on the stored one. The policy route's test mocks gain getPolicyWithVersion. --- app/(main)/admin/_pages/plugins.tsx | 9 +- app/(main)/admin/_pages/policy.tsx | 9 +- app/(main)/admin/_pages/themes.tsx | 9 +- .../__tests__/policy-bearer-route.test.ts | 1 + .../admin/__tests__/policy-etag-race.test.ts | 84 +++++++++ .../admin/__tests__/policy-if-match.test.ts | 16 ++ .../__tests__/policy-scopes-route.test.ts | 1 + app/api/admin/policy/route.ts | 13 +- components/admin/org/use-org-policy-draft.ts | 23 ++- lib/__tests__/instance-details-gates.test.ts | 1 + .../__tests__/config-manager-shared.test.ts | 97 ++++++++++ lib/admin/config-manager.ts | 172 ++++++++++++++---- 12 files changed, 384 insertions(+), 51 deletions(-) create mode 100644 app/api/admin/__tests__/policy-etag-race.test.ts diff --git a/app/(main)/admin/_pages/plugins.tsx b/app/(main)/admin/_pages/plugins.tsx index eb09a808e..4dda13db7 100644 --- a/app/(main)/admin/_pages/plugins.tsx +++ b/app/(main)/admin/_pages/plugins.tsx @@ -85,7 +85,14 @@ export function PluginsPage() { }); if (res.ok) { policyETag.current = res.headers.get('ETag'); - setSavedPolicy(policy); + // The server sanitises; continue from what it stored. + const stored = ((await res.json().catch(() => null)) as { policy?: SettingsPolicy } | null)?.policy; + if (stored) { + setPolicy(stored); + setSavedPolicy(stored); + } else { + setSavedPolicy(policy); + } return null; } const data = await res.json().catch(() => ({})); diff --git a/app/(main)/admin/_pages/policy.tsx b/app/(main)/admin/_pages/policy.tsx index 3922976c4..4e765d468 100644 --- a/app/(main)/admin/_pages/policy.tsx +++ b/app/(main)/admin/_pages/policy.tsx @@ -229,7 +229,14 @@ export function PolicyPage() { if (res.ok) { policyETag.current = res.headers.get('ETag'); - setSavedPolicy(policy); + // The server sanitises; continue from what it stored. + const stored = ((await res.json().catch(() => null)) as { policy?: SettingsPolicy } | null)?.policy; + if (stored) { + setPolicy(stored); + setSavedPolicy(stored); + } else { + setSavedPolicy(policy); + } setMessage({ type: 'success', text: 'Policy saved. Users see the change on their next login.' }); setDirty(false); } else { diff --git a/app/(main)/admin/_pages/themes.tsx b/app/(main)/admin/_pages/themes.tsx index 8d472a43b..6bffbb308 100644 --- a/app/(main)/admin/_pages/themes.tsx +++ b/app/(main)/admin/_pages/themes.tsx @@ -160,7 +160,14 @@ export function ThemesPage() { }); if (res.ok) { policyETag.current = res.headers.get('ETag'); - setSavedPolicy(policy); + // The server sanitises; continue from what it stored. + const stored = ((await res.json().catch(() => null)) as { policy?: SettingsPolicy } | null)?.policy; + if (stored) { + setPolicy(withThemeDefaults(stored)); + setSavedPolicy(withThemeDefaults(stored)); + } else { + setSavedPolicy(policy); + } return null; } const data = await res.json().catch(() => ({})); diff --git a/app/api/admin/__tests__/policy-bearer-route.test.ts b/app/api/admin/__tests__/policy-bearer-route.test.ts index 4bf99a360..fafe3ec9d 100644 --- a/app/api/admin/__tests__/policy-bearer-route.test.ts +++ b/app/api/admin/__tests__/policy-bearer-route.test.ts @@ -28,6 +28,7 @@ vi.mock('@/lib/admin/config-manager', () => ({ organisation: { disclaimer: { html: '

Acme GmbH

', text: 'Acme GmbH' } }, scopes: SCOPES, }), + getPolicyWithVersion() { return { policy: this.getPolicy(), version: 'v1' }; }, }, })); const logs = vi.hoisted(() => ({ calls: [] as unknown[][] })); diff --git a/app/api/admin/__tests__/policy-etag-race.test.ts b/app/api/admin/__tests__/policy-etag-race.test.ts new file mode 100644 index 000000000..23da59c54 --- /dev/null +++ b/app/api/admin/__tests__/policy-etag-race.test.ts @@ -0,0 +1,84 @@ +// @vitest-environment node +// GET /api/admin/policy waits on the store for the identity checks. A +// change from another replica applied meanwhile must not pair the policy +// read before with the version read after: a page saving that body with +// that ETag would put the old policy back over the other replica's save. +import { afterEach, beforeEach, expect, it, vi } from 'vitest'; +import { mkdtemp, rm } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import path from 'node:path'; +import { NextRequest } from 'next/server'; + +const hooks: { duringIdentity?: () => Promise } = {}; +vi.mock('@/lib/auth/instance-details', () => ({ + canSeeInstanceDetails: async () => true, + signedInIdentity: async () => { + // Stands in for the store round trip verifyCurrentAdminSession makes. + await hooks.duringIdentity?.(); + return { admin: true }; + }, +})); +vi.mock('@/lib/admin/session', () => ({ + requireAdminAuth: async () => ({ payload: { role: 'admin' } }), + getClientIP: () => '127.0.0.1', +})); +vi.mock('@/lib/admin/audit', () => ({ auditLog: async () => {} })); + +import { configManager } from '@/lib/admin/config-manager'; +import { adminConfigDocs } from '@/lib/admin/admin-store'; +import { setBootState, setStoreForTests } from '@/lib/shared-store'; +import { resetLocalStoreForTests } from '@/lib/shared-store/local'; +import { INIT_EPOCH, markEpochsSeen, pollChanges, resetChangesForTests } from '@/lib/shared-store/changes'; +import { GET, PUT } from '@/app/api/admin/policy/route'; +import { separateFakeStore } from '@/lib/admin/__tests__/shared-store-fake'; + +let dir: string; +let store: ReturnType; + +beforeEach(async () => { + dir = await mkdtemp(path.join(tmpdir(), 'bw-review-etag-')); + vi.stubEnv('ADMIN_CONFIG_DIR', path.join(dir, 'config')); + vi.stubEnv('ADMIN_STATE_DIR', path.join(dir, 'state')); + resetLocalStoreForTests(); + resetChangesForTests(); + store = separateFakeStore(path.join(dir, 'store')); + setStoreForTests(store); + await store.epochs.bump(INIT_EPOCH); + await adminConfigDocs().put('policy.json', JSON.stringify({ features: { calendarEnabled: true } })); + await markEpochsSeen(); + await configManager.load(); + setBootState('ready'); +}); + +afterEach(async () => { + hooks.duringIdentity = undefined; + setStoreForTests(null); + resetChangesForTests(); + resetLocalStoreForTests(); + vi.unstubAllEnvs(); + await rm(dir, { recursive: true, force: true }); +}); + +it('sends the version of the policy it sends, when a change lands during the request', async () => { + hooks.duringIdentity = async () => { + // Another replica saves calendarEnabled:false; the poll lands mid-GET. + await adminConfigDocs().put('policy.json', JSON.stringify({ features: { calendarEnabled: false, filesEnabled: false } })); + await store.epochs.bump('policy'); + await pollChanges(); + }; + const res = await GET(new NextRequest('https://mail.example/api/admin/policy')); + hooks.duringIdentity = undefined; + const body = await res.json(); + const etag = res.headers.get('ETag')!; + // Old body... + expect(body.features.calendarEnabled).toBe(true); + // ...saved with the ETag it came with: refused, the other save stays. + const put = await PUT(new NextRequest('https://mail.example/api/admin/policy', { + method: 'PUT', + headers: { 'content-type': 'application/json', 'if-match': etag }, + body: JSON.stringify(body), + })); + expect(put.status).toBe(409); + const stored = JSON.parse((await adminConfigDocs().get('policy.json'))!.data.toString('utf-8')); + expect(stored.features).toEqual({ calendarEnabled: false, filesEnabled: false }); +}); diff --git a/app/api/admin/__tests__/policy-if-match.test.ts b/app/api/admin/__tests__/policy-if-match.test.ts index f98ce7b4f..2671b86cb 100644 --- a/app/api/admin/__tests__/policy-if-match.test.ts +++ b/app/api/admin/__tests__/policy-if-match.test.ts @@ -94,6 +94,22 @@ describe('policy save with If-Match', () => { expect((await stored()).forceEnabledPlugins).toEqual([]); }); + it('drops a deleted plugin without undoing a save another replica made on the same volume', async () => { + await load(); + await writeFile(path.join(dir, 'policy.json'), JSON.stringify({ features: { calendarEnabled: false }, forceEnabledPlugins: ['demo', 'other'] })); + await forgetExtensionInPolicy('plugin', 'demo'); + expect((await stored()).forceEnabledPlugins).toEqual(['other']); + expect((await stored()).features.calendarEnabled).toBe(false); + }); + + it('answers a save with what it stored', async () => { + const page = await load(); + const res = await save({ ...page.policy, features: { ...(page.policy.features as object), calendarEnabled: false } }, page.etag); + const body = await res.json(); + expect(body.policy.features.calendarEnabled).toBe(false); + expect(res.headers.get('ETag')).toBe((await load()).etag); + }); + it('replaces the policy without If-Match, as before', async () => { await load(); await configManager.setPolicy({ ...configManager.getPolicy(), forceEnabledPlugins: [] }); diff --git a/app/api/admin/__tests__/policy-scopes-route.test.ts b/app/api/admin/__tests__/policy-scopes-route.test.ts index 70c6f3b70..40ed2582d 100644 --- a/app/api/admin/__tests__/policy-scopes-route.test.ts +++ b/app/api/admin/__tests__/policy-scopes-route.test.ts @@ -29,6 +29,7 @@ vi.mock('@/lib/admin/config-manager', () => ({ scopes: SCOPES, }), getPolicyVersion: () => 'v1', + getPolicyWithVersion() { return { policy: this.getPolicy(), version: 'v1' }; }, }, })); vi.mock('@/lib/logger', () => ({ logger: { error: vi.fn(), warn: vi.fn(), info: vi.fn(), debug: vi.fn() } })); diff --git a/app/api/admin/policy/route.ts b/app/api/admin/policy/route.ts index a799231df..06f160cb2 100644 --- a/app/api/admin/policy/route.ts +++ b/app/api/admin/policy/route.ts @@ -1,5 +1,5 @@ import { NextRequest, NextResponse } from 'next/server'; -import { configManager, isPolicyConflict } from '@/lib/admin/config-manager'; +import { completePolicy, configManager, isPolicyConflict } from '@/lib/admin/config-manager'; import { requireAdminAuth, getClientIP } from '@/lib/admin/session'; import { auditLog } from '@/lib/admin/audit'; import { logger } from '@/lib/logger'; @@ -37,7 +37,9 @@ function ifMatchVersion(request: NextRequest): string | undefined { export async function GET(request: NextRequest) { try { await configManager.ensureLoaded(); - const policy = configManager.getPolicy(); + // Together: the identity checks below wait on the store, and a change + // applied meanwhile must not pair this policy with a newer version. + const { policy, version } = configManager.getPolicyWithVersion(); if (!(await canSeeInstanceDetails(request))) { const tokenIdentity = await bearerIdentity(request); if (tokenIdentity) return userPolicyResponse(policy, tokenIdentity.addresses); @@ -70,7 +72,7 @@ export async function GET(request: NextRequest) { // The whole policy, as the admin pages edit it: with its version, which // they send back as If-Match when they save. return NextResponse.json({ ...policy, scopes: policy.scopes ?? [] }, { - headers: { 'Cache-Control': 'no-store', ETag: policyETag(configManager.getPolicyVersion()) }, + headers: { 'Cache-Control': 'no-store', ETag: policyETag(version) }, }); } catch (error) { logger.error('Policy read error', { error: error instanceof Error ? error.message : 'Unknown error' }); @@ -146,7 +148,10 @@ export async function PUT(request: NextRequest) { defaultSidebarAppCount: configManager.getPolicy().defaultSidebarApps?.length ?? 0, }, ip); - return NextResponse.json({ ok: true }, { headers: { ETag: policyETag(version) } }); + // What was stored, with its version: the page continues from this + // instead of reading it back from a replica that may not have it yet. + const saved = completePolicy(policy); + return NextResponse.json({ ok: true, policy: { ...saved, scopes: saved.scopes ?? [] } }, { headers: { ETag: policyETag(version) } }); } catch (error) { const readOnly = configReadOnlyResponse(error); if (readOnly) return readOnly; diff --git a/components/admin/org/use-org-policy-draft.ts b/components/admin/org/use-org-policy-draft.ts index 283f0d831..ac2470427 100644 --- a/components/admin/org/use-org-policy-draft.ts +++ b/components/admin/org/use-org-policy-draft.ts @@ -58,20 +58,24 @@ export function useOrgPolicyDraft(dirtyId: string) { const scopeId = useOrgScopeStore((s) => s.scopeId); const setScopeId = useOrgScopeStore((s) => s.setScopeId); + /** Start editing from `data`, the policy stored at version `etag`. */ + const adopt = useCallback((data: SettingsPolicy, etag: string | null) => { + policyETag.current = etag; + const normalised = { ...DEFAULT_POLICY, ...data, organisation: sanitizeOrganisationPolicy(data.organisation), scopes: data.scopes ?? [] }; + setPolicy(normalised); + setSaved(clone(normalised)); + }, []); + const load = useCallback(async () => { try { const res = await apiFetch('/api/admin/policy'); if (!res.ok) throw new Error(); - const data = (await res.json()) as SettingsPolicy; - policyETag.current = res.headers.get('ETag'); - const normalised = { ...DEFAULT_POLICY, ...data, organisation: sanitizeOrganisationPolicy(data.organisation), scopes: data.scopes ?? [] }; - setPolicy(normalised); - setSaved(clone(normalised)); + adopt((await res.json()) as SettingsPolicy, res.headers.get('ETag')); } catch { setMessage({ type: 'error', text: 'Could not load the policy' }); setPolicy((p) => p ?? { ...DEFAULT_POLICY, organisation: clone(DEFAULT_ORGANISATION_POLICY), scopes: [] }); } - }, [setMessage]); + }, [adopt, setMessage]); useEffect(() => { void load(); }, [load]); @@ -194,8 +198,11 @@ export function useOrgPolicyDraft(dirtyId: string) { body: JSON.stringify(policy), }); if (res.ok) { - // The server sanitises; show what it kept. - await load(); + // The server sanitises; show what it kept. From the answer, not a new + // read, which another instance might serve before it has the change. + const stored = ((await res.json().catch(() => null)) as { policy?: SettingsPolicy } | null)?.policy; + if (stored) adopt(stored, res.headers.get('ETag')); + else await load(); setMessage({ type: 'success', text: 'Saved. Users get the change the next time Bulwark loads.' }); } else { const data = await res.json().catch(() => ({})); diff --git a/lib/__tests__/instance-details-gates.test.ts b/lib/__tests__/instance-details-gates.test.ts index 2da5a6969..8be1eb7da 100644 --- a/lib/__tests__/instance-details-gates.test.ts +++ b/lib/__tests__/instance-details-gates.test.ts @@ -33,6 +33,7 @@ vi.mock('@/lib/admin/config-manager', () => ({ get: (key: string, fallback: unknown) => (key in config.values ? config.values[key] : fallback), getPolicy: () => POLICY, getPolicyVersion: () => 'v1', + getPolicyWithVersion() { return { policy: this.getPolicy(), version: 'v1' }; }, }, })); vi.mock('@/lib/logger', () => ({ logger: { error: vi.fn(), warn: vi.fn(), info: vi.fn(), debug: vi.fn() } })); diff --git a/lib/admin/__tests__/config-manager-shared.test.ts b/lib/admin/__tests__/config-manager-shared.test.ts index 4d66303b9..3021b93b1 100644 --- a/lib/admin/__tests__/config-manager-shared.test.ts +++ b/lib/admin/__tests__/config-manager-shared.test.ts @@ -39,6 +39,8 @@ import { separateFakeStore } from './shared-store-fake'; let dir: string; let store: ReturnType; +/** Runs inside the store's document calls, to land another replica's change mid-request. */ +const hooks: { onGet?: (id: string) => Promise; afterUpdate?: (id: string) => Promise } = {}; const stored = async (id: string) => { const doc = await adminConfigDocs().get(id); @@ -71,6 +73,24 @@ beforeEach(async () => { resetLocalStoreForTests(); resetChangesForTests(); store = separateFakeStore(path.join(dir, 'store')); + const collection = store.collection.bind(store); + store.collection = (name, options) => { + const docs = collection(name, options); + return { + ...docs, + get: async (id) => { + await hooks.onGet?.(id); + return docs.get(id); + }, + update: async (id, fn, updateOptions) => { + const result = await docs.update(id, fn, updateOptions); + const hook = hooks.afterUpdate; + hooks.afterUpdate = undefined; + await hook?.(id); + return result; + }, + }; + }; setStoreForTests(store); await store.epochs.bump(INIT_EPOCH); await adminConfigDocs().put('config.json', JSON.stringify({ appName: 'First', setupComplete: true })); @@ -81,6 +101,8 @@ beforeEach(async () => { }); afterEach(async () => { + hooks.onGet = undefined; + hooks.afterUpdate = undefined; setStoreForTests(null); resetChangesForTests(); resetLocalStoreForTests(); @@ -160,7 +182,82 @@ describe('config manager with a shared store', () => { }); }); +describe('change notices racing writes', () => { + it('fails a reload when writes of this replica kept landing while it read, so the poller tries again', async () => { + let reads = 0; + let n = 0; + hooks.onGet = async (id) => { + if (id !== 'config.json') return; + reads++; + await configManager.setAdminConfig({ loginShowVersion: ++n % 2 === 0 }); + }; + // The listener the change poller runs for a remote change. + const reload = (configManager as unknown as { reloadShared(area: string): Promise }).reloadShared('config'); + await expect(reload).rejects.toThrow(/kept changing/); + expect(reads).toBe(3); + }); + + it('keeps another replica\'s change that was reloaded while a save of this one was in flight', async () => { + hooks.afterUpdate = async () => { + const current = await stored('config.json'); + await otherReplicaSaves('config.json', { ...current, appName: 'Saved elsewhere' }); + await pollChanges(); + }; + await configManager.setAdminConfig({ loginShowVersion: false }); + expect(configManager.get('appName')).toBe('Saved elsewhere'); + expect(configManager.get('loginShowVersion')).toBe(false); + }); +}); + +describe('updateAdminConfig with a shared store', () => { + it('builds each of two saves on two replicas on the other\'s result', async () => { + const other = await otherReplicaManager(); + const add = (host: string) => (list: Array<{ host: string }>) => [...list, { host }]; + await Promise.all([ + configManager.updateAdminConfig('domainBranding', add('a.example'), []), + other.updateAdminConfig('domainBranding', add('b.example'), []), + ]); + const hosts = (await stored('config.json')).domainBranding.map((e: { host: string }) => e.host).sort(); + expect(hosts).toEqual(['a.example', 'b.example']); + }); + + it('starts from the value get() would give and leaves the config alone on undefined', async () => { + vi.stubEnv('APP_NAME', 'From env'); + expect(await configManager.updateAdminConfig('appName', (current) => `${current}!`)).toBe('First!'); + await configManager.removeAdminOverride('appName'); + expect(await configManager.updateAdminConfig('appName', (current) => `${current}!`)).toBe('From env!'); + const before = await adminConfigDocs().get('config.json'); + expect(await configManager.updateAdminConfig('appName', () => undefined)).toBeUndefined(); + expect((await adminConfigDocs().get('config.json'))!.version).toBe(before!.version); + }); +}); + describe('policy saves with a shared store', () => { + it('lets a save replace a stored policy that doesn\'t parse', async () => { + await adminConfigDocs().put('policy.json', '{ not json'); + await store.epochs.bump('policy'); + await pollChanges(); + const page = await load(); + expect(page.etag).toBe(`"${(await adminConfigDocs().get('policy.json'))!.version}"`); + expect((await save(page.policy, page.etag)).status).toBe(200); + expect(await stored('policy.json')).toMatchObject({ features: page.policy.features }); + + // The same after a restart. + await adminConfigDocs().put('policy.json', '{ not json'); + await configManager.reload(); + const again = await load(); + expect((await save(again.policy, again.etag)).status).toBe(200); + }); + + it('answers a save with the stored policy and its version', async () => { + const page = await load(); + const res = await save({ ...page.policy, features: { ...page.policy.features, calendarEnabled: false } }, page.etag); + const body = await res.json(); + expect(body.policy.features.calendarEnabled).toBe(false); + expect(body.policy.scopes).toEqual([]); + expect(res.headers.get('ETag')).toBe(`"${(await adminConfigDocs().get('policy.json'))!.version}"`); + }); + async function load() { const res = await GET(new NextRequest('https://mail.example/api/admin/policy')); return { policy: await res.json(), etag: res.headers.get('ETag')! }; diff --git a/lib/admin/config-manager.ts b/lib/admin/config-manager.ts index 5ffe686eb..1bb5d3f35 100644 --- a/lib/admin/config-manager.ts +++ b/lib/admin/config-manager.ts @@ -70,9 +70,12 @@ export function isPolicyConflict(error: unknown): error is PolicyConflictError { /** Policy writes in this process, one after another, so a version check and its write can't interleave. */ const POLICY_WRITE_QUEUE = 'admin-config\0policy.json'; +/** Config read-modify-writes (updateAdminConfig) in this process, one after another. */ +const CONFIG_WRITE_QUEUE = 'admin-config\0config.json'; interface ParsedDoc { - data: Record; + /** null when the stored document doesn't parse (shared mode). */ + data: Record | null; /** As the shared store computes it: changes whenever the bytes do. */ version: string; } @@ -81,9 +84,19 @@ function parseDoc(raw: string): ParsedDoc { return { data: JSON.parse(raw), version: versionOf(Buffer.from(raw, 'utf-8')) }; } -/** A document from the shared store; throws when it doesn't parse. */ -function parseStored(doc: StoredDoc | null): ParsedDoc | null { - return doc ? { data: JSON.parse(doc.data.toString('utf-8')), version: doc.version } : null; +/** + * A document from the shared store. Data that doesn't parse comes back as + * null with the document's version, so an If-Match save can still replace + * it. + */ +function parseStored(doc: StoredDoc | null, onError?: (error: unknown) => void): ParsedDoc | null { + if (!doc) return null; + try { + return { data: JSON.parse(doc.data.toString('utf-8')), version: doc.version }; + } catch (error) { + onError?.(error); + return { data: null, version: doc.version }; + } } /** Which stored document a change area reloads. */ @@ -113,6 +126,8 @@ class ConfigManager { private sharedLoad: Promise | null = null; /** Shared mode: counts writes applied from their result, so a reload that raced one reads again. */ private writes = 0; + /** Shared mode: counts reloads applied, so a write that raced one reads its result again. */ + private reloads = 0; private unlisten: (() => void) | null = null; /** @@ -159,12 +174,9 @@ class ConfigManager { /** As a broken file on disk: logged, and the defaults apply. */ private parseOrWarn(filename: string, doc: StoredDoc | null): ParsedDoc | null { - try { - return parseStored(doc); - } catch (error) { + return parseStored(doc, (error) => { logger.warn(`Failed to read ${filename} from the shared store`, { error: error instanceof Error ? error.message : 'Unknown error' }); - return null; - } + }); } /** Reload config or policy when another replica changed it. Registered again on every load. */ @@ -178,24 +190,53 @@ class ConfigManager { } /** - * Read one document again. Throws when the store can't be read or the - * document doesn't parse, keeping the copy in memory; the change poller - * then tries again. + * Read one document again. Throws when the store can't be read, or when + * writes of this process kept landing while reading; the change poller + * then tries again. A document that doesn't parse keeps the copy in + * memory (the policy takes its version, so a save can replace it). */ private async reloadShared(area: ConfigArea): Promise { for (let attempt = 0; attempt < 3; attempt++) { const writes = this.writes; - const doc = parseStored(await adminConfigDocs().get(AREA_FILES[area])); + const doc = this.parseOrWarn(AREA_FILES[area], await adminConfigDocs().get(AREA_FILES[area])); // A write of this process landed while reading: its result may be newer. if (writes !== this.writes) continue; - if (area === 'config') { - this.adminConfig = doc?.data || {}; - this.applyLoggerSettings(); - } else { - this.applyPolicyDoc(doc); - } + this.applyReloaded(area, doc); return; } + throw new Error(`${AREA_FILES[area]} kept changing while it was reloaded`); + } + + private applyReloaded(area: ConfigArea, doc: ParsedDoc | null): void { + this.reloads++; + if (doc && doc.data === null) { + if (area === 'policy') this.policyVersion = doc.version; + return; + } + if (area === 'config') { + this.adminConfig = doc?.data || {}; + this.applyLoggerSettings(); + } else { + this.applyPolicyDoc(doc); + } + } + + /** + * After a write of this process: apply its result, unless a reload of + * another replica's change was applied while it was in flight. That one + * may be newer than the write, so the document is read again instead. + */ + private async applyWritten(area: ConfigArea, reloadsBefore: number, apply: () => void): Promise { + this.writes++; + if (this.reloads === reloadsBefore) { + apply(); + return; + } + try { + this.applyReloaded(area, this.parseOrWarn(AREA_FILES[area], await adminConfigDocs().get(AREA_FILES[area]))); + } catch { + apply(); + } } /** @@ -203,8 +244,9 @@ class ConfigManager { * the object it gets), so only the keys a request names change and a * concurrent save of other keys on another replica stays. */ - private async updateSharedConfig(change: (config: Record) => void): Promise { + private async updateSharedConfig(change: (config: Record) => void | false): Promise { const inMemory = this.adminConfig; + const reloads = this.reloads; const result = await adminConfigDocs().update('config.json', (current) => { let base: Record = {}; if (current) { @@ -215,12 +257,14 @@ class ConfigManager { base = { ...inMemory }; } } - change(base); + if (change(base) === false) return undefined; return JSON.stringify(base, null, 2); }); - this.writes++; - if (result.data) this.adminConfig = JSON.parse(result.data.toString('utf-8')); - this.applyLoggerSettings(); + if (result.data === undefined) return; + await this.applyWritten('config', reloads, () => { + if (result.data) this.adminConfig = JSON.parse(result.data.toString('utf-8')); + this.applyLoggerSettings(); + }); await announceChange('config'); } @@ -298,12 +342,17 @@ class ConfigManager { // Synchronous callers can't wait: a due check runs in the background and // the next read sees its result. void this.checkForChanges(); + return ConfigManager.resolve(this.adminConfig, key, defaultValue); + } + + /** A value as get() reads it, from the admin overrides in `adminConfig`. */ + private static resolve(adminConfig: Record, key: string, defaultValue?: T): T { // Admin override (highest priority) const mapping = CONFIG_ENV_MAP[key]; - if (key in this.adminConfig) { - return this.adminConfig[key] as T; - } else if (mapping && mapping.fileKey && mapping.fileKey in this.adminConfig) { - const fileVal = readFileEnv(this.adminConfig[mapping.fileKey]); + if (key in adminConfig) { + return adminConfig[key] as T; + } else if (mapping && mapping.fileKey && mapping.fileKey in adminConfig) { + const fileVal = readFileEnv(adminConfig[mapping.fileKey]); if (fileVal !== null) { return parseEnvValue(fileVal, mapping.type) as T; } @@ -380,7 +429,11 @@ class ConfigManager { */ async setAdminConfig(updates: Record): Promise { assertWritable('update admin config'); - if (this.usesStore()) return this.updateSharedConfig((config) => Object.assign(config, updates)); + if (this.usesStore()) { + return this.updateSharedConfig((config) => { + Object.assign(config, updates); + }); + } // Start from what is on disk, so a save here doesn't undo a change another // replica saved a moment ago. Same before every write below. await this.reloadIfChanged(); @@ -389,6 +442,36 @@ class ConfigManager { this.applyLoggerSettings(); } + /** + * Set one admin override to `change(current)`, where `current` is the value + * as get() would return it from the stored config (override, else env, + * else `defaultValue`). For values built on the current one, such as a + * list with one entry changed: two saves at once, here or on another + * replica, each build on the other's result. Return undefined to leave + * it. Returns the value stored. + */ + async updateAdminConfig(key: string, change: (current: T) => T | undefined, defaultValue?: T): Promise { + assertWritable('update admin config'); + let stored: T | undefined; + if (this.usesStore()) { + await this.updateSharedConfig((config) => { + stored = change(ConfigManager.resolve(config, key, defaultValue)); + if (stored === undefined) return false; + config[key] = stored; + }); + return stored; + } + return serializeLocal(CONFIG_WRITE_QUEUE, async () => { + await this.reloadIfChanged(); + stored = change(ConfigManager.resolve(this.adminConfig, key, defaultValue)); + if (stored === undefined) return undefined; + this.adminConfig[key] = stored; + await this.writeJsonFile('config.json', this.adminConfig); + this.applyLoggerSettings(); + return stored; + }); + } + /** * Remove an admin override, reverting to env/default. */ @@ -446,6 +529,15 @@ class ConfigManager { return this.policyVersion ?? DEFAULT_POLICY_VERSION; } + /** + * The policy and its version, read together. Use this whenever both go + * out (a body with its ETag): an await between two separate reads lets a + * change from another replica pair an old policy with the new version. + */ + getPolicyWithVersion(): { policy: SettingsPolicy; version: string } { + return { policy: this.getPolicy(), version: this.getPolicyVersion() }; + } + /** * Update the settings policy. Writes to disk. With `ifVersion` (from * getPolicyVersion when the caller read the policy) it refuses with a @@ -477,6 +569,8 @@ class ConfigManager { assertWritable('update settings policy'); if (this.usesStore()) return this.updateSharedPolicy(change); await serializeLocal(POLICY_WRITE_QUEUE, async () => { + // Change what is on disk now, not what this process saw last. + await this.reloadIfChanged(); const next = change(this.policyCache); if (next === undefined) return; this.policyCache = ConfigManager.completePolicy(next); @@ -487,24 +581,29 @@ class ConfigManager { private async setSharedPolicy(policy: SettingsPolicy, ifVersion: string | undefined): Promise { const completed = ConfigManager.completePolicy(policy); const expected = ifVersion === DEFAULT_POLICY_VERSION ? null : ifVersion; + const reloads = this.reloads; const result = await adminConfigDocs().put( 'policy.json', JSON.stringify(completed, null, 2), expected === undefined ? {} : { ifVersion: expected }, ); - this.writes++; if (!result.ok) { + this.writes++; // Show what is stored now, so a retry or a reload compares with it. - this.applyPolicyDoc(this.parseOrWarn('policy.json', result.current)); + this.applyReloaded('policy', this.parseOrWarn('policy.json', result.current)); throw new PolicyConflictError(); } - this.policyCache = completed; - this.policyVersion = result.version; + const version = result.version; + await this.applyWritten('policy', reloads, () => { + this.policyCache = completed; + this.policyVersion = version; + }); await announceChange('policy'); - return result.version; + return version; } private async updateSharedPolicy(change: (current: SettingsPolicy) => SettingsPolicy | undefined): Promise { + const reloads = this.reloads; const result = await adminConfigDocs().update('policy.json', (current) => { const stored = current ? ConfigManager.completePolicy(JSON.parse(current.toString('utf-8')) as Partial) @@ -513,8 +612,9 @@ class ConfigManager { return next === undefined ? undefined : JSON.stringify(ConfigManager.completePolicy(next), null, 2); }); if (result.data === undefined) return; - this.writes++; - this.applyPolicyDoc(result.data && result.version ? parseStored({ data: result.data, version: result.version }) : null); + await this.applyWritten('policy', reloads, () => { + this.applyPolicyDoc(result.data && result.version ? parseStored({ data: result.data, version: result.version }) : null); + }); await announceChange('policy'); } From 6d22814e5f135d897cb188ce4f29ce44a4890e91 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Sat, 31 Oct 2026 23:57:08 +0100 Subject: [PATCH 19/31] fix: move settings and plugin credentials that no longer decrypt aside instead of refusing every save #1066 The earlier fix answered 503 to every read and write of a settings file or plugin vault that doesn't decrypt, so a rotated SESSION_SECRET left the user with no way out: even disconnecting a plugin failed. Now such a document reads as nothing stored, and the next write first moves it aside, then saves as usual. The bytes are kept unchanged, so the data can be restored with the old secret: - local mode renames the file to `.unreadable-