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

VERIFIED FIXED in mozilla2.0b7

Status

()

Toolkit
Add-ons Manager
VERIFIED FIXED
7 years ago
7 years ago

People

(Reporter: mossop, Assigned: azakai)

Tracking

({regression})

Trunk
mozilla2.0b7
regression
Points:
---
Bug Flags:
in-testsuite +
in-litmus -

Firefox Tracking Flags

(blocking2.0 betaN+, status1.9.2 unaffected, status1.9.1 unaffected)

Details

(Whiteboard: [sg:low spoof])

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

7 years ago
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
(Reporter)

Comment 1

7 years ago
Created attachment 471675 [details] [diff] [review]
automated test

This is an automated test that tests the problem.
(Reporter)

Updated

7 years ago
blocking2.0: --- → betaN+
(Reporter)

Comment 2

7 years ago
Created attachment 471689 [details] [diff] [review]
automated test
Attachment #471675 - Attachment is obsolete: true
status1.9.1: --- → unaffected
status1.9.2: --- → unaffected
Whiteboard: [sg:low spoof]
(Assignee)

Comment 3

7 years ago
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.
Depends on: 593025
(Assignee)

Comment 4

7 years ago
Created attachment 475154 [details] [diff] [review]
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.
Attachment #475154 - Flags: review?(dtownsend)
(Assignee)

Updated

7 years ago
No longer depends on: 593025
(Reporter)

Comment 5

7 years ago
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
(Assignee)

Comment 6

7 years ago
Would the proper behavior be to focus it together with showing the auth prompt?
(Reporter)

Comment 7

7 years ago
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.
(Assignee)

Comment 8

7 years ago
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?
(Reporter)

Comment 9

7 years ago
(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.
(Assignee)

Comment 10

7 years ago
Created attachment 475287 [details] [diff] [review]
Patch v2

Restore this.window, and set it to a proper value.
Attachment #475154 - Attachment is obsolete: true
Attachment #475287 - Flags: review?(dtownsend)
Attachment #475154 - Flags: review?(dtownsend)
(Reporter)

Comment 11

7 years ago
Comment on attachment 471689 [details] [diff] [review]
automated test

Want to do the quick review on this.
Attachment #471689 - Flags: review?(azakai)
(Reporter)

Comment 12

7 years ago
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.
Attachment #475287 - Flags: review?(dtownsend) → review-
(Assignee)

Comment 13

7 years ago
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.
Attachment #471689 - Flags: review?(azakai) → review+
(Assignee)

Comment 14

7 years ago
Created attachment 475336 [details] [diff] [review]
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?)
Attachment #475287 - Attachment is obsolete: true
Attachment #475336 - Flags: review?(dtownsend)
(Reporter)

Comment 15

7 years ago
Comment on attachment 475336 [details] [diff] [review]
Patch v3

Looks good
Attachment #475336 - Flags: review?(dtownsend) → review+
(Reporter)

Comment 16

7 years ago
Landed code and test: http://hg.mozilla.org/mozilla-central/rev/c9c3523a0771
http://hg.mozilla.org/mozilla-central/rev/7b2907b65967
Assignee: nobody → azakai
Flags: in-testsuite+
Flags: in-litmus-
Target Milestone: --- → mozilla2.0b7
Dave, so we can call it fixed?
(Reporter)

Comment 18

7 years ago
Oops, yes
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
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?
(Reporter)

Comment 20

7 years ago
(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.
Group: core-security
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/
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.