1229890 - Convert JS callsites to use asyncOpen2 within mobile/

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: use_asyncopen2_mobile.patch

Jonas, not sure if you want to review those bits. When converting the to NewChannel2 within Bug 1087737 we had Wesj review those bits.

I don't really know what CastingApps does, I am just guessing out of the blue here with a Same Origin Policy. Most likely we need to pass a different flag.

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8694880 [details] [diff] [review]
use_asyncopen2_mobile.patch

Review of attachment 8694880 [details] [diff] [review]:
-----------------------------------------------------------------

This seems broken. You need to supply at least a loadingprincipal unless it's a toplevel document load, but that does not seem to be the case here.

Setting SEC_REQUIRE_SAME_ORIGIN_DATA_INHERITS doesn't seem to have a meaning unless there's a principal to enforce it against?

In fact, surprised that this doesn't assert? Why do we calling AsyncOpen2 without a loading principal and without a triggering principal?

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Jonas Sicking (:sicking) from comment #2)
> In fact, surprised that this doesn't assert? Why do we calling AsyncOpen2
> without a loading principal and without a triggering principal?

let channel = Services.io.newChannelFromURI2(aURI,
                                             aElement,
                                             null, // aLoadingPrincipal
                                             null, // aTriggeringPrincipal


I assume you are confused by the comments, I will update that in the final patch. Anyway, fact is that we are passing 'aElement' and query the loadingPrincipal from that.

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7e717a810420
https://hg.mozilla.org/integration/mozilla-inbound/rev/91f6e72fa9f4

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/3aacf4a71634 (along with the other bugs from this push) for failures:
https://treeherder.mozilla.org/logviewer.html#?job_id=18296895&repo=mozilla-inbound

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Ah, good point. Please convert that callsite to NetUtil.jsm which would make that much more clear.

That still leaves the problem that you seem to be enforcing a policy which was not previously enforced? Why use a same-origin policy and not one of the other ones?

Did this code not have any security checks in the past? How is that not a security problem?

Or is aElement a chrome element?

Definitely seems like you should have someone that knows this code do the review.

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Wes Kocher (:KWierso) from comment #5)
> Backed out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/3aacf4a71634 (along
> with the other bugs from this push) for failures:
> https://treeherder.mozilla.org/logviewer.html#?job_id=18296895&repo=mozilla-
> inbound

In fact, the patch for this bug didn't get pushed to inbound - the other patches had the wrong bugnumber attached.

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1229890_asyncopen2_mobile.patch

Wesley, I don't fully understand what this code is doing and I was wondering if you could help us out. You already helped us back when you reviewed Bug 1087737. What we are trying to do is to find out what kind of security checks we need to perform for that channel, but in order to do so we would need to know exactly what the code is doing.

A few questions which should bring us on the right track:
* Should we enforce the same-origin policy (note that SOP is currently not enforced, but maybe it should)?
* Currently we are not performing any security checks, do we need to perform some?
* As Jonas pointed out, is it possible that aElement is a chrome element?

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #8)
> Created attachment 8698195 [details] [diff] [review]
> bug_1229890_asyncopen2_mobile.patch
> 
> Wesley, I don't fully understand what this code is doing and I was wondering
> if you could help us out. You already helped us back when you reviewed Bug
> 1087737. What we are trying to do is to find out what kind of security
> checks we need to perform for that channel, but in order to do so we would
> need to know exactly what the code is doing.
> 
> A few questions which should bring us on the right track:
> * Should we enforce the same-origin policy (note that SOP is currently not
> enforced, but maybe it should)?
> * Currently we are not performing any security checks, do we need to perform
> some?
> * As Jonas pointed out, is it possible that aElement is a chrome element?

Wes, review ping, any update when you will get to that? If you are busy, could you suggest another reviewer who can help use move forward with this bug?

Comment 10

[Wesley Johnston (:wesj)]

Comment on attachment 8698195 [details] [diff] [review]
bug_1229890_asyncopen2_mobile.patch

I'm not around to review this anymore. Mark should. Sorry for the delay :(

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #8)

The purpose of _getContentTypeForURI is to get the mime type of a video when a mime type can't be inferred from the <video> or <source>  elements. _getContentTypeForURI does a GET on the video URL itself, just enough for the server to send the content-type, then we shutdown the channel.

> A few questions which should bring us on the right track:
> * Should we enforce the same-origin policy (note that SOP is currently not
> enforced, but maybe it should)?

I assume we need to act like the video element would when streaming the video content. If the video/media system would fail to play a video due to SOP, then we should use SOP too. If the video plays, even though SOP is violated, then we should not use SOP.

> * Currently we are not performing any security checks, do we need to perform
> some?

I don't know

> * As Jonas pointed out, is it possible that aElement is a chrome element?

No. aElement should always be a <video> element in web content.

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8698195 [details] [diff] [review]
bug_1229890_asyncopen2_mobile.patch

I am fine with this change, as long as it continues to get content-type for video content. If we need to change the security situation, let's do that in a followup, or additional patch in this bug.

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

Thanks Mark, I suppose using SEC_REQUIRE_CORS_DATA_INHERITS is fine then, but allow me one more question. In comment 11 you said we are trying to get the mime type of a video. If so, I think it would make more sense to use nsIContentPolicy::TYPE_INTERNAL_VIDEO instead of TYPE_OTHER like we do in HTMLMediaElement [1] and also in MediaResource [2]. Do you agree?

One last note, I think potentially we need to enforce CORS, but I'll clarify that with Jonas.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/html/HTMLMediaElement.cpp#1302
[2] http://mxr.mozilla.org/mozilla-central/source/dom/media/MediaResource.cpp#852

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #13)
> Thanks Mark, I suppose using SEC_REQUIRE_CORS_DATA_INHERITS is fine then,
> but allow me one more question. In comment 11 you said we are trying to get
> the mime type of a video. If so, I think it would make more sense to use
> nsIContentPolicy::TYPE_INTERNAL_VIDEO instead of TYPE_OTHER like we do in
> HTMLMediaElement [1] and also in MediaResource [2]. Do you agree?

I really don't know. I defer to someone how knows what TYPE_INTERNAL_VIDEO means.

Comment 15

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8698195 [details] [diff] [review]
bug_1229890_asyncopen2_mobile.patch

Jonas, 2 questions:

1) Based on comment 11 I think we should update the patch to use nsIContentPolicy::TYPE_INTERNAL_VIDEO instead of TYPE_OTHER like we do in HTMLMediaElement [1] and also in MediaResource [2]. Do you agree?

2) Do we need to enforce CORS or is SEC_REQUIRE_CORS_DATA_INHERITS the right securityFlag?

[1] http://mxr.mozilla.org/mozilla-central/source/dom/html/HTMLMediaElement.cpp#1302
[2] http://mxr.mozilla.org/mozilla-central/source/dom/media/MediaResource.cpp#852

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> 1) Based on comment 11 I think we should update the patch to use
> nsIContentPolicy::TYPE_INTERNAL_VIDEO instead of TYPE_OTHER like we do in
> HTMLMediaElement [1] and also in MediaResource [2]. Do you agree?

Yes.

> 2) Do we need to enforce CORS or is SEC_REQUIRE_CORS_DATA_INHERITS the right
> securityFlag?

"enforce CORS" is the same thing as use SEC_REQUIRE_CORS_DATA_INHERITS, no?

Based on the filename "CastingApps.js" I'm guessing this code is used to load a video stream which is then used for casting?

Ideally we'd then use 'crossorigin' attribute.

<video>: Use SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS
<video crossorigin="use-credentials">: Use SEC_REQUIRE_CORS_DATA_INHERITS | SEC_COOKIES_INCLUDE
<video crossorigin="anything else">: Use SEC_REQUIRE_CORS_DATA_INHERITS

Comment 17

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1229890_asyncopen2_mobile.patch

(In reply to Jonas Sicking (:sicking) from comment #16)
> > 2) Do we need to enforce CORS or is SEC_REQUIRE_CORS_DATA_INHERITS the right
> > securityFlag?
> 
> "enforce CORS" is the same thing as use SEC_REQUIRE_CORS_DATA_INHERITS, no?

Yes, that was a typo and what I meant was 'do we need to enforce CORS or is SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS the right flag. I think we both agree that we potentially need to enforce CORS here.

> Based on the filename "CastingApps.js" I'm guessing this code is used to
> load a video stream which is then used for casting?
> 
> Ideally we'd then use 'crossorigin' attribute.

So, I have to admit, I haven't tested that code yet - what do you think Jonas? Do we need a test for that? Or is running this through treeherder sufficient? Probably we should ask mfinkle as well!

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I'll defer to the owners of this feature.

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8711945 [details] [diff] [review]
bug_1229890_asyncopen2_mobile.patch

Review of attachment 8711945 [details] [diff] [review]:
-----------------------------------------------------------------

Other than that this looks good to me. But someone that knows this code should review.

::: mobile/android/chrome/content/CastingApps.js
@@ +355,5 @@
> +      secFlags = Ci.nsILoadInfo.SEC_REQUIRE_CORS_DATA_INHERITS;
> +      if (aElement.crossOrigin === "use-credentials") {
> +        secFlags |= Ci.nsILoadInfo.SEC_COOKIES_INCLUDE;
> +      }
> +    }

I don't know if .crossOrigin returns the appropriate value if the attribute is missing or set to an invalid value.

Please check that it does.

Comment 20

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8711945 [details] [diff] [review]
bug_1229890_asyncopen2_mobile.patch

Makes sure this passes a Try run. We'll need to test in a "live" video casting situation as well after it lands.

Comment 21

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Mark Finkle (:mfinkle) from comment #20)
> Comment on attachment 8711945 [details] [diff] [review]
> bug_1229890_asyncopen2_mobile.patch
> 
> Makes sure this passes a Try run. We'll need to test in a "live" video
> casting situation as well after it lands.

Thanks Mark, one last question:
* Can you suggest any tests we should run/perform?
* What would be a good 'live' video casting situation? I would like to make sure this works as expected before landing the conversion to asyncOpen2 for mobile!

Comment 22

[Mark Finkle (:mfinkle) (use needinfo?)]

Christoph - Do you have a Roku you can test with? Even then, we'd need to make a test case where we have a video element that does not specify the mime type either in the the <source> child or with an extension on the filename we could use to guess.

Turns out we might be able to add a test to automation. We already test our other ways of determining the mimetype using this test and content page:
Test: http://mxr.mozilla.org/mozilla-central/source/mobile/android/tests/browser/chrome/test_video_discovery.html?force=1
Content: http://mxr.mozilla.org/mozilla-central/source/mobile/android/tests/browser/chrome/video_discovery.html?force=1

Those tests use "fake", non-existent video files, since we are testing the use of an extension to guess the mime-type.

I wonder if we could easily add a SJS file the returns a content-type (a good one and a bad one) so we could test _getContentTypeForURI.

Let me take a look, or if you are familiar with chrome-mochitests you could take a crack.

Comment 23

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: more-videodiscovery-tests v0.1

Margaret - I would have given this review to Wes, but.... You should learn more about CastingApps anyway :)

I added a pass and fail test that _should_ use _getContentTypeForURI

Try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ef312fc76d8

Comment 24

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Mark Finkle (:mfinkle) from comment #23)
> Created attachment 8712970 [details] [diff] [review]

> Try run:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ef312fc76d8

Test failed, so I am debugging.

Comment 25

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Mark Finkle (:mfinkle) from comment #22)
> Christoph - Do you have a Roku you can test with? Even then, we'd need to
> make a test case where we have a video element that does not specify the
> mime type either in the the <source> child or with an extension on the
> filename we could use to guess.

Thanks for adding a test for this - much appreciated!

Comment 26

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: more-videodiscovery-tests v0.2

While making a test I found the approach we use for _getContentTypeForURI was broken. The approach is async, but we didn't wait for the async methods/checks to complete before assuming failure.

I also found that the existing test worked well for sync methods of guessing the mimetype, but failed to wait for any async methods.

So we had two bugs to fixed before the tests started working as expected.

This passes locally for me. Try build:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=46f0d274f466

Christoph - You might need to rebase your patch if I land first. And landing the test first might be a good idea.

Comment 27

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Mark Finkle (:mfinkle) from comment #26)
> Christoph - You might need to rebase your patch if I land first. And landing
> the test first might be a good idea.

Agreed, please land your test first and mark the bug as leave-open - once you test hits mozilla-central, I'll rebase my patch, make sure everything is fine and push the changes. Thanks for your help here!

Comment 28

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: more-videodiscovery-tests v0.3

OK. The Try run worked, but I had to bump up the expected assertions in debug runs from 6 to 10. Looks like the networking calls cause 2 debug assertions per call and I added 2 new networking calls.

This patch bumps the expected assertions to 10.

Comment 29

[:Margaret Leibovic]

Comment on attachment 8714610 [details] [diff] [review]
more-videodiscovery-tests v0.3

Review of attachment 8714610 [details] [diff] [review]:
-----------------------------------------------------------------

I don't have a thorough understanding of this, but the change seems reasonable to me.

Comment 30

[Christoph Kerschbaumer [:ckerschb]]

Mark, I think your testcase is ready to land, right? If so, can you please land your patches and mark the bug as 'leave-open'? Once the code it's mozilla-central, I'll rebase my patch and land the conversion of the callsite to use asyncOpen2(). Thanks for your help on this and providing the test!

Comment 31

[Mark Finkle (:mfinkle) (use needinfo?)]

https://hg.mozilla.org/integration/fx-team/rev/47242d5db87a

Comment 32

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1229890_asyncopen2_mobile.patch

Rebased the patch on top of the changes from mfinkle. Carrying over r+ from sicking and mfinkle.

Comment 33

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6c2cfa599981

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6c2cfa599981

Comment 35

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Carsten Book [:Tomcat] from comment #34)
> https://hg.mozilla.org/mozilla-central/rev/6c2cfa599981

All the patches within that bug landed.