Support sanitization using <template for="..." sanitize> [chromium/src : main]
Noam Rosenthal (Gerrit)
Noam Rosenthal added 1 comment![]( "Open in Gerrit")
StreamingSanitizer* sanitizer() const { return sanitizer_.Get(); }Noam Rosenthalnit: Blink Style Guide: Naming - Use 'CamelCase' for all function names, and prefix with 'Get' if the name collides with a type name. Consider renaming this getter to `GetSanitizer()` since `Sanitizer` is an existing class name.
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
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Vogelheim (Gerrit)
Daniel Vogelheim added 4 comments![]( "Open in Gerrit")
Generally very happy with this. But I'm weirded out af the syntax of the attribute. By my reading, the empty string is the only (!!) way to actually sanitize?
bool HTMLConstructionSite::SanitizeIfNeeded(HTMLConstructionSiteTask& task) {I find it difficult to reason about what the boolean return parameter means here. Maybe add a comment? (here, or in the header)
if (sanitize_val.empty()) {Not sure if I'm reading this correctly... If we have no 'sanitize' attribute, or if we have an attribute with any content (other than the empty string) sanitizer will remain nullptr?
I think the test cases support this intent, but I find it weird...
- `<template for sanitize>` => default sanitization
- `<template for>` => no sanitization
- `<template for sanitize="unsafe">` => also no sanitization
- `<template for sanitize="super duper extra safe">` => also no sanitization.
- `<template for sanitize=" ">` => Space? No sanitization.
<template for="inner-marker-1" sanitize="unsafe">... what is this supposed to do?
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 3 comments![]( "Open in Gerrit")
bool HTMLConstructionSite::SanitizeIfNeeded(HTMLConstructionSiteTask& task) {I find it difficult to reason about what the boolean return parameter means here. Maybe add a comment? (here, or in the header)
Added a comment.
Not sure if I'm reading this correctly... If we have no 'sanitize' attribute, or if we have an attribute with any content (other than the empty string) sanitizer will remain nullptr?
I think the test cases support this intent, but I find it weird...
- `<template for sanitize>` => default sanitization
- `<template for>` => no sanitization
- `<template for sanitize="unsafe">` => also no sanitization
- `<template for sanitize="super duper extra safe">` => also no sanitization.
- `<template for sanitize=" ">` => Space? No sanitization.
This is supposed to be a boolean attribute (third_party/blink/web_tests/external/wpt/html/dom/partial-updates/tentative/fragment/sanitize-nested.html)
So `<template for sanitize>`, `<template for sanitize="">` and `<template for sanitize="sanitize">` should work. I updated the code and examples.
To be extended later when we have sanitizer presets
... what is this supposed to do?
- Daniel Vogelheim
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Vogelheim (Gerrit)
Daniel Vogelheim voted and added 4 comments![]( "Open in Gerrit")
Votes added by Daniel Vogelheim
| Code-Review | +1 |
4 comments
for (HTMLStackItem* item = open_elements_.TopStackItem(); item;This "smells" like a O(n*log n) algo, since it would seem to walk up the stack of elements, on potentially every element. It might be worth checking what this does to performance on large HTML strings being streamed.
But then... perf analysis by "looking at code" tends to not be very reliable, so I'll leave it to you whether this needs more work or not.
bool sanitizer_allows_is_attribute = true;
if (auto* active_sanitizer = ActiveSanitizer()) {
sanitizer_allows_is_attribute =
active_sanitizer->AllowIsAttribute(html_names::kScriptTag);
}I think I'd find it slightly more readable if ActiveSanitizer() is assigned to a name first. Like so:
auto* active_sanitizer = ActiveSanitizer();
bool sanitizer_allows_is_attribute =
!active_sanitizer || active_sanitizer->AllowIsAttribute(html_names::kScriptTag);
(That's just personal preference, however. Proceed as you see fit.)
<template for="marker-empty-val" sanitize="">Maybe also a test case with " " (i.e., non-empty, but only whitespace)?
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. |