media: Require SecureContext for EME APIs

media: Require SecureContext for EME APIs

In this CL requestMediaKeySystemAccess() requires SecureContext. On
non-SecureContext, this API will not be visible.

Some notes for future reference:

1. file:// is always treated as SecureContext.
2. file:// is not a unique origin. The origin string is "file://"
3. LoadFromData is treated as non-SecureContext. See the updated [1]
   in this CL.
4. LoadFromData is also treated as unique origin. This is what [1]
   used to test without this CL.
5. allowFileAccessFromFileURLs has nothing to do with SecureContext.
   Rather it controls whether you can load other files from file://.
   - So if you run a simple html page on file://, and that file
     doesn't depend on any other file, you don't need
     allowFileAccessFromFileURLs. This is why for [2] we don't
     really need allowFileAccessFromFileURLs.
   - If the html page depends on other js files, without
     allowFileAccessFromFileURLs, those js files will not be loaded.
     This will cause a lot of test failures. That's why in content
     shell, we have allowFileAccessFromFileURLs enabled by default.

[1]
third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-unique-origin.html
[2] android_webview/test/shell/assets/key-system-test.html

BUG=672605
TEST=Manually tested and made sure EME APIs are not available on
insecure origins.

Review-Url: https://codereview.chromium.org/2678433003
Cr-Commit-Position: refs/heads/master@{#449344}
Committed: https://chromium.googlesource.com/chromium/src/+/4de117544debac896ba8205f8835ba9ed484a4d3

[xhwang, 2017-02-04 07:08:04]

The CQ bit was checked by xhwang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-04 07:08:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-04 08:01:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-04 08:01:14]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[xhwang, 2017-02-05 20:47:34]

The CQ bit was checked by xhwang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-05 20:47:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xhwang, 2017-02-05 20:57:43]

The CQ bit was checked by xhwang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-05 20:57:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xhwang, 2017-02-05 21:01:49]

Patchset #2 (id:20001) has been deleted

[xhwang, 2017-02-05 21:01:58]

Patchset #1 (id:1) has been deleted

[xhwang, 2017-02-05 21:44:14]

The CQ bit was checked by xhwang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-05 21:44:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xhwang, 2017-02-05 23:08:35]

Patchset #1 (id:40001) has been deleted

[commit-bot: I haz the power, 2017-02-06 00:28:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-06 00:28:38]

Dry run: This issue passed the CQ dry run.

[xhwang, 2017-02-06 05:22:05]

xhwang@chromium.org changed reviewers:
+ jrummell@chromium.org

[xhwang, 2017-02-06 05:22:05]

xhwang@chromium.org changed required reviewers:
+ jrummell@chromium.org

[xhwang, 2017-02-06 05:22:05]

jrummell: PTAL
ddorwin: FYI

[jrummell, 2017-02-06 21:08:21]

lgtm w/ suggestion to move properly working test to a separate file.

https://codereview.chromium.org/2678433003/diff/60001/android_webview/test/sh...
File android_webview/test/shell/assets/key-system-test.html (right):

https://codereview.chromium.org/2678433003/diff/60001/android_webview/test/sh...
android_webview/test/shell/assets/key-system-test.html:17:
navigator.requestMediaKeySystemAccess(keySystem, [{}])
You might want to change the empty configuration to [{audioCapabilities:
[{contentType: 'audio/webm; codec=\"vorbis\"'}]}, {videoCapabilities:
[{contentType: 'video/mp4; codecs=\"avc1.4D000C\"'}]}] now, since the rebase on
my CL won't do it.

https://codereview.chromium.org/2678433003/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/security/powerfulFeatureRestrictions/old-powerful-features-on-insecure-origin.html
(right):

https://codereview.chromium.org/2678433003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/powerfulFeatureRestrictions/old-powerful-features-on-insecure-origin.html:88:
}, 'requestMediaKeySystemAccess');
I'm not sure, but I thought this file was simply the set of calls that should
fail on http but currently don't. I would assume that this test is not needed
here (although it is a good test, so not sure where it should go). Maybe this
should become a new file (encrypted-media-on-insecure-orgin.html)?

[xhwang, 2017-02-06 21:50:54]

The CQ bit was checked by xhwang@chromium.org to run a CQ dry run

[xhwang, 2017-02-06 21:51:01]

https://codereview.chromium.org/2678433003/diff/60001/android_webview/test/sh...
File android_webview/test/shell/assets/key-system-test.html (right):

https://codereview.chromium.org/2678433003/diff/60001/android_webview/test/sh...
android_webview/test/shell/assets/key-system-test.html:17:
navigator.requestMediaKeySystemAccess(keySystem, [{}])
On 2017/02/06 21:08:21, jrummell wrote:
> You might want to change the empty configuration to [{audioCapabilities:
> [{contentType: 'audio/webm; codec=\"vorbis\"'}]}, {videoCapabilities:
> [{contentType: 'video/mp4; codecs=\"avc1.4D000C\"'}]}] now, since the rebase
on
> my CL won't do it.

Done.

https://codereview.chromium.org/2678433003/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/security/powerfulFeatureRestrictions/old-powerful-features-on-insecure-origin.html
(right):

https://codereview.chromium.org/2678433003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/powerfulFeatureRestrictions/old-powerful-features-on-insecure-origin.html:88:
}, 'requestMediaKeySystemAccess');
On 2017/02/06 21:08:21, jrummell wrote:
> I'm not sure, but I thought this file was simply the set of calls that should
> fail on http but currently don't. I would assume that this test is not needed
> here (although it is a good test, so not sure where it should go). Maybe this
> should become a new file (encrypted-media-on-insecure-orgin.html)?

See line 34. All the tests below that line are for "Tests for APIs that have
been turned off on insecure origins". I feel this test does exactly that.

[commit-bot: I haz the power, 2017-02-06 21:51:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xhwang, 2017-02-06 21:51:50]

xhwang@chromium.org changed reviewers:
+ torne@chromium.org

[xhwang, 2017-02-06 21:51:50]

torne@chromium.org: Please OWNERS review changes in android_webview/.

[commit-bot: I haz the power, 2017-02-06 23:26:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-06 23:27:00]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[xhwang, 2017-02-06 23:42:28]

The CQ bit was checked by xhwang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-06 23:43:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xhwang, 2017-02-06 23:57:46]

xhwang@chromium.org changed reviewers:
+ foolip@chromium.org

[xhwang, 2017-02-06 23:57:47]

foolip: Please OWNERS review changes in third_party/WebKit/Source/core/*

[foolip, 2017-02-07 00:06:11]

third_party/WebKit/ lgtm % nits

https://codereview.chromium.org/2678433003/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/security/powerfulFeatureRestrictions/old-powerful-features-on-insecure-origin.html
(right):

https://codereview.chromium.org/2678433003/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/security/powerfulFeatureRestrictions/old-powerful-features-on-insecure-origin.html:86:
assert_false(navigator.hasOwnProperty('requestMediaKeySystemAccess'),
navigator.hasOwnProperty('requestMediaKeySystemAccess') was false before this
change as well because the method is on the prototype.
assert_false('requestMediaKeySystemAccess' in navigator) or assert_equals(typeof
navigator.requestMediaKeySystemAccess, "undefined") would work.

I'd suggest omitting the assert description since there's just one in the test.
(If kept, it should ideally describe the actual value, i.e. the first argument,
the resulting error messages make more sense this way.)

https://codereview.chromium.org/2678433003/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-unique-origin.html
(right):

https://codereview.chromium.org/2678433003/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-unique-origin.html:38:
'        if (navigator.hasOwnProperty(\'requestMediaKeySystemAccess\')) {' +
Ditto.

[xhwang, 2017-02-07 00:23:56]

https://codereview.chromium.org/2678433003/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/security/powerfulFeatureRestrictions/old-powerful-features-on-insecure-origin.html
(right):

https://codereview.chromium.org/2678433003/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/security/powerfulFeatureRestrictions/old-powerful-features-on-insecure-origin.html:86:
assert_false(navigator.hasOwnProperty('requestMediaKeySystemAccess'),
On 2017/02/07 00:06:11, foolip wrote:
> navigator.hasOwnProperty('requestMediaKeySystemAccess') was false before this
> change as well because the method is on the prototype.
> assert_false('requestMediaKeySystemAccess' in navigator) or
assert_equals(typeof
> navigator.requestMediaKeySystemAccess, "undefined") would work.

I am not super familiar with this code, but here's what I think how things work.

navigator is the same as window.navigator which returns a reference to the
Navigator object. So you don't need the "prototype" part.
https://developer.mozilla.org/en-US/docs/Web/API/Window/navigator

Navigator (note the uppercase N) is the interface, so you would need the
prototype part.

I tried without my change that both of the following return true:

navigator.hasOwnProperty('requestMediaKeySystemAccess')
Navigator.prototype.hasOwnProperty('requestMediaKeySystemAccess')

I actually had "'requestMediaKeySystemAccess' in navigator" first, but then felt
hasOwnProperty() would seem a better way to do it. 

But I really don't have any strong opinion. With the explanation above, do you
still recommend to use "'requestMediaKeySystemAccess' in navigator" in the test?
 
> I'd suggest omitting the assert description since there's just one in the
test.
> (If kept, it should ideally describe the actual value, i.e. the first
argument,
> the resulting error messages make more sense this way.)

Will do.

[foolip, 2017-02-07 00:27:12]

https://codereview.chromium.org/2678433003/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/security/powerfulFeatureRestrictions/old-powerful-features-on-insecure-origin.html
(right):

https://codereview.chromium.org/2678433003/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/security/powerfulFeatureRestrictions/old-powerful-features-on-insecure-origin.html:86:
assert_false(navigator.hasOwnProperty('requestMediaKeySystemAccess'),
On 2017/02/07 00:23:56, xhwang_slow wrote:
> On 2017/02/07 00:06:11, foolip wrote:
> > navigator.hasOwnProperty('requestMediaKeySystemAccess') was false before
this
> > change as well because the method is on the prototype.
> > assert_false('requestMediaKeySystemAccess' in navigator) or
> assert_equals(typeof
> > navigator.requestMediaKeySystemAccess, "undefined") would work.
> 
> I am not super familiar with this code, but here's what I think how things
work.
> 
> navigator is the same as window.navigator which returns a reference to the
> Navigator object. So you don't need the "prototype" part.
> https://developer.mozilla.org/en-US/docs/Web/API/Window/navigator
> 
> Navigator (note the uppercase N) is the interface, so you would need the
> prototype part.
> 
> I tried without my change that both of the following return true:
> 
> navigator.hasOwnProperty('requestMediaKeySystemAccess')
> Navigator.prototype.hasOwnProperty('requestMediaKeySystemAccess')
> 
> I actually had "'requestMediaKeySystemAccess' in navigator" first, but then
felt
> hasOwnProperty() would seem a better way to do it. 
> 
> But I really don't have any strong opinion. With the explanation above, do you
> still recommend to use "'requestMediaKeySystemAccess' in navigator" in the
test?

That's odd, I've tested in Chrome Stable and Chrome Canary, and in both
navigator.hasOwnProperty('requestMediaKeySystemAccess') are false even though
navigator.requestMediaKeySystemAccess exists and is a method.

Navigator.prototype.hasOwnProperty('requestMediaKeySystemAccess') is true
though, which is precisely what I expected. Can you re-confirm that
navigator.hasOwnProperty('requestMediaKeySystemAccess') returns true on ToT
without these changes?

[xhwang, 2017-02-07 00:34:46]

On 2017/02/07 00:27:12, foolip wrote:
>
https://codereview.chromium.org/2678433003/diff/100001/third_party/WebKit/Lay...
> File
>
third_party/WebKit/LayoutTests/http/tests/security/powerfulFeatureRestrictions/old-powerful-features-on-insecure-origin.html
> (right):
> 
>
https://codereview.chromium.org/2678433003/diff/100001/third_party/WebKit/Lay...
>
third_party/WebKit/LayoutTests/http/tests/security/powerfulFeatureRestrictions/old-powerful-features-on-insecure-origin.html:86:
> assert_false(navigator.hasOwnProperty('requestMediaKeySystemAccess'),
> On 2017/02/07 00:23:56, xhwang_slow wrote:
> > On 2017/02/07 00:06:11, foolip wrote:
> > > navigator.hasOwnProperty('requestMediaKeySystemAccess') was false before
> this
> > > change as well because the method is on the prototype.
> > > assert_false('requestMediaKeySystemAccess' in navigator) or
> > assert_equals(typeof
> > > navigator.requestMediaKeySystemAccess, "undefined") would work.
> > 
> > I am not super familiar with this code, but here's what I think how things
> work.
> > 
> > navigator is the same as window.navigator which returns a reference to the
> > Navigator object. So you don't need the "prototype" part.
> > https://developer.mozilla.org/en-US/docs/Web/API/Window/navigator
> > 
> > Navigator (note the uppercase N) is the interface, so you would need the
> > prototype part.
> > 
> > I tried without my change that both of the following return true:
> > 
> > navigator.hasOwnProperty('requestMediaKeySystemAccess')
> > Navigator.prototype.hasOwnProperty('requestMediaKeySystemAccess')
> > 
> > I actually had "'requestMediaKeySystemAccess' in navigator" first, but then
> felt
> > hasOwnProperty() would seem a better way to do it. 
> > 
> > But I really don't have any strong opinion. With the explanation above, do
you
> > still recommend to use "'requestMediaKeySystemAccess' in navigator" in the
> test?
> 
> That's odd, I've tested in Chrome Stable and Chrome Canary, and in both
> navigator.hasOwnProperty('requestMediaKeySystemAccess') are false even though
> navigator.requestMediaKeySystemAccess exists and is a method.
> 
> Navigator.prototype.hasOwnProperty('requestMediaKeySystemAccess') is true
> though, which is precisely what I expected. Can you re-confirm that
> navigator.hasOwnProperty('requestMediaKeySystemAccess') returns true on ToT
> without these changes?

It seems this is because I have "EME logger" extension enabled. After I disable
the extension I got the same behavior as yours. Thanks for catching it!

I'll make the change as suggested.

[xhwang, 2017-02-07 00:49:48]

The CQ bit was checked by xhwang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-07 00:50:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xhwang, 2017-02-07 00:53:23]

On 2017/02/07 00:34:46, xhwang_slow wrote:
> On 2017/02/07 00:27:12, foolip wrote:
> >
>
https://codereview.chromium.org/2678433003/diff/100001/third_party/WebKit/Lay...
> > File
> >
>
third_party/WebKit/LayoutTests/http/tests/security/powerfulFeatureRestrictions/old-powerful-features-on-insecure-origin.html
> > (right):
> > 
> >
>
https://codereview.chromium.org/2678433003/diff/100001/third_party/WebKit/Lay...
> >
>
third_party/WebKit/LayoutTests/http/tests/security/powerfulFeatureRestrictions/old-powerful-features-on-insecure-origin.html:86:
> > assert_false(navigator.hasOwnProperty('requestMediaKeySystemAccess'),
> > On 2017/02/07 00:23:56, xhwang_slow wrote:
> > > On 2017/02/07 00:06:11, foolip wrote:
> > > > navigator.hasOwnProperty('requestMediaKeySystemAccess') was false before
> > this
> > > > change as well because the method is on the prototype.
> > > > assert_false('requestMediaKeySystemAccess' in navigator) or
> > > assert_equals(typeof
> > > > navigator.requestMediaKeySystemAccess, "undefined") would work.
> > > 
> > > I am not super familiar with this code, but here's what I think how things
> > work.
> > > 
> > > navigator is the same as window.navigator which returns a reference to the
> > > Navigator object. So you don't need the "prototype" part.
> > > https://developer.mozilla.org/en-US/docs/Web/API/Window/navigator
> > > 
> > > Navigator (note the uppercase N) is the interface, so you would need the
> > > prototype part.
> > > 
> > > I tried without my change that both of the following return true:
> > > 
> > > navigator.hasOwnProperty('requestMediaKeySystemAccess')
> > > Navigator.prototype.hasOwnProperty('requestMediaKeySystemAccess')
> > > 
> > > I actually had "'requestMediaKeySystemAccess' in navigator" first, but
then
> > felt
> > > hasOwnProperty() would seem a better way to do it. 
> > > 
> > > But I really don't have any strong opinion. With the explanation above, do
> you
> > > still recommend to use "'requestMediaKeySystemAccess' in navigator" in the
> > test?
> > 
> > That's odd, I've tested in Chrome Stable and Chrome Canary, and in both
> > navigator.hasOwnProperty('requestMediaKeySystemAccess') are false even
though
> > navigator.requestMediaKeySystemAccess exists and is a method.
> > 
> > Navigator.prototype.hasOwnProperty('requestMediaKeySystemAccess') is true
> > though, which is precisely what I expected. Can you re-confirm that
> > navigator.hasOwnProperty('requestMediaKeySystemAccess') returns true on ToT
> > without these changes?
> 
> It seems this is because I have "EME logger" extension enabled. After I
disable
> the extension I got the same behavior as yours. Thanks for catching it!
> 
> I'll make the change as suggested.

Done.

[foolip, 2017-02-07 02:27:07]

updated tests lgtm

[commit-bot: I haz the power, 2017-02-07 02:37:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-07 02:37:20]

Dry run: This issue passed the CQ dry run.

[Torne, 2017-02-07 11:24:54]

https://codereview.chromium.org/2678433003/diff/120001/android_webview/javate...
File
android_webview/javatests/src/org/chromium/android_webview/test/KeySystemTest.java
(right):

https://codereview.chromium.org/2678433003/diff/120001/android_webview/javate...
android_webview/javatests/src/org/chromium/android_webview/test/KeySystemTest.java:54:
allowFileAccessFromFileURLs(mAwContents);
Several questions here:

1) I don't get why allowFileAccessFromFileURLs has anything to do with whether
file: is considered secure or not. Isn't file: always secure?

2) Why is using a data URL a problem in the first place? Isn't that also secure?

[xhwang, 2017-02-07 18:35:47]

The CQ bit was checked by xhwang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-07 18:36:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xhwang, 2017-02-07 18:43:56]

Thanks for the comments. Please take a look again!

https://codereview.chromium.org/2678433003/diff/120001/android_webview/javate...
File
android_webview/javatests/src/org/chromium/android_webview/test/KeySystemTest.java
(right):

https://codereview.chromium.org/2678433003/diff/120001/android_webview/javate...
android_webview/javatests/src/org/chromium/android_webview/test/KeySystemTest.java:54:
allowFileAccessFromFileURLs(mAwContents);
On 2017/02/07 11:24:54, Torne wrote:
> Several questions here:
> 
> 1) I don't get why allowFileAccessFromFileURLs has anything to do with whether
> file: is considered secure or not. Isn't file: always secure?

I think you are right. I had a misunderstanding of the flag.

Reverted this change.

> 2) Why is using a data URL a problem in the first place? Isn't that also
secure?

Data URL is unique origin, which is not SecureContext.

Please check the change in
third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-unique-origin.html
in this CL to see an example.

[commit-bot: I haz the power, 2017-02-07 20:21:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-07 20:21:58]

Dry run: This issue passed the CQ dry run.

[Torne, 2017-02-08 14:56:41]

On 2017/02/07 18:43:56, xhwang_slow wrote:
> Thanks for the comments. Please take a look again!
> 
>
https://codereview.chromium.org/2678433003/diff/120001/android_webview/javate...
> File
>
android_webview/javatests/src/org/chromium/android_webview/test/KeySystemTest.java
> (right):
> 
>
https://codereview.chromium.org/2678433003/diff/120001/android_webview/javate...
>
android_webview/javatests/src/org/chromium/android_webview/test/KeySystemTest.java:54:
> allowFileAccessFromFileURLs(mAwContents);
> On 2017/02/07 11:24:54, Torne wrote:
> > Several questions here:
> > 
> > 1) I don't get why allowFileAccessFromFileURLs has anything to do with
whether
> > file: is considered secure or not. Isn't file: always secure?
> 
> I think you are right. I had a misunderstanding of the flag.
> 
> Reverted this change.
> 
> > 2) Why is using a data URL a problem in the first place? Isn't that also
> secure?
> 
> Data URL is unique origin, which is not SecureContext.

I don't entirely know how all this stuff is implemented (since it's general
blink stuff), but in that case I suspect you might actually need
allowFileAccessFromFileURLs, because I think if you don't enable that then file
origins are *also* unique origins. If that means that they aren't considered
secure, then that seems like a larger problem with how these types of origin
work.. it seems to me that file origins should always be secure, whether they're
unique or not. I don't really know what the actual behaviour is here; you might
need to test it and see if I'm talking nonsense :)

> Please check the change in
>
third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-unique-origin.html
> in this CL to see an example.

[xhwang, 2017-02-08 19:17:39]

On 2017/02/08 14:56:41, Torne wrote:

(removed older comments)

> > > 2) Why is using a data URL a problem in the first place? Isn't that also
> > secure?
> > 
> > Data URL is unique origin, which is not SecureContext.
> 
> I don't entirely know how all this stuff is implemented (since it's general
> blink stuff), but in that case I suspect you might actually need
> allowFileAccessFromFileURLs, because I think if you don't enable that then
file
> origins are *also* unique origins. If that means that they aren't considered
> secure, then that seems like a larger problem with how these types of origin
> work.. it seems to me that file origins should always be secure, whether
they're
> unique or not. I don't really know what the actual behaviour is here; you
might
> need to test it and see if I'm talking nonsense :)

Good questions. I am not an expert on Blink/SecureContext/UniqueOrigin either.
So I just tested a bunch and here's my understanding:

1. file:// is always treated as SecureContext. 
2. file:// is not a unique origin. The origin string is "file://"
3. LoadFromData is always treated as non-SecureContext. See the updated [1] in
this CL.
4. LoadFromData is also treated as unique origin. This is what [1] used to test
without my CL.
5. allowFileAccessFromFileURLs has nothing to do with SecureContext. Rather it
controls whether you can load other files from file://.
   - So if you run a simple html page on file://, and that file doesn't depend
on any other file, you don't need allowFileAccessFromFileURLs. This is why for
android_webview/test/shell/assets/key-system-test.html I don't really need
allowFileAccessFromFileURLs.
   - If the html page depends on other js files, without
allowFileAccessFromFileURLs, those js files will not be loaded. This will cause
a lot of test failures. That's why in content shell, we have
allowFileAccessFromFileURLs enabled by default.

[1]
third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-unique-origin.html

[xhwang, 2017-02-08 19:43:25]

Description was changed from

==========
media: Require SecureContext for EME APIs

In this CL requestMediaKeySystemAccess() requires SecureContext. On
non-SecureContext, this API will not be visible.

BUG=672605
TEST=Manually tested and made sure EME APIs are not available on
insecure origins.
==========

to

==========
media: Require SecureContext for EME APIs

In this CL requestMediaKeySystemAccess() requires SecureContext. On
non-SecureContext, this API will not be visible.

Some notes for future reference:

1. file:// is always treated as SecureContext.
2. file:// is not a unique origin. The origin string is "file://"
3. LoadFromData is treated as non-SecureContext. See the updated [1]
   in this CL.
4. LoadFromData is also treated as unique origin. This is what [1]
   used to test without this CL.
5. allowFileAccessFromFileURLs has nothing to do with SecureContext.
   Rather it controls whether you can load other files from file://.
   - So if you run a simple html page on file://, and that file
     doesn't depend on any other file, you don't need
     allowFileAccessFromFileURLs. This is why for [2] we don't
     really need allowFileAccessFromFileURLs.
   - If the html page depends on other js files, without
     allowFileAccessFromFileURLs, those js files will not be loaded.
     This will cause a lot of test failures. That's why in content
     shell, we have allowFileAccessFromFileURLs enabled by default.

[1]
third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-unique-origin.html
[2] android_webview/test/shell/assets/key-system-test.html

BUG=672605
TEST=Manually tested and made sure EME APIs are not available on
insecure origins.
==========

[xhwang, 2017-02-09 06:50:23]

rebase only

[xhwang, 2017-02-09 06:51:35]

The CQ bit was checked by xhwang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-09 06:51:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-09 08:41:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-09 08:41:20]

Dry run: This issue passed the CQ dry run.

[Torne, 2017-02-09 13:47:56]

OK, thanks for checking stuff out here. LGTM as-is in that case :)

[xhwang, 2017-02-09 17:40:02]

The CQ bit was checked by xhwang@chromium.org

[xhwang, 2017-02-09 17:40:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
jrummell@chromium.org, foolip@chromium.org
Link to the patchset: https://codereview.chromium.org/2678433003/#ps160001
(title: "rebase only")

[commit-bot: I haz the power, 2017-02-09 17:40:46]

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

[commit-bot: I haz the power, 2017-02-09 17:47:21]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1486662002609850,
"parent_rev": "ef71bd0bc3e889890de79958ae65052029d429f1", "commit_rev":
"4de117544debac896ba8205f8835ba9ed484a4d3"}

[commit-bot: I haz the power, 2017-02-09 17:47:54]

Description was changed from

==========
media: Require SecureContext for EME APIs

In this CL requestMediaKeySystemAccess() requires SecureContext. On
non-SecureContext, this API will not be visible.

Some notes for future reference:

1. file:// is always treated as SecureContext.
2. file:// is not a unique origin. The origin string is "file://"
3. LoadFromData is treated as non-SecureContext. See the updated [1]
   in this CL.
4. LoadFromData is also treated as unique origin. This is what [1]
   used to test without this CL.
5. allowFileAccessFromFileURLs has nothing to do with SecureContext.
   Rather it controls whether you can load other files from file://.
   - So if you run a simple html page on file://, and that file
     doesn't depend on any other file, you don't need
     allowFileAccessFromFileURLs. This is why for [2] we don't
     really need allowFileAccessFromFileURLs.
   - If the html page depends on other js files, without
     allowFileAccessFromFileURLs, those js files will not be loaded.
     This will cause a lot of test failures. That's why in content
     shell, we have allowFileAccessFromFileURLs enabled by default.

[1]
third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-unique-origin.html
[2] android_webview/test/shell/assets/key-system-test.html

BUG=672605
TEST=Manually tested and made sure EME APIs are not available on
insecure origins.
==========

to

==========
media: Require SecureContext for EME APIs

In this CL requestMediaKeySystemAccess() requires SecureContext. On
non-SecureContext, this API will not be visible.

Some notes for future reference:

1. file:// is always treated as SecureContext.
2. file:// is not a unique origin. The origin string is "file://"
3. LoadFromData is treated as non-SecureContext. See the updated [1]
   in this CL.
4. LoadFromData is also treated as unique origin. This is what [1]
   used to test without this CL.
5. allowFileAccessFromFileURLs has nothing to do with SecureContext.
   Rather it controls whether you can load other files from file://.
   - So if you run a simple html page on file://, and that file
     doesn't depend on any other file, you don't need
     allowFileAccessFromFileURLs. This is why for [2] we don't
     really need allowFileAccessFromFileURLs.
   - If the html page depends on other js files, without
     allowFileAccessFromFileURLs, those js files will not be loaded.
     This will cause a lot of test failures. That's why in content
     shell, we have allowFileAccessFromFileURLs enabled by default.

[1]
third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-unique-origin.html
[2] android_webview/test/shell/assets/key-system-test.html

BUG=672605
TEST=Manually tested and made sure EME APIs are not available on
insecure origins.

Review-Url: https://codereview.chromium.org/2678433003
Cr-Commit-Position: refs/heads/master@{#449344}
Committed:
https://chromium.googlesource.com/chromium/src/+/4de117544debac896ba8205f8835...
==========

[commit-bot: I haz the power, 2017-02-09 17:47:55]

Committed patchset #6 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/4de117544debac896ba8205f8835...