Skip to content

Normative: Allow CodeLike object evaluation#3294

Open
lukewarlow wants to merge 2 commits intotc39:mainfrom
lukewarlow:codelike-hostensurecancompilestrings
Open

Normative: Allow CodeLike object evaluation#3294
lukewarlow wants to merge 2 commits intotc39:mainfrom
lukewarlow:codelike-hostensurecancompilestrings

Conversation

@lukewarlow
Copy link
Copy Markdown

@lukewarlow lukewarlow commented Mar 7, 2024

This change introduces a new HostGetCodeForEval host hook, this is used to allow evaluating certain objects.

This change also provides the compilation type and the original arguments, to the host hook HostEnsureCanCompileStrings.

This PR upstreams a reduced version of the https://github.com/tc39/proposal-dynamic-code-brand-checks proposal

Closes #1498, #938

@ljharb

This comment was marked as outdated.

@ljharb ljharb added normative change Affects behavior required to correctly evaluate some ECMAScript source text needs consensus This needs committee consensus before it can be eligible to be merged. needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 pending stage 4 This proposal has not yet achieved stage 4, but may otherwise be ready to merge. proposal This is related to a specific proposal, and will be closed/merged when the proposal reaches stage 4. labels Mar 7, 2024
Comment thread spec.html
@caridy
Copy link
Copy Markdown
Contributor

caridy commented Mar 7, 2024

probably a little premature to do this at stage 1 :-)

@ljharb I think the reasoning behind this PR is that it doesn't really change any functionality, it is just layering. The original proposal from @mikesamuel was a lot bigger, this is just a small piece of that and we were hoping to just ask for consensus rather than pushing this thru the staging process. @cc @nicolo-ribaudo

@ljharb
Copy link
Copy Markdown
Member

ljharb commented Mar 7, 2024

@caridy if it's normative, it's not just layering - but sure, if the goal is to try to turn it into a needs consensus PR, then this is fine, but my recollection is that that approach was rejected in favor of a proposal, I'm not sure how it'll turn out.

@lukewarlow
Copy link
Copy Markdown
Author

@ljharb my understanding was that this would just be layering in the same way #3222 was.

@ljharb
Copy link
Copy Markdown
Member

ljharb commented Mar 8, 2024

You might be right :-) would this PR obviate the entire linked proposal?

@lukewarlow
Copy link
Copy Markdown
Author

lukewarlow commented Mar 8, 2024

Yeah with this PR the use case for that proposal, trusted types protection of eval and Function would be entirely addressed. That proposal would subsequently be withdrawn. The changes have been reduced over time and that linked PR addressed some of the required changes anyway.

@michaelficarra
Copy link
Copy Markdown
Member

FYI @lukewarlow @nicolo-ribaudo this wouldn't be appropriate for stage 3 before testing and integration work has been completed. Maybe you should change the agenda item asking for stage 3 to stage 2.7?

Comment thread spec.html Outdated
@michaelficarra
Copy link
Copy Markdown
Member

I don't really like the design here. Instead of adding a Boolean slot, could we add a String slot? The thing I don't like about having a Boolean slot and calling ToString on it is that there's no guarantee that it produces the same String each time. There's also no guarantee that ToString completes normally. If the String can't be precomputed and guaranteed constant, we could also use the slot to memoise it: ~not-computed~ or a String.

If it doesn't limit what you're trying to do with this proposal, I would much prefer a design like that.

Comment thread spec.html Outdated
Comment thread spec.html Outdated
Comment thread spec.html Outdated
@nicolo-ribaudo
Copy link
Copy Markdown
Member

nicolo-ribaudo commented Apr 6, 2024

@michaelficarra This is currently tested in WPT. I asked to @ptomato how to test this in test262 but, given that the only observable change is that there might be objects that are not returned as-is by eval() but we don't actually say which objects they are (or we don't request that they exist), he suggested that this isn't really testable in test262 and that it's better to have it in WPT.

The test that shows that "there exist an object that is not returned as-is by eval" is at https://github.com/web-platform-tests/wpt/blob/23894a7e427bc088cfd4b3aa6ba8a8927b904713/trusted-types/eval-csp-no-tt.html#L13-L15. We could try somehow moving it to test262 if you prefer, maybe by requesting runners to provide an optional $262.getObjectForEval(toStringResult).

There is also a test about how it interacts with new Function (at https://github.com/web-platform-tests/wpt/blob/master/trusted-types/eval-function-constructor.html), but given that whether a call to new Function is allowed or not is entirely host-defined I don't think it can be moved to test262 in any way.


Regarding the host integration, it's specified at https://w3c.github.io/trusted-types/dist/spec/#host-ensure-can-compile-strings and https://w3c.github.io/trusted-types/dist/spec/#csp-eval.

@nicolo-ribaudo
Copy link
Copy Markdown
Member

nicolo-ribaudo commented Apr 6, 2024

I don't really like the design here. Instead of adding a Boolean slot, could we add a String slot? The thing I don't like about having a Boolean slot and calling ToString on it is that there's no guarantee that it produces the same String each time. There's also no guarantee that ToString completes normally. If the String can't be precomputed and guaranteed constant, we could also use the slot to memoise it: ~not-computed~ or a String.

If it doesn't limit what you're trying to do with this proposal, I would much prefer a design like that.

I recommended using toString() to convert objects to string in eval() because new Function already uses toString() to convert objects to strings. When doing new Function(someObject) twice, there is already no guarantee that it will generate two functions with the same code.

When it comes to Trusted Types, the host is checking not only that the new Function/eval is being called with an object that has the capability of being converted into code, but also that the object gives the permission to convert the specific string obtained through ecma262 into code (step 2.4.1 of https://w3c.github.io/trusted-types/dist/spec/#csp-eval)

@nicolo-ribaudo
Copy link
Copy Markdown
Member

@syg I was reading tc39/proposal-source-phase-imports#59 — should we replace the internal slot on the host-provided object with a HostEvalShouldStringifyObject hook for the same reason?

Comment thread spec.html Outdated
Comment thread spec.html Outdated
Comment thread spec.html Outdated
Comment thread spec.html Outdated
@lukewarlow lukewarlow force-pushed the codelike-hostensurecancompilestrings branch from df8ab1f to f248964 Compare April 15, 2024 14:34
Comment thread spec.html Outdated
Comment thread spec.html Outdated
Comment thread spec.html Outdated
@lukewarlow lukewarlow force-pushed the codelike-hostensurecancompilestrings branch from 769995d to 63dbfae Compare April 15, 2024 19:13
@lukewarlow lukewarlow requested a review from jmdyck April 15, 2024 19:13
@nicolo-ribaudo
Copy link
Copy Markdown
Member

@bakkot I was re-reading the new Function spec -- you mentioned that the sourceText created in CreateDynamicFunction is not exposed. However, my reading is that step 4 of OrdinaryFunctionCreate stores it in F.[[SourceText]], and then Function.prototype.toString will return it.

@bakkot
Copy link
Copy Markdown
Member

bakkot commented Apr 17, 2024

I don't remember saying such a thing, but either way, yes, that's correct.

@michaelficarra
Copy link
Copy Markdown
Member

@nicolo-ribaudo You may be referring to what I said, and I think I may have confused dynamic functions with built-in functions. While the exact whitespace/comments/etc in the String returned by Function.prototype.toString for built-ins is intentionally underspecified, the String returned for dynamic functions is indeed fully specified. If you want, it seems okay to pass that to the host AO. Sorry for the mistake.

@syg
Copy link
Copy Markdown
Contributor

syg commented Apr 17, 2024

@syg I was reading tc39/proposal-source-phase-imports#59 — should we replace the internal slot on the host-provided object with a HostEvalShouldStringifyObject hook for the same reason?

Is this a comment referencing an earlier version of the PR? I don't see an internal slot currently.

@lukewarlow
Copy link
Copy Markdown
Author

@syg yes there was previously a slot but we've changed it to a host hook (which itself may or may not check a slot)

@lukewarlow
Copy link
Copy Markdown
Author

If you want, it seems okay to pass that to the host AO. Sorry for the mistake.

@michaelficarra is that something we can do without needing to represent that change? Passing the built code string through would indeed be the best course from our side (CSP is in fact already specced that way).

@michaelficarra
Copy link
Copy Markdown
Member

@lukewarlow Any normative change at stage 2.7 or later should be presented to the committee, but I don't think this one should require consensus. It can be just a quick update.

@lukewarlow lukewarlow marked this pull request as ready for review May 7, 2024 11:26
fred-wang added a commit to fred-wang/proposal-dynamic-code-brand-checks that referenced this pull request Nov 27, 2024
fred-wang added a commit to fred-wang/proposal-dynamic-code-brand-checks that referenced this pull request Nov 27, 2024
fred-wang added a commit to fred-wang/proposal-dynamic-code-brand-checks that referenced this pull request Nov 27, 2024
fred-wang added a commit to fred-wang/proposal-dynamic-code-brand-checks that referenced this pull request Nov 27, 2024
fred-wang added a commit to fred-wang/proposal-dynamic-code-brand-checks that referenced this pull request Nov 27, 2024
@lukewarlow lukewarlow force-pushed the codelike-hostensurecancompilestrings branch from 21861c1 to db2e2bb Compare April 30, 2026 11:27
This change introduces a new HostGetCodeForEval host hook, this is used to allow hosts to evaluate certain objects.

This change also provides the full code string, the compilation type and the original arguments, to the host hook HostEnsureCanCompileStrings.
@lukewarlow lukewarlow force-pushed the codelike-hostensurecancompilestrings branch from db2e2bb to 10a2eff Compare April 30, 2026 13:39
@nicolo-ribaudo
Copy link
Copy Markdown
Member

Removing the [needs test262 tests] label per the April 2024 discussion (https://github.com/tc39/notes/blob/main/meetings/2024-04/april-10.md#evalnew-function-changes-for-trusted-types-as-normative-pr-or-stage-3) (the change is not easily testable in test262 since it's just about how much info is exposed to hosts, and it's covered by WPT)


Regarding the esmeta failure, it seems like it's checking updated call sites to HostEnsureCanCompileStrings using the old signature of the host hook, rather than the new one from this PR?

@nicolo-ribaudo nicolo-ribaudo removed the needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 label Apr 30, 2026
Copy link
Copy Markdown
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

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

This PR introduces _sourceStr_ and _codeString_ variables: we'll want to use wither String or Str consistently, but we need to wait for #3837 to decide which one.

--

For other reviewers:

Comment thread spec.html
</h1>
<dl class="header">
<dt>description</dt>
<dd>It allows host environments to return a String of code from _argument_ to be used by eval, rather than eval returning _argument_.</dd>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
<dd>It allows host environments to return a String of code from _argument_ to be used by eval, rather than eval returning _argument_.</dd>
<dd>It allows host environments to return a String representing _argument_ to be used as code by eval, rather than eval returning _argument_ as-is.</dd>

Comment thread spec.html
<dt>description</dt>
<dd>It allows host environments to return a String of code from _argument_ to be used by eval, rather than eval returning _argument_.</dd>
</dl>
<p>_argument_ represents the Object to be checked for code.</p>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would remove this, it's repeating what the <dl> description says.

Comment thread spec.html
Comment on lines +29912 to +29915
_parameterStrings_ represents the strings that, when using one of the function constructors, will be concatenated together to build the parameters list.
_bodyString_ represents the function body or the string passed to an `eval` call.
_codeString_ represents the compiled string for a Function or the string passed to an `eval` call.
_parameterArgs_ are the values passed as the leading parameters to one of the Function constructors. _bodyArg_ is either the final parameter passed to one of the Function constructors or the value passed to an `eval` call.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This list is missing _compilationType_.

Also, it's has so many entries now that maybe it should be a <ol> list, even though that's slightly unusual. @tc39/ecma262-editors thoughts?

@nicolo-ribaudo nicolo-ribaudo added the editor call to be discussed in the next editor call label Apr 30, 2026
@nicolo-ribaudo
Copy link
Copy Markdown
Member

@jhnaldo This PR will need a corresponding update in https://github.com/es-meta/esmeta/blob/main/src/main/resources/manuals/funcs/HostEnsureCanCompileStrings.ir.

Should we add this to esmeta-ignore.json for now, so that ESMeta can be updated after that this PR is merged, or is it possible somehow for ESMeta to typecheck both this PR and what's currently on main?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

editor call to be discussed in the next editor call needs consensus This needs committee consensus before it can be eligible to be merged. normative change Affects behavior required to correctly evaluate some ECMAScript source text pending stage 4 This proposal has not yet achieved stage 4, but may otherwise be ready to merge. proposal This is related to a specific proposal, and will be closed/merged when the proposal reaches stage 4.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants