otp/capability/shortlink: atomic JSON persistence (temp + rename)

melvincarvalho · melvincarvalho · commit ca523733b390 · 2026-07-11T10:32:11.000+02:00
Audit finding: these tables were written with a direct
fs.writeFileSync, so a crash mid-write truncates the whole file and the
loader's loadJson swallows the parse error into {} — total loss, and for
capability a truncated revoked.json silently revives a revoked token.
Each plugin now writes to a unique temp file in the same dir and
renameSync's over the target (atomic on one fs); temp cleaned up on
failure, no stale .tmp left. Does NOT address the read-modify-write
TOCTOU (out of scope, noted). Regression tests assert valid on-disk JSON
and no leftover .tmp after a persisting op.
diff --git a/capability/plugin.js b/capability/plugin.js
@@ -51,6 +51,27 @@ const MIME = {
 };
 const mimeFor = (name) => MIME[path.extname(name).toLowerCase()] ?? 'application/octet-stream';
 
+// ------------------------------------------------------- durable write
+// Atomic persist: write to a uniquely-named temp file in the SAME directory
+// (rename is only atomic within one filesystem), then rename it over the
+// target. A crash mid-write truncates the throwaway temp, never the live
+// file — so a truncated REVOCATION LEDGER can no longer silently reset to {}
+// at boot and let a revoked capability come back to life. The random suffix
+// avoids concurrent-writer temp collisions; a failed rename unlinks the temp
+// so no partial file and no stale `.tmp` are left behind. (This does NOT
+// address the read-modify-write TOCTOU between concurrent persisters — a
+// separate, larger concern.)
+function atomicWrite(file, data) {
+  const tmp = `${file}.${crypto.randomBytes(6).toString('hex')}.tmp`;
+  try {
+    fs.writeFileSync(tmp, data);
+    fs.renameSync(tmp, file);
+  } catch (err) {
+    try { fs.unlinkSync(tmp); } catch { /* best-effort cleanup */ }
+    throw err;
+  }
+}
+
 // ---------------------------------------------------------------- token
 // Exported as pure functions so tests (and operators) can craft and check
 // tokens against a known secret without booting a server.
@@ -117,8 +138,8 @@ export async function activate(api) {
   const revoked = new Map(Object.entries(loadJson(revokedFile)).filter(([, exp]) => exp == null || exp > now));
   const issued = new Map(Object.entries(loadJson(issuedFile)).filter(([, v]) => v?.exp > now));
   const persist = () => {
-    fs.writeFileSync(revokedFile, JSON.stringify(Object.fromEntries(revoked)));
-    fs.writeFileSync(issuedFile, JSON.stringify(Object.fromEntries(issued)));
+    atomicWrite(revokedFile, JSON.stringify(Object.fromEntries(revoked)));
+    atomicWrite(issuedFile, JSON.stringify(Object.fromEntries(issued)));
   };
 
   // -------------------------------------------------------- resources
diff --git a/capability/test.js b/capability/test.js
@@ -193,4 +193,26 @@ describe('capability plugin', () => {
     assert.strictEqual(res.status, 200);
     assert.strictEqual((await fetch(`${jss.base}${cap.url}`)).status, 401);
   });
+
+  it('the revocation ledger persists atomically: valid JSON with the jti, no leftover .tmp', async () => {
+    const cap = await (await issue({ resource: 'report.pdf', ttl: 3600 }, alice.access_token)).json();
+    const revoke = await fetch(`${jss.base}/cap/revoke`, {
+      method: 'POST',
+      headers: { 'content-type': 'application/json', authorization: `Bearer ${alice.access_token}` },
+      body: JSON.stringify({ token: cap.token }),
+    });
+    assert.strictEqual(revoke.status, 200);
+
+    const pluginDir = path.join(jss.root, '.plugins', 'capability');
+
+    // The revocation ledger survived the write and parses. A truncated write
+    // would leave invalid JSON the boot loader swallows into {} — and a
+    // revoked capability would silently come back to life. That's the bug.
+    const revoked = JSON.parse(fs.readFileSync(path.join(pluginDir, 'revoked.json'), 'utf8'));
+    assert.ok(cap.jti in revoked, 'the revoked jti is on disk in the ledger');
+
+    // Neither ledger write may leave a temp file behind in the plugin dir.
+    const leftovers = fs.readdirSync(pluginDir).filter((f) => f.endsWith('.tmp'));
+    assert.deepStrictEqual(leftovers, [], `no leftover temp files, saw: ${leftovers}`);
+  });
 });
diff --git a/otp/plugin.js b/otp/plugin.js
@@ -122,6 +122,26 @@ function generateCode(len) {
   return s;
 }
 
+// ------------------------------------------------------- durable write
+// Atomic persist: write to a uniquely-named temp file in the SAME directory
+// (rename is only atomic within one filesystem), then rename it over the
+// target. A crash mid-write truncates the throwaway temp, never the live
+// table — so a truncated table can no longer silently reset to {} at boot
+// and re-open a consumed OTP row. The random suffix avoids concurrent-writer
+// temp collisions; a failed rename unlinks the temp so no partial file and
+// no stale `.tmp` are left behind. (This does NOT address the read-modify-
+// write TOCTOU between concurrent persisters — a separate, larger concern.)
+function atomicWrite(file, data) {
+  const tmp = `${file}.${crypto.randomBytes(6).toString('hex')}.tmp`;
+  try {
+    fs.writeFileSync(tmp, data);
+    fs.renameSync(tmp, file);
+  } catch (err) {
+    try { fs.unlinkSync(tmp); } catch { /* best-effort cleanup */ }
+    throw err;
+  }
+}
+
 export async function activate(api) {
   const prefix = api.prefix || '/otp';
   const cfg = api.config || {};
@@ -154,7 +174,7 @@ export async function activate(api) {
   // pruned at boot.
   const now0 = Math.floor(Date.now() / 1000);
   const table = new Map(Object.entries(loadJson(tableFile)).filter(([, e]) => e && e.exp > now0));
-  const persist = () => fs.writeFileSync(tableFile, JSON.stringify(Object.fromEntries(table)));
+  const persist = () => atomicWrite(tableFile, JSON.stringify(Object.fromEntries(table)));
   const keyFor = (identifier, purpose) => `${identifier}${NUL}${purpose}`;
 
   // ---------------------------------------------------- channel adapter
diff --git a/otp/test.js b/otp/test.js
@@ -6,6 +6,7 @@
 
 import { describe, it, before, after } from 'node:test';
 import assert from 'node:assert';
+import fs from 'node:fs';
 import path from 'node:path';
 import { fileURLToPath } from 'node:url';
 import { startJss } from '../helpers.js';
@@ -152,6 +153,28 @@ describe('otp plugin', () => {
     assert.strictEqual(statuses[3], 429, 'fourth request throttled');
   });
 
+  it('persistence is atomic: otp.json stays valid JSON with the row, no leftover .tmp', async () => {
+    const id = 'durable@example.com';
+    assert.strictEqual((await requestCode(id, 'session')).status, 201);
+
+    // pluginDir lives at <root>/.plugins/<id> (same as capability/'s secret).
+    const pluginDir = path.join(jss.root, '.plugins', 'otp');
+    const tableFile = path.join(pluginDir, 'otp.json');
+
+    // The whole table survived the write and parses — a truncated write would
+    // have left invalid JSON that the boot loader swallows into {} (the bug).
+    const table = JSON.parse(fs.readFileSync(tableFile, 'utf8'));
+    const rows = Object.values(table);
+    assert.ok(
+      rows.some((e) => e && e.identifier === id && e.purpose === 'session'),
+      'the just-minted OTP row is present on disk',
+    );
+
+    // The atomic write must not leave any temp file behind in the plugin dir.
+    const leftovers = fs.readdirSync(pluginDir).filter((f) => f.endsWith('.tmp'));
+    assert.deepStrictEqual(leftovers, [], `no leftover temp files, saw: ${leftovers}`);
+  });
+
   it('a tampered or missing session token is refused at whoami', async () => {
     const id = 'tamper@example.com';
     await requestCode(id, 'session');
diff --git a/shortlink/plugin.js b/shortlink/plugin.js
@@ -63,6 +63,39 @@ export function randomSlug(len = 7) {
   return s;
 }
 
+// ------------------------------------------------------- durable write
+// Atomic persist: write to a uniquely-named temp file in the SAME directory
+// (rename is only atomic within one filesystem), then rename it over the
+// target. A crash mid-write truncates the throwaway temp, never the live
+// links table — so a truncated table can no longer silently reset to {} at
+// boot and lose every link. The random suffix avoids concurrent-writer temp
+// collisions; a failed rename unlinks the temp so no partial file and no
+// stale `.tmp` are left behind. Both the sync (create/delete) and async
+// (hit-count flush) writers go through this — a crashed hit-count flush must
+// not truncate the table either. (This does NOT address the read-modify-
+// write TOCTOU between concurrent persisters — a separate, larger concern.)
+function atomicWriteSync(file, data) {
+  const tmp = `${file}.${crypto.randomBytes(6).toString('hex')}.tmp`;
+  try {
+    fs.writeFileSync(tmp, data);
+    fs.renameSync(tmp, file);
+  } catch (err) {
+    try { fs.unlinkSync(tmp); } catch { /* best-effort cleanup */ }
+    throw err;
+  }
+}
+
+async function atomicWriteAsync(file, data) {
+  const tmp = `${file}.${crypto.randomBytes(6).toString('hex')}.tmp`;
+  try {
+    await fsp.writeFile(tmp, data);
+    await fsp.rename(tmp, file);
+  } catch (err) {
+    try { await fsp.unlink(tmp); } catch { /* best-effort cleanup */ }
+    throw err;
+  }
+}
+
 export async function activate(api) {
   const prefix = api.prefix || '/shortlink';
   const cfg = api.config || {};
@@ -102,7 +135,7 @@ export async function activate(api) {
   };
   const links = new Map(Object.entries(loadJson(tableFile)));
 
-  const persist = () => fs.writeFileSync(tableFile, JSON.stringify(Object.fromEntries(links)));
+  const persist = () => atomicWriteSync(tableFile, JSON.stringify(Object.fromEntries(links)));
 
   // Fire-and-forget flush for hit counts: serialized on a promise chain so
   // concurrent redirects never interleave writes to the same file. A crash
@@ -111,7 +144,7 @@ export async function activate(api) {
   let pending = Promise.resolve();
   const persistLazy = () => {
     pending = pending
-      .then(() => fsp.writeFile(tableFile, JSON.stringify(Object.fromEntries(links))))
+      .then(() => atomicWriteAsync(tableFile, JSON.stringify(Object.fromEntries(links))))
       .catch((err) => api.log.warn(`shortlink: hit-count flush failed: ${err.message}`));
   };
 
diff --git a/shortlink/test.js b/shortlink/test.js
@@ -8,6 +8,7 @@
 
 import { describe, it, after } from 'node:test';
 import assert from 'node:assert';
+import fs from 'node:fs';
 import path from 'node:path';
 import { fileURLToPath } from 'node:url';
 import { probePort, startJss } from '../helpers.js';
@@ -192,6 +193,24 @@ describe('shortlink plugin', () => {
     assert.strictEqual(mine.find((l) => l.slug === slug).hits, 2);
   });
 
+  it('persistence is atomic: links.json stays valid JSON with the record, no leftover .tmp', async () => {
+    const res = await post(alice, { url: `${base}/alice/durable/thing.ttl`, slug: 'durable-link' });
+    assert.strictEqual(res.status, 201);
+
+    const pluginDir = path.join(jss.root, '.plugins', 'shortlink');
+    const tableFile = path.join(pluginDir, 'links.json');
+
+    // The table survived the write and parses. A truncated write would leave
+    // invalid JSON the boot loader swallows into {} — losing every link.
+    const table = JSON.parse(fs.readFileSync(tableFile, 'utf8'));
+    assert.ok(table['durable-link'], 'the just-created link is present on disk');
+    assert.strictEqual(table['durable-link'].url, `${base}/alice/durable/thing.ttl`);
+
+    // The atomic write must not leave any temp file behind in the plugin dir.
+    const leftovers = fs.readdirSync(pluginDir).filter((f) => f.endsWith('.tmp'));
+    assert.deepStrictEqual(leftovers, [], `no leftover temp files, saw: ${leftovers}`);
+  });
+
   it('unknown slug → 404, even one far past maxParamLength (wildcard route)', async () => {
     assert.strictEqual((await follow('n0such1')).status, 404);
     const long = 'x'.repeat(150); // a named :slug param would 404 at the router with Fastify's default body
