Websocket handler fails in some cases of HTTPError/HTTPStatus

[sashn-citiqprepaid]

Hi,

Writing ASGI falcon using websockets, using the process_request_ws middleware handler and app.add_error_handler(HTTPError, my_error_handler) error handler.

When raising a falcon.HTTPError in the middleware its picked up by the error handler via the ex param. The object is not none, however this is my stacktrace:

I raise the "falcon.http_error.HTTPError: <HTTPError: 403 Forbidden>" in the middleware, and then raise the "falcon.http_error.HTTPError: <HTTPError: 500>" in the error handler, which results in the final traceback (falcon internal).

'''
[2023-03-29 13:39:47 +0200] [82450] [INFO] ('127.0.0.1', 57224) - "WebSocket /message" 403
[2023-03-29 13:39:47 +0200] [82450] [ERROR] Exception in ASGI application
Traceback (most recent call last):
  File ".pyenv/versions/wm/lib/python3.11/site-packages/falcon/asgi/app.py", line 1000, in _handle_websocket
    await process_request_ws(req, web_socket)
  File "Desktop/mirror/whatsapp-service/whatsapp_service/middleware/basic_ws_middleware.py", line 106, in process_request_ws
    raise HTTPError(status=falcon.HTTP_403, title="Invalid header")
falcon.http_error.HTTPError: <HTTPError: 403 Forbidden>

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File ".pyenv/versions/wm/lib/python3.11/site-packages/falcon/asgi/app.py", line 1095, in _handle_exception
    await err_handler(req, resp, ex, params, **kwargs)
  File "Desktop/mirror/whatsapp-service/whatsapp_service/utils/handlers/error_handlers.py", line 24, in my_error_handler
    raise HTTPError(status=500, title='Custom Websocket Error')
falcon.http_error.HTTPError: <HTTPError: 500>

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File ".pyenv/versions/wm/lib/python3.11/site-packages/uvicorn/protocols/websockets/websockets_impl.py", line 238, in run_asgi
    result = await self.app(self.scope, self.asgi_receive, self.asgi_send)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File ".pyenv/versions/wm/lib/python3.11/site-packages/uvicorn/middleware/proxy_headers.py", line 78, in __call__
    return await self.app(scope, receive, send)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File ".pyenv/versions/wm/lib/python3.11/site-packages/falcon/asgi/app.py", line 312, in __call__
    await self._handle_websocket(spec_version, scope, receive, send)
  File ".pyenv/versions/wm/lib/python3.11/site-packages/falcon/asgi/app.py", line 1018, in _handle_websocket
    if not await self._handle_exception(req, None, ex, params, ws=web_socket):
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File ".pyenv/versions/wm/lib/python3.11/site-packages/falcon/asgi/app.py", line 1100, in _handle_exception
    self._compose_error_response(req, resp, error)
  File "falcon/app.py", line 975, in falcon.app.App._compose_error_response
'''

The usage of the method resolution order is incorrect here as the type(exc) invocation returns type not the exception: https://github.com/falconry/falcon/blob/18503813059e648c693d03064adf2acdbe68d654/falcon/app.py#L1001

[sashn-citiqprepaid]

Steps to reproduce:

from falcon.errors import HTTPError
from falcon.http_status import HTTPStatus

ehs = [Exception, HTTPError(status=400), HTTPStatus(status=400)]

# current implementation (bug, see: https://docs.python.org/3/library/functions.html#type)
for exc in type(HTTPError).__mro__[:-1]:
    assert exc in (type, object)

try:
    # preffered but still has a bug:
    for exc in HTTPError.__mro__[:-1]:
        assert exc in (HTTPError, HTTPStatus, Exception)
except AssertionError as e:
    print('assertion error')

# preffered bug free
for exc in HTTPError.__mro__[:-1]:
    assert exc in (HTTPError, Exception, BaseException)

[CaselIT]

Hi,

I don't think the issue is type(ex) here, since ex in an instance of an exception, not a class. The stack trace is is not complete, so I don't know what caused it

[sashn-citiqprepaid]

Hi @CaselIT ,

Even with an instance of exception, because all classes inherit from object, calling type() on any instance irrespective of inheritance or the class itself will return <class 'type'>.

Thats why this example which is similar to the code in falcon, passes the assertion, regardless of the mro calling type() on any object even on int will result in the type class on which dunder mro is then called:

for exc in type(HTTPError).__mro__[:-1]:
    assert exc in (type, object)

python docs: "The isinstance built-in function is recommended for testing the type of an object, because it takes subclasses into account. " - type()

Another example:

# Exception (from python std lib)
class ParentException(Exception):
    pass

class ChildException(Exception):
    pass

print('ChildException.mro():', ChildException.mro())
# >>> ChildException.mro(): [<class '__main__.ChildException'>, <class 'Exception'>, <class 'BaseException'>, <class 'object'>]

# irrespective of the object, :type(): will always return an instance of type
print('type(ChildException): ',type(ChildException)) # >>> type(ChildException):  <class 'type'>

print('type(ParentException):', type(ParentException)) # >>> type(ParentException): <class 'type'>

print('type(int): ', type(int))  # >>> type(int):  <class 'type'>

# therefore calling the method resolution order of type yields (type, object)
print('type(ChildException).__mro__:' , type(ChildException).__mro__)   # >>> type(ChildException).__mro__: (<class 'type'>, <class 'object'>)

[CaselIT]

Even with an instance of exception, because all classes inherit from object, calling type() on any instance irrespective of inheritance or the class itself will return <class 'type'>.

Sorry but this statement is wrong:

print(type(Exception()).__mro__[:-1])
# (<class 'Exception'>, <class 'BaseException'>)

there is no type here. Removing the -1 will revel object. Since an instance it's not a type

print(type(Exception()).__mro__)
# (<class 'Exception'>, <class 'BaseException'>, <class 'object'>)

The following is also wrong, since type is called classes not instances. That is not representative of what falcon is doing. It's calling type() in instances not classes.

Returning on the exception in the opening post, can you provide the complete stack trace?

[sashn-citiqprepaid]

Hi @CaselIT ,

That is my entire stack trace.

I see the usage of instances now, the distinction between instances and classes when using type(), very different behavior, thanks for pointing it out.

Then I am unsure why the error is raised internally to falcon.

The line that seems to be errororing is https://github.com/falconry/falcon/blob/18503813059e648c693d03064adf2acdbe68d654/falcon/app.py#L974

[vytas7]

Hi @sashn-citiqprepaid, and thanks for reporting this. Echoing @CaselIT, could you post a small program that illustrates the problem; not Falcon's own internal code, but your program, ideally as small as possible, so that we could reproduce it?

You have added some interesting analyses why Falcon's way to map exceptions to error handlers works or doesn't work as expected, but we still want to think of this in terms of the framework's user experience. When I have this program, what happens? Also, we could then test if this is specific to WebSocket, or it affects all kinds of errors, like HTTP APIs too.

[sashn-citiqprepaid]

Hi @vytas7 .

It might be my implementation that is unexpected, but worth exploring. Will put together an example.

[sashn-citiqprepaid]

Hi @vytas7,

Ive made available https://github.com/sashn-citiqprepaid/2146

[CaselIT]

Ok as suspected the stack trace was not complete. Here is the complete stack trace:

Traceback (most recent call last):
  File "me\falcon\falcon\asgi\app.py", line 998, in _handle_websocket
    await process_request_ws(req, web_socket)
  File "me\falcon\try2.py", line 31, in process_request_ws
    raise HTTPError(status=falcon.HTTP_401, title="Invalid header")
falcon.http_error.HTTPError: <HTTPError: 401 Unauthorized>

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "me\falcon\falcon\asgi\app.py", line 1093, in _handle_exception
    await err_handler(req, resp, ex, params, **kwargs)
  File "me\falcon\try2.py", line 60, in my_error_handler
    raise HTTPError(status=418, title='Custom Websocket Error')
falcon.http_error.HTTPError: <HTTPError: 418>

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "also_me\venvs\falcon\Lib\site-packages\uvicorn\protocols\websockets\websockets_impl.py", line 254, in run_asgi
    result = await self.app(self.scope, self.asgi_receive, self.asgi_send)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "also_me\venvs\falcon\Lib\site-packages\uvicorn\middleware\proxy_headers.py", line 78, in __call__
    return await self.app(scope, receive, send)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "me\falcon\falcon\asgi\app.py", line 310, in __call__
    await self._handle_websocket(spec_version, scope, receive, send)
  File "me\falcon\falcon\asgi\app.py", line 1016, in _handle_websocket
    if not await self._handle_exception(req, None, ex, params, ws=web_socket):
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "me\falcon\falcon\asgi\app.py", line 1098, in _handle_exception
    self._compose_error_response(req, resp, error)
  File "me\falcon\falcon\app.py", line 974, in _compose_error_response
    resp.status = error.status
    ^^^^^^^^^^^
AttributeError: 'NoneType' object has no attribute 'status'

and that's indeed a bug of falcon, since there is no resp when dealing with a websocket.

cc @kgriffs

[CaselIT]

Looking a bit, I think that things are quite limited here, since if the web-socket has not yet been accepted the only thing available is close.

I guess we can do the same as the default error handler (with a different log message) https://github.com/falconry/falcon/blob/d2981f684c91598f229e6c2fa2408a3a857da0da/falcon/asgi/app.py#L1035-L1042

same for status?

[CaselIT]

I don't think so.

The issue is that error raised in error handlers when using websockets were not correctly considered by falcon, so we try to serialize it on the a response object that missing since it's a websocket response.

A workaround is to instead of raising the error in the error handler, to close the websocket if the websocket object is provided to the handler

[sashn-citiqprepaid]

Hi @CaselIT , Just removed the comment, saw the same stack-trace. Ive forked falcon, and having a look aswell.

[sashn-citiqprepaid]

@CaselIT would this entail a new error response composer for a websocket?

[CaselIT]

I don't think so, since the only think that can be safely done is close, we can document that that's what the framework will do

[CaselIT]

btw thanks @sashn-citiqprepaid for supplying a complete example that reproduced your issue

[sashn-citiqprepaid]

@CaselIT absolute pleasure, write a fair amount of falcon code and thoroughly enjoy using it, favorite feature has to be the request.context in the middleware thats passed onto collections.

[vytas7]

It seems it is even worse for the case of HTTPStatus, as no complex custom error handlers are even required to reproduce this issue, the MRE becomes even more trivial:

import falcon
import falcon.asgi

class Messages:
    async def on_websocket(self, req, ws):
        raise falcon.HTTPStatus(204)

app = falcon.asgi.App()
app.add_route('/messages', Messages())

[CaselIT]

I've opened https://github.com/falconry/falcon/pull/2150 with a fix