Closed Bug 1381744 Opened 2 years ago Closed 2 years ago

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

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: allstars.chh, Assigned: hchang)

References

Details

(Whiteboard: [domsecurity-active])

Attachments

(2 files)

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.
Priority: -- → P3
Assignee: nobody → 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.
Status: NEW → ASSIGNED
Priority: P3 → P2
Whiteboard: [domsecurity-backlog1] → [domsecurity-active]
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
Attachment #8894835 - Flags: review?(cam)
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 :)
See Also: → 1387983, 1388606
I can review the font one. :-)  I'll take a look tomorrow.
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?
Attachment #8894835 - Flags: review?(cam) → review-
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.
Attachment #8895757 - Flags: review?(cam)
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:jdaggett@mozilla.com">

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.
Attachment #8895757 - Flags: review?(cam) → review+
(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 :)
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 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!
Attachment #8894835 - Flags: review?(cam) → review+
Pushed by hchang@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/1cfbeeded8da
https://hg.mozilla.org/mozilla-central/rev/3dc3465a86b4
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.