Opened 14 years ago

Closed 13 years ago

Last modified 12 years ago

#13433 closed (fixed)

EmailMessage mangles body text

Reported by: Leo Shklovskii Owned by: Leo Shklovskii
Component: Core (Mail) Version: 1.2-beta
Severity: Keywords: mangle_from_
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

This is a fun one. If I do the following code in Django:

from django.core.mail import EmailMessage
message = EmailMessage('blah', 'From puppies','bob@hope.com', ['bob@hope.com'])
message.send()

The message that appears on the other end has this in the body:

>From puppies

A From at the beginning of any lines in the body gets a > prepended to it.

Digging deep into Python's innards reveals that this is a somewhat esoteric protection in case you're writing out Unix mailbox files. The specific issue is here: http://docs.python.org/release/2.6.2/library/email.generator.html#email.generator.Generator and involves the mangle_from_ flag.

The hugely more likely case is that if you're using Django's EmailMessage class you're sending emails rather than writing Unix mailbox files and are running into this bug that way.

One proposed solution would be to override the __str()__ methods on django.core.mail.SafeMIMEText and django.core.mail.SafeMIMEMultipart. That method by default calls as_string() which in turn creates a Generator with mangle_from_ set to True - see email.message.Message. If the implementation in the Django classes looked something like this, it would work fine:

def __str__(self):
        from email.generator import Generator
        fp = StringIO()
        g = Generator(fp, mangle_from_ = False)
        g.flatten(self, unixfrom=self.unixfrom)
        return fp.getvalue()

Happy to do patches/test/docs as appropriate but I'd like to have a core committer take a look at it and bless the approach (or punt the bug to 1.3/wontfix/etc...)

Attachments (1)

bug13433.patch (7.6 KB ) - added by Leo Shklovskii 14 years ago.
Patch against r13040

Download all attachments as: .zip

Change History (15)

comment:1 by Chris Beaven, 14 years ago

milestone: 1.2
Triage Stage: UnreviewedDesign decision needed

We're in a feature freeze, so the 1.2 milestone is reserved for targeted features or bugs caused by 1.2 added features.

The issue seems valid (it doesn't seem like we need mangle_from_) but I'd bring up the implementation approaches on the django-dev group.

comment:2 by Russell Keith-Magee, 14 years ago

Triage Stage: Design decision neededAccepted

It's not pretty, but I really don't see much of an alternative, either. My only question is whether overriding str is the right place; given that as_string() is the affected interface, it seem like that should be the method that is fixed. However, that's just based on an initial inspection; I'm willing to be convinced that I'm wrong on that one.

comment:3 by Greg Wogan-Browne, 14 years ago

Looking at the Django email backend code, they all seem to call as_string() directly (not implicitly converting via str), so I'd agree with russellm here that as_string() would be the correct place to fix this bug.

by Leo Shklovskii, 14 years ago

Attachment: bug13433.patch added

Patch against r13040

comment:4 by Leo Shklovskii, 14 years ago

Has patch: set
Owner: changed from nobody to Leo Shklovskii
Status: newassigned

Patch attached. A few notes:

  • I've cleaned up some text & comment formatting in the doctest.
  • Some of the doctests were not running as intended since they did not use the <BLANKLINE> syntax to indicate blank lines in the intended output (added in Python 2.4, so fine for Django)

comment:5 by Leo Shklovskii, 14 years ago

milestone: 1.3

comment:6 by Ramiro Morales, 13 years ago

Patch needs improvement: set

Tests need to be converted to unit tests.

comment:7 by Leo Shklovskii, 13 years ago

@ramiro converting all of the tests to unit tests seems like a fairly high bar for a bug fix like this. What about setting this one to ready to checkin (if that's the only issue) and then opening up a new bug to convert these doctests into unit tests?

comment:8 by Russell Keith-Magee, 13 years ago

@Leo - one of the major accomplishments of Django 1.3 has been to purge doctests from our codebase. We're not going to start introducing new doctests into our codebase at this point.

comment:9 by Leo Shklovskii, 13 years ago

Sorry, I'm unclear, are you asking me to convert all of the existing doctests to unit tests? or just the new one that I wrote?

comment:10 by Greg Wogan-Browne, 13 years ago

All the existing tests have already been converted.

comment:11 by Ramiro Morales, 13 years ago

Resolution: fixed
Status: assignedclosed

In [15669]:

Fixed #13433 -- Changed default behavior of Django email message wrappers to not mangle lines starting with 'From '. Thanks Leo for the report and patch.

comment:12 by Ramiro Morales, 13 years ago

In [15670]:

[1.2.X] Fixed #13433 -- Changed default behavior of Django email message wrappers to not mangle lines starting with 'From '. Thanks Leo for the report and patch.

Backport of [15669] from trunk.

in reply to:  12 comment:13 by dobcey, 13 years ago

Just a heads up, sorry for not having a full patch and documentation for this but I just noticed.

This patch breaks in Python 2.4.

In 2.4:

from email.generator import Generator

have to be:

from email.Generator import Generator

in order to work. Ie Generator with a capital G.

The problem seems to be that all modules have been renamed according to PEP 8 standards in version 4 of the mail package in Python, as stated here: http://docs.python.org/library/email.html#package-history although it says to be compatible with Python 2.3 to 2.5.

Last edited 13 years ago by dobcey (previous) (diff)

comment:14 by Jacob, 12 years ago

milestone: 1.3

Milestone 1.3 deleted

Note: See TracTickets for help on using tickets.
Back to Top