Draft integration with Trusted Types, take 2. #1247

koto · 2024-01-23T16:50:09Z

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 PR #809.

At least two implementers are interested (and none opposed):
- Gecko
- Chromium (already implemented)
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
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

annevk

I see it's still a draft, but I thought I'd post some initial feedback.

annevk · 2024-01-24T16:38:51Z

dom.bs

@@ -6354,6 +6355,10 @@ given a <var>document</var>, <var>localName</var>, <var>namespace</var>, and opt
 <a>attribute</a> <var>attribute</var> to <var>value</var>, run these steps:

 <ol>
+<li><p>Set <var>value</var> to the result of calling <a>Get Trusted Types-compliant attribute


Nit: indentation is wrong. And DOM doesn't do newlines in phrasing-level elements (i.e., wrap before <a>).

Question: this ordering means that if value is changed by TT the mutation record reflects that. Does that have test coverage?

Fixed the formatting, thanks for pointers. You're correct re: MutationObserver.

@mbrodesser-Igalia, we need to add a MutationObserver test (https://jsfiddle.net/014ze36t/2/) to WPT.

dom.bs

annevk · 2024-01-24T16:54:17Z

dom.bs

+
+ <li><p><a>Handle attribute changes</a> for <var>attribute</var> with <var>element</var>, null, and
+ <var>attribute</var>'s <a for=Attr>value</a>.
+ <li><p>Return.


I don't understand why the changes here (which are for setAttributeNS() and prolly some other callers) are much more elaborate than they are for setAttribute().

Why can't "append an attribute" handle this in the way it already does per the changes above?

Append might be too late, at that point Attr value (a string) is set, so the checks bubbled up to the calling algorithms. Check the current version, I think I missed one callsite in the one you reviewed. Now it should be OK, with the intentional omission of clone.

#1247 (comment)

dom.bs

annevk · 2024-01-25T12:49:11Z

dom.bs

+ <ol>
+  <li><p>Set <var>attribute</var> to a new <a>attribute</a> whose <a for=Attr>namespace</a> is
+  <var>namespace</var>, <a for=Attr>namespace prefix</a> is <var>prefix</var>,
+  <a for=Attr>local name</a> is <var>localName</var> and <a for=Node>node document</a> is
+  <var>element</var>'s <a for=Node>node document</a>.
+
+  <li><p><a>Validate and set attribute value</a> <var>value</var> for <var>attribute</var> with
+  <var>element</var>.
+
+  <li><p><a lt="append an attribute">Append</a> <var>attribute</var> to <var>element</var>.
+
+  <li><p>Return.
+ </ol>


This seems wrong. This should be behind the "If attribute is null" check above, presumably?

However, I'm also not sure why this cannot be part of "append an attribute" which would give us some deduplication. If you do the validation before the attribute is appended to the element, it seems fine? What am I missing?

In particular because in these cases we end up creating a new attribute and so if something throws, it would simply be as if that attribute was not created, right?

And it seems that the current algorithms miss the toggleAttribute() endpoint.

If we edit append an attribute we might end up with duplicated calls to validate.

For example in the screenshot below you'll see that we need to validate before step 5, but then step 6 which appends would also call validate?

Having said that we can reorder these steps so that step 4 is inside of 5's if so it becomes 5.1 and 5.2 is to replace?

And then we can append attr and handle validation inside of append?

(I've also just spotted it refers to a newAttr which doesn't exist in this algorithm I'll make sure to fix that)

Is toggleAttribute() actually an issue? Don't you still need to go through one of the other algorithms to actually set a value?

Having said that it seems to trigger an error in Chrome when triggering "on" so might aswell cover in the spec.

Addressed in #1268

mbrodesser-Igalia · 2024-01-31T09:37:44Z

Is it possible to get the preview linked at #1247 (comment) fixed? Would simplify reading the diff.

smaug---- · 2024-02-09T10:46:52Z

dom.bs

+    <var>element</var>'s <a for=Node>node document</a>.
+
+    <li><p><a>Validate and set attribute value</a> <var>value</var> for <var>attribute</var> with
+    <var>element</var>.


This should probably clarify that validation may throw an exception. What should happen in that case?

smaug---- · 2024-02-09T10:51:30Z

dom.bs

+    <li><p><a>Validate and set attribute value</a> <var>value</var> for <var>attribute</var> with
+    <var>element</var>.
+
+    <li><p><a lt="append an attribute">Append</a> <var>attribute</var> to <var>element</var>.


This is problematic. Validation may have run scripts, and scripts may have already added another attribute with same name. That can't be allowed.

Addressed in #1268 by rechecking the attribute state and throwing an exception if the default policy has done something funky.

smaug---- · 2024-02-15T09:54:50Z

dom.bs

+   <li><p><a>Validate and set attribute value</a> <var>value</var> for <var>attribute</var>,
+   with <a>this</a>.
+
+   <li><p><a lt="append an attribute">Append</a> <var>attribute</var> to <a>this</a>.


This has also the problem that since validation may run scripts, the attribute list may now already have attribute with the same name. And validation may throw an exception.

(but yeah, in general these checks do need to happen very early when we're about to set an attribute)

mbrodesser-Igalia · 2024-02-26T09:21:57Z

@koto can the PR preview please be fixed? The changes themselves are sufficiently complicated, so let's make our life easier by simplifying reviewing.

dom.bs

lukewarlow · 2024-02-26T15:42:37Z

I might be missing something but this doesn't seem to account for event handler attributes? I know there's separate handling for them in TT but wont this change block them being set? Or do they go down a different codepath?

See w3c/trusted-types#418 and whatwg#789. Supercedes PR whatwg#809.

koto · 2024-02-26T16:09:03Z

@koto can the PR preview please be fixed? The changes themselves are sufficiently complicated, so let's make our life easier by simplifying reviewing.

This is done, I'll proceed with resolving all the comments.

lukewarlow · 2024-03-13T13:28:48Z

@koto what remaining steps need to be taken on this?

https://bugs.webkit.org/show_bug.cgi?id=270436. Reviewed by NOBODY (OOPS!). Implement the spec updates at whatwg/dom#1247. It also remove the some expectations in GTK as the results should be in line with the general expectation file. * LayoutTests/platform/gtk/TestExpectations: * Source/WebCore/dom/Element.cpp: (WebCore::getTrustedTypesCompliantAttributeValue): (WebCore::Element::setAttribute): (WebCore::Element::setElementsArrayAttribute): (WebCore::appendAttributes): (WebCore::Element::setAttributeNS): * Source/WebCore/dom/Element.h: * Source/WebCore/dom/Element.idl: * Source/WebCore/dom/TrustedType.cpp: (WebCore::stringToTrustedType): (WebCore::getAttributeTrustedType): * Source/WebCore/dom/TrustedType.h: * Source/WebCore/dom/TrustedTypePolicyFactory.cpp: (WebCore::TrustedTypePolicyFactory::getAttributeType const):

https://bugs.webkit.org/show_bug.cgi?id=270436 Reviewed by NOBODY (OOPS!). Implement the spec updates at whatwg/dom#1247 It also removes some expectations in GTK as the results should be in line with the general expectation file. * LayoutTests/TestExpectations: * LayoutTests/imported/w3c/web-platform-tests/trusted-types/Element-setAttribute-respects-Elements-node-documents-globals-CSP-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/trusted-types/GlobalEventHandlers-onclick-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/trusted-types/TrustedTypePolicyFactory-metadata.tentative-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/trusted-types/block-string-assignment-to-Element-setAttribute-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/trusted-types/block-string-assignment-to-Element-setAttributeNS-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/trusted-types/trusted-types-event-handlers-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/trusted-types/trusted-types-event-handlers.html: * LayoutTests/imported/w3c/web-platform-tests/trusted-types/trusted-types-svg-script-set-href-expected.txt: * LayoutTests/platform/gtk/TestExpectations: * Source/WebCore/dom/Element.cpp: (WebCore::trustedTypesCompliantAttributeValue): (WebCore::Element::validateAttributeIndex const): (WebCore::Element::toggleAttribute): (WebCore::Element::setAttribute): (WebCore::Element::setElementsArrayAttribute): (WebCore::appendAttributes): (WebCore::Element::setAttributeNode): (WebCore::Element::setAttributeNodeNS): (WebCore::Element::setAttributeNS): * Source/WebCore/dom/Element.h: * Source/WebCore/dom/Element.idl: * Source/WebCore/dom/TrustedScript.h: * Source/WebCore/dom/TrustedScriptURL.h: (WebCore::TrustedScriptURL::toString const): Deleted. (WebCore::TrustedScriptURL::toJSON const): Deleted. * Source/WebCore/dom/TrustedType.cpp: (WebCore::stringToTrustedType): (WebCore::trustedTypeForAttribute): * Source/WebCore/dom/TrustedType.h: * Source/WebCore/dom/TrustedTypePolicyFactory.cpp: (WebCore::TrustedTypePolicyFactory::getAttributeType const): * Source/WebKit/WebProcess/InjectedBundle/API/mac/WKDOMElement.mm: (-[WKDOMElement setAttribute:value:]): * Source/WebKitLegacy/mac/DOM/DOMElement.mm: (-[DOMElement setAttribute:value:]): (-[DOMElement setAttributeNS:qualifiedName:value:]):

https://bugs.webkit.org/show_bug.cgi?id=270436 Reviewed by Ryosuke Niwa. Implement the spec updates at whatwg/dom#1247 It also removes some expectations in GTK as the results should be in line with the general expectation file. * LayoutTests/TestExpectations: * LayoutTests/imported/w3c/web-platform-tests/trusted-types/Element-setAttribute-respects-Elements-node-documents-globals-CSP-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/trusted-types/GlobalEventHandlers-onclick-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/trusted-types/TrustedTypePolicyFactory-metadata.tentative-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/trusted-types/block-string-assignment-to-Element-setAttribute-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/trusted-types/block-string-assignment-to-Element-setAttributeNS-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/trusted-types/trusted-types-event-handlers-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/trusted-types/trusted-types-event-handlers.html: * LayoutTests/imported/w3c/web-platform-tests/trusted-types/trusted-types-svg-script-set-href-expected.txt: * LayoutTests/platform/gtk/TestExpectations: * Source/WebCore/dom/Element.cpp: (WebCore::trustedTypesCompliantAttributeValue): (WebCore::Element::validateAttributeIndex const): (WebCore::Element::toggleAttribute): (WebCore::Element::setAttribute): (WebCore::Element::setElementsArrayAttribute): (WebCore::appendAttributes): (WebCore::Element::setAttributeNode): (WebCore::Element::setAttributeNodeNS): (WebCore::Element::setAttributeNS): * Source/WebCore/dom/Element.h: * Source/WebCore/dom/Element.idl: * Source/WebCore/dom/TrustedScript.h: * Source/WebCore/dom/TrustedScriptURL.h: (WebCore::TrustedScriptURL::toString const): Deleted. (WebCore::TrustedScriptURL::toJSON const): Deleted. * Source/WebCore/dom/TrustedType.cpp: (WebCore::stringToTrustedType): (WebCore::trustedTypeForAttribute): * Source/WebCore/dom/TrustedType.h: * Source/WebCore/dom/TrustedTypePolicyFactory.cpp: (WebCore::TrustedTypePolicyFactory::getAttributeType const): * Source/WebKit/WebProcess/InjectedBundle/API/mac/WKDOMElement.mm: (-[WKDOMElement setAttribute:value:]): * Source/WebKitLegacy/mac/DOM/DOMElement.mm: (-[DOMElement setAttribute:value:]): (-[DOMElement setAttributeNS:qualifiedName:value:]): Canonical link: https://commits.webkit.org/278817@main

koto mentioned this pull request Jan 23, 2024

Add attribute validate steps. #809

Closed

annevk reviewed Jan 24, 2024

View reviewed changes

annevk reviewed Jan 25, 2024

View reviewed changes

smaug---- reviewed Feb 9, 2024

View reviewed changes

koto mentioned this pull request Feb 12, 2024

Should the "default" policy be called when setAttributeNode is called with a node from the same realm? w3c/trusted-types#434

Closed

smaug---- reviewed Feb 15, 2024

View reviewed changes

lukewarlow mentioned this pull request Feb 22, 2024

Integration with trusted types inside of node conversion #1258

Merged

5 tasks

lukewarlow reviewed Feb 26, 2024

View reviewed changes

dom.bs Outdated Show resolved Hide resolved

koto added 5 commits February 26, 2024 17:00

Draft integration with Trusted Types, take 2.

5bcc0af

See w3c/trusted-types#418 and whatwg#789. Supercedes PR whatwg#809.

Added integration in 'set an attribute value'.

dba4438

Removed stringification.

417460c

Fixed formatting.

acb802a

Fixed copy-paste error in setAttribute() and indentation.

2495cb4

koto force-pushed the tt-integration branch from 455d8e7 to 2495cb4 Compare February 26, 2024 16:04

Fix autolinking to get TT-compliant attribute value.

10ce041

lukewarlow mentioned this pull request Mar 14, 2024

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

Open

mbrodesser-Igalia mentioned this pull request Mar 27, 2024

Add test for importing a srcdoc attribute node from a non-TT realm to a TT iframe element throws web-platform-tests/wpt#44323

Merged

ziransun mentioned this pull request Mar 27, 2024

Implement trusted types integrations with DOM attribute APIs. WebKit/WebKit#26519

Merged

lukewarlow mentioned this pull request Mar 27, 2024

Trusted types attributes #1268

Open

5 tasks

lukewarlow closed this Mar 27, 2024

ziransun mentioned this pull request Apr 18, 2024

Use "event handler content attribute" while getting Trusted Type data for attribute. WebKit/WebKit#27444

Closed

lukewarlow mentioned this pull request May 15, 2024

Implement trusted types integrations with DOM attribute APIs. WebKit/WebKit#28590

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Draft integration with Trusted Types, take 2. #1247

Draft integration with Trusted Types, take 2. #1247

koto commented Jan 23, 2024 •

edited by pr-preview bot

Loading

annevk left a comment

annevk Jan 24, 2024

annevk Jan 24, 2024

koto Jan 25, 2024

annevk Jan 24, 2024

koto Jan 25, 2024

annevk Jan 25, 2024

annevk Jan 25, 2024

annevk Jan 25, 2024

annevk Jan 25, 2024

lukewarlow Apr 10, 2024

lukewarlow Apr 10, 2024

lukewarlow Apr 10, 2024 •

edited

Loading

lukewarlow Apr 11, 2024

mbrodesser-Igalia commented Jan 31, 2024

smaug---- Feb 9, 2024

smaug---- Feb 9, 2024

lukewarlow Apr 11, 2024

smaug---- Feb 15, 2024

mbrodesser-Igalia commented Feb 26, 2024

lukewarlow commented Feb 26, 2024

koto commented Feb 26, 2024

lukewarlow commented Mar 13, 2024

Draft integration with Trusted Types, take 2. #1247

Draft integration with Trusted Types, take 2. #1247

Conversation

koto commented Jan 23, 2024 • edited by pr-preview bot Loading

annevk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukewarlow Apr 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbrodesser-Igalia commented Jan 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbrodesser-Igalia commented Feb 26, 2024

lukewarlow commented Feb 26, 2024

koto commented Feb 26, 2024

lukewarlow commented Mar 13, 2024

koto commented Jan 23, 2024 •

edited by pr-preview bot

Loading

lukewarlow Apr 10, 2024 •

edited

Loading