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.

Sign up for GitHub

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

Jump to bottom

Does the DOM spec really need to special case script elements for replaceChildren etc. #537

Closed
lukewarlow opened this issue Jul 19, 2024 · 5 comments · Fixed by #562
Closed

Does the DOM spec really need to special case script elements for replaceChildren etc. #537

lukewarlow opened this issue Jul 19, 2024 · 5 comments · Fixed by #562
Labels
spec
Milestone
v1

Comments

@lukewarlow
Copy link
Member

lukewarlow commented Jul 19, 2024 •
edited
Loading

Currently the DOM spec, and both WebKit and Chromium have special casing in DOM APIs such as node.replaceChildren for script elements. Such that if you try and replace the children with a string or a Text node it would cause a Trusted Types violation.

This was specced in DOM to match the chromium implementation, but was never originally specified in the TT spec.

It's just struck me that this code might be entirely pointless?

Even if you use Trusted types for these APIs they won't set the script elements internal slot to actually allow execution. You can also trivially "bypass" them by calling appendChild(new Text(''))?

So my question is, why are these implemented in Chromium? Is there something I'm missing or can we drop them from the spec and implementations?

cc @koto @otherdaniel @mbrodesser-Igalia
@lukewarlow lukewarlow added this to the v1 milestone Jul 19, 2024
@mbrodesser-Igalia
Copy link
Collaborator

Even if you use Trusted types for these APIs they won't set the script elements internal slot to actually allow execution. You can also trivially "bypass" them by calling appendChild(new Text(''))?

Presumably https://w3c.github.io/trusted-types/dist/spec/#htmlscriptelement-script-text is meant here.

@lukewarlow
Copy link
Member Author

Presumably https://w3c.github.io/trusted-types/dist/spec/#htmlscriptelement-script-text is meant here.

Yeah that's it. Fwiw even the new approach in #533 doesn't change things here.

@lukewarlow
Copy link
Member Author

After a bit of investigation it seems this is indeed a left over of a previous model of script enforcement.

https://chromium-review.googlesource.com/c/chromium/src/+/1547746 - July 14th 2019 introduced a similar change to another DOM API (this is no longer in situ)

https://chromium-review.googlesource.com/c/chromium/src/+/1688030 - July 18th 2019 introduced the DOM API handling being discussed.

https://chromium-review.googlesource.com/c/chromium/src/+/1924523 - Dec 2019 introduced current script enforcement model, reverting most of the script checks but forgot the specific ones being discussed.

@lukewarlow lukewarlow mentioned this issue Jul 22, 2024
Merged
5 tasks
lukewarlow added a commit to lukewarlow/WebKit that referenced this issue Jul 22, 2024
@lukewarlow
Remove trusted types enforcement from DOM node APIs
4aae90b 
https://bugs.webkit.org/show_bug.cgi?id=276881

Reviewed by NOBODY (OOPS!).

The enforcement of Trusted Types in various node manipulation APIs in DOM
was based on a previous model for protecting script elements.

This patch removes it as its not needed.

See whatwg/dom#1299 and w3c/trusted-types#537

* LayoutTests/imported/w3c/web-platform-tests/trusted-types/Node-multiple-arguments-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/trusted-types/Node-multiple-arguments-tt-enforced-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/trusted-types/Node-multiple-arguments-tt-enforced.html: Renamed from LayoutTests/imported/w3c/web-platform-tests/trusted-types/block-Node-multiple-arguments.html.
* Source/WebCore/dom/ChildNode.idl:
* Source/WebCore/dom/ContainerNode.cpp:
(WebCore::ContainerNode::append):
(WebCore::ContainerNode::prepend):
(WebCore::ContainerNode::replaceChildren):
* Source/WebCore/dom/ContainerNode.h:
* Source/WebCore/dom/Node.cpp:
(WebCore::nodeSetPreTransformedFromNodeOrStringVector):
(WebCore::Node::convertNodesOrStringsIntoNode):
(WebCore::Node::convertNodesOrStringsIntoNodeVector):
(WebCore::Node::before):
(WebCore::Node::after):
(WebCore::Node::replaceWith):
(WebCore::nodeSetPreTransformedFromNodeOrStringOrTrustedScriptVector): Deleted.
(WebCore::Node::convertNodesOrStringsOrTrustedScriptsIntoNode): Deleted.
(WebCore::Node::convertNodesOrStringsOrTrustedScriptsIntoNodeVector): Deleted.
* Source/WebCore/dom/Node.h:
* Source/WebCore/dom/ParentNode.idl:
* Source/WebCore/dom/TrustedType.cpp:
(WebCore::processNodeOrStringAsTrustedType): Deleted.
* Source/WebCore/dom/TrustedType.h:
@lukewarlow lukewarlow mentioned this issue Jul 22, 2024
Merged
webkit-commit-queue pushed a commit to lukewarlow/WebKit that referenced this issue Jul 24, 2024
@lukewarlow
Remove trusted types enforcement from DOM node APIs
10507bc 
https://bugs.webkit.org/show_bug.cgi?id=276881

Reviewed by Ryosuke Niwa.

The enforcement of Trusted Types in various node manipulation APIs in DOM
was based on a previous model for protecting script elements.

This patch removes it as its not needed.

See whatwg/dom#1299 and w3c/trusted-types#537

* LayoutTests/imported/w3c/web-platform-tests/trusted-types/Node-multiple-arguments-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/trusted-types/Node-multiple-arguments-tt-enforced-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/trusted-types/Node-multiple-arguments-tt-enforced.html: Renamed from LayoutTests/imported/w3c/web-platform-tests/trusted-types/block-Node-multiple-arguments.html.
* Source/WebCore/dom/ChildNode.idl:
* Source/WebCore/dom/ContainerNode.cpp:
(WebCore::ContainerNode::append):
(WebCore::ContainerNode::prepend):
(WebCore::ContainerNode::replaceChildren):
* Source/WebCore/dom/ContainerNode.h:
* Source/WebCore/dom/Node.cpp:
(WebCore::nodeSetPreTransformedFromNodeOrStringVector):
(WebCore::Node::convertNodesOrStringsIntoNode):
(WebCore::Node::convertNodesOrStringsIntoNodeVector):
(WebCore::Node::before):
(WebCore::Node::after):
(WebCore::Node::replaceWith):
(WebCore::nodeSetPreTransformedFromNodeOrStringOrTrustedScriptVector): Deleted.
(WebCore::Node::convertNodesOrStringsOrTrustedScriptsIntoNode): Deleted.
(WebCore::Node::convertNodesOrStringsOrTrustedScriptsIntoNodeVector): Deleted.
* Source/WebCore/dom/Node.h:
* Source/WebCore/dom/ParentNode.idl:
* Source/WebCore/dom/TrustedType.cpp:
(WebCore::processNodeOrStringAsTrustedType): Deleted.
* Source/WebCore/dom/TrustedType.h:

Canonical link: https://commits.webkit.org/281285@main
@charliewolfe charliewolfe mentioned this issue Jul 29, 2024
Merged
annevk pushed a commit to whatwg/dom that referenced this issue Aug 13, 2024
@lukewarlow
Revert 'Trusted Types integration for "converting nodes into a node"'
809bfa2 
This reverts commit 720a4fc.

These changes aren't needed given the current protection model used for script elements. See w3c/trusted-types#537 for context.
@fred-wang
Copy link
Contributor

@lukewarlow So did you get an answser to your question? I see you reverted your PR, but it is referenced on https://w3c.github.io/trusted-types/dist/spec/#integration-with-dom (will send a PR to remove the link).

@lukewarlow
Copy link
Member Author

Ah yeah this issue can be closed once the link to it is dropped from the spec.

fred-wang added a commit to fred-wang/trusted-types that referenced this issue Nov 6, 2024
@fred-wang
Remove link to whatwg/dom#1258
f2f903a 
This was merged and reverted in
whatwg/dom@809bfa2

closes w3c#537
@fred-wang fred-wang mentioned this issue Nov 6, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
spec
Projects
None yet
Milestone
v1
Development

Successfully merging a pull request may close this issue.

Remove link to https://github.com/whatwg/dom/pull/1258 fred-wang/trusted-types
3 participants
@fred-wang @lukewarlow @mbrodesser-Igalia