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 `<ol start=...>` 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><svg/onload=...>` 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 `<ol>` 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) <noreply@anthropic.com>
This commit is contained in:
John McLear 2026-06-10 09:53:26 +01:00 committed by GitHub
parent 68d322957f
commit d828185efc
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
2 changed files with 73 additions and 2 deletions

View file

@ -102,12 +102,21 @@ domline.createDomLine = (nonEmpty, doesWrap, optBrowser, optDocument) => {
postHtml = `</li></ul>${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 <ol> start is
// only ever an integer: coerce it and HTML-escape it defensively so
// a value such as `1><svg/onload=...>` 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 +=
`<ol start=${start[1]} class="list-${Security.escapeHTMLAttribute(listType)}"><li>`;
`<ol${startAttr} class="list-${Security.escapeHTMLAttribute(listType)}"><li>`;
} else {
// Handles pasted contents into existing lists
preHtml += `<ol class="list-${Security.escapeHTMLAttribute(listType)}"><li>`;

View file

@ -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 `<ol start=...>` 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><svg/onload=alert(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('<!DOCTYPE html><html><body></body></html>');
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><svg/onload=alert(document.domain)>'));
assert.equal(node.querySelector('svg'), null,
'malicious start value must not be parsed into a live <svg> element');
assert.ok(!node.innerHTML.includes('<svg'),
`rendered markup must not contain a raw <svg> 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 <ol> start attribute');
});
});