bpo-27321: email: don't try to replace headers that aren't set by kyrias · Pull Request #1977 · python/cpython (original) (raw)
| 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]) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that looks good.
| Test if UTF-8 messages with no Content-Transfer-Encoding set can be as_string'd: |
| Föö bär |
| ------5F0D91A9C8C24D91AF71E1D7F2F7877E-- |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
This problem is affecting me. What's the status?
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.
@bitdancer Hey, sorry it took so long, not had any time to work on stuff lately.
| @@ -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 = """\ |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, just copied the first inline tests I saw, sorry.
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 johannes@kyriasis.com
Signed-off-by: Johannes Löthberg johannes@kyriasis.com
Signed-off-by: Johannes Löthberg johannes@kyriasis.com
I have made the requested changes; please review again
Thanks for making the requested changes!
@bitdancer: please review the changes made to this pull request.
I would love to see this review completed as well ! @bitdancer If I understand correctly ?
| 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]) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kyrias, please address the code review request. Thank you!
This PR should be considered obsolete and closed in favor of #18074
miss-islington pushed a commit that referenced this pull request
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
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request
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 mark@msapiro.net
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request
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 mark@msapiro.net
miss-islington added a commit that referenced this pull request
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 mark@msapiro.net
miss-islington added a commit that referenced this pull request
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 mark@msapiro.net
adorilson pushed a commit to adorilson/cpython that referenced this pull request
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
mcepl pushed a commit to openSUSE-Python/cpython that referenced this pull request
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 mark@msapiro.net Date: Mon, 19 Oct 2020 16:11:37 -0700 Fixes: bsc#1208443 Patch: bpo27321-email-no-replace-header.patch
mcepl pushed a commit to openSUSE-Python/cpython that referenced this pull request
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 mark@msapiro.net Date: Mon, 19 Oct 2020 16:11:37 -0700 Fixes: bsc#1208443 Patch: bpo27321-email-no-replace-header.patch
mcepl pushed a commit to openSUSE-Python/cpython that referenced this pull request
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 mark@msapiro.net Date: Mon, 19 Oct 2020 16:11:37 -0700 Fixes: bsc#1208443 Patch: bpo27321-email-no-replace-header.patch
mcepl pushed a commit to openSUSE-Python/cpython that referenced this pull request
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 mark@msapiro.net Date: Mon, 19 Oct 2020 16:11:37 -0700 Fixes: bsc#1208443 Patch: bpo27321-email-no-replace-header.patch
mcepl pushed a commit to openSUSE-Python/cpython that referenced this pull request
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 mark@msapiro.net Date: Mon, 19 Oct 2020 16:11:37 -0700 Fixes: bsc#1208443 Patch: bpo27321-email-no-replace-header.patch
mcepl pushed a commit to openSUSE-Python/cpython that referenced this pull request
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 mark@msapiro.net Date: Mon, 19 Oct 2020 16:11:37 -0700 Fixes: bsc#1208443 Patch: bpo27321-email-no-replace-header.patch