Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Denying WebSocket connection in WebSocketEndpoint.on_connect leads to Exception #1560

Closed
Kludex opened this issue Mar 30, 2022 Discussed in #1555 · 4 comments · Fixed by #1574
Closed

Denying WebSocket connection in WebSocketEndpoint.on_connect leads to Exception #1560

Kludex opened this issue Mar 30, 2022 Discussed in #1555 · 4 comments · Fixed by #1574
Labels
help wanted Feel free to help

Comments

@Kludex
Copy link
Sponsor Member

Kludex commented Mar 30, 2022 •
edited
Loading

Discussed in #1555

Originally posted by dingensundso March 27, 2022
I created a WebSocketEndpoint class in which I want to deny the connection in certain conditions.
When the WebSocket is closed in on_connect I receive the following exception:

  File "starlette/endpoints.py", line 83, in dispatch
    close_code = int(message.get("code", status.WS_1000_NORMAL_CLOSURE))
TypeError: int() argument must be a string, a bytes-like object or a real number, not 'NoneType'

So code is None when the server denies the connection instead of non-existant.

starlette/starlette/endpoints.py

Lines 72 to 87 in e086fc2

await self.on_connect(websocket)
close_code = status.WS_1000_NORMAL_CLOSURE
try:
while True:
message = await websocket.receive()
if message["type"] == "websocket.receive":
data = await self.decode(websocket, message)
await self.on_receive(websocket, data)
elif message["type"] == "websocket.disconnect":
close_code = int(message.get("code", status.WS_1000_NORMAL_CLOSURE))
break
except Exception as exc:
close_code = status.WS_1011_INTERNAL_ERROR
raise exc

Changing line 83 to the following should fix the issue:

close_code = int(message.get("code") or status.WS_1000_NORMAL_CLOSURE)
@Kludex Kludex added the help wanted Feel free to help label Mar 30, 2022
@Kludex
Copy link
Sponsor Member Author

Kludex commented Mar 30, 2022

Can someone confirm this?

@aminalaee
Copy link
Member

I can't reproduce this, a minimal example would be very helpful.

@dingensundso
Copy link

While creating a minimal example I noticed that the problem only exists when using wsproto instead of websockets.

Minimal example:

from starlette.applications import Starlette
from starlette.endpoints import WebSocketEndpoint, HTTPEndpoint
from starlette.responses import HTMLResponse
from starlette.routing import Route, WebSocketRoute

html = """
<!DOCTYPE html>
<script>
    var ws = new WebSocket("ws://localhost:8000/ws");
</script>
"""

class Homepage(HTTPEndpoint):
    async def get(self, request):
        return HTMLResponse(html)

class Echo(WebSocketEndpoint):
    async def on_connect(self, websocket):
        await websocket.close()

routes = [
    Route("/", Homepage),
    WebSocketRoute("/ws", Echo)
]

app = Starlette(routes=routes)

Running it with uvicorn ws:app --ws=wsproto and opening the URL produces the Exception.

@Kludex Kludex mentioned this issue Apr 5, 2022
Closed
1 task
@Kludex
Copy link
Sponsor Member Author

Kludex commented Apr 5, 2022

As per https://asgi.readthedocs.io/en/latest/specs/www.html#disconnect-receive-event-ws (image below), the application should receive the "code" from the server. There's no default on the specs, meaning that the server MUST send it.

image

The interpretation of "if the server" should default to a value, or if the application should be the one to default to, is confusing... That being said, I'll create a fix for Starlette as well.

@Kludex Kludex mentioned this issue Apr 5, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
help wanted Feel free to help
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Avoid TypeError on websocket.disconnect when code is None encode/starlette
3 participants
@dingensundso @Kludex @aminalaee