diff --git a/src/node/db/PadManager.ts b/src/node/db/PadManager.ts index 292261531..28b0588fe 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 678da2568..d52b07a68 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/PadManager.ts b/src/tests/backend/specs/PadManager.ts new file mode 100644 index 000000000..c4ddf79a0 --- /dev/null +++ b/src/tests/backend/specs/PadManager.ts @@ -0,0 +1,48 @@ +'use strict'; + +// Unit coverage for PadManager.isValidPadId. +// +// isValidPadId is a pure function (a regex test), so this spec just requires +// PadManager and exercises it directly — no running database is needed. +// PadManager's import-time `require`s (DB, Pad, customError) only *define* +// things; the database connection happens lazily in DB.init(), so loading the +// module here has no side effects. This runs under the mocha backend suite +// (`--import=tsx`), where `require()` resolves the `.ts` sources natively. + +import {strict as assert} from 'assert'; + +const padManager = require('../../../node/db/PadManager'); + +describe(__filename, function () { + describe('isValidPadId', function () { + it('accepts ordinary pad ids', async function () { + for (const id of [ + 'foo', + 'TF-EVC', + 'TF-LEC_IP03-EMS-CSM', + 'a.b', + '.foo', + 'foo.', + "a'b", + 'g.s8oes9dhwrvt0zif$bar', // group pad + ]) { + assert.equal(padManager.isValidPadId(id), true, `expected "${id}" to be valid`); + } + }); + + // 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', async function () { + assert.equal(padManager.isValidPadId('.'), false); + assert.equal(padManager.isValidPadId('..'), false); + }); + + it('still rejects empty ids and ids containing "$"', async function () { + assert.equal(padManager.isValidPadId(''), false); + assert.equal(padManager.isValidPadId('a$b'), false); + }); + }); +}); diff --git a/src/tests/backend/specs/admin/padLoadResilience.ts b/src/tests/backend/specs/admin/padLoadResilience.ts index 28c5d284f..b69517d74 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 */ } + } + }); });