Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

plain text http responses to errors #3483

Merged
merged 12 commits into from
Jan 7, 2019
Merged

plain text http responses to errors #3483

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
90d6e3e
plain text http responses to errors
samuelcolvin Jan 4, 2019
11f9b9a
fix html error body
samuelcolvin Jan 4, 2019
85578d6
tweak tests, use plain text for non 500 response
samuelcolvin Jan 4, 2019
1e751aa
cleanup handle_error
samuelcolvin Jan 4, 2019
188f810
cleanup handle_error more
samuelcolvin Jan 4, 2019
8f80fac
include 'Server got itself in trouble' each time
samuelcolvin Jan 4, 2019
903d5c4
Apply suggestions from code review
webknjaz Jan 4, 2019
f30b8bb
Construct HTTP 500 page title from response const
webknjaz Jan 4, 2019
1afc5ec
Drop invalid debug function call from tests
webknjaz Jan 4, 2019
45c92b8
Make flake8 happy
webknjaz Jan 4, 2019
fea25c1
Construct http-500 error page title with format
webknjaz Jan 4, 2019
d76270a
fix title and remove unnecessary extra message
samuelcolvin Jan 7, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGES/3483.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Internal Server Errors in plain text if the browser does not support HTML.
2 changes: 1 addition & 1 deletion aiohttp/test_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,7 @@ async def _make_runner(self,
debug: bool=True,
**kwargs: Any) -> ServerRunner:
srv = Server(
self._handler, loop=self._loop, debug=True, **kwargs)
self._handler, loop=self._loop, debug=debug, **kwargs)
return ServerRunner(srv, debug=debug, **kwargs)


Expand Down
31 changes: 19 additions & 12 deletions aiohttp/web_protocol.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
from collections import deque
from contextlib import suppress
from html import escape as html_escape
from http import HTTPStatus
from logging import Logger
from typing import (
TYPE_CHECKING,
Expand Down Expand Up @@ -523,24 +524,30 @@ def handle_error(self,
information. It always closes current connection."""
self.log_exception("Error handling request", exc_info=exc)

if status == 500:
msg = "<h1>500 Internal Server Error</h1>"
ct = 'text/plain'
if status == HTTPStatus.INTERNAL_SERVER_ERROR:
title = '500 Internal Server Error'
Copy link
Member

@webknjaz webknjaz Jan 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
title = '500 Internal Server Error'
title = ' '.join((str(HTTPStatus.INTERNAL_SERVER_ERROR.value), HTTPStatus.INTERNAL_SERVER_ERROR.phrase))

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

In [1]: import http                                                                                                                                                                            

In [3]: http.HTTPStatus.INTERNAL_SERVER_ERROR                                                                                                                                                  
Out[3]: <HTTPStatus.INTERNAL_SERVER_ERROR: 500>

In [5]: http.HTTPStatus.INTERNAL_SERVER_ERROR.phrase                                                                                                                                           
Out[5]: 'Internal Server Error'

In [6]: http.HTTPStatus.INTERNAL_SERVER_ERROR.value                                                                                                                                            
Out[6]: 500

In [8]: http.HTTPStatus.INTERNAL_SERVER_ERROR.description                                                                                                                                      
Out[8]: 'Server got itself in trouble'

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@webknjaz honestly the original (a plain string) looks more readable for me.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I can live with any variant, too negligeable reason for contestion.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. I'd probably go for

Suggested change
title = '500 Internal Server Error'
title = str(HTTPStatus.INTERNAL_SERVER_ERROR.value) + ' ' + HTTPStatus.INTERNAL_SERVER_ERROR.phrase

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it be better readable?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit surprised that http.HTTPStatus doesn't have a method to construct a status string. IMHO it should be smth like title = http.HTTPStatus.INTERNAL_SERVER_ERROR.to_status_string()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HTTP reason is gone in htttp/2 BTW.
I suspect that very many Web frameworks have own copies of these constants.
+0 for adding to_status_string() to CPython, at least I don't want to make a Pull Request (but you can count me on the PR review).

msg = HTTPStatus.INTERNAL_SERVER_ERROR.description
tb = None
if self.debug:
with suppress(Exception):
tb = traceback.format_exc()

if 'text/html' in request.headers.get('Accept', ''):
if tb:
tb = html_escape(tb)
msg += '<br><h2>Traceback:</h2>\n<pre>'
msg += tb
msg += '</pre>'
msg = '<h2>Traceback:</h2>\n<pre>{}</pre>'.format(tb)
message = (
"<html><head>"
"<title>{title}</title>"
"</head><body>\n<h1>{title}</h1>"
"<br>\n{msg}\n</body></html>\n"
).format(title=title, msg=msg)
ct = 'text/html'
else:
msg += "Server got itself in trouble"
msg = ("<html><head><title>500 Internal Server Error</title>"
"</head><body>" + msg + "</body></html>")
resp = Response(status=status, text=msg, content_type='text/html')
else:
resp = Response(status=status, text=message,
content_type='text/html')
message = title + '\n\n' + (tb or msg)
samuelcolvin marked this conversation as resolved.
Show resolved Hide resolved

resp = Response(status=status, text=message, content_type=ct)
resp.force_close()

# some data already got sent, connection is broken
Expand Down
2 changes: 1 addition & 1 deletion tests/test_web_protocol.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,7 +299,7 @@ async def test_handle_error__utf(
await asyncio.sleep(0)

assert b'HTTP/1.0 500 Internal Server Error' in buf
assert b'Content-Type: text/html; charset=utf-8' in buf
assert b'Content-Type: text/plain; charset=utf-8' in buf
pattern = escape("RuntimeError: что-то пошло не так")
assert pattern.encode('utf-8') in buf
assert not srv._keepalive
Expand Down
60 changes: 57 additions & 3 deletions tests/test_web_server.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,13 +26,15 @@ async def handler(request):
raise exc

logger = mock.Mock()
server = await aiohttp_raw_server(handler, logger=logger)
server = await aiohttp_raw_server(handler, logger=logger, debug=False)
samuelcolvin marked this conversation as resolved.
Show resolved Hide resolved
cli = await aiohttp_client(server)
resp = await cli.get('/path/to')
assert resp.status == 500
assert resp.headers['Content-Type'].startswith('text/plain')

txt = await resp.text()
assert "<h1>500 Internal Server Error</h1>" in txt
assert txt.startswith("500 Internal Server Error")
samuelcolvin marked this conversation as resolved.
Show resolved Hide resolved
assert 'Traceback' not in txt

logger.exception.assert_called_with(
"Error handling request",
Expand Down Expand Up @@ -102,10 +104,62 @@ async def handler(request):
cli = await aiohttp_client(server)
resp = await cli.get('/path/to')
assert resp.status == 500
assert resp.headers['Content-Type'].startswith('text/plain')

txt = await resp.text()
assert "<h2>Traceback:</h2>" in txt
assert 'Traceback (most recent call last):\n' in txt

logger.exception.assert_called_with(
"Error handling request",
exc_info=exc)


async def test_raw_server_html_exception(aiohttp_raw_server, aiohttp_client):
exc = RuntimeError("custom runtime error")

async def handler(request):
raise exc

logger = mock.Mock()
server = await aiohttp_raw_server(handler, logger=logger, debug=False)
samuelcolvin marked this conversation as resolved.
Show resolved Hide resolved
cli = await aiohttp_client(server)
resp = await cli.get('/path/to', headers={'Accept': 'text/html'})
assert resp.status == 500
assert resp.headers['Content-Type'].startswith('text/html')

txt = await resp.text()
assert txt == (
'<html><head><title>500 Internal Server Error</title></head><body>\n'
'<h1>500 Internal Server Error</h1><br>\n'
'Server got itself in trouble\n'
'</body></html>\n'
)

logger.exception.assert_called_with(
"Error handling request", exc_info=exc)


async def test_raw_server_html_exception_debug(aiohttp_raw_server,
aiohttp_client):
exc = RuntimeError("custom runtime error")

async def handler(request):
raise exc

logger = mock.Mock()
server = await aiohttp_raw_server(handler, logger=logger, debug=True)
cli = await aiohttp_client(server)
resp = await cli.get('/path/to', headers={'Accept': 'text/html'})
assert resp.status == 500
assert resp.headers['Content-Type'].startswith('text/html')

txt = await resp.text()
assert txt.startswith(
'<html><head><title>500 Internal Server Error</title></head><body>\n'
'<h1>500 Internal Server Error</h1><br>\n'
'<h2>Traceback:</h2>\n'
'<pre>Traceback (most recent call last):\n'
)

logger.exception.assert_called_with(
"Error handling request", exc_info=exc)