media: Require SecureContext for EME APIs (prototype)

media: Require SecureContext for EME APIs (prototype)

$$$ Uploaded for discussion. Not for full review and commit yet. $$$

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.

[xhwang, 2017-01-26 18:29:46]

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

[commit-bot: I haz the power, 2017-01-26 18:30:12]

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

[xhwang, 2017-01-26 18:42:33]

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

$$$ Uploaded for discussion. Not for full review and commit yet. $$$

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.
==========

[xhwang, 2017-01-26 18:42:33]

xhwang@chromium.org changed reviewers:
+ bashi@chromium.org, haraken@chromium.org, mkwst@chromium.org,
yukishiino@chromium.org

[xhwang, 2017-01-26 18:47:50]

haraken, Yuki, Mike West and bashi:

This is work in progress. But I am hitting multiple "issues". So I am uploading
PSs to demonstrate what I am seeing and ask questions.

PS1 compiles. Then when I test it:

- requestMediaKeySystemAccess() and setMediaKeys() are not defined on http,
which is expected!

- But also on http, in dev console, when I type "MediaKeys", it shows:

  MediaKeys
  function MediaKeys() { [native code] }

Is this expected? Note that in the CL I mark the whole MediaKeys interface with
SecureContext.

[xhwang, 2017-01-26 18:53:52]

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

[commit-bot: I haz the power, 2017-01-26 18:54:10]

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

[xhwang, 2017-01-26 18:55:47]

Then in PS2, I marked mediaKeys attribute on HTMLMeidaElement us SecureContest:

[SecureContext] readonly attribute MediaKeys mediaKeys;

Then I hit the following compiling errors. Did I use this wrong?

gen/blink/bindings/modules/v8/V8HTMLMediaElementPartial.cpp:352:28: error:
redefinition of 'signature'
  v8::Local<v8::Signature> signature = v8::Signature::New(isolate,
interfaceTemplate);
                           ^
gen/blink/bindings/modules/v8/V8HTMLMediaElementPartial.cpp:347:28: note:
previous definition is here
  v8::Local<v8::Signature> signature = v8::Signature::New(isolate,
interfaceTemplate);
                           ^
gen/blink/bindings/modules/v8/V8HTMLMediaElementPartial.cpp:353:21: error:
redefinition of 'executionContext'
  ExecutionContext* executionContext =
toExecutionContext(prototypeObject->CreationContext());
                    ^
gen/blink/bindings/modules/v8/V8HTMLMediaElementPartial.cpp:346:21: note:
previous definition is here
  ExecutionContext* executionContext = toExecutionContext(context);
                    ^
2 errors generated.

[xhwang, 2017-01-26 19:05:00]

Then in PS3, I marked the whole "partial interface HTMLMediaElement" using
SecureContest. I think I am following the model in this test:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/tests...

But again, this doesn't compile. Any suggestions?

gen/blink/bindings/modules/v8/V8HTMLMediaElementPartial.cpp:358:28: error:
redefinition of 'signature'
  v8::Local<v8::Signature> signature = v8::Signature::New(isolate,
interfaceTemplate);
                           ^
gen/blink/bindings/modules/v8/V8HTMLMediaElementPartial.cpp:345:28: note:
previous definition is here
  v8::Local<v8::Signature> signature = v8::Signature::New(isolate,
interfaceTemplate);
                           ^
gen/blink/bindings/modules/v8/V8HTMLMediaElementPartial.cpp:359:21: error:
redefinition of 'executionContext'
  ExecutionContext* executionContext =
toExecutionContext(prototypeObject->CreationContext());
                    ^
gen/blink/bindings/modules/v8/V8HTMLMediaElementPartial.cpp:344:21: note:
previous definition is here
  ExecutionContext* executionContext = toExecutionContext(context);
                    ^
2 errors generated.

[xhwang, 2017-01-26 19:05:53]

jrummell / ddorwin: FYI

[bashi, 2017-01-27 00:08:36]

On 2017/01/26 18:47:50, xhwang_slow wrote:
> haraken, Yuki, Mike West and bashi:
> 
> This is work in progress. But I am hitting multiple "issues". So I am
uploading
> PSs to demonstrate what I am seeing and ask questions.
> 
> PS1 compiles. Then when I test it:
> 
> - requestMediaKeySystemAccess() and setMediaKeys() are not defined on http,
> which is expected!
> 
> - But also on http, in dev console, when I type "MediaKeys", it shows:
> 
>   MediaKeys
>   function MediaKeys() { [native code] }
> 
> Is this expected? Note that in the CL I mark the whole MediaKeys interface
with
> SecureContext.

I guess this isn't an expected behavior and it might be a bug in the code
generator. yukishiino@: what do you think?

[xhwang, 2017-01-27 04:43:55]

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

[commit-bot: I haz the power, 2017-01-27 04:44:11]

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

[xhwang, 2017-01-27 04:48:07]

Thank you for looking into this!

Another question. In PS one, some layouts are failing, probably because the test
isn't running in SecureContext. What's the recommendation on testing APIs
requiring SecureContext in layout tests?

[commit-bot: I haz the power, 2017-01-27 04:56:03]

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

[commit-bot: I haz the power, 2017-01-27 04:56:05]

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

[Yuki, 2017-01-27 07:32:09]

First of all, the implementation of [SecureContext] is imperfect. 
https://crrev.com/2634923003 is now facing to mostly the same issue;
[SecureContext] for interfaces is not supported.

mkwst@, do you have any thoughts on how to proceed this?  Are you going to
resume your work on [SecureContext]?

For layout testing, I vaguely guess that we may need to run the test in https:. 
You can put tests in http/tests/... and navigate to https:... if it's http:.

[xhwang, 2017-02-03 19:52:26]

On 2017/01/27 07:32:09, Yuki wrote:
> First of all, the implementation of [SecureContext] is imperfect. 
> https://crrev.com/2634923003 is now facing to mostly the same issue;
> [SecureContext] for interfaces is not supported.
> 
> mkwst@, do you have any thoughts on how to proceed this?  Are you going to
> resume your work on [SecureContext]?
> 
> For layout testing, I vaguely guess that we may need to run the test in
https:. 
> You can put tests in http/tests/... and navigate to https:... if it's http:.

mkwst@: Kindly ping!

For layout testing, currently most tests are running on file:///, which is
treated as SecureContext because we use --allow-file-access-from-files for
layout tests (I guess). So the test isn't a blocking issue at this moment.

[xhwang, 2017-02-04 20:43:23]

Description was changed from

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

$$$ Uploaded for discussion. Not for full review and commit yet. $$$

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 (prototype)

$$$ Uploaded for discussion. Not for full review and commit yet. $$$

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.
==========

[Yuki, 2017-02-06 08:24:39]

On 2017/02/03 19:52:26, xhwang_slow wrote:
> On 2017/01/27 07:32:09, Yuki wrote:
> > First of all, the implementation of [SecureContext] is imperfect. 
> > https://crrev.com/2634923003 is now facing to mostly the same issue;
> > [SecureContext] for interfaces is not supported.
> > 
> > mkwst@, do you have any thoughts on how to proceed this?  Are you going to
> > resume your work on [SecureContext]?
> > 
> > For layout testing, I vaguely guess that we may need to run the test in
> https:. 
> > You can put tests in http/tests/... and navigate to https:... if it's http:.
> 
> mkwst@: Kindly ping!
> 
> For layout testing, currently most tests are running on file:///, which is
> treated as SecureContext because we use --allow-file-access-from-files for
> layout tests (I guess). So the test isn't a blocking issue at this moment.

Just FYI, layout tests under http/tests/ run with a http server, and other tests
run without a http server using file:///.  So, if you want to use http: or
https:, then put your tests in http/tests/ .  If you're okay with using
file:///, that's fine, too.  Just my two cents.

[xhwang, 2017-02-06 17:57:42]

On 2017/02/06 08:24:39, Yuki wrote:
> On 2017/02/03 19:52:26, xhwang_slow wrote:
> > On 2017/01/27 07:32:09, Yuki wrote:
> > > First of all, the implementation of [SecureContext] is imperfect. 
> > > https://crrev.com/2634923003 is now facing to mostly the same issue;
> > > [SecureContext] for interfaces is not supported.
> > > 
> > > mkwst@, do you have any thoughts on how to proceed this?  Are you going to
> > > resume your work on [SecureContext]?
> > > 
> > > For layout testing, I vaguely guess that we may need to run the test in
> > https:. 
> > > You can put tests in http/tests/... and navigate to https:... if it's
http:.
> > 
> > mkwst@: Kindly ping!
> > 
> > For layout testing, currently most tests are running on file:///, which is
> > treated as SecureContext because we use --allow-file-access-from-files for
> > layout tests (I guess). So the test isn't a blocking issue at this moment.
> 
> Just FYI, layout tests under http/tests/ run with a http server, and other
tests
> run without a http server using file:///.  So, if you want to use http: or
> https:, then put your tests in http/tests/ .  If you're okay with using
> file:///, that's fine, too.  Just my two cents.

Thank you so much for the suggestions. Our tests do need to run on https. Do you
know how to run http/tests/* on https?

Currently file:/// seems to work well for my use case. I have an updated CL at
https://chromiumcodereview.appspot.com/2678433003/

[Yuki, 2017-02-09 08:12:09]

On 2017/02/06 17:57:42, xhwang_slow wrote:
> On 2017/02/06 08:24:39, Yuki wrote:
> > On 2017/02/03 19:52:26, xhwang_slow wrote:
> > > On 2017/01/27 07:32:09, Yuki wrote:
> > > > First of all, the implementation of [SecureContext] is imperfect. 
> > > > https://crrev.com/2634923003 is now facing to mostly the same issue;
> > > > [SecureContext] for interfaces is not supported.
> > > > 
> > > > mkwst@, do you have any thoughts on how to proceed this?  Are you going
to
> > > > resume your work on [SecureContext]?
> > > > 
> > > > For layout testing, I vaguely guess that we may need to run the test in
> > > https:. 
> > > > You can put tests in http/tests/... and navigate to https:... if it's
> http:.
> > > 
> > > mkwst@: Kindly ping!
> > > 
> > > For layout testing, currently most tests are running on file:///, which is
> > > treated as SecureContext because we use --allow-file-access-from-files for
> > > layout tests (I guess). So the test isn't a blocking issue at this moment.
> > 
> > Just FYI, layout tests under http/tests/ run with a http server, and other
> tests
> > run without a http server using file:///.  So, if you want to use http: or
> > https:, then put your tests in http/tests/ .  If you're okay with using
> > file:///, that's fine, too.  Just my two cents.
> 
> Thank you so much for the suggestions. Our tests do need to run on https. Do
you
> know how to run http/tests/* on https?
> 
> Currently file:/// seems to work well for my use case. I have an updated CL at
> https://chromiumcodereview.appspot.com/2678433003/

You can navigate to the https: version of the layout tests.  See the following
test for example.
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/http/test...