Created attachment 356176 [details] [diff] [review]
I've just been debugging some mailnews issues with promptPassword.
What I have realised is that some user names can be of the format:
When mailnews forms the url, it encodes this so we get urls such as:
When promptPassword comes along and extracts the user name from the url, it is using the escaped version rather than the unescaped version to compare to the stored logins, and hence isn't matching.
We should be storing the unescaped form (as we can and its prettier to display), indeed, the migration code already stores the unescaped version, we should do the same in promptPassword.
The fix is simple, attaching patch and test case - I'm still just rebuilding Firefox and testing there, so holding off on requesting review.
Created attachment 356181 [details] [diff] [review]
The fix v2
Minor change to patch to add login2C to the list of variables. All FF password manager tests still pass.
Assuming this is the reason my migrated username for gmail smtp didn't work until I let the prompt save a new "philringnalda%40gmail%2Ecom" one, tb3needs rather badly.
Does promptUsernameAndPassword() also need to support this?
Do all the mailnews callers explicitly urlencode the username part of the realm string when constructing it? I'm a little wary of this unencoding something a caller didn't expect. EG: aPasswordRealm == "http://email@example.com"
Seems like the better, or else there may be funky attacks if someone can make the caller think the username is "firstname.lastname@example.org/" (slash is important), such that the URL parsing gets confused.
Should probably also add a comment to the IDL saying that callers *must* urlencode the username to avoid these problems.
(In reply to comment #3)
> Does promptUsernameAndPassword() also need to support this?
Not as far as I can tell because that's not extracting usernames from URIs.
> Do all the mailnews callers explicitly urlencode the username part of the realm
> string when constructing it?
I need to check this still.
> I'm a little wary of this unencoding something a
> caller didn't expect. EG: aPasswordRealm == "http://email@example.com"
> Seems like the better, or else there may be funky attacks if someone can make
> the caller think the username is "firstname.lastname@example.org/" (slash is important),
> such that the URL parsing gets confused.
I also need to check if the url parsing is going to get confused with smtp://user@host@realhost/
Depending on the answers to the questions above my current thoughts are:
a) do this patch with additional comments in the idl.
b) don't apply this bug, and back out bug 461341 and have escaped names shown in password manager
c) keep bug 461341 and don't escape names in mailnews uris (which may be difficult as all the non-smtp ones are essentially rdf uris which prefs rely on).
I'll do some digging tomorrow to confirm what mailnews is doing in the various areas.
Created attachment 356589 [details] [diff] [review]
The fix v3
Updated patch to include comment as discussed on irc.
Requesting blocking as TB 3 needs this. We either need to land straight after 1.9.1b3 or before - in any case I'd like to get it into 1.9.1 by approx 28th to allow time to bake for TB 3 beta 2.
This bug is needed for mailnews to switch to toolkit's password manager. Thunderbird/SeaMonkey will be using promptPassword, afaik Firefox doesn't so this is little effect on Firefox. If we don't get this bug in in time then there's at least two other bugs we need to find alternative fixes for, and it will increase work in extensions which we are trying to get to support Thunderbird as well as Firefox (e.g. weave).
Comment on attachment 356589 [details] [diff] [review]
The fix v3
r+, but also add a test to ensure usenames like "100%beef" are correctly handled (ie, test for too much unescaping).
Also flagging mconnor for sr, just because nsIAuthPrompt has been around forever, and this makes an interesting test of the proposed "sr all API changes" policy. :)
Created attachment 357115 [details] [diff] [review]
Patch for checkin
100%beef in a uri comes up as malformed, so I included two additional tests, one with the uri username of 100%25beef, and the other with 100@beef. This is the patch I'll be pushing.
Checked into trunk: http://hg.mozilla.org/mozilla-central/rev/5542884b9bdc
Checked into 1.9.1: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/a5770527971e
>+ login2E.init("http://example2.com", null, "http://example2.com",
>+ "100%beef", "user3pass", "", "");
>+isOk = prompter1.promptPassword(dialogTitle(), dialogText, "http://email@example.com",
>+ Ci.nsIAuthPrompt.SAVE_PASSWORD_NEVER, pword);
Right, that's what I was expecting. Caller has the username encoded, and login manger is decoding it *once*. If it decoded it twice, it would get a 0xBE character and not match the login.