Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft integration with Trusted Types, take 2. #1247

Closed
wants to merge 6 commits into from

Conversation

koto
Copy link

@koto koto commented Jan 23, 2024

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.

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


Preview | Diff

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

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

dom.bs Outdated
@@ -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
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

<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.
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

dom.bs Outdated Show resolved Hide resolved
dom.bs Outdated Show resolved Hide resolved
dom.bs Outdated Show resolved Hide resolved
dom.bs Outdated
Comment on lines 6518 to 6530
<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>
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Screenshot 2024-04-10 at 16 12 14

Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Member

@lukewarlow lukewarlow Apr 10, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Addressed in #1268

@mbrodesser-Igalia
Copy link
Member

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

<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>.
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

<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>.
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

<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>.
Copy link
Collaborator

Choose a reason for hiding this comment

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

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
Copy link
Member

@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 Outdated Show resolved Hide resolved
@lukewarlow
Copy link
Member

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?

@koto
Copy link
Author

koto commented Feb 26, 2024

@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
Copy link
Member

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

ziransun added a commit to ziransun/WebKit that referenced this pull request Apr 18, 2024
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):
lukewarlow pushed a commit to lukewarlow/WebKit that referenced this pull request May 15, 2024
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:]):
lukewarlow pushed a commit to lukewarlow/WebKit that referenced this pull request May 15, 2024
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:]):
webkit-commit-queue pushed a commit to ziransun/WebKit that referenced this pull request May 15, 2024
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants