feat(gdpr): pad deletion controls (PR1 of #6701)#7546
Merged
JohnMcLear merged 19 commits intodevelopfrom May 1, 2026
Merged
Conversation
Review Summary by Qodo(Agentic_describe updated until commit baa11b5)feat(gdpr): pad deletion controls with one-time recovery tokens (PR1 of #6701)
WalkthroughsDescription• One-time sha256-hashed deletion token returned plaintext on pad creation • Three-way authorization for pad deletion: creator cookie, valid token, or settings flag • Browser creators see one-time token modal and can delete via recovery-token field • Comprehensive backend and frontend test coverage for token lifecycle and authorization matrix Diagramflowchart LR
CreatePad["createPad/createGroupPad"]
TokenMgr["PadDeletionManager"]
DB["Database"]
Response["API Response + clientVars"]
Modal["Token Modal"]
Delete["Delete Pad"]
Auth["3-way Authorization"]
CreatePad -->|createDeletionTokenIfAbsent| TokenMgr
TokenMgr -->|store sha256 hash| DB
TokenMgr -->|return plaintext once| Response
Response -->|browser creator| Modal
Modal -->|copy & acknowledge| Delete
Delete -->|socket PAD_DELETE| Auth
Auth -->|creator cookie OR valid token OR flag| Delete
File Changes1. src/node/db/PadDeletionManager.ts
|
Code Review by Qodo
Context used 1. Deletion flag unusable in UI
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+1004
to
+1012
| // 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
Comment on lines
520
to
+546
|
|
||
| // 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
Comment on lines
+13
to
+20
| 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
This was referenced Apr 19, 2026
JohnMcLear
force-pushed
the
feat-gdpr-pad-deletion
branch
2 times, most recently
from
64c3527 to
f64ba91
Compare
Member
The button is kind of off. It looks more like a text than something I can click on.
|
Member
Author
|
Yea sorry this needs to be draft. |
ⓘ You've reached your Qodo monthly free-tier limit. Reviews pause until next month — upgrade your plan to continue now, or link your paid account if you already have one. |
|
Persistent review updated to latest commit baa11b5 |
Member
Author
|
Changed some strings, based, fixed some practical and some visual bugs.. Should be pretty much ready for you to review again :) |
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.
|
|
||
| // create pad | ||
| await getPadSafe(padID, false, text, authorId); | ||
| return {deletionToken: await padDeletionManager.createDeletionTokenIfAbsent(padID)}; |
There was a problem hiding this comment.
1. createpad response shape changed 📘 Rule violation ⚙ Maintainability
createPad now returns data: {deletionToken: ...} (or {deletionToken: null}) instead of `data:
null`, changing the response schema for existing API consumers. This is a potentially breaking
public API change that should be avoided or versioned/opt-in and clearly documented as breaking.
Agent Prompt
## Issue description
The `createPad` API response `data` changes from `null` to an object containing `deletionToken`, which can break clients that assume `data` is always `null` for this endpoint.
## Issue Context
This is a public HTTP API behavior change. To comply, preserve backward compatibility (or introduce a versioned/opt-in path).
## Fix Focus Areas
- src/node/db/API.ts[518-524]
- doc/api/http_api.md[519-536]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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>
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>
…1y/i18n - PadMessageHandler: a supplied deletion token must validate; do not fall back to the creator-cookie path when the token is wrong (was deleting the pad anyway when the creator pasted a wrong token into the field). - Skip token issuance + UI when requireAuthentication is on (creator identity is stable, recovery token is redundant noise). - Server emits messageKey instead of hardcoded English; both shout handlers (inline alert and global gritter) localize via html10n. - Suppress the global "Admin message" gritter for pad.deletionToken.* shouts to avoid the "Admin message: undefined" duplicate. - Token-modal a11y: role=dialog, aria-modal, aria-labelledby/describedby, visually-hidden label on the token input, aria-live on Copy, focus to the token input on open and restore on dismiss. - Style the "Delete Pad with Token" disclosure to match the Delete pad button; align the Copy/value row; pad the disclosure label. Tests: Playwright now covers the creator-with-wrong-token path, asserts no "Admin message" / "undefined" gritter on denial; backend API test covers requireAuthentication suppressing the token. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
JohnMcLear
force-pushed
the
feat-gdpr-pad-deletion
branch
from
baa11b5 to
703c5a3
Compare
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
Member
Author
|
/review |
ⓘ You've reached your Qodo monthly free-tier limit. Reviews pause until next month — upgrade your plan to continue now, or link your paid account if you already have one. |
|
Persistent review updated to latest commit 703c5a3 |
Comment on lines
+268
to
+278
| const suppliedToken = padDeleteMessage.data.deletionToken; | ||
| const tokenSupplied = typeof suppliedToken === 'string' && suppliedToken !== ''; | ||
| const tokenOk = tokenSupplied && | ||
| await padDeletionManager.isValidDeletionToken(padId, suppliedToken); | ||
| // When a token is supplied it must validate. We deliberately do NOT fall | ||
| // 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; | ||
|
|
There was a problem hiding this comment.
1. Deletion flag unusable in ui 🐞 Bug ≡ Correctness
allowPadDeletionByAllUsers is only honored when no deletionToken is supplied, but the only non-creator UI control always supplies a token, so enabling the flag still doesn’t let regular editors delete via the UI. This is also inconsistent with the REST deletePad behavior, which bypasses token checks when the flag is enabled even if a bogus token is provided.
Agent Prompt
### Issue description
`allowPadDeletionByAllUsers` is intended to allow any editor to delete without needing the one-time token, but the socket handler only applies the flag when *no token is supplied*. The UI’s non-creator delete path always supplies a token, so non-creators cannot delete via UI even when the flag is enabled.
### Issue Context
- Socket handler: `flagOk` is gated on `!tokenSupplied`.
- UI token-delete always sends `deletionToken`, so `tokenSupplied` is always true.
- Non-creators don’t have a UI path to send `PAD_DELETE` without a token because `canEditPadSettings` is creator-only.
- REST deletePad bypasses validation when the flag is enabled, even if a bogus token is supplied, so socket/REST are inconsistent.
### Fix Focus Areas
- src/node/handler/PadMessageHandler.ts[258-302]
- src/static/js/pad_editor.ts[140-200]
- src/node/handler/PadMessageHandler.ts[1123-1165]
- src/templates/pad.html[256-264]
- src/node/db/API.ts[543-552]
### Suggested approach
Choose one consistent policy and implement it across socket + UI:
1) **Server-side simplest:** If `settings.allowPadDeletionByAllUsers` is true, allow deletion regardless of `deletionToken` presence/validity (ignore token like REST does), and only use token validation when the flag is false.
2) **UI-side alternative:** Pass `allowPadDeletionByAllUsers` (or a `canDeletePad` boolean) via `clientVars` and, when enabled, have the UI send `PAD_DELETE` without `deletionToken` (or omit it) so the existing `flagOk` branch is reached.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+170
to
+173
| return { | ||
| padID, | ||
| deletionToken: await padDeletionManager.createDeletionTokenIfAbsent(padID), | ||
| }; |
There was a problem hiding this comment.
2. Group pads leak deletion token 🐞 Bug ⛨ Security
createGroupPad always creates/returns a deletion token, even when settings.requireAuthentication is enabled, contradicting createPad’s explicit decision to suppress token issuance in that mode. This reintroduces the “extra surface to leak” for group pads and creates inconsistent behavior between createPad and createGroupPad.
Agent Prompt
### Issue description
Group-pad creation (`createGroupPad`) always issues and returns a plaintext deletion token, even when `settings.requireAuthentication` is enabled. This contradicts `createPad`, which explicitly avoids issuing a token in that mode to reduce credential leakage surface.
### Issue Context
- `API.createPad` returns `deletionToken: null` when `settings.requireAuthentication` is true.
- `API.createGroupPad` is currently a direct export of `GroupManager.createGroupPad`, which always calls `createDeletionTokenIfAbsent()` and returns the plaintext.
### Fix Focus Areas
- src/node/db/GroupManager.ts[140-173]
- src/node/db/API.ts[46-52]
- src/node/db/API.ts[508-529]
### Suggested fix
Mirror `createPad` behavior:
- Import `settings` into `GroupManager.ts`.
- If `settings.requireAuthentication` is true, return `{padID, deletionToken: null}` and **do not** call `createDeletionTokenIfAbsent`.
- Keep the return type consistent (`string | null`) and update any related docs/tests if necessary.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
2 tasks
JohnMcLear
added a commit
that referenced
this pull request
May 1, 2026
PR #7546 added a relative link in `doc/privacy.md` pointing to `../docs/superpowers/specs/2026-04-18-gdpr-pr1-deletion-controls-design.md`, which lives outside vitepress's `doc/` source root. VitePress reports it as a dead link and the docs deploy on develop fails: (!) Found dead link ./../docs/superpowers/specs/2026-04-18-gdpr-pr1-deletion-controls-design in file /home/runner/work/etherpad/etherpad/doc/privacy.md Error: 1 dead link(s) found. Point the link at the file on GitHub instead so the published site resolves it and readers can still find the spec. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
3 tasks
JohnMcLear
added a commit
that referenced
this pull request
May 1, 2026
…theft (#7643) PR #7546 added a one-time pad-deletion-token modal that opens via the clientVars handshake on creator sessions and synchronously focuses its input through setTimeout(0). `goToNewPad`'s previous mitigation hid the modal element after `waitForEditorReady`, but the editor iframe attaches before clientVars arrives, so the hide runs against a still- hidden modal, short-circuits, and the modal opens later mid-test — stealing focus and dropping the next Enter / Tab. Visible on develop in `enter.spec.ts:33` and `indentation.spec.ts:9` across all four Playwright jobs (run 25214868650). Intercept `clientVars` assignment via `page.addInitScript` and null out `padDeletionToken` before `pad.ts`'s `showDeletionTokenModalIfPresent` can read it, so the modal-show short-circuits at the source. The deletion-token spec navigates inline with `page.goto` and does not call this helper, so its modal still appears. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
3 tasks
JohnMcLear
added a commit
that referenced
this pull request
May 1, 2026
* ci(docs): build on PRs and pin Node 22 (Qodo follow-up to #7640) Qodo flagged two reliability gaps on the oxc-minify fix that landed in #7640: 1. The Deploy Docs to GitHub Pages workflow only ran on push to develop, so a PR that broke `pnpm run docs:build` was not caught until after merge — exactly how the dead-link regression in #7546 escaped. Add a pull_request trigger that runs the same build but skips the deploy/upload steps via `if: github.event_name == 'push'`. Also include the workflow file itself in the path filter so changes to it are exercised on PR. 2. oxc-minify@0.128.0 requires Node ^20.19.0 || >=22.12.0, but the workflow did not pin Node and the repo declared engines.node >=22.0.0 with engineStrict: true — a runner image (or local dev) on Node 22.0–22.11 would refuse to install. Pin Node 22 in the docs workflow with actions/setup-node@v6 (matching the rest of CI), and bump engines.node to >=22.12.0 so the project's engineStrict gate matches the actual minimum. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * ci(docs): split build and deploy so PR runs do not hit pages env protection The previous attempt put `if: github.event_name == 'push'` on individual deploy steps but kept the single job's `environment: github-pages` binding. Environment protection rules reject any non-develop ref (including `refs/pull/N/merge`), so the runner failed the entire job at creation time before any step could execute: Branch "refs/pull/7645/merge" is not allowed to deploy to github-pages due to environment protection rules. Split into two jobs: `build` runs on every trigger (PR + push) and uploads the artifact only on push, `deploy` depends on `build`, runs only on push, and is the only job bound to the github-pages environment. Standard GHA pages-deploy pattern; PR builds never attempt to enter the protected environment. * docs: align Node minimum references with bumped engines.node (Qodo round 2 on #7645) Qodo flagged that engines.node moved from >=22.0.0 to >=22.12.0 in this PR but documentation still claimed the old requirement. Sync the three places that pinned a specific minimum: - README.md installation requirements (>= 22 → >= 22.12) - doc/npm-trusted-publishing.md publish prerequisites (>=22.0.0 → >=22.12.0, with oxc-minify cited as the driver) - CHANGELOG.md 2.7.3 breaking-changes entry (22 → 22.12, with the same oxc-minify justification) --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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