[Trusted Types] Implement script src attribute without StringContext. [chromium/src : main]
Daniel Vogelheim (Gerrit)
Daniel Vogelheim added 1 comment![]( "Open in Gerrit")
Hi Joey, please take a look. I adopted the naming suggestions from our email discussion.
Related details
- Joey Arhar
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar added 3 comments![]( "Open in Gerrit")
This CL should not introduce any behavioural changes. If the updatedSo with this patch, you can now pass a TrustedScriptURL instead of a USVString to the changed IDL methods. That's not observable?
}So it seems like we will start running trusted types checks on this setter without any runtime flag. Is trusted types already enabled by default? Is this change guaranteed to not have any issues? Or does TrustedTypesCheckForScriptURL not do anything different when its passed a USVString?
[CEReactions, RaisesException=Setter, URL] attribute (TrustedScriptURL or USVString) src;Is this the IDL that is agreed on in a spec PR or discussion? If so, want to link to it in the commit description? Is the difference observable from script?
Also, based on the commit message I expected this to remove A StringContext= IDL attribute, but it looks like there isn't one here.
Related details
- Daniel Vogelheim
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Vogelheim (Gerrit)
Daniel Vogelheim added 1 comment![]( "Open in Gerrit")
This CL should not introduce any behavioural changes. If the updatedSo with this patch, you can now pass a TrustedScriptURL instead of a USVString to the changed IDL methods. That's not observable?
No, not all. There must be a misunderstanding here somewhere...
Trusted Types has been default enabled since 2020: https://chromium-review.googlesource.com/c/chromium/src/+/2095548
In any recent Chrome/Chromium, go to about:blank, and in the DevTools console, do this:
```
const script = document.createElement("script");
const str = "https://blabla";
const trusted_script_url = trustedTypes.createPolicy("bla", {createScriptURL: s => s}).createScriptURL(str);
typeof str; // 'string'
typeof trusted_script_url; // 'object'
trusted_script_url; // TrustedScriptURL 'https://blabla'
script.src = str; // passes.
script.src = trusted_script_url; // passes.
// enable TT
document.head.setHTMLUnsafe(`<meta http-equiv="Content-Security-Policy" content="require-trusted-types-for 'script'">`);
script.src = str; // throws.
script.src = trusted_script_url; // passes.
```
Note how current Chrome will accept both TrustedScriptURL and a string, and will behave quite differently between those once TT is enabled.
Here is a WPT test that checks this behaviour (also for HTMLScriptElement src assignment), under various conditions: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/trusted-types/HTMLElement-generic.html;l=28-72 This test continues to pass.
The code that implements this is generated by the bindings, based on the [StringContext] IDL extended attribute. You can inspect it here: https://source.chromium.org/chromium/chromium/src/+/main:out/win-Debug/gen/third_party/blink/renderer/bindings/core/v8/v8_html_script_element.cc;l=103-131?q=bindings%20HTMLScriptElement%20src%20callback&ss=chromium%2Fchromium%2Fsrc
Note the type that gets passed to the NativeValueTraits, here: https://source.chromium.org/chromium/chromium/src/+/main:out/win-Debug/gen/third_party/blink/renderer/bindings/core/v8/v8_html_script_element.cc;l=123;drc=3a5d99f59bc052bf3c578a74ef8d20a79b60ed14
This, through one more indirection, calls this code: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/bindings/core/v8/native_value_traits_impl.h;l=570-593;drc=a32603b1d241bc57afade4ee4c7c5e320a722c5b
Note the Trusted Types check, here: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/bindings/core/v8/native_value_traits_impl.h;l=589-591;drc=a32603b1d241bc57afade4ee4c7c5e320a722c5b
What my CL does: It implements the exact same check, in the exact same sequence, but as first thing in the receiving code in the DOM, rather than in the Bindings just before DOM is called. Note that it *removes* the [StringContext] annotation. I guess one could say that this, more or less, "inlines" the check into hand-written code, rather than relying on the IDL compiler.
I need to do this, because 1) the HTML working group didn't want to standardize [StringContext], and 2) in some cases -- but not here -- the HTML working group has decided that other checks get run first, and that will be observable. In order to implement the latter thing, I need to have the check in "regular" code first, because I can't do that with the bindings compiler.
I indeed very much expect this CL (and all others in the same sequence) to be unobservable.
Related details
- Joey Arhar
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar added 2 comments![]( "Open in Gerrit")
Thanks for the explanation!
Note that it removes the [StringContext] annotation.
I still don't see "StringContext" anywhere in the diff
So it seems like we will start running trusted types checks on this setter without any runtime flag. Is trusted types already enabled by default? Is this change guaranteed to not have any issues? Or does TrustedTypesCheckForScriptURL not do anything different when its passed a USVString?
- Daniel Vogelheim
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Vogelheim (Gerrit)
Daniel Vogelheim added 1 comment![]( "Open in Gerrit")
My bad, it's a typedef: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/trustedtypes/trusted_script_url.idl;l=7
The current type is `ScriptURLString` (for `src`, `data`, `codeBase`) which is a typedef for `[StringContext=TrustedScriptURL] USVString`, and `ScriptString` (for `text`) which is a typedef for `[StringContext=TrustedScript] DOMString`.
I should have said, e.g. for HTMLScriptElement.src, that it replaces `ScriptURLString`, which is a typedef for `[StringContext=TrustedScriptURL] USVString`, with `(TrustedScriptURL or USVString)`. `StringContext` indeed does not literally occur in the diff. However, the [StringContext] IDL extended attribute is currently applied to those attributes; and is no longer after this change. The same check is now run explicitly. The before and after types should accept the exact same set of JS objects, and exhibit the same behaviour.
Definition ScriptURLString:
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/trustedtypes/trusted_script_url.idl;l=7
Definition ScriptString:
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/trustedtypes/trusted_script.idl;l=7
Related details
- Joey Arhar
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar voted and added 2 comments![]( "Open in Gerrit")
Votes added by Joey Arhar
| Code-Review | +1 |
2 comments
This CL should not introduce any behavioural changes. If the updatedAh I see now StringContext is used now, I didn't know that typedefs existed like this in idl. thanks!
[CEReactions, RaisesException=Setter, URL] attribute (TrustedScriptURL or USVString) src;Is this the IDL that is agreed on in a spec PR or discussion? If so, want to link to it in the commit description? Is the difference observable from script?
Also, based on the commit message I expected this to remove A StringContext= IDL attribute, but it looks like there isn't one here.
I still think it would be nice to link to some of the spec discussion, but I don't insist if it is also linked somewhere in the attached bug.
Related details
- Daniel Vogelheim
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Vogelheim (Gerrit)
Daniel Vogelheim added 1 comment![]( "Open in Gerrit")
[CEReactions, RaisesException=Setter, URL] attribute (TrustedScriptURL or USVString) src;Joey ArharIs this the IDL that is agreed on in a spec PR or discussion? If so, want to link to it in the commit description? Is the difference observable from script?
Also, based on the commit message I expected this to remove A StringContext= IDL attribute, but it looks like there isn't one here.
I still think it would be nice to link to some of the spec discussion, but I don't insist if it is also linked somewhere in the attached bug.
Yeah, that's a good callout. Should have done this wright away... :)
I've added:
- https://g-issues.chromium.org/issues/330516530#comment14
- https://g-issues.chromium.org/issues/330516530#comment15
These should explain in good detail what I'm trying to do here, including links to spec (etc.).
Related details
- Joey Arhar
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Vogelheim (Gerrit)
Daniel Vogelheim added 1 comment![]( "Open in Gerrit")
[CEReactions, RaisesException=Setter, URL] attribute (TrustedScriptURL or USVString) src;Joey ArharIs this the IDL that is agreed on in a spec PR or discussion? If so, want to link to it in the commit description? Is the difference observable from script?
Also, based on the commit message I expected this to remove A StringContext= IDL attribute, but it looks like there isn't one here.
Daniel VogelheimI still think it would be nice to link to some of the spec discussion, but I don't insist if it is also linked somewhere in the attached bug.
Yeah, that's a good callout. Should have done this wright away... :)
I've added:
- https://g-issues.chromium.org/issues/330516530#comment14
- https://g-issues.chromium.org/issues/330516530#comment15
These should explain in good detail what I'm trying to do here, including links to spec (etc.).
Done
Related details
- Joey Arhar
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Vogelheim (Gerrit)
Daniel Vogelheim voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Joey Arhar
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Unreviewed changes
9 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
Change information
[Trusted Types] Implement script src attribute without StringContext.
The [StringContext=] extendend IDL attribute is a Chrome-specific
mechanism to implement Trusted Types. TT is currently being integrated
into the HTML spec, and the spec editors decided against the
StringContext IDL attribute, and instead specify Trusted Types checks
"manually" in the spec. In some instances -- but not here -- the
difference between the IDL-based and the "manual" TT checks is observable, due to the when the TT check is being made.
This CL re-implements the Trusted Types check of the following properties, without StringContext:
- HTMLScriptElement, src + text
- HTMLObjectElement, data + codeBase
- HTMLEmbedElement, src
This CL should not introduce any behavioural changes. If the updated
HTML specs introduces changes relative to Chrome's current behaviour,
we will implement those semantic changes in a separate CL, behind a
flag. Here, we only change the mechanism.
- M third_party/blink/renderer/core/html/html_embed_element.cc
- M third_party/blink/renderer/core/html/html_embed_element.h
- M third_party/blink/renderer/core/html/html_embed_element.idl
- M third_party/blink/renderer/core/html/html_object_element.cc
- M third_party/blink/renderer/core/html/html_object_element.h
- M third_party/blink/renderer/core/html/html_object_element.idl
- M third_party/blink/renderer/core/html/html_script_element.cc
- M third_party/blink/renderer/core/html/html_script_element.h
- M third_party/blink/renderer/core/html/html_script_element.idl
- M third_party/blink/renderer/core/speculation_rules/speculation_rule_set_test.cc
Code-Review: +1 by Joey Arhar
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |