Confirm launching executable more often on Windows

VERIFIED FIXED in Firefox -esr52

Status

()

defect
P1
normal
VERIFIED FIXED
2 years ago
10 months ago

People

(Reporter: mstriemer, Assigned: mstriemer)

Tracking

unspecified
Firefox 55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr5261+ verified, firefox55 verified)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
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 hidden (mozreview-request)

Comment 2

2 years ago
mozreview-review
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.
Attachment #8874460 - Flags: review?(paolo.mozmail)
Comment hidden (mozreview-request)

Comment 4

2 years ago
mozreview-review
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.
Attachment #8874460 - Flags: review?(paolo.mozmail) → review+

Updated

2 years ago
Status: NEW → ASSIGNED
Flags: qe-verify+
Priority: -- → P1
(Assignee)

Comment 5

2 years ago
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
Keywords: checkin-needed

Comment 6

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/9dbf22ca2932
Confirm launch of executables other than .exe on Windows r=Paolo
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9dbf22ca2932
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Blocks: 1468217
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
Attachment #8874460 - Flags: approval-mozilla-esr52?
FYI, I did the rebase of this for ESR52 and ran it through Try already.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=06e0c4483602aee03b9560ab68f54575060acb8d
Flags: qe-verify+
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.
Attachment #8874460 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
This bug is also verified fixed on 52.9.0-build2 esr (20180621064021), running Windows 10 x64.
Flags: qe-verify+

Updated

10 months ago
Depends on: 1465458
You need to log in before you can comment on or make changes to this bug.