[PR aio-libs#7366/1a48add0 backport][3.9] Improve error messages from…

… C parser (aio-libs#7379) **This is a backport of PR aio-libs#7366 as merged into master (1a48add).** This adds information to the error messages to show what causes the error. Exception output looks like: ``` aiohttp.http_exceptions.BadHttpMessage: 400, message: Invalid header value char: b'Set-Cookie: abc\x01def\r' ^ ``` Co-authored-by: Sam Bull <[email protected]>
jgosmann · Jul 18, 2023 · 15d1d92 · 15d1d92
1 parent e7fe416
commit 15d1d92
Show file tree

Hide file tree

Showing 5 changed files with 50 additions and 22 deletions.
diff --git a/CHANGES/7366.feature 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
@@ -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 - <char*>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
@@ -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
@@ -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) == ("<HttpProcessingError: 500, " "message='Internal error'>")
+        assert repr(err) == ("<HttpProcessingError: 500, message='Internal error'>")
 
 
 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) == (
-            "<InvalidHeader: 400, " "message='Invalid HTTP Header: X-Spam'>"
-        )
+        expected = "<InvalidHeader: 400, message='Invalid HTTP Header: X-Spam'>"
+        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
@@ -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_headers_longline(parser: Any) -> None:
     invalid_unicode_byte = b"\xd9"
     header_name = b"Test" + invalid_unicode_byte + b"Header" + b"A" * 8192
@@ -437,7 +458,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)
 
@@ -465,7 +486,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)
 
@@ -493,7 +514,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)
 
@@ -616,7 +637,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")
 
@@ -659,7 +680,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")
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1 @@
		Added information to C parser exceptions to show which character caused the error. -- by :user:`Dreamsorcerer`