Closed Bug 1146907 Opened 5 years ago Closed 5 years ago

Dismissing the unblock related dialog window unblocks the download.

Categories

(Firefox :: Downloads Panel, defect)

38 Branch
defect
Not set
Points:
2

Tracking

()

VERIFIED FIXED
Firefox 39
Iteration:
39.3 - 30 Mar
Tracking Status
firefox38 --- verified
firefox39 --- verified

People

(Reporter: VarCat, Assigned: jaws)

References

Details

Attachments

(1 file)

Environment:

FF 38
Build id: 20150323004010
OS: Win 7 x64, Ubuntu 14.04 x86, Mac Os X 10.7.5

STR:

1. Go to http://www.eicar.org/85-0-Download.html 
2. Download eicar.com 68 Bytes 
3. From the download panel Unblock eicar.com
4. Dismiss the unblock related dialog window.

Issue:
Dismissing the unblock related dialog window unblocks the download instead of keeping it blocked.
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Madhava, this bug is due to bug 345067 and uncovers an issue that we won't be able to get fixed for Firefox 38.

This bug means that when a user clicks on the "unblock" button, they are shown a dialog asking them if they are sure they want to unblock the malicious download.

The default button on this dialog is "keep me safe" which keeps the download blocked. The other option on the dialog is to "unblock anyway" which allows the malicious download to be used. If the user clicks the "X" button, or hits Escape, then "unblock anyway" is chosen.

This violates user expectations since "X"/Escape is synonymous with "cancel", which in the way that the wording of this dialog is it would equate to "keep me safe". However, we wanted the default button to be "keep me safe", therefore "cancel" actually maps to "unblock anyways"

So these are our two options:
1) Do nothing, users who hit the X or Escape will have it unblocked unexpectedly.
2) Change the default button to "unblock anyways". User who hit Enter when the dialog appears will unblock the download, but Escape will cancel the action as many users have been trained to do.

What should we do here?
Flags: needinfo?(madhava)
Attached patch PatchSplinter Review
After talking with Gavin, we've got this:

The button ordering doesn't match the way that UX has requested, but the default button stays "keep me safe" and Escape/X will result in "keep me safe" behavior.
Flags: needinfo?(madhava)
Attachment #8582544 - Flags: review?(paolo.mozmail)
Iteration: --- → 39.3 - 30 Mar
Points: --- → 2
Flags: qe-verify+
Flags: in-testsuite+
Flags: firefox-backlog+
Comment on attachment 8582544 [details] [diff] [review]
Patch

Review of attachment 8582544 [details] [diff] [review]:
-----------------------------------------------------------------

Interesting bug!

::: browser/components/downloads/DownloadsCommon.jsm
@@ +550,5 @@
>    confirmUnblockDownload: Task.async(function* (aType, aOwnerWindow) {
>      let s = DownloadsCommon.strings;
>      let title = s.unblockHeader;
>      let buttonFlags = (Ci.nsIPrompt.BUTTON_TITLE_IS_STRING * Ci.nsIPrompt.BUTTON_POS_0) +
> +                      (Ci.nsIPrompt.BUTTON_TITLE_IS_STRING * Ci.nsIPrompt.BUTTON_POS_1 + Ci.nsIPrompt.BUTTON_POS_1_DEFAULT);

nit: you can close the parenthesis on the second line and indent the third option on the third line.

@@ +592,5 @@
>        }
>      });
>  
>      let rv = Services.prompt.confirmEx(aOwnerWindow, title, message, buttonFlags,
> +                                       okButton, cancelButton, null, null, {});

Should add a comment on why the ordering of the buttons is chosen this way.
Attachment #8582544 - Flags: review?(paolo.mozmail) → review+
Comment on attachment 8582544 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/regressing bug #]: regression found in bug 1137909
[User impact if declined]: users may accidentally unblock a malicious download
[Describe test coverage new/current, TreeHerder]: mochitest covers this codepath
[Risks and why]: none expected
[String/UUID change made/needed]: none
Attachment #8582544 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/aebc558ecfea
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
QA Contact: catalin.varga
Attachment #8582544 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 1149412
No longer depends on: 1149412
Verified as fixed using the following environment:

FF 38.0b2
Build Id: 20150406174117
OS: Win 7 x64, Mac Os X 10.9.5, Ubuntu 14.04 x64

FF 39
Build id: 20150409004007
OS: Win 7 x64
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.