Security quick wins: show deletion auth, debug default off, request size metrics

README.md

@@ -86,6 +86,23 @@ docker-compose up -d
 
 DigiScript will be available at `http://localhost:8080`
 
+### Request size metrics
+
+The server exposes two Prometheus histograms at `/debug/metrics` so that request size limits can be
+tuned from real usage (Tornado's defaults currently apply: 100 MB HTTP bodies, 10 MiB WebSocket messages):
+
+| Metric | Labels | Meaning |
+|--------|--------|---------|
+| `digiscript_http_request_body_bytes` | `handler`, `method` | Size of HTTP request bodies (requests with a body only) |
+| `digiscript_ws_message_bytes` | none | Size of inbound WebSocket messages |
+
+Buckets are powers of 4 from 1 KiB to 256 MiB. For example, the 99th percentile WebSocket message size
+over the last day in Prometheus:
+
+```
+histogram_quantile(0.99, sum(rate(digiscript_ws_message_bytes_bucket[1d])) by (le))
+```
+
 ### Manual Installation
 
 #### Client
@@ -110,6 +127,8 @@ python3 main.py
 
 The server will be available at `http://localhost:8080`
 
+The server runs in production mode by default. When developing (for example with the `client-v3` Vite dev server), start it with `python3 main.py --debug` to enable autoreload and accept WebSocket connections from other origins. Never use `--debug` on a deployed server.
+
 ### First Launch Setup
 
 On first launch, DigiScript will prompt you to create an administrator user. From there, you can create shows, configure users, and begin building your production.

client-v3/README.md

@@ -12,7 +12,7 @@ pnpm install
 pnpm run dev
 ```
 
-The dev server starts at `http://localhost:5173` and proxies API requests to `http://localhost:8080`.
+The dev server starts at `http://localhost:5173` and proxies API requests to `http://localhost:8080`. Run the backend with `python3 main.py --debug` while developing.
 
 ### Compiles and minifies for production
 ```

client-v3/src/composables/useWebSocket.closeTooBig.test.ts

@@ -0,0 +1,147 @@
+import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
+
+// Close code 1009 (message too big) must back off instead of reconnecting at the minimum
+// delay forever.
+
+vi.mock('@/router', () => ({ default: { currentRoute: { value: { path: '/' } }, push: vi.fn() } }));
+vi.mock('@/js/toast', () => ({
+  toast: { info: vi.fn(), error: vi.fn(), success: vi.fn() },
+}));
+
+class FakeWebSocket {
+  static OPEN = 1;
+  static CONNECTING = 0;
+  static instances: FakeWebSocket[] = [];
+  readyState = 3;
+  onopen: (() => Promise<void> | void) | null = null;
+  onmessage: ((e: { data: string }) => void) | null = null;
+  onclose: ((e?: { code: number }) => void) | null = null;
+  onerror: (() => void) | null = null;
+
+  constructor() {
+    FakeWebSocket.instances.push(this);
+  }
+
+  send(): void {}
+}
+
+// The composable keeps its backoff state at module level, so load a fresh copy per test.
+let useWebSocket: typeof import('./useWebSocket').useWebSocket;
+let toast: typeof import('@/js/toast').toast;
+
+describe('close code 1009', () => {
+  beforeEach(async () => {
+    vi.resetModules();
+    FakeWebSocket.instances = [];
+    vi.useFakeTimers();
+    vi.stubGlobal('WebSocket', FakeWebSocket);
+    const { createPinia, setActivePinia } = await import('pinia');
+    setActivePinia(createPinia());
+    ({ useWebSocket } = await import('./useWebSocket'));
+    ({ toast } = await import('@/js/toast'));
+  });
+
+  afterEach(() => {
+    vi.useRealTimers();
+    vi.unstubAllGlobals();
+  });
+
+  it('re-syncs the live session after reconnecting from a 1009 close', async () => {
+    const { useShowStore } = await import('@/stores/show');
+    const showStore = useShowStore();
+    showStore.currentSession = { id: 1 } as never;
+    const resync = vi.spyOn(showStore, 'getShowSessionData').mockResolvedValue();
+
+    useWebSocket().connect();
+    FakeWebSocket.instances[0].onclose?.({ code: 1009 });
+    vi.advanceTimersByTime(2100);
+    await FakeWebSocket.instances[1].onopen?.();
+
+    expect(resync).toHaveBeenCalledTimes(1);
+    expect(toast.success).toHaveBeenCalledWith('WebSocket reconnected after 1 attempt');
+  });
+
+  it('forgets earlier 1009 closes once a connection has stayed up for 30s', async () => {
+    useWebSocket().connect();
+    FakeWebSocket.instances[0].onclose?.({ code: 1009 });
+    vi.advanceTimersByTime(2100);
+    // Second connection stays up long enough to clear the earlier 1009 count.
+    await FakeWebSocket.instances[1].onopen?.();
+    vi.advanceTimersByTime(30100);
+    FakeWebSocket.instances[1].onclose?.({ code: 1009 });
+    // Count was cleared, so this is a first close again: 2s delay, not 4s.
+    vi.advanceTimersByTime(2100);
+    expect(FakeWebSocket.instances).toHaveLength(3);
+  });
+
+  it('keeps the 1009 count when the connection was up for less than 30s', async () => {
+    useWebSocket().connect();
+    FakeWebSocket.instances[0].onclose?.({ code: 1009 });
+    vi.advanceTimersByTime(2100);
+    await FakeWebSocket.instances[1].onopen?.();
+    vi.advanceTimersByTime(10000);
+    FakeWebSocket.instances[1].onclose?.({ code: 1009 });
+    // Second consecutive close: 4s delay.
+    vi.advanceTimersByTime(3500);
+    expect(FakeWebSocket.instances).toHaveLength(2);
+    vi.advanceTimersByTime(600);
+    expect(FakeWebSocket.instances).toHaveLength(3);
+  });
+
+  it('keeps backing off when authentication succeeds between closes', async () => {
+    useWebSocket().connect();
+    FakeWebSocket.instances[0].onclose?.({ code: 1009 });
+    vi.advanceTimersByTime(2100);
+    expect(FakeWebSocket.instances).toHaveLength(2);
+
+    // Reconnect and re-authenticate (this resets the generic error counter).
+    FakeWebSocket.instances[1].onmessage?.({ data: JSON.stringify({ OP: 'WS_AUTH_SUCCESS' }) });
+    await vi.advanceTimersByTimeAsync(0);
+    FakeWebSocket.instances[1].onclose?.({ code: 1009 });
+
+    // Delay must have doubled to 4s rather than starting over at 2s.
+    vi.advanceTimersByTime(3500);
+    expect(FakeWebSocket.instances).toHaveLength(2);
+    vi.advanceTimersByTime(600);
+    expect(FakeWebSocket.instances).toHaveLength(3);
+    expect(toast.error).toHaveBeenCalledTimes(1);
+  });
+
+  it('reconnects even if showing the toast throws', () => {
+    vi.mocked(toast.error).mockImplementationOnce(() => {
+      throw new Error('toast failed');
+    });
+    useWebSocket().connect();
+    FakeWebSocket.instances[0].onclose?.({ code: 1009 });
+    vi.advanceTimersByTime(2100);
+    expect(FakeWebSocket.instances).toHaveLength(2);
+  });
+
+  it('does not back off or notify on a normal close', () => {
+    useWebSocket().connect();
+    FakeWebSocket.instances[0].onclose?.({ code: 1000 });
+    expect(toast.error).not.toHaveBeenCalled();
+    vi.advanceTimersByTime(1100);
+    expect(FakeWebSocket.instances).toHaveLength(2);
+  });
+
+  it('notifies the user once and backs off between reconnects', () => {
+    useWebSocket().connect();
+    FakeWebSocket.instances[0].onclose?.({ code: 1009 });
+    expect(toast.error).toHaveBeenCalledTimes(1);
+
+    // First reconnect delay is 2s (1s * 2^1): nothing after 1.5s, a new socket after 2s.
+    vi.advanceTimersByTime(1500);
+    expect(FakeWebSocket.instances).toHaveLength(1);
+    vi.advanceTimersByTime(600);
+    expect(FakeWebSocket.instances).toHaveLength(2);
+
+    FakeWebSocket.instances[1].onclose?.({ code: 1009 });
+    expect(toast.error).toHaveBeenCalledTimes(1);
+    // Second delay doubles to 4s.
+    vi.advanceTimersByTime(3500);
+    expect(FakeWebSocket.instances).toHaveLength(2);
+    vi.advanceTimersByTime(600);
+    expect(FakeWebSocket.instances).toHaveLength(3);
+  });
+});

client-v3/src/composables/useWebSocket.ts

@@ -12,13 +12,24 @@ import type { WsMessage } from '@/types/api/websocket';
 
 const INITIAL_RECONNECT_DELAY_MS = 1000;
 const MAX_RECONNECT_DELAY_MS = 30000;
+/** RFC 6455 close code sent by the server when a message exceeds its size limit. */
+const WS_CLOSE_MESSAGE_TOO_BIG = 1009;
+const STABLE_CONNECTION_MS = 30000;
 
 let ws: WebSocket | null = null;
 let reconnectTimer: ReturnType<typeof setTimeout> | null = null;
 let errorCount = 0;
+// Consecutive 1009 closes. Kept apart from errorCount, which a successful authentication resets,
+// so a client that keeps tripping the limit still backs off. Cleared once a connection has stayed
+// up for STABLE_CONNECTION_MS.
+let tooBigCount = 0;
+let stableTimer: ReturnType<typeof setTimeout> | null = null;
 
 function getReconnectDelay(): number {
-  return Math.min(INITIAL_RECONNECT_DELAY_MS * 2 ** errorCount, MAX_RECONNECT_DELAY_MS);
+  return Math.min(
+    INITIAL_RECONNECT_DELAY_MS * 2 ** Math.max(errorCount, tooBigCount),
+    MAX_RECONNECT_DELAY_MS
+  );
 }
 
 /** Returns false (after logging) if the socket isn't open, so callers can react to a dropped frame. */
@@ -177,14 +188,18 @@ function connect(): void {
 
   ws.onopen = async () => {
     try {
-      const wasErrored = errorCount > 0;
+      const attempts = Math.max(errorCount, tooBigCount);
+      const wasErrored = attempts > 0;
       wsStore.$patch({ isConnected: true });
       if (wasErrored) {
-        toast.success(
-          `WebSocket reconnected after ${errorCount} attempt${errorCount > 1 ? 's' : ''}`
-        );
+        toast.success(`WebSocket reconnected after ${attempts} attempt${attempts > 1 ? 's' : ''}`);
       }
       log.info('WebSocket connected');
+      if (stableTimer) clearTimeout(stableTimer);
+      stableTimer = setTimeout(() => {
+        stableTimer = null;
+        tooBigCount = 0;
+      }, STABLE_CONNECTION_MS);
       if (wasErrored) {
         const showStore = useShowStore();
         if (showStore.currentSession != null) {
@@ -205,10 +220,30 @@ function connect(): void {
     }
   };
 
-  ws.onclose = () => {
+  ws.onclose = (event?: CloseEvent) => {
     wsStore.$patch({ isConnected: false, authenticated: false });
+    if (stableTimer) {
+      clearTimeout(stableTimer);
+      stableTimer = null;
+    }
+    const tooBig = event?.code === WS_CLOSE_MESSAGE_TOO_BIG;
+    if (tooBig) {
+      // The server dropped us for sending an over-sized message. Back off (and tell the user)
+      // rather than reconnecting at the minimum delay, which would loop if the oversized
+      // message is re-sent on every connect.
+      tooBigCount++;
+      log.error('WebSocket closed by server: message exceeded the maximum size');
+    }
     log.info('WebSocket closed, scheduling reconnect');
+    // Schedule before notifying so a failing toast can never stop the reconnect.
     scheduleReconnect();
+    if (tooBig && tooBigCount === 1) {
+      try {
+        toast.error('Server closed the connection: message too large');
+      } catch (e) {
+        log.error('Failed to show WebSocket close toast:', e);
+      }
+    }
   };
 
   ws.onerror = () => {

server/README.md

@@ -10,6 +10,8 @@ pip install -r requirements.txt
 python3 ./main.py
 ```
 
+Pass `--debug` for development (autoreload, relaxed WebSocket origin checks). It is off by default and must not be used on a deployed server.
+
 This requires that the client has been build first in order to server the static files
 
 ## Project Structure

server/controllers/api/v1/show/shows.py

@@ -244,6 +244,8 @@ async def patch(self):
                 self.set_status(404)
                 await self.finish({"message": ERROR_SHOW_NOT_FOUND})
 
+    @api_authenticated
+    @require_admin
     @requires_show
     @no_live_session
     async def delete(self):

server/controllers/ws_controller.py

@@ -32,6 +32,7 @@
 from rbac.role import Role
 from utils.web.base_controller import DatabaseMixin
 from utils.web.pending_disconnects import disconnect_key
+from utils.web.request_size_metrics import WSMessageSizeMetricsMixin
 from utils.web.route import ApiRoute, ApiVersion
 from utils.web.ws_session_lifecycle import (
     assign_session_user,
@@ -63,7 +64,7 @@
 
 
 @ApiRoute("ws", ApiVersion.V1)
-class WebSocketController(DatabaseMixin, WebSocketHandler):
+class WebSocketController(WSMessageSizeMetricsMixin, DatabaseMixin, WebSocketHandler):
     # Live-show ops only the leader may send, and the method that handles each.
     _LEADER_OP_HANDLERS = {
         "SCRIPT_SCROLL": "_op_script_scroll",

server/digi_server/app_server.py

@@ -44,6 +44,7 @@
 from utils.version_checker import VersionChecker
 from utils.web.jwt_service import JWTService
 from utils.web.pending_disconnects import PendingDisconnects
+from utils.web.request_size_metrics import observe_http_request
 from utils.web.route import Route
 
 
@@ -331,6 +332,8 @@ class AlembicVersion(self._db.Model):
         super().__init__(
             handlers=handlers,
             debug=debug,
+            # Never return stack traces to clients, even in debug mode.
+            serve_traceback=False,
             db=self._db,
             websocket_ping_interval=5,
             cookie_secret=self._get_cookie_secret(),
@@ -340,6 +343,8 @@ class AlembicVersion(self._db.Model):
     def log_request(self, handler):
         from tornado.log import access_log  # noqa: PLC0415
 
+        observe_http_request(handler)
+
         if handler.request.path in Route.ignored_logging_routes():
             return
 

server/main.py

@@ -38,7 +38,13 @@ def ensure_writable_db_path(path):
 add_logging_level("TRACE", logging.DEBUG - 5)
 get_logger().setLevel(logging.DEBUG)
 
-define("debug", type=bool, default=not IS_FROZEN, help="auto reload")
+define(
+    "debug",
+    type=bool,
+    default=False,
+    help="Development mode: autoreload, relaxed WebSocket origin checks. "
+    "Never enable on a deployed server.",
+)
 define("port", type=int, default=8080, help="port to listen on")
 define("settings_path", type=str, default=None, help="Path to settings JSON file")
 define("skip_migrations", type=bool, default=False, help="skip database migrations")

server/test/controllers/api/v1/show/test_shows.py

@@ -420,6 +420,40 @@ def test_delete_show_cleans_up_rbac_mapping_rows(self):
                 " the script",
             )
 
+    def test_delete_show_unauthenticated(self):
+        """An unauthenticated request cannot delete a show."""
+        show_b_id = self._create_show()
+        response = self.fetch(
+            f"/api/v1/show?id={show_b_id}",
+            method="DELETE",
+            allow_nonstandard_methods=True,
+        )
+        self.assertEqual(401, response.code)
+        with self._app.get_db().sessionmaker() as session:
+            self.assertIsNotNone(session.get(Show, show_b_id))
+
+    def test_delete_show_unauthenticated_nonexistent_show(self):
+        """Auth is checked before the show lookup, so existence is not leaked."""
+        response = self.fetch(
+            "/api/v1/show?id=99999", method="DELETE", allow_nonstandard_methods=True
+        )
+        self.assertEqual(401, response.code)
+
+    def test_delete_show_non_admin(self):
+        """A logged-in non-admin cannot delete a show."""
+        show_b_id = self._create_show()
+        with self._app.get_db().sessionmaker() as session:
+            user = User(username="regular", is_admin=False, password="test")
+            session.add(user)
+            session.flush()
+            user_id = user.id
+            session.commit()
+        token = self._app.jwt_service.create_access_token({"user_id": user_id})
+        response = self._delete(show_b_id, token=token)
+        self.assertEqual(401, response.code)
+        with self._app.get_db().sessionmaker() as session:
+            self.assertIsNotNone(session.get(Show, show_b_id))
+
     def test_delete_currently_loaded_show(self):
         """Attempting to delete the currently loaded show returns 400."""
         response = self._delete(self.show_a_id)