-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Wire up 'HostEnsureCanCompileStrings' to CSP #1005
Conversation
@domenic, @annevk: PTAL. I didn't follow Domenic's suggestions exactly, as this patch explicitly delegates to an operation defined in CSP rather than delegating the definition to CSP. I've done that because it's possible that something like https://wicg.github.io/ContentPerformancePolicy/ will also want to take a crack at limiting If you'd prefer to resolve this differently (e.g. delegate entirely to CSP until some other spec comes along), I'm happy to defer to y'all on that. |
data-x="js-HostEnsureCanCompileStrings">HostEnsureCanCompileStrings</span>(<var>callerRealm</var>, | ||
<var>calleeRealm</var>) abstract operation <ref spec=JAVASCRIPT>. User agents must use the <span | ||
data-x="csp-EnsureCSPDoesNotBlockStringCompilation">EnsureCSPDoesNotBlockStringCompilation(<var>callerRealm</var>, | ||
<var>calleeRealm</var>)</span> implementation defined in <ref spec=CSP>.</p> |
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.
The <ref>
s should be after the final dot, before </p>
. Your approach seems okay to me.
This approach does seem nice to me; I like that it makes HTML responsible for defining all the HostWhatever operations in one place. Let me get to work and compile and view the output before giving final LGTM on editorial issues. |
Addressed nits. Happy to make more changes once you take a closer look. :) |
@@ -86244,9 +86244,10 @@ dictionary <dfn>PromiseRejectionEventInit</dfn> : <span>EventInit</span> { | |||
|
|||
<p>JavaScript contains an implementation-defined <span | |||
data-x="js-HostEnsureCanCompileStrings">HostEnsureCanCompileStrings</span>(<var>callerRealm</var>, | |||
<var>calleeRealm</var>) abstract operation <ref spec=JAVASCRIPT>. User agents must use the <span | |||
<var>calleeRealm</var>) abstract operation . User agents must use the <span |
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.
You forgot to remove the space before the dot.
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.
Ugh. Fixed. :)
OK, so, a bit of work to do still I think. I think we want to follow the pattern previously established, which means you say:
Also, either you or I can do this, but we need to actually patch setTimeout and setInterval too to call this. I guess maybe we should wait for #951 to land to avoid conflicts though. |
CSP defines an implementation of 'HostEnsureCanCompileStrings' which throws an 'EvalError' if string compilation is disallowed. This patch wires that implementation up to the 'HostEnsureCanCompileStrings' abstract operation. #271
Should event handlers call this too? |
Done.
I took a stab at this. WDYT?
No. Event handler attributes are "inline" JavaScript vs. dynamically compiled script. |
@mikewest hmm, |
Huh. I guess it is. It's not clear to me that there's much value in gating it on both Still, it might be more internally consistent to treat |
I think in HTML would make the most sense. |
I think I agree with @Ms2ger. It would mean |
Ok. Wherever we do the Filed w3c/webappsec-csp#67 to cover those changes. |
That sounds fine to me. I'll let @domenic land this though and do a final review. |
Shouldn't this cover |
Going to leave a couple comments, but I'll fix up and merge myself since you did all the hard work and I haven't been a super-responsive reviewer so I don't want to put you through more days of back and forth. |
<ol> | ||
|
||
<li><p>Perform ? <span | ||
data-x="csp-EnsureCSPDoesNotBlockStringCompilation">EnsureCSPDoesNotBlockStringCompilation(<var>callerRealm</var>, |
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.
span should wrap the function name, not the function call
Merged as 374b54d; forgot the "PR: ..." line sadly. |
CSP defines an implementation of 'HostEnsureCanCompileStrings' which throws an
'EvalError' if string compilation is disallowed. This patch wires that implementation
up to the 'HostEnsureCanCompileStrings' abstract operation.
#271