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

Update dynamic code compilation support #445

Merged
merged 1 commit into from
Feb 26, 2024
Merged

Update dynamic code compilation support #445

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 2 additions & 2 deletions spec/index.bs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@lukewarlow lukewarlow Feb 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

#458 follow up issue

source string to compile</ins> if compilation is allowed, and
throws an "`EvalError`" if not:

1. <ins>If |wasCodeLike| is true, let |sourceToValidate| be a new instance of
the {{TrustedScript}} interface, with its `[[Data]]` internal slot value
set to |source|. Otherwise, let |sourceToValidate| be |source|.</ins>

1. <ins>Let |compilationSink| be `"Function"` if |compilationType| is `*FUNCTION*`, otherwise `"Eval"`.</ins>
1. <ins>Let |sourceString| be the result of executing the
[$Get Trusted Type compliant string$] algorithm, with:
* |calleeRealm| as |global|,
Expand Down
Loading