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

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

Can you write a test case for this?

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 ?

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.

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.

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

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.

Thanks,

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

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?

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.

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.

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

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.

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.

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.

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

3.5 patch.

3.6 patch(..._05.patch file)

Sorry to be a pain, but I think the new logic for HEAD is wrong. See review.

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.

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.

Thanks for the suggestions, I've updated the test cases and created a patch for 3.6.

Patch 07 looks fine. I presume you still want to do the porting to 3.5 and 2.7.

Great, thanks for checking. Attaching patch for 2.7. 3.5 will follow.

A patch for 3.5 attached.

New changeset 1dc495007b8f by Martin Panter in branch '3.5':
Issue bpo-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 bpo-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 bpo-25738: Merge HTTP server from 3.5
https://hg.python.org/cpython/rev/de5e1eb4a88d