|Summary:||CSP: 'eval()' is blocked in report-only mode.|
|Product:||WebKit||Reporter:||Mike West <mkwst>|
|Component:||WebCore Misc.||Assignee:||Mike West <mkwst>|
|Severity:||Normal||CC:||abarth, mkwst+watchlist, webkit.review.bot|
|Version:||528+ (Nightly build)|
|Bug Depends on:|
Description Mike West 2013-03-08 10:03:48 PST
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.
Comment 2 Adam Barth 2013-03-08 11:52:05 PST
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.
Comment 3 Mike West 2013-03-08 11:57:46 PST
(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
Comment 4 Adam Barth 2013-03-08 12:16:19 PST
Doesn't the HeaderType argument tell you?
Comment 5 Adam Barth 2013-03-08 12:18:15 PST
Comment on attachment 192248 [details] Patch I understand now. After the loop we're querying all previous headers too.
Comment 6 WebKit Review Bot 2013-03-08 14:21:58 PST
Comment on attachment 192248 [details] Patch Clearing flags on attachment: 192248 Committed r145268: <http://trac.webkit.org/changeset/145268>
Comment 7 WebKit Review Bot 2013-03-08 14:22:02 PST
All reviewed patches have been landed. Closing bug.