CSP: 'eval()' is blocked in report-only mode.

[Mike West]

In report-only mode, CSP should allow 'eval()' to execute; it should not be blocked. That's not what we're currently doing. See http://stackoverflow.com/questions/15273841/eval-isnt-executed-in-chrome-when-using-content-security-policy-report-only/15300012#15300012.

[Mike West]

Created attachment 192248 [details]
Patch

[Adam Barth]

Comment on attachment 192248 [details]
Patch

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

> Source/WebCore/page/ContentSecurityPolicy.cpp:-1460
> -    if (!allowEval(0, SuppressReport))
> -        m_scriptExecutionContext->disableEval(evalDisabledErrorMessage());

Why did you move this into the loop?  It seems like we can just add a check for |type| to this code here.

[Mike West]

(In reply to comment #2)
> (From update of attachment 192248 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=192248&action=review
> 
> > Source/WebCore/page/ContentSecurityPolicy.cpp:-1460
> > -    if (!allowEval(0, SuppressReport))
> > -        m_scriptExecutionContext->disableEval(evalDisabledErrorMessage());
> 
> Why did you move this into the loop?  It seems like we can just add a check for |type| to this code here.

I moved it into the loop because it was the simplest mechanism for checking whether a specific policy was report-only, as opposed to checking if the combined set of policies allowed eval or not.

We don't currently have a mechanism of distinguishing between these two cases:

* Content-Security-Policy: script-src 'self';

and

* Content-Security-Policy-Report-Only: script-src 'self';
* Content-Security-Policy: img-src 'none';

For most directives, this doesn't matter, as we do the report-only check inline with the request. For eval, on the other hand, we have to tell V8/JSC whether it should allow or deny 'eval()' a priori. See also bug 111869 for another consequence of this.

I think the behavior of ContentSecurityPolicy::allowEval should probably change, as you suggest, but I'm not sure how that needs to look

[Adam Barth]

Doesn't the HeaderType argument tell you?

[Adam Barth]

Comment on attachment 192248 [details]
Patch

I understand now.  After the loop we're querying all previous headers too.

[WebKit Review Bot]

Comment on attachment 192248 [details]
Patch

Clearing flags on attachment: 192248

Committed r145268: <http://trac.webkit.org/changeset/145268>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.