Opened 7 months ago

Closed 7 days ago

#35033 closed Bug (fixed)

EmailMessage repeats header if provided via the headers kwargs

Reported by: Aalekh Patel Owned by: Mike Edmunds
Component: Core (Mail) Version: dev
Severity: Normal Keywords: email, headers, compat32
Cc: Adam Johnson, Florian Apolloner, David Wobrock, Mike Edmunds 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 (last modified by Aalekh Patel)

If you create an EmailMessage instance with a "To" key in the headers= kwarg, it attaches the To header to the email two times, violating RFC 5322#3.6.

My suspicion is that it attaches it the first time from extra_headers in self._set_list_header_if_not_empty(msg, 'To', self.to) at django.core.mail.message:266 and the second time again from extra_headers at django.core.mail.message:282

        message = EmailMessage(
            subject="test subject",
            body="test body",
            from_email="from@example.com",
            to=["guy1@example.com"],
            headers={
                "To": ", ".join(["guy1@example.com", "guy2@example.com", "guy3@example.com"]),
            },
        )

For example, here is a Python 3.9.18 shell output for the EmailMessage above that shows the To header appears twice.

>>> from django.core.mail import EmailMessage

>>> message = EmailMessage(subject="test subject", body="test body", from_email="from@example.com",to=["guy1@example.com"], headers={"To": ", ".join(["guy1@example.com", "guy2@example.com", "guy3@example.com"])})

>>> print(list(message.message().raw_items()))
[('Content-Type', 'text/plain; charset="utf-8"'), ('MIME-Version', '1.0'), ('Content-Transfer-Encoding', '7bit'), ('Subject', 'test subject'), ('From', 'from@example.com'), ('To', 'guy1@example.com, guy2@example.com, guy3@example.com'), ('Date', 'Wed, 13 Dec 2023 15:59:31 -0000'), ('Message-ID', '<170248317136.759.5778419642073676754@036d358ca984>'), ('To', 'guy1@example.com, guy2@example.com, guy3@example.com')]

I've provided a patch for this here: django/django#17606

Change History (37)

comment:1 by Aalekh Patel, 7 months ago

Has patch: set

comment:2 by Aalekh Patel, 7 months ago

Easy pickings: set
Needs tests: set

comment:3 by Aalekh Patel, 7 months ago

Component: UncategorizedCore (Mail)
Description: modified (diff)

comment:4 by Natalia Bidart, 7 months ago

Cc: Adam Johnson added
Easy pickings: unset
Patch needs improvement: set
Version: 3.2dev

Reproduced, and accepting based on the RFC which states:

   +----------------+--------+------------+----------------------------+
   | Field          | Min    | Max number | Notes                      |
   |                | number |            |                            |
   +----------------+--------+------------+----------------------------+
   | to             | 0      | 1          |                            |

This is related to #9233, and I would encourage for the solution to this ticket to cover for all those headers that should provide at most 1 occurrence.

comment:5 by Natalia Bidart, 7 months ago

Triage Stage: UnreviewedAccepted

comment:6 by Natalia Bidart, 7 months ago

Summary: EmailMessage repeats "To" header if provided via the headers kwargsEmailMessage repeats header if provided via the headers kwargs

comment:7 by Adam Johnson, 7 months ago

Cc: Adam Johnson removed

Please don’t CC me on tickets I have no relation to.

comment:8 by Adam Johnson, 7 months ago

Cc: Adam Johnson added

Sorry, I thought the CC was done by OP. It happens sometimes that people just ping me because they’ve seen my blog or something. Why did you add me Natalia? I don’t remember working on emails 😅

comment:9 by Natalia Bidart, 7 months ago

Hey Adam, I should have been more explicit, my bad! I CC'd you because I read your comment in ticket:32907#comment:1 and it seemed that you were interested/knowledgeable in the topic. Sorry if that was hasty!

comment:10 by Florian Apolloner, 7 months ago

Should we override with the data in headers or should be throw an exception? As the behavior is broken now an exception wouldn't be the worst thing and feels more correct (ie use the existing API to set to because that might do other things under the hood etc…)

comment:11 by Florian Apolloner, 7 months ago

Cc: Florian Apolloner added

comment:12 by Adam Johnson, 7 months ago

I agree that an exception sounds more correct, probably a ValueError, for any keys in headers that correspond to the explicit arguments like “to”.

Natalia - no worries, I just saw this message after a number of other spammy ones 😅

comment:13 by Mariusz Felisiak, 7 months ago

Has patch: unset
Needs tests: unset
Patch needs improvement: unset

comment:14 by Salvo Polizzi, 7 months ago

comment:15 by Salvo Polizzi, 7 months ago

Has patch: set

comment:16 by David Wobrock, 7 months ago

Cc: David Wobrock added
Needs tests: set
Owner: changed from nobody to Salvo Polizzi
Patch needs improvement: set
Status: newassigned
Version 0, edited 7 months ago by David Wobrock (next)

in reply to:  16 comment:17 by Salvo Polizzi, 7 months ago

Replying to David Wobrock:

This is the PR you meant https://github.com/django/django/pull/17642, right? :)

Yes, it is

comment:18 by Salvo Polizzi, 7 months ago

Needs tests: unset
Patch needs improvement: unset

comment:19 by Florian Apolloner, 7 months ago

Needs tests: set
Patch needs improvement: set

I do not see any tests in the PR, just because no tests fail after changes doesn't mean there are no tests required ;)

comment:20 by Florian Apolloner, 7 months ago

When working on the patch, please consider the history of the file: https://github.com/django/django/commit/5e75678c8b was added to ensure that to in extra_headers takes precedence and should have prevented a duplicate to header. Apparently this got broken in https://github.com/django/django/commit/da82939e5a31dea21a4f4d5085dfcd449fcbed3a

Whatever the final solution is (raising an error if possible is preferred -- I fear it is not), existing tests and usecases shouldn't break.

in reply to:  19 comment:21 by Salvo Polizzi, 7 months ago

Replying to Florian Apolloner:

I do not see any tests in the PR, just because no tests fail after changes doesn't mean there are no tests required ;)

I'm sorry, I will provide tests. Thanks for giving me some feedback

comment:22 by Salvo Polizzi, 7 months ago

Needs tests: unset
Patch needs improvement: unset

comment:23 by Mariusz Felisiak, 7 months ago

Patch needs improvement: set

comment:24 by Salvo Polizzi, 7 months ago

Patch needs improvement: unset

comment:25 by Mariusz Felisiak, 7 months ago

Patch needs improvement: set

in reply to:  25 comment:26 by Salvo Polizzi, 7 months ago

Replying to Mariusz Felisiak:
Hi Mariusz, what do i need to fix in the PR? I'm asking you because I haven't seen any reviews in the PR anymore.

comment:27 by Mariusz Felisiak, 7 months ago

Sorry, I forgot to click "Submit the review".

comment:28 by Salvo Polizzi, 7 months ago

I left some comments in the new PR: https://github.com/django/django/pull/17674

comment:29 by Salvo Polizzi, 6 months ago

Patch needs improvement: unset

comment:30 by Mariusz Felisiak, 6 months ago

Patch needs improvement: set

We need to find answers before this PR will be reviewable again. We cannot review PR if we are not sure what we want to achieve. I don't see any discussion on the forum or the mailing list, or any answers for my questions.

comment:32 by Mike Edmunds, 4 weeks ago

Cc: Mike Edmunds added

Note: #9214 added special handling for supplying from_email=... with a different headers={"From": ...}: the header value is displayed in the message, the from_email value is used as envelope-from/return-path.

I can't find the reference now, but I believe using to=... with a different headers={"To": ...} was added around the same time and has a similar purpose: specifying the recipient separately from the displayed recipient. (This is sometimes used for distribution lists, where the list name is displayed in the header To field. It's also used for spam.)

In both cases, the headers value needs to override the property value in the generated message header. (Not create an additional header.)

It seems like there might also be missing tests for these special cases?

[django-anymail maintainer here; a few years back, we got a specific request to match the Django SMTPBackend's handling of these headers.]

comment:33 by Mike Edmunds, 3 weeks ago

Keywords: compat32 added

Ah, the forum mentions the ticket I couldn't find: #17444 allowed different to=... and headers={"To": ...}

I think this got broken when EmailMessage._set_list_header_if_not_empty() was added. And cc and reply_to are likely broken in the same way (based on reading the code).

The problem is that Python's email.message.Message behaves as a multi-value dict when assigning to header keys, but returns only one of the values when reading from keys. The current logic in _set_list_header_if_not_empty() assumes it works like a regular dict, and the tests from #17444 were insufficient to catch that mistake.

The minimal and safe fix is something like:

  1. First make the existing tests fail on current buggy behavior: update test_to_header() to use get_all("To") and verify there's only the one correct value in the list. Same thing in test_reply_to_header(). Wouldn't hurt to update some other nearby tests in the same way, and add a test_cc_header().
  1. Then fix this line in _set_list_header_if_not_empty() so it only assigns to the header if there's not already a header there. (Indent it one level so it only runs in the except clause.)

I would suggest we not try to enforce ​RFC 5322-3.6 email header counting rules in Django. If/when Django moves to Python's modern email API, those (and many other email header restrictions) will be enforced by Python.

comment:34 by Mike Edmunds, 3 weeks ago

Easy pickings: set

(Also, that minimal and safe fix should be easy pickings)

comment:35 by Mike Edmunds, 3 weeks ago

Patch needs improvement: unset

The problem originally reported in this ticket was a regression introduced in #28912 PR #9454.

I've added a new patch PR #18325 which addresses that specific regression. (Without attempting to add some of the other enhancements and email RFC enforcement contemplated by other discussion here.)

comment:36 by Sarah Boyce, 8 days ago

Owner: changed from Salvo Polizzi to Mike Edmunds
Triage Stage: AcceptedReady for checkin

comment:37 by Sarah Boyce <42296566+sarahboyce@…>, 7 days ago

Resolution: fixed
Status: assignedclosed

In b909853:

Fixed #35033, Refs #28912 -- Fixed repeated headers in EmailMessage.

Fixed a regression which would cause multiple To, Cc, and
Reply-To headers in the result of EmailMessage.message() if
values were supplied for both to/cc/reply_to and the
corresponding extra_headers fields.

Updated related tests to check the generated message() has
exactly one of each expected header using get_all().

Regression in b03d5002955256c4b3ed7cfae5150eb79c0eb97e.

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