fix: carry the pad deletion token across movePad (#7995)

CHANGELOG.md

@@ -1,3 +1,9 @@
+# 3.3.4
+
+### Notable fixes
+
+- **API — `movePad` now carries the pad's deletion token to the new id (#7995).** `movePad` is implemented as `copy()` + `remove()`, but `Pad.copy()` only copies the `pad:<id>`, `:revs:N` and `:chat:N` records — never `pad:<id>:deletionToken` — and `remove()` then deleted the source pad's token. The renamed pad therefore had no token at all: the token the creator had been told to save no longer deleted anything, and because the copy keeps the same revision-0 author, their next visit tripped `createDeletionTokenIfAbsent()` and popped a second "save your pad deletion token" modal. The token record is now handed over to the destination as part of the move, so the saved token keeps working and the modal does not reappear. `force`-overwriting an existing destination discards that pad's own token along with its content. `copyPad` is deliberately unchanged — two pads sharing one secret would let a token saved for one delete the other.
+
 # 3.3.3
 
 3.3.3 is a security release. It closes a **critical unauthenticated arbitrary-file-read** in the `/static/*` handler (GHSA-mc8w-wjhw-45x5) and bundles the fixes for a batch of privately reported issues that had already landed on `develop`: an OpenID Connect provider hardcoded cookie key and permissive CORS reflection (GHSA-pp5v-mvwg-76mp), session-fixation on authentication (GHSA-73h9-c5xp-gfg4), a same-socket cross-pad write TOCTOU (GHSA-6mcx-x5h6-rpw2), and a pad-id delimiter injection in `copyPad`/`movePad` (GHSA-wg58-mhwv-35pq). Alongside the security work it migrates the server build to TypeScript 7 (`tsgo`), fixes PageDown/PageUp navigation across consecutive long wrapped lines, and makes the docker `plugin_packages` volume mountpoint writable.

doc/api/http_api.md

@@ -645,6 +645,13 @@ Note that all the revisions will be lost! In most of the cases one should use `c
 
 moves a pad. If force is true and the destination pad exists, it will be overwritten.
 
+A move is a rename, so the pad's `deletionToken` travels with it: the token
+issued for `sourceID` keeps working against `destinationID`, and the creator is
+not handed a second token when they open the renamed pad. When `force`
+overwrites an existing destination pad, that pad's own token is discarded along
+with its content. **copyPad** does not do this — a copy is a separate pad and
+gets its own token.
+
 *Example returns:*
 * `{code: 0, message:"ok", data: null}`
 * `{code: 1, message:"padID does not exist", data: null}`

src/node/db/API.ts

@@ -766,6 +766,11 @@ Example returns:
 exports.movePad = async (sourceID: string, destinationID: string, force:boolean) => {
   const pad = await getPadSafe(sourceID, true);
   await pad.copy(destinationID, force);
+  // A move is a rename, so the pad's deletion token travels with it: the token
+  // the creator saved keeps working, and returning to the renamed pad does not
+  // hand them a second one (issue #7995). Must run before remove(), which drops
+  // the source pad's token record.
+  await padDeletionManager.transferDeletionToken(sourceID, destinationID);
   await pad.remove();
 };
 

src/node/db/PadDeletionManager.ts

@@ -10,16 +10,27 @@ const getDeletionTokenKey = (padId: string) => `pad:${padId}:deletionToken`;
 const hashDeletionToken = (deletionToken: string) =>
   crypto.createHash('sha256').update(deletionToken, 'utf8').digest();
 
-// Per-pad serialisation for token creation. Without this, two concurrent
-// `createDeletionTokenIfAbsent()` calls for the same pad can both observe
-// an empty slot, both write a hash, and leave the earlier caller holding a
-// plaintext token that no longer validates. The chain is cleaned up once the
-// outstanding call resolves so this map doesn't grow unbounded.
-const inflightCreate: Map<string, Promise<string | null>> = new Map();
-
-exports.createDeletionTokenIfAbsent = async (padId: string): Promise<string | null> => {
-  const prior = inflightCreate.get(padId);
-  const next = (prior || Promise.resolve()).then(async () => {
+// Per-pad serialisation for the read-then-write token paths. Without this, two
+// concurrent `createDeletionTokenIfAbsent()` calls for the same pad can both
+// observe an empty slot, both write a hash, and leave the earlier caller holding
+// a plaintext token that no longer validates. `transferDeletionToken()` shares
+// the queue because it is the same read-then-write against the same slot. The
+// chain is cleaned up once the outstanding call resolves so this map doesn't
+// grow unbounded.
+const inflight: Map<string, Promise<any>> = new Map();
+
+const withPadTokenLock = <T>(padId: string, fn: () => Promise<T>): Promise<T> => {
+  const prior = inflight.get(padId);
+  const next = (prior || Promise.resolve()).then(fn);
+  const tracked = next.finally(() => {
+    if (inflight.get(padId) === tracked) inflight.delete(padId);
+  });
+  inflight.set(padId, tracked);
+  return next;
+};
+
+exports.createDeletionTokenIfAbsent = async (padId: string): Promise<string | null> =>
+  await withPadTokenLock(padId, async () => {
     if (await DB.db.get(getDeletionTokenKey(padId)) != null) return null;
     const deletionToken = randomString(32);
     await DB.db.set(getDeletionTokenKey(padId), {
@@ -28,12 +39,6 @@ exports.createDeletionTokenIfAbsent = async (padId: string): Promise<string | nu
     });
     return deletionToken;
   });
-  const tracked = next.finally(() => {
-    if (inflightCreate.get(padId) === tracked) inflightCreate.delete(padId);
-  });
-  inflightCreate.set(padId, tracked);
-  return next;
-};
 
 exports.isValidDeletionToken = async (padId: string, deletionToken: string | null | undefined) => {
   if (typeof deletionToken !== 'string' || deletionToken === '') return false;
@@ -44,5 +49,30 @@ exports.isValidDeletionToken = async (padId: string, deletionToken: string | nul
   return expected.length === actual.length && crypto.timingSafeEqual(expected, actual);
 };
 
+// Hand the token over to a renamed pad. A move is the same pad under a new id,
+// so the token the creator was told to save must keep working there — and they
+// must not be prompted to save a second one on arrival (issue #7995). Only
+// movePad uses this: copyPad's destination is a separate pad, and two pads
+// sharing one secret would let a token saved for one delete the other.
+//
+// The source record is deliberately left in place — the caller's `Pad.remove()`
+// drops it as part of the move. Removing it here would strand the creator of a
+// source pad that survives a failed `remove()`.
+exports.transferDeletionToken = async (srcPadId: string, dstPadId: string) =>
+  await withPadTokenLock(dstPadId, async () => {
+    const stored = await DB.db.get(getDeletionTokenKey(srcPadId));
+    // Nothing to hand over: the instance suppresses tokens, or the pad predates
+    // them. Leave the destination alone — a force-overwrite has already dropped
+    // the replaced pad's token via Pad.remove().
+    if (stored == null) return;
+    // Never clobber a token the destination has already handed out in plaintext.
+    // If the creator opened the new id in the window between Pad.copy() writing
+    // the pad records and this transfer, they were shown a freshly minted token
+    // and that one has to keep working; they simply keep it instead of the
+    // source's.
+    if (await DB.db.get(getDeletionTokenKey(dstPadId)) != null) return;
+    await DB.db.set(getDeletionTokenKey(dstPadId), stored);
+  });
+
 exports.removeDeletionToken = async (padId: string) =>
   await DB.db.remove(getDeletionTokenKey(padId));

src/tests/backend/specs/api/movePadDeletionToken.ts

@@ -0,0 +1,117 @@
+'use strict';
+
+import {strict as assert} from 'assert';
+
+const common = require('../../common');
+const padDeletionManager = require('../../../../node/db/PadDeletionManager');
+
+let agent: any;
+let apiVersion = 1;
+
+const endPoint = (p: string) => `/api/${apiVersion}/${p}`;
+
+const makeId = () => `movetok_${Date.now()}_${Math.random().toString(36).slice(2, 8)}`;
+
+const callApi = async (point: string, query: Record<string, string> = {}) => {
+  const qs = new URLSearchParams(query).toString();
+  const path = qs ? `${endPoint(point)}?${qs}` : endPoint(point);
+  return await agent.get(path)
+      .set('authorization', await common.generateJWTToken())
+      .expect(200)
+      .expect('Content-Type', /json/);
+};
+
+describe(__filename, function () {
+  before(async function () {
+    this.timeout(60000);
+    agent = await common.init();
+    const res = await agent.get('/api/').expect(200);
+    apiVersion = res.body.currentVersion;
+  });
+
+  it('movePad carries the deletionToken to the destination (issue #7995)', async function () {
+    const srcId = makeId();
+    const dstId = `${srcId}_dst`;
+    const create = await callApi('createPad', {padID: srcId});
+    const token = create.body.data.deletionToken;
+    assert.equal(typeof token, 'string');
+
+    const move = await callApi('movePad', {sourceID: srcId, destinationID: dstId});
+    assert.equal(move.body.code, 0, JSON.stringify(move.body));
+
+    const del = await callApi('deletePad', {padID: dstId, deletionToken: token});
+    assert.equal(del.body.code, 0, JSON.stringify(del.body));
+  });
+
+  it('the moved pad does not offer its creator a second token (issue #7995)', async function () {
+    const srcId = makeId();
+    const dstId = `${srcId}_dst`;
+    await callApi('createPad', {padID: srcId});
+    await callApi('movePad', {sourceID: srcId, destinationID: dstId});
+
+    // This is what the creator's next CLIENT_READY does. A non-null result here
+    // is exactly the second "save your pad deletion token" modal they reported.
+    assert.equal(await padDeletionManager.createDeletionTokenIfAbsent(dstId), null);
+
+    await callApi('deletePad', {padID: dstId});
+  });
+
+  it('movePad --force replaces the destination pad\'s own token', async function () {
+    const srcId = makeId();
+    const dstId = `${srcId}_existing`;
+    const src = await callApi('createPad', {padID: srcId});
+    const dst = await callApi('createPad', {padID: dstId});
+    const srcToken = src.body.data.deletionToken;
+    const dstToken = dst.body.data.deletionToken;
+    assert.notEqual(srcToken, dstToken);
+
+    await callApi('movePad', {sourceID: srcId, destinationID: dstId, force: 'true'});
+
+    // The overwritten pad's content is gone, so its old token must be too.
+    const stale = await callApi('deletePad', {padID: dstId, deletionToken: dstToken});
+    assert.equal(stale.body.code, 1, JSON.stringify(stale.body));
+    const del = await callApi('deletePad', {padID: dstId, deletionToken: srcToken});
+    assert.equal(del.body.code, 0, JSON.stringify(del.body));
+  });
+
+  it('the transfer never invalidates a token the destination already issued', async function () {
+    // Pad.copy() writes the destination records before movePad transfers the
+    // token, so the creator can open the new id in between and be shown a
+    // freshly minted token. That token has been handed out in plaintext, so the
+    // transfer must leave it alone rather than overwrite it.
+    const srcId = makeId();
+    const dstId = `${srcId}_raced`;
+    const src = await callApi('createPad', {padID: srcId});
+    const srcToken = src.body.data.deletionToken;
+    const dstToken = await padDeletionManager.createDeletionTokenIfAbsent(dstId);
+
+    await padDeletionManager.transferDeletionToken(srcId, dstId);
+
+    assert.equal(await padDeletionManager.isValidDeletionToken(dstId, dstToken), true);
+    assert.equal(await padDeletionManager.isValidDeletionToken(dstId, srcToken), false);
+    // The source token survives the transfer — Pad.remove() drops it when the
+    // move completes, so a move that fails midway leaves the source deletable.
+    assert.equal(await padDeletionManager.isValidDeletionToken(srcId, srcToken), true);
+
+    await padDeletionManager.removeDeletionToken(dstId);
+    await callApi('deletePad', {padID: srcId});
+  });
+
+  it('copyPad does NOT share the source deletionToken with the copy', async function () {
+    const srcId = makeId();
+    const dstId = `${srcId}_copy`;
+    const create = await callApi('createPad', {padID: srcId});
+    const token = create.body.data.deletionToken;
+
+    const copy = await callApi('copyPad', {sourceID: srcId, destinationID: dstId});
+    assert.equal(copy.body.code, 0, JSON.stringify(copy.body));
+
+    const del = await callApi('deletePad', {padID: dstId, deletionToken: token});
+    assert.equal(del.body.code, 1, JSON.stringify(del.body));
+    assert.match(del.body.message, /invalid deletionToken/);
+
+    // cleanup
+    await callApi('deletePad', {padID: srcId});
+    await callApi('deletePad', {padID: dstId});
+  });
+});