Closed Bug 1777207 Opened 2 years ago Closed 2 years ago

Since I updated to Firefox 102 there is a problem with files in extension .js

Categories

(Firefox :: File Handling, defect, P2)

Firefox 102
Desktop
Windows
defect

Tracking

()

VERIFIED FIXED
106 Branch
Tracking Status
firefox-esr91 --- unaffected
firefox-esr102 --- verified
firefox102 --- wontfix
firefox103 --- wontfix
firefox104 --- wontfix
firefox105 --- wontfix
firefox106 --- verified

People

(Reporter: espace.live, Assigned: enndeakin)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: regression)

Attachments

(2 files)

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:102.0) Gecko/20100101 Firefox/102.0

Steps to reproduce:

Since I upgraded to Firefox 102, so whenever I download a .js file (example: bouncer.js) from the web manager on Webmin it arrives in my "Downloads" folder but it's not "bouncer.js" but "bouncer.js.xml"
Try it you will see!

Attached image 38bce9578d257fb1a1fed2624be79811.png (deleted) —

each time it adds ".xml" at the end of the file

The Bugbug bot thinks this bug should belong to the 'Firefox::File Handling' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Component: Untriaged → File Handling

I created an "index.html" file and put this inside:

<a href="https://raw.githubusercontent.com/freenode/jbnc/master/bouncer.js">https://raw.githubusercontent.com/freenode/jbnc/master/bouncer.js</a>

then right click on the link to save as, and the file will save with "bouncer.js.txt", this time it's not .xml but .txt"
The problem really comes from Firefox 102

In fact, no need to create an html file, because this page is already in html
Just right click here and save as: https://raw.githubusercontent.com/freenode/jbnc/master/bouncer.js
You will see the "txt" extension at the end

When will it be fixed on firefox? this week ?
Because it will be quite boring to rename files to ".js" 30 times a day

I fixed the issue by downgrading to Firefox 101.0.1 using "--allow-downgrade" on first launch to bypass downgrade blocker
".js" files are in ".js" now

I tested on Windows 11 with latest version of Firefox, it saves the javascript file as "bouncer.js.txt" instead of "bouncer.js"
You have to rename the file.js.txt file to file.js each time to open it with VSCode.

Before updating firefox on Windows 11, I had firefox 101.0.1 and it saved the bouncer.js file directly without putting ".txt" at the end.

On Windows 7 the extension at the end is not ".js.txt" but ".js.xml", that's a problem, it should be ".js" only

Blocks: 1778322

I don't see this issue myself.

Can you post your handlers.json?

Does this happen in a new profile?

"Does this happen in a new profile?"
Yes

handlers.json (new profile) :
{"defaultHandlersVersion":{},"mimeTypes":{},"schemes":{},"isDownloadsImprovementsAlreadyMigrated":true,"isSVGXMLAlreadyMigrated":true}

I tested downloading these following links with Firefox 101.0.1 and 102, I right clicked and saved as directly on the url:

Firefox seems to give importance to the header of a file rather than to the extension, and this from the Firefox 102 version

Hello I have tried to reproduce the issue with firefox 104.0a1(2022-07-07), 103.0b5 on Windows 10 but unfortunately I wasn't able to reproduce the issue as described in the first comment, in my case clicking on the "Save Page As" it saves the file as bouncer.js.

Could you please answer the following questions in order to further investigate this issue

  1. Does this issue happen with a new profile? Here is a link on how to create one: https://support.mozilla.org/en-US/kb/profile-manager-create-remove-switch-firefox-profiles
  2. Does this issue happen in the latest nightly? Here is a link from where you can download it: https://www.mozilla.org/en-US/firefox/channel/desktop/
  3. Do you have any addons installed if so can you list them?

Does this issue happen with a new profile? Here is a link on how to create one: https://support.mozilla.org/en-US/kb/profile-manager-create-remove-switch-firefox-profiles

Yes, and also under a new formatting of Windows 11 (VM) without any migration from an old OS

--

Do you have any addons installed if so can you list them?
Adblock plus, (but on Windows 11 I installed none, everything is default and the problem still occurs with the extension of downloaded js files)

--

Does this issue happen in the latest nightly? Here is a link from where you can download it: https://www.mozilla.org/en-US/firefox/channel/desktop/

I will test

I tested with Firefox Nightly 104.0a1 and check out this gif:
https://www.zupimages.net/up/22/27/rp4d.gif (5MB)
It's a Firefox without any plugins added
Has been tested on Windows 11 (in VM) and it does exactly the same thing on my Windows 7 (but I keep my version 101.0.1 I won't test because it's a hassle then to go back to version 101.0.1)

On your computer, in the options of Windows Explorer, uncheck "Hide extensions whose type is known" you will see better, you will see that the esntesions are in bouncer.js.txt

comment 10 is easily reproducible, just right click on the link, Save As, will save as bouncer.js.txt.
The header says content-type: text/plain; charset=utf-8

I suspect this may be related to bug 1772758? .js is in the "executable" list, it is served as text/plain and thus we append .txt.
See https://bugzilla.mozilla.org/show_bug.cgi?id=1772758#c3 and https://bugzilla.mozilla.org/show_bug.cgi?id=1772758#c4

This issue is still reproducible on Firefox 103.0b8(build ID: 20220712185923) and Nightly 104.0a1(build ID: 20220712093327) using the first link from Comment 10 and the steps from Comment 15. Tested on Windows 7, Windows 10 and Windows 11(all 64-bits). macOS and Ubuntu are unaffected.

OS: Unspecified → Windows
Hardware: Unspecified → Desktop

Marking as New based on comment16.

Status: UNCONFIRMED → NEW
Ever confirmed: true

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

For more information, please visit auto_nag documentation.

Flags: needinfo?(gijskruitbosch+bugs)

I suspect that both here and in bug 1772758 we're seeing increased reports because this only used to be an issue when setting files to open automatically and now we validate "even" for files "just" saved to disk. The automatic opening, as well as bug 1568003, was why we had/have this validation in the first place (.js files will normally automatically open with the windows script runner, and that's basically the same as automatically running a .exe file). I'm not entirely convinced these are really the same issues.

Either way, maybe we can consider not overwriting the filename in these cases, but disabling auto-open if it's set (instead saving to disk) ? Neil, does that sound reasonable, or do the abstractions in the code make that very tricky? Are there other solutions that don't reintroduce security problems that I'm missing?

Severity: -- → S2
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(enndeakin)
Keywords: regression
Priority: -- → P2
Regressed by: 1746052

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

Either way, maybe we can consider not overwriting the filename in these cases, but disabling auto-open if it's set (instead saving to disk) ? Neil, does that sound reasonable, or do the abstractions in the code make that very tricky? Are there other solutions that don't reintroduce security problems that I'm missing?

I'm not clear what's being asked here. Which are the 'cases' that you are asking to distinguish?

Flags: needinfo?(enndeakin) → needinfo?(gijskruitbosch+bugs)

(In reply to Neil Deakin from comment #21)

Either way, maybe we can consider not overwriting the filename in these cases, but disabling auto-open if it's set (instead saving to disk) ? Neil, does that sound reasonable, or do the abstractions in the code make that very tricky? Are there other solutions that don't reintroduce security problems that I'm missing?

I'm not clear what's being asked here. Which are the 'cases' that you are asking to distinguish?

Cases where we decide the filename is for an executable file and as a result we overwrite it with .txt or another extension based on the mimetype (ie bug 1772758 where we overwrite with zip). By overwriting we're mitigating against auto-executing, but the filename ends up being wrong (when compared to content-disposition header or download link attribute). I'm asking if we can keep the filename as-is, but change the action we take for that specific file away from "always execute", even if that is what is set for that mimetype. Does that make more sense?

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(enndeakin)

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

Cases where we decide the filename is for an executable file and as a result we overwrite it with .txt or another extension based on the mimetype (ie bug 1772758 where we overwrite with zip). By overwriting we're mitigating against auto-executing, but the filename ends up being wrong (when compared to content-disposition header or download link attribute). I'm asking if we can keep the filename as-is, but change the action we take for that specific file away from "always execute", even if that is what is set for that mimetype. Does that make more sense?

I don't believe the code that changes the extension (in HelperAppDlg.jsm) relevant to this bug gets run when a helper application is assigned to open for that type. I assume this is what you mean by auto-execute. This code (and bug 1772758) only apply when the user chooses Save. Two cases could happen:

  • the file chooser appears defaulting to 'bouncer.js' but it is then saved as 'bouncer.js.txt', which is different that what the user asked for. This also happens if the user entered some other filename that is identified as an executable.
  • the file chooser is skipped, and the file is automatically saved with the wrong extension.
Flags: needinfo?(enndeakin)

Is there anything we can do to help move this bug forward? Seems like a pretty high-visibility S2 regression to be carrying over from release to release (and soon be introducing to the enterprise crowd when they're migrated from 91 to 102).

(In reply to Ryan VanderMeulen [:RyanVM] from comment #25)

Is there anything we can do to help move this bug forward?

I'm pretty swamped right now and about to go on PTO. Perhaps Neil has cycles to investigate further exactly what is happening here and if we could fix this somehow (I'm being a bit handwavy because I don't think I fully understand the root cause given recent comments) without running the risk of files being opened with the wrong helper app as in bug 1568003.

Seems like a pretty high-visibility S2 regression to be carrying over from release to release (and soon be introducing to the enterprise crowd when they're migrated from 91 to 102).

Honestly I'm not sure how high-visibility and/or S2 it should be considered. Like, I don't like this bug because I'm an engineer and I want this code to not have bugs, and I understand it's super annoying for the reporter. But realistically, AFAICT this happens for some file extensions (ones in the executable list), being served with the "wrong" mimetype (text/plain for JS; zip file mimetype in bug 1772758).

It's not 100% clear to me if they have the same root cause, given Neil's recent comments. The zip case honestly seems more common than this one. The jar/zip thing has duplicates, but here it's been over 2 months and there are no dupes or "me too" comments or CCs. So I'm surfacing this for Molly for (re-)consideration for S2 triage. (To be clear: keeping this as S2 is very much a legitimate outcome. I just think we should take another look at this point.)

Flags: needinfo?(mhowell)

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

So I'm surfacing this for Molly for (re-)consideration for S2 triage.

The definition of S2 requires that no reasonable workaround for the problem exist. Given that, I think it's quite clear that this bug does not qualify, because changing the extension manually is a pretty reasonable workaround.

Severity: S2 → S3
Flags: needinfo?(mhowell)

I think the code in question that is causing this bug at https://searchfox.org/mozilla-central/rev/3f9dcc016dd96a0336d46f4a19aeabdd796ab9e9/toolkit/mozapps/downloads/HelperAppDlg.jsm#464 may just not be needed any more. The filename and extension is now already being validated by validateFileNameForSaving().

I verified that this code does not run when launching an application and only runs when saving a file.

Flags: needinfo?(enndeakin)
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED
Pushed by neil@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/9fd7ef3c9d43 don't replace extensions of executables when saving on Windows any more; the filename has already been validated and it can cause the filename a user chose to be incorrect, r=mhowell
Flags: needinfo?(enndeakin)
Pushed by neil@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8833ae91b6fb don't replace extensions of executables when saving on Windows any more; the filename has already been validated and it can cause the filename a user chose to be incorrect, r=mhowell
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 106 Branch
Flags: qe-verify+

I managed to reproduce this issue on Firefox 105.0(build ID: 20220915150737) on Windows 10 using the first link from Comment 10 and the steps from Comment 15. Verified as fixed on Firefox 106.0b4(build ID: 20220925185751) and Nightly 107.0a1(build ID: 20220926213808) on Windows 10, Windows 11.

Status: RESOLVED → VERIFIED
Flags: qe-verify+

Hi Neil, this seems like something we'll want to take on ESR as well. Please nominate when you get a chance.

Flags: needinfo?(enndeakin)
Blocks: 1778597

This is needed in esr102 as it is a dependant to uplift Bug 1778597

Comment on attachment 9293558 [details]
Bug 1777207, don't replace extensions of executables when saving on Windows any more; the filename has already been validated and it can cause the filename a user chose to be incorrect, r=mhowell

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: The filename when saving on Windows can sometimes be given the wrong extension, different from the one selected in the save dialog.
  • User impact if declined: User could be confused and believe that a file was not saved.
  • Fix Landed on Version: 106
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Removes a windows-specific block of code that is now redundant.
Flags: needinfo?(enndeakin)
Attachment #9293558 - Flags: approval-mozilla-esr102?

Comment on attachment 9293558 [details]
Bug 1777207, don't replace extensions of executables when saving on Windows any more; the filename has already been validated and it can cause the filename a user chose to be incorrect, r=mhowell

Approved for 102.5esr.

Attachment #9293558 - Flags: approval-mozilla-esr102? → approval-mozilla-esr102+
Flags: qe-verify+
QA Whiteboard: [qa-triaged]

Hello,
I confirm this fix is verified on Firefox 102.5.0esr(build ID: 20221107191025) on Windows 10 and Windows 11.

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

Attachment

General

Creator:
Created:
Updated:
Size: