Closed Bug 1676345 Opened 4 years ago Closed 3 years ago

Win10 .exe file download from WeTransfer picked up and saved with .jpg extension

Categories

(Toolkit :: Downloads API, defect, P2)

Firefox 84
Desktop
All
defect

Tracking

()

VERIFIED FIXED
85 Branch
Tracking Status
firefox-esr78 --- unaffected
firefox82 --- unaffected
firefox83 --- unaffected
firefox84 --- verified
firefox85 --- verified

People

(Reporter: cfogel, Assigned: Gijs)

References

(Regression, )

Details

(Keywords: regression)

Attachments

(4 files)

Attached image nexJpg.png

Affected versions

  • 84.0a1(2020-11-07)

Affected platforms

  • Windows 10;

Steps to reproduce

  1. Launch Firefox;
  2. Access https://we.tl/t-tKmO6XTDcF
  3. Download file;

Expected result

  • file is saved on device without issues;

Actual result

  • file is saved with .jpg extension not .exe;

Regression range

  • last good: 2020-10-20;
  • first bad: 2020-10-21;
  • Pushlog URL;

Additional notes

  • attached screenshot to illustrate the issue;
  • checked, but no weird extension/associations are made on the OS level;
  • suggested severity is S4, since it appears to be 1off case;
  • other browsers such as Chrome, handle the download of file as expected as well.
Attached image association.gif

Attaching a .gif with associations already made with the browser, in case it might be of any help to debug this.

Has Regression Range: --- → no
Has STR: --- → yes
Component: Extension Compatibility → Downloads API
Product: Firefox → Toolkit
QA Whiteboard: [qa-regression-triage]

Cannot reproduce this issue in order to get a regression range since this appears to be limited to 1 profile.

checked, but no weird extension/associations are made on the OS level;

It'll be about the handlers.json file in your profile.

(In reply to Raluca from comment #2)

Cannot reproduce this issue in order to get a regression range since this appears to be limited to 1 profile.

You can use --profile to pass the profile into mozregression in order to reproduce.

I suspect this regressed with bug 1667787, and should have been fixed by bug 1652520, but it doesn't look like that's the case.

Can you confirm the regression window?

Flags: needinfo?(cristian.fogel)

(In reply to :Gijs (he/him) from comment #3)

I suspect this regressed with bug 1667787, and should have been fixed by bug 1652520, but it doesn't look like that's the case.

In fact, after some testing, perhaps this has been broken longer. It'd be useful to know.

Thanks for the suggestion with the profile load.
Will update the main comment with the regression range complete data.

Indeed mozregression points towards bug 1667787. Prior to that, the downloader window was picking up the file as .exe and not prompting to open with other software.

Flags: needinfo?(cristian.fogel)
Regressed by: 1667787

Set release status flags based on info from the regressing bug 1667787

(In reply to Cristian Fogel, QA [:cfogel] from comment #5)

Thanks for the suggestion with the profile load.
Will update the main comment with the regression range complete data.

Indeed mozregression points towards bug 1667787. Prior to that, the downloader window was picking up the file as .exe and not prompting to open with other software.

With what extension does the file get saved if you do choose to save it?

Flags: needinfo?(cristian.fogel)

As .jpg

Flags: needinfo?(cristian.fogel)

(In reply to Cristian Fogel, QA [:cfogel] from comment #8)

As .jpg

If this happens before bug 1667787 (which is what I think is the case), can you find a regression window for that?

Flags: needinfo?(cristian.fogel)

Sorry for the late reply. As on comment 5, that was it.
Prior to the mozregression build that pointed towards that bug, the downloader did not pick up the file as .jpg or [Save As]/[Open With]; it got downloaded directly as .exe file.

Flags: needinfo?(cristian.fogel)

(In reply to Cristian Fogel, QA [:cfogel] from comment #10)

Sorry for the late reply. As on comment 5, that was it.
Prior to the mozregression build that pointed towards that bug, the downloader did not pick up the file as .jpg or [Save As]/[Open With]; it got downloaded directly as .exe file.

But it's not fixed on current nightly? Can you attach the handlers.json file from the profile with which you can reproduce this?

Flags: needinfo?(cristian.fogel)
Attached file handlers.json

Attached file.
84.0a1(20201115093418) is still affected.

Flags: needinfo?(cristian.fogel)

Setting this to affected again because this is a recent regression and it basically will break downloads for some users, so I think we need a fix in 84.

Flags: needinfo?(gijskruitbosch+bugs)
OS: Windows → All
Hardware: Unspecified → Desktop

There's a whole sequence of stupid things combining to wreak havoc here...

  1. the website doesn't send a filename with Content-Disposition: attachment
  2. the website sends a useless mimetype (binary/octet-stream)
  3. the website does have the filename in the URL, but adds (tracking?) URL query parameters, which now properly disable getting the extension out of the URL (that changed with 1667787 which is how this went from "working" to "broken")
  4. the user profile has bogus information for the bogus mimetype (namely, use "jpg" for these files)

bug 1659008 should fix (4).

There are a number of tempting fixes here otherwise. The simplest that may still be upliftable is probably to partially revert (3) in that we should take the extension out of the URL (if there's no header info) for application/octet-stream, binary/octet-stream and application/x-msdownload or whatever that is.

Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Flags: needinfo?(gijskruitbosch+bugs)

This restores extension discovery logic to using the URL for requests where
there's no filename in the Content-Disposition header information, and where
the mimetype gives no indication of the 'correct' extension.

This is a short-term workaround for the larger issue that we should not have
bad local information for these mimetypes - fixing that is bug 1659008, but
requires using the extension to get a better mimetype, which this patch will
also help with.

The severity field is not set for this bug.
:mak, could you have a look please?

For more information, please visit auto_nag documentation.

Flags: needinfo?(mak)
Severity: -- → S3
Flags: needinfo?(mak)
Priority: -- → P2
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/8960c091c763
honour extensions from URLs with query parameters for useless mimetypes to avoid taking the extension from bogus local data, r=mak
Pushed by btara@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cc51b2d4ae85
Fix clang-format issue CLOSED TREE
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 85 Branch

Comment on attachment 9189285 [details]
Bug 1676345 - honour extensions from URLs with query parameters for useless mimetypes to avoid taking the extension from bogus local data, r?mak

Beta/Release Uplift Approval Request

  • User impact if declined: Files may download with wrong extensions
  • 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: 1. upload a .exe file to wetransfer
  1. create a new firefox profile. Open Firefox with it. Close Firefox.
  2. drop the handlers.json attachment from https://bugzilla.mozilla.org/show_bug.cgi?id=1676345#c12 into that profile
  3. open firefox again
  4. try to download the file
  • List of other uplifts needed: n/a
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): There is decent test coverage and we're basically partially-restoring pre-regressing-patch behaviour, and it's a very very small change - the test was 90% of the effort in writing this...

If we take this, please also take and/or merge in the formatting patch; apparently my clang-format was outdated or something?

  • String changes made/needed: none
Attachment #9189285 - Flags: approval-mozilla-beta?
Flags: qe-verify+
QA Whiteboard: [qa-regression-triage] → [qa-regression-triage][qa-triaged]

Comment on attachment 9189285 [details]
Bug 1676345 - honour extensions from URLs with query parameters for useless mimetypes to avoid taking the extension from bogus local data, r?mak

Seems like this has been a game of whack-a-mole, but I agree that it'd be good to avoid shipping a new regression in 84. Thanks for including automated test coverage. Approved for 84.0b8.

Attachment #9189285 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Verified with 85.0a1 (2020-12-03) and 84.0b4 on Windows 10.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: