994131 - Loop client needs to use HTTPS when communicating with TokBox servers

Status: RESOLVED FIXED

(Hello (Loop) :: Client, defect, P2)

Description

[Adam Roach [:abr]]

Currently, interactions between the Loop client and the TokBox servers are all performed over http rather than https. For example, right now, the token exchange request looks something like:

> GET http://anvil.opentok.com/session/2_MX...

Long term, we probably want to ask TokBox for library changes, as the logic right now appears to be: "use https if the page is loaded via 'https' or 'chrome-extension', and use http otherwise" (this should probably be reversed to use http if the page is loaded via http, and https for all other schemes).

In the short term, we can fix this in our own code by overriding OT.properties after importing TB.js / opentok.js:

> OT.properties.assetURL = OT.properties.cdnURLSSL + "/webrtc/" + OT.properties.version;
> OT.properties.loggingURL = OT.properties.loggingURLSSL;
> OT.properties.apiURL = OT.properties.apiURLSSL;
> OT.properties.configURL = OT.properties.assetURL + "/js/dynamic_config.min.js";
> OT.properties.cssURL = OT.properties.assetURL + "/css/ot.min.css";

(Yes, this is fragile, which is why we want to ask for changes -- but I note that this patching should work for both versions 2.0 and 2.2 of the library)

Comment 1

[Mark Banner (:standard8)]

When I looked at this the other day, my understanding was that the SDK would detect if the site of the page it was loaded into was https or http and use the scheme matching the site.

Comment 2

[Adam Roach [:abr]]

(In reply to Mark Banner (:standard8) from comment #1)
> When I looked at this the other day, my understanding was that the SDK would
> detect if the site of the page it was loaded into was https or http and use
> the scheme matching the site.

Yes, and that's the exact problem. It only goes over https if the current page scheme is "https:". Ours won't be. Ours will be "about:".

If you add the code I include above, then we switch over to https instead of http -- it's the same thing the SDK code does when it detects a scheme of "https".

Comment 3

[Maire Reavy [:mreavy]]

We definitely need this before we ride the trains, but I think we can land without this for MLP.  I've added ride-train in the whiteboard to remind us that we need it for Loop to ride the trains.

Comment 4

[Adam Roach [:abr]]

I have an email in to Rob Hainer about this, to see if they can add something to the library to help us out.

Comment 5

[Adam Roach [:abr]]

It looks like TokBox may fix this internally. Their response: "Looks like we can make a minor change on our side to make sure the comms between SDK and Tokbox Servers is always HTTPS. In my opinion, HTTPS should be the default and if there is a call to the OT object it should be to used to override and fallback to HTTP. On the surface this seems simple and something we should be able to turn around quickly but let me double check that developers and QA."

Comment 6

[Maire Reavy [:mreavy]]

Adam -- Can I ask you to follow up on this to make sure we have a good solution here?  Standard8 is busy with 3 other MLP bugs.

Comment 7

[Mark Banner (:standard8)]

Just some thoughts:

- In the latest 2.2.5 upgrade, it appeared that the SDK changed how they used apiURL but not by changing the configured url at the top of the file (https://hg.mozilla.org/projects/elm/file/5a48b9b064d6/browser/components/loop/content/shared/libs/sdk.js#l29), but deeper down (https://hg.mozilla.org/projects/elm/rev/5a48b9b064d6#l1.2628).

That seems fragile to me for future changes, though I assume it is their responsibility not to break it.

- Additionally, the logging information seems to use http by default still (https://hg.mozilla.org/projects/elm/file/5a48b9b064d6/browser/components/loop/content/shared/libs/sdk.js#l26), I think it should probably switch to https, though I've also not taken an indepth look at the info (though I believe it contains uuids).

- The cdn url doesn't use https all the time, but we're overriding that for desktop anyway.

Comment 8

[Adam Roach [:abr]]

Response from TokBox: "Please go ahead modify the [sdk] file as needed to send the log traffic over HTTPS for now. We will address this with our next SDK release."

Comment 9

[Adam Roach [:abr]]

Need to land the CDN patch first, since they'll conflict with each other. The fix here is trivial, though.

Comment 10

[Adam Roach [:abr]]

Attachment: Change URLs in OT.properties to use https

Comment 11

[Adam Roach [:abr]]

Attachment: Change URLs in OT.properties to use https

Comment 12

[Mark Banner (:standard8)]

Comment on attachment 8435045 [details] [diff] [review]
Change URLs in OT.properties to use https

Looks great, r=Standard8

Comment 13

[Adam Roach [:abr]]

https://hg.mozilla.org/projects/elm/rev/560186f7d5a4

Comment 14

[Mark Banner (:standard8)]

https://hg.mozilla.org/mozilla-central/rev/560186f7d5a4

Comment 15

[u279076]

Does this need QA testing? If so, please advise.