"Unable to find application to perform this action" when tapping on downloaded XPI file in Android "My Files" view

RESOLVED FIXED in Firefox 31

Status

()

Firefox for Android
Add-on Manager
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: nalexander, Assigned: esawin)

Tracking

Trunk
Firefox 31
All
Android
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 7 obsolete attachments)

(Reporter)

Description

4 years ago
I downloaded rnewman's locale-switcher.xpi from https://github.com/rnewman/locale-switch/blob/master/locale-switcher.xpi.

When I navigate to the XPI file in Android's "My Files" app and tap it, I get "Unable to find application to perform this action".  Presumably we need to register an intent handler for this file type.

(Firefox for Android didn't recognize the .xpi as an installable add-on; I think this is because the link isn't to the raw version and/or the mime-type is not correct.  This could be a bug, but I think not.)
(Reporter)

Updated

4 years ago
See Also: → bug 950420
This should be a pretty small patch, albeit with some testing.

http://stackoverflow.com/questions/1733195/android-intent-filter-for-a-particular-file-extension
Keywords: polish
(Assignee)

Comment 2

4 years ago
Created attachment 8384620 [details] [diff] [review]
Enable XPI file install (WIP)

Here is an early draft for the patch.

Finished:
* intent-filter for XPI files
* Java -> JS event passing
* JS install routine

Todo:
* proper install procedure with install prompt
* cleaning up

Any feedback is welcome. Do we want to shortcut the procedure and handle it directly in JS when the XPI file extension is detected? That way we would avoid the JS -> MIME detection -> intent filtering -> Java -> JS event passing.
Assignee: nobody → esawin
Attachment #8384620 - Flags: feedback?(rnewman)
Comment on attachment 8384620 [details] [diff] [review]
Enable XPI file install (WIP)

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

Do we really need all this special casing for the xpi? Won't opening that URI trigger all the add-on machinery "automatically"? It seems like all we need is the intent-filter...

::: mobile/android/base/AndroidManifest.xml.in
@@ +159,5 @@
>                  <category android:name="android.intent.category.DEFAULT" />
>              </intent-filter>
>          </activity>
>  
> +        <activity android:name="org.mozilla.gecko.webapp.AddonInstaller" >

I don't think we need a separate activity here. Just stick it with the other browser intents.

@@ +163,5 @@
> +        <activity android:name="org.mozilla.gecko.webapp.AddonInstaller" >
> +            <intent-filter>
> +                <action android:name="android.intent.action.VIEW" />
> +                <category android:name="android.intent.category.DEFAULT" />
> +                <data android:scheme="file" />

We should be able to handle non-file schemes, right?
(Assignee)

Updated

4 years ago
Attachment #8384620 - Flags: feedback?(rnewman)
(Assignee)

Comment 4

4 years ago
Created attachment 8384697 [details] [diff] [review]
Add XPI intent filter

The proper solution without adding a new activity.
Attachment #8384620 - Attachment is obsolete: true
Attachment #8384697 - Flags: review?(snorp)
Attachment #8384697 - Flags: review?(snorp) → review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/7e424486f60e
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
We need to back this patch out. Sadly it is an attack surface for bad APKs to inject bad XPIs into Firefox.

We can look into supporting this approach when we block this by default, and have a better prompt for letting the user add an exception.
Whiteboard: [fixed-in-fx-team]
We need a way to trigger the "Fennec prevented this site from asking you..." doorhanger. That doorhanger allows the user to make an exception for this add-on. The code to show the doorhanger is here:
https://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#5675

We need to get the AddonManager to fire the notification.
"addon-install-blocked" is fired from onWebInstallBlocked here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/amWebInstallListener.js#293

Which is called from installAddonsFromWebpage here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/AddonManager.jsm#1751

And installAddonsFromWebpage is used in a few places that are not webpages. Like cmd_installFromFile here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/content/extensions.js#1137

Can we do the same somehow?
(Assignee)

Comment 10

4 years ago
Created attachment 8385305 [details] [diff] [review]
Enable XPI file install (WIP)

This version prevents the add-on from installing, shows a doorhanger warning, on |Allow| it proceeds with the installation prompt.
Installing from local files is whitelisted by default, which disables the doorhanger warning, but enabled on mobile (Android, Metro).

Some details need discussion here:
1. whitelisting defaults
2. disabling whitelisting for Android is done on |browser.js| load, is there a better location to do it?
3. doorhanger text
4. with |xpinstall.whitelist.required| set to |false|, local file installation is automatically whitelisted, do we want to decouple that?
Attachment #8384697 - Attachment is obsolete: true
Attachment #8385305 - Flags: feedback?(wjohnston)
Attachment #8385305 - Flags: feedback?(snorp)
(Assignee)

Updated

4 years ago
Status: NEW → ASSIGNED
Comment on attachment 8385305 [details] [diff] [review]
Enable XPI file install (WIP)

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

Looks reasonable to me, but I'm not an expert.
Attachment #8385305 - Flags: feedback?(snorp) → feedback+
Comment on attachment 8385305 [details] [diff] [review]
Enable XPI file install (WIP)

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

Mossop will need to review the toolkit parts of this (or someone on his team who's a peer in this code.

::: browser/metro/profile/metro.js
@@ +232,5 @@
>  pref("xpinstall.whitelist.add", "addons.mozilla.org");
>  pref("extensions.autoupdate.enabled", false);
>  pref("extensions.update.enabled", false);
> +// Disable whitelisting of add-on installations from local files.
> +pref("xpinstall.whitelist.localFile", false);

I don't really think Metro needs this. I guess thats up to their drivers. For context, after talking to Mossop yesterday the reason this UI exists is to prevent pages from spamming the XPIInstall API and showing users tons of prompts. Its does not provide extra security.

The main reason we've been talking about it on Android is that people seem extra paranoid about drive-by installs there, even if the installs have prompts in them before they ever run any downloaded code. I think that comes from a lack of understanding of what an APK is there (and parity with other browsers). I'm not sure if Metro has seen the same level of paranoia.

cc'ing Matt to make sure he sees this though.

::: mobile/android/chrome/content/browser.js
@@ +82,5 @@
>                                    "resource://gre/modules/WebappManager.jsm");
>  #endif
>  
> +// Disable whitelisting of add-on installations from local files.
> +Services.prefs.setBoolPref("xpinstall.whitelist.localFile", false);

Left over?

@@ +5682,5 @@
>          if (!tab)
>            return;
>  
>          let host = installInfo.originatingURI.host;
> +        let isLocalInstall = !host;

I think I'd use a real test for a local file here... or change this variable name.

@@ +5711,5 @@
>            }
>          } else {
>            notificationName = "xpinstall";
> +          if (isLocalInstall) {
> +            let addonName = "unknown";

You'd have to localize this. But again, I'd pick your mesage here based on what info you had available, and not based on "is this a local file" or not. If we have a host, I would probably prefer to show it (so that you can know who's spamming you if someone is).

::: toolkit/mozapps/extensions/amContentHandler.js
@@ +65,5 @@
>          referer = aRequest.getPropertyAsInterface("docshell.internalReferrer",
>                                                  Ci.nsIURI);
>        }
> +      // Local files need an artificial referer, we use the URI for that.
> +      referer = referer || uri;

This little bit may get you in trouble with Mossop. Its going to affect desktop platforms as well, and I think they've decided explicitly not to show these prompts.
Attachment #8385305 - Flags: feedback?(wjohnston)
Attachment #8385305 - Flags: feedback?(mbrubeck)
Attachment #8385305 - Flags: feedback+
Comment on attachment 8385305 [details] [diff] [review]
Enable XPI file install (WIP)

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

::: browser/metro/profile/metro.js
@@ +232,5 @@
>  pref("xpinstall.whitelist.add", "addons.mozilla.org");
>  pref("extensions.autoupdate.enabled", false);
>  pref("extensions.update.enabled", false);
> +// Disable whitelisting of add-on installations from local files.
> +pref("xpinstall.whitelist.localFile", false);

Indeed, we don't need this.  We do not even use XPIProvider in Firefox for Metro (bug 946296), so this should have no effect currently.  When we do enable XPI add-ons, they will likely be shared with desktop Firefox.
Attachment #8385305 - Flags: feedback?(mbrubeck)
(Assignee)

Comment 14

4 years ago
Created attachment 8385733 [details] [diff] [review]
Enable XPI file install

With this patch, add-on installations from XPI files will be blocked (doorhanger), if |xpinstall.whitelist.localFile| is set to false. We set it to false for Android; it defaults to true for desktop, leaving the procedure there untouched. For this, we set the |referer| in |amContentHandler| to the URI for direct file installs, which has no effect on desktop, because file referrer are whitelisted there.
Attachment #8385305 - Attachment is obsolete: true
Attachment #8385733 - Flags: review?(wjohnston)
Attachment #8385733 - Flags: review?(dtownsend+bugmail)
Comment on attachment 8385733 [details] [diff] [review]
Enable XPI file install

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

My understanding is that the goal is to always show the doorhanger even for installs requested through the intent for http or file locations. I think this fixes it for file but not http where you aren't setting the referer. I'd like to see tests verifying that both of these cases do what is expected on when the pref is set. The xpinstall tests should be a straightforward way of doing that, a modification of browser_localfile.js for the file case for example.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm
@@ +3663,2 @@
>        return false;
> +    }

Please don't add unnecessary bracing
Attachment #8385733 - Flags: review?(dtownsend+bugmail) → review-
(Assignee)

Updated

4 years ago
Attachment #8385733 - Flags: review?(wjohnston)
(Assignee)

Comment 16

4 years ago
Created attachment 8387184 [details] [diff] [review]
Enable direct request add-on installs

Added tests for blocked installations from file: and http: locations.
Tests for the default non-blocked case were already available.
Test results:
1. https://tbpl.mozilla.org/?tree=Try&rev=ff32e94bd616
2. https://tbpl.mozilla.org/?tree=Try&rev=fea659157169

With this patch, an installation request invoked by opening an XPI file by directly calling the URL without a referrer is blocked if |xpinstall.whitelist.directRequest| is set to |false| and allowed otherwise.
When blocked, an appropriate doorhanger message is displayed.
Attachment #8385733 - Attachment is obsolete: true
Attachment #8387184 - Flags: review?(wjohnston)
Attachment #8387184 - Flags: review?(dtownsend+bugmail)
Comment on attachment 8387184 [details] [diff] [review]
Enable direct request add-on installs

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

Sorry, looks like this does need to be slightly more complicated since we're dealing with changing both the direct request case and the local file case.

::: toolkit/mozapps/extensions/amContentHandler.js
@@ +96,1 @@
>        }

You should check but I think with the directRequest test in XPIProvider.jsm you might not need to make this change here.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm
@@ +3667,5 @@
> +        return true;
> +
> +      // chrome: and file: don't need whitelisted hosts.
> +      if (aUri.schemeIs("chrome") || aUri.schemeIs("file"))
> +        return true;

By having both of these in the same if block you're making the directRequest pref serve two different purposes. This tends to cause problems down the road when we don't realise that changing the pref to change one thing is also changing something else so I'd like to avoid that by having a second pref to control whether "local" installs are whitelisted. The pref from your previous patch should be good for that.

With that you'll probably want an extra local file test. One that verifies that a local page trying to install a local file is blocked with directRequest true and localFile false.
Attachment #8387184 - Flags: review?(dtownsend+bugmail) → review-
(Assignee)

Comment 18

4 years ago
(In reply to Dave Townsend (:Mossop) from comment #17)
> Sorry, looks like this does need to be slightly more complicated since we're
> dealing with changing both the direct request case and the local file case.
With this patch, we add mechanics to block direct requests (requests without a referrer host), independent of the scheme used.
If having |chrome:| and |file:| referrer hosts is an actual use case, we can change the handling without introducing another preference variable. We can also add an additional preference for local file referrer whitelisting, if that is useful.

> You should check but I think with the directRequest test in XPIProvider.jsm
> you might not need to make this change here.
If |directRequest| is set to true or is undefined, we preserve the previous code path, which automatically whitelisted all direct requests with |referer == null|. Without the check in |amContentHandler.js|, we would set the referrer to the target URL and therefore disable direct request whitelisting.
Flags: needinfo?(dtownsend+bugmail)
(In reply to Eugen Sawin [:esawin] from comment #18)
> (In reply to Dave Townsend (:Mossop) from comment #17)
> > Sorry, looks like this does need to be slightly more complicated since we're
> > dealing with changing both the direct request case and the local file case.
> With this patch, we add mechanics to block direct requests (requests without
> a referrer host), independent of the scheme used.
> If having |chrome:| and |file:| referrer hosts is an actual use case, we can
> change the handling without introducing another preference variable. We can
> also add an additional preference for local file referrer whitelisting, if
> that is useful.

I was under the impression that you wanted other apps to be able to instruct Fennec to install an XPI from the local filesystem. If that isn't the case then just remove it from the if block.

> > You should check but I think with the directRequest test in XPIProvider.jsm
> > you might not need to make this change here.
> If |directRequest| is set to true or is undefined, we preserve the previous
> code path, which automatically whitelisted all direct requests with |referer
> == null|. Without the check in |amContentHandler.js|, we would set the
> referrer to the target URL and therefore disable direct request whitelisting.

I meant you could leave the referer URL null, i.e. not make any changes at all in amContentHandler.js.
Flags: needinfo?(dtownsend+bugmail)
(Assignee)

Updated

4 years ago
Attachment #8387184 - Flags: review?(wjohnston)
(Assignee)

Comment 20

4 years ago
Created attachment 8390484 [details] [diff] [review]
Enable direct request add-on installs

After evaluating the other options, I full agree with comment 17.
This patch provides two additional preference variables, one for direct requests and the other for requests from local file referrers.

Tests for the new blocking cases were added and show green: https://tbpl.mozilla.org/?tree=Try&rev=4cfc42bb8de7.

Issues:
I think we have a mild case of bit rot here, as something has changed in the web install procedures, which prevents the add-on install info to be passed to the event handler in the case of direct requests. The effect is, that we don't have the names of the add-ons to address in the doorhanger message (name is |null|).
I'm looking into it, but it shouldn't require changes to this patch.
Attachment #8387184 - Attachment is obsolete: true
Attachment #8390484 - Flags: feedback?(dtownsend+bugmail)
Comment on attachment 8390484 [details] [diff] [review]
Enable direct request add-on installs

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

I'm happy with this patch from my side. I don't need to see it again unless there are changes made.

::: mobile/android/locales/en-US/chrome/browser.properties
@@ +70,5 @@
>  blockPopups.label=Block Popups
>  
>  # XPInstall
>  xpinstallPromptWarning2=%S prevented this site (%S) from asking you to install software on your device.
> +xpinstallPromptWarning2Local=%S prevented this add-ons (%S) from installing on your device.

s/this add-ons/this add-on/

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm
@@ +3453,5 @@
> +   *
> +   * @return true if installing by direct requests is whitelisted
> +   */
> +  isDirectRequestWhitelisted: function XPI_isDirectRequestWhitelisted() {
> +    // Default to enabled if the preference does not exist.

Change enabled to whitelisted here, same below
Attachment #8390484 - Flags: feedback?(dtownsend+bugmail) → review+
(Assignee)

Comment 22

4 years ago
Created attachment 8392355 [details] [diff] [review]
Enable direct request add-on installs

Adjusted doorhanger message since last patch version.

For direct install requests from local filesystem, we address the add-on directly using its name; for direct requests via HTTP, we don't have the add-on manifest yet, so we just show a generic doorhanger message.
Attachment #8390484 - Attachment is obsolete: true
Attachment #8392355 - Flags: review?(wjohnston)
Attachment #8392355 - Flags: feedback?(mark.finkle)
Comment on attachment 8392355 [details] [diff] [review]
Enable direct request add-on installs

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

Seems good. Sorry for the delay.

::: mobile/android/base/AndroidManifest.xml.in
@@ +150,5 @@
> +                <data android:scheme="https" />
> +                <data android:mimeType="application/x-xpinstall" />
> +            </intent-filter>
> +
> +            <!-- For XPI installs from external file explorers. -->

These could be coming from other apps just firing intents even. I'd just say "from file:// urls"

::: mobile/android/chrome/content/browser.js
@@ +5732,5 @@
> +            // We have a host which asked for the install.
> +            message = strings.formatStringFromName("xpinstallPromptWarning2", [brandShortName, host], 2);
> +          } else {
> +            if (installInfo.installs.length < 0) {
> +              // Don't show doorhanger if there are no installs.

Should "< 0" be "<= 0"? Also, does this happen? A comment explaining why would be a little nicer.

@@ +5734,5 @@
> +          } else {
> +            if (installInfo.installs.length < 0) {
> +              // Don't show doorhanger if there are no installs.
> +              return;
> +            }

Add a a blank line
Attachment #8392355 - Flags: review?(wjohnston) → review+
(In reply to Wesley Johnston (:wesj) from comment #23)
> ::: mobile/android/chrome/content/browser.js
> @@ +5732,5 @@
> > +            // We have a host which asked for the install.
> > +            message = strings.formatStringFromName("xpinstallPromptWarning2", [brandShortName, host], 2);
> > +          } else {
> > +            if (installInfo.installs.length < 0) {
> > +              // Don't show doorhanger if there are no installs.
> 
> Should "< 0" be "<= 0"? Also, does this happen? A comment explaining why
> would be a little nicer.

I don't think this should ever happen, if it does please file another bug so we can fix it.
Comment on attachment 8392355 [details] [diff] [review]
Enable direct request add-on installs

The approach looks fine. I'll leave Wes and Mossop to handle the correctness.
Attachment #8392355 - Flags: feedback?(mark.finkle) → feedback+
(Assignee)

Comment 26

4 years ago
(In reply to Dave Townsend (:Mossop) from comment #24)
> (In reply to Wesley Johnston (:wesj) from comment #23)
> > ::: mobile/android/chrome/content/browser.js
> > @@ +5732,5 @@
> > > +            // We have a host which asked for the install.
> > > +            message = strings.formatStringFromName("xpinstallPromptWarning2", [brandShortName, host], 2);
> > > +          } else {
> > > +            if (installInfo.installs.length < 0) {
> > > +              // Don't show doorhanger if there are no installs.
> > 
> > Should "< 0" be "<= 0"? Also, does this happen? A comment explaining why
> > would be a little nicer.
> 
> I don't think this should ever happen, if it does please file another bug so
> we can fix it.

Right, that is supposed to be a |< 1| or |== 0| and should never happen.

The current code path prevents the case in http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/addonManager.js#79-80.
However, should we ever receive the event from a different code path in future, which may have similar handling as in http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/addonManager.js#100-101 but without the check, it would be safer to keep the check in |browser.js|.
(Assignee)

Comment 27

4 years ago
Created attachment 8393743 [details] [diff] [review]
Enable direct request add-on installs

Fixed issues from review.
Attachment #8392355 - Attachment is obsolete: true
Attachment #8393743 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: polish → checkin-needed
(Assignee)

Updated

4 years ago
Duplicate of this bug: 950420
https://hg.mozilla.org/integration/fx-team/rev/cb74aa26cb49
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/cb74aa26cb49
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 31
Can we please use orderer variables when there are multiple elements in a string?

> xpinstallPromptWarningLocal=%S prevented this add-on (%S) from installing on your device.
%1$S prevented this add-on (%2$S) from installing on your device.

Localizers can switch between the two forms, but not all of them are aware of this.

I'm aware that's not the current style in mobile/browser.properties, but that should be fixed at least in new strings (check /browser/browser.properties for example).
Depends on: 986458
You need to log in before you can comment on or make changes to this bug.