1381744 - Add web-platform tests for image, css and fonts for data: URI

Status: RESOLVED FIXED

(Core :: DOM: Security, enhancement, P2)

Description

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

As requested by BZ in https://bugzilla.mozilla.org/show_bug.cgi?id=1373513#c26
We need to have more web-platform tests for data: URIs.

Comment 1

[Henry Chang [:hchang]]

We already have some wpt test cases for data:image. What we lack is
for data:stylesheet and data:font. What I fixed in bug 1387983
(for data:stylesheet) should be easy to port since it doesn't
have any gecko-specific dependency.

Comment 2

[Henry Chang [:hchang]]

Attachment: Bug 1381744 - Extend current web-platform test to do data:css same-origin check.

Review commit: https://reviewboard.mozilla.org/r/165992/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/165992/

Comment 3

[Henry Chang [:hchang]]

I've found an existing web-platform test [1] which can be extended to
test if a data:stylesheet is the same-origin resource.

As for the "font" test, I didn't find relevant one so I might have to write
a new test case.

[1] http://searchfox.org/mozilla-central/source/testing/web-platform/tests/cssom/stylesheet-same-origin.sub.html

Comment 4

[Henry Chang [:hchang]]

Attachment: Bug 1381744 - New web-platform test case for data:font same-origin check.

Review commit: https://reviewboard.mozilla.org/r/167088/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/167088/

Comment 5

[Henry Chang [:hchang]]

Comment on attachment 8894835 [details]
Bug 1381744 - Extend current web-platform test to do data:css same-origin check.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/165992/diff/1-2/

Comment 6

[Henry Chang [:hchang]]

Hey Cameron,

Similar to bug 1387983 which you have just reviewed, this patch extends
current wpt css test case to do the data:css same-origin check. Could you
also review this patch? Thanks :)

Besides, there is another patch also for the same-origin check but for data:font.
I used to ask Christoph for review but he's on PTO. Do you think who is familiar
with css:font enough to have it a review?

Thanks :)

Comment 7

[Cameron McCormack (:heycam)]

I can review the font one. :-)  I'll take a look tomorrow.

Comment 8

[Cameron McCormack (:heycam)]

Comment on attachment 8894835 [details]
Bug 1381744 - Extend current web-platform test to do data:css same-origin check.

https://reviewboard.mozilla.org/r/165992/#review172576

The intent of the patch is fine, but a couple of changes below would be good.

::: testing/web-platform/tests/cssom/stylesheet-same-origin.sub.html:11
(Diff revision 2)
>      <script src="/resources/testharness.js"></script>
>      <script src="/resources/testharnessreport.js"></script>
>  
>      <link id="crossorigin" href="http://www1.{{host}}:{{ports[http][1]}}/stylesheet-same-origin.css" rel="stylesheet">
>      <link id="sameorigin" href="stylesheet-same-origin.css" rel="stylesheet">
> +    <link id="datacss" href="data:text/css,.green-text{color:rgb(0, 255, 0)}" rel="stylesheet">

Nit: I think "sameorigindata" might be a better id, to be consistent with the other two.

::: testing/web-platform/tests/cssom/stylesheet-same-origin.sub.html:16
(Diff revision 2)
> +    <link id="datacss" href="data:text/css,.green-text{color:rgb(0, 255, 0)}" rel="stylesheet">
>  
>      <script>
>          var crossorigin = document.getElementById("crossorigin").sheet;
>          var sameorigin = document.getElementById("sameorigin").sheet;
> +        var datacss = document.getElementById("sameorigin").sheet;

Wrong id used in here...

::: testing/web-platform/tests/cssom/stylesheet-same-origin.sub.html:37
(Diff revision 2)
>          test(function() {
>              assert_equals(sameorigin.cssRules.length, 1, "Same origin stylesheet.cssRules should be accessible.");
>              sameorigin.insertRule("#test { margin: 10px; }", 1);
>              assert_equals(sameorigin.cssRules.length, 2, "Same origin stylesheet.insertRule should be accessible.");
>              sameorigin.deleteRule(0);
>              assert_equals(sameorigin.cssRules.length, 1, "Same origin stylesheet.deleteRule should be accessible.");
> +            assert_equals(datacss.cssRules.length, 1, "data:css stylesheet.cssRules should be accessible.");
>          }, "Origin-clean check in same-origin CSSOM Stylesheets");

I think it would be nice to run the same tests on both same origin style sheets.  Can you make this loop over sameorigin and datacss (or sameorigindata, if you rename the variable) to do the length/insertRule/deleteRule checks on both?

Comment 9

[Cameron McCormack (:heycam)]

Comment on attachment 8894835 [details]
Bug 1381744 - Extend current web-platform test to do data:css same-origin check.

https://reviewboard.mozilla.org/r/165992/#review172580

Also I think you might need to run the WPT manifest update script again after your changes, so it updates the hashes in the manifest file.

Comment 10

[Cameron McCormack (:heycam)]

Comment on attachment 8895757 [details]
Bug 1381744 - New web-platform test case for data:font same-origin check.

https://reviewboard.mozilla.org/r/167088/#review172584

This looks good.  By the way, since you are relying on the Font Loading API, you might be able to do the same test a bit more directly, like:

  var face = new FontFace("TestFont", "url(data:...)");
  face.load().then(function() { /* success */ }).catch(function() { /* fail */ })

If you wanted to avoid relying on the Font Loading API, I guess you could do something like check the size of the <p> element, assuming you know something about the font you have there.  But you'd need to do some looping or something because without the Font Loading API, there's no good way to know when the font has loaded.  (And while for Gecko data: URL fonts are loaded synchronously, I don't think that's required, and I'm not sure what other browsers do.)

Anyway, what you've got here is fine. :-)

::: testing/web-platform/tests/css/css-fonts-3/test_datafont_same_origin.html:6
(Diff revision 1)
> +<!DOCTYPE HTML>
> +<html>
> +<head>
> +  <meta charset=utf-8>
> +  <title>data:font same-origin test</title>
> +  <link rel="author" title="Herny Chang" href="mailto:[e-mail]">

Nit: "Henry", and update the email address. :-)

::: testing/web-platform/tests/css/css-fonts-3/test_datafont_same_origin.html:26
(Diff revision 1)
> +<style type="text/css" id="testbox"></style>
> +
> +<script type="text/javascript">
> +  async_test(function(t) {
> +    var text = document.createElement('p');
> +      // Cross-domain font will not load according to [1] so we try to apply

Nit: Indenting from here onwards is wrong.

Comment 11

[Henry Chang [:hchang]]

(In reply to Cameron McCormack (:heycam) from comment #10)
> Comment on attachment 8895757 [details]
> Bug 1381744 - New web-platform test case for data:font same-origin check.
> 
> https://reviewboard.mozilla.org/r/167088/#review172584
> 
> This looks good.  By the way, since you are relying on the Font Loading API,
> you might be able to do the same test a bit more directly, like:
> 
>   var face = new FontFace("TestFont", "url(data:...)");
>   face.load().then(function() { /* success */ }).catch(function() { /* fail
> */ })
> 
> If you wanted to avoid relying on the Font Loading API, I guess you could do
> something like check the size of the <p> element, assuming you know
> something about the font you have there.  But you'd need to do some looping
                                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> or something because without the Font Loading API, there's no good way to
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> know when the font has loaded.  (And while for Gecko data: URL fonts are
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

  That's the main reason which led me to using Font API :)
  So, do you think it's okay to use Font API given that I am writing
  web-platform test but not gecko-specific test? Should I specify
  any API dependency or move to different directory?

  Anyways, thanks for you review :)

Comment 12

[Cameron McCormack (:heycam)]

Nah, I think it's fine to rely on.  I'm not sure if there is any specific way to annotate that the test relies on this API.  Someone will fix it if it's needed, I guess. :)

Comment 13

[Henry Chang [:hchang]]

Comment on attachment 8895757 [details]
Bug 1381744 - New web-platform test case for data:font same-origin check.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167088/diff/1-2/

Comment 14

[Henry Chang [:hchang]]

Comment on attachment 8894835 [details]
Bug 1381744 - Extend current web-platform test to do data:css same-origin check.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/165992/diff/2-3/

Comment 15

[Cameron McCormack (:heycam)]

Comment on attachment 8894835 [details]
Bug 1381744 - Extend current web-platform test to do data:css same-origin check.

https://reviewboard.mozilla.org/r/165992/#review172640

Looks good, thanks!

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1cfbeeded8da
New web-platform test case for data:font same-origin check. r=heycam
https://hg.mozilla.org/integration/autoland/rev/3dc3465a86b4
Extend current web-platform test to do data:css same-origin check. r=heycam

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/1cfbeeded8da
https://hg.mozilla.org/mozilla-central/rev/3dc3465a86b4