http.server doesn't handle RESET CONTENT status correctly

[spaceone]

BPO	25738
Nosy	@bitdancer, @vadmium, @spaceone
Files	issue25738_http_reset_content.patch issue25738_http_reset_content_02.patch issue25738_http_reset_content_03.patch issue25738_http_reset_content_04.patch issue25738_http_reset_content_2.7_01.patch issue25738_http_reset_content_3.5_01.patch issue25738_http_reset_content_05.patch issue25738_http_reset_content_06.patch issue25738_http_reset_content_07.patch issue25738_http_reset_content_2.7_02.patch issue25738_http_reset_content_3.5_02.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2016-06-08.12:16:58.838>
created_at = <Date 2015-11-27.00:26:18.842>
labels = ['type-bug', 'library']
title = "http.server doesn't handle RESET CONTENT status correctly"
updated_at = <Date 2016-06-08.12:16:58.837>
user = 'https://github.com/spaceone'

bugs.python.org fields:

activity = <Date 2016-06-08.12:16:58.837>
actor = 'martin.panter'
assignee = 'none'
closed = True
closed_date = <Date 2016-06-08.12:16:58.838>
closer = 'martin.panter'
components = ['Library (Lib)']
creation = <Date 2015-11-27.00:26:18.842>
creator = 'spaceone'
dependencies = []
files = ['43129', '43182', '43203', '43204', '43223', '43224', '43225', '43255', '43277', '43291', '43292']
hgrepos = []
issue_num = 25738
keywords = ['patch']
message_count = 26.0
messages = ['255443', '255448', '260336', '260389', '260417', '266996', '267200', '267210', '267217', '267218', '267291', '267298', '267355', '267358', '267361', '267362', '267363', '267364', '267390', '267517', '267526', '267613', '267763', '267764', '267766', '267826']
nosy_count = 5.0
nosy_names = ['r.david.murray', 'python-dev', 'martin.panter', 'spaceone', 'Susumu Koshiba']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue25738'
versions = ['Python 2.7', 'Python 3.5', 'Python 3.6']

[spaceone]

The status 205 RESET CONTENT is not correctly evaluated by http.server.
It MUST NOT write a response body to the client.

Patch: spaceone/cpython@17048e2

[vadmium]

I left some comments about the tangential changes on Git Hub.

Can you write a test case for this?

[deronnax]

I was looking at this issue, and actually the problem is on a different level.
The function the patch takes place is "send_errror". As its name suggests, it's only used to send error (I checked in the code : it's only used to send 4XX/5XX reply). I'm sure none of this reply forbid to carry a body.
So I think the whole "if code >= 200 and code >= 200 and code not in (code not in (HTTPStatus.NO_CONTENT, HTTPStatus.NOT_MODIFIED)):" should just be replaced by a assert 400 <= code < 600.

And more seriously : who could be using this code for a modern real world usage ? Why not delete it ? Isn't it harmful that unwarned might use it ?

[vadmium]

Mathieu: What code are you suggesting to delete? The handling of NO_CONTENT etc? That could potentially break people’s code, and I don’t see the harm in leaving it as it is. I would prefer to either bless it in the documentation, or deprecate it if there is a problem.

[deronnax]

Oh, my mistake ; I though send_error was to be used internally only, but it's actually a documented public method, that does not enforce to only use "actual" HTTP error code (I wonder what's the point of calling send_error with a non-error status code but that's beyond the point of this bug).

I will finish the work of SpaceOne : do a clean patch with just the modification (no rename of the variable nor comments) and write a test case.

[SusumuKoshiba]

Patched the behaviors when NO_CONTENT and RESET_CONTENT are sent via send_error(). According to RFCs, they aren't actually errors so sending them through send_response() seems to make the most sense, however.

send_error()'s behavior changes in this patch are:

• Removed content-length field in the header for NO_CONTENT as per the RFC
• Removed content-length field in the header for RESET_CONTENT also to comply with option c) mentioned in RFC7321 6.3.6

[bitdancer]

Well, the reason it makes sense to use send_error, or at least the reason I suspect a lot of people do it, is because then you don't have to call end_headers() yourself. So, acknowleding this, it might be worth mentioning in the docs that the body isn't sent for certain return codes. Especially seeing as we already do it for NO_CONTENT and NOT_MODIFIED.

[SusumuKoshiba]

Thanks,

I've added some comments to the doc and updated the patch.

[vadmium]

I suggest to drop the RESET_CONTENT exception. Content-Length: 0 is explicitly allowed by <https://tools.ietf.org/html/rfc7231#section-6.3.6\> option a), and is not very different to the general case IMO. Content-Length was added here to help with buggy clients, see bpo-16088.

But while we are here, I think we should avoid Content-Length for 304 Not Modified. See <https://tools.ietf.org/html/rfc7230#section-3.3.2\>. This is a special case a bit like HEAD, where the length would refer to a cached response rather than the “error”/explanation message.

IMO this could be treated as a bug fix for 3.5 and 2.7, and no versionchanged is necessary. What do you think?

[vadmium]

Ah, forget that bit about RESET_CONTENT. If we include Content-Length, it will probably be non-zero, which will be wrong. So your patch is better in that regard.

[SusumuKoshiba]

Thanks a lot for a quick review. I've created a new patch with below changes.

1. No content-length and the body to be sent for:
   a. 204(No Content)
   b. 205(Reset Content)
   c. 304(Not Modified)
   d. response to HEAD request method
   e. 1xx
2. Documentation change to remove version changed, in case we are making this patch to 2.7 and 3.5

I've added test cases for most of the scenarios mentioned above, but the behavior of 100(Continue) is left out due to the fact that sending 100(Continue) via send_error() gets tricky as it's not meant to close the connection after sending this response. I'm a bit undecided about the handling of 100(Continue) code in send_error() so I'll leave it as-is for now and possibly file a separate ticket depending on what we decide to do.

Happy to patch 2.7, 3.5 as a bug fix also.

[SusumuKoshiba]

Sorry, re-created a patch to do the right word wrapping and made my comments less verbose.

[vadmium]

For a HEAD request, I think we should continue to send Content-Length (except in combination with one of the special responses). HEAD is slightly different to 304 Not Modified. With HEAD vs GET, the response code and other header values do not change, but the body is omitted. So it is plausible for a user to make the same send_error() call for both GET and HEAD.

With 304 Not Modified, the response code _does_ change (304 is instead of e.g. 200), although other header values are allowed to be the same. The user would have to make a different send_error() call to trigger the different response code, and the body and length that would be generated is different.