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

MUST NOT send Transfer-Encoding or Content-Length for 1xx or 204 #166

Merged
merged 2 commits into from Sep 1, 2018

Conversation

Projects
None yet
4 participants
@alexanderlukanin13
Contributor

alexanderlukanin13 commented Jun 6, 2017

Fixes #165

@bertjwregeer

This comment has been minimized.

Show comment
Hide comment
@bertjwregeer

bertjwregeer Jun 6, 2017

Member

Also fixes/closes: #152

Member

bertjwregeer commented Jun 6, 2017

Also fixes/closes: #152

@mmerickel

This comment has been minimized.

Show comment
Hide comment
@mmerickel

mmerickel Jun 6, 2017

Member

This fix does not affect the content-length at all so I think #152 is still valid but probably needs to be fixed in webob, not waitress.

Member

mmerickel commented Jun 6, 2017

This fix does not affect the content-length at all so I think #152 is still valid but probably needs to be fixed in webob, not waitress.

@bertjwregeer

This comment has been minimized.

Show comment
Hide comment
@bertjwregeer

bertjwregeer Jun 6, 2017

Member

Do we have a test that will verify waitress will not send any content even if the WSGI app returns a body for the 204 and that we still close the iterator correctly (even if we don't iterate over it)?

Member

bertjwregeer commented Jun 6, 2017

Do we have a test that will verify waitress will not send any content even if the WSGI app returns a body for the 204 and that we still close the iterator correctly (even if we don't iterate over it)?

@bertjwregeer

This comment has been minimized.

Show comment
Hide comment
@bertjwregeer

bertjwregeer Jun 6, 2017

Member

You may be correct @mmerickel. Need to verify.

Member

bertjwregeer commented Jun 6, 2017

You may be correct @mmerickel. Need to verify.

@@ -202,6 +202,23 @@ def test_build_response_header_v11_200_no_content_length(self):
self.assertEqual(inst.close_on_finish, True)
self.assertTrue(('Connection', 'close') in inst.response_headers)

This comment has been minimized.

@tseaver

tseaver Jun 6, 2017

Member

Also needs a test for a 10x status.

@tseaver

tseaver Jun 6, 2017

Member

Also needs a test for a 10x status.

This comment has been minimized.

@bertjwregeer

bertjwregeer Jun 6, 2017

Member

There is a test for if the status code starts with 1.

@bertjwregeer

bertjwregeer Jun 6, 2017

Member

There is a test for if the status code starts with 1.

This comment has been minimized.

@tseaver

tseaver Jun 6, 2017

Member

@bertjwregeer I'm talking about branch coverage: the PR does not add a new testcase for the 10x status code, only for 204. It also doesn't update any existing testcases which might have triggered the same branch.

@tseaver

tseaver Jun 6, 2017

Member

@bertjwregeer I'm talking about branch coverage: the PR does not add a new testcase for the 10x status code, only for 204. It also doesn't update any existing testcases which might have triggered the same branch.

This comment has been minimized.

@alexanderlukanin13

alexanderlukanin13 Aug 17, 2017

Contributor

@tseaver @bertjwregeer Added test for HTTP 1xx (specifically 100 Continue) code. Test is the same as for 204. If you want more in-depth testing, please ask someone with a good knowledge of waitress.

@alexanderlukanin13

alexanderlukanin13 Aug 17, 2017

Contributor

@tseaver @bertjwregeer Added test for HTTP 1xx (specifically 100 Continue) code. Test is the same as for 204. If you want more in-depth testing, please ask someone with a good knowledge of waitress.

# 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')):

This comment has been minimized.

@tseaver

tseaver Jun 6, 2017

Member

I would be included to factor the test into a self.no_body() method, and then test that for the different status codes.

@tseaver

tseaver Jun 6, 2017

Member

I would be included to factor the test into a self.no_body() method, and then test that for the different status codes.

@bertjwregeer

This comment has been minimized.

Show comment
Hide comment
@bertjwregeer

bertjwregeer Jun 6, 2017

Member

@mmerickel That's not an issue in WebOb.

The issue is related to how Waitress deals with responses that don't contain a Content-Length header:

def test_execute_app_returns_204_no_content_without_cl(self):
        def app(environ, start_response):
            start_response('204 No Content', [])
            return [b'']

        inst = self._makeOne()
        inst.channel.server.application = app
        inst.execute()
        self.assertEqual(inst.content_length, None)
======================================================================
FAIL: test_execute_app_returns_204_no_content_without_cl (waitress.tests.test_task.TestWSGITask)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/xistence/Projects/waitress/waitress/tests/test_task.py", line 486, in test_execute_app_returns_204_no_content_without_cl
    self.assertEqual(inst.content_length, None)
AssertionError: 0 != None
Member

bertjwregeer commented Jun 6, 2017

@mmerickel That's not an issue in WebOb.

The issue is related to how Waitress deals with responses that don't contain a Content-Length header:

def test_execute_app_returns_204_no_content_without_cl(self):
        def app(environ, start_response):
            start_response('204 No Content', [])
            return [b'']

        inst = self._makeOne()
        inst.channel.server.application = app
        inst.execute()
        self.assertEqual(inst.content_length, None)
======================================================================
FAIL: test_execute_app_returns_204_no_content_without_cl (waitress.tests.test_task.TestWSGITask)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/xistence/Projects/waitress/waitress/tests/test_task.py", line 486, in test_execute_app_returns_204_no_content_without_cl
    self.assertEqual(inst.content_length, None)
AssertionError: 0 != None
@bertjwregeer

This comment has been minimized.

Show comment
Hide comment
@bertjwregeer

bertjwregeer Aug 16, 2017

Member

@alexanderlukanin13 do you have time to incorporate the changes that @tseaver requested?

Member

bertjwregeer commented Aug 16, 2017

@alexanderlukanin13 do you have time to incorporate the changes that @tseaver requested?

@bertjwregeer

This comment has been minimized.

Show comment
Hide comment
@bertjwregeer

bertjwregeer Aug 17, 2017

Member

Thank you @alexanderlukanin13, I'll try and get some work done on this tonight!

Member

bertjwregeer commented Aug 17, 2017

Thank you @alexanderlukanin13, I'll try and get some work done on this tonight!

@bertjwregeer bertjwregeer merged commit 45b1adc into Pylons:master Sep 1, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

bertjwregeer added a commit that referenced this pull request Sep 1, 2018

@bertjwregeer

This comment has been minimized.

Show comment
Hide comment
@bertjwregeer

bertjwregeer Sep 1, 2018

Member

Please see #202 for a follow-up to this PR that includes a fix for the 304 issue and goes a step further to make sure we don't accidentally send a further response than expected.

Member

bertjwregeer commented Sep 1, 2018

Please see #202 for a follow-up to this PR that includes a fix for the 304 issue and goes a step further to make sure we don't accidentally send a further response than expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment