Description
@Lukasa reported | |
---|---|
Trac ID | trac#8317 |
Type | defect |
Created | 2016-05-04 10:41:41Z |
Branch | https://github.com/twisted/twisted/tree/why-stop-a-request-when-you-can-keep-writing-8317 |
Spotted while attempting to surgically remove pipelining from twisted.web.
A test in twisted.web.test_http
(TestParsing.test_invalidHeaderNoColon
) attempts to test that Twisted 400s when receiving invalid header blocks, specifically those which contain a header with no colon.
Unfortunately, that test doesn't quite test the correct behaviour. This is because Twisted delays header processing to allow for the possibility of header folding (deprecated as of RFC 7230). That means that Twisted only processes a header on the line after it is received.
That's fine if the bad header appears almost anywhere in the header block, but if it appears as the last header then Twisted will parse that header and then unconditionally continue to process the response. That can cause problems.
To reproduce this problem, take a clean Twisted installation from trunk and start the twistd web server, serving the twisted codebase: twistd -n web --path .
Then, put this in a file:
import socket
import time
data = (
b'GET /setup.py HTTP/1.1\r\n'
b'Host: localhost\r\n'
b'Headername \r\n'
b'\r\n'
)
s = socket.create_connection(('localhost', 8080))
s.sendall(data)
time.sleep(0.3) # Let the socket layer do its thing.
print s.recv(65535)
As you can see, the last header in the block contains no colon, leading the block to be incorrectly formatted. The expected result of receiving this is that the terminal prints HTTP/1.1 400 Bad Request
and nothing else. Instead, the terminal prints HTTP/1.1 400 Bad Request
, a blank line, and also a 200 OK response to the request:
HTTP/1.1 400 Bad Request
HTTP/1.1 200 OK
Content-Length: 1418
Accept-Ranges: bytes
Server: TwistedWeb/16.1.1
Last-Modified: Fri, 18 Mar 2016 10:11:34 GMT
Date: Wed, 04 May 2016 10:38:40 GMT
Content-Type: text/x-python
#!/usr/bin/env python
# Copyright (c) Twisted Matrix Laboratories.
# See LICENSE for details.
"""
Setuptools installer for Twisted.
"""
import os
import sys
import setuptools
# Tell Twisted not to enforce zope.interface requirement on import, since
# we're going to have to import twisted.python.dist and can rely on
# setuptools to install dependencies.
setuptools._TWISTED_NO_CHECK_REQUIREMENTS = True
def main(args):
"""
Invoke twisted.python.dist with the appropriate metadata about the
Twisted package.
"""
# On Python 3, use setup3.py until Python 3 port is done:
if sys.version_info[0] > 2:
import setup3
setup3.main()
return
if os.path.exists('twisted'):
sys.path.insert(0, '.')
requirements = ["zope.interface >= 3.6.0"]
from twisted.python.dist import (
STATIC_PACKAGE_METADATA, getExtensions, getScripts,
setup, _EXTRAS_REQUIRE)
setup_args = STATIC_PACKAGE_METADATA.copy()
setup_args.update(dict(
packages=setuptools.find_packages(),
install_requires=requirements,
conditionalExtensions=getExtensions(),
scripts=getScripts(),
include_package_data=True,
zip_safe=False,
extras_require=_EXTRAS_REQUIRE,
))
setup(**setup_args)
if __name__ == "__main__":
try:
main(sys.argv[1:])
except KeyboardInterrupt:
sys.exit(1)
The twistd logs also contain a 200 response to that request.
The expected behaviour can be seen by adding a new, good header underneath the bad one. That causes twisted to only return the 400, and to otherwise not dispatch the request.
Attachments:
- 8317_1.patch (7046 bytes) - added by Lukasa on 2016-05-04 11:45:40Z - First draft patch
Searchable metadata
trac-id__8317 8317
type__defect defect
reporter__Lukasa Lukasa
priority__normal normal
milestone__None None
branch__branches_why_stop_a_request_when_you_can_keep_writing_8317 branches/why-stop-a-request-when-you-can-keep-writing-8317
branch_author__hawkowl hawkowl
status__closed closed
resolution__fixed fixed
component__web web
keywords__None None
time__1462358501138537 1462358501138537
changetime__1474198049187875 1474198049187875
version__None None
owner__None None
Activity
twisted-trac commentedon May 4, 2016
I've uploaded a patch that fixes this behaviour by aborting if parsing the final header in the block fails. The testing works by ensuring that the tests that check invalid headers don't cause the request to be processed.
This is good for review. Can reviewers please note that this issue blocks HTTP/2 work.
twisted-trac commentedon May 4, 2016
(In [47341]) Branching to why-stop-a-request-when-you-can-keep-writing-8317.
twisted-trac commentedon May 4, 2016
(In [47342]) apply patch from lukasa, refs #8317
twisted-trac commentedon May 4, 2016
closed
(In [47343]) Merge why-stop-a-request-when-you-can-keep-writing-8317: Fix HTTPChannel both sending a failure and processing a request with an invalid final header.
Author: lukasa
Reviewer: hawkowl
Fixes: #8317
twisted-trac commentedon Sep 18, 2016
[mass edit] Removing review from closed tickets.