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

@@ -108,7 +108,15 @@ const padList = new class {
  */
 exports.getPad = async (id: string, text?: string|null, authorId:string|null = ''):Promise<PadType> => {
   // check if this is a valid padId
-  if (!exports.isValidPadId(id)) {
+  //
+  // An id that is no longer valid to *create* is still served when a pad with
+  // that exact id already exists: `:` was accepted until GHSA-wg58-mhwv-35pq, so
+  // pads carrying one exist in the wild (that is why padIdTransforms maps `:` at
+  // all) and rejecting them here would lock their content away. doesPadExist()
+  // requires a top-level `atext`, which only a real pad record has — the
+  // `pad:<id>:revs:<n>` / `:chat:<n>` sub-records an injected id would address do
+  // not have one, so they stay unreachable.
+  if (!exports.isValidPadId(id) && !(await exports.doesPadExist(id))) {
     throw new CustomError(`${id} is not a valid padId`, 'apierror');
   }
 
@@ -199,8 +207,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,32 @@ 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);
 
+      // A pad that already exists keeps its URL even if its id is no longer a
+      // valid one to *create*: `:` was accepted by isValidPadId until
+      // GHSA-wg58-mhwv-35pq, so pads carrying one exist in the wild (that is why
+      // padIdTransforms maps `:` in the first place) and 404ing them would lock
+      // their content away. Only ids that are invalid AND unknown are rejected —
+      // opening a pad URL creates the pad, so this is what keeps new invalid ids
+      // out.
+      if (!padManager.isValidPadId(sanitizedPadId) &&
+          !(await padManager.doesPadExist(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/padIdColonLegacyPad.ts

@@ -0,0 +1,57 @@
+'use strict';
+
+/**
+ * GHSA-wg58-mhwv-35pq made `:` invalid in a pad id. `:` was legal before, so
+ * pads carrying one exist in the wild (padIdTransforms maps `:` -> `_` precisely
+ * to keep those reachable). Rejecting them outright would lock their content
+ * away, so an invalid id is only refused when no pad with that exact id exists.
+ *
+ * The injection primitive the advisory closes stays closed: doesPadExist()
+ * requires a top-level `atext`, which only a real pad record has — the
+ * `pad:<id>:revs:<n>` sub-records an injected id addresses do not have one.
+ */
+
+const assert = require('assert').strict;
+const common = require('../common');
+const db = require('../../../node/db/DB');
+const padManager = require('../../../node/db/PadManager');
+
+const LEGACY_ID = 'wg58legacy:pad';
+
+let agent: any;
+
+describe(__filename, function () {
+  this.timeout(30000);
+
+  before(async function () {
+    agent = await common.init();
+    // Plant a legacy pad the way an old Etherpad would have stored it: `:` in the
+    // id is no longer creatable through getPad(), so write the record directly.
+    const donor = await padManager.getPad('wg58legacy_donor', 'legacy content\n');
+    await donor.saveToDatabase();
+    await db.set(`pad:${LEGACY_ID}`, await db.get('pad:wg58legacy_donor'));
+    padManager.unloadPad(LEGACY_ID);
+  });
+
+  it('getPad still serves an existing pad whose id contains ":"', async function () {
+    const pad = await padManager.getPad(LEGACY_ID);
+    assert.equal(pad.id, LEGACY_ID);
+    assert.match(pad.text(), /legacy content/);
+  });
+
+  it('the legacy pad URL is not 404ed', async function () {
+    const res = await agent.get(`/p/${encodeURIComponent(LEGACY_ID)}`);
+    assert.notEqual(res.status, 404, 'an existing legacy pad must stay reachable');
+  });
+
+  it('getPad still rejects a ":" id that is not an existing pad', async function () {
+    // The injection target: `pad:<victim>:revs:0` exists as a db record but is not
+    // a pad (no top-level atext), so it must not be addressable as a pad id.
+    const victim = await padManager.getPad('wg58legacy_victim', 'victim\n');
+    await victim.saveToDatabase();
+    assert.ok(await db.get('pad:wg58legacy_victim:revs:0'), 'victim rev-0 record exists');
+    await assert.rejects(
+        padManager.getPad('wg58legacy_victim:revs:0'),
+        /valid padId|apierror/i);
+  });
+});

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