Open Bug 180798 Opened 21 years ago Updated 1 year ago

Add option to automatically unzip (extract) .zip files when they are saved as attachments

Categories

(MailNews Core :: Attachments, enhancement)

Product:
MailNews Core
Component:
Attachments
Type:
enhancement

Tracking

(Not tracked)

People

(Reporter: Bienvenu, Unassigned)

References

(Blocks 1 open bug)

Details

(Whiteboard: [patchlove])

Attachments

(1 file, 4 obsolete files)

Remove pref [checked in]
856 bytes, patch
Bienvenu
: review+
Details | Diff | Splinter Review 
If the user sets the appropriate pref, we want to automatically extract the
contents of a zip file into a directory with the name of the .zip file (w/o the
.zip) underneath the save as directory.
Attached patch proposed fix, first cut (obsolete) — — Splinter Review 
this patch seems to work on Windows. I'll need some help trying it on the mac
and linux. One thing I'm unclear on is what to do about errors. We might just
have to put up a message saying "Error extracting contents of zip file" or
something vague like that. We also need to add a UI for this pref, and figure
out where that should go.
the pref is "messenger.auto_unzip_saved_attachments" - if you set it to true,
and try to save a .zip attachment, it will behave as described above. Jennifer,
where do you think we should put the pref ui for this setting? And what error
msg should we put up on failure? Also, (and I really don't want to go here, but
I think we have to at some point), do we want this to apply to the browser as
well? In which case we'd need to change the pref name, and someone would have to
make this work in the browser too.
Status: NEW → ASSIGNED
The only place I can see to put this global mail pref is in general settings,
since we don't have a place for attachment handling prefs. Jen, what do you think?

        Attachment #106713 -
        Attachment is obsolete: true

    
    

    
  
nice work!  A cool feature.

comments:

1)  

don't forget the default of the new pref in mailnews.js  (else, it will fail and
give error message)

2)

looking at the code, here's an edge case that we could avoid:

get email with bar.zip (bar.zip contains a.gif and b.gif)
choose directory foo (foo has subdir bar, with a file b.gif)
choose to save bar.zip in foo.

+          if (NS_FAILED(rv) && rv != NS_ERROR_FILE_ALREADY_EXISTS)
+            return rv;

will let me choose bar, and continue on.

while saving, I'll save a.gif, and then fail when trying to save b.gif

Instead, can we do the trick where we do CreateUnique()?

In the scenario above, we'd end up with:

foo/bar/a.gif
foo/bar-1/a.gif
foo/bar-1/b.gif

and no error.

3)

what happens in your code if from linux I create foo:bar.zip and send it to you?

foo:bar is not legal on windows.  Will the Create() [or CreateUnique(), see
comment #2]
fail?  

Or will the name be cleaned up by the time it gets to you.

4) what if I send you foo.zip from unix, and it contains a file a:b.gif, and you
save on windows?

will rv = entryFile->Create(nsIFile::NORMAL_FILE_TYPE, 0777); fail?

5)

given that you are using CStrings, there are probably i18n issues.

6a)  [question for mstoltz?]

If I were to double click on a .zip attachment and launch winzip, we hand over
trust to winzip to not allow evil things like "..\..\autoexec.bat".

are we sure that our nsIZipReader won't give us back bad things like this?

or if it does, is AppendNative()

+ rv = entryFile->AppendNative(nsDependentCString(entryName));

smart enough to not allow things like c:\ or ..\..?

6b)  [question for mstoltz?]

same goes for file zip attachment names like ..\..\..\WINDOWS.zip  (will that
map to c:\WINDOWS?)

6c)  [question for mstoltz?]

we're exposing the zip code in mozilla to external data.  Is that asking for
someone to send us something evil? 

Our zip code might be very "trusting", since it might only unzip .xpi or .jar
(which we ask the user if they trust the source.)

this now means it can get used on external .zip files, once that can exploit bugs.

(does this mean we need to prompt the user?)

8)

can a .zip file ever not be .zip?  (like what about .gz?)

I'd understand if we just punted if not .zip

9)

question for jglick:

In the UI, we have "Open | Save | Save All".  should we snarf the name, and if
.zip, have "Open | Save | Unzip | Save All"?


    
    

    
  
8) Mac user uses .sit most of the time!

    
    

    
  
Comment on attachment 106817 [details] [diff] [review]
patch with error message in failure case


>+          zipDir->SetNativeLeafName(nsDependentCString(leafName));
>+          rv = zipDir->Create(nsIFile::DIRECTORY_TYPE, 0755);
0755 makes the dir world-writable, doesn't it? Isn't that too permissive?

Also, and this is a bit out of scope, but is nsIFile::Create guaranteed to be
atomic? Could the file be moved or replaced between checking to see if it
already exists and actually creating the file?

>+                    rv = entryFile->Create(nsIFile::NORMAL_FILE_TYPE, 0777);
Here again, please make sure these are the standard permissions we use for
saving files.

Other than that, r=mstoltz.

        Attachment #106817 -
        Flags: review+

    
    

    
  
> 6a) If I were to double click on a .zip attachment and launch winzip, we hand
> over trust to winzip to not allow evil things like "..\..\autoexec.bat".
> are we sure that our nsIZipReader won't give us back bad things like this?or 
> if it does, is AppendNative() smart enough to not allow things like c:\ or ..\..?

I don't know how smart AppendNative() is. My guess is that nsIZipReader will not
give you any ../ paths, but hopefully dveditz can confirm that.

ccing dveditz.

> we're exposing the zip code in mozilla to external data.  Is that asking for
> someone to send us something evil? 

> Our zip code might be very "trusting", since it might only unzip .xpi or .jar
> (which we ask the user if they trust the source.)
No, we already pass untrusted files to the zipreader (nsJAR.cpp). You can
already do that with a jar:http: URL. So that's not a new thing here. (we have
had our share of problems with maliciously modified .zip files, but this patch
gives no additional cause for concern there)


    
    

    
  
I'm using cstrings because that's what zlib gives me. I wonder if the file names
are encoded by zip in any way. Naoki, do you know? What happens if you zip up
some files that have non-ascii names? What does the zip directory say?

Jean-Francois, are .sit files really zip files, or some other format? I thought
there were some other format.

    
    

    
  
Yeah, General Settings is the most logical place for this setting right now. 
Is this a Windows feature only? Can we say 'zip' file or do we need to be more 
generic, like 'compressed file'?

Cc'ing Robin:
"Automatically expand compressed files when saving attachments"
Or
"Expand compressed files (.zip, .gz, etc.) when saving attachments"
Or
"Automatically expand zip files when saving attachments" (if windows only 
feature)


As for the error, do we provide the error or would WinZip? Do we know why it 
failed? For example, the .zip file isn't valid? 

Dialog Name - "<ProductName>"
"An error occurring when trying to uncompress the file <name>. It is either not 
a valid compressed file, or is corrupted. Try downloading it again." 

    
    

    
  
This feature only works with zip files, not .gz files or .sit files or anything
other than .zip files. We're not using winzip - we're using zlib, which is just
a library of code that unzips .zip files, and has no UI. I don't know what zlib
tells us as far as errors are concerned. We can distinguish between errors in
creating the output directory or empty output files, and errors that happening
reading the zip file, or extracting files out of it, if you think that's
important. I'd be willing to just put up a general error message along the lines
you described, perhaps adding something more general that would encompass things
like running out of disk space or other errors that refer to the save process
but distinct from the format of the .zip file.

    
    

    
  
oh, and this is an xp feature. Seth verified that it works on Linux, and
Jean-Francois verified that it works on the Mac.

    
    

    
  
I do not have information about what charset encoding used in Winzip.
But it supports non ASCII file names. I tried Winzip 8.1 on WinXP JA and
Japanese file names are supported for both zip file name and attached file
names. However, I was not able to create .zip file and attach files in Korean
name on WinXP JA.
So I assume that it supports the current systems default charset. That case, we
need something similar to the import code that is converting from system default
charset to Unicode.


    
    

    
  
OK, since its always only zip files how about:
"Automatically expand .zip files when saving attachments"  
Is this turned on by default?

Robin, is it ok to say ".zip" or should it be "zip"?

Error dialog: if we can distinguish Why the action failed, its good to provide 
that info to the user and what they can do to try and fix it, so i'd probably 
recommend two sep error dialogs if possible. Robin can maybe suggest an eloquent 
way to combine them. 

"An error occurred when trying to unzip the file <name>. It is either not 
a valid .zip file, or is corrupted. Try downloading it again." 

"An error occurred when trying to save the file <name>. Make sure you have 
enough disk space and that the save location specified is valid."



    
    

    
  
I think I can distinguish between an error opening the zip file (if it's really
not a zip file, for example), and other errors. I took a non zip file, changed
it's name to .zip, and tried to save it, and got an error from the zlib Open
call. So my plan is to put up one error msg for that case, along the lines of
your message "...It is either not a valid .zip file, or is corrupted". I've also
reworked the code so that we don't create the directory if we can't open the zip
file. I'll attach a new patch in a minute.

The second message ""An error occurred when trying to save the file <name>. Make
sure you have enough disk space and that the save location specified is valid."

I think should start out instead : "An error occurred when trying to unzip the
contents of file <name>..." instead of "trying to save".

    
    

    
  

      
      
      
      

        Attached patch
          
          
        better error handling (obsolete)
        — 
          — Splinter Review
      

    


  
patch as I described before. The remaining issue is the one Naoki pointed out -
is there some code I can look at that handles the non-ascii file names?

        Attachment #106817 -
        Attachment is obsolete: true

    
    

    
  
>I think should start out instead : "An error occurred when trying to unzip
>the contents of file <name>..." instead of "trying to save".

ok.

    
    

    
  
>"Automatically expand .zip files when saving attachments"  

I think it's fine to use ".zip". 

>"An error occurred when trying to unzip the file <name>. It is either not 
>a valid .zip file, or is corrupted. Try downloading it again." 

If it's not valid or is corrupted, will downloading it again possibly fix the
problem? If not, we should leave off the "Try downloading it again." 

    
    

    
  
I'd say that downloading it again is almost never going to help. 99.99% of the
time, I'd bet the .zip attachment is wrong - I'd leave that part out. Jen, what
do you think?

    
    

    
  
Agree, if it won't work, we shouldn't suggest it. And 'corrupted' isn't a real 
word according to webster's ;-)

"An error occurred when trying to unzip the file <name>. It is either not 
a valid .zip file, or is corrupt." 

    
    

    
  
www.dictionary.com, says corrupted is valid, but
http://www.m-w.com/cgi-bin/dictionary (merriam-webster) doesn't include it as a 
valid form. Weird.

Robin, we'll let you make the call on this one. ;-)

    
    

    
  
American Heritage dictionary (our dept. standard) lists "corrupted" as an
alternate form. Let's stick with "corrupt".

>"An error occurred when trying to unzip the file <name>. It is either not 
>a valid .zip file, or is corrupt." 

This is fine.



    
    

    
  
Catching up on previous comments.

a name in a zip file is just a bunch of text, probably in the OS character set
of the machine that created it, but there's no way to tag it and no way to be
sure. The best you can do is assume the native file character set of the
*recipient's* machine and hope most friends are in the same region using the
same charset. Be ready to deal with invalid filenames for the exception cases
(error? translate invalid chars to '_'?).

The format is also easily hacked, you should definitely defend against "..", and
you'll have to do it yourself since nsLocalFile doesn't appear to disallow it.

We are already exposed to external zip files through the not very well known
jar: protocol. we've had to fix security bugs in our handling of malicious zip
files so the hackers are definitely aware of this as an attack vector.

I believe you want AppendRelativeNativePath(), Append[Native]() only allows for
simple leaf names. AppendRelative requires paths be '/' delimited, but that's OK
because the zip archive is defined that way, too.

Some entries in a zip archive are "directory" entries. Skip them, or create a
directory from them, but if you create a file for them you'll screw yourself
when you need to later create a subfile. You can recognize these as ending with a'/'

    
    

    
  
Comment on attachment 106817 [details] [diff] [review]
patch with error message in failure case

In light of Dan's comments, I'm removing my r+ until the ../ issues are
resolved.

        Attachment #106817 -
        Flags: review+ → review-

    
    

    
  
Thanks for the info, Dan. I want to make sure I'm handling this .. part
correctly. It sounds like if I use AppendRelativeNativePath, I'd need to check
for the native ../ path, so "../" on unix and "..\" on windows and just ignore
files with those strings in the name. Is there anything like that on the Mac?
I'm assuming not.

Or, is it the case that I should be using AppendRelativePath, not
AppendRelativeNativePath, because zlib uses '/' as the directory separator? And
should I just check for ".." and not worry about if it's "../" or "..\"? From
what I can tell, that seems to be the way to go.  Or, do I want to use
AppendRelativeNativePath because in theory it deals with the native charset
filename conversion?

    
    

    
  
The difference between Native and not is CString vs Unicode. Either way
AppendRelative[Native]Path requires unix-style '/' on all platforms. In this
case conveniently matching zip archive format.

Mac has a version of ".." natively, "::" I believe. But doesn't matter in this case.

    
    

    
  
I didn't mean to suggest unicode vs. ascii was the difference, though there is a
comment in nsILocalFile.idl that implies that a charset conversion is done with
the RelativeNative method - I thought the main difference was the hierarchy
delimiter, so that with RelativeNative, on windows, it treated '\' as a
hierarchy delimiter and '/' for Linux, and with Relative, it treats '/' as the
delimiter on both windows and linux.

    
    

    
  
The "Native" functions were added when the API added Unicode entrypoints so
scripted access would work correctly for non-ascii text. AppendRelativePath is
implemented as "convert Unicode to native, call AppendRelativeNativePath".

For convenience or consistency, the delimiter '/' is used on all platforms,
Unicode or not. I'm guessing so ends can be stripped out of file:// or resource:
urls easily, or because they store relative paths that way in the component
registry or something.

    
    

    
  
From what I can tell, unless I'm reading the code wrong, at least on windows,
neither AppendRelativeNativePath or AppendNative allow '/' in the file name, so
I think I have to break apart the zlib entry file names by hand. 

    
    

    
  
Argh, I'm so sorry for the misinformation (and I'd better go re-check my own
code stat). Yes, it looks like AppendRelative requires the OS delimiter. You'll
have to look for '/' in the zip path and add each node using AppendNative().

    
  
Blocks: 178310

    
    

    
  
um
wouldn't it be far more intuitive if the context menu for attachments had an
option "Unzip To..." if the attachment is a zip file? You coudl then choose for
each zip file separetely if you want to save or unzip it, and it would also be
more discoverable.

    
    

    
  


  
I check for ".." in the path, and take it apart by hand now. It seems to work
fine now.

    
  

        Attachment #108078 -
        Flags: review?(mstoltz)

    
    

    
  
I've requested a review from Mitch, but Dan, if you want to look at it instead,
that would be fine too.

    
    

    
  
Comment on attachment 108078 [details] [diff] [review]
patch addressing review comments.

sr=sspitzer

are there any UI spin off bugs?

        Attachment #108078 -
        Flags: superreview+

    
    

    
  
We need to make sure there's no way to slip in a ".." around the check, say by
using . with the high bit set, or maybe by some sort of escape sequence that the
OS will unescape. We've had problems like this before.

    
    

    
  
any suggestions? It seems to me the only way to find that out is to put support
for that in nsLocalFile. Is there support for that there?

    
    

    
  
could somebody comment on my suggestion in comment 31? or should I open a new
bug for that?

    
    

    
  
yes, that would be more intuitive - I'm more worried about getting the backend
checked in at the moment. I'm not sure how easy it is to customize that context
menu based on the attachment name, and we'd have to add a new command to
nsIMessenger for unzip...

    
    

    
  
does either nsIFile::target or nsIFile::path give the "unescaped" name?  It
looks like target might give me that. When we had this problem before, how did
we solve it?
QA Contact: yulian → stephend

    
    

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

    
    

    
  
Mitch or Dan, any further comments on how to avoid malicious file names in .zip
files?

    
    

    
  
David, can we come up with some tests of zip files containing malicious file
names, and see if things are escaped properly?

    
    

    
  
sure, I can try setting the high bit on some special chars

    
    

    
  
Also try the %xx escape sequence for '.'

    
    

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

    
    

    
  
See also bug 175008, "extract .zip files 'on the fly' during download".

    
    

    
  
Let me know the results of your high-bit tests and any other malformed filename
tests you have come up with, and I can re-review this. Also, please add the
tests to this bug so that QA can add them to the suite.

    
    

    
  
any update on the cool cool feature?

    
    

    
  
As useful as this feature is, it would be far more useful if instead of having
the global preference:
  "Automatically unzip downloaded zip files"
One instead had:
  "Prompt to unzip downloaded zip files"

So when a zip file had finished downloading, a message box popped up saying
"Finished downloading foo.zip. Would you like to unpack it to
c:\files\...\stuff\foo\ ?    YES | NO"

    
    

    
  
I personally think the best solution is to have an unzip context menu in the
attachment pane that shows up if the attachment is a zip file.

I don't have time to work on this - if someone can construct the test cases that
Mitch mentioned in some of his comments, then we could make progress to checking
this in.

    
    

    
  
David, I don't want to mess around with your bugs. But Mitchell seems to be gone
from Mozilla development. Perhaps you should shift your review request to
someone else like dveditz for example.

Something like that would be a valuable addition to Thunderbird for sure.

    
    

    
  
Comment on attachment 108078 [details] [diff] [review]
patch addressing review comments.

If we're going to do something like this we need to change it along the lines
suggested, either prompt to expand (ick) or a new (context?) menu item.
Expanding automatically is going to mess with a lot of people (and is of
questionable utility given that most OS's can be set up to expand these
already).

        Attachment #108078 -
        Flags: review?(security-bugs) → review-
Product: MailNews → Core
Summary: Add option to automatically unzip .zip files when they are saved as attachments → Add option to automatically unzip (extract) .zip files when they are saved as attachments

    
  
QA Contact: stephend → attachments
Product: Core → MailNews Core

    
    

    
  
is this still wanted?
Severity: normal → enhancement
Whiteboard: [patchlove]

    
    

    
  
I'd say so - mail.app does this, for example, I believe (or mac osx?)

    
    

    
  
(In reply to comment #53)
> is this still wanted?

Nominating wanted-thunderbird3 to keep it on the radar.
Flags: wanted-thunderbird3?

    
    

    
  
Comment on attachment 106934 [details] [diff] [review]
better error handling

Patch has bitrotted.

$ patch --dry-run < ~/Desktop/tbTestPatches/a180798.diff 
(Stripping trailing CRs from patch.)
patching file pref-mailnews.xul
Hunk #1 FAILED at 37.
Hunk #2 FAILED at 55.
2 out of 2 hunks FAILED -- saving rejects to file pref-mailnews.xul.rej
(Stripping trailing CRs from patch.)
can't find file to patch at input line 37
Perhaps you should have used the -p or --strip option?
The text leading up to this was:
--------------------------
|Index: prefs/resources/locale/en-US/pref-mailnews.dtd
|===================================================================
|RCS file: /cvsroot/mozilla/mailnews/base/prefs/resources/locale/en-US/pref-mailnews.dtd,v
|retrieving revision 1.26
|diff -u -w -r1.26 pref-mailnews.dtd
|--- prefs/resources/locale/en-US/pref-mailnews.dtd     8 Oct 2002 23:16:20 -0000       1.26
|+++ prefs/resources/locale/en-US/pref-mailnews.dtd     20 Nov 2002 20:15:23 -0000
--------------------------
File to patch:

        Attachment #106934 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 108078 [details] [diff] [review]
patch addressing review comments.

Patch has bitrotted.

$ patch --dry-run < ~/Desktop/tbTestPatches/b180798.txt 
(Stripping trailing CRs from patch.)
patching file nsMessenger.cpp
Hunk #1 FAILED at 131.
Hunk #2 succeeded at 146 with fuzz 1 (offset 6 lines).
Hunk #3 FAILED at 271.
Hunk #4 FAILED at 1064.
Hunk #5 succeeded at 1559 (offset -93 lines).
Hunk #6 succeeded at 1752 with fuzz 1 (offset -63 lines).
Hunk #7 succeeded at 1871 with fuzz 1 (offset -38 lines).
3 out of 7 hunks FAILED -- saving rejects to file nsMessenger.cpp.rej
(Stripping trailing CRs from patch.)
patching file nsMessenger.h
Hunk #1 FAILED at 62.
1 out of 1 hunk FAILED -- saving rejects to file nsMessenger.h.rej

    
    

    
  
Not a priority for tb3; wanted‑thunderbird3-
Assignee: bienvenu → nobody
Flags: wanted-thunderbird3? → wanted-thunderbird3-

    
    

    
  
Comment on attachment 108078 [details] [diff] [review]
patch addressing review comments.

Obsoleting due to rejected review.

        Attachment #108078 -
        Attachment is obsolete: true

    
  
Status: ASSIGNED → NEW

    
    

    
  


  
While Seth was certainly right about the need to add a default value for the pref before checking it, as it turns out adding it more than 8 years before checking it was probably premature ;)

        Attachment #487223 -
        Flags: review?(bienvenu)

    
  

        Attachment #487223 -
        Flags: review?(bienvenu) → review+

    
    

    
  
Comment on attachment 487223 [details] [diff] [review]
Remove pref [checked in]

http://hg.mozilla.org/comm-central/rev/712e1423558d

        Attachment #487223 -
        Attachment description: Remove pref → Remove pref [checked in]

    
  
Severity: normal → S3

          You need to log in
          before you can comment on or make changes to this bug.