Merge pull request #1714 from gevent/issue1708

Avoid printing a TypeError traceback on certain types of invalid client requests

CHANGES.rst

@@ -19,7 +19,7 @@ Features
 
   On Linux, this can reduce the number of system calls libev makes.
   Originally provided by Josh Snyder.
-  See :issue:`issue1648`.
+  See :issue:`1648`.
 
 
 Bugfixes
@@ -31,7 +31,7 @@ Bugfixes
   On Python 3.7 and above, the module ``gevent.contextvars`` is no
   longer monkey-patched into the standard library. contextvars are now
   both greenlet and asyncio task local. See :issue:`1656`.
-  See :issue:`issue1674`.
+  See :issue:`1674`.
 - The ``DummyThread`` objects created automatically by certain
   operations when the standard library threading module is
   monkey-patched now match the naming convention the standard library
@@ -43,6 +43,7 @@ Bugfixes
   .. caution:: This currently means that it can be imported. But it
                cannot yet be used. gevent has a pinned dependency on
                dnspython < 2 for now.
+
   See :issue:`1661`.
 
 
@@ -64,7 +65,7 @@ Features
   unknown, and this may bitrot in future releases.
 
   Thanks to berkakinci for the patch.
-  See :issue:`issue1645`.
+  See :issue:`1645`.
 
 
 Bugfixes
@@ -74,7 +75,7 @@ Bugfixes
 
   Previously it was pinned to 5.6.3 for PyPy2, except for on Windows,
   where it was excluded. It is now treated the same as CPython again.
-  See :issue:`issue1643`.
+  See :issue:`1643`.
 
 
 ----
@@ -97,7 +98,7 @@ Bugfixes
 
 - On Python 2, the dnspython resolver can be used without having
   selectors2 installed. Previously, an ImportError would be raised.
-  See :issue:`issue1641`.
+  See :issue:`1641`.
 - Python 3 ``gevent.ssl.SSLSocket`` objects no longer attempt to catch
   ``ConnectionResetError`` and treat it the same as an ``SSLError`` with
   ``SSL_ERROR_EOF`` (typically by suppressing it).
@@ -222,7 +223,7 @@ Bugfixes
   Also, the underlying Semaphore always behaves in an atomic fashion (as
   if the GIL was not released) when PURE_PYTHON is set. Previously, it
   only correctly did so on PyPy.
-  See :issue:`issue1437`.
+  See :issue:`1437`.
 - Rename gevent's C accelerator extension modules using a prefix to
   avoid clashing with other C extensions.
   See :issue:`1480`.

docs/changes/1708.bugfix

+gevent.pywsgi: Avoid printing an extra traceback ("TypeError: not
+enough arguments for format string") to standard error on certain
+invalid client requests.
+
+Reported by Steven Grimm.

src/gevent/pywsgi.py

@@ -102,11 +102,15 @@ class _InvalidClientInput(IOError):
 
 
 class _InvalidClientRequest(ValueError):
-    # Internal exception raised by WSGIHandler.read_request
-    # indicating that the client sent an HTTP request that cannot
-    # be parsed (e.g., invalid grammar). The result *should* be an
-    # HTTP 400 error
-    pass
+    # Internal exception raised by WSGIHandler.read_request indicating
+    # that the client sent an HTTP request that cannot be parsed
+    # (e.g., invalid grammar). The result *should* be an HTTP 400
+    # error. It must have exactly one argument, the fully formatted
+    # error string.
+
+    def __init__(self, message):
+        ValueError.__init__(self, message)
+        self.formatted_message = message
 
 
 class Input(object):
@@ -564,12 +568,20 @@ class WSGIHandler(object):
 
         return True
 
+    _print_unexpected_exc = staticmethod(traceback.print_exc)
+
     def log_error(self, msg, *args):
-        try:
-            message = msg % args
-        except Exception: # pylint:disable=broad-except
-            traceback.print_exc()
-            message = '%r %r' % (msg, args)
+        if not args:
+            # Already fully formatted, no need to do it again; msg
+            # might contain % chars that would lead to a formatting
+            # error.
+            message = msg
+        else:
+            try:
+                message = msg % args
+            except Exception: # pylint:disable=broad-except
+                self._print_unexpected_exc()
+                message = '%r %r' % (msg, args)
         try:
             message = '%s: %s' % (self.socket, message)
         except Exception: # pylint:disable=broad-except
@@ -578,7 +590,7 @@ class WSGIHandler(object):
         try:
             self.server.error_log.write(message + '\n')
         except Exception: # pylint:disable=broad-except
-            traceback.print_exc()
+            self._print_unexpected_exc()
 
     def read_requestline(self):
         """
@@ -1030,9 +1042,10 @@ class WSGIHandler(object):
             # handle_error method?
             traceback.print_exc()
         if isinstance(ex, _InvalidClientRequest):
-            # These come with good error messages, and we want to let
-            # log_error deal with the formatting, especially to handle encoding
-            self.log_error(*ex.args)
+            # No formatting needed, that's already been handled. In fact, because the
+            # formatted message contains user input, it might have a % in it, and attempting
+            # to format that with no arguments would be an error.
+            self.log_error(ex.formatted_message)
         else:
             self.log_error('Invalid request: %s', str(ex) or ex.__class__.__name__)
         return ('400', _BAD_REQUEST_RESPONSE)

src/gevent/tests/test__pywsgi.py

@@ -1019,7 +1019,7 @@ class TestInputN(TestCase):
         self.urlopen()
 
 
-class TestError(TestCase):
+class TestErrorInApplication(TestCase):
 
     error = object()
     error_fatal = False
@@ -1034,7 +1034,7 @@ class TestError(TestCase):
         self.assert_error(greentest.ExpectedException, self.error)
 
 
-class TestError_after_start_response(TestError):
+class TestError_after_start_response(TestErrorInApplication):
 
     def application(self, env, start_response):
         self.error = greentest.ExpectedException('TestError_after_start_response.application')
@@ -1175,6 +1175,12 @@ class BadRequestTests(TestCase):
     # pywsgi checks content-length, but wsgi does not
     content_length = None
 
+    assert TestCase.handler_class._print_unexpected_exc
+
+    class handler_class(TestCase.handler_class):
+        def _print_unexpected_exc(self):
+            raise AssertionError("Should not print a traceback")
+
     def application(self, env, start_response):
         self.assertEqual(env['CONTENT_LENGTH'], self.content_length)
         start_response('200 OK', [('Content-Type', 'text/plain')])
@@ -1192,6 +1198,15 @@ class BadRequestTests(TestCase):
             fd.write('GET / HTTP/1.1\r\nHost: localhost\r\nContent-Length: %s\r\n\r\n' % self.content_length)
             read_http(fd, code=(200, 400))
 
+    def test_bad_request_line_with_percent(self):
+        # If the request is invalid and contains Python formatting characters (%)
+        # we don't fail to log the error and we do generate a 400.
+        # https://github.com/gevent/gevent/issues/1708
+        bad_request = 'GET / HTTP %\r\n'
+        with self.makefile() as fd:
+            fd.write(bad_request)
+            read_http(fd, code=400)
+
 
 class ChunkedInputTests(TestCase):
     dirt = ""