From 0796637bdff1c695812d185593facdaed3663958 Mon Sep 17 00:00:00 2001 From: Tim Bell Date: Thu, 13 Sep 2018 03:36:37 +1000 Subject: [PATCH 1/8] Add test for spurious header warnings; #1438 --- test/with_dummyserver/test_socketlevel.py | 29 +++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/test/with_dummyserver/test_socketlevel.py b/test/with_dummyserver/test_socketlevel.py index ec2f01d227..b1646b9d9d 100644 --- a/test/with_dummyserver/test_socketlevel.py +++ b/test/with_dummyserver/test_socketlevel.py @@ -1328,6 +1328,35 @@ def test_header_without_colon_or_value(self): ]) +@pytest.mark.skipif( + issubclass(httplib.HTTPMessage, MimeToolMessage), + reason='Header parsing errors not available' +) +class TestOkayHeaders(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): def test_chunked_head_response_does_not_hang(self): self.start_response_handler( From 7e78f7990dd81182dff6e0022743cf0b76b988b0 Mon Sep 17 00:00:00 2001 From: Tim Bell Date: Thu, 13 Sep 2018 03:37:35 +1000 Subject: [PATCH 2/8] Simplify assert_header_parsing; fixes #1438 --- src/urllib3/exceptions.py | 4 ++-- src/urllib3/util/response.py | 10 ++-------- test/test_exceptions.py | 3 +-- 3 files changed, 5 insertions(+), 12 deletions(-) diff --git a/src/urllib3/exceptions.py b/src/urllib3/exceptions.py index 7bbaa9871f..383a2aee11 100644 --- a/src/urllib3/exceptions.py +++ b/src/urllib3/exceptions.py @@ -236,8 +236,8 @@ def __init__(self, scheme): class HeaderParsingError(HTTPError): "Raised by assert_header_parsing, but we convert it to a log.warning statement." - def __init__(self, defects, unparsed_data): - message = '%s, unparsed data: %r' % (defects or 'Unknown', unparsed_data) + def __init__(self, defects): + message = '%s' % (defects or 'Unknown') super(HeaderParsingError, self).__init__(message) diff --git a/src/urllib3/util/response.py b/src/urllib3/util/response.py index 67cf730ab0..d26df46e98 100644 --- a/src/urllib3/util/response.py +++ b/src/urllib3/util/response.py @@ -37,7 +37,6 @@ def is_fp_closed(obj): def assert_header_parsing(headers): """ - Asserts whether all headers have been successfully parsed. Extracts encountered errors from the result of parsing headers. Only works on Python 3. @@ -56,14 +55,9 @@ def assert_header_parsing(headers): type(headers))) defects = getattr(headers, 'defects', None) - get_payload = getattr(headers, 'get_payload', None) - unparsed_data = None - if get_payload: # Platform-specific: Python 3. - unparsed_data = get_payload() - - if defects or unparsed_data: - raise HeaderParsingError(defects=defects, unparsed_data=unparsed_data) + if defects: + raise HeaderParsingError(defects=defects) def is_response_to_head(response): diff --git a/test/test_exceptions.py b/test/test_exceptions.py index 8e8c4e1ec7..6b0bdc01c3 100644 --- a/test/test_exceptions.py +++ b/test/test_exceptions.py @@ -32,7 +32,6 @@ def test_exceptions(self, exception): class TestFormat(object): def test_header_parsing_errors(self): - hpe = HeaderParsingError('defects', 'unparsed_data') + hpe = HeaderParsingError('defects') assert 'defects' in str(hpe) - assert 'unparsed_data' in str(hpe) From 54727008a47dccb65f2c031b42f80b4bc4e9dfd2 Mon Sep 17 00:00:00 2001 From: Tim Bell Date: Thu, 13 Sep 2018 03:37:35 +1000 Subject: [PATCH 3/8] Revert "Simplify assert_header_parsing; fixes #1438" This reverts commit 7e78f7990dd81182dff6e0022743cf0b76b988b0. --- src/urllib3/exceptions.py | 4 ++-- src/urllib3/util/response.py | 10 ++++++++-- test/test_exceptions.py | 3 ++- 3 files changed, 12 insertions(+), 5 deletions(-) diff --git a/src/urllib3/exceptions.py b/src/urllib3/exceptions.py index 383a2aee11..7bbaa9871f 100644 --- a/src/urllib3/exceptions.py +++ b/src/urllib3/exceptions.py @@ -236,8 +236,8 @@ def __init__(self, scheme): class HeaderParsingError(HTTPError): "Raised by assert_header_parsing, but we convert it to a log.warning statement." - def __init__(self, defects): - message = '%s' % (defects or 'Unknown') + def __init__(self, defects, unparsed_data): + message = '%s, unparsed data: %r' % (defects or 'Unknown', unparsed_data) super(HeaderParsingError, self).__init__(message) diff --git a/src/urllib3/util/response.py b/src/urllib3/util/response.py index d26df46e98..67cf730ab0 100644 --- a/src/urllib3/util/response.py +++ b/src/urllib3/util/response.py @@ -37,6 +37,7 @@ def is_fp_closed(obj): def assert_header_parsing(headers): """ + Asserts whether all headers have been successfully parsed. Extracts encountered errors from the result of parsing headers. Only works on Python 3. @@ -55,9 +56,14 @@ def assert_header_parsing(headers): type(headers))) defects = getattr(headers, 'defects', None) + get_payload = getattr(headers, 'get_payload', None) - if defects: - raise HeaderParsingError(defects=defects) + unparsed_data = None + if get_payload: # Platform-specific: Python 3. + unparsed_data = get_payload() + + if defects or unparsed_data: + raise HeaderParsingError(defects=defects, unparsed_data=unparsed_data) def is_response_to_head(response): diff --git a/test/test_exceptions.py b/test/test_exceptions.py index 6b0bdc01c3..8e8c4e1ec7 100644 --- a/test/test_exceptions.py +++ b/test/test_exceptions.py @@ -32,6 +32,7 @@ def test_exceptions(self, exception): class TestFormat(object): def test_header_parsing_errors(self): - hpe = HeaderParsingError('defects') + hpe = HeaderParsingError('defects', 'unparsed_data') assert 'defects' in str(hpe) + assert 'unparsed_data' in str(hpe) From 129a895d57b7604719c6493bf6d8836f53331827 Mon Sep 17 00:00:00 2001 From: Tim Bell Date: Thu, 13 Sep 2018 10:18:44 +1000 Subject: [PATCH 4/8] Fix header test cases to remove unintended header separator b'\r\n'.join(headers) means the headers passed into _test_broken_header_parsing don't need to be terminated with b'\r\n'; however the final header needs b'\r\n\r\n' appended --- test/with_dummyserver/test_socketlevel.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/test/with_dummyserver/test_socketlevel.py b/test/with_dummyserver/test_socketlevel.py index b1646b9d9d..b89b13de08 100644 --- a/test/with_dummyserver/test_socketlevel.py +++ b/test/with_dummyserver/test_socketlevel.py @@ -1294,7 +1294,7 @@ def _test_broken_header_parsing(self, headers): 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) @@ -1311,14 +1311,14 @@ def _test_broken_header_parsing(self, 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): From 7b134572f7b706b9550b469ed9fbab123bf8bbe9 Mon Sep 17 00:00:00 2001 From: Tim Bell Date: Thu, 13 Sep 2018 10:27:07 +1000 Subject: [PATCH 5/8] Improve test to check for unparsed_data value --- test/with_dummyserver/test_socketlevel.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/test/with_dummyserver/test_socketlevel.py b/test/with_dummyserver/test_socketlevel.py index b89b13de08..952c742918 100644 --- a/test/with_dummyserver/test_socketlevel.py +++ b/test/with_dummyserver/test_socketlevel.py @@ -1289,7 +1289,7 @@ 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): self.start_response_handler(( b'HTTP/1.1 200 OK\r\n' b'Content-Length: 0\r\n' @@ -1306,7 +1306,8 @@ 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): @@ -1325,7 +1326,7 @@ def test_header_without_colon_or_value(self): self._test_broken_header_parsing([ b'Broken Header', b'Another: Header', - ]) + ], 'Broken Header') @pytest.mark.skipif( From 5c0f06c164c655e254c1b19fda18ac678fbf35c9 Mon Sep 17 00:00:00 2001 From: Tim Bell Date: Thu, 13 Sep 2018 10:29:23 +1000 Subject: [PATCH 6/8] Rename header parsing test and fix so it passes; fixes #1438 --- src/urllib3/util/response.py | 8 +++++++- test/with_dummyserver/test_socketlevel.py | 6 +----- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/urllib3/util/response.py b/src/urllib3/util/response.py index 67cf730ab0..e55848af65 100644 --- a/src/urllib3/util/response.py +++ b/src/urllib3/util/response.py @@ -60,7 +60,13 @@ def assert_header_parsing(headers): unparsed_data = None if get_payload: # Platform-specific: Python 3. - unparsed_data = get_payload() + # get_payload is actually email.message.Message.get_payload; + # we're only interested in the result if it's not a multipart message + if not headers.is_multipart(): + payload = get_payload() + + if isinstance(payload, (bytes, str)): + unparsed_data = payload if defects or unparsed_data: raise HeaderParsingError(defects=defects, unparsed_data=unparsed_data) diff --git a/test/with_dummyserver/test_socketlevel.py b/test/with_dummyserver/test_socketlevel.py index 952c742918..8f2cf9e957 100644 --- a/test/with_dummyserver/test_socketlevel.py +++ b/test/with_dummyserver/test_socketlevel.py @@ -1329,11 +1329,7 @@ def test_header_without_colon_or_value(self): ], 'Broken Header') -@pytest.mark.skipif( - issubclass(httplib.HTTPMessage, MimeToolMessage), - reason='Header parsing errors not available' -) -class TestOkayHeaders(SocketDummyServerTestCase): +class TestHeaderParsingContentType(SocketDummyServerTestCase): def _test_okay_header_parsing(self, header): self.start_response_handler(( From 76491114b4b98bf4e892202a6e76d9ac1f6060bd Mon Sep 17 00:00:00 2001 From: Tim Bell Date: Thu, 13 Sep 2018 10:36:03 +1000 Subject: [PATCH 7/8] Updated changelog --- CHANGES.rst | 3 +++ CONTRIBUTORS.txt | 3 +++ 2 files changed, 6 insertions(+) diff --git a/CHANGES.rst b/CHANGES.rst index 3d19f8ec13..06e20c4e9f 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -11,6 +11,9 @@ dev (master) * Drop support for EOL Python 2.6 (Pull #1429 and Pull #1430) +* Fixed bug where responses with header Content-Type: message/* erroneously + raised HeaderParsingError, resulting in a warning being logged. (Pull #1439) + * ... [Short description of non-trivial change.] (Issue #) diff --git a/CONTRIBUTORS.txt b/CONTRIBUTORS.txt index 2413592138..72b65e8df4 100644 --- a/CONTRIBUTORS.txt +++ b/CONTRIBUTORS.txt @@ -266,5 +266,8 @@ In chronological order: * Hugo van Kemenade * Drop support for EOL Python 2.6 +* Tim Bell + * Bugfix for responses with Content-Type: message/* logging warnings + * [Your name or handle] <[email or website]> * [Brief summary of your changes] From f22b64b5c840f5c10e32f5fb16e524a5b0761108 Mon Sep 17 00:00:00 2001 From: Tim Bell Date: Tue, 18 Sep 2018 13:02:09 +1000 Subject: [PATCH 8/8] Remove platform-specific marker --- src/urllib3/util/response.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/urllib3/util/response.py b/src/urllib3/util/response.py index e55848af65..3d5486485a 100644 --- a/src/urllib3/util/response.py +++ b/src/urllib3/util/response.py @@ -59,7 +59,7 @@ def assert_header_parsing(headers): get_payload = getattr(headers, 'get_payload', None) unparsed_data = None - if get_payload: # Platform-specific: Python 3. + if get_payload: # get_payload is actually email.message.Message.get_payload; # we're only interested in the result if it's not a multipart message if not headers.is_multipart():