5.37 KB, patch
|Details | Diff | Splinter Review|
Fix full path bug (includes previous patch, addresses security concern and incompatabilities with previous Linux browsers)
12.96 KB, patch
|Details | Diff | Splinter Review|
From Bugzilla Helper: User-Agent: Mozilla/4.77 [en] (X11; U; Linux 2.4.2-SGI_XFS_1.0 i686) BuildID: 2001052706 When I upload a file, it's some bytes longer afterwards, and I have "ntent-Transfer-Encoding: Binary" prepended at the beginning of the file. Reproducible: Always Steps to Reproduce: just use a php based website to upload a file (using enctype="multipart/form-data") and look at the uploaded file.
To reproduce, do this: - go to http://mail.domains.com - use "demo" as the username and password - once logged in, click on COMPOSE - attach a file - send the email to yourself - try to open the attachment on the email upon reception.
Assignee: rods → pollmann
Status: UNCONFIRMED → NEW
Ever confirmed: true
There is another (probably linked) side effect; when a FUL PATH is specified as the attachment source file, this is also sent as the attachment filename, preventing you from opening the attachment in your mail program. To see a quick exemple, look into the "sent-mail" folder on the demo account and try to open the mail with "FULL PATH" as a subject. I tried opening this email with Netscape 4.76 and it shows the same behavior.
Target Milestone: --- → mozilla0.9.2
The corruption / header data that bleeds into the document is because the server side makes incorrect assumptions about the number of headers sent for each part of a multipart form. In short, this is a server-side bug and should be corrected on the server side. (For reference, the Content-Transfer-Encoding header is recommended in the HTML spec here: http://www.w3.org/TR/html4/interact/forms.html#h-220.127.116.11 and headers are not limited to a fixed number in the mime spec. This problem became apparent as a result of the checkin for bug 58189. The server is assuming there are exactly two headers, followed by two bytes (assuming those are CR and LF), then the data. Since we checked in the patch on bug 58189, we have three headers, so the two bytes that are ignored are the C and o of "Content-Transfer-Encoding: Binary" instead of the expected CR and LF. I will look into the full-path issue...
Status: NEW → ASSIGNED
This is a known problem with the PHP multipart/form-data parser that is mentioned in the comments for bug 44064. PHP's broken parser expects the last header to be Content-Type... anything after it it treats as data. (To give you an idea of how broken the parser is, PHP will also fail if there's more or less whitespace around tokens, and will fail if some values are not in double-quotes). Basically, the file main/rfc1867.c in the PHP source code needs to be rewritten. PHP is still popular and we can't ignore the existing parsers out there, so the solution is to make sure Content-Type is last (and that the whitespace and token-quoting usage follow what PHP 4.0.4pl1 expect) As bug 44064 mentions, I've tested MS IIS/ASP, Perl's CGI.pm, and other popular multipart/form-data server parsers-- PHP is the only (popular) server-side that seems to be inflexible regarding the header order and whitespace/quote usage. Later patches for bug 44064, which address Content-Transfer-Encoding, address this issue. As for the full-path issue, this is because someone (rods@?) removed the nsFormFrame::GetFileNameWithinPath call/func from nsFormFrame.cpp. I assume they did this because it makes non-crossplatform/I18N unsafe assumptions in its current form. This also is addressed in the patches in bug 44064. (Made GetFileNameWithinPath grok Unicode and Mac dir seps correctly)
FWIW, I removed GetFileNameWithinPath for bug 59408, to make us compatible with IE and Nav 4.x - if we are now incompatible, we should determine what cases cause IE/Nav 4.x to send the path, and which do not...
*** Bug 85577 has been marked as a duplicate of this bug. ***
See fix on bug 44464
Whiteboard: fix in hand
After some investigation, the problem with uploading a file and sending it's full path appears to be Linux specific. (Have not tested Mac) Nav 4.x and IE 5.x both send the full path on Windows, and sending the full path with Moz/Netscape 6.x is handled properly on the server side. I noticed that when I attached an image file on Windows, and sent it to another account, even then I could not see the image. The problem turns out to be the corruption due to PHP, so should be fixed by the patch on 44464. I'll update the patch there to make only Linux truncate the file path to a leaf file name.
On second thought, reviewing bug 59408, it should probably have just been marked WONTFIX. The security concerns raised there seem like they could be valid, and I have not seen any bugs due to our differences from IE and Nav in the past (not checked in until 11-April). Will update that one...
Since this is fixed by Adrian's patch on bug 44464, marking DUP to keep things all under one roof... *** This bug has been marked as a duplicate of 44464 ***
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → DUPLICATE
Status: RESOLVED → VERIFIED
*** Bug 83442 has been marked as a duplicate of this bug. ***
Sorry, on second thought, marking this as a duplicate confuses things more than it helps. I'll split out the section of the patch that fixes this bug and post it here, since the rest of the patch likely won't be able to be checked in.
Status: VERIFIED → REOPENED
Resolution: DUPLICATE → ---
Created attachment 38891 [details] [diff] [review] Fix corruption (turn off ContentTransferEncoding header by default)
This first patch make generation of the Content-Transfer-Encoding header (fix for bug 58189) controllable by a pref, and default to not sending it, as there are known problems with some web sites. It is unfortunate that we must do this to create a browser with an overall better user experience, but for now, this is the case. It also moves the Content-Transfer-Encoding header up above the Content-Disposition and Content-Type headers, which fixes compatability with the broken PHP form upload parser.
Status: REOPENED → ASSIGNED
Created attachment 38892 [details] [diff] [review] Fix full path bug (includes previous patch, addresses security concern and incompatabilities with previous Linux browsers)
This second patch includes the first patch, but also backs out bug 59408 so that we will no longer send the full path of files that are being uploaded to the server. This fixes the problem with linux on the mail.domains.com testcase, and addresses potential security issues with sending the full path. It also includes some i18n work done by Adrian to make GetFileNameWithinPath work for Unicode file names. Both patches were work done by Adrian Havill for bug 44464, but slightly simplified and split out from that patch so that they can be checked in sooner (as the patch there is now out of date).
CC'ing people who might be interested in reviewing this code. Adrian, Scott, can you r/sr the second patch (which includes both the full path and header fixes)? Thanks!
Whiteboard: fix in hand → fix in hand, need r/sr/a
Pollman: your patch looks good to me. I'm wondering whether sending the entire path is a good idea (security concerns about revealing the path?) Will play with IE to see whether it sends the full path or not.
IE does send the entire path on Windows, as does Netscape Communicator 4.x. We traditionally have not (did not until the 0.9.1 milestone build). I checked in a patch that 'fixed' this incompatability and checked it in without thinking to get a security review. However, my fix caused problems with server side scripts that did *not* expect the full path from UN*X systems (Communicator 4.x never sent the full path on UN*X). So, since there were never any problems reported all the way up to 0.9.1 with just sending the leaf file name, my suggestion is that we revert my 'fix' (along with the changes you made to internationalize GetFileNameWithinPath), and use the second patch above. This will cause us to again, as before, only send the leaf file name and not the full path.
Adrian, thanks for the call. :) Adrian said (essentially) "I've looked at the patch and it looks good to me" which I think qualifies as firstname.lastname@example.org for code Adrian wrote and email@example.com for how I split it apart.
Whiteboard: fix in hand, need r/sr/a → fix in hand, need approval
Fix checked in. To verify: 1) Corruption fix Go to http://geocities.yahoo.com/filemanager/upload Log in with: login: testcase83442, password: testcase Try to upload an image file. You should not get an error message "Invalid file" 2) Full path fix On a Linux or other UN*X box, go to http://mail.domains.com Log in with: login: demo, password: demo Click on Compose Attach a file (an image file, for example) Send the message (to yourself or whoever) Now go to the Sent folder, and find the message you sent. Click on the Attachment link. You should be able to view the image. Thanks Adrian for analyzing the problem, and coming up with the fix for both of these problems!
*** Bug 85742 has been marked as a duplicate of this bug. ***
Whiteboard: fix in hand, need approval → fix in hand, need approval,PDT+
Oops, forgot to mark this FIXED. See my previous comment for verification instructions.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago → 17 years ago
Resolution: --- → FIXED
Verifying fixed on build 2001-06-21-04-trunk windows 98 and build 2001-06-20-21-trunk linux red hat 6.2
Status: RESOLVED → VERIFIED
*** Bug 93860 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.