[EME] Modify mochitest and WPT test to run in https before removing support for EME on insecure contexts.

RESOLVED FIXED in Firefox 58

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: JamesCheng, Assigned: JamesCheng)

Tracking

(Blocks 2 bugs)

unspecified
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(5 attachments)

Assignee

Description

2 years ago
If we make the EME WebAPI HTTPS only, I think the mochitest and WPT test will be affected.

So, I need to make some change before Bug 1322517

For mochitest,
We could do some change in .ini file like 
========================
scheme=https
[test_eme_xxxx.html]
========================

But for WPT test, I need to ask the expert what change should I need to take care.
Assignee

Comment 1

2 years ago
Hi jgraham,

Based on Bug 1322517, the EME API will turn into https-only someday...

I foresee that the WPT test might be broken due to the change.(There are some htmls using this kind of API)

I would like to ask you what should I do to deal that the WebAPI is only be executed by https in WPT test?

I saw the wiki[1] and there is nothing talking about this...

I see some file names in WPT test is end up with `xxx.https.html`

But I don't know how to handle this kind of change in WPT. 

Could you please give me some example bugs or information that I can take reference?

Thank you!

[1]
https://developer.mozilla.org/en-US/docs/Mozilla/QA/web-platform-tests
Flags: needinfo?(james)
wpt indeed loads all tests with .https. in the filename over HTTPS.

It perhaps isn't very obvious, but this is documented at [1].

So basically you need to rename the relevant tests, and then apply the same change to the metadata in testing/web-platform/meta (note that you need to also adjust the filename inside those files, not just move the files).

Does that answer your questions?

[1] http://web-platform-tests.org/writing-tests/general-guidelines.html
Flags: needinfo?(james)
Assignee

Comment 3

2 years ago
Thanks for your quick response.

Let me make sure if I understand correctly.

For example,

I wanna change from `clearkey-events-session-closed-event.html` to `clearkey-events-session-closed-event.https.html`

I would also change the filename from
`clearkey-events-session-closed-event.html.ini` to `clearkey-events-session-closed-event.https.html.ini`

and I also need to change the content [1] from`clearkey-events-session-closed-event.html` to `clearkey-events-session-closed-event.https.html`.

If above step is correct,
then

1. Do I set reviewer to you for this change?
2. Can I land this change to gecko code base?
3. If I can land this code, who needs to pull into upstream? Or any other steps that I should do?


Many thanks.



[1]
http://searchfox.org/mozilla-central/rev/423b2522c48e1d654e30ffc337164d677f934ec3/testing/web-platform/meta/encrypted-media/clearkey-events-session-closed-event.html.ini#1
Flags: needinfo?(james)
Assignee

Comment 4

2 years ago
Hi Chris,

I noticed that the upstream

https://github.com/w3c/web-platform-tests/tree/master/encrypted-media

the file name is without `.https.html`

but Chrome has already removed the insecure context in their 58 https://www.chromestatus.com/feature/5724389932793856

Why they don't PR for the change to the upstream?

Just like other https-only API

https://github.com/w3c/web-platform-tests/tree/master/WebCryptoAPI/derive_bits_keys

Do you know the working flow for this kind of change?

Thanks.
Flags: needinfo?(cpearce)
Those steps are correct. If we land it in mozilla-central it will be automatically upstreamed. I'll review the changes if you like.

It's possible that Chrome aren't running these tests or fail them for unrelated reasons; I can ask.
Flags: needinfo?(james)
Assignee

Updated

2 years ago
Attachment #8924822 - Flags: review?(kikuo)
Assignee

Comment 7

2 years ago
I think it's not harmful to land the patches before bug 1322517. Actually, we need to land first that we can make EME API https-only without making the treeherder failure.

Hi kilik, please help to review the part1 first then I will r? cpearce after you reviewed.

I will start to write the remaining works for WPT test.

Thanks jgraham for your feedback.

Comment 9

2 years ago
mozreview-review
Comment on attachment 8924822 [details]
Bug 1413427 - Part1 - Make Mochitest runs EME with https.

https://reviewboard.mozilla.org/r/196058/#review201290

::: dom/media/test/eme.js:152
(Diff revision 1)
>  }
>  
>  function MaybeCrossOriginURI(test, uri)
>  {
>    if (test.crossOrigin) {
> -    return "http://test2.mochi.test:8888/tests/dom/media/test/allowed.sjs?" + uri;
> +    return "https://example.com/tests/dom/media/test/allowed.sjs?" + uri;

Not sure if there's any specific difference for the request handler dealing with this url w/ or w/o port number.
According to the comment [1], I'd suggest to add the port number.

[1] http://searchfox.org/mozilla-central/rev/af86a58b157fbed26b0e86fcd81f1b421e80e60a/build/pgo/server-locations.txt#19-20,100

::: dom/media/test/test_eme_autoplay.html:8
(Diff revision 1)
>  <head>
>    <title>Test Encrypted Media Extensions</title>
>    <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
>    <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
>    <script type="text/javascript" src="manifest.js"></script>
> -  <script type="text/javascript" src="http://test1.mochi.test:8888/tests/dom/media/test/eme.js"></script>
> +  <script type="text/javascript" src="https://example.com/tests/dom/media/test/eme.js"></script>

ditto

::: dom/media/test/test_eme_playback.html:8
(Diff revision 1)
>  <head>
>    <title>Test Encrypted Media Extensions</title>
>    <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
>    <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
>    <script type="text/javascript" src="manifest.js"></script>
> -  <script type="text/javascript" src="http://test1.mochi.test:8888/tests/dom/media/test/eme.js"></script>
> +  <script type="text/javascript" src="https://example.com/tests/dom/media/test/eme.js"></script>

ditto

::: dom/media/test/test_eme_waitingforkey.html:8
(Diff revision 1)
>  <head>
>    <title>Test Encrypted Media Extensions</title>
>    <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
>    <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
>    <script type="text/javascript" src="manifest.js"></script>
> -  <script type="text/javascript" src="http://test1.mochi.test:8888/tests/dom/media/test/eme.js"></script>
> +  <script type="text/javascript" src="https://example.com/tests/dom/media/test/eme.js"></script>

ditto
Attachment #8924822 - Flags: review?(kikuo) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8924912 [details]
Bug 1413427 - Part2 - Change the file extension from .html to .https.html.

https://reviewboard.mozilla.org/r/196178/#review201356

Why did you delete  testing/web-platform/tests/encrypted-media/encrypted-media-default-feature-policy.https.sub.html?
Attachment #8924912 - Flags: review?(james) → review-
Comment on attachment 8924913 [details]
Bug 1413427 - Part3 - Change the file extension from .html.ini to .https.html.ini.

https://reviewboard.mozilla.org/r/196180/#review201358
Attachment #8924913 - Flags: review?(james) → review+
Comment on attachment 8924914 [details]
Bug 1413427 - Part4 - Rename the content of the .ini file and update the MANIFEST.JSON files by command.

https://reviewboard.mozilla.org/r/196182/#review201360

Assuming this all passes try. I didn't check if the tests themselves reference any of the files you renamed.
Attachment #8924914 - Flags: review?(james) → review+
Comment hidden (mozreview-request)
Assignee

Comment 22

2 years ago
(In reply to James Graham [:jgraham] from comment #17)
> Comment on attachment 8924912 [details]
> Bug 1413427 - Part2 - Change the file extension from .html to .https.html.
> 
> https://reviewboard.mozilla.org/r/196178/#review201356
> 
> Why did you delete 
> testing/web-platform/tests/encrypted-media/encrypted-media-default-feature-
> policy.https.sub.html?

Oops, it is accidentally merged into Part3....I will adjust and put it back to Part2.

Thanks.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8924912 [details]
Bug 1413427 - Part2 - Change the file extension from .html to .https.html.

https://reviewboard.mozilla.org/r/196178/#review201404

This file already has .https. in the url. r+ assuming that you revert this and any other changes that add .https. to the url twice.
Attachment #8924912 - Flags: review?(james) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Attachment #8924915 - Flags: review?(bzbarsky)

Comment 32

2 years ago
mozreview-review
Comment on attachment 8924915 [details]
Bug 1413427 - Part5 - Fix cross origin test failure.

https://reviewboard.mozilla.org/r/196184/#review201578

::: commit-message-6a2ee:5
(Diff revision 5)
> +Bug 1413427 - Part5 - Fix cross origin test failure.
> +
> +Bug 1322517 will make the EME APIs only run on secure context.
> +This bug will try to make WPT test run with https.
> +Therefore, navigator.requestMediaKeySystemAccess will not be recognized in the original code since it regards as cross-origin access.

I don't understand what this comment is trying to say...

In this testcase, the iframe runs in a sandbox, so it shouldn't matter too much whether it's loaded as a data: src or a sandbox... why is this change needed?
Attachment #8924915 - Flags: review?(bzbarsky)
(In reply to James Cheng[:JamesCheng] from comment #4)
> Hi Chris,
> 
> I noticed that the upstream
> 
> https://github.com/w3c/web-platform-tests/tree/master/encrypted-media
> 
> the file name is without `.https.html`
> 
> but Chrome has already removed the insecure context in their 58
> https://www.chromestatus.com/feature/5724389932793856
> 
> Why they don't PR for the change to the upstream?
> 
> Just like other https-only API
> 
> https://github.com/w3c/web-platform-tests/tree/master/WebCryptoAPI/
> derive_bits_keys
> 
> Do you know the working flow for this kind of change?
> 
> Thanks.


I don't know. Maybe they don't run the WPT in their CI?
Flags: needinfo?(cpearce)

Comment 34

2 years ago
mozreview-review
Comment on attachment 8924822 [details]
Bug 1413427 - Part1 - Make Mochitest runs EME with https.

https://reviewboard.mozilla.org/r/196058/#review201706
Attachment #8924822 - Flags: review?(cpearce) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 40

2 years ago
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #32)
> Comment on attachment 8924915 [details]
> Bug 1413427 - Part5 - Fix cross origin test failure.
> 
> https://reviewboard.mozilla.org/r/196184/#review201578
> 
> ::: commit-message-6a2ee:5
> (Diff revision 5)
> > +Bug 1413427 - Part5 - Fix cross origin test failure.
> > +
> > +Bug 1322517 will make the EME APIs only run on secure context.
> > +This bug will try to make WPT test run with https.
> > +Therefore, navigator.requestMediaKeySystemAccess will not be recognized in the original code since it regards as cross-origin access.
> 
> I don't understand what this comment is trying to say...
> 
> In this testcase, the iframe runs in a sandbox, so it shouldn't matter too
> much whether it's loaded as a data: src or a sandbox... why is this change
> needed?

I reword the commit message,

The full error is 

=========================================
"JavaScript error: data:text/html,<script>%20%20%20%20window.onmessage%20=%20function(e)%20{%20%20%20%20%20%20%20%20navigator.requestMediaKeySystemAccess("org.w3.clearkey",%20[{%20%20%20%20%20%20%20%20%20%20%20initDataTypes:%20["cenc"],%20%20%20%20%20%20%20%20%20%20%20audioCapabilities:%20[%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20{%20contentType:'audio/mp4;codecs="mp4a.40.2"'},%20%20%20%20%20%20%20%20%20%20%20]%20%20%20%20%20%20%20}]).then(function(access)%20{%20%20%20%20%20%20%20%20%20%20%20%20return%20access.createMediaKeys();%20%20%20%20%20%20%20%20}).then(function(mediaKeys)%20{%20%20%20%20%20%20%20%20%20%20%20%20window.parent.postMessage({result:%20'allowed'},%20'*');%20%20%20%20%20%20%20%20},%20function(error)%20{%20%20%20%20%20%20%20%20%20%20%20%20window.parent.postMessage({result:%20'failed'},%20'*');%20%20%20%20%20%20%20%20});%20%20%20%20};</script>, line 1: TypeError: navigator.requestMediaKeySystemAccess is not a function"
=========================================

Based on the spec

https://www.w3.org/TR/secure-contexts/#is-url-trustworthy

The only way I can think of is the way I did in this patch.

Thanks.
Is the test testing that requestMediaKeySystemAccess _is_ available in the iframe, or is _not_?  That is, is being sandboxed without allow-same-origin supposed to affect the securecontext state?
Flags: needinfo?(jacheng)
Assignee

Comment 42

2 years ago
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #41)
> Is the test testing that requestMediaKeySystemAccess _is_ available in the
> iframe, or is _not_?  
Bug 1322517 will intend to mark `requestMediaKeySystemAccess` as `SecureContext`
=======================================
partial interface Navigator {
  [NewObject, SecureContext]
  Promise<MediaKeySystemAccess>
  requestMediaKeySystemAccess(DOMString keySystem,
                              sequence<MediaKeySystemConfiguration> supportedConfigurations);
};
=======================================
requestMediaKeySystemAccess is available in the iframe. But it should fail when invoking createMediaKeys [1]

But without this patch,

I haven't tested the "createMediaKeys" but blocked by the [2] (TypeError when using requestMediaKeySystemAccess).

I guess navigator.storage is SecureContext also so their test case used the same method to design.[3] 

Thanks.
> That is, is being sandboxed without allow-same-origin
> supposed to affect the securecontext state?

[1]
http://searchfox.org/mozilla-central/rev/7e090b227f7a0ec44d4ded604823d48823158c51/testing/web-platform/tests/encrypted-media/scripts/unique-origin.js#2-4

[2]
https://www.w3.org/TR/secure-contexts/#is-url-trustworthy

[3]
http://searchfox.org/mozilla-central/source/testing/web-platform/tests/storage/opaque-origin.https.html#14,32,68
Flags: needinfo?(jacheng)
> Bug 1322517 will intend to mark `requestMediaKeySystemAccess` as `SecureContext`

OK...

> requestMediaKeySystemAccess is available in the iframe. But it should fail when invoking createMediaKeys [1]

Why is it available?  Sandboxed things are not secure contexts, last I checked, per spec.  Is it that we simply don't implement that part of the spec?  But this is a web platform test; it's supposed to be testing the spec, not whatever our buggy behavior is.
Flags: needinfo?(jacheng)
Assignee

Comment 44

2 years ago
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #43)
> > Bug 1322517 will intend to mark `requestMediaKeySystemAccess` as `SecureContext`
> 
> OK...
> 
> > requestMediaKeySystemAccess is available in the iframe. But it should fail when invoking createMediaKeys [1]
> 
> Why is it available?  Sandboxed things are not secure contexts, last I
> checked, per spec.  Is it that we simply don't implement that part of the
> spec?  But this is a web platform test; it's supposed to be testing the
> spec, not whatever our buggy behavior is.
I forgot to mention that the wpt test is run in secure context(Part2~Part4 did by adding .http.html to its filename).

So not only sandboxed but also the whole HTML is executed with https.

And the situation is just like ```navigator.storage```[1] which is also a secure-context only API.

Hope the explanation makes sense. 

Thank you.

[1]
http://searchfox.org/mozilla-central/source/testing/web-platform/tests/storage/opaque-origin.https.html#14,32,68
Flags: needinfo?(jacheng)
No, it still doesn't.  Per spec, a sandboxed iframe is not a secure context, no matter what the URL of the containing page or the URL in the iframe, unless the allow-secure-context sandbox keyword is specified.  But this test doesn't specify that keyword.
Comment hidden (mozreview-request)
Assignee

Comment 47

2 years ago
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #45)
> No, it still doesn't.  Per spec, a sandboxed iframe is not a secure context,
> no matter what the URL of the containing page or the URL in the iframe,
> unless the allow-secure-context sandbox keyword is specified.  But this test
> doesn't specify that keyword.

I think I got your point and I modified my path with 'allow-secure-context'.

In fact, I cannot find our implementation about 'allow-secure-context' so I guess we haven't dealt with that.

But for correctness, I should add this factory into iframe's sandbox!

Thank you.
Flags: needinfo?(bzbarsky)

Comment 48

2 years ago
mozreview-review
Comment on attachment 8924915 [details]
Bug 1413427 - Part5 - Fix cross origin test failure.

https://reviewboard.mozilla.org/r/196184/#review202900

This makes a lot more sense.  ;)
Attachment #8924915 - Flags: review?(bzbarsky) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Flags: needinfo?(bzbarsky)
Assignee

Comment 54

2 years ago
Thank you for all the reviews~
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 60

2 years ago
Pushed by jacheng@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c7c230ee775e
Part1 - Make Mochitest runs EME with https. r=cpearce,kikuo
https://hg.mozilla.org/integration/autoland/rev/047b40ad4a46
Part2 - Change the file extension from .html to .https.html. r=jgraham
https://hg.mozilla.org/integration/autoland/rev/294ccf3ffc05
Part3 - Change the file extension from .html.ini to .https.html.ini. r=jgraham
https://hg.mozilla.org/integration/autoland/rev/603972699502
Part4 - Rename the content of the .ini file and update the MANIFEST.JSON files by command. r=jgraham
https://hg.mozilla.org/integration/autoland/rev/8a306a5bc1da
Part5 - Fix cross origin test failure. r=bz,jgraham
:jkt -- What documentation needs to be updated? This seems to be a test framework change, not a change that affects web developers.
Flags: needinfo?(jkt)
Sorry I confused this with Bug 1322517 which will need the secure context page when that happens.
Flags: needinfo?(jkt)
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.