From 90d6e3e2402bee3b60ae404463bd047398e77b34 Mon Sep 17 00:00:00 2001 From: Samuel Colvin Date: Fri, 4 Jan 2019 14:01:56 +0000 Subject: [PATCH 01/12] plain text http responses to errors --- aiohttp/test_utils.py | 2 +- aiohttp/web_protocol.py | 41 +++++++++++++++++++++--------- tests/test_web_protocol.py | 2 +- tests/test_web_server.py | 52 +++++++++++++++++++++++++++++++++++--- 4 files changed, 80 insertions(+), 17 deletions(-) diff --git a/aiohttp/test_utils.py b/aiohttp/test_utils.py index 177441a42d7..d1db1a76c8d 100644 --- a/aiohttp/test_utils.py +++ b/aiohttp/test_utils.py @@ -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) diff --git a/aiohttp/web_protocol.py b/aiohttp/web_protocol.py index fc72a982b06..33bde796fda 100644 --- a/aiohttp/web_protocol.py +++ b/aiohttp/web_protocol.py @@ -1,5 +1,6 @@ import asyncio import asyncio.streams +import textwrap import traceback import warnings from collections import deque @@ -524,19 +525,35 @@ def handle_error(self, self.log_exception("Error handling request", exc_info=exc) if status == 500: - msg = "

500 Internal Server Error

" - if self.debug: - with suppress(Exception): - tb = traceback.format_exc() - tb = html_escape(tb) - msg += '

Traceback:

\n
'
-                    msg += tb
-                    msg += '
' + if 'text/html' in request.headers.get('Accept', ''): + msg = "

500 Internal Server Error

" + if self.debug: + with suppress(Exception): + tb = traceback.format_exc() + tb = html_escape(tb) + msg += '

Traceback:

\n
'
+                        msg += tb
+                        msg += '
' + else: + msg += "Server got itself in trouble" + msg = ( + "" + "500 Internal Server Error" + "{msg}" + ).format(msg) + resp = Response(status=status, text=msg, + content_type='text/html') else: - msg += "Server got itself in trouble" - msg = ("500 Internal Server Error" - "" + msg + "") - resp = Response(status=status, text=msg, content_type='text/html') + msg = '500 Internal Server Error' + if self.debug: + with suppress(Exception): + msg += '\n\nTraceback:\n' + tb = traceback.format_exc() + msg += textwrap.indent(tb, ' ') + else: + msg += '\n\nServer got itself in trouble' + resp = Response(status=status, text=msg, + content_type='text/plain') else: resp = Response(status=status, text=message, content_type='text/html') diff --git a/tests/test_web_protocol.py b/tests/test_web_protocol.py index 829d1ec4fae..49d0dd7e982 100644 --- a/tests/test_web_protocol.py +++ b/tests/test_web_protocol.py @@ -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 diff --git a/tests/test_web_server.py b/tests/test_web_server.py index 7fe714b578e..9b5cd937a38 100644 --- a/tests/test_web_server.py +++ b/tests/test_web_server.py @@ -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) 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 "

500 Internal Server Error

" in txt + assert txt.startswith("500 Internal Server Error") + assert "Traceback:" not in txt logger.exception.assert_called_with( "Error handling request", @@ -102,10 +104,54 @@ 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 "

Traceback:

" in txt + assert "Traceback:" 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) + 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("

500 Internal Server Error

") + assert "Traceback" not in txt + + 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("

500 Internal Server Error

") + assert "

Traceback:

" in txt + + logger.exception.assert_called_with( + "Error handling request", exc_info=exc) From 11f9b9a1fd405cbffe30a199092a24463434138f Mon Sep 17 00:00:00 2001 From: Samuel Colvin Date: Fri, 4 Jan 2019 15:08:03 +0000 Subject: [PATCH 02/12] fix html error body --- CHANGES/3483.feature | 1 + aiohttp/web_protocol.py | 27 +++++++++++---------------- tests/test_web_server.py | 17 +++++++++++++---- 3 files changed, 25 insertions(+), 20 deletions(-) create mode 100644 CHANGES/3483.feature diff --git a/CHANGES/3483.feature b/CHANGES/3483.feature new file mode 100644 index 00000000000..082f66b2803 --- /dev/null +++ b/CHANGES/3483.feature @@ -0,0 +1 @@ +Internal Server Errors in plain text if the browser does not support HTML. diff --git a/aiohttp/web_protocol.py b/aiohttp/web_protocol.py index 33bde796fda..7e615697771 100644 --- a/aiohttp/web_protocol.py +++ b/aiohttp/web_protocol.py @@ -1,6 +1,5 @@ import asyncio import asyncio.streams -import textwrap import traceback import warnings from collections import deque @@ -526,32 +525,28 @@ def handle_error(self, if status == 500: if 'text/html' in request.headers.get('Accept', ''): - msg = "

500 Internal Server Error

" if self.debug: with suppress(Exception): tb = traceback.format_exc() tb = html_escape(tb) - msg += '

Traceback:

\n
'
-                        msg += tb
-                        msg += '
' + msg = '

Traceback:

\n
{}
'.format(tb) else: - msg += "Server got itself in trouble" - msg = ( - "" - "500 Internal Server Error" - "{msg}" - ).format(msg) + msg = 'Server got itself in trouble' + msg = ( + "" + "500 Internal Server Error" + "\n

500 Internal Server Error

" + "
\n{msg}\n\n" + ).format(msg=msg) resp = Response(status=status, text=msg, content_type='text/html') else: - msg = '500 Internal Server Error' if self.debug: with suppress(Exception): - msg += '\n\nTraceback:\n' - tb = traceback.format_exc() - msg += textwrap.indent(tb, ' ') + msg = traceback.format_exc() else: - msg += '\n\nServer got itself in trouble' + msg = 'Server got itself in trouble' + msg = '500 Internal Server Error\n\n' + msg resp = Response(status=status, text=msg, content_type='text/plain') else: diff --git a/tests/test_web_server.py b/tests/test_web_server.py index 9b5cd937a38..d6988d20e72 100644 --- a/tests/test_web_server.py +++ b/tests/test_web_server.py @@ -107,7 +107,7 @@ async def handler(request): assert resp.headers['Content-Type'].startswith('text/plain') txt = await resp.text() - assert "Traceback:" in txt + assert 'Traceback (most recent call last):\n' in txt logger.exception.assert_called_with( "Error handling request", @@ -128,7 +128,12 @@ async def handler(request): assert resp.headers['Content-Type'].startswith('text/html') txt = await resp.text() - assert txt.startswith("

500 Internal Server Error

") + assert txt == ( + '500 Internal Server Error\n' + '

500 Internal Server Error


\n' + 'Server got itself in trouble\n' + '\n' + ) assert "Traceback" not in txt logger.exception.assert_called_with( @@ -150,8 +155,12 @@ async def handler(request): assert resp.headers['Content-Type'].startswith('text/html') txt = await resp.text() - assert txt.startswith("

500 Internal Server Error

") - assert "

Traceback:

" in txt + assert txt.startswith( + '500 Internal Server Error\n' + '

500 Internal Server Error


\n' + '

Traceback:

\n' + '
Traceback (most recent call last):\n'
+    )
 
     logger.exception.assert_called_with(
         "Error handling request", exc_info=exc)

From 85578d6ddf660364b4b81698e6ea5abfdfdd2807 Mon Sep 17 00:00:00 2001
From: Samuel Colvin 
Date: Fri, 4 Jan 2019 17:24:27 +0000
Subject: [PATCH 03/12] tweak tests, use plain text for non 500 response

---
 aiohttp/web_protocol.py  | 2 +-
 tests/test_web_server.py | 3 +--
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/aiohttp/web_protocol.py b/aiohttp/web_protocol.py
index 7e615697771..7b1b914bb9e 100644
--- a/aiohttp/web_protocol.py
+++ b/aiohttp/web_protocol.py
@@ -551,7 +551,7 @@ def handle_error(self,
                                 content_type='text/plain')
         else:
             resp = Response(status=status, text=message,
-                            content_type='text/html')
+                            content_type='text/plain')
 
         resp.force_close()
 
diff --git a/tests/test_web_server.py b/tests/test_web_server.py
index d6988d20e72..c84506c823d 100644
--- a/tests/test_web_server.py
+++ b/tests/test_web_server.py
@@ -34,7 +34,7 @@ async def handler(request):
 
     txt = await resp.text()
     assert txt.startswith("500 Internal Server Error")
-    assert "Traceback:" not in txt
+    assert 'Traceback' not in txt
 
     logger.exception.assert_called_with(
         "Error handling request",
@@ -134,7 +134,6 @@ async def handler(request):
         'Server got itself in trouble\n'
         '\n'
     )
-    assert "Traceback" not in txt
 
     logger.exception.assert_called_with(
         "Error handling request", exc_info=exc)

From 1e751aacbcd6c9d4c8dde2be89313f58b97c9e6e Mon Sep 17 00:00:00 2001
From: Samuel Colvin 
Date: Fri, 4 Jan 2019 18:03:27 +0000
Subject: [PATCH 04/12] cleanup handle_error

---
 aiohttp/web_protocol.py | 22 +++++++---------------
 1 file changed, 7 insertions(+), 15 deletions(-)

diff --git a/aiohttp/web_protocol.py b/aiohttp/web_protocol.py
index 7b1b914bb9e..7ad4ea496f1 100644
--- a/aiohttp/web_protocol.py
+++ b/aiohttp/web_protocol.py
@@ -523,36 +523,28 @@ def handle_error(self,
         information. It always closes current connection."""
         self.log_exception("Error handling request", exc_info=exc)
 
+        ct = 'text/plain'
         if status == 500:
+            msg = 'Server got itself in trouble'
             if 'text/html' in request.headers.get('Accept', ''):
                 if self.debug:
                     with suppress(Exception):
-                        tb = traceback.format_exc()
-                        tb = html_escape(tb)
+                        tb = html_escape(traceback.format_exc())
                         msg = '

Traceback:

\n
{}
'.format(tb) - else: - msg = 'Server got itself in trouble' - msg = ( + message = ( "" "500 Internal Server Error" "\n

500 Internal Server Error

" "
\n{msg}\n\n" ).format(msg=msg) - resp = Response(status=status, text=msg, - content_type='text/html') + ct = 'text/html' else: if self.debug: with suppress(Exception): msg = traceback.format_exc() - else: - msg = 'Server got itself in trouble' - msg = '500 Internal Server Error\n\n' + msg - resp = Response(status=status, text=msg, - content_type='text/plain') - else: - resp = Response(status=status, text=message, - content_type='text/plain') + message = '500 Internal Server Error\n\n' + msg + resp = Response(status=status, text=message, content_type=ct) resp.force_close() # some data already got sent, connection is broken From 188f8106a2a54a241518ee27cbafa0e04a3e5d26 Mon Sep 17 00:00:00 2001 From: Samuel Colvin Date: Fri, 4 Jan 2019 18:20:57 +0000 Subject: [PATCH 05/12] cleanup handle_error more --- aiohttp/web_protocol.py | 29 ++++++++++++++++------------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/aiohttp/web_protocol.py b/aiohttp/web_protocol.py index 7ad4ea496f1..532d34bbde4 100644 --- a/aiohttp/web_protocol.py +++ b/aiohttp/web_protocol.py @@ -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, @@ -524,25 +525,27 @@ def handle_error(self, self.log_exception("Error handling request", exc_info=exc) ct = 'text/plain' - if status == 500: - msg = 'Server got itself in trouble' + if status == HTTPStatus.INTERNAL_SERVER_ERROR: + title = '500 Internal Server Error' + 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 self.debug: - with suppress(Exception): - tb = html_escape(traceback.format_exc()) - msg = '

Traceback:

\n
{}
'.format(tb) + if tb: + tb = html_escape(tb) + msg = '

Traceback:

\n
{}
'.format(tb) message = ( "" - "500 Internal Server Error" - "\n

500 Internal Server Error

" + "{title}" + "\n

{title}

" "
\n{msg}\n\n" - ).format(msg=msg) + ).format(title=title, msg=msg) ct = 'text/html' else: - if self.debug: - with suppress(Exception): - msg = traceback.format_exc() - message = '500 Internal Server Error\n\n' + msg + message = title + '\n\n' + (tb or msg) resp = Response(status=status, text=message, content_type=ct) resp.force_close() From 8f80fac78b3042f9a3e5ff1e57e222033487dc1b Mon Sep 17 00:00:00 2001 From: Samuel Colvin Date: Fri, 4 Jan 2019 18:30:51 +0000 Subject: [PATCH 06/12] include 'Server got itself in trouble' each time --- aiohttp/web_protocol.py | 8 +++++--- tests/test_web_server.py | 6 ++++-- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/aiohttp/web_protocol.py b/aiohttp/web_protocol.py index 532d34bbde4..1249769fa09 100644 --- a/aiohttp/web_protocol.py +++ b/aiohttp/web_protocol.py @@ -536,16 +536,18 @@ def handle_error(self, if 'text/html' in request.headers.get('Accept', ''): if tb: tb = html_escape(tb) - msg = '

Traceback:

\n
{}
'.format(tb) + msg += '\n

Traceback:

\n
{}
'.format(tb) message = ( "" "{title}" "\n

{title}

" - "
\n{msg}\n\n" + "\n{msg}\n\n" ).format(title=title, msg=msg) ct = 'text/html' else: - message = title + '\n\n' + (tb or msg) + if tb: + msg += '\n' + tb + message = title + '\n\n' + msg resp = Response(status=status, text=message, content_type=ct) resp.force_close() diff --git a/tests/test_web_server.py b/tests/test_web_server.py index c84506c823d..5326075f669 100644 --- a/tests/test_web_server.py +++ b/tests/test_web_server.py @@ -128,9 +128,10 @@ async def handler(request): assert resp.headers['Content-Type'].startswith('text/html') txt = await resp.text() + debug(txt) assert txt == ( '500 Internal Server Error\n' - '

500 Internal Server Error


\n' + '

500 Internal Server Error

\n' 'Server got itself in trouble\n' '\n' ) @@ -156,7 +157,8 @@ async def handler(request): txt = await resp.text() assert txt.startswith( '500 Internal Server Error\n' - '

500 Internal Server Error


\n' + '

500 Internal Server Error

\n' + 'Server got itself in trouble\n' '

Traceback:

\n' '
Traceback (most recent call last):\n'
     )

From 903d5c42ccaf8021d42a0752c8f8b3f5952e3456 Mon Sep 17 00:00:00 2001
From: Sviatoslav Sydorenko 
Date: Fri, 4 Jan 2019 18:53:04 +0000
Subject: [PATCH 07/12] Apply suggestions from code review

Co-Authored-By: samuelcolvin 
---
 tests/test_web_server.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/test_web_server.py b/tests/test_web_server.py
index 5326075f669..216e80ee0a1 100644
--- a/tests/test_web_server.py
+++ b/tests/test_web_server.py
@@ -33,7 +33,7 @@ async def handler(request):
     assert resp.headers['Content-Type'].startswith('text/plain')
 
     txt = await resp.text()
-    assert txt.startswith("500 Internal Server Error")
+    assert txt.startswith('500 Internal Server Error')
     assert 'Traceback' not in txt
 
     logger.exception.assert_called_with(

From f30b8bb82a7fc0fcdb9becf653a6e3da918e1a57 Mon Sep 17 00:00:00 2001
From: Sviatoslav Sydorenko 
Date: Sat, 5 Jan 2019 00:35:38 +0100
Subject: [PATCH 08/12] Construct HTTP 500 page title from response const

---
 aiohttp/web_protocol.py | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/aiohttp/web_protocol.py b/aiohttp/web_protocol.py
index 1249769fa09..07f5e189531 100644
--- a/aiohttp/web_protocol.py
+++ b/aiohttp/web_protocol.py
@@ -526,7 +526,10 @@ def handle_error(self,
 
         ct = 'text/plain'
         if status == HTTPStatus.INTERNAL_SERVER_ERROR:
-            title = '500 Internal Server Error'
+            title = (
+                str(HTTPStatus.INTERNAL_SERVER_ERROR.value) + ' '
+                + HTTPStatus.INTERNAL_SERVER_ERROR.phrase
+            )
             msg = HTTPStatus.INTERNAL_SERVER_ERROR.description
             tb = None
             if self.debug:

From 1afc5eca2c157796aa1a4267f8f2db1aaf11e75c Mon Sep 17 00:00:00 2001
From: Sviatoslav Sydorenko 
Date: Sat, 5 Jan 2019 00:36:17 +0100
Subject: [PATCH 09/12] Drop invalid debug function call from tests

---
 tests/test_web_server.py | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tests/test_web_server.py b/tests/test_web_server.py
index 216e80ee0a1..d726ada2bac 100644
--- a/tests/test_web_server.py
+++ b/tests/test_web_server.py
@@ -128,7 +128,6 @@ async def handler(request):
     assert resp.headers['Content-Type'].startswith('text/html')
 
     txt = await resp.text()
-    debug(txt)
     assert txt == (
         '500 Internal Server Error\n'
         '

500 Internal Server Error

\n' From 45c92b8be930a7a42f72d92f60d639f6f5d69b16 Mon Sep 17 00:00:00 2001 From: Sviatoslav Sydorenko Date: Sat, 5 Jan 2019 00:40:53 +0100 Subject: [PATCH 10/12] Make flake8 happy --- aiohttp/web_protocol.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/aiohttp/web_protocol.py b/aiohttp/web_protocol.py index 07f5e189531..6aa0c24cece 100644 --- a/aiohttp/web_protocol.py +++ b/aiohttp/web_protocol.py @@ -527,8 +527,8 @@ def handle_error(self, ct = 'text/plain' if status == HTTPStatus.INTERNAL_SERVER_ERROR: title = ( - str(HTTPStatus.INTERNAL_SERVER_ERROR.value) + ' ' - + HTTPStatus.INTERNAL_SERVER_ERROR.phrase + str(HTTPStatus.INTERNAL_SERVER_ERROR.value) + + ' ' + HTTPStatus.INTERNAL_SERVER_ERROR.phrase ) msg = HTTPStatus.INTERNAL_SERVER_ERROR.description tb = None From fea25c16afb26b88028e3aa94c70e1d174b01f75 Mon Sep 17 00:00:00 2001 From: Sviatoslav Sydorenko Date: Sat, 5 Jan 2019 00:57:49 +0100 Subject: [PATCH 11/12] Construct http-500 error page title with format --- aiohttp/web_protocol.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/aiohttp/web_protocol.py b/aiohttp/web_protocol.py index 6aa0c24cece..9a7ca6f2f7d 100644 --- a/aiohttp/web_protocol.py +++ b/aiohttp/web_protocol.py @@ -526,9 +526,9 @@ def handle_error(self, ct = 'text/plain' if status == HTTPStatus.INTERNAL_SERVER_ERROR: - title = ( - str(HTTPStatus.INTERNAL_SERVER_ERROR.value) + - ' ' + HTTPStatus.INTERNAL_SERVER_ERROR.phrase + title = '{http_status_code:d} {http_response_reason}'.format( + http_status_code=str(HTTPStatus.INTERNAL_SERVER_ERROR.value), + http_response_reason=HTTPStatus.INTERNAL_SERVER_ERROR.phrase, ) msg = HTTPStatus.INTERNAL_SERVER_ERROR.description tb = None From d76270aad35cf184dadede151d1d58c0d040ea04 Mon Sep 17 00:00:00 2001 From: Samuel Colvin Date: Mon, 7 Jan 2019 11:24:59 +0000 Subject: [PATCH 12/12] fix title and remove unnecessary extra message --- aiohttp/web_protocol.py | 9 ++++----- tests/test_web_server.py | 1 - 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/aiohttp/web_protocol.py b/aiohttp/web_protocol.py index 9a7ca6f2f7d..c736a78ef8c 100644 --- a/aiohttp/web_protocol.py +++ b/aiohttp/web_protocol.py @@ -526,9 +526,8 @@ def handle_error(self, ct = 'text/plain' if status == HTTPStatus.INTERNAL_SERVER_ERROR: - title = '{http_status_code:d} {http_response_reason}'.format( - http_status_code=str(HTTPStatus.INTERNAL_SERVER_ERROR.value), - http_response_reason=HTTPStatus.INTERNAL_SERVER_ERROR.phrase, + title = '{0.value} {0.phrase}'.format( + HTTPStatus.INTERNAL_SERVER_ERROR ) msg = HTTPStatus.INTERNAL_SERVER_ERROR.description tb = None @@ -539,7 +538,7 @@ def handle_error(self, if 'text/html' in request.headers.get('Accept', ''): if tb: tb = html_escape(tb) - msg += '\n

Traceback:

\n
{}
'.format(tb) + msg = '

Traceback:

\n
{}
'.format(tb) message = ( "" "{title}" @@ -549,7 +548,7 @@ def handle_error(self, ct = 'text/html' else: if tb: - msg += '\n' + tb + msg = tb message = title + '\n\n' + msg resp = Response(status=status, text=message, content_type=ct) diff --git a/tests/test_web_server.py b/tests/test_web_server.py index d726ada2bac..57ffa110557 100644 --- a/tests/test_web_server.py +++ b/tests/test_web_server.py @@ -157,7 +157,6 @@ async def handler(request): assert txt.startswith( '500 Internal Server Error\n' '

500 Internal Server Error

\n' - 'Server got itself in trouble\n' '

Traceback:

\n' '
Traceback (most recent call last):\n'
     )