Opened 5 months ago
Last modified 4 months ago
#35033 assigned Bug
EmailMessage repeats header if provided via the headers kwargs
Reported by: | Aalekh Patel | Owned by: | Salvo Polizzi |
---|---|---|---|
Component: | Core (Mail) | Version: | dev |
Severity: | Normal | Keywords: | email, headers |
Cc: | Adam Johnson, Florian Apolloner, David Wobrock | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description (last modified by )
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 (31)
comment:1 by , 5 months ago
Has patch: | set |
---|
comment:2 by , 5 months ago
Easy pickings: | set |
---|---|
Needs tests: | set |
comment:3 by , 5 months ago
Component: | Uncategorized → Core (Mail) |
---|---|
Description: | modified (diff) |
comment:4 by , 5 months ago
Cc: | added |
---|---|
Easy pickings: | unset |
Patch needs improvement: | set |
Version: | 3.2 → dev |
comment:5 by , 5 months ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:6 by , 5 months ago
Summary: | EmailMessage repeats "To" header if provided via the headers kwargs → EmailMessage repeats header if provided via the headers kwargs |
---|
comment:8 by , 5 months ago
Cc: | 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 , 5 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 , 5 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 , 5 months ago
Cc: | added |
---|
comment:12 by , 5 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 , 5 months ago
Has patch: | unset |
---|---|
Needs tests: | unset |
Patch needs improvement: | unset |
comment:14 by , 4 months ago
Hi, i made a PR for this bug: https://github.com/django/django/pull/17606/files
comment:15 by , 4 months ago
Has patch: | set |
---|
follow-up: 17 comment:16 by , 4 months ago
Cc: | added |
---|---|
Needs tests: | set |
Owner: | changed from | to
Patch needs improvement: | set |
Status: | new → assigned |
This is the PR you meant https://github.com/django/django/pull/17642, right? :)
comment:17 by , 4 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 , 4 months ago
Needs tests: | unset |
---|---|
Patch needs improvement: | unset |
follow-up: 21 comment:19 by , 4 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 , 4 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.
comment:21 by , 4 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 , 4 months ago
Needs tests: | unset |
---|---|
Patch needs improvement: | unset |
comment:23 by , 4 months ago
Patch needs improvement: | set |
---|
comment:24 by , 4 months ago
Patch needs improvement: | unset |
---|
follow-up: 26 comment:25 by , 4 months ago
Patch needs improvement: | set |
---|
comment:26 by , 4 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:28 by , 4 months ago
I left some comments in the new PR: https://github.com/django/django/pull/17674
comment:29 by , 4 months ago
Patch needs improvement: | unset |
---|
comment:30 by , 4 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.
Reproduced, and accepting based on the RFC which states:
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.