From 02e80c595dbe5af9752c2bd291bbc5cabbff7676 Mon Sep 17 00:00:00 2001 From: John McLear Date: Sat, 25 Jul 2026 18:36:56 +0100 Subject: [PATCH 1/3] 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/3] 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); + }); +}); From bc3687ecb245bb9540a71346c89a856846cc415e Mon Sep 17 00:00:00 2001 From: SamTV12345 <40429738+samtv12345@users.noreply.github.com> Date: Mon, 27 Jul 2026 20:16:21 +0200 Subject: [PATCH 3/3] security(padid): keep existing pads with ":" reachable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rejecting ":" in isValidPadId also gates padManager.getPad(), so pads whose id contains a ":" — legal before GHSA-wg58-mhwv-35pq, which is why padIdTransforms maps ":" at all — became unopenable, not just uncreatable: sanitizePadId returns such an id unchanged once the pad exists, and getPad then threw. Refuse an invalid id only when no pad with that exact id exists (in getPad and in the pad-URL param). The injection primitive stays closed: doesPadExist() requires a top-level `atext`, which the `pad::revs:` sub-records an injected destinationID addresses do not have. Copy destinations keep the strict check, so no new ":" id can be created. Co-Authored-By: Claude Opus 5 (1M context) --- src/node/db/PadManager.ts | 10 +++- src/node/hooks/express/padurlsanitize.ts | 10 +++- .../backend/specs/padIdColonLegacyPad.ts | 57 +++++++++++++++++++ 3 files changed, 75 insertions(+), 2 deletions(-) create mode 100644 src/tests/backend/specs/padIdColonLegacyPad.ts diff --git a/src/node/db/PadManager.ts b/src/node/db/PadManager.ts index 91238ff61c8..67a5af886cc 100644 --- a/src/node/db/PadManager.ts +++ b/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 => { // 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::revs:` / `:chat:` 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'); } diff --git a/src/node/hooks/express/padurlsanitize.ts b/src/node/hooks/express/padurlsanitize.ts index 7bf4ea0f13c..650f1315ee9 100644 --- a/src/node/hooks/express/padurlsanitize.ts +++ b/src/node/hooks/express/padurlsanitize.ts @@ -21,7 +21,15 @@ exports.expressCreateServer = (hookName:string, args:ArgsExpressType, cb:Functio // invalid after sanitizing (e.g. one containing `$`) is forbidden. const sanitizedPadId = await padManager.sanitizePadId(padId); - if (!padManager.isValidPadId(sanitizedPadId)) { + // 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; } diff --git a/src/tests/backend/specs/padIdColonLegacyPad.ts b/src/tests/backend/specs/padIdColonLegacyPad.ts new file mode 100644 index 00000000000..eb98a9e207f --- /dev/null +++ b/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::revs:` 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::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); + }); +});