Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ignore response body if status code doesn't allow a body #202

Merged
merged 6 commits into from Sep 6, 2018
Copy path View file
@@ -155,6 +155,7 @@ class Task(object):
content_length = None
content_bytes_written = 0
logged_write_excess = False
logged_write_no_body = False
complete = False
chunked_response = False
logger = logger
@@ -182,6 +183,13 @@ def service(self):
finally:
pass

@property
def has_body(self):
return not (self.status.startswith('1') or
self.status.startswith('204') or
self.status.startswith('304')
)

def cancel(self):
self.close_on_finish = True

@@ -203,7 +211,12 @@ def build_response_header(self):
[x.capitalize() for x in headername.split('-')]
)
if headername == 'Content-Length':
content_length_header = headerval
if self.has_body:
content_length_header = headerval
else:
del response_headers[i]
continue # pragma: no cover

This comment has been minimized.

Copy link
@mmerickel

mmerickel Sep 1, 2018

Member

deletion while iteration - sure this is ok?

This comment has been minimized.

Copy link
@bertjwregeer

This comment has been minimized.

Copy link
@bertjwregeer

bertjwregeer Sep 4, 2018

Author Member

I guess I need to find out if enumerate creates a copy of the list or if it iterates over the list internally.

This comment has been minimized.

Copy link
@bertjwregeer

bertjwregeer Sep 4, 2018

Author Member

This will break in subtle ways if there is more than one Content-Length header set (which is illegal, but possible)

This comment has been minimized.

Copy link
@tseaver

tseaver Sep 4, 2018

Member

FWIW, enumerate has generator semantics (see PEP 279).

This comment has been minimized.

Copy link
@bertjwregeer

bertjwregeer Sep 4, 2018

Author Member

Then the other code and this code are both subtly broken... will fix


if headername == 'Date':
date_header = headerval
if headername == 'Server':
@@ -213,7 +226,11 @@ def build_response_header(self):
# replace with properly capitalized version
response_headers[i] = (headername, headerval)

if content_length_header is None and self.content_length is not None:
if (
content_length_header is None and
self.content_length is not None and
self.has_body
):
content_length_header = str(self.content_length)
self.response_headers.append(
('Content-Length', content_length_header)
@@ -239,11 +256,12 @@ def close_on_finish():

if not content_length_header:
# RFC 7230: MUST NOT send Transfer-Encoding or Content-Length
# for any response with a status code of 1xx or 204.
if not (self.status.startswith('1') or
self.status.startswith('204')):
# for any response with a status code of 1xx, 204 or 304.

if self.has_body:
response_headers.append(('Transfer-Encoding', 'chunked'))
self.chunked_response = True

if not self.close_on_finish:
close_on_finish()

@@ -297,7 +315,8 @@ def write(self, data):
rh = self.build_response_header()
channel.write_soon(rh)
self.wrote_header = True
if data:

if data and self.has_body:
towrite = data
cl = self.content_length
if self.chunked_response:
@@ -314,6 +333,18 @@ def write(self, data):
self.logged_write_excess = True
if towrite:
channel.write_soon(towrite)
else:
# Cheat, and tell the application we have written all of the bytes,
# even though the response shouldn't have a body and we are
# ignoring it entirely.
self.content_bytes_written += len(data)

if not self.logged_write_no_body:
self.logger.warning(
'application-written content was ignored due to HTTP '
'response that may not contain a message-body: (%s)' % self.status)
self.logged_write_no_body = True


class ErrorTask(Task):
""" An error task produces an error response
Copy path View file
@@ -245,6 +245,23 @@ def test_build_response_header_v11_1xx_no_content_length_or_transfer_encoding(se
self.assertEqual(inst.close_on_finish, True)
self.assertTrue(('Connection', 'close') in inst.response_headers)

def test_build_response_header_v11_304_no_content_length_or_transfer_encoding(self):
# RFC 7230: MUST NOT send Transfer-Encoding or Content-Length
# for any response with a status code of 1xx, 204 or 304.
inst = self._makeOne()
inst.request = DummyParser()
inst.version = '1.1'
inst.status = '304 Not Modified'
result = inst.build_response_header()
lines = filter_lines(result)
self.assertEqual(len(lines), 4)
self.assertEqual(lines[0], b'HTTP/1.1 304 Not Modified')
self.assertEqual(lines[1], b'Connection: close')
self.assertTrue(lines[2].startswith(b'Date:'))
self.assertEqual(lines[3], b'Server: waitress')
self.assertEqual(inst.close_on_finish, True)
self.assertTrue(('Connection', 'close') in inst.response_headers)

def test_build_response_header_via_added(self):
inst = self._makeOne()
inst.request = DummyParser()
@@ -561,6 +578,34 @@ def app(environ, start_response):
self.assertEqual(inst.close_on_finish, True)
self.assertEqual(len(inst.logger.logged), 0)

def test_execute_app_without_body_204_logged(self):
def app(environ, start_response):
start_response('204 No Content', [('Content-Length', '3')])
return [b'abc']
inst = self._makeOne()
inst.channel.server.application = app
inst.logger = DummyLogger()
inst.execute()
self.assertEqual(inst.close_on_finish, True)
self.assertNotIn(b'abc', inst.channel.written)
self.assertNotIn(b'Content-Length', inst.channel.written)
self.assertNotIn(b'Transfer-Encoding', inst.channel.written)
self.assertEqual(len(inst.logger.logged), 1)

def test_execute_app_without_body_304_logged(self):
def app(environ, start_response):
start_response('304 Not Modified', [('Content-Length', '3')])
return [b'abc']
inst = self._makeOne()
inst.channel.server.application = app
inst.logger = DummyLogger()
inst.execute()
self.assertEqual(inst.close_on_finish, True)
self.assertNotIn(b'abc', inst.channel.written)
self.assertNotIn(b'Content-Length', inst.channel.written)
self.assertNotIn(b'Transfer-Encoding', inst.channel.written)
self.assertEqual(len(inst.logger.logged), 1)

def test_execute_app_returns_closeable(self):
class closeable(list):
def close(self):
@@ -915,9 +915,10 @@ def handle_close(self):
self.flag = True
self.close()

# def handle_expt(self):
# self.flag = True
# self.close()
def handle_expt(self): # pragma: no cover
# needs to exist for MacOS testing
self.flag = True
self.close()

class TestHandler(BaseTestHandler):

ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.