Issue with script enforcement

[lukewarlow]

Apologies if I'm missing something but I believe that the spec as currently written blocks any inline script elements from executing.

https://w3c.github.io/trusted-types/dist/spec/#enforcement-in-scripts

A new [[ScriptText]] slot is added and is initially null, this is only every set via the IDL OR when running "prepare the script text" algorithm.

The problem is that the "prepare the script text" algorithm will always throw unless a default policy exists. This means that the test suite for example fails to run.

Chromium I believe has an additional step that parsed script elements set their [[ScriptText]] slot when parsing has finished, this needs speccing.

[mbrodesser-Igalia]

Apologies if I'm missing something but I believe that the spec as currently written blocks any inline script elements from executing.

https://w3c.github.io/trusted-types/dist/spec/#enforcement-in-scripts

A new [[ScriptText]] slot is added and is initially null, this is only every set via the IDL OR when running "prepare the script text" algorithm.

That section also mentions "Equivalent to script’s child text content. Initially null.". Perhaps "Equivalent to script’s child text content" should be changed to a non-normative note.

The problem is that the "prepare the script text" algorithm will always throw unless a default policy exists. This means that the test suite for example fails to run.

Chromium I believe has an additional step that parsed script elements set their [[ScriptText]] slot when parsing has finished, this needs speccing.

[koto]

@otherdaniel do you remember how the parser sets the slot value? What algorithm is it a part of?

[lukewarlow]

I also don't think the slot can be initially null either.

data:text/html,<meta http-equiv="Content-Security-Policy" content="require-trusted-types-for 'script'; trusted-types default;"><div id="container"></div><script>const s = document.createElement('script'); container.appendChild(s);</script

Else something like this would trigger a trusted types violation as it's slot is null and it's child text content is an empty string?

[lukewarlow]

So I did some digging and in Chromium it's done in the FinishParsingChildren function

I think that corresponds to somewhere inside of https://html.spec.whatwg.org/multipage/parsing.html#create-an-element-for-the-token but it's possible there's a higher level algorithm to hook into.

Either way the Chrome patch is here: https://chromium.googlesource.com/chromium/src/+/78a020504fe5bd400ca5ea8d903c38ecfc5821aa%5E%21/

Something that is especially key to gather in the spec is Chromium's children_changed_by_api_ flag which was implemented here: https://chromium.googlesource.com/chromium/src/+/cae50d5dbb6c35b33c5feef7f3c07c721ffd65db%5E%21/

void HTMLScriptElement::FinishParsingChildren() {
  Element::FinishParsingChildren();

  // We normally expect the parser to finish parsing before any script gets
  // a chance to manipulate the script. However, if script parsing gets
  // deferrred (or similar; see crbug.com/1033101) then a script might get
  // access to the HTMLScriptElement before. In this case, we cannot blindly
  // accept the current TextFromChildren as a parser result.
  DCHECK(children_changed_by_api_ || !script_text_internal_slot_.length());
  if (!children_changed_by_api_)
    script_text_internal_slot_ = ParkableString(TextFromChildren().Impl());
}

[lukewarlow]

Interestingly the spec is also specifically focused on HTMLScriptElement but the chrome patch also touches SVGScriptElement so some clarity there would be good too.

[otherdaniel]

@otherdaniel do you remember how the parser sets the slot value? What algorithm is it a part of?

I'm not sure. If I remember correctly, I initially followed the proposed spec, and then deviated as we found bugs. In particular, we found out that the "parser inserted" flag didn't mean what we thought it meant, and then came up with alternatives. I now see that we never fed that back in the spec proposal. Apologies for the mess.

Something that is especially key to gather in the spec is Chromium's children_changed_by_api_ flag which was implemented here:

IMHO, the best way might be to define a new flag that is true only if the script content is the actual script content that the parser saw. That is, instead of retro-spec-ing our work-around, have a clean definition of "parser inserted, but really", and then use that. I'll gladly update our implementation to match whatever we come up with.

[lukewarlow]

While implementing this in WebKit I'm getting an error where replaceWith is causing a trusted types error that doesn't happen in chromium and that also seems to have other unspecced changes that lead to it. See #438

Okay so I realised that my behaviour is largely matching Chrome's aside from Chrome throwing when not using a trusted type which is due to the above mentioned IDL changes being missing.

[mbrodesser-Igalia]

I also don't think the slot can be initially null either.

data:text/html,<meta http-equiv="Content-Security-Policy" content="require-trusted-types-for 'script'; trusted-types default;"><div id="container"></div><script>const s = document.createElement('script'); container.appendChild(s);</script

Else something like this would trigger a trusted types violation as it's slot is null and it's child text content is an empty string?

Per which part of the spec would that trigger a TT violation?

[lukewarlow]

Per which part of the spec would that trigger a TT violation?

When it's running prepare script text it will compare the inner slot value to the child text contents. Child text contents is always a string so it'd be comparing null to a string, which would then run through the process of trying to put that through the default policy which might not exist (and shouldn't need to for the above to work)

[mbrodesser-Igalia]

Per which part of the spec would that trigger a TT violation?

When it's running prepare script text it will compare the inner slot value to the child text contents. Child text contents is always a string so it'd be comparing null to a string, which would then run through the process of trying to put that through the default policy which might not exist (and shouldn't need to for the above to work)

So that's https://w3c.github.io/trusted-types/dist/spec/#prepare-script-text.

I also don't think the slot can be initially null either.

To overcommunicate: for external scripts (e.g. <script src=...>), the [[ScriptText]] slot should possibly be null, initially.

[mbrodesser-Igalia]

Per which part of the spec would that trigger a TT violation?

When it's running prepare script text it will compare the inner slot value to the child text contents. Child text contents is always a string so it'd be comparing null to a string, which would then run through the process of trying to put that through the default policy which might not exist (and shouldn't need to for the above to work)

So that's https://w3c.github.io/trusted-types/dist/spec/#prepare-script-text.

I also don't think the slot can be initially null either.

To overcommunicate: for external scripts (e.g. <script src=...>), the [[ScriptText]] slot should possibly be null, initially.

Oh, the child text content indeed always is a string (https://dom.spec.whatwg.org/#concept-child-text-content). The empty one for above case.

[lukewarlow]

I've made a PR that attempts to add the node manipulation APIs to the spec: #440

The algorithm is slightly unwieldily and I feel like it can probably be simplified so any feedback is very welcome.

[lukewarlow]

Related issue: #252

[lukewarlow]

Closing this as the parser now at least sets the "script text" value and the null vs empty string issue has been resolved. #525 covers discrepencies between implementation and spec.