Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions src/node/db/API.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down
35 changes: 28 additions & 7 deletions src/node/handler/PadMessageHandler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Comment on lines +1310 to 1316

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

2. Auth token ui mismatch 🐞 Bug ≡ Correctness

handleClientReady can now issue a padDeletionToken even when settings.requireAuthentication is true
(if there is no durable getAuthorId mapping), but the pad UI hides the token-based deletion controls
whenever requireAuthentication is enabled. This can strand authenticated creators on other
devices/cookie-loss scenarios because handlePadDelete requires creator identity or a valid deletion
token.
Agent Prompt
## Issue description
`handleClientReady` may emit `clientVars.padDeletionToken` (and `canDeleteWithoutToken=false`) in configurations where `settings.requireAuthentication=true` but there is no durable `getAuthorId` mapping. However, the token-based delete UI (`#delete-pad-with-token`) is currently not rendered at all when `requireAuthentication` is enabled, so users cannot supply the recovery token from another browser/device to delete the pad.

## Issue Context
- Server-side delete authorization allows deletion with a valid token regardless of creator cookie, but only if the client can send `PAD_DELETE` with `deletionToken`.
- The EJS template currently removes the token-delete form under `requireAuthentication`, which conflicts with the new behavior that can still require a token.
- The HTTP API `createPad` path still unconditionally suppresses `deletionToken` when `requireAuthentication` is enabled, which diverges from the socket/UI path.

## Fix Focus Areas
- Decide on one consistent contract:
  - **Option A (preferred if token recovery should work under requireAuth without durable identity):** Render the token-delete UI even when `requireAuthentication` is true, and hide it dynamically (client-side) when `clientVars.canDeleteWithoutToken` is true.
  - **Option B:** If you do *not* want token recovery under `requireAuthentication`, restore the old server-side suppression and remove the new backend test expectations.
- If Option A:
  - Update `src/templates/pad.html` to always include (or conditionally include based on a new server-provided flag) the `#delete-pad-with-token` form.
  - Update `src/static/js/pad_editor.ts` to hide/show the disclosure based on `clientVars.canDeleteWithoutToken` (and/or presence of `clientVars.padDeletionToken`).
  - Consider aligning `src/node/db/API.ts#createPad` to the same durable-identity rules so API-created pads aren’t treated differently.

### Fix Focus Areas (exact locations)
- src/node/handler/PadMessageHandler.ts[1284-1316]
- src/templates/pad.html[390-398]
- src/static/js/pad_editor.ts[152-191]
- src/node/db/API.ts[552-560]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — real bug, fixed in 8e923f0 (your Option A).

  • The recovery disclosure is no longer gated on !requireAuthentication: pad.html now renders #delete-pad-with-token for every session (default hidden), and pad_editor.ts shows it only when clientVars.canDeleteWithoutToken is false. So an authenticated creator who is issued a token (requireAuthentication + no durable getAuthorId mapping) now has the UI to enter it on another device.
  • API.createPad aligned: it also returns deletionToken: null under allowPadDeletionByAllUsers now (it already suppressed under requireAuthentication). I left the requireAuthentication suppression on the API path as-is otherwise — the API caller is apikey-trusted and has no req.session.user/hook context, so the durable-identity rules don't map there; the relevant consistency fix is the allowAll suppression.

Verified live in Chromium: allowAll=true → disclosure hidden, no modal; default anonymous → disclosure visible, modal shown. Added a createPad-under-allowPadDeletionByAllUsers backend test.


Expand All @@ -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().
Expand Down
10 changes: 10 additions & 0 deletions src/static/js/pad_editor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
8 changes: 5 additions & 3 deletions src/templates/pad.html
Original file line number Diff line number Diff line change
Expand Up @@ -387,15 +387,17 @@ <h2 data-l10n-id="pad.settings.padSettings"></h2>
<button class="btn btn-danger" data-l10n-id="pad.settings.deletePad" id="delete-pad">Delete pad</button>
</div><% } %>
</div>
<% if (!settings.requireAuthentication) { %>
<details id="delete-pad-with-token">
<!-- Visibility is controlled at runtime in pad_editor.ts: hidden when
clientVars.canDeleteWithoutToken (issue #7926). Rendered for every
session (incl. requireAuthentication) because a recovery token may
be issued whenever the creator lacks a durable cross-device identity. -->
<details id="delete-pad-with-token" hidden>
<summary data-l10n-id="pad.deletionToken.deleteWithToken">Delete with token</summary>
<label for="delete-pad-token-input" data-l10n-id="pad.deletionToken.tokenFieldLabel">Pad deletion token</label>
<input type="password" id="delete-pad-token-input" autocomplete="off" spellcheck="false">
<button id="delete-pad-token-submit" type="button" class="btn btn-danger"
data-l10n-id="pad.settings.deletePad">Delete pad</button>
</details>
<% } %>
<h2 data-l10n-id="pad.settings.about">About</h2>
<span data-l10n-id="pad.settings.poweredBy">Powered by</span>
<a href="https://etherpad.org" target="_blank" referrerpolicy="no-referrer" rel="noopener">Etherpad</a>
Expand Down
11 changes: 11 additions & 0 deletions src/tests/backend/specs/api/deletePad.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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});
Expand Down
47 changes: 47 additions & 0 deletions src/tests/backend/specs/socketio.ts
Original file line number Diff line number Diff line change
Expand Up @@ -493,21 +493,41 @@ 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);
await p.remove();
}
};

// 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');
});

Expand All @@ -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 () {
Expand All @@ -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 () {
Expand Down
Loading