---- Reported by rschroll@gmail.com 2013-08-03 16:10:00 -0700 ----

Original Redmine bug id: 7299
Original URL: http://redmine.yorba.org/issues/7299
Searchable id: yorba-bug-7299
Original author: Robert Schroll
Original description:

This is an issue on the feature/attachments branch.

I got an email that has an attached image that was included in the HTML body.
This worked, but the recently created inline attachments feature also
triggered and displayed the image at the end of the message as well.

The problem, I suspect, is that the attached image didn't declare its content
disposition. RFC822.Message.construct_body_from_mime_parts() treats such parts
as inline. Here are its headers:

    
    Content-Type: image/jpeg; name="Seminario Adeline Pons, 6 agosto 2013.jpg" 
    Content-Transfer-Encoding: base64
    Content-ID: <ii_1403f4841da1e6d3>
    X-Attachment-Id: ii_1403f4841da1e6d3

  
Perhaps it would be better to require explicit designation as inline, and
treat unspecified dispositions as attached.

Related issues:
related to geary - 7300: Text parts shown as attachments (Fixed)



---- Additional Comments From geary-maint@gnome.bugs 2013-09-09 15:53:00 -0700 ----

### History

####

#1

Updated by Jim Nelson 4 months ago

  * **Priority** changed from _Normal_ to _High_
  * **Target version** set to _0.4.0_

####

#2

Updated by Jim Nelson 3 months ago

  * **Subject** changed from _Inline image included twice_ to _Inline image included twice (Unspecified Content-Disposition problems)_

Another bug has been reported that's similar: the user received an email with
a text file attached with no Content-Disposition. Geary does not display this
inline or as an attachment. (He's installed from our Daily PPA, so the recent
attachment work has not corrected this problem.)

[This is interesting](https://tools.ietf.org/html/rfc2183#section-2):

    
    
    Content-Disposition is an optional header field. In its absence, the MUA may use whatever presentation method it deems suitable.
    

I was hoping #7300 would fix this for him, but it didn't. I suspect we have to
revise our attachment code to deal with disposition-missing problems on a
case-by-case basis, examining the MIME type to determine how to handle the
attachment when disposition is not specified.

I believe in the code today there are methods that return a nullable enum when
Content-Disposition is unavailable. My suggestion is to remove the nullable
and add a new ContentDisposition value, NOT_SPECIFIED. Code that examines the
ContentDisposition enum should always account for all three values (my
suggestion: switch-case statements).

The text file bug is reported at https://bugs.launchpad.net/geary/+bug/1205320

####

#3

Updated by Robert Schroll 3 months ago

The text attachment one is going to be tricky. How do we tell a text
attachment with no Content-Disposition from a body part with no Content-
Disposition? Did this Mime part have a filename specified? That's something
that the body parts probably won't have.

####

#4

Updated by Jim Nelson 3 months ago

It did have a filename. Here's the MIME header for that section of the

email:

Content-Type: application/octet-stream;

name="91001178_2472013153756.txt"

Content-Transfer-Encoding: base64

Interesting that the Content-Type is octet-stream; that's a clue as

well.

####

#5

Updated by Avi Levy 3 months ago

  * **Assignee** set to _Avi Levy_

####

#6

Updated by Avi Levy 3 months ago

I think that adding a NOT_SPECIFIED value to the Disposition enum will require
a database upgrade (so that old emails will be treated correctly). Is that OK
this late in the release cycle?

Also, I am having trouble finding test cases. Does anyone have an example
email that causes this bug?

####

#7

Updated by Jim Nelson 3 months ago

Avi Levy wrote:

> I think that adding a NOT_SPECIFIED value to the Disposition enum will
require a database upgrade (so that old emails will be treated correctly). Is
that OK this late in the release cycle?

That's fine. We have time.

> Also, I am having trouble finding test cases. Does anyone have an example
email that causes this bug?

I don't have such an email. Robert, if you forward the message, is the problem
preserved?

Although it's a bit of work, Avi, you might consider creating a test email by
hand. I would take the source of an email with an inline image, save it to
disk, then hand edit it. Remove the additional headers added by MTAs
(generally everything above the "Date:" header, which is by convention the
first when composing a message) and the Message-ID/In-Reply-To/References
headers (for sanity), and remove the Content-Disposition from the body. Use
geary-console to login to your email server and type these commands:

    
    
    > gmail
    > login <username> <password>
    > append Folder /home/avi/email.txt
    

Although it's a bit of work, this gives you a lot of control over what's being
passed into Geary and how it's processed.

####

#8

Updated by Robert Schroll 3 months ago

  * **File** HOY -_ Ciclo de Seminarios SMAT-C 2013, Dra. Adeline Pons, 6 Agosto 2013.eml added

Here's an email that causes problems. I've Xed out some others' email
addresses, but that shouldn't change anything.

####

#9

Updated by Avi Levy 3 months ago

  * **File** 7299v1.patch added

I've spent a lot of time scratching my head on this one, and I finally know
what's going on. Since I'm new to MIME, I still get confused when I try to
figure out where each MIME part is intended to end up. In this case, the
original code for doing image replacement was written with a specific use case
in mind: so that photos sent through Apple Mail would be shown correctly. The
mental model is that sequential inline parts should be "stitched" together
into a message.

Things are completely different here (i.e. Robert's example email); we would
like to realize that the image part should be filtered out of the message,
instead of stitched into it. This case was previously handled with an explicit
disposition check for an attachment. Of course, this bug is about doing the
right thing even when there is no disposition to guide us.

"Great! So it should be as simple as changing the disposition == attachment
check into disposition != inline", I thought. So I tried this within minutes
of hearing about the bug, and suddenly lots of emails ended up really, really
broken. So I assumed I misunderstood the situation, and spent a long long time
investigating and fixing other things that I thought were the cause of this
bug.

Unfortunately, the code inside `construct_body_from_mime_parts()` conceals
some subtleties. First, we handle inline text parts, then we handle inline
images. So I made the assumption that "inline" means the same thing for both,
which I check before processing either.

Now I know that "inline" means two different things. For text parts, "inline"
means "disposition != attachment". But for images, "inline" should mean
"disposition == inline". In other words, our policy for handling text parts
with unspecified disposition is not the same as our policy for handling image
parts with unspecified disposition.

I went ahead and made a minimal patch that illustrates what I'm saying, and as
a consequence seems to fix this bug. Just for simplicity, I didn't switch over
to using a Disposition.NOT_SPECIFIED value (instead of null).

####

#10

Updated by Avi Levy 3 months ago

  * **File** 7299v2.patch added

I'm sorry, disregard that last patch. Had a typo.

####

#11

Updated by Jim Nelson 3 months ago

  * **Status** changed from _Open_ to _Review_

Well, I can tell you really dug into this one and I think you figured out the
ins and outs of the issues here. I just tried it out on an email that exhibits
this problem and it seems to fix it right away. Checking at some other emails
with attachments of both flavors, it seems to work just fine. Good job!

Are you comfortable with this patch as-is? Or would you like to work further
on it? The only thing I'd ask for the patch as it stands is a comment
explaining why the method bails out at that moment.

Great work!

####

#12

Updated by Avi Levy 3 months ago

  * **File** 7299v3.patch added

Thanks for the positive feedback. I am comfortable with the patch because it
seems to close this ticket. However, I think what's going on behind the scenes
is a bit convoluted and could stand to be improved. In any case, that would be
a different task altogether, and probably has low priority right now.

I added comments.

####

#13

Updated by Avi Levy 2 months ago

  * **Status** changed from _Review_ to _Fixed_

Applied in changeset 6cdba3a77ff2dcb244a91a8c333fb43559542512.

####

#14

Updated by Jim Nelson 2 months ago

I agree, it's something we need to return to and clean up. I'm happy to get
this in, however, so we'll take this now.



--- Bug imported by chaz@yorba.org 2013-11-21 20:21 UTC  ---

This bug was previously known as _bug_ 7299 at http://redmine.yorba.org/show_bug.cgi?id=7299
Imported an attachment (id=260735)
Imported an attachment (id=260736)
Imported an attachment (id=260737)
Imported an attachment (id=260738)

Unknown milestone "unknown in product geary. 
   Setting to default milestone for this product, "---".
Setting qa contact to the default for this product.
   This bug either had no qa contact or an invalid one.