bpo-27321: email: don't try to replace headers that aren't set #1977

kyrias · 2017-06-06T22:48:02Z

Currently the email module sometimes blows up on emails that don't contain e.g. a Content-Transfer-Encoding header, even though it doesn't need to exist. Fix this by only replacing it if it's already set.

https://bugs.python.org/issue27321

mention-bot · 2017-06-06T22:48:06Z

@kyrias, thanks for your PR! By analyzing the history of the files in this pull request, we identified @bitdancer, @serhiy-storchaka and @ezio-melotti to be potential reviewers.

bitdancer · 2017-06-19T23:32:03Z

Lib/email/generator.py

+            if msg.get('content-transfer-encoding') is not None:
+                msg.replace_header('content-transfer-encoding', munge_cte[0])
+            if msg.get('content-type') is not None:
+                msg.replace_header('content-type', munge_cte[1])


The CTE is munged only when (a) we are translating to ASCII-only strings (Generator and not BytesGenerator) and (b) we have a non-None 'charset', which means we have a content-type header. So the content-type header should be set unconditionally so that if someone changes (b) we might notice. Which means we should also have a test that has non-ascii and neither header...which might find a new error :)

The original bug report https://bugs.python.org/issue27321 was for a message which had a Content-Type: but no Content-Transfer-Encoding:.
Based on @bitdancer comment, perhaps the code should be:

msg.replace_header('content-type', munge_cte[1]) if msg.get('content-transfer-encoding') is not None: msg.replace_header('content-transfer-encoding', munge_cte[0] else: msg['Content-Transfer-Encoding'] = munge_cte[0]

Of course, a test with a non-ascii message with neither header wouldn't hurt in any case.

Yes that looks good.

bitdancer · 2017-06-19T23:36:00Z

Lib/test/test_email/data/msg_47.txt

+
+Test if UTF-8 messages with no Content-Transfer-Encoding set can be as_string'd:
+Föö bär
+------5F0D91A9C8C24D91AF71E1D7F2F7877E--


I would prefer to have the message text encoded in the test file rather than introduce a new file in the data directory. I think it makes the tests easier to read and understand. You'll find a number of other tests where I've done that elsewhere in the file.

carlbordum · 2017-11-22T19:05:07Z

This problem is affecting me. What's the status?

bitdancer · 2017-12-15T02:36:45Z

This problem is affecting me. What's the status

Someone needs to address my review comments. It doesn't have to be the original PR author, although that would be ideal.

kyrias · 2018-01-25T19:51:43Z

@bitdancer Hey, sorry it took so long, not had any time to work on stuff lately.

bitdancer · 2018-01-30T13:50:43Z

Lib/test/test_email/test_email.py

@@ -311,6 +311,34 @@ def test_as_string_policy(self):
        g.flatten(msg)
        self.assertEqual(fullrepr, s.getvalue())

+    def test_nonascii_as_string_without_cte(self):
+        m = """\


You can use textwrap.dedent here to indent the message. Then wrap the content lines so the whole thing still stays under 80 characters (that isn't always possible, but looks to be so in this case).

Ah, just copied the first inline tests I saw, sorry.

bedevere-bot · 2018-01-30T13:52:19Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

Signed-off-by: Johannes Löthberg <[email protected]>

kyrias · 2018-01-30T16:27:54Z

I have made the requested changes; please review again

bedevere-bot · 2018-01-30T16:27:57Z

Thanks for making the requested changes!

@bitdancer: please review the changes made to this pull request.

kyrias · 2018-03-07T19:30:36Z

@bitdancer Ping

ses4j · 2019-02-26T02:41:09Z

This bug causes coddingtonbear/django-mailbox#136. It would be very helpful if this review were completed. Thanks!

@bitdancer.

csabella · 2019-06-05T01:16:04Z

cc @maxking

erk3 · 2019-06-05T20:09:43Z

I would love to see this review completed as well ! @bitdancer If I understand correctly ?

maxking · 2019-06-06T21:58:18Z

Lib/email/generator.py

            msg.replace_header('content-type', munge_cte[1])
+            if msg.get('content-transfer-encoding') is not None:
+                msg.replace_header('content-transfer-encoding', munge_cte[0])


Can you set the header if it doesn't already exists?

See #1977 (comment)

csabella · 2020-01-25T20:19:48Z

@kyrias, please address the code review request. Thank you!

msapiro · 2020-01-26T01:26:03Z

This PR should be considered obsolete and closed in favor of #18074

csabella · 2020-01-26T10:07:09Z

@msapiro, thank you!

@warsaw

GH-18074) This PR replaces #1977. The reason for the replacement is two-fold. The fix itself is different is that if the CTE header doesn't exist in the original message, it is inserted. This is important because the new CTE could be quoted-printable whereas the original is implicit 8bit. Also the tests are different. The test_nonascii_as_string_without_cte test in #1977 doesn't actually test the issue in that it passes without the fix. The test_nonascii_as_string_without_content_type_and_cte test is improved here, and even though it doesn't fail without the fix, it is included for completeness. Automerge-Triggered-By: @warsaw

@warsaw

pythonGH-18074) This PR replaces pythonGH-1977. The reason for the replacement is two-fold. The fix itself is different is that if the CTE header doesn't exist in the original message, it is inserted. This is important because the new CTE could be quoted-printable whereas the original is implicit 8bit. Also the tests are different. The test_nonascii_as_string_without_cte test in pythonGH-1977 doesn't actually test the issue in that it passes without the fix. The test_nonascii_as_string_without_content_type_and_cte test is improved here, and even though it doesn't fail without the fix, it is included for completeness. Automerge-Triggered-By: @warsaw (cherry picked from commit bf83822) Co-authored-by: Mark Sapiro <[email protected]>

@warsaw

pythonGH-18074) This PR replaces pythonGH-1977. The reason for the replacement is two-fold. The fix itself is different is that if the CTE header doesn't exist in the original message, it is inserted. This is important because the new CTE could be quoted-printable whereas the original is implicit 8bit. Also the tests are different. The test_nonascii_as_string_without_cte test in pythonGH-1977 doesn't actually test the issue in that it passes without the fix. The test_nonascii_as_string_without_content_type_and_cte test is improved here, and even though it doesn't fail without the fix, it is included for completeness. Automerge-Triggered-By: @warsaw (cherry picked from commit bf83822) Co-authored-by: Mark Sapiro <[email protected]>

@warsaw

GH-18074) This PR replaces GH-1977. The reason for the replacement is two-fold. The fix itself is different is that if the CTE header doesn't exist in the original message, it is inserted. This is important because the new CTE could be quoted-printable whereas the original is implicit 8bit. Also the tests are different. The test_nonascii_as_string_without_cte test in GH-1977 doesn't actually test the issue in that it passes without the fix. The test_nonascii_as_string_without_content_type_and_cte test is improved here, and even though it doesn't fail without the fix, it is included for completeness. Automerge-Triggered-By: @warsaw (cherry picked from commit bf83822) Co-authored-by: Mark Sapiro <[email protected]>

@warsaw

GH-18074) This PR replaces GH-1977. The reason for the replacement is two-fold. The fix itself is different is that if the CTE header doesn't exist in the original message, it is inserted. This is important because the new CTE could be quoted-printable whereas the original is implicit 8bit. Also the tests are different. The test_nonascii_as_string_without_cte test in GH-1977 doesn't actually test the issue in that it passes without the fix. The test_nonascii_as_string_without_content_type_and_cte test is improved here, and even though it doesn't fail without the fix, it is included for completeness. Automerge-Triggered-By: @warsaw (cherry picked from commit bf83822) Co-authored-by: Mark Sapiro <[email protected]>

@warsaw

pythonGH-18074) This PR replaces python#1977. The reason for the replacement is two-fold. The fix itself is different is that if the CTE header doesn't exist in the original message, it is inserted. This is important because the new CTE could be quoted-printable whereas the original is implicit 8bit. Also the tests are different. The test_nonascii_as_string_without_cte test in python#1977 doesn't actually test the issue in that it passes without the fix. The test_nonascii_as_string_without_content_type_and_cte test is improved here, and even though it doesn't fail without the fix, it is included for completeness. Automerge-Triggered-By: @warsaw

This PR replaces pythonGH-1977. The reason for the replacement is two-fold. The fix itself is different is that if the CTE header doesn't exist in the original message, it is inserted. This is important because the new CTE could be quoted-printable whereas the original is implicit 8bit. Also the tests are different. The test_nonascii_as_string_without_cte test in pythongh-1977 doesn't actually test the issue in that it passes without the fix. The test_nonascii_as_string_without_content_type_and_cte test is improved here, and even though it doesn't fail without the fix, it is included for completeness. Fixed KeyError exception when flattening an email to a string attempts to replace a non-existent Content-Transfer-Encoding header. Code is originally from gh#python/cpython@371146a, it was released upstream in 3.8.7 Co-authored-by: Mark Sapiro <[email protected]> Date: Mon, 19 Oct 2020 16:11:37 -0700 Fixes: bsc#1208443 Patch: bpo27321-email-no-replace-header.patch

the-knights-who-say-ni added the CLA signed label Jun 6, 2017

bitdancer requested changes Jun 19, 2017

View reviewed changes

kyrias force-pushed the bpo-27321 branch from f02ff0a to 3016af0 Compare January 25, 2018 19:51

kyrias requested a review from a team as a code owner January 25, 2018 19:51

kyrias force-pushed the bpo-27321 branch from 3016af0 to 48cd758 Compare January 25, 2018 20:11

bitdancer requested changes Jan 30, 2018

View reviewed changes

bedevere-bot added the awaiting changes label Jan 30, 2018

bitdancer mentioned this pull request Jan 30, 2018

bpo-32634: fix write in email/generator.py #5267

Closed

kyrias added 3 commits January 30, 2018 17:26

bpo-27321: Only replace_header if the header is already set

0bb8e4b

Signed-off-by: Johannes Löthberg <[email protected]>

Add tests for bpo-27321

2c3379c

Signed-off-by: Johannes Löthberg <[email protected]>

Add a NEWS.d blurb regarding bpo-27321

429c140

Signed-off-by: Johannes Löthberg <[email protected]>

kyrias force-pushed the bpo-27321 branch from 91d69c8 to 429c140 Compare January 30, 2018 16:27

bedevere-bot added awaiting change review and removed awaiting changes labels Jan 30, 2018

csabella requested a review from bitdancer March 1, 2019 16:41

maxking reviewed Jun 6, 2019

View reviewed changes

msapiro mentioned this pull request Jan 20, 2020

bpo-27321 Fix email.generator.py to not replace a non-existent header. #18074

Merged

csabella added awaiting changes and removed awaiting change review labels Jan 25, 2020

csabella closed this Jan 26, 2020

miss-islington mentioned this pull request Oct 19, 2020

[3.9] bpo-27321 Fix email.generator.py to not replace a non-existent header. (GH-18074) #22796

Merged

miss-islington mentioned this pull request Oct 19, 2020

[3.8] bpo-27321 Fix email.generator.py to not replace a non-existent header. (GH-18074) #22797

Merged

Uh oh!

bpo-27321: email: don't try to replace headers that aren't set #1977

bpo-27321: email: don't try to replace headers that aren't set #1977

Uh oh!

Conversation

kyrias commented Jun 6, 2017 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mention-bot commented Jun 6, 2017

Uh oh!

bitdancer Jun 19, 2017

Choose a reason for hiding this comment

Uh oh!

msapiro Jan 23, 2018

Choose a reason for hiding this comment

Uh oh!

bitdancer Jan 23, 2018

Choose a reason for hiding this comment

Uh oh!

bitdancer Jun 19, 2017

Choose a reason for hiding this comment

Uh oh!

carlbordum commented Nov 22, 2017

Uh oh!

bitdancer commented Dec 15, 2017

Uh oh!

kyrias commented Jan 25, 2018

Uh oh!

bitdancer Jan 30, 2018

Choose a reason for hiding this comment

Uh oh!

kyrias Jan 30, 2018

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Jan 30, 2018

Uh oh!

kyrias commented Jan 30, 2018

Uh oh!

bedevere-bot commented Jan 30, 2018

Uh oh!

kyrias commented Mar 7, 2018

Uh oh!

ses4j commented Feb 26, 2019

Uh oh!

csabella commented Jun 5, 2019

Uh oh!

erk3 commented Jun 5, 2019

Uh oh!

maxking Jun 6, 2019

Choose a reason for hiding this comment

Uh oh!

csabella commented Jan 25, 2020

Uh oh!

msapiro commented Jan 26, 2020

Uh oh!

csabella commented Jan 26, 2020

Uh oh!

Uh oh!

kyrias commented Jun 6, 2017 •

edited by bedevere-bot

Loading