Update dynamic code compilation support #445
Conversation
| @@ -1728,15 +1728,15 @@ The Trusted Types portion of this algorithm uses |calleeRealm| and its CSP setti | |||
| </pre> | |||
| </div> | |||
|
|
|||
| Given a [[ECMA-262#realm|realm]] (|calleeRealm|), a string | |||
| (|source|) <ins>, a boolean |wasCodeLike| and a string |compilationSink|</ins>, this algorithm returns <del>normally</del><ins>the | |||
| Given a [[ECMA-262#realm|realm]] (|calleeRealm|), a list of strings (|parameterStrings|), a string (|bodyString|), <ins> a string (|source|), an enum (|compilationType|), and a boolean |wasCodeLike|</ins>, this algorithm returns <del>normally</del><ins>the | |||
There was a problem hiding this comment.
We potentially could pass bodyString and parameterStrings as extra params to the default policy?
This could give policy authors a nicer format of data for them to work with, without needing to first parse the function string?
This wouldn't help them with manipulating it back into a stringified function though?
I feel like we could make the ergonomics of this better for the Function constructor though.
There was a problem hiding this comment.
Having said that they can just do /\(function anonymous\((.*,+.*)\\n\) {\\n(.*)\\n}\)/ to capture the csv of params and the body string process them and then recreate the string to return?
There was a problem hiding this comment.
I kinda like the ergonomics of separating the function arguments. There is still a problem as the default policy can't tell which one(s) of the arguments/body wasn't a TT initially, but it might be a bit too involved to check all of them separately in a callback (and we'd probably need a TrustedFunctionArgument at least).
As is, the PR doesn't use the passed arguments. I'm happy to merge, but this is aligned with https://specs.lukewarlow.dev/dynamic-code-brand-checks/; do you know how likely is that shape to be merged to ECMAScript? Not separating arguments is an easier integration for sure.
cc @otherdaniel too.
There was a problem hiding this comment.
So the separate args for body and parameter are already merged, this would include the fully compiled string.
The separate body and parameter are needed for other host functionality such as extensions to CSP (hashing).
Ideally we wouldn't need to additionally pass the fully compiled string but the default policy needs it to be web compatible. So we'd either need to separately compile it ourselves or pass it through.
I think it could be useful to pass through the separate values to the default policy additionally (this would be something that's different from Chrome's current implementation).
To clarify do you agree we should also pass them through as arguments to the default policy @koto I can update this PR to do so quite easily
There was a problem hiding this comment.
As for how likely that proposal is to get into ECMAScript idk. The web compatability impacting change (reording of stringifier calls) has already occured outside of this proposal so that's one contentious bit down.
As far as I see it the HostDefinedCodeLike slot is the thing most likely to stop it. But that's an academic discussion as we'd be using a slot in web land even if not in ecma land...
There was a problem hiding this comment.
@koto before merging this it would be good to get the proposal update PR merged too that way they're both inline with each other
There was a problem hiding this comment.
OK let's do it. Since for now the PR doesn't change the default policy behavior, there's no compatibility risk. I'd like @otherdaniel to comment on the Function constructor + the changes to the default policy arguments, but we have https://issues.chromium.org/issues/40133092, so there's probably no compatibility risk whatsoever.
There was a problem hiding this comment.
I thought so too but that's just that TrustedScript isn't "trusted" the strings still go through the default policy which is the compat risk (if we change arguments). Having said that if everything is considered unsafe it's probably not being used very heavily, not sure if we have use counters.
lukewarlow
force-pushed
the
dynamic-code-changes
branch
from
8a51220 to
79dfc9b
Compare
SHA: d32bb50 Reason: push, by koto Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Changes to go along with tc39/proposal-dynamic-code-brand-checks#10
Preview | Diff