-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Update HostEnsureCanCompileStrings definition #10204
Conversation
There was a problem hiding this 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.
Is the CSP integration correct though? It doesn't make sense we'd invoke something there that just drops an argument on the floor. |
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. |
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. |
There was a problem hiding this 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).
The CSP PR expects both bodyString and source but we only pass one of them here. I guess that's a bug? |
Yeah that's tracked at tc39/ecma262#938, it's a pre-existing issue. I guess CSP could either:
|
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. |
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. |
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. |
efe1eac
to
a52101f
Compare
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. |
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. |
@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). |
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 I do think we should have a coherent story between that TC39 PR, the CSP PR, and this HTML PR. |
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'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. |
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lots of editorial
1bf6e88
to
404ed3b
Compare
This LGTM. @annevk do you want to take a final look? We'll want to ensure w3c/webappsec-csp#650 is merged around the same time. I'm not sure who has permissions to do that. I guess we should wait for at least PR approval before merging this. |
As for permissions, I can merge the CSP PR (the repo doesn't even seem to require an approval which seems like an oversight in the configuration?), just need an approval on it. @annevk I'm assuming you have permission to do that? Would be good to try and get this across the line. I'll rebase this HTML PR now. |
404ed3b
to
98b796b
Compare
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
55ca8ac
to
e1d8470
Compare
Rebased and fixed merge conflict |
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
(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 )