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

Conversation

Projects
None yet
3 participants
@bertjwregeer
Copy link
Member

bertjwregeer commented Sep 1, 2018

This is a follow-up to #166 and fixes the issue presented in #197.

This goes a step further than just making sure waitress doesn't generate any extra headers/content, it also removes it even if the application sent data it shouldn't have. This will help make sure that message bodies are not accidentally sent with HTTP status codes that SHOULD NOT contain message bodies.

Closes #197

bertjwregeer added some commits Sep 1, 2018

Add new has_body property
This is used to test if a response should have a message body or not.
1xx, 204 and 304 should not have message bodies.
Ignore body for status codes that don't have a body
For responses that should not include a message body, we now explicitly
ignore the body from the application and no longer send it on to the
requesting client.

We also log a warning to let developers/users know.
content_length_header = headerval
else:
del response_headers[i]
continue # pragma: no cover

This comment has been minimized.

@mmerickel

mmerickel Sep 1, 2018

Member

deletion while iteration - sure this is ok?

This comment has been minimized.

@bertjwregeer

This comment has been minimized.

@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.

@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.

@tseaver

tseaver Sep 4, 2018

Member

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

This comment has been minimized.

@bertjwregeer

bertjwregeer Sep 4, 2018

Author Member

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

Rewrite removal of item from response_headers
We don't want to delete items from a list while iterating over the list.
@@ -263,20 +284,28 @@ def close_on_finish():
if not date_header:
response_headers.append(('Date', build_http_date(self.start_time)))

self.response_headers = response_headers

This comment has been minimized.

@bertjwregeer

bertjwregeer Sep 6, 2018

Author Member

Not strictly required in this function, but required for testing.

@bertjwregeer bertjwregeer merged commit 4ce8f82 into master Sep 6, 2018

4 checks passed

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

@bertjwregeer bertjwregeer deleted the body-less-304 branch Sep 6, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.