bpo-36226: Fix multipart false positive header defects

[grische]

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: https://github.com/python/cpython/commit/89285439c7f94a3e62cee3d15e343218903c2af8

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: https://github.com/python/cpython/pull/12214/commits/a82e662ab3339072d7b86a8090989fba60ef9c37

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://stackoverflow.com/questions/49338811/does-requests-properly-support-multipart-responses
• https://github.com/urllib3/urllib3/issues/800

https://bugs.python.org/issue36226

[the-knights-who-say-ni]

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]

@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]

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]

@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]

@grische not sure who you emailed but you should email [email protected] to find out what's going on with your CLA.

[bedevere-bot]

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.

[grische]

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]

Thanks for making the requested changes!

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

[maxking]

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]

@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]

@maxking are there any further changes you are requesting?

[grische]

@maxking ping

[github-actions[bot]]

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

[grische]

@maxking ping

[github-actions[bot]]

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

[fadenb]

Still an issue

[bedevere-app[bot]]

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.