593187 - Request for authentication for an extension install doesn't focus the correct tab

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Dave Townsend [:mossop]]

If a background tab starts an extension install that requires authentication then the auth dialog should show and the tab in question focused to make it clear where the request is coming from. This is a problem because a website could open a seemingly safe site like Google and make the auth prompt look like it came from there

Comment 1

[Dave Townsend [:mossop]]

Attachment: automated test

This is an automated test that tests the problem.

Comment 2

[Dave Townsend [:mossop]]

Attachment: automated test

Comment 3

[Alon Zakai (:azakai)]

To be able to do this properly we need to be able to send window IDs over IPC - content should send an ID inside the current IPC message being sent, and chrome would find the ChromeWindow for that content and bring it to the front. Bug 593025 has a proposed patch for that.

Alternatively, we can try the hack Fennec uses with DOMWillOpenModalDialog. I don't know if this would work in Firefox though, and in any case it isn't foolproof (there can be race conditions, so the security problem would be improved but not fixed). So I'd prefer the previous approach.

Comment 4

[Alon Zakai (:azakai)]

Attachment: Patch

Patch that manually focuses the proper tab in response to an InstallTrigger message. Passes the automated test in Firefox.

Sadly, as mentioned in a comment in the patch, this can't be fixed in a proper way for the IPC case/Fennec. Bug 596109 should make that possible though.

Comment 5

[Dave Townsend [:mossop]]

Comment on attachment 475154 [details] [diff] [review]
Patch

This looks like it focuses the tab before the download even starts. That doesn't seem right at all

Comment 6

[Alon Zakai (:azakai)]

Would the proper behavior be to focus it together with showing the auth prompt?

Comment 7

[Dave Townsend [:mossop]]

That is what used to happen. The problem is that any delay allows for the possibility that the user (or another website) switches to a different tab in the meantime.

Comment 8

[Alon Zakai (:azakai)]

Hmm, sounds like the auth prompt isn't working right then.

We used to send a window to the auth prompt, but we stopped doing so in

http://hg.mozilla.org/mozilla-central/rev/552065e24bc8

Specifically

> return factory.getPrompt(this.window, Ci.nsIAuthPrompt);

has been changed in that revision to have null instead of this.window. Why?

Comment 9

[Dave Townsend [:mossop]]

(In reply to comment #8)
> Hmm, sounds like the auth prompt isn't working right then.
> 
> We used to send a window to the auth prompt, but we stopped doing so in
> 
> http://hg.mozilla.org/mozilla-central/rev/552065e24bc8
> 
> Specifically
> 
> > return factory.getPrompt(this.window, Ci.nsIAuthPrompt);
> 
> has been changed in that revision to have null instead of this.window. Why?

Because that code was broken, this.window is never defined.

Comment 10

[Alon Zakai (:azakai)]

Attachment: Patch v2

Restore this.window, and set it to a proper value.

Comment 11

[Dave Townsend [:mossop]]

Comment on attachment 471689 [details] [diff] [review]
automated test

Want to do the quick review on this.

Comment 12

[Dave Townsend [:mossop]]

Comment on attachment 475287 [details] [diff] [review]
Patch v2

>diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm
>--- a/toolkit/mozapps/extensions/XPIProvider.jsm
>+++ b/toolkit/mozapps/extensions/XPIProvider.jsm
>@@ -4080,16 +4080,17 @@ function AddonInstall(aCallback, aInstal
>   this.installLocation = aInstallLocation;
>   this.sourceURI = aUrl;
>   this.releaseNotesURI = aReleaseNotesURI;
>   this.hash = aHash;
>   this.loadGroup = aLoadGroup;
>   this.listeners = [];
>   this.existingAddon = aExistingAddon;
>   this.error = 0;
>+  this.window = aLoadGroup.notificationCallbacks.getInterface(Ci.nsIDOMWindow);

Tests are failing with this patch, I think you need to check that aLoadGroup isn't null here.

Comment 13

[Alon Zakai (:azakai)]

Comment on attachment 471689 [details] [diff] [review]
automated test

Works great. Only nit is some lines are >80 chars, but I guess in tests we don't care about that stuff.

Comment 14

[Alon Zakai (:azakai)]

Attachment: Patch v3

Added check for aLoadGroup being null.

(Sadly on my local machine, some of the extensions tests don't work anyhow, so I can't reliably run those tests. The Ubuntu modifications to Firefox perhaps?)

Comment 15

[Dave Townsend [:mossop]]

Comment on attachment 475336 [details] [diff] [review]
Patch v3

Looks good

Comment 16

[Dave Townsend [:mossop]]

Landed code and test: http://hg.mozilla.org/mozilla-central/rev/c9c3523a0771
http://hg.mozilla.org/mozilla-central/rev/7b2907b65967

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Dave, so we can call it fixed?

Comment 18

[Dave Townsend [:mossop]]

Oops, yes

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Dave, to verify the fix I would have to setup a HTTP Auth protected folder which contains the XPI? When running the test I have to switch to another tab before the auth dialog is fired. Is that correct?

Comment 20

[Dave Townsend [:mossop]]

(In reply to comment #19)
> Dave, to verify the fix I would have to setup a HTTP Auth protected folder
> which contains the XPI? When running the test I have to switch to another tab
> before the auth dialog is fired. Is that correct?

Yes that is correct.

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b12pre) Gecko/20110209 Firefox/4.0b12pre ID:20110209030359

I setup my own HTTP auth protected folder for the extension:
https://www.hskupin.info/mozilla/addons/auth/