Discrepancy between the behavior of exception handling in documentation and source code + leaking internal information in error responses #3060
Replies: 2 comments
-
This is definitely a bug.
It shouldn't really. If you mount e.g. a Starlette app, exceptions will be handled entirely within that application. It shouldn't be able to raise an exception that reached the outer Litestar application as an exception; It should only ever make it there as an exception response. The only thing affected by this would be raising e.g. Starlette exceptions directly from your Litestar app itself, which is not, and shouldn't be, supported. IMO neither the docs nor the code are correct here. Support for Starlette exception is a remnant from the times Litestar was based on Starlette, but it hasn't been for quite some time now. |
Beta Was this translation helpful? Give feedback.
-
Opened #3082 for this. |
Beta Was this translation helpful? Give feedback.
-
I have posted this on Discord before but it did not really go anywhere.
As I am unsure whether this should be a bug or enhancement I post it now on Github as a discussion and then can be converted to either if needed. (This is basically the same message as the one on Discord, nothing new has been added just structured slightly differently.)
The documented behavior of exception handling mentions:
Which is incorrect; what actually is happening is that Litestar tries to retrieve the value of the
status_code
field/property, regardless whether the object is an instance ofHTTPException
or not.Code;
litestar/litestar/middleware/exceptions/middleware.py
Lines 125 to 127 in 2409574
litestar/litestar/middleware/exceptions/middleware.py
Lines 113 to 116 in 2409574
Before the documentation just gets changed to reflect what actually is happening I would also like to point out that this behavior is problematic.
I only noticed this issue because I got an unhandled Elasticsearch error back from the backend stringified in the exception details:
Related code: https://github.com/elastic/elasticsearch-py/blob/5014ce5337594f66040c81a2610220b1e8c0527e/elasticsearch/exceptions.py#L43-L46
This error also contains other information such as how (e.g. IP:port) the client is connected to elastic and headers which expose versions of used modules.
I do not think this kind of stuff generally should make it out to the client by default, debug mode was also not enabled.
For now I have added a custom exception handler for Elastic errors but on the long term this does not feel right, I would essentially need check out all external modules whether they have a
status_code
filed/property or not and if it would pose a problem.I am not really sure what the right(?, better?, proper?) way to handle this would be, I've mentioned an exception class whitelist mechanic for the exception handler in the Discord comment as a potential solution but I imagine it would also affect the mounting of other ASGI apps, like Starlette apps and would be very breaking.
Beta Was this translation helpful? Give feedback.
All reactions