Improve error messages from C parser

.gitmodules

@@ -1,4 +1,4 @@
 [submodule "vendor/llhttp"]
     path = vendor/llhttp
     url = https://github.com/nodejs/llhttp.git
-    branch = v8.1.1
+    branch = v8.x

CHANGES/7366.feature

@@ -0,0 +1 @@
+Added information to C parser exceptions to show which character caused the error. -- by :user:`Dreamsorcerer`

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))

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):

tests/test_http_exceptions.py

@@ -32,13 +32,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:
@@ -61,7 +61,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={})
@@ -88,9 +88,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")
@@ -120,25 +119,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")

tests/test_http_parser.py

@@ -2,6 +2,7 @@
 # Tests for aiohttp/protocol.py
 
 import asyncio
+import re
 from typing import Any, List
 from unittest import mock
 from urllib.parse import quote
@@ -117,6 +118,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
@@ -436,7 +457,7 @@ def test_max_header_field_size(parser: Any, size: Any) -> 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)
 
@@ -464,7 +485,7 @@ def test_max_header_value_size(parser: Any, size: Any) -> 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)
 
@@ -492,7 +513,7 @@ def test_max_header_value_size_continuation(parser: Any, size: Any) -> 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)
 
@@ -615,7 +636,7 @@ def test_http_request_parser_bad_version(parser: Any) -> None:
 @pytest.mark.parametrize("size", [40965, 8191])
 def test_http_request_max_status_line(parser: Any, size: Any) -> 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")
 
@@ -660,7 +681,7 @@ def test_http_response_parser_bad_status_line_too_long(
     response: Any, size: Any
 ) -> 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")
 
@@ -694,6 +715,7 @@ def test_http_response_parser_bad(response: Any) -> None:
         response.feed_data(b"HTT/1\r\n\r\n")
 
 
+@pytest.mark.skipif(not NO_EXTENSIONS, reason="Behaviour has changed in C parser")
 def test_http_response_parser_code_under_100(response: Any) -> None:
     msg = response.feed_data(b"HTTP/1.1 99 test\r\n\r\n")[0][0][0]
     assert msg.code == 99

vendor/README

@@ -0,0 +1,19 @@
+LLHTTP
+======
+
+To build the llhttp parser, first get/update the submodule:
+
+    git submodule update --init --recursive
+
+Then build llhttp:
+
+    cd vendor/llhttp/
+    npm install
+    make
+
+Then build our parser:
+
+    cd -
+    make cythonize
+
+Then you can build or install it with ``python -m build`` or ``pip install .``