1762189 - Include localized app marketplace Icons in langpacks

Status: VERIFIED FIXED

(Firefox :: Messaging System, enhancement, P1)

Description

[Meg Viar [:mviar]]

Follow up to bug 1754247

For FX100, we added app marketplace icons for the top seven mobile download languages to MC. This works for the near-term, but we'd like to implement a more permanent solution that doesn't required including the full set of icons for every locale. Ideally, we'll include the localized icons in their respective langpacks. Based on discussion in bug 1754247, these images can be placed in the correct location in the langpack and then loaded via the chrome URL. Currently, we only do this with strings.

Comment 1

[Meg Viar [:mviar]]

Attachment: Bug 1762189 - Include localized app marketplace Icons in langpacks

Comment 2

[Meg Viar [:mviar]]

Attachment: Bug 1762189 - Include localized app marketplace Icons as remote images

Comment 3

[Meg Viar [:mviar]]

We're exploring the ides of using remotely hosted images rather than putting assets in langpacks/localized builds. Two possible approaches:

• Use Remote Images & host badges in ms-images bucket
  • Pros: full control over assets (image url won't change unless we change it), may provide caching in the future
  • Con: we'd need to include a map of the image URLs with their UUIDs in MC (if we're only updating CSS) OR set the image source via JS in MobileDownloads.jsx
• Use badges already hosted on Bedrock
  • Pros: predictable naming convention for URL assets, so we only need to store a list of supported locales in MC, avoids storing duplicates of the same assets (a goal here is to be consistent with Bedrock's badges), can implement using only CSS changes
  • Cons: URL naming conventions and/or assets themselves could change without our knowledge (we'd then fallback to English badge with translated alt text)

@Gijs, you raised a few concerns in bug 175424 about requesting remote assets:

• it breaks in offline mode / without a network
  (this may change for Remote Images in the future - with both approaches we'd use the English fallback in MC for offline mode)
• it adds fingerprinting surface to the browser usage (a passive network adversary can see the network requests and can know that you're opening a private browsing window)
• it adds security risk because we'd be embedding remote resources directly into an in-product surface, so an active network attacker could load resources into the browser's privileged environment that way (and, with the right images, exploit UAF bugs in our png/jpg/webp/svg code without needing an additional sandbox escape). See also e.g. bug 1513445, bug 1607483. We already disallow requests like this for scripts and documents - we haven't restricted images yet but we really should.

Am I correct that these concerns apply to both approaches outlined above? If so, it seems we've accepted those risks for the use of Remote Images in other cases.

Comment 4

[:Gijs (he/him)]

(In reply to Meg Viar from comment #3)

We're exploring the ides of using remotely hosted images rather than putting assets in langpacks/localized builds.

I'm a bit confused why we would do that? We have an existing way (langpacks) of getting localized content to users, why not use it? I looked at the phab revision but I didn't see any strong reasons not to move forward with this approach - perhaps I missed them?

Two possible approaches:

• Use Remote Images & host badges in ms-images bucket
  • Pros: full control over assets (image url won't change unless we change it), may provide caching in the future
  • Con: we'd need to include a map of the image URLs with their UUIDs in MC (if we're only updating CSS) OR set the image source via JS in MobileDownloads.jsx
• Use badges already hosted on Bedrock
  • Pros: predictable naming convention for URL assets, so we only need to store a list of supported locales in MC, avoids storing duplicates of the same assets (a goal here is to be consistent with Bedrock's badges), can implement using only CSS changes
  • Cons: URL naming conventions and/or assets themselves could change without our knowledge (we'd then fallback to English badge with translated alt text)

@Gijs, you raised a few concerns in bug 175424 about requesting remote assets:

• it breaks in offline mode / without a network
  (this may change for Remote Images in the future - with both approaches we'd use the English fallback in MC for offline mode)
• it adds fingerprinting surface to the browser usage (a passive network adversary can see the network requests and can know that you're opening a private browsing window)
• it adds security risk because we'd be embedding remote resources directly into an in-product surface, so an active network attacker could load resources into the browser's privileged environment that way (and, with the right images, exploit UAF bugs in our png/jpg/webp/svg code without needing an additional sandbox escape). See also e.g. bug 1513445, bug 1607483. We already disallow requests like this for scripts and documents - we haven't restricted images yet but we really should.

Am I correct that these concerns apply to both approaches outlined above?

I don't really understand the two approaches fully. But at a glance, it looks like RemoteImages relies on remote settings and will therefore do hash checks, which mitigates some of the security risks, which bedrock wouldn't (ie if someone compromises bedrock or a proxy/mitm attack serves different content, in the bedrock case we'd just load untrusted remote images, whereas in the RemoteImages case we will refuse to do so).

If so, it seems we've accepted those risks for the use of Remote Images in other cases.

Well, the code is from February and there is only one consumer so far, so I don't think we've really put this to widespread use yet. It's also not clear that these considerations were surfaced and accepted, rather than missed, and even if they were, their weight might differ based on the proposed usecase (e.g. for a custom animation that has a fallback, network issues may be acceptable; for critical onboarding components that may not be the case).

I'm not very familiar with the code, but at a glance it appears the only consumer has no error handling at all - neither for a failure to fetch (due to being offline), or for getting broken content (ie this call can throw because via patchMessage it invokes load which invokes download which invokes downloadAsBytes which can throw for either network errors or for broken/mis-hashed content being returned). Perhaps there is error handling higher up the stack? But again, I couldn't find it.

Basically, ISTM that using remote images when we could package the images into the binary and/or langpacks introduces risk, and because these particular images are unlikely to change (ie ability to update the images remotely is usually a justification, but doesn't seem to be so here), I don't see a corresponding/weightier benefit that would justify that additional risk. So I think we should go the langpack route.

Comment 5

[Meg Viar [:mviar]]

Thanks @Gijs, this is helpful. We've been having a larger conversation around remote assets, but I agree that for this specific case of localized assets, the lang pack / localized builds approach (like in this patch) makes sense.

I'm curious what the process for adding assets to lang packs and localized builds looks like (or who to ask). As far as I know, we only handle localized strings in language packs at the moment. I haven't heard any definite reasons for not adding images other than that there might be unexpected complexity. :lgreco noted that we'd want to ensure nothing on the addons-linter side that would fail when resources other than text files are packaged into a langpack xpi. They also encouraged writing tests to confirm that the images wouldn't keep the xpi in use because of a lingering reference to the image somewhere in the Firefox UI.

I suppose we could also explore encoding the images as strings (since we know strings are supported).

Comment 6

[Meg Viar [:mviar]]

After some discussion, we'll pursue packaging these assets with langpacks. While the system wasn't designed with this purpose in mind, there don't seem to be any major blockers. The tentative plan is to add all of the translated badges to MC in a directory with /$LANG/ in the structure. Then, update l10n.mk to handle packaging these assets with their respective langpacks.

:nalexander - thanks for your input on this issue in Firefox Desktop Development on Element. Do you have any thoughts on this approach or complexities worth noting here?

Comment 7

[Nick Alexander :nalexander [he/him] (PTO Feb 24-March 3)]

(In reply to Meg Viar from comment #6)

After some discussion, we'll pursue packaging these assets with langpacks. While the system wasn't designed with this purpose in mind, there don't seem to be any major blockers. The tentative plan is to add all of the translated badges to MC in a directory with /$LANG/ in the structure. Then, update l10n.mk to handle packaging these assets with their respective langpacks.

:nalexander - thanks for your input on this issue in Firefox Desktop Development on Element. Do you have any thoughts on this approach or complexities worth noting here?

Nothing that's not been captured above, other than that touching l10n.mk and the langpack code is delicate. But do you need to? :glandium, how would it be to have:

• a moz.build that defined MARKETPLACE_AB_CD = AB_CD if AB_CD in [...], the set of locales with this asset
• a .png resource declared in some jar.mn that is guarded by MARKETPLACE_AB_CD and references MARKETPLACE_AB_CD in its path
  Would that be sufficient, and not require meaningful changes to l10n.mk and repackaging, etc?

Comment 8

[Meg Viar [:mviar]]

:nalexander that's an interesting alternative. AB_CD isn't among the variables available in moz.build, but I'm investigating other ways to access the locale in moz.build.

Comment 9

[:Gijs (he/him)]

(In reply to Meg Viar from comment #8)

:nalexander that's an interesting alternative. AB_CD isn't among the variables available in moz.build, but I'm investigating other ways to access the locale in moz.build.

https://searchfox.org/mozilla-central/rev/ea1234192518e01694a88eac8ff090e4cadf5ca4/mobile/android/chrome/moz.build#13 may be a useful example? See also https://searchfox.org/mozilla-central/rev/ea1234192518e01694a88eac8ff090e4cadf5ca4/browser/app/Makefile.in#68

Comment 10

[Mike Hommey [:glandium]]

I'm confused why not put these in l10n-central?

Comment 11

[Francesco Lodolo [:flod]]

(In reply to Mike Hommey [:glandium] from comment #10)

I'm confused why not put these in l10n-central?

It doesn't scale (it literally requires someone to land those images in X separate repositories), for both setup and maintenance.

More important, it goes into the opposite direction of what we've been trying to do for the past years: taking out of l10n repositories assets that locales shouldn't modify (search engines, protocol handlers, soon intl properties settings). I think we want to make sure those images are not changed, and updated timely if needed, and one way to do it is to centralize them in m-c.

Comment 12

[Meg Viar [:mviar]]

Hi :nalexander, thanks for pointing me in the right direction regarding creating shippable l10n lang repacks on try. This confirmed that, with the current patch, AB_CD evaluates to "en-US" when the new moz.build file I added to browser/themes/shared/app-marketplace-icons is evaluated, regardless of the repack locale.

I'm assuming the icons are packaged with the base en-US build and included in the final lang repacks as-is in English. I've been experimenting with how to ensure these assets are repackaged (?) with the new value for AB_CD when building lang repacks, but haven't had any luck. Do you have any pointers?

Comment 13

[Nick Alexander :nalexander [he/him] (PTO Feb 24-March 3)]

Attachment: WIP: Bug 1762189 - TO FOLD - Convince build system to take in-tree resources during single-locale repacks.

There is deep magic involved here. The moz.build system doesn't
really understand single-locale repacks, so the "obvious approach" of
using MOZ_UI_LOCALE cannot succeed, since that configure-time
variable isn't changed (and henced is not correct) at repack-time.
This commit pushes the logic below the moz.build system into the
Make-based system. In order for the langpack system to work, the
jar.mn resources really need to be localized (even if the inputs to
those resources aren't localized in the sense of coming from
l10n-central).

This patch produces a sensible browser/omni.ja in en-US containing:

  -rw-r--r--     4441   1-Jan-2010 00:00:00  chrome/browser/locale/en-US/app-marketplace-icons/android.png
  -rw-r--r--     9033   1-Jan-2010 00:00:00  chrome/browser/locale/en-US/app-marketplace-icons/ios.svg

and a sensible browser/omni.ja in a repack (e.g, de) containing:

  -rw-r--r--     4430   1-Jan-2010 00:00:00  chrome/browser/locale/de/app-marketplace-icons/android.png
  -rw-r--r--     6926   1-Jan-2010 00:00:00  chrome/browser/locale/de/app-marketplace-icons/ios.svg

The langpack is also sensible, containing:

  -rw-r--r--   4430   1-Jan-2010 00:00:00  browser/chrome/browser/locale/de/app-marketplace-icons/android.png
  -rw-r--r--   6926   1-Jan-2010 00:00:00  browser/chrome/browser/locale/de/app-marketplace-icons/ios.svg

All references to chrome://app-marketplace-icons/content/... will
need to be made locale-aware, I think; I haven't dug into that at all.
But this should be standard.

Comment 14

[Nick Alexander :nalexander [he/him] (PTO Feb 24-March 3)]

(In reply to Meg Viar from comment #12)

Hi :nalexander, thanks for pointing me in the right direction regarding creating shippable l10n lang repacks on try. This confirmed that, with the current patch, AB_CD evaluates to "en-US" when the new moz.build file I added to browser/themes/shared/app-marketplace-icons is evaluated, regardless of the repack locale.

I'm assuming the icons are packaged with the base en-US build and included in the final lang repacks as-is in English. I've been experimenting with how to ensure these assets are repackaged (?) with the new value for AB_CD when building lang repacks, but haven't had any luck. Do you have any pointers?

I've answered in the form of a patch, to be folded into yours: https://phabricator.services.mozilla.com/D147383. Try build is percolating at https://treeherder.mozilla.org/#/jobs?repo=try&revision=50ad4ec2cfcc013e06a6c63aa6829ac8b1e8e150. I've done just the Linux builds for now, but we can extend the run or we can show you how to do single-locale repacks locally so that you can grind through the chrome:// addressing changes that this approach will need.

We'll need to get :glandium's review on this; there may be simpler mechanisms than the one I found, which is fairly awful.

Comment 15

[Nick Alexander :nalexander [he/him] (PTO Feb 24-March 3)]

(In reply to Nick Alexander :nalexander [he/him] from comment #14)

(In reply to Meg Viar from comment #12)

Hi :nalexander, thanks for pointing me in the right direction regarding creating shippable l10n lang repacks on try. This confirmed that, with the current patch, AB_CD evaluates to "en-US" when the new moz.build file I added to browser/themes/shared/app-marketplace-icons is evaluated, regardless of the repack locale.

I'm assuming the icons are packaged with the base en-US build and included in the final lang repacks as-is in English. I've been experimenting with how to ensure these assets are repackaged (?) with the new value for AB_CD when building lang repacks, but haven't had any luck. Do you have any pointers?

I've answered in the form of a patch, to be folded into yours: https://phabricator.services.mozilla.com/D147383. Try build is percolating at https://treeherder.mozilla.org/#/jobs?repo=try&revision=50ad4ec2cfcc013e06a6c63aa6829ac8b1e8e150. I've done just the Linux builds for now, but we can extend the run or we can show you how to do single-locale repacks locally so that you can grind through the chrome:// addressing changes that this approach will need.

Try build looks healthy -- check the browser/omni.ja files in the ar locale, for example. You can read these with chrome://app-marketplace-icons/locale/android.png, for example.

Comment 16

[Ed Lee :Mardak]

Attachment: example cs firefox build

(In reply to Nick Alexander :nalexander [he/him] from comment #15)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=50ad4ec2cfcc013e06a6c63aa6829ac8b1e8e150

I grabbed a cs firefox build and indeed it showed the cs icons at chrome://app-marketplace-icons/locale/android.png and chrome://app-marketplace-icons/locale/ios.svg

Comment 17

[Ed Lee :Mardak]

Attachment: example da langpack on cs build

Then I installed the da langpack, and while the text did live update (except for about:privatebrowsing tab title remained cs), the images did not live update (although probably not high priority at this time), but showed up fine after refreshing it (no need to restart).

And just to confirm unzipping the langpack:

extracting: browser/chrome/browser/locale/da/app-marketplace-icons/android.png  
 inflating: browser/chrome/browser/locale/da/app-marketplace-icons/ios.svg  

🎉

Comment 18

[Nick Alexander :nalexander [he/him] (PTO Feb 24-March 3)]

(In reply to Ed Lee :Mardak from comment #17)

Created attachment 9278294 [details]
example da langpack on cs build

Then I installed the da langpack, and while the text did live update (except for about:privatebrowsing tab title remained cs), the images did not live update (although probably not high priority at this time), but showed up fine after refreshing it (no need to restart).

This is entirely expected: the Fluent integration is specific to various localized textual elements, and wouldn't cover things that just happen to be pulled from localized resources / langpacks. It's possible that mviar's initial patch does this live-updating, but I've undoubtedly broken that patch because the chrome:// URLs will need to be updated. It's also possible that you can convince Fluent to localize data- elements in a way that auto-updates the images, but I expect you'll have an easier time listening for intl:app-locales-changed observer notifications.

Comment 19

[Meg Viar [:mviar]]

Big thanks, everyone! I pulled @nalexander's patch into mine, adjusted the chrome references, and did some testing with local single-locale repacks. Everything looks good so far.

I'll investigate options for live-updating (keeping the recommendations re: observer notifications in mind) and request @glandium's review on the patch.

Comment 20

[Sam Foster [:sfoster] (he/him)]

If memory serves we had to get a legal review when we added 3rd-party images to m-c for showing mobile marketplace buttons (to install the Lockwise app) on about:logins. I don't see that has happened here yet? That was bug 1550165 for reference.

Comment 21

[Meg Viar [:mviar]]

@sfoster we received legal review on the original implementation from @mhoye in bug 1759759 (update added to about:license in D141022).

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/876e930054d4
Include localized app marketplace Icons in langpacks r=barret,glandium

Comment 23

[bszekely]

https://hg.mozilla.org/mozilla-central/rev/876e930054d4

Comment 24

[Alice Varro [avarro], Ecosystem QA]

I have verified this issue using Firefox Nightly 103.0a1 (Build ID:20220613215309) on Windows 10x64, macOS 12.3.1 and Linux Mint 20.2 x64 and I can confirm the following:

• For the es-AR build both badges are displayed in Spanish
• For the af build the Android badge is displayed in Afrikaans and the iOS badge is displayed in English
• For the de build both badges are displayed in German
• For the tl build both badges are displayed in English
• For the en-US build both badges are displayed in English