Support the WebSocket Denial Response ASGI extension by kristjanvalur · Pull Request #2041 · Kludex/starlette

kristjanvalur · 2023-02-18T21:41:21Z

This PR adds support for the "websocket.http.response" ASGI extension in the WebSocket class as well as in the
TestClient (see https://asgi.readthedocs.io/en/latest/extensions.html)

A new send_response() method is added to WebSocket, taking a Response object. This must only be called before accept().
- If the extension is supported, the response is sent as per extension specification
- Otherwise, a websocket.close(1008) is performed which an ASGI server should turn into a 403 response.
A new starlette.testlient.WebSocketReject exception is added which represents a disconnect before a websocket.accept() call. In a real HTTP client, it would correspond to a failure response to the Upgrade request.
Added documentation about rejecting a WebSocket connection, either with send_response() or a close() before accept()
Initially raised as discussion #...

This PR was spurred by a perceived need to standardize better how to do authentication and connection rejection in ASGI frameworks. In particular, FastAPI seems a bit wobbly there.

the API presented is a suggestion. We could use close_response() for example,.

We could also provide a code and reason for the fallback case (when the extension isn't supported by the server). How exactly a server should construct 403 out of the code and reason isn't specified by ASGI Different servers might or might not add that information to the response Body so providing that would be a pure convenience.

Kludex · 2023-02-18T21:51:49Z

Related to Initial support for Websocket Denial Response #1535

Kludex

Would you like to implement the support for Websocket Denial Response on Uvicorn?

starlette/testclient.py

tests/test_staticfiles.py

Kludex · 2023-03-17T08:50:31Z

starlette/websockets.py

+        else:
+            await self.close(code=1008, reason=f"HTTP Response {response.status_code}")


Maybe we can raise a runtime exception with "The server doesn't support the Websocket Denial Response extension." instead?

Why do you think we should have a fallback?

My thinking was that the application shouldn't have to have code dealing with server peculiarities. The application could simply always create an error response, and leave it to Starlette to decide what to do with it. If the server supports it, then fine. If not, Starlette automatically does the next best thing, simply close the socket, which is specified to create a 403 response.
The fallback code and message are arbitrary, because ASGI does not specify what should happen to these codes in case of a 403. maybe the server adds that to the 403 payload, maybe not. I haven't looked at what uvicorn does.

If this is the path we want to go, then let's remove the code and reason. Both doesn't make sense, since the handshake didn't happen, and the server actually should ignore both code and reason.

I'm still not sure if we want this... If you are using send_response, I'd expect that working out, and if it doesn't, as a developer I'd like to see an exception 🤔

But that exception appearing or not depends on which server you choose to run your app. The response here is informative only. Starlette is ASGI toolkit aimed at simplifying development.
As I see it, here are the options:

just close the connection(). This is always supported and is the original way of rejecting a connection. Server provides a 403 response.

Close the response, but proide additional information for those interested.
2a) Server supports the extension, and a response body is delivered along with the message.
2b) Server does not support the response and body is ignored.

It is th case 2b) which worries you. IMHO, the response body is informative only. Depending on web server a response body can or can not be delivered. If the information is somehow protocol critical (not recommended), then the application can always check for the presence of the extension.

Ok, how about this: Instead of an exception, how about a warning? This will get flagged to the developer but it won't crash your application if an extension isn't supported. And we won't have to push the burden on the developer to always check the state for the presence of the extension before using a close call with an extended api?

I think the RuntimeError makes more sense.

I understand what you are saying, but I don't think the premise is right.

If you start using send_denial_response, and you don't have an error, you'll believe it's working as expected, when it actually is not.

My thinking is that the denial response is an informative event. It augments the plain close event, possibly with
some nicely formatted data, information about what is missing, etc. But it is not essential to the operation of the service.

I understand it as "I'm not accepting this connection. Here is some additional information to the client, if I can send it and if he wants to receive it, but otherwise, I'm done with the connection."

This is why I suggested that maybe a warning were appropriate.

But I don't feel strongly about this anymore and am happy to do it either way, so having made my point, I can make it an exception.

starlette/websockets.py

kristjanvalur · 2023-03-17T10:06:52Z

Would you like to implement the support for Websocket Denial Response on Uvicorn?

Sure, at least, I can have a look.

starlette/websockets.py

docs/websockets.md

starlette/testclient.py

starlette/websockets.py

kristjanvalur · 2023-03-17T11:22:01Z

I'm re-adding close_code and close_reason for WebSocketReject, because this is a TestClient class, designed to capture the state when a connection is rejected, either by a close() or a send_response() before the connection is accepted.

The exception then contains the information as provided by the application.

Previously a close() in this situation would raise a WebSocketClose, now it raises a WebSocketReject, a special case with the same information, but allowing the test client application to distinguish this from a regular close.

kristjanvalur · 2023-03-22T16:53:09Z

I've also made sure that the Testclient now delivers the websocket.disconnect to the app, when the app initiates a close, either by doing a websocket.close() while open, or while rejecting a connection.

kristjanvalur · 2023-03-22T17:12:30Z

I guess the only thing here that needs deciding is if websocket.send_response() should default to websocket.close() if there is no extension. IMHO it should, because that's the only other possible thing that can be done (it is the standard, non-extended way of rejecting) and that relieves removes boilerplate logic from the app into Starlette.

But I've stated my case and will do this however you like :)

kristjanvalur · 2023-06-17T10:14:38Z

Would you like to implement the support for Websocket Denial Response on Uvicorn?

Sure, at least, I can have a look.

Kludex/uvicorn#1916

Kludex

@kristjanvalur Can you review my changes here, please?

@paulo-raca if you want to review as well...

There's still a test, and documentation to be fixed.

Kludex · 2023-12-18T12:22:51Z

starlette/websockets.py

+    async def send_response(self, response: Response) -> None:
+        if self._have_response_extension():
+            await response(self.scope, self.receive, self.send)
+        else:
+            await self.close(code=1008, reason=f"HTTP Response {response.status_code}")


Instead of this API, does it make sense to allow return Response()?

Kludex · 2023-12-18T12:24:08Z

starlette/websockets.py

+        else:
+            await self.close(code=1008, reason=f"HTTP Response {response.status_code}")


I think the RuntimeError makes more sense.

Kludex · 2023-12-19T19:56:16Z

docs/websockets.md

+
+### Rejecting the connection
+
+Before the calling `websocket.accept()` it is possible to reject the connection,


Suggested change

Before the calling `websocket.accept()` it is possible to reject the connection,

Before calling `websocket.accept()` it is possible to reject the connection,

docs/websockets.md

Kludex · 2023-12-20T11:47:38Z

tests/test_websockets.py

+def test_send_disconnect_no_code(test_client_factory: Callable[..., TestClient]):
+    close_msg: Message = {}
+
+    async def app(scope: Scope, receive: Receive, send: Send) -> None:
+        nonlocal close_msg
+        websocket = WebSocket(scope, receive=receive, send=send)
+        await websocket.accept()
+        close_msg = await websocket.receive()
+
+    client = test_client_factory(app)
+    with client.websocket_connect("/") as websocket:
+        websocket.send({"type": "websocket.disconnect"})
+
+    assert close_msg == {"type": "websocket.disconnect"}


This test needs to be fixed, I guess?

kristjanvalur

Well, this is a substantial rewrite of my PR so I don't think you need my approval for any of it.
I didn't review the tests changes for that reason.

The changes, as I see them, are

in the TestClient where you removed the ability to test for specific types of close events
Removal of protocol verification in the send() command
Removal of the return of the local disconnect event from receive().
Requiring application to add boilerplate code if it wants to terminate with a response.

Fine by me, but like I said, it is almost not my PR anymore :)

starlette/testclient.py

kristjanvalur · 2023-12-20T12:16:55Z

starlette/websockets.py

                )
            if message_type == "websocket.disconnect":
                self.client_state = WebSocketState.DISCONNECTED
-                if "code" not in message:


yes, further reading of standard indicates that this code must be application level only.

kristjanvalur · 2023-12-20T12:33:12Z

starlette/websockets.py

        """
        Receive ASGI websocket messages, ensuring valid state transitions.
        """
-        if self.app_disconnect_msg is not None:


Not sure why you are removing this. If I read asgiref correctly, a receive on a disconnected connection should return the close message which caused the disconnection, even if it was a local message.

kristjanvalur · 2023-12-20T12:33:52Z

starlette/websockets.py

            if message_type == "websocket.close":
                self.application_state = WebSocketState.DISCONNECTED
-                # no close frame is sent, then the default is 1006
-                self.app_disconnect_msg = {"type": "websocket.disconnect", "code": 1006}


see above. we are creating a close message to be read by the application, not to send over the wire. 1006 is appropriate code in this case

This is unrelated to the PR.

Strictly yes, although the PR was an overhaul of the disconnect mechanism and it seemed prudent to fill in the cracks, and it fitted well with the testing added. But I can remove the app_disconnect_msg system from this pr, and so an app won't receive() any local close().

If you can do that, it would be cool. Sorry for the back and forth here...

starlette/websockets.py

kristjanvalur · 2023-12-20T12:40:32Z

starlette/websockets.py

-            await self.close(code=1008, reason=f"HTTP Response {response.status_code}")
+        if "websocket.http.response" in self.scope.get("extensions", {}):
+            return await response(self.scope, self.receive, self.send)
+        raise RuntimeError("The server doesn't support the WebSocket Denial extension.")


For the record, I disagree here. You are forcing the creation of boilerplate in the application. When the server does not support the extension, a close() is the only available option. This requires the application to either always check for the presence of the extension, or have a try_catch mechanism built in. IMHO, sensible middleware should simply do the right thing, possibly issuing a warning. Middleware like Starlette is supposed to make the interaction with ASGI simpler, and remove such considerations from the application developer.

I agree, this should fallback to close() if the extension isn't supported

paulo-raca

Thank you for picking this up ❤️

paulo-raca · 2023-12-21T16:04:16Z

starlette/testclient.py

+    def __init__(
+        self,
+        status_code: int,
+        headers: typing.List[typing.Tuple[bytes, bytes]] = [],


Seems clumsy -- Why not Dict[str, str] (or starlette.datastructures.Headers)?

In fact, this should probably contain a httpx.Response -- this would allow reusing code more easily

paulo-raca · 2023-12-21T16:09:13Z

starlette/websockets.py

-            await self.close(code=1008, reason=f"HTTP Response {response.status_code}")
+        if "websocket.http.response" in self.scope.get("extensions", {}):
+            return await response(self.scope, self.receive, self.send)
+        raise RuntimeError("The server doesn't support the WebSocket Denial extension.")


I agree, this should fallback to close() if the extension isn't supported

paulo-raca · 2023-12-21T16:11:54Z

starlette/websockets.py

            {"type": "websocket.close", "code": code, "reason": reason or ""}
        )

+    async def send_response(self, response: Response) -> None:


nit: I would call it send_denial_response, as it is a better match to the specs and makes it clear that it is a failure scenario.

paulo-raca · 2023-12-21T16:22:02Z

starlette/testclient.py

        self.session = session


+class DenialResponse(Exception):


nit: maybe make it clear it is websocket-only exception?

Suggested change

class DenialResponse(Exception):

class WebsocketDenialResponse(Exception):

Kludex · 2024-01-20T20:55:50Z

I've reverted my changes. I'll take your comments in consideration.

Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>

Kludex · 2024-02-03T17:10:26Z

tests/test_websockets.py

+def test_send_disconnect_no_code(test_client_factory: Callable[..., TestClient]):
+    """Test that a client close message with a missing status code is accepted,
+    and verify the message passed to the application."""
+
+    close_msg: Message = {}
+
+    async def app(scope: Scope, receive: Receive, send: Send) -> None:
+        nonlocal close_msg
+        websocket = WebSocket(scope, receive=receive, send=send)
+        await websocket.accept()
+        close_msg = await websocket.receive()
+
+    client = test_client_factory(app)
+    with client.websocket_connect("/") as websocket:
+        websocket.send({"type": "websocket.disconnect"})
+
+    assert close_msg == {"type": "websocket.disconnect"}


@kristjanvalur This test doesn't make sense to me. Also, this is more a test for the TestClient than the WebSocket, so it should go in test_testclient.py.

I'll remove it, but if you want, feel free to open a new PR adding the code on the TestClient.

sure, I'll remove it.

Kludex · 2024-02-03T17:12:09Z

starlette/testclient.py

+            if not reject:
+                raise WebSocketDisconnect(
+                    message.get("code", 1000), message.get("reason", "")
+                )
+            else:
+                raise WebSocketDisconnect(
+                    code=message.get("code", 1000),
+                    reason=message.get("reason"),
+                )


Isn't this the same exception? The default empty string proves something?

it is, previously we used to raise a different exception there, to indicate a special "kind" of close. Since the distinction is now gone, I can simplify.

Ok, I see you already did all that, so never mind.

starlette/testclient.py

Kludex · 2024-02-03T17:25:14Z

@kristjanvalur I've invited you to the organization. Thanks for the help on the extension! 🙏

Please check if you are happy with my last commits, and if so, feel free to merge it. 👍

Kludex · 2024-02-03T17:25:53Z

@paulo-raca Vlw pelo review! 🙏

English: Thanks for the review! 🙏

…ymore.

…eject

kristjanvalur · 2024-02-04T17:33:29Z

Thank you, your commits were fine, will do.

Kludex · 2024-10-13T10:54:55Z

This may not have been the most intuitive API. 🤔

Maybe raising HTTPException before the acceptance would have been more intuitive?

kristjanvalur · 2024-10-14T14:59:43Z

As an alternative to calling await websocket.close() which sends the default rejection response? That would be rather unintuitive. This is on the ASGI protocol level where the handling on of the connection is handled by ASGI messages. I guess an application could use exception handling to affect the same.

Any reason why this comment comes up now? Are there any conflicts with current development?

Kludex · 2024-10-14T15:04:56Z

As an alternative to calling await websocket.close() which sends the default rejection response?

No. In contrast to raising an exception, like what I intend with this: #2725

Also, don't get me wrong, this PR was great. I'm just unsure about the API on the websocket.send_denial_response. It looks a bit weird when coding to construct the Response object before calling this.

Any reason why this comment comes up now? Are there any conflicts with current development?

I was adding this to a presentation I'm going to do, and then I noticed that we don't support raising HTTPException before the websocket.accept() is called, so I implemented it here: #2725. I just noticed that having that would be enough here.

kristjanvalur · 2024-10-14T20:07:01Z

I see. Well, exceptions are super. We probably should have considered exceptions, but my thinking was that close() was the existing API to reject a connection, and to reject the connection with extra information should be done in a similar imperative way by calling a method. I think supporting exceptions is a great alternative way to do this.

kristjanvalur marked this pull request as ready for review February 18, 2023 21:44

Kludex reviewed Mar 17, 2023

View reviewed changes

starlette/websockets.py Outdated Show resolved Hide resolved

Kludex requested changes Mar 17, 2023

View reviewed changes

docs/websockets.md Outdated Show resolved Hide resolved

starlette/testclient.py Outdated Show resolved Hide resolved

starlette/websockets.py Outdated Show resolved Hide resolved

kristjanvalur force-pushed the kristjan/reject branch from 463564a to 8810e35 Compare March 22, 2023 16:50

kristjanvalur force-pushed the kristjan/reject branch from 0c2f9b0 to 3e39873 Compare March 22, 2023 17:04

kristjanvalur mentioned this pull request Apr 10, 2023

Add handler method to process websocket connections strawberry-graphql/strawberry#2617

Closed

11 tasks

Kludex added this to the Version 1.x milestone Jul 5, 2023

mnixry mentioned this pull request Aug 31, 2023

Add WebSocket handling support for HTTP security dependencies fastapi/fastapi#10147

Open

Kludex changed the title ~~Support the "websocket.http.response" ASGI extension~~ Support the WebSocket Denial Response ASGI extension Dec 17, 2023

Kludex force-pushed the kristjan/reject branch from 2882d32 to 4a42c3c Compare December 17, 2023 10:12

Kludex reviewed Dec 20, 2023

View reviewed changes

kristjanvalur commented Dec 20, 2023

View reviewed changes

paulo-raca reviewed Dec 21, 2023

View reviewed changes

Kludex mentioned this pull request Jan 8, 2024

Version 0.35.0 #2404

Merged

1 task

kristjanvalur and others added 8 commits January 20, 2024 21:58

supply asgi_extensions to TestClient

83f863d

Add WebSocket.send_response()

2e311be

Add response support for WebSocket testclient

936a075

fix test for filesystem line-endings

b0edc2b

lintint

a230ba5

support websocket.http.response extension by default

86ff3a1

Improve coverate

9c6ddc4

Apply suggestions from code review

9e9773c

Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>

kristjanvalur added 2 commits February 3, 2024 14:56

Add type hings to test methods

a561041

Add doc string to test.

1e70746

kristjanvalur force-pushed the kristjan/reject branch 2 times, most recently from 69efd9e to b70cdbd Compare February 3, 2024 15:47

Fix mypy complaining about mismatching parent methods.

4f27aba

kristjanvalur force-pushed the kristjan/reject branch from b70cdbd to 4f27aba Compare February 3, 2024 15:49

nitpick & remove test

f53efa6

Kludex reviewed Feb 3, 2024

View reviewed changes

Kludex added 2 commits February 3, 2024 18:18

Simplify the documentation

dd90d1b

Merge branch 'master' into kristjan/reject

fa7c84e

Kludex reviewed Feb 3, 2024

View reviewed changes

starlette/testclient.py Outdated Show resolved Hide resolved

Update starlette/testclient.py

9bd6db5

Kludex reviewed Feb 3, 2024

View reviewed changes

starlette/testclient.py Outdated Show resolved Hide resolved

Update starlette/testclient.py

721a2d2

Kludex approved these changes Feb 3, 2024

View reviewed changes

Kludex mentioned this pull request Feb 3, 2024

Version 0.37.0 #2457

Merged

1 task

kristjanvalur added 3 commits February 4, 2024 17:25

Remove an unnecessary test

a70b7dd

there is no special "close because of rejection" in the testclient an…

0f4e1f2

…ymore.

Merge remote-tracking branch 'origin/kristjan/reject' into kristjan/r…

04817a6

…eject

kristjanvalur and others added 3 commits February 4, 2024 17:33

Merge branch 'master' into kristjan/reject

bc3ecbd

Merge branch 'master' into kristjan/reject

aa224dd

Merge branch 'master' into kristjan/reject

094f080

kristjanvalur merged commit 93e74a4 into Kludex:master Feb 4, 2024

		else:
		await self.close(code=1008, reason=f"HTTP Response {response.status_code}")


		### Rejecting the connection

		Before the calling `websocket.accept()` it is possible to reject the connection,

	Before the calling `websocket.accept()` it is possible to reject the connection,
	Before calling `websocket.accept()` it is possible to reject the connection,

	class DenialResponse(Exception):
	class WebsocketDenialResponse(Exception):

Uh oh!

Conversation

kristjanvalur commented Feb 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kludex commented Feb 18, 2023

Uh oh!

Kludex left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kristjanvalur commented Mar 17, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kristjanvalur commented Mar 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kristjanvalur commented Mar 22, 2023

Uh oh!

kristjanvalur commented Mar 22, 2023

Uh oh!

kristjanvalur commented Jun 17, 2023

Uh oh!

Kludex left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kristjanvalur left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulo-raca left a comment

Choose a reason for hiding this comment

Uh oh!

kristjanvalur commented Feb 18, 2023 •

edited

Loading

kristjanvalur commented Mar 17, 2023 •

edited

Loading