mirror of
https://github.com/ether/etherpad-lite.git
synced 2026-07-17 16:47:05 +00:00
* fix(pad): keep token-less Delete pad reachable without pad-wide settings (#7959) The token-less "Delete pad" button (#delete-pad) was nested inside the enablePadWideSettings-gated pad-settings section, so disabling pad-wide settings removed the only way to delete a pad without a recovery token. Combined with #7926 hiding the token disclosure when deletion needs no token (e.g. allowPadDeletionByAllUsers), a user who was allowed to delete could be left with no deletion UI at all. Pad deletion is unrelated to pad-wide settings, so: - Move #delete-pad out of the enablePadWideSettings block in pad.html; it is now always rendered and hidden by default. - Add a canDeletePad clientVar (isCreator || allowPadDeletionByAllUsers) and drive the button's visibility from it in pad_editor.ts, mirroring the existing canDeleteWithoutToken handling for the token disclosure. The two controls are now mutually coherent and neither depends on enablePadWideSettings: the plain button shows when this session can delete without a token, the recovery-token disclosure shows otherwise. Tests: - backend padDeletionUiPlacement.ts: #delete-pad is rendered with enablePadWideSettings both on and off (fails without the template move). - backend socketio.ts: canDeletePad reflects the creator/allow-all matrix, including a non-creator who only gains it under allowPadDeletionByAllUsers. - frontend pad_settings.spec.ts: asserts #delete-pad is no longer a descendant of #pad-settings-section. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(pad): never let readonly sessions delete via token-less paths (#7959) Qodo review of #7960: `canDeletePad` was `isCreator || allowPadDeletionByAllUsers`, so under allowPadDeletionByAllUsers a readonly viewer received canDeletePad=true and the relocated #delete-pad button unhid for them. Worse, the server-side handlePadDelete `flagOk`/`creatorOk` branches never checked session.readonly either, so a readonly-link holder could actually delete the pad without a token — a data-loss hole that the new always-rendered button would expose. Exclude readonly sessions from both the clientVar and the server's token-less authorization paths. A valid recovery token (tokenOk) stays a sufficient credential regardless of session mode. Test: socketio.ts asserts a readonly viewer gets canDeletePad=false and that a token-less PAD_DELETE from a readonly session leaves the pad intact (red before this change on the clientVar assertion). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
John McLear
GitHub
parent
d67aa3ebd3
commit
ce038b0b0f
6 changed files with 156 additions and 8 deletions
|
|
@ -305,8 +305,13 @@ const handlePadDelete = async (socket: any, padDeleteMessage: PadDeleteMessage)
|
|||
// back to the creator-cookie path, otherwise a creator pasting a wrong
|
||||
// recovery token into the disclosure field would still succeed — masking a
|
||||
// typo and contradicting the UI.
|
||||
const creatorOk = !tokenSupplied && isCreator;
|
||||
const flagOk = !tokenSupplied && !isCreator && settings.allowPadDeletionByAllUsers;
|
||||
// Readonly sessions can never delete via the token-less paths: they cannot
|
||||
// edit the pad, so they must not be able to destroy it just because
|
||||
// allowPadDeletionByAllUsers is on (issue #7959). A valid recovery token
|
||||
// (tokenOk) remains a sufficient credential regardless of session mode.
|
||||
const writable = !session.readonly;
|
||||
const creatorOk = !tokenSupplied && isCreator && writable;
|
||||
const flagOk = !tokenSupplied && !isCreator && settings.allowPadDeletionByAllUsers && writable;
|
||||
|
||||
if (creatorOk || tokenOk || flagOk) {
|
||||
await retrievedPad.remove();
|
||||
|
|
@ -1321,6 +1326,16 @@ const handleClientReady = async (socket:any, message: ClientReadyMessage) => {
|
|||
const hasGetAuthorIdHook = (plugins.hooks.getAuthorId || []).length > 0;
|
||||
const hasDurableIdentity = hasGetAuthorIdHook && !!(user && user.username);
|
||||
const canDeleteWithoutToken = settings.allowPadDeletionByAllUsers || hasDurableIdentity;
|
||||
// Whether this session may delete the pad with no token at all: the creator
|
||||
// on this device (creator-cookie still present), or any user when the
|
||||
// instance opted everyone in. Drives the plain "Delete pad" button, which is
|
||||
// independent of enablePadWideSettings (issue #7959) — deletion is not a
|
||||
// pad-wide setting and must stay reachable when that section is disabled.
|
||||
// Readonly viewers are excluded: they cannot edit, let alone delete, so
|
||||
// allowPadDeletionByAllUsers must not hand them a delete button (the server
|
||||
// enforces the same in handlePadDelete).
|
||||
const canDeletePad =
|
||||
!sessionInfo.readonly && (isCreator || settings.allowPadDeletionByAllUsers);
|
||||
const padDeletionToken =
|
||||
isCreator && !canDeleteWithoutToken
|
||||
? await padDeletionManager.createDeletionTokenIfAbsent(sessionInfo.padId)
|
||||
|
|
@ -1346,6 +1361,7 @@ const handleClientReady = async (socket:any, message: ClientReadyMessage) => {
|
|||
// redundant, so the client labels the action "Delete Pad" instead of
|
||||
// "Delete with token" (issue #7926). See showDeletionTokenModalIfPresent.
|
||||
canDeleteWithoutToken,
|
||||
canDeletePad,
|
||||
// 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().
|
||||
|
|
|
|||
|
|
@ -159,6 +159,13 @@ const padeditor = (() => {
|
|||
$('#delete-pad-with-token').prop(
|
||||
'hidden', !!(window as any).clientVars?.canDeleteWithoutToken);
|
||||
|
||||
// The plain "Delete pad" button is shown whenever this session can delete
|
||||
// without a token (creator on this device, or allowPadDeletionByAllUsers).
|
||||
// It is independent of pad-wide settings so it stays reachable when that
|
||||
// section is disabled (issue #7959).
|
||||
$('#delete-pad').prop(
|
||||
'hidden', !(window as any).clientVars?.canDeletePad);
|
||||
|
||||
// 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();
|
||||
|
|
|
|||
|
|
@ -391,13 +391,17 @@
|
|||
</p>
|
||||
<% e.end_block(); %>
|
||||
</div>
|
||||
<button class="btn btn-danger" data-l10n-id="pad.settings.deletePad" id="delete-pad">Delete pad</button>
|
||||
</div><% } %>
|
||||
</div>
|
||||
<!-- 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. -->
|
||||
<!-- Pad deletion is independent of pad-wide settings (issue #7959):
|
||||
both controls are always rendered and pad_editor.ts shows exactly
|
||||
the one that applies. #delete-pad (token-less) is shown when
|
||||
clientVars.canDeletePad — the creator on this device, or any user
|
||||
when allowPadDeletionByAllUsers is on. The recovery-token
|
||||
disclosure is shown when clientVars.canDeleteWithoutToken is false
|
||||
(issue #7926): a creator on a second device, or under
|
||||
requireAuthentication without a durable cross-device identity. -->
|
||||
<button class="btn btn-danger" data-l10n-id="pad.settings.deletePad" id="delete-pad" hidden>Delete pad</button>
|
||||
<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>
|
||||
|
|
|
|||
44
src/tests/backend/specs/padDeletionUiPlacement.ts
Normal file
44
src/tests/backend/specs/padDeletionUiPlacement.ts
Normal file
|
|
@ -0,0 +1,44 @@
|
|||
'use strict';
|
||||
|
||||
import {MapArrayType} from '../../../node/types/MapType';
|
||||
import settings from '../../../node/utils/Settings';
|
||||
|
||||
const assert = require('assert').strict;
|
||||
const common = require('../common');
|
||||
|
||||
// Regression coverage for issue #7959. The token-less "Delete pad" button
|
||||
// (#delete-pad) used to be nested inside the `enablePadWideSettings`-gated
|
||||
// pad-settings section, so disabling pad-wide settings removed the only way to
|
||||
// delete a pad without a recovery token. Pad deletion is unrelated to pad-wide
|
||||
// settings, so the button must be rendered regardless of that flag (its
|
||||
// visibility is then driven at runtime by clientVars.canDeletePad).
|
||||
describe(__filename, function () {
|
||||
this.timeout(30000);
|
||||
let agent: any;
|
||||
const backup: MapArrayType<any> = {};
|
||||
|
||||
before(async function () { agent = await common.init(); });
|
||||
|
||||
beforeEach(async function () {
|
||||
backup.enablePadWideSettings = settings.enablePadWideSettings;
|
||||
});
|
||||
|
||||
afterEach(async function () {
|
||||
settings.enablePadWideSettings = backup.enablePadWideSettings;
|
||||
});
|
||||
|
||||
const hasDeletePadButton = (html: string): boolean =>
|
||||
/id="delete-pad"/.test(html);
|
||||
|
||||
it('renders the Delete pad button with pad-wide settings enabled', async function () {
|
||||
settings.enablePadWideSettings = true;
|
||||
const res = await agent.get('/p/deleteUiPlacementOn').expect(200);
|
||||
assert.equal(hasDeletePadButton(res.text), true);
|
||||
});
|
||||
|
||||
it('renders the Delete pad button with pad-wide settings disabled (#7959)', async function () {
|
||||
settings.enablePadWideSettings = false;
|
||||
const res = await agent.get('/p/deleteUiPlacementOff').expect(200);
|
||||
assert.equal(hasDeletePadButton(res.text), true);
|
||||
});
|
||||
});
|
||||
|
|
@ -540,6 +540,9 @@ describe(__filename, function () {
|
|||
'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);
|
||||
// The creator can always delete without a token on this device, so the
|
||||
// plain "Delete pad" button is offered (issue #7959).
|
||||
assert.equal(cv.data.canDeletePad, true);
|
||||
});
|
||||
|
||||
it('no token (and so no modal) when allowPadDeletionByAllUsers is true', async function () {
|
||||
|
|
@ -554,6 +557,75 @@ describe(__filename, function () {
|
|||
// can already delete the pad without a token in this configuration.
|
||||
assert.equal(cv.data.padDeletionToken, null);
|
||||
assert.equal(cv.data.canDeleteWithoutToken, true);
|
||||
assert.equal(cv.data.canDeletePad, true);
|
||||
});
|
||||
|
||||
it('non-creator gets canDeletePad=false by default, true under allowPadDeletionByAllUsers (#7959)',
|
||||
async function () {
|
||||
const supertest = require('supertest');
|
||||
// The creator (default cookie jar) establishes the pad's rev-0 author.
|
||||
const resCreator = await agent.get('/p/pad').expect(200);
|
||||
socket = await common.connect(resCreator);
|
||||
const cvCreator: any = await common.handshake(socket, 'pad');
|
||||
assert.equal(cvCreator.data.canDeletePad, true, 'creator can always delete');
|
||||
|
||||
// A different browser (separate cookie jar) is NOT the creator, so with
|
||||
// allowPadDeletionByAllUsers off it must not be offered the token-less
|
||||
// Delete pad button.
|
||||
const otherBrowser = supertest(common.baseUrl);
|
||||
const resOther = await otherBrowser.get('/p/pad').expect(200);
|
||||
const otherSocket = await common.connect(resOther);
|
||||
try {
|
||||
const cvOther: any = await common.handshake(otherSocket, 'pad');
|
||||
assert.equal(cvOther.data.canDeletePad, false,
|
||||
'non-creator must not see Delete pad by default');
|
||||
} finally {
|
||||
otherSocket.close();
|
||||
}
|
||||
|
||||
// With everyone opted in, the same non-creator CAN delete, so the
|
||||
// button must be offered — independent of enablePadWideSettings (#7959).
|
||||
// @ts-ignore - public setting toggled per test
|
||||
settings.allowPadDeletionByAllUsers = true;
|
||||
const otherBrowser2 = supertest(common.baseUrl);
|
||||
const resOther2 = await otherBrowser2.get('/p/pad').expect(200);
|
||||
const otherSocket2 = await common.connect(resOther2);
|
||||
try {
|
||||
const cvOther2: any = await common.handshake(otherSocket2, 'pad');
|
||||
assert.equal(cvOther2.data.canDeletePad, true,
|
||||
'allowPadDeletionByAllUsers must offer Delete pad to everyone');
|
||||
} finally {
|
||||
otherSocket2.close();
|
||||
}
|
||||
});
|
||||
|
||||
it('readonly viewer is denied canDeletePad and token-less deletion under allowPadDeletionByAllUsers (#7959)',
|
||||
async function () {
|
||||
// @ts-ignore - public setting toggled per test
|
||||
settings.allowPadDeletionByAllUsers = true;
|
||||
// Creator establishes the pad (rev-0 author) and yields its read-only id.
|
||||
const resCreator = await agent.get('/p/pad').expect(200);
|
||||
const creatorSocket = await common.connect(resCreator);
|
||||
const cvCreator: any = await common.handshake(creatorSocket, 'pad');
|
||||
const readOnlyId = cvCreator.data.readOnlyId;
|
||||
assert.ok(readOnlyManager.isReadOnlyId(readOnlyId));
|
||||
creatorSocket.close();
|
||||
|
||||
// A read-only viewer must NOT be offered the token-less delete button,
|
||||
// even with deletion opened to all users — readonly viewers cannot edit,
|
||||
// let alone delete (issue #7959).
|
||||
const resRo = await agent.get(`/p/${readOnlyId}`).expect(200);
|
||||
socket = await common.connect(resRo);
|
||||
const cvRo: any = await common.handshake(socket, readOnlyId);
|
||||
assert.equal(cvRo.data.readonly, true);
|
||||
assert.equal(cvRo.data.canDeletePad, false,
|
||||
'readonly viewers must not get the token-less Delete pad button');
|
||||
|
||||
// ...and the server must refuse a token-less PAD_DELETE from a readonly
|
||||
// session, or allowPadDeletionByAllUsers becomes a data-loss hole.
|
||||
await common.sendPadDelete(socket, {padId: 'pad'}).catch(() => {});
|
||||
assert.ok(await padManager.doesPadExist('pad'),
|
||||
'readonly session must not be able to delete the pad without a token');
|
||||
});
|
||||
|
||||
it('authenticated creator WITHOUT a getAuthorId hook still gets a token', async function () {
|
||||
|
|
@ -567,6 +639,7 @@ describe(__filename, function () {
|
|||
assert.equal(cv.type, 'CLIENT_VARS');
|
||||
assert.equal(typeof cv.data.padDeletionToken, 'string');
|
||||
assert.equal(cv.data.canDeleteWithoutToken, false);
|
||||
assert.equal(cv.data.canDeletePad, true);
|
||||
});
|
||||
|
||||
it('authenticated creator WITH a getAuthorId hook gets no token (durable identity)',
|
||||
|
|
@ -579,6 +652,7 @@ describe(__filename, function () {
|
|||
assert.equal(cv.type, 'CLIENT_VARS');
|
||||
assert.equal(cv.data.padDeletionToken, null);
|
||||
assert.equal(cv.data.canDeleteWithoutToken, true);
|
||||
assert.equal(cv.data.canDeletePad, true);
|
||||
});
|
||||
});
|
||||
|
||||
|
|
|
|||
|
|
@ -3,7 +3,7 @@ import {goToNewPad, goToPad, sendChatMessage, showChat} from "../helper/padHelpe
|
|||
import {showSettings} from "../helper/settingsHelper";
|
||||
|
||||
test.describe('creator-owned pad settings', () => {
|
||||
test('shows pad settings only to the creator and keeps delete pad there', async ({page, browser}) => {
|
||||
test('shows pad settings only to the creator; delete pad is creator-gated but separate', async ({page, browser}) => {
|
||||
const padId = await goToNewPad(page);
|
||||
|
||||
const context2 = await browser.newContext();
|
||||
|
|
@ -19,6 +19,9 @@ test.describe('creator-owned pad settings', () => {
|
|||
await expect(page.locator('#pad-settings-section')).toBeVisible();
|
||||
await expect(page.locator('#delete-pad')).toBeVisible();
|
||||
await expect(page.locator('#padsettings-enforcecheck')).toBeVisible();
|
||||
// The delete-pad button is no longer nested inside the pad-wide settings
|
||||
// section: deletion is independent of enablePadWideSettings (issue #7959).
|
||||
await expect(page.locator('#pad-settings-section #delete-pad')).toHaveCount(0);
|
||||
|
||||
await expect(page2.locator('#user-settings-section > h2')).toHaveText('User Settings');
|
||||
await expect(page2.locator('#theme-toggle-row')).toBeVisible();
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue