Bug 155842 - CSP: Move logic for reporting a violation from ContentSecurityPolicyDirectiveList to ContentSecurityPolicy
Summary: CSP: Move logic for reporting a violation from ContentSecurityPolicyDirective...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Local Build
Hardware: All All
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-03-24 10:22 PDT by Daniel Bates
Modified: 2016-03-24 19:14 PDT (History)
4 users (show)

See Also:


Attachments
Patch (103.58 KB, patch)
2016-03-24 10:49 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch (105.61 KB, patch)
2016-03-24 11:05 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch (110.93 KB, patch)
2016-03-24 11:21 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch (110.94 KB, patch)
2016-03-24 11:34 PDT, Daniel Bates
bfulgham: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 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.
Comment 1 Radar WebKit Bug Importer 2016-03-24 10:22:52 PDT
<rdar://problem/25340377>
Comment 2 Daniel Bates 2016-03-24 10:49:33 PDT
Created attachment 274841 [details]
Patch
Comment 3 Daniel Bates 2016-03-24 11:05:02 PDT
Created attachment 274843 [details]
Patch

Actually add file ContentSecurityPolicyDirective.cpp and rebase patch.
Comment 4 Daniel Bates 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.
Comment 5 Daniel Bates 2016-03-24 11:21:37 PDT
Created attachment 274845 [details]
Patch
Comment 6 Daniel Bates 2016-03-24 11:34:46 PDT
Created attachment 274846 [details]
Patch
Comment 7 Brent Fulgham 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!
Comment 8 Daniel Bates 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.
Comment 9 Daniel Bates 2016-03-24 19:14:18 PDT
Committed r198657: <http://trac.webkit.org/changeset/198657>