RESOLVED FIXED 155842
CSP: Move logic for reporting a violation from ContentSecurityPolicyDirectiveList to ContentSecurityPolicy
https://bugs.webkit.org/show_bug.cgi?id=155842
Summary CSP: Move logic for reporting a violation from ContentSecurityPolicyDirective...
Daniel Bates
Reported 2016-03-24 10:22:20 PDT
As a step towards fixing bug #114317, move the responsibility of logging a console message and sending a violation report from class ContentSecurityPolicyDirectiveList to class ContentSecurityPolicy. ContentSecurityPolicyDirectiveList should be responsible for parsing a policy and providing functions that a caller can use to check a source against the policy. The caller of ContentSecurityPolicyDirectiveList, the ContentSecurityPolicy object, should be responsible for taking the appropriate action, which may include logging a console message and sending a violation report.
Attachments
Patch (103.58 KB, patch)
2016-03-24 10:49 PDT, Daniel Bates
no flags
Patch (105.61 KB, patch)
2016-03-24 11:05 PDT, Daniel Bates
no flags
Patch (110.93 KB, patch)
2016-03-24 11:21 PDT, Daniel Bates
no flags
Patch (110.94 KB, patch)
2016-03-24 11:34 PDT, Daniel Bates
bfulgham: review+
Radar WebKit Bug Importer
Comment 1 2016-03-24 10:22:52 PDT
Daniel Bates
Comment 2 2016-03-24 10:49:33 PDT
Daniel Bates
Comment 3 2016-03-24 11:05:02 PDT
Created attachment 274843 [details] Patch Actually add file ContentSecurityPolicyDirective.cpp and rebase patch.
Daniel Bates
Comment 4 2016-03-24 11:17:03 PDT
We likely can further clean up the code in ContentSecurityPolicy and remove duplication. I will look to do such clean up either as part of the fix for bug #114317 or in a subsequent bug/patch.
Daniel Bates
Comment 5 2016-03-24 11:21:37 PDT
Daniel Bates
Comment 6 2016-03-24 11:34:46 PDT
Brent Fulgham
Comment 7 2016-03-24 15:15:44 PDT
Comment on attachment 274846 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=274846&action=review I got worried you might have a copy/paste error, but looking further I think it was written as intended. r=me. > Source/WebCore/page/csp/ContentSecurityPolicy.cpp:270 > + return isReportOnly; This section seems identical to the code in 'allowJavaScriptURLs". Is this intentional? Why is the error name and all other feature of this "inline event hander" case the same as the "allow JavaScript URLs" case? And if they are identical, why keep both methods? > Source/WebCore/page/csp/ContentSecurityPolicy.cpp:-437 > - ASSERT(!m_frame || effectiveDirective == "frame-ancestors"); Yay!
Daniel Bates
Comment 8 2016-03-24 16:53:35 PDT
(In reply to comment #7) > Comment on attachment 274846 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=274846&action=review > > I got worried you might have a copy/paste error, but looking further I think > it was written as intended. r=me. > > > Source/WebCore/page/csp/ContentSecurityPolicy.cpp:270 > > + return isReportOnly; > > This section seems identical to the code in 'allowJavaScriptURLs". Is this > intentional? Yes, this was intentional for the moment. I initially planned to change the error message text for an inline event handler violation in a subsequent patch because this patch was already large and I thought we might need to update the expected results for many tests. As it turns out, we have only three tests for inline event handlers (maybe we can add more?). So, its seems reasonable to simply change the error message for an inline event handler violation in this patch before landing and include updated results for those three affected tests. Before I land this patch I will change line 256 in the patch (attachment #274846 [details]) from: String consoleMessage = consoleMessageForViolation(ContentSecurityPolicyDirectiveNames::scriptSrc, *violatedDirective, URL(), "Refused to execute a script", "its hash, its nonce, or 'unsafe-inline'"); to String consoleMessage = consoleMessageForViolation(ContentSecurityPolicyDirectiveNames::scriptSrc, *violatedDirective, URL(), "Refused to execute a script for an inline event handler", "'unsafe-inline'"); Then we will emit the following error message for an inline event handler violation when the policy defines directive script-src and does not define directive default-src: Refused to execute a script for an inline event handler because 'unsafe-inline' does not appear in the script-src directive of the Content Security Policy. And we will emit the following error message for an inline event handler violation when the policy does not define directive script-src and does define directive default-src: Refused to execute a script for an inline event handler because 'unsafe-inline' appears in neither the script-src directive nor the default-src directive of the Content Security Policy.
Daniel Bates
Comment 9 2016-03-24 19:14:18 PDT
Note You need to log in before you can comment on or make changes to this bug.