Closed Bug 422135 Opened 14 years ago Closed 14 years ago

not showing authentication dialog box when request is made throught XMLHttpRequest (nsIAuthPrompt implementation doesn't handle realms produced by NS_GetAuthKey)

Categories

(Toolkit :: Password Manager, defect, P2)

defect

Tracking

()

VERIFIED FIXED
mozilla1.9beta5

People

(Reporter: hidehisa, Assigned: Gavin)

References

()

Details

(Keywords: regression)

Attachments

(3 files, 2 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; ja; rv:1.9b4) Gecko/2008030714 Firefox/3.0b4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; ja; rv:1.9b4) Gecko/2008030714 Firefox/3.0b4

When you try to get content from password-restricted area (basic / digest), no authentication dialog box is showed, and authentication automatically fails (or succeeds if you have visited the same area in the same sesssion).

authentication dialog box is showed when trying to get content from restricted area using Ajax

Reproducible: Always

Steps to Reproduce:
try to get a password-protected (basic/digest) file through XmlHttpRequest

shown here -> http://nebosuker.net/ff3-ajax-auth/

Actual Results:  
request fails automatically if you have not succeeded in authentication in the session before

Expected Results:  
authentication dialog box (to enter username and password manually) is showed  if you have not succeeded in authentication in the session before
Version: unspecified → Trunk
Running the test case, I do see the behavior that the reporter describes using  Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b5pre) Gecko/2008031104 Minefield/3.0b5pre. Need to check to see if this is a regression from the previous release (Reporter - did this work in the last beta?)
Status: UNCONFIRMED → NEW
Ever confirmed: true
promptUsernameAndPassword is failing, because aPasswordRealm is "nebosuker.net:80 (ff3ajaxauth)", and _getFormattedHostname throws when it tries to get .host after turning it into a URI.

Not sure how that ends up happening. It looks like we're hitting http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/embedding/components/windowwatcher/public/nsPromptUtils.h&rev=1.3&mark=153-168#141 
(called from AuthPromptWrapper::PromptAuth), but I don't see how the channel could fail to QI to nsIHTTPChannel.
Component: General → Password Manager
Flags: blocking-firefox3?
QA Contact: general → password.manager
Keywords: regression
OS: Windows XP → All
Hardware: PC → All
(In reply to comment #2)
> Not sure how that ends up happening. It looks like we're hitting
> http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/embedding/components/windowwatcher/public/nsPromptUtils.h&rev=1.3&mark=153-168#141 
> (called from AuthPromptWrapper::PromptAuth), but I don't see how the channel
> could fail to QI to nsIHTTPChannel.

Oh, I misread - the early return is the !http case. In that case, I'm not sure how HTTP prompting is working at all...
> Reporter - did this work in the last beta?

I'm not sure if this is regression or not.

I tried b5pre, found this bug, tried b4, and bug was there.

Sorry I'm not good at English nor am not used to reporting bugs.
This is probably a regression from FF2, but I'm not really when it broke. Bug 413249 reported the same problem but I never got around to it. :(

I sorta suspect the problem may begin back when the XHR creates a channel... The channel's mCallbacks determine what ends up being invoked. I don't think it should be ending up at the point it is, but it's hard to tell because way the backend prompting bits work is still kind of confusing to me. :/

http://mxr.mozilla.org/seamonkey/source/content/base/src/nsXMLHttpRequest.cpp

I *think* it's using itself for the callbacks, so nsXMLHttpRequest::GetInterface() (~line 2450) is the interesting bit that gets invoked via nsHttpChannel::PromptForIdentity. Maybe this just needs adjusted to deal with nsIAuthPrompt2?

I can poke more at this later, but maybe biesi sees the problem offhand. :)
I think, after staring at this for a little while, that this is caused by nsXMLHttpRequest::GetInterface not supporting nsIAuthPrompt2, and only nsIAuthPrompt.

So when we end up in NS_QueryAuthPrompt2 [1] called from GetAuthPrompt() [2], it figures it has to wrap the nsIAuthPrompt, and we end up with an AuthPromptWrapper [3] which calls NS_GetAuthKey, which produces a realm that loginmanager's PromptUsernameAndPassword will choke on.

I'm not sure whether nsXMLHttpRequest::GetInterface should also forward nsIAuthPrompt2 to nsIWindowWatcher::GetAuthPrompt(), or whether login manager's nsIAuthPrompt implementation should deal with NS_GetAuthKey-generated realms, or both. Probably both?



[1] http://lxr.mozilla.org/seamonkey/source/netwerk/base/public/nsNetUtil.h#1171
[2] http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp&rev=1.328#2479
[3] http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/embedding/components/windowwatcher/src/nsPrompt.cpp&rev=1.34#536

Flags: blocking-firefox3? → blocking-firefox3+
Priority: -- → P2
well if loginmanager implements nsIAuthPrompt then it should also be prepared to deal with the keys nsIAuthPrompt used...

but yeah, XHR should also forward nsIAuthPrompt2.
IMO the correct fix is to rip out the nsIAuthPrompt from XHR and also from the XSLT code. This will make us use the one on the docshell which really is the one we want. Not sure if there are other places like this that we want to rip out too.
(In reply to comment #8)
> IMO the correct fix is to rip out the nsIAuthPrompt from XHR and also from the
> XSLT code. This will make us use the one on the docshell which really is the
> one we want. Not sure if there are other places like this that we want to rip
> out too.

Hmm, didn't see this comment before filing bug 422808. Can you comment there? I think we have this bug be about the broken nsIAuthPrompt implementation in the Firefox password manager.

Summary: not showing authentication dialog box when request is made throught XMLHttpRequest → not showing authentication dialog box when request is made throught XMLHttpRequest (nsIAuthPrompt implementation doesn't handle realms produced by NS_GetAuthKey)
I can confirm this behaviour with Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b5pre) Gecko/2008031304 Minefield/3.0b5pre
I also tried it with FF3b4 and bug was there. I see it as a regression from FF2.
After having looked at this for a bit, I don't think implementing password saving for http[s] via nsIAuthPrompt is worth it, because the realm we're passed doesn't include the scheme (NS_GetAuthKey strips it out), so we can't differentiate between http and https.

I think we should just recognize that we're being passed a realm that we don't support password saving for, and just prompt in that case (without the option to save). There should be no users of nsIAuthPrompt for http[s] authentication once bug 422808 lands, and if there are we should be able to fix them to use nsIAuthPrompt2 if it's really a problem.
Hrm, I have a patch that fixes this for promptUsernameAndPassword, and it works. I was going to extend it to also fix promptPassword, since it too can be called via AuthPromptWrapper::PromptAuth, but it seems to deal with the realm in a completely different way (creating a new realm out of the hostname+pathname, and parsing out the username). Shouldn't these two methods be consistent, at least with regard to which realm they end up using for a given aPasswordRealm?
This consolidates realm parsing for the nsIAuthPrompt methods, and just avoids trying to save passwords if we detect a realm produced by the HTTP case.

This changes the realm we use for these methods, so it breaks filling in any existing stored logins with those realms. Since we've not shipped this code, and since as far as I know no one uses it yet (SeaMonkey hasn't switched over), I don't think that is a problem.
Assignee: nobody → gavin.sharp
Status: NEW → ASSIGNED
Attached patch same as -w (obsolete) — Splinter Review
Attachment #309749 - Flags: review?(dolske)
Attachment #309749 - Flags: review?(bugzilla)
Whiteboard: [has patch][needs review dolske/standard8]
Target Milestone: --- → Firefox 3 beta5
Comment on attachment 309749 [details] [diff] [review]
same as -w

>Index: toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js
>===================================================================

>-        var hostname = this._getFormattedHostname(aPasswordRealm);
>-
>-        this.log("===== promptUsernameAndPassword() called =====");
>+        var [hostname, realm, ] = this._getRealmInfo(aPasswordRealm);

Maybe I'm just not used to the syntax, but add an "username" (or "unused") arg there? I have a feeling I'll look at this in the future and think "uh oh, we forgot something there".

>@@ -252,45 +254,44 @@ LoginManagerPrompter.prototype = {

>-        if (!ok || !checkBox.value)
>+        if (!ok || !checkBox.value || !hostname)
>             return ok;

The !hostname check isn't needed, because the checkbox value should always be false (we don't unhide the checkbox in this case, so the use can't change it).


>@@ -302,72 +303,104 @@ LoginManagerPrompter.prototype = {

>-        if (ok && checkBox.value) {
>+        if (ok && checkBox.value && hostname) {

As above, checkbox.value should be false when there's no hostname.


>+    /* ---------- nsIAuthPrompt helpers ---------- */
>+
>+    /**
>+     * Given aRealmString, a realm produced by NS_GetAuthKey, returns:
>+     *    - a formatted hostname (for use by loginmgr)
>+     *    - a realm relevant to the loginmgr (hostname+path).
>+     *    - a username
>+     *
>+     * Returns null values for each of these if aRealmString is an HTTP[S]
>+     * realm produced by NS_GetAuthKey, e.g.:
>+     *
>+     * example.com:80 (httprealm)
>+     *
>+     * since we don't have a scheme for those, which we need to properly store
>+     * the login info. Returning null lets the callers know that they can't
>+     * save the login for this realm.
>+     */

* Given aRealmString, such as "http://user@example.com/foo", returns an array of:
*   - the formatted hostname
*   - the realm (hostname + path)
*   - the username, if present
*
* If aRealmString is in the format produced by NS_GetAuthKey for HTTP[S] channels,
* e.g. "example.com:80 (httprealm)", null is returned for all arguments to let callers know
* the login can't be saved because we don't know the exact scheme.
Attachment #309749 - Flags: review?(dolske) → review+
(In reply to comment #15)
> >+        var [hostname, realm, ] = this._getRealmInfo(aPasswordRealm);
> 
> Maybe I'm just not used to the syntax, but add an "username" (or "unused") arg
> there? I have a feeling I'll look at this in the future and think "uh oh, we
> forgot something there".

Sure.

> The !hostname check isn't needed, because the checkbox value should always be
> false (we don't unhide the checkbox in this case, so the use can't change it).

Yeah, I didn't have it there initially, but I figured I'd add in an explicit check for hostname because relying on the nsIPrompt checkbox behavior is non-obvious...

> (better comment)

Thanks, I'll use that :)
Whiteboard: [has patch][needs review dolske/standard8] → [has patch][needs review standard8]
Flags: in-testsuite?
Comment on attachment 309749 [details] [diff] [review]
same as -w

+    _getRealmInfo : function (aRealmString) {
...
+        return [formattedHostname, formattedHostname + pathname, uri.username];
+    }

You missed the comma after the }.

r=me with that and Justin's comments fixed.
Attachment #309749 - Flags: review?(bugzilla) → review+
Attached patch updated patchSplinter Review
(In reply to comment #18)
> You missed the comma after the }.

Yeah, somehow that mistake snuck in after I had tested but before I |diff|ed, or something. Tests pass now, anyhow :)
Attachment #309748 - Attachment is obsolete: true
Attachment #309749 - Attachment is obsolete: true
Keywords: checkin-needed
Whiteboard: [has patch][needs review standard8] → [has patch]
mozilla/toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js 	1.32
mozilla/toolkit/components/passwordmgr/test/test_prompt.html 	1.7
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [has patch]
Flags: in-testsuite? → in-testsuite+
Martin: not sure what that's suppose to show. Please file a new bug and attach it there?
IMHO it's the same bug like http://nebosuker.net/ff3-ajax-auth/, but it's in chrome(extension).
I can confirm this. When you run the extension like this "firefox -chrome chrome://extension/content" or open window with "chrome" parameter, auth dialog is not shown and 401 returned. 
Adam or Martin: please file a new bug on that issue, and CC me. Commenting here means the issue is likely to be forgotten.
Before this regression this was handled more elegantly:
If several requests were made to the same password-restricted area the user was only prompted once for his credentials.
I have a XUL app that at upon opening requests as many as five RDF-XML documents. I have to enter my username and password for each one of them now.
That's a problem unfortunately common to all our prompting code, and isn't specific to the XHR fix here. I'll be fixing that after FF3.
I've verified that this is fixed with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008050804 Minefield/3.0pre using the test cases.
Status: RESOLVED → VERIFIED
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.