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.

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

Normative: Provide source text to HostEnsureCanCompileStrings #3222

Merged
merged 1 commit into from
Feb 14, 2024

Conversation

ptomato
Copy link
Contributor

@ptomato ptomato commented Nov 16, 2023

This is based on Mike Samuel's work in #1498, but ultimately goes in a different direction. Due to #2670, this change can be made simpler than it previously was.

This change provides the source text to be evaluated, and the grammar symbol that should be used to parse it, to the host hook HostEnsureCanCompileStrings.

One example of where this is needed is for allowing a Content Security Policy to provide hashes for code executed via eval() or new Function():
w3c/webappsec-csp#623
This is useful on its own, but has come up again in the topic of ShadowRealm-HTML integration. In a ShadowRealm you can either execute code asynchronously, with ShadowRealm.p.importValue, or synchronously, with ShadowRealm.p.evaluate. Because the latter uses eval() inside the ShadowRealm, it's subject to CSP rules, so the only CSP policy that will let you execute synchronously in the realm is unsafe-eval.

The original purpose of #1498 was to support Trusted Types, which is still a goal of this PR.

This is a separate needs-consensus PR, rather than being part of the ShadowRealm proposal, because it's useful independently of ShadowRealm, and also ShadowRealm would go forward regardless of whether this goes forward.

Prior art: https://github.com/tc39/proposal-dynamic-code-brand-checks

@ptomato
Copy link
Contributor Author

ptomato commented Nov 16, 2023

cc @mikesamuel

spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
jmdyck
jmdyck previously requested changes Nov 16, 2023
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
@ptomato ptomato force-pushed the source-text-hostensurecancompilestrings branch from d07cd69 to 8b2f141 Compare November 16, 2023 19:41
@ptomato ptomato requested a review from jmdyck November 16, 2023 19:48
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
@ptomato ptomato force-pushed the source-text-hostensurecancompilestrings branch 2 times, most recently from 2a8a0a0 to a4dbfc8 Compare November 17, 2023 18:42
spec.html Outdated Show resolved Hide resolved
@ptomato ptomato force-pushed the source-text-hostensurecancompilestrings branch 2 times, most recently from 9659303 to a0c6261 Compare November 29, 2023 00:59
@ptomato
Copy link
Contributor Author

ptomato commented Nov 29, 2023

Updated to reflect the consensus in TC39 plenary of 2023-11-27:

  • eval() and friends pass their argument to HostEnsureCanCompileStrings
  • new Function() and friends pass their unstringified argument to HostEnsureCanCompileStrings, only if there is a single argument. If there is more than one argument, the previous behaviour is maintained.
  • In the case of direct eval, we inform HostEnsureCanCompileStrings of this.

@caridy
Copy link
Contributor

caridy commented Nov 29, 2023

thanks for the update @ptomato, this is looking good!

spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
@ptomato ptomato force-pushed the source-text-hostensurecancompilestrings branch from a0c6261 to e149547 Compare November 29, 2023 23:05
@ptomato
Copy link
Contributor Author

ptomato commented Nov 29, 2023

I've updated this to reflect the option that had the most support in today's plenary meeting: passing the body string to the host hook after stringifying it.

(I've also removed the references to #1498 from the commit message and reset the author; this PR no longer subsumes the goal of #1498, and in fact has ship-of-Theseus'ed so that it now contains nothing from the original)

cc @erights for further discussion

spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
@ptomato ptomato force-pushed the source-text-hostensurecancompilestrings branch from e149547 to 82fc88b Compare November 30, 2023 17:22
spec.html Outdated Show resolved Hide resolved
@ptomato ptomato force-pushed the source-text-hostensurecancompilestrings branch from 82fc88b to f3bc8eb Compare November 30, 2023 17:37
@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Dec 6, 2023

Note that Firefox, Safari and Chrome already implement Option 2. Test case:

<html>
<head>
  <meta http-equiv="Content-Security-Policy" content="script-src 'unsafe-inline'">
</head>
<script>
new Function({
  toString: () => {
    console.log("Should not happen");
    return "return 2"
  },
});
</script>
</html>

All the three browsers log before throwing.

The observable change introduced my option 2 would thus just be matching web reality :)

@bakkot
Copy link
Contributor

bakkot commented Dec 6, 2023

For posterity, "option 2" refers to this slide.

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

(note: do not merge yet because we haven't had the chance to talk about this with MM, which he asked to do before merging)

spec.html Outdated Show resolved Hide resolved
@ptomato ptomato force-pushed the source-text-hostensurecancompilestrings branch from 1ebabde to db61ea2 Compare December 14, 2023 16:29
spec.html Outdated Show resolved Hide resolved
Copy link
Collaborator

@jmdyck jmdyck left a comment

Choose a reason for hiding this comment

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

Looks okay to me.

@ptomato
Copy link
Contributor Author

ptomato commented Feb 7, 2024

@tc39/ecma262-editors I realized this one wasn't in the list of things that would be added before ES2024 is cut. But I believe all of the discussions have been resolved, is there anything else preventing it from making the cut?

@michaelficarra michaelficarra added the es2024 to be landed before es2024 label Feb 7, 2024
@michaelficarra michaelficarra added the ready to merge Editors believe this PR needs no further reviews, and is ready to land. label Feb 9, 2024
)

This change provides the source text to be evaluated, and the grammar
symbol that should be used to parse it, to the host hook
HostEnsureCanCompileStrings.

One example of where this is needed is for allowing a Content Security
Policy to provide hashes for code executed via `eval()` or
`new Function()`:
w3c/webappsec-csp#623
This is useful on its own, but has come up again in the topic of
ShadowRealm-HTML integration. In a ShadowRealm you can either execute code
asynchronously, with ShadowRealm.p.importValue, or synchronously, with
ShadowRealm.p.evaluate. Because the latter uses `eval()` inside the
ShadowRealm, it's subject to CSP rules, so the only CSP policy that will
let you execute synchronously in the realm is `unsafe-eval`.

This is a separate needs-consensus PR, rather than being part of the
ShadowRealm proposal, because it's useful independently of ShadowRealm,
and also ShadowRealm would go forward regardless of whether this goes
forward.

Prior art: https://github.com/tc39/proposal-dynamic-code-brand-checks
@ljharb ljharb force-pushed the source-text-hostensurecancompilestrings branch from af086c8 to b07ca06 Compare February 14, 2024 05:28
@ljharb ljharb dismissed jmdyck’s stale review February 14, 2024 05:38

changes addressed

@ljharb ljharb merged commit b07ca06 into tc39:main Feb 14, 2024
7 checks passed
ptomato added a commit to ptomato/proposal-shadowrealm that referenced this pull request Feb 15, 2024
As of tc39/ecma262#3222 the signature of
HostEnsureCanCompileStrings changed. This adapts to the new signature by
pulling in the latest version of ecma262-biblio and adding the missing
arguments.

Closes: tc39#367
caridy pushed a commit to tc39/proposal-shadowrealm that referenced this pull request Feb 16, 2024
As of tc39/ecma262#3222 the signature of
HostEnsureCanCompileStrings changed. This adapts to the new signature by
pulling in the latest version of ecma262-biblio and adding the missing
arguments.

Closes: #367
lukewarlow added a commit to lukewarlow/proposal-dynamic-code-brand-checks that referenced this pull request Feb 20, 2024
…changed. This updates the README to reflect the current state.
lukewarlow added a commit to lukewarlow/proposal-dynamic-code-brand-checks that referenced this pull request Feb 20, 2024
…changed. This updates the spec to better reflect the current state.
lukewarlow added a commit to lukewarlow/proposal-dynamic-code-brand-checks that referenced this pull request Feb 20, 2024
…changed. This updates the spec to better reflect the current state.

This also updates the README accordingly.
lukewarlow added a commit to lukewarlow/proposal-dynamic-code-brand-checks that referenced this pull request Feb 20, 2024
…changed. This updates the spec to better reflect the current state.

This also updates the README accordingly.
lukewarlow added a commit to lukewarlow/proposal-dynamic-code-brand-checks that referenced this pull request Feb 20, 2024
…changed. This updates the spec to better reflect the current state.

This also updates the README accordingly.
lukewarlow added a commit to lukewarlow/proposal-dynamic-code-brand-checks that referenced this pull request Feb 20, 2024
…changed. This updates the spec to better reflect the current state.

This also updates the README accordingly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
es2024 to be landed before es2024 has consensus This has committee consensus. layering affects the public spec interface and may require updates to integrating specs normative change Affects behavior required to correctly evaluate some ECMAScript source text ready to merge Editors believe this PR needs no further reviews, and is ready to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants