Skip to content

Commit a0a10a4

Browse files
committed
feat: loosened the style check a bit again
test: added correct test coverage for template scrubbing
1 parent ca39c59 commit a0a10a4

10 files changed

Lines changed: 83 additions & 72 deletions

File tree

dist/purify.cjs.js

Lines changed: 8 additions & 12 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

dist/purify.cjs.js.map

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

dist/purify.es.mjs

Lines changed: 8 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1466,13 +1466,8 @@ function createDOMPurify() {
14661466
if (SAFE_FOR_XML && currentNode.hasChildNodes() && !_isNode(currentNode.firstElementChild) && regExpTest(ELEMENT_MARKUP_PROBE, currentNode.textContent) && regExpTest(ELEMENT_MARKUP_PROBE, currentNode.innerHTML)) {
14671467
return true;
14681468
}
1469-
/* Remove risky CSS construction leading to mXSS. A <style> element only
1470-
ever holds CSS text, so an element child is always a parser-mutation
1471-
signal. Not gated on HTML_NAMESPACE: declarative partial updates can
1472-
teleport a <style> into an <svg>/<math> foreign-content marker
1473-
(WICG/declarative-partial-updates), where the surviving element would
1474-
otherwise be SVG/MathML-namespaced and slip past an HTML-only check. */
1475-
if (SAFE_FOR_XML && tagName === 'style' && _isNode(currentNode.firstElementChild)) {
1469+
/* Remove risky CSS construction leading to mXSS */
1470+
if (SAFE_FOR_XML && currentNode.namespaceURI === HTML_NAMESPACE && tagName === 'style' && _isNode(currentNode.firstElementChild)) {
14761471
return true;
14771472
}
14781473
/* Remove any occurrence of processing instructions */
@@ -1633,13 +1628,14 @@ function createDOMPurify() {
16331628
`for` is legitimate only on <label>/<output>; anywhere else (notably
16341629
<template for>) it links the element to a patch target and teleports or
16351630
removes an arbitrary DOM range by id/marker name. `patchsrc` fetches
1636-
remote markup and is treated as a script-loading mechanism (CSP). Neither
1637-
has a safe non-patch use, so drop them regardless of ADD_ATTR. Processing
1638-
instructions used as range markers are already removed by _isUnsafeNode. */
1639-
if (lcName === 'patchsrc') {
1631+
remote markup and is treated as a script-loading mechanism (CSP). Gated
1632+
on SAFE_FOR_XML so the removal groups with the other structural-threat
1633+
checks and stays overridable, consistent with the rest of the codebase.
1634+
PI range markers are already removed by _isUnsafeNode. */
1635+
if (SAFE_FOR_XML && lcName === 'patchsrc') {
16401636
return false;
16411637
}
1642-
if (lcName === 'for' && lcTag !== 'label' && lcTag !== 'output') {
1638+
if (SAFE_FOR_XML && lcName === 'for' && lcTag !== 'label' && lcTag !== 'output') {
16431639
return false;
16441640
}
16451641
/* Make sure attribute cannot clobber */

dist/purify.es.mjs.map

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

dist/purify.js

Lines changed: 8 additions & 12 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

dist/purify.js.map

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

dist/purify.min.js

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

dist/purify.min.js.map

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

src/purify.ts

Lines changed: 13 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1631,14 +1631,10 @@ function createDOMPurify(window: WindowLike = getGlobal()): DOMPurify {
16311631
return true;
16321632
}
16331633

1634-
/* Remove risky CSS construction leading to mXSS. A <style> element only
1635-
ever holds CSS text, so an element child is always a parser-mutation
1636-
signal. Not gated on HTML_NAMESPACE: declarative partial updates can
1637-
teleport a <style> into an <svg>/<math> foreign-content marker
1638-
(WICG/declarative-partial-updates), where the surviving element would
1639-
otherwise be SVG/MathML-namespaced and slip past an HTML-only check. */
1634+
/* Remove risky CSS construction leading to mXSS */
16401635
if (
16411636
SAFE_FOR_XML &&
1637+
currentNode.namespaceURI === HTML_NAMESPACE &&
16421638
tagName === 'style' &&
16431639
_isNode(currentNode.firstElementChild)
16441640
) {
@@ -1858,14 +1854,20 @@ function createDOMPurify(window: WindowLike = getGlobal()): DOMPurify {
18581854
`for` is legitimate only on <label>/<output>; anywhere else (notably
18591855
<template for>) it links the element to a patch target and teleports or
18601856
removes an arbitrary DOM range by id/marker name. `patchsrc` fetches
1861-
remote markup and is treated as a script-loading mechanism (CSP). Neither
1862-
has a safe non-patch use, so drop them regardless of ADD_ATTR. Processing
1863-
instructions used as range markers are already removed by _isUnsafeNode. */
1864-
if (lcName === 'patchsrc') {
1857+
remote markup and is treated as a script-loading mechanism (CSP). Gated
1858+
on SAFE_FOR_XML so the removal groups with the other structural-threat
1859+
checks and stays overridable, consistent with the rest of the codebase.
1860+
PI range markers are already removed by _isUnsafeNode. */
1861+
if (SAFE_FOR_XML && lcName === 'patchsrc') {
18651862
return false;
18661863
}
18671864

1868-
if (lcName === 'for' && lcTag !== 'label' && lcTag !== 'output') {
1865+
if (
1866+
SAFE_FOR_XML &&
1867+
lcName === 'for' &&
1868+
lcTag !== 'label' &&
1869+
lcTag !== 'output'
1870+
) {
18691871
return false;
18701872
}
18711873

test/test-suite.js

Lines changed: 41 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -259,38 +259,41 @@
259259
// Declarative Partial Updates — patch-directive attributes
260260
// https://github.com/WICG/declarative-partial-updates/blob/main/patching-explainer.md
261261
//
262-
// The proposal lets a `<template for="...">` teleport / range-replace
263-
// content into a pre-existing element (by id or marker name) and lets
264-
// `patchsrc` fetch remote markup (script-loading equivalent for CSP).
265-
// The patch is applied on connection/stream — AFTER a parse-time
266-
// sanitizer has run over a detached fragment — so these attributes must
267-
// never survive sanitization. Processing-instruction range markers are
268-
// already dropped by _isUnsafeNode; these tests also lock that in.
262+
// <template for="..."> teleports / range-replaces content into a
263+
// pre-existing element (by id or marker name); patchsrc fetches remote
264+
// markup (script-loading equivalent for CSP). Patches apply on
265+
// connection/stream — AFTER a parse-time sanitizer has run over a detached
266+
// fragment — so these must never survive sanitization. PI range markers are
267+
// already dropped by _isUnsafeNode; removal is gated on SAFE_FOR_XML. Each
268+
// test uses a fresh instance so a persistent setConfig left by an earlier
269+
// test cannot suppress _parseConfig and swallow the per-call config.
269270
// =======================================================================
270271

271272
QUnit.module('Declarative partial updates');
272273

273274
QUnit.test(
274275
'for teleport directive is stripped on non-label/output',
275276
(assert) => {
277+
const purify = DOMPurify(window);
276278
assert.equal(
277-
DOMPurify.sanitize('<div for="account-panel">x</div>'),
279+
purify.sanitize('<div for="account-panel">x</div>'),
278280
'<div>x</div>'
279281
);
280282
assert.equal(
281-
DOMPurify.sanitize('<section for="cart#total">$0.00</section>'),
283+
purify.sanitize('<section for="cart#total">$0.00</section>'),
282284
'<section>$0.00</section>'
283285
);
284286
}
285287
);
286288

287289
QUnit.test('for is preserved on label and output', (assert) => {
290+
const purify = DOMPurify(window);
288291
assert.equal(
289-
DOMPurify.sanitize('<label for="email">Email</label>'),
292+
purify.sanitize('<label for="email">Email</label>'),
290293
'<label for="email">Email</label>'
291294
);
292295
assert.equal(
293-
DOMPurify.sanitize('<output for="a b">42</output>', {
296+
purify.sanitize('<output for="a b">42</output>', {
294297
ADD_TAGS: ['output'],
295298
}),
296299
'<output for="a b">42</output>'
@@ -300,39 +303,57 @@
300303
QUnit.test(
301304
'template[for] loses its patch directive when template is allowed',
302305
(assert) => {
306+
const purify = DOMPurify(window);
303307
assert.contains(
304-
DOMPurify.sanitize(
305-
'<template for="account-panel"><b>x</b></template>',
306-
{
307-
ADD_TAGS: ['template'],
308-
}
309-
),
308+
purify.sanitize('<template for="account-panel"><b>x</b></template>', {
309+
ADD_TAGS: ['template'],
310+
}),
310311
['<template><b>x</b></template>', '<template></template>', ''],
311312
'template survives but the `for` patch directive does not'
312313
);
313314
}
314315
);
315316

316317
QUnit.test('patchsrc is stripped even when explicitly added', (assert) => {
318+
const purify = DOMPurify(window);
317319
assert.equal(
318-
DOMPurify.sanitize('<div patchsrc="//evil.example/p">x</div>', {
320+
purify.sanitize('<div patchsrc="//evil.example/p">x</div>', {
319321
ADD_ATTR: ['patchsrc'],
320322
}),
321323
'<div>x</div>'
322324
);
323325
});
324326

325327
QUnit.test('processing-instruction range markers are removed', (assert) => {
328+
const purify = DOMPurify(window);
326329
assert.equal(
327-
DOMPurify.sanitize('<section><?start name="g">hi<?end></section>'),
330+
purify.sanitize('<section><?start name="g">hi<?end></section>'),
328331
'<section>hi</section>'
329332
);
330333
assert.equal(
331-
DOMPurify.sanitize('<ul><li>a</li><?marker name="m"><li>b</li></ul>'),
334+
purify.sanitize('<ul><li>a</li><?marker name="m"><li>b</li></ul>'),
332335
'<ul><li>a</li><li>b</li></ul>'
333336
);
334337
});
335338

339+
QUnit.test(
340+
'SAFE_FOR_XML=false is the escape hatch for patch attributes',
341+
(assert) => {
342+
const purify = DOMPurify(window);
343+
assert.equal(
344+
purify.sanitize('<div for="target">x</div>', { SAFE_FOR_XML: false }),
345+
'<div for="target">x</div>'
346+
);
347+
assert.equal(
348+
purify.sanitize('<div patchsrc="//e">x</div>', {
349+
ADD_ATTR: ['patchsrc'],
350+
SAFE_FOR_XML: false,
351+
}),
352+
'<div patchsrc="//e">x</div>'
353+
);
354+
}
355+
);
356+
336357
// =======================================================================
337358
// Config: ALLOW_DATA_ATTR
338359
// =======================================================================

0 commit comments

Comments
 (0)