Repository: tornadoweb/tornado

Issue:
Empty reply on malformed HTTP request
When an incorrect HTTP request is performed, tornado makes no reply (log + return in code). I suggest returning an 400 error code.

For example, when working behind an nginx proxy-server, it causes a 502 error code to the end-user and a record in nginx's error log, which is incorrect (should be 400 and a warning).


Discussion:
jehiah: RFR @bdarnell 

I've also done some preliminary validation that this solves the original issue I ran into.
jehiah: 🤦‍♂️  w/r/t executable bit. It seems my editor was doing that because those files begin with `#!/usr/bin/env python`. Is there a reason for/value in that if they should not be executable?
bdarnell: Thanks, just one more thing to figure out: the large-body chunked test cases are failing for some reason on windows (the fact that this happens on 2 of the 4 configurations on appveyor doesn't mean anything - we only run the full test suite on 2 configurations because tornado on windows is so slow). The `ExpectLog` is passing so the correct exception is getting raised. Maybe closing immediately after `yield stream.write` is not actually waiting for the outgoing data to be sent on windows because there is also unread data in the incoming socket buffer. This isn't too important so maybe these tests should just accept both 400 and 599 response codes. 

There's no reason for the `#!/usr/bin/env python` line to be present in files that are not executable; this line could just be removed.
jehiah: @bdarnell updated to address flaky tests; if this looks good i'll squash/cleanup commits.

Patch:
diff --git a/tornado/http1connection.py b/tornado/http1connection.py
index c6d3e336..6069e027 100644
--- a/tornado/http1connection.py
+++ b/tornado/http1connection.py
@@ -250,6 +250,8 @@ class HTTP1Connection(httputil.HTTPConnection):
         except httputil.HTTPInputError as e:
             gen_log.info("Malformed HTTP message from %s: %s",
                          self.context, e)
+            if not self.is_client:
+                yield self.stream.write(b'HTTP/1.1 400 Bad Request\r\n\r\n')
             self.close()
             raise gen.Return(False)
         finally:
diff --git a/tornado/httputil.py b/tornado/httputil.py
old mode 100644
new mode 100755
index 818ea914..5b87ce61
--- a/tornado/httputil.py
+++ b/tornado/httputil.py
@@ -829,6 +829,8 @@ def parse_request_start_line(line):
     try:
         method, path, version = line.split(" ")
     except ValueError:
+        # https://tools.ietf.org/html/rfc7230#section-3.1.1
+        # invalid request-line SHOULD respond with a 400 (Bad Request)
         raise HTTPInputError("Malformed HTTP request line")
     if not re.match(r"^HTTP/1\.[0-9]$", version):
         raise HTTPInputError(


Test patch:
diff --git a/tornado/test/httpserver_test.py b/tornado/test/httpserver_test.py
index f5f91a9d..4169a43a 100644
--- a/tornado/test/httpserver_test.py
+++ b/tornado/test/httpserver_test.py
@@ -29,18 +29,19 @@ from io import BytesIO
 
 def read_stream_body(stream, callback):
     """Reads an HTTP response from `stream` and runs callback with its
-    headers and body."""
+    start_line, headers and body."""
     chunks = []
 
     class Delegate(HTTPMessageDelegate):
         def headers_received(self, start_line, headers):
             self.headers = headers
+            self.start_line = start_line
 
         def data_received(self, chunk):
             chunks.append(chunk)
 
         def finish(self):
-            callback((self.headers, b''.join(chunks)))
+            callback((self.start_line, self.headers, b''.join(chunks)))
     conn = HTTP1Connection(stream, True)
     conn.read_response(Delegate())
 
@@ -217,7 +218,7 @@ class HTTPConnectionTest(AsyncHTTPTestCase):
                              [utf8("Content-Length: %d" % len(body))]) +
                 newline + newline + body)
             read_stream_body(stream, self.stop)
-            headers, body = self.wait()
+            start_line, headers, body = self.wait()
             return body
 
     def test_multipart_form(self):
@@ -406,7 +407,15 @@ class HTTPServerRawTest(AsyncHTTPTestCase):
         self.io_loop.add_timeout(datetime.timedelta(seconds=0.001), self.stop)
         self.wait()
 
-    def test_malformed_first_line(self):
+    def test_malformed_first_line_response(self):
+        self.stream.write(b'asdf\r\n\r\n')
+        read_stream_body(self.stream, self.stop)
+        start_line, headers, response = self.wait()
+        self.assertEqual('HTTP/1.1', start_line.version)
+        self.assertEqual(400, start_line.code)
+        self.assertEqual('Bad Request', start_line.reason)
+
+    def test_malformed_first_line_log(self):
         with ExpectLog(gen_log, '.*Malformed HTTP request line'):
             self.stream.write(b'asdf\r\n\r\n')
             # TODO: need an async version of ExpectLog so we don't need
@@ -438,7 +447,7 @@ bar
 
 """.replace(b"\n", b"\r\n"))
         read_stream_body(self.stream, self.stop)
-        headers, response = self.wait()
+        start_line, headers, response = self.wait()
         self.assertEqual(json_decode(response), {u'foo': [u'bar']})
 
     def test_chunked_request_uppercase(self):
@@ -457,7 +466,7 @@ bar
 
 """.replace(b"\n", b"\r\n"))
         read_stream_body(self.stream, self.stop)
-        headers, response = self.wait()
+        start_line, headers, response = self.wait()
         self.assertEqual(json_decode(response), {u'foo': [u'bar']})
 
     def test_invalid_content_length(self):
@@ -627,7 +636,7 @@ class UnixSocketTest(AsyncTestCase):
             self.stream.write(b"garbage\r\n\r\n")
             self.stream.read_until_close(self.stop)
             response = self.wait()
-        self.assertEqual(response, b"")
+        self.assertEqual(response, b"HTTP/1.1 400 Bad Request\r\n\r\n")
 
 
 class KeepAliveTest(AsyncHTTPTestCase):
@@ -1036,24 +1045,26 @@ class BodyLimitsTest(AsyncHTTPTestCase):
     def test_large_body_buffered(self):
         with ExpectLog(gen_log, '.*Content-Length too long'):
             response = self.fetch('/buffered', method='PUT', body=b'a' * 10240)
-        self.assertEqual(response.code, 599)
+        self.assertEqual(response.code, 400)
 
     def test_large_body_buffered_chunked(self):
         with ExpectLog(gen_log, '.*chunked body too large'):
             response = self.fetch('/buffered', method='PUT',
                                   body_producer=lambda write: write(b'a' * 10240))
-        self.assertEqual(response.code, 599)
+        # this test is flaky on windows; accept 400 (expected) or 599
+        self.assertIn(response.code, [400, 599])
 
     def test_large_body_streaming(self):
         with ExpectLog(gen_log, '.*Content-Length too long'):
             response = self.fetch('/streaming', method='PUT', body=b'a' * 10240)
-        self.assertEqual(response.code, 599)
+        self.assertEqual(response.code, 400)
 
     def test_large_body_streaming_chunked(self):
         with ExpectLog(gen_log, '.*chunked body too large'):
             response = self.fetch('/streaming', method='PUT',
                                   body_producer=lambda write: write(b'a' * 10240))
-        self.assertEqual(response.code, 599)
+        # this test is flaky on windows; accept 400 (expected) or 599
+        self.assertIn(response.code, [400, 599])
 
     def test_large_body_streaming_override(self):
         response = self.fetch('/streaming?expected_size=10240', method='PUT',
@@ -1090,14 +1101,14 @@ class BodyLimitsTest(AsyncHTTPTestCase):
             stream.write(b'PUT /streaming?expected_size=10240 HTTP/1.1\r\n'
                          b'Content-Length: 10240\r\n\r\n')
             stream.write(b'a' * 10240)
-            headers, response = yield gen.Task(read_stream_body, stream)
+            start_line, headers, response = yield gen.Task(read_stream_body, stream)
             self.assertEqual(response, b'10240')
             # Without the ?expected_size parameter, we get the old default value
             stream.write(b'PUT /streaming HTTP/1.1\r\n'
                          b'Content-Length: 10240\r\n\r\n')
             with ExpectLog(gen_log, '.*Content-Length too long'):
                 data = yield stream.read_until_close()
-            self.assertEqual(data, b'')
+            self.assertEqual(data, b'HTTP/1.1 400 Bad Request\r\n\r\n')
         finally:
             stream.close()