Problems updating passwords

[jfitzell]

As reported in #83 this comment, there seem to be some problems updating passwords:

Looking up and adding passwords seem to work great but I was running into problems with updating passwords for existing logins - do the following work for you?

1. Signing in to a form with an existing account but new password: doorhanger asks if I want to update but incorrectly shows the old password value; if I retype the new value though, pressing Update correctly updates the password
2. Signing in to basic auth with an existing account but new password: doorhanger asks if I want to update and shows the new password value but incorrectly shows Remember as the button label; clicking the button fails with Error: can't add a login without a httpRealm or formSubmitURL. The logs show a call to searchLogins(nsIPropertyBag{httpRealm: null, hostname: 'http://localhost', formSubmitURL: null, schemeUpgrades: true}) but that makes little sense since, as the error says, those values can't both be null.

Not sure whether those are bugs in the new FF49+ implementation of the password manager or if they're somehow caused by the implementation of searchLogins(), but would be good to be sure before doing a release.

Re: your comment about wildcard behaviour, I have the same interpretation as you: properties that are not provided must be wildcards. I suspect, though, that an empty string should match only an empty string, rather than being a wildcard as it is when passed to findLogin(). I see an empty string being passed in for formSubmitURL in some of my test cases, so I wonder if that's related somehow?

I added some more detailed debugging of the nsIPropertyBags that are passed into searchLogins() and modifyLogin() but wasn't able to spot the problem immediately. I'd obviously appreciate your help if you have time.

Would also be good to log any unsupported properties passed into searchLogins() (similar to what's done in modifyLogin() as it makes it easier to see what's going on when people report bugs and provide the logs.

[jfitzell]

Comment from bkimmett:

I found the problem.

Basically, the doorhanger generation function, _showChangeLoginNotification() in nsLoginManagerPrompter.js:1178, has this code in it:

if (aNotifyObj == this._getPopupNote()) {

  aOldLogin.hostname = aNewLogin.hostname;

  aOldLogin.formSubmitURL = aNewLogin.formSubmitURL;

  aOldLogin.password = aNewLogin.password;

  aOldLogin.username = aNewLogin.username;

  this._showLoginCaptureDoorhanger(aOldLogin, "password-change");
}`

Anyway, you set up a custom getter for your returned keychain login object, but I don't think there's a custom setter!! So the custom getter overrides it and aOldLogin's original password is pulled.

I think this is what is happening, anyway.

The correct (or at least, nice) solution is to check if it's safe to junk those first four lines of code inside the if block and just call:

this._showLoginCaptureDoorhanger(aNewLogin, "password-change");

...and then just submit a patch (for Firefox) to Mozilla if this is the case. If we're going this way, it's worth noting that the doorhanger doesn't take the username and password values passed into it for storage— it takes the values from the displayed fields after the user's edited those fields (if they do), as is both observed and expected behavior.

The hacky solution is to modify your returned login object so it doesn't use a Proxy for its password and instead just stores it the normal way. I, uh, don't know how secure this is, though.

[bkimmett]

Slight edit for anyone reading this about my last comments about the object not using a Proxy: It should use a Proxy, otherwise, every password that might match is brought up from the Keychain by the extension during searching and the user gets ten thousand password use dialog boxes. The solution I was thinking of implementing was: add an extra field to the login object named "passwordChanged". Normally, this is false. If the setter is used, this is set to true and the value of password is set to whatever was given to the setter. (And then the getter returns that value instead of querying the proxy.)

[yustepanov]

Hello! Please have a look at attachment. It may have a huge amount of information. If you need some specific details, please let me know. Firefox.log