Trusted types attributes

[lukewarlow]

This calls the get Trusted Types-compliant attribute value algorithm from Trusted Types (w3c/trusted-types#418) from attribute's change, append, and replace.

Changed the signature of setAttribute and setAttributeNS to accept Trusted Types as values. The underlying Attr node's values continue to be DOMString, so moving nodes across elements or adding standalone attributes to elements can cause TT violations. This matches WPT tests and the Chromium's implementation.

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: …
• The top of this comment includes a clear commit message to use.

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

---

Preview | Diff

[lukewarlow]

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

[lukewarlow]

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

[otherdaniel]

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.

[lukewarlow]

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.

[lukewarlow]

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]

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.

[lukewarlow]

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]

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

[lukewarlow]

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

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

[lukewarlow]

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]

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]

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

[fred-wang]

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

[otherdaniel]

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:

<div onclick="1+1;" id="test"></div>
<script>
test(t => {
  var attribute = document.getElementById("test").getAttributeNode("id");
  assert_throws_dom("InUseAttributeError", _ => {
    document.body.setAttributeNode(attribute);
  }, "Expect InUseAttributeError DOMException.");

  attribute = document.getElementById("test").getAttributeNode("onclick");
  assert_throws_js(TypeError, _ => {
    document.body.setAttributeNode(attribute);
  }, "Expect a Trusted-Types style TypeError, not InUseAttributeError DOMException.");
}, "Element.setAttribute competing exceptions");
</script>

[fred-wang]

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]

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:

1. Let verifiedValue be value.
2. If verify is true: [... TT check; assign to verifiedValue ...]
3. If attr’s element is neither null nor element, throw an "InUseAttributeError" DOMException
4. [... 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]

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

[lukewarlow]

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

[smaug----]

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]

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.

[annevk]

Suggested change

	<li><p>Let <var>verifiedValue</var> be the result of calling <a>verify attribute value</a>
	<var>attr</var>'s <a for=Attr>value</a> for <var>attr</var>, with <var>element</var>.
	<li><p>Let <var>verifiedValue</var> be the result of <a>verifying attribute value</a> given
	<var>attr</var>'s <a for=Attr>value</a>, <var>attr</var>, and <var>element</var>.

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

[lukewarlow]

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.

[smaug----]

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.

[lukewarlow]

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.

[lukewarlow]

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?