diff --git a/server/README.md b/server/README.md index 1fffee3..51fdeaa 100644 --- a/server/README.md +++ b/server/README.md @@ -233,8 +233,11 @@ All endpoints except `/api/health`, `/api/docs`, `/api/openapi.json`, ## WebSocket protocol Connect to `/ws/games/{game_id}` with your session cookie. Only seated -players are accepted; unauthenticated, unknown-game and non-player -connections are closed with codes `4401`, `4404` and `4403` respectively. +players are accepted; the upgrade is refused before the handshake +completes with an HTTP status mirroring `GET /api/games/{id}`: `401` +unauthenticated, `404` unknown game, `403` not seated. (Browsers report +a refused handshake as an abnormal close — there is no close code to +read.) Server → client messages are JSON objects with a `type`: diff --git a/server/packages/tavolo-platform/src/tavolo/platform/ws.py b/server/packages/tavolo-platform/src/tavolo/platform/ws.py index b4e77be..0b0f5ff 100644 --- a/server/packages/tavolo-platform/src/tavolo/platform/ws.py +++ b/server/packages/tavolo-platform/src/tavolo/platform/ws.py @@ -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() diff --git a/server/packages/tavolo-platform/tests/test_ws.py b/server/packages/tavolo-platform/tests/test_ws.py index a972b3a..7006d13 100644 --- a/server/packages/tavolo-platform/tests/test_ws.py +++ b/server/packages/tavolo-platform/tests/test_ws.py @@ -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__": diff --git a/server/tests/test_websocket.py b/server/tests/test_websocket.py index dc74ced..b904bf3 100644 --- a/server/tests/test_websocket.py +++ b/server/tests/test_websocket.py @@ -131,7 +131,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: @@ -146,7 +146,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: @@ -156,7 +156,7 @@ 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) async def _seed_last_play_state(hand_ack_timeout: int = 30) -> str: diff --git a/web/src/api.rs b/web/src/api.rs index 1c61413..13dd9cf 100644 --- a/web/src/api.rs +++ b/web/src/api.rs @@ -87,16 +87,33 @@ pub async fn join_preview(code: &str) -> Result, String> { resp.json().await.map(Some).map_err(|e| e.to_string()) } -#[allow(dead_code)] -pub async fn game_state(game_id: &str) -> Result { - let resp = Request::get(&format!("/api/games/{game_id}")) - .send() - .await - .map_err(|e| e.to_string())?; - if !resp.ok() { - return Err(server_error(resp.status())); +/// Why a game connection failed before the first snapshot arrived. +/// +/// A refused WebSocket handshake is invisible to the browser: the close +/// event reports an abnormal closure (1006) with no HTTP status, so the +/// game page asks the REST API to tell a deliberate rejection from a +/// transient network problem. +pub enum GameEntry { + /// Not logged in (`401` from `GET /api/games/{id}`). + Unauthenticated, + /// Unknown game (`404`) or the user is not seated (`403`). + Unavailable, + /// The API could not be asked (or answered unexpectedly): treat the + /// connection loss as transient and retry. + Transient, +} + +/// Classify a failed game connection via `GET /api/games/{game_id}`, +/// mirroring the statuses used to refuse the websocket upgrade. +pub async fn probe_game(game_id: &str) -> GameEntry { + match Request::get(&format!("/api/games/{game_id}")).send().await { + Ok(resp) => match resp.status() { + 401 => GameEntry::Unauthenticated, + 403 | 404 => GameEntry::Unavailable, + _ => GameEntry::Transient, + }, + Err(_) => GameEntry::Transient, } - resp.json().await.map_err(|e| e.to_string()) } pub async fn my_matches(cursor: Option<&str>) -> Result { diff --git a/web/src/pages/game.rs b/web/src/pages/game.rs index 4410c37..4fb33f1 100644 --- a/web/src/pages/game.rs +++ b/web/src/pages/game.rs @@ -4,7 +4,9 @@ use std::cell::RefCell; use std::rc::Rc; use sycamore::prelude::*; +use wasm_bindgen_futures::spawn_local; +use crate::api; use crate::components::card::{card_back, card_img}; use crate::components::summary::{hand_summary_modal, summary_rows}; use crate::components::toast::toast; @@ -135,23 +137,38 @@ fn start_connect(id: Rc, ctx: ConnCtx, alive: Rc>) { let on_close = { let id = id.clone(); let alive = alive.clone(); - move |code: Option| { + move |_code: Option| { if !alive.get() { return; } ctx.closed.set(true); - match code { - Some(4401) => { - ctx.fatal.set(true); - ctx.error - .set(Some("Session expired — please log in again.".to_string())); - } - Some(4403) | Some(4404) => { - ctx.fatal.set(true); - ctx.error - .set(Some("This game is no longer available.".to_string())); - } - _ => schedule_retry(id.clone(), ctx, alive.clone()), + if ctx.game.get_clone().is_none() && ctx.attempts.get() == 0 { + // The connection ended before any snapshot arrived. A + // refused handshake looks exactly like a network drop to + // the browser (close code 1006, no HTTP status), so ask + // the API whether the server deliberately rejected us — + // in which case retrying is pointless — or the loss was + // transient. Only on the first failure: later attempts + // are governed by the retry backoff. + let id = id.clone(); + let alive = alive.clone(); + spawn_local(async move { + match api::probe_game(&id).await { + api::GameEntry::Unauthenticated => { + ctx.fatal.set(true); + ctx.error + .set(Some("Session expired — please log in again.".to_string())); + } + api::GameEntry::Unavailable => { + ctx.fatal.set(true); + ctx.error + .set(Some("This game is no longer available.".to_string())); + } + api::GameEntry::Transient => schedule_retry(id, ctx, alive), + } + }); + } else { + schedule_retry(id.clone(), ctx, alive.clone()); } } }; diff --git a/web/src/ws.rs b/web/src/ws.rs index 3e07eb9..8fdb363 100644 --- a/web/src/ws.rs +++ b/web/src/ws.rs @@ -55,8 +55,11 @@ impl GameSocket { /// could not be created. /// /// `on_close` fires exactly once when the connection ends; it receives the -/// server close code when one was sent (e.g. 4401 unauthenticated, 4403 not -/// seated, 4404 unknown game) or `None` for an abnormal network loss. +/// server close code when one was sent (e.g. 1000 for a normal closure) or +/// `None` for an abnormal loss. A *refused* handshake is not distinguishable +/// here: browsers report it as an abnormal closure (1006) and never expose +/// the HTTP status the server answered with, so callers must classify the +/// failure out of band. pub fn connect( game_id: &str, on_message: impl Fn(ServerMessage) + 'static,