1766480 - Add OS authentication to Remove Login and Remove All Logins commands

Status: NEW

(Firefox :: about:logins, enhancement, P3)

Description

[Sergey Galich [:serg]]

We ask for OS authentication for edit commands, but we forgot to ask for removal. Lets fix that!

Comment 1

[Zoltán Szatmáry]

I think it is the desired behavior of the remove button, the copy and the edit buttons only ask for permissions because they're capable of leaking sensitive data. Removing the login credentials for a particular site isn't really a security concern.

Comment 2

[Sergey Galich [:serg]]

(In reply to Zoltán Szatmáry from comment #1)

I think it is the desired behavior of the remove button, the copy and the edit buttons only ask for permissions because they're capable of leaking sensitive data. Removing the login credentials for a particular site isn't really a security concern.

True, it's not usable to leak data. But here is another attack scenario:

• victim leaves their laptop unlocked (yes, it's a bad habit, but we are all humans)
• attacker removes login to some web site
• attacker creates new login to the same web site, but with their username/password
• careless victim visits that web site, makes a purchase or other payment

It's a bit elaborate, but not unrealistic.

Another example: damage on purpose. Bad actor can easily erase all victim's logins across all their devices, it takes only a few seconds.

Comment 3

[gingivere15]

Attachment: WIP: Bug 1766480 - Add OS authentication to Remove Login(s). r?sgalich

Comment 4

[gingivere15]

I've submitted a partial fix for this bug. It asks permission on the individual Remove Login command, but not on the Remove All Logins command. Please review my commit here: https://phabricator.services.mozilla.com/D153837

This is my first ever commit to Mozilla and Firefox, so if I've made any mistakes or incorrectly followed the contribution guidelines, please let me know how I can fix them and prevent them from occurring in the future

Comment 5

[Sergey Galich [:serg]]

:gingivere15 thank you and welcome! I've added myself as a reviewer there, will review it some time next week. Have a great weekend!

Comment 6

[gingivere15]

Attachment: WIP: Bug 1766480 - Ad OS authentication to Remove All Logins command

r?[e-mail]

Depends on D153837

Comment 7

[gingivere15]

:serg Wanted to give this a bump and say in advance I appreciate you looking over this!

Comment 8

[gingivere15]

(In reply to Sergey Galich [:serg] from comment #5)

:gingivere15 thank you and welcome! I've added myself as a reviewer there, will review it some time next week. Have a great weekend!

Hi :serg , gonna give this one last bump before moving on. If you do review this, please be sure to review both of the PRs as I've added a second to cover the full scope of the bug.

Thanks!

Comment 9

[Sergey Galich [:serg]]

:gingivere15 sorry, it is still marked as "changes planned" and was not at the top of my dashboard! I'll go review it now.

Comment 10

[Sergey Galich [:serg]]

Please "request review" in Phabricator (there is "Add actions" dropdown at the bottom of the page) so we can accept it.

Comment 11

[gingivere15]

(In reply to Sergey Galich [:serg] from comment #10)

Please "request review" in Phabricator (there is "Add actions" dropdown at the bottom of the page) so we can accept it.

I believe I have completed the steps you needed me to in order for you to review. I also amended this commit to include all of the required changes, instead of having 2 separate commits. The child commit D153908 may need to be removed. Sorry for the confusion; I'm still learning how all this works 😅 Thanks!

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

This good-first-bug hasn't had any activity for 2 months, it is automatically unassigned.
For more information, please visit auto_nag documentation.

Comment 13

[tosinsajo]

Hi, If there's no one assigned to this bug can I get assigned.

Comment 14

[Favour]

Please boss can i be assigned the bug?
Thank you...

Comment 15

[Sergey Galich [:serg]]

The bugs are automatically assigned once you submit a patch to Phabricator.

Comment 16

[Favour]

Hi, sir serg
I will appreciate it if you can guide me through fixing this bugThank you

Comment 17

[divyaramaswamy2]

I believe Favour is no longer working on this, so I am now working on submitting a patch for this one.

Comment 18

[BugBot [:suhaib / :marco/ :calixte]]

This good-first-bug hasn't had any activity for 2 months, it is automatically unassigned.
For more information, please visit BugBot documentation.

Comment 19

[Cruz Hernandez]

Attachment: WIP: Bug 1766480 - Add OS authentication to Remove Login and Remove All Logins commands

Comment 20

[Cruz Hernandez]

Hello, I was trying to address point 2 in the previous WIP patch:

"I'd prefer to have reauth everytime for the "remove all" command, right now first edit or remove will trigger OS reauth dialog, but subsequent attempts to remove will not be challenged. Remove all is not something user supposed to run often, but has a huge harm potential. Asking for reauth every time is reasonable."

I noticed that subsequent OS auths will trigger but only after a hard page refresh, any advice on how to proceed with this? Thanks

Comment 21

[Cruz Hernandez]

Attachment: Bug 1766480 - Add OS authentication to Remove Login and Remove All Logins commands. r?dimi

Comment 22

[Cruz Hernandez]

Hello, I've submitted a WIP patch for review (also see comment 20) Thanks!

Comment 23

[Micah [:mtigley] (she/her)]

Thank you for the WIP patch! The team is planning to assess areas where OS Authentication can be improved (including when it should prompt!).

Comment 24

[[:Joe Cardoso] Loan de Deus Vieira Cardoso]

Hi Micah,

I would like to work on this bug. Have your team finished the assessment? Should I start working on this bug?

Thank you