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/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..0382a813a00 100644 --- a/src/static/js/pad_editor.ts +++ b/src/static/js/pad_editor.ts @@ -149,6 +149,16 @@ const padeditor = (() => { } }); + // 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', () => { const token = String($('#delete-pad-token-input').val() || '').trim(); 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}); 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 () {