Change Script Enforcement Mechanism to use flags #533

lukewarlow · 2024-07-04T14:39:06Z

Also add SVGScriptElement to spec

spec/index.bs

lukewarlow · 2024-07-04T14:44:49Z

spec/index.bs


-Modify the [=The text insertion mode=] algorithm as follows:
+1.  If <var ignore=''>parserChange</var> is false, set [=this=]'s [=HTMLScriptElement/is trusted=] to false.


This parserChange is a placeholder for what we end up speccing in whatwg/dom#1288

Which issues, besides the one mentioned in #533 (comment), is this PR intended to fix?

#525, and #507 I think should both be closable once this PR is finished alongside the SVG specific one.

When "parserChange" is false and changed by trusted sink is true, couldn't still malicious code have been injected? E.g. if a trusted sink called only someScript.innerText = someScript.innerText that'd make the untrusted code trusted.

E.g. if a trusted sink called only someScript.innerText = someScript.innerText that'd make the untrusted code trusted.

That would only work if a default policy had sanctioned that value. Else the assignment would fail before the "changed by trusted sink" Boolean is set

lukewarlow · 2024-07-04T14:46:38Z

spec/index.bs

-    <dt id="scriptEndTag">An end tag whose tag name is "script"</dt>
-    <dd>
-        <p>...</p>
+1.  If [=this=]'s [=HTMLScriptElement/changed by trusted sink=] is true, set [=this=]'s [=HTMLScriptElement/is trusted=] to true.


This flag is used to say hey this is an API change but it's a trusted one. We unset the flag once used.

lukewarlow · 2024-07-04T14:47:40Z

spec/index.bs

-        element</span> <var>script</var>. This might cause some script to execute, which might cause
-        <span data-x="dom-document-write">new characters to be inserted into the tokenizer</span>, and
-        might cause the tokenizer to output more tokens, resulting in a [=reentrant invocation of the parser=].</p>
+Issue: Need to double check how [part of script element's spec](https://html.spec.whatwg.org/#prepare-the-script-element:~:text=When%20a%20script%20element%20el%20that%20is%20not%20parser%2Dinserted%20experiences) fits into this. These steps need to happen before prepare the script is called.


I think we need to change the html spec when upstreaming to run the prepare the script (under relevant conditions) at the end of the children changed steps.

Interestingly it would rely on having a bit more information in the children changed steps algorithm if we want to inline it. Because it needs to know what type of change it is (insertion specifically in this case).

I suspect this is why some Chrome and WebKit's childrenChanged functions include more than the dom spec's algorithm. (And is why Firefox implements it in a way that also gives them this more granular informaion).

@domfarolino should probably look at this.

Also would that create issues with re-entrant invocations?

It would seem that whatwg/html#10188 already changes that part of the HTML spec to be defined in terms of the children changed steps so I think we'd just need to put our new steps first and then run the post-insertion steps and it'll fix the concerns I had here.

Also add SVGScriptElement to spec

spec/index.bs

annevk · 2024-07-04T15:22:30Z

spec/index.bs

-        element</span> <var>script</var>. This might cause some script to execute, which might cause
-        <span data-x="dom-document-write">new characters to be inserted into the tokenizer</span>, and
-        might cause the tokenizer to output more tokens, resulting in a [=reentrant invocation of the parser=].</p>
+Issue: Need to double check how [part of script element's spec](https://html.spec.whatwg.org/#prepare-the-script-element:~:text=When%20a%20script%20element%20el%20that%20is%20not%20parser%2Dinserted%20experiences) fits into this. These steps need to happen before prepare the script is called.


@domfarolino should probably look at this.

Also would that create issues with re-entrant invocations?

spec/index.bs

lukewarlow · 2024-07-09T12:11:38Z

@annevk one question I've got with this boolean approach. Does something like .innerText =a/nb cause multiple children changed steps invocations? Wondering if the <br> element generation might somehow mess things up?

Edit: Turns out the bug I found is a pre-existing interop bug

lukewarlow · 2024-07-10T13:16:48Z

If children changed steps are never called by the parser then I think I can just remove the new boolean and this PR is basically ready to go. I was under the impression they would be called by the parser but I don't think they are.

@domfarolino do you know off the top of your head if that's true? I know you've used them for the post connection steps work youve been doing

https://bugs.webkit.org/show_bug.cgi?id=276426 Reviewed by NOBODY (OOPS!). Scripts now have two boolean flags rather than storing a duplicate of their contents. Spec PR: w3c/trusted-types#533 * LayoutTests/imported/w3c/web-platform-tests/trusted-types/HTMLScriptElement-innerText-expected.txt: Added. * LayoutTests/imported/w3c/web-platform-tests/trusted-types/HTMLScriptElement-innerText.html: Added. * LayoutTests/imported/w3c/web-platform-tests/trusted-types/HTMLScriptElement-midparse-appendChild-expected.txt: Added. * LayoutTests/imported/w3c/web-platform-tests/trusted-types/HTMLScriptElement-midparse-appendChild.html: Added. * LayoutTests/imported/w3c/web-platform-tests/trusted-types/HTMLScriptElement-midparse-innerHTML-expected.txt: Added. * LayoutTests/imported/w3c/web-platform-tests/trusted-types/HTMLScriptElement-midparse-innerHTML.html: Added. * LayoutTests/imported/w3c/web-platform-tests/trusted-types/HTMLScriptElement-midparse-nodeValue-expected.txt: Added. * LayoutTests/imported/w3c/web-platform-tests/trusted-types/HTMLScriptElement-midparse-nodeValue.html: Added. * LayoutTests/imported/w3c/web-platform-tests/trusted-types/HTMLScriptElement-src-expected.txt: Renamed from LayoutTests/imported/w3c/web-platform-tests/trusted-types/HTMLScriptElement-internal-slot-expected.txt. * LayoutTests/imported/w3c/web-platform-tests/trusted-types/HTMLScriptElement-src.html: Renamed from LayoutTests/imported/w3c/web-platform-tests/trusted-types/HTMLScriptElement-internal-slot.html. * LayoutTests/imported/w3c/web-platform-tests/trusted-types/HTMLScriptElement-text-expected.txt: Added. * LayoutTests/imported/w3c/web-platform-tests/trusted-types/HTMLScriptElement-text.html: Added. * LayoutTests/imported/w3c/web-platform-tests/trusted-types/HTMLScriptElement-textContent-expected.txt: Added. * LayoutTests/imported/w3c/web-platform-tests/trusted-types/HTMLScriptElement-textContent.html: Added. * LayoutTests/imported/w3c/web-platform-tests/trusted-types/block-Node-multiple-arguments-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/trusted-types/block-text-node-insertion-into-script-element-expected.txt: * Source/WebCore/dom/CharacterData.cpp: (WebCore::canUseSetDataOptimization): * Source/WebCore/dom/ScriptElement.cpp: (WebCore::ScriptElement::childrenChanged): (WebCore::ScriptElement::prepareScript): (WebCore::ScriptElement::finishParsingChildren): Deleted. (WebCore::ScriptElement::setTrustedScriptText): Deleted. * Source/WebCore/dom/ScriptElement.h: (WebCore::ScriptElement::setChangedByTrustedSink): * Source/WebCore/html/HTMLScriptElement.cpp: (WebCore::HTMLScriptElement::setTextContent): (WebCore::HTMLScriptElement::setInnerText): (WebCore::HTMLScriptElement::finishParsingChildren): Deleted. * Source/WebCore/html/HTMLScriptElement.h: * Source/WebCore/svg/SVGScriptElement.cpp: (WebCore::SVGScriptElement::finishParsingChildren): Deleted. * Source/WebCore/svg/SVGScriptElement.h:

lukewarlow · 2024-07-10T13:34:43Z

I've updated this PR to remove the parserChange boolean because it appears it's not needed. Spec wise the children changed steps seem to never actually get called by the parser. So parserChange is always false (aka it's always an API).

fred-wang

@lukewarlow I guess you are still waiting for a review? I checked the PR and it looks good to me, but I really think someone more familiar with the spec/code should review.

SHA: a71f29c Reason: push, by koto Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

lukewarlow mentioned this pull request Jul 4, 2024

Add script protection mechanisms to SVGScriptElement #524

Closed

lukewarlow commented Jul 4, 2024

View reviewed changes

spec/index.bs Show resolved Hide resolved

lukewarlow commented Jul 4, 2024

View reviewed changes

lukewarlow force-pushed the script-protection-v2 branch from 0c7d33e to f22111f Compare July 4, 2024 14:45

lukewarlow commented Jul 4, 2024

View reviewed changes

Change Script Enforcement Mechanism to use flags

ac68dd7

Also add SVGScriptElement to spec

lukewarlow force-pushed the script-protection-v2 branch from f22111f to ac68dd7 Compare July 4, 2024 14:56

lukewarlow requested a review from annevk July 4, 2024 15:06

annevk reviewed Jul 4, 2024

View reviewed changes

lukewarlow added 2 commits July 4, 2024 16:27

Correct ordering

f1edbe5

Change to CSV for attribute

6c48e04

lukewarlow mentioned this pull request Jul 9, 2024

Request for mechanism to determine if children changed steps are called as a result of script or parser whatwg/dom#1288

Open

lukewarlow marked this pull request as ready for review July 9, 2024 11:36

lukewarlow mentioned this pull request Jul 9, 2024

Should all 3 script IDL setters change the associated script text value identically #517

Open

lukewarlow mentioned this pull request Jul 10, 2024

Re-implement Trusted Types script enforcement using booleans WebKit/WebKit#30649

Draft

lukewarlow added 2 commits July 10, 2024 14:28

Remove parserChange bool

cf99945

Add note about initially true value for is trusted boolean

856b814

lukewarlow requested a review from koto July 10, 2024 13:34

This was referenced Jul 22, 2024

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

Closed

Script element mid-parse protection mechanism #525

Open

fred-wang reviewed Oct 24, 2024

View reviewed changes

koto approved these changes Oct 28, 2024

View reviewed changes

koto merged commit a71f29c into w3c:main Oct 28, 2024
3 checks passed

github-actions bot added a commit that referenced this pull request Oct 28, 2024

Change Script Enforcement Mechanism to use flags (#533)

83e9d78

SHA: a71f29c Reason: push, by koto Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change Script Enforcement Mechanism to use flags #533

Change Script Enforcement Mechanism to use flags #533

lukewarlow commented Jul 4, 2024 •

edited by pr-preview bot

Loading

lukewarlow Jul 4, 2024

mbrodesser-Igalia Jul 8, 2024

lukewarlow Jul 8, 2024

mbrodesser-Igalia Jul 9, 2024

lukewarlow Jul 9, 2024 •

edited

Loading

lukewarlow Jul 4, 2024

lukewarlow Jul 4, 2024

lukewarlow Jul 4, 2024

annevk Jul 4, 2024

lukewarlow Jul 9, 2024

annevk Jul 4, 2024

lukewarlow commented Jul 9, 2024 •

edited

Loading

lukewarlow commented Jul 10, 2024

lukewarlow commented Jul 10, 2024

fred-wang left a comment


		Modify the [=The text insertion mode=] algorithm as follows:
		1. If <var ignore=''>parserChange</var> is false, set [=this=]'s [=HTMLScriptElement/is trusted=] to false.

Change Script Enforcement Mechanism to use flags #533

Change Script Enforcement Mechanism to use flags #533

Conversation

lukewarlow commented Jul 4, 2024 • edited by pr-preview bot Loading

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 Jul 9, 2024 • edited Loading

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 commented Jul 9, 2024 • edited Loading

lukewarlow commented Jul 10, 2024

lukewarlow commented Jul 10, 2024

fred-wang left a comment

Choose a reason for hiding this comment

lukewarlow commented Jul 4, 2024 •

edited by pr-preview bot

Loading

lukewarlow Jul 9, 2024 •

edited

Loading

lukewarlow commented Jul 9, 2024 •

edited

Loading