Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't log a warning on content-type: multipart/* responses #1665

Merged
merged 9 commits into from
Sep 3, 2020

Conversation

badcure
Copy link
Contributor

@badcure badcure commented Aug 16, 2019

This is to address the multipart errors that are showing up in urllib. The real fix is upstream, but the goal is to remove the false warnings that are showing up as described in #800 .

If the exceptions show up, I am simply removing them from the list. The exceptions would only apply if it is processing the body as well. In the urllib3 code path, it only cares about the headers at that point.

Closes #800

@codecov-io
Copy link

codecov-io commented Sep 27, 2019

Codecov Report

Merging #1665 into master will decrease coverage by 0.4%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1665      +/-   ##
==========================================
- Coverage   99.75%   99.35%   -0.41%     
==========================================
  Files          22       22              
  Lines        2006     2000       -6     
==========================================
- Hits         2001     1987      -14     
- Misses          5       13       +8
Impacted Files Coverage Δ
src/urllib3/util/response.py 100% <100%> (ø) ⬆️
src/urllib3/connection.py 93.75% <0%> (-4.2%) ⬇️
src/urllib3/response.py 99.44% <0%> (-0.56%) ⬇️
src/urllib3/util/url.py 98.97% <0%> (-0.03%) ⬇️
src/urllib3/util/timeout.py 100% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d369183...18fd8a8. Read the comment docs.

@badcure
Copy link
Contributor Author

badcure commented Oct 21, 2019

Hello @pquentin , wanted to see if you had time to look over this. The goal of this is to remove the warnings, and wanted to see if there were concerns going down this route.

@pquentin
Copy link
Member

pquentin commented Oct 23, 2019

@badcure Thank you for putting this pull request together! It's easy to read, and the change does make sense.

However, I don't know enough about header parsing to reason about possible issues (eg. security) so I'm not going to merge this myself, sorry. :(

@harryhobson
Copy link

harryhobson commented Apr 7, 2020

Hi @pquentin, who is the right person to review and accept this pull request? It has been sitting dormant for a while so I just want to check this hasn't been forgotten.

@pquentin
Copy link
Member

pquentin commented Apr 7, 2020

@sethmlarson Any ideas?

@sethmlarson sethmlarson added this to the v1.26 milestone Aug 27, 2020
@sethmlarson
Copy link
Member

sethmlarson commented Aug 28, 2020

Sorry that this took so long, I've done the following:

  • Reviewed the change, I believe that this is a good change for us 👍
  • Confirmed that totally valid HTTP multipart/mixed responses receive these defects (even when using raw httplib) so we definitely should be ignoring them if we're only parsing headers.
  • Added a test case that asserts that the log.warning() isn't called when receiving a multipart/mixed message
  • Made some changes to the PR to remove the unnecessary pytest.mark.parametrize() and use onlyPy3
  • Reworded the docs for the behavior and simplified the filtering

@pquentin @hodbn could one of you review my changes to this PR?

@sethmlarson sethmlarson changed the title Fix multipart processing Don't log a warning on content-type: multipart/* responses Aug 31, 2020
hodbn
hodbn previously approved these changes Sep 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multipart mixed responses generate warnings
6 participants