Code

Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#13433 closed (fixed)

EmailMessage mangles body text

Reported by: Leo Owned by: Leo
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: UI/UX:

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 4 years ago.
Patch against r13040

Download all attachments as: .zip

Change History (15)

comment:1 Changed 4 years ago by SmileyChris

  • milestone 1.2 deleted
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Design 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 Changed 4 years ago by russellm

  • Triage Stage changed from Design decision needed to Accepted

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 Changed 4 years ago by wogan

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.

Changed 4 years ago by Leo

Patch against r13040

comment:4 Changed 4 years ago by Leo

  • Has patch set
  • Owner changed from nobody to Leo
  • Status changed from new to assigned

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 Changed 4 years ago by Leo

  • milestone set to 1.3

comment:6 Changed 3 years ago by ramiro

  • Patch needs improvement set

Tests need to be converted to unit tests.

comment:7 Changed 3 years ago by Leo

@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 Changed 3 years ago by russellm

@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 Changed 3 years ago by Leo

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 Changed 3 years ago by wogan

All the existing tests have already been converted.

comment:11 Changed 3 years ago by ramiro

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

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 follow-up: Changed 3 years ago by ramiro

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.

comment:13 in reply to: ↑ 12 Changed 3 years ago by dobcey

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 3 years ago by dobcey (previous) (diff)

comment:14 Changed 3 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

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.