--- src/twisted/web/http.py | 32 +++++++++-- src/twisted/web/newsfragments/11976.bugfix | 7 ++ src/twisted/web/test/test_web.py | 81 ++++++++++++++++++++++++++++- 3 files changed, 114 insertions(+), 6 deletions(-) --- a/src/twisted/web/http.py +++ b/src/twisted/web/http.py @@ -2419,14 +2419,38 @@ class HTTPChannel(basic.LineReceiver, po self._handlingRequest = True + # We go into raw mode here even though we will be receiving lines next + # in the protocol; however, this data will be buffered and then passed + # back to line mode in the setLineMode call in requestDone. + self.setRawMode() + req = self.requests[-1] req.requestReceived(command, path, version) - def dataReceived(self, data): + def rawDataReceived(self, data: bytes) -> None: """ - Data was received from the network. Process it. + This is called when this HTTP/1.1 parser is in raw mode rather than + line mode. + + It may be in raw mode for one of two reasons: + + 1. All the headers of a request have been received and this + L{HTTPChannel} is currently receiving its body. + + 2. The full content of a request has been received and is currently + being processed asynchronously, and this L{HTTPChannel} is + buffering the data of all subsequent requests to be parsed + later. + + In the second state, the data will be played back later. + + @note: This isn't really a public API, and should be invoked only by + L{LineReceiver}'s line parsing logic. If you wish to drive an + L{HTTPChannel} from a custom data source, call C{dataReceived} on + it directly. + + @see: L{LineReceive.rawDataReceived} """ - # If we're currently handling a request, buffer this data. if self._handlingRequest: self._dataBuffer.append(data) if ( @@ -2438,9 +2462,7 @@ class HTTPChannel(basic.LineReceiver, po # ready. See docstring for _optimisticEagerReadSize above. self._networkProducer.pauseProducing() return - return basic.LineReceiver.dataReceived(self, data) - def rawDataReceived(self, data): self.resetTimeout() try: --- /dev/null +++ b/src/twisted/web/newsfragments/11976.bugfix @@ -0,0 +1,7 @@ +In Twisted 16.3.0, we changed twisted.web to stop dispatching HTTP/1.1 +pipelined requests to application code. There was a bug in this change which +still allowed clients which could send multiple full HTTP requests in a single +TCP segment to trigger asynchronous processing of later requests, which could +lead to out-of-order responses. This has now been corrected and twisted.web +should never process a pipelined request over HTTP/1.1 until the previous +request has fully completed. --- a/src/twisted/web/test/test_web.py +++ b/src/twisted/web/test/test_web.py @@ -8,6 +8,7 @@ Tests for various parts of L{twisted.web import os import zlib from io import BytesIO +from typing import List from zope.interface import implementer from zope.interface.verify import verifyObject @@ -17,10 +18,13 @@ from twisted.internet.address import IPv from twisted.internet.task import Clock from twisted.logger import LogLevel, globalLogPublisher from twisted.python import failure, reflect +from twisted.python.compat import iterbytes from twisted.python.filepath import FilePath -from twisted.test.proto_helpers import EventLoggingObserver +from twisted.test.proto_helpers import EventLoggingObserver, StringTransport from twisted.trial import unittest from twisted.web import error, http, iweb, resource, server +from twisted.web.resource import Resource +from twisted.web.server import NOT_DONE_YET, Request, Site from twisted.web.static import Data from twisted.web.test.requesthelper import DummyChannel, DummyRequest from ._util import assertIsFilesystemTemporary @@ -1849,3 +1853,78 @@ class ExplicitHTTPFactoryReactor(unittes factory = http.HTTPFactory() self.assertIs(factory.reactor, reactor) + + +class QueueResource(Resource): + """ + Add all requests to an internal queue, + without responding to the requests. + You can access the requests from the queue and handle their response. + """ + + isLeaf = True + + def __init__(self) -> None: + super().__init__() + self.dispatchedRequests: List[Request] = [] + + def render_GET(self, request: Request) -> int: + self.dispatchedRequests.append(request) + return NOT_DONE_YET + + +class TestRFC9112Section932(unittest.TestCase): + """ + Verify that HTTP/1.1 request ordering is preserved. + """ + + def test_multipleRequestsInOneSegment(self) -> None: + """ + Twisted MUST NOT respond to a second HTTP/1.1 request while the first + is still pending. + """ + qr = QueueResource() + site = Site(qr) + proto = site.buildProtocol(None) + serverTransport = StringTransport() + proto.makeConnection(serverTransport) + proto.dataReceived( + b"GET /first HTTP/1.1\r\nHost: a\r\n\r\n" + b"GET /second HTTP/1.1\r\nHost: a\r\n\r\n" + ) + # The TCP data contains 2 requests, + # but only 1 request was dispatched, + # as the first request was not yet finalized. + self.assertEqual(len(qr.dispatchedRequests), 1) + # The first request is finalized and the + # second request is dispatched right away. + qr.dispatchedRequests[0].finish() + self.assertEqual(len(qr.dispatchedRequests), 2) + + def test_multipleRequestsInDifferentSegments(self) -> None: + """ + Twisted MUST NOT respond to a second HTTP/1.1 request while the first + is still pending, even if the second request is received in a separate + TCP package. + """ + qr = QueueResource() + site = Site(qr) + proto = site.buildProtocol(None) + serverTransport = StringTransport() + proto.makeConnection(serverTransport) + raw_data = ( + b"GET /first HTTP/1.1\r\nHost: a\r\n\r\n" + b"GET /second HTTP/1.1\r\nHost: a\r\n\r\n" + ) + # Just go byte by byte for the extreme case in which each byte is + # received in a separate TCP package. + for chunk in iterbytes(raw_data): + proto.dataReceived(chunk) + # The TCP data contains 2 requests, + # but only 1 request was dispatched, + # as the first request was not yet finalized. + self.assertEqual(len(qr.dispatchedRequests), 1) + # The first request is finalized and the + # second request is dispatched right away. + qr.dispatchedRequests[0].finish() + self.assertEqual(len(qr.dispatchedRequests), 2)