Last Comment Bug 693743 - 3rd party add-on check doesn't disable those add-ons on start-up, if they were installed into the profile or application folder
: 3rd party add-on check doesn't disable those add-ons on start-up, if they wer...
Status: VERIFIED FIXED
qa!
: dev-doc-complete, verified-aurora, verified-beta
Product: Toolkit
Classification: Components
Component: Add-ons Manager (show other bugs)
: 8 Branch
: All All
: -- normal with 1 vote (vote)
: mozilla10
Assigned To: Dave Townsend [:mossop]
:
Mentors:
: 691280 (view as bug list)
Depends on: 700615 696870 699175
Blocks: 476430 695436 696027
  Show dependency treegraph
 
Reported: 2011-10-11 12:38 PDT by Verdi [:verdi]
Modified: 2012-06-30 11:30 PDT (History)
28 users (show)
emorley: in‑testsuite+
hskupin: in‑litmus?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
fixed


Attachments
Third party add-ons in Firefox 8 (220.31 KB, image/jpeg)
2011-10-11 12:38 PDT, Verdi [:verdi]
no flags Details
extensions.sqlite from the profile folder (384.00 KB, application/octet-stream)
2011-10-11 12:40 PDT, Verdi [:verdi]
no flags Details
patch rev 1 (29.83 KB, patch)
2011-10-19 10:21 PDT, Dave Townsend [:mossop]
bmcbride: review+
Details | Diff | Splinter Review
patch rev 2 (33.98 KB, patch)
2011-10-20 14:48 PDT, Dave Townsend [:mossop]
bmcbride: review+
christian: approval‑mozilla‑aurora+
christian: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Verdi [:verdi] 2011-10-11 12:38:51 PDT
Created attachment 566298 [details]
Third party add-ons in Firefox 8

1. I did a clean install of Firefox 8.0b2 in Windows 7. It had a fresh profile. 
2. I then downloaded and installed a variety of programs that bundle Firefox add-ons:
* Skype - http://skype.com
* Weatherbug - http://weatherbug.com
* Free FLV player for Windows - http://applian.com/flvplayer/
* Advanced SystemCare - http://www.iobit.com/advancedsystemcareper.html
* PDFCreator - http://www.pdfforge.org/pdfcreator
* Defraggler - http://www.piriform.com/defraggler/download

3. After restarting Firefox 8 I did not see anything letting me know that add-ons had been installed or giving me an opportunity to opt-out. 
4. Firefox 8 has a lot of third party add-ons.
Comment 1 Verdi [:verdi] 2011-10-11 12:40:09 PDT
Created attachment 566300 [details]
extensions.sqlite from the profile folder
Comment 2 Dave Townsend [:mossop] 2011-10-11 18:40:04 PDT
Thanks for the updates, the problems here are all because the third party add-ons are getting installed to the profile or application directory, I'm trying to work out whether it's possible to solve this problem.
Comment 3 Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01) 2011-10-12 00:48:28 PDT
Nominating as tracking for 8, because blocking extensions like this was scheduled to be a Firefox 8 feature.
Comment 4 David Tenser [:djst] 2011-10-12 07:51:51 PDT
(In reply to Dave Townsend (:Mossop) from comment #2)
> Thanks for the updates, the problems here are all because the third party
> add-ons are getting installed to the profile or application directory, I'm
> trying to work out whether it's possible to solve this problem.

Just to make sure I understand this, you're saying that this is because all of the above extensions are installed in the profile folder rather than the app folder? 

How come Skype wasn't included in the QA of this feature? It's a very common extension.
Comment 5 Dave Townsend [:mossop] 2011-10-12 09:42:08 PDT
(In reply to David Tenser [:djst] from comment #4)
> (In reply to Dave Townsend (:Mossop) from comment #2)
> > Thanks for the updates, the problems here are all because the third party
> > add-ons are getting installed to the profile or application directory, I'm
> > trying to work out whether it's possible to solve this problem.
> 
> Just to make sure I understand this, you're saying that this is because all
> of the above extensions are installed in the profile folder rather than the
> app folder? 

The above extensions are all installed in either the profile folder or the app folder, we don't treat either of those as third party locations.
Comment 6 David Tenser [:djst] 2011-10-12 14:48:34 PDT
(In reply to Dave Townsend (:Mossop) from comment #5)
> The above extensions are all installed in either the profile folder or the
> app folder, we don't treat either of those as third party locations.

So that's also the trick for any malicious add-on to get installed without us disabling it?
Comment 7 Dave Townsend [:mossop] 2011-10-12 14:51:08 PDT
(In reply to David Tenser [:djst] from comment #6)
> (In reply to Dave Townsend (:Mossop) from comment #5)
> > The above extensions are all installed in either the profile folder or the
> > app folder, we don't treat either of those as third party locations.
> 
> So that's also the trick for any malicious add-on to get installed without
> us disabling it?

This was never going to protect against malicious add-ons
Comment 8 David E. Ross 2011-10-15 09:51:26 PDT
From the Summary, I infer this means that a third-party add-on that was previously enabled should be disabled every time the application does a start-up.  I hope this is not true and that I have misinterpreted the Summary.
Comment 9 Scoobidiver (away) 2011-10-15 10:05:28 PDT
(In reply to David E. Ross from comment #8)
> From the Summary, I infer this means that a third-party add-on that was
> previously enabled should be disabled every time the application does a
> start-up.
... soon after the add-on installation.
Comment 10 Henrik Skupin (:whimboo) 2011-10-17 07:00:14 PDT
It doesn't happen for each start of Firefox. The 3rd party check will only happen once with the upgrade from Firefox 7 to Firefox 8 (beta). Updating summary again to better reflect the behavior.
Comment 11 Dave Townsend [:mossop] 2011-10-17 07:10:52 PDT
Bug 693698 and bug 694575 are about the upgrade add-on selection UI, this bug is about normal Firefox startup
Comment 12 Henrik Skupin (:whimboo) 2011-10-17 07:17:44 PDT
Right. But isn't it the same underlying issue and upcoming fix to handle both of those cases? It's that we do not detect those add-ons as 3rd party add-ons. Or do we really need a separate patch here?
Comment 13 Dave Townsend [:mossop] 2011-10-17 07:19:06 PDT
(In reply to Henrik Skupin (:whimboo) from comment #12)
> Right. But isn't it the same underlying issue and upcoming fix to handle
> both of those cases? It's that we do not detect those add-ons as 3rd party
> add-ons. Or do we really need a separate patch here?

Yes, we need separate patches for each issue
Comment 14 Dave Townsend [:mossop] 2011-10-19 10:21:59 PDT
Created attachment 568106 [details] [diff] [review]
patch rev 1

This patch does two things. It detects and auto-disables any new add-ons that appear in the profile and application directories (except for the default theme) and it also marks them in the database as being thirdParty should we want to do anything clever with that in the future. This property is migrated and across app and add-on upgrades.

One downside here, if a user manually installs a new version of an add-on marked thirdParty then it will still be marked thirdParty. We don't really have a way to tell that a user did something explicitly or not for the upgrade case.
Comment 15 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2011-10-20 02:23:28 PDT
Comment on attachment 568106 [details] [diff] [review]
patch rev 1

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

::: toolkit/mozapps/extensions/XPIProvider.jsm
@@ +145,5 @@
>  const DB_METADATA        = ["installDate", "updateDate", "size", "sourceURI",
>                              "releaseNotesURI", "applyBackgroundUpdates"];
>  const DB_BOOL_METADATA   = ["visible", "active", "userDisabled", "appDisabled",
>                              "pendingUninstall", "bootstrap", "skinnable",
> +                            "softDisabled", "thirdParty"];

Will need to change DB_SCHEMA for this.

@@ +2558,2 @@
>          if (!newAddon) {
> +          //load the manifest from the add-on

Nit: Missing leading space, missing capital first letter, missing full stop.

@@ +2560,5 @@
>            let file = aInstallLocation.getLocationForID(aId);
>            newAddon = loadManifestFromFile(file);
> +
> +          // The default theme is never third party
> +          if (newAddon.type != "theme" || newAddon.internalName != XPIProvider.defaultSkin)

Shouldn't this be &&? Right now no themes can be marked as third party, which is contrary to the comment.

@@ +2587,5 @@
>        newAddon.installDate = aAddonState.mtime;
>        newAddon.updateDate = aAddonState.mtime;
>  
> +      // Check if the add-on is a third party install and is in a scope where
> +      // add-ons that were dropped in should default to disabled

Nit: Full stop.
Comment 16 Justin Wood (:Callek) 2011-10-20 06:29:51 PDT
(In reply to Blair McBride (:Unfocused) from comment #15)
> Comment on attachment 568106 [details] [diff] [review] [diff] [details] [review]
> patch rev 1

> @@ +2560,5 @@
> >            let file = aInstallLocation.getLocationForID(aId);
> >            newAddon = loadManifestFromFile(file);
> > +
> > +          // The default theme is never third party
> > +          if (newAddon.type != "theme" || newAddon.internalName != XPIProvider.defaultSkin)
> 
> Shouldn't this be &&? Right now no themes can be marked as third party,
> which is contrary to the comment.
> 

newAddon.type != "theme" || newAddon.internalName != XPIProvider.defaultSkin

is equal to:

!(newAddon.type == "theme" && newAddon.internalName == XPIProvider.defaultSkin)

Mossop has it right.
Comment 17 Dave Townsend [:mossop] 2011-10-20 14:48:08 PDT
Created attachment 568524 [details] [diff] [review]
patch rev 2

Updated from comments, I think || is right (though I actually did originally use &&) but I've added checks that themes are being marked correctly to verify. I also marked personas as not third party and plugins as always third party.

One final question, I wonder if externalInstall might be a better name since we are really talking about add-ons that were installed outside of Firefox?
Comment 18 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2011-10-20 16:51:08 PDT
(In reply to Dave Townsend (:Mossop) from comment #17)
> One final question, I wonder if externalInstall might be a better name since
> we are really talking about add-ons that were installed outside of Firefox?

Hm, unfortunately that term is already used for installs that don't use an install object (onExternalInstall). Which, in light of this, is an unfortunate name (onBasicInstall might have been better). Best alternative I can come up with is "foreignInstall".
Comment 19 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2011-10-20 16:55:59 PDT
(In reply to Dave Townsend (:Mossop) from comment #17)
> Updated from comments, I think || is right (though I actually did originally
> use &&) but I've added checks that themes are being marked correctly to
> verify. 

*sigh* Yea, my sleep-deprived mistake.

> I also marked personas as not third party and plugins as always
> third party.

I would have been fine with it being an optional property that defaulted to false, but that works too. Doing it this way makes the API feel more deliberate, which is nice.
Comment 20 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2011-10-20 17:03:35 PDT
Comment on attachment 568524 [details] [diff] [review]
patch rev 2

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

Random thought: Could use a hotfix to detect known thirdparty installs (from a pre-defined list we create) in the profile directory, and mark them as such in the database. Would fix that problem without the long-term maintenance cost of that code.

::: toolkit/mozapps/extensions/XPIProvider.jsm
@@ +4367,5 @@
>        sql.push("SELECT internal_id, id, location, userDisabled, " +
>                 "softDisabled, installDate, version, applyBackgroundUpdates, " +
> +               "sourceURI, releaseNotesURI, thirdParty FROM addon");
> +      sql.push("SELECT internal_id, id, location, userDisabled, " +
> +               "softDisabled, installDate, version, applyBackgroundUpdates, " +

I wonder if this strategy could be improved on (in a different bug, of course). Have a list of possible fields, and generate a query to get each one separately. Otherwise we're going to have to manually maintain a growing list of hand-written queries for the various possible combinations of fields.
Comment 21 Dave Townsend [:mossop] 2011-10-21 12:36:52 PDT
Landed on inbound, did the change to foreignInstall though as that felt nicer to me.
https://hg.mozilla.org/integration/mozilla-inbound/rev/15e15fead382
Comment 22 Dave Townsend [:mossop] 2011-10-21 12:49:02 PDT
This is going to require some docs and outreach I think. One implication of this change is that when devs develop their add-on by dropping it into the profile folder as is pretty common on the first startup Firefox will not enable it, instead showing the UI for letting the user choose to install this. If developers want to get around this they can set the pref extensions.autoDisableScopes to 14 in their profile.
Comment 23 Daniel Holbert [:dholbert] 2011-10-21 12:59:55 PDT
Backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/29edecdbbea7
because this was causing timeouts in crashtest, jsreftest, and reftest on Linux.
Comment 24 Henrik Skupin (:whimboo) 2011-10-21 15:17:05 PDT
(In reply to Dave Townsend (:Mossop) from comment #22)
> this. If developers want to get around this they can set the pref
> extensions.autoDisableScopes to 14 in their profile.

So for my fix on bug 696027 you have proposed to use 'extensions.autoDisableScopes=10'. Has something changed here or why I would have to use 14 now?
Comment 25 Dave Townsend [:mossop] 2011-10-21 16:03:02 PDT
(In reply to Henrik Skupin (:whimboo) from comment #24)
> (In reply to Dave Townsend (:Mossop) from comment #22)
> > this. If developers want to get around this they can set the pref
> > extensions.autoDisableScopes to 14 in their profile.
> 
> So for my fix on bug 696027 you have proposed to use
> 'extensions.autoDisableScopes=10'. Has something changed here or why I would
> have to use 14 now?

10 allows add-ons to be dropped into the profile or application, I figure that is probably the right choice for testing environments like mozmill, jetpack etc. 14 only allows add-ons to be dropped into the profile
Comment 26 Dave Townsend [:mossop] 2011-10-22 11:39:56 PDT
Second try: https://hg.mozilla.org/integration/mozilla-inbound/rev/66f51670652f
Comment 27 Ed Morley [:emorley] 2011-10-23 09:30:21 PDT
https://hg.mozilla.org/mozilla-central/rev/66f51670652f
Comment 28 Alex Vincent [:WeirdAl] 2011-10-24 14:05:21 PDT
Mossop, please clarify.  When a third-party extension is already present in the profile/extensions folder, and the user upgrades to a newer version, seeing the addons dialog:  will that third-party extension be grandfathered in as enabled, or will the default behavior be to disable that extension?
Comment 29 Dave Townsend [:mossop] 2011-10-24 20:49:17 PDT
(In reply to Alex Vincent [:WeirdAl] from comment #28)
> Mossop, please clarify.  When a third-party extension is already present in
> the profile/extensions folder, and the user upgrades to a newer version,
> seeing the addons dialog:  will that third-party extension be grandfathered
> in as enabled, or will the default behavior be to disable that extension?

This code doesn't affect add-ons that are already installed
Comment 30 christian 2011-10-25 20:10:43 PDT
Comment on attachment 568524 [details] [diff] [review]
patch rev 2

---------------------------------[ Triage Comment ]---------------------------------

Approving for 8beta and 9aurora as this is a good fix for a new feature and was approved via in person conversations.

Please land ASAP.
Comment 33 Virgil Dicu [:virgil] [QA] 2011-11-07 07:17:05 PST
Mozilla/5.0 (Windows NT 5.1; rv:8.0) Gecko/20100101 Firefox/8.0
Mozilla/5.0 (Windows NT 6.1; rv:8.0) Gecko/20100101 Firefox/8.0
Mozilla/5.0 (X11; Linux x86_64; rv:8.0) Gecko/20100101 Firefox/8.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:8.0) Gecko/20100101 Firefox/8.0

Verified fixed on Nightly, Aurora and Beta on Windows 7, Windows XP, Ubuntu 11.10, Mac Os 10.6 using Yontoo Layers third party add-on, Internet download manager and by manually copying add-ons in the profile directory on Ubuntu and Mac OS. The third party notification was displayed at start-up in separate tabs for each third party add-on as specified in http://people.mozilla.com/~faaborg/files/firefox4Mockups/userAddonSelection/addOnLocalInstall-i2.png
Comment 34 Henrik Skupin (:whimboo) 2011-11-07 07:25:06 PST
Dave, I assume some Litmus tests with real 3rd party add-ons would be kinda useful here.
Comment 35 Dave Townsend [:mossop] 2011-11-07 13:17:36 PST
(In reply to Henrik Skupin (:whimboo) from comment #34)
> Dave, I assume some Litmus tests with real 3rd party add-ons would be kinda
> useful here.

Yes I think so. The automated tests attempt to simulate this but a real-world test would be good for verification.
Comment 36 Gregory Szorc [:gps] 2011-11-14 17:59:06 PST
addon.foreignInstall needs to be documented at https://developer.mozilla.org/en/Addons/Add-on_Manager/Addon.
Comment 37 Dave Townsend [:mossop] 2012-01-01 13:22:04 PST
*** Bug 691280 has been marked as a duplicate of this bug. ***
Comment 38 Ed Morley [:emorley] 2012-05-22 06:28:56 PDT
https://hg.mozilla.org/mozilla-central/rev/9c36c1c74dfc
Comment 39 Florian Scholz [:fscholz] (MDN) 2012-06-30 11:30:37 PDT
(In reply to Gregory Szorc [:gps] from comment #36)
> addon.foreignInstall needs to be documented at
> https://developer.mozilla.org/en/Addons/Add-on_Manager/Addon.

Added foreignInstall to "Required properties".

Note You need to log in before you can comment on or make changes to this bug.