Bug 155777

<rdar://problem/25304031>

The following is a small selection of error message text that we emit currently and we would emit with the proposed patch. Some details that we currently emit have been omitted because I felt that such details would be obvious given context (source file, line and column info - I plan to implement this for all messages in a subsequent commit(s)) or can be easily deduced. I am open to suggestions. 1. When CSP defines script-src and a script is blocked: Before: Refused to load the script 'http://127.0.0.1:8000/security/contentSecurityPolicy/resources/script.js' because it violates the following Content Security Policy directive: "script-src 'n". With proposed patch: Refused to load http://127.0.0.1:8000/security/contentSecurityPolicy/resources/script.js because it does not appear in the script-src directive of the Content Security Policy. 2. When CSP defines default-src and does not define script-src and script is blocked: Before: Refused to load the script 'http://127.0.0.1:8080/security/contentSecurityPolicy/resources/alert-fail.js' because it violates the following Content Security Policy directive: "default-src 'self'". Note that 'script-src' was not explicitly set, so 'default-src' is used as a fallback. With proposed patch: Refused to load http://127.0.0.1:8080/security/contentSecurityPolicy/resources/alert-fail.js because it appears in neither the script-src directive nor the default-src directive of the Content Security Policy. 3. When CSP defines script-src with a script hash and a script is blocked: Before: Refused to execute inline script because it violates the following Content Security Policy directive: "script-src 'self' 'sha256-rrdh0QCl46qqHxfnnk08ydh/rkhVi2JvD6DLuUP30MI='". With proposed patch: Refused to execute a script because its hash, its nonce, or 'unsafe-inline' does not appear in the script-src directive of the Content Security Policy. (Note, I plan to further improve this message in a subsequent patch by including source file, line and column information). 4. When CSP defines plugin-types and content is blocked: Before: Refused to load 'http://127.0.0.1:8000/plugins/resources/mock-plugin.pl' (MIME type 'application/x-webkit-test-netscape') because it violates the following Content Security Policy Directive: 'plugin-types text/plain'. With proposed patch: Refused to load http://127.0.0.1:8000/plugins/resources/mock-plugin.pl because its MIME type does not appear in the plugin-types directive of the Content Security Policy.

Created attachment 274703 [details] Patch

Comment on attachment 274703 [details] Patch Attachment 274703 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1023206 New failing tests: http/tests/security/contentSecurityPolicy/inline-event-handler-blocked-after-injecting-meta.html fast/images/image-error-event-not-firing.html inspector/debugger/csp-exceptions.html

Created attachment 274710 [details] Archive of layout-test-results from ews116 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-yosemite Platform: Mac OS X 10.10.5

Comment on attachment 274703 [details] Patch Attachment 274703 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1023229 New failing tests: http/tests/security/contentSecurityPolicy/inline-event-handler-blocked-after-injecting-meta.html fast/images/image-error-event-not-firing.html inspector/debugger/csp-exceptions.html

Created attachment 274711 [details] Archive of layout-test-results from ews105 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5

Comment on attachment 274703 [details] Patch Attachment 274703 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1023226 New failing tests: http/tests/security/contentSecurityPolicy/inline-event-handler-blocked-after-injecting-meta.html fast/images/image-error-event-not-firing.html

Created attachment 274713 [details] Archive of layout-test-results from ews125 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.10.5

Comment on attachment 274703 [details] Patch Attachment 274703 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1023254 New failing tests: http/tests/security/contentSecurityPolicy/inline-event-handler-blocked-after-injecting-meta.html fast/images/image-error-event-not-firing.html inspector/debugger/csp-exceptions.html

Created attachment 274714 [details] Archive of layout-test-results from ews100 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-yosemite Platform: Mac OS X 10.10.5

Created attachment 274724 [details] Patch Update expected results for tests http/tests/security/contentSecurityPolicy/inline-event-handler-blocked-after-injecting-meta.html fast/images/image-error-event-not-firing.html and inspector/debugger/csp-exceptions.html

Comment on attachment 274724 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=274724&action=review I like the direction here, but we should be even more "ruthless" about shared functions for repeated idioms to make it terse at each call site. > Source/WebCore/page/csp/ContentSecurityPolicyDirectiveList.cpp:195 > +static String consoleMessageForViolation(const String& effectiveDirective, const Optional<URL>& blockedURL, bool violatesDefaultSrc, const String& prefix, const String& subject = ASCIILiteral("it")) It’s nice to be clear that URL is optional with this interface, but URL already has both null and empty values, so I am not sure we need Optional; could just use { } to mean "no URL" instead of Nullopt. For any arguments that are always ASCII literals, we could make the code more efficient by using the type const char*, instead of String, which causes us to allocate and destroy a string every time. Instead StringBuilder::appendLiteral could be used. > Source/WebCore/page/csp/ContentSecurityPolicyDirectiveList.cpp:325 > + String consoleMessage = consoleMessageForViolation(scriptSrc, url, operativeDirective == m_defaultSrc.get(), ASCIILiteral("Refused to load")); > + reportViolation(operativeDirective->text(), scriptSrc, consoleMessage, url); Seems like we should have a helper function for this repeated idiom.

(In reply to comment #13) > > Source/WebCore/page/csp/ContentSecurityPolicyDirectiveList.cpp:195 > > +static String consoleMessageForViolation(const String& effectiveDirective, const Optional<URL>& blockedURL, bool violatesDefaultSrc, const String& prefix, const String& subject = ASCIILiteral("it")) > > It’s nice to be clear that URL is optional with this interface, but URL > already has both null and empty values, so I am not sure we need Optional; > could just use { } to mean "no URL" instead of Nullopt. > Will remove the use of Optional and only include a URL in the console message if URL.isEmpty() returns false. For violations that were triggered without a URL (e.g. CSP blocked the instantiating of a plugin: <object type="application/x-webkit-test-netscape"></object>) the error message text will change from: Refused to load because it does not appear in the object-src directive of the Content Security Policy. to Refused to load because it does not appear in the object-src directive of the Content Security Policy. Notice there is now only one space character between the words "load" and "because". I was using the extra space character to help make the error message less ambiguous between the time when this patch lands and when we fix bug #114317 and begin associating source file, line number, and column number information with each console message. I hope to fix bug #114317 shortly, the value afforded by distinguishing "no URL" from having a URL via the use of two space characters is minimal and is unlikely to come up much on real world sites. Both the current and new error messages are ambiguous until we fix bug #114317. We could use alternative wording for the "no URL" case but I would like to defer such a change until we first fix bug #114317 as I hope the associated source metadata will help bring context to such a violation. > For any arguments that are always ASCII literals, we could make the code > more efficient by using the type const char*, instead of String, which > causes us to allocate and destroy a string every time. Will use const char* instead of String for arguments prefix and subject. > Instead StringBuilder::appendLiteral could be used. > As far as I can tell, we cannot use StringBuilder::appendLiteral() unless we make consoleMessageForViolation() a template function because StringBuilder::appendLiteral() takes a const char [N] for some N not a const char*. On another note, I would expect making consoleMessageForViolation() a template function to increase code size (should I be concerned?). I hope you do not mind that I defer making consoleMessageForViolation() a template function for now. > > Source/WebCore/page/csp/ContentSecurityPolicyDirectiveList.cpp:325 > > + String consoleMessage = consoleMessageForViolation(scriptSrc, url, operativeDirective == m_defaultSrc.get(), ASCIILiteral("Refused to load")); > > + reportViolation(operativeDirective->text(), scriptSrc, consoleMessage, url); > > Seems like we should have a helper function for this repeated idiom. I hope you do not mind that I defer adding a helper function for this repeated code at this time as I plan to move all of this logic from class ContentSecurityPolicyDirectiveList.cpp to class ContentSecurityPolicy shortly so that it is closer to the script execution context, which can query for source file metadata. Once we move this code we can evaluate the best way to reduce duplication.

Comment on attachment 274724 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=274724&action=review > Source/WebCore/page/csp/ContentSecurityPolicyDirectiveList.cpp:224 > + String consoleMessage = consoleMessageForViolation(scriptSrc, Nullopt, operativeDirective == m_defaultSrc.get(), ASCIILiteral("Refused to execute a script"), ASCIILiteral("its hash, its nonce, or 'unsafe-inline'")); > + reportViolation(operativeDirective->text(), scriptSrc, consoleMessage, URL(), contextURL, contextLine); Instead of putting the URL in the message, it would be good to put as the location of the error. You could address that for bug 114317.

(In reply to comment #15) > Instead of putting the URL in the message, it would be good to put as the > location of the error. You could address that for bug 114317. I will look to do this as part of the fix for bug #114317.

Committed r198591: <http://trac.webkit.org/changeset/198591>