Repository: aaugustin/websockets (python)

Issue:
Improve error messages during HTTP parsing
For example, if the remote endpoint closes the connection without sending a HTTP response, currently this results in:

```
Traceback (most recent call last):
  File "/Users/myk/dev/websockets/src/websockets/client.py", line 90, in read_http_response
    status_code, headers = yield from read_response(self.reader)
  File "/Users/myk/dev/websockets/src/websockets/http.py", line 122, in read_response
    status_line = yield from read_line(stream)
  File "/Users/myk/dev/websockets/src/websockets/http.py", line 196, in read_line
    raise ValueError("Line without CRLF")
ValueError: Line without CRLF

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "issue-338-client.py", line 20, in <module>
    loop.run_until_complete(echo())
  File "/Users/myk/.pyenv/versions/3.7.0/lib/python3.7/asyncio/base_events.py", line 568, in run_until_complete
    return future.result()
  File "issue-338-client.py", line 11, in echo
    async with websockets.connect("ws://localhost:8765") as websocket:
  File "/Users/myk/dev/websockets/src/websockets/py35/client.py", line 2, in __aenter__
    return await self
  File "/Users/myk/dev/websockets/src/websockets/py35/client.py", line 20, in __await_impl__
    extra_headers=protocol.extra_headers,
  File "/Users/myk/dev/websockets/src/websockets/client.py", line 283, in handshake
    status_code, response_headers = yield from self.read_http_response()
  File "/Users/myk/dev/websockets/src/websockets/client.py", line 92, in read_http_response
    raise InvalidMessage("Malformed HTTP message") from exc
websockets.exceptions.InvalidMessage: Malformed HTTP message
```

which isn't enlightening.

Pull request description:
Improve HTTP parsing error messages.
Fix #494.

Patch:
diff --git a/docs/changelog.rst b/docs/changelog.rst
index c271956..92cbce5 100644
--- a/docs/changelog.rst
+++ b/docs/changelog.rst
@@ -66,6 +66,8 @@ Also:
 
 * Avoided a crash of a ``extra_headers`` callable returns ``None``.
 
+* Improved error messages when HTTP parsing fails.
+
 * Enabled readline in the interactive client.
 
 * Added type hints (:pep:`484`).
diff --git a/src/websockets/client.py b/src/websockets/client.py
index e6131ed..79b03d9 100644
--- a/src/websockets/client.py
+++ b/src/websockets/client.py
@@ -98,8 +98,8 @@ class WebSocketClientProtocol(WebSocketCommonProtocol):
         """
         try:
             status_code, reason, headers = await read_response(self.reader)
-        except ValueError as exc:
-            raise InvalidMessage("Malformed HTTP message") from exc
+        except Exception as exc:
+            raise InvalidMessage("did not receive a valid HTTP response") from exc
 
         logger.debug("%s < HTTP/1.1 %d %s", self.side, status_code, reason)
         logger.debug("%s < %r", self.side, headers)
diff --git a/src/websockets/http.py b/src/websockets/http.py
index f0c5806..6fbe5eb 100644
--- a/src/websockets/http.py
+++ b/src/websockets/http.py
@@ -39,6 +39,21 @@ MAX_LINE = 4096
 USER_AGENT = f"Python/{sys.version[:3]} websockets/{websockets_version}"
 
 
+class SecurityError(ValueError):
+    """
+    HTTP request or response exceeds security limits.
+
+    """
+
+
+def d(value: bytes) -> str:
+    """
+    Decode a bytestring for interpolating into an error message.
+
+    """
+    return value.decode(errors="backslashreplace")
+
+
 # See https://tools.ietf.org/html/rfc7230#appendix-B.
 
 # Regex for validating header names.
@@ -85,15 +100,20 @@ async def read_request(stream: asyncio.StreamReader) -> Tuple[str, "Headers"]:
     # version and because path isn't checked. Since WebSocket software tends
     # to implement HTTP/1.1 strictly, there's little need for lenient parsing.
 
-    request_line = await read_line(stream)
+    try:
+        request_line = await read_line(stream)
+    except EOFError as exc:
+        raise EOFError("connection closed while reading HTTP request line") from exc
 
-    # This may raise "ValueError: not enough values to unpack"
-    method, raw_path, version = request_line.split(b" ", 2)
+    try:
+        method, raw_path, version = request_line.split(b" ", 2)
+    except ValueError:  # not enough values to unpack (expected 3, got 1-2)
+        raise ValueError(f"invalid HTTP request line: {d(request_line)}") from None
 
     if method != b"GET":
-        raise ValueError("Unsupported HTTP method: %r" % method)
+        raise ValueError(f"unsupported HTTP method: {d(method)}")
     if version != b"HTTP/1.1":
-        raise ValueError("Unsupported HTTP version: %r" % version)
+        raise ValueError(f"unsupported HTTP version: {d(version)}")
     path = raw_path.decode("ascii", "surrogateescape")
 
     headers = await read_headers(stream)
@@ -125,19 +145,26 @@ async def read_response(stream: asyncio.StreamReader) -> Tuple[int, str, "Header
     # As in read_request, parsing is simple because a fixed value is expected
     # for version, status_code is a 3-digit number, and reason can be ignored.
 
-    status_line = await read_line(stream)
+    try:
+        status_line = await read_line(stream)
+    except EOFError as exc:
+        raise EOFError("connection closed while reading HTTP status line") from exc
 
-    # This may raise "ValueError: not enough values to unpack"
-    version, raw_status_code, raw_reason = status_line.split(b" ", 2)
+    try:
+        version, raw_status_code, raw_reason = status_line.split(b" ", 2)
+    except ValueError:  # not enough values to unpack (expected 3, got 1-2)
+        raise ValueError(f"invalid HTTP status line: {d(status_line)}") from None
 
     if version != b"HTTP/1.1":
-        raise ValueError("Unsupported HTTP version: %r" % version)
-    # This may raise "ValueError: invalid literal for int() with base 10"
-    status_code = int(raw_status_code)
+        raise ValueError(f"unsupported HTTP version: {d(version)}")
+    try:
+        status_code = int(raw_status_code)
+    except ValueError:  # invalid literal for int() with base 10
+        raise ValueError(f"invalid HTTP status code: {d(raw_status_code)}") from None
     if not 100 <= status_code < 1000:
-        raise ValueError("Unsupported HTTP status code: %d" % status_code)
+        raise ValueError(f"unsupported HTTP status code: {d(raw_status_code)}")
     if not _value_re.fullmatch(raw_reason):
-        raise ValueError("Invalid HTTP reason phrase: %r" % raw_reason)
+        raise ValueError(f"invalid HTTP reason phrase: {d(raw_reason)}")
     reason = raw_reason.decode()
 
     headers = await read_headers(stream)
@@ -162,24 +189,29 @@ async def read_headers(stream: asyncio.StreamReader) -> "Headers":
 
     headers = Headers()
     for _ in range(MAX_HEADERS + 1):
-        line = await read_line(stream)
+        try:
+            line = await read_line(stream)
+        except EOFError as exc:
+            raise EOFError("connection closed while reading HTTP headers") from exc
         if line == b"":
             break
 
-        # This may raise "ValueError: not enough values to unpack"
-        raw_name, raw_value = line.split(b":", 1)
+        try:
+            raw_name, raw_value = line.split(b":", 1)
+        except ValueError:  # not enough values to unpack (expected 2, got 1)
+            raise ValueError(f"invalid HTTP header line: {d(line)}") from None
         if not _token_re.fullmatch(raw_name):
-            raise ValueError("Invalid HTTP header name: %r" % raw_name)
+            raise ValueError(f"invalid HTTP header name: {d(raw_name)}")
         raw_value = raw_value.strip(b" \t")
         if not _value_re.fullmatch(raw_value):
-            raise ValueError("Invalid HTTP header value: %r" % raw_value)
+            raise ValueError(f"invalid HTTP header value: {d(raw_value)}")
 
         name = raw_name.decode("ascii")  # guaranteed to be ASCII at this point
         value = raw_value.decode("ascii", "surrogateescape")
         headers[name] = value
 
     else:
-        raise ValueError("Too many HTTP headers")
+        raise SecurityError("too many HTTP headers")
 
     return headers
 
@@ -197,10 +229,10 @@ async def read_line(stream: asyncio.StreamReader) -> bytes:
     line = await stream.readline()
     # Security: this guarantees header values are small (hard-coded = 4 KiB)
     if len(line) > MAX_LINE:
-        raise ValueError("Line too long")
+        raise SecurityError("line too long")
     # Not mandatory but safe - https://tools.ietf.org/html/rfc7230#section-3.5
     if not line.endswith(b"\r\n"):
-        raise ValueError("Line without CRLF")
+        raise EOFError("line without CRLF")
     return line[:-2]
 
 
diff --git a/src/websockets/server.py b/src/websockets/server.py
index 9882eab..c8eb463 100644
--- a/src/websockets/server.py
+++ b/src/websockets/server.py
@@ -227,8 +227,8 @@ class WebSocketServerProtocol(WebSocketCommonProtocol):
         """
         try:
             path, headers = await read_request(self.reader)
-        except ValueError as exc:
-            raise InvalidMessage("Malformed HTTP message") from exc
+        except Exception as exc:
+            raise InvalidMessage("did not receive a valid HTTP request") from exc
 
         logger.debug("%s < GET %s HTTP/1.1", self.side, path)
         logger.debug("%s < %r", self.side, headers)


Test patch:
diff --git a/tests/test_http.py b/tests/test_http.py
index 60cdb9a..8ba1d19 100644
--- a/tests/test_http.py
+++ b/tests/test_http.py
@@ -29,6 +29,33 @@ class HTTPAsyncTests(AsyncioTestCase):
         self.assertEqual(path, "/chat")
         self.assertEqual(headers["Upgrade"], "websocket")
 
+    def test_read_request_empty(self):
+        self.stream.feed_eof()
+        with self.assertRaisesRegex(
+            EOFError, "connection closed while reading HTTP request line"
+        ):
+            self.loop.run_until_complete(read_request(self.stream))
+
+    def test_read_request_invalid_request_line(self):
+        self.stream.feed_data(b"GET /\r\n\r\n")
+        with self.assertRaisesRegex(ValueError, "invalid HTTP request line: GET /"):
+            self.loop.run_until_complete(read_request(self.stream))
+
+    def test_read_request_unsupported_method(self):
+        self.stream.feed_data(b"OPTIONS * HTTP/1.1\r\n\r\n")
+        with self.assertRaisesRegex(ValueError, "unsupported HTTP method: OPTIONS"):
+            self.loop.run_until_complete(read_request(self.stream))
+
+    def test_read_request_unsupported_version(self):
+        self.stream.feed_data(b"GET /chat HTTP/1.0\r\n\r\n")
+        with self.assertRaisesRegex(ValueError, "unsupported HTTP version: HTTP/1.0"):
+            self.loop.run_until_complete(read_request(self.stream))
+
+    def test_read_request_invalid_header(self):
+        self.stream.feed_data(b"GET /chat HTTP/1.1\r\nOops\r\n")
+        with self.assertRaisesRegex(ValueError, "invalid HTTP header line: Oops"):
+            self.loop.run_until_complete(read_request(self.stream))
+
     def test_read_response(self):
         # Example from the protocol overview in RFC 6455
         self.stream.feed_data(
@@ -46,29 +73,41 @@ class HTTPAsyncTests(AsyncioTestCase):
         self.assertEqual(reason, "Switching Protocols")
         self.assertEqual(headers["Upgrade"], "websocket")
 
-    def test_request_method(self):
-        self.stream.feed_data(b"OPTIONS * HTTP/1.1\r\n\r\n")
-        with self.assertRaises(ValueError):
-            self.loop.run_until_complete(read_request(self.stream))
+    def test_read_response_empty(self):
+        self.stream.feed_eof()
+        with self.assertRaisesRegex(
+            EOFError, "connection closed while reading HTTP status line"
+        ):
+            self.loop.run_until_complete(read_response(self.stream))
 
-    def test_request_version(self):
-        self.stream.feed_data(b"GET /chat HTTP/1.0\r\n\r\n")
-        with self.assertRaises(ValueError):
-            self.loop.run_until_complete(read_request(self.stream))
+    def test_read_request_invalid_status_line(self):
+        self.stream.feed_data(b"Hello!\r\n")
+        with self.assertRaisesRegex(ValueError, "invalid HTTP status line: Hello!"):
+            self.loop.run_until_complete(read_response(self.stream))
 
-    def test_response_version(self):
+    def test_read_response_unsupported_version(self):
         self.stream.feed_data(b"HTTP/1.0 400 Bad Request\r\n\r\n")
-        with self.assertRaises(ValueError):
+        with self.assertRaisesRegex(ValueError, "unsupported HTTP version: HTTP/1.0"):
             self.loop.run_until_complete(read_response(self.stream))
 
-    def test_response_status(self):
+    def test_read_response_invalid_status(self):
+        self.stream.feed_data(b"HTTP/1.1 OMG WTF\r\n\r\n")
+        with self.assertRaisesRegex(ValueError, "invalid HTTP status code: OMG"):
+            self.loop.run_until_complete(read_response(self.stream))
+
+    def test_read_response_unsupported_status(self):
         self.stream.feed_data(b"HTTP/1.1 007 My name is Bond\r\n\r\n")
-        with self.assertRaises(ValueError):
+        with self.assertRaisesRegex(ValueError, "unsupported HTTP status code: 007"):
             self.loop.run_until_complete(read_response(self.stream))
 
-    def test_response_reason(self):
+    def test_read_response_invalid_reason(self):
         self.stream.feed_data(b"HTTP/1.1 200 \x7f\r\n\r\n")
-        with self.assertRaises(ValueError):
+        with self.assertRaisesRegex(ValueError, "invalid HTTP reason phrase: \\x7f"):
+            self.loop.run_until_complete(read_response(self.stream))
+
+    def test_read_response_invalid_header(self):
+        self.stream.feed_data(b"HTTP/1.1 500 Internal Server Error\r\nOops\r\n")
+        with self.assertRaisesRegex(ValueError, "invalid HTTP header line: Oops"):
             self.loop.run_until_complete(read_response(self.stream))
 
     def test_header_name(self):
@@ -94,7 +133,7 @@ class HTTPAsyncTests(AsyncioTestCase):
 
     def test_line_ending(self):
         self.stream.feed_data(b"foo: bar\n\n")
-        with self.assertRaises(ValueError):
+        with self.assertRaises(EOFError):
             self.loop.run_until_complete(read_headers(self.stream))