diff --git a/httptools/parser/parser.pyx b/httptools/parser/parser.pyx index 2fa5026..3fb53ef 100644 --- a/httptools/parser/parser.pyx +++ b/httptools/parser/parser.pyx @@ -128,7 +128,7 @@ cdef class HttpParser: self._maybe_call_on_header() if self._proto_on_headers_complete is not None: - self._proto_on_headers_complete() + return self._proto_on_headers_complete() cdef _on_chunk_header(self): if (self._current_header_value is not None or @@ -352,13 +352,22 @@ cdef int cb_on_header_value(cparser.llhttp_t* parser, cdef int cb_on_headers_complete(cparser.llhttp_t* parser) except -1: cdef HttpParser pyparser = parser.data + cdef object result try: - pyparser._on_headers_complete() + result = pyparser._on_headers_complete() except BaseException as ex: pyparser._last_error = ex return -1 else: - if pyparser._cparser.upgrade: + # A protocol can veto an upgrade it doesn't want to honor (e.g. an + # `Upgrade: h2c` it plans to ignore) by returning False from its own + # `on_headers_complete`. Without this, llhttp always pauses parsing + # right here whenever the client asked for any upgrade, even one the + # application never intends to act on, leaving the rest of the + # message (its body, on a request that still carries one) stuck + # behind an HttpParserUpgrade the caller has no clean way to resume + # from, since the leftover bytes aren't a fresh message of their own. + if pyparser._cparser.upgrade and result is not False: return 1 else: return 0 diff --git a/tests/test_parser.py b/tests/test_parser.py index 96dc6f0..227f24f 100644 --- a/tests/test_parser.py +++ b/tests/test_parser.py @@ -439,6 +439,79 @@ def on_message_complete(self): else: self.fail('HttpParserUpgrade was not raised') + def test_parser_request_upgrade_veto_keeps_body(self): + # A request can carry an Upgrade the receiving application has no + # intention of honoring (e.g. `Upgrade: h2c`, which plenty of clients + # send speculatively). RFC 7230 6.7 lets a server just ignore that + # and process the request as an ordinary HTTP/1.1 message, but until + # now on_headers_complete had no way to say so: llhttp always paused + # right there, and any body the request had (declared via + # Content-Length or Transfer-Encoding) was never delivered. + # Returning False from on_headers_complete opts out of the pause and + # lets the rest of the message parse normally. + request = ( + b'POST / HTTP/1.1\r\n' + b'Host: example.com\r\n' + b'Upgrade: h2c\r\n' + b'Connection: Upgrade, HTTP2-Settings\r\n' + b'Transfer-Encoding: chunked\r\n' + b'\r\n' + b'3\r\nabc\r\n0\r\n\r\n' + ) + + class Protocol: + def __init__(self): + self.upgrade = None + self.body = b'' + self.message_complete = False + + def on_header(self, name, value): + if name.lower() == b'upgrade': + self.upgrade = value + + def on_body(self, data): + self.body += data + + def on_message_complete(self): + self.message_complete = True + + def on_headers_complete(self): + if self.upgrade is not None and self.upgrade.lower() == b'h2c': + return False + + protocol = Protocol() + parser = httptools.HttpRequestParser(protocol) + try: + parser.feed_data(request) + except httptools.HttpParserUpgrade: + # Still raised once the (now fully-parsed) message ends, so the + # application can act on the upgrade request afterwards if it + # wants to - vetoing the pause doesn't erase that it happened. + pass + else: + self.fail('HttpParserUpgrade was not raised') + + self.assertTrue(protocol.message_complete) + self.assertEqual(protocol.body, b'abc') + + def test_parser_request_upgrade_not_vetoed_still_pauses(self): + # An upgrade the application doesn't veto (the common case, e.g. a + # real WebSocket handshake) must keep behaving exactly as before: + # pause immediately at headers-complete, without waiting on a body. + m = mock.Mock() + m.on_headers_complete.return_value = None + p = httptools.HttpRequestParser(m) + + try: + p.feed_data(UPGRADE_REQUEST1) + except httptools.HttpParserUpgrade as ex: + offset = ex.args[0] + else: + self.fail('HttpParserUpgrade was not raised') + + self.assertEqual(UPGRADE_REQUEST1[offset:], b'Hot diggity dogg') + self.assertFalse(m.on_body.called) + def test_parser_request_error_in_on_header(self): class Error(Exception): pass