security(padid): reject ueberdb delimiter ':' in pad ids (copyPad/movePad injection)

src/node/db/Pad.ts

@@ -632,6 +632,15 @@ class Pad {
   }
 
   async copy(destinationID: string, force: boolean) {
+    // Reject a destinationID that isn't a valid pad id BEFORE any db write. The
+    // copy path writes `pad:${destinationID}...` records directly (bypassing
+    // getPad), so a destinationID carrying the ueberdb delimiter `:` would
+    // otherwise clobber another pad's internal sub-records and slip past the
+    // force=false existence guard. (GHSA-wg58-mhwv-35pq.)
+    if (!padManager.isValidPadId(destinationID)) {
+      throw new CustomError('destinationID is not a valid padId', 'apierror');
+    }
+
     // Kick everyone from this pad.
     // This was commented due to https://github.com/ether/etherpad-lite/issues/3183.
     // Do we really need to kick everyone out?
@@ -729,6 +738,12 @@ class Pad {
   }
 
   async copyPadWithoutHistory(destinationID: string, force: string|boolean, authorId = '') {
+    // See copy(): reject an invalid destinationID (notably one containing the
+    // ueberdb delimiter `:`) before any db write. (GHSA-wg58-mhwv-35pq.)
+    if (!padManager.isValidPadId(destinationID)) {
+      throw new CustomError('destinationID is not a valid padId', 'apierror');
+    }
+
     // flush the source pad
     this.saveToDatabase();
 

src/node/db/PadManager.ts

@@ -199,8 +199,13 @@ exports.sanitizePadId = async (padId: string) => {
 // can never be created.
 const dotSegmentPadId = /^\.{1,2}$/;
 
+// `:` is the ueberdb key-namespace delimiter (records are stored under
+// `pad:<id>`, `pad:<id>:revs:<n>`, `pad:<id>:chat:<n>`). A pad id containing a
+// `:` can therefore address another pad's internal sub-records, so it is never
+// valid — the name portion excludes `$` (group-pad separator) and `:`.
+// (GHSA-wg58-mhwv-35pq: copyPad/movePad destinationID injection.)
 exports.isValidPadId = (padId: string) =>
-  /^(g.[a-zA-Z0-9]{16}\$)?[^$]{1,50}$/.test(padId) && !dotSegmentPadId.test(padId);
+  /^(g.[a-zA-Z0-9]{16}\$)?[^$:]{1,50}$/.test(padId) && !dotSegmentPadId.test(padId);
 
 /**
  * Removes the pad from database and unloads it.

src/node/hooks/express/padurlsanitize.ts

@@ -8,14 +8,24 @@ exports.expressCreateServer = (hookName:string, args:ArgsExpressType, cb:Functio
   // redirects browser to the pad's sanitized url if needed. otherwise, renders the html
   args.app.param('pad', (req:any, res:any, next:Function, padId:string) => {
     (async () => {
-      // ensure the padname is valid and the url doesn't end with a /
-      if (!padManager.isValidPadId(padId) || /\/$/.test(req.url)) {
+      // Reject URLs ending in `/` outright.
+      if (/\/$/.test(req.url)) {
         res.status(404).send('Such a padname is forbidden');
         return;
       }
 
+      // Sanitize FIRST, then validate the sanitized result. sanitizePadId maps
+      // legacy characters (whitespace and the ueberdb delimiter `:`) to `_`, so
+      // a URL like `/p/foo:bar` still redirects to `/p/foo_bar` even though `:`
+      // is not itself a valid pad id (GHSA-wg58-mhwv-35pq). An id that stays
+      // invalid after sanitizing (e.g. one containing `$`) is forbidden.
       const sanitizedPadId = await padManager.sanitizePadId(padId);
 
+      if (!padManager.isValidPadId(sanitizedPadId)) {
+        res.status(404).send('Such a padname is forbidden');
+        return;
+      }
+
       if (sanitizedPadId === padId) {
         // the pad id was fine, so just render it
         next();

src/tests/backend/specs/PadManager.ts

@@ -44,5 +44,15 @@ describe(__filename, function () {
       assert.equal(padManager.isValidPadId(''), false);
       assert.equal(padManager.isValidPadId('a$b'), false);
     });
+
+    // `:` is the ueberdb key-namespace delimiter (`pad:<id>:revs:<n>`). A pad id
+    // carrying a `:` can address another pad's internal sub-records, so it must
+    // never be a valid pad id. Regression for the copyPad/movePad destinationID
+    // injection (GHSA-wg58-mhwv-35pq).
+    it('rejects ids containing the ueberdb delimiter ":"', async function () {
+      assert.equal(padManager.isValidPadId('victim:revs:0'), false);
+      assert.equal(padManager.isValidPadId('a:b'), false);
+      assert.equal(padManager.isValidPadId('g.s8oes9dhwrvt0zif$bar:revs:0'), false);
+    });
   });
 });

src/tests/backend/specs/copyPadColonInjection.ts

@@ -0,0 +1,57 @@
+'use strict';
+
+/**
+ * Regression for GHSA-wg58-mhwv-35pq: copyPad/movePad/copyPadWithoutHistory
+ * accepted a `destinationID` containing the ueberdb key delimiter `:`
+ * (e.g. `victim:revs:0`), which bypassed the force=false overwrite guard and
+ * clobbered another pad's internal revision records.
+ */
+
+const assert = require('assert').strict;
+const common = require('../common');
+const api = require('../../../node/db/API');
+const padManager = require('../../../node/db/PadManager');
+
+describe(__filename, function () {
+  this.timeout(30000);
+
+  before(async function () {
+    await common.init();
+  });
+
+  it('rejects a copyPad destinationID that targets another pad\'s revs record', async function () {
+    const victim = 'wg58_victim';
+    const src = 'wg58_src';
+    await padManager.getPad(victim, 'TOP-SECRET-victim-content\n');
+    await padManager.getPad(src, 'attacker source\n');
+
+    const before = await api.getRevisionChangeset(victim, '0');
+    assert.ok(before, 'victim rev-0 changeset exists before the attack');
+
+    // force=false — the whole point is that `:` bypassed the existence guard.
+    await assert.rejects(
+        api.copyPad(src, `${victim}:revs:0`, false),
+        /valid padId|apierror/i,
+        'copyPad must reject a destinationID containing ":"');
+
+    const after = await api.getRevisionChangeset(victim, '0');
+    assert.equal(after, before, 'victim rev-0 changeset must be untouched');
+  });
+
+  it('rejects copyPadWithoutHistory with a ":" destinationID', async function () {
+    const src = 'wg58_src2';
+    await padManager.getPad(src, 'src2\n');
+    await assert.rejects(
+        api.copyPadWithoutHistory(src, 'other:revs:0', false),
+        /valid padId|apierror/i);
+  });
+
+  it('still allows a normal copyPad to a clean destination', async function () {
+    const src = 'wg58_ok_src';
+    await padManager.getPad(src, 'hello\n');
+    // force=true so the test is idempotent across runs (the backend DB persists
+    // pads between runs); the point here is that a valid destinationID copies.
+    await api.copyPad(src, 'wg58_ok_dst', true);
+    assert.ok(await padManager.doesPadExist('wg58_ok_dst'), 'destination pad exists after copy');
+  });
+});

src/tests/backend/specs/padUrlColonRedirect.ts

@@ -0,0 +1,34 @@
+'use strict';
+
+/**
+ * `:` is rejected as a pad id (GHSA-wg58-mhwv-35pq), but a browser visiting a
+ * legacy `/p/<id with ":">` URL must still be redirected to the sanitized `_`
+ * form (padManager.sanitizePadId maps `:` -> `_`) rather than getting a 404.
+ * Regression guard for the padurlsanitize ordering.
+ */
+
+const assert = require('assert').strict;
+const common = require('../common');
+
+let agent: any;
+
+describe(__filename, function () {
+  this.timeout(30000);
+  before(async function () { agent = await common.init(); });
+
+  it('redirects a legacy pad URL containing ":" to the sanitized "_" form', async function () {
+    const res = await agent.get('/p/foo:bar').expect(302);
+    assert.match(res.headers.location, /foo_bar/,
+        `expected redirect to the sanitized id, got Location: ${res.headers.location}`);
+  });
+
+  it('still 404s a pad id containing "$" (no sanitizing transform)', async function () {
+    await agent.get('/p/foo$bar').expect(404);
+  });
+
+  it('serves a clean pad id without redirecting', async function () {
+    // A valid id renders (200) or is handled normally — never a sanitize redirect.
+    const res = await agent.get('/p/CleanPadId123');
+    assert.notEqual(res.status, 404);
+  });
+});