Open Bug 180798 Opened 19 years ago Updated 11 years ago
Add option to automatically unzip (extract) .zip files when they are saved as attachments
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.
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?
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".
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().
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.
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-
16 years ago
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
is this still wanted?
Severity: normal → enhancement
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.
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
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]
You need to log in before you can comment on or make changes to this bug.