From d828185efcac9abc2e46e6da1c171393500fa319 Mon Sep 17 00:00:00 2001 From: John McLear Date: Wed, 10 Jun 2026 09:53:26 +0100 Subject: [PATCH] security: escape and integer-coerce numbered-list start attribute (GHSA-f7h5-v9hm-548j) (#7937) The numbered-list branch of `domline.appendSpan` interpolated the line's `start` attribute value into an `
    ` tag unquoted and unescaped, then assigned the result to `node.innerHTML`. The value comes verbatim from the attribute pool, which an attacker can populate via a crafted `.etherpad` import (`ImportEtherpad.setPadRaw` validates attribute keys but not values). A space-free value such as `1>` therefore broke out of the tag and produced a live element, executing attacker JavaScript in the pad's origin for every subsequent viewer of the pad or timeslider. An `
      ` start is only ever an integer, so coerce it with `Number.parseInt`, omit the attribute entirely when the value is not a number, and HTML-escape + quote it (the same `Security.escapeHTMLAttribute` treatment the sibling `listType` class already gets, and that #7905 applied to the export sink). Adds a backend regression test that drives the real domline render path through jsdom and asserts the malicious value cannot produce a live element. Co-authored-by: Claude Opus 4.8 (1M context) --- src/static/js/domline.ts | 13 +++- src/tests/backend/specs/domline_list_start.ts | 62 +++++++++++++++++++ 2 files changed, 73 insertions(+), 2 deletions(-) create mode 100644 src/tests/backend/specs/domline_list_start.ts diff --git a/src/static/js/domline.ts b/src/static/js/domline.ts index bb78c3aeb..339616a25 100644 --- a/src/static/js/domline.ts +++ b/src/static/js/domline.ts @@ -102,12 +102,21 @@ domline.createDomLine = (nonEmpty, doesWrap, optBrowser, optDocument) => { postHtml = `${postHtml}`; } else { if (start) { // is it a start of a list with more than one item in? - if (Number.parseInt(start[1]) === 1) { // if its the first one at this level? + // The `start` value comes verbatim from the attribute pool (which + // an attacker can populate via a crafted `.etherpad` import), so it + // must never be interpolated raw into the markup. An
        start is + // only ever an integer: coerce it and HTML-escape it defensively so + // a value such as `1>` cannot break out of the tag. + const startNum = Number.parseInt(start[1]); + if (startNum === 1) { // if its the first one at this level? // Add start class to DIV node lineClass = `${lineClass} ` + `list-start-${listType}`; } + const startAttr = Number.isNaN(startNum) + ? '' + : ` start="${Security.escapeHTMLAttribute(String(startNum))}"`; preHtml += - `
        1. `; + `
        2. `; } else { // Handles pasted contents into existing lists preHtml += `
          1. `; diff --git a/src/tests/backend/specs/domline_list_start.ts b/src/tests/backend/specs/domline_list_start.ts new file mode 100644 index 000000000..17700483e --- /dev/null +++ b/src/tests/backend/specs/domline_list_start.ts @@ -0,0 +1,62 @@ +'use strict'; + +/* + * Regression test for GHSA-f7h5-v9hm-548j. + * + * The numbered-list branch of `domline.appendSpan` used to interpolate the + * line's `start` attribute value into an `
              ` tag unquoted and + * unescaped, then assign the result to `node.innerHTML`. The value comes + * verbatim from the attribute pool, which an attacker can populate via a + * crafted `.etherpad` import, so a value such as `1>` + * broke out of the tag and produced a live element -> stored XSS for every + * viewer of the pad/timeslider. + */ + +const assert = require('assert').strict; +const domline = require('../../../static/js/domline').domline; +const {lineAttributeMarker} = require('../../../static/js/linestylefilter'); +import jsdom from 'jsdom'; + +// Build the per-span class string exactly as linestylefilter would for a +// numbered-list line marker carrying the given `start` value. +const listCls = (start: string) => + `${lineAttributeMarker} list:number1 start:${start}`; + +// Render a single line-marker span through the real domline sink and return +// the resulting DOM node. +const renderLine = (cls: string) => { + const {window} = new jsdom.JSDOM(''); + const node = domline.createDomLine(true, false, window, window.document); + node.clearSpans(); + node.appendSpan('*', cls); + node.finishUpdate(); + return node.node as HTMLElement; +}; + +describe(__filename, function () { + it('does not create a live element from a malicious start value', async function () { + // Space-free payload: satisfies the `\S+` capture the sink matches on. + const node = renderLine(listCls('1>')); + assert.equal(node.querySelector('svg'), null, + 'malicious start value must not be parsed into a live element'); + assert.ok(!node.innerHTML.includes(' tag: ${node.innerHTML}`); + // The numbered list itself still renders; only the bogus value is dropped. + assert.ok(node.querySelector('ol'), 'a numbered list should still render'); + }); + + it('renders a legitimate integer start value safely', async function () { + const node = renderLine(listCls('2')); + const ol = node.querySelector('ol'); + assert.ok(ol, 'a numbered list should render'); + assert.equal(ol!.getAttribute('start'), '2'); + }); + + it('coerces a non-integer start value away instead of emitting it', async function () { + const node = renderLine(listCls('notanumber')); + const ol = node.querySelector('ol'); + assert.ok(ol, 'a numbered list should still render'); + assert.equal(ol!.getAttribute('start'), null, + 'a non-integer start value must not reach the
                start attribute'); + }); +});