1332165 - Password fields consisting of only whitespace can cause prompts to save a password

Status: RESOLVED FIXED

(Toolkit :: Password Manager: Site Compatibility, defect)

Description

[(no longer active)]

This affects NextCloud instances with the landing of bug 1166947.

STR:

1. Log in to a NextCloud website.
2. I get a password prompt like this when I get to the Files view.

Alternatively:

1. Log in to a NextCloud website and switch to another tab.
2. After a while (a few minutes, I haven't pinpointed it) switch back the NextCloud tab.  I get a password prompt immediately upon tab switch.

This is really annoying.  We should consider turning this feature off in 51.  Fortunately it has a pref.

Comment 1

[(no longer active)]

Attachment: Screenshot

Comment 2

[(no longer active)]

When I set the signon.debug pref to true, I get the following which is relevant:

onStateChange: loadType isn't LOAD_CMD_NORMAL: 2  LoginManagerContent.jsm:104
Created non-form FormLike for rootElement: <unavailable>  LoginManagerContent.jsm:1482
adding <unavailable> to loginFormRootElements for <unavailable>  LoginManagerContent.jsm:1486
onDOMInputPasswordAdded: <unavailable> <unavailable>  LoginManagerContent.jsm:369
Creating a DeferredTask to call _fetchLoginsFromParentAndFillForm soon  LoginManagerContent.jsm:373
Created non-form FormLike for rootElement: <unavailable>  LoginManagerContent.jsm:1482
adding <unavailable> to loginFormRootElements for <unavailable>  LoginManagerContent.jsm:1486
Arming the onDOMInputPasswordAdded DeferredTask due to DOMContentLoaded  LoginManagerContent.jsm:399
Running deferred processing of onDOMInputPasswordAdded <unavailable>  LoginManagerContent.jsm:380
_detectInsecureFormLikes "https://cloud.ehsan.ws/apps/files/?dir=/"  LoginManagerContent.jsm:456
nsLoginManager:Searching for logins  nsLoginManager.js:401
Login storage:_searchLogins: returning 1 logins for Object { hostname: "https://cloud.ehsan.ws", formSubmitURL: "https://cloud.ehsan.ws" } with options Object { schemeUpgrades: true }  storage-json.js:360
LoginManagerParent:sendLoginDataToChild: 1 deduped logins  LoginManagerParent.jsm:217
_fillForm <unavailable>  LoginManagerContent.jsm:930
_filterRecipesForForm <unavailable>  LoginRecipes.jsm:202
getFieldOverrides: filtered recipes: <unavailable>  LoginRecipes.jsm:227
(form -- no username field found)  LoginManagerContent.jsm:723
Password field <unavailable> has name:  question  LoginManagerContent.jsm:733
form not filled, the password field was already filled  LoginManagerContent.jsm:1068
_detectInsecureFormLikes "https://cloud.ehsan.ws/apps/files/?dir=/"  LoginManagerContent.jsm:456
onLocationChange handled: "https://cloud.ehsan.ws/apps/files/?dir=/&fileid=2" <unavailable>  LoginManagerContent.jsm:81
_onNavigation: state: <unavailable> loginFormRootElements size: 1 document: <unavailable>  LoginManagerContent.jsm:809
_onFormSubmit <unavailable>  LoginManagerContent.jsm:835
_filterRecipesForForm <unavailable>  LoginRecipes.jsm:202
getFieldOverrides: filtered recipes: <unavailable>  LoginRecipes.jsm:227
(form -- no username field found)  LoginManagerContent.jsm:723
Password field <unavailable> has name:  question  LoginManagerContent.jsm:733
nsLoginManager:Checking if logins to "https://cloud.ehsan.ws" can be saved.  nsLoginManager.js:444
nsLoginManager:Searching for logins  nsLoginManager.js:401
Login storage:_searchLogins: returning 1 logins for Object { hostname: "https://cloud.ehsan.ws", formSubmitURL: "https://cloud.ehsan.ws" } with options Object { schemeUpgrades: true }  storage-json.js:360
LoginManagerPrompter:===== initialized =====  nsLoginManagerPrompter.js:751
LoginManagerPrompter:promptToSavePassword  nsLoginManagerPrompter.js:763

Comment 3

[(no longer active)]

Matt, can you please take a look?  Let me know if you need a test account for testing.

Comment 4

[(no longer active)]

The website has this code which is relevant:

<div id="sudo-login-form" class="hidden">
			This action requires you to confirm your password:<br>
			<input class="question" autocomplete="off" name="question" value=" " placeholder="Confirm your password" type="password">
			<input class="confirm icon-confirm" title="Confirm" value="" type="submit">
		</div>

Note that the password field's default value is " ", and I bet that's the star showing up in the screenshot!  It's unclear to me why we're prompting for saving the default value of the password field though.

Comment 5

[(no longer active)]

(And the password field is hidden, which is worse.  I actually don't know how to invoke the UI which this appears on!)

Comment 6

[Matthew N. [:MattN]]

(In reply to :Ehsan Akhgari from comment #0)
> 2. After a while (a few minutes, I haven't pinpointed it) switch back the
> NextCloud tab.  I get a password prompt immediately upon tab switch.

Is it only in Nightly that the prompt appears upon tab switch? When that happens is there a new "promptToSavePassword" in the logging from signon.debug? If you answered yes and then no, that may be a regression from the changes to make doorhangers re-appear when switching back to a tab that had one open (bug 1004061). That shouldn't be happening for ones that aren't set to be persistent (the pwmgr one was made non-persistent on Nightly in the last week or so) and would be worth filing and fixing separately.

> This is really annoying.  We should consider turning this feature off in 51.
> Fortunately it has a pref.

I guess that depends on your answer to my questions above but I'll note that it's easy to dismiss the doorhanger and there is a Never For This Site option that would also fix it. We can also contact the site to remove the space character from their password field value.

Comment 7

[Matthew N. [:MattN]]

There are a few options:
A) Don't prompt to save if neither the username nor password field were modified. We could possibly use an API for the "dirty value flag"[1].
B) Partial solution to reduce annoyance and something that was going to be implemented for another bug: Don't re-prompt for same username and password on that was already prompted for in a dismissed doorhanger. There would be no subsequent prompts as long as the doorhanger anchor is present.
C) Site-specific solution: Add a recipe to prevent offering to save this password (this would mean we wouldn't save if the user actually filled it either though). Since this seems like software that can run on arbitrary domains it would also require bug 1134859 with probably a "*" rule.
D) Site-specific solution: Contact the developer to remove the space character in the password field value.

[1] https://html.spec.whatwg.org/multipage/forms.html#the-input-element:concept-input-value-dirty-flag

Comment 8

[Matthew N. [:MattN]]

FWIW I prefer option (A) which would ideally require exposing mLastValueChangeWasInteractive from HTMLInputElement.cpp. We may be able to settle for exposing mValueChanged or comparing value to defaultValue.

Comment 9

[Matthew N. [:MattN]]

Simplest and easiest to uplift:
E) Add a .trim() in the code that tries to not prompt for empty password fields. (No details because I'm on my phone)
There is a really small chance that we would break something with that.

Comment 10

[Gerry Chang [:gchang]]

We will release 51 today and it's late for 51 now. Mark 51 won't fix.

Comment 11

[(no longer active)]

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #6)
> (In reply to :Ehsan Akhgari from comment #0)
> > 2. After a while (a few minutes, I haven't pinpointed it) switch back the
> > NextCloud tab.  I get a password prompt immediately upon tab switch.
> 
> Is it only in Nightly that the prompt appears upon tab switch?

I mainly run Nightly, so that's the only thing I have tested.

> When that
> happens is there a new "promptToSavePassword" in the logging from
> signon.debug?

I get a password save prompt like the one in the screenshot.

> If you answered yes and then no, that may be a regression from
> the changes to make doorhangers re-appear when switching back to a tab that
> had one open (bug 1004061).

FWIW mozregression pinpointed this to bug 1166947, and the bug goes away when I set the signon.formlessCapture.enabled pref to false.

> That shouldn't be happening for ones that aren't
> set to be persistent (the pwmgr one was made non-persistent on Nightly in
> the last week or so) and would be worth filing and fixing separately.
> 
> > This is really annoying.  We should consider turning this feature off in 51.
> > Fortunately it has a pref.
> 
> I guess that depends on your answer to my questions above but I'll note that
> it's easy to dismiss the doorhanger and there is a Never For This Site
> option that would also fix it. We can also contact the site to remove the
> space character from their password field value.

What's the intended change in bug signon.formlessCapture.enabled?  There is no navigation happening at least one that is visible to the user.  Is it expected for a website to get this behavior if they leave a default value in the password field?

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #7)
> There are a few options:
> A) Don't prompt to save if neither the username nor password field were
> modified. We could possibly use an API for the "dirty value flag"[1].

This is my preferred option as well based on my understanding of the original intention.

> B) Partial solution to reduce annoyance and something that was going to be
> implemented for another bug: Don't re-prompt for same username and password
> on that was already prompted for in a dismissed doorhanger. There would be
> no subsequent prompts as long as the doorhanger anchor is present.

FWIW I have been in situation with many tabs to my cloud drive open and after a while when I switch back to them I get this prompt on every single one.  Even once per tab can be bad in some cases.

> C) Site-specific solution: Add a recipe to prevent offering to save this
> password (this would mean we wouldn't save if the user actually filled it
> either though). Since this seems like software that can run on arbitrary
> domains it would also require bug 1134859 with probably a "*" rule.

FWIW NextCloud is a server side software anyone can install on their own domain, so it won't be possible to come up with a domain whitelist.

> D) Site-specific solution: Contact the developer to remove the space
> character in the password field value.

See above.  Contacting NextCloud is an option for sure, but I'm not sure if the site is doing anything illegitimate here.  All they have is some hidden piece of UI that they don't even show most of the time with this password field...

Comment 12

[Matthew N. [:MattN]]

Attachment: Bug 1332165 - Trim values for the purposes of 'skipEmptyFields' in _getPasswordFields.

We will no longer prompt to save passwords consisting solely of whitespace.

Review commit: https://reviewboard.mozilla.org/r/106778/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/106778/

Comment 13

[Matthew N. [:MattN]]

(In reply to :Ehsan Akhgari from comment #11)
> (In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from
> comment #6)
> > (In reply to :Ehsan Akhgari from comment #0)
> > > 2. After a while (a few minutes, I haven't pinpointed it) switch back the
> > > NextCloud tab.  I get a password prompt immediately upon tab switch.
> > 
> > Is it only in Nightly that the prompt appears upon tab switch?
> 
> I mainly run Nightly, so that's the only thing I have tested.
> 
> > When that
> > happens is there a new "promptToSavePassword" in the logging from
> > signon.debug?
> 
> I get a password save prompt like the one in the screenshot.

Yeah, I understand that but it doesn't answer my question. I'm trying to figure out if you're seeing PopupNotifications.jsm re-opening the existing prompt or if you're seeing pwmgr formless capture detection detecting a new submission at tab switch time. Where "promptToSavePassword" appears in the logs is relevant to what is happening. Both problems will go away for your use case if we fix the hidden input bug with the whitespace value but it sounded like there may be a PopupNotifications regression too that would affect pwmgr in normal cases.

> > If you answered yes and then no, that may be a regression from
> > the changes to make doorhangers re-appear when switching back to a tab that
> > had one open (bug 1004061).
> 
> FWIW mozregression pinpointed this to bug 1166947, and the bug goes away
> when I set the signon.formlessCapture.enabled pref to false.

I understand but it sounded like you may have been hitting two separate bugs because you were seeing the issue on tab switch which recently change in popup notifications.

> > That shouldn't be happening for ones that aren't
> > set to be persistent (the pwmgr one was made non-persistent on Nightly in
> > the last week or so) and would be worth filing and fixing separately.
> > 
> > > This is really annoying.  We should consider turning this feature off in 51.
> > > Fortunately it has a pref.
> > 
> > I guess that depends on your answer to my questions above but I'll note that
> > it's easy to dismiss the doorhanger and there is a Never For This Site
> > option that would also fix it. We can also contact the site to remove the
> > space character from their password field value.
> 
> What's the intended change in bug signon.formlessCapture.enabled?  There is
> no navigation happening at least one that is visible to the user.  Is it
> expected for a website to get this behavior if they leave a default value in
> the password field?

I guess you made a typo with "bug signon.formlessCapture.enabled" but anyways I agree we shouldn't prompt for forms the user didn't interact with and that's a known issue (e.g. bug 1298952 but I believe there are older bugs).

> (In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from
> comment #7)
> > There are a few options:
> > A) Don't prompt to save if neither the username nor password field were
> > modified. We could possibly use an API for the "dirty value flag"[1].
> 
> This is my preferred option as well based on my understanding of the
> original intention.

OK, I'll leave that to bug 1298952 since that's not something I can fix working on part-time on password manager.

> > B) Partial solution to reduce annoyance and something that was going to be
> > implemented for another bug: Don't re-prompt for same username and password
> > on that was already prompted for in a dismissed doorhanger. There would be
> > no subsequent prompts as long as the doorhanger anchor is present.
> 
> FWIW I have been in situation with many tabs to my cloud drive open and
> after a while when I switch back to them I get this prompt on every single
> one.  Even once per tab can be bad in some cases.

OK

> > C) Site-specific solution: Add a recipe to prevent offering to save this
> > password (this would mean we wouldn't save if the user actually filled it
> > either though). Since this seems like software that can run on arbitrary
> > domains it would also require bug 1134859 with probably a "*" rule.
> 
> FWIW NextCloud is a server side software anyone can install on their own
> domain, so it won't be possible to come up with a domain whitelist.

Right, I was thinking of using a global wildcard "*" and indicating that the selector never matches a password field on any site: "#sudo-login-form[class='hidden'] > input[name='question']"

> > D) Site-specific solution: Contact the developer to remove the space
> > character in the password field value.
> 
> See above.  Contacting NextCloud is an option for sure, but I'm not sure if
> the site is doing anything illegitimate here.  All they have is some hidden
> piece of UI that they don't even show most of the time with this password
> field...

Sure, but I'm also not sure that they have a legitimate reason to have whitespace in the value either.

I went with option (E) since I think it's the simplest, reasonable solution.

Comment 14

[(no longer active)]

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #13)
> (In reply to :Ehsan Akhgari from comment #11)
> > (In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from
> > comment #6)
> > > (In reply to :Ehsan Akhgari from comment #0)
> > > > 2. After a while (a few minutes, I haven't pinpointed it) switch back the
> > > > NextCloud tab.  I get a password prompt immediately upon tab switch.
> > > 
> > > Is it only in Nightly that the prompt appears upon tab switch?
> > 
> > I mainly run Nightly, so that's the only thing I have tested.
> > 
> > > When that
> > > happens is there a new "promptToSavePassword" in the logging from
> > > signon.debug?
> > 
> > I get a password save prompt like the one in the screenshot.
> 
> Yeah, I understand that but it doesn't answer my question. I'm trying to
> figure out if you're seeing PopupNotifications.jsm re-opening the existing
> prompt or if you're seeing pwmgr formless capture detection detecting a new
> submission at tab switch time. Where "promptToSavePassword" appears in the
> logs is relevant to what is happening. Both problems will go away for your
> use case if we fix the hidden input bug with the whitespace value but it
> sounded like there may be a PopupNotifications regression too that would
> affect pwmgr in normal cases.

Apologies for my ignorance here, but can you please tell me how to differentiate between the two?  :-) 

> > > If you answered yes and then no, that may be a regression from
> > > the changes to make doorhangers re-appear when switching back to a tab that
> > > had one open (bug 1004061).
> > 
> > FWIW mozregression pinpointed this to bug 1166947, and the bug goes away
> > when I set the signon.formlessCapture.enabled pref to false.
> 
> I understand but it sounded like you may have been hitting two separate bugs
> because you were seeing the issue on tab switch which recently change in
> popup notifications.

Now I understand.  My knowledge about the terminology here is super thin as I have demonstrated!

> > > That shouldn't be happening for ones that aren't
> > > set to be persistent (the pwmgr one was made non-persistent on Nightly in
> > > the last week or so) and would be worth filing and fixing separately.
> > > 
> > > > This is really annoying.  We should consider turning this feature off in 51.
> > > > Fortunately it has a pref.
> > > 
> > > I guess that depends on your answer to my questions above but I'll note that
> > > it's easy to dismiss the doorhanger and there is a Never For This Site
> > > option that would also fix it. We can also contact the site to remove the
> > > space character from their password field value.
> > 
> > What's the intended change in bug signon.formlessCapture.enabled?  There is
> > no navigation happening at least one that is visible to the user.  Is it
> > expected for a website to get this behavior if they leave a default value in
> > the password field?
> 
> I guess you made a typo with "bug signon.formlessCapture.enabled"

Oops that should have been bug 1166947.  Copy paste snafu.

> but
> anyways I agree we shouldn't prompt for forms the user didn't interact with
> and that's a known issue (e.g. bug 1298952 but I believe there are older
> bugs).
> 
> > (In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from
> > comment #7)
> > > There are a few options:
> > > A) Don't prompt to save if neither the username nor password field were
> > > modified. We could possibly use an API for the "dirty value flag"[1].
> > 
> > This is my preferred option as well based on my understanding of the
> > original intention.
> 
> OK, I'll leave that to bug 1298952 since that's not something I can fix
> working on part-time on password manager.

Hmm.  It's not really my place to say who should fix which bug, but looking at the patch here, if the only thing preventing you is the difficulty of a fix, wouldn't checking |element.value != element.defaultValue| do what we want here?

> > > B) Partial solution to reduce annoyance and something that was going to be
> > > implemented for another bug: Don't re-prompt for same username and password
> > > on that was already prompted for in a dismissed doorhanger. There would be
> > > no subsequent prompts as long as the doorhanger anchor is present.
> > 
> > FWIW I have been in situation with many tabs to my cloud drive open and
> > after a while when I switch back to them I get this prompt on every single
> > one.  Even once per tab can be bad in some cases.
> 
> OK
> 
> > > C) Site-specific solution: Add a recipe to prevent offering to save this
> > > password (this would mean we wouldn't save if the user actually filled it
> > > either though). Since this seems like software that can run on arbitrary
> > > domains it would also require bug 1134859 with probably a "*" rule.
> > 
> > FWIW NextCloud is a server side software anyone can install on their own
> > domain, so it won't be possible to come up with a domain whitelist.
> 
> Right, I was thinking of using a global wildcard "*" and indicating that the
> selector never matches a password field on any site:
> "#sudo-login-form[class='hidden'] > input[name='question']"

Oh, I see.  Neat!

> > > D) Site-specific solution: Contact the developer to remove the space
> > > character in the password field value.
> > 
> > See above.  Contacting NextCloud is an option for sure, but I'm not sure if
> > the site is doing anything illegitimate here.  All they have is some hidden
> > piece of UI that they don't even show most of the time with this password
> > field...
> 
> Sure, but I'm also not sure that they have a legitimate reason to have
> whitespace in the value either.
> 
> I went with option (E) since I think it's the simplest, reasonable solution.

I guess if we don't go with the suggestion above, this still fixes the case this bug is talking about, so it's fine.

Thanks for looking into it!

Comment 15

[Matthew N. [:MattN]]

(In reply to :Ehsan Akhgari from comment #14)
> (In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from
> comment #13)
> > (In reply to :Ehsan Akhgari from comment #11)
> > > (In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from
> > > comment #6)
> > > > (In reply to :Ehsan Akhgari from comment #0)
> > > > > 2. After a while (a few minutes, I haven't pinpointed it) switch back the
> > > > > NextCloud tab.  I get a password prompt immediately upon tab switch.
> > > > 
> > > > Is it only in Nightly that the prompt appears upon tab switch?
> > > 
> > > I mainly run Nightly, so that's the only thing I have tested.
> > > 
> > > > When that
> > > > happens is there a new "promptToSavePassword" in the logging from
> > > > signon.debug?
> > > 
> > > I get a password save prompt like the one in the screenshot.
> > 
> > Yeah, I understand that but it doesn't answer my question. I'm trying to
> > figure out if you're seeing PopupNotifications.jsm re-opening the existing
> > prompt or if you're seeing pwmgr formless capture detection detecting a new
> > submission at tab switch time. Where "promptToSavePassword" appears in the
> > logs is relevant to what is happening. Both problems will go away for your
> > use case if we fix the hidden input bug with the whitespace value but it
> > sounded like there may be a PopupNotifications regression too that would
> > affect pwmgr in normal cases.
> 
> Apologies for my ignorance here, but can you please tell me how to
> differentiate between the two?  :-) 
> 
> > > > If you answered yes and then no, that may be a regression from
> > > > the changes to make doorhangers re-appear when switching back to a tab that
> > > > had one open (bug 1004061).
> > > 
> > > FWIW mozregression pinpointed this to bug 1166947, and the bug goes away
> > > when I set the signon.formlessCapture.enabled pref to false.
> > 
> > I understand but it sounded like you may have been hitting two separate bugs
> > because you were seeing the issue on tab switch which recently change in
> > popup notifications.
> 
> Now I understand.  My knowledge about the terminology here is super thin as
> I have demonstrated!

What I'm interested in for this potentially separate bug is about the case where you immediately see the password prompt upon switching back to the tab as doorhanger notifications were recently changed to re-show in some cases when going back to a tab that was showing a notification when you left the tab. This shouldn't happen for the password manager prompt as it wasn't opted into this new behaviour but it sounded like maybe that's what you were seeing. I'm trying to figure out if it's PopupNotifications.jsm (doorhanger code) itself or pwmgr that is opening the prompt upon switching back. To answer that question, rephrasing what I asked in comment 6:

When the prompt appears immediately upon switching back to a tab, was there a new "promptToSavePassword" line in the signon.debug logging output from after you left the tab? i.e. switch away from the NextCloud tab and clear the logs, when you switch back to the NextCloud tab and immediately see the prompt, do you see a "promptToSavePassword" line (for the NextCloud tab) that appeared while the tab wasn't selected?

> > > > That shouldn't be happening for ones that aren't
> > > > set to be persistent (the pwmgr one was made non-persistent on Nightly in
> > > > the last week or so) and would be worth filing and fixing separately.
> > > > 
> > > > > This is really annoying.  We should consider turning this feature off in 51.
> > > > > Fortunately it has a pref.
> > > > 
> > > > I guess that depends on your answer to my questions above but I'll note that
> > > > it's easy to dismiss the doorhanger and there is a Never For This Site
> > > > option that would also fix it. We can also contact the site to remove the
> > > > space character from their password field value.
> > > 
> > > What's the intended change in bug signon.formlessCapture.enabled?  There is
> > > no navigation happening at least one that is visible to the user.  Is it
> > > expected for a website to get this behavior if they leave a default value in
> > > the password field?
> > 
> > I guess you made a typo with "bug signon.formlessCapture.enabled"
> 
> Oops that should have been bug 1166947.  Copy paste snafu.
> 
> > but
> > anyways I agree we shouldn't prompt for forms the user didn't interact with
> > and that's a known issue (e.g. bug 1298952 but I believe there are older
> > bugs).
> > 
> > > (In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from
> > > comment #7)
> > > > There are a few options:
> > > > A) Don't prompt to save if neither the username nor password field were
> > > > modified. We could possibly use an API for the "dirty value flag"[1].
> > > 
> > > This is my preferred option as well based on my understanding of the
> > > original intention.
> > 
> > OK, I'll leave that to bug 1298952 since that's not something I can fix
> > working on part-time on password manager.
> 
> Hmm.  It's not really my place to say who should fix which bug, but looking
> at the patch here, if the only thing preventing you is the difficulty of a
> fix, wouldn't checking |element.value != element.defaultValue| do what we
> want here?

Yeah, I'm aware that the comparison you state would fix this site but I suspect it would also result in us not prompting to save in cases where we should. Sites do all kinds of unusual tricks with hidden fields and I'm thinking about cases where a site creates a new input and sets the value attribute to the the password the user typed. That would cause |element.value == element.defaultValue| yet the user may have actually typed it on that page.

> > > > B) Partial solution to reduce annoyance and something that was going to be
> > > > implemented for another bug: Don't re-prompt for same username and password
> > > > on that was already prompted for in a dismissed doorhanger. There would be
> > > > no subsequent prompts as long as the doorhanger anchor is present.
> > > 
> > > FWIW I have been in situation with many tabs to my cloud drive open and
> > > after a while when I switch back to them I get this prompt on every single
> > > one.  Even once per tab can be bad in some cases.
> > 
> > OK
> > 
> > > > C) Site-specific solution: Add a recipe to prevent offering to save this
> > > > password (this would mean we wouldn't save if the user actually filled it
> > > > either though). Since this seems like software that can run on arbitrary
> > > > domains it would also require bug 1134859 with probably a "*" rule.
> > > 
> > > FWIW NextCloud is a server side software anyone can install on their own
> > > domain, so it won't be possible to come up with a domain whitelist.
> > 
> > Right, I was thinking of using a global wildcard "*" and indicating that the
> > selector never matches a password field on any site:
> > "#sudo-login-form[class='hidden'] > input[name='question']"
> 
> Oh, I see.  Neat!
> 
> > > > D) Site-specific solution: Contact the developer to remove the space
> > > > character in the password field value.
> > > 
> > > See above.  Contacting NextCloud is an option for sure, but I'm not sure if
> > > the site is doing anything illegitimate here.  All they have is some hidden
> > > piece of UI that they don't even show most of the time with this password
> > > field...
> > 
> > Sure, but I'm also not sure that they have a legitimate reason to have
> > whitespace in the value either.
> > 
> > I went with option (E) since I think it's the simplest, reasonable solution.
> 
> I guess if we don't go with the suggestion above, this still fixes the case
> this bug is talking about, so it's fine.

Obviously solution E stops prompting for users with whitespace-only passwords but I think that's less likely than users affected by this bug and less risky that the potential problem I mention about about defaultValue.

> Thanks for looking into it!

You're welcome

Comment 16

[(no longer active)]

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #15)
> What I'm interested in for this potentially separate bug is about the case
> where you immediately see the password prompt upon switching back to the tab
> as doorhanger notifications were recently changed to re-show in some cases
> when going back to a tab that was showing a notification when you left the
> tab. This shouldn't happen for the password manager prompt as it wasn't
> opted into this new behaviour but it sounded like maybe that's what you were
> seeing. I'm trying to figure out if it's PopupNotifications.jsm (doorhanger
> code) itself or pwmgr that is opening the prompt upon switching back. To
> answer that question, rephrasing what I asked in comment 6:
> 
> When the prompt appears immediately upon switching back to a tab, was there
> a new "promptToSavePassword" line in the signon.debug logging output from
> after you left the tab? i.e. switch away from the NextCloud tab and clear
> the logs, when you switch back to the NextCloud tab and immediately see the
> prompt, do you see a "promptToSavePassword" line (for the NextCloud tab)
> that appeared while the tab wasn't selected?

No I just confirmed that is not the case.  These are the lines immediately printed when I switch to the tab as the password prompt shows up:

nsLoginManager:Searching for logins  nsLoginManager.js:401
Login storage:_searchLogins: returning 1 logins for Object { httpRealm: null, hostname: "https://cloud.ehsan.ws", formSubmitURL: "https://cloud.ehsan.ws" } with options Object { schemeUpgrades: true }  storage-json.js:360
browser.windows.onFocusChanged, windowID: 27  background.bundle.js:359:5
nsLoginManager:Searching for logins  nsLoginManager.js:401
Login storage:_searchLogins: returning 1 logins for Object { httpRealm: null, hostname: "https://cloud.ehsan.ws", formSubmitURL: "https://cloud.ehsan.ws" } with options Object { schemeUpgrades: true }  storage-json.js:360

> > Hmm.  It's not really my place to say who should fix which bug, but looking
> > at the patch here, if the only thing preventing you is the difficulty of a
> > fix, wouldn't checking |element.value != element.defaultValue| do what we
> > want here?
> 
> Yeah, I'm aware that the comparison you state would fix this site but I
> suspect it would also result in us not prompting to save in cases where we
> should. Sites do all kinds of unusual tricks with hidden fields and I'm
> thinking about cases where a site creates a new input and sets the value
> attribute to the the password the user typed. That would cause
> |element.value == element.defaultValue| yet the user may have actually typed
> it on that page.

Ah I see.  :/

Do you think exposing something like the mLastValueChangeWasInteractive member of HTMLInputElement to chrome code would be helpful?  This will be true if the last change to the value of the text control has been made by the user.

Comment 17

[Julien Cristau [:jcristau]]

Tracking for 52/53 as recent regression on some sites.

Comment 18

[Justin Dolske [:Dolske]]

Comment on attachment 8829771 [details]
Bug 1332165 - Trim values for the purposes of 'skipEmptyFields' in _getPasswordFields.

https://reviewboard.mozilla.org/r/106778/#review109504

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ed8b01610029
Trim values for the purposes of 'skipEmptyFields' in _getPasswordFields. r=Dolske

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ed8b01610029

Comment 21

[Matthew N. [:MattN]]

(In reply to :Ehsan Akhgari from comment #16)
> (In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from
> comment #15)
> > What I'm interested in for this potentially separate bug is about the case
> > where you immediately see the password prompt upon switching back to the tab
> > as doorhanger notifications were recently changed to re-show in some cases
> > when going back to a tab that was showing a notification when you left the
> > tab. This shouldn't happen for the password manager prompt as it wasn't
> > opted into this new behaviour but it sounded like maybe that's what you were
> > seeing. I'm trying to figure out if it's PopupNotifications.jsm (doorhanger
> > code) itself or pwmgr that is opening the prompt upon switching back. To
> > answer that question, rephrasing what I asked in comment 6:
> > 
> > When the prompt appears immediately upon switching back to a tab, was there
> > a new "promptToSavePassword" line in the signon.debug logging output from
> > after you left the tab? i.e. switch away from the NextCloud tab and clear
> > the logs, when you switch back to the NextCloud tab and immediately see the
> > prompt, do you see a "promptToSavePassword" line (for the NextCloud tab)
> > that appeared while the tab wasn't selected?
> 
> No I just confirmed that is not the case.  These are the lines immediately
> printed when I switch to the tab as the password prompt shows up:

Sorry, I forgot to ask an important detail: Did the key icon appear in the awesomebar before switching away from these tabs?

> nsLoginManager:Searching for logins  nsLoginManager.js:401
> Login storage:_searchLogins: returning 1 logins for Object { httpRealm:
> null, hostname: "https://cloud.ehsan.ws", formSubmitURL:
> "https://cloud.ehsan.ws" } with options Object { schemeUpgrades: true } 
> storage-json.js:360
> browser.windows.onFocusChanged, windowID: 27  background.bundle.js:359:5
> nsLoginManager:Searching for logins  nsLoginManager.js:401
> Login storage:_searchLogins: returning 1 logins for Object { httpRealm:
> null, hostname: "https://cloud.ehsan.ws", formSubmitURL:
> "https://cloud.ehsan.ws" } with options Object { schemeUpgrades: true } 
> storage-json.js:360
> 
> > > Hmm.  It's not really my place to say who should fix which bug, but looking
> > > at the patch here, if the only thing preventing you is the difficulty of a
> > > fix, wouldn't checking |element.value != element.defaultValue| do what we
> > > want here?
> > 
> > Yeah, I'm aware that the comparison you state would fix this site but I
> > suspect it would also result in us not prompting to save in cases where we
> > should. Sites do all kinds of unusual tricks with hidden fields and I'm
> > thinking about cases where a site creates a new input and sets the value
> > attribute to the the password the user typed. That would cause
> > |element.value == element.defaultValue| yet the user may have actually typed
> > it on that page.
> 
> Ah I see.  :/
> 
> Do you think exposing something like the mLastValueChangeWasInteractive
> member of HTMLInputElement to chrome code would be helpful?  This will be
> true if the last change to the value of the text control has been made by
> the user.

I'm not sure yet since we also want to be able to capture the user's last change before the site either clobbers or munges it in some way e.g. blanks the field or masks some characters: j●●●smith. I can file a bug and implement that if/when needed.

Comment 22

[Matthew N. [:MattN]]

Comment on attachment 8829771 [details]
Bug 1332165 - Trim values for the purposes of 'skipEmptyFields' in _getPasswordFields.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1332165 exposed the issue to users but in some sense it's a long-standing issue with only whitespace in password fields.
[User impact if declined]: NextCloud and other sites/services with only whitespace in the password field will get prompts to remember the useless/empty password
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Ehsan confirmed that my patch fixed the issue for him
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None for this part of pwmgr
[Is the change risky?]: No
[Why is the change risky/not risky?]: Trivial one line .trim() change. It will drop support for prompting to save white-space only passwords so if users were depending on that it we won't prompt them anymore. The pwmgr will still fill for them though.
[String changes made/needed]: None

Comment 23

[(no longer active)]

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #21)
> (In reply to :Ehsan Akhgari from comment #16)
> > (In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from
> > comment #15)
> > > What I'm interested in for this potentially separate bug is about the case
> > > where you immediately see the password prompt upon switching back to the tab
> > > as doorhanger notifications were recently changed to re-show in some cases
> > > when going back to a tab that was showing a notification when you left the
> > > tab. This shouldn't happen for the password manager prompt as it wasn't
> > > opted into this new behaviour but it sounded like maybe that's what you were
> > > seeing. I'm trying to figure out if it's PopupNotifications.jsm (doorhanger
> > > code) itself or pwmgr that is opening the prompt upon switching back. To
> > > answer that question, rephrasing what I asked in comment 6:
> > > 
> > > When the prompt appears immediately upon switching back to a tab, was there
> > > a new "promptToSavePassword" line in the signon.debug logging output from
> > > after you left the tab? i.e. switch away from the NextCloud tab and clear
> > > the logs, when you switch back to the NextCloud tab and immediately see the
> > > prompt, do you see a "promptToSavePassword" line (for the NextCloud tab)
> > > that appeared while the tab wasn't selected?
> > 
> > No I just confirmed that is not the case.  These are the lines immediately
> > printed when I switch to the tab as the password prompt shows up:
> 
> Sorry, I forgot to ask an important detail: Did the key icon appear in the
> awesomebar before switching away from these tabs?

It does the first time that I load the page, and whether I remove it or not has no affect over what happens when you switch back to the tab after a while.

Comment 24

[Julien Cristau [:jcristau]]

Comment on attachment 8829771 [details]
Bug 1332165 - Trim values for the purposes of 'skipEmptyFields' in _getPasswordFields.

ignore whitespace-only fields in password manager, aurora53+, beta52+

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/fee1e9a5de81

Comment 26

[Carsten Book [:Tomcat]]

needs rebase for beta

grafting 387377:fee1e9a5de81 "Bug 1332165 - Trim values for the purposes of 'skipEmptyFields' in _getPasswordFields. r=Dolske a=jcristau"
merging toolkit/components/passwordmgr/LoginManagerContent.jsm
merging toolkit/components/passwordmgr/test/unit/test_getPasswordFields.js
warning: conflicts while merging toolkit/components/passwordmgr/LoginManagerContent.jsm! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')

Comment 27

[Matthew N. [:MattN]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e293c1aa1c10