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

Change Script Enforcement Mechanism to use flags #533

Merged
merged 5 commits into from
Oct 28, 2024
Merged

Change Script Enforcement Mechanism to use flags #533

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ac68dd7
Change Script Enforcement Mechanism to use flags
lukewarlow Jun 12, 2024
f1edbe5
Correct ordering
lukewarlow Jul 4, 2024
6c48e04
Change to CSV for attribute
lukewarlow Jul 4, 2024
cf99945
Remove parserChange bool
lukewarlow Jul 10, 2024
856b814
Add note about initially true value for is trusted boolean
lukewarlow Jul 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 37 additions & 41 deletions spec/index.bs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,8 +127,6 @@ spec:ECMA-262; urlPrefix: https://tc39.github.io/ecma262/
type:dfn; text:current realm record; url: current-realm
spec: HTML; urlPrefix: https://html.spec.whatwg.org/
type: dfn; text: prepare the script element; url: prepare-the-script-element
type: dfn; text: The text insertion mode; url: parsing-main-incdata
type: dfn; text: reentrant invocation of the parser; url: nestedParsing
type: dfn; text: get the text steps; url: get-the-text-steps
type: dfn; text: set the inner text steps; url: set-the-inner-text-steps
type: dfn; text: src; url: attr-script-src
Expand Down Expand Up @@ -1073,20 +1071,6 @@ Given a {{TrustedType}} type (|expectedType|), a [=realm/global object=] (|globa
1. Return a new instance of an interface with a type
name |trustedTypeName|, with its associated data value set to |dataString|.

## <dfn abstract-op>Prepare the script text</dfn> ## {#prepare-script-text}

Given an {{HTMLScriptElement}} (|script|), this algorithm performs the following steps:

1. If |script|'s [=script text=] value is not equal to its [=child text content=],
set |script|'s [=script text=] to the result of executing [$Get Trusted Type compliant string$], with the following arguments:
* {{TrustedScriptURL}} as |expectedType|,
* |script|'s {{Document}}'s [=relevant global object=] as |global|,
* |script|'s [=child text content=] attribute value,
* `HTMLScriptElement text` as |sink|,
* `'script'` as |sinkGroup|.

If the algorithm threw an error, rethrow the error.

## Get Trusted Types-compliant attribute value ## {#validate-attribute-mutation}
To <dfn abstract-op export>get Trusted Types-compliant attribute value</dfn> on {{Attr}} |attribute| with {{Element}} |element| and {{TrustedType}} or a string |newValue|, perform the following steps:

Expand Down Expand Up @@ -1179,10 +1163,19 @@ partial interface HTMLScriptElement {

This document modifies {{HTMLScriptElement}}s. Each script has:

: an associated string <dfn export for="HTMLScriptElement">script text</dfn>.
:: A string, containing the body of the script to execute that was set
through a compliant sink. Equivalent to script's
[=child text content=]. Initially an empty string.
: an associated boolean <dfn export for="HTMLScriptElement">is trusted</dfn>.
lukewarlow marked this conversation as resolved.
Show resolved Hide resolved
:: A boolean indicating whether a script element is considered trustworthy for execution.
Initially true.
lukewarlow marked this conversation as resolved.
Show resolved Hide resolved

: an associated boolean <dfn export for="HTMLScriptElement">changed by trusted sink</dfn>.
:: A boolean indicating whether a script element has been modified by a trusted sink.
Initially false.

This document also modifies {{SVGScriptElement}}s. Each script has:

: an associated boolean <dfn export for="SVGScriptElement">is trusted</dfn>.
:: A boolean indicating whether a script element is considered trustworthy for execution.
Initially true.

#### The {{HTMLScriptElement/innerText}} IDL attribute #### {#the-innerText-idl-attribute}

Expand All @@ -1191,7 +1184,7 @@ The {{HTMLScriptElement/innerText}} setter steps are:
1. Let |value| be the result of calling [$Get Trusted Type compliant string$] with
{{TrustedScript}}, [=this=]'s [=relevant global object=], the given value, `HTMLScriptElement innerText`, and
`script`.
1. Set [=this=]'s [=script text=] value to |value|.
1. Set [=this=]'s [=HTMLScriptElement/changed by trusted sink=] to true.
1. Run [=set the inner text steps=] with [=this=] and |value|.

The {{HTMLScriptElement/innerText}} getter steps are:
Expand All @@ -1206,8 +1199,8 @@ empty string instead, and then do as described below:
1. Let |value| be the result of calling [$Get Trusted Type compliant string$] with
{{TrustedScript}}, [=this=]'s [=relevant global object=], the given value, `HTMLScriptElement textContent`, and
`script`.
1. Set [=this=]'s [=script text=] value to |value|.
1. Run [=set text content=] with [=this=] and |value|.
1. Set [=this=]'s [=HTMLScriptElement/changed by trusted sink=] to true.
lukewarlow marked this conversation as resolved.
Show resolved Hide resolved

The {{HTMLScriptElement/textContent}} getter steps are:

Expand All @@ -1220,8 +1213,8 @@ Update the {{HTMLScriptElement/text}} setter steps algorithm as follows.
1. <ins>Let |value| be the result of calling [$Get Trusted Type compliant string$] with
{{TrustedScript}}, [=this=]'s [=relevant global object=], the given value, `HTMLScriptElement text`, and
`script`.</ins>
1. <ins>Set [=this=]'s [=script text=] value to the given value.</ins>
1. [=String replace all=] with the given value within [=this=].
1. Set [=this=]'s [=HTMLScriptElement/changed by trusted sink=] to true.


#### The {{HTMLScriptElement/src}} IDL attribute #### {#the-src-idl-attribute}
Expand All @@ -1233,29 +1226,21 @@ The {{HTMLScriptElement/src}} setter steps are:
`script`.</ins>
1. <ins>Set [=this=]'s [=src=] content attribute to |value|.</ins>

#### Setting slot values from parser #### {#setting-slot-values-from-parser}
#### Script children changed steps #### {#script-children-changed-steps}

This document modifies the HTML parser to set the [=script text=] value when the script is created.
This document modifies the [=children changed steps=] for {{HTMLScriptElement}} as follows:

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

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@lukewarlow lukewarlow Jul 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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


<dl class="switch">
<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.
Copy link
Member Author

Choose a reason for hiding this comment

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

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


<ins><p>Set <var>script</var>'s [=script text=] value to its [=child text content=].</p></ins>
1. Set [=this=]'s [=HTMLScriptElement/changed by trusted sink=] to false.

<p>If the <span>active speculative HTML parser</span> is null, then <span>prepare the script
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.
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

@domfarolino should probably look at this.

Also would that create issues with re-entrant invocations?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.


<p>...</p>
</dd>
</dl>
This document modifies the [=children changed steps=] for {{SVGScriptElement}} as follows:

Issue: The above algorithm doesn't account for the case when the script element's content is changed mid-parse. Implementors should ensure they protect against this case. See [https://github.com/w3c/trusted-types/issues/507](https://github.com/w3c/trusted-types/issues/507).
1. If <var ignore=''>parserChange</var> is false, set [=this=]'s [=SVGScriptElement/is trusted=] to false.

#### Slot value verification #### {#slot-value-verification}

Expand All @@ -1276,11 +1261,22 @@ The first few steps of the [=prepare the script element=] algorithm are modified
<p class=note>This is done so that if a parser-inserted <code id=script-processing-model:the-script-element-28><a href=https://html.spec.whatwg.org/#the-script-element>script</a></code> element fails to
run when the parser tries to run it, but it is later executed after a script dynamically
updates it, it will execute in an async fashion even if the <code id=script-processing-model:attr-script-async-5><a href=https://html.spec.whatwg.org/#attr-script-async>async</a></code> attribute isn't set.</p>
<li><ins><p>Execute the [$Prepare the script text$] algorithm on <var>el</var>. If that algorithm threw an error, then return.</p></ins></li>
<li><p>Let <var ignore="">source text</var> be <var>el</var>'s <del><a id=script-processing-model:child-text-content href=https://dom.spec.whatwg.org/#concept-child-text-content data-x-internal=child-text-content>child text content</a>.</del> <ins>[=script text=] value.</ins>

<li><p>Let <var>source text</var> be <var>el</var>'s <a id=script-processing-model:child-text-content href=https://dom.spec.whatwg.org/#concept-child-text-content data-x-internal=child-text-content>child text content</a>.

<li><ins>
<p>If <var>el</var>'s [=HTMLScriptElement/is trusted=] is false:
<ol>
<li><p>Set <var>source text</var> to the result of executing [$Get Trusted Type compliant string$], with
{{TrustedScript}}, <var>el</var>'s [=relevant global object=], <var>source text</var>, `'HTMLScriptElement text'`,
and `'script'`.
<p>If that algorithm threw an error, then return.
</ol></ins>
<li>...
</ol>

Issue: There's no proper definition for the processing of SVG script elements. However, you should apply a similar change to the processing of {{SVGScriptElement}}s.

## Integration with DOM ## {#integration-with-dom}

Note: See [https://github.com/whatwg/dom/pull/1258](https://github.com/whatwg/dom/pull/1258) and [https://github.com/whatwg/dom/pull/1268](https://github.com/whatwg/dom/pull/1268) which upstream this integration.
Expand Down
Loading