bpo-36226: Fix multipart false positive header defects #12214

grische · 2019-03-07T16:09:11Z

The current implementation of multipart/related in urllib triggers header defects even though the headers are valid:
[StartBoundaryNotFoundDefect(), MultipartInvariantViolationDefect()]

The example header is valid according to RFC 2387 (https://tools.ietf.org/html/rfc2387):

Content-Type: multipart/related; boundary="==="

Both defects are triggered by the fact that httplib only passes on headers to the underlying email parser, while the email parser assumes to receive a full message. The simple fix is to tell the underlying email parser that we are only passing the header: 8928543

The second issue is related, but independent: The underlying email parser checks if the parsed message is of type multipart by checking of the object "root" is of type list. As we only passed the header (and set headersonly=True), the check does makes no sense anymore at this point, creating a false positive: a82e662

This issue has been a while with python (backport to 3.7 easily possible) and can also be found in urllib3 (hence also requests, etc):

https://bugs.python.org/issue36226

the-knights-who-say-ni · 2019-03-07T16:09:15Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

You can check yourself to see if the CLA has been received.

Thanks again for your contribution, we look forward to reviewing it!

csabella · 2019-06-05T16:03:31Z

@grische, thank you for your interest in contributing to CPython. In order to have someone review your change, you'll need to sign the Contributor agreement. Thanks!

brettcannon · 2019-08-08T23:47:17Z

Thanks for the PR, but closing as the CLA has not been signed within the last month. If you do decide to sign the CLA we can re-open this PR.

grische · 2019-08-08T23:57:49Z

@brettcannon the CLA has been signed a long time ago, but the system doesn't show the results. Your legal team has been informed and should be aware of the issue.

brettcannon · 2019-08-09T21:35:28Z

@grische not sure who you emailed but you should email contributors@python.org to find out what's going on with your CLA.

maxking

Changes overall looks good to me. I have some small comments inline.

Also, please add a news entry using the blurb tool.

maxking · 2019-09-05T00:58:44Z

Lib/test/test_email/test_parser.py

@@ -105,6 +105,12 @@ def message_from_binary_file(s, *args, **kw):
 class TestBytesParser(TestParserBase, TestEmailBase):
    parsers = (message_from_bytes, message_from_binary_file)

+class TestFeedParser(TestCustomMessage):
+    def test_multipart_message_with_headers_only(self):
+        import email.parser


Move this to the top of the file.

Moved it to the top

maxking · 2019-09-05T01:00:36Z

Lib/test/test_email/test_parser.py

@@ -105,6 +105,12 @@ def message_from_binary_file(s, *args, **kw):
 class TestBytesParser(TestParserBase, TestEmailBase):
    parsers = (message_from_bytes, message_from_binary_file)

+class TestFeedParser(TestCustomMessage):


I don't see a need for this to inherit from TestCustomMessage, let's just do TestEmailBase instead?

Changed to TestEmailBase

bedevere-bot · 2019-09-05T01:01:30Z

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.

root cannot be a list if only the headers have been passed on and are being parsed. This fixes false MultipartInvariantViolationDefects

Currently parse_headers only gives headers to the parser. For the parser to correctly handle this case, it also needs to set headersonly=True This fixes false StartBoundaryNotFoundDefects

grische · 2019-09-05T10:19:41Z

I added the news entries for both commits, as they are fixing different parts.

I have made the requested changes; please review again

bedevere-bot · 2019-09-05T10:19:44Z

Thanks for making the requested changes!

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

maxking · 2019-09-14T18:57:38Z

Catching up on 4 years of conversations on b.p.o, this issue is way more complicated than it looks :)

Most of the context (and one another associated patch) is present in https://bugs.python.org/issue24363, https://bugs.python.org/issue29353. Martin Panter thinks using headersonly can cause problems when trying to convert it back to string

Parser().parsestr("Content-Type: message/delivery-status\r\nInvalid line\r\n\r\n", headersonly=True).as_string()

See bpo-29353 has context on this side effect of using headersonly.

grische · 2019-09-16T10:07:40Z

@maxking Thanks for digging into that.

I am aware that there are more issues in the parser. However, I am confident that my patches are on the right track to improve the situation without breaking backwards compatibility, as I didn't observe any failing tests.

It seems that you are taking care of the outstanding (and related) issues on bpo-24363? It would be good to have these sorted as well.

Regarding the as_string conversion, I agree to David Murray, that the lack of tests for this conversion seems like it is not an expected use-case and hence does not seem like a hard blocker to get this PR merged?

grische · 2019-09-25T08:11:28Z

@maxking are there any further changes you are requesting?

grische · 2020-01-20T15:48:57Z

@maxking ping

github-actions · 2025-06-15T00:13:24Z

This PR is stale because it has been open for 30 days with no activity.

grische · 2025-06-15T06:46:36Z

@maxking ping

grische requested a review from a team as a code owner March 7, 2019 16:09

the-knights-who-say-ni added the CLA not signed label Mar 7, 2019

bedevere-bot added the awaiting review label Mar 7, 2019

csabella removed the CLA not signed label Jun 5, 2019

the-knights-who-say-ni added the CLA not signed label Jun 5, 2019

brettcannon closed this Aug 8, 2019

badcure mentioned this pull request Aug 12, 2019

cannot suppress urllib3 warnings diyan/pywinrm#269

Closed

brettcannon reopened this Sep 4, 2019

brettcannon removed the CLA not signed label Sep 4, 2019

the-knights-who-say-ni added the CLA signed label Sep 4, 2019

maxking suggested changes Sep 5, 2019

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Sep 5, 2019

grische added 2 commits September 5, 2019 12:15

bpo-36226: Ignore multipart check for headersonly

6dd5999

root cannot be a list if only the headers have been passed on and are being parsed. This fixes false MultipartInvariantViolationDefects

bpo-36226: Fix email parsing more than headers

bb9dd1b

Currently parse_headers only gives headers to the parser. For the parser to correctly handle this case, it also needs to set headersonly=True This fixes false StartBoundaryNotFoundDefects

grische force-pushed the fix-issue-36226 branch from 8928543 to bb9dd1b Compare September 5, 2019 10:18

bedevere-bot added awaiting change review and removed awaiting changes labels Sep 5, 2019

brettcannon requested a review from maxking September 9, 2019 15:49

csabella requested review from maxking and removed request for maxking January 20, 2020 17:46

ezio-melotti removed the CLA signed label Jul 13, 2022

brokenenglish mannequin mentioned this pull request Nov 23, 2023

Incorrect handling of HTTP response with "Content-Type: message/rfc822" header #73539

Open

github-actions bot added the stale Stale PR or inactive for long period of time. label Jun 15, 2025

Uh oh!

bpo-36226: Fix multipart false positive header defects #12214

Are you sure you want to change the base?

bpo-36226: Fix multipart false positive header defects #12214

Uh oh!

Conversation

grische commented Mar 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

the-knights-who-say-ni commented Mar 7, 2019

Uh oh!

csabella commented Jun 5, 2019

Uh oh!

brettcannon commented Aug 8, 2019

Uh oh!

grische commented Aug 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brettcannon commented Aug 9, 2019

Uh oh!

maxking left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maxking Sep 5, 2019

Choose a reason for hiding this comment

Uh oh!

grische Sep 5, 2019

Choose a reason for hiding this comment

Uh oh!

maxking Sep 5, 2019

Choose a reason for hiding this comment

Uh oh!

grische Sep 5, 2019

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Sep 5, 2019

Uh oh!

grische commented Sep 5, 2019

Uh oh!

bedevere-bot commented Sep 5, 2019

Uh oh!

maxking commented Sep 14, 2019 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

grische commented Sep 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

grische commented Sep 25, 2019

Uh oh!

grische commented Jan 20, 2020

Uh oh!

github-actions bot commented Jun 15, 2025

Uh oh!

grische commented Jun 15, 2025

Uh oh!

Uh oh!

grische commented Mar 7, 2019 •

edited

Loading

grische commented Aug 8, 2019 •

edited

Loading

maxking left a comment •

edited

Loading

maxking commented Sep 14, 2019 •

edited by bedevere-bot

Loading

grische commented Sep 16, 2019 •

edited

Loading