609380 - Password sync: don't add apply records with both a httpRealm and formSubmitURL or null/empty passwords

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[juan becerra [:juanb]]

Using latest nightly on Ubuntu 10.10 (64bit). After sync'ing I saw this in the error console:

Error: [Exception... "'Can't add a login with both a httpRealm and formSubmitURL.' when calling method: [nsILoginManager::addLogin]"  nsresult: "0x8057001e (NS_ERROR_XPC_JS_THREW_STRING)"  location: "JS frame :: resource://services-sync/engines/passwords.js :: PasswordStore__create :: line 192"  data: no]
Source File: resource://services-sync/engines/passwords.js
Line: 192

Comment 2

[:Gavin Sharp [email: [e-mail]]]

I've also hit this. Two of the entries in my trunk signons.sqlite have a non-empty formSubmitURL (e.g. https://mail.mozilla.com) and a non-null httpRealm (equal to ""). Sync is failing with this exception in my 3.6.13/1.6 profile, presumably because it's trying to sync those two entries.

I don't know how my trunk profile got into this state (some glitch from prior to bug 407567's landing?), but it seems like Sync probably needs to handle this case gracefully.

Comment 3

[Justin Dolske [:Dolske]]

(In reply to comment #2)
> I've also hit this. Two of the entries in my trunk signons.sqlite have a
> non-empty formSubmitURL (e.g. https://mail.mozilla.com) and a non-null
> httpRealm (equal to "").

Huh. Not sure how that would have happened.

On the upside, Firefox itself shouldn't ever actually be using these signons. Whenever it looks for a form login or http login, it only looks for logins with the other field set to null. So if you have a login with both fields set to non-null values, we don't used it.

tl;dr: sync should be able to safely ignore these logins. (But might still need some try/catch guards anyway?)

Comment 4

[Philipp von Weitershausen [:philikon]]

(In reply to comment #3)
> (In reply to comment #2)
> > I've also hit this. Two of the entries in my trunk signons.sqlite have a
> > non-empty formSubmitURL (e.g. https://mail.mozilla.com) and a non-null
> > httpRealm (equal to "").
> 
> Huh. Not sure how that would have happened.
> 
> On the upside, Firefox itself shouldn't ever actually be using these signons.
> Whenever it looks for a form login or http login, it only looks for logins with
> the other field set to null. So if you have a login with both fields set to
> non-null values, we don't used it.

Ok, that's good to know. So the question is how did this record get created in the first place? At first site I can't find anything in our codebase that would suggest we merge password records. Could there be a bug in the password manager?

> tl;dr: sync should be able to safely ignore these logins. (But might still need
> some try/catch guards anyway?)

Yeah we should probably do that.

Comment 5

[Justin Dolske [:Dolske]]

Oh, I meant to add:

Gavin, could you look at a DB dump for these records, specifically the timestamps for the timeCreated and timeLastUsed columns? Maybe that will shed some light on when this happened.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

> SELECT timeCreated FROM moz_logins WHERE formSubmitURL IS NOT NULL AND httpRealm IS NOT NULL
1269449765833

Which is Wed Mar 24 2010 12:56:05 GMT-0400 (EST).

Comment 7

[:Gavin Sharp [email: [e-mail]]]

(timeLastUsed is the same)

Comment 8

[Justin Dolske [:Dolske]]

Bug 465636 (which added timestamps) landed on March 19th, so most likely this login already existed. But that confirms we're not using it (since timeLastUsed would update).

Comment 10

[Joe Drew (not getting mail)]

> SELECT timeCreated FROM moz_logins WHERE formSubmitURL IS NOT NULL AND httpRealm IS NOT NULL;
1269135855526
1269135855526
1269135855526

which, if those are microseconds since the epoch, is

$ date -jf "%s" 1269135855
Sat 20 Mar 2010 21:44:15 EDT

Comment 12

[Philipp von Weitershausen [:philikon]]

We've been getting lots of reports lately for these issues. Nominating this for a softblocker.

Comment 14

[r.goff]

I'm a user on 4.0b9/Win7 and I got here through a chain of duplicate bugs. I have removed several password entries with null passwords and now my 3.6 clients on Win7 and WinXP will sync. However, the 4.0b9/Win7 client continues to report unknown error, and the sync-log doesn't even have entries for the current date. Is there a better place to report this?

Comment 15

[Philipp von Weitershausen [:philikon]]

(In reply to comment #14)
> I'm a user on 4.0b9/Win7 and I got here through a chain of duplicate bugs. I
> have removed several password entries with null passwords and now my 3.6
> clients on Win7 and WinXP will sync. However, the 4.0b9/Win7 client continues
> to report unknown error, and the sync-log doesn't even have entries for the
> current date. Is there a better place to report this?

The log is disabled by default on Firefox 4. Go to about:config, find services.sync.log.appender.debugLog.enabled and set it to true, then restart.

Comment 16

[Mike Connor [:mconnor]]

Yeah, not sure how pwmgr is getting these, but let's just ignore them.

Comment 17

[Richard Newman [:rnewman]]

Grabbing this; should be quick.

Comment 18

[Richard Newman [:rnewman]]

Attachment: Proposed patch. v1

This patch does a few things.

* Minor: add contributors.
* Minor: add two missing do_test_pending() calls.

Let me know if you want me to split these out.

* PasswordStore._nsLoginInfoFromRecord now logs and returns null if it'll produce a bad record. _findDupe and friends check for null.

* _nLIFR creates an _nsLoginInfo. Said constructor treats undefined as "", so it's possible to get the "both httpRealm and formSubmitURL" error with one of the inputs to _nLIFR being undefined! I added an explicit || null, which results in sane behavior.

* addLogin and modifyLogin calls are wrapped in log+muffle try/catch blocks. We simply want to ignore such invalid records. I'm open to discussion whether we want to do this belt-and-braces approach, given that it could muffle other exceptions, too.

* Add test_password_engine.js, with a basic HTTP-based test for good and bad records. This should eventually be fleshed out for full coverage, but it'll do for the purposes of this bug.

Running CW now.

Comment 19

[Richard Newman [:rnewman]]

(In reply to comment #18)

> Running CW now.

All passed.

Comment 20

[Justin Dolske [:Dolske]]

Comment on attachment 507353 [details] [diff] [review]
Proposed patch. v1

>+    // Passing in "undefined" results in an empty string, which later
>+    // counts as a value. Explicitly `|| null` these fields according to JS
>+    // truthiness.
>     let info = new this._nsLoginInfo(record.hostname,
>-                                     record.formSubmitURL,
>-                                     record.httpRealm,
>+                                     record.formSubmitURL || null,
>+                                     record.httpRealm     || null,

Hmm, you probably don't want that. It is definitely possible to have (old) logins where the formSubmitURL is empty-string ("") (this is how form logins stored previous to bug 360493). It might also be possible to have old HTTP auth logins where the httpRealm is empty-string (I've never seen them in the wild, though).

Where is sync getting |undefined| from? Login manager should never emit such values...

Comment 21

[Richard Newman [:rnewman]]

(In reply to comment #20)


> Where is sync getting |undefined| from? Login manager should never emit such
> values...

Tests. If I construct a WBO without formSubmitURL (not even null), this chunk of code would retrieve |undefined| from the WBO, and the resulting nsLoginInfo will have an empty string value, not null.

It's quite possible that another Sync client (non-Firefox) could produce such records, and right now they'll reliably cause an error in addLogin/modifyLogin.

I'm pretty sure that *if* |undefined| is input to this function, it should never be treated as an empty string, but instead be coerced to null... do you agree?

If so, I'll switch this to an empty-string-preserving approach...

Comment 22

[Justin Dolske [:Dolske]]

I'd be inclined to say that getting undefined should just be handled as an error, but coercing to null is probably ok. [The way login manager handles these two properties, with a mix of null and empty-string, is sucky design I wish I hadn't done. It can be confusing, and as such I've tried to handle them strictly to help avoid footguns.]

Comment 23

[Richard Newman [:rnewman]]

Attachment: Proposed patch. v2

Add a function to translate undefined (only) into null.

Comment 24

[Richard Newman [:rnewman]]

Comment on attachment 507750 [details] [diff] [review]
Proposed patch. v2

Throwing this at mconnor.

Comment 25

[Richard Newman [:rnewman]]

Pushed:

default: http://hg.mozilla.org/services/fx-sync/rev/991a4bd21a19
1.6.x:   http://hg.mozilla.org/services/fx-sync/rev/d58f3606730a

Comment 26

[juan becerra [:juanb]]

Today I tried to sync my account on my Linux machine. Both the Linux machine and the Mac machine with my working profile are running the latest trunk nightlies. When I went through the "add device" procedure to syn the Linux machine, I saw the same type of errors in its error console. The only thing that changed was the line number (192 -> 202)

Let me know if there's another way to verify this bug, and whether it's reasonable to expect that I should have seen this error disappear if I'm using this crufty profile.

Comment 27

[Philipp von Weitershausen [:philikon]]

(In reply to comment #26)
> Today I tried to sync my account on my Linux machine. Both the Linux machine
> and the Mac machine with my working profile are running the latest trunk
> nightlies. When I went through the "add device" procedure to syn the Linux
> machine, I saw the same type of errors in its error console. The only thing
> that changed was the line number (192 -> 202)
> 
> Let me know if there's another way to verify this bug, and whether it's
> reasonable to expect that I should have seen this error disappear if I'm using
> this crufty profile.

This bug has landed on mozilla-central, hence nightlies yet. The merge tracking bug is bug 629780.

Comment 30

[Cristian Nicolae (:xti)]

Attachment: screenshot

This issue is still reproducing on:
Build id : Mozilla/5.0 (Android;Linux armv7l;rv:7.0a1)Gecko/20110525
Firefox/7.0a1 Fennec/7.0a1
Device: HTC Desire
OS: Android 2.2

[Exception... "'Can't add a login with a null or empty password.'
when calling method: [nsILoginManager::addLogin]" nsresult:
"0x8057001e (NS_ERROR_XPC_JS_THREW_STRING)" location: "JS
frame :: resource://services-sync/identity.js :: persist :: line 139"
data:no]
Source File: resource://services-sync/identity.js

Comment 31

[Richard Newman [:rnewman]]

(In reply to comment #30)

> Source File: resource://services-sync/identity.js

That's actually a different issue; this bug is for the passwords Sync engine. The trace you've provided is for persisting credentials (see the different source files in the trace), which is a little odd.

Please file a new bug in the same component; I'll see if I can repro. What were you doing when you got that error?

Comment 32

[Cristian Nicolae (:xti)]

I've browsed to several websites while the app was synced, but I'm not sure about the steps to reproduce.