Refuse websocket upgrades with HTTP statuses, not close codes
A close() before accept() carries the HTTP status of the refused
upgrade, not a WebSocket close code: RSGI (granian) uses the value
verbatim as the response status, so 4401 panicked on the invalid status
(InvalidStatusCode) and — granian being built with panic = "abort" —
aborted the worker and took the whole app down. ASGI servers answer 403
regardless, and browsers report a refused handshake as an abnormal close
(1006), so the 4401/4403/4404 codes never reached clients anyway.
Reject 401/404/403 to mirror GET /api/games/{id}, and classify a failed
first connection in the game page through the REST API so players still
see "session expired" / "game no longer available" instead of a retry
loop. Add a regression test asserting every pre-accept rejection uses a
status in the HTTP range.
This commit is contained in:
@@ -68,6 +68,21 @@ def _error(message: str, code: str = "invalid") -> Dict[str, Any]:
|
||||
return {"type": "error", "code": code, "message": message}
|
||||
|
||||
|
||||
async def _reject(ws: WebSocket, status: int) -> None:
|
||||
"""Refuse a websocket upgrade with the HTTP ``status``.
|
||||
|
||||
Rejections happen before :meth:`WebSocket.accept`, so ``close()``
|
||||
carries the HTTP status of the refused upgrade rather than a
|
||||
WebSocket close code: ASGI servers answer ``403`` per the spec, and
|
||||
RSGI (granian) uses the value verbatim as the response status.
|
||||
Passing a 4xxx close code here would panic granian's RSGI layer on
|
||||
the invalid status — and since granian is built with
|
||||
``panic = "abort"``, that kills the worker and takes the whole app
|
||||
down.
|
||||
"""
|
||||
await ws.close(status)
|
||||
|
||||
|
||||
def _state_message(platform: "Platform", session: GameSession, sub: str) -> Dict[str, Any]:
|
||||
engine = platform.registry.require(session.game_type)
|
||||
return {
|
||||
@@ -89,17 +104,17 @@ def register(app: KayaApp, platform: "Platform") -> None:
|
||||
user = auth.get_ws_user(ws)
|
||||
if user is None:
|
||||
log.debug("websocket %s rejected: no authenticated user", game_id)
|
||||
await ws.close(4401)
|
||||
await _reject(ws, 401)
|
||||
return
|
||||
|
||||
session = await platform.game_store.load(game_id)
|
||||
if session is None:
|
||||
log.debug("websocket rejected: unknown game %s", game_id)
|
||||
await ws.close(4404)
|
||||
await _reject(ws, 404)
|
||||
return
|
||||
if not session.seated(user.sub):
|
||||
log.debug("websocket %s rejected: %s is not seated", game_id, user.sub)
|
||||
await ws.close(4403)
|
||||
await _reject(ws, 403)
|
||||
return
|
||||
|
||||
await ws.accept()
|
||||
|
||||
@@ -2,12 +2,46 @@
|
||||
from __future__ import annotations
|
||||
|
||||
import unittest
|
||||
from typing import Any, Mapping, Sequence
|
||||
|
||||
from httpx import ASGITransport, AsyncClient
|
||||
from httpx_ws import WebSocketDisconnect, aconnect_ws
|
||||
from httpx_ws.transport import ASGIWebSocketTransport
|
||||
from kaya.core import WebSocket, WebSocketMessage
|
||||
|
||||
from helpers import async_test, make_platform, make_user, oidc_user, ws_users
|
||||
from tavolo.platform import GameSession, Seat
|
||||
|
||||
|
||||
class RecordingSocket(WebSocket):
|
||||
"""A socket that only records pre-accept rejections."""
|
||||
|
||||
def __init__(self, path: str) -> None:
|
||||
self.path = path
|
||||
self.query_string = ""
|
||||
self.scheme = "ws"
|
||||
self.headers: Mapping[str, Sequence[str]] = {}
|
||||
self.client = ("127.0.0.1", 12345)
|
||||
self.server = ("127.0.0.1", 80)
|
||||
self.close_codes: list[int] = []
|
||||
|
||||
async def accept(self, headers: Any = None) -> None:
|
||||
raise AssertionError("a rejected socket must not be accepted")
|
||||
|
||||
async def receive(self) -> WebSocketMessage:
|
||||
raise AssertionError("a rejected socket must not receive")
|
||||
|
||||
async def send_text(self, data: str) -> None:
|
||||
raise AssertionError("a rejected socket must not send")
|
||||
|
||||
async def send_bytes(self, data: bytes) -> None:
|
||||
raise AssertionError("a rejected socket must not send")
|
||||
|
||||
async def close(self, code: int = 1000) -> None:
|
||||
self.close_codes.append(code)
|
||||
|
||||
async def __anext__(self) -> WebSocketMessage:
|
||||
raise AssertionError("a rejected socket must not receive")
|
||||
|
||||
|
||||
class WebSocketTest(unittest.TestCase):
|
||||
@@ -124,7 +158,7 @@ class WebSocketTest(unittest.TestCase):
|
||||
with self.assertRaises(WebSocketDisconnect) as caught:
|
||||
async with aconnect_ws("/ws/games/no-such-game", ws_client):
|
||||
pass
|
||||
self.assertEqual(4404, caught.exception.code)
|
||||
self.assertEqual(404, caught.exception.code)
|
||||
|
||||
@async_test
|
||||
async def test_non_player_is_closed(self) -> None:
|
||||
@@ -140,7 +174,7 @@ class WebSocketTest(unittest.TestCase):
|
||||
with self.assertRaises(WebSocketDisconnect) as caught:
|
||||
async with aconnect_ws(f"/ws/games/{game_id}", ws_client):
|
||||
pass
|
||||
self.assertEqual(4403, caught.exception.code)
|
||||
self.assertEqual(403, caught.exception.code)
|
||||
|
||||
@async_test
|
||||
async def test_unauthenticated_is_closed(self) -> None:
|
||||
@@ -151,7 +185,45 @@ class WebSocketTest(unittest.TestCase):
|
||||
with self.assertRaises(WebSocketDisconnect) as caught:
|
||||
async with aconnect_ws("/ws/games/whatever", ws_client):
|
||||
pass
|
||||
self.assertEqual(4401, caught.exception.code)
|
||||
self.assertEqual(401, caught.exception.code)
|
||||
|
||||
|
||||
class RejectionStatusTest(unittest.TestCase):
|
||||
"""Pre-accept rejections must carry HTTP statuses, not close codes.
|
||||
|
||||
Granian's RSGI layer uses the value passed to ``close()`` before
|
||||
``accept()`` as the HTTP status of the refused upgrade; a WebSocket
|
||||
close code like 4401 is not a valid status and panics the worker
|
||||
(``InvalidStatusCode``, and with ``panic = "abort"`` the whole app
|
||||
dies). Drive the rejections through the app with a recording socket
|
||||
so the invariant is checked without a real server.
|
||||
"""
|
||||
|
||||
@async_test
|
||||
async def test_pre_accept_rejections_are_http_statuses(self) -> None:
|
||||
app, platform, _ = make_platform()
|
||||
session = GameSession(
|
||||
id="seated-game",
|
||||
game_type="dummy",
|
||||
join_code="SEAT01",
|
||||
creator_sub="alice",
|
||||
players=[Seat(user_sub="alice", display_name="Alice")],
|
||||
)
|
||||
engine = platform.registry.require("dummy")
|
||||
engine.create(session, {})
|
||||
await platform.game_store.save(session)
|
||||
cases = [
|
||||
("unauthenticated", [], "/ws/games/whatever", 401),
|
||||
("unknown game", [make_user("alice")], "/ws/games/no-such-game", 404),
|
||||
("not seated", [make_user("mallory")], f"/ws/games/{session.id}", 403),
|
||||
]
|
||||
for label, users, path, expected in cases:
|
||||
ws = RecordingSocket(path)
|
||||
with ws_users(users):
|
||||
await app.handle_websocket(ws)
|
||||
self.assertEqual([expected], ws.close_codes, label)
|
||||
for code in ws.close_codes:
|
||||
self.assertTrue(100 <= code <= 599, f"{label}: {code} is not an HTTP status")
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
|
||||
Reference in New Issue
Block a user