Code

Opened 22 months ago

Closed 11 months ago

Last modified 11 months ago

#18967 closed Bug (fixed)

django.core.mail.EmailMessage._create_mime_attachment incorrectly encodes message/rfc822 attachments in base64, breaks in Evolution and Thunderbird

Reported by: micolous Owned by: nobody
Component: Core (Mail) Version: master
Severity: Normal Keywords:
Cc: cuerty Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

Django appears to insert message/rfc822 attachments in base64 encoding. This breaks display in Novell Evolution #651197 and Mozilla Thunderbird #333880.

This behaviour is similar to some other mail clients (namely Gmail and Microsoft Outlook), but is non-standard according to RFC2046:

5.2.1. RFC822 Subtype
No encoding other than "7bit", "8bit", or "binary" is permitted for the body of a "message/rfc822" entity.

The Mozilla Thunderbird bug entry seems to indicate that with certain versions of Thunderbird, this behaviour is accepted, but this seems to change sporadically. They believe that encoding message/rfc822 attachments in this fashion is not acceptable behaviour.

Attachments (0)

Change History (11)

comment:1 Changed 22 months ago by micolous

  • Easy pickings set
  • Has patch set
  • Needs documentation set
  • Needs tests set
  • Patch needs improvement unset
  • Summary changed from django.core.mail._create_mime_attachment incorrectly encodes message/rfc822 attachments in base64, breaks in Evolution and Thunderbird to django.core.mail.EmailMessage._create_mime_attachment incorrectly encodes message/rfc822 attachments in base64, breaks in Evolution and Thunderbird

I've created a patch and pull request: https://github.com/django/django/pull/373

This patch is against Django master.

There are some changes in the behaviour of django.core.mail.EmailMessage.attach:

  • message/rfc822 attachments may be a email.message.Message object (not just a string)
  • When passing in a string attachment, Django will now automatically convert it to email.message.Message internally for email.mime.message.MIMEMessage. This means that if the message/rfc822 attachment is not well-formed, an exception may be thrown.
  • Per RFC2046, the attached message headers must be ASCII encoded. As a result SafeMIMEMessage checks headers with ASCII encoding.

This also implements SafeMIMEMessage (like the other SafeMIME* classes).

The tests currently pass, however there are no specific tests in this patch for this functionality.

Because this changes some of the behaviour of Django when attaching messages which has a chance to break existing applications, I'm unsure whether this bugfix should be backported to earlier supported versions of Django.

Last edited 22 months ago by micolous (previous) (diff)

comment:2 Changed 22 months ago by micolous

  • Needs tests unset

I've added regression tests for the bug, and fixed an issue where it was applying the transforms to all message/* mimetypes, and not just message/rfc822. I've also allowed the EmailMessage.attach method to take in Django's EmailMessage object as an argument for message/rfc822 objects.

I've updated my pull request (above) with these changes.

comment:3 Changed 21 months ago by claudep

  • Triage Stage changed from Unreviewed to Accepted

comment:4 Changed 14 months ago by micolous

I notice the patch for this bug was marked as "Accepted" 7 months ago, however the patch for the issue has not been merged. I'm confused to what is going on.

This is still an outstanding bug, as Django continues to produce non-RFC2046 s5.2.1-compliant mail, which breaks in multiple popular email clients when attempting to send message/rfc822 attachments from Django.

I've now rebased this patch against the stable 1.5 tree as at today, however this has now got GitHub very confused, as it's pulled in hundreds of commits in the last 8 months. I've since created a new pull request that fixes this:

https://github.com/django/django/pull/1202

If there is something needed from me to make this proceed please let me know. Thanks.

Last edited 14 months ago by micolous (previous) (diff)

comment:5 Changed 14 months ago by aaugustin

  • Patch needs improvement set

This ticket was marked as "Accepted" because the issue was recognized as valid.

Once someone has written a patch (with tests and docs if appropriate), someone else can review it, and mark the ticket as "Ready for Checkin" if the patch is good.

Then the patch can be merged.

A pull request is an appropriate way to provide a patch, however, it must be done against the master branch, not a stable branch.

comment:6 Changed 14 months ago by micolous

I will write some documentation and rebase this against master.

How can I target the stable versions of Django for getting this patch in future bugfix releases? This bug affects Django 1.3, 1.4 and 1.5... though probably also other releases as well.

I'm going to have to backport this patch to 1.5.x anyway, because it causes my application to fail in many of it's email-related tasks, as it is sending emails attached to emails quite frequently.

Trying to get this fixed in other email clients is an uphill battle, as it's non-RFC and has been an open bug in Thunderbird for 7 years.

Version 0, edited 14 months ago by micolous (next)

comment:7 Changed 14 months ago by micolous

  • Needs documentation unset
  • Patch needs improvement unset
  • Version changed from 1.4 to 1.5

I've written documentation for the change. I've got another pull request against django master:

https://github.com/django/django/pull/1222

As this fixes a bug in Django, it would be great if this was also pulled into Django 1.5.2. Pull request 1202 has been updated against current stable/1.5.x.

This already has unit tests which identify the buggy behaviour.

comment:8 Changed 14 months ago by lukeplant

I'm afraid that are policy with bug fixes is that this can only go into master - it won't be backported. To ensure it gets into 1.6 it would help if you can find someone to review your patch.

comment:9 Changed 14 months ago by cuerty

  • Cc cuerty added
  • Triage Stage changed from Accepted to Ready for checkin
  • Version changed from 1.5 to master

IMHO: This patch seems good.

I was able to reproduce the bug with the test provided and the patch fix it. Also the last version of the patch merges without problems with master.

comment:10 Changed 11 months ago by Ramiro Morales <cramm0@…>

  • Resolution set to fixed
  • Status changed from new to closed

In f9d1d5dc1377cb21b39452b0897e7a79a3d02844:

Fixed #18967 -- Don't base64-encode message/rfc822 attachments.

Thanks Michael Farrell for the report and his work on the fix.

comment:11 Changed 11 months ago by Andrew Godwin <andrew@…>

In 01223840f34ff2eacf1425bc133c347564fe2614:

Fixed #18967 -- Don't base64-encode message/rfc822 attachments.

Thanks Michael Farrell for the report and his work on the fix.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.