Merge pull request #2348 from davidism/bad-request

make debugging bad key errors easier

CHANGES

@@ -62,6 +62,9 @@ Major release, unreleased
   parameters for use when building base URL. (`#1621`_)
 - Set ``APPLICATION_ROOT = '/'`` by default. This was already the implicit
   default when it was set to ``None``.
+- ``TRAP_BAD_REQUEST_ERRORS`` is enabled by default in debug mode.
+  ``BadRequestKeyError`` has a message with the bad key in debug mode instead
+  of the generic bad request message. (`#2348`_)
 
 .. _#1489: https://github.com/pallets/flask/pull/1489
 .. _#1621: https://github.com/pallets/flask/pull/1621
@@ -80,6 +83,7 @@ Major release, unreleased
 .. _#2316: https://github.com/pallets/flask/pull/2316
 .. _#2319: https://github.com/pallets/flask/pull/2319
 .. _#2326: https://github.com/pallets/flask/pull/2326
+.. _#2348: https://github.com/pallets/flask/pull/2348
 
 Version 0.12.2
 --------------

docs/config.rst

@@ -109,9 +109,10 @@ The following configuration values are used internally by Flask:
     Trying to access a key that doesn't exist from request dicts like ``args``
     and ``form`` will return a 400 Bad Request error page. Enable this to treat
     the error as an unhandled exception instead so that you get the interactive
-    debugger. This is a more specific version of ``TRAP_HTTP_EXCEPTIONS``.
+    debugger. This is a more specific version of ``TRAP_HTTP_EXCEPTIONS``. If
+    unset, it is enabled in debug mode.
 
-    Default: ``False``
+    Default: ``None``
 
 .. py:data:: SECRET_KEY
 

flask/app.py

@@ -18,7 +18,8 @@ from threading import Lock
 
 from werkzeug.datastructures import ImmutableDict, Headers
 from werkzeug.exceptions import BadRequest, HTTPException, \
-    InternalServerError, MethodNotAllowed, default_exceptions
+    InternalServerError, MethodNotAllowed, default_exceptions, \
+    BadRequestKeyError
 from werkzeug.routing import BuildError, Map, RequestRedirect, Rule
 
 from . import cli, json
@@ -317,7 +318,7 @@ class Flask(_PackageBoundObject):
         'SESSION_REFRESH_EACH_REQUEST':         True,
         'MAX_CONTENT_LENGTH':                   None,
         'SEND_FILE_MAX_AGE_DEFAULT':            timedelta(hours=12),
-        'TRAP_BAD_REQUEST_ERRORS':              False,
+        'TRAP_BAD_REQUEST_ERRORS':              None,
         'TRAP_HTTP_EXCEPTIONS':                 False,
         'EXPLAIN_TEMPLATE_LOADING':             False,
         'PREFERRED_URL_SCHEME':                 'http',
@@ -1543,12 +1544,20 @@ class Flask(_PackageBoundObject):
         traceback.  This is helpful for debugging implicitly raised HTTP
         exceptions.
         
+        .. versionchanged:: 1.0
+            Bad request errors are not trapped by default in debug mode.
+
         .. versionadded:: 0.8
         """
         if self.config['TRAP_HTTP_EXCEPTIONS']:
             return True
-        if self.config['TRAP_BAD_REQUEST_ERRORS']:
+
+        trap_bad_request = self.config['TRAP_BAD_REQUEST_ERRORS']
+
+        # if unset, trap based on debug mode
+        if (trap_bad_request is None and self.debug) or trap_bad_request:
             return isinstance(e, BadRequest)
+
         return False
 
     def handle_user_exception(self, e):
@@ -1559,16 +1568,30 @@ class Flask(_PackageBoundObject):
         function will either return a response value or reraise the
         exception with the same traceback.
 
+        .. versionchanged:: 1.0
+            Key errors raised from request data like ``form`` show the the bad
+            key in debug mode rather than a generic bad request message.
+
         .. versionadded:: 0.7
         """
         exc_type, exc_value, tb = sys.exc_info()
         assert exc_value is e
-
         # ensure not to trash sys.exc_info() at that point in case someone
         # wants the traceback preserved in handle_http_exception.  Of course
         # we cannot prevent users from trashing it themselves in a custom
         # trap_http_exception method so that's their fault then.
 
+        # MultiDict passes the key to the exception, but that's ignored
+        # when generating the response message. Set an informative
+        # description for key errors in debug mode or when trapping errors.
+        if (
+            (self.debug or self.config['TRAP_BAD_REQUEST_ERRORS'])
+            and isinstance(e, BadRequestKeyError)
+            # only set it if it's still the default description
+            and e.description is BadRequestKeyError.description
+        ):
+            e.description = "KeyError: '{0}'".format(*e.args)
+
         if isinstance(e, HTTPException) and not self.trap_http_exception(e):
             return self.handle_http_exception(e)
 

tests/test_basic.py

@@ -975,12 +975,17 @@ def test_trapping_of_bad_request_key_errors(app, client):
     def fail():
         flask.request.form['missing_key']
 
-    assert client.get('/fail').status_code == 400
+    rv = client.get('/fail')
+    assert rv.status_code == 400
+    assert b'missing_key' not in rv.data
 
     app.config['TRAP_BAD_REQUEST_ERRORS'] = True
+
     with pytest.raises(KeyError) as e:
         client.get("/fail")
+
     assert e.errisinstance(BadRequest)
+    assert 'missing_key' in e.value.description
 
 
 def test_trapping_of_all_http_exceptions(app, client):

tests/test_helpers.py

@@ -44,6 +44,7 @@ class TestJSON(object):
 
     def test_post_empty_json_adds_exception_to_response_content_in_debug(self, app, client):
         app.config['DEBUG'] = True
+        app.config['TRAP_BAD_REQUEST_ERRORS'] = False
 
         @app.route('/json', methods=['POST'])
         def post_json():
@@ -56,6 +57,7 @@ class TestJSON(object):
 
     def test_post_empty_json_wont_add_exception_to_response_if_no_debug(self, app, client):
         app.config['DEBUG'] = False
+        app.config['TRAP_BAD_REQUEST_ERRORS'] = False
 
         @app.route('/json', methods=['POST'])
         def post_json():