407567 - Can't add login with empty formSubmitURL and null httpRealm

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect)

Description

[Justin Scott [:fligtar]]

When trying to add a login with an empty ("") formSubmitURL and a null httpRealm, an exception is thrown: Can't add a login without a httpRealm or formSubmitURL.

An empty formSubmitURL is a wildcard and should be permissible, per dolske.

dolske's suggested workaround seems to work for now - using a fake formSubmitURL and then modifying the login to use an empty one.

Comment 1

[Justin Dolske [:Dolske]]

I was probably thinking that this case shouldn't be allowed, since one should presumably have a formSubmitURL for new logins. But adding back old login entries as you're doing from your Password Exporter extension seems like a legitimate use case too.

Comment 2

[Justin Dolske [:Dolske]]

Attachment: Patch v.1 for review

Comment 3

[Justin Dolske [:Dolske]]

Untargeting, but we should probably take this for 3.0.1.

Comment 4

[Justin Dolske [:Dolske]]

Attachment: Patch v.2

Updated to trunk, and cleaned up the checking in addLogin. It's a little more verbose than it could be, but I think that helps keep it understandable.

One subtle change from the last patch is that a login with both formSubmitURL and httpRealm set to empty-string will result in the "can't set both" error. I had been trying to throw the "need to set one" error because it's a bit less confusing, but that just ends up complicating things, so meh. I suppose we could just add a couple of lines to treat that as a special case, though.

Comment 5

[Justin Dolske [:Dolske]]

Comment on attachment 318948 [details] [diff] [review]
Patch v.2

a1.9?... narrowly scoped change, exhaustive test coverage, no change to how the browser itself saves passwords.

There's a slight risk of breaking login-saving for an extension setting both both the formSubmitURL and httpRealm fields. Such logins would never be used by Firefox, but it's conceivable an extension could hit this. Talked with Gavin about this, felt it was not likely. If it did happen, fixing the extension should be obvious and trivial. Better to shake out any such use now, than it be a problem later.

Comment 6

[Damon Sicore (:damons)]

Comment on attachment 318948 [details] [diff] [review]
Patch v.2

a1.9+=damons

Comment 7

[Justin Dolske [:Dolske]]

Checking in toolkit/components/passwordmgr/src/nsLoginManager.js;
  new revision: 1.30; previous revision: 1.29
Checking in toolkit/components/passwordmgr/test/unit/head_storage_legacy_1.js;
  new revision: 1.11; previous revision: 1.10
Checking in toolkit/components/passwordmgr/test/unit/test_top_loginmgr_1.js;
  initial revision: 1.1