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.

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

make debugging bad key errors easier #2348

Merged
merged 3 commits into from
May 30, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 5 additions & 1 deletion CHANGES
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ Major release, unreleased
work with the ``flask`` command. If they take a single parameter or a
parameter named ``script_info``, the ``ScriptInfo`` object will be passed.
(`#2319`_)
- FLASK_APP=myproject.app:create_app('dev') support.
- FLASK_APP=myproject.app:create_app('dev') support.
- ``FLASK_APP`` can be set to an app factory, with arguments if needed, for
example ``FLASK_APP=myproject.app:create_app('dev')``. (`#2326`_)
- ``View.provide_automatic_options = True`` is set on the view function from
Expand All @@ -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
Expand All @@ -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
--------------
Expand Down
5 changes: 3 additions & 2 deletions docs/config.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
31 changes: 27 additions & 4 deletions flask/app.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@

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
Expand Down Expand Up @@ -317,7 +318,7 @@ def _set_request_globals_class(self, value):
'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',
Expand Down Expand Up @@ -1542,13 +1543,21 @@ def trap_http_exception(self, e):
exception is not called and it shows up as regular exception in the
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):
Expand All @@ -1559,16 +1568,30 @@ def handle_user_exception(self, e):
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)

Expand Down
7 changes: 6 additions & 1 deletion tests/test_basic.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down
2 changes: 2 additions & 0 deletions tests/test_helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ def test_ignore_cached_json(self, app):

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():
Expand All @@ -56,6 +57,7 @@ def post_json():

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():
Expand Down