EmailMessage.add_attachment() creates parts with spurious MIME-Version header.

[63aebbe0-3e30-4c50-ba49-65bac9b351bf]

BPO	25235
Nosy	@warsaw, @bitdancer, @eriker-fsc
Files	test_add_attachment_does_not_add_MIME_Version_in_attachment.patch: failing test case test_add_attachment_does_not_add_MIME_Version_in_attachment.patch: revised failing tests test_MIME_Version.patch: Actually revised patch. fix_mime_version_headers.patch fix_mime_version_headers-2.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields: ```python assignee = None closed_at = None created_at = labels = ['type-bug', 'expert-email'] title = 'EmailMessage.add_attachment() creates parts with spurious MIME-Version header.' updated_at = user = 'https://bugs.python.org/groner' ``` bugs.python.org fields: ```python activity = actor = 'eriker' assignee = 'none' closed = False closed_date = None closer = None components = ['email'] creation = creator = 'groner' dependencies = [] files = ['40576', '40577', '40578', '44520', '44644'] hgrepos = [] issue_num = 25235 keywords = ['patch'] message_count = 11.0 messages = ['251600', '251601', '275490', '275552', '275836', '275840', '275963', '276365', '277320', '277337', '384936'] nosy_count = 4.0 nosy_names = ['barry', 'r.david.murray', 'groner', 'eriker'] pr_nums = [] priority = 'normal' resolution = None stage = 'commit review' status = 'open' superseder = None type = 'behavior' url = 'https://bugs.python.org/issue25235' versions = ['Python 3.5', 'Python 3.6'] ```

[63aebbe0-3e30-4c50-ba49-65bac9b351bf]

Because MIMEPart.add_attachment() creates parts using type(self), EmailMessage.add_attachment() creates parts of type EmailMessage. This results in a MIME-Version header being added to parts where it isn't needed.

https://tools.ietf.org/html/rfc2045#section-4

[63aebbe0-3e30-4c50-ba49-65bac9b351bf]

Relatedly EmailMessage.make_mixed(), etc don't set MIME-Version on the multipart (it is only set on the part). Additional tests attached.

[bitdancer]

It turns out that solving this header problem via MIMEPart isn't really a good idea, given the rest of the API. I don't want to not use 'type', or rather I do want to use the new policy.message_factory, and I don't really want to have to have two message_factory attributes. I'm considering the possibility of only adding a MIME header in the Generator at the top level if and only if there are no parser derived headers. (This will do the right think only if you stick to the new API, since the old API adds the header indiscriminately in several places).

[bitdancer]

Attached is my proposal (absent the doc changes). I'm not happy with it, though, because it means that you don't have the MIME-Version header until you do bytes(msg) (or otherwise generate it), *and* you get a MIME-Version header if you do str(part) for a subpart. That makes debugging significantly confusing, but only with respect to whether or not you have a MIME-Version header.

I'm inclined to commit it, since this whole thing applies *only* to fully constructed messages, rather than parsed-and-manipulated messages. I don't like it, but I haven't come up with a better solution so far.

[63aebbe0-3e30-4c50-ba49-65bac9b351bf]

From https://tools.ietf.org/html/rfc2045#section-4

Note that the MIME-Version header field is required at the top level of a message. It is not required for each body part of a multipart entity. It is required for the embedded headers of a body of type "message/rfc822" or "message/partial" if and only if the embedded message is itself claimed to be MIME-conformant.

This patch looks like it avoids adding a MIME-Version header to parts with no mime metadata, but it can actually be omitted from most parts.

[63aebbe0-3e30-4c50-ba49-65bac9b351bf]

I agree that there isn't a way to know if a header should be added implicitly, until Generator is called.

A possible change to the Generator behavior is to generate the header in the serialized output, without modifying the original. I don't know that it makes debugging easier, but it would keep the Generator from having this side effect.

Maybe being explicit is better, a utility factory could be used to keep the common case simple. I don't know if such a change is reasonable at this point.

[bitdancer]

Hmm. Only doing it in the output is an interesting idea. It's a bit ugly to implement, but doable.

[bitdancer]

Here's patch that only puts the MIME-Version in the output stream and doesn't modify the message (permanently, at least). I like this better. This patch is against 3.5 (the previous one was against 3.6). Since this stuff is provisional in 3.5, I'm thinking I'll make the full change there, including deleting MIMEMessage from the docs.

[bitdancer]

Barry, would you care to render an opinion on this proposed fix?

[warsaw]

On Sep 24, 2016, at 05:06 PM, R. David Murray wrote:

Barry, would you care to render an opinion on this proposed fix?

I think the general approach is probably the best you can do. I noticed a couple of things though with RDM's v.2 patch.

First, I get test failures when applying to the 3.5 branch, specifically test_mime_version_added_to_mime_message() fails. I won't attach the failure I'm seeing unless you can't reproduce it.

Second, if I'm reading RFC 2045#section-4 correctly, I think the embedded rfc822 attachment should have a MIME-Version header, in this code:

-----snip snip-----

from email.message import EmailMessage

m = EmailMessage()
m.set_content('This is a body')

o = EmailMessage()
o.add_attachment(m)

print(o)

print(m['mime-version'])
-----snip snip

---

But instead I get:

Content-Type: multipart/mixed; boundary="===============4744209610526815348==" MIME-Version: 1.0

--===============4744209610526815348== Content-Type: message/rfc822 Content-Transfer-Encoding: 8bit Content-Disposition: attachment

MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit

This is a body

--===============4744209610526815348==--

1.0

[f03fd216-1ef4-4020-9d5c-703aa88fe301]

Duplicate of https://bugs.python.org/issue11021 but this one is more current and has a patch.