Bug 94332 - V8 should throw a more descriptive exception when blocking 'eval' via CSP.
Summary: V8 should throw a more descriptive exception when blocking 'eval' via CSP.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mike West
URL:
Keywords: WebExposed
Depends on:
Blocks: 93198
  Show dependency treegraph
 
Reported: 2012-08-17 04:20 PDT by Mike West
Modified: 2012-10-17 03:00 PDT (History)
6 users (show)

See Also:


Attachments
Patch (28.51 KB, patch)
2012-08-21 05:21 PDT, Ulan Degenbaev
no flags Details | Formatted Diff | Diff
Patch (6.67 KB, patch)
2012-09-15 02:13 PDT, Mike West
no flags Details | Formatted Diff | Diff
New method. (6.94 KB, patch)
2012-09-17 01:40 PDT, Mike West
no flags Details | Formatted Diff | Diff
Rebaselining. (12.31 KB, patch)
2012-09-20 08:31 PDT, Mike West
no flags Details | Formatted Diff | Diff
Patch (13.07 KB, patch)
2012-09-22 10:42 PDT, Mike West
no flags Details | Formatted Diff | Diff
Rebasing onto ToT (12.50 KB, patch)
2012-10-11 01:11 PDT, Mike West
no flags Details | Formatted Diff | Diff
Rebased again, for the last time (maybe). (12.51 KB, patch)
2012-10-17 02:00 PDT, Mike West
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mike West 2012-08-17 04:20:37 PDT
When 'eval' and 'eval'-like constructs are blocked by a 'script-src' Content Security Policy directive, V8 currently throws an exception reading "Code generation from strings disallowed for this context", which is fairly miserable. It would be nice if that message included more detail about what exactly had gone wrong. I'd like to see a string more along the lines of <Refused to evaluate a string as JavaScript because it violates the following Content Security Policy directive: "script-src 'unsafe-inline'">.
Comment 1 Mike West 2012-08-20 23:50:54 PDT
Hey Ulan! We talked about this change a few weeks back; https://bugs.webkit.org/show_bug.cgi?id=94331 is a patch to change the message in JSC. I'd like to get your help changing the message in V8.

Could you take a look at the JSC patch and help me evaluate how to make it work in V8?

Thanks!
Comment 2 Ulan Degenbaev 2012-08-21 05:21:46 PDT
Created attachment 159662 [details]
Patch
Comment 3 Ulan Degenbaev 2012-08-21 05:26:31 PDT
Hi Mike,

I uploaded a draft patch that depends on your patch and on this V8 CL: https://chromiumcodereview.appspot.com/10837358/

At the moment it passes tests, but there is one call site in WebKit where we need to pass the error message:

WebKit/Source/WebCore/bindings/v8/ScriptState.cpp : setEvalEnabled(ScriptState* scriptState, bool enabled)

It is used from inspector. Do you know how we can pass error message there?
Comment 4 Mike West 2012-08-21 05:50:17 PDT
Thanks Ulan. Let's let the JSC change land first (https://bugs.webkit.org/show_bug.cgi?id=94331); the patch to wire things up to the new V8 patch you've put together should then be quite small.

It looks like fixing the Web Inspector bit should be pretty straightforward as well. I'll just add some code to the bindings to expose the ErrorMessageForCodeGenerationFromStrings method that you've added, and then we should be able to save/rewrite that rather than just toggling the boolean.

Thank you!
Comment 5 Mike West 2012-09-15 01:01:21 PDT
Hey Ulan! The JSC patch just landed last night. *phew* :)

I'll rebase this patch on top of the current trunk. Would you mind taking another look at landing https://codereview.chromium.org/10837358/ in the meantime? I think we'll need that in place before we can land the WebKit side.
Comment 6 Mike West 2012-09-15 02:13:35 PDT
Created attachment 164279 [details]
Patch
Comment 7 Mike West 2012-09-15 02:14:45 PDT
(In reply to comment #6)
> Created an attachment (id=164279) [details]
> Patch

I'll rebaseline the tests once the V8 portion of this change lands and is rolled into WebKit.

It doesn't yet take care of ScriptState.cpp, as I think we'll need to chat about what the V8 API needs to look like. I'll poke you on Monday, Uan. :) Thanks!
Comment 8 Eric Seidel (no email) 2012-09-15 08:02:55 PDT
Comment on attachment 164279 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164279&action=review

Dr. Barth is your man.

> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:373
> +            context->AllowCodeGenerationFromStrings(m_frame->document()->contentSecurityPolicy()->allowEval(0, ContentSecurityPolicy::SuppressReport), v8String(m_frame->document()->contentSecurityPolicy()->evalDisabledErrorMessage()));

A ContentSecurityPolicy local would make this line much shorter. :)

> Source/WebCore/bindings/v8/WorkerContextExecutionProxy.cpp:89
> +    , m_disableEvalPending(String())

I can never remember if we have a nullString() which wer'e supposed to use instead these days.
Comment 9 Adam Barth 2012-09-15 13:44:07 PDT
Comment on attachment 164279 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164279&action=review

>> Source/WebCore/bindings/v8/WorkerContextExecutionProxy.cpp:89
>> +    , m_disableEvalPending(String())
> 
> I can never remember if we have a nullString() which wer'e supposed to use instead these days.

You can actually skip this line.  The compiler calls the default constructor for you.  nullAtom is only needed when you're returning something by reference.
Comment 10 Adam Barth 2012-09-15 13:44:22 PDT
Comment on attachment 164279 [details]
Patch

Patch looks fine.
Comment 11 Mike West 2012-09-17 01:40:36 PDT
Created attachment 164357 [details]
New method.
Comment 12 Mike West 2012-09-17 01:41:24 PDT
Hey Ulan, here's an updated version of the patch to match the API changes you've made. It doesn't compile without those changes, so... Let's hope it compiles on your machine. :)
Comment 13 Ulan Degenbaev 2012-09-17 02:43:11 PDT
(In reply to comment #12)
> Hey Ulan, here's an updated version of the patch to match the API changes you've made. It doesn't compile without those changes, so... Let's hope it compiles on your machine. :)

Yep, it compiles and produces the following outputs for 
eval-blocked-in-about-blank-iframe.html, eval-blocked.html, function-constructor-blocked.html:


CONSOLE MESSAGE: line 1: Uncaught EvalError: Refused to evaluate a string as JavaScript because 'unsafe-eval' is not an allowed source of script in the following Content Security Policy directive: "script-src 'unsafe-inline'".

CONSOLE MESSAGE: line 12: Uncaught EvalError: Refused to evaluate a string as JavaScript because 'unsafe-eval' is not an allowed source of script in the following Content Security Policy directive: "script-src 'unsafe-inline'".

CONSOLE MESSAGE: line 15: Uncaught EvalError: Refused to evaluate a string as JavaScript because 'unsafe-eval' is not an allowed source of script in the following Content Security Policy directive: "script-src 'unsafe-inline'".

ONSOLE MESSAGE: line 12: Uncaught EvalError: Refused to evaluate a string as JavaScript because 'unsafe-eval' is not an allowed source of script in the following Content Security Policy directive: "script-src 'unsafe-inline'".
Comment 14 Mike West 2012-09-17 02:47:18 PDT
(In reply to comment #13)
> Yep, it compiles and produces the following outputs for 
> eval-blocked-in-about-blank-iframe.html, eval-blocked.html, function-constructor-blocked.html:

Looks good. Would you mind landing the V8 side of the patch? Once it gets rolled into the Chromium port, I'll see about landing this side.

Thanks, Ulan!
Comment 15 Ulan Degenbaev 2012-09-17 03:08:29 PDT
I landed the V8 CL as http://code.google.com/p/v8/source/detail?r=12525 to V8 bleeding_edge. It looks like we won't roll V8 until Chrome branches (V8 trunk is frozen).
Comment 16 Mike West 2012-09-17 03:18:30 PDT
(In reply to comment #15)
> I landed the V8 CL as http://code.google.com/p/v8/source/detail?r=12525 to V8 bleeding_edge. It looks like we won't roll V8 until Chrome branches (V8 trunk is frozen).

Makes sense. So, what timeframe are we looking at? Beginning of next weekish?
Comment 17 Ulan Degenbaev 2012-09-17 03:52:02 PDT
> So, what timeframe are we looking at? Beginning of next weekish?
Yes. Probably, next week Wed.
Comment 18 Mike West 2012-09-20 08:31:03 PDT
Created attachment 164926 [details]
Rebaselining.
Comment 19 Mike West 2012-09-20 08:33:00 PDT
(In reply to comment #18)
> Created an attachment (id=164926) [details]
> Rebaselining.

I've rolled DEPS locally and rebaselined the tests. I think we should be able to land this directly after the V8 change rolls into WebKit ToT next week.
Comment 20 Mike West 2012-09-22 10:42:48 PDT
Created attachment 165266 [details]
Patch
Comment 21 Mike West 2012-09-22 10:43:48 PDT
Comment on attachment 165266 [details]
Patch

Dropping the r?, throwing to the bots. If the bots are happy, I'll carry Adam's r+ over and ask for CQ?. :)
Comment 22 Ulan Degenbaev 2012-09-24 00:48:55 PDT
V8 roll that this patch depends on is going to be reverted because of high crash rate :(
Comment 23 Mike West 2012-09-24 00:51:41 PDT
(In reply to comment #22)
> V8 roll that this patch depends on is going to be reverted because of high crash rate :(

Ah, too bad!

Good that I waited until Monday. :)
Comment 24 Mike West 2012-10-04 04:52:17 PDT
(In reply to comment #23)
> (In reply to comment #22)
> > V8 roll that this patch depends on is going to be reverted because of high crash rate :(

Looks like the next try at a roll was reverted too. :(
Comment 25 Mike West 2012-10-08 02:59:54 PDT
(In reply to comment #24)
> (In reply to comment #23)
> > (In reply to comment #22)
> > > V8 roll that this patch depends on is going to be reverted because of high crash rate :(
> 
> Looks like the next try at a roll was reverted too. :(

And the next. I'm putting this on ice until the V8 roll happens and sticks. Probably next week? Bleh. :/
Comment 26 Mike West 2012-10-11 01:11:44 PDT
Created attachment 168159 [details]
Rebasing onto ToT
Comment 27 Mike West 2012-10-17 02:00:30 PDT
Created attachment 169126 [details]
Rebased again, for the last time (maybe).
Comment 28 Mike West 2012-10-17 02:05:54 PDT
V8 has rolled in the fix I was waiting for, and a fix to that fix. Things work locally; throwing this into the queue. :)
Comment 29 WebKit Review Bot 2012-10-17 03:00:21 PDT
Comment on attachment 169126 [details]
Rebased again, for the last time (maybe).

Clearing flags on attachment: 169126

Committed r131573: <http://trac.webkit.org/changeset/131573>
Comment 30 WebKit Review Bot 2012-10-17 03:00:27 PDT
All reviewed patches have been landed.  Closing bug.