starlette: Custom 500 or Exception handlers do not run through middleware like other handled exceptions.

Checklist

The bug is reproducible against the latest release and/or master.
There are no similar issues or pull requests to fix it yet.

Describe the bug

To reproduce

from starlette.applications import Starlette
from starlette.middleware import Middleware
from starlette.middleware.cors import CORSMiddleware
from starlette.responses import JSONResponse

app = Starlette(routes=routes, middleware=middleware)

async def not_found(request, exc):
    return JSONResponse(content={"error": 404}, status_code=exc.status_code)

async def server_error(request, exc):
    return JSONResponse(content={"error": 500}, status_code=exc.status_code)

exception_handlers = {
    404: not_found,
    500: server_error
}

middleware = [
    Middleware(CORSMiddleware, allow_origins=['*'])
]

app = Starlette(routes=routes, middleware=middleware, exception_handlers=exception_handlers)

Expected behavior

both 404 and 500 errors will respond with their relevant json response, and correct CORS headers

Actual behavior

the 404 error will response with correct CORS headers but the 500 error will not.

Debugging material

Because Exception and 500 are special cased in build_middleware_stack any error bubbles up to the outer most ServerErrorMiddleware which is outside the other middleware, instead of being handled by the ExceptionMiddleware and passing through the custom middleware.

see: https://github.com/encode/starlette/blob/master/starlette/applications.py#L74-L77

I can see four possible fixes, none of them perfect…

handle custom 500 errors in ExceptionMiddleware and prevent ServerErrorMiddleware showing its helpful debug info when a custom handler is present.
have ServerErrorMiddleware run the response of the error handler through the middleware stack again, and if that fails resort to existing behaviour, adding a bunch of complexity.
move the debuging info to a separate middleware and put that in front of the ErrorMiddleware, which would then cause it to trigger for every exception, but would allow ExceptionMiddleware handle 500s as expected, and ServerErrorMiddleware becomes a last resort.

update https://www.starlette.io/exceptions/ to explain 500 errors are handled differently, and to get the above behaviour something like the following work-around is required:

@app.middleware("http")
def exception_middleware(request, call_next)
    try:
        return await call_next(request)
    except Exception:
        return JSONResponse(content={"error": 500}, status_code=500))

Environment

OS: N/A
Python version: N/A
Starlette version: 0.13.6 and master branch

Additional Context

#1116 seems to be have similar but refers to the generic error_response on ServerErrorMiddleware, this is similar but specifically about custom error handlers so i didn’t want to hijack that issue.

About this issue

Original URL
State: closed
Created 3 years ago
Reactions: 10
Comments: 18 (7 by maintainers)

Most upvoted comments

the 404 error will response with correct CORS headers but the 500 error will not.

That… seems totally okay to me.

If you have an unhandled exception you really do just want to break out with a 500 error, with minimal other interference.

It’s a broken state that you do want to see, and it shouldn’t at all matter what CORS headers it does or doesn’t include.

(Also it’d be perfectly possible for the user to get a 5xx response from some intermediary, such as an overloaded server, or a gateway proxy, and you wouldn’t be seeing any CORS headers on those either)

tomchristie on Feb 11, 2022

We serialise errors to Problem JSON format and display them in the browser, including 500 errors. Just because an error has occurred on the server, it doesn’t mean that you can’t show a useful error to the user containing the details of what has gone wrong. Right now, the obvious approach you get from following the documentation fails. Instead you need a clumsy workaround that goes against the documentation’s recommendation.

It’s a broken state that you do want to see

That’s precisely the point. If you have an SPA loading API requests and there is a 500 error, the CORS headers are what allows you to see what is going on with the broken state, instead of just "Sorry, something failed but I can’t tell you what because I’m not allowed to look at the details”.

JimDabell on Feb 11, 2022

+1 There is absolutely no reason to remove the CORS headers.

If you want to hide the error details (5xx or 4xx) from normal users, then it is a frontend task to prettify the message for the user.

On the other hand you definitely want to hide the internal exception details from attackers. But the removal of CORS headers does not stop anyone who can open the browsers developer tools. Even without CORS headers attackers can see the response body. The response body must not contain any sensitive information and this has nothing to do with CORS headers.

vibpe on Aug 24, 2022

So, if you really want to do that, then wrap the middleware around the entire app…

# Make sure the entire app is wrapped up in CORSMiddleware,
# including 5xx error responses.
app = Starlette(...)
app = CORSMiddleware(app, ...)

tomchristie on Feb 11, 2022

I’m not sure I’d want the debug flag being the arbiter of behavior here. I’d want the behavior to be consistent across development and production. It seems to me that starlette is being overly prescriptive in the way it handles unexpected errors. (not to say it’s not already pretty flexible, but just that it could be improved).

I commented on the #1116 and put up a straw-man PR focusing on the CORS issue. Ultimately, I think we need to have a way to add middleware layers outside of the implicit builtin ServerErrorMiddleware, at least for response manipulation. I like the guarantee that ServerErrorMiddleware provides - that a Response object will have been created and invoked. Being able to configure further response processing would provide a natural place to apply the CORSMiddleware with minimal changes to the cors module.

jcwilson on Jul 19, 2021

Okay I think I’ve read through the thread.

@JonasKs I still don’t think there’s anything wrong with that. FWIW, if I were to see this in a code review I think I might ask for a one line comment explaining why we are doing it but other than that I would be perfectly happy with that.

I do think not moving 500/Exception handlers into ServerErrorMiddleware seems reasonable (“handle custom 500 errors in ExceptionMiddleware and prevent ServerErrorMiddleware showing its helpful debug info when a custom handler is present”).

I also wonder if one could wrap CORSMiddleware with ExceptionMiddleware, I’ll give it a quick shot.

adriangb on Jul 26, 2022

Ah thanks @JimDabell - that does make some sense.

Just because an error has occurred on the server, it doesn’t mean that you can’t show a useful error to the user containing the details of what has gone wrong.

Right, which we do. (Proviso: but only if you’re in debug mode. If you hit a server error on prod you don’t want to expose any of that info in the response, but you will want to be monitoring it with something like Sentry)

If you have an SPA loading API requests and there is a 500 error, the CORS headers are what allows you to see what is going on with the broken state, instead of just "Sorry, something failed but I can’t tell you what because I’m not allowed to look at the details”.

Gotcha - I’d not figured that.

Is there a liberal header policy that can be used in that case, perhaps?

I’m pretty sure Django’s error handling has a similar style to what I’ve just described. I don’t know about flask. How do they deal with this particular use-case?

tomchristie on Feb 11, 2022