[issue28548] http.server parse_request() bug and error reporting

[Donald Stufft]

Changes by Donald Stufft :


--
pull_requests: +986

___
Python tracker 

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

[issue28548] http.server parse_request() bug and error reporting

[Martin Panter]

Martin Panter added the comment:

Okay committed to 3.7 for the moment. I think that is all we can reasonably do 
unless we drop the pseudo-HTTP-0.9 support.

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

[issue28548] http.server parse_request() bug and error reporting

[Roundup Robot]

Roundup Robot added the comment:

New changeset 7c98768368cb by Martin Panter in branch 'default':
Issue #28548: Parse HTTP request version even if too many words received
https://hg.python.org/cpython/rev/7c98768368cb

--
nosy: +python-dev

___
Python tracker 

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

[issue28548] http.server parse_request() bug and error reporting

[Martin Panter]

Changes by Martin Panter :


--
versions: +Python 3.7 -Python 3.5
Added file: http://bugs.python.org/file45300/parse-version.v2.patch

___
Python tracker 

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

[issue28548] http.server parse_request() bug and error reporting

[Barry A. Warsaw]

Barry A. Warsaw added the comment:

On Oct 29, 2016, at 02:20 AM, Martin Panter wrote:

>Here is a patch to parse the version from the request in more cases. Since it
>is more of a cosmetic improvement for handling erroneous requests, I would
>probably only apply it to the next Python version (3.7 atm). Or do you think
>it should go into bug fix versions as well?

That would be a RM decision.

--

___
Python tracker 

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

[issue28548] http.server parse_request() bug and error reporting

[Martin Panter]

Martin Panter added the comment:

Here is a patch to parse the version from the request in more cases. Since it 
is more of a cosmetic improvement for handling erroneous requests, I would 
probably only apply it to the next Python version (3.7 atm). Or do you think it 
should go into bug fix versions as well?

--
keywords: +patch
stage:  -> patch review
Added file: http://bugs.python.org/file45259/parse-version.patch

___
Python tracker 

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

[issue28548] http.server parse_request() bug and error reporting

[Barry A. Warsaw]

Barry A. Warsaw added the comment:

On Oct 28, 2016, at 10:57 PM, Martin Panter wrote:

>Parsing the version from words[-1] rather than words[2] may be a minor
>improvement however.

Indeed; I thought about that too.

--

___
Python tracker 

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

[issue28548] http.server parse_request() bug and error reporting

[Barry A. Warsaw]

Barry A. Warsaw added the comment:

On Oct 28, 2016, at 10:42 PM, Martin Panter wrote:

>I think you should be able to reproduce this without Mailman or tox, by just
>running “python -m http.server”.

Yep, indeed.

>The problem is the “HTTP/0.9” protocol that Python is assuming does not
>include a header section, so there is no place to put a 400 status code or
>header fields. The HTTP 0.9 response is supposed to only be a HTML body; see
> and
>.
>
>I think we should drop HTTP 0.9 response support from Python’s HTTP server,
>as well as the attempted but buggy request support. But there was a bit of
>resistance; see Issue 10721.
>
>Another possibility would be to change default_request_version so that error
>responses are sent as HTTP 1.0. But there may be more fixes needed for this
>to continue the buggy HTTP 0.9 support; see Issue 26578.

I think it may indeed make sense to set default_request_version to 1.0.  It's
probably too late to do that for 3.6, but I do think it makes sense for 3.7.
I'm nosying on the issues you mentioned.

I've proposed essentially that workaround for Mailman.

https://gitlab.com/mailman/mailman/issues/288

We can get away with requiring at least HTTP/1.1 for our REST clients.

--

___
Python tracker 

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

[issue28548] http.server parse_request() bug and error reporting

[Martin Panter]

Martin Panter added the comment:

Parsing the version from words[-1] rather than words[2] may be a minor 
improvement however.

--

___
Python tracker 

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

[issue28548] http.server parse_request() bug and error reporting

[Martin Panter]

Martin Panter added the comment:

I think you should be able to reproduce this without Mailman or tox, by just 
running “python -m http.server”.

The problem is the “HTTP/0.9” protocol that Python is assuming does not include 
a header section, so there is no place to put a 400 status code or header 
fields. The HTTP 0.9 response is supposed to only be a HTML body; see 
 and 
.

I think we should drop HTTP 0.9 response support from Python’s HTTP server, as 
well as the attempted but buggy request support. But there was a bit of 
resistance; see Issue 10721.

Another possibility would be to change default_request_version so that error 
responses are sent as HTTP 1.0. But there may be more fixes needed for this to 
continue the buggy HTTP 0.9 support; see Issue 26578.

--
nosy: +martin.panter

___
Python tracker 

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

[issue28548] http.server parse_request() bug and error reporting

[Barry A. Warsaw]

New submission from Barry A. Warsaw:

This might also affect other Python version; I haven't checked, but I know it 
affects Python 3.5.

In Mailman 3, we have a subclass of WSGIRequestHandler for our REST API, and we 
got a bug report about an error condition returning a 400 in the body of the 
response, but still returning an implicit 200 header.

https://gitlab.com/mailman/mailman/issues/288

This is pretty easily reproducible with the following recipe.

$ git clone https://gitlab.com/mailman/mailman.git
$ cd mailman
$ tox -e py35 --notest -r
$ .tox/py35/bin/python3.5 
/home/barry/projects/mailman/trunk/.tox/py35/bin/runner --runner=rest:0:1 -C 
/home/barry/projects/mailman/trunk/var/etc/mailman.cfg 

(Note that you might need to run `.tox/py35/bin/mailman info` first, and of 
course you'll have to adjust the directories for your own local fork.)

Now, in another shell, do the following:

$ curl -v -i -u restadmin:restpass "http://localhost:8001/3.0/lists/list 
example.com"

Note specifically that there is a space right before the "example.com" bit.

Take note also that we're generating an HTTP/1.1 request as per curl default.

The response you get is:

*   Trying 127.0.0.1...
* Connected to localhost (127.0.0.1) port 8001 (#0)
* Server auth using Basic with user 'restadmin'
> GET /3.0/lists/list example.com HTTP/1.1
> Host: localhost:8001
> Authorization: Basic cmVzdGFkbWluOnJlc3RwYXNz
> User-Agent: curl/7.50.1
> Accept: */*
> 
http://www.w3.org/TR/html4/strict.dtd;>



Error response


Error response
Error code: 400
Message: Bad request syntax ('GET /3.0/lists/list example.com 
HTTP/1.1').
Error code explanation: HTTPStatus.BAD_REQUEST - Bad request syntax 
or unsupported method.


* Connection #0 to host localhost left intact


Notice the lack of response headers, and thus the implicit 200 return even 
though the proper error code is in the body of the response.  Why does this 
happen?

Now look at http.server.BaseHTTPRequestHandler.  The default_request_version is 
"HTTP/0.9".  Given the request, you'd expect to see the version set to 
"HTTP/1.1", but that doesn't happen because the extra space messes up the 
request parsing.  parse_request() splits the request line by spaces and when 
this happens, the wrong number of words shows up.  We get 4 words, thus the 
'else:' clause in parse_request() gets triggered.  So far so good.

This eventually leads us to send_error() and from there into send_response() 
with the error code (properly 400) and message.  From there we get to 
.send_response_only() and tracing into this function shows you where things go 
wrong.

send_response_only() has an explicit test on self.request_version != 
'HTTP/0.9', in which case it adds nothing to the _header_buffer.  Well, because 
the request parsing got the unexpected number of words, in fact request_version 
*is* the default HTTP/0.9.  Thus the error headers are never added.

There are several problems here.  Why are the headers never added when the 
request is HTTP/0.9?  (I haven't read the spec.)  Also, since parse_request() 
isn't setting the request version to 'HTTP/1.1'.  It should probably dig out 
the words[-1] and see if that looks like a version string.

Clearly the request isn't properly escaped, but http.server should not be 
sending an implicit 200 when the request is bogus.

--
components: Library (Lib)
messages: 279611
nosy: barry
priority: normal
severity: normal
status: open
title: http.server parse_request() bug and error reporting
versions: Python 3.5

___
Python tracker 

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