Closed Bug 1229890 Opened 7 years ago Closed 7 years ago

Convert JS callsites to use asyncOpen2 within mobile/

Categories

(Core :: DOM: Security, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla47

People

(Reporter: ckerschb, Assigned: ckerschb)

References

Details

Attachments

(2 files, 5 obsolete files)

No description provided.
Assignee: nobody → mozilla
Blocks: 1182535
Status: NEW → ASSIGNED
Attached patch use_asyncopen2_mobile.patch (obsolete) — Splinter Review
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.
Attachment #8694880 - Flags: review?(jonas)
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?
Attachment #8694880 - Flags: review?(jonas) → review-
(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.
Flags: needinfo?(jonas)
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.
Flags: needinfo?(jonas)
(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.
Flags: needinfo?(mozilla)
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?
Attachment #8694880 - Attachment is obsolete: true
Attachment #8698195 - Flags: review?(wjohnston2000)
(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?
Flags: needinfo?(wjohnston2000)
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 :(
Flags: needinfo?(wjohnston2000)
Attachment #8698195 - Flags: review?(wjohnston2000) → review?(mark.finkle)
(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 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.
Attachment #8698195 - Flags: review?(mark.finkle) → review+
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
Flags: needinfo?(mark.finkle)
(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.
Flags: needinfo?(mark.finkle)
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
Attachment #8698195 - Flags: review?(jonas)
> 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
(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!
Attachment #8711945 - Flags: review?(jonas)
I'll defer to the owners of this feature.
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.
Attachment #8711945 - Flags: review?(mark.finkle)
Attachment #8711945 - Flags: review?(jonas)
Attachment #8711945 - Flags: feedback+
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.
Attachment #8711945 - Flags: review?(mark.finkle) → review+
Attachment #8698195 - Attachment is obsolete: true
(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!
Flags: needinfo?(mark.finkle)
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.
Flags: needinfo?(mark.finkle)
Attached patch more-videodiscovery-tests v0.1 (obsolete) — Splinter Review
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
Attachment #8712970 - Flags: review?(margaret.leibovic)
(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.
(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!
Attached patch more-videodiscovery-tests v0.2 (obsolete) — Splinter Review
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.
Attachment #8714480 - Flags: review?(margaret.leibovic)
Attachment #8712970 - Attachment is obsolete: true
Attachment #8712970 - Flags: review?(margaret.leibovic)
(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!
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.
Attachment #8714480 - Attachment is obsolete: true
Attachment #8714480 - Flags: review?(margaret.leibovic)
Attachment #8714610 - Flags: review?(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.
Attachment #8714610 - Flags: review?(margaret.leibovic) → review+
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!
Flags: needinfo?(mark.finkle)
Rebased the patch on top of the changes from mfinkle. Carrying over r+ from sicking and mfinkle.
Attachment #8711945 - Attachment is obsolete: true
Attachment #8717007 - Flags: review+
(In reply to Carsten Book [:Tomcat] from comment #34)
> https://hg.mozilla.org/mozilla-central/rev/6c2cfa599981

All the patches within that bug landed.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.