Integrate Trusted Types enforcement into attribute handling #1268

lukewarlow · 2024-03-27T19:22:16Z

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

At least two implementers are interested (and none opposed):
- Chromium
- Gecko
Tests are written and can be reviewed and commented upon at:
- https://github.com/web-platform-tests/wpt/tree/master/trusted-types
Implementation bugs are filed:
- Chromium:https://bugs.chromium.org/p/chromium/issues/detail?id=739170
- Gecko: https://bugzilla.mozilla.org/show_bug.cgi?id=1508286
- WebKit: https://bugs.webkit.org/show_bug.cgi?id=266630
MDN issue is filed: N/A
The top of this comment includes a clear commit message to use.

(See WHATWG Working Mode: Changes for more details.)

Preview | Diff

lukewarlow · 2024-03-27T19:22:41Z

This is a clone of #1247 where I will finish any outstanding work to get this across the line

lukewarlow · 2024-04-11T12:21:53Z

See also #1258 which is another integration point with the DOM spec that we need for TT.

dom.bs

otherdaniel · 2024-04-15T17:13:57Z

dom.bs

+   <li><p><a>Validate and set attribute value</a> <var>attr</var>'s <a for="Attr">value</a> for
+   <var>attr</var> with <var>element</var>.
+
+   <li><p>If <var>element</var> <a lt="has an attribute">has</a> an <a>attribute</a>


I still find it difficult to wrap my head around this logic. I think conceptually, we want to have the old value -- from before the call, and thus before a default policy might have mucked with it. That's what should go into the change attribute logic. But once we have that, I'm not sure why we'd need to throw an exception here. I'm not sure why we'd have to care whether the default policy does anything funny with the attribute in the mean time.

I've read through this spec path and I believe that, if the default policy removes the existing attribute node, then this will call replace an attribute, which in turn calls replace a list item, which results in a no-op because the old value no longer exists to be replaced.

So I think in this situation you're probably right the spec doesn't need to do anything, will make that change.

Having said that I'm slightly uneasy about stuff like attribute change steps still firing and how that all works spec wise.

Both Chromium and WebKit don't actually follow the spec 100% here and so I think would actually result in a different behaviour to this spec (the index lookup happens after the TT call) so that's also not ideal.

lukewarlow · 2024-04-22T13:18:23Z

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. iframe.getAttributeNode('srcdoc').value = trustedHTMLObj; will throw unless allowed by a default policy). I think in pratice this will be fine, and in some cases is the only real option we have without nodes keeping track of whether they're trusted or not (which would add lots of complexity)

annevk

This change is either incomplete or makes many cosmetic changes that would be best proposed separately as they confuse me quite a bit.

dom.bs

lukewarlow · 2024-04-22T15:47:56Z

This change is either incomplete or makes many cosmetic changes that would be best proposed separately as they confuse me quite a bit.

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

annevk · 2024-04-22T16:07:37Z

dom.bs

+ <li><p>If <var>attribute</var>'s <a for=Attr>element</a> <a lt="has an attribute">has</a>
+ an <a>attribute</a> <var>attribute</var>, then <a>handle attribute changes</a> for
+ <var>attribute</var> with <var>attribute</var>'s <a for=Attr>element</a>, <var>oldValue</var>, and
+ <var>value</var>.


Should this be value or attribute's value? They can be different, right?

dom.bs

lukewarlow · 2024-05-07T15:54:52Z

@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)

annevk · 2024-05-13T14:58:26Z

Attributes are stored in a list and those do have indices per Infra. What am I missing?

lukewarlow · 2024-05-13T15:03:24Z

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.

annevk · 2024-05-13T15:11:38Z

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.

fred-wang · 2025-02-10T17:44:31Z

Just to be more explicit, I believe we have three categories of APIs:

Element.setAttribute/Element.setAttributeNS: It seems browsers agree to always successfully set the attribute in any case. If the attribute node is moved to another element in the default's policy callback, we end up with the attribute set on two different elements.
Element.setAttributeNode, Element.setAttributeNodeNS, NamedNodeMap.setNamedItem, NamedNodeMap.setNamedItemNS: If the attribute node is moved to another element in the default's policy callback, WebKit and Chromium may end up associating the attribute node to two different element's attribute maps. My proposal is to throw an InUseAttributeError error if the attribute node was moved to another element (which is what would be happening if we re-run https://dom.spec.whatwg.org/#concept-element-attributes-set without TT check).
Attr.value, Attr.nodeValue, Attr.textContent: These APIs essentially operate on an attribute. If the attribute's element is non-null, then WebKit and Chromium just calls setAttributeNS on that element and no new issue arises. The spec instead performs the TT check before setting the element's attribute but does not say what to do when the attribute's element changes (i.e. attribute is detached from the element or moved to another element). My proposal is again to just re-run current https://dom.spec.whatwg.org/#set-an-existing-attribute-value without TT check. This would differ from WebKit/Chromium's current implementation.

I updated https://phabricator.services.mozilla.com/D236161 to match this proposal ; and https://phabricator.services.mozilla.com/D236107 makes all these new tests pass. I'm happy to change to something else depending on what we decide at the end.

otherdaniel · 2025-02-13T14:00:28Z

The solution outlined here is correct & self-consistent, and if all implementations agree I'll be happy.

I still think we can be simpler, though: The default policy introduces a seemingly concurrent operation. All operations, on all three categories of APIs outlined above, can be explained by serializing these operations.

Consider: "To set an attribute given an attribute and an element":
As spec'ed here: First check structure; then run TT check; then re-check structure.
- TT error => throws TypeException
- structure error but no TT error => DOMException AttributeInUseError
- structure error created in TT callback => DOMException AttributeInUseError
- TT error and structure error =>TypeException

If instead, we spec it like so:

Let verifiedValue be value.
If verify is true: [... TT check; assign to verifiedValue ...]
If attr’s element is neither null nor element, throw an "InUseAttributeError" DOMException
[... old spec, but verifiedValue instead of value ...]

I think this would get us all of the same error conditions, but fewer operations. And IMHO easier to understand. And I like it, because explaining this behaviour in terms of serializing the checks and running the TT check gives us a guideline which we can apply to all similar cases.

There is one observable difference I can think of: If we have a structure error, and if the TT policy itself throws an exception, then in the first case we'd get AttributeInUseError, and in the second we'd get TypeException. I think that's okay.

annevk · 2025-02-18T16:46:34Z

Any updates here? I rather like @otherdaniel's suggestion.

lukewarlow · 2025-02-18T16:52:55Z

If others are happy with that idea I'm happy to look into updating this PR to match.

smaug---- · 2025-02-18T21:42:51Z

I think that would work for setNameItem* and setAttributeNode*.
Current code in Gecko does "If attr’s element is neither null nor element" before and after, and maybe doing it only after is good enough. right @fred-wang ?

Attr.value/.nodeValue/.textContent should do validation (if there is element) too before modifying the value.
(chromium and webkit seem to do something else, assuming I'm reading the code correctly)

Element.setAttribute/Element.setAttributeNS in the PR needs some tweaking too. Implementations don't necessarily create Attr nodes when those APIs are used, but if they do, what happens if TT callback moves the Attr away. As fred-wang mentioned earlier, implementations already just create a new attribute on the element.

lukewarlow · 2025-02-20T14:57:35Z

I've pushed up the change to "set an attribute", and to "set an existing attribute value".

I'm looking into the setAttribute methods now.

Attr.value/.nodeValue/.textContent should do validation (if there is element) too before modifying the value.
(chromium and webkit seem to do something else, assuming I'm reading the code correctly)

WebKit at least calls element->setAttribute so the TT check happens slightly later and this can lead to the logic running on the wrong element.

I've updated the spec to make it clearer what should happen here. I believe this is slightly different to Firefox's recently implementation so will need test changes.

Element.setAttribute/Element.setAttributeNS in the PR needs some tweaking too. Implementations don't necessarily create Attr nodes when those APIs are used, but if they do, what happens if TT callback moves the Attr away. As fred-wang mentioned earlier, implementations already just create a new attribute on the element.

I'll investigate what Firefox is doing here and see what the best approach to take is. Generally it seems we want to move TT earlier in the process if possible so I'll try to achieve that.

Perhaps we can move step 6 higher up, but it would have to be after step 3 at the earliest.

annevk

I did a quick editorial review. Please double check the comments throughout as some apply several times.

dom.bs

Move the Trusted Types check -- which can modify the DOM -- *before* the structure checks, to prevent inconsistencies from concurrent manipulation of the same attribute. This fixes a number of WPT subtests in trusted-types/modify-attributes-in-callback.html and trusted-types/set-attributes-mutations-in-callback.tentative.html Spec: whatwg/dom#1268 Bug: 394760815, 330516530 Change-Id: I9a394a75348598cc68e5b073d8769c826cd0605a Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6243963 Commit-Queue: Daniel Vogelheim <vogelheim@chromium.org> Reviewed-by: Joey Arhar <jarhar@chromium.org> Cr-Commit-Position: refs/heads/main@{#1431595}

dom.bs

…Attr.value like setters The draft spec PR now early returns in the case the element of the attribute changes rather than updating the value. See whatwg/dom#1268

dom.bs

annevk

I would love to have a more detailed description of what we are trying to accomplish here. I don't think the resulting algorithms are very clear. Have you looked at whether it's possible to abstract certain bits?

dom.bs

annevk · 2025-05-19T08:13:05Z

dom.bs

+ <li><p>If <var>attribute</var> is null, then set <var>attribute</var> to an <a>attribute</a>
+ whose <a for=Attr>local name</a> is <var>qualifiedName</var>, <a for=Attr>value</a> is
+ <var>value</var>, and <a for=Node>node document</a> is <a>this</a>'s <a for=Node>node document</a>.


Is the idea here to create a dummy attribute that you don't append in order to calculate the verified value?

Why is that done this way? This is quite confusing to read through.

Apologies for the slow reply on here I thought I replied before but it must have got lost.

TLDR: I agree with you I can't really remember why it's done this way. I updated the TT spec to just take the local name and namespace directly rather than an attribute object. As part of doing this change I realised these algorithms were overly complicated for what was actually needed.

Hopefully now the integration points should be much cleaner and less invasive.

… from the various algorithms, also update call signature to match changes in TT.

lukewarlow · 2025-07-03T08:57:52Z

dom.bs

@@ -7013,6 +7020,8 @@ string <var>namespace</var> (default null):</p>

 <li><p>Otherwise, <a lt="append an attribute">append</a> <var>attr</var> to <var>element</var>.

+ <li><p>Set <var>attr</var>'s <a for=Attr>value</a> to <var>verifiedValue</var>.


Need to check if this is in the right place in the algorithm.

cc @fred-wang

(1) So in case oldAttr == attr, its value isn't updated.

(2) 'append an attribute' calls 'Handle attribute changes', and verifiedValue is set to attr after that. That doesn't seem right. Shouldn't value be set before?

So in case oldAttr == attr, its value isn't updated.

Yes, I think that makes sense? If the attribute hasn't changed does it make sense to put it through Trusted Types default policy? I'll double check what browsers actually do in this case though.

'append an attribute' calls 'Handle attribute changes', and verifiedValue is set to attr after that. That doesn't seem right. Shouldn't value be set before?

Ah yeah, you're right, step 7 should be moved to step 5.

I've fixed 2 now.

So I double checked and for point 1 there's actually an interop bug here, WebKit (and Gecko) doesn't throw a TT error. Chromium currently does, so we can choose whichever behaviour we think is best here.

cc @annevk and @otherdaniel for your opinions

TLDR should doing

element.setAttributeNode(element.attributes[1]) - throw a TT error when TT is enforced (and where element.attributes[1] is something like an onclick attribute?

lukewarlow · 2025-07-03T09:58:44Z

@otherdaniel I believe this spec PR fully matches your proposals to call TT as early as possible now. If you've got time I'd be thankful for a review to get another pair of eyes on this.

…ired.

lukewarlow force-pushed the trusted-types-attributes branch 2 times, most recently from 3562fcb to ac5b4aa Compare April 10, 2024 15:52

lukewarlow mentioned this pull request Apr 11, 2024

Draft integration with Trusted Types, take 2. #1247

Closed

5 tasks

lukewarlow force-pushed the trusted-types-attributes branch from 524d8cd to f8877b6 Compare April 11, 2024 12:18

lukewarlow marked this pull request as ready for review April 11, 2024 12:21

lukewarlow marked this pull request as draft April 15, 2024 10:22

lukewarlow force-pushed the trusted-types-attributes branch from f8877b6 to ee9915e Compare April 15, 2024 11:45

otherdaniel reviewed Apr 15, 2024

View reviewed changes

dom.bs Outdated Show resolved Hide resolved

otherdaniel reviewed Apr 15, 2024

View reviewed changes

lukewarlow marked this pull request as ready for review April 22, 2024 12:53

lukewarlow requested review from annevk and smaug---- April 22, 2024 13:13

lukewarlow force-pushed the trusted-types-attributes branch from ba80870 to 36476b5 Compare April 22, 2024 13:57

lukewarlow mentioned this pull request Apr 22, 2024

[Meta] Upstream changes w3c/trusted-types#476

Open

annevk reviewed Apr 22, 2024

View reviewed changes

dom.bs Outdated Show resolved Hide resolved

dom.bs Outdated Show resolved Hide resolved

dom.bs Outdated Show resolved Hide resolved

dom.bs Outdated Show resolved Hide resolved

dom.bs Outdated Show resolved Hide resolved

lukewarlow force-pushed the trusted-types-attributes branch from 370f5c9 to 4421d50 Compare April 22, 2024 15:46

lukewarlow force-pushed the trusted-types-attributes branch 2 times, most recently from 1d42460 to 1eeaf00 Compare April 22, 2024 15:52

annevk reviewed Apr 22, 2024

View reviewed changes

lukewarlow force-pushed the trusted-types-attributes branch 2 times, most recently from 4089eaf to 02db8c7 Compare May 16, 2024 15:41

lukewarlow requested a review from otherdaniel May 16, 2024 15:43

lukewarlow force-pushed the trusted-types-attributes branch from 622f78d to b047352 Compare February 20, 2025 14:47

annevk reviewed Feb 20, 2025

View reviewed changes

dom.bs Outdated Show resolved Hide resolved

dom.bs Outdated Show resolved Hide resolved

dom.bs Outdated Show resolved Hide resolved

dom.bs Outdated Show resolved Hide resolved

dom.bs Outdated Show resolved Hide resolved

dom.bs Outdated Show resolved Hide resolved

otherdaniel reviewed Mar 13, 2025

View reviewed changes

dom.bs Outdated Show resolved Hide resolved

lukewarlow mentioned this pull request Mar 19, 2025

Update test assertions when default policy moves an attribute during … web-platform-tests/wpt#51459

Open

smaug---- reviewed Mar 20, 2025

View reviewed changes

dom.bs Outdated Show resolved Hide resolved

smaug---- reviewed Mar 20, 2025

View reviewed changes

dom.bs Outdated Show resolved Hide resolved

TimvdLippe mentioned this pull request Apr 1, 2025

Implement Trusted Types servo/servo#36258

Open

lukewarlow added 2 commits April 24, 2025 13:33

Trusted Types

a665ee4

Revert change to "set an attribute value"

d86e02f

lukewarlow force-pushed the trusted-types-attributes branch from 69967de to d86e02f Compare April 24, 2025 12:42

Update setAttributeNS to include TT check

1d25719

lukewarlow requested a review from fred-wang April 24, 2025 13:15

annevk reviewed May 19, 2025

View reviewed changes

Remove extra algorithm that called through to TT and call TT directly…

15e9f57

… from the various algorithms, also update call signature to match changes in TT.

lukewarlow force-pushed the trusted-types-attributes branch from f635814 to 15e9f57 Compare July 3, 2025 08:55

lukewarlow commented Jul 3, 2025

View reviewed changes

Revert larger change to setAttributeNs as its no longer neccessary.

aaaae9c

lukewarlow changed the title ~~Trusted types attributes~~ Integrate Trusted Types enforcement into attribute handling Jul 3, 2025

Set attr's value to verifiedValue before attribute change steps are f…

4ed8160

…ired.

		@@ -7013,6 +7020,8 @@ string <var>namespace</var> (default null):</p>

		<li><p>Otherwise, <a lt="append an attribute">append</a> <var>attr</var> to <var>element</var>.

		<li><p>Set <var>attr</var>'s <a for=Attr>value</a> to <var>verifiedValue</var>.

Integrate Trusted Types enforcement into attribute handling #1268

Are you sure you want to change the base?

Integrate Trusted Types enforcement into attribute handling #1268

Uh oh!

Conversation

lukewarlow commented Mar 27, 2024 • edited by pr-preview bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lukewarlow commented Mar 27, 2024

Uh oh!

lukewarlow commented Apr 11, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lukewarlow commented Apr 22, 2024

Uh oh!

annevk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lukewarlow commented Apr 22, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lukewarlow commented May 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

annevk commented May 13, 2024

Uh oh!

lukewarlow commented May 13, 2024

Uh oh!

annevk commented May 13, 2024

Uh oh!

fred-wang commented Feb 10, 2025

Uh oh!

otherdaniel commented Feb 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

annevk commented Feb 18, 2025

Uh oh!

lukewarlow commented Feb 18, 2025

Uh oh!

smaug---- commented Feb 18, 2025

Uh oh!

lukewarlow commented Feb 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

annevk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lukewarlow commented Mar 27, 2024 •

edited by pr-preview bot

Loading

lukewarlow commented May 7, 2024 •

edited

Loading

otherdaniel commented Feb 13, 2025 •

edited

Loading

lukewarlow commented Feb 20, 2025 •

edited

Loading

lukewarlow Jul 3, 2025 •

edited

Loading