Closed Bug 1413427 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: Audio/Video: Playback, enhancement, P3)

Product:
Core
Component:
Audio/Video: Playback
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: JamesCheng, Assigned: JamesCheng)

References

(Blocks 1 open bug)

Details

Attachments

(5 files)

Bug 1413427 - Part1 - Make Mochitest runs EME with https.
59 bytes, text/x-review-board-request
kikuo
: review+
cpearce
: review+
Details
Bug 1413427 - Part2 - Change the file extension from .html to .https.html.
59 bytes, text/x-review-board-request
jgraham
: review+
Details
Bug 1413427 - Part3 - Change the file extension from .html.ini to .https.html.ini.
59 bytes, text/x-review-board-request
jgraham
: review+
Details
Bug 1413427 - Part4 - Rename the content of the .ini file and update the MANIFEST.JSON files by command.
59 bytes, text/x-review-board-request
jgraham
: review+
Details
Bug 1413427 - Part5 - Fix cross origin test failure.
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
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.
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)
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)
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)
Attachment #8924822 - Flags: review?(kikuo)
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 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 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 on attachment 8924915 [details] Bug 1413427 - Part5 - Fix cross origin test failure. https://reviewboard.mozilla.org/r/196184/#review201362
Attachment #8924915 - Flags: review+
(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 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+
Attachment #8924915 - Flags: review?(bzbarsky)
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 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+
(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)
(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)
(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.
(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 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+
Flags: needinfo?(bzbarsky)
Thank you for all the reviews~
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.

Attachment

General

Creator:
Created:
Updated:
Size: