WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2016-03-24 10:22:52 PDT
<
rdar://problem/25340377
>
Daniel Bates
Comment 2
2016-03-24 10:49:33 PDT
Created
attachment 274841
[details]
Patch
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
Created
attachment 274845
[details]
Patch
Daniel Bates
Comment 6
2016-03-24 11:34:46 PDT
Created
attachment 274846
[details]
Patch
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
Committed
r198657
: <
http://trac.webkit.org/changeset/198657
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug