396295 - non-HTTP password management is broken

Status: VERIFIED FIXED

(Toolkit :: Password Manager, defect, P3)

Description

[Nickolay_Ponomarev]

Attachment: patch

QueryInterface throws, not returns null.

The patch also cleans up some code.

Comment 1

[Nickolay_Ponomarev]

Comment on attachment 281036 [details] [diff] [review]
patch

There's more brokenness...

Comment 2

[Nickolay_Ponomarev]

Attachment: patch

For non-HTTP auth (e.g. FTP) there's no "realm", as far as I can see. Current code assumes there either is a actionURL or a "realm". The updated patch fixes this assumption, while also fixing the typos in the prompter.

I checked that old signons for HTTP/FTP/form authorization are loaded properly and that FTP logins can now be saved.

I'm not familiar with the login manager, so I may be breaking something else.

Comment 3

[Justin Dolske [:Dolske]]

Comment on attachment 281043 [details] [diff] [review]
patch

>Index: toolkit/components/passwordmgr/src/nsLoginManager.js
>===================================================================
>-        if (!login.httpRealm && !login.formSubmitURL)
>-            throw "Can't add a login without a httpRealm or formSubmitURL.";
>-

Keep this (see next comment).

>+        if (!(aChannel instanceof Ci.nsIHttpChannel)) {
>             key = aChannel.URI.prePath;
>-            this.log("_GetAuthKey: got http channel, key is: " + key);
>-            return key;
>+            this.log("_GetAuthKey: got non-http channel, key is: " + key);
>+            return [key, null];

I think this should be |return [key, key];|, so that the "httpRealm" for non-HTTP connections is the same as the hostname. [This should also be what happens when an HTTP 401 response doesn't include an explicit realm, although now I want to test that. :-(]

>                 case STATE.ACTIONURL:
>-                    var formSubmitURL = line.value;
>-                    if (!formSubmitURL && entry.httpRealm)
>-                        entry.formSubmitURL = null;
>-                    else
>-                        entry.formSubmitURL = formSubmitURL;
>+                    entry.formSubmitURL = line.value ? line.value : null; 

Keep this (see previous comment).

The rest looks good, great catch!

Comment 4

[Nickolay_Ponomarev]

Sorry, didn't get to updating the patch quickly -- had to go. Should this be marked a dupe of bug 396316, now that you're fixing these typos in a patch there?

An alternative would be to check in my (temporary) fix here if the other bug might take longer to get fixed. In that case, I think returning [key, null] is better, since that's compatible with the old signons2.txt format. The security issue can then be fixed in bug 396316 (along with the necessary signons.txt migration code).

Comment 5

[Justin Dolske [:Dolske]]

It's probably easiest to just fix the typos and other issues as part of 396316, and then marked this bug as FIXED. I don't think this patch is correct as a temporary fix, and it's probably best to just avoid getting entangled in issues that 396316 will fix anyway.

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

(Blocking to ensure that this gets resolved, even if it does so as described in comment 5)

Comment 8

[Justin Dolske [:Dolske]]

Bug 396316 landed yesterday, and this now seems to be fixed.

Comment 9

[Nickolay_Ponomarev]

Yeah, this is fixed now. Too bad I can't reassign to you without reopening and thus causing a little bugspamfest...