1137498 - Prevent updates changing short_name

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Paul Theriault [:pauljt] (no longer reading bugmail)]

AFAIU, bug 1001861 will result in always only showing the short_name to the user when such value is present in the manifest. We currently prevent updates from changing an apps name, to prevent apps spoofing other apps after they have been installed.

If we are going to replace all places where app name is shown with shortname, we should apply this same restriction to short_name.

Comment 1

[Paul Theriault [:pauljt] (no longer reading bugmail)]

FYI The code in question is here:
https://mxr.mozilla.org/mozilla-central/source/dom/apps/Webapps.jsm#3871

Comment 2

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Actually there's a AppUtils funciton that does the actuall manifest checking. I guess just this can be updated:

https://mxr.mozilla.org/mozilla-central/source/dom/apps/AppsUtils.jsm#520

Comment 3

[Gregor Wagner [:gwagner]]

security related

Comment 4

[Gregor Wagner [:gwagner]]

Can you take this one?

Comment 5

[Candice Serran (:cserran)]

This is another one that got missed as a feature, not a blocking-b2g ...so you're aware bajaj. Risk is low

Comment 6

[Ted Clancy [:tedders1]]

Attachment: bug-1137498-fix.patch

Comment 7

[Ted Clancy [:tedders1]]

Green try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed19f615b0e5

Comment 8

[[:fabrice] Fabrice Desré]

Comment on attachment 8575267 [details] [diff] [review]
bug-1137498-fix.patch

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

r=me with nits fixed.
Thanks for adding tests!

::: dom/apps/AppsUtils.jsm
@@ +530,4 @@
>        for (let locale in aNewManifest.locales) {
> +        let newLocaleEntry = aNewManifest.locales[locale];
> +
> +        let oldLocaleEntry = aOldManifest && 'locales' in aOldManifest &&

nit: double quotes on "locales"

@@ +535,5 @@
> +
> +        if (newLocaleEntry.name) {
> +          // In case previous manifest didn't had a name,
> +          // we use the default app name
> +          newLocaleEntry.name = 

nit: trailing whitespace

@@ +541,3 @@
>          }
> +        if (newLocaleEntry.short_name) {
> +          newLocaleEntry.short_name = 

here too

Comment 9

[Ted Clancy [:tedders1]]

Attachment: bug-1137498-fix.patch

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/82536efb0b2e

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/82536efb0b2e

Comment 12

[Ted Clancy [:tedders1]]

Comment on attachment 8576683 [details] [diff] [review]
bug-1137498-fix.patch

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
Bug 1001861

User impact if declined: 
Possible security risk.

Testing completed: 
Tests are included in the patch.

Risk to taking this patch (and alternatives if risky): 
None forseen.

String or UUID changes made by this patch:
None

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/fba907f72eb3