From 1692705a4b5bf6798a9c3ed14978d1992ef18de0 Mon Sep 17 00:00:00 2001 From: forkivan Date: Mon, 15 Jun 2026 15:40:26 +0200 Subject: [PATCH 1/2] PadManager: reject unreachable '.' and '..' pad ids isValidPadId accepted pad ids that consist only of URL dot-segments ('.' and '..'). Per the WHATWG URL standard a browser normalises "/p/." to "/p/" and "/p/.." to "/", so such a pad can never be opened or exported: the request arrives at "/p/" and Etherpad answers "Cannot GET /p/". The pad is created in the database but is forever unreachable. Reject these ids in isValidPadId so the broken pads can no longer be created, and add a regression test that fails without the fix. --- src/node/db/PadManager.ts | 10 +++- .../backend-new/specs/PadManager.test.ts | 56 +++++++++++++++++++ 2 files changed, 65 insertions(+), 1 deletion(-) create mode 100644 src/tests/backend-new/specs/PadManager.test.ts diff --git a/src/node/db/PadManager.ts b/src/node/db/PadManager.ts index 29226153103..28b0588fe60 100644 --- a/src/node/db/PadManager.ts +++ b/src/node/db/PadManager.ts @@ -192,7 +192,15 @@ exports.sanitizePadId = async (padId: string) => { return padId; }; -exports.isValidPadId = (padId: string) => /^(g.[a-zA-Z0-9]{16}\$)?[^$]{1,50}$/.test(padId); +// Pad IDs consisting only of URL "dot-segments" ('.' or '..') are unreachable: +// per the WHATWG URL standard a browser normalises "/p/." to "/p/" and "/p/.." +// to "/", so the pad can never be opened or exported — the request arrives as +// "/p/" and Etherpad answers "Cannot GET /p/". Reject them so such (broken) pads +// can never be created. +const dotSegmentPadId = /^\.{1,2}$/; + +exports.isValidPadId = (padId: string) => + /^(g.[a-zA-Z0-9]{16}\$)?[^$]{1,50}$/.test(padId) && !dotSegmentPadId.test(padId); /** * Removes the pad from database and unloads it. diff --git a/src/tests/backend-new/specs/PadManager.test.ts b/src/tests/backend-new/specs/PadManager.test.ts new file mode 100644 index 00000000000..5ce98532dc2 --- /dev/null +++ b/src/tests/backend-new/specs/PadManager.test.ts @@ -0,0 +1,56 @@ +/** + * Unit coverage for PadManager.isValidPadId. + * + * PadManager pulls in the database layer (`./DB`) and `Pad` at import time, so + * those are mocked away — isValidPadId is a pure function and must not require a + * running database to test. + */ + +import {describe, it, expect, vi} from 'vitest'; + +// Must appear before the PadManager import; vi.mock is hoisted by vitest. +vi.mock('../../../node/db/DB', () => ({ + default: {}, + get: vi.fn(), + set: vi.fn(), + remove: vi.fn(), +})); +vi.mock('../../../node/db/Pad', () => ({Pad: class {}})); + +// PadManager is a CommonJS module; under vitest it must be loaded with `import` +// (the CommonJS `require` resolver cannot resolve the `.ts` source). The mocks +// above are hoisted, so they are already in place when this import evaluates. +const mod: any = await import('../../../node/db/PadManager'); +const padManager = mod.default ?? mod; + +describe('PadManager.isValidPadId', () => { + it('accepts ordinary pad ids', () => { + for (const id of [ + 'foo', + 'TF-EVC', + 'TF-LEC_IP03-EMS-CSM', + 'a.b', + '.foo', + 'foo.', + "a'b", + 'g.s8oes9dhwrvt0zif$bar', // group pad + ]) { + expect(padManager.isValidPadId(id)).toBe(true); + } + }); + + // Regression test for "Cannot GET /p/": a pad id that is a URL dot-segment + // ('.' or '..') is normalised away by the browser per the WHATWG URL standard + // ('/p/.' -> '/p/', '/p/..' -> '/'), so the pad can never be opened or + // exported. Such ids must be rejected. Before the fix isValidPadId returned + // true for both, so this test would fail. + it('rejects URL dot-segment pad ids that would be unreachable', () => { + expect(padManager.isValidPadId('.')).toBe(false); + expect(padManager.isValidPadId('..')).toBe(false); + }); + + it('still rejects empty ids and ids containing "$"', () => { + expect(padManager.isValidPadId('')).toBe(false); + expect(padManager.isValidPadId('a$b')).toBe(false); + }); +}); From 9e5176d8c6273034a4dc83fd1dd859d5bc9badd3 Mon Sep 17 00:00:00 2001 From: forkivan Date: Mon, 15 Jun 2026 17:58:42 +0200 Subject: [PATCH 2/2] adminsettings: allow deleting legacy '.'/'..' pads The isValidPadId tightening makes getPad() reject the pad ids '.' and '..', which also blocks their deletion: a pad with such an id created before the change still exists (doesPadExists is true), so the admin deletePad handler takes the "healthy" branch where getPad() now throws. The outer catch swallowed that error without emitting a terminal results:deletePad, leaving an undeletable orphan in the admin UI. Fall back to the existing raw key purge when getPad() throws, so these pads can still be removed. Adds a regression test. --- src/node/hooks/express/adminsettings.ts | 23 +++++++++++++------ .../backend/specs/admin/padLoadResilience.ts | 23 +++++++++++++++++++ 2 files changed, 39 insertions(+), 7 deletions(-) diff --git a/src/node/hooks/express/adminsettings.ts b/src/node/hooks/express/adminsettings.ts index 678da256891..d52b07a6869 100644 --- a/src/node/hooks/express/adminsettings.ts +++ b/src/node/hooks/express/adminsettings.ts @@ -305,13 +305,22 @@ exports.socketio = (hookName: string, {io}: any) => { socket.on('deletePad', async (padId: string) => { try { if (await padManager.doesPadExists(padId)) { - // Healthy pad — full relational cleanup (revs, chat, readonly, - // authors, deletion token, hooks). - logger.info(`Deleting pad: ${padId}`); - const pad = await padManager.getPad(padId); - await pad.remove(); - socket.emit('results:deletePad', padId); - return; + try { + // Healthy pad — full relational cleanup (revs, chat, readonly, + // authors, deletion token, hooks). + logger.info(`Deleting pad: ${padId}`); + const pad = await padManager.getPad(padId); + await pad.remove(); + socket.emit('results:deletePad', padId); + return; + } catch (err) { + // getPad() runs isValidPadId() and rejects ids that are no longer + // valid — e.g. legacy '.'/'..' pads created before that validation + // was tightened. Don't give up: fall through to the raw key purge + // below so the orphan can still be deleted from the admin UI. + logger.warn(`Relational cleanup failed for "${padId}" ` + + `(${safeErr(err)}); falling back to raw key purge`); + } } // doesPadExists() is false either because nothing is stored under diff --git a/src/tests/backend/specs/admin/padLoadResilience.ts b/src/tests/backend/specs/admin/padLoadResilience.ts index 28c5d284f19..b69517d7410 100644 --- a/src/tests/backend/specs/admin/padLoadResilience.ts +++ b/src/tests/backend/specs/admin/padLoadResilience.ts @@ -177,4 +177,27 @@ describe(__filename, function () { assert.ok(!names.includes(corruptId), `corrupt pad still listed: ${JSON.stringify(names)}`); assert.ok(names.includes(goodId), `good pad missing after delete: ${JSON.stringify(names)}`); }); + + // Regression for the isValidPadId tightening that rejects '.' and '..': a + // legacy pad with such an id predates the validation, so it still exists in + // the DB (doesPadExists is true → deletePad takes the "healthy" branch) but + // getPad() now throws on it. deletePad must fall back to the raw key purge + // instead of failing silently, otherwise the orphan is undeletable from the + // admin UI. Before the fallback this `ask()` never gets `results:deletePad` + // and times out. + it("a legacy '.' pad (now an invalid id) can still be deleted", async function () { + this.timeout(30000); + const dotId = '.'; + // getPad('.') would now throw, so seed the record directly with a truthy + // `atext` so doesPadExists() returns true and the handler enters the branch + // where getPad() throws. + await db.set(`pad:${dotId}`, {atext: {text: '\n', attribs: ''}, pool: {}, head: -1, savedRevisions: []}); + try { + const ack = await ask(socket, 'deletePad', dotId, 'results:deletePad'); + assert.equal(ack, dotId, `expected deletePad to ack "${dotId}", got ${JSON.stringify(ack)}`); + } finally { + try { await db.remove(`pad:${dotId}`); } catch { /* ignore */ } + try { padManager.unloadPad(dotId); } catch { /* ignore */ } + } + }); });