Integrate Trusted Types enforcement into attribute handling #1268
Conversation
|
This is a clone of #1247 where I will finish any outstanding work to get this across the line |
3562fcb to
ac5b4aa
Compare
524d8cd to
f8877b6
Compare
|
See also #1258 which is another integration point with the DOM spec that we need for TT. |
f8877b6 to
ee9915e
Compare
|
I think I've addressed all the comments from #1247. I do want to point out Chrome and WebKit don't (or at least not in a way obvious to me) 100% follow the flow of the spec and as a result this may result in differences specifically in weird cases with attribute mutation. So that bit especially it would be good to get feedback on. It's also worth being aware that like Chromium's implementation this spec means that certain ways to update a nodes value don't work with a trusted type object as a user might expect. (e.g. |
ba80870 to
36476b5
Compare
There was a problem hiding this comment.
This change is either incomplete or makes many cosmetic changes that would be best proposed separately as they confuse me quite a bit.
370f5c9 to
4421d50
Compare
Apologies I thought I'd reverted all of these changes but I missed a few, it's because I changed stuff and then changed it back and this led to some wonky diffs. Have hopefully reverted all of these unnecessary changes |
1d42460 to
1eeaf00
Compare
|
@otherdaniel, @annevk , and @smaug---- regarding the case where the default policy changes assumptions about the existence of an attribute mid-way through what would you prefer the spec say to do? Currently I've specced to throw, but Chromium currently re-looks up the index (spec doesn't explicitly work on an index basis but Chromium and WebKit do) |
|
Attributes are stored in a list and those do have indices per Infra. What am I missing? |
|
Sorry I mean algorithmically the spec and implementations don't follow the same flow. So it's trickier to reason between the spec and implementation. This might just be my lack of familiarity with these APIs too. |
|
The path of least resistance is prolly matching Chromium. Introducing new paths that throw is always risky. If you are looking for guidance as to how, I'd need quite a bit more context to provide helpful suggestions. |
4089eaf to
02db8c7
Compare
Additionally, several methods were updated with spec comments. That's because the "adopt the document from the element document" step was missing. By adding these spec comments, I also restructured some code to avoid duplication of mutation records and custom element reaction queueing. Node.textContent doesn't propagate the error yet, as that method has a lot of separate callers of elements that wouldn't fail. I will refactor those in a follow-up PR to keep things manageable. This implements part of the DOM integration from whatwg/dom#1268 Part of #36258 --------- Signed-off-by: Tim van der Lippe <tvanderlippe@gmail.com> Signed-off-by: Tim van der Lippe <TimvdLippe@users.noreply.github.com>
Additionally, several methods were updated with spec comments. That's because the "adopt the document from the element document" step was missing. By adding these spec comments, I also restructured some code to avoid duplication of mutation records and custom element reaction queueing. Node.textContent doesn't propagate the error yet, as that method has a lot of separate callers of elements that wouldn't fail. I will refactor those in a follow-up PR to keep things manageable. This implements part of the DOM integration from whatwg/dom#1268 Part of #36258 --------- Signed-off-by: Tim van der Lippe <tvanderlippe@gmail.com> Signed-off-by: Tim van der Lippe <TimvdLippe@users.noreply.github.com>
…S and non-event-handler attri…, a=testonly Automatic update from web-platform-tests Add tests for setAttribute/setAttributeNS and non-event-handler attributes (#50046) This is a tentative test for [1] covering bullet 3 from [2]. There is a separate PR to cover the bullet 2 [3]. E901 Most of these are probably already covered by other tests but assertions are scattered into multiple files as a side check in the default policy callback, so they are hard to understand and to guarantee they actually run (some mistakes were previously found about this in the past). Additionally, this new test cover both setAttribute and setAttributeNS and ensure the correct callback is called exactly once with proper attributes (e.g. to verify a call to setAttribute does not trigger the callback with the parameters of a reflected IDL attribute). [1] whatwg/dom#1268 [2] https://w3c.github.io/trusted-types/dist/spec/#get-trusted-type-data-for-attribute [3] web-platform-tests/wpt#50025 -- wpt-commits: 35de05425036bf714b270c76dd00892900002963 wpt-pr: 50046
…es-svg-script-set-href.html, a=testonly Automatic update from web-platform-tests Remove unrelated checks from trusted-types-svg-script-set-href.html (#50043) * Remove unrelated checks from trusted-types-svg-script-set-href.html This test is intended to cover setting href [1] [2] but it also currently verifies some (currently not specified) unrelated behavior when setting the text of the script via innerHTML. A similar test exist in block-text-node-insertion-into-svg-script-element.html, but for a disconnected script. This PR just move the unrelated test to trusted-types-svg-script.html instead. [1] w3c/svgwg#934 [2] whatwg/dom#1268 -- wpt-commits: 696792b6aa714c2d7c54b986814bbdbbdecf1dd8 wpt-pr: 50043
… from the various algorithms, also update call signature to match changes in TT.
3c8c6fa to
6fe40a3
Compare
There was a problem hiding this comment.
@fred-wang @otherdaniel if you could reply to @lukewarlow's comment above that would help a lot, thanks! @smaug---- did you want to have another look at this PR as well?
…attribute moves elements during the TT callback
|
🎉 |
|
Indeed, thanks a lot @lukewarlow for finishing this and @smaug---- for his careful reviews! And of course everyone else that helped improve Trusted Types integration over the years. 🎉 |
This updates the DOM spec to add the neccessary integration with the Trusted Types spec to ensure attribute values are protected by TT enforcement.
The Element.setAttribute() and Element.setAttributeNS() method steps, along with the "set an attribute" algorithm (which is used by Element.setAttributeNode(), Element.setAttributeNodeNs(), NamedNodeMap.setNamedItem() and NamedNodeMap.setNamedItemNs()), and the "set an existing attribute value" algorithm (which is used by Attr.value, Attr.nodeValue, and Attr.textContent), are all updated to allow Trusted Type's to verify the updated attribute value before it's set.
The IDL definitions for Element.setAttribute() and Element.setAttributeNS() are both updated to take a TrustedType object (can be all 3 types) in addition to DOMString, these two methods are the only "blessed" way to update an attribute value when Trusted Types is enforced.
See and #789. Supercedes #809 and #1247
(See WHATWG Working Mode: Changes for more details.)
Preview | Diff