1245503 - Don't allow empty passwords for logins (e.g. add or update time)

Status: RESOLVED FIXED

(Firefox for iOS :: Data Storage, defect)

Description

[Matthew N. [:MattN]]

The new password management feature allows me to blank the password field. This should be disallowed like on other platforms as this violates assumptions that toolkit password manager has which is a problem due to sync even if the forked code isn't affected.

IMO this should be enforced both in storage and at the UI level,

Comment 1

[Richard Newman [:rnewman]]

Sounds fair to me.

Does this mean that we don't support empty passwords on desktop, or that we only support saving them (not editing)?

Comment 2

[Matthew N. [:MattN]]

(In reply to Richard Newman [:rnewman] from comment #1)
> Does this mean that we don't support empty passwords on desktop, or that we
> only support saving them (not editing)?

Sorry, I filed this from my phone so didn't was a bit terse and didn't provide links.

We don't support logins with an empty password anywhere in toolkit code (which is also used by Android). This is enforced at the storage level. We really shouldn't ship as-is if they're going to end up getting synced to Desktop/Android.

References:
https://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/LoginHelper.jsm?rev=fcd050cd03e3#237
https://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/nsLoginManager.js?rev=f76692f0fcf8&mark=280-281#271

Comment 3

[Stephan Leroux [:sleroux]]

This sounds bad. I'll investigate.

Comment 4

[Stephan Leroux [:sleroux]]

Actually wouldn't this require a new string to tell the user they can't have a blank password? If this is a blocker we could just cancel the submission. Not the greatest UX but if it prevents login chaos I can see it being justified until the next release.

Comment 5

[Richard Newman [:rnewman]]

(In reply to Matthew N. [:MattN] from comment #2)

> We don't support logins with an empty password anywhere in toolkit code

Huh, interesting. My reading of the RFCs suggests that Basic Auth allows an empty password; do we just not support saving that in Firefox, or am I misreading?

Comment 6

[Richard Newman [:rnewman]]

(In reply to Stephan Leroux [:sleroux] from comment #4)
> Actually wouldn't this require a new string to tell the user they can't have
> a blank password? If this is a blocker we could just cancel the submission.
> Not the greatest UX but if it prevents login chaos I can see it being
> justified until the next release.

Just don't offer to save in the UI -- no prompt when browsing, no save button if editing -- if the string is empty.

Then make the storage layer reject.

Comment 7

[Stephan Leroux [:sleroux]]

There's also the ability to edit the login from the new detail page. I'll add validation in there.

Comment 8

[Stephan Leroux [:sleroux]]

Does this also apply for usernames or is it just passwords?

Comment 9

[Matthew N. [:MattN]]

(In reply to Stephan Leroux [:sleroux] from comment #8)
> Does this also apply for usernames or is it just passwords?

No, empty usernames are fine as long as they are an empty string (instead of null).  It's common for sites to have login forms that don't require a username so we definitely want to support them.

Take a look at the two links in comment 2 for other validation code.

If you have some kind of form validation UI you could use that to indicate that the password field is invalid in the UI (e.g. http://grab.by/NQVA) to avoid a string. Either don't allow the user to leave that edit page (don't save) until correcting or have it revert to the old password upon leaving.

Comment 10

[Stephan Leroux [:sleroux]]

I'm going to move this up to 2.0 so its on our radar in Triage. Feel free to put it somewhere else if it doesn't belong there.

Comment 11

[Stephan Leroux [:sleroux]]

Attachment: Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1501

Adding two reviewers since we're close to launch.

Comment 12

[Stephan Leroux [:sleroux]]

master c72264407b08b760b9227f0751dd3fe9b16e2b0f