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
Fix for 'Failed to parse headers' warning #1439
Changes from 7 commits
0796637
7e78f79
5472700
129a895
7b13457
5c0f06c
7649111
9fff9af
f22b64b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1289,12 +1289,12 @@ def socket_handler(listener): | |
) | ||
class TestBrokenHeaders(SocketDummyServerTestCase): | ||
|
||
def _test_broken_header_parsing(self, headers): | ||
def _test_broken_header_parsing(self, headers, unparsed_data_check=None): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why were these test cases changed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you mean the addition of unparsed_data_check specifically? I looked at the various Stackoverflow questions where this had come up, and realised we already had a test case where unparsed_data was non-empty, so I thought it would be good to check for that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I meant more the input data, it looks like you've changed the number of CRLF within the test case, what was the motivation for those changes? I'd be more comfortable adding a new testcase for unparsed_data than changing these existing ones. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As for the changes in the values of b'HTTP/1.1 200 OK\r\nContent-Length: 0\r\nContent-type: text/plain\r\n: Value\r\n\r\nAnother: Header\r\n\r\n'
b'HTTP/1.1 200 OK\r\nContent-Length: 0\r\nContent-type: text/plain\r\n:\r\n\r\nAnother: Header\r\n\r\n' Looking in more detail, the self._test_broken_header_parsing([
b': Value\r\n',
b'Another: Header\r\n',
]) combined with The duplicate CRLF before The best fix seemed to me to remove the CRLFs from the test bytestrings, and ensure the headers are terminated with an explicit CRLFCRLF. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's next for this PR? Are you happy with my explanations, or would you prefer me to rework the tests? |
||
self.start_response_handler(( | ||
b'HTTP/1.1 200 OK\r\n' | ||
b'Content-Length: 0\r\n' | ||
b'Content-type: text/plain\r\n' | ||
) + b'\r\n'.join(headers) + b'\r\n' | ||
) + b'\r\n'.join(headers) + b'\r\n\r\n' | ||
) | ||
|
||
pool = HTTPConnectionPool(self.host, self.port, retries=False) | ||
|
@@ -1306,26 +1306,52 @@ def _test_broken_header_parsing(self, headers): | |
for record in logs: | ||
if 'Failed to parse headers' in record.msg and \ | ||
pool._absolute_url('/') == record.args[0]: | ||
return | ||
if unparsed_data_check is None or unparsed_data_check in record.getMessage(): | ||
return | ||
self.fail('Missing log about unparsed headers') | ||
|
||
def test_header_without_name(self): | ||
self._test_broken_header_parsing([ | ||
b': Value\r\n', | ||
b'Another: Header\r\n', | ||
b': Value', | ||
b'Another: Header', | ||
]) | ||
|
||
def test_header_without_name_or_value(self): | ||
self._test_broken_header_parsing([ | ||
b':\r\n', | ||
b'Another: Header\r\n', | ||
b':', | ||
b'Another: Header', | ||
]) | ||
|
||
def test_header_without_colon_or_value(self): | ||
self._test_broken_header_parsing([ | ||
b'Broken Header', | ||
b'Another: Header', | ||
]) | ||
], 'Broken Header') | ||
|
||
|
||
class TestHeaderParsingContentType(SocketDummyServerTestCase): | ||
|
||
def _test_okay_header_parsing(self, header): | ||
self.start_response_handler(( | ||
b'HTTP/1.1 200 OK\r\n' | ||
b'Content-Length: 0\r\n' | ||
) + header + b'\r\n\r\n' | ||
) | ||
|
||
pool = HTTPConnectionPool(self.host, self.port, retries=False) | ||
self.addCleanup(pool.close) | ||
|
||
with LogRecorder() as logs: | ||
pool.request('GET', '/') | ||
|
||
for record in logs: | ||
assert 'Failed to parse headers' not in record.msg | ||
|
||
def test_header_text_plain(self): | ||
self._test_okay_header_parsing(b'Content-type: text/plain') | ||
|
||
def test_header_message_rfc822(self): | ||
self._test_okay_header_parsing(b'Content-type: message/rfc822') | ||
|
||
|
||
class TestHEAD(SocketDummyServerTestCase): | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we change the
# Platform-specific: Python 3
into a different comment format to ensure we're getting coverage below. Ourcoveragerc
makes any branch with this comment not count towards coverage.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
# Platform-specific: Python 3
comment was there originally, not added by me; also I note that the docstring for the function saysOnly works on Python 3.
(These were modified/added in commit 275cfda in 2015.) I'm not familiar with the project's usage of this marker and why it's excluded from coverage, so I'll follow your lead.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know you didn't add them, sorry if what I said above was misunderstood :) We use markers like that to exclude a function from test coverage but (imo) a lot of these are actually not great because having more coverage is a good thing! So I try to remove them if we can get 100% test coverage there.
Since it's documented elsewhere I think it's good to just remove it. If we lose coverage on any of those branches we should create test cases to add it back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've made that change; it doesn't seem to have changed the coverage.