826555 - Ensure that updates can't rename an app

Status: VERIFIED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

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

Consider the following

1. User goes to untrusted website.
2. Website offers to install "hellokitty" app.
3. User accepts.
4. A week passes.
5. App signals that it has an update available.
6. User accepts update of this and 5 other apps.
7. App is now called "Bank of America".
8. User wants to do some banking.
9. User clicks on "Bank of America" and enters bank username and password.
10. Profit.

We need to ensure that neither packaged or hosted apps can rename themselves when they are updated. If an update contains a different name than the original name, we need to reject the update.

Comment 1

[Rob Hudson [:robhudson]]

(In reply to Jonas Sicking (:sicking) from comment #0)
> We need to ensure that neither packaged or hosted apps can rename themselves
> when they are updated. If an update contains a different name than the
> original name, we need to reject the update.

Aren't there legitimate cases where the app may change its name?

The Marketplace does plan on supporting names changes via the manifest, at least after review. For example, if a hosted app changes its manifest with a minor name update it would trigger a re-review on Marketplace and we would also update our listing pages to display the new name. Likewise for packaged apps if a new version is uploaded with a minor name change and is approved we would update the listing pages to show the new name. When that app is updated and this bug is implemented, it would then break updates.

Comment 2

[Andrew Williamson [:eviljeff]]

I agree with Rob that there are many legitimate use cases where the app may want to change its name.  From trivial (e.g. "Foo Bar App (beta)") > "Foo Bar App" to branding ("Mozilla Marketplace" > "Firefox Marketplace" :p )

I don't particularly care about off-marketplace apps, but for apps installed via Marketplace we have sufficient checks in place (or can add them) to negate the risk of rogue apps spoofing names.

Comment 3

[Jason Smith [:jsmith]]

(In reply to Andrew Williamson [:eviljeff] from comment #2)
> I agree with Rob that there are many legitimate use cases where the app may
> want to change its name.  From trivial (e.g. "Foo Bar App (beta)") > "Foo
> Bar App" to branding ("Mozilla Marketplace" > "Firefox Marketplace" :p )
> 
> I don't particularly care about off-marketplace apps, but for apps installed
> via Marketplace we have sufficient checks in place (or can add them) to
> negate the risk of rogue apps spoofing names.

Right - I think something like this is better implemented as a workflow on marketplace, not in gecko.

Wont fix this bug?

Comment 4

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

(In reply to Andrew Williamson [:eviljeff] from comment #2)
> I don't particularly care about off-marketplace apps, but for apps installed
> via Marketplace we have sufficient checks in place (or can add them) to
> negate the risk of rogue apps spoofing names.

I'm not sure what you mean by that you don't care about off-marketplace apps. The whole point of webapps echo system we are building is that it's not gated on a particular marketplace.

We need to keep users safe no matter which marketplace they install applications from.

In particular, consider self-hosted app developer where the developer website is the one installing the app.

Additionally, even for the firefox-marketplace case, you have no way of ensuring that a hosted app won't rename itself after you reviewed it.

Comment 5

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

I agree there are legitimate reasons for an app to rename itself. But unless we can make it safe we can't provide that feature.

There are legitimate use-cases for many things that are unsafe unfortunately.

Comment 6

[Bobby Holley (:bholley)]

Can we just prompt during update when the app wants to rename itself, just like we (presumably) do when it wants different permissions?

Comment 7

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

We can in v2 :)

Comment 8

[Andrew Williamson [:eviljeff]]

(In reply to Jonas Sicking (:sicking) from comment #4)
> (In reply to Andrew Williamson [:eviljeff] from comment #2)
> > I don't particularly care about off-marketplace apps, but for apps installed
> > via Marketplace we have sufficient checks in place (or can add them) to
> > negate the risk of rogue apps spoofing names.
> 
> I'm not sure what you mean by that you don't care about off-marketplace
> apps. The whole point of webapps echo system we are building is that it's
> not gated on a particular marketplace.

I wasn't meaning to be flippant, its just *my* concern is with app developers who have their app on Marketplace and how they'll be able to manage their apps under very common scenarios.  This could quite easily include our Tier 1-3 partners and pre-installed apps.

> We need to keep users safe no matter which marketplace they install
> applications from.
> 
> In particular, consider self-hosted app developer where the developer
> website is the one installing the app.

I agree its a concern.  Can we exclude marketplace and pre- installed apps from the block and only stop off-marketplace ones from changing the name? (until the prompt is implemented at least)

> Additionally, even for the firefox-marketplace case, you have no way of
> ensuring that a hosted app won't rename itself after you reviewed it.

Well, in the case of Marketplace hosted apps, the daily cron job would pick up the name change in manifest and flag it for us to re-review and reject/disable/blocklist as appropriate.  So if they change it again it will be flagged again.

Comment 9

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

(In reply to Andrew Williamson [:eviljeff] from comment #8)
> > Additionally, even for the firefox-marketplace case, you have no way of
> > ensuring that a hosted app won't rename itself after you reviewed it.
> 
> Well, in the case of Marketplace hosted apps, the daily cron job would pick
> up the name change in manifest and flag it for us to re-review and
> reject/disable/blocklist as appropriate.  So if they change it again it will
> be flagged again.

There is no way to disable or blocklist a hosted app in v1. So while you guys can re-review the app, we don't have any actions we can take to protect users that have already installed the app.

But yes, we technically could white-list that are packaged and installed through the mozilla marketplace. I'm somewhat reluctant to give the mozilla marketplace additional powers over other marketplaces though. More than necessitated by the security model.

Comment 10

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Jonas and I discussed it last night. I thought (still think) that it would be easy to modify the update logic so that we notified the user that the app is being renamed as part of the prompt where we ask them if they want to download the update. However, Jonas said that we can't do any UI changes like that now, and that it would be too much of a change.

Comment 11

[Alexandre Poirot [:ochameau]]

Attachment: Bug 826555 - Ensure that updates can't rename an app

Comment 12

[Andrew Overholt [:overholt]]

Do we need a reviwer?

Comment 13

[Rob Hudson [:robhudson]]

Should we document somewhere that if the developer renames their app in the manifest users will no longer receive updates? And the only way to update would be to uninstall the app and re-install it?

(I'm not sure where those docs would go, but it seems important to share the impact of this.)

Comment 14

[Alexandre Poirot [:ochameau]]

Could be worth adding a note in:
  https://developer.mozilla.org/en-US/docs/Apps/Manifest

Comment 15

[Alexandre Poirot [:ochameau]]

Comment on attachment 699862 [details] [diff] [review]
Bug 826555 - Ensure that updates can't rename an app

I was trying to run tests before asking review, but webapps tests pass.

Comment 16

[[:fabrice] Fabrice Desré]

Comment on attachment 699862 [details] [diff] [review]
Bug 826555 - Ensure that updates can't rename an app

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

::: dom/apps/src/Webapps.jsm
@@ +1292,5 @@
>          if (!AppsUtils.checkManifest(manifest)) {
>            sendError("INVALID_MANIFEST");
>          } else if (!AppsUtils.checkInstallAllowed(manifest, app.installOrigin)) {
>            sendError("INSTALL_FROM_DENIED");
> +        } else if (app.name && app.name != manifest.name) {

You need to check the names in all the locales, not just the default name field.

Comment 17

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

After talking to the marketplace team, I think we should not completely reject an update if the update changes the application name.

Instead we should perform the update, but still retain the old name of the app. This means that we need to change the name in the manifest that is saved in the application registry to the old name of the app before updating the registry.

For packaged apps I do think it's ok that the manifest stored in the application package still has the new, changed, name.

Let me know if this is too complex to do. If so we should reject the update.


The reason for this is that this way the marketplace can accept application renames so that at least new users get the correct name. It also gives us a smoother update path to a future where we use UI to allow updates to rename an app after we check with the user that the rename is ok.

Comment 18

[Jason Smith [:jsmith]]

I feel like product input is needed here. I'd like to know more about what Android's app store and Apple's app store does here and why. That might help confirm or disprove the proposal in comment 17.

Right now my mind still doesn't really see there's enough data to determine a cut and dry best solution to this problem.

Comment 19

[Jason Smith [:jsmith]]

David Bialer is in the CA timezone right now, so pinging him won't get use a quick response, but maybe Peter can act as a backup.

Peter - Can you provide insight into comment 18? Feel free to stop by and talk with me at the work week about this - I'd greatly appreciate your input on this.

Comment 20

[Alexandre Poirot [:ochameau]]

Attachment: Bug 826555 - Ensure that updates can't rename an app r=fabrice

Comment 21

[Alexandre Poirot [:ochameau]]

Comment on attachment 700360 [details] [diff] [review]
Bug 826555 - Ensure that updates can't rename an app r=fabrice

Here is an updated patch that match comment 17 from Jonas (ignore name from updates) and take care of your previous review comment (name field from locales)

Comment 22

[Alexandre Poirot [:ochameau]]

And btw, "TEST_PATH=dom/tests/mochitest/webapps/ make -C obj-x86_64-unknown-linux-gnu/ mochitest-chrome" successfully pass!

Comment 23

[Peter Dolanjski [:pdol]]

I think this is a policy decision on the marketplace/app team side that I'm unfortunately not in a position to make a call on.  I've emailed Rick/Caitlin to get input.

Personally, I'd suggest staying on the cautious side for now and not allowing renames.

Comment 24

[Jason Smith [:jsmith]]

(In reply to Peter Dolanjski from comment #23)
> I think this is a policy decision on the marketplace/app team side that I'm
> unfortunately not in a position to make a call on.  I've emailed
> Rick/Caitlin to get input.
> 
> Personally, I'd suggest staying on the cautious side for now and not
> allowing renames.

From the QA side, I want the least risky solution at this point. There's no time to take any risk on complex solutions.

Comment 25

[Caitlin Galimidi [:c8o]]

Hiya team
Sitting w rick Fant and bill maggs to get feedback. Peter asked us to weigh in

Re Jason: good question for comparative evaluation. Apple and google play, all apps are native, so its not quite the same issue

Re rob Hudson... Bill and rick agree: if the app is renamed, it cannot be updated. Mus be submitted and reap proved.

Sorry, thumbs.

Helpful?

Comment 26

[Alexandre Poirot [:ochameau]]

Note that with previous patch that was rejecting renames was rejecting marketplace updates that were renaming "Marketplace" to "Firefox Marketplace".
The new patch that accept the update but keep original name let it update itself.

Comment 27

[Jason Smith [:jsmith]]

(In reply to Caitlin Galimidi from comment #25)
> Hiya team
> Sitting w rick Fant and bill maggs to get feedback. Peter asked us to weigh
> in
> 
> Re Jason: good question for comparative evaluation. Apple and google play,
> all apps are native, so its not quite the same issue

True but they are points of comparison for marketplaces on how they handle app renaming. And we originally intended to produce a "native" experience with web apps. Market insights is a good group to get information on this usually cause they'll understand the comparison factor here.

Chrome Web Store might be a better comparison point possibly though.

> 
> Re rob Hudson... Bill and rick agree: if the app is renamed, it cannot be
> updated. Mus be submitted and reap proved.
> 
> Sorry, thumbs.
> 
> Helpful?

Well...not really? I was more looking for data here to reinforce which direction to go. Telling me X agrees with a point isn't going to help here.

Comment 28

[Jason Smith [:jsmith]]

Got my answer in IRC - Bill Maggs indicated that Google Play and Apple block updates for apps getting renamed. If the app is renamed, update should be rejected and app should be resubmitted.

Comment 29

[David Bialer [:dbialer]]

(In reply to Caitlin Galimidi from comment #25)
> Hiya team
> Sitting w rick Fant and bill maggs to get feedback. Peter asked us to weigh
> in
> 
> Re Jason: good question for comparative evaluation. Apple and google play,
> all apps are native, so its not quite the same issue
Correct - they also have a cert associated with the name and can disable any app.
They do allow app renames with a new cert.  We don't give certs to hosted apps or have any authentication on hosted apps from the client side.  So we are not able to duplicate their functionality with the same level of security.
> 
> Re rob Hudson... Bill and rick agree: if the app is renamed, it cannot be
> updated. Mus be submitted and reap proved.  
I believe Jonas in Comment #17 has the right answer for now.

We do need reapproval.
The reapproval does not solve any problem of updating names of apps already on the device though.  They would need to uninstall and reinstall, which is a bummer.

Reapproval will allow new users to get an approved name, but existing users either need to be a) notified on install of the name change b) not allowed to update their device if the name changes even if the new name is approved since there is no authentication that the new manifest is approved.

The same issue exists for icons (contents of the icon, not the file name).  I can update my icon to be Bank of America Icon using the name "My Banking" - as these visual cues of the app can both be misleading.  I don't think there is much we can do about this on hosted apps.

We need push notifications to Marketplace to work to update or disable apps.  Or a device side way to authenticate that the manifest has been approved when installing/reinstalling.
> 
> Sorry, thumbs.
> 
> Helpful?

Comment 30

[Jason Smith [:jsmith]]

I think we've got enough information at this point to move forward.

Comment 31

[[:fabrice] Fabrice Desré]

Comment on attachment 700360 [details] [diff] [review]
Bug 826555 - Ensure that updates can't rename an app r=fabrice

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

We're almost there!
One case that must also work is as follows:
- current locale of the device is es-SP, but the manifest has default fields in en-US, and a fr-FR. So at this point we present en-US strings to the user.
- in an update, the es-SP locale is added to the manifest. In this case, we don't want to use the new es-SP locale, but keep the user to the en-US one. I think the easiest way is to delete the es-SP locale from the manifest.

::: dom/apps/src/AppsUtils.jsm
@@ +235,5 @@
>        return false;
>      }
>      return true;
>    },
>  

Nit: Can you ad a comment that explains what this method does?

@@ +236,5 @@
>      }
>      return true;
>    },
>  
> +  normalizeManifest: function normalizeManifest(manifest, app) {

aManifest, aApp please

@@ +239,5 @@
>  
> +  normalizeManifest: function normalizeManifest(manifest, app) {
> +    // Only normalize when updating the app
> +    if (!app.name)
> +      return;

That doesn't hurt, but that should never happen since name is the only mandatory property. Also, use {} even for single line ifs.

Comment 32

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

We should also not allow the es-SP localized name if the user is currently using the en-SP locale and the default name is different from the es-SP name.

Basically, we need to prevent that

getLocalizedName(oldmanifest) != getLocalizedName(newmanifest)

Exactly which properties we add/remove when the names differ I don't really care about. As long as the result is that the localized names doesn't end up changing.

Comment 33

[Alexandre Poirot [:ochameau]]

Attachment: Bug 826555 - Ensure that updates can't rename an app r=fabrice

Comment 34

[[:fabrice] Fabrice Desré]

Comment on attachment 701028 [details] [diff] [review]
Bug 826555 - Ensure that updates can't rename an app r=fabrice

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

Looks good to me, thanks Alex!

Comment 35

[Ben Turner (not reading bugmail, use the needinfo flag!)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/819ce0c7ac9e
https://hg.mozilla.org/releases/mozilla-b2g18/rev/262ed9216954

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/819ce0c7ac9e

Comment 37

[Mark Giffin [:markg]]

I added a note about this in these two MDN topics (and maybe elsewhere later):

https://developer.mozilla.org/en-US/docs/Apps/Updating_apps
https://developer.mozilla.org/en-US/docs/Apps/Manifest#name

Here is the note:

Note: If you change the name of your app after the Marketplace has approved it, you will have to submit your app for approval again.

Please verify that this makes sense.

Comment 38

[Andrew Williamson [:eviljeff]]

(In reply to Mark Giffin from comment #37)
> I added a note about this in these two MDN topics (and maybe elsewhere
> later):
> 
> https://developer.mozilla.org/en-US/docs/Apps/Updating_apps
> https://developer.mozilla.org/en-US/docs/Apps/Manifest#name
> 
> Here is the note:
> 
> Note: If you change the name of your app after the Marketplace has approved
> it, you will have to submit your app for approval again.
> 
> Please verify that this makes sense.

That's not really accurate right now - it gets flagged for re-review but it doesn't require a submission or pre-approval.  And bug 828737 should correct the name on the listing too.  I think its more important to point out that existing users won't get the updated name without reinstalling.

Comment 39

[Mark Giffin [:markg]]

> That's not really accurate right now - it gets flagged for re-review but it
> doesn't require a submission or pre-approval.  And bug 828737 should correct
> the name on the listing too.  I think its more important to point out that
> existing users won't get the updated name without reinstalling.

What about this:

Note: If you change the name of your app, it will be automatically submitted for re-review. It will have to be approved before it will be available on the Marketplace. Your old app will remain available on the Marketplace until the renamed one is approved. When the renamed app is approved, users will have to reinstall the app before they get the new name.

Comment 40

[Andrew Williamson [:eviljeff]]

(In reply to Mark Giffin from comment #39)
> What about this:
> 
> Note: If you change the name of your app, it will be automatically submitted
> for re-review. It will have to be approved before it will be available on
> the Marketplace. Your old app will remain available on the Marketplace until
> the renamed one is approved. When the renamed app is approved, users will
> have to reinstall the app before they get the new name.

More like just:

Note: If you change the name of your app, it may take up to 24 hours for the Marketplace listing to reflect the new name. New users will see the name immediately; existing users will have to reinstall the app before they get the new name.

- there is a daily cron job that flags the app for re-review but the name change on Marketplace would be affected immediately without us approving it.  As installs read off the manifest directly a new user would see the new name immediately, even before the cron job picks it up.

Comment 41

[Jason Smith [:jsmith]]

Hmm. So I tried the following scenario:

1. install a packaged app with name X
2. Update mini-manifest and packaged app name to name Y
3. Check for updates

Result - no updates found.

But based on what I read on this bug, we should still get an update, right? Except without the name changed? Or should we never get an update?

Comment 42

[Jason Smith [:jsmith]]

Actually I screwed up the test. Disregard...

Comment 43

[Jason Smith [:jsmith]]

Sanity Tests (all done with packaged apps):

- Rename "name" parameter on update (used packaged app) - PASS
- Rename "name" parameter on locale override name on update - FAIL
- Add a locale override to override name to something different on update - FAIL

Verified, but there's two followups here for the 2 tests that failed.