[e10s] If a background tab requests HTTP auth the tab isn't switched to before displaying the auth dialog

RESOLVED FIXED in mozilla36

Status

()

Toolkit
Password Manager
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mossop, Assigned: mossop)

Tracking

unspecified
mozilla36
Points:
3
Bug Flags:
firefox-backlog +
in-testsuite +
qe-verify -

Firefox Tracking Flags

(e10sm4+)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

3 years ago
This is a means to spoof the user into giving the background tab their credentials for whatever the foreground tab is.

I think the bug lies here:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/nsLoginManagerPrompter.js#581

In e10s we don't have the content window, this._window is just the main browser window.
Flags: firefox-backlog+
I noticed this too, thanks for filing it.

I had assumed it was caused by issues with DOMWillOpenModalDialog not firing or not properly being propagated to the parent process, but looking into it just a bit further, http://hg.mozilla.org/mozilla-central/annotate/80e18ff7c7b2/toolkit/components/passwordmgr/LoginManagerParent.jsm#l210 looks pretty fishy.
I suppose the setE10sData call is already forwarding the browser over to the nsILoginManagerPrompter, but there's no nice way to forward that even further along to the underlying prompt service here:

http://hg.mozilla.org/mozilla-central/annotate/80e18ff7c7b2/toolkit/components/passwordmgr/nsLoginManagerPrompter.js#l367

Prompting APIs are all window based :(
(Assignee)

Updated

3 years ago
Assignee: nobody → dtownsend+bugmail
Status: NEW → ASSIGNED
Points: --- → 3
Flags: qe-verify-
(Assignee)

Updated

3 years ago
Iteration: --- → 36.2
tracking-e10s: ? → m4+
(Assignee)

Comment 3

3 years ago
Created attachment 8515099 [details] [diff] [review]
patch

This makes TabParent forward along the browser to the auth prompter. I'm not a fan of having to use the tabbrowser APIs to switch to the tab but can't think of an alternative right now.

I'm not positive that "*aResult = prompt.forget().take()" is the right way to return the prompter from C++ so let me know if there is a better way to do that.
Attachment #8515099 - Flags: review?(mrbkap)
(In reply to Dave Townsend [:mossop] from comment #3)
> I'm not a fan of having to use the tabbrowser APIs to switch to the tab but can't
> think of an alternative right now.

Adjust the promptservice API to allow passing in the opener somehow, and have it fire DOMWillOpenModalDialog properly? We'll need to do this eventually anyhow to fix a bunch of other prompts.
(Assignee)

Comment 5

3 years ago
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #4)
> (In reply to Dave Townsend [:mossop] from comment #3)
> > I'm not a fan of having to use the tabbrowser APIs to switch to the tab but can't
> > think of an alternative right now.
> 
> Adjust the promptservice API to allow passing in the opener somehow, and
> have it fire DOMWillOpenModalDialog properly? We'll need to do this
> eventually anyhow to fix a bunch of other prompts.

Aha, I wasn't aware we had that. Looks like it takes a browser already so I can just use that.
(Assignee)

Comment 6

3 years ago
Created attachment 8516067 [details] [diff] [review]
patch rev 2
Attachment #8515099 - Attachment is obsolete: true
Attachment #8515099 - Flags: review?(mrbkap)
Attachment #8516067 - Flags: review?(mrbkap)
(Assignee)

Updated

3 years ago
Attachment #8516067 - Attachment is obsolete: true
Attachment #8516067 - Flags: review?(mrbkap)
(Assignee)

Comment 7

3 years ago
Created attachment 8516172 [details] [diff] [review]
patch rev 3
Attachment #8516172 - Flags: review?(mrbkap)
Comment on attachment 8516172 [details] [diff] [review]
patch rev 3

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

::: dom/ipc/TabParent.cpp
@@ +71,5 @@
>  #include "nsAuthInformationHolder.h"
>  #include "nsICancelable.h"
>  #include "gfxPrefs.h"
>  #include <algorithm>
> +#include "nsILoginManagerPrompter.h"

Nit: I believe the style guide asks us to keep #include ""s blocked separately from #include <>s.

@@ +1890,5 @@
> +    nsCOMPtr<nsIDOMElement> browser = do_QueryInterface(mFrameElement);
> +    prompter->SetE10sData(browser, nullptr);
> +  }
> +
> +  *aResult = prompt.forget().take();

prompt.forget(aResult);
Attachment #8516172 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 9

3 years ago
(In reply to Blake Kaplan (:mrbkap) from comment #8)
> @@ +1890,5 @@
> > +    nsCOMPtr<nsIDOMElement> browser = do_QueryInterface(mFrameElement);
> > +    prompter->SetE10sData(browser, nullptr);
> > +  }
> > +
> > +  *aResult = prompt.forget().take();
> 
> prompt.forget(aResult);

I tried that before and it fails to compile with "error: assigning to 'void *' from incompatible type 'already_AddRefed<nsISupports>'"
Flags: needinfo?(mrbkap)
Gah, sorry. Your original attempt was the right way to go.
Flags: needinfo?(mrbkap)
(Assignee)

Comment 11

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/56019d2d1da2
https://hg.mozilla.org/mozilla-central/rev/56019d2d1da2
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.