classification
Title: http.server doesn't handle RESET CONTENT status correctly
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.6, Python 3.5, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Susumu Koshiba, martin.panter, python-dev, r.david.murray, spaceone
Priority: normal Keywords: patch

Created on 2015-11-27 00:26 by spaceone, last changed 2016-06-08 12:16 by martin.panter. This issue is now closed.

Files
File name Uploaded Description Edit
issue25738_http_reset_content.patch Susumu Koshiba, 2016-06-03 00:01 review
issue25738_http_reset_content_02.patch Susumu Koshiba, 2016-06-04 00:49 review
issue25738_http_reset_content_03.patch Susumu Koshiba, 2016-06-04 19:05 review
issue25738_http_reset_content_04.patch Susumu Koshiba, 2016-06-04 19:20 review
issue25738_http_reset_content_2.7_01.patch Susumu Koshiba, 2016-06-05 02:22 review
issue25738_http_reset_content_3.5_01.patch Susumu Koshiba, 2016-06-05 02:26 review
issue25738_http_reset_content_05.patch Susumu Koshiba, 2016-06-05 02:30 review
issue25738_http_reset_content_06.patch Susumu Koshiba, 2016-06-06 05:19 review
issue25738_http_reset_content_07.patch Susumu Koshiba, 2016-06-07 10:03 review
issue25738_http_reset_content_2.7_02.patch Susumu Koshiba, 2016-06-08 02:54 review
issue25738_http_reset_content_3.5_02.patch Susumu Koshiba, 2016-06-08 03:05 review
Messages (26)
msg255443 - (view) Author: SpaceOne (spaceone) Date: 2015-11-27 00:26
The status 205 RESET CONTENT is not correctly evaluated by http.server.
It MUST NOT write a response body to the client.
 
Patch: https://github.com/spaceone/cpython/commit/17048e2e7349cc4861c7fe90299f2c092b8e1604
msg255448 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-11-27 02:08
I left some comments about the tangential changes on Git Hub.

Can you write a test case for this?
msg260336 - (view) Author: Mathieu Dupuy (deronnax) * Date: 2016-02-15 22:47
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 ?
msg260389 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-02-17 11:01
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.
msg260417 - (view) Author: Mathieu Dupuy (deronnax) * Date: 2016-02-18 00:32
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.
msg266996 - (view) Author: Susumu Koshiba (Susumu Koshiba) * Date: 2016-06-03 00:01
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
msg267200 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2016-06-04 00:33
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.
msg267210 - (view) Author: Susumu Koshiba (Susumu Koshiba) * Date: 2016-06-04 00:49
Thanks,

I've added some comments to the doc and updated the patch.
msg267217 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-06-04 01:22
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 Issue 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?
msg267218 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-06-04 01:28
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.
msg267291 - (view) Author: Susumu Koshiba (Susumu Koshiba) * Date: 2016-06-04 19:05
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.
msg267298 - (view) Author: Susumu Koshiba (Susumu Koshiba) * Date: 2016-06-04 19:20
Sorry, re-created a patch to do the right word wrapping and made my comments less verbose.
msg267355 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-06-05 00:33
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.
msg267358 - (view) Author: Susumu Koshiba (Susumu Koshiba) * Date: 2016-06-05 01:04
Thanks again for valuable comments.

I agree that there could be a case where send_error() gets used for both HEAD and GET, in which case we could send content-length field as an optional field. However, if send_error() is not used for both HEAD and GET, then the body will likely be different and RFC specifies that we must not send content-length field with different values so I have a slight preference towards not sending it for HEAD request. 

If you are OK with leaving content-length out for HEAD, I'd like to proceed with creating a small patch to reflect your comments in code review and also start creating a patches for 2.7 and 3.5.
msg267361 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-06-05 01:51
I think it would be unusual to have different error handling for GET vs HEAD, given HEAD is supposed to be exactly like GET but without a body. On the other hand, see SimpleHTTPRequestHandler.send_head() for an example of calling send_error(NOT_FOUND) for both GET and HEAD. Both Content-Length and Content-Type are for the would-be error message body:

HEAD /nonexistant HTTP/1.0

HTTP/1.0 404 File not found
Server: SimpleHTTP/0.6 Python/3.5.1
Date: Sun, 05 Jun 2016 01:40:55 GMT
Content-Type: text/html;charset=utf-8
Connection: close
Content-Length: 469

Removing Content-Length (and arguably also Content-Type) for HEAD would be an unwanted change in behaviour IMO. Instead, perhaps you would prefer to document that equivalent calls to send_error() should be made for HEAD and GET responses?

A separate 2.7 patch would be helpful (the filenames are different for a start, and the code is slightly different; I don’t think there is any Content-Length added). But I doubt a patch for 3.5 will be worthwhile. Instead, I will probably just apply your 3.6 patch to 3.5 and then merge forward.
msg267362 - (view) Author: Susumu Koshiba (Susumu Koshiba) * Date: 2016-06-05 02:22
Thanks, makes sense to me.

I've created patches for 2.7, 3.5, 3.6. 3.5's implementation looked slightly different from 3.6 so I've decided to create a separate patch, just in case. I will upload them 1 by 1 starting with 2.7. It will get a bit noisy here(sorry).
msg267363 - (view) Author: Susumu Koshiba (Susumu Koshiba) * Date: 2016-06-05 02:26
3.5 patch.
msg267364 - (view) Author: Susumu Koshiba (Susumu Koshiba) * Date: 2016-06-05 02:30
3.6 patch(..._05.patch file)
msg267390 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-06-05 06:12
Sorry to be a pain, but I think the new logic for HEAD is wrong. See review.
msg267517 - (view) Author: Susumu Koshiba (Susumu Koshiba) * Date: 2016-06-06 05:19
Ah yes, you are right. I didn't think about the cases where it will send HEAD for codes like 204(No Content). I've created a patch for 3.6 to see if this looks reasonable, if review passes, I'll create patches for the rest of the releases.

Thanks a lot for pointing this out.
msg267526 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-06-06 10:41
Thanks Susumu, I think the logic is right this time. I left some new suggestions for the tests, if you want to take them on or not, that’s up to you.
msg267613 - (view) Author: Susumu Koshiba (Susumu Koshiba) * Date: 2016-06-07 10:03
Thanks for the suggestions, I've updated the test cases and created a patch for 3.6.
msg267763 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-06-08 02:34
Patch 07 looks fine. I presume you still want to do the porting to 3.5 and 2.7.
msg267764 - (view) Author: Susumu Koshiba (Susumu Koshiba) * Date: 2016-06-08 02:54
Great, thanks for checking. Attaching patch for 2.7. 3.5 will follow.
msg267766 - (view) Author: Susumu Koshiba (Susumu Koshiba) * Date: 2016-06-08 03:05
A patch for 3.5 attached.
msg267826 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-06-08 09:50
New changeset 1dc495007b8f by Martin Panter in branch '3.5':
Issue #25738: Don’t send message body for 205 Reset Content
https://hg.python.org/cpython/rev/1dc495007b8f

New changeset b1041ddb1391 by Martin Panter in branch '2.7':
Issue #25738: Don’t send message body for 205 Reset Content
https://hg.python.org/cpython/rev/b1041ddb1391

New changeset de5e1eb4a88d by Martin Panter in branch 'default':
Issue #25738: Merge HTTP server from 3.5
https://hg.python.org/cpython/rev/de5e1eb4a88d
History
Date User Action Args
2016-06-08 12:16:58martin.pantersetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2016-06-08 09:50:50python-devsetnosy: + python-dev
messages: + msg267826
2016-06-08 03:05:23Susumu Koshibasetfiles: + issue25738_http_reset_content_3.5_02.patch

messages: + msg267766
2016-06-08 02:54:10Susumu Koshibasetfiles: + issue25738_http_reset_content_2.7_02.patch

messages: + msg267764
2016-06-08 02:34:43martin.pantersetmessages: + msg267763
2016-06-07 10:03:34Susumu Koshibasetfiles: + issue25738_http_reset_content_07.patch

messages: + msg267613
2016-06-06 10:41:40martin.pantersetmessages: + msg267526
2016-06-06 09:04:38deronnaxsetnosy: - deronnax
2016-06-06 05:19:03Susumu Koshibasetfiles: + issue25738_http_reset_content_06.patch

messages: + msg267517
2016-06-05 06:12:12martin.pantersetmessages: + msg267390
2016-06-05 02:30:17Susumu Koshibasetfiles: + issue25738_http_reset_content_05.patch

messages: + msg267364
2016-06-05 02:26:12Susumu Koshibasetfiles: + issue25738_http_reset_content_3.5_01.patch

messages: + msg267363
2016-06-05 02:22:25Susumu Koshibasetfiles: + issue25738_http_reset_content_2.7_01.patch

messages: + msg267362
2016-06-05 01:51:30martin.pantersetmessages: + msg267361
versions: + Python 2.7, Python 3.5
2016-06-05 01:04:16Susumu Koshibasetmessages: + msg267358
2016-06-05 00:33:36martin.pantersetmessages: + msg267355
2016-06-04 19:20:49Susumu Koshibasetfiles: + issue25738_http_reset_content_04.patch

messages: + msg267298
2016-06-04 19:05:22Susumu Koshibasetfiles: + issue25738_http_reset_content_03.patch

messages: + msg267291
2016-06-04 01:28:52martin.pantersetmessages: + msg267218
2016-06-04 01:22:45martin.pantersetmessages: + msg267217
2016-06-04 00:49:59Susumu Koshibasetfiles: + issue25738_http_reset_content_02.patch

messages: + msg267210
2016-06-04 00:33:33r.david.murraysetnosy: + r.david.murray
messages: + msg267200
2016-06-03 00:01:12Susumu Koshibasetfiles: + issue25738_http_reset_content.patch
versions: + Python 3.6
nosy: + Susumu Koshiba

messages: + msg266996

keywords: + patch
2016-02-18 00:32:23deronnaxsetmessages: + msg260417
2016-02-17 11:01:51martin.pantersetmessages: + msg260389
2016-02-15 22:47:41deronnaxsetnosy: + deronnax
messages: + msg260336
2015-11-27 02:08:45martin.pantersetnosy: + martin.panter
messages: + msg255448

type: behavior
stage: patch review
2015-11-27 00:26:18spaceonecreate