1369771 - Confirm launching executable more often on Windows

Status: VERIFIED FIXED

(Firefox :: Downloads Panel, defect, P1)

Description

[Mark Striemer [:mstriemer]]

Currently the confirm launching executable prompt only appears on Mac and Linux since Windows has a native prompt that will confirm with users in some cases.

For certain file types however it might not be clear to the user that the file is an executable and there is no prompt shown by Windows, for example .jar files.

We should update the downloads panel to show the confirmation in all cases except for .exe where we know Windows will always show the native prompt.

The full downloads history always shows this prompt but its behaviour should match that of the downloads panel (always show, except for .exe).

Comment 1

[Mark Striemer [:mstriemer]]

Attachment: Bug 1369771 - Confirm launch of executables other than .exe on Windows

Review commit: https://reviewboard.mozilla.org/r/145828/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/145828/

Comment 2

[:Paolo Amadini]

Comment on attachment 8874460 [details]
Bug 1369771 - Confirm launch of executables other than .exe on Windows

https://reviewboard.mozilla.org/r/145828/#review150218

The EXE extension can be mixed case.

nit: use == instead of ===, please.

::: browser/components/downloads/DownloadsCommon.jsm:431
(Diff revision 1)
> +    let isWindows = AppConstants.platform === "win";
> +    let fileExtension;
> +    let match = aFile.leafName.match(/\.([^.]+)$/);
> +    if (match) {
> +      fileExtension = match[1];
> +    }
> +
>      let promiseShouldLaunch;
> -    if (aFile.isExecutable()) {
> +    // Don't prompt on Windows for .exe since there will be a native prompt.
> +    if (aFile.isExecutable() && !(isWindows && fileExtension === "exe")) {

Just use leafName.toLowerCase().endsWith(".exe") or a dedicated RegExp match here, this code path is different from the DownloadIntegration one ayways.

I'd separate the second part of the if statement to a single isWindowsExe variable for clarity, and you can likely inline other variables. Same goes for the DownloadIngtegration changes.

::: toolkit/components/jsdownloads/src/DownloadIntegration.jsm
(Diff revision 1)
> -    // Ask for confirmation if the file is executable, except on Windows where
> -    // the operating system will show the prompt based on the security zone.
> -    // We do this here, instead of letting the caller handle the prompt
> -    // separately in the user interface layer, for two reasons.  The first is
> -    // because of its security nature, so that add-ons cannot forget to do
> -    // this check.  The second is that the system-level security prompt would
> -    // be displayed at launch time in any case.

Most of this comment still applies.

Comment 3

[Mark Striemer [:mstriemer]]

Comment on attachment 8874460 [details]
Bug 1369771 - Confirm launch of executables other than .exe on Windows

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/145828/diff/1-2/

Comment 4

[:Paolo Amadini]

Comment on attachment 8874460 [details]
Bug 1369771 - Confirm launch of executables other than .exe on Windows

https://reviewboard.mozilla.org/r/145828/#review150370

Looks good! Make sure to test this manually in different scenarios. Writing steps to reproduce on the bug will also be helpful for QA.

Comment 5

[Mark Striemer [:mstriemer]]

STR

On Windows

Firefox prompt received

1. Download a .jar file
2. launch the .jar from the downloads panel, full download list, or via an extension
3. you are prompted to confirm launching the file

No Firefox prompt

1. Download a .exe file
2. launch the .exe from the downloads panel, full download list, or via an extension
3. you are not prompted by Firefox, the Windows dialog confirms whether you'd like to run the file

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9dbf22ca2932
Confirm launch of executables other than .exe on Windows r=Paolo

Comment 7

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

https://hg.mozilla.org/mozilla-central/rev/9dbf22ca2932

Comment 8

[Ciprian Georgiu, Desktop QA]

I've reproduced this bug with an old Nightly build from 2017-06-02.

I can confirm that Firefox prompt is properly displayed on 55.0b8 (20170710085521) when launching a .jar file. Tested on Windows 10 x64.

Comment 9

[Daniel Veditz [:dveditz]]

Comment on attachment 8874460 [details]
Bug 1369771 - Confirm launch of executables other than .exe on Windows

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:

  The fix for security bug 1468217 depends on this patch (which
  already exists in other supported branches)

User impact if declined: 
  Won't be able to fix bug 1468217 for ESR-52
Fix Landed on Version:
  Firefox 55
Risk to taking this patch (and alternatives if risky): 
  Well tested code
String or UUID changes made by this patch:
  None

Comment 10

[Ryan VanderMeulen [:RyanVM]]

FYI, I did the rebase of this for ESR52 and ran it through Try already.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=06e0c4483602aee03b9560ab68f54575060acb8d

Comment 11

[Julien Cristau [:jcristau]]

Comment on attachment 8874460 [details]
Bug 1369771 - Confirm launch of executables other than .exe on Windows

let's take this for 52.9 build2 since we're respinning anyway.

Comment 12

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/fab16ad7f256

Comment 13

[Ciprian Georgiu, Desktop QA]

This bug is also verified fixed on 52.9.0-build2 esr (20180621064021), running Windows 10 x64.