Closed Bug 732169 Opened 10 years ago Closed 7 years ago

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

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(firefox13 affected, firefox34 verified, firefox-esr31 wontfix, blocking-fennec1.0 soft)

VERIFIED FIXED
Firefox 34
Tracking Status
firefox13 --- affected
firefox34 --- verified
firefox-esr31 --- wontfix
blocking-fennec1.0 --- soft

People

(Reporter: lmandel, Assigned: wesj)

Details

(Keywords: sec-low, Whiteboard: [mtd][sg:low][adv-main34-])

Attachments

(2 files, 2 obsolete files)

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.
OS: Mac OS X → Android
Hardware: x86 → ARM
Version: unspecified → Trunk
Thins is kinda ugly, noming.
blocking-fennec1.0: --- → ?
blocking-fennec1.0: ? → +
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.
Wes - Is there anywhere we can associate the tab to a prompt? I guess desktop does this in PromptService. We could do something similar.
Attached patch Patch (obsolete) — Splinter Review
As per discussions in IRC, this patch will load the page in background, if the prompt dialog is being shown for any website.
Attachment #610706 - Flags: review?(mark.finkle)
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)
Attachment #610706 - Flags: review?(mark.finkle) → review-
Assignee: nobody → sriram
blocking-fennec1.0: + → soft
Curtis, can we figure out if this is exploitable or not?
Group: core-security
It's not directly exploitable, but could make for some good spoofing in a multi-tab situation.
Keywords: sec-low
Whiteboard: [mtd] → [mtd][sg:low]
Throwing this at Wes based on Comment 3; beats having it on Sriram's plate :D
Assignee: sriram.mozilla → wjohnston
Hardware: ARM → All
Attached patch Patch 1/2 - refactor (obsolete) — Splinter Review
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.
Attachment #610706 - Attachment is obsolete: true
Attachment #8472594 - Flags: review?(lucasr.at.mozilla)
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.
Attachment #8472597 - Flags: review?(lucasr.at.mozilla)
Old patch had some code from patch 2 in it.
Attachment #8472594 - Attachment is obsolete: true
Attachment #8472594 - Flags: review?(lucasr.at.mozilla)
Attachment #8472598 - Flags: review?(lucasr.at.mozilla)
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?
Attachment #8472597 - Flags: review?(lucasr.at.mozilla) → review+
Comment on attachment 8472598 [details] [diff] [review]
Patch 1/2 - refactor

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

Nice.
Attachment #8472598 - Flags: review?(lucasr.at.mozilla) → review+
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
Whiteboard: [mtd][sg:low] → [mtd][sg:low][adv-main34-]
Group: core-security → core-security-release
Group: core-security-release
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.