From 18140b39f3ecb52b16e4244dc408d07aeffc90d8 Mon Sep 17 00:00:00 2001 From: Paddy Mullen Date: Wed, 20 May 2026 23:32:05 -0400 Subject: [PATCH 1/3] test(server): failing test for WS on_message malformed-shape robustness MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #805: ``on_message`` calls ``msg.get(...)`` on whatever ``json.loads`` returns without checking it's a dict. ``null`` in particular kills the WS — ``None.get`` raises ``AttributeError``, Tornado swallows the exception, the stream closes. Adjacent: unknown message types (``{"type": 42}``, missing ``type``, empty ``{}``) get silently dropped — no error frame, no log, client times out. Test sends 7 malformed shapes and asserts each gets a structured ``{"type": "error"}`` frame within 3 seconds. Today fails on ``bare_null`` (AttributeError closes the WS) and would also fail on the other 6 (silent drop, response timeout). Co-Authored-By: Claude Opus 4.7 (1M context) --- tests/unit/server/test_load_expr.py | 48 +++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/tests/unit/server/test_load_expr.py b/tests/unit/server/test_load_expr.py index e4d6cc9db..3f176ca7f 100644 --- a/tests/unit/server/test_load_expr.py +++ b/tests/unit/server/test_load_expr.py @@ -141,6 +141,54 @@ async def test_ws_search_pushdown(self): finally: shutil.rmtree(builds_root, ignore_errors=True) + @tornado.testing.gen_test + async def test_ws_message_robustness(self): + """Regression for #805: ``on_message`` did ``msg.get(...)`` on + whatever ``json.loads`` returned, which is unsafe when the JSON + is not an object (``null``, bare arrays, scalars). ``null`` in + particular killed the WS — ``None.get`` raises ``AttributeError``, + Tornado swallows it, the stream closes. + + Adjacent: unknown message types (``{"type": 42}``, missing + ``type``, empty ``{}``) were silently dropped. Clients couldn't + debug because no response came. + + This test sends each malformed shape and asserts the server + returns a structured error frame, NOT a silent drop or a + crashed WS. + """ + import asyncio + await _post(self.get_http_port(), "/load", + {"session": "ws-guard", + "path": "/tmp/restaurant-complaints-pandas.parquet", + "mode": "buckaroo", "no_browser": True}) + ws = await tornado.websocket.websocket_connect( + f"ws://localhost:{self.get_http_port()}/ws/ws-guard") + await ws.read_message() # discard initial_state + + # Each entry: (label, raw_ws_message). Server must respond + # to each with a structured error frame within 2s. + cases = [ + ("bare_null", "null"), + ("bare_array", "[1,2,3]"), + ("bare_scalar", "42"), + ("empty_object", "{}"), + ("missing_type", json.dumps({"payload": "x"})), + ("type_as_int", json.dumps({"type": 42, "payload": "x"})), + ("unknown_type", + json.dumps({"type": "buckaroo_invented_command"})), + ] + for label, raw in cases: + ws.write_message(raw) + frame = await asyncio.wait_for(ws.read_message(), timeout=3.0) + self.assertIsNotNone(frame, f"{label}: no response (silent drop)") + d = json.loads(frame) + self.assertEqual(d.get("type"), "error", + f"{label}: expected error frame, got {d.get('type')!r}") + self.assertIn("error_code", d, + f"{label}: error frame missing error_code") + ws.close() + @tornado.testing.gen_test async def test_session_reuse_xorq_then_pandas(self): """A client that POSTs /load_expr and then POSTs /load with the From 228fdbf9fd490eb5e11b90d9108217dcefaeabf4 Mon Sep 17 00:00:00 2001 From: Paddy Mullen Date: Wed, 20 May 2026 23:32:55 -0400 Subject: [PATCH 2/3] fix(server): guard WS on_message against non-dict JSON + unknown types MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #805. ``on_message`` was calling ``msg.get(...)`` on whatever ``json.loads`` returned without checking it was a dict. ``null`` in particular killed the WS — ``None.get`` raises ``AttributeError``, Tornado swallows the exception, the stream closes permanently. Easy hostile DoS: send one literal ``null`` to close the WS. Adjacent: unknown / missing ``type`` values were silently dropped — no response, no log entry, clients with no way to debug. This patch adds two guards at the top of ``on_message``: - If the parsed JSON isn't a dict (null, arrays, scalars), return ``{"type":"error","error_code":"invalid_message_shape", ...}``. - If ``msg_type`` doesn't match any known branch, return ``{"type":"error","error_code":"unknown_message_type", ...}``. Test from the previous commit now passes (7/7 malformed shapes get structured error frames). Co-Authored-By: Claude Opus 4.7 (1M context) --- buckaroo/server/websocket_handler.py | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/buckaroo/server/websocket_handler.py b/buckaroo/server/websocket_handler.py index da5917932..01c9cac29 100644 --- a/buckaroo/server/websocket_handler.py +++ b/buckaroo/server/websocket_handler.py @@ -42,11 +42,27 @@ def on_message(self, message): self.write_message(json.dumps({"type": "error", "error_code": "invalid_json", "message": "Invalid JSON"})) return + # Reject non-object JSON (null, arrays, scalars) explicitly. + # Without this guard, ``msg.get(...)`` below raises ``AttributeError`` + # on ``None`` etc., Tornado swallows it, and the WS closes + # permanently. See #805. + if not isinstance(msg, dict): + self.write_message(json.dumps({"type": "error", + "error_code": "invalid_message_shape", + "message": f"WS messages must be JSON objects; got {type(msg).__name__}"})) + return + msg_type = msg.get("type") if msg_type == "infinite_request": self._handle_infinite_request(msg.get("payload_args", {})) elif msg_type == "buckaroo_state_change": self._handle_buckaroo_state_change(msg.get("new_state") or {}) + else: + # Pre-#805 unknown / missing ``type`` was silently dropped — + # clients had no way to debug. Tell them what they sent. + self.write_message(json.dumps({"type": "error", + "error_code": "unknown_message_type", + "message": f"Unknown WS message type: {msg_type!r}"})) def _handle_buckaroo_state_change(self, new_state): sessions = self.application.settings["sessions"] From b4534d4d5b1490ede25faa10d0cb790417090ad5 Mon Sep 17 00:00:00 2001 From: Paddy Mullen Date: Thu, 21 May 2026 19:46:49 -0400 Subject: [PATCH 3/3] test(server): make test_ws_message_robustness portable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The test pointed `/load` at a hardcoded `/tmp/restaurant-complaints-pandas.parquet` that only existed on the author's machine. CI runners 404'd silently (`raise_error=False`), the WS opened against a session with no data so `open()` sent no `initial_state`, and `await ws.read_message()` blocked forever — `gen_test` timed out at 5s on Python 3.11/3.12/3.13. 3.14 only "passed" because the module's `pytest.importorskip("xorq.api")` skipped the whole file there. The malformed-shape guards run before any session lookup, so the test doesn't need `/load` at all. Drop the setup and the `discard initial_state` read, hoist `import asyncio` to module level, and add a liveness check at the end: a well-formed `infinite_request` must still get a response after the malformed barrage — that's the exact regression #805 was about (the WS dying permanently on `null`). Co-Authored-By: Claude Opus 4.7 (1M context) --- tests/unit/server/test_load_expr.py | 30 +++++++++++++++++++++-------- 1 file changed, 22 insertions(+), 8 deletions(-) diff --git a/tests/unit/server/test_load_expr.py b/tests/unit/server/test_load_expr.py index 3f176ca7f..4d6ebeca2 100644 --- a/tests/unit/server/test_load_expr.py +++ b/tests/unit/server/test_load_expr.py @@ -1,5 +1,6 @@ """End-to-end tests for POST /load_expr — server load path for XorqBuckarooInfiniteWidget over a xorq/ibis expression.""" +import asyncio import io import json import os @@ -155,19 +156,19 @@ async def test_ws_message_robustness(self): This test sends each malformed shape and asserts the server returns a structured error frame, NOT a silent drop or a - crashed WS. + crashed WS. After the barrage, a well-formed infinite_request + confirms the WS is still alive — the headline pre-fix bug was + that ``null`` permanently killed the stream. + + No ``/load`` setup: the guards run before any session lookup, + so the test works against an empty session and is portable + across machines. """ - import asyncio - await _post(self.get_http_port(), "/load", - {"session": "ws-guard", - "path": "/tmp/restaurant-complaints-pandas.parquet", - "mode": "buckaroo", "no_browser": True}) ws = await tornado.websocket.websocket_connect( f"ws://localhost:{self.get_http_port()}/ws/ws-guard") - await ws.read_message() # discard initial_state # Each entry: (label, raw_ws_message). Server must respond - # to each with a structured error frame within 2s. + # to each with a structured error frame within 3s. cases = [ ("bare_null", "null"), ("bare_array", "[1,2,3]"), @@ -187,6 +188,19 @@ async def test_ws_message_robustness(self): f"{label}: expected error frame, got {d.get('type')!r}") self.assertIn("error_code", d, f"{label}: error frame missing error_code") + + # Liveness: WS survived the malformed barrage. Send a well-formed + # request and confirm we still get a response. With no session + # data loaded, _handle_infinite_request returns an infinite_resp + # with empty data — what matters is that *any* frame arrives. + ws.write_message(json.dumps({ + "type": "infinite_request", + "payload_args": {"start": 0, "end": 10, + "sourceName": "default", "origEnd": 10}})) + frame = await asyncio.wait_for(ws.read_message(), timeout=3.0) + self.assertIsNotNone(frame, "WS died after malformed barrage") + d = json.loads(frame) + self.assertEqual(d.get("type"), "infinite_resp") ws.close() @tornado.testing.gen_test