1272507 - HTTP passwords should be used on the HTTPS version of HTTP auth on the same domain

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect)

Description

[Matthew N. [:MattN]]

Bug 667233 will use HTTP logins for autocomplete/fill/contextmenu on the HTTPS version of the same domain. This bug will handle filling of HTTP logins in HTTP auth dialogs that have upgraded to HTTPS.

This should require changes in nsLoginManagerPrompter.js (promptPassword, promptAuth, and promptUsernameAndPassword) and tests should be added.

Comment 1

[Matthew N. [:MattN]]

Attachment: Bug 1272507 - Upgrade HTTP auth passwords to HTTPS on the same domain.

It seems like in general it may not be a good idea to upgrade the httpRealm but in the case of promptPassword and promptUsernameAndPassword maybe it's fine. Otherwise we could leave those two methods alone and only upgrade for promptAuth which gets used for HTTP Basic/Digest Auth.

Review commit: https://reviewboard.mozilla.org/r/74050/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/74050/

Comment 2

[Justin Dolske [:Dolske]]

Comment on attachment 8784714 [details]
Bug 1272507 - Upgrade HTTP auth passwords to HTTPS on the same domain.

https://reviewboard.mozilla.org/r/74050/#review72170

::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js:403
(Diff revision 1)
>     *
>     * If a password is not found in the database, the user will be prompted
>     * with a dialog with a text field and ok/cancel buttons. If the user
>     * allows it, then the password will be saved in the database.
>     */
>    promptPassword : function (aDialogTitle, aText, aPasswordRealm,

promptUsernameAndPassword() is the only function that gets called for HTTP auth. These other functions are legacy stuff really only used by mailnews. So these changes should only go there.

::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js:431
(Diff revision 1)
>  
>        if (!aPassword.value) {
>          // Look for existing logins.
> -        var foundLogins = this._pwmgr.findLogins({}, hostname, null,
> -                                          realm);
> +        var foundLogins = LoginHelper.searchLoginsWithObject({
> +          hostname,
> +          schemeUpgrades: LoginHelper.schemeUpgrades,

Seems like realm needs to be specified here, or you're going to get form logins back too, no?

Ah, I see, I think you're trying to deal with the empty-realm fixup we do in _getAuthTarget().

Maybe we should just ignore that. Usually the realm is just an arbitrary text string returned by the server. Looks like I added this fallback back in bug 396316 because such logins on non-HTTP[S] protocols don't have any such notion of "realm", so we had to provide _something_ to differentiate these from form-based logins.

I think that's a moot point now, since we don't store logins in signons.txt, and the protocol upgrade only apples to HTTP. So I think there's no need to ever attempt this, realms for HTTP logins shouldn't really ever get this fallback. (And if it can I'd expect it to be rare, and we can ignore it.)

::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js:434
(Diff revision 1)
>          // XXX Like the original code, we can't deal with multiple
>          // account selection (bug 227632). We can deal with finding the
>          // account based on the supplied username - but in this case we'll
>          // just return the first match.

I didn't really think about it much, but promptUsernameAndPassword has basically the same issue -- our prompt can't offer multiple logins, and so we basically just give up if we find more than 1 possible login to use.

Seems like there might be some risk of scheme-upgrades triggering this? Although with the de-duping maybe this is fairly unlikely.

Comment 3

[Matthew N. [:MattN]]

(In reply to Justin Dolske [:Dolske] from comment #2)
> ::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js:403
> (Diff revision 1)
> >     *
> >     * If a password is not found in the database, the user will be prompted
> >     * with a dialog with a text field and ok/cancel buttons. If the user
> >     * allows it, then the password will be saved in the database.
> >     */
> >    promptPassword : function (aDialogTitle, aText, aPasswordRealm,
> 
> promptUsernameAndPassword() is the only function that gets called for HTTP
> auth. These other functions are legacy stuff really only used by mailnews.
> So these changes should only go there.

You mean promptAuth, right? That's what I see getting used when I visit an HTTP auth site.

Comment 4

[Justin Dolske [:Dolske]]

Yeah, sorry, the call chain here is so confusing.

start
  nsLoginManagerPrompter's promptAuth() [nsIAuthPrompt2]
to
  nsPrompter.js's promptAuth() [nsIPromptService2]
to
  nsPrompter.js's promptAuth() [nsIAuthPrompt2]
to
  nsPrompter.js's nsIAuthPrompt_promptUsernameAndPassword()
to
  nsPrompter.js's nsIPrompt_promptUsernameAndPassword()

Comment 5

[Matthew N. [:MattN]]

Comment on attachment 8784714 [details]
Bug 1272507 - Upgrade HTTP auth passwords to HTTPS on the same domain.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/74050/diff/1-2/

Comment 6

[Matthew N. [:MattN]]

We can't use LoginHelper.searchLoginsWithObject({ schmeUpgrades: true, …}) in place of countLogins since it would trigger a MP prompt.

This approach adds some inconsistency but until we have a login picker in the auth dialog (bug 227632), I don't think we will want much looser searches anyways.

The other option I considered was adding a `decrypt` option to `searchLoginsWithObject` which gets passed to `searchLogins` and would allow storage to return encrypted logins (just so they could be counted) but I think we both didn't want to leak encrypted logins outside of storage before.

I still need to write a test (which would ideally depend on bug 1298543).

Comment 7

[Justin Dolske [:Dolske]]

Comment on attachment 8784714 [details]
Bug 1272507 - Upgrade HTTP auth passwords to HTTPS on the same domain.

https://reviewboard.mozilla.org/r/74050/#review77172

Comment 8

[Matthew N. [:MattN]]

Attachment: Bug 1272507 - Test upgrading HTTP auth passwords to HTTPS on the same domain.

Review commit: https://reviewboard.mozilla.org/r/78610/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/78610/

Comment 9

[Matthew N. [:MattN]]

Comment on attachment 8784714 [details]
Bug 1272507 - Upgrade HTTP auth passwords to HTTPS on the same domain.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/74050/diff/2-3/

Comment 10

[Justin Dolske [:Dolske]]

Comment on attachment 8791061 [details]
Bug 1272507 - Test upgrading HTTP auth passwords to HTTPS on the same domain.

https://reviewboard.mozilla.org/r/78610/#review77470

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5555d5ae4ca2
Upgrade HTTP auth passwords to HTTPS on the same domain. r=Dolske
https://hg.mozilla.org/integration/autoland/rev/9879b00007bd
Test upgrading HTTP auth passwords to HTTPS on the same domain. r=Dolske

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5555d5ae4ca2
https://hg.mozilla.org/mozilla-central/rev/9879b00007bd