Closed Bug 937917 Opened 6 years ago Closed 6 years ago

[Facebook] Fix to support https and enable webshare activities

Categories

(Firefox OS Graveyard :: General, defect, P2, major)

Other
Gonk (Firefox OS)

Tracking

(blocking-b2g:koi+, firefox27 wontfix, firefox28 fixed, firefox29 fixed, b2g-v1.2 fixed, b2g-v1.3 fixed)

RESOLVED FIXED
1.2 C5(Nov22)
blocking-b2g koi+
Tracking Status
firefox27 --- wontfix
firefox28 --- fixed
firefox29 --- fixed
b2g-v1.2 --- fixed
b2g-v1.3 --- fixed

People

(Reporter: kward, Assigned: fabrice)

References

Details

Attachments

(1 file)

Moving email discussion to this bug.
Facebook app needs https to enable webshare activities but app currently pre-installed on ALL FFOS phones has http manifest URL.
Proposed solution from Fabrice: patch gecko so that if both the http *and* https version of an app are installed we uninstall the http version. Users will have to login again in the https app but I guess that's the only consequence for an app like facebook. This is a bit like forcing "http everywhere" for apps.
Nominating as Koi+ in order to provide best end user experience on FB app as soon as possible.
blocking-b2g: --- → koi+
(In reply to Karen Ward [:kward] from comment #1)
> Nominating as Koi+ in order to provide best end user experience on FB app as
> soon as possible.

This one actually needs to go through triage. We're late in the game at this point.
blocking-b2g: koi+ → koi?
Can we just change the Facebook manifest URL for 1.2?

My concern is that comment 0 intends to have this backported to all previous versions which isn't the case with koi+ bugs.
Status: NEW → ASSIGNED
Priority: -- → P2
Fabrice,

What approach are you planning to take? This is real late for 1.2 and please advise if we can wait till 1.3 to implement the right solution.
Flags: needinfo?(fabrice)
Ok, so what I propose is this.

When we do an update-check for a manifest we check if we get a redirect response. Specifically we should check for a 301 redirect. If we do, and the redirect is from a http URL to *the same* URL but using https, then we change the manifest URL of the existing app to be https.

This is the *only* redirect which we should let affect the manifest URL. I.e. only a 301 redirect where the URLs match except the scheme changes from http to https. Everything else about the URL must remain the same.

So we wouldn't move any of the data. Which mean that something like IDB data would be lost. But that's ok in the case of facebook since they don't use IDB anyway.

Given that we still have the one-app-per-origin restriction, we should also only do this if there isn't already an app installed on the https origin.
Fabrice explained the problem to me better over irc.

So I think it's fine that if we as part of a FOTA/OTA update install an app, and there already is an app with the *exact* same URL except that the existing app uses http and the FOTA-provided app uses https, then I think it's ok to change the manifestURL and origin of the existing app from http to https.

It would also be ok to uninstall the existing app and install the new one. But that will probably result in more dataloss. In the particular case of facebook the user will likely continue to stay logged in if we just change the manifestURL of the existing app.

But I'm not sure if changing the manifestURL (and thus the origin) of an existing app is too hard.


We can still do the redirect thing. But I think that's orthogonal to this bug.
This was discussed in triage.  We need to understand the risk.  Jonas, do you have a good understanding of that risk so that we can make a call?

Unless the risk is minor, I'm inclined to let this ride the trains.
Flags: needinfo?(jonas)
I think the risk if we go with the uninstall-old-app-and-install-the-new-app solution is pretty low.

I don't know what the risk is if we go with the modify-existing-app-so-that-it-has-a-new-manifest-url solution. Fabrice is looking at that as part of writing the patch. But if the risk is too high we won't go with this solution but rather with the uninstall/install solution.

Of course, if we can defer to 1.3 then that's great. It probably does mean that we will have to wait with rolling out the https facebook until 1.3.

But I'm almost always going to be in favor of not doing feature work after branching. Security is one of those rare exceptions though. And I take it the switch from http to https is for security reasons.
Flags: needinfo?(jonas)
(In reply to Jonas Sicking (:sicking) Please ni? if you want feedback from comment #8)
> I think the risk if we go with the uninstall-old-app-and-install-the-new-app
> solution is pretty low.
> 
> I don't know what the risk is if we go with the
> modify-existing-app-so-that-it-has-a-new-manifest-url solution. Fabrice is
> looking at that as part of writing the patch. But if the risk is too high we
> won't go with this solution but rather with the uninstall/install solution.
> 
> Of course, if we can defer to 1.3 then that's great. It probably does mean
> that we will have to wait with rolling out the https facebook until 1.3.
> 
> But I'm almost always going to be in favor of not doing feature work after
> branching. Security is one of those rare exceptions though. And I take it
> the switch from http to https is for security reasons.

I don't think this is being requested for security reasons - it's being requested to support the share activity in the facebook app.
For 1.2 I'd rather go with the uninstall then reinstall to. Swapping the manifest url has too many unknowns to take this risk.
Flags: needinfo?(fabrice)
Chiming in here about the initial problems. The manifest for Facebook is currently installed from a HTTP origin. Users are by default on HTTPS ("Secure Browsing" option). FB redirecting HTTPS wasn't a problem until they added new endpoints for web activities. Image sharing will now fail for most users until they re-install FB.

There is no easy upgrade path for users other than installing the "new" app from a HTTPS based manifest URL and uninstalling the old app. I am working with FB and Marketplace to initiate that path, but there is not much we can do other than good messaging.

Thus an automated update way would get some users updated without the aforementioned path.
(In reply to Harald Kirschner :digitarald from comment #11)
> Chiming in here about the initial problems. The manifest for Facebook is
> currently installed from a HTTP origin. Users are by default on HTTPS
> ("Secure Browsing" option). FB redirecting HTTPS wasn't a problem until they
> added new endpoints for web activities. Image sharing will now fail for most
> users until they re-install FB.

Was image sharing ever working?
Harald, I also don't understand why image sharing will now fail.
Let's say I'm using http-fb. I get an updated manifest with the activity declaration. Once I share an image from the gallery, we will open the http fb page they declared. What happens at this point? Are they redirecting this page to https and then losing the activity?
I could see it failing if facebook is forced to put a same-origin message handler for the "activity" message, but that then that page immediately redirects after loading. Will the system message be sent to the redirected page?

Though if they use a normal HTTP redirect then I would imagine it should work.

Another way they could make it work is by using HSTS rather than HTTP-redirects. But I'm not sure if there are reasons HSTS wouldn't work for facebook?
(In reply to Jason Smith [:jsmith] from comment #12)
> Was image sharing ever working?

Yes, FB added the share web activity. Check the share menu in Gallery.


(In reply to Fabrice Desré [:fabrice] from comment #13)
> Are they redirecting this page to https and then losing the activity?

When the share activity opens home.php it redirects to HTTPS. After that the activity message handler isn't called anymore. Sharing works with "Secure Browsing" switched off in FB.
(In reply to Jonas Sicking (:sicking) Please ni? if you want feedback from comment #14)
> Though if they use a normal HTTP redirect then I would imagine it should
> work.

They use a 302 redirect.
So why can't this be solved by Tech Evangelism with Facebook?
What kind of fix do you propose? Not redirecting to HTTPS?

They use a normal HTTP redirect; which Jonas said should work; but it doesn't; which might be a bug on our side. But apart from that edge case I expected that web activities, just like other features and permissions, are bound to app origin. So any new permissions will also not work.

Can we go back to the idea of fixing the app origin on system update or is this off the table?
I didn't mean that I think that it *should* work. I meant that I thought that it would work.

It would be good to understand why the message doesn't get delivered after the redirect. It's quite possible that we get caught in some security check in the system-messages code which check the principal of the destination page. That would make sense.

And if that is the case, then using HSTS won't help either.

*If* that is the problem (which is so far just a guess), then I would be fine with disabling this security check for the "activity" message. It doesn't really prevent any known attacks anyway.

So that could be a potential fix as well.

To answer the blocking-status question. I think the risk here is low enough to keep this a blocker. But that's not an argument for it being a blocker, that's the lack of an argument against it.

The question of if this should be a blocker I think depends on if facebook is ok with not redirecting to https here.
Indeed the message is not delivered after the redirect because messages are delivered based on the page location, and that doesn't match anymore. That's the expected behavior.

I feel our best bet is still the uninstall http when installing https.
(In reply to Jonas Sicking (:sicking) Please ni? if you want feedback from comment #19)
> The question of if this should be a blocker I think depends on if facebook
> is ok with not redirecting to https here.

I don't see FB relaxing their "Secure Browsing", that would open a security hole on their side.

(In reply to Fabrice Desré [:fabrice] from comment #20)
> I feel our best bet is still the uninstall http when installing https.

So the flow would be that the update installs a new FB and removes the old one? I don't see a big issue here, other than that the user gets logged out; which might be unexpected.
(In reply to Harald Kirschner :digitarald from comment #21)
> I don't see FB relaxing their "Secure Browsing", that would open a security
> hole on their side.

Given this and that users would have broken functionality (image sharing to FB) which we know is such a critical use case in the shipping markets (the lack of this feature in earlier releases was often cited in feedback), I think we should indeed block on this under the assumption that we can fix it in a low risk way.
blocking-b2g: koi? → koi+
Component: Gaia::Homescreen → General
Duplicate of this bug: 945529
I haven't tested yet, since I need to set up a local ota with the new app.
Attachment #8342774 - Flags: review?(ferjmoreno)
I did several tests, that worked fine. Note that we do that only after first run or updates, not as part as the "normal" app install cycle.
Comment on attachment 8342774 [details] [diff] [review]
https-duplicate.patch

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

The code looks good, but it seems that we would be loosing any IDB and/or localStorage data this way. Are we sure that we want this change without a way to migrate the data from the original http app to the new https one? It might not be the case for FB but it could be for other apps.
Attachment #8342774 - Flags: review?(ferjmoreno)
Yes, we'll lose all private data for the app. I think the consensus is that we're ok with that, since it's really just to fix facebook mistake. I hope we'll remove that in 1.4.
Comment on attachment 8342774 [details] [diff] [review]
https-duplicate.patch

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

::: dom/apps/src/Webapps.jsm
@@ +460,5 @@
> +    if (!app || !app.origin.startsWith("http://")) {
> +      return;
> +    }
> +
> +    let manifestPath = app.manifestURL.substring("http://".length);

Why don't just use something like:
let httpsManifestURL = "https://" + app.manifestURL.substring("http://".length);

for (let id in this.webapps) {
   if (this.webapps[id].manifestURL === httpsManifestURL) {
     debug("Found a http/https match: " + app.manifestURL + " / " +
	   other.manifestURL);
     this.uninstall(app.manifestURL, function() {}, function() {});
     return;
   }
}
(In reply to Antonio Manuel Amaya Calvo (:amac) from comment #28)
> Comment on attachment 8342774 [details] [diff] [review]
> https-duplicate.patch
> 
> Review of attachment 8342774 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/apps/src/Webapps.jsm
> @@ +460,5 @@
> > +    if (!app || !app.origin.startsWith("http://")) {
> > +      return;
> > +    }
> > +
> > +    let manifestPath = app.manifestURL.substring("http://".length);
> 
> Why don't just use something like:
> let httpsManifestURL = "https://" +
> app.manifestURL.substring("http://".length);
> 
> for (let id in this.webapps) {
>    if (this.webapps[id].manifestURL === httpsManifestURL) {
>      debug("Found a http/https match: " + app.manifestURL + " / " +
> 	   other.manifestURL);
>      this.uninstall(app.manifestURL, function() {}, function() {});
>      return;
>    }
> }

Yeah, that's a bit better. Feel free to steal, I only care about getting that landed.
(In reply to Fabrice Desré [:fabrice] from comment #27)
> Yes, we'll lose all private data for the app. I think the consensus is that
> we're ok with that, since it's really just to fix facebook mistake. I hope
> we'll remove that in 1.4.

I can't see how can we safely remove this from 1.4. Isn't it possible to get a 1.4 OTA update on a 1.2 device?

IMHO it would be safer to just be able to migrate the data from the old app to the new one. In fact, we might need this functionality for other cases in the future.

So let's land this how it is and file a follow up bug for the data migration issue, please.
Comment on attachment 8342774 [details] [diff] [review]
https-duplicate.patch

r=me with Antonio's suggestion addressed, please. Also, add a comment with a TODO pointing to the bug for the data migration issue, please.
Attachment #8342774 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/25fd60694d45
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Duplicate of this bug: 949523
You need to log in before you can comment on or make changes to this bug.