From 97acec05068e8a7a31496628630b238bb7de6afa Mon Sep 17 00:00:00 2001 From: John McLear Date: Tue, 4 Aug 2026 11:41:56 +0100 Subject: [PATCH 1/2] fix: carry the pad deletion token across movePad (#7995) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `movePad` is `Pad.copy()` + `Pad.remove()`, but `copy()` only replicates the `pad:`, `:revs:N` and `:chat:N` records — never `pad::deletionToken` — and `remove()` then dropped the source pad's token. So a renamed pad had no token at all: the token the creator was told to save stopped deleting anything, and because the copy preserves the revision-0 author the creator is still `isCreator` on arrival, so `createDeletionTokenIfAbsent()` minted a fresh one and popped a second "save your pad deletion token" modal. Hand the token record 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` deliberately does not do this: two pads sharing one secret would let a token saved for one delete the other. Adds backend coverage for all four cases (three of them fail on the pre-fix code) plus http_api.md and CHANGELOG notes. Reported by @dcht00. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 6 ++ doc/api/http_api.md | 7 ++ src/node/db/API.ts | 5 + src/node/db/PadDeletionManager.ts | 18 ++++ .../backend/specs/api/movePadDeletionToken.ts | 94 +++++++++++++++++++ 5 files changed, 130 insertions(+) create mode 100644 src/tests/backend/specs/api/movePadDeletionToken.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 01be3e86cb9..df5624d9ba9 100644 --- a/CHANGELOG.md +++ b/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:`, `:revs:N` and `:chat:N` records — never `pad::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. diff --git a/doc/api/http_api.md b/doc/api/http_api.md index 7167754b9e9..4e31c70610b 100644 --- a/doc/api/http_api.md +++ b/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}` diff --git a/src/node/db/API.ts b/src/node/db/API.ts index b79975b6c69..be2844d18dc 100644 --- a/src/node/db/API.ts +++ b/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(); }; diff --git a/src/node/db/PadDeletionManager.ts b/src/node/db/PadDeletionManager.ts index e37a240b6da..3a13c65b340 100644 --- a/src/node/db/PadDeletionManager.ts +++ b/src/node/db/PadDeletionManager.ts @@ -44,5 +44,23 @@ 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). +// Deliberately a move and not a copy: two pads sharing one secret would let a +// token saved for one of them delete the other, which is why copyPad does not +// use this. +exports.transferDeletionToken = async (srcPadId: string, dstPadId: string) => { + const stored = await DB.db.get(getDeletionTokenKey(srcPadId)); + if (stored == null) { + // The destination pad is being replaced wholesale, so any token it still + // carries belongs to content that no longer exists. + await DB.db.remove(getDeletionTokenKey(dstPadId)); + return; + } + await DB.db.set(getDeletionTokenKey(dstPadId), stored); + await DB.db.remove(getDeletionTokenKey(srcPadId)); +}; + exports.removeDeletionToken = async (padId: string) => await DB.db.remove(getDeletionTokenKey(padId)); diff --git a/src/tests/backend/specs/api/movePadDeletionToken.ts b/src/tests/backend/specs/api/movePadDeletionToken.ts new file mode 100644 index 00000000000..f804b45217e --- /dev/null +++ b/src/tests/backend/specs/api/movePadDeletionToken.ts @@ -0,0 +1,94 @@ +'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 = {}) => { + 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('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}); + }); +}); From ea49ba8bcf0c0823a5ed37ae92c61fc46789fe2d Mon Sep 17 00:00:00 2001 From: John McLear Date: Tue, 4 Aug 2026 12:45:52 +0100 Subject: [PATCH 2/2] fix: harden the deletion-token transfer against partial moves and races MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review feedback on #8089: - Don't remove the source token in `transferDeletionToken()`. `movePad` calls it before `pad.remove()`, which drops the record itself on success — so the eager removal only mattered when `remove()` failed, and there it stranded the creator of a source pad that survived. - Don't overwrite a token the destination has already issued. `Pad.copy()` writes the destination records before the transfer runs, so a creator opening the new id in that window is shown a freshly minted token; that token has been handed out in plaintext and has to keep working. - Run the transfer through the same per-pad queue as `createDeletionTokenIfAbsent()` (extracted as `withPadTokenLock`), so a concurrent mint can't interleave with the transfer's read-then-write. - Drop the "source has no token" branch that cleared the destination's: a force-overwrite already drops the replaced pad's token via `Pad.remove()`, so the branch could only clobber a live token. Co-Authored-By: Claude Opus 5 (1M context) --- src/node/db/PadDeletionManager.ts | 72 +++++++++++-------- .../backend/specs/api/movePadDeletionToken.ts | 23 ++++++ 2 files changed, 65 insertions(+), 30 deletions(-) diff --git a/src/node/db/PadDeletionManager.ts b/src/node/db/PadDeletionManager.ts index 3a13c65b340..0c2b0c00aab 100644 --- a/src/node/db/PadDeletionManager.ts +++ b/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> = new Map(); +// 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> = new Map(); -exports.createDeletionTokenIfAbsent = async (padId: string): Promise => { - const prior = inflightCreate.get(padId); - const next = (prior || Promise.resolve()).then(async () => { +const withPadTokenLock = (padId: string, fn: () => Promise): Promise => { + 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 => + 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 { - 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; @@ -46,21 +51,28 @@ exports.isValidDeletionToken = async (padId: string, deletionToken: string | nul // 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). -// Deliberately a move and not a copy: two pads sharing one secret would let a -// token saved for one of them delete the other, which is why copyPad does not -// use this. -exports.transferDeletionToken = async (srcPadId: string, dstPadId: string) => { - const stored = await DB.db.get(getDeletionTokenKey(srcPadId)); - if (stored == null) { - // The destination pad is being replaced wholesale, so any token it still - // carries belongs to content that no longer exists. - await DB.db.remove(getDeletionTokenKey(dstPadId)); - return; - } - await DB.db.set(getDeletionTokenKey(dstPadId), stored); - await DB.db.remove(getDeletionTokenKey(srcPadId)); -}; +// 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)); diff --git a/src/tests/backend/specs/api/movePadDeletionToken.ts b/src/tests/backend/specs/api/movePadDeletionToken.ts index f804b45217e..57889283db4 100644 --- a/src/tests/backend/specs/api/movePadDeletionToken.ts +++ b/src/tests/backend/specs/api/movePadDeletionToken.ts @@ -74,6 +74,29 @@ describe(__filename, function () { 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`;