feat(gdpr): pad deletion controls (PR1 of #6701)#7546
feat(gdpr): pad deletion controls (PR1 of #6701)#7546JohnMcLear wants to merge 18 commits intodevelopfrom
Conversation
Review Summary by Qodofeat(gdpr): pad deletion controls with one-time tokens and recovery flow
WalkthroughsDescription• One-time sha256-hashed deletion token, returned plaintext once on pad creation • Three-way authorization: creator cookie, valid token, or allowPadDeletionByAllUsers flag • Browser creators see token modal and can delete via recovery-token field in settings • Comprehensive backend and frontend test coverage with Playwright and Mocha specs Diagramflowchart LR
CreatePad["createPad/createGroupPad"] -->|generates token| PDM["PadDeletionManager"]
PDM -->|stores hash| DB["Database"]
PDM -->|returns plaintext| API["API Response"]
API -->|browser only| ClientVars["clientVars.padDeletionToken"]
ClientVars -->|creator session| Modal["Token Modal"]
Modal -->|acknowledged| Settings["Settings Popup"]
Settings -->|delete-with-token| PAD_DELETE["PAD_DELETE Message"]
PAD_DELETE -->|three-way auth| Handler["handlePadDelete"]
Handler -->|creator OR token OR flag| Remove["Pad.remove"]
Remove -->|cleanup| PDM
File Changes1. src/node/db/PadDeletionManager.ts
|
Code Review by Qodo
1. Deletion token not feature-flagged
|
| // Only the original creator of the pad (revision 0 author) receives the | ||
| // deletion token, and only on their first arrival — subsequent visits get | ||
| // null because createDeletionTokenIfAbsent() only emits a plaintext token | ||
| // once. Readonly sessions never see it. | ||
| const isCreator = | ||
| !sessionInfo.readonly && sessionInfo.author === await pad.getRevisionAuthor(0); | ||
| const padDeletionToken = isCreator | ||
| ? await padDeletionManager.createDeletionTokenIfAbsent(sessionInfo.padId) | ||
| : null; |
There was a problem hiding this comment.
1. Deletion token not feature-flagged 📘 Rule violation ☼ Reliability
Pad deletion tokens (API response + creator modal) are enabled unconditionally, changing default behavior rather than being gated behind a disabled-by-default feature flag. This violates the requirement that new features be behind an off-by-default flag to preserve pre-change behavior unless explicitly enabled.
Agent Prompt
## Issue description
Deletion tokens are generated and surfaced by default (API response and UI modal) without a feature flag. Compliance requires new features to be behind a disabled-by-default flag, with the default path matching pre-change behavior.
## Issue Context
Current behavior:
- `createPad()` always returns `{deletionToken: ...}`
- Creator sessions always attempt token creation and receive `clientVars.padDeletionToken`
## Fix Focus Areas
- src/node/db/API.ts[520-546]
- src/node/handler/PadMessageHandler.ts[1004-1012]
- src/node/utils/Settings.ts[172-176]
- settings.json.template[480-486]
- settings.json.docker[483-489]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
|
||
| // create pad | ||
| await getPadSafe(padID, false, text, authorId); | ||
| return {deletionToken: await padDeletionManager.createDeletionTokenIfAbsent(padID)}; | ||
| }; | ||
|
|
||
| /** | ||
| deletePad(padID) deletes a pad | ||
| deletePad(padID, [deletionToken]) deletes a pad | ||
|
|
||
| Example returns: | ||
|
|
||
| {code: 0, message:"ok", data: null} | ||
| {code: 1, message:"padID does not exist", data: null} | ||
| {code: 1, message:"invalid deletionToken", data: null} | ||
| @param {String} padID the id of the pad | ||
| @param {String} [deletionToken] recovery token issued by createPad | ||
| */ | ||
| exports.deletePad = async (padID: string) => { | ||
| exports.deletePad = async (padID: string, deletionToken?: string) => { | ||
| const pad = await getPadSafe(padID, true); | ||
| // apikey-authenticated callers (no deletionToken supplied) are trusted. | ||
| // When a caller supplies a deletionToken, it must validate unless the | ||
| // instance has opted everyone in via allowPadDeletionByAllUsers. | ||
| if (deletionToken !== undefined && deletionToken !== '' && | ||
| !settings.allowPadDeletionByAllUsers && | ||
| !await padDeletionManager.isValidDeletionToken(padID, deletionToken)) { | ||
| throw new CustomError('invalid deletionToken', 'apierror'); | ||
| } |
There was a problem hiding this comment.
2. Http api docs not updated 📘 Rule violation ⚙ Maintainability
The PR changes the public HTTP API (createPad now returns deletionToken; deletePad accepts optional deletionToken) but the HTTP API documentation still describes the old signatures/returns. This can cause integrator confusion and violates the requirement to update docs in the same PR when changing APIs.
Agent Prompt
## Issue description
The HTTP API docs are out of date relative to the changed endpoints:
- `createPad` now returns `data: {deletionToken: string|null}`
- `deletePad` now accepts optional `deletionToken` and may return `invalid deletionToken`
## Issue Context
The OpenAPI schema arg list was updated, but the human-readable HTTP API documentation (`doc/api/http_api.md`) still shows the previous signatures and example returns.
## Fix Focus Areas
- src/node/db/API.ts[520-548]
- src/node/handler/APIHandler.ts[53-56]
- doc/api/http_api.md[519-592]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| exports.createDeletionTokenIfAbsent = async (padId: string): Promise<string | null> => { | ||
| if (await DB.db.get(getDeletionTokenKey(padId)) != null) return null; | ||
| const deletionToken = randomString(32); | ||
| await DB.db.set(getDeletionTokenKey(padId), { | ||
| createdAt: Date.now(), | ||
| hash: hashDeletionToken(deletionToken).toString('hex'), | ||
| }); | ||
| return deletionToken; |
There was a problem hiding this comment.
3. Token creation race 🐞 Bug ≡ Correctness
createDeletionTokenIfAbsent() uses a non-atomic read-then-write, so concurrent calls for the same pad can each return different plaintext tokens and overwrite the stored hash, making at least one returned token permanently invalid.
Agent Prompt
### Issue description
`createDeletionTokenIfAbsent()` does a check-then-set on the DB key. Under concurrency, two calls for the same `padId` can both return different plaintext tokens, but only the last stored hash will validate, causing at least one client to save an unusable recovery token.
### Issue Context
This function is called from multiple entry points (creator `CLIENT_READY`, `createPad`, `createGroupPad`). Even without multi-process, two creator tabs can race.
### Fix Focus Areas
- src/node/db/PadDeletionManager.ts[13-21]
- src/node/handler/PadMessageHandler.ts[1008-1012]
### Suggested fix
Implement a per-`padId` in-memory mutex/in-flight promise map around the token creation path:
- Re-check existence after acquiring the lock.
- Ensure only one call performs the `set` and returns the plaintext token; concurrent callers should reliably return `null`.
If Etherpad can run multi-process in your deployment, document this limitation or consider a DB-level atomic primitive if available.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
a5b23e0 to
f6f2d58
Compare
First of five GDPR PRs tracked in #6701. PR1 covers deletion controls: one-time deletion token, allowPadDeletionByAllUsers flag, authorisation matrix for handlePadDelete and the REST deletePad endpoint, a single token-display modal for browser pad creators, and test coverage. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
13 TDD-structured tasks covering PadDeletionManager unit tests, socket + REST three-way auth, clientVars wiring, one-time token modal, delete-with-token UI, Playwright coverage, and PR handoff. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PadDeletionManager stores a sha256-hashed per-pad deletion token and verifies it with timing-safe comparison. createPad / createGroupPad return the plaintext token once on first creation, and Pad.remove() cleans it up. Gated behind the new allowPadDeletionByAllUsers flag which defaults to false to preserve existing behaviour. Part of #6701 (GDPR PR1). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Capturing DB.db at module-load time was null until DB.init() ran, which broke importing the module outside a live server (including from the test runner). Switch to DB.db.* at call time and add unit tests exercising create/verify/remove plus timing-safe comparison. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Creator cookie → valid deletion token → allowPadDeletionByAllUsers flag. Anyone else still gets the existing refusal shout.
Revision-0 author on their first CLIENT_READY visit receives the plaintext token; all subsequent CLIENT_READYs receive null because createDeletionTokenIfAbsent is idempotent. Readonly sessions and any other user never see the token.
The token modal introduced in PR1 blocks clicks for every Playwright test that creates a new pad via the shared helper. Add a one-line dismissal so unrelated tests keep passing, and have the deletion-token spec navigate inline via newPadKeepingModal() when it needs the modal open to capture the token. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Clicking the ack button transferred focus out of the pad iframe, which made subsequent keyboard-driven tests (Tab / Enter) silently miss the editor. Swap the click for a page.evaluate() that hides the modal and nulls clientVars.padDeletionToken directly, leaving focus where it was. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Qodo review: - createDeletionTokenIfAbsent() was a non-atomic read-then-write. Two concurrent callers for the same pad could both return different plaintext tokens while only the later hash was stored, leaving the first caller with an unusable recovery token. Serialise per-pad via a Promise chain and add a regression test that fires 8 concurrent calls and asserts exactly one plaintext is emitted and validates. - doc/api/http_api.md now documents createPad returning deletionToken and deletePad accepting the optional deletionToken parameter. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
64c3527 to
f64ba91
Compare
The rebase onto develop placed the delete-pad-with-token details inside the pad-settings-section conditional, which is only rendered when enablePadWideSettings is true AND the section is toggled visible. Second-device recovery (typing the captured token on a fresh browser) must work without pad-wide settings enabled, so move the details out to sit alongside the existing pad_deletion_token.spec.ts expectations. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
allowPadDeletionByAllUsersflag (defaults tofalse) to widen deletion rightsPAD_DELETEand RESTdeletePad: creator cookie, valid token, or settings flagFirst of the five GDPR PRs outlined in #6701. Remaining scope (IP audit, identity hardening, cookie banner, author erasure) stays in follow-ups.
Design spec:
docs/superpowers/specs/2026-04-18-gdpr-pr1-deletion-controls-design.mdImplementation plan:
docs/superpowers/plans/2026-04-18-gdpr-pr1-deletion-controls.mdTest plan
pnpm --filter ep_etherpad-lite run ts-checkmocha tests/backend/specs/padDeletionManager.ts tests/backend/specs/api/deletePad.ts— 14 tests passnpx playwright test pad_deletion_token --project=chromium— 3 tests pass