Closed Bug 1618899 Opened 4 years ago Closed 4 years ago

Hitting Enter/Return in a permissions exceptions dialog always triggers Allow (instead of Block) even if it's hidden

Categories

(Toolkit :: Password Manager, defect, P2)

Desktop
All
defect

Tracking

()

VERIFIED FIXED
81 Branch
Tracking Status
firefox73 --- wontfix
firefox74 --- wontfix
firefox75 --- wontfix
firefox76 --- wontfix
firefox77 --- wontfix
firefox81 --- verified

People

(Reporter: cbaica, Assigned: tgiles, Mentored)

References

(Depends on 1 open bug)

Details

(Whiteboard: [passwords:management] [lang=js])

Attachments

(5 files)

Affected versions

  • Fx74.0b9
  • Fx75.0a1
  • Fx73.0.1

Affected platforms

  • Windows 7
  • Windows 10
  • Ubuntu 16.04

Steps to reproduce

  1. Launch Firefox.
  2. Navigate to about:preferences#privacy, scroll down to 'Logins and Passwords' and click the exceptions button.
  3. Add www.facebook.com, twitter.com and www.twitter.com and click 'block' for each input.
  4. In a new tab, navigate to twitter.com and login with valid credentials.
  5. In another new tab, navigate to facebook.com and login with valid credentials.

Expected result

  • No prompt to save the password is received for either of the websites.

Actual result

  • At least for one of the websites the 'save password' prompt is displayed.

Regression range

  • Will come back with a regression range ASAP.

Additional notes

  • Please note that the prompt has inconsistent behavior, but couldn't figure out the cause for that.

This does not look like a recent regression. I went back as far as Fx 59.0a1 and I could still reproduce the issue (password save prompt was received) with one mention here, for older builds both https and http had to be added for each input since we had a different behavior on address adding.
Please note that I could reproduce the issue on macOS 10.13 as well.

Has STR: --- → yes

I'm not able to reproduce this with these STR. When I add www.facebook.com and www.twitter.com I get both http:// and https:// entries for each hostname. Attempting to login to either doesnt produce a save password doorhanger, and in the browser console (with signon.debug enabled) I see:

(form submission ignored -- saving is disabled for: https://www.facebook.com

:cbaica, can you enable debug logging and attach the logs here from these STR?

Flags: needinfo?(cristian.baica)

Hello Sam
I've enabled debug logging and copied over the content from browser console. I've done this for facebook.com and twitter.com. I've attached the files accordingly.

Flags: needinfo?(cristian.baica)

The priority flag is not set for this bug.
:MattN, could you have a look please?

For more information, please visit auto_nag documentation.

Flags: needinfo?(MattN+bmo)
Flags: needinfo?(MattN+bmo) → needinfo?(sfoster)

I can see in both these logs that where we check to see if a site has saving enabled, the check is passing so we go ahead and prompt to save. But unfortunately we don't log out exactly which origins are marked as saving-disabled so I can't be sure why that checking is failing or what is going on here. Could you add a screenshot of the exceptions dialog which has this list? I.e. go to preferences#privacy and click on the "Exceptions..." button under Logins and Passwords.

Flags: needinfo?(sfoster) → needinfo?(cristian.baica)

I've attached a short video. Let me know if you need a better quality, because the initial video was 25mb and I had to compress it.
It also displays the requested section of preferences#privacy -> Login and Passwords -> Exceptions.

Flags: needinfo?(cristian.baica)

The priority flag is not set for this bug.
:MattN, could you have a look please?

For more information, please visit auto_nag documentation.

Flags: needinfo?(MattN+bmo)
Flags: needinfo?(MattN+bmo) → needinfo?(sfoster)

I was able to reproduce this issue initially with a local build from moz-central, but not on mozregression. When I went to investigate further found I was unable to reproduce with a fresh profile (and lost the previous profile in the process)

I can confirm a few things:

  • The STR are correct - in the sense that they should result in the expected behavior. The origins in the exception list exactly match the origin of the login we end up prompting for.
  • In the video, you can see :cbaica add the facebook and twitter origins and click to save changes; there's no user error here.
  • When I was able to reproduce, I tracked this down as far as the call to Services.logins.getLoginSavingEnabled in LoginManagerParent#onFormSubmit. That in turn calls Services.perms.testPermissionFromPrincipal in LoginManager, which was returning 0 instead of the expected 2 (DENY_ACTION)

That last point was what I tried to confirm with mozgression but got back the expected DENY_ACTION every way I tried.
I've looked at the patch on bug 1574486 which was the last significant change to this area, but nothing jumps out at me.
The profile I was able to reproduce with was created when we rolled over to v77 - so not very old.

:cbaica, if this still reproduces for you, would you be able to send me the permissions.sqlite from that profile (via email not here)
Also, can you confirm if this reproduces for you with a fresh profile?

Flags: needinfo?(sfoster) → needinfo?(cristian.baica)

Hi Sam,
Yes, the issue is still reproducible, on a fresh profile (the check was done on latest nightly - Fx77.0a1). I'm sending over the requested file in a minute (it will be from the profile created on nightly).

Flags: needinfo?(cristian.baica)

(In reply to Cristian Baica [:cbaica], Release Desktop QA from comment #12)

Hi Sam,
Yes, the issue is still reproducible, on a fresh profile (the check was done on latest nightly - Fx77.0a1). I'm sending over the requested file in a minute (it will be from the profile created on nightly).

Thanks for that. Here's the records of interest:

"3"	"http://www.facebook.com"	"login-saving"	"2"	"0"	"0"	"1587549775708"
"4"	"https://www.facebook.com"	"login-saving"	"2"	"0"	"0"	"1587549775708"
"5"	"http://www.twitter.com"	"login-saving"	"1"	"0"	"0"	"1587549775709"
"6"	"https://www.twitter.com"	"login-saving"	"1"	"0"	"0"	"1587549775709"
"7"	"http://twitter.com"	"login-saving"	"1"	"0"	"0"	"1587549775709"
"8"	"https://twitter.com"	"login-saving"	"1"	"0"	"0"	"1587549775709"
"9"	"https://www.facebook.com"	"storageAccessAPI"	"1"	"2"	"1590141780614"	"1587549780614"
"10"	"https://www.facebook.com"	"3rdPartyStorage^https://www.facebook.com"	"1"	"2"	"1587550689233"	"1587549789234"
"11"	"https://twitter.com"	"storageAccessAPI"	"1"	"2"	"1590141803247"	"1587549803247"
"12"	"https://twitter.com"	"3rdPartyStorage^https://twitter.com"	"1"	"2"	"1587550710034"	"1587549810034"

I've tried copying this permissions.sqlite into my profile and playing through STR and I'm still unable to reproduce. I get expected "(form submission ignored -- saving is disabled for: https://www.facebook.com ) " log message and no doorhanger. So there is some other variable in play here. I don't have any solid theories. That fact that we can't find a regression range suggests that some detail in the profile is necessary. But :cbaica is able to reproduce on a new profile.

I can put together a try build with extra logging, but I'm not sure just yet what to log for. Certainly it would be helpful if the logging in getLoginSavingEnabled logged out the result, but that isn't going to tell us anything new by itself. Do you have any ideas :mattn?

Flags: needinfo?(MattN+bmo)
Attached file permissions.sqlite

This is from a fresh profile from our QA with no personal info, so I'm just going to attach it to the bug so others can investigate and try to reproduce.

Tabular form with headings:

sqlite> SELECT * FROM moz_perms WHERE type = 'login-saving';
id  origin                      type              permission  expireType  expireTime  modificationTime
--  --------------------------  ----------------  ----------  ----------  ----------  ----------------
3   http://www.facebook.com     login-saving      2           0           0           1587549775708   
4   https://www.facebook.com    login-saving      2           0           0           1587549775708   
5   http://www.twitter.com      login-saving      1           0           0           1587549775709   
6   https://www.twitter.com     login-saving      1           0           0           1587549775709   
7   http://twitter.com          login-saving      1           0           0           1587549775709   
8   https://twitter.com         login-saving      1           0           0           1587549775709 

We should never be saving a login-saving permission value of ALLOW_ACTION = 1 so something is wrong here.

Seeing the ALLOW_ACTION = 1 and watching the video lead me to realize that the problem is with the exceptions dialog, not with the permission storage or password manager…

The issue is https://searchfox.org/mozilla-central/rev/41c3ea3ee8eab9ce7b82932257cb80b703cbba67/browser/components/preferences/permissions.js#362-364

  onHostKeyPress(event) {
    if (event.keyCode == KeyEvent.DOM_VK_RETURN) {
      document.getElementById("btnAllow").click();

^ we need btnBlock in this configuration of the exceptions dialog when the allow button is hidden.

Clicking "Block" is fine, hitting Enter/Return isn't as it records as "Allow"

Unfortunately this means we should ideally cleanup existing incorrect permissions… It looks like this is limited to login-saving in current m-c. No others use blockVisble: true with allowVisible: false.

TIL that sitePermissions.xhtml/js is a separate file/dialog even though it's very similar…

https://searchfox.org/mozilla-central/rev/41c3ea3ee8eab9ce7b82932257cb80b703cbba67/browser/components/preferences/permissions.js#369-371 probably shouldn't enable buttons that are hidden which I assume would prevent the .click() from working?

We could also make onHostKeyPress smarter if we think it's worth it.

We should add a test to https://searchfox.org/mozilla-central/source/toolkit/components/passwordmgr/test/browser/browser_exceptions_dialog.js

Mentor: MattN+bmo
Flags: needinfo?(MattN+bmo) → in-testsuite?
Priority: -- → P2
Summary: Password save prompt is received for a website that was added as blocked in the exceptions list → Hitting Enter/Return in a permissions exceptions dialog always triggers Allow (instead of Block) even if it's hidden
Whiteboard: [passwords:management] [lang=js]
Assignee: nobody → tgiles
Status: NEW → ASSIGNED
Attachment #9166283 - Attachment description: Bug 1618899 - Prevent Enter/Return key from activating hidden buttons in exceptions dialog. r=bdanforth → Bug 1618899 - Prevent Enter/Return key from activating hidden buttons in exceptions dialog. r=ntim
Pushed by btara@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d6903f517ff3
Prevent Enter/Return key from activating hidden buttons in exceptions dialog. r=preferences-reviewers,ntim,MattN,bdanforth
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 81 Branch
Flags: qe-verify+

Updated steps to reproduce:

  1. Ensure you have no saved logins for www.facebook.com
  2. Navigate to about:preferences
  3. Navigate to the "Privacy & Security" section
  4. Navigate to "Logins and Passwords"
  5. Open the Exceptions dialog
  6. Enter "www.facebook.com" into the "Address of website" field
  7. Activate "Enter/Return" key to add facebook to the block list
  8. Save Changes
  9. Navigate to www.facebook.com
  10. Log in
  11. The save password doorhanger does not appear

Great job, Tim! Thank you for fixing the issue!

Hey Tim,
I just wanted to show you what happens in case you update from an older version to a newer one. Basically if the sites were previously set in the older version (let's say 80.0a1 - a version where the bug was occurring) and then we do an update to the lastest Nightly ( 81.0a1 (2020-08-06) ) the bug will still persist.
Unless the user manually deletes and then re-enters the sites, the prompt will still be shown.
It's easy for us since we're using Profile Manager to mark this as passed but normal users have one profile and I doubt they will know to delete the sites on every update.
Please see this attached recording for more details.
Video : https://streamable.com/t2wrnd

Flags: needinfo?(tgiles)

Good point, though I think very few people are affected… can you file a follow-up to cleanup the bad data?

Flags: needinfo?(tgiles) → needinfo?(andrei.purice)
Depends on: 1657881
Flags: needinfo?(andrei.purice)

Since the follow-up up bug was submitted this can be closed as verified-fixed.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.