Implement (unsafe) streaming sanitizer [chromium/src : main]
AI Code Reviewer (Gerrit)
AI Code Reviewer added 2 comments![]( "Open in Gerrit")
bool safe_ = false;nit: Precede boolean values with words like “is” and “did”. Consider renaming 'safe_' to 'is_safe_'. (Blink Style Guide: Naming - Precede boolean values with words like “is” and “did”)
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
StreamingSanitizer(const Sanitizer* sanitizer,Blink Style Guide: Don't mix Create () factory methods and public constructors in one class. Please make this constructor private (or protected) and use `base::PassKey` to allow `MakeGarbageCollected` to access it, as this class provides `CreateSafe`/`CreateUnsafe` factory methods.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Related details
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Vogelheim (Gerrit)
Daniel Vogelheim added 13 comments![]( "Open in Gerrit")
Hi! Generally LGTM, and I like this approach! I think there's one bug (about the <template> thing in sanitizer.cc); and I'd prefer to drop the friend class. Please see comments there.
There's a whole bunch of nitpicks. Please feel free to ignore those if you feel those aren't helpful.
#include "third_party/blink/renderer/core/sanitizer/streaming_sanitizer.h"Nitpick: I think forward-declared StreamingSanitizer would be enough.
while (sanitizer_->ShouldReplaceWithChildren(next->GetNode())) {I don't understand this loop. What's the intent?
class HTMLTreeBuilder final : public GarbageCollected<HTMLTreeBuilder> {super nitpick: :) Empty line before class HTMLTreeBuilder.
friend class StreamingSanitizer;Hmm. I think this is for access to Action/ActionForNode/SanitizeSingleNode. I think I'd prefer to just make those methods public, rather than adding a friend class.
#include "third_party/blink/renderer/core/dom/container_node.h"nitpick: I don't see ContainerNode being used. Is this needed?
// Step 1: Assert: child implements Text, Comment, Element, DocType.Hmm. This comment is based on the following note in the spec: https://wicg.github.io/sanitizer-api/#sanitize-core
I wonder whether streaming HTML upholds the same guarantees. I'd expect to, but...am not sure. I.e., could there be a processing instruction or somesuch?
switch (action) {nitpick: I think action isn't used anywhere else, so we just `switch (ActionForNode(node, root)) {
ProcessElement(To<Element>(node), safe);Hmm. I think this also needs SanitizeTemplate, in case a <template> is in the input stream. Maybe we can move that into ProcessElement, too.
#include "third_party/blink/renderer/core/sanitizer/streaming_sanitizer.h"nitpick: I think the forward declaration is sufficient.
enum class Action {nitpick: Is this used in the public API? If not, maybe move to implementation file.
// Copyright 2026The Chromium Authorssuper nitpick: Space between year and name.
Sanitizer API has a few tests based on the "html5lib" format, where we test against a specific tree of nodes. Long-term, it'd be really nice if those could be run through the streaming parser, too.
- third_party/blink/web_tests/external/wpt/sanitizer-api/sanitizer-basic-filtering.tentative.html
- third_party/blink/web_tests/external/wpt/sanitizer-api/sethtml-safety.tentative.html
- third_party/blink/web_tests/external/wpt/sanitizer-api/sethtml-tree-construction.tentative.html
Related details
- Noam Rosenthal
- Philip Jägenstedt
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Noam Rosenthal (Gerrit)
Noam Rosenthal added 13 comments![]( "Open in Gerrit")
while (sanitizer_->ShouldReplaceWithChildren(next->GetNode())) {I don't understand this loop. What's the intent?
Added a comment. This is for "replaceWithChildren". We want to find the next item in the stack that is allowed to be added.
class HTMLTreeBuilder final : public GarbageCollected<HTMLTreeBuilder> {super nitpick: :) Empty line before class HTMLTreeBuilder.
Done
Hmm. I think this is for access to Action/ActionForNode/SanitizeSingleNode. I think I'd prefer to just make those methods public, rather than adding a friend class.
Done
#include "third_party/blink/renderer/core/dom/container_node.h"nitpick: I don't see ContainerNode being used. Is this needed?
Done
// Step 1: Assert: child implements Text, Comment, Element, DocType.Hmm. This comment is based on the following note in the spec: https://wicg.github.io/sanitizer-api/#sanitize-core
I wonder whether streaming HTML upholds the same guarantees. I'd expect to, but...am not sure. I.e., could there be a processing instruction or somesuch?
Added a TODO.
nitpick: I think action isn't used anywhere else, so we just `switch (ActionForNode(node, root)) {
Done
Hmm. I think this also needs SanitizeTemplate, in case a <template> is in the input stream. Maybe we can move that into ProcessElement, too.
Te children of the element would be sanitized as they are added to the template's content. Added a test to verify.
#include "third_party/blink/renderer/core/sanitizer/streaming_sanitizer.h"nitpick: I think the forward declaration is sufficient.
Done
nitpick: Is this used in the public API? If not, maybe move to implementation file.
Done
bool safe_ = false;nit: Precede boolean values with words like “is” and “did”. Consider renaming 'safe_' to 'is_safe_'. (Blink Style Guide: Naming - Precede boolean values with words like “is” and “did”)
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
OK But Won't Fix
StreamingSanitizer(const Sanitizer* sanitizer,Blink Style Guide: Don't mix Create () factory methods and public constructors in one class. Please make this constructor private (or protected) and use `base::PassKey` to allow `MakeGarbageCollected` to access it, as this class provides `CreateSafe`/`CreateUnsafe` factory methods.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Acknowledged
super nitpick: Space between year and name.
Done
Sanitizer API has a few tests based on the "html5lib" format, where we test against a specific tree of nodes. Long-term, it'd be really nice if those could be run through the streaming parser, too.
- third_party/blink/web_tests/external/wpt/sanitizer-api/sanitizer-basic-filtering.tentative.html
- third_party/blink/web_tests/external/wpt/sanitizer-api/sethtml-safety.tentative.html
- third_party/blink/web_tests/external/wpt/sanitizer-api/sethtml-tree-construction.tentative.html
Agreed, I wanted a small set of "happy" tests first just to get momentum. I'll keep this in mind when finishing up.
Related details
- Daniel Vogelheim
- Philip Jägenstedt
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Noam Rosenthal (Gerrit)
Noam Rosenthal added 1 comment![]( "Open in Gerrit")
#include "third_party/blink/renderer/core/sanitizer/streaming_sanitizer.h"Noam RosenthalNitpick: I think forward-declared StreamingSanitizer would be enough.
Done
Daniel Vogelheim (Gerrit)
Daniel Vogelheim voted and added 8 comments![]( "Open in Gerrit")
Votes added by Daniel Vogelheim
| Code-Review | +1 |
8 comments
Thanks, this looks good!
Some more nitpicks on headers + forward decls.
#include "third_party/blink/renderer/core/sanitizer/streaming_sanitizer.h"This is included and forward-declared below. I think forward-declaring is sufficient.
while (sanitizer_->ShouldReplaceWithChildren(next->GetNode())) {Noam RosenthalI don't understand this loop. What's the intent?
Added a comment. This is for "replaceWithChildren". We want to find the next item in the stack that is allowed to be added.
Thanks, I get it now.
There is one more, admittedly rather theoretical concern: Sanitizer instances are mutable. So if one were to do a streaming write; then modify the Sanitizer used; and then do another streaming write; then this might apply the current config to elements parsed with the previous one.
That said, I'm not sure what to do... Maybe a TODO would work? Or should this be raised as an issue on the spec repo?
while (sanitizer_->ShouldReplaceWithChildren(next->GetNode())) {Noam RosenthalI don't understand this loop. What's the intent?
Added a comment. This is for "replaceWithChildren". We want to find the next item in the stack that is allowed to be added.
Marked as resolved.
#include "third_party/blink/renderer/core/sanitizer/streaming_sanitizer.h"I'm not seeing a usage of StreamingSanitizer in this header. Do we need this include?
// Step 1: Assert: child implements Text, Comment, Element, DocType.Noam RosenthalHmm. This comment is based on the following note in the spec: https://wicg.github.io/sanitizer-api/#sanitize-core
I wonder whether streaming HTML upholds the same guarantees. I'd expect to, but...am not sure. I.e., could there be a processing instruction or somesuch?
Added a TODO.
Marked as resolved.
That's quite a few headers here.... I'm not seeing use of:
- callback_forward.h
- v8_sanitizer_presets.h
- container_node.h
- qualified_name.h
- sanitizer_names.h
- script_wrappable.h
- heap_hash_map.h
Can we use forward-declarations instead?
#include "third_party/blink/renderer/core/dom/container_node.h"I'm not seeing any non-trivial usage of ContainerNode. Maybe we could forward-declare that?
Related details
- Noam Rosenthal
- Philip Jägenstedt
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Vogelheim (Gerrit)
Daniel Vogelheim voted and added 1 comment![]( "Open in Gerrit")
Votes added by Daniel Vogelheim
| Code-Review | +1 |
1 comment
Related details
- Noam Rosenthal
- Philip Jägenstedt
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philip Jägenstedt (Gerrit)
Philip Jägenstedt voted and added 4 comments![]( "Open in Gerrit")
Votes added by Philip Jägenstedt
| Code-Review | +1 |
4 comments
await writer.write("<table><td>Content");Can you also test the case <table><p> where the <p> would be inserted before the <table> element?
}, `element.${method}: sanitizer takes affect after foster parenting`);```suggestion
}, `element.${method}: sanitizer takes effect after foster parenting`);
```
}, `element.${method}: sanitizer takes affect after adoption agency`);```suggestion
}, `element.${method}: sanitizer takes effect after adoption agency`);
```
}, `element.${method}: mutating the sanitizer while streaming does not affect the originally given sanitizer`);```suggestion
}, `element.${method}: mutating the sanitizer while streaming does not effect the originally given sanitizer`);
```
Related details
- Noam Rosenthal
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Noam Rosenthal (Gerrit)
Noam Rosenthal added 7 comments![]( "Open in Gerrit")
#include "third_party/blink/renderer/core/sanitizer/streaming_sanitizer.h"Noam RosenthalThis is included and forward-declared below. I think forward-declaring is sufficient.
Done
That's quite a few headers here.... I'm not seeing use of:
- callback_forward.h
- v8_sanitizer_presets.h
- container_node.h
- qualified_name.h
- sanitizer_names.h
- script_wrappable.h
- heap_hash_map.h
Can we use forward-declarations instead?
Done
#include "third_party/blink/renderer/core/dom/container_node.h"Noam RosenthalI'm not seeing any non-trivial usage of ContainerNode. Maybe we could forward-declare that?
Done
Can you also test the case <table><p> where the <p> would be inserted before the <table> element?
Done
}, `element.${method}: sanitizer takes affect after foster parenting`);```suggestion
}, `element.${method}: sanitizer takes effect after foster parenting`);
```
Fix applied.
}, `element.${method}: sanitizer takes affect after adoption agency`);```suggestion
}, `element.${method}: sanitizer takes effect after adoption agency`);
```
Fix applied.
}, `element.${method}: mutating the sanitizer while streaming does not affect the originally given sanitizer`);```suggestion
}, `element.${method}: mutating the sanitizer while streaming does not effect the originally given sanitizer`);
```
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Noam Rosenthal (Gerrit)
Noam Rosenthal voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/57331.
When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.
WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
7 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: third_party/blink/web_tests/external/wpt/html/dom/partial-updates/tentative/stream-html-sanitizer.html
Insertions: 17, Deletions: 3.
@@ -159,7 +159,21 @@
await writer.write("<table><td>Content");
await writer.close();
assert_equals(placeholder.innerHTML, "<table><tbody><td>Content</td></tbody></table>");
- }, `element.${method}: sanitizer takes affect after foster parenting`);
+ }, `element.${method}: sanitizer takes effect after foster parenting`);
+
+ promise_test(async (t) => {
+ const placeholder = document.createElement("div");
+ const writer = placeholder
+ [method]({
+ sanitizer: {
+ replaceWithChildrenElements: ["tr"]
+ },
+ })
+ .getWriter();
+ await writer.write("<table><p><p>Content<td>");
+ await writer.close();
+ assert_equals(placeholder.innerHTML, "<p></p><p>Content</p><table><tbody><td></td></tbody></table>");
+ }, `element.${method}: sanitizer with foster parenting: <table><p><p>`);
promise_test(async (t) => {
const placeholder = document.createElement("div");
@@ -173,7 +187,7 @@
await writer.write("<p>1<b>2<i>3</b>4</i>5</p>");
await writer.close();
assert_equals(placeholder.innerHTML, "<p>1<b>2</b>5</p>");
- }, `element.${method}: sanitizer takes affect after adoption agency`);
+ }, `element.${method}: sanitizer takes effect after adoption agency`);
promise_test(async (t) => {
const placeholder = document.createElement("div");
@@ -186,6 +200,6 @@
await writer.write("<p>2</p>");
await writer.close();
assert_equals(placeholder.textContent, "12");
- }, `element.${method}: mutating the sanitizer while streaming does not affect the originally given sanitizer`);
+ }, `element.${method}: mutating the sanitizer while streaming does not effect the originally given sanitizer`);
}
</script>
```
Change information
Implement (unsafe) streaming sanitizer
This is based on the draft algorithm in
https://github.com/WICG/sanitizer-api/issues/190#issuecomment-3633021106
- extract single-node decisions/operations in Sanitizer to own methods
- expose a method that sanitizes a single node,
and another one that queries specifically for replacing element with
children.
- The streaming sanitizer is passed to the parser when using the
streaming methods.
- when a node is about to be inserted, we consult the streaming
sanitizer about whether the node itself should be modified/dropped,
and whether it should skip some of the stack due to
replaceElementWithChildren.
For now only the unsafe version is implemented, as streamHTML is not yet
exposed.
This needs a much wider test coverage, for now this is a prototype to
feed the spec discussion.
- M third_party/blink/renderer/core/html/html_stream.cc
- M third_party/blink/renderer/core/html/parser/html_construction_site.cc
- M third_party/blink/renderer/core/html/parser/html_construction_site.h
- M third_party/blink/renderer/core/html/parser/html_document_parser.cc
- M third_party/blink/renderer/core/html/parser/html_document_parser.h
- M third_party/blink/renderer/core/html/parser/html_tree_builder.cc
- M third_party/blink/renderer/core/html/parser/html_tree_builder.h
- M third_party/blink/renderer/core/sanitizer/build.gni
- M third_party/blink/renderer/core/sanitizer/sanitizer.cc
- M third_party/blink/renderer/core/sanitizer/sanitizer.h
- M third_party/blink/renderer/core/sanitizer/sanitizer_api.cc
- M third_party/blink/renderer/core/sanitizer/sanitizer_api.h
- A third_party/blink/renderer/core/sanitizer/streaming_sanitizer.cc
- A third_party/blink/renderer/core/sanitizer/streaming_sanitizer.h
- A third_party/blink/web_tests/external/wpt/html/dom/partial-updates/tentative/stream-html-sanitizer.html
Code-Review: +1 by Daniel Vogelheim, +1 by Philip Jägenstedt
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/57331
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |