270159 - Download manager adds extension regardless of file's own extension

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[Andre Costa]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0

Download manager always adds a new extension to a file, usually leading to
duplicate extensions (such as .tar.bz2.bz2, .cfg.cfg) or even wrong extensions
(such as .tbz.bin)

Reproducible: Always
Steps to Reproduce:
1. download a file
2. accept default dir location
3.

Actual Results:  
File is saved with duplicate/additional extension appended

Expected Results:  
Keep original extension

Comment 1

[Andre Costa]

Attachment: Screenshot showing download manager dialog with wrong file names

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

maybe caused by bug 267122 in which case it would be ffox only. since mozilla
does not add an extension when it would be identical, -> firefox

Comment 3

[Darin Fisher]

ben: perhaps we did not test our fix for the extension thing carefully enough :(

i also saw this on linux with ffox 1.0 while downloading firefox-1.0.tar.gz.torrent

the resulting file was firefox-1.0.tar.gz.torrent.gz :-(

confirming.

Comment 4

[Darin Fisher]

I suspect that we should have ifdef'd the fixup code to only happen on windows
(or at least not on unix and osx).

Comment 5

[Ali Ebrahim]

*** Bug 264796 has been marked as a duplicate of this bug. ***

Comment 6

[Ali Ebrahim]

Is anyone seeing this on the trunk? If so it should probably block aviary1.1. I
haven't seen this behaviour yet myself...

Comment 7

[Boris Zbarsky [:bzbarsky]]

This would certainly happen on trunk, per the patch in bug 267122

Comment 8

[Jerry Baker]

This wouldn't be a problem if we would stop using ShellExecute on Windows, but
we had this discussion several years ago in another related bug.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Jerry, did you read bug 267122?  The problem was us saving files and then users
double-clicking them, not us launching files.  Nothing to do with your
ShellExecute hobby-horse.

Comment 10

[Jerry Baker]

Who cares what users do after we save the file? If you want to protect users
from actions they take after we're done with the file, why not lock the file so
they can't rename it with the wrong extension and then run it.

Comment 11

[Boris Zbarsky [:bzbarsky]]

(In reply to comment #10)
> Who cares what users do after we save the file?

We do, apparently, since people are actively using browsers as attack vectors by
tricking users into saving files that seem to have one extension while in
reality they have a different one (yay Windows' hiding of extensions).  Please
do read the relevant spoofing vulnerability announcements.

Back to the topic at hand. I strongly urge all this extension stuff be
conditioned on Win32, at least....

Comment 12

[Jerry Baker]

Another trick is phishing. Lets make it so that the user can't enter passwords
or credit card numbers on forms. We do want to protect them from themselves
after all.

Comment 13

[Boris Zbarsky [:bzbarsky]]

As it happens, we've had at least 3 separate patches go in recently to make
phishing more difficult.

So cut out with the useless and misplaced attempts at sarcasm, please.

Comment 14

[Jerry Baker]

My point is that it is a lost cause to try and stop users from doing something
that is beyond your control in the first place. If users can be tricked into
running attachments that they shouldn't be, what makes you think they can't be
tricked into renaming them, or running a special application included to "view"
the file? All this is doing is making life difficult for everyone else.

Does the Mozilla Team have a set of written guidelines to help us determine
which user actions are within the scope of Mozilla's protective efforts, and
which are not?

Comment 15

[yoruk]

As it appears, this only happens when downloading to a Fat32 filesystem. When
saving files to ext2 and ext3, it is ok. However, when I save files (such as
.pdf, .doc and .xls files) to a partition shared between windows and linux, FF
adds extra extension to the original filename, duplicating the extension,
sometimes with wrong extension particularly with tar balls. Hope this makes an
invaluable contribution.

Comment 16

[Jerry Baker]

Happens to NTFS as well.

Comment 17

[yoruk]

Well, since I don't have the guts to write to NTFS from linux, I only mentioned
FAT32. Is this Firefox related or something else? If it is due to FF, will there
be patch for this? Because it is really annoying.

Comment 18

[Boris Zbarsky [:bzbarsky]]

The reason this happens for Windows partitions written from Linux is that the
code has an isExecutable() check, and all files on Windows partition typically
have the execute bit set.

Could we please at least restrict this extension fixup to Windows, where the
isExecutable() check is a little more meaninful?

Comment 19

[nospam]

Quote: "Download manager always adds a new extension to a file, usually leading to
duplicate extensions (such as .tar.bz2.bz2, .cfg.cfg) or even wrong extensions
(such as .tbz.bin)"

I get the exct same behaviour (downloaded filename saved with double file
extensions) on Linux with FFox 1.0. This has never hapened on my FFox 1.0
installation on Windows - As far as I can see, this is only an issue on Linux
builds?

I run FFox 1.0 on both WindowsXP and Linux (Suse 9.1) with a shared (mounted)
fat32 partition destined as the default download location on both OS'es.

I must say, renaming all the downloaded files from Linux does get quite
irritating after a while.

Comment 20

[Boris Zbarsky [:bzbarsky]]

It's an issue with any build.  It will happen only if the file you're saving to
is "executable".  On windows that means having an appropriate executable
extension.  On Linux, it means having the right permission bits set.  Note that
those bits are typically always set if you're saving the file to a non-native
(eg FAT or NTFS) filesystem.  I'll bet you money that saving your files to your
Linux partition under Linux will not mess with the extensions.  Changing the
mount flags for your FAT partition would likewise work.

Comment 21

[nospam]

Quote: "Changing the mount flags for your FAT partition would likewise work."

Which mount flags (I assume in fstab) do you suggest I should change? I'm happy
to try it out and will let you know if it worked.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Try umask=111 (assuming you have 000 now; if you have something like 077, do
177). Note that this may make actually saving there difficult, depending on how
stupid the filepicker you're using decides to be.

Comment 23

[Donnie Roberts]

> Try umask=111
That will make directories inaccessible. On FAT partitions, you can use fmask
instead. It also works on NTFS, but mount (as of version 2.12p) will incorrectly
interpret the value as a decimal unless the value starts with a 0 (ie. use
fmask=0111 or fmask=0177)

For FAT filesystems, there is another option:
Use showexec, this removes the executable bit from all files except .EXE, .COM,
and .BAT.

Comment 24

[Donnie Roberts]

*** Bug 286353 has been marked as a duplicate of this bug. ***

Comment 25

[Tom Malfrère]

I have this problem also.
I only have it with FireFox on Linux.
I use Firefox on Windows98 and XP also, never had the problem there.

I save my files on a local disk formatted with FAT32.

Comment 26

[Michael Scovetta (Scovetta Labs)]

It looks to me like this may be a problem in the
nsOSHelperAppService::GetByExtension function method in
uriloader/exthandler/win/nsOSHelperHappService.cpp. 

Also, it seems to only be a problem when opening files (as opposed to saving
them). Specifically, .tar.gz files, when opened into an external handler, become
.tar.gz.tar files. The content-type given is application/x-tar, which doesn't
seem to be defined in Firefox's mime tables (or in the registry).

Comment 27

[zug_treno]

*** Bug 326808 has been marked as a duplicate of this bug. ***

Comment 28

[Philip Withnall (unavailable)]

*** Bug 327802 has been marked as a duplicate of this bug. ***

Comment 29

[Marco Antonucci]

This bug is still occurring in Firefox version 2.0 on Linux.

Comment 30

[Dave Townsend [:mossop]]

*** Bug 361866 has been marked as a duplicate of this bug. ***

Comment 31

[Laurent Wozniak]

There is another case linked to this where the file is created empty but with the correct extension.
See: Bug 336113

Comment 32

[Alexander Sack]

Attachment: no more special treatment for executables on non-windows architectures in nsHelperAppDlg.js.in

This patch is against current trunk.

The code section I marked as XP_WIN makes no sense on non-windows architectures imo. The patch is ment to fix bug 270159 (extension duplication on fat/ntfs partisions) as well as bug 336113 (empty file on fat/ntfs).

Comment 33

[Alexander Sack]

Attachment: same but just disables code on unix architectures

same patch ... but just prevents unix builds to use that code.

Comment 34

[:Gavin Sharp [email: [e-mail]]]

[e-mail] is the wrong address to use for reviews, use ":bs" instead.

Comment 35

[Christian :Biesinger (don't email me, ping me on IRC)]

how does this fix bug 336113 (empty file on fat/ntfs)?

Comment 36

[Alexander Sack]

(In reply to comment #35)
> how does this fix bug 336113 (empty file on fat/ntfs)?
> 

Looks like the code assumes that all files for which aLocalFile.isExecutable() && !this.mLauncher.targetFile.isExecutable() is true have a primaryExtension in mimeinfo; this is probably true on windows, but not in case where OS sets executable flag for every file - e.g. fat/ntfs unix without special mount tweaks; thus, when saving
  http://www.mozilla.com/en-US/products/download.html?product=firefox-1.5.0.7&os=linux&lang=en-US 

to fat partision we get:

Error: [Exception... "Component returned failure code: 0xc1f30001 (NS_ERROR_NOT_INITIALIZED) [nsIMIMEInfo.primaryExtension]"  nsresult: "0xc1f30001 (NS_ERROR_NOT_INITIALIZED)"  location: "JS frame :: file:///usr/lib/firefox/components/nsHelperAppDlg.js :: anonymous :: line 242"  data: no]
Source File: file:///usr/lib/firefox/components/nsHelperAppDlg.js
Line: 242

Comment 37

[Christian :Biesinger (don't email me, ping me on IRC)]

ah, ok. that makes sense, thanks.

Comment 38

[Mike Connor [:mconnor]]

Comment on attachment 269231 [details] [diff] [review]
no more special treatment for executables on non-windows architectures in nsHelperAppDlg.js.in

r=me

IMO it makes more sense to #ifdef XP_WIN this

I really don't know if this matters for any non-Unix platform other than Windows, I'm inclined to think not, so we should try this.

Comment 39

[Mike Beltzner [:beltzner, not reading bugmail]]

Not a blocker as we've shipped twice with it and the case is pretty edge, but we'd definitely take this.

Comment 40

[Shawn Wilsher :sdwilsh]

did this ever get checked in?

Comment 41

[Reed Loden [:reed]]

Mardak, did bug 396477 affect this?

Comment 42

[Ed Lee :Mardak]

Well, bug 396477 just makes sure to not add an extension if it's already there. The purpose of the extension adding is to prevent users from saving a file as an executable when it's not really an executable - e.g., test.tgz -> save as test.exe becomes test.exe.tgz.

If the ntfs mount is incorrectly setting the executable bit, the code would potentially make "file.tar.gz" saved as "file.tgz" by the user on that mount as "file.tgz.gz". However, "file.tar.gz" saved as "f.tar.gz" won't get the extra extension because of bug 396477's fix.

What's the purpose of this extension adding logic, and does it only really make sense for Windows?

Comment 43

[Jerry Baker]

(In reply to comment #42)
> What's the purpose of this extension adding logic, and does it only really make
> sense for Windows?

It doesn't even make sense for Windows. Despite the futility of trying to protect the ignorant from themselves, there remain quite a few people at the decision-making level convinced that it is an achievable goal, and that the inconvenience to experienced users is of no consequence. Note the double standard where Mozilla Foundation undertakes large efforts to keep careless shooters from shooting themselves in the foot, but will not do things like encrypt mbox files "because file security is the OS's job."

Comment 44

[Ed Lee :Mardak]

Should be fixed by bug 398551.