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

NEW
Unassigned

Status

MailNews Core
Attachments
9 years ago
2 years ago

People

(Reporter: Tobias Koenig, Unassigned)

Tracking

(Blocks: 1 bug, {qawanted})

1.9.1 Branch
qawanted
Bug Flags:
wanted-thunderbird3 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [needs slightly updated patch])

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

9 years ago
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)
(Reporter)

Comment 1

9 years ago
Created attachment 337026 [details] [diff] [review]
Fixes the content-type depending on the file extension
Attachment #337026 - Flags: review?(dmose)

Comment 2

9 years ago
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.

Updated

9 years ago
Assignee: nobody → tokoe
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Comment 3

9 years ago
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

Comment 4

9 years ago
Which reminds me, there's a very related and more general patch in bug 444759. Doing it only for .pdf seems wrong.
(Reporter)

Comment 5

9 years ago
@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

Comment 6

9 years ago
http://mxr.mozilla.org/mozilla/ident?i=file_type_fn ->
http://mxr.mozilla.org/mozilla/source/mailnews/mime/src/mimemoz2.cpp#1637 ->
... so looks like here: http://mxr.mozilla.org/mozilla/source/mailnews/mime/src/mimemoz2.cpp#726

I haven't tested, but probably removing the image check would do what you want.
(Reporter)

Comment 7

9 years ago
@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?
(Reporter)

Comment 8

9 years ago
Created attachment 339937 [details] [diff] [review]
Override content-type by file name extension

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)

Updated

9 years ago
Duplicate of this bug: 455274

Comment 10

9 years ago
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

Comment 11

9 years ago
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!

Comment 12

9 years ago
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.
Created attachment 371779 [details] [diff] [review]
patch, v3

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+
Created attachment 371950 [details] [diff] [review]
patch, v4

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+

Updated

9 years ago
Whiteboard: [has patch, needs review dmose] → [has patch, needs sr bienvenu]

Comment 17

9 years ago
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 18

9 years ago
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-

Updated

8 years ago
Whiteboard: [has patch, needs sr bienvenu] → [needs updated patch]

Comment 19

8 years ago
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).
Duplicate of this bug: 549582

Updated

8 years ago
Component: General → Attachments
Product: Thunderbird → MailNews Core
QA Contact: general → attachments

Updated

6 years ago
Duplicate of this bug: 690288

Comment 23

6 years ago
Tobias or Dmose, will you continue with this patch?
Or should I try it?

Updated

6 years ago
Blocks: 66677

Comment 24

6 years ago
(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.

Comment 25

6 years ago
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

Comment 26

6 years ago
Created attachment 608787 [details]
Sample .eml with PDF attachment originally from M$Windows XP + Tb10 workstation

Comment 27

6 years ago
(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...

Comment 28

6 years ago
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

Comment 32

6 years ago
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.

Updated

6 years ago
Status: ASSIGNED → NEW
Keywords: qawanted
Version: Trunk → 1.9.1 Branch

Updated

6 years ago
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.
See Also: → bug 517796

Comment 34

2 years ago
Created attachment 8748489 [details]
sample.docx
You need to log in before you can comment on or make changes to this bug.