classification
Title: http.server.BaseHTTPRequestHandler send_response_only doesn't check the type and value of the code.
Type: enhancement Stage: patch review
Components: Library (Lib) Versions: Python 3.5
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: berker.peksag, dmi.baranov, karlcow, martin.panter, orsenthil, yselivanov
Priority: normal Keywords: patch

Created on 2013-02-28 04:12 by karlcow, last changed 2015-09-19 06:28 by martin.panter.

Files
File name Uploaded Description Edit
issue17319.patch dmi.baranov, 2013-05-01 20:53 review
Messages (4)
msg183201 - (view) Author: karl (karlcow) * Date: 2013-02-28 04:12
def send_response_only(self, code, message=None):
http://hg.python.org/cpython/file/3.3/Lib/http/server.py#l448

There is no type checking on code or if the code is appropriate. Let's take 


==============================================
#!/usr/bin/env python3.3
import http.server


class HTTPHandler(http.server.BaseHTTPRequestHandler):
    "A very simple server"
    def do_GET(self):
        self.send_response(200)
        self.send_header('Content-type', 'text/plain')
        self.end_headers()
        self.wfile.write(bytes('Response body\n\n', 'latin1'))

if __name__ == '__main__':
    addr = ('', 9000)
    http.server.HTTPServer(addr, HTTPHandler).serve_forever()
=====================================================

A request is working well.

=========================================
→ http GET localhost:9000
HTTP/1.0 200 OK
Server: BaseHTTP/0.6 Python/3.3.0
Date: Thu, 28 Feb 2013 04:00:44 GMT
Content-type: text/plain

Response body

=========================================

And the server log is 

127.0.0.1 - - [27/Feb/2013 23:00:44] "GET / HTTP/1.1" 200 -

Then let's try


=========================================
#!/usr/bin/env python3.3
import http.server


class HTTPHandler(http.server.BaseHTTPRequestHandler):
    "A very simple server"
    def do_GET(self):
        self.send_response(999)
        self.send_header('Content-type', 'text/plain')
        self.end_headers()
        self.wfile.write(bytes('Response body\n\n', 'latin1'))

if __name__ == '__main__':
    addr = ('', 9000)
    http.server.HTTPServer(addr, HTTPHandler).serve_forever()
=========================================

The response is

=========================================
→ http GET localhost:9000
HTTP/1.0 999 
Server: BaseHTTP/0.6 Python/3.3.0
Date: Thu, 28 Feb 2013 03:55:54 GMT
Content-type: text/plain

Response body

=========================================

and the log server is

127.0.0.1 - - [27/Feb/2013 22:55:12] "GET / HTTP/1.1" 999 -

And finally 

=========================================
#!/usr/bin/env python3.3
import http.server


class HTTPHandler(http.server.BaseHTTPRequestHandler):
    "A very simple server"
    def do_GET(self):
        self.send_response('foobar')
        self.send_header('Content-type', 'text/plain')
        self.end_headers()
        self.wfile.write(bytes('Response body\n\n', 'latin1'))

if __name__ == '__main__':
    addr = ('', 9000)
    http.server.HTTPServer(addr, HTTPHandler).serve_forever()
=========================================

The response is then

=========================================
→ http GET localhost:9000
HTTPConnectionPool(host='localhost', port=9000): Max retries exceeded with url: /
=========================================

and the server log is 

127.0.0.1 - - [27/Feb/2013 22:56:39] "GET / HTTP/1.1" foobar -
----------------------------------------
Exception happened during processing of request from ('127.0.0.1', 53917)
Traceback (most recent call last):
  File "/Library/Frameworks/Python.framework/Versions/3.3/lib/python3.3/socketserver.py", line 306, in _handle_request_noblock
    self.process_request(request, client_address)
  File "/Library/Frameworks/Python.framework/Versions/3.3/lib/python3.3/socketserver.py", line 332, in process_request
    self.finish_request(request, client_address)
  File "/Library/Frameworks/Python.framework/Versions/3.3/lib/python3.3/socketserver.py", line 345, in finish_request
    self.RequestHandlerClass(request, client_address, self)
  File "/Library/Frameworks/Python.framework/Versions/3.3/lib/python3.3/socketserver.py", line 666, in __init__
    self.handle()
  File "/Library/Frameworks/Python.framework/Versions/3.3/lib/python3.3/http/server.py", line 400, in handle
    self.handle_one_request()
  File "/Library/Frameworks/Python.framework/Versions/3.3/lib/python3.3/http/server.py", line 388, in handle_one_request
    method()
  File "../25/server.py", line 8, in do_GET
    self.send_response('foobar')
  File "/Library/Frameworks/Python.framework/Versions/3.3/lib/python3.3/http/server.py", line 444, in send_response
    self.send_response_only(code, message)
  File "/Library/Frameworks/Python.framework/Versions/3.3/lib/python3.3/http/server.py", line 459, in send_response_only
    (self.protocol_version, code, message)).encode(
TypeError: %d format: a number is required, not str
----------------------------------------

Both error messages and server logs are not very helpful. 
Shall we fix it? 
What others think?

I guess there should be test cases too?
I'm happy to make unit test cases for it though I might need a bit of guidance as I'm not comfortable with unit test cases mocking connections.
msg188242 - (view) Author: Dmi Baranov (dmi.baranov) * Date: 2013-05-01 20:53
Attached patch for checking status code based at RFC 2616 [1].
Covered by tests.

[1] http://tools.ietf.org/html/rfc2616#section-6.1.1
msg227506 - (view) Author: karl (karlcow) * Date: 2014-09-25 05:55
Where this is defined in the new RFC.

http://tools.ietf.org/html/rfc7230#section-3.1.2

     status-line = HTTP-version SP status-code SP reason-phrase CRLF

Things to enforce

     status-code    = 3DIGIT

Response status code are now defined in 
http://tools.ietf.org/html/rfc7231#section-6
with something important.

   HTTP status codes are extensible.  HTTP clients are not required to
   understand the meaning of all registered status codes, though such
   understanding is obviously desirable.  However, a client MUST
   understand the class of any status code, as indicated by the first
   digit, and treat an unrecognized status code as being equivalent to
   the x00 status code of that class, with the exception that a
   recipient MUST NOT cache a response with an unrecognized status code.

   For example, if an unrecognized status code of 471 is received by a
   client, the client can assume that there was something wrong with its
   request and treat the response as if it had received a 400 (Bad
   Request) status code.  The response message will usually contain a
   representation that explains the status.

That should help.

The full registry of status code is defined here
http://www.iana.org/assignments/http-status-codes/http-status-codes.xhtml


@dmi.baranov 

In the patch
+def _is_valid_status_code(code):
+        return isinstance(code, int) and 0 <= code <= 999

Maybe there is a missing check where the len(str(code)) == 3
msg251054 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-09-19 06:28
The proposed patch changes the code to 500 if the code is invalid (rather than raising an exception as I initially assumed).

I would be inclined to leave send_response() without any extra error checking or handling, unless this is a common problem and there is a real need for it. Although I reckon it might be nice to have a generic (higher-level) exception handler for the HTTP server that responds with “500 Internal server error” if possible.
History
Date User Action Args
2015-09-19 06:28:46martin.pantersetnosy: + martin.panter
messages: + msg251054
2015-09-19 06:23:43martin.pantersetmessages: - msg227647
2015-09-19 06:23:26martin.pantersetmessages: - msg227645
2014-09-26 20:32:04yselivanovsetmessages: + msg227647
2014-09-26 20:30:23yselivanovsetnosy: + yselivanov

messages: + msg227645
versions: + Python 3.5, - Python 3.4
2014-09-25 05:55:44karlcowsetmessages: + msg227506
2013-05-02 05:05:34berker.peksagsetnosy: + berker.peksag
stage: patch review

versions: + Python 3.4, - Python 3.3
2013-05-01 20:53:14dmi.baranovsetfiles: + issue17319.patch

nosy: + dmi.baranov
messages: + msg188242

keywords: + patch
2013-02-28 04:12:05karlcowcreate