Skip to content

Commit 64c3527

Browse files
JohnMcLearclaude
andcommitted
fix(gdpr): PadDeletionManager race + document createPad/deletePad
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>
1 parent f6f2d58 commit 64c3527

File tree

3 files changed

+58
-9
lines changed

3 files changed

+58
-9
lines changed

doc/api/http_api.md

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -519,12 +519,20 @@ Group pads are normal pads, but with the name schema GROUPID$PADNAME. A security
519519
#### createPad(padID, [text], [authorId])
520520
* API >= 1
521521
* `authorId` in API >= 1.3.0
522+
* returns `deletionToken` once, since the same release that added `allowPadDeletionByAllUsers`
522523

523524
creates a new (non-group) pad. Note that if you need to create a group Pad, you should call **createGroupPad**.
524525
You get an error message if you use one of the following characters in the padID: "/", "?", "&" or "#".
525526

527+
`data.deletionToken` is a one-shot recovery token tied to this pad. It is
528+
returned in plaintext on the first call for a given padID and is `null` on
529+
subsequent calls (the token itself is stored on the server as a sha256 hash).
530+
Pass it to **deletePad** (or the socket `PAD_DELETE` message) to delete the
531+
pad without the creator's author cookie.
532+
526533
*Example returns:*
527-
* `{code: 0, message:"ok", data: null}`
534+
* `{code: 0, message:"ok", data: {deletionToken: "…32-char random string…"}}`
535+
* `{code: 0, message:"ok", data: {deletionToken: null}}` — pad already existed
528536
* `{code: 1, message:"padID does already exist", data: null}`
529537
* `{code: 1, message:"malformed padID: Remove special characters", data: null}`
530538

@@ -581,14 +589,24 @@ returns the list of users that are currently editing this pad
581589
* `{code: 0, message:"ok", data: {padUsers: [{colorId:"#c1a9d9","name":"username1","timestamp":1345228793126,"id":"a.n4gEeMLsvg12452n"},{"colorId":"#d9a9cd","name":"Hmmm","timestamp":1345228796042,"id":"a.n4gEeMLsvg12452n"}]}}`
582590
* `{code: 0, message:"ok", data: {padUsers: []}}`
583591

584-
#### deletePad(padID)
592+
#### deletePad(padID, [deletionToken])
585593
* API >= 1
594+
* `deletionToken` in the same release as `allowPadDeletionByAllUsers`
595+
596+
deletes a pad.
586597

587-
deletes a pad
598+
`deletionToken` is the one-shot recovery token returned by `createPad` /
599+
`createGroupPad`. An apikey-authenticated caller can pass any (or no) token
600+
and the call still succeeds — trusted admins bypass the check. An
601+
unauthenticated caller (or a caller that explicitly passes a wrong token)
602+
is rejected with `invalid deletionToken` unless the operator has set
603+
`allowPadDeletionByAllUsers: true` in `settings.json`, in which case the
604+
token is ignored.
588605

589606
*Example returns:*
590607
* `{code: 0, message:"ok", data: null}`
591608
* `{code: 1, message:"padID does not exist", data: null}`
609+
* `{code: 1, message:"invalid deletionToken", data: null}`
592610

593611
#### copyPad(sourceID, destinationID[, force=false])
594612
* API >= 1.2.8

src/node/db/PadDeletionManager.ts

Lines changed: 21 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -10,14 +10,29 @@ const getDeletionTokenKey = (padId: string) => `pad:${padId}:deletionToken`;
1010
const hashDeletionToken = (deletionToken: string) =>
1111
crypto.createHash('sha256').update(deletionToken, 'utf8').digest();
1212

13+
// Per-pad serialisation for token creation. Without this, two concurrent
14+
// `createDeletionTokenIfAbsent()` calls for the same pad can both observe
15+
// an empty slot, both write a hash, and leave the earlier caller holding a
16+
// plaintext token that no longer validates. The chain is cleaned up once the
17+
// outstanding call resolves so this map doesn't grow unbounded.
18+
const inflightCreate: Map<string, Promise<string | null>> = new Map();
19+
1320
exports.createDeletionTokenIfAbsent = async (padId: string): Promise<string | null> => {
14-
if (await DB.db.get(getDeletionTokenKey(padId)) != null) return null;
15-
const deletionToken = randomString(32);
16-
await DB.db.set(getDeletionTokenKey(padId), {
17-
createdAt: Date.now(),
18-
hash: hashDeletionToken(deletionToken).toString('hex'),
21+
const prior = inflightCreate.get(padId);
22+
const next = (prior || Promise.resolve()).then(async () => {
23+
if (await DB.db.get(getDeletionTokenKey(padId)) != null) return null;
24+
const deletionToken = randomString(32);
25+
await DB.db.set(getDeletionTokenKey(padId), {
26+
createdAt: Date.now(),
27+
hash: hashDeletionToken(deletionToken).toString('hex'),
28+
});
29+
return deletionToken;
30+
});
31+
const tracked = next.finally(() => {
32+
if (inflightCreate.get(padId) === tracked) inflightCreate.delete(padId);
1933
});
20-
return deletionToken;
34+
inflightCreate.set(padId, tracked);
35+
return next;
2136
};
2237

2338
exports.isValidDeletionToken = async (padId: string, deletionToken: string | null | undefined) => {

src/tests/backend/specs/padDeletionManager.ts

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,22 @@ describe(__filename, function () {
3737
await padDeletionManager.removeDeletionToken(a);
3838
await padDeletionManager.removeDeletionToken(b);
3939
});
40+
41+
it('concurrent calls for the same pad produce a single validating token',
42+
async function () {
43+
const padId = uniqueId();
44+
const results = await Promise.all(
45+
Array.from({length: 8},
46+
() => padDeletionManager.createDeletionTokenIfAbsent(padId)));
47+
// Exactly one caller should get the plaintext token; the rest see null.
48+
const nonNull = results.filter((r) => r != null);
49+
assert.equal(nonNull.length, 1, `results: ${JSON.stringify(results)}`);
50+
const [token] = nonNull;
51+
assert.equal(
52+
await padDeletionManager.isValidDeletionToken(padId, token), true,
53+
'the one token returned must validate against the stored hash');
54+
await padDeletionManager.removeDeletionToken(padId);
55+
});
4056
});
4157

4258
describe('isValidDeletionToken', function () {

0 commit comments

Comments
 (0)