Sanitize while parsing

[noamr]

Instead of parsing into a fragment and then statically sanitizing that fragment, sanitize as we parse.
This entails the following changes:

• The sanitizer config is propagated to the parser and kept as a new parser flag ("parser sanitizer configuration").
• The "safe" flavor of sanitization is applied up front: "get a sanitizer config from options" removes unsafe from the configuration (including javascriptURLs), so the parser only needs the configuration itself.
• The fragment created to hold the result is created in the inert document when sanitizing to avoid creation-time side effects (see Nodes are made non-inert before sanitizing #12560).
• Instead of a recursive "sanitize" algorithm operating on a node tree, we define a "sanitize" algorithm that operates on a single Element, built from "get the sanitizer action for an element name" and "sanitize attributes", along with helper checks "sanitizer config allows an attribute", "sanitizer config allows comments", and "sanitizer config allows processing instruction target" (other node types do not need to check the sanitizer).
• For "replace with children", we replaced the parser's "root insertion target" flag with an "insertion target redirection map" (mapping a node to an insertion location). This map is used to redirect insertions from the root dummy element to the target DocumentFragment, and also to redirect elements to their nearest non-replaced ancestor when a child is stripped but its children are kept. The adoption agency algorithm updates the entries of replaced elements it moves.
• "Create an element for the token" consults the sanitizer configuration before creating the element: elements that will be removed or replaced with their children get no custom element definition (so no constructor runs for them), and an is attribute the sanitizer would remove is ignored.

Closes #12560
Closes #12543

• At least two implementers are interested (and none opposed):
  • Chromium
  • WebKit (as per @annevk)
  • Mozilla (as per @mozfreddyb)
• Tests are written and can be reviewed and commented upon at:
  • This mostly keeps existing behavior. The observable difference is coalescing of text nodes while parsing. This was modified in tests in Remove special text node behavior from streaming sanitizer web-platform-tests/wpt#59541.
  • https://wpt.fyi/results/sanitizer-api/sanitizer-inert-document.html checks that images are created as part of an inert document
  • Additional sanitizer tests for streaming sanitizer web-platform-tests/wpt#62492 for additional edge-casey behaviors
• Implementation bugs are filed:
  • Chromium: https://issues.chromium.org/issues/562116090
  • Gecko: see https://bugzilla.mozilla.org/show_bug.cgi?id=2043150
  • WebKit: doesn't require its own bug report
• MDN issue is filed: N/A, no behavior change
• The top of this comment includes a clear commit message to use.

(See WHATWG Working Mode: Changes for more details.)

---

/dynamic-markup-insertion.html ( diff )
/parsing.html ( diff )

[zcorpan]

Missing null check; nits.

[zcorpan]

From web-platform-tests/wpt#62492 (comment)

sanitizer-in-adoption-agency.sub.dat:125-134 — <b><div>Text</b><span>More</span></div> with div replaced is a three-way disagreement. The expectation is <b>Text</b><b><span>More</span></b>, Chrome gives <b>Text</b><b></b><span>More</span>, and I read the spec as giving <b>Text<b></b><span>More</span></b>, since map[div] is set to (b, null) when the <div> is replaced and both the AAA "Keep" insertion and the later <span> resolve through it.

The relevant test was:

#data
<b><div>Text</b><span>More</span></div>
#config
{ "replaceWithChildrenElements": ["div"] }
#document
| <b>
|   "Text"
| <b>
|   <span>
|     "More"

Any thoughts on what should happen here?

[zcorpan]

The above comment was from an older revision of this PR, and the current revision should produce <b>Text</b><b></b><span>More</span> which matches Chromium. However, I asked Claude to compare Chromium's implementation with this PR for this case, and it found there's still a difference if you insert another formatting element to the test:

For the exact wpt case they agree, but only by coincidence, and the two use different mechanisms that diverge as soon as the inner loop does any work.

html_tree_builder.cc:1984-2000 (Chrome 155.0.8052.0 canary):

case Sanitizer::Action::kReplaceWithChildren:
  // Skip DOM operations. Children safely remain in furthest_block.
  continue;
case Sanitizer::Action::kDrop:
  tree_.TakeAllChildren(new_item, furthest_block);
  break;
case Sanitizer::Action::kKeep:
case Sanitizer::Action::kKeepElement:
  tree_.TakeAllChildren(new_item, furthest_block);
  tree_.Reparent(furthest_block, new_item);
  break;

So kDrop is exactly keithamus's suggestion. But kReplaceWithChildren records nothing: Blink has no redirection map. HTMLConstructionSite::Reparent and TakeAllChildren both route through AdjustInsertionLocation (html_construction_site.cc:1246-1285, :878-918), which walks up the stack of open elements from the target, skipping items whose action is kReplaceWithChildren, and resolves the root through root_insertion_point_. That is a dynamic re-resolution at each insertion, where the spec stores a resolved pair in the map.

In <b><div>Text</b><span>More</span></div> the inner loop breaks on its first iteration, so the nearest kept stack ancestor of div just is commonAncestor (b having been removed from the stack), and source:148169 lands in the same place. Add a kept formatting element in between and they part ways:

<b><em><div>Text</b>after with { replaceWithChildrenElements: ["div"] }

Canary:

| <b>
|   <em>
|     "Text"
| <em>
|   <b>
|   "after"

Spec at 4edf8660: map[div] exists, so :148169 sets it to the adjusted insertion location given (commonAncestor, null), and commonAncestor is the root, so it resolves to the container. Both newElement and "after" then go to the container rather than into the cloned <em>:

| <b>
|   <em>
|     "Text"
| <em>
| <b>
| "after"

With <b><em><u><div>Text</b>after Canary puts them inside the cloned <u>, so it really is the nearest kept ancestor, not commonAncestor.

Canary looks right to me: the plain parse of that input puts the div inside the cloned <em>, so div's children should go there too. The redirect at :148169 is standing in for the "append lastNode to node" that :148089 suppressed by forcing lastNode to null when furthestBlock isn't "Keep", so it should point at the first "Keep" node the inner loop reaches (falling back to commonAncestor when the loop breaks immediately), not at commonAncestor unconditionally.

Edit: fixed in 5452f09

[zcorpan]

One more divergence from Chromium:

<b><div><div><div><div><div><div><div><div><section>Text</b>after with { replaceWithChildrenElements: ["section"] }

After 8 iterations section is still the current node, and its redirection map entry is still (div8, null) from when it was inserted. Chromium's stack walk from section now finds b^8 (inserted immediately below div8), so Canary gives:

| <div> (div8)
|   <b>
|     "Textafter"

The spec as written gives:

| <div> (div8)
|   <b>
|     "Text"
|   "after"

(In a normal parse, "Textafter" is one text node in section in b.)

More generally, the map entry of any replaced-with-children element directly below furthestBlock in the stack goes stale once newElement is inserted between them, but in every other case those elements get popped by the final outer iteration before they can be an insertion target again.

I think the fix is to update the entries for the run of replaced elements immediately below furthestBlock after inserting newElement into the stack, pointing them at the adjusted insertion location given (newElement, null).

Edit: fixed in 16e9bb5

[zcorpan]

A few more issues found by Claude:

AI review

Here's the consolidated picture, with Canary 155.0.8059.0 as the reference and each item verified by trace against the spec at 16e9bb5.

Spec problems

• Foster parenting regression in the AAA, also without a sanitizer. Main's "adjusted insertion location" re-ran "appropriate place for inserting a node" with the passed target, so lastNode at a table-ish commonAncestor was foster-parented. The PR's version at source:145518-145524 uses the passed location verbatim, so <table><b><div>x</b>y now moves the div into the table (Canary and main put it before). With removeElements: ["table"] the div, "x" and "y" vanish. Fix: at source:148195 compute the appropriate place for inserting a node given commonAncestor and pass that in. The newElement step is fine, since main appended to furthestBlock directly.
• Custom element lookup isn't gated on the sanitizer. "Create an element for the token" at source:145410-145428 looks up the definition (and is) before sanitizing, and the fragment root carries the target's registry, so a dropped <x-foo> still gets an upgrade reaction and its constructor runs at the end of setHTML. Chromium skips the lookup for disallowed elements, and sanitizer-custom-elements-constructor.html already asserts that. Same for is removed by config: the spec builds the customized built-in, Canary makes a plain element. This is the existing XXX at source:145419.
• Noah's Ark wording. source:142357-142362 compares attributes "as they were when created by the parser". With attribute sanitization at insertion that reads as token attributes, which is what Chromium compares. Behavior matches; a note would settle it.

Chromium bugs, spec looks right

• Foster-parented replaced-with-children content goes into the table: <table><tr><div>x</div><td>y replacing div gives tr > ("x", td) in Canary. Same with <table>a<b>y</b>c<tr>... and the AAA variant <table><b><div>x</b>y replacing div. Chromium walks the stack from the element rather than using its foster location. Non-streaming Chromium matches the spec.
• Document-level comments and processing instructions survive comments: false in parseHTML/parseHTMLUnsafe, because ShouldInsertChild skips sanitization when the parent is a Document.
• parseHTMLUnsafe with no sanitizer option drops in-tree comments, unlike setHTMLUnsafe and DOMParser.
• template in replaceWithChildrenElements hoists template contents as light DOM (a<template>x</template>b gives "axb"), including declarative shadow roots. The spec drops the contents.
• Passing any options object to setHTML/setHTMLUnsafe flips Chromium to scripting-enabled parsing, so <noscript><p>x</noscript>y parses the <p>. Not caused by this PR.
• On a disconnected target Canary constructs custom elements only on connection, while the spec upgrades at the end of the call.

Both agree, but differ from parse-then-sanitize

• Foster-parented content with a replaced table lands after the hoisted tbody rather than before it.
  • Fixing this would be somewhat involved, I think we can leave this as a known difference.
• A declarative shadow root template that is removed or replaced loses the shadow root in both streaming implementations, where post-parse sanitization keeps it.
  • This is intentional I think.

Everything else the forks tried matched: all AAA Remove paths, reconstruction, <a>/<nobr>, scope with detached elements, refChild handling, text coalescing, html/head/body/frameset, fragment contexts, foreign content, raw text elements, <image>, select/option, form pointer, and duplicate <html>/<body> attribute merging.

Edit: 3 spec issues fixed.