🐛 Bug Report - Catching all exceptions makes error handling and application debugging imposible

[vokimon]

Bug description

OAuth2Middleware handles exceptions too widely by catching Exception and masking any error raised in user code.

Because that middleware wraps the whole application and, more importantly, user code, any programming error in user code, like a missing import, or a misspelled variable will be reported as an exception related to Authentication and a 401 http error is emitted, which is confusing and hard to debug because the usual backtrace is not shown. Common fastapi behaviour is emiting a 500 error and an informative traceback on the console.

This makes development quite hard and mask other expected runtime exceptions you might want to handle in upper levels. My current workaround is to edit the library in my development environment by removing that general exception handler. But, on one hand, that is not an easily replicable environment, and secondly, i guess that we will miss some Auth related problem that the original code actually intended to catch.

I propose wrapping code more selectively, keeping user code outside the wrap, and expecting more specific exceptions than Exception.

fastapi-oauth2/src/fastapi_oauth2/middleware.py

Line 151 in d22bb4e

except (JOSEError, Exception) as e:

I use to disable other wide excepts, in paranoid mode, but a shallow look at them i would say they do not interfer with use code. Maybe i am wrong.

fastapi-oauth2/src/fastapi_oauth2/core.py

Line 129 in d22bb4e

except (OAuth2Error, httpx.HTTPError) as e:

fastapi-oauth2/src/fastapi_oauth2/core.py

Line 131 in d22bb4e

except (AuthException, Exception) as e:

Reproduction URL

No response

Reproduction steps

Take the example, in any of the entry points in router_ssr.py append a line with boo (an undefined identifier). You get the error message in the browser, if you are not doing rest, but even if you are authenticated the error is a 401. The normal behaviour, that you get if you remove the wide range except is to have a 500 error and a proper backtrace on the server.

Screenshots

Logs

No response

Browsers

Firefox

OS

Linux

[ArtyomVancyan]

Hi @vokimon, thanks for the detailed description. I didn't clearly get your point in your proposal, but I am looking forward to your pull request on your proposal.

[vokimon]

@ArtyomVancyan i wrote a draft PR, #44. I need more context from you to complete it.

I tried to redo your commit aa8f4b3 without wrapping user code in the error handling.

I am quite confident to have moved the JOSEError handling to the proper context.
But i have no clue of what you tried to catch with Exception.

Also i handled it in mirror of what the PR #43 did with expirated tokens. But the exception we are throwing is not an HttpException from FastApi and framework default handlers are missing it.

[ArtyomVancyan]

The Exception was for in case default AuthenticationMiddleware raises an exception, but I think it could be caught and reraised separately. The commit you refer to is for handling REST API cases. Also, I agree with the HttpException; you can include its fix in this PR as well.

Also, the handling is what the PR #43 did with expirated tokens. But the exception we are throwing is not an HttpException from FastApi, and framework default handlers are missing it.

I think you haven't noticed, but the OAuth2AuthenticationError is HTTPException. Please take a look at src/fastapi_oauth2/exceptions.py file.

[vokimon]

You are right I was handling FastApi.HTTPException, a subclass of Starlette's. That's why my handling ignored them.

Rest API cases with a PlainText response? weird.

Take a look at:

• starlette middleware default implementation,
• Starlette documentation

If we adhere to what Starlette does I would say that what this library should raise starlette.authentication.AuthenticationError which is not a HTTPException. Starlette handles such an exception internally in the default middleware returning a plain text 400 response. That can be overriden by providing the on_error callback to the default middleware. I was hoping that FastApi changes this behaviour to raise a more REST-like response but i didn¡'t find any override in Fastapi.

In summary, my proposal would be:

• Raise starlette.authentication.AuthenticationError from authenticate.
• As library define the on_error if we want a different behaviour than starlette default 400 response.
• Still make possible to provide an on_error callback from user code, in my case would be either let the exception go up or rising a properly json formated 400 or 401.

I am about to update de PR with those changes.

I'll be away on vacation for some weeks so forgive me if i don't reply further messages until then.

[ArtyomVancyan]

I looked at the links you provided and agreed with all the changes you have made. I made a few minor changes on the same branch that I won't merge before you return from vacation. I will update the related documentation as well.

The main thing I have changed is using AuthenticationMiddleware.default_on_error when on_error isn't provided. Also, fixed some typing issues for earlier versions of Python and the failing tests - the cause of the tests failing was that you were calling client.get("/auth") for login simulation, and the OAuth2Middleware syncs the Authorization cookie with headers and it looks for a token in the headers first, so updating only cookies wasn't enough. I should either update client.headers instead of client.cookies or remove the login simulation.

[ArtyomVancyan]

@vokimon, please let me know once you review my changes on your branch so I know that we have a consensus and I can merge and include them in the upcoming release.

[vokimon]

I am still on vacation and i cannot try them but all your changes makes perfect sense. Thanks for the fixes on coding convention and documentation. You can proceed with the integration.