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

Clear fewer headers on 1xx/204/304 responses #2727

Merged
merged 1 commit into from
Oct 21, 2019
Merged

Conversation

andersk
Copy link
Contributor

@andersk andersk commented Aug 12, 2019

This function is called on more than just 304 responses; it’s important to permit the Allow header on 204 responses. Also, the relevant RFCs have changed significantly.

Fixes #2726.

@bdarnell
Copy link
Member

We should definitely permit the Allow header for 204 responses, but the name _clear_headers_for_304 is no longer literally accurate so I don't know if this is the change we want to make.

The primary purpose of this method is to clear the Content- headers from responses with no body, especially the Content-Type header that is added by set_default_headers. It's called for 1xx, 204, and 304 responses. I think we still want to do at least this much.

The RFCs have changed significantly since this function was written. It cites the definition of entity headers from RFC 2616 §7.1, which appears to have been replaced by a much shorter list of representation metadata headers in RFC 7231 §3.1 (Allow is no longer on the list). RFC 2616 §10.3.5 said that 304 responses MUST NOT contain other entity headers, while RFC 7232 §4.1 makes it a SHOULD NOT (and Last-Modified is given as an example of a header that's sometimes useful).

So this is what I'd do: rename _clear_headers_for_304 to _clear_representation_headers, change it to only clear Content-Type, Content-Language, and Content-Encoding, and keep calling it for all 1xx responses, 204, and 304. Make sense?

@andersk andersk changed the title Only call _clear_headers_for_304 on 304 responses. Clear fewer headers on 1xx/204/304 responses Aug 30, 2019
@andersk
Copy link
Contributor Author

andersk commented Aug 30, 2019

Sure. Updated as such.

@bdarnell
Copy link
Member

bdarnell commented Sep 2, 2019

Looks good, now the test test_static_304_if_modified_since just needs to be fixed up.

tornado/web.py Outdated
# 304 responses should not contain entity headers (defined in
# http://www.w3.org/Protocols/rfc2616/rfc2616-sec7.html#sec7.1)
def _clear_representation_headers(self) -> None:
# 304 responses should not representation metadata headers (defined in
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... shoud not contain representation metadata headers...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, fixed, now with passing CI.

This function is called on more than just 304 responses; it’s
important to permit the Allow header on 204 responses.  Also, the
relevant RFCs have changed significantly.

Fixes tornadoweb#2726.

Signed-off-by: Anders Kaseorg <andersk@mit.edu>
@bdarnell bdarnell merged commit d81d685 into tornadoweb:master Oct 21, 2019
@bdarnell
Copy link
Member

Thanks!

@andersk andersk deleted the 304 branch March 17, 2022 22:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrectly removes ‘Allow’ header from HTTP 204 response
3 participants