Handle CSRFErrors as a subset of 400 errors

[katstevens]

Trello: https://trello.com/c/1K5ePclo/372-your-session-has-expired-instead-of-ugly-csrf-message

Redirects the user to the login page with an accompanying flash message. We can move this to utils in a separate PR.

TLDR: Flask is doing error handling weirdly, we can work around it by catching all 400s but only handling CSRFErrors.

Long version:
I have a theory why Flask won't recognise the CSRFError in its own handler. The recommended advice from Flask-WTF (http://flask-wtf.readthedocs.io/en/stable/csrf.html) is to do something like this:

@application.error_handler(CSRFError):
def csrf_handler(e):
    # custom behaviour
    ...

(We actually define our error handlers on the blueprint, not the application, however this is just a wrapper for the app functionality, and shouldn't make a difference.)

A bit of background on Flask error handling sheds some light:

• An exception gets raised from a request, and if it's not caught in a try.. except somewhere, it goes to Flask's error handling at handle_exception (https://github.com/pallets/flask/blob/0.12.4/flask/app.py#L1520).
• Flask decides whether to propagate it immediately (e.g. if in DEBUG mode), use a custom handler from either the app or the blueprints, or give up and raise a 500.
• The app and blueprint handlers are collected in a dictionary current_app.error_handler_spec, and split into HTTP status code handlers (using 404, 500 etc as the key) and a list of any user-defined ones (e.g. for Brief Responses we define APIError and QuestionNotFoundError), with the snappily-named key 'None' for that list. An example error_handler_spec looks like this:

{
    None: {
        None: [
            (<class 'dmapiclient.errors.APIError'>, <function api_error_handler at 0x10472e8c8>), 
            (<class 'dmcontent.errors.QuestionNotFoundError'>, <function content_loader_error_handler at 0x10472eae8>),
            (<class 'flask_wtf.csrf.CSRFError'>, <function csrf_error_handler at 0x10472e950>)  # this ends up in the right place when registering the handlers
        ], 
        404: <function page_not_found at 0x10472ee18>, 
        500: <function internal_server_error at 0x104732048>, 
        503: <function service_unavailable at 0x1047321e0>
    }
}

• Flask decides which handler to use (i.e. which dictionary key) based on whether there's a status code or not. If the code is None, it compares the exception classes in the tuples.
• Or does it? The _find_error_handler function looks for a handler using a status code if there is one (https://github.com/pallets/flask/blob/0.12.4/flask/app.py#L1429), so if an exception has a status code but its handler is defined by its class, that handler will never get called. 😿

It turns out that APIError and QuestionNotFoundError are not based on HTTP exceptions, whereas CSRFError is a subclass of BadRequest (400 status). There's a check here https://github.com/pallets/flask/blob/0.12.4/flask/app.py#L1110 for the exception subclass that manages to wrangle a 400 status code out of the CSRFError. It uses that code to look up the 400 handler in the dictionary and ignores our custom handler.

So to solve this for now, I decided to go with the flow and have my handler listen for all 400s and ignore anything that wasn't a CSRF.

Unless I've missed something daft, this feels like a bug in Flask. Or perhaps Flask-WTF's advice is just misguided. Thoughts welcome.

[risicle]

Looks good - but yeah would be good in -utils.

[katstevens]

The code in question does appear to be different in Flask 1.0 ("Error handlers that match specific classes are now checked first, thereby allowing catching exceptions that are subclasses of HTTP exceptions (in werkzeug.exceptions). This makes it possible for an extension author to create exceptions that will by default result in the HTTP error of their choosing, but may be caught with a custom error handler if desired."), so I'm not going to complain to them just yet. I'll make a note on the Flask 1.0 card that this is something we should be aware of.