From 02e80c595dbe5af9752c2bd291bbc5cabbff7676 Mon Sep 17 00:00:00 2001 From: John McLear Date: Sat, 25 Jul 2026 18:36:56 +0100 Subject: [PATCH 1/2] security(padid): reject ueberdb delimiter ':' in pad ids (copyPad/movePad injection) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GHSA-wg58-mhwv-35pq. copyPad / movePad / copyPadWithoutHistory take their destination via the `destinationID` API field, which — unlike padID / padName — is never run through sanitizePadId. Because isValidPadId only forbade `$`, a destinationID like `victim:revs:0` survived into the engine: the embedded `:` (the ueberdb key-namespace delimiter) let it address another pad's internal `pad::revs:` records. That both bypassed the force=false "destination already exists" guard (which checks only the top-level `atext`) and clobbered the victim pad's revision history. - isValidPadId now rejects `:` (never legal in a pad id; it's the DB key delimiter). The name portion excludes `$` and `:`. - Pad.copy() and Pad.copyPadWithoutHistory() validate destinationID via isValidPadId before any db write; movePad routes through copy(), so the check runs before the source is removed. Adds isValidPadId unit cases and an integration test proving copyPad / copyPadWithoutHistory reject a `:`-bearing destinationID with force=false and leave the victim's rev-0 changeset untouched, while a normal copy still works. Reported privately (finder credited on the advisory). Co-Authored-By: Claude Opus 4.8 (1M context) --- src/node/db/Pad.ts | 15 +++++ src/node/db/PadManager.ts | 7 ++- src/tests/backend/specs/PadManager.ts | 10 ++++ .../backend/specs/copyPadColonInjection.ts | 57 +++++++++++++++++++ 4 files changed, 88 insertions(+), 1 deletion(-) create mode 100644 src/tests/backend/specs/copyPadColonInjection.ts diff --git a/src/node/db/Pad.ts b/src/node/db/Pad.ts index 8ab3ac9d944..d879f9eaeb1 100644 --- a/src/node/db/Pad.ts +++ b/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(); diff --git a/src/node/db/PadManager.ts b/src/node/db/PadManager.ts index 28b0588fe60..91238ff61c8 100644 --- a/src/node/db/PadManager.ts +++ b/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:`, `pad::revs:`, `pad::chat:`). 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. diff --git a/src/tests/backend/specs/PadManager.ts b/src/tests/backend/specs/PadManager.ts index c4ddf79a057..39f4b476d69 100644 --- a/src/tests/backend/specs/PadManager.ts +++ b/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::revs:`). 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); + }); }); }); diff --git a/src/tests/backend/specs/copyPadColonInjection.ts b/src/tests/backend/specs/copyPadColonInjection.ts new file mode 100644 index 00000000000..7924e5dd398 --- /dev/null +++ b/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'); + }); +}); From 6781c98e899bf2916e739760b285e793a4d2b4ba Mon Sep 17 00:00:00 2001 From: John McLear Date: Sat, 25 Jul 2026 19:04:06 +0100 Subject: [PATCH 2/2] security(padid): sanitize pad URL before validating so legacy ":" URLs still redirect Addresses Qodo review on #8073. Rejecting ":" in isValidPadId made padurlsanitize's validate-first ordering return 404 for a browser visiting a legacy `/p/` URL, instead of redirecting it to the sanitized `_` form. Reorder padurlsanitize to sanitize FIRST (sanitizePadId maps whitespace and ":" to "_"), then validate the sanitized id. `/p/foo:bar` now redirects to `/p/foo_bar` as before; an id that stays invalid after sanitizing (e.g. containing "$") is still forbidden. This restores the pre-fix redirect behavior while keeping ":" out of stored pad ids. Adds a regression test for the ":" redirect, the "$" 404, and a clean id. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/node/hooks/express/padurlsanitize.ts | 14 ++++++-- .../backend/specs/padUrlColonRedirect.ts | 34 +++++++++++++++++++ 2 files changed, 46 insertions(+), 2 deletions(-) create mode 100644 src/tests/backend/specs/padUrlColonRedirect.ts diff --git a/src/node/hooks/express/padurlsanitize.ts b/src/node/hooks/express/padurlsanitize.ts index 8679bcfe346..7bf4ea0f13c 100644 --- a/src/node/hooks/express/padurlsanitize.ts +++ b/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(); diff --git a/src/tests/backend/specs/padUrlColonRedirect.ts b/src/tests/backend/specs/padUrlColonRedirect.ts new file mode 100644 index 00000000000..b5f6fc30d45 --- /dev/null +++ b/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/` 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); + }); +});