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/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-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); + }); +}); 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 */ } + } + }); });