http.server.BaseHTTPRequestHandler.send_error , ability send a detailed response #57130
Comments
|
Calling http.server.BaseHTTPRequestHandler.send_error(code,message) with a message that contains a trailing newline does not display properly in Firefox, Chrome. Listing 1 #!/usr/bin/env python3.2 import http.server
import traceback
class httphandler(http.server.BaseHTTPRequestHandler):
def do_GET(self):
try:
assert(False)
except:
self.send_error(500,traceback.format_exc())
if __name__=='__main__':
addr=('',9000)
http.server.HTTPServer(addr,httphandler).serve_forever()Consider Listing 1. A typical programming idiom would be to wrap do_GET with a try/except block that reports an HTTP error with an HTML formatted stack trace. However, when I view this with Firefox and Chrome the error message is unformatted, i.e. raw HTML is displayed. A simple workaround is to call strip() on the message. This could be suggested to the user in the docs, or added as an automatic preprocessing feature to the library. With strip(), the message is formatted. Adding or documenting strip() resolves the bug. The remainder of this report is a feature request. The default error_message_format is probably not what people want for a stack trace. It leaves formatting of whitespace to the HTML which removes the newlines. This is desirable for short, unformatted messages but undesirable for a preformatted traceback. In Listing 2 I give a working solution and suggest including it in the library if the community feels that preformatted messages are common enough to warrant extra attention. I feel it is since "try/except: print traceback" is almost mandatory for error prone internet operations. Listing 2 #!/usr/bin/env python3.2 import http.server
import traceback
class httphandler(http.server.BaseHTTPRequestHandler):
def content_aware_error_message_format(self,m):
oldfmt='<p>Message: %(message)s.\n'
if oldfmt in self.error_message_format:
# use <pre> if the message contains a newline internally
# otherwise let the html control line breaks
self.error_message_format=self.error_message_format.replace(oldfmt,'<p><pre>%(message)s</pre>\n') if '\n' in m else self.error_message_format.replace(oldfmt,'<p>%(message)s\n')
def do_GET(self):
try:
assert(False)
except:
m=traceback.format_exc().strip()
self.content_aware_error_message_format(m)
self.send_error(500,m)
if __name__=='__main__':
addr=('',9000)
http.server.HTTPServer(addr,httphandler).serve_forever() |
PaulUpchurch
mannequin
added
type-bug
|
Testing your code in Listing 1 → curl -sI http://localhost:9000/ So this is normal, http://tools.ietf.org/html/draft-ietf-httpbis-p2-semantics-22#section-6.6.2 except that it would be better to use "501 Not Implemented" through the prose is optional. The content-type is also kind of useless. That would deserve to open another bug. And → curl http://localhost:9000/ <!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN"
"http://www.w3.org/TR/html4/strict.dtd">
<html>
<head>
<meta http-equiv="Content-Type" content="text/html;charset=utf-8">
<title>Error response</title>
</head>
<body>
<h1>Error response</h1>
<p>Error code: 500</p>
<p>Message: Traceback (most recent call last):
File "server.py", line 9, in do_GET
assert(False)
AssertionError
.</p>
<p>Error code explanation: 500 - Server got itself in trouble.</p>
</body>
</html>OK. The server is answering with HTTP/1.0 and then a Traceback… which has nothing to do here. We can see that in more details with a telnet → telnet localhost 9000 HTTP/1.0 500 Traceback (most recent call last):
File "server.py", line 9, in do_GET
assert(False)
AssertionErrorServer: BaseHTTP/0.6 Python/3.3.0 <!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN"
"http://www.w3.org/TR/html4/strict.dtd">
<html>
<head>
<meta http-equiv="Content-Type" content="text/html;charset=utf-8">
<title>Error response</title>
</head>
<body>
<h1>Error response</h1>
<p>Error code: 500</p>
<p>Message: Traceback (most recent call last):
File "server.py", line 9, in do_GET
assert(False)
AssertionError
.</p>
<p>Error code explanation: 500 - Server got itself in trouble.</p>
</body>
</html>Note that when not sending the traceback with the following code #!/usr/bin/env python3.3 import http.server
import traceback
class httphandler(http.server.BaseHTTPRequestHandler):
def do_GET(self):
try:
assert(False)
except:
self.send_error(500)
if __name__=='__main__':
addr=('',9000)
http.server.HTTPServer(addr,httphandler).serve_forever()Everything is working well. → telnet localhost 9000 HTTP/1.0 500 Internal Server Error <!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN" I'm looking at http://hg.python.org/cpython/file/3.3/Lib/http/server.py#l404 For the second part of your message. I don't think the two issues should be mixed. Maybe open another bug report. |
|
OK I had understand a bit better. self.send_error(code, msg) is used for
That's bad, very bad. I do not think it should be used for the HTTP Header at all. |
|
ok I modify the code of server.py so that the server doesn't send the private message but the one which is already assigned by the library as it should. If there is a need for customization, there should be two separate variables, but which could lead to the same issues. After modifications this is what I get. → telnet localhost 9000 HTTP/1.0 500 Internal Server Error <!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN"
"http://www.w3.org/TR/html4/strict.dtd">
<html>
<head>
<meta http-equiv="Content-Type" content="text/html;charset=utf-8">
<title>Error response</title>
</head>
<body>
<h1>Error response</h1>
<p>Error code: 500</p>
<p>Message: Traceback (most recent call last):
File "server.py", line 11, in do_GET
assert(False)
AssertionError
.</p>
<p>Error code explanation: 500 - Server got itself in trouble.</p>
</body>
</html>
Connection closed by foreign host.I joined the patch: server.bpo-12921.patch |
|
orsenthil, I made a proper patch for it with hg diff. It is very short. |
|
Karl, |
|
orsenthil, When you have done a patch for testing I would love to see it. I could not find a proper way of doing it. I'm eager to learn. Thanks. |
|
New changeset c31d700dea8b by Senthil Kumaran in branch '2.7': New changeset 5126e62c60af by Senthil Kumaran in branch '3.2': New changeset 5d76a4746d9d by Senthil Kumaran in branch '3.3': New changeset b87792757ee8 by Senthil Kumaran in branch 'default': |
|
Karl - thanks for your telnet debugging session output. Helped realized the problem better. So I had been thinking that sending message is okay. But had not realized that same variable name was used and was causing problem. I have gone ahead with the fix for now. And for the tests, I think, will be written very similar to test_header_buffering_of_send_error of test_header_bufferring_of_send_error - when send_error is sent and then error asserted. (Usually, I try to change test and patch together and that's our protocol too. In this case, I only tested it manually, thinking that there is not test coverage for this portion (yet). But later when I peeked into tests, I saw that it would be written for this scenario). Will include and then close this report. The other suggestion of Paul can be included in another report and so the change of the error status to proper 501 Not Implemented. Thanks. |
|
There is a test failure. which leads me to believe that "Error message" may probably be relied upon by some applications. FAIL: test_header_length (test.test_httpservers.BaseHTTPRequestHandlerTestCase) Traceback (most recent call last):
File "/export/home/buildbot/64bits/3.2.cea-indiana-amd64/build/Lib/test/test_httpservers.py", line 605, in test_header_length
self.assertEqual(result[0], b'HTTP/1.1 400 Line too long\r\n')
AssertionError: b'HTTP/1.1 400 Bad Request\r\n' != b'HTTP/1.1 400 Line too long\r\n'I think, I am going revert the change I made first. Because, looking at documentation and usage, leads me to believe that we are bit assuming much from a misleading example. send_error(code, message) - message here is optional, but when sent, the server is responsible for displaying it. It is *never* advertised that the traceback can be sent as a message. However, I do think there a chance for improvement and that is the reason, I went ahead with the change. The improvement can be send_error - response header could be the uniform shortmsg and response body can be the message that is sent by send_error(code, message). Now, this will be improvement and it should be discussed in that aspect rather than as a fix for anything. Going ahead with the revert. Sorry for the mistake. |
|
New changeset 4e6c46d5f77d by Senthil Kumaran in branch '2.7': New changeset 637d7c953b10 by Senthil Kumaran in branch '3.2': New changeset 84e7a7f6ddb8 by Senthil Kumaran in branch '3.3': New changeset b0890674bc21 by Senthil Kumaran in branch 'default': |
|
The culprit is here That an application or a person decides to send another message is ok. Designer choice. That the library is sending something optional as a test seems more uncomfortable. The list of codes for 4xx is declared at In HTTP, only the code is mandatory, the text is optional. But indeed you reveal a loophole, which means that someone might want to send a message for the body. That said, I still do not think it should lend in the header for reasons explained previously (serious breakage). I will come up with something which is fixing the issue without breaking the existent. That said, do I open another bug for the test? The test should be fixed too. It should test 400 only. and not the full status-line. The previous test is also an issue, testing 414. Because the prose in the spec is "URI Too Long" and not "HTTP/1.1 414 Request-URI Too Long" Hmmm… I see all tests are like this. It should be fixed. Opening bug? Thought? |
|
Senthil, I created another bug reports for the tests which are fragile. |
|
Which I closed, sorry. I agree it should be possible to control the text produced with the error code separately from the text in the body. I prefer the specific text produced by our http server to the generic message, but I would only be -0 on changing this so that the default was the generic message and the specific message only went in the body. Since changing that would change the behavior of existing code, this is an enhacement request and not a bug fix. (That is, it may be a design bug, but those get fixed as enhancements :) Either way, the httpserver tests will need to be modified to test the API enhancement, and the possible default text change, proposed here. |
bitdancer
added
type-feature
|
hehe. No hard feelings. I still do not think it is a good idea to test the "error code" and its associated message in the same test. :) For example, in RFC2616, 414 is defined as and in the HTTP1.1bis (which will not get a new version number) because the goal of the work was to just clarify and not make incompatible changes, 414 is defined as which is fine because the message is optional. With the current tests, it will make it hard to modify :) # More about this specific issue Right now, send_error groks everything, which is not very good in terms of security and side effects. def send_error(self, code, message=None):Then later on:
When the message is not defined it takes the shortmsg but if defined it sends everything whatever it is Checking the status-line 3.1.2. Status Line The first line of a response message is the status-line, consisting The status-code element is a 3-digit integer code describing the The reason-phrase element exists for the sole purpose of providing a
A hacky way to mitigate the issue would be to
Thoughts? |
|
Your suggestion could probably be applied as a bug fix to maintenance releases, but is it worth the effort? We don't generally try to protect programmers from themselves in Python. On the other hand, there should clearly be a way to provide the 'explain' text as well as the message text. In 3.4 we could add an 'explain' keyword to send_error. If only explain is specified, we'd use the shortmsg for message. That way, a programmer writing new code can make it work the way they want it to, while existing code will continue to work the way it used to. |
|
R.david Trying another patch just for understanding if it's what you meant. What it does:
For the part 3, TODO: |
|
Karl - I reviewed the patch and like it. Here are some comments. At first, I did not see the need for both "message" and "explain" in the API both having almost similar purposes. But given that "explain" is already used by send_error and at the moment, un-customizable and I think it is barely useful. Your 3rd patch may improve it's utility value a bit further. One review comment:
+ {'code': code, 'message': message, 'explain': _quote_html(explain)}) I would go with _quote_html(message) too. That was fix for XSS security issue. Also. I will not confuse the newline checking with status line in the same patch /commit. I have a suspicion that RFC says Status Line should be a single line and you went ahead with including that in patch. Is that correct? If you referred to any specific section of RFC, could you point me to that? Thanks! |
|
I have added explain argument in 3.4, committed changeset 82669:59292f366b53 I think the short message no trailing line assertion should be taken by next. Either a new issue it if already open or create one just for the purpose. Thanks Karl for the patch. |
orsenthil
changed the title
|
Thanks! Yes we should open a separate issue. I will look at the current patch and see what I can propose for the next step. |
|
just a minor issue you made a mistake in the commit message. The issue number is 12921 |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: