From 0c0f5a8ec473efc59a75e3e57e649da2365bc512 Mon Sep 17 00:00:00 2001
From: forkivan <157495907+forkivan@users.noreply.github.com>
Date: Tue, 16 Jun 2026 12:13:02 +0200
Subject: [PATCH] PadManager: reject unreachable '.' and '..' pad ids (#7962)

* 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.

* 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.

* tests: run the isValidPadId regression under the mocha suite

The original unit test lived in the vitest backend-new suite, but PadManager
loads DB, Pad and customError with CommonJS require() at import time. Under
vitest those require() calls are resolved by Node natively and fail on the .ts
sources ("Cannot find module '../utils/customError'"); vi.mock could not
intercept them, so the suite errored before any test ran. It also used a
top-level `await import`, which tripped tsc TS1378 under the project tsconfig.

Move the test to the mocha backend suite, which runs with --import=tsx and
resolves the .ts requires natively, so PadManager can be required directly with
no mocking. isValidPadId is a pure function and DB only connects lazily in
DB.init(), so loading the module has no side effects and no database is needed.
---
 src/node/db/PadManager.ts                     | 10 +++-
 src/node/hooks/express/adminsettings.ts       | 23 ++++++---
 src/tests/backend/specs/PadManager.ts         | 48 +++++++++++++++++++
 .../backend/specs/admin/padLoadResilience.ts  | 23 +++++++++
 4 files changed, 96 insertions(+), 8 deletions(-)
 create mode 100644 src/tests/backend/specs/PadManager.ts

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