Closed Bug 453805 Opened 16 years ago Closed 6 years ago

PDF attachments with Content-Type: application/octet-stream are handled bad

Categories

(MailNews Core :: Attachments, defect)

Product:
MailNews Core
Component:
Attachments
Version:
1.9.1 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED WORKSFORME

People

(Reporter: tokoe, Unassigned)

References

(Blocks 1 open bug)

Details

(Whiteboard: [needs slightly updated patch])

Attachments

(4 files, 3 obsolete files)

patch, v4
(deleted), patch
dmosedale
: review+
Bienvenu
: superreview-
Details | Diff | Splinter Review
Sample .eml with PDF attachment originally from M$Windows XP + Tb10 workstation
(deleted), text/plain; charset="iso-8859-1"
Details
sample.docx
(deleted), application/vnd.openxmlformats-officedocument.wordprocessingml.document
Details
bug453805.patch - rebased
(deleted), patch
Details | Diff | Splinter Review
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.1) Gecko/2008071618 Iceweasel/3.0.1 (Debian-3.0.1-1) Build Identifier: Some mail applications out there (especially web mailers) send PDF attachments with content-type application/octet-stream instead of application/pdf. When you open such a mail, Thunderbird only looks at the MIME header and does not open acroread when it encounters application/octet-stream. As PDF is an often used format and there are somany broken mailers I'd propose to use the attached patch (or any adaption of it) that let Thunderbird check whether the attachment with the type octet-stream has a name that ends with '.pdf' and in that case resets the content-type to application/pdf. Reproducible: Always Steps to Reproduce: 1.Send yourself a mail with PDF attachment and content-type 'application/octet-stream' 2.Open the mail in the viewer window 3.Click on attachment Actual Results: A dialog opens where the found attachment is recognized as 'binary data' and you get asked what to do with it. Expected Results: The attachment should be recognized as PDF (from the file name) and the user be asked whether to open it with acroread (or what ever)
Attachment #337026 - Flags: review?(dmose)
Would you be willing to look through the MIME and content-type bugs that are currently open in Bugzilla (search for those two words in products MailNews Core and Thunderbird), and see how many of them can be consolidated into one patch that isn't PDF-specific? Having glanced through a number of similar bugs yesterday, I think Thunderbird already has a database of extensions, so you might be able to tap into that pretty easily to make a more general patch.
Assignee: nobody → tokoe
Status: UNCONFIRMED → NEW
Ever confirmed: true
Hej, what exactly do you mean with a 'more general' patch? Instead of hardcoding that case only for PDF, always look for the extension of an 'octet-stream' marked attachment and chaning the mime-type according to the well known extension? Ciao, Tobias
Which reminds me, there's a very related and more general patch in bug 444759. Doing it only for .pdf seems wrong.
@Magnus is the functions opts->file_type_fn() which is used in that patch always set to a valid pointer? And where can I find the implementation of that function? As far as I can see it would fix my case as well, when the explicit image check is removed from the patch, or? Shouldn't it be enough to just check whether a valid mimetype was returned by the file_type_fn method and assign it to the current mimetype variable in that case? Ciao, Tobias
@Magnus no, unfortunately it doesn't work... I tried to add code like that at other places as well (mimemsg.cpp or mimemult.cpp) but there seems to be other places where the content-type header is accessed directly via MimeHeaders_get() and the rewrite has no effect. The only way I see is to make the rewrite inside MimeHeaders_get(), however there is no MimeDisplayOptions argument available to query the filename via MimeHeaders_get_name(). Any idea how to fix that? Where is the best place to add such a rewrite check?
Attached patch Override content-type by file name extension (obsolete) (deleted) — — Splinter Review
Hej, this is a more generic patch for that problem. Instead of hardcoding the PDF case, it uses mime_file_type() from mimemoz2.cpp to find out the real content type of the mail part. The patch from bug 444759 doesn't help us here, as it is in a code path that isn't passed in our case.
Attachment #337026 - Attachment is obsolete: true
Attachment #339937 - Flags: review?(dmose)
Attachment #337026 - Flags: review?(dmose)
Marking this as wanted-tb3+, assuming the patch works I think it would resolve some fairly visible attachment opening issues.
Status: NEW → ASSIGNED
Flags: wanted-thunderbird3+
Whiteboard: [has patch, needs review dmose]
Target Milestone: --- → Thunderbird 3.0b2
Fixing this bug would be a good boost to the Thunderbird user experience. People have switched away from Thunderbird due to its inability to display images inline if they are marked as 'application/octet-stream'. See this thread from 2006: http://forums.mozillazine.org/viewtopic.php?f=31&t=394801&st=0&sk=t&sd=a The patch looks like it will fix this image issue and the PDF issue. IMHO users will be more delighted by being able to view images. Great works guys!
I just applied this patch to the Thunderbird 2.0.0.21 release and it works. Images now appear inline. Only took 5 mins to build and test the patch. And about 6 hours downloading figuring out which SDKs you need to build it, plus time spent fixing MozillaBuild to make it work on Vista64.
Agreed; sorry for my review lag. I started to poke at this some more last week, I should have reviewed fairly soon.
Attached patch patch, v3 (obsolete) (deleted) — — Splinter Review
Since this patch was generated a while ago, it no longer applied. Here's a fixed version updated to apply and compile given linkage changes.
Attachment #339937 - Attachment is obsolete: true
Attachment #371779 - Flags: review?(dmose)
Attachment #339937 - Flags: review?(dmose)
Comment on attachment 371779 [details] [diff] [review] patch, v3 This looks good. >+ // some mailer use application/octet-stream wrongly as content-type, >+ // fix it here by determine content type from name extension >+ if (contentType == nsCAutoString(APPLICATION_OCTET_STREAM)) { It's possible to avoid constructing a new string object here by taking advantage of nsCAutoString.Equals ability to take literal arguments. >+ char *name = MimeHeaders_get_name(headers, mdd->options); >+ if (name) { >+ contentType = nsCAutoString(mime_file_type(name, mdd->options->stream_closure)); nsCAutoString.Assign can be used similarly. >+ PR_Free(name); Since MimeHeaders_get_name allocates using PL_strdup, its return values want to be freed with PL_strfree. The same applies to the code added in mimemult.cpp. >+// Utility to get mime type from file name >+extern "C" char *mime_file_type (const char *filename, void *stream_closure); We're trying to move towards a better world of automatically generated documentation by using doxygen-style comments when possible. I've made the above tweaks in a new iteration of the patch which I'll upload momentarily.
Attachment #371779 - Flags: review?(dmose) → review+
Attached patch patch, v4 (deleted) — — Splinter Review
I think it's important that bienvenu do the superreview here, as he seems most likely to catch any unintended consequences this patch could have. Relevant stuff: * I haven't been able to test this meaningfully, since this bug doesn't seem to occur on Mac. * Are _both_ of the application/octet-stream override clauses actually required? * I spent some time a little while back talking to dveditz, sayre, and bz about whether there was any obvious security risk of overriding the sender-specified content type. After talking this over a bunch, we didn't see one. One use case that I asuth and I concocted last night was that of a security researcher emailing known malware around and typing it as application/octet-stream as a way of making it slightly less likely to be accidentally invoked. But this seems like the .000001% use case, so optimizing for that rather than optimizing for usability seems like a bad idea. Finally, Tobias, I'm very sorry that it's taken me so long to get this, and thanks for the patch!
Attachment #371779 - Attachment is obsolete: true
Attachment #371950 - Flags: superreview?(bienvenu)
Attachment #371950 - Flags: review+
Whiteboard: [has patch, needs review dmose] → [has patch, needs sr bienvenu]
Comment on attachment 371950 [details] [diff] [review] patch, v4 it looks like this patch will overwrite the application_octet_stream type with "" when mime_file_type doesn't find a type - that seems wrong. Is that intentional?
Comment on attachment 371950 [details] [diff] [review] patch, v4 minusing per previous comment...seems like it would be easy enough to fix.
Attachment #371950 - Flags: superreview?(bienvenu) → superreview-
Whiteboard: [has patch, needs sr bienvenu] → [needs updated patch]
Tobias: any update?
OS: Linux → All
Hardware: x86 → All
Whiteboard: [needs updated patch] → [needs slightly updated patch]
Target Milestone: Thunderbird 3.0b2 → Thunderbird 3.1a1
Same problem seen here on French forums: http://www.geckozone.org/forum/viewtopic.php?f=4&t=82035&start=0 Would be great to have a solution because it is very annoying for the user (and hardly understandable because attachment is correctly open if saved on the desktop).
Component: General → Attachments
Product: Thunderbird → MailNews Core
QA Contact: general → attachments
Tobias or Dmose, will you continue with this patch? Or should I try it?
Blocks: 66677
(In reply to :aceman from comment #23) > Tobias or Dmose, will you continue with this patch? > Or should I try it? Please Go on! Last Tobias Words: 2008-09-23 01:55:00 PDT Comment 8 and Dan Mosedale (:dmose) 2009-04-13 16:32:36 PDT Comment 16 I'm looking for a solution and I trust in Mozilla Thunderbird Project.
Can somebody attach a sample email message with this problem (wrong content-type), exported as EML file?
Assignee: tokoe → acelists
Target Milestone: Thunderbird 3.1a1 → ---
Version: unspecified → Trunk
(In reply to stephane.gaudiche from comment #26) > Created attachment 608787 [details] > Sample .eml with PDF attachment originally from M$Windows XP + Tb10 > workstation Sorry this one has been edited by me! I removed =?windows-1252?q? in the line discribing Content-Type > Content-Type: =?windows-1252?q?application/pdf; by > Content-Type: application/pdf; And then it open-able with Thunderbird (Tested under Ubuntu or Windows7) With M$ Outlook it just work. No comment. So not really a Mozilla bug! But Mozilla shouldn't relay such mistake...
I think we need a msg with octet-stream but I guess I can edit the content-type too.
Attachment #608787 - Attachment mime type: application/octet-stream → text/plain
Attachment #608787 - Attachment mime type: text/plain → text/plain; charset="iso-8859-1"
(In reply to Stephane G. from comment #27) > Sorry this one has been edited by me! I removed =?windows-1252?q? > in the line discribing Content-Type > > Content-Type: =?windows-1252?q?application/pdf; > by > > Content-Type: application/pdf; > And then it open-able with Thunderbird (Tested under Ubuntu or Windows7) > With M$ Outlook it just work. No comment. > So not really a Mozilla bug! But Mozilla shouldn't relay such mistake... Sorry but this bug is not for broken/corrupted Mime-type like =?windows-1252?q?application/pdf
Woops, too early enter... This bug is for issue with Content-Type: application/octet-stream for PDF file. For broken Content-Type:, read bug 503309 and bugs listed in dependency tree for that bug (bug 580971, bug 659355, bug 685112, bug 738284) please.
Adding application/octet-stream to bug summary to avoid confusion. Checked with valid/well-formed test mail attached to comment #27 by Stephane G., with changing to Content-Type: application/octet-stream, using Tb 11.0 on MS Win-XP, with no entry in mimeTypes.rdf and no entry in Tools/Opions/Attachments. I couldn't reproduce problem of this bug on my Win-XP. Tb asked for action(open, save, detach, delete, ...), and when "Open" is selected, "Open with" was checked and "Adobe Reader 9.5(default)" was pre-selected. To all problem reporters in this bug: Do you still see this bug in recent released Tb(Tb 11) or Tb trunk builds?
Summary: PDF attachments with content-type octet-stream are handled bad → PDF attachments with Content-Type: application/octet-stream are handled bad
Confirming what WADA says. I changed the content-type in the attached test message to application/octet-stream and imported into Thunderbird 14 (Daily). It sees an attachment in the message properly. Double clicking on it produces the dialog "You have chosen to open <filename> which is a *pdf File* (94,5KB), etc." What is wrong with that? This may have been fixed by other bug.
Status: ASSIGNED → NEW
Keywords: qawanted
Version: Trunk → 1.9.1 Branch
Assignee: acelists → nobody
(In reply to :aceman from comment #32) > Confirming what WADA says. I changed the content-type in the attached test > message to application/octet-stream and imported into Thunderbird 14 > (Daily). It sees an attachment in the message properly. Double clicking on > it produces the dialog "You have chosen to open <filename> which is a *pdf > File* (94,5KB), etc." > > What is wrong with that? > This may have been fixed by other bug. +1. pdf attachment with content-type:application/octet-stream; worksforme (tested on attachment 675317 [details] tb16/winxp) And per Comment 29, the problem which unfortunately was eliminated from attachment 608787 [details] (see Comment 27) is a different case which needs a different bug. So particular case of comment 0 is fixed. Is there anything we need to carry over from this bug, along the lines of comment 4? Otherwise this can be closed wfm.
Attached file sample.docx (deleted) —

aceman, see questions in comment 33

Flags: needinfo?(acelists)
Keywords: qawanted

So funny, application/octet-stream works in a local mailbox, for sure. But on IMAP, that's a different story and it DOES NOT WORK, see bug 1418444.

Gene, you were experimenting in bug 1418444. Please take a look at the patch here, they look OK to me, we just need to fix the issue from comment #17.

Flags: needinfo?(acelists) → needinfo?(gds)

I have a sample email that has 2 pdfs attached, one with application/octet-stream and the other with application/pdf. Both seem to open OK with no prompt for "which app do you want to open this file with?". Opens right up into a linux pdf app called "okular".

This is with an imap folder with no offline storage (mem cache only) and with and without attachments set inline (not that it would matter with a pdf since it can't be inline AFAIK).

Comment 33 above says the same thing about this bug, i.e., wfm.

Am I missing something when you say "application/octet-stream works in local but not IMAP mailboxes"? What do you mean by "works"?

However, I do still see the problem in bug 1418444 and maybe the attached patch above will fix it. Maybe that's what you are referring to. I will check that now.

Flags: needinfo?(gds)

OK, I'm confused then. Bug 1418444 is about saving. Well, let's see whether we can get that fixed.

I tried the patch for this bug (attachment 371950 [details] [diff] [review]) on the bug 1418444 problems and it didn't help. It also seems like there is an error in the patch in that the check for the string "application/octet-string" uses PL_strcasecmp() and it should use !PL_strcasecmp(). I tried it with "!" and it still didn't make a difference.

I will now check again the fixes I had proposed in Bug 1418444.

Attached patch bug453805.patch - rebased (deleted) — — Splinter Review

I can confirm that this patch doesn't help for bug 1418444. If ever you want to revive it for other purposes, please note comment #17.

WFM as per comment #33.

Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: