fix: carry the pad deletion token across movePad (#7995) - #8089
Open
JohnMcLear wants to merge 2 commits into
Open
fix: carry the pad deletion token across movePad (#7995)#8089JohnMcLear wants to merge 2 commits into
JohnMcLear wants to merge 2 commits into
Conversation
`movePad` is `Pad.copy()` + `Pad.remove()`, but `copy()` only replicates the `pad:<id>`, `:revs:N` and `:chat:N` records — never `pad:<id>:deletionToken` — and `remove()` then dropped the source pad's token. So a renamed pad had no token at all: the token the creator was told to save stopped deleting anything, and because the copy preserves the revision-0 author the creator is still `isCreator` on arrival, so `createDeletionTokenIfAbsent()` minted a fresh one and popped a second "save your pad deletion token" modal. Hand the token record over to the destination as part of the move, so the saved token keeps working and the modal does not reappear. `force`-overwriting an existing destination discards that pad's own token along with its content. `copyPad` deliberately does not do this: two pads sharing one secret would let a token saved for one delete the other. Adds backend coverage for all four cases (three of them fail on the pre-fix code) plus http_api.md and CHANGELOG notes. Reported by @dcht00. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
PR Summary by QodoFix movePad to transfer pad deletionToken on rename
AI Description
Diagram
High-Level Assessment
Files changed (5)
|
There was a problem hiding this comment.
Pull request overview
Fixes Etherpad’s movePad behavior so a pad’s deletion token is preserved across a move (rename), preventing both (a) previously-issued tokens from becoming invalid and (b) the creator being prompted for a second token after the move.
Changes:
- Added
PadDeletionManager.transferDeletionToken(src, dst)to move the stored deletion token record between pad IDs. - Updated
API.movePadto transfer the token aftercopy()and beforeremove(). - Added backend regression tests for move/copy token behavior; updated HTTP API docs and the changelog.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/node/db/PadDeletionManager.ts |
Adds deletion-token transfer helper used during pad moves. |
src/node/db/API.ts |
Calls token transfer during movePad to preserve token across rename. |
src/tests/backend/specs/api/movePadDeletionToken.ts |
Adds regression coverage for token behavior across movePad and copyPad. |
doc/api/http_api.md |
Documents movePad deletion-token behavior (including force overwrite semantics). |
CHANGELOG.md |
Adds release note describing the fix and its user-visible impact. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+55
to
+60
| if (stored == null) { | ||
| // The destination pad is being replaced wholesale, so any token it still | ||
| // carries belongs to content that no longer exists. | ||
| await DB.db.remove(getDeletionTokenKey(dstPadId)); | ||
| return; | ||
| } |
Code Review by Qodo
1. Token transfer not locked
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+54
to
+55
| const stored = await DB.db.get(getDeletionTokenKey(srcPadId)); | ||
| if (stored == null) { |
There was a problem hiding this comment.
2. Token transfer not locked 🐞 Bug ☼ Reliability
transferDeletionToken() is not serialized with createDeletionTokenIfAbsent(), so concurrent creator connections can interleave token creation with a move and cause the transfer to observe “no token” or be overwritten by a late token creation. This can still result in a moved pad with no usable carried-over token (or a token different from what the creator was shown).
Agent Prompt
### Issue description
`createDeletionTokenIfAbsent()` uses a per-pad in-memory queue (`inflightCreate`) to prevent concurrent token creations from racing and invalidating the plaintext token returned to the creator. `transferDeletionToken()` introduces another token-mutating path (read/set/remove across two pads) but does not participate in that serialization, so it can race with token creation on either the source or destination pad.
### Issue Context
- Token creation is triggered on creator connect (`PadMessageHandler`) and on API/group pad creation.
- `movePad` now runs `transferDeletionToken` after the destination pad has been created by `Pad.copy()` but before the source is removed.
### Fix Focus Areas
- src/node/db/PadDeletionManager.ts[13-63]
- src/node/handler/PadMessageHandler.ts[1322-1366]
- src/node/db/API.ts[754-776]
### Suggested change
- Introduce a shared per-pad token-operation queue/lock (not just for "create") and use it for **both** `createDeletionTokenIfAbsent()` and `transferDeletionToken()`.
- For transfer across two pads, acquire locks in a deterministic order (e.g., lexicographic by padId) to avoid deadlocks, then perform the `get`/`set` (and any cleanup) while holding the locks.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Code Review by Qodo
Context used 1. Premature token deletion
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+769
to
774
| // A move is a rename, so the pad's deletion token travels with it: the token | ||
| // the creator saved keeps working, and returning to the renamed pad does not | ||
| // hand them a second one (issue #7995). Must run before remove(), which drops | ||
| // the source pad's token record. | ||
| await padDeletionManager.transferDeletionToken(sourceID, destinationID); | ||
| await pad.remove(); |
There was a problem hiding this comment.
2. Movepad token race 🐞 Bug ≡ Correctness
There is a window where Pad.copy() has written the destination pad records but movePad has not yet transferred the deletion token. If the creator opens the destination during that window, createDeletionTokenIfAbsent(destinationID) can mint and return a plaintext token that transferDeletionToken later overwrites, leaving the user with an invalid saved token.
Agent Prompt
### Issue description
`movePad` currently performs `pad.copy()` (which creates the destination pad records) and only afterwards transfers the deletion token. During that interval, the destination pad can be opened and the creator path can call `createDeletionTokenIfAbsent(dst)`, returning a plaintext token that becomes invalid once the transfer overwrites the stored hash.
### Issue Context
- `Pad.copy()` writes `pad:${destinationID}...` records directly.
- `CLIENT_READY` can call `createDeletionTokenIfAbsent(padId)` and return a plaintext token exactly once.
- `transferDeletionToken()` overwrites the destination token record.
### Fix Focus Areas
- Eliminate or reduce the copy→transfer window by moving the token write into the copy/move operation (e.g., add a move-specific option/path in `Pad.copy()` or implement a dedicated `Pad.move()` that sets the token as part of the DB writes).
- Alternatively, introduce a move-in-progress guard so destination token creation cannot emit a plaintext token until after transfer is completed.
### Fix Focus Areas (code refs)
- src/node/db/API.ts[766-775]
- src/node/db/Pad.ts[634-673]
- src/node/db/PadDeletionManager.ts[20-35]
- src/node/db/PadDeletionManager.ts[53-63]
- src/node/handler/PadMessageHandler.ts[1324-1366]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Review feedback on #8089: - Don't remove the source token in `transferDeletionToken()`. `movePad` calls it before `pad.remove()`, which drops the record itself on success — so the eager removal only mattered when `remove()` failed, and there it stranded the creator of a source pad that survived. - Don't overwrite a token the destination has already issued. `Pad.copy()` writes the destination records before the transfer runs, so a creator opening the new id in that window is shown a freshly minted token; that token has been handed out in plaintext and has to keep working. - Run the transfer through the same per-pad queue as `createDeletionTokenIfAbsent()` (extracted as `withPadTokenLock`), so a concurrent mint can't interleave with the transfer's read-then-write. - Drop the "source has no token" branch that cleared the destination's: a force-overwrite already drops the replaced pad's token via `Pad.remove()`, so the branch could only clobber a live token. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Member
Author
|
Actioned the review feedback in ea49ba8 — both bots landed on the same two real issues. 1. Source token removed early (qodo ×2) — valid, fixed. 2. copy → transfer window (qodo #2) — valid, fixed. Not taken: a lock across both pad ids. Only the destination's slot is read-then-written; the source is about to be deleted, and a two-id lock needs deterministic ordering for no benefit here. 3. Copilot — reword the Added a regression test for the no-clobber guard (destination token issued mid-move survives; source token survives the transfer). Backend suite: 1618 passing, 0 failing. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/node/db/PadDeletionManager.ts:30
- withPadTokenLock() stores
tracked(the promise returned by.finally()) in the inflight map but returnsnext. Iffn()rejects, callers handlenext’s rejection, buttrackedis a separate rejecting promise with no handler, which can trigger an unhandled rejection. Returningtrackedalso removes the need forPromise<any>in the map type.
return next;
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
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.
Closes #7995 (the "moved pads don't keep, offer a new deletion token" half — the "let me turn the popup off" half is #7996).
Repro (from @dcht00)
abc→ deletion-token modal, save the tokenmovePadabc→defdef→ a second deletion-token modal, and the token saved in step 1 no longer deletes anythingRoot cause
API.movePadisPad.copy()+Pad.remove():Pad.copy()replicates only thepad:<id>,pad:<id>:revs:Nandpad:<id>:chat:Nrecords — it never touchespad:<id>:deletionToken(src/node/db/Pad.ts).Pad.remove()then callspadDeletionManager.removeDeletionToken(sourceID). Net effect: the token record is destroyed and never recreated at the new id.Two visible consequences:
deletePad(def, <token>)returnsinvalid deletionToken;isCreator,createDeletionTokenIfAbsent()finds an empty slot, andPadMessageHandlerships a freshpadDeletionToken→ the modal fires a second time.Fix
New
PadDeletionManager.transferDeletionToken(src, dst), called frommovePadbetweencopy()andremove()(it must run beforeremove()drops the source record). A move is a rename, so the token moves with the pad.force-overwriting an existing destination discards that pad's own token —copy()already removed the overwritten pad and its content, so its token would be a key to nothing.copyPadis deliberately left alone. A copy is a separate pad; sharing one hash between two pads would mean a token saved for one deletes the other. The copy gets its own token on the creator's first visit, as today.Tests
src/tests/backend/specs/api/movePadDeletionToken.ts— 4 cases:invalid deletionTokenmovePad --forcereplaces the destination's own tokencopyPaddoes not share the source tokenFull backend suite:
1617 passing, 0 failing.pnpm run ts-checkclean.Docs:
movePadsection ofdoc/api/http_api.md+ CHANGELOG entry.Reported by @dcht00.
🤖 Generated with Claude Code