Merge why-stop-a-request-when-you-can-keep-writing-8317: Fix HTTPChan…

…nel both sending a failure and processing a request with an invalid final header. Author: lukasa Reviewer: hawkowl Fixes: twisted#8317 git-svn-id: svn://svn.twistedmatrix.com/svn/Twisted/trunk@47343 bbbe8e31-12d6-0310-92fd-ac37d47ddeeb
JanMalte · May 4, 2016 · 6db6539 · 6db6539
1 parent 1b480b3
commit 6db6539
Show file tree

Hide file tree

Showing 3 changed files with 61 additions and 9 deletions.
diff --git a/twisted/web/http.py b/twisted/web/http.py
@@ -1681,7 +1681,11 @@ def lineReceived(self, line):
         elif line == b'':
             # End of headers.
             if self.__header:
-                self.headerReceived(self.__header)
+                ok = self.headerReceived(self.__header)
+                # If the last header we got is invalid, we MUST NOT proceed
+                # with processing. We'll have sent a 400 anyway, so just stop.
+                if not ok:
+                    return
             self.__header = ''
             self.allHeadersReceived()
             if self.length == 0:
@@ -1713,12 +1717,15 @@ def headerReceived(self, line):
         @type line: C{bytes}
         @param line: A line from the header section of a request, excluding the
             line delimiter.
+
+        @return: A flag indicating whether the header was valid.
+        @rtype: L{bool}
         """
         try:
             header, data = line.split(b':', 1)
         except ValueError:
             _respondToBadRequestAndDisconnect(self.transport)
-            return
+            return False
 
         header = header.lower()
         data = data.strip()
@@ -1728,7 +1735,7 @@ def headerReceived(self, line):
             except ValueError:
                 _respondToBadRequestAndDisconnect(self.transport)
                 self.length = None
-                return
+                return False
             self._transferDecoder = _IdentityTransferDecoder(
                 self.length, self.requests[-1].handleContentChunk, self._finishRequestBody)
         elif header == b'transfer-encoding' and data.lower() == b'chunked':
@@ -1747,7 +1754,9 @@ def headerReceived(self, line):
         self._receivedHeaderCount += 1
         if self._receivedHeaderCount > self.maxHeaders:
             _respondToBadRequestAndDisconnect(self.transport)
-            return
+            return False
+
+        return True
 
 
     def allContentReceived(self):

diff --git a/twisted/web/test/test_http.py b/twisted/web/test/test_http.py
@@ -812,12 +812,19 @@ def test_invalidNonAsciiMethod(self):
         When client sends invalid HTTP method containing
         non-ascii characters HTTP 400 'Bad Request' status will be returned.
         """
+        processed = []
+        class MyRequest(http.Request):
+            def process(self):
+                processed.append(self)
+                self.finish()
+
         badRequestLine = b"GE\xc2\xa9 / HTTP/1.1\r\n\r\n"
-        channel = self.runRequest(badRequestLine, http.Request, 0)
+        channel = self.runRequest(badRequestLine, MyRequest, 0)
         self.assertEqual(
             channel.transport.value(),
             b"HTTP/1.1 400 Bad Request\r\n\r\n")
         self.assertTrue(channel.transport.disconnecting)
+        self.assertEqual(processed, [])
 
 
     def test_basicAuth(self):
@@ -895,25 +902,39 @@ def test_invalidContentLengthHeader(self):
         (Bad Request) response is sent to the client and the connection is
         closed.
         """
+        processed = []
+        class MyRequest(http.Request):
+            def process(self):
+                processed.append(self)
+                self.finish()
+
         requestLines = [b"GET / HTTP/1.0", b"Content-Length: x", b"", b""]
-        channel = self.runRequest(b"\n".join(requestLines), http.Request, 0)
+        channel = self.runRequest(b"\n".join(requestLines), MyRequest, 0)
         self.assertEqual(
             channel.transport.value(),
             b"HTTP/1.1 400 Bad Request\r\n\r\n")
         self.assertTrue(channel.transport.disconnecting)
+        self.assertEqual(processed, [])
 
 
     def test_invalidHeaderNoColon(self):
         """
         If a header without colon is received a 400 (Bad Request) response
         is sent to the client and the connection is closed.
         """
+        processed = []
+        class MyRequest(http.Request):
+            def process(self):
+                processed.append(self)
+                self.finish()
+
         requestLines = [b"GET / HTTP/1.0", b"HeaderName ", b"", b""]
-        channel = self.runRequest(b"\n".join(requestLines), http.Request, 0)
+        channel = self.runRequest(b"\n".join(requestLines), MyRequest, 0)
         self.assertEqual(
             channel.transport.value(),
             b"HTTP/1.1 400 Bad Request\r\n\r\n")
         self.assertTrue(channel.transport.disconnecting)
+        self.assertEqual(processed, [])
 
 
     def test_headerLimitPerRequest(self):
@@ -963,13 +984,24 @@ def test_headersTooBigInitialCommand(self):
         on the size of headers received per request starting from initial
         command line.
         """
+        processed = []
+        class MyRequest(http.Request):
+            def process(self):
+                processed.append(self)
+                self.finish()
+
         channel = http.HTTPChannel()
         channel.totalHeadersSize = 10
         httpRequest = b'GET /path/longer/than/10 HTTP/1.1\n'
 
         channel = self.runRequest(
-            httpRequest=httpRequest, channel=channel, success=False)
+            httpRequest=httpRequest,
+            requestFactory=MyRequest,
+            channel=channel,
+            success=False
+        )
 
+        self.assertEqual(processed, [])
         self.assertEqual(
             channel.transport.value(),
             b"HTTP/1.1 400 Bad Request\r\n\r\n")
@@ -981,6 +1013,12 @@ def test_headersTooBigOtherHeaders(self):
         on the size of headers received per request counting first line
         and total headers.
         """
+        processed = []
+        class MyRequest(http.Request):
+            def process(self):
+                processed.append(self)
+                self.finish()
+
         channel = http.HTTPChannel()
         channel.totalHeadersSize = 40
         httpRequest = (
@@ -989,8 +1027,12 @@ def test_headersTooBigOtherHeaders(self):
             )
 
         channel = self.runRequest(
-            httpRequest=httpRequest, channel=channel, success=False)
+            httpRequest=httpRequest,
+            requestFactory=MyRequest,
+            channel=channel, success=False
+        )
 
+        self.assertEqual(processed, [])
         self.assertEqual(
             channel.transport.value(),
             b"HTTP/1.1 400 Bad Request\r\n\r\n")

diff --git a/twisted/web/topfiles/8317.bugfix b/twisted/web/topfiles/8317.bugfix
@@ -0,0 +1 @@
+twisted.web.http.HTTPChannel no longer processes requests that have invalid headers as the final header in their header block.
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1 @@
		twisted.web.http.HTTPChannel no longer processes requests that have invalid headers as the final header in their header block.