Add WebSocket handling support for HTTP security dependencies #10147

mnixry · 2023-08-26T07:09:44Z

Related: #8983

Since encode/starlette#1263 has been merged, that makes possible for us to add WebSocket handling support for current HTTP security dependencies.

This PR fixes exceptions like this:

Traceback (most recent call last):
  File "<string>", line 17, in <module>
  File "/docker/nb2-test/venv/lib/python3.8/site-packages/nonebot/__init__.py", line 309, in run
    get_driver().run(*args, **kwargs)
  File "/docker/nb2-test/venv/lib/python3.8/site-packages/nonebot/drivers/fastapi.py", line 198, in run
    uvicorn.run(
  File "/docker/nb2-test/venv/lib/python3.8/site-packages/uvicorn/main.py", line 578, in run
    server.run()
  File "/docker/nb2-test/venv/lib/python3.8/site-packages/uvicorn/server.py", line 61, in run
    return asyncio.run(self.serve(sockets=sockets))
  File "/usr/lib/python3.8/asyncio/runners.py", line 44, in run
    return loop.run_until_complete(main)
> File "/docker/nb2-test/venv/lib/python3.8/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 "/docker/nb2-test/venv/lib/python3.8/site-packages/uvicorn/middleware/proxy_headers.py", line 78, in __call__
    return await self.app(scope, receive, send)
  File "/docker/nb2-test/venv/lib/python3.8/site-packages/fastapi/applications.py", line 290, in __call__
    await super().__call__(scope, receive, send)
  File "/docker/nb2-test/venv/lib/python3.8/site-packages/starlette/applications.py", line 122, in __call__
    await self.middleware_stack(scope, receive, send)
  File "/docker/nb2-test/venv/lib/python3.8/site-packages/starlette/middleware/errors.py", line 149, in __call__
    await self.app(scope, receive, send)
  File "/docker/nb2-test/venv/lib/python3.8/site-packages/starlette/middleware/exceptions.py", line 79, in __call__
    raise exc
  File "/docker/nb2-test/venv/lib/python3.8/site-packages/starlette/middleware/exceptions.py", line 68, in __call__
    await self.app(scope, receive, sender)
  File "/docker/nb2-test/venv/lib/python3.8/site-packages/fastapi/middleware/asyncexitstack.py", line 20, in __call__
    raise e
  File "/docker/nb2-test/venv/lib/python3.8/site-packages/fastapi/middleware/asyncexitstack.py", line 17, in __call__
    await self.app(scope, receive, send)
  File "/docker/nb2-test/venv/lib/python3.8/site-packages/starlette/routing.py", line 718, in __call__
    await route.handle(scope, receive, send)
  File "/docker/nb2-test/venv/lib/python3.8/site-packages/starlette/routing.py", line 443, in handle
    await self.app(scope, receive, send)
  File "/docker/nb2-test/venv/lib/python3.8/site-packages/fastapi/applications.py", line 290, in __call__
    await super().__call__(scope, receive, send)
  File "/docker/nb2-test/venv/lib/python3.8/site-packages/starlette/applications.py", line 122, in __call__
    await self.middleware_stack(scope, receive, send)
  File "/docker/nb2-test/venv/lib/python3.8/site-packages/starlette/middleware/errors.py", line 149, in __call__
    await self.app(scope, receive, send)
  File "/docker/nb2-test/venv/lib/python3.8/site-packages/starlette/middleware/gzip.py", line 26, in __call__
    await self.app(scope, receive, send)
  File "/docker/nb2-test/venv/lib/python3.8/site-packages/starlette/middleware/base.py", line 26, in __call__
    await self.app(scope, receive, send)
  File "/docker/nb2-test/venv/lib/python3.8/site-packages/starlette/middleware/exceptions.py", line 79, in __call__
    raise exc
  File "/docker/nb2-test/venv/lib/python3.8/site-packages/starlette/middleware/exceptions.py", line 68, in __call__
    await self.app(scope, receive, sender)
  File "/docker/nb2-test/venv/lib/python3.8/site-packages/fastapi/middleware/asyncexitstack.py", line 20, in __call__
    raise e
  File "/docker/nb2-test/venv/lib/python3.8/site-packages/fastapi/middleware/asyncexitstack.py", line 17, in __call__
    await self.app(scope, receive, send)
  File "/docker/nb2-test/venv/lib/python3.8/site-packages/starlette/routing.py", line 718, in __call__
    await route.handle(scope, receive, send)
  File "/docker/nb2-test/venv/lib/python3.8/site-packages/starlette/routing.py", line 341, in handle
    await self.app(scope, receive, send)
  File "/docker/nb2-test/venv/lib/python3.8/site-packages/starlette/routing.py", line 82, in app
    await func(session)
  File "/docker/nb2-test/venv/lib/python3.8/site-packages/fastapi/routing.py", line 283, in app
    solved_result = await solve_dependencies(
  File "/docker/nb2-test/venv/lib/python3.8/site-packages/fastapi/dependencies/utils.py", line 593, in solve_dependencies
    solved_result = await solve_dependencies(
  File "/docker/nb2-test/venv/lib/python3.8/site-packages/fastapi/dependencies/utils.py", line 622, in solve_dependencies
    solved = await call(**sub_values)
TypeError: __call__() missing 1 required positional argument: 'request'

Kludex · 2023-08-31T06:41:09Z

I think the encode/starlette#1263 is not enough for this feature.

mnixry · 2023-08-31T08:15:04Z

I think the encode/starlette#1263 is not enough for this feature.

Yes, in large extent, it is caused by the limitations of ASGI. send for WS connections or HTTP is completely different. For standard ASGI, the only thing we can do is disconnect before accept, which will result in a 403 error.

Although there are some ASGI extensions, allow us to custom denial response, but starlette does not support this feature right now: encode/starlette#2041, and only little amount of ASGI servers has implemented this extension.

However, returning a 403 error has been better than raising a "request not found" exception on server side. Additionally, it makes more sense that these dependencies support both HTTP and WS requests.

Kludex · 2023-08-31T08:57:27Z

Hypercorn supports the WSDR (WebSocket Denial Response), and Uvicorn has a PR to implement it already. And... There aren't many server implementations around. Also, I'm a maintainer of Starlette, and I'm keen to have support for WSDR.

I think it's better to wait Uvicorn and Starlette instead of getting this in.

YuriiMotov · 2024-02-23T08:12:13Z

Hypercorn supports the WSDR (WebSocket Denial Response), and Uvicorn has a PR to implement it already. And... There aren't many server implementations around. Also, I'm a maintainer of Starlette, and I'm keen to have support for WSDR.

I think it's better to wait Uvicorn and Starlette instead of getting this in.

Am I understand correctly, that disagreements are about using @handle_exc_for_ws to handle authentication errors? As I understand it will be handled in WSDR after FastAPI is updated to use Startlette 0.37.

Should we exclude @handle_exc_for_ws from this PR and merge it? It will allow using these security tools for websockets at least with auto_error=False. And it will fully work after FastAPI is updated to use Startlette 0.37.

fastapi/security/utils.py

mnixry · 2024-04-02T09:42:34Z

Referencing the discussion in #11146 (comment), it appears that FastAPI won't be upgrading to starlette >= 0.37 anytime soon. We might need to explore other options, or else we could be stuck without progress indefinitely.

Although the @handle_exc_for_ws decorator isn't the most elegant solution, finding an alternative approach hasn't been straightforward. Perhaps we could implement some modifications within the SecurityBase class.

WSH032 · 2024-04-02T10:07:06Z

FastAPI has upgraded Starlette to version >=0.37.2 a few hours ago #11266 . I'm looking forward to this PR being merged as it can solve many downstream issues.

mnixry · 2024-04-02T10:09:56Z

FastAPI has upgraded Starlette to version >=0.37.2 a few hours ago #11266 . I'm looking forward to this PR being merged as it can solve many downstream issues.

Oh, thanks for bringing this to my attention! I hadn't noticed that PR before. I'll get this PR prepared soon.

mnixry · 2024-04-03T06:05:09Z

Updated. Security dependencies are now functional for incoming WebSocket connections when auto_error is set to False.

However, I'm currently uncertain about how to integrate HTTPException to handle WebSocket connections when auto_error is set to True.

mnixry · 2024-04-03T11:35:32Z

I wonder if the WebSocket Denial Response is supposed to be sent behind the scenes when HttpException(status_code=HTTP_403_FORBIDDEN,..) occurs, or should we use WebSocket.send_denial_response to send it?

When HTTPException is raised during a WebSocket connection, it won't be processed correctly and will default to the ExceptionMiddleware, resulting in an internal server error (status code 500). In the worst-case scenario, if ASGI doesn't support WebSocket Denial Response (WSDR), this will lead to a RuntimeError without sending any response, potentially causing the request to block indefinitely, which is clearly undesirable.

In other words, do we need something like this?

The code snippet you've provided has two critical flaws:

Utilizing send_denial_response doesn't stop the processing of the request, which means the dependencies will still be executed. This could lead to an authentication bypass vulnerability. Therefore, stopping the control flow through an Exception is a better solution.
The absence of detection for WSDR support can lead to a RuntimeError if the ASGI server doesn't support it.

To address these problems, introducing a custom middleware that specifically handles HTTPException for WebSocket connections might be necessary.

mnixry added 2 commits August 26, 2023 14:58

Add WebSocket handling support for HTTP security dependencies

cad7564

Fix linting

a10a186

mnixry mentioned this pull request Aug 26, 2023

TypeError: __call()__ missing 1 required positional argument: ‘request’ mnixry/nonebot-plugin-gocqhttp#684

Closed

Add tests for websocket with authorization

6a4ab61

monometa mentioned this pull request Oct 17, 2023

[BUG/Question] Fixing TypeError during WebSocket Authentication Migration from FastAPI 0.96 to 0.97 Intility/fastapi-azure-auth#155

Open

alejsdev added feature New feature or request p3 labels Jan 14, 2024

YuriiMotov mentioned this pull request Feb 23, 2024

Fix APIKeyCookie to accept websocket #9591

Open

Kludex reviewed Feb 23, 2024

View reviewed changes

fastapi/security/utils.py Outdated Show resolved Hide resolved

Merge branch 'master' into master

c68fb05

Merge branch 'master' into master

274803a

mnixry added 2 commits April 2, 2024 23:01

Remove handle_exc_for_ws function from security utils

7edb2d9

Refactor test script with auto_error=False case.

6846708

mnixry requested review from YuriiMotov and Kludex April 3, 2024 06:05

This comment was marked as resolved.

Sign in to view

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add WebSocket handling support for HTTP security dependencies #10147

Add WebSocket handling support for HTTP security dependencies #10147

mnixry commented Aug 26, 2023

Kludex commented Aug 31, 2023

mnixry commented Aug 31, 2023

Kludex commented Aug 31, 2023

YuriiMotov commented Feb 23, 2024

mnixry commented Apr 2, 2024

WSH032 commented Apr 2, 2024

mnixry commented Apr 2, 2024

mnixry commented Apr 3, 2024

This comment was marked as resolved.

mnixry commented Apr 3, 2024 •

edited

Add WebSocket handling support for HTTP security dependencies #10147

Are you sure you want to change the base?

Add WebSocket handling support for HTTP security dependencies #10147

Conversation

mnixry commented Aug 26, 2023

Kludex commented Aug 31, 2023

mnixry commented Aug 31, 2023

Kludex commented Aug 31, 2023

YuriiMotov commented Feb 23, 2024

mnixry commented Apr 2, 2024

WSH032 commented Apr 2, 2024

mnixry commented Apr 2, 2024

mnixry commented Apr 3, 2024

This comment was marked as resolved.

mnixry commented Apr 3, 2024 • edited

mnixry commented Apr 3, 2024 •

edited