Bug 93198 - When `eval` is blocked by CSP, the error message should make that clear.
Summary: When `eval` is blocked by CSP, the error message should make that clear.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mike West
URL:
Keywords:
Depends on: 94331 94332
Blocks: 93197
  Show dependency treegraph
 
Reported: 2012-08-05 02:41 PDT by Mike West
Modified: 2012-10-22 07:47 PDT (History)
7 users (show)

See Also:


Attachments
Patch (22.97 KB, patch)
2012-08-16 08:13 PDT, Mike West
buildbot: commit-queue-
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-05 02:41:50 PDT
"Code generation from strings disallowed for this context" doesn't provide enough context for developers whose `eval()`-like code is blocked by CSP.

Ideally, we'd have something more like "Refused to execute `eval()` because it violates the following Content Security Policy Directive: 'script-src 'self''" to match the rest of the logged warnings.
Comment 1 Mike West 2012-08-16 08:13:11 PDT
Created attachment 158824 [details]
Patch
Comment 2 Mike West 2012-08-16 08:13:43 PDT
Comment on attachment 158824 [details]
Patch

Dropping the r?. This is nowhere near ready to land. :)
Comment 3 Mike West 2012-08-16 08:15:13 PDT
Hey Jochen! This is a very much WIP patch that I'd appreciate some help with. Can you recommend someone who knows things about JSC to help me evaluate an approach that they might accept? :)
Comment 4 jochen 2012-08-16 08:34:26 PDT
Comment on attachment 158824 [details]
Patch

ggaren is your man

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

> Source/JavaScriptCore/ChangeLog:3
> +        When `eval` is blocked by CSP, the error message should make that clear.

'eval' :)

> Source/JavaScriptCore/runtime/JSGlobalObject.h:300
> +        void setEvalEnabled(bool enabled, const String& reason);

I'd pick a more descriptive name instead of reason. Something that indicates that this will be included in the error message if eval is disabled and something tries to eval

> Source/WebCore/page/ContentSecurityPolicy.cpp:643
> +    String m_allowEvalReason;

it's more a disallow reason, isn't it?
Comment 5 Mike West 2012-08-16 08:44:21 PDT
(In reply to comment #3)
> Hey Jochen! This is a very much WIP patch that I'd appreciate some help with. Can you recommend someone who knows things about JSC to help me evaluate an approach that they might accept? :)

(In reply to comment #4)
> (From update of attachment 158824 [details])
> ggaren is your man

Thanks Jochen! Hi Geoffrey! :)

Would you mind helping me work out a mechanism for passing information down into JSC to improve the experience for developers who unexpectedly run into Content Security Policy's ability to disable 'eval' and its friends?

I'd like on the one hand to pass in a more detailed string to be used as the error message, and on the other to generate a console warning (as we're seeing reports from developers using libraries that swallow the exception in a try/catch block). To make that possible, V8 has a callback mechanism (see http://code.google.com/searchframe#OAMlx_jo-ck/src/v8/src/runtime.cc&exact_package=chromium&q=allow_code_gen_callback&type=cs&l=9406) that I'd like to implement in JSC as well.

The current patch is an attempt at the former. I'd like to talk about the latter.

WDYT?
Comment 6 Build Bot 2012-08-16 08:44:31 PDT
Comment on attachment 158824 [details]
Patch

Attachment 158824 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13511598
Comment 7 Build Bot 2012-08-16 08:57:35 PDT
Comment on attachment 158824 [details]
Patch

Attachment 158824 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13512509
Comment 8 Geoffrey Garen 2012-08-16 15:10:16 PDT
> I'd like on the one hand to pass in a more detailed string to be used as the error message

Current patch looks good for that. I agree that you should use something more descriptive than "reason", which denotes an error message.

> and on the other to generate a console warning

The Web Inspector already gets callbacks when exceptions are thrown. Does that mechanism work for you? Does it need to be expanded in some way?
Comment 9 Mike West 2012-08-17 04:13:42 PDT
(In reply to comment #8)
> > I'd like on the one hand to pass in a more detailed string to be used as the error message
> 
> Current patch looks good for that. I agree that you should use something more descriptive than "reason", which denotes an error message.

Ok. Then I'm closer than I expected. :)

I'll split this bug into two: one for JSC and one for V8. The latter's going to be a bit more work. *cough*

> > and on the other to generate a console warning
> 
> The Web Inspector already gets callbacks when exceptions are thrown. Does that mechanism work for you? Does it need to be expanded in some way?

The goal is to provide developers who use libraries that swallow exceptions some idea of what's going on. Many libraries parse JSON with 'eval', wrap it in try/catch, and assume that an exception means "Invalid JSON!" It would be nice if we generated a console warning in addition to throwing an exception at the point where 'eval' is called.

I'll take a look at the throwError implementation to see if there's a simple way of hooking into the preexisting Web Inspector code. Thanks for the tip.
Comment 10 Mike West 2012-10-22 07:47:58 PDT
Both V8 and JSC landed. Closing this out.