From d7bcb0d63f5bd9fd9c57c1a0bac4aeb4d5cf3b7e Mon Sep 17 00:00:00 2001 From: John McLear Date: Tue, 9 Jun 2026 14:29:26 +0100 Subject: [PATCH 1/2] feat(pad): suppress deletion token for durable identities; relabel recovery action (#7926) Builds on the allowPadDeletionByAllUsers suppression with the rest of the ideas discussed on the issue. Server (handleClientReady): - A creator's deletion token is now also withheld when they have a *durable* identity: authenticated (req.session.user with a username) AND the deployment pins that identity to a stable authorID via a getAuthorId hook. Only then does the creator path (author === revision-0 author) survive a cookie clear or a different device, making the recovery token redundant. - This deliberately tightens the previous `requireAuthentication => always suppress` rule: without a getAuthorId hook the authorID still comes from the per-browser token cookie, so an authenticated user on a second device is NOT the creator. Withholding the token there would strand them, so they now keep getting one. SSO deployments using the documented getAuthorId pattern get the clean no-modal experience. - New `canDeleteWithoutToken` clientVar (allowPadDeletionByAllUsers OR durable identity) drives the client label. Client (pad_editor.ts): - When canDeleteWithoutToken, the recovery-token disclosure summary is labelled plainly "Delete Pad" (reusing the already-translated pad.settings.deletePad key) instead of the jargon "Delete with token". Tests (socketio.ts): anonymous -> token; allowPadDeletionByAllUsers -> none; authenticated without a getAuthorId hook -> token; authenticated with one -> none. Verified in a browser for both label/modal outcomes. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/node/handler/PadMessageHandler.ts | 35 ++++++++++++++++---- src/static/js/pad_editor.ts | 11 +++++++ src/tests/backend/specs/socketio.ts | 47 +++++++++++++++++++++++++++ 3 files changed, 86 insertions(+), 7 deletions(-) diff --git a/src/node/handler/PadMessageHandler.ts b/src/node/handler/PadMessageHandler.ts index 0bf559dfdd8..ab813d737b1 100644 --- a/src/node/handler/PadMessageHandler.ts +++ b/src/node/handler/PadMessageHandler.ts @@ -1287,15 +1287,31 @@ const handleClientReady = async (socket:any, message: ClientReadyMessage) => { // once. Readonly sessions never see it. const isCreator = !sessionInfo.readonly && sessionInfo.author === await pad.getRevisionAuthor(0); - // Skip token issuance — and so the client never shows the "Save your pad - // deletion token" modal (issue #7926) — when the token cannot help: - // - requireAuthentication: every creator already has a stable identity, so - // the cookie/identity path is sufficient. + // The deletion token is a recovery handle for the one class of creator that + // can otherwise lose the ability to delete their pad: a user whose creator + // status lives only in a per-browser author-token cookie. It is pointless — + // and the "Save your pad deletion token" modal only overwhelms users who + // will never need it (issue #7926) — when either of these holds: + // // - allowPadDeletionByAllUsers: anyone can delete the pad with no token at - // all (see handlePadDelete's flagOk branch), so a recovery token is noise - // and the modal only overwhelms users who will never need it. + // all (see handlePadDelete's flagOk branch). + // - the creator has a *durable* identity: authenticated (req.session.user + // with a username) AND the deployment maps that identity to a stable + // authorID via a getAuthorId hook. Only then does `isCreator` + // (author === revision-0 author) survive a cookie clear or a different + // device, so the creator path replaces the token on any device. + // + // Note we deliberately do NOT treat requireAuthentication alone as durable: + // without a getAuthorId hook the authorID still comes from the per-browser + // token cookie (AuthorManager.getAuthorId -> getAuthor4Token), so an + // authenticated user on a second device is NOT the creator and would be + // stranded if we also withheld the token. The getAuthorId hook is the + // documented way (doc/api/hooks_server-side) to pin authorID to username. + const hasGetAuthorIdHook = (plugins.hooks.getAuthorId || []).length > 0; + const hasDurableIdentity = hasGetAuthorIdHook && !!(user && user.username); + const canDeleteWithoutToken = settings.allowPadDeletionByAllUsers || hasDurableIdentity; const padDeletionToken = - isCreator && !settings.requireAuthentication && !settings.allowPadDeletionByAllUsers + isCreator && !canDeleteWithoutToken ? await padDeletionManager.createDeletionTokenIfAbsent(sessionInfo.padId) : null; @@ -1314,6 +1330,11 @@ const handleClientReady = async (socket:any, message: ClientReadyMessage) => { enablePadWideSettings: settings.enablePadWideSettings, enablePluginPadOptions: settings.enablePluginPadOptions, padDeletionToken, + // Drives the deletion-button label/visibility in pad settings: when the + // user can already delete without a token the recovery-token disclosure is + // redundant, so the client labels the action "Delete Pad" instead of + // "Delete with token" (issue #7926). See showDeletionTokenModalIfPresent. + canDeleteWithoutToken, // Allow-listed copy — settings.privacyBanner could carry extra nested // keys from a hand-edited settings.json; sending those by reference // would leak them to every browser. See getPublicPrivacyBanner(). diff --git a/src/static/js/pad_editor.ts b/src/static/js/pad_editor.ts index 96db7879a28..13f73e3f7a2 100644 --- a/src/static/js/pad_editor.ts +++ b/src/static/js/pad_editor.ts @@ -149,6 +149,17 @@ const padeditor = (() => { } }); + // When the user can already delete the pad without a token (everyone may + // delete, or they have a durable authenticated identity — issue #7926), + // the recovery-token disclosure is redundant: label it plainly "Delete + // Pad" rather than the jargon "Delete with token". Reuses the existing, + // already-translated pad.settings.deletePad key. + if ((window as any).clientVars?.canDeleteWithoutToken) { + const $summary = $('#delete-pad-with-token > summary'); + $summary.attr('data-l10n-id', 'pad.settings.deletePad') + .text(html10n.get('pad.settings.deletePad')); + } + // delete pad using a recovery token (second device / no creator cookie) $('#delete-pad-token-submit').on('click', () => { const token = String($('#delete-pad-token-input').val() || '').trim(); diff --git a/src/tests/backend/specs/socketio.ts b/src/tests/backend/specs/socketio.ts index 84a306282ea..441f8110e08 100644 --- a/src/tests/backend/specs/socketio.ts +++ b/src/tests/backend/specs/socketio.ts @@ -493,6 +493,8 @@ describe(__filename, function () { }); describe('Pad deletion token issuance (#7926)', function () { + let getAuthorIdBackup: any; + const removeIfExists = async (padId: string) => { if (await padManager.doesPadExist(padId)) { const p = await padManager.getPad(padId); @@ -500,14 +502,32 @@ describe(__filename, function () { } }; + // A getAuthorId hook that pins authorID to the authenticated username — the + // documented way (doc/api/hooks_server-side) to give a user a stable + // identity across cookie clears and devices. Its mere presence is what makes + // an authenticated session "durable" for token-suppression purposes. + const installStableIdentityHook = () => { + plugins.hooks.getAuthorId = [{hook_fn: async (hookName: string, context: any) => { + const username = context.user && context.user.username; + if (!username) return; + context.dbKey = `username=${username}`; + return ''; + }}]; + }; + beforeEach(async function () { // @ts-ignore - public setting toggled per test settings.allowPadDeletionByAllUsers = false; + // The outer harness only backs up preAuthorize/authenticate/authorize, so + // manage getAuthorId ourselves to avoid leaking it into later specs. + getAuthorIdBackup = plugins.hooks.getAuthorId; + plugins.hooks.getAuthorId = []; await removeIfExists('pad'); }); afterEach(async function () { if (socket) socket.close(); socket = null; + plugins.hooks.getAuthorId = getAuthorIdBackup; await removeIfExists('pad'); }); @@ -519,6 +539,7 @@ describe(__filename, function () { assert.equal(typeof cv.data.padDeletionToken, 'string', 'creator should get a token so the client can show the save-token modal'); assert.ok(cv.data.padDeletionToken.length >= 32); + assert.equal(cv.data.canDeleteWithoutToken, false); }); it('no token (and so no modal) when allowPadDeletionByAllUsers is true', async function () { @@ -532,7 +553,33 @@ describe(__filename, function () { // client, so the "Save your pad deletion token" modal never appears. Anyone // can already delete the pad without a token in this configuration. assert.equal(cv.data.padDeletionToken, null); + assert.equal(cv.data.canDeleteWithoutToken, true); }); + + it('authenticated creator WITHOUT a getAuthorId hook still gets a token', async function () { + // requireAuthentication alone is NOT durable: the authorID still comes from + // the per-browser token cookie, so this user would be stranded on a second + // device if the token were withheld. They must keep getting one. + settings.requireAuthentication = true; + const res = await agent.get('/p/pad').auth('user', 'user-password').expect(200); + socket = await common.connect(res); + const cv: any = await common.handshake(socket, 'pad'); + assert.equal(cv.type, 'CLIENT_VARS'); + assert.equal(typeof cv.data.padDeletionToken, 'string'); + assert.equal(cv.data.canDeleteWithoutToken, false); + }); + + it('authenticated creator WITH a getAuthorId hook gets no token (durable identity)', + async function () { + settings.requireAuthentication = true; + installStableIdentityHook(); + const res = await agent.get('/p/pad').auth('user', 'user-password').expect(200); + socket = await common.connect(res); + const cv: any = await common.handshake(socket, 'pad'); + assert.equal(cv.type, 'CLIENT_VARS'); + assert.equal(cv.data.padDeletionToken, null); + assert.equal(cv.data.canDeleteWithoutToken, true); + }); }); describe('SocketIORouter.js', function () { From 8e923f06054c1aa300de67c953470bca8fde4a09 Mon Sep 17 00:00:00 2001 From: John McLear Date: Tue, 9 Jun 2026 15:30:15 +0100 Subject: [PATCH 2/2] fix(pad): render recovery disclosure for all sessions; hide it (not relabel) when no token is needed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses Qodo review on #7930: 1. Token UI fully suppressed when not needed (was: only the summary relabelled). When canDeleteWithoutToken (allowPadDeletionByAllUsers or a durable identity), pad_editor.ts now hides the whole #delete-pad-with-token disclosure — label, token field and submit — so no deletion-token wording remains. With the plain "Delete Pad" button present this is also the cleaner UX. 2. Recovery disclosure no longer gated on !requireAuthentication. Because a token can now be issued under requireAuthentication when the deployment lacks a durable getAuthorId mapping, the template must render the recovery form there too — otherwise an authenticated creator gets a token with no UI to enter it on another device. It is rendered hidden by default and shown by the client only when canDeleteWithoutToken is false. 3. API.createPad aligned: also returns null deletionToken under allowPadDeletionByAllUsers, matching the socket/UI path. Tests: deletePad.ts gains a createPad-under-allowPadDeletionByAllUsers case. Verified live in Chromium: allowAll -> disclosure hidden, no modal; default anonymous -> disclosure visible, modal shown. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/node/db/API.ts | 9 +++++---- src/static/js/pad_editor.ts | 19 +++++++++---------- src/templates/pad.html | 8 +++++--- src/tests/backend/specs/api/deletePad.ts | 11 +++++++++++ 4 files changed, 30 insertions(+), 17 deletions(-) diff --git a/src/node/db/API.ts b/src/node/db/API.ts index bdc659b05d3..b79975b6c69 100644 --- a/src/node/db/API.ts +++ b/src/node/db/API.ts @@ -551,10 +551,11 @@ exports.createPad = async (padID: string, text: string, authorId = '') => { // create pad await getPadSafe(padID, false, text, authorId); - // When requireAuthentication is on, every creator has a stable identity, so - // the cookie/identity path covers recovery and the one-time token is just - // an extra surface to leak. - const deletionToken = settings.requireAuthentication + // No recovery token when it cannot help: requireAuthentication gives every + // creator a stable identity, and allowPadDeletionByAllUsers lets anyone delete + // the pad with no token at all (issue #7926). Either way the token is just an + // extra surface to leak. + const deletionToken = settings.requireAuthentication || settings.allowPadDeletionByAllUsers ? null : await padDeletionManager.createDeletionTokenIfAbsent(padID); return {deletionToken}; diff --git a/src/static/js/pad_editor.ts b/src/static/js/pad_editor.ts index 13f73e3f7a2..0382a813a00 100644 --- a/src/static/js/pad_editor.ts +++ b/src/static/js/pad_editor.ts @@ -149,16 +149,15 @@ const padeditor = (() => { } }); - // When the user can already delete the pad without a token (everyone may - // delete, or they have a durable authenticated identity — issue #7926), - // the recovery-token disclosure is redundant: label it plainly "Delete - // Pad" rather than the jargon "Delete with token". Reuses the existing, - // already-translated pad.settings.deletePad key. - if ((window as any).clientVars?.canDeleteWithoutToken) { - const $summary = $('#delete-pad-with-token > summary'); - $summary.attr('data-l10n-id', 'pad.settings.deletePad') - .text(html10n.get('pad.settings.deletePad')); - } + // The recovery-token disclosure (#delete-pad-with-token) is rendered for + // every session because a token may now be issued under requireAuthentication + // too (when no getAuthorId hook pins a durable authorID — issue #7926). + // Show it only when the user actually needs a token: when they can already + // delete without one (everyone may delete, or they have a durable + // authenticated identity) the plain "Delete Pad" button suffices, so hide + // the disclosure and all its token wording entirely. + $('#delete-pad-with-token').prop( + 'hidden', !!(window as any).clientVars?.canDeleteWithoutToken); // delete pad using a recovery token (second device / no creator cookie) $('#delete-pad-token-submit').on('click', () => { diff --git a/src/templates/pad.html b/src/templates/pad.html index 6bbc359c6ef..40c7ad0a943 100644 --- a/src/templates/pad.html +++ b/src/templates/pad.html @@ -387,15 +387,17 @@

<% } %> - <% if (!settings.requireAuthentication) { %> -
+ + - <% } %>

About

Powered by Etherpad diff --git a/src/tests/backend/specs/api/deletePad.ts b/src/tests/backend/specs/api/deletePad.ts index fe118aa4e4b..1268f4e9a52 100644 --- a/src/tests/backend/specs/api/deletePad.ts +++ b/src/tests/backend/specs/api/deletePad.ts @@ -80,6 +80,17 @@ describe(__filename, function () { await callApi('deletePad', {padID: padId}); }); + it('createPad returns null deletionToken when allowPadDeletionByAllUsers is on', async function () { + // Anyone can delete the pad with no token at all, so the recovery token is + // pointless — matches the socket/UI path (issue #7926). + settings.allowPadDeletionByAllUsers = true; + const padId = makeId(); + const res = await callApi('createPad', {padID: padId}); + assert.equal(res.body.code, 0, JSON.stringify(res.body)); + assert.equal(res.body.data.deletionToken, null); + await callApi('deletePad', {padID: padId}); + }); + it('JWT admin call (no deletionToken) still works — admins stay trusted', async function () { const padId = makeId(); await callApi('createPad', {padID: padId});