Wire up 'HostEnsureCanCompileStrings' to CSP

[mikewest]

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

[mikewest]

@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 eval(), so I'd suggest that keeping the definition in HTML makes sense.

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.

[annevk]

The <ref>s should be after the final dot, before </p>. Your approach seems okay to me.

[domenic]

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.

[mikewest]

Addressed nits. Happy to make more changes once you take a closer look. :)

[annevk]

You forgot to remove the space before the dot.

[mikewest]

Ugh. Fixed. :)

[domenic]

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:

User agents must use the following implementation: [JAVASCRIPT]

1. Perform ? EnsureCSPDoesNotBlockStringCompilation(callerRealm, calleeRealm). [CSP]

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.

[annevk]

Should event handlers call this too?

[mikewest]

I think we want to follow the pattern previously established

Done.

we need to actually patch setTimeout and setInterval too

I took a stab at this. WDYT?

Should event handlers call this too?

No. Event handler attributes are "inline" JavaScript vs. dynamically compiled script.

[annevk]

@mikewest hmm, setAttribute("onclick", "...") is not dynamically compiled?

[mikewest]

hmm, setAttribute("onclick", "...") is not dynamically compiled?

Huh. I guess it is. It's not clear to me that there's much value in gating it on both 'unsafe-inline' and 'unsafe-eval' though (since if the former is present, the code could just be injected directly via innerHTML and the like.

Still, it might be more internally consistent to treat setAttribute the same as setTimeout if it's setting a handler. I don't think the spec currently distinguishes between the two, and it's not clear whether you'd want me to handle that in DOM (with setAttribute) or in HTML (in "get the current value of the event handler", I suppose?). WDYT?

[Ms2ger]

I think in HTML would make the most sense.

[annevk]

I think I agree with @Ms2ger. It would mean setAttribute() would not throw and the value would be set as-is, but it would not work either since it never got compiled. So far setAttribute() has remained clean from any special cases and it seems nice to preserve that.

[mikewest]

Ok. Wherever we do the setAttribute check (and I've come around to the idea that it's a good thing to do), would you mind if we did it in a separate patch? This patch is basically behavior that exists. The next patch would be behavior that doesn't exist. I think it would be helpful to label the patch as a change, and to tie it to a relevant change in the CSP spec.

Filed w3c/webappsec-csp#67 to cover those changes.

[annevk]

That sounds fine to me. I'll let @domenic land this though and do a final review.

[shekyan]

Shouldn't this cover cloneNode ?

[domenic]

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.

[domenic]

span should wrap the function name, not the function call

[domenic]

Merged as 374b54d; forgot the "PR: ..." line sadly.