Closed Bug 693743 Opened 13 years ago Closed 13 years ago

3rd party add-on check doesn't disable those add-ons on start-up, if they were installed into the profile or application folder

Categories

(Toolkit :: Add-ons Manager, defect)

8 Branch
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla10
Tracking Status
firefox8 + fixed
firefox9 --- fixed

People

(Reporter: verdi, Assigned: mossop)

References

Details

(Keywords: dev-doc-complete, verified-aurora, verified-beta, Whiteboard: qa!)

Attachments

(3 files, 1 obsolete file)

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.
Attachment #566300 - Attachment mime type: text/plain → application/octet-stream
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.
Nominating as tracking for 8, because blocking extensions like this was scheduled to be a Firefox 8 feature.
OS: Windows 7 → All
Hardware: x86 → All
Summary: Newly installed 3rd Party Add-ons are not disabled on startup → Some 3rd party add-ons are getting installed into the profile and are not disabled on start-up
(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.
(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.
(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?
(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
Assignee: nobody → dtownsend
Summary: Some 3rd party add-ons are getting installed into the profile and are not disabled on start-up → 3rd party add-ons installed into the profile or application directory and are not disabled on start-up
Summary: 3rd party add-ons installed into the profile or application directory and are not disabled on start-up → 3rd party add-ons installed into the profile or application folder are not disabled on start-up
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.
(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.
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.
Status: NEW → ASSIGNED
Summary: 3rd party add-ons installed into the profile or application folder are not disabled on start-up → 3rd party add-on check doesn't disable those add-ons on start-up, if they were installed into the profile or application folder
Bug 693698 and bug 694575 are about the upgrade add-on selection UI, this bug is about normal Firefox startup
No longer depends on: 693698, 694575
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?
(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
Blocks: 695436
No longer blocks: 695435
Attached patch patch rev 1 (obsolete) — Splinter Review
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.
Attachment #568106 - Flags: review?(bmcbride)
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.
Attachment #568106 - Flags: review?(bmcbride) → review+
(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.
Attached patch patch rev 2Splinter Review
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?
Attachment #568106 - Attachment is obsolete: true
Attachment #568524 - Flags: review?(bmcbride)
(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".
(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 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.
Attachment #568524 - Flags: review?(bmcbride) → review+
Landed on inbound, did the change to foreignInstall though as that felt nicer to me.
https://hg.mozilla.org/integration/mozilla-inbound/rev/15e15fead382
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.
Keywords: dev-doc-needed
Backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/29edecdbbea7
because this was causing timeouts in crashtest, jsreftest, and reftest on Linux.
(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?
(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
https://hg.mozilla.org/mozilla-central/rev/66f51670652f
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
Attachment #568524 - Flags: approval-mozilla-beta?
Attachment #568524 - Flags: approval-mozilla-aurora?
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?
(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 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.
Attachment #568524 - Flags: approval-mozilla-beta?
Attachment #568524 - Flags: approval-mozilla-beta+
Attachment #568524 - Flags: approval-mozilla-aurora?
Attachment #568524 - Flags: approval-mozilla-aurora+
Depends on: 696870
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
Status: RESOLVED → VERIFIED
Whiteboard: qa!
Dave, I assume some Litmus tests with real 3rd party add-ons would be kinda useful here.
Flags: in-litmus?
(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.
Depends on: 700615
addon.foreignInstall needs to be documented at https://developer.mozilla.org/en/Addons/Add-on_Manager/Addon.
Depends on: 699175
(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".

Restricting comments due to spam.

Restrict Comments: true
You need to log in before you can comment on or make changes to this bug.