Skip to content

Sanitize while parsing - #12645

Closed
noamr wants to merge 3 commits into
mainfrom
streaming-sanitizer
Closed

Sanitize while parsing#12645
noamr wants to merge 3 commits into
mainfrom
streaming-sanitizer

Conversation

@noamr

@noamr noamr commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

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 kept as a boolean flag in the parser ("remove javascript navigation URLs"), as that is a special safety check not kept in the sanitizer configuration.
  • 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, along with two helper checks: "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.

Note that this still leaves out important follow ups, where we need to sometimes sanitize before the element is even created (e.g. declarative shadow roots and is), and having a live sanitizer config while parsing gives us the infrastructure for that.

Closes #12560
Closes #12543

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


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

@noamr noamr changed the title Streaming sanitizer Streaming sanitizer (depends on 12624) Jul 2, 2026
@noamr
noamr force-pushed the streaming-sanitizer branch 3 times, most recently from 2e43740 to 3ae9149 Compare July 9, 2026 12:15
@noamr noamr changed the title Streaming sanitizer (depends on 12624) Sanitize while parsing Jul 9, 2026
@noamr
noamr marked this pull request as ready for review July 9, 2026 13:20
@noamr

noamr commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

@noamr
noamr force-pushed the streaming-sanitizer branch 2 times, most recently from 22ab343 to 6714ff1 Compare July 9, 2026 15:08
@noamr
noamr requested review from annevk and zcorpan July 13, 2026 11:27

@otherdaniel otherdaniel left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks good to me, but I don't think I fully understand the subtleties here.


I had expected some sort of sanitizer tie-in somewhere around "create an element for a token" (parsing.html#create-an-element-for-the-token), to close the custom element/is-attribute loopholes. Did I overlook something?

Comment thread source
Comment thread source
Comment thread source
@noamr

noamr commented Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

Generally looks good to me, but I don't think I fully understand the subtleties here.

I had expected some sort of sanitizer tie-in somewhere around "create an element for a token" (parsing.html#create-an-element-for-the-token), to close the custom element/is-attribute loopholes. Did I overlook something?

You didn't, that would be a follow up where we apply the sanitizer on a token in particular situations.
I wanted this PR to not change behavior and then iterate on those fixes.

I added a comment to the OP about this.

@noamr noamr added the agenda+ To be discussed at a triage meeting label Jul 15, 2026
Comment thread source Outdated
@noamr
noamr force-pushed the streaming-sanitizer branch from 5814031 to b7dd067 Compare July 15, 2026 13:57
@noamr noamr removed the agenda+ To be discussed at a triage meeting label Jul 23, 2026
Comment thread source Outdated
<li>
<p><span data-x="list iterate">For each</span> <var>child</var> of <var>node</var>'s <span
data-x="concept-tree-child">children</span>:</p>
<p>If <var>configuration</var>["<code

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it would be cleaner to have something like:

  1. If configuration["processingInstructions"] exists, then:
    1. Return whether configuration["processingInstructions"] contains piTarget.
  2. Otherwise, return whether configuration["removeProcessingInstructions"] does not contain piTarget.

First time reading this the fallthrough from 1.1. tripped to 3. tripped me up.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This wouldn't be according to the spec style, but I reworded it a bit to be more readable perhaps

@evilpie

evilpie commented Jul 29, 2026

Copy link
Copy Markdown
Member

Nice, I am not an expert on the parser, but that actually looks like quite an elegant integration. Especially the redirectLocation handling should be reviewed with more experience in this area.

I also really like how much cleaner the the sanitize algorithm looks now.

This does not yet handle the is attribute, right? I would have expected some adjustments in create an element for a token in that case.

@noamr

noamr commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

Nice, I am not an expert on the parser, but that actually looks like quite an elegant integration. Especially the redirectLocation handling should be reviewed with more experience in this area.

I also really like how much cleaner the the sanitize algorithm looks now.

This does not yet handle the is attribute, right? I would have expected some adjustments in create an element for a token in that case.

Right, I was planning to do that as a follow up.

noamr added 3 commits July 29, 2026 15:59
Editorial: style and line length improvements in Sanitizer algorithms

Editorial: restore sanitize/sanitized cross-references in domintro blocks

Editorial: undo unnecessary line wrapping on Document node insertion check

Create fragment in the inert document

Editorial: remove unreachable null checks of adjusted insertion location result

adjusted insertion location is never null

Preserve reference child in insertion redirection for foster parenting

ref-node

Restored explanatory notes for redirection map and reference node with correct formatting and indentation

Remove redundant parse HTML
@noamr
noamr force-pushed the streaming-sanitizer branch from b7dd067 to 9c3dc52 Compare July 29, 2026 15:24
@noamr

noamr commented Aug 4, 2026

Copy link
Copy Markdown
Contributor Author

Closing in favor of #12756 (stacked PR)

@noamr noamr closed this Aug 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Nodes are made non-inert before sanitizing Streaming sanitizer support

3 participants