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)
Tracking
()
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
- Launch Firefox.
- Navigate to about:preferences#privacy, scroll down to 'Logins and Passwords' and click the exceptions button.
- Add www.facebook.com, twitter.com and www.twitter.com and click 'block' for each input.
- In a new tab, navigate to twitter.com and login with valid credentials.
- 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.
Reporter | ||
Comment 1•4 years ago
|
||
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.
Comment 2•4 years ago
|
||
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
Comment 3•4 years ago
|
||
:cbaica, can you enable debug logging and attach the logs here from these STR?
Reporter | ||
Comment 4•4 years ago
|
||
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.
Reporter | ||
Comment 5•4 years ago
|
||
Reporter | ||
Comment 6•4 years ago
|
||
Comment 7•4 years ago
|
||
The priority flag is not set for this bug.
:MattN, could you have a look please?
For more information, please visit auto_nag documentation.
Updated•4 years ago
|
Comment 8•4 years ago
|
||
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.
Reporter | ||
Comment 9•4 years ago
|
||
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.
Comment 10•4 years ago
|
||
The priority flag is not set for this bug.
:MattN, could you have a look please?
For more information, please visit auto_nag documentation.
Updated•4 years ago
|
Comment 11•4 years ago
|
||
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 callsServices.perms.testPermissionFromPrincipal
in LoginManager, which was returning0
instead of the expected2
(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?
Reporter | ||
Comment 12•4 years ago
|
||
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).
Comment 13•4 years ago
|
||
(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?
Comment 14•4 years ago
|
||
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.
Comment 15•4 years ago
•
|
||
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.
Comment 17•4 years ago
•
|
||
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…
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
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Comment 18•4 years ago
|
||
Updated•4 years ago
|
Comment 19•4 years ago
|
||
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
Comment 20•4 years ago
|
||
bugherder |
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Comment 21•4 years ago
|
||
Updated steps to reproduce:
- Ensure you have no saved logins for
www.facebook.com
- Navigate to
about:preferences
- Navigate to the "Privacy & Security" section
- Navigate to "Logins and Passwords"
- Open the Exceptions dialog
- Enter "www.facebook.com" into the "Address of website" field
- Activate "Enter/Return" key to add facebook to the block list
- Save Changes
- Navigate to
www.facebook.com
- Log in
- The save password doorhanger does not appear
Comment 22•4 years ago
|
||
Great job, Tim! Thank you for fixing the issue!
Comment 23•4 years ago
|
||
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
Comment 24•4 years ago
|
||
Good point, though I think very few people are affected… can you file a follow-up to cleanup the bad data?
Updated•4 years ago
|
Comment 25•4 years ago
|
||
Filed the follow-up here : https://bugzilla.mozilla.org/show_bug.cgi?id=1657881
Comment 26•4 years ago
|
||
Since the follow-up up bug was submitted this can be closed as verified-fixed.
Description
•