From 135a45e9d655d56e4ebad78abe84f1cb7b5c62dc Mon Sep 17 00:00:00 2001 From: Sam Bull Date: Tue, 18 Jul 2023 21:43:36 +0100 Subject: [PATCH] Improve error messages from C parser (#7366) (#7380) (cherry picked from commit 1a48add026e310bb42b7bd38689b281f6651d127) --- CHANGES/7366.feature | 1 + aiohttp/_http_parser.pyx | 12 +++++++++--- aiohttp/http_exceptions.py | 6 ++++-- tests/test_http_exceptions.py | 22 ++++++++++------------ tests/test_http_parser.py | 31 ++++++++++++++++++++++++++----- 5 files changed, 50 insertions(+), 22 deletions(-) create mode 100644 CHANGES/7366.feature diff --git a/CHANGES/7366.feature b/CHANGES/7366.feature new file mode 100644 index 00000000000..8e38f70f898 --- /dev/null +++ b/CHANGES/7366.feature @@ -0,0 +1 @@ +Added information to C parser exceptions to show which character caused the error. -- by :user:`Dreamsorcerer` diff --git a/aiohttp/_http_parser.pyx b/aiohttp/_http_parser.pyx index bebd9894374..4f39dd0c978 100644 --- a/aiohttp/_http_parser.pyx +++ b/aiohttp/_http_parser.pyx @@ -546,7 +546,13 @@ cdef class HttpParser: ex = self._last_error self._last_error = None else: - ex = parser_error_from_errno(self._cparser) + after = cparser.llhttp_get_error_pos(self._cparser) + before = data[:after - self.py_buf.buf] + after_b = after.split(b"\n", 1)[0] + before = before.rsplit(b"\n", 1)[-1] + data = before + after_b + pointer = " " * (len(repr(before))-1) + "^" + ex = parser_error_from_errno(self._cparser, data, pointer) self._payload = None raise ex @@ -797,7 +803,7 @@ cdef int cb_on_chunk_complete(cparser.llhttp_t* parser) except -1: return 0 -cdef parser_error_from_errno(cparser.llhttp_t* parser): +cdef parser_error_from_errno(cparser.llhttp_t* parser, data, pointer): cdef cparser.llhttp_errno_t errno = cparser.llhttp_get_errno(parser) cdef bytes desc = cparser.llhttp_get_error_reason(parser) @@ -829,4 +835,4 @@ cdef parser_error_from_errno(cparser.llhttp_t* parser): else: cls = BadHttpMessage - return cls(desc.decode('latin-1')) + return cls("{}:\n\n {!r}\n {}".format(desc.decode("latin-1"), data, pointer)) diff --git a/aiohttp/http_exceptions.py b/aiohttp/http_exceptions.py index c885f80f322..b5d16ea4ec1 100644 --- a/aiohttp/http_exceptions.py +++ b/aiohttp/http_exceptions.py @@ -1,6 +1,7 @@ """Low-level http related exceptions.""" +from textwrap import indent from typing import Optional, Union from .typedefs import _CIMultiDict @@ -35,10 +36,11 @@ def __init__( self.message = message def __str__(self) -> str: - return f"{self.code}, message={self.message!r}" + msg = indent(self.message, " ") + return f"{self.code}, message:\n{msg}" def __repr__(self) -> str: - return f"<{self.__class__.__name__}: {self}>" + return f"<{self.__class__.__name__}: {self.code}, message={self.message!r}>" class BadHttpMessage(HttpProcessingError): diff --git a/tests/test_http_exceptions.py b/tests/test_http_exceptions.py index 26a5adb3bfc..29d5b91fa29 100644 --- a/tests/test_http_exceptions.py +++ b/tests/test_http_exceptions.py @@ -31,13 +31,13 @@ def test_str(self) -> None: err = http_exceptions.HttpProcessingError( code=500, message="Internal error", headers={} ) - assert str(err) == "500, message='Internal error'" + assert str(err) == "500, message:\n Internal error" def test_repr(self) -> None: err = http_exceptions.HttpProcessingError( code=500, message="Internal error", headers={} ) - assert repr(err) == ("") + assert repr(err) == ("") class TestBadHttpMessage: @@ -60,7 +60,7 @@ def test_pickle(self) -> None: def test_str(self) -> None: err = http_exceptions.BadHttpMessage(message="Bad HTTP message", headers={}) - assert str(err) == "400, message='Bad HTTP message'" + assert str(err) == "400, message:\n Bad HTTP message" def test_repr(self) -> None: err = http_exceptions.BadHttpMessage(message="Bad HTTP message", headers={}) @@ -87,9 +87,8 @@ def test_pickle(self) -> None: def test_str(self) -> None: err = http_exceptions.LineTooLong(line="spam", limit="10", actual_size="12") - assert str(err) == ( - "400, message='Got more than 10 bytes (12) " "when reading spam.'" - ) + expected = "400, message:\n Got more than 10 bytes (12) when reading spam." + assert str(err) == expected def test_repr(self) -> None: err = http_exceptions.LineTooLong(line="spam", limit="10", actual_size="12") @@ -119,25 +118,24 @@ def test_pickle(self) -> None: def test_str(self) -> None: err = http_exceptions.InvalidHeader(hdr="X-Spam") - assert str(err) == "400, message='Invalid HTTP Header: X-Spam'" + assert str(err) == "400, message:\n Invalid HTTP Header: X-Spam" def test_repr(self) -> None: err = http_exceptions.InvalidHeader(hdr="X-Spam") - assert repr(err) == ( - "" - ) + expected = "" + assert repr(err) == expected class TestBadStatusLine: def test_ctor(self) -> None: err = http_exceptions.BadStatusLine("Test") assert err.line == "Test" - assert str(err) == "400, message=\"Bad status line 'Test'\"" + assert str(err) == "400, message:\n Bad status line 'Test'" def test_ctor2(self) -> None: err = http_exceptions.BadStatusLine(b"") assert err.line == "b''" - assert str(err) == "400, message='Bad status line \"b\\'\\'\"'" + assert str(err) == "400, message:\n Bad status line \"b''\"" def test_pickle(self) -> None: err = http_exceptions.BadStatusLine("Test") diff --git a/tests/test_http_parser.py b/tests/test_http_parser.py index 950c243ef6b..ca6c32207ce 100644 --- a/tests/test_http_parser.py +++ b/tests/test_http_parser.py @@ -1,6 +1,7 @@ # Tests for aiohttp/protocol.py import asyncio +import re from typing import Any, List from unittest import mock from urllib.parse import quote @@ -118,6 +119,26 @@ def test_parse_headers(parser: Any) -> None: assert not msg.upgrade +@pytest.mark.skipif(NO_EXTENSIONS, reason="Only tests C parser.") +def test_invalid_character(loop: Any, protocol: Any, request: Any) -> None: + parser = HttpRequestParserC( + protocol, + loop, + 2**16, + max_line_size=8190, + max_field_size=8190, + ) + text = b"POST / HTTP/1.1\r\nHost: localhost:8080\r\nSet-Cookie: abc\x01def\r\n\r\n" + error_detail = re.escape( + r""": + + b'Set-Cookie: abc\x01def\r' + ^""" + ) + with pytest.raises(http_exceptions.BadHttpMessage, match=error_detail): + parser.feed_data(text) + + def test_parse(parser) -> None: text = b"GET /test HTTP/1.1\r\n\r\n" messages, upgrade, tail = parser.feed_data(text) @@ -429,7 +450,7 @@ def test_max_header_field_size(parser, size) -> None: name = b"t" * size text = b"GET /test HTTP/1.1\r\n" + name + b":data\r\n\r\n" - match = f"400, message='Got more than 8190 bytes \\({size}\\) when reading" + match = f"400, message:\n Got more than 8190 bytes \\({size}\\) when reading" with pytest.raises(http_exceptions.LineTooLong, match=match): parser.feed_data(text) @@ -457,7 +478,7 @@ def test_max_header_value_size(parser, size) -> None: name = b"t" * size text = b"GET /test HTTP/1.1\r\n" b"data:" + name + b"\r\n\r\n" - match = f"400, message='Got more than 8190 bytes \\({size}\\) when reading" + match = f"400, message:\n Got more than 8190 bytes \\({size}\\) when reading" with pytest.raises(http_exceptions.LineTooLong, match=match): parser.feed_data(text) @@ -485,7 +506,7 @@ def test_max_header_value_size_continuation(parser, size) -> None: name = b"T" * (size - 5) text = b"GET /test HTTP/1.1\r\n" b"data: test\r\n " + name + b"\r\n\r\n" - match = f"400, message='Got more than 8190 bytes \\({size}\\) when reading" + match = f"400, message:\n Got more than 8190 bytes \\({size}\\) when reading" with pytest.raises(http_exceptions.LineTooLong, match=match): parser.feed_data(text) @@ -608,7 +629,7 @@ def test_http_request_parser_bad_version(parser) -> None: @pytest.mark.parametrize("size", [40965, 8191]) def test_http_request_max_status_line(parser, size) -> None: path = b"t" * (size - 5) - match = f"400, message='Got more than 8190 bytes \\({size}\\) when reading" + match = f"400, message:\n Got more than 8190 bytes \\({size}\\) when reading" with pytest.raises(http_exceptions.LineTooLong, match=match): parser.feed_data(b"GET /path" + path + b" HTTP/1.1\r\n\r\n") @@ -651,7 +672,7 @@ def test_http_response_parser_utf8(response) -> None: @pytest.mark.parametrize("size", [40962, 8191]) def test_http_response_parser_bad_status_line_too_long(response, size) -> None: reason = b"t" * (size - 2) - match = f"400, message='Got more than 8190 bytes \\({size}\\) when reading" + match = f"400, message:\n Got more than 8190 bytes \\({size}\\) when reading" with pytest.raises(http_exceptions.LineTooLong, match=match): response.feed_data(b"HTTP/1.1 200 Ok" + reason + b"\r\n\r\n")