Re: [Django] #35033: EmailMessage repeats header if provided via the headers kwargs
Django
--------------------------------+------------------------------------
Reporter: Aalekh Patel | Owner: nobody
Type: Bug | Status: new
Component: Core (Mail) | Version: dev
Severity: Normal | Resolution:
Keywords: email, headers | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
--------------------------------+------------------------------------
Changes (by Adam Johnson):
* cc: Adam Johnson (removed)
Comment:
Please don’t CC me on tickets I have no relation to.
--
Ticket URL: <https://code.djangoproject.com/ticket/35033#comment:7>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
--------------------------------+------------------------------------
Reporter: Aalekh Patel | Owner: nobody
Type: Bug | Status: new
Component: Core (Mail) | Version: dev
Severity: Normal | Resolution:
Keywords: email, headers | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
--------------------------------+------------------------------------
Changes (by Adam Johnson):
* cc: Adam Johnson (added)
Comment:
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 😅
--
Ticket URL: <https://code.djangoproject.com/ticket/35033#comment:8>
Django
--------------------------------+------------------------------------
Reporter: Aalekh Patel | Owner: nobody
Type: Bug | Status: new
Component: Core (Mail) | Version: dev
Severity: Normal | Resolution:
Keywords: email, headers | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
--------------------------------+------------------------------------
Comment (by Natalia Bidart):
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!
--
Ticket URL: <https://code.djangoproject.com/ticket/35033#comment:9>
Django
--------------------------------+------------------------------------
Reporter: Aalekh Patel | Owner: nobody
Type: Bug | Status: new
Component: Core (Mail) | Version: dev
Severity: Normal | Resolution:
Keywords: email, headers | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
--------------------------------+------------------------------------
Comment (by Florian Apolloner):
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…)
--
Ticket URL: <https://code.djangoproject.com/ticket/35033#comment:10>
Django
--------------------------------+------------------------------------
Reporter: Aalekh Patel | Owner: nobody
Type: Bug | Status: new
Component: Core (Mail) | Version: dev
Severity: Normal | Resolution:
Keywords: email, headers | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
--------------------------------+------------------------------------
* cc: Florian Apolloner (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/35033#comment:11>
Django
--------------------------------+------------------------------------
Reporter: Aalekh Patel | Owner: nobody
Type: Bug | Status: new
Component: Core (Mail) | Version: dev
Severity: Normal | Resolution:
Keywords: email, headers | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
--------------------------------+------------------------------------
Comment (by Adam Johnson):
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 😅
--
Ticket URL: <https://code.djangoproject.com/ticket/35033#comment:12>
Django
--------------------------------+------------------------------------
Reporter: Aalekh Patel | Owner: nobody
Type: Bug | Status: new
Component: Core (Mail) | Version: dev
Severity: Normal | Resolution:
Keywords: email, headers | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------+------------------------------------
* needs_better_patch: 1 => 0
* has_patch: 1 => 0
* needs_tests: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/35033#comment:13>
Django
--------------------------------+------------------------------------
Reporter: Aalekh Patel | Owner: nobody
Type: Bug | Status: new
Component: Core (Mail) | Version: dev
Severity: Normal | Resolution:
Keywords: email, headers | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------+------------------------------------
Comment (by Salvo Polizzi):
Hi, i made a PR for this bug:
https://github.com/django/django/pull/17606/files
--
Ticket URL: <https://code.djangoproject.com/ticket/35033#comment:14>
Django
--------------------------------+------------------------------------
Reporter: Aalekh Patel | Owner: nobody
Type: Bug | Status: new
Component: Core (Mail) | Version: dev
Severity: Normal | Resolution:
Keywords: email, headers | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------+------------------------------------
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/35033#comment:15>
Django
Reporter: Aalekh Patel | Owner: Salvo Polizzi
Type: Bug | Status: assigned
Component: Core (Mail) | Version: dev
Severity: Normal | Resolution:
Keywords: email, headers | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Changes (by David Wobrock):
* cc: David Wobrock (added)
* needs_better_patch: 0 => 1
* status: new => assigned
* owner: nobody => Salvo Polizzi
* needs_tests: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/35033#comment:16>
Django
--------------------------------+-----------------------------------------
Reporter: Aalekh Patel | Owner: Salvo Polizzi
Type: Bug | Status: assigned
Component: Core (Mail) | Version: dev
Severity: Normal | Resolution:
Keywords: email, headers | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
--------------------------------+-----------------------------------------
Comment (by Salvo Polizzi):
Replying to [comment:16 David Wobrock]:
> This is the PR you meant https://github.com/django/django/pull/17642,
right? :)
Yes, it is
--
Ticket URL: <https://code.djangoproject.com/ticket/35033#comment:17>
Django
--------------------------------+-----------------------------------------
Reporter: Aalekh Patel | Owner: Salvo Polizzi
Type: Bug | Status: assigned
Component: Core (Mail) | Version: dev
Severity: Normal | Resolution:
Keywords: email, headers | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
--------------------------------+-----------------------------------------
* needs_better_patch: 1 => 0
* needs_tests: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/35033#comment:18>
Django
--------------------------------+-----------------------------------------
Reporter: Aalekh Patel | Owner: Salvo Polizzi
Type: Bug | Status: assigned
Component: Core (Mail) | Version: dev
Severity: Normal | Resolution:
Keywords: email, headers | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
--------------------------------+-----------------------------------------
* needs_better_patch: 0 => 1
* needs_tests: 0 => 1
Comment:
I do not see any tests in the PR, just because no tests fail after changes
doesn't mean there are no tests required ;)
--
Ticket URL: <https://code.djangoproject.com/ticket/35033#comment:19>
Django
--------------------------------+-----------------------------------------
Reporter: Aalekh Patel | Owner: Salvo Polizzi
Type: Bug | Status: assigned
Component: Core (Mail) | Version: dev
Severity: Normal | Resolution:
Keywords: email, headers | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
--------------------------------+-----------------------------------------
Comment (by Florian Apolloner):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/35033#comment:20>
Django
--------------------------------+-----------------------------------------
Reporter: Aalekh Patel | Owner: Salvo Polizzi
Type: Bug | Status: assigned
Component: Core (Mail) | Version: dev
Severity: Normal | Resolution:
Keywords: email, headers | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
--------------------------------+-----------------------------------------
Comment (by Salvo Polizzi):
Replying to [comment:19 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
--
Ticket URL: <https://code.djangoproject.com/ticket/35033#comment:21>
Django
--------------------------------+-----------------------------------------
Reporter: Aalekh Patel | Owner: Salvo Polizzi
Type: Bug | Status: assigned
Component: Core (Mail) | Version: dev
Severity: Normal | Resolution:
Keywords: email, headers | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
--------------------------------+-----------------------------------------
* needs_better_patch: 1 => 0
* needs_tests: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/35033#comment:22>
Django
--------------------------------+-----------------------------------------
Reporter: Aalekh Patel | Owner: Salvo Polizzi
Type: Bug | Status: assigned
Component: Core (Mail) | Version: dev
Severity: Normal | Resolution:
Keywords: email, headers | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------+-----------------------------------------
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/35033#comment:23>
Django
--------------------------------+-----------------------------------------
Reporter: Aalekh Patel | Owner: Salvo Polizzi
Type: Bug | Status: assigned
Component: Core (Mail) | Version: dev
Severity: Normal | Resolution:
Keywords: email, headers | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------+-----------------------------------------
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/35033#comment:24>
Django
--------------------------------+-----------------------------------------
Reporter: Aalekh Patel | Owner: Salvo Polizzi
Type: Bug | Status: assigned
Component: Core (Mail) | Version: dev
Severity: Normal | Resolution:
Keywords: email, headers | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------+-----------------------------------------
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/35033#comment:25>
Django
--------------------------------+-----------------------------------------
Reporter: Aalekh Patel | Owner: Salvo Polizzi
Type: Bug | Status: assigned
Component: Core (Mail) | Version: dev
Severity: Normal | Resolution:
Keywords: email, headers | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
--------------------------------+-----------------------------------------
Comment (by Salvo Polizzi):
Replying to [comment:25 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/35033#comment:26>
Django
--------------------------------+-----------------------------------------
Reporter: Aalekh Patel | Owner: Salvo Polizzi
Type: Bug | Status: assigned
Component: Core (Mail) | Version: dev
Severity: Normal | Resolution:
Keywords: email, headers | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
--------------------------------+-----------------------------------------
Comment (by Mariusz Felisiak):
Sorry, I forgot to click "Submit the review".
--
Ticket URL: <https://code.djangoproject.com/ticket/35033#comment:27>
Django
--------------------------------+-----------------------------------------
Reporter: Aalekh Patel | Owner: Salvo Polizzi
Type: Bug | Status: assigned
Component: Core (Mail) | Version: dev
Severity: Normal | Resolution:
Keywords: email, headers | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
--------------------------------+-----------------------------------------
Comment (by Salvo Polizzi):
I left some comments in the new PR:
https://github.com/django/django/pull/17674
--
Ticket URL: <https://code.djangoproject.com/ticket/35033#comment:28>
Django
--------------------------------+-----------------------------------------
Reporter: Aalekh Patel | Owner: Salvo Polizzi
Type: Bug | Status: assigned
Component: Core (Mail) | Version: dev
Severity: Normal | Resolution:
Keywords: email, headers | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------+-----------------------------------------
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/35033#comment:29>
Django
--------------------------------+-----------------------------------------
Reporter: Aalekh Patel | Owner: Salvo Polizzi
Type: Bug | Status: assigned
Component: Core (Mail) | Version: dev
Severity: Normal | Resolution:
Keywords: email, headers | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------+-----------------------------------------
* needs_better_patch: 0 => 1
Comment:
We need to find
[https://github.com/django/django/pull/17642#issuecomment-1875327543
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/35033#comment:30>
Django
--------------------------------+-----------------------------------------
Reporter: Aalekh Patel | Owner: Salvo Polizzi
Type: Bug | Status: assigned
Component: Core (Mail) | Version: dev
Severity: Normal | Resolution:
Keywords: email, headers | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
--------------------------------+-----------------------------------------
Comment (by Natalia Bidart):
[https://forum.djangoproject.com/t/repeated-headers-in-email-message/26748
Forum post]
--
Ticket URL: <https://code.djangoproject.com/ticket/35033#comment:31>
Django
--------------------------------+-----------------------------------------
Reporter: Aalekh Patel | Owner: Salvo Polizzi
Type: Bug | Status: assigned
Component: Core (Mail) | Version: dev
Severity: Normal | Resolution:
Keywords: email, headers | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
--------------------------------+-----------------------------------------
* cc: Mike Edmunds (added)
Comment:
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
[[https://github.com/anymail/django-anymail/issues/91|specific request]]
to match the Django SMTPBackend's handling of these headers.]
--
Ticket URL: <https://code.djangoproject.com/ticket/35033#comment:32>
Django
| Polizzi
Type: Bug | Status: assigned
Component: Core (Mail) | Version: dev
Severity: Normal | Resolution:
compat32 |
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Mike Edmunds):
* keywords: email, headers => email, headers, compat32
Comment:
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`
[https://docs.python.org/3/library/email.compat32-message.html#email.message.Message.__setitem__
behaves as a multi-value dict when assigning to header keys], but
[https://docs.python.org/3/library/email.compat32-message.html#email.message.Message.__getitem__
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
[https://github.com/django/django/blob/stable/5.1.x/tests/mail/tests.py#L410-L412
test_to_header()] to use
[https://docs.python.org/3/library/email.compat32-message.html#email.message.Message.get_all
get_all("To")] and verify there's only the one correct value in the list.
Same thing in
[https://github.com/django/django/blob/stable/5.1.x/tests/mail/tests.py#L439
test_reply_to_header()]. Wouldn't hurt to update some other nearby tests
in the same way, and add a test_cc_header().
2. Then fix
[https://github.com/django/django/blob/stable/5.1.x/django/core/mail/message.py#L432
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 [https://groups.google.com/g
/django-developers/c/2zf9GQtjdIk moves to Python's modern email API],
those (and many other email header restrictions) will be enforced by
Python.
--
Ticket URL: <https://code.djangoproject.com/ticket/35033#comment:33>
Django
-------------------------------------+-------------------------------------
Reporter: Aalekh Patel | Owner: Salvo
| Polizzi
Type: Bug | Status: assigned
Component: Core (Mail) | Version: dev
Severity: Normal | Resolution:
Keywords: email, headers, | Triage Stage: Accepted
compat32 |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Changes (by Mike Edmunds):
Comment:
(Also, that minimal and safe fix should be easy pickings)
--
Ticket URL: <https://code.djangoproject.com/ticket/35033#comment:34>