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 EnsureCSPDoesNotBlockStringCompilation to match updated HostEnsureCanCompileStrings definition #650

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

Conversation

lukewarlow
Copy link
Member

@lukewarlow lukewarlow commented Mar 14, 2024

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

Why is bodyString passed to CSP if it's completely ignored?

@lukewarlow
Copy link
Member Author

Mentioned on the HTML discussion but just so it's easily findable here to, CSP has future plans to make use of it (see #623 for the use case)

@annevk
Copy link
Member

annevk commented Mar 15, 2024

A bigger problem is that HTML doesn't pass in source.

@lukewarlow
Copy link
Member Author

A bigger problem is that HTML doesn't pass in source.

Yeah I can't speak for past reasoning but it seems pretty wild that Ecmascript didn't just update to include the full source back in the day when CSP needed it. And it seems CSP went with the approach of just saying well we need it so you as the implementor have to figure that bit out.

I guess we can wait and see what happens to ECMAScript, or we can update HTML to pass it through and then it's just ecmascript that needs dealing with (though trusted types requires extra params too)?

I'll defer to your editorial experience on the best path forwards :)

@annevk
Copy link
Member

annevk commented Mar 15, 2024

I think we should start with ECMAScript. They take PRs too. 😊

@lukewarlow lukewarlow marked this pull request as draft April 17, 2024 11:06
@lukewarlow lukewarlow force-pushed the update-EnsureCSPDoesNotBlockStringCompilation branch from 7613b69 to 961e959 Compare April 17, 2024 11:31
index.bs Outdated

1. If |compilationType| is `*FUNCTION*`:

1. Set |source| to `"function anonymous("`
Copy link
Member Author

Choose a reason for hiding this comment

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

TC39 didn't approve passing through the compiled code string so instead we reconstruct it here.

This matches WebKit and Firefox but Chromium currently wraps the entire string in () brackets.

This also doesn't have the context of whether it's an async function or generator, so this is a change in that regard but the usage of their constructors is probably very low (cc @nicolo-ribaudo)

Copy link
Member

Choose a reason for hiding this comment

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

Can you link the discussion?

Not sure who has to approve this for Chromium. @andypaicu and @mikewest maybe?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry for the delay on getting back to this. The discussion isn't published yet (I think soon?).

After a bit of a back and forth it turns out the reasoning behind the objection was flawed and so we're going to ask (I don't forsee there being push back personally) them to change it in the next plenary. In the mean time I'll change this spec PR (and HTMLs) to assume it does get passed through. Given CSP at least is already "broken" in that regard it doesn't seem bad to continue it for a bit longer till we get the tc39 side updated.

Copy link
Member Author

Choose a reason for hiding this comment

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

Given this is well defined in ECMA262 it would be interesting to see where the discrepency in Chrome comes from and whether there's test coverage for that particular aspect.

@lukewarlow lukewarlow marked this pull request as ready for review April 17, 2024 11:33
@lukewarlow lukewarlow requested a review from annevk April 17, 2024 11:33
@lukewarlow lukewarlow marked this pull request as draft April 17, 2024 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants