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

Update HostEnsureCanCompileStrings definition #10204

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

lukewarlow
Copy link
Member

@lukewarlow lukewarlow commented Mar 14, 2024

Update the HostEnsureCanCompileStrings definition to match dynamic code brand checks stage 3 proposal.

Also update the call to EnsureCSPDoesNotBlockStringCompilation to pass these new arguments through.

Also update the timer initialization steps to call EnsureCSPDoesNotBlockStringCompilation directly, and include the new parameters.

Also define HostGetCodeForEval implementation

See w3c/webappsec-csp#650 for corresponding CSP PR

Also see #10202 for context

  • At least two implementers are interested (and none opposed):
  • Tests are written and can be reviewed and commented upon at:
  • Implementation bugs are filed:
    • Chromium: …
    • Gecko: …
    • WebKit: …
    • Deno (only for timers, structured clone, base64 utils, channel messaging, module resolution, web workers, and web storage): …
    • Node.js (only for timers, structured clone, base64 utils, channel messaging, and module resolution): …
  • MDN issue is filed: …
  • The top of this comment includes a clear commit message to use.

(See WHATWG Working Mode: Changes for more details.)


/index.html ( diff )
/infrastructure.html ( diff )
/references.html ( diff )
/timers-and-user-prompts.html ( diff )
/webappapis.html ( diff )

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

LGTM. Seems fine to merge this now, without necessarily waiting on CSP? It would just move the broken boundary from JS -> HTML to HTML -> CSP.

@annevk
Copy link
Member

annevk commented Mar 15, 2024

Is the CSP integration correct though? It doesn't make sense we'd invoke something there that just drops an argument on the floor.

@domenic
Copy link
Member

domenic commented Mar 15, 2024

I think the CSP integration is correct for current browsers but there are ambitions of adding more checks using more arguments in the future. As such, ECMAScript refactored itself.

I guess the alternative is to have HTML drop the arguments on the floor for now, and only upgrade to passing them along when CSP starts using it.

@annevk
Copy link
Member

annevk commented Mar 15, 2024

Okay, if that's the story I think this PR is fine and the CSP PR needs a note at the end of the algorithm describing that the argument is intentionally ignored and might be used in the future.

Copy link
Contributor

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

Looks good, I agree with the principle of just passing through all the parameters to CSP so that their handling is fully contained just in ECMA-262 and CSP (rather than having to read algorithms across three different specs).

@annevk
Copy link
Member

annevk commented Mar 15, 2024

The CSP PR expects both bodyString and source but we only pass one of them here. I guess that's a bug?

@nicolo-ribaudo
Copy link
Contributor

Yeah that's tracked at tc39/ecma262#938, it's a pre-existing issue. I guess CSP could either:

@annevk
Copy link
Member

annevk commented Mar 15, 2024

It seems at that point we'd have to update HTML again as well? I guess I'd rather we figure out a coherent story first.

@lukewarlow
Copy link
Member Author

Yeah so a second change will be needed if tc39/ecma262#3294 gets accepted. Though if it's not CSP would have to reconstruct the string itself (if it even has all the information it needs to do that). Happy to wait on the HTML end till we have the follow up change accepted if you'd prefer. Thought I'd do it now as it seemed worth while updating HTML to match current ecmascript either way.

@annevk
Copy link
Member

annevk commented Mar 15, 2024

Well the status quo is broken. But if merge this PR and the CSP as-is, it's still broken. So I'm not sure what that's buying us.

@lukewarlow lukewarlow marked this pull request as draft April 9, 2024 11:04
@lukewarlow lukewarlow force-pushed the update-HostEnsureCanCompileStrings branch from 0fe532d to efe1eac Compare April 15, 2024 16:00
Update the HostEnsureCanCompileStrings definition to match dynamic code brand checks stage 3 proposal.

Also update the call to EnsureCSPDoesNotBlockStringCompilation to pass these new arguments through.

Also update the timer initialization steps to call EnsureCSPDoesNotBlockStringCompilation directly, and include the new parameters.

Also define HostGetCodeForEval implementation
source Outdated Show resolved Hide resolved
@lukewarlow lukewarlow marked this pull request as draft April 17, 2024 14:29
@lukewarlow lukewarlow marked this pull request as ready for review April 23, 2024 16:55
@lukewarlow
Copy link
Member Author

Idk if it's neccessary given it will be addressed soon (all going well) but this currently has an extra parameter than the actual proposal doc and PR associated with it, so should I add an issue note?

We were told to remove the codeString param and recreate it in web land, but the reasoning was flawed and so we're going to re-request passing that value through at the next TC39 plenary. It deosn't seem useful to hold off on updating the specs till then (June time) espeically as this would delay upstreaming core parts of the trusted types spec to HTML and CSP.

@domenic
Copy link
Member

domenic commented Apr 24, 2024

From what I understand of @annevk's position, he would prefer we not change this more than once, so I think holding off is going to be necessary.

@nicolo-ribaudo
Copy link
Contributor

@domenic Part of the consensus from the last TC39 meeting was that this change should be a proposal (already at stage 3) rather than just a PR to the spec, so this PR can already link to the final version of the host hook from the proposal rather than waiting for it to be merged in ECMA-262 (also because that will require two shipping implementations).

@annevk
Copy link
Member

annevk commented Apr 24, 2024

If those participating in TC39 think acceptance of tc39/ecma262#3294 is a given I guess I'm willing to trust that, but it seems weird that it's still a draft. I'm also not entirely sure what the ~ syntax means.

I do think we should have a coherent story between that TC39 PR, the CSP PR, and this HTML PR.

@lukewarlow
Copy link
Member Author

If those participating in TC39 think acceptance of tc39/ecma262#3294 is a given I guess I'm willing to trust that, but it seems weird that it's still a draft. I'm also not entirely sure what the ~ syntax means.

So the PR is more so in preperation for it moving to stage 4 (once we have 2 shipping implementations). The proposal document https://tc39.es/proposal-dynamic-code-brand-checks/ is the live proposal, so I wouldn't worry too much that it's in draft.

The ~ syntax is for an enum.

I do think we should have a coherent story between that TC39 PR, the CSP PR, and this HTML PR.

I'll have a look at whether we can update that TC39 PR before the next meeting, so that CSP and HTML match it. Or at the very least whether we can update that proposal document.

@lukewarlow
Copy link
Member Author

@domenic and @annevk I've updated the tc39 PR and have tc39/proposal-dynamic-code-brand-checks#17 which just needs merging for the proposal document. Then we have alignment between JS PR, HTML PR, and CSP PR.

@lukewarlow lukewarlow requested a review from annevk May 13, 2024 18:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants