241705 - XPInstall Permission Manager UI

Status: RESOLVED FIXED

(Firefox :: General, defect, P2)

Description

[Ben Goodger (use ben at mozilla dot org for email)]

This is the front end component of bug 240552. Provide a UI for manipulating the
list of whitelisted install sites.

Comment 1

[Daniel Veditz [:dveditz]]

*** Bug 246131 has been marked as a duplicate of this bug. ***

Comment 2

[Daniel Veditz [:dveditz]]

To be explicit (bug 246131) part of this UI will of course include a "blocked
install" indicator something like the blocked popup one.

Comment 3

[David E. Ross]

Suggest the UI be an addtion to the Software Installation pane at
Edit>Preferences>Advanced>Software Installation.  Alternatively, the UI should
be launched from a button on that pane.  

Comment 4

[Bradley Chapman (not reading bugmail, still gone but not forever)]

http://www.pryan.org/mozilla/site/TheOneKEA/extensions/xpiwhitelist_02.xpi

A credible attempt at UI. I added an overlay to the Advanced preference panel's
Software tree which adds a button entitled "Modify Whitelist..." - this, along
with the EM entry, provides access to the UI.

Basically all I have is a checkbox for xpinstall.whitelist.required and a
dynamic listbox for xpinstall.whitelist.add. Simple, but it works.

I can try patching it into the chrome, if anyone is interested.

Comment 5

[Bradley Chapman (not reading bugmail, still gone but not forever)]

http://www.pryan.org/mozilla/site/TheOneKEA/extensions/xpiwhitelist_021.xpi

A small cleanup that prevents duplicates getting into the list.

Comment 6

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: part 1

... notification area.

Comment 7

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: patch

more progress... remaining work: clean up and the exceptions dialog for
options.

Comment 8

[Ben Goodger (use ben at mozilla dot org for email)]

screenshot of progress: http://www.bengoodger.com/software/mb/blocked.png

Comment 9

[Bradley Chapman (not reading bugmail, still gone but not forever)]

Oh cool! You're going to overlay it right on top of the page!

BTW, v0.3.1 of my extension is available now on its own homepage, for anyone
interested in using it.

http://www.pryan.org/mozilla/site/TheOneKEA/xpiwhitelist/

Comment 10

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: full diffs

Comment 11

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: xpinstall patch

Comment 12

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 152588 [details] [diff] [review]
xpinstall patch

this basically adds observer notifications in three places:

1) the content handler for when an XPI link is clicked
2) InstallTrigger.install
3) InstallTrigger.installChrome

it passes the associated docshell as aSubject, so the FE can locate the
window/tab to show the notification above.

Comment 13

[Prognathous]

Judging from how many clueless IE users handle ActiveX installation dialogs, it
might be a good idea to not even display this dialog/toolbar if a site has bad
track record. This can be done using a predefined list of blacklisted domains
(not that there are any at the moment). 

For reference, you can find a very extensive list of IE-hostile sites here:
https://netfiles.uiuc.edu/ehowes/www/res/ie-spyad.zip

Prog.

Comment 14

[PikeUK]

See Bug 250445 for a bug with this feature, and Bug 250430, Bug 250431 & Bug
250423 for possible regressions (not so sure about the last one).

Comment 15

[Prognathous]

(In reply to comment #14)
> See Bug 250445 for a bug with this feature

This bug talks about Firefox using wrong terminology ("theme" instead of
"extension"). It does not suggest blacklisting domains (predefined or not). It
does mention *whitelisting*, but this is a completely different concept.

Are there any other bugs that might fit my suggestion?

Prog.

Comment 16

[PikeUK]

See also Bug 250488 for another regression.

p.s. Prog, my comment 14 was not in reply to your comment 13, it was just a list
of some problems caused by the check-in of this feature :-)

Comment 17

[Prognathous]

I've separated the request for blacklist functionality to another bug:
http://bugzilla.mozilla.org/show_bug.cgi?id=250854

Prog.

Comment 18

[Daniel Veditz [:dveditz]]

Comment on attachment 152588 [details] [diff] [review]
xpinstall patch

>@@ -265,17 +266,22 @@ nsInstallTrigger::HandleContent(const ch
>-        // TODO: fire event signaling blocked Install attempt
>+        nsCOMPtr<nsIObserverService> os(do_GetService("@mozilla.org/observer-service;1"));
>+        if (os) {
>+            os->NotifyObservers(globalObject->GetDocShell(), 
>+                                "xpinstall-install-blocked", 
>+                                NS_LITERAL_STRING("install-chrome").get());
>+        }

This should be "install" rather than install-chrome, HandleContent will only
get full .xpi's, never a theme install. Test by clicking on a .xpi link.

Could we instead pass the urispec in this argument instead? This would give the
information bar the information it would need to be able to install that one
item without making the user whitelist the site for the future.

>@@ -170,18 +172,25 @@ InstallTriggerGlobalInstall(JSContext *c
>@@ -326,18 +335,25 @@ InstallTriggerGlobalInstallChrome(JSCont

You also need this in InstallTriggerGlobalStartSoftwareUpdate

>-  if (!enabled || !globalObject)
>+  if (!enabled || !globalObject) {

nit: predominant style here is brace below if

>@@ -326,18 +335,25 @@ InstallTriggerGlobalInstallChrome(JSCont
>-  if (!enabled || !globalObject)
>+  if (!enabled || !globalObject) {

nit: ditto

>+      nsCOMPtr<nsIObserverService> os(do_GetService("@mozilla.org/observer-service;1"));
>+      if (os) {
>+          os->NotifyObservers(globalObject->GetDocShell(), 
>+                              "xpinstall-install-blocked", 
>+                              NS_LITERAL_STRING("install-chrome").get());
>+      }

You need to move the UpdateEnabled check after argument parsing -- if the type
is anything other than SKIN this should be exactly the same as the other
installs. Unless you change the 3rd parameter to be the urispec, in which case
you can lump chrome installs together since they can't be restarted from just
the urispec.

One thing that troubles me is that if the user has turned off software installs
entirely they'll still get these notifications. Could be left for later I
suppose, most people won't turn off installs. One solution might be to return
more than just a boolean from UpdateEnabled(), using different codes to
indicate whether the feature is off or the site was blocked.

Rather than sending a notification in 4 different places, could this be moved
inside AllowInstall() itself? Then we'd have to pass a flag into
UpdateEnabled() about whether we're trying an install, or just the web site
asking about it.

r- predominantly because of the missing startSoftwareUpdate spot and the
"install-chrome" in HandleContent. You probably don't have the bandwidth to
re-write this following the other suggestions and that's OK, I'll look into
that on the trunk.

Comment 19

[Michael Lefevre]

Some of the "clean up" that happened in pref-features.js actually changed the
image checkboxes worked and caused bug 252953.

Comment 20

[chris hofmann]

sounds like this is not on the branch.  should fixed-aviary1.0 be removed from
status whiteboard?  if fixed it should be in the keyword field

Comment 21

[Brian King [:kinger]]

I'm seeing it on the branch (20040725):

Tools->Options->Web Features->Allow web sites to install software->Exceptions

Comment 22

[Doug Wright]

If I go Options->Web Features I see a box marked 'Allow Web sites to install
software'. Then I see a button marked exceptions. This implies that all websites
can install software (without permission?) EXCEPT those listed seperately  
which are blocked (although the manager is worded correctly). Same goes for load
images (although not under the scope of this bug).

Comment 23

[Brian King [:kinger]]

Agreed, the UI is not intuitive. The wording does not even mention extensions,
so doesn't tie in very well with Tools->Extensions. "Allow web sites to install
software" could be any ol' software in the users mind (i.e. nasty executables).

Comment 24

[Doug Wright]

bug 250543 deals with the poor wording

Comment 25

[David E. Ross]

The Risks Digest 23.49 contains an item at
<http://catless.ncl.ac.uk/Risks/23.49.html#subj9> cites a hostile exploit of the
problem this UI is intended to fix.  The exploit is detailed at
<http://www.squarefree.com/archives/000487.html>.  

I notice this bug is an Aviary blocker.  Does that include blocking Mozilla 1.8?
 If not, it should.  

Comment 26

[Kathleen :Brade]

I am not sure if this idea has been proposed, or if this is the best bug to
mention it in but... someone looking over my shoulder just mentioned that as you
exit after installing one or more extensions, Firefox should present a list of
the extensions that will be installed upon restart (and allow you to change your
mind by removing one or all from the list).  Of course this would be another
annoyance to people who install a lot of extensions but it might help people
notice and "undo" an undesired pending install.

Comment 27

[Worcester12345]

Need to change the target milestone since the one there has now passed.

Comment 28

[Steffen Wilberg]

Branch checkin was 2004-07-08 00:54.

Trunk checkin was mostly with the aviary landing, with the exception of:
- mozilla/xpcom/windbgdlg/windbgdlg.cpp
- mozilla/xpinstall/src/nsJSInstallTriggerGlobal.cpp

Comment 29

[Worcester12345]

(In reply to comment #28)
> Branch checkin was 2004-07-08 00:54.
> 
> Trunk checkin was mostly with the aviary landing, with the exception of:
> - mozilla/xpcom/windbgdlg/windbgdlg.cpp
> - mozilla/xpinstall/src/nsJSInstallTriggerGlobal.cpp

Any idea on the ETA for those?

Comment 30

[Steffen Wilberg]

I guess this is fixed on trunk finally by bug 274271.

Comment 31

[Steffen Wilberg]

Removing aviary-landing keyword.