Bug 158811 – CSP: Content Security Policy should allow '*' to match the originating page's scheme

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Jiewen Tan]

CSP: Content Security Policy should allow '*' to match the originating page's scheme

[Jiewen Tan]

Created attachment 281389 [details]
Patch

[Jiewen Tan]

<rdar://problem/26819568>

[Daniel Bates]

Comment on attachment 281389 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=281389&action=review

> LayoutTests/security/content-security-policy-matches-star-with-originator-scheme.html:8
> +    <meta http-equiv="Content-Security-Policy" content="script-src * ">

Nit: There is trailing whitespace after '*'

> LayoutTests/security/content-security-policy-matches-star-with-originator-scheme.html:11
> +<p>Test passes if the javascript src is loaded.</p>

Nit: "javascript src" => "external JavaScript script"

> LayoutTests/security/content-security-policy-matches-star-with-originator-scheme.html:12
> +<script src="resources/append-body.js"></script>

Can we make use of an existing script instead of adding a new script, append-body.js?

[Build Bot]

Comment on attachment 281389 [details]
Patch

Attachment 281389 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1509373

New failing tests:
fast/dom/HTMLImageElement/image-with-file-url-blocked-by-csp-img-src-star.html
fast/dom/HTMLLinkElement/link-with-file-url-blocked-by-csp-style-src-star.html
media/video-with-file-url-blocked-by-csp-media-src-star.html

[Build Bot]

Created attachment 281395 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 281389 [details]
Patch

Attachment 281389 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1509377

New failing tests:
fast/dom/HTMLImageElement/image-with-file-url-blocked-by-csp-img-src-star.html
fast/dom/HTMLLinkElement/link-with-file-url-blocked-by-csp-style-src-star.html
media/video-with-file-url-blocked-by-csp-media-src-star.html

[Build Bot]

Created attachment 281396 [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

[Build Bot]

Comment on attachment 281389 [details]
Patch

Attachment 281389 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1509379

New failing tests:
fast/dom/HTMLImageElement/image-with-file-url-blocked-by-csp-img-src-star.html
fast/dom/HTMLLinkElement/link-with-file-url-blocked-by-csp-style-src-star.html

[Build Bot]

Created attachment 281397 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.4

[Build Bot]

Comment on attachment 281389 [details]
Patch

Attachment 281389 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1509380

New failing tests:
fast/dom/HTMLImageElement/image-with-file-url-blocked-by-csp-img-src-star.html
fast/dom/HTMLLinkElement/link-with-file-url-blocked-by-csp-style-src-star.html
media/video-with-file-url-blocked-by-csp-media-src-star.html

[Build Bot]

Created attachment 281398 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Jiewen Tan]

Comment on attachment 281389 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=281389&action=review

Thanks Dan for reviewing my patch!

>> LayoutTests/security/content-security-policy-matches-star-with-originator-scheme.html:8
>> +    <meta http-equiv="Content-Security-Policy" content="script-src * ">
> 
> Nit: There is trailing whitespace after '*'

Fixed.

>> LayoutTests/security/content-security-policy-matches-star-with-originator-scheme.html:11
>> +<p>Test passes if the javascript src is loaded.</p>
> 
> Nit: "javascript src" => "external JavaScript script"

Fixed.

>> LayoutTests/security/content-security-policy-matches-star-with-originator-scheme.html:12
>> +<script src="resources/append-body.js"></script>
> 
> Can we make use of an existing script instead of adding a new script, append-body.js?

I don't see any usable javascript script inside the resources folder. Hence, I think there is no choice but to have append-body.js.

[Jiewen Tan]

Created attachment 281445 [details]
Patch

[Daniel Bates]

Comment on attachment 281445 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=281445&action=review

> LayoutTests/ChangeLog:17
> +        * fast/dom/HTMLImageElement/image-with-file-url-allowed-by-csp-img-src-star-expected.html: Renamed from LayoutTests/fast/dom/HTMLImageElement/image-with-file-url-blocked-by-csp-img-src-star-expected.html.
> +        * fast/dom/HTMLImageElement/image-with-file-url-allowed-by-csp-img-src-star.html: Renamed from LayoutTests/fast/dom/HTMLImageElement/image-with-file-url-blocked-by-csp-img-src-star.html.
> +        * fast/dom/HTMLLinkElement/link-with-file-url-allowed-by-csp-style-src-star-expected.html: Renamed from LayoutTests/fast/dom/HTMLLinkElement/link-with-file-url-blocked-by-csp-style-src-star-expected.html.
> +        * fast/dom/HTMLLinkElement/link-with-file-url-allowed-by-csp-style-src-star.html: Renamed from LayoutTests/fast/dom/HTMLLinkElement/link-with-file-url-blocked-by-csp-style-src-star.html.
> +        * media/video-with-file-url-allowed-by-csp-media-src-star-expected.html: Copied from LayoutTests/media/video-with-file-url-blocked-by-csp-media-src-star.html.
> +        * media/video-with-file-url-allowed-by-csp-media-src-star.html: Renamed from LayoutTests/media/video-with-file-url-blocked-by-csp-media-src-star.html.
> +        * media/video-with-file-url-blocked-by-csp-media-src-star-expected.html: Removed.
> +        * security/content-security-policy-matches-star-with-originator-scheme-expected.txt: Added.
> +        * security/content-security-policy-matches-star-with-originator-scheme.html: Added.

I suggest that we create a new directory hierarchy LayoutTests/security/contentSecurityPolicy and move all these tests into LayoutTests/security/contentSecurityPolicy I choose LayoutTests/security/contentSecurityPolicy so as to more closely match the directory hierarchy we use for HTTP CSP tests in LayoutTests/http/tests/security/contentSecurityPolicy.

> LayoutTests/fast/dom/HTMLImageElement/image-with-file-url-allowed-by-csp-img-src-star-expected.html:5
> +<img src="resources/green.png" width="128" height="128" alt="FAIL">

I suggest that we remove the HTML attribute alt to make this test more robust and less error prone. This will prevent the test from passing in the unlikely event that resources/green.png is removed or this file is modified such that the src attribute has a typo among other issues.

> LayoutTests/fast/dom/HTMLImageElement/image-with-file-url-allowed-by-csp-img-src-star.html:1
> +<!DOCTYPE html>

If we choose to move this file to LayoutTests/security/contentSecurityPolicy then I suggest that remove "csp" from the name of this file because its ambient directory name conveys that this is a Content Security Policy test and call this file image-with-file-url-allowed-by-img-src-star.html.

> LayoutTests/fast/dom/HTMLLinkElement/link-with-file-url-allowed-by-csp-style-src-star.html:1
> +<!DOCTYPE html>

If we choose to move this file to LayoutTests/security/contentSecurityPolicy then I suggest that remove "csp" from the name of this file because its ambient directory name conveys that this is a Content Security Policy test and call this file link-with-file-url-allowed-by-style-src-star.html.

> LayoutTests/fast/dom/HTMLLinkElement/link-with-file-url-allowed-by-csp-style-src-star.html:15
> +<p>This tests that loading a stylesheet with a file URL is allowed when the page that is loaded from file URL has Content Security Policy &quot;style-src *&quot;. This test PASSED if you see a red square below. Otherwise, it FAILED.</p>

We tend to follow the recommendations of the CSS2.1 Test Case Authoring Guidelines, <http://www.w3.org/Style/CSS/Test/guidelines.html>, when writing layout tests and indicate success and failure by using the color green [1] and red [2], respectively. Therefore, we should indicate that the test PASSED if you see a green square and failure should be indicated by a red square. Towards this we can make use of the external stylesheet <http://trac.webkit.org/browser/trunk/LayoutTests/fast/dom/HTMLLinkElement/resources/green-background-color.css> and initially set the background color of <div id="test"> to red.

[1] <http://www.w3.org/Style/CSS/Test/guidelines.html#indicating>
[2] <http://www.w3.org/Style/CSS/Test/guidelines.html#indicating0>

> LayoutTests/media/video-with-file-url-allowed-by-csp-media-src-star.html:1
> +<!DOCTYPE html>

If we choose to move this file to LayoutTests/security/contentSecurityPolicy then I suggest that remove "csp" from the name of this file because its ambient directory name conveys that this is a Content Security Policy test and call this file video-with-file-url-allowed-by-media-src-star.html.

> LayoutTests/media/video-with-file-url-allowed-by-csp-media-src-star.html:36
> +<p>This tests that loading a video with a file URL is allowed when the page that is loaded from file URL has Content Security Policy &quot;media-src *&quot;. This test PASSED if you don't see a solid green square. Otherwise, it FAILED.</p>

As aforementioned we should use the color red to signify failure instead of green.

> LayoutTests/security/content-security-policy-matches-star-with-originator-scheme.html:1
> +<!DOCTYPE html>

I would name this file script-with-file-url-allowed-by-script-src-star.html and put this file in LayoutTests/security/contentSecurityPolicy.

> LayoutTests/security/content-security-policy-matches-star-with-originator-scheme.html:8
> +    <script>
> +    if (window.testRunner)
> +        testRunner.dumpAsText();
> +    </script>
> +    <meta http-equiv="Content-Security-Policy" content="script-src *">

Minor: The indentation for the children of the HTML head element differs from the indentation used for children of the HTML body element. I suggest that we do not indent the children of the HTML head element. Regardless, we should use a consistent indentation style.

> LayoutTests/security/resources/append-body.js:1
> +document.body.innerHTML += '<p>Loaded. Test passes.</p>';

I would name this file alert-pass.js to match the name of file of the same purpose in LayoutTests/http/tests/security/contentSecurityPolicy/resources and I would change the implementation to:

alert("PASS");

[Jiewen Tan]

Created attachment 281483 [details]
Patch

[Jiewen Tan]

Comment on attachment 281445 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=281445&action=review

>> LayoutTests/ChangeLog:17
>> +        * security/content-security-policy-matches-star-with-originator-scheme.html: Added.
> 
> I suggest that we create a new directory hierarchy LayoutTests/security/contentSecurityPolicy and move all these tests into LayoutTests/security/contentSecurityPolicy I choose LayoutTests/security/contentSecurityPolicy so as to more closely match the directory hierarchy we use for HTTP CSP tests in LayoutTests/http/tests/security/contentSecurityPolicy.

Fixed.

>> LayoutTests/fast/dom/HTMLImageElement/image-with-file-url-allowed-by-csp-img-src-star-expected.html:5
>> +<img src="resources/green.png" width="128" height="128" alt="FAIL">
> 
> I suggest that we remove the HTML attribute alt to make this test more robust and less error prone. This will prevent the test from passing in the unlikely event that resources/green.png is removed or this file is modified such that the src attribute has a typo among other issues.

Fixed.

>> LayoutTests/fast/dom/HTMLImageElement/image-with-file-url-allowed-by-csp-img-src-star.html:1
>> +<!DOCTYPE html>
> 
> If we choose to move this file to LayoutTests/security/contentSecurityPolicy then I suggest that remove "csp" from the name of this file because its ambient directory name conveys that this is a Content Security Policy test and call this file image-with-file-url-allowed-by-img-src-star.html.

Fixed.

>> LayoutTests/fast/dom/HTMLLinkElement/link-with-file-url-allowed-by-csp-style-src-star.html:1
>> +<!DOCTYPE html>
> 
> If we choose to move this file to LayoutTests/security/contentSecurityPolicy then I suggest that remove "csp" from the name of this file because its ambient directory name conveys that this is a Content Security Policy test and call this file link-with-file-url-allowed-by-style-src-star.html.

Fixed.

>> LayoutTests/fast/dom/HTMLLinkElement/link-with-file-url-allowed-by-csp-style-src-star.html:15
>> +<p>This tests that loading a stylesheet with a file URL is allowed when the page that is loaded from file URL has Content Security Policy &quot;style-src *&quot;. This test PASSED if you see a red square below. Otherwise, it FAILED.</p>
> 
> We tend to follow the recommendations of the CSS2.1 Test Case Authoring Guidelines, <http://www.w3.org/Style/CSS/Test/guidelines.html>, when writing layout tests and indicate success and failure by using the color green [1] and red [2], respectively. Therefore, we should indicate that the test PASSED if you see a green square and failure should be indicated by a red square. Towards this we can make use of the external stylesheet <http://trac.webkit.org/browser/trunk/LayoutTests/fast/dom/HTMLLinkElement/resources/green-background-color.css> and initially set the background color of <div id="test"> to red.
> 
> [1] <http://www.w3.org/Style/CSS/Test/guidelines.html#indicating>
> [2] <http://www.w3.org/Style/CSS/Test/guidelines.html#indicating0>

Fixed.

>> LayoutTests/media/video-with-file-url-allowed-by-csp-media-src-star.html:1
>> +<!DOCTYPE html>
> 
> If we choose to move this file to LayoutTests/security/contentSecurityPolicy then I suggest that remove "csp" from the name of this file because its ambient directory name conveys that this is a Content Security Policy test and call this file video-with-file-url-allowed-by-media-src-star.html.

Fixed.

>> LayoutTests/media/video-with-file-url-allowed-by-csp-media-src-star.html:36
>> +<p>This tests that loading a video with a file URL is allowed when the page that is loaded from file URL has Content Security Policy &quot;media-src *&quot;. This test PASSED if you don't see a solid green square. Otherwise, it FAILED.</p>
> 
> As aforementioned we should use the color red to signify failure instead of green.

Fixed.

>> LayoutTests/security/content-security-policy-matches-star-with-originator-scheme.html:1
>> +<!DOCTYPE html>
> 
> I would name this file script-with-file-url-allowed-by-script-src-star.html and put this file in LayoutTests/security/contentSecurityPolicy.

Fixed.

>> LayoutTests/security/content-security-policy-matches-star-with-originator-scheme.html:8
>> +    <meta http-equiv="Content-Security-Policy" content="script-src *">
> 
> Minor: The indentation for the children of the HTML head element differs from the indentation used for children of the HTML body element. I suggest that we do not indent the children of the HTML head element. Regardless, we should use a consistent indentation style.

Fixed

>> LayoutTests/security/resources/append-body.js:1
>> +document.body.innerHTML += '<p>Loaded. Test passes.</p>';
> 
> I would name this file alert-pass.js to match the name of file of the same purpose in LayoutTests/http/tests/security/contentSecurityPolicy/resources and I would change the implementation to:
> 
> alert("PASS");

Fixed

[Daniel Bates]

Comment on attachment 281483 [details]
Patch

Please rebase this patch after landing the fix for bug #158859.

[Jiewen Tan]

Created attachment 281518 [details]
Patch for landing

[Build Bot]

Comment on attachment 281518 [details]
Patch for landing

Attachment 281518 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1515563

New failing tests:
security/contentSecurityPolicy/video-with-file-url-allowed-by-media-src-star.html

[Build Bot]

Created attachment 281522 [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

[Jiewen Tan]

Committed r202155: <http://trac.webkit.org/changeset/202155>

[Ryan Haddad]

(In reply to comment #19)
> Comment on attachment 281518 [details]
> Patch for landing
> 
> Attachment 281518 [details] did not pass mac-wk2-ews (mac-wk2):
> Output: http://webkit-queues.webkit.org/results/1515563
> 
> New failing tests:
> security/contentSecurityPolicy/video-with-file-url-allowed-by-media-src-star.
> html

This test is frequently failing on the bots

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=security%2FcontentSecurityPolicy%2Fvideo-with-file-url-allowed-by-media-src-star.html