Harden JSON-backed persistence: atomic writes + per-path serialization

backend/src/services/__tests__/apiTokenService.test.ts

@@ -8,6 +8,7 @@ import os from 'os';
 import path from 'path';
 import crypto from 'crypto';
 import { ApiTokenService } from '../apiTokenService';
+import { flushFileLock } from '../../utils/atomicJson';
 
 const sha256hex = (raw: string): string =>
   crypto.createHash('sha256').update(raw).digest('hex');
@@ -26,6 +27,9 @@ describe('ApiTokenService', () => {
 
   afterEach(async () => {
     jest.restoreAllMocks();
+    // verifyToken persists its lastUsedAt touch fire-and-forget; drain any
+    // in-flight atomic write so its temp file cannot race the directory removal.
+    await flushFileLock(filePath);
     await fs.rm(dir, { recursive: true, force: true });
   });
 

backend/src/services/apiTokenService.ts

@@ -12,6 +12,7 @@
 import { promises as fs } from 'fs';
 import path from 'path';
 import crypto from 'crypto';
+import { writeJsonAtomic } from '../utils/atomicJson';
 
 const TOKENS_FILE = path.join(process.cwd(), 'data', 'api-tokens.json');
 
@@ -107,7 +108,10 @@ export class ApiTokenService {
    */
   private async save(): Promise<void> {
     const arr = Array.from(this.tokens.values());
-    await fs.writeFile(this.filePath, JSON.stringify(arr, null, 2), 'utf-8');
+    // Atomic + per-path serialized write. This matters most for the throttled,
+    // fire-and-forget lastUsedAt touch in verifyToken(), which can otherwise
+    // race a create/revoke and lose an update.
+    await writeJsonAtomic(this.filePath, arr);
   }
 
   /**

backend/src/services/backupService.ts

@@ -4,6 +4,7 @@
 import { promises as fs } from 'fs';
 import path from 'path';
 import { BackupConfig, resolveBackupConfig } from '../config/backups';
+import { writeJsonAtomic, removeFileLocked } from '../utils/atomicJson';
 
 const MAX_BACKUPS_PER_ZONE = 50;
 
@@ -276,14 +277,20 @@ export class BackupService {
     }
   }
 
-  /** Write changed zone files; remove any that were emptied by eviction. */
+  /**
+   * Write changed zone files; remove any that were emptied by eviction. Each
+   * write is atomic (temp file + rename) and each file's write/remove is
+   * serialized against other writers of that same path, so a crash cannot
+   * truncate a zone file and a concurrent createBackup for another zone cannot
+   * clobber this one. Distinct files still persist concurrently.
+   */
   private async persistFiles(files: Map<string, DNSBackup[]>, changed: Set<string>): Promise<void> {
     for (const file of changed) {
       const arr = files.get(file) ?? [];
       if (arr.length === 0) {
-        await fs.rm(file, { force: true });
+        await removeFileLocked(file);
       } else {
-        await fs.writeFile(file, JSON.stringify(arr, null, 2), 'utf-8');
+        await writeJsonAtomic(file, arr);
       }
     }
   }
@@ -313,8 +320,8 @@ export class BackupService {
       // Remove the backup
       backups = backups.filter(b => b.id !== backupId);
 
-      // Save updated list
-      await fs.writeFile(filePath, JSON.stringify(backups, null, 2), 'utf-8');
+      // Save updated list atomically and serialized against other writers.
+      await writeJsonAtomic(filePath, backups);
 
       console.log(`Backup deleted: ${backupId} from zone ${zone} by user ${userId}`);
     } catch (error) {

backend/src/services/ssoConfigService.ts

@@ -3,6 +3,7 @@ import { promises as fs } from 'fs';
 import path from 'path';
 import crypto from 'crypto';
 import { SSOConfig, SSOConfigResponse, SSOProvider } from '../types/sso';
+import { writeJsonAtomic } from '../utils/atomicJson';
 
 const SSO_CONFIG_FILE = path.join(process.cwd(), 'data', 'sso-config.json');
 const ENCRYPTION_KEY = process.env.ENCRYPTION_KEY || 'default-encryption-key-change-in-production';
@@ -99,8 +100,8 @@ class SSOConfigService {
       configToStore.clientSecret = this.encrypt(configToStore.clientSecret);
     }
 
-    // Save to file
-    await fs.writeFile(SSO_CONFIG_FILE, JSON.stringify(configToStore, null, 2), 'utf-8');
+    // Save to file atomically and serialized against other writers.
+    await writeJsonAtomic(SSO_CONFIG_FILE, configToStore);
 
     console.log(`SSO config updated: ${this.config.enabled ? 'enabled' : 'disabled'} (${this.config.provider})`);
   }

backend/src/services/tsigKeyService.ts

@@ -5,6 +5,7 @@ import { promises as fs } from 'fs';
 import path from 'path';
 import crypto from 'crypto';
 import { resolveTsigEncryptionKey } from '../config/secrets';
+import { writeJsonAtomic } from '../utils/atomicJson';
 
 const KEYS_FILE = path.join(process.cwd(), 'data', 'tsig-keys.json');
 
@@ -122,7 +123,9 @@ class TSIGKeyService {
    */
   private async saveKeys(): Promise<void> {
     const keysArray = Array.from(this.keys.values());
-    await fs.writeFile(KEYS_FILE, JSON.stringify(keysArray, null, 2), 'utf-8');
+    // Atomic + per-path serialized write so a crash cannot truncate the file and
+    // concurrent callers cannot clobber each other.
+    await writeJsonAtomic(KEYS_FILE, keysArray);
   }
 
   /**

backend/src/services/userService.ts

@@ -3,6 +3,7 @@ import bcrypt from 'bcrypt';
 import { promises as fs } from 'fs';
 import path from 'path';
 import { User, UserCreateData, UserResponse, UserRole } from '../types/auth';
+import { writeJsonAtomic } from '../utils/atomicJson';
 
 const SALT_ROUNDS = 12;
 const USERS_FILE = path.join(process.cwd(), 'data', 'users.json');
@@ -127,7 +128,10 @@ class UserService {
   private async saveUsers(): Promise<void> {
     try {
       const usersArray = Array.from(this.users.values());
-      await fs.writeFile(USERS_FILE, JSON.stringify(usersArray, null, 2), 'utf-8');
+      // Atomic + per-path serialized write so a crash cannot truncate the file
+      // and concurrent saves (e.g. a login touching lastLogin while an admin
+      // edits a user) cannot clobber each other.
+      await writeJsonAtomic(USERS_FILE, usersArray);
     } catch (error) {
       console.error('Failed to save users:', error);
       throw error;

backend/src/services/webhookConfigService.ts

@@ -3,6 +3,7 @@
 
 import { promises as fs } from 'fs';
 import path from 'path';
+import { writeJsonAtomic } from '../utils/atomicJson';
 
 const WEBHOOK_CONFIG_FILE = path.join(process.cwd(), 'data', 'webhook-configs.json');
 
@@ -59,7 +60,8 @@ class WebhookConfigService {
    */
   private async saveConfigs(): Promise<void> {
     const configsArray = Array.from(this.configs.values());
-    await fs.writeFile(WEBHOOK_CONFIG_FILE, JSON.stringify(configsArray, null, 2), 'utf-8');
+    // Atomic + per-path serialized write (see utils/atomicJson).
+    await writeJsonAtomic(WEBHOOK_CONFIG_FILE, configsArray);
   }
 
   /**

backend/src/utils/__tests__/atomicJson.test.ts

@@ -0,0 +1,122 @@
+// backend/src/utils/__tests__/atomicJson.test.ts
+// Verifies the two guarantees of the shared JSON persistence helper:
+//  1. Crash safety: a write failure never leaves the target truncated/corrupt —
+//     the temp file is renamed into place, so the target is only ever complete
+//     JSON (or its previous complete contents).
+//  2. Serialization: many concurrent writes to the same path apply one at a time
+//     in call order, so the last write wins and no update is lost or interleaved.
+
+import { promises as fs } from 'fs';
+import os from 'os';
+import path from 'path';
+import { writeJsonAtomic, writeFileAtomic, removeFileLocked, withFileLock } from '../atomicJson';
+
+describe('atomicJson', () => {
+  let dir: string;
+  let target: string;
+
+  beforeEach(async () => {
+    dir = await fs.mkdtemp(path.join(os.tmpdir(), 'snap-atomic-'));
+    target = path.join(dir, 'data.json');
+  });
+
+  afterEach(async () => {
+    jest.restoreAllMocks();
+    await fs.rm(dir, { recursive: true, force: true });
+  });
+
+  it('writes via a temp file and renames over the target', async () => {
+    const renameSpy = jest.spyOn(fs, 'rename');
+
+    await writeJsonAtomic(target, { hello: 'world' });
+
+    // The final write must be a rename onto the target, not an in-place write.
+    expect(renameSpy).toHaveBeenCalledTimes(1);
+    const [from, to] = renameSpy.mock.calls[0];
+    expect(to).toBe(target);
+    expect(from).not.toBe(target);
+
+    const parsed = JSON.parse(await fs.readFile(target, 'utf-8'));
+    expect(parsed).toEqual({ hello: 'world' });
+  });
+
+  it('leaves an existing target intact and drops the temp file when the write fails', async () => {
+    // Seed a valid target so we can prove it survives a failed overwrite.
+    await writeJsonAtomic(target, { good: true });
+
+    // Simulate a crash mid-rename (data already streamed to temp, rename fails).
+    jest.spyOn(fs, 'rename').mockRejectedValueOnce(new Error('simulated crash'));
+
+    await expect(writeJsonAtomic(target, { good: false })).rejects.toThrow('simulated crash');
+
+    // Target still parses and still holds the previous complete contents —
+    // never a truncated/partial file.
+    const parsed = JSON.parse(await fs.readFile(target, 'utf-8'));
+    expect(parsed).toEqual({ good: true });
+
+    // No stray temp files left behind in the directory.
+    const leftovers = (await fs.readdir(dir)).filter(n => n.endsWith('.tmp'));
+    expect(leftovers).toEqual([]);
+  });
+
+  it('creates a fresh target atomically even when none exists', async () => {
+    await writeFileAtomic(target, 'plain-contents');
+    expect(await fs.readFile(target, 'utf-8')).toBe('plain-contents');
+  });
+
+  it('serializes many concurrent writes to the same path with no lost update', async () => {
+    // Fire 50 overlapping writes at the same file. Without serialization their
+    // temp-file writes and renames would interleave; with it, the last enqueued
+    // write wins and the file is always complete JSON.
+    const count = 50;
+    const writes = Array.from({ length: count }, (_, i) => writeJsonAtomic(target, { seq: i }));
+    await Promise.all(writes);
+
+    const parsed = JSON.parse(await fs.readFile(target, 'utf-8'));
+    // The final enqueued write (highest seq) is the durable one.
+    expect(parsed).toEqual({ seq: count - 1 });
+  });
+
+  it('runs same-path tasks in enqueue order and never overlaps them', async () => {
+    const events: string[] = [];
+    const gated = (id: number, delayMs: number) =>
+      withFileLock(target, async () => {
+        events.push(`start-${id}`);
+        await new Promise(r => setTimeout(r, delayMs));
+        events.push(`end-${id}`);
+      });
+
+    // First task is slowest; if tasks overlapped, its end would come after a
+    // later task's start. Serialization forces strict start/end pairing in order.
+    await Promise.all([gated(0, 30), gated(1, 5), gated(2, 5)]);
+
+    expect(events).toEqual([
+      'start-0', 'end-0',
+      'start-1', 'end-1',
+      'start-2', 'end-2',
+    ]);
+  });
+
+  it('does not wedge the queue when one task rejects', async () => {
+    const results: string[] = [];
+    const ok1 = withFileLock(target, async () => { results.push('a'); });
+    const boom = withFileLock(target, async () => { throw new Error('boom'); });
+    const ok2 = withFileLock(target, async () => { results.push('c'); });
+
+    await ok1;
+    await expect(boom).rejects.toThrow('boom');
+    await ok2; // later caller still runs despite the middle rejection
+
+    expect(results).toEqual(['a', 'c']);
+  });
+
+  it('removes a file under the same lock, serialized against writes', async () => {
+    await writeJsonAtomic(target, { x: 1 });
+    // Enqueue a write then a remove; the remove must win as the last operation.
+    const w = writeJsonAtomic(target, { x: 2 });
+    const r = removeFileLocked(target);
+    await Promise.all([w, r]);
+
+    await expect(fs.readFile(target, 'utf-8')).rejects.toMatchObject({ code: 'ENOENT' });
+  });
+});

backend/src/utils/atomicJson.ts

@@ -0,0 +1,117 @@
+// backend/src/utils/atomicJson.ts
+// Crash-safe, race-free JSON persistence shared by the JSON-backed services.
+//
+// Two hazards this addresses:
+//  1. Torn writes. A plain fs.writeFile truncates the target and streams bytes
+//     in place, so a crash mid-write leaves a truncated/corrupt file that fails
+//     to parse on the next start. We instead write a sibling temp file and
+//     fs.rename() it over the target: rename is atomic on the same filesystem,
+//     so a reader ever only observes the old complete file or the new complete
+//     file, never a partial one.
+//  2. Lost updates. The services do read-modify-write against in-memory state
+//     and then persist. Two overlapping writers (e.g. a fire-and-forget
+//     "lastUsedAt" touch racing a create) can interleave their rename calls and
+//     clobber each other. We serialize all writes to a given path through a
+//     per-path promise chain so they apply one at a time in call order.
+
+import { promises as fs } from 'fs';
+import path from 'path';
+import crypto from 'crypto';
+
+// One in-flight promise chain per absolute file path. Each new operation is
+// appended to the tail so callers targeting the same file run sequentially;
+// operations on different files stay fully concurrent.
+const locks = new Map<string, Promise<void>>();
+
+/**
+ * Run `task` with exclusive access to `filePath` relative to every other caller
+ * that goes through this helper. Tasks for the same resolved path execute in the
+ * order they were enqueued; a task that rejects does not break the chain for
+ * later callers. The map entry is dropped once the chain drains so it cannot
+ * grow without bound.
+ */
+export function withFileLock<T>(filePath: string, task: () => Promise<T>): Promise<T> {
+  const key = path.resolve(filePath);
+  const prior = locks.get(key) ?? Promise.resolve();
+
+  // Chain after any in-flight work regardless of whether it settled ok or not.
+  const result = prior.then(task, task);
+
+  // The stored tail must always resolve so one caller's failure never wedges the
+  // queue for the next caller.
+  const tail = result.then(
+    () => undefined,
+    () => undefined,
+  );
+  locks.set(key, tail);
+
+  // Best-effort cleanup: only clear the entry if nothing newer replaced it.
+  void tail.then(() => {
+    if (locks.get(key) === tail) locks.delete(key);
+  });
+
+  return result;
+}
+
+/**
+ * Resolve once all currently-enqueued work for `filePath` has drained. Useful
+ * for graceful shutdown and for tests that trigger fire-and-forget writes (e.g.
+ * apiTokenService's throttled lastUsedAt touch) and must wait for them to hit
+ * disk before tearing the directory down.
+ */
+export function flushFileLock(filePath: string): Promise<void> {
+  const key = path.resolve(filePath);
+  const prior = locks.get(key);
+  return prior ? prior.then(() => undefined, () => undefined) : Promise.resolve();
+}
+
+/**
+ * Atomically replace `filePath` with `contents`: write to a uniquely-named temp
+ * file in the same directory, fsync it so the bytes are durable, then rename
+ * over the target. The temp file is cleaned up on failure. Callers should
+ * normally use writeJsonAtomic; this is exposed for non-JSON payloads.
+ */
+export async function writeFileAtomic(filePath: string, contents: string): Promise<void> {
+  const dir = path.dirname(filePath);
+  const tmp = path.join(
+    dir,
+    `.${path.basename(filePath)}.${process.pid}.${crypto.randomBytes(6).toString('hex')}.tmp`,
+  );
+
+  try {
+    // Open, write and fsync via a handle so the temp file is fully on disk
+    // before it becomes visible under the target name.
+    const handle = await fs.open(tmp, 'w');
+    try {
+      await handle.writeFile(contents, 'utf-8');
+      await handle.sync();
+    } finally {
+      await handle.close();
+    }
+
+    await fs.rename(tmp, filePath);
+  } catch (error) {
+    // Never leave a stray temp file behind on a failed write.
+    await fs.rm(tmp, { force: true }).catch(() => undefined);
+    throw error;
+  }
+}
+
+/**
+ * Serialize `value` as pretty-printed JSON (matching the services' existing
+ * on-disk shape) and write it atomically, serialized against other writers of
+ * the same path.
+ */
+export function writeJsonAtomic(filePath: string, value: unknown): Promise<void> {
+  const contents = JSON.stringify(value, null, 2);
+  return withFileLock(filePath, () => writeFileAtomic(filePath, contents));
+}
+
+/**
+ * Remove `filePath` (no error if absent), serialized against writers of the same
+ * path so a delete and a concurrent write cannot interleave. Used by
+ * backupService when eviction empties a zone file.
+ */
+export function removeFileLocked(filePath: string): Promise<void> {
+  return withFileLock(filePath, () => fs.rm(filePath, { force: true }));
+}