Trusted types attributes by lukewarlow · Pull Request #1268 · whatwg/dom (original) (raw)

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation116 Commits3 Checks1 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

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

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

dom.bs Outdated Show resolved Hide resolved

Validate and set attribute value `attr`'s value for
`attr` with `element`.

If `element` has an attribute

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved

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

If `attribute`'s element has
an attribute `attribute`, then handle attribute changes for
`attribute` with `attribute`'s element, `oldValue`, and
`value`.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

dom.bs Outdated Show resolved Hide resolved

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

gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request

Jan 16, 2025

…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]. 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

UltraBlame original commit: 4846e5ab139c44c223566809053c5948cd2d8d16

gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request

Jan 16, 2025

…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

UltraBlame original commit: c08a3d36866a0421ce1e7f511543715b7a9c1b38

i3roly pushed a commit to i3roly/firefox-dynasty that referenced this pull request

Jan 17, 2025

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

[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

i3roly pushed a commit to i3roly/firefox-dynasty that referenced this pull request

Jan 17, 2025

…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

[1] w3c/svgwg#934 [2] whatwg/dom#1268

wpt-commits: 696792b6aa714c2d7c54b986814bbdbbdecf1dd8 wpt-pr: 50043

Tests are written and can be reviewed and commented upon at:
https://github.com/web-platform-tests/wpt/tree/master/trusted-types

On #1268 (comment) you mentioned the affected APIs are

setAttribute
setAttributeNS
Element.setAttributeNode
Element.setAttributeNodeNS
NamedNodeMap.setNamedItem
NamedNodeMap.setNamedItemNS
Attr.value
Node.textContent
Node.nodeValue

Is that an exhautive list?

I think we would need tests for each of the affected API to check:

That attribute change is blocked by the default policy.
That a non-trivial default policy is applied before attribute change.

I found the following tests:

block-string-assignment-to-attribute-via-attribute-node.html
block-string-assignment-to-Element-setAttribute.html
testing/web-platform/tests/trusted-types/TrustedType-AttributeNodes.html

But they don't cover e.g. setAttributeNodeNS or setNamedItemNS and the last two are a bit messy (https://phabricator.services.mozilla.com/D227943 was enough to make the last one pass in Firefox, which seems dubious to me).

We probably also need tests:

Check what happens when we pass a trusted type, which is treated differently depending on whether the API accept a TrustedXXXOrString argument).
Check what happens when the default policy does some DOM mutations (we have one such test for setAttribute/setAttributeNS).

Hi all, sorry I'm a bit late with feedback. It turns out, what Chromium does here is buggy since we are running structure checks before the Trusted Type checks and then getting inconsistent results when a default policy modifies the attribute that is being checked. Thanks for pointing this out.

My (new) preference would be to always run TT checks before any other steps; here and elsewhere. We mostly already do this. That would provide a strict sequencing of TT before any other DOM checks or DOM manipulations, and should IMHO solve this class of problems entirely. I'd be happier if we had a general rule here, and I'd rather do some more work on our end than risk us overlooking another issue like this.

For testing, I'd propose to add WPT tests with competing error conditions and checking for the correct exception being thrown. (I.e., setting an attribute node that's already connected should throw the TT-style TypeException, not the InUseAttributeError DOMException). Something like the following seems to work when I try it locally:

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.

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.

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

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.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

dom.bs Outdated Show resolved Hide resolved

Comment on lines 6654 to 6655

Let `verifiedValue` be the result of calling verify attribute value
`attr`'s value for `attr`, with `element`.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let `verifiedValue` be the result of calling verify attribute value
`attr`'s value for `attr`, with `element`.
Let `verifiedValue` be the result of verifying attribute value given
`attr`'s value, `attr`, and `element`.

However, this doesn't match the signature above. What gives?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated this and the signature to hopefully be clearer. I think it does match the signature it was just written rather poorly. I've changed the signature to a more convetional one and updated the call sites accordingly.

dom.bs Outdated Show resolved Hide resolved

aarongable pushed a commit to chromium/chromium that referenced this pull request

Mar 12, 2025

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 Outdated Show resolved Hide resolved

lukewarlow added a commit to web-platform-tests/wpt that referenced this pull request

Mar 19, 2025

…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

lukewarlow added a commit to web-platform-tests/wpt that referenced this pull request

Mar 19, 2025

…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 Outdated Show resolved Hide resolved


Let `verifiedValue` be the result of
verifying an attribute value given `value`,
`attribute`, and `element`.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, why do we need this algorithm to verify the value? I'd expect the relevant webidl attribute setters to be modified, similarly to set HTMLScriptElement.src.

In fact, HTMLIFrameElement.srcdoc setter already does TT check to get a reasonable string and this would do it again?

setAttributeNS should do TT check explicitly?

Or am I missing something.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yeah you're right this is shared by property reflection...

So yeah this will need moving out to the method algorithm. I think in this case it's probably different enough to just duplicate it a bit.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be addressed now. I essentially reverted that algorithm and copied the updated version of it to setAttributeNS. I think that's probably the cleanest approach?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@@ -6946,6 +6948,14 @@ steps:
value.


To verify attribute value given a {{TrustedType}} or string `value`, an

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To verify attribute value given a {{TrustedType}} or string `value`, an
To verify an attribute value given a {{TrustedType}} or string `value`, an

Although why do we even have this algorithm as it directly calls into the Trusted Types algorithm.

Comment on lines +7314 to +7316

If `attribute` is null, then set `attribute` to an attribute
whose local name is `qualifiedName`, value is
`value`, and node document is this's node document.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.