732169 - Authentication dialog remains open for site that doesn't require authentication

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Lawrence Mandel [:lmandel] (use needinfo)]

The authentication dialog can remain open when clicking a link to a site that doesn't require authentication if it was not dismissed before closing the browser.

Steps to reproduce:
1. Open Firefox and go to a URL that requires authentication, like http://intranet.mozilla.com.
2. Close the browser without dismissing the dialog but clicking the home button on your phone.
3. Click on a link that doesn't require authentication, like http://mozilla.org, in another application and open it in Firefox. The link loads but the authentication dialog is shown even though the site doesn't require authentication.

Actual results:
mozilla.org opens but is blocked by an authentication dialog.

Expected results:
mozilla.org should open and I should be able to read/interact with the page. The authentication dialog should be displayed when I tab back to the page that requires authentication.

Comment 1

[Kevin Brosnan [Ex-Mozilla]]

Thins is kinda ugly, noming.

Comment 2

[Sriram Ramasubramanian [:sriram]]

This holds good for any prompt shown -- say an dialog for <select>. This happens because, the PromptService is a "pseudo blocking call" (new name? ;)) which shows a dialog until it gets a result. This is basically not tied to "any tab". Gecko Events are still processed by polling (pseudo -- gets its name here).

The change would be to "tie a prompt dialog" to a tab (may be a list of prompts in the order of addition) and not make it blocking. The dialog will return the result, once the user selects an option.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Wes - Is there anywhere we can associate the tab to a prompt? I guess desktop does this in PromptService. We could do something similar.

Comment 4

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

As per discussions in IRC, this patch will load the page in background, if the prompt dialog is being shown for any website.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

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

loadUrlInBackgroundIfNeeded and loadUrlInBackgroundTab kinda duplicate loadUrlInTab and it's adds some complexity to the intents. Also, we really want the intents to go through GeckoEvent.createLoadEvent and not GeckoEvent.createBroadcastEvent because the BrowserCLH handles routing the URL to nsBrowserAccess (nsIBrowserDOMWindow) in browser,js so it can do cool things for webapps and bookmarks.

Therefore, I'd be more interested in finding a way in browser.js to ask "is there a prompt showing?" and handle the background load there.

We could also pass "-background" as a commandline param to BrowserCLH via GeckoEvent.createLoadEvent but we'd need to add support to background tabs in nsBrowserAccess (nsIBrowserDOMWindow)

Comment 6

[Brad Lassey [:blassey] (use needinfo?)]

Curtis, can we figure out if this is exploitable or not?

Comment 7

[Daniel Veditz [:dveditz]]

It's not directly exploitable, but could make for some good spoofing in a multi-tab situation.

Comment 8

[Richard Newman [:rnewman]]

Throwing this at Wes based on Comment 3; beats having it on Sriram's plate :D

Comment 9

[Wesley Johnston (:wesj)]

Attachment: Patch 1/2 - refactor

I needed to know if the tab associated with this dialog was selected before showing it, so I had to do a little refactoring. For instance processMessage actually called show() at its end before. Since show() created and showed the dialog, there was no opportunity between when the dialog was created and shown to stop.

This removes processMessage entirely because we don't really need it (i.e. show(json) should do that anyway). It replaces show() with a create() and show() methods to separate the process in two.

Comment 10

[Wesley Johnston (:wesj)]

Attachment: Patch 2/2 - Make prompts tab aware

This does the work to actually make prompts tab-aware. Note, we're still not quite tab-modal, i.e. you can't switch tabs while a prompt is showing. But if you exit Fennec and come back in to a new tab, we'll hold onto the dialog until you switch back to the tab it was on.

Comment 11

[Wesley Johnston (:wesj)]

Attachment: Patch 1/2 - refactor

Old patch had some code from patch 2 in it.

Comment 12

[Lucas Rocha (:lucasr)]

Comment on attachment 8472597 [details] [diff] [review]
Patch 2/2 - Make prompts tab aware

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

Looks good with the suggested change.

::: mobile/android/base/prompts/Prompt.java
@@ +56,5 @@
>  
>      private static boolean mInitialized;
>      private static int mInputPaddingSize;
>  
> +    private Tab mTab;

We should probably avoid keeping local references to Tab instances as they are owned by the Tabs class. We might end up with dangling Tab instances. It seems you only really need to keep a tab ID here. So I suggest changing mTab to be an int and set it to -1 when the tabId is not defined in the message.

@@ +109,5 @@
>              choiceMode = ListView.CHOICE_MODE_MULTIPLE;
>          }
>  
> +        if (message.has("tabId")) {
> +            final int id = message.optInt("tabId");

It's probably a good idea to be more explicit here and set a fallback value i.e. message.optInt("tabId", -1);

::: mobile/android/modules/Prompt.jsm
@@ -36,5 @@
>  
>    if ("hint" in aOptions && aOptions.hint != null)
>      this.msg.hint = aOptions.hint;
> -
> -  let idService = Cc["@mozilla.org/uuid-generator;1"].getService(Ci.nsIUUIDGenerator);

Unrelated?

Comment 13

[Lucas Rocha (:lucasr)]

Comment on attachment 8472598 [details] [diff] [review]
Patch 1/2 - refactor

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

Nice.

Comment 14

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/fx-team/rev/ea86f3073411
https://hg.mozilla.org/integration/fx-team/rev/e08243c7ea06

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ea86f3073411
https://hg.mozilla.org/mozilla-central/rev/e08243c7ea06