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

RESOLVED FIXED

Status

()

RESOLVED FIXED
8 years ago
25 days ago

People

(Reporter: jbecerra, Assigned: rnewman)

Tracking

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 final+)

Details

(Whiteboard: [softblocker][has patch][has review])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

8 years ago
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
Duplicate of this bug: 619427
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.
Component: Firefox Sync: UI → Firefox Sync: Backend
QA Contact: sync-ui → sync-backend
(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?)
(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.
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.
> 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).
(timeLastUsed is the same)
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).
Duplicate of this bug: 621058
OS: Linux → All
Hardware: x86_64 → All
> 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
Duplicate of this bug: 627079
We've been getting lots of reports lately for these issues. Nominating this for a softblocker.
blocking2.0: --- → ?
Summary: 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. → Password sync: don't add apply records with both a httpRealm and formSubmitURL or null/empty passwords
Whiteboard: [softblocker]
Duplicate of this bug: 621024

Comment 14

8 years ago
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?
(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.
Yeah, not sure how pwmgr is getting these, but let's just ignore them.
blocking2.0: ? → final+
(Assignee)

Comment 17

8 years ago
Grabbing this; should be quick.
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
(Assignee)

Comment 18

8 years ago
Created attachment 507353 [details] [diff] [review]
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.
Attachment #507353 - Flags: review?(philipp)
(Assignee)

Updated

8 years ago
Whiteboard: [softblocker] → [softblocker][has patch][needs review philipp]
(Assignee)

Comment 19

8 years ago
(In reply to comment #18)

> Running CW now.

All passed.
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...
Attachment #507353 - Flags: feedback-
(Assignee)

Comment 21

8 years ago
(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...
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.]
Whiteboard: [softblocker][has patch][needs review philipp] → [softblocker][has patch][needs revised patch]
(Assignee)

Comment 23

8 years ago
Created attachment 507750 [details] [diff] [review]
Proposed patch. v2

Add a function to translate undefined (only) into null.
Attachment #507353 - Attachment is obsolete: true
Attachment #507750 - Flags: review?(philipp)
Attachment #507353 - Flags: review?(philipp)
(Assignee)

Updated

8 years ago
Whiteboard: [softblocker][has patch][needs revised patch] → [softblocker][has patch][needs review philipp]
(Assignee)

Comment 24

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

Throwing this at mconnor.
Attachment #507750 - Flags: review?(philipp) → review?(mconnor)
(Assignee)

Updated

8 years ago
Whiteboard: [softblocker][has patch][needs review philipp] → [softblocker][has patch][needs review mconnor]

Updated

8 years ago
Attachment #507750 - Flags: review?(mconnor) → review+

Updated

8 years ago
Whiteboard: [softblocker][has patch][needs review mconnor] → [softblocker][has patch][has review]
(Assignee)

Comment 25

8 years ago
Pushed:

default: http://hg.mozilla.org/services/fx-sync/rev/991a4bd21a19
1.6.x:   http://hg.mozilla.org/services/fx-sync/rev/d58f3606730a
Blocks: 629780
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Reporter)

Comment 26

8 years ago
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.
(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.
Duplicate of this bug: 631938
(Assignee)

Updated

8 years ago
Duplicate of this bug: 614984
Created attachment 535289 [details]
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
(Assignee)

Comment 31

8 years ago
(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?
I've browsed to several websites while the app was synced, but I'm not sure about the steps to reproduce.
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.