1749028 - Hold Down / repeatedly pressing Enter Key will still Automatically Launch Downloaded Executable File

Status: VERIFIED FIXED

(Firefox :: Downloads Panel, defect, P2)

Description

[:Gijs (he/him)]

+++ This bug was initially created as a clone of Bug #1743226 +++

See the original bug for context.

Dan pointed out there that we need to make sure that repeatedly pressing [enter] and/or holding it pressed while opening the downloads panel still doesn't open the file immediately - at least for executables.

Comment 1

[:Gijs (he/him)]

Suggested remedies:

(In reply to Daniel Veditz [:dveditz] from comment #17)

The new design removes all the impediments to launching an executable from the web, all the opportunities to stop and ask yourself if that's what you want to do. That's nice when that's what you intended, I guess, but it also makes it easier to accidentally launch evil executables because there are no speedbumps slowing down the victim.

I don't suppose you could reset the dialog delay timer each keystroke that comes in during the delay period? That would stop this attack, and the similar "press return repeatedly as fast as you can". (It also might annoy impatient people who have to wait longer and longer the more they jump the gun on the time. Speaking of which, without a focus ring or an actual button, there's no visual clue (that I've noticed) that tells the user that the dialog is disabled initially. I don't know if that's much of a problem.

Another thing that might work would be if you detect keystrokes during the delay period, then remove the focus from the item/launch-"button" and make people do some keyboard navigation (tab probably? arrow?) to get that behavior. I guess that means the panel as a whole still needs to have focus so "tab" would do the right thing, so maybe move focus to the last "show all downloads" item so the default is non-dangerous and a tab keypress puts you on the first item.

Comment 2

[:Gijs (he/him)]

Attachment: Bug 1749028, r?dveditz!,kpatenio!

Comment 3

[Daniel Veditz [:dveditz]]

To document the change here rather than make people read the patch: you took the first suggested approach and restart the delay timer on impatient keystrokes (of Enter and Space). This patch doesn't mess with the dialog focus.

Comment 4

[:Gijs (he/him)]

https://hg.mozilla.org/integration/autoland/rev/b1d00f32893d8050b5d7956ac285ff7eaec79818

Comment 5

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for causing mochitest failures in test_expandable.xhtml:

https://hg.mozilla.org/integration/autoland/rev/de6acc53873a47bc05f093af4c113090ab2fc1c9

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&selectedTaskRun=Uq7LsdgbSGuHC4ve3QjiCQ.0&resultStatus=testfailed%2Cbusted%2Cexception%2Cusercancel&revision=b1d00f32893d8050b5d7956ac285ff7eaec79818
Failure log: https://treeherder.mozilla.org/logviewer?job_id=363855511&repo=autoland

TEST-UNEXPECTED-FAIL | accessible/tests/mochitest/states/test_expandable.xhtml | Test timed out. -
TEST-UNEXPECTED-FAIL | accessible/tests/mochitest/states/test_frames.html | Can't get accessible for [ 'menulist@id="menulist" node', address: [object XULMenuElement] 

followed by more failures.

Also: https://treeherder.mozilla.org/logviewer?job_id=363867151&repo=autoland&lineNumber=3935
TEST-UNEXPECTED-FAIL | browser/components/preferences/tests/browser_permissions_dialog.js | The menulist inside the selected richlistitem is focused now - Got [object XULMenuElement], expected [object XULMenuElement]

Comment 6

[:Gijs (he/him)]

Oops, the listener didn't get removed. Fixed now.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

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

Comment 8

[:Gijs (he/him)]

Comment on attachment 9258096 [details]
Bug 1749028, r?dveditz!,kpatenio!

Beta/Release Uplift Approval Request

• User impact if declined: security spoof risk wrt downloads
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: See comment 0 / bug 1743226 and the attachment there
• List of other uplifts needed: n/a
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This is a JS-only change in the downloads code wrt enabling items in the downloads panel, and there's decent automated test coverage
• String changes made/needed: nope

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9258096 [details]
Bug 1749028, r?dveditz!,kpatenio!

Approved for 97.0b3.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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

Comment 11

[Hani Yacoub, Desktop QA]

Verified as fixed on Firefox Nightly 98.0a1 (2022-01-13) on Windows 10 x64, macOS 11.6 and on Ubuntu 20.04 x64.

Comment 12

[Hani Yacoub, Desktop QA]

Verified as fixed on Firefox 97.0b3 on Windows 10 x64, macOS 11.6 and on Ubuntu 20.04 x64.