[issue25738] http.server doesn't handle RESET CONTENT status correctly

[Martin Panter]

Changes by Martin Panter :


--
resolution:  -> fixed
stage: patch review -> resolved
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25738] http.server doesn't handle RESET CONTENT status correctly

[Roundup Robot]

Roundup Robot added the comment:

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

--
nosy: +python-dev

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25738] http.server doesn't handle RESET CONTENT status correctly

[Susumu Koshiba]

Susumu Koshiba added the comment:

A patch for 3.5 attached.

--
Added file: 
http://bugs.python.org/file43292/issue25738_http_reset_content_3.5_02.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25738] http.server doesn't handle RESET CONTENT status correctly

[Susumu Koshiba]

Susumu Koshiba added the comment:

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

--
Added file: 
http://bugs.python.org/file43291/issue25738_http_reset_content_2.7_02.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25738] http.server doesn't handle RESET CONTENT status correctly

[Martin Panter]

Martin Panter added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25738] http.server doesn't handle RESET CONTENT status correctly

[Susumu Koshiba]

Susumu Koshiba added the comment:

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

--
Added file: 
http://bugs.python.org/file43277/issue25738_http_reset_content_07.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25738] http.server doesn't handle RESET CONTENT status correctly

[Martin Panter]

Martin Panter added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25738] http.server doesn't handle RESET CONTENT status correctly

[Mathieu Dupuy]

Changes by Mathieu Dupuy :


--
nosy:  -deronnax

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25738] http.server doesn't handle RESET CONTENT status correctly

[Susumu Koshiba]

Susumu Koshiba added the comment:

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.

--
Added file: 
http://bugs.python.org/file43255/issue25738_http_reset_content_06.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25738] http.server doesn't handle RESET CONTENT status correctly

[Martin Panter]

Martin Panter added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25738] http.server doesn't handle RESET CONTENT status correctly

[Susumu Koshiba]

Susumu Koshiba added the comment:

3.6 patch(..._05.patch file)

--
Added file: 
http://bugs.python.org/file43225/issue25738_http_reset_content_05.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25738] http.server doesn't handle RESET CONTENT status correctly

[Susumu Koshiba]

Susumu Koshiba added the comment:

3.5 patch.

--
Added file: 
http://bugs.python.org/file43224/issue25738_http_reset_content_3.5_01.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25738] http.server doesn't handle RESET CONTENT status correctly

[Susumu Koshiba]

Susumu Koshiba added the comment:

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

--
Added file: 
http://bugs.python.org/file43223/issue25738_http_reset_content_2.7_01.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25738] http.server doesn't handle RESET CONTENT status correctly

[Martin Panter]

Martin Panter added the comment:

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.

--
versions: +Python 2.7, Python 3.5

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25738] http.server doesn't handle RESET CONTENT status correctly

[Susumu Koshiba]

Susumu Koshiba added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25738] http.server doesn't handle RESET CONTENT status correctly

[Martin Panter]

Martin Panter added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25738] http.server doesn't handle RESET CONTENT status correctly

[Susumu Koshiba]

Susumu Koshiba added the comment:

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

--
Added file: 
http://bugs.python.org/file43204/issue25738_http_reset_content_04.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25738] http.server doesn't handle RESET CONTENT status correctly

[Susumu Koshiba]

Susumu Koshiba added the comment:

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.

--
Added file: 
http://bugs.python.org/file43203/issue25738_http_reset_content_03.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25738] http.server doesn't handle RESET CONTENT status correctly

[Martin Panter]

Martin Panter added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25738] http.server doesn't handle RESET CONTENT status correctly

[Martin Panter]

Martin Panter added the comment:

I suggest to drop the RESET_CONTENT exception. Content-Length: 0 is explicitly 
allowed by  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 . 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?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25738] http.server doesn't handle RESET CONTENT status correctly

[Susumu Koshiba]

Susumu Koshiba added the comment:

Thanks,

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

--
Added file: 
http://bugs.python.org/file43182/issue25738_http_reset_content_02.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25738] http.server doesn't handle RESET CONTENT status correctly

[R. David Murray]

R. David Murray added the comment:

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.

--
nosy: +r.david.murray

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25738] http.server doesn't handle RESET CONTENT status correctly

[Susumu Koshiba]

Susumu Koshiba added the comment:

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

--
keywords: +patch
nosy: +Susumu Koshiba
versions: +Python 3.6
Added file: http://bugs.python.org/file43129/issue25738_http_reset_content.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25738] http.server doesn't handle RESET CONTENT status correctly

[Mathieu Dupuy]

Mathieu Dupuy added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25738] http.server doesn't handle RESET CONTENT status correctly

[Mathieu Dupuy]

Mathieu Dupuy added the comment:

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 ?

--
nosy: +deronnax

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25738] http.server doesn't handle RESET CONTENT status correctly

[SpaceOne]

New submission from SpaceOne:

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

--
components: Library (Lib)
messages: 255443
nosy: spaceone
priority: normal
severity: normal
status: open
title: http.server doesn't handle RESET CONTENT status correctly

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25738] http.server doesn't handle RESET CONTENT status correctly

[Martin Panter]

Martin Panter added the comment:

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

Can you write a test case for this?

--
nosy: +martin.panter
stage:  -> patch review
type:  -> behavior

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com