Issue 2763253002: Fixed external wpt csp img-src/* tests.

Issue 2763253002: Fixed external wpt csp img-src/* tests. (Closed)

Created:
3 years, 9 months ago by andypaicu

Modified:
3 years, 9 months ago

Reviewers:
Mike West, qyearsley

CC:
blink-reviews, blink-reviews-w3ctests_chromium.org, chromium-reviews

Target Ref:
refs/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

Fixed external wpt csp img-src/* tests. Fixed the existing test Moved over some tests from http/test/security/contentSecurityPolicy Un-skipped the img-src folder BUG=694525 Review-Url: https://codereview.chromium.org/2763253002 Cr-Commit-Position: refs/heads/master@{#459053} Committed: https://chromium.googlesource.com/chromium/src/+/a124c835aadb7c241e5b2bf8c06d758bef4b93fd

Patch Set 3 : Switched to placeholders instead of hardcoded URLs #

Total comments: 1

Created: 3 years, 9 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+162 lines, -127 lines)			Patch
M	third_party/WebKit/LayoutTests/TestExpectations	View	1	1 chunk	+1 line, -0 lines	0 comments	Download
D	third_party/WebKit/LayoutTests/external/wpt/content-security-policy/img-src/img-src-4_1.html	View		1 chunk	+0 lines, -46 lines	0 comments	Download
D	third_party/WebKit/LayoutTests/external/wpt/content-security-policy/img-src/img-src-4_1.html.sub.headers	View		1 chunk	+0 lines, -6 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/external/wpt/content-security-policy/img-src/img-src-4_1.sub.html	View	1 2	1 chunk	+41 lines, -0 lines	1 comment	Download
A	third_party/WebKit/LayoutTests/external/wpt/content-security-policy/img-src/img-src-full-host-wildcard-blocked.sub.html	View	1	1 chunk	+20 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/external/wpt/content-security-policy/img-src/img-src-host-partial-wildcard-allowed.sub.html	View	1	1 chunk	+20 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/external/wpt/content-security-policy/img-src/img-src-none-blocks.html	View		1 chunk	+20 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/external/wpt/content-security-policy/img-src/img-src-port-wildcard-allowed.sub.html	View	1	1 chunk	+20 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/external/wpt/content-security-policy/img-src/img-src-wildcard-allowed.html	View	1	1 chunk	+40 lines, -0 lines	0 comments	Download
D	third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/image-allowed.html	View		1 chunk	+0 lines, -13 lines	0 comments	Download
D	third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/image-allowed-expected.txt	View		1 chunk	+0 lines, -2 lines	0 comments	Download
D	third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/image-blocked.html	View		1 chunk	+0 lines, -14 lines	0 comments	Download
D	third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/image-blocked-alt-content.html	View		1 chunk	+0 lines, -10 lines	0 comments	Download
D	third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/image-blocked-expected.txt	View		1 chunk	+0 lines, -4 lines	0 comments	Download
D	third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/image-full-host-wildcard-fails.html	View		1 chunk	+0 lines, -13 lines	0 comments	Download
D	third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/image-full-host-wildcard-fails-expected.txt	View		1 chunk	+0 lines, -4 lines	0 comments	Download
D	third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/image-host-wildcard-allowed.html	View		1 chunk	+0 lines, -13 lines	0 comments	Download
D	third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/image-host-wildcard-allowed-expected.txt	View		1 chunk	+0 lines, -2 lines	0 comments	Download

Messages

Total messages: 27 (18 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

Mike West

Thanks! These are looking good! https://codereview.chromium.org/2763253002/diff/1/third_party/WebKit/LayoutTests/external/wpt/content-security-policy/img-src/img-src-4_1.sub.html File third_party/WebKit/LayoutTests/external/wpt/content-security-policy/img-src/img-src-4_1.sub.html (right): https://codereview.chromium.org/2763253002/diff/1/third_party/WebKit/LayoutTests/external/wpt/content-security-policy/img-src/img-src-4_1.sub.html#newcode38 third_party/WebKit/LayoutTests/external/wpt/content-security-policy/img-src/img-src-4_1.sub.html:38: </html> As long as ...

3 years, 9 months ago (2017-03-22 11:56:23 UTC) #7

Thanks! These are looking good!

https://codereview.chromium.org/2763253002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/external/wpt/content-security-policy/img-src/img-src-4_1.sub.html
(right):

https://codereview.chromium.org/2763253002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/content-security-policy/img-src/img-src-4_1.sub.html:38:
</html>
As long as you're rewriting the test, I'd suggest something like:

```
<!DOCTYPE html>
<script src='/resources/testharness.js'></script>
<script src='/resources/testharnessreport.js'></script>

<!-- Allow 'self' and www.web-platform.test -->
<meta http-equiv="Content-Security-Policy" content="img-src 'self'
{{domains[www]}}:{{ports[http][0]}}">

<script>
  async_test(t => {
    var i = new Image();
    i.onload = t.step_func_done(_ => {
      assert_equals([whatever pass.png's width is], i.naturalWidth);
      assert_equals([whatever pass.png's height is], i.naturalHeight);
    });
    i.onerror = t.unreached_func();
    i.src = '/content-security-policy/support/pass.png';
  }, "img-src for relative path should load");
</script>
(and so on)
```

This has the advantage of making each test self-contained, and reduces the
inconsistency between the tests (two rely on `<img>`, the third is all in JS).

https://codereview.chromium.org/2763253002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/external/wpt/content-security-policy/img-src/img-src-full-host-wildcard-blocked.sub.html
(right):

https://codereview.chromium.org/2763253002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/content-security-policy/img-src/img-src-full-host-wildcard-blocked.sub.html:15:
<img
src='http://127.0.0.1:{{ports[http][0]}}/content-security-policy/support/fail.png'
Nit: Please change both `127.0.0.1` instances to `{{host}}`.

https://codereview.chromium.org/2763253002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/content-security-policy/img-src/img-src-full-host-wildcard-blocked.sub.html:17:
onerror='t1.done();'>
Nit: I vaguely prefer the scripty version above, but it's up to you.

https://codereview.chromium.org/2763253002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/external/wpt/content-security-policy/img-src/img-src-host-partial-wildcard-allowed.sub.html
(right):

https://codereview.chromium.org/2763253002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/content-security-policy/img-src/img-src-host-partial-wildcard-allowed.sub.html:15:
<img
src='http://127.0.0.1:{{ports[http][0]}}/content-security-policy/support/pass.png'
I'd suggest `*.{{host}}` and `{{domains[www]}}` instead of `*.0.1` and
`127.0.0.1`. Honestly, if those pass in Chrome, then it seems like it has to be
a bug in our implementation. Wildcarding IP addresses makes no sense.

https://codereview.chromium.org/2763253002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/external/wpt/content-security-policy/img-src/img-src-port-wildcard-allowed.sub.html
(right):

https://codereview.chromium.org/2763253002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/content-security-policy/img-src/img-src-port-wildcard-allowed.sub.html:15:
<img
src='http://www.{{host}}:{{ports[http][0]}}/content-security-policy/support/pass.png'
s/www.{{host}}/{{domains[www]}}/

https://codereview.chromium.org/2763253002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/external/wpt/content-security-policy/img-src/img-src-wildcard-allowed.html
(right):

https://codereview.chromium.org/2763253002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/content-security-policy/img-src/img-src-wildcard-allowed.html:17:
onerror='t1.step(function() { assert_unreached("Image should have loaded");
t1.done(); });'>
Can you add a `blob:` test here? That shouldn't match `*` (but would match `*
blob:`).

andypaicu

https://codereview.chromium.org/2763253002/diff/1/third_party/WebKit/LayoutTests/external/wpt/content-security-policy/img-src/img-src-4_1.sub.html File third_party/WebKit/LayoutTests/external/wpt/content-security-policy/img-src/img-src-4_1.sub.html (right): https://codereview.chromium.org/2763253002/diff/1/third_party/WebKit/LayoutTests/external/wpt/content-security-policy/img-src/img-src-4_1.sub.html#newcode38 third_party/WebKit/LayoutTests/external/wpt/content-security-policy/img-src/img-src-4_1.sub.html:38: </html> On 2017/03/22 at 11:56:22, Mike West wrote: > ...

3 years, 9 months ago (2017-03-23 09:22:11 UTC) #10

https://codereview.chromium.org/2763253002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/external/wpt/content-security-policy/img-src/img-src-4_1.sub.html
(right):

https://codereview.chromium.org/2763253002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/content-security-policy/img-src/img-src-4_1.sub.html:38:
</html>
On 2017/03/22 at 11:56:22, Mike West wrote:
> As long as you're rewriting the test, I'd suggest something like:
> 
> ```
> <!DOCTYPE html>
> <script src='/resources/testharness.js'></script>
> <script src='/resources/testharnessreport.js'></script>
> 
> <!-- Allow 'self' and www.web-platform.test -->
> <meta http-equiv="Content-Security-Policy" content="img-src 'self'
{{domains[www]}}:{{ports[http][0]}}">
> 
> <script>
>   async_test(t => {
>     var i = new Image();
>     i.onload = t.step_func_done(_ => {
>       assert_equals([whatever pass.png's width is], i.naturalWidth);
>       assert_equals([whatever pass.png's height is], i.naturalHeight);
>     });
>     i.onerror = t.unreached_func();
>     i.src = '/content-security-policy/support/pass.png';
>   }, "img-src for relative path should load");
> </script>
> (and so on)
> ```
> 
> This has the advantage of making each test self-contained, and reduces the
inconsistency between the tests (two rely on `<img>`, the third is all in JS).

done

https://codereview.chromium.org/2763253002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/external/wpt/content-security-policy/img-src/img-src-full-host-wildcard-blocked.sub.html
(right):

https://codereview.chromium.org/2763253002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/content-security-policy/img-src/img-src-full-host-wildcard-blocked.sub.html:15:
<img
src='http://127.0.0.1:{{ports[http][0]}}/content-security-policy/support/fail.png'
On 2017/03/22 at 11:56:22, Mike West wrote:
> Nit: Please change both `127.0.0.1` instances to `{{host}}`.

done

https://codereview.chromium.org/2763253002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/external/wpt/content-security-policy/img-src/img-src-host-partial-wildcard-allowed.sub.html
(right):

https://codereview.chromium.org/2763253002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/content-security-policy/img-src/img-src-host-partial-wildcard-allowed.sub.html:15:
<img
src='http://127.0.0.1:{{ports[http][0]}}/content-security-policy/support/pass.png'
On 2017/03/22 at 11:56:22, Mike West wrote:
> I'd suggest `*.{{host}}` and `{{domains[www]}}` instead of `*.0.1` and
`127.0.0.1`. Honestly, if those pass in Chrome, then it seems like it has to be
a bug in our implementation. Wildcarding IP addresses makes no sense.

done

https://codereview.chromium.org/2763253002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/external/wpt/content-security-policy/img-src/img-src-port-wildcard-allowed.sub.html
(right):

https://codereview.chromium.org/2763253002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/content-security-policy/img-src/img-src-port-wildcard-allowed.sub.html:15:
<img
src='http://www.{{host}}:{{ports[http][0]}}/content-security-policy/support/pass.png'
On 2017/03/22 at 11:56:22, Mike West wrote:
> s/www.{{host}}/{{domains[www]}}/

done

https://codereview.chromium.org/2763253002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/external/wpt/content-security-policy/img-src/img-src-wildcard-allowed.html
(right):

https://codereview.chromium.org/2763253002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/content-security-policy/img-src/img-src-wildcard-allowed.html:17:
onerror='t1.step(function() { assert_unreached("Image should have loaded");
t1.done(); });'>
On 2017/03/22 at 11:56:22, Mike West wrote:
> Can you add a `blob:` test here? That shouldn't match `*` (but would match `*
blob:`).

done

Mike West

LGTM with 2 tiny nits. Thanks! https://codereview.chromium.org/2763253002/diff/20001/third_party/WebKit/LayoutTests/external/wpt/content-security-policy/img-src/img-src-4_1.sub.html File third_party/WebKit/LayoutTests/external/wpt/content-security-policy/img-src/img-src-4_1.sub.html (right): https://codereview.chromium.org/2763253002/diff/20001/third_party/WebKit/LayoutTests/external/wpt/content-security-policy/img-src/img-src-4_1.sub.html#newcode27 third_party/WebKit/LayoutTests/external/wpt/content-security-policy/img-src/img-src-4_1.sub.html:27: i.src = 'http://www1.web-platform.test/content-security-policy/support/fail.png'; ...

3 years, 9 months ago (2017-03-23 10:19:36 UTC) #11

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2763253002/40001

3 years, 9 months ago (2017-03-23 12:02:03 UTC) #20

commit-bot: I haz the power

Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/a124c835aadb7c241e5b2bf8c06d758bef4b93fd

3 years, 9 months ago (2017-03-23 12:08:39 UTC) #23

qyearsley

The upstream web-platform-tests continuous integration is currently rejecting this change because one test method in ...

3 years, 9 months ago (2017-03-23 18:00:41 UTC) #25

Mike Wittman

This is flaking on the Chrome build too. I'm going to revert.

3 years, 9 months ago (2017-03-23 18:36:46 UTC) #26

Mike Wittman

3 years, 9 months ago (2017-03-23 18:39:38 UTC) #27

Message was sent while issue was closed.

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/2768073004/ by wittman@chromium.org.

The reason for reverting is: Flaking on several Chrome builds. For example:
https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Win7...

08:08:14.766 2480 worker/4
external/wpt/content-security-policy/img-src/img-src-4_1.sub.html output stderr
lines:
08:08:14.766 2480   CONSOLE ERROR: Refused to load the image
'http://www1.web-platform.test/content-security-policy/support/fail.png' because
it violates the following Content Security Policy directive: "img-src 'self'
www.web-platform.test:8001".
08:08:14.766 2480   
08:08:14.780 2796 [18526/52077]
external/wpt/content-security-policy/img-src/img-src-4_1.sub.html failed
unexpectedly (asserts failed)
08:08:14.778 2480 worker/4
external/wpt/content-security-policy/img-src/img-src-4_1.sub.html failed:
08:08:14.778 2480 worker/4  asserts failed
.

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages