Repository navigation
New HTML setters - #46127
New HTML setters#46127tunetheweb wants to merge 24 commits into
Conversation
| The **`afterHTML()`** method of the {{domxref("CharacterData")}} interface provides an XSS-safe method to parse and sanitize a string of HTML and insert it into the DOM after the existing character data. | ||
|
|
||
| The method removes any elements and attributes that are considered XSS-unsafe, even if allowed by a passed sanitizer. | ||
| Notably, the following elements are always removed: {{HTMLElement("script")}}, {{HTMLElement("frame")}}, {{HTMLElement("iframe")}}, {{HTMLElement("embed")}}, {{HTMLElement("object")}}, {{SVGElement("use")}}, and event handler attributes. |
There was a problem hiding this comment.
This is a lot of duplication. I see this was added in #41695, in response to #41655, but I'm not sure that was a good choice. Am I right in thinking that the "elements that are always removed" are the subset of the ones in https://developer.mozilla.org/en-US/docs/Web/API/HTML_Sanitizer_API/Default_sanitizer_configuration listed in the first bullet as "Those that are known to be XSS-unsafe:"? If so this list omits <base>, is there a reason for this?
It would be better IMO to adapt https://developer.mozilla.org/en-US/docs/Web/API/HTML_Sanitizer_API/Default_sanitizer_configuration so it gives us a clear place to link "XSS-unsafe items", and link to it.
Also attributes are not elements, so "the following elements are always removed: ... and event handler attributes." is wrong.
There was a problem hiding this comment.
This is a lot of duplication.
Yeah I said "There is a lot of duplication here though so I'm open to ideas of how to structure this better" in this initial PR comment because of things like this.
Am I right in thinking that the "elements that are always removed" are the subset of the ones in https://developer.mozilla.org/en-US/docs/Web/API/HTML_Sanitizer_API/Default_sanitizer_configuration listed in the first bullet as "Those that are known to be XSS-unsafe:"? If so this list omits , is there a reason for this?
I think that's correct. And no good reason that <base> isn't included, other than I guess it's not a common an element and I think the "Notably" here is meant to indicate this is a subset rather than a complete list. But agree it could be clearer.
I'm torn between linking, and avoiding readers having to link off to another page just to grasp the concepts.
Also attributes are not elements, so "the following elements are always removed: ... and event handler attributes." is wrong.
This would probably better worded as "and similarly for unsafe event handler attributes (for example onclick)". Happy to make that change across the board if we keep this text here.
There was a problem hiding this comment.
Changed it to this everywhere:
The method removes any elements and attributes that are considered XSS-unsafe, even if allowed by a passed sanitizer.
Notably, the following elements are always removed: {{HTMLElement("script")}}, {{HTMLElement("frame")}}, {{HTMLElement("iframe")}}, {{HTMLElement("embed")}}, {{HTMLElement("object")}}, {{HTMLElement("base")}}, {{SVGElement("use")}}, as well event handler content attributes (for example,onclick,onload,onerrorand so forth). See default sanitizer configuration for more information.
There was a problem hiding this comment.
I'm torn between linking, and avoiding readers having to link off to another page just to grasp the concepts.
I think linking to a single source is better for readers, because they know, for sure, what the concept means. As you say, "Notably" is kind of a weasel word: it suggests "there might be other things in the definition" and as a reader of a reference page, I want to know: what are they?
There was a problem hiding this comment.
I was hoping adding the link would satisfy your concern here.
There's lots of nuance and detail on the https://developer.mozilla.org/en-US/docs/Web/API/HTML_Sanitizer_API/Default_sanitizer_configuration but I think the important point (that most developers will care about) is provided in this sentence. I'm not sure excluding this completely and only linking to really long page with complete details is better.
So I guess what I'm saying is that "Notably" here is a a high-level summary of the main points that devs need to care about, rather than a weasel word trying to say something without really saying anything. And the link is then provided for those that do want more detail.
Does that persuade you at all?
There was a problem hiding this comment.
There's lots of nuance and detail on the https://developer.mozilla.org/en-US/docs/Web/API/HTML_Sanitizer_API/Default_sanitizer_configuration
Yes, there is, but that's because, according to the page itself, it's not just about XSS-unsafe:
This configuration removes the following sorts of items:
- Those that are known to be XSS-unsafe:
<base>,<embed>,<frame>,<iframe>,<object>,<script>, and<use>.- All event handler content attributes, such as onafterprint, onbeforeinput, and so on.
- ...
...
Note: Calling Sanitizer.removeUnsafe(), or passing a custom sanitizer to the safe sanitization method, only removes the XSS-unsafe items. It does not remove the additional items, comments, and data-* attributes.
That's why I didn't suggest just linking to the page, but adapting it so it gives us a clear place to link "XSS-unsafe items", and linking to that place, so we can have one concise definition of it that everyone uses.
So I guess what I'm saying is that "Notably" here is a a high-level summary of the main points that devs need to care about, rather than a weasel word trying to say something without really saying anything.
I meant "weasel word" just in a technical sense not pejoratively, but I'm sorry if it seemed like that. But now you added <base> to the list, this doesn't seem to be a summary of what's in the page, but just the same thing. The page doesn't add more detail about XSS unsafe, AFAICS.
There was a problem hiding this comment.
FWIW I think https://developer.mozilla.org/en-US/docs/Web/API/HTML_Sanitizer_API/Using_the_HTML_Sanitizer_API also suffers from not having a definitive place to link "XSS unsafe":
For example, the following code will remove all XSS-unsafe elements and attributes in the input string (in this case the <script> element), along with any elements that aren't permitted as children of the target element by the HTML specification:
There was a problem hiding this comment.
I think you're right. But also a little worried about defining this when I'm not 100% sure I'd be right about it! It's kinda an existing issue and orthogonal to documenting these new APIs that I'm trying to do here.
I think this is the official definition: https://html.spec.whatwg.org/multipage/dynamic-markup-insertion.html#remove-unsafe but not confident enough to translate that spec language to MDN. Or even to understand it fully truth be told! I hate the HTML spec FWIW :-)
|
Any more thoughts on this one @wbamberg ? Appreciate it's a big one and will take some time to review! |
|
Sorry @tunetheweb , I will take a look today. |
|
I haven't looked at everything here yet but I think it's worth making some high-level choices before getting into all the details. I've looked fairly quickly at all the non-unsafe setter pages, and I think we should:
|
wbamberg
left a comment
There was a problem hiding this comment.
Sorry this is a little chaotic. I had a few more comments for now, some of which I made before but never submitted apparently.
| The **`afterHTML()`** method of the {{domxref("CharacterData")}} interface provides an XSS-safe method to parse and sanitize a string of HTML and insert it into the DOM after the existing character data. | ||
|
|
||
| The method removes any elements and attributes that are considered XSS-unsafe, even if allowed by a passed sanitizer. | ||
| Notably, the following elements are always removed: {{HTMLElement("script")}}, {{HTMLElement("frame")}}, {{HTMLElement("iframe")}}, {{HTMLElement("embed")}}, {{HTMLElement("object")}}, {{SVGElement("use")}}, and event handler attributes. |
There was a problem hiding this comment.
I'm torn between linking, and avoiding readers having to link off to another page just to grasp the concepts.
I think linking to a single source is better for readers, because they know, for sure, what the concept means. As you say, "Notably" is kind of a weasel word: it suggests "there might be other things in the definition" and as a reader of a reference page, I want to know: what are they?
FWIW the existing intro to safe vs unsafe methods is https://developer.mozilla.org/en-US/docs/Web/API/HTML_Sanitizer_API#sanitization_methods ... but where you might also update (for cross linking) is https://developer.mozilla.org/en-US/docs/Web/API/HTML_Sanitizer_API#extensions_to_other_interfaces |
I've added these methods to the first section. I'm not sure the section section adds much at all beyond that. And I wouldn't see these as extensions to those interfaces per se. Especially the "safe" versions where the Sanitzier API is no so much an "extension" as an integral part of this API! Now it's true that Safari has shipped |
I think this is already covered? Though I've tweaked it a little. And ensured all the new methods are listed too. Though technically they are not really part of the Sanitizer API, but it's a bit blurry anyway since they are all part of HTML rather than a separate "Sanitizer API" spec.
I've added a link to the Sanitizer API docs but I've left most of the text in place as well as this. Most of the duplicated text talks about config to these methods, rather than specific sanitizer options. So not sure this text should be replaced with links to the Sanitizer API. Re-reading it I'm not sure it's that much duplicated text. And if what is duplicated even is a bad thing. But open to more suggestions here... |
"extensions to other interfaces" is how we capture the situation where "APIs" (aka specifications) extend existing interfaces, rather than cases where they define completely new interfaces. Here, the specification defines a couple of new interfaces, |
Yeah I get that, and I think that makes sense when an external spec extends another spec. In this case it's a little greyer IMHO as both are in the same spec (HTML). Though admittedly in different sections 8.5 and 8.6, though they are right beside each other and all under section 8. Plus the "safe" methods (and arguably the "unsafe" ones too - see previous comment) don't make any sense without this — you can't have safe methods without sanitizer, so it's not like they existed first, and then the sanitizer API extended them. One view is that they are all part of the sanitizer — in fact @ddbeck and I had pretty much this discussion on the web-features PR for the HTML setters. IMHO the Sanitizer API is part of HTML now, and keeping this "extensions to other interfaces" section when it doesn't say much beyond the "sanitization methods: section is not adding any value, and making this page much longer than it needs to be. It's also questionable whether it makes sense since it only lists methods that are already listed as methods of this API in the left hand nav. And all this will only get worse once we add these 20 APIs to this section (and another 20 when we document the streaming versions that just landed in HTML yesterday). We wouldn't say |
|
This pull request has merge conflicts that must be resolved before it can be merged. |
Description
This documents the static HTML setters as detailed here:
https://developer.chrome.com/docs/web-platform/declarative-partial-updates#renewed_html_insertion_and_streaming_methods
I have excluded the streaming APIs (will follow up with that in a future PR), and also excluded
TrustedParserOptions(will follow up with that in a future PR).This is quite a big PR, but most of it is copied and pasted from the existing
setHTML()andsetHMTLUnsafe()docs with minor updates.There is a lot of duplication here though so I'm open to ideas of how to structure this better if we want minimal docs in
ShadowRootandCharacterDataversions of these docs and leave the main docs to just theElementversions of these APIs and reference it? Or alternatively we could leave most of the docs insetHTML,setHTMLUnsafeand reference those from the other docs?Motivation
These were released in Chrome 157 (they were available behind a flag before that) and have also been merged in to the HTML Living standard (whatwg/html#12753)
Additional details
Docs: https://developer.chrome.com/docs/web-platform/declarative-partial-updates#renewed_html_insertion_and_streaming_methods
Chrome Status: https://chromestatus.com/feature/5054329641893888
Spec PR: whatwg/html#12753
Related issues and pull requests
They were already added to BCD in mdn/browser-compat-data#30328 and a follow up with spec links mdn/browser-compat-data#30874