Closed Bug 1091287 Opened 8 years ago Closed 8 years ago

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

Categories

(Toolkit :: Password Manager, defect)

defect
Not set
normal
Points:
3

Tracking

()

RESOLVED FIXED
mozilla36
Iteration:
36.2
Tracking Status
e10s m4+ ---

People

(Reporter: mossop, Assigned: mossop)

References

Details

Attachments

(1 file, 2 obsolete files)

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: nobody → dtownsend+bugmail
Status: NEW → ASSIGNED
Points: --- → 3
Flags: qe-verify-
Iteration: --- → 36.2
Attached patch patch (obsolete) — Splinter Review
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.
(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.
Attached patch patch rev 2 (obsolete) — Splinter Review
Attachment #8515099 - Attachment is obsolete: true
Attachment #8515099 - Flags: review?(mrbkap)
Attachment #8516067 - Flags: review?(mrbkap)
Attachment #8516067 - Attachment is obsolete: true
Attachment #8516067 - Flags: review?(mrbkap)
Attached patch patch rev 3Splinter Review
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+
(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)
https://hg.mozilla.org/mozilla-central/rev/56019d2d1da2
Status: ASSIGNED → RESOLVED
Closed: 8 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.