Sanitize while parsing - #12756
Conversation
7b105e3 to
e24c23b
Compare
4ff3705 to
8551681
Compare
c374a0b to
e8c7cba
Compare
1d9e935 to
4453c2a
Compare
| <li><p>Create a new <span>HTML parser</span> whose <span>parser sanitizer configuration</span> is | ||
| <var>configuration</var>, <span>allow declarative shadow roots</span> is | ||
| <var>allowDeclarativeShadowRoots</var>, <span>scripting mode</span> is | ||
| <var>scriptingMode</var>, and <span>remove javascript navigation URLs</span> | ||
| is <var>safe</var>, and associate it with <var>document</var>.</p></li> | ||
|
|
||
| <li><p>If <var>contextDocument</var>'s <span data-x="concept-n-script">scripting is | ||
| disabled</span>, then set <var>scriptingMode</var> to <span | ||
| data-x="scripting-mode-disabled">Disabled</span>.</p></li> |
There was a problem hiding this comment.
From chatgpt's re-review:
The context document’s scripting-disabled state is still applied only after constructing the parser. The following step mutates the local scriptingMode; it does not update the parser created in the previous step. Please calculate the final mode before parser construction, or explicitly update the parser’s stored mode afterward. This commit should be correct independently of #12758. A <noscript> fragment test with scripting disabled would catch the ordering error.
There was a problem hiding this comment.
Switched the order, should be OK now (this is also changed in the next PR in the stack)
4453c2a to
931141c
Compare
| <li><p>If <var>lastNode</var> is non-null, insert whatever <var>lastNode</var> ended up being | ||
| in the previous step at the <span>adjusted insertion location</span> given | ||
| <var>insertionLocation</var>.</p></li> |
There was a problem hiding this comment.
Does this insert lastNode without a sanitizer check?
data:text/html,<!DOCTYPE html><body><div id=div></div><script>div.setHTMLUnsafe('<b><div><b>x', { sanitizer: {removeElements: ['div'] }});</script>
<b>is pushed<div>is created, sanitize removes, so insert is aborted but its still pushed</b>runs AAA, furthestBlock is removed div,commonAncestorisb.- Inner loop breaks,
lastNodeisfurthestBlock - This step insets removed div into
commonAncestor.
Perhaps worth a test at least.
There was a problem hiding this comment.
Good catch! This is fixed now (Adding tests as part of https://chromium-review.git.corp.google.com/c/chromium/src/+/8250959, once they're in I'll add them to the un-tentative PR)
There was a problem hiding this comment.
Added multiple test cases for this: web-platform-tests/wpt@4409cc2
(They're not tentative as they should be sanitized with the post-processing sanitizer as well)
| <li><p>If the parser's <span>insertion target redirection map</span>[<var>targetParent</var>] | ||
| does not <span data-x="map exists">exist</span>, then return | ||
| <var>adjustedInsertionLocation</var>.</p></li> |
There was a problem hiding this comment.
So here, as removed elements never end up in the map, this can return early and - as the adjusted insertion location becomes a detached element - all children end up being added to that. While it's unobservable, it's a bit slow - but if we added a map of removed elements we could do something like:
If the parser's sanitizer-removed elements contains targetParent, then return null.
To no-op sooner, when targetParent is detatched. Of course implementations could do this and still be conformant but it may be nice to either specify or at least comment & hint at it.
There was a problem hiding this comment.
I don't know about adding more non-observable optimizations to the spec just for the purpose of AAA... seems like an additional potential point of failure.
32e5b9a to
1b77b8d
Compare
Sanitization should take place while performing the adoption agency algorithm, otherwise some nodes can be missed. Since adoption agency often adds an intermediate element, the ReplaceWithChildren directive essentially negates it, while Remove reparents the children into the intermediate element and removes all of them. This is in line with spec changes made as part of whatwg/html#12756 Bug: 498272014 Change-Id: Ieb755555635e197f8ed7daf06a08679a0f2d38c3 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8250959 Commit-Queue: Noam Rosenthal <nrosenthal@google.com> Reviewed-by: Daniel Vogelheim <vogelheim@chromium.org> Cr-Commit-Position: refs/heads/main@{#1680770}
Sanitization should take place while performing the adoption agency algorithm, otherwise some nodes can be missed. Since adoption agency often adds an intermediate element, the ReplaceWithChildren directive essentially negates it, while Remove reparents the children into the intermediate element and removes all of them. This is in line with spec changes made as part of whatwg/html#12756 Bug: 498272014 Change-Id: Ieb755555635e197f8ed7daf06a08679a0f2d38c3 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8250959 Commit-Queue: Noam Rosenthal <nrosenthal@google.com> Reviewed-by: Daniel Vogelheim <vogelheim@chromium.org> Cr-Commit-Position: refs/heads/main@{#1680770}
Sanitization should take place while performing the adoption agency algorithm, otherwise some nodes can be missed. Since adoption agency often adds an intermediate element, the ReplaceWithChildren directive essentially negates it, while Remove reparents the children into the intermediate element and removes all of them. This is in line with spec changes made as part of whatwg/html#12756 Bug: 498272014 Change-Id: Ieb755555635e197f8ed7daf06a08679a0f2d38c3 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8250959 Commit-Queue: Noam Rosenthal <nrosenthal@google.com> Reviewed-by: Daniel Vogelheim <vogelheim@chromium.org> Cr-Commit-Position: refs/heads/main@{#1680770}
…owIfMissing polarity
…x TT throwIfMissing polarity" This reverts commit 7898aca.
…ensure the adjusted insertion location is correct
1b77b8d to
2eb51b7
Compare
…ency edge cases, a=testonly Automatic update from web-platform-tests Fix some streaming-sanitizer/adoption-agency edge cases Sanitization should take place while performing the adoption agency algorithm, otherwise some nodes can be missed. Since adoption agency often adds an intermediate element, the ReplaceWithChildren directive essentially negates it, while Remove reparents the children into the intermediate element and removes all of them. This is in line with spec changes made as part of whatwg/html#12756 Bug: 498272014 Change-Id: Ieb755555635e197f8ed7daf06a08679a0f2d38c3 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8250959 Commit-Queue: Noam Rosenthal <nrosenthal@google.com> Reviewed-by: Daniel Vogelheim <vogelheim@chromium.org> Cr-Commit-Position: refs/heads/main@{#1680770} -- wpt-commits: 4409cc231d390712ee52ffb5529177ebb0cd0da3 wpt-pr: 61987
Instead of parsing into a fragment and then statically sanitizing that fragment, sanitize as we parse.
This entails the following changes:
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.)
/dom.html ( diff )
/dynamic-markup-insertion.html ( diff )
/iframe-embed-object.html ( diff )
/infrastructure.html ( diff )
/parsing.html ( diff )
/timers-and-user-prompts.html ( diff )