From 2b2d4a72ae5c6b63c80d43af9fb3f2d44b7b2f25 Mon Sep 17 00:00:00 2001 From: Tim Bradgate Date: Mon, 19 Oct 2026 21:19:39 +0100 Subject: [PATCH 01/11] Harden WebSocket message handling and stop logging secrets - Log only op name and size for WS messages (payloads carry JWTs) - Validate DATA for leader ops; ignore non-object frames and non-string ops - Guard update_session against a deleted user - Recursively redact sensitive keys from REST request body logs; do not log non-JSON bodies - Default log_level to INFO Co-Authored-By: Claude Sonnet 5.5 --- server/controllers/ws_controller.py | 58 ++++++++++++---- server/digi_server/settings.py | 2 +- server/test/controllers/test_ws_controller.py | 66 ++++++++++++++++++ server/test/utils/web/test_log_redaction.py | 69 +++++++++++++++++++ server/utils/web/base_controller.py | 6 +- server/utils/web/log_redaction.py | 46 +++++++++++++ 6 files changed, 233 insertions(+), 14 deletions(-) create mode 100644 server/test/utils/web/test_log_redaction.py create mode 100644 server/utils/web/log_redaction.py diff --git a/server/controllers/ws_controller.py b/server/controllers/ws_controller.py index 5953c27d..7fd2baaf 100644 --- a/server/controllers/ws_controller.py +++ b/server/controllers/ws_controller.py @@ -131,8 +131,12 @@ def update_session(self, user_id=None) -> bool: user_id=user_id, ) ) - if self.current_user_id: - user = session.get(User, self.current_user_id) + user = ( + session.get(User, self.current_user_id) + if self.current_user_id + else None + ) + if user is not None: user.last_seen = datetime.datetime.now(tz=datetime.timezone.utc) session.commit() return cleared @@ -466,17 +470,25 @@ async def on_message(self, message: Union[str, bytes]): if self.current_username else self.request.remote_ip ) - get_logger().debug(f"WebSocket message from {user_part}: {message}") + size = len(message) try: message = json.loads(message) ws_op = message["OP"] - except (json.JSONDecodeError, KeyError) as e: + if not isinstance(ws_op, str): + raise TypeError("OP must be a string") + except (json.JSONDecodeError, KeyError, TypeError) as e: get_logger().warning( - f"Malformed WS message from {self.request.remote_ip}: {e}" + f"Malformed WS message ({size} bytes) from " + f"{self.request.remote_ip}: {type(e).__name__}" ) return + # Never log the payload: AUTHENTICATE / REFRESH_TOKEN carry live JWTs. + get_logger().debug( + f"WebSocket message from {user_part}: {ws_op} ({size} bytes)" + ) + # Connection boundary: an exception escaping on_message ends Tornado's # read loop without calling on_close, leaving a handler that no longer # reads frames but still counts as a connected owner. Each op keeps its @@ -535,8 +547,12 @@ async def _handle_session_op(self, ws_op: str, message: dict) -> None: ) # Ownership rule: only the authenticated show user on its own row. if self._is_leader(session, show_session): + data = message.get("DATA") + if not isinstance(data, dict): + get_logger().warning(f"Ignored {ws_op}: DATA must be an object") + return handler = getattr(self, self._LEADER_OP_HANDLERS[ws_op]) - await handler(session, show_session, entry, message["DATA"]) + await handler(session, show_session, entry, data) else: get_logger().debug( f"Ignored {ws_op} from client " @@ -551,7 +567,7 @@ async def _handle_session_op(self, ws_op: str, message: dict) -> None: ) if show_session and self._user_has_show_execute_access(session, show): await self._op_jump_to_page( - session, show_session, entry, message["DATA"] + session, show_session, entry, message.get("DATA") ) else: get_logger().debug( @@ -637,19 +653,35 @@ def _resume_client(self, new_uuid: Any) -> None: async def _op_script_scroll(self, session, show_session, entry, data) -> None: """Leader op ``SCRIPT_SCROLL``: record and broadcast the leader's line.""" - show_session.latest_line_ref = data["current_line"] + line_ref = data.get("current_line") + if not isinstance(line_ref, str): + get_logger().warning("Ignored SCRIPT_SCROLL: invalid current_line") + return + show_session.latest_line_ref = line_ref session.commit() await self.application.ws_send_to_all("NOOP", "SCRIPT_SCROLL", data) async def _op_begin_interval(self, session, show_session, entry, data) -> None: """Leader op ``BEGIN_INTERVAL``: start an interval after the given act.""" - act: Act = session.get(Act, data["actId"]) - if not entry: + act_id = data.get("actId") + length = data.get("length") + if ( + not entry + or isinstance(act_id, bool) + or not isinstance(act_id, int) + or isinstance(length, bool) + or not isinstance(length, (int, float)) + ): + get_logger().warning("Ignored BEGIN_INTERVAL: invalid actId or length") + return + act: Optional[Act] = session.get(Act, act_id) + if act is None: + get_logger().warning("Ignored BEGIN_INTERVAL: unknown act") return show_interval = Interval( session_id=show_session.id, act_id=act.id, - initial_length=data["length"], + initial_length=length, ) session.add(show_interval) session.flush() @@ -825,7 +857,9 @@ async def _handle_script_room_op(self, ws_op: str, message: dict): get_logger().warning("RoomManager not available, ignoring collab OP") return - data = message.get("DATA", {}) + data = message.get("DATA") + if not isinstance(data, dict): + data = {} # Everything that touches the shared draft only exists in collaborative mode; # in classic mode a stray client must not be able to open a room. diff --git a/server/digi_server/settings.py b/server/digi_server/settings.py index ae5b73b3..37737242 100644 --- a/server/digi_server/settings.py +++ b/server/digi_server/settings.py @@ -265,7 +265,7 @@ def init_settings(self): self.define( "log_level", str, - "DEBUG", + "INFO", True, self._application.regen_logging, display_name="Log Level", diff --git a/server/test/controllers/test_ws_controller.py b/server/test/controllers/test_ws_controller.py index 08525d2f..296bb8d4 100644 --- a/server/test/controllers/test_ws_controller.py +++ b/server/test/controllers/test_ws_controller.py @@ -3474,6 +3474,72 @@ async def test_jump_to_page_without_page_does_not_kill_connection(self): await self._still_responsive(ws_l, self.admin_id) ws_l.close() + @gen_test + async def test_leader_ops_with_malformed_data_are_ignored(self): + """Non-object / wrong-typed DATA on leader ops is rejected without + changing state or killing the connection. + """ + ws_l, uuid_l = await self._connect_and_auth(self.admin_id) + self._start_live_session(uuid_l, self.admin_id) + + def latest(): + with self._app.get_db().sessionmaker() as session: + show = session.get(Show, self.show_id) + live = session.get(ShowSession, show.current_session_id) + return live.latest_line_ref + + before = latest() + for op in ("SCRIPT_SCROLL", "BEGIN_INTERVAL", "END_INTERVAL"): + for bad in (None, [], "x", 5): + await ws_l.write_message(json.dumps({"OP": op, "DATA": bad})) + await ws_l.write_message(json.dumps({"OP": op})) + await ws_l.write_message( + json.dumps({"OP": "SCRIPT_SCROLL", "DATA": {"current_line": ["x"]}}) + ) + await ws_l.write_message( + json.dumps({"OP": "BEGIN_INTERVAL", "DATA": {"actId": "a", "length": 1}}) + ) + await ws_l.write_message(json.dumps({"OP": "LIVE_SHOW_JUMP_TO_PAGE"})) + await self._still_responsive(ws_l, self.admin_id) + self.assertEqual(before, latest()) + ws_l.close() + + @gen_test + async def test_non_object_and_non_string_op_frames_are_ignored(self): + ws, _ = await self._connect_and_auth(self.admin_id) + for frame in ("[]", '"abc"', "5", "null", '{"OP": ["x"]}', '{"OP": {}}'): + await ws.write_message(frame) + await self._still_responsive(ws, self.admin_id) + ws.close() + + @gen_test + async def test_ws_payload_and_tokens_are_not_logged(self): + """AUTHENTICATE tokens never reach the log; only op name and size do.""" + ws, _ = await self._connect_and_auth() + sentinel = self._app.jwt_service.create_access_token( + data={"user_id": self.admin_id} + ) + with self.assertLogs(get_logger(), level="DEBUG") as captured: + await ws.write_message( + json.dumps({"OP": "AUTHENTICATE", "DATA": {"token": sentinel}}) + ) + await self._read_until(ws, op="WS_AUTH_SUCCESS") + joined = "\n".join(captured.output) + self.assertIn("AUTHENTICATE", joined) + self.assertNotIn(sentinel, joined) + ws.close() + + @gen_test + async def test_update_session_survives_deleted_user(self): + ws, uuid = await self._connect_and_auth(self.admin2_id) + handler = self._app.get_ws(uuid) + with self._app.get_db().sessionmaker() as session: + session.get(Session, uuid).user_id = None + session.delete(session.get(User, self.admin2_id)) + session.commit() + handler.update_session() # must not raise + ws.close() + @gen_test async def test_db_error_in_reconcile_leaves_connection_unauthenticated(self): """A failure while settling ownership must not leave the socket half diff --git a/server/test/utils/web/test_log_redaction.py b/server/test/utils/web/test_log_redaction.py new file mode 100644 index 00000000..1f25e8b5 --- /dev/null +++ b/server/test/utils/web/test_log_redaction.py @@ -0,0 +1,69 @@ +import json +import unittest + +from tornado.httpclient import HTTPRequest +from tornado.testing import gen_test + +from test.conftest import DigiScriptTestCase +from utils.web.log_redaction import REDACTED, redact_sensitive_keys + + +class TestRedactSensitiveKeys(unittest.TestCase): + def test_redacts_nested_and_case_insensitive(self): + data = { + "username": "bob", + "Password": "p1", + "nested": {"new_password": "p2", "list": [{"api_token": "t"}, 3]}, + "token": "abc", + } + result = redact_sensitive_keys(data) + self.assertEqual("bob", result["username"]) + self.assertEqual(REDACTED, result["Password"]) + self.assertEqual(REDACTED, result["nested"]["new_password"]) + self.assertEqual(REDACTED, result["nested"]["list"][0]["api_token"]) + self.assertEqual(3, result["nested"]["list"][1]) + self.assertEqual(REDACTED, result["token"]) + + def test_does_not_mutate_input(self): + data = {"password": "p"} + redact_sensitive_keys(data) + self.assertEqual({"password": "p"}, data) + + def test_scalars_pass_through(self): + self.assertEqual("x", redact_sensitive_keys("x")) + self.assertIsNone(redact_sensitive_keys(None)) + + +class TestRestBodyLogging(DigiScriptTestCase): + @gen_test + async def test_sentinel_secrets_never_logged(self): + body = { + "username": "nobody", + "password": "SENTINEL-PASSWORD", + "extra": {"access_token": "SENTINEL-TOKEN"}, + } + request = HTTPRequest( + self.get_url("/api/v1/auth/login"), + method="POST", + body=json.dumps(body), + ) + with self.assertLogs(level="DEBUG") as captured: + await self.http_client.fetch(request, raise_error=False) + joined = "\n".join(captured.output) + self.assertIn("/api/v1/auth/login", joined) + self.assertNotIn("SENTINEL-PASSWORD", joined) + self.assertNotIn("SENTINEL-TOKEN", joined) + + @gen_test + async def test_non_json_body_not_logged(self): + request = HTTPRequest( + self.get_url("/api/v1/auth/login"), + method="POST", + body="password=SENTINEL-RAW", + ) + with self.assertLogs(level="DEBUG") as captured: + await self.http_client.fetch(request, raise_error=False) + self.assertNotIn("SENTINEL-RAW", "\n".join(captured.output)) + + def test_default_log_level_is_info(self): + self.assertEqual("INFO", self._app.digi_settings.settings["log_level"].default) diff --git a/server/utils/web/base_controller.py b/server/utils/web/base_controller.py index 0794f217..e1a42f52 100644 --- a/server/utils/web/base_controller.py +++ b/server/utils/web/base_controller.py @@ -15,6 +15,7 @@ from models.user import User from rbac.role import Role from schemas.schemas import ShowSchema, UserSchema +from utils.web.log_redaction import redact_sensitive_keys if TYPE_CHECKING: @@ -182,8 +183,10 @@ def on_finish(self): try: body = escape.json_decode(self.request.body) except BaseException: + # Not JSON, so it cannot be redacted by key: log only its size. get_logger().debug( - f"{self.request.method} {self.request.path} {self.request.body}{user_suffix}", + f"{self.request.method} {self.request.path} " + f"{user_suffix}", extra=self._log_extra(), ) else: @@ -195,6 +198,7 @@ def on_finish(self): ): body = deepcopy(body) redacted_data_paths.apply(body) + body = redact_sensitive_keys(body) log_method( f"{self.request.method} {self.request.path} {body}{user_suffix}", diff --git a/server/utils/web/log_redaction.py b/server/utils/web/log_redaction.py new file mode 100644 index 00000000..01733751 --- /dev/null +++ b/server/utils/web/log_redaction.py @@ -0,0 +1,46 @@ +"""Key-based redaction of secrets from structured data before it is logged.""" + +from __future__ import annotations + +from typing import Any + + +REDACTED = "<-- REDACTED -->" + +# Lower-cased dict keys whose values must never reach a log file. +SENSITIVE_KEYS = frozenset( + { + "password", + "new_password", + "old_password", + "current_password", + "token", + "access_token", + "refresh_token", + "api_token", + "secret", + } +) + + +def redact_sensitive_keys(value: Any) -> Any: + """Return a copy of *value* with the values of sensitive keys replaced. + + Recurses through dicts and lists. Key matching is case-insensitive. The input + is not modified. + + :param value: Decoded JSON data (dict, list or scalar). + :returns: A redacted copy. + """ + if isinstance(value, dict): + return { + key: ( + REDACTED + if isinstance(key, str) and key.lower() in SENSITIVE_KEYS + else redact_sensitive_keys(item) + ) + for key, item in value.items() + } + if isinstance(value, list): + return [redact_sensitive_keys(item) for item in value] + return value From 357b3769d1f60cbb95de2b6d7092d7eebe95550e Mon Sep 17 00:00:00 2001 From: Tim Bradgate Date: Mon, 19 Oct 2026 21:21:07 +0100 Subject: [PATCH 02/11] Security hygiene: JSON body helper, 403 for non-admin, RBAC validation, settings allow-list, security headers Co-Authored-By: Claude Sonnet 5.5 --- server/controllers/api/v1/auth/user.py | 14 +- server/controllers/api/v1/rbac.py | 104 ++++++------ server/controllers/api/v1/settings.py | 4 +- .../api/v1/show/session/sessions.py | 4 +- server/controllers/api/v1/user/settings.py | 36 +++-- server/controllers/api/v2/users/users.py | 6 +- .../test/controllers/api/v1/auth/test_auth.py | 2 +- .../controllers/api/v1/test_db_backups.py | 8 +- .../controllers/api/v1/test_logs_viewer.py | 8 +- .../api/v1/test_security_hygiene.py | 151 ++++++++++++++++++ .../controllers/api/v1/test_system_info.py | 4 +- .../test/controllers/api/v1/test_websocket.py | 2 +- server/test/controllers/api/v2/test_users.py | 6 +- server/utils/web/base_controller.py | 10 ++ server/utils/web/json_body.py | 27 ++++ server/utils/web/web_decorators.py | 6 +- 16 files changed, 301 insertions(+), 91 deletions(-) create mode 100644 server/test/controllers/api/v1/test_security_hygiene.py create mode 100644 server/utils/web/json_body.py diff --git a/server/controllers/api/v1/auth/user.py b/server/controllers/api/v1/auth/user.py index fe6999af..bf7d6451 100644 --- a/server/controllers/api/v1/auth/user.py +++ b/server/controllers/api/v1/auth/user.py @@ -1,7 +1,6 @@ from datetime import datetime, timezone from sqlalchemy import select -from tornado import escape from digi_server.logger import get_logger from models.session import Session @@ -10,6 +9,7 @@ from schemas.schemas import UserSchema from services.password_service import PasswordService from utils.web.base_controller import BaseAPIController +from utils.web.json_body import get_json_object from utils.web.route import ApiRoute, ApiVersion from utils.web.web_decorators import ( allow_when_password_required, @@ -37,7 +37,7 @@ async def post(self): if has_any_users: self.requires_admin() - data = escape.json_decode(self.request.body) + data = get_json_object(self) username = data.get("username", "") if not username: @@ -99,7 +99,7 @@ class UserDeleteController(BaseAPIController): @require_admin @no_live_session async def post(self): - data = escape.json_decode(self.request.body) + data = get_json_object(self) user_to_delete = data.get("id", None) if not user_to_delete: self.set_status(400) @@ -152,7 +152,7 @@ async def post(self): class LoginHandler(BaseAPIController): @redact_data_paths(paths=["/password"]) async def post(self): - data = escape.json_decode(self.request.body) + data = get_json_object(self) username = data.get("username", "") if not username: @@ -261,7 +261,7 @@ def _log_out_client(self, session_id: str) -> None: @api_authenticated @allow_when_password_required async def post(self): - data = escape.json_decode(self.request.body) + data = get_json_object(self) if self.current_user: # Revoke the JWT first, so a failure below can never leave the token @@ -335,7 +335,7 @@ async def patch(self): :return: 200 on success, 401 if old password wrong, 400 if new password invalid """ - data = escape.json_decode(self.request.body) + data = get_json_object(self) old_password = data.get("old_password", "") new_password = data.get("new_password", "") @@ -417,7 +417,7 @@ async def post(self): :return: 200 with temporary password on success, 400 if validation fails """ - data = escape.json_decode(self.request.body) + data = get_json_object(self) user_id = data.get("user_id") if not user_id: diff --git a/server/controllers/api/v1/rbac.py b/server/controllers/api/v1/rbac.py index a36df59b..19c8f683 100644 --- a/server/controllers/api/v1/rbac.py +++ b/server/controllers/api/v1/rbac.py @@ -1,12 +1,13 @@ from collections import defaultdict from sqlalchemy import inspect -from tornado import escape +from tornado.web import HTTPError from models.user import User from rbac.role import Role from registry.schema import get_registry from utils.web.base_controller import BaseAPIController +from utils.web.json_body import get_json_object from utils.web.route import ApiRoute, ApiVersion from utils.web.web_decorators import ( allow_when_password_required, @@ -15,6 +16,43 @@ ) +def _parse_role_change(data: dict) -> tuple[str, dict, int, Role]: + """Validate the body of a role grant/revoke request. + + :param data: The decoded JSON request body. + :returns: ``(resource, object, user_id, role)`` with types normalised. + :raises HTTPError: With status 400 if a field is missing or has the wrong type, + or if ``role`` is not a valid role value. + """ + resource = data.get("resource", None) + rbac_object = data.get("object", None) + user = data.get("user", None) + role = data.get("role", None) + + for name, value in ( + ("resource", resource), + ("object", rbac_object), + ("user", user), + ("role", role), + ): + if not value: + raise HTTPError(400, reason=f"{name} body parameter not fulfilled") + + if not isinstance(resource, str): + raise HTTPError(400, reason="resource must be a string") + if not isinstance(rbac_object, dict): + raise HTTPError(400, reason="object must be an object") + try: + user_id = int(user) + except (TypeError, ValueError): + raise HTTPError(400, reason="user must be an integer") from None + try: + role_value = Role(role) + except (TypeError, ValueError): + raise HTTPError(400, reason="invalid role") from None + return resource, rbac_object, user_id, role_value + + @ApiRoute("rbac/roles", ApiVersion.V1) class RBACRolesHandler(BaseAPIController): @allow_when_password_required @@ -125,28 +163,8 @@ class RBACRolesGrantHandler(BaseAPIController): @api_authenticated @require_admin async def post(self): - data = escape.json_decode(self.request.body) - resource = data.get("resource", None) - rbac_object = data.get("object", None) - user = data.get("user", None) - role = data.get("role", None) - - if not resource: - self.set_status(400) - await self.finish({"message": "resource body parameter not fulfilled"}) - return - if not rbac_object: - self.set_status(400) - await self.finish({"message": "object body parameter not fulfilled"}) - return - if not user: - self.set_status(400) - await self.finish({"message": "user body parameter not fulfilled"}) - return - if not role: - self.set_status(400) - await self.finish({"message": "role body parameter not fulfilled"}) - return + data = get_json_object(self) + resource, rbac_object, user, role = _parse_role_change(data) mapper = self.application.get_db().get_mapper_for_table(resource) if not mapper: @@ -155,7 +173,7 @@ async def post(self): return with self.make_session() as session: - user = session.get(User, int(user)) + user = session.get(User, user) if not user: self.set_status(404) await self.finish({"message": "user not found"}) @@ -165,12 +183,16 @@ async def post(self): cols.update( {col.key: rbac_object.get(col.key) for col in resource.primary_key} ) + if any(v is None for v in cols.values()): + self.set_status(400) + await self.finish({"message": "object is missing primary key fields"}) + return rbac_object = session.get(mapper, cols) if not rbac_object: self.set_status(404) await self.finish({"message": "object not found"}) return - self.application.rbac.give_role(user, rbac_object, Role(role)) + self.application.rbac.give_role(user, rbac_object, role) for socket in self.application.get_all_ws(user.id): await socket.write_message( {"OP": "NOOP", "DATA": {}, "ACTION": "GET_CURRENT_RBAC"} @@ -182,28 +204,8 @@ class RBACRolesRevokeHandler(BaseAPIController): @api_authenticated @require_admin async def post(self): - data = escape.json_decode(self.request.body) - resource = data.get("resource", None) - rbac_object = data.get("object", None) - user = data.get("user", None) - role = data.get("role", None) - - if not resource: - self.set_status(400) - await self.finish({"message": "resource body parameter not fulfilled"}) - return - if not rbac_object: - self.set_status(400) - await self.finish({"message": "object body parameter not fulfilled"}) - return - if not user: - self.set_status(400) - await self.finish({"message": "user body parameter not fulfilled"}) - return - if not role: - self.set_status(400) - await self.finish({"message": "role body parameter not fulfilled"}) - return + data = get_json_object(self) + resource, rbac_object, user, role = _parse_role_change(data) mapper = self.application.get_db().get_mapper_for_table(resource) if not mapper: @@ -212,7 +214,7 @@ async def post(self): return with self.make_session() as session: - user = session.get(User, int(user)) + user = session.get(User, user) if not user: self.set_status(404) await self.finish({"message": "user not found"}) @@ -222,12 +224,16 @@ async def post(self): cols.update( {col.key: rbac_object.get(col.key) for col in resource.primary_key} ) + if any(v is None for v in cols.values()): + self.set_status(400) + await self.finish({"message": "object is missing primary key fields"}) + return rbac_object = session.get(mapper, cols) if not rbac_object: self.set_status(404) await self.finish({"message": "object not found"}) return - self.application.rbac.revoke_role(user, rbac_object, Role(role)) + self.application.rbac.revoke_role(user, rbac_object, role) for socket in self.application.get_all_ws(user.id): await socket.write_message( {"OP": "NOOP", "DATA": {}, "ACTION": "GET_CURRENT_RBAC"} diff --git a/server/controllers/api/v1/settings.py b/server/controllers/api/v1/settings.py index 94a8f984..56fe472e 100644 --- a/server/controllers/api/v1/settings.py +++ b/server/controllers/api/v1/settings.py @@ -1,11 +1,11 @@ from sqlalchemy import select -from tornado import escape from controllers.api.constants import ERROR_COLLAB_MODE_CHANGE_BLOCKED from digi_server.logger import get_logger from digi_server.settings import COLLAB_EDITING_SETTING, Settings from models.session import Session from utils.web.base_controller import BaseAPIController +from utils.web.json_body import get_json_object from utils.web.route import ApiRoute, ApiVersion from utils.web.web_decorators import ( allow_when_password_required, @@ -60,7 +60,7 @@ async def get(self): async def patch(self): settings: Settings = self.application.digi_settings - data = escape.json_decode(self.request.body) + data = get_json_object(self) get_logger().debug(f"New settings data patched: {data}") if COLLAB_EDITING_SETTING in data and not isinstance( diff --git a/server/controllers/api/v1/show/session/sessions.py b/server/controllers/api/v1/show/session/sessions.py index c84e6cc9..63fac7b7 100644 --- a/server/controllers/api/v1/show/session/sessions.py +++ b/server/controllers/api/v1/show/session/sessions.py @@ -2,7 +2,6 @@ from typing import List from sqlalchemy import select -from tornado import escape from controllers.api.constants import ERROR_SHOW_NOT_FOUND from models.script import Script @@ -11,6 +10,7 @@ from rbac.role import Role from schemas.schemas import IntervalSchema, ShowSessionSchema from utils.web.base_controller import BaseAPIController +from utils.web.json_body import get_json_object from utils.web.route import ApiRoute, ApiVersion from utils.web.web_decorators import requires_show @@ -100,7 +100,7 @@ async def post(self): await self.finish({"message": ERROR_SHOW_NOT_FOUND}) return self.requires_role(show, Role.EXECUTE) - data = escape.json_decode(self.request.body) + data = get_json_object(self) status, result = self._start_problem(session, show, data) if status is not None: self.set_status(status) diff --git a/server/controllers/api/v1/user/settings.py b/server/controllers/api/v1/user/settings.py index dbf3fdd5..3cddd327 100644 --- a/server/controllers/api/v1/user/settings.py +++ b/server/controllers/api/v1/user/settings.py @@ -1,13 +1,32 @@ -from tornado import escape - from digi_server.logger import get_logger from models.user import UserSettings from schemas.schemas import UserSettingsSchema from utils.web.base_controller import BaseAPIController +from utils.web.json_body import get_json_object from utils.web.route import ApiRoute, ApiVersion from utils.web.web_decorators import allow_when_password_required, api_authenticated +# Settings a user may change themselves. Anything else (including internal +# ``_``-prefixed columns and non-column attributes) is ignored. +EDITABLE_USER_SETTINGS = frozenset( + { + "enable_script_auto_save", + "script_auto_save_interval", + "cue_position_right", + "script_text_alignment", + "console_log_level", + "character_mru_sort", + "character_combined_dropdown", + "preferred_ui", + "table_page_sizes", + "default_sd_text_colour", + "default_sd_background_colour", + "show_current_cue_footer", + } +) + + @ApiRoute("user/settings", ApiVersion.V1) class UserSettingsController(BaseAPIController): @api_authenticated @@ -28,7 +47,7 @@ async def get(self): @api_authenticated async def patch(self): log = get_logger() - data = escape.json_decode(self.request.body) + data = get_json_object(self) with self.make_session() as session: user_settings = session.get(UserSettings, self.current_user["id"]) if not user_settings: @@ -38,16 +57,11 @@ async def patch(self): session.flush() for k, v in data.items(): - if hasattr(user_settings, k): - if k[0] == "_": - log.warning( - f"User attempted to set protected setting: {k}. Ignoring." - ) - else: - setattr(user_settings, k, v) + if k in EDITABLE_USER_SETTINGS: + setattr(user_settings, k, v) else: log.warning( - f"User attempted to set unknown setting: {k}. Ignoring." + f"User attempted to set non-editable setting: {k}. Ignoring." ) session.commit() diff --git a/server/controllers/api/v2/users/users.py b/server/controllers/api/v2/users/users.py index 71860c35..a583aeb2 100644 --- a/server/controllers/api/v2/users/users.py +++ b/server/controllers/api/v2/users/users.py @@ -1,13 +1,13 @@ from datetime import datetime, timezone from sqlalchemy import select -from tornado import escape from models.user import User from registry.named_locks import NamedLockRegistry from schemas.schemas import UserSchema from services.password_service import PasswordService from utils.web.base_controller import BaseAPIController +from utils.web.json_body import get_json_object from utils.web.route import ApiRoute, ApiVersion from utils.web.web_decorators import ( api_authenticated, @@ -37,7 +37,7 @@ async def post(self): if has_any_users: self.requires_admin() - data = escape.json_decode(self.request.body) + data = get_json_object(self) username = data.get("username", "") if not username: @@ -100,7 +100,7 @@ async def patch(self): await self.finish({"message": "Id missing"}) return - data = escape.json_decode(self.request.body) + data = get_json_object(self) with self.make_session() as session: user: User = session.get(User, int(user_id)) diff --git a/server/test/controllers/api/v1/auth/test_auth.py b/server/test/controllers/api/v1/auth/test_auth.py index af589061..46bd5a7a 100644 --- a/server/test/controllers/api/v1/auth/test_auth.py +++ b/server/test/controllers/api/v1/auth/test_auth.py @@ -1186,7 +1186,7 @@ def test_admin_reset_password_requires_admin(self): headers={"Authorization": f"Bearer {user_token}"}, ) - self.assertEqual(401, response.code) + self.assertEqual(403, response.code) def test_admin_reset_password_user_not_found(self): """Test password reset fails for non-existent user""" diff --git a/server/test/controllers/api/v1/test_db_backups.py b/server/test/controllers/api/v1/test_db_backups.py index 4aa53fec..9827da3d 100644 --- a/server/test/controllers/api/v1/test_db_backups.py +++ b/server/test/controllers/api/v1/test_db_backups.py @@ -61,11 +61,11 @@ def test_get_unauthenticated_returns_401(self): resp = self._fetch_backups() self.assertEqual(401, resp.code) - def test_get_non_admin_returns_401(self): + def test_get_non_admin_returns_403(self): admin_token = self._create_and_login_admin() user_token = self._create_and_login_user(admin_token) resp = self._fetch_backups(token=user_token) - self.assertEqual(401, resp.code) + self.assertEqual(403, resp.code) # ------------------------------------------------------------------ # GET — response shape @@ -129,11 +129,11 @@ def test_delete_unauthenticated_returns_401(self): resp = self._delete_backup(1700000000) self.assertEqual(401, resp.code) - def test_delete_non_admin_returns_401(self): + def test_delete_non_admin_returns_403(self): admin_token = self._create_and_login_admin() user_token = self._create_and_login_user(admin_token) resp = self._delete_backup(1700000000, token=user_token) - self.assertEqual(401, resp.code) + self.assertEqual(403, resp.code) # ------------------------------------------------------------------ # DELETE — validation diff --git a/server/test/controllers/api/v1/test_logs_viewer.py b/server/test/controllers/api/v1/test_logs_viewer.py index 974aa6f8..c721a8d0 100644 --- a/server/test/controllers/api/v1/test_logs_viewer.py +++ b/server/test/controllers/api/v1/test_logs_viewer.py @@ -83,11 +83,11 @@ def test_unauthenticated_returns_401(self): resp = self._fetch_view() self.assertEqual(401, resp.code) - def test_non_admin_returns_401(self): + def test_non_admin_returns_403(self): admin_token = self._create_and_login_admin() user_token = self._create_and_login_user(admin_token) resp = self._fetch_view(token=user_token) - self.assertEqual(401, resp.code) + self.assertEqual(403, resp.code) def test_admin_returns_200(self): token = self._create_and_login_admin() @@ -367,11 +367,11 @@ def test_stream_unauthenticated_returns_401(self): resp = self._fetch_stream_headers() self.assertEqual(401, resp.code) - def test_stream_non_admin_returns_401(self): + def test_stream_non_admin_returns_403(self): admin_token = self._create_and_login_admin() user_token = self._create_and_login_user(admin_token) resp = self._fetch_stream_headers(token=user_token) - self.assertEqual(401, resp.code) + self.assertEqual(403, resp.code) def test_stream_backfill_contains_existing_entries(self): """Entries injected before connection must appear in the first chunk. diff --git a/server/test/controllers/api/v1/test_security_hygiene.py b/server/test/controllers/api/v1/test_security_hygiene.py new file mode 100644 index 00000000..f4299e4c --- /dev/null +++ b/server/test/controllers/api/v1/test_security_hygiene.py @@ -0,0 +1,151 @@ +"""Tests for the security hygiene hardening: JSON body parsing, admin status codes, +RBAC input validation, user settings allow-list and default security headers.""" + +from tornado import escape + +from models.user import UserSettings +from test.conftest import DigiScriptTestCase + + +class TestSecurityHygiene(DigiScriptTestCase): + def _auth(self, token): + return {"Authorization": f"Bearer {token}"} + + def _post(self, path, body, token=None, method="POST"): + return self.fetch( + path, + method=method, + body=body if isinstance(body, str) else escape.json_encode(body), + headers=self._auth(token) if token else {}, + raise_error=False, + ) + + # ─── Security headers ────────────────────────────────────────────────────── + + def test_security_headers_present(self): + resp = self.fetch("/api/v1/health", raise_error=False) + self.assertEqual("nosniff", resp.headers.get("X-Content-Type-Options")) + self.assertEqual("SAMEORIGIN", resp.headers.get("X-Frame-Options")) + self.assertEqual("same-origin", resp.headers.get("Referrer-Policy")) + + def test_security_headers_present_on_error_responses(self): + resp = self.fetch("/api/v1/rbac/user/resources", raise_error=False) + self.assertEqual(401, resp.code) + self.assertEqual("nosniff", resp.headers.get("X-Content-Type-Options")) + + # ─── JSON body helper ────────────────────────────────────────────────────── + + def test_login_invalid_json_returns_400(self): + self.assertEqual(400, self._post("/api/v1/auth/login", "{not json").code) + + def test_login_non_object_json_returns_400(self): + self.assertEqual(400, self._post("/api/v1/auth/login", "[1, 2]").code) + + def test_login_empty_body_returns_400(self): + self.assertEqual(400, self._post("/api/v1/auth/login", "").code) + + def test_create_user_invalid_json_returns_400(self): + self.assertEqual(400, self._post("/api/v1/auth/create", "nope").code) + + def test_v2_create_user_invalid_json_returns_400(self): + self.assertEqual(400, self._post("/api/v2/users", "nope").code) + + def test_change_password_invalid_json_returns_400(self): + token = self._create_and_login_admin() + resp = self._post("/api/v1/auth/change-password", "nope", token, "PATCH") + self.assertEqual(400, resp.code) + + def test_rbac_grant_invalid_json_returns_400(self): + token = self._create_and_login_admin() + self.assertEqual( + 400, self._post("/api/v1/rbac/user/roles/grant", "nope", token).code + ) + + def test_user_settings_patch_non_object_returns_400(self): + token = self._create_and_login_admin() + resp = self._post("/api/v1/user/settings", "[1]", token, "PATCH") + self.assertEqual(400, resp.code) + + def test_system_settings_patch_invalid_json_returns_400(self): + token = self._create_and_login_admin() + resp = self._post("/api/v1/settings", "nope", token, "PATCH") + self.assertEqual(400, resp.code) + + def test_session_start_invalid_json_returns_400(self): + token = self._create_and_login_admin() + resp = self._post("/api/v1/show/sessions/start", "nope", token) + # No show is loaded in this fixture, so requires_show answers first. + self.assertEqual(400, resp.code) + + # ─── require_admin status codes ──────────────────────────────────────────── + + def test_require_admin_unauthenticated_is_401(self): + resp = self._post("/api/v1/rbac/user/roles/grant", {}) + self.assertEqual(401, resp.code) + + def test_require_admin_non_admin_is_403(self): + admin_token = self._create_and_login_admin() + user_token = self._create_and_login_user(admin_token) + resp = self._post("/api/v1/rbac/user/roles/grant", {}, user_token) + self.assertEqual(403, resp.code) + + # ─── RBAC grant / revoke validation ──────────────────────────────────────── + + def _role_body(self, **overrides): + body = { + "resource": "shows", + "object": {"id": 1}, + "user": 1, + "role": 1, + } + body.update(overrides) + return body + + def test_rbac_grant_and_revoke_invalid_input_returns_400(self): + token = self._create_and_login_admin() + bad_bodies = [ + self._role_body(role=999999), + self._role_body(role="admin"), + self._role_body(user="abc"), + self._role_body(object="not-a-dict"), + self._role_body(object={"unrelated": 1}), + self._role_body(resource=["shows"]), + ] + for path in ( + "/api/v1/rbac/user/roles/grant", + "/api/v1/rbac/user/roles/revoke", + ): + for body in bad_bodies: + with self.subTest(path=path, body=body): + resp = self._post(path, body, token) + self.assertEqual(400, resp.code) + + def test_rbac_grant_missing_field_returns_400(self): + token = self._create_and_login_admin() + body = self._role_body() + del body["role"] + resp = self._post("/api/v1/rbac/user/roles/grant", body, token) + self.assertEqual(400, resp.code) + + # ─── User settings allow-list ────────────────────────────────────────────── + + def test_user_settings_patch_sets_allowed_and_ignores_others(self): + token = self._create_and_login_admin() + resp = self._post( + "/api/v1/user/settings", + { + "cue_position_right": True, + "_user_id": 999, + "metadata": "x", + "registry": "x", + "nonexistent": 1, + }, + token, + "PATCH", + ) + self.assertEqual(200, resp.code) + + with self._app.get_db().sessionmaker() as session: + settings = session.scalars(session.query(UserSettings).statement).first() + self.assertTrue(settings.cue_position_right) + self.assertNotEqual(999, settings._user_id) diff --git a/server/test/controllers/api/v1/test_system_info.py b/server/test/controllers/api/v1/test_system_info.py index 3c89428d..2585140a 100644 --- a/server/test/controllers/api/v1/test_system_info.py +++ b/server/test/controllers/api/v1/test_system_info.py @@ -16,11 +16,11 @@ def test_unauthenticated_returns_401(self): resp = self._fetch_system_info() self.assertEqual(401, resp.code) - def test_non_admin_returns_401(self): + def test_non_admin_returns_403(self): admin_token = self._create_and_login_admin() user_token = self._create_and_login_user(admin_token) resp = self._fetch_system_info(token=user_token) - self.assertEqual(401, resp.code) + self.assertEqual(403, resp.code) def test_authenticated_returns_200_with_expected_keys(self): token = self._create_and_login_admin() diff --git a/server/test/controllers/api/v1/test_websocket.py b/server/test/controllers/api/v1/test_websocket.py index aa49990b..59ea545e 100644 --- a/server/test/controllers/api/v1/test_websocket.py +++ b/server/test/controllers/api/v1/test_websocket.py @@ -22,7 +22,7 @@ def test_get_sessions_rejects_non_admin(self): response = self.fetch( "/api/v1/ws/sessions", headers={"Authorization": f"Bearer {user_token}"} ) - self.assertEqual(401, response.code) + self.assertEqual(403, response.code) def test_get_sessions_empty(self): """Test GET /api/v1/ws/sessions with no sessions. diff --git a/server/test/controllers/api/v2/test_users.py b/server/test/controllers/api/v2/test_users.py index decdbb72..8d3b0a9a 100644 --- a/server/test/controllers/api/v2/test_users.py +++ b/server/test/controllers/api/v2/test_users.py @@ -74,7 +74,7 @@ def test_get_users_requires_admin(self): method="GET", headers={"Authorization": f"Bearer {nonadmin_token}"}, ) - self.assertEqual(401, resp.code) + self.assertEqual(403, resp.code) # ─── POST /api/v2/users ──────────────────────────────────────────────────── @@ -183,7 +183,7 @@ def test_patch_requires_admin(self): body=escape.json_encode({"is_admin": True}), headers={"Authorization": f"Bearer {user_token}"}, ) - self.assertEqual(401, resp.code) + self.assertEqual(403, resp.code) def test_patch_missing_id(self): admin_token = self._setup_admin() @@ -265,7 +265,7 @@ def test_delete_requires_admin(self): allow_nonstandard_methods=True, headers={"Authorization": f"Bearer {user_token}"}, ) - self.assertEqual(401, resp.code) + self.assertEqual(403, resp.code) def test_delete_missing_id(self): admin_token = self._setup_admin() diff --git a/server/utils/web/base_controller.py b/server/utils/web/base_controller.py index 0794f217..e1fe4457 100644 --- a/server/utils/web/base_controller.py +++ b/server/utils/web/base_controller.py @@ -45,6 +45,16 @@ def __init__( self.application: DigiScriptServer = self.application self.current_show: Optional[dict] = None + def set_default_headers(self) -> None: + """Add baseline security headers to every response. + + No Content-Security-Policy is set: the SPA and the bundled docs rely on + inline styles/scripts, so a policy strict enough to be useful would break them. + """ + self.set_header("X-Content-Type-Options", "nosniff") + self.set_header("X-Frame-Options", "SAMEORIGIN") + self.set_header("Referrer-Policy", "same-origin") + async def prepare( self, ) -> Optional[Awaitable[None]]: diff --git a/server/utils/web/json_body.py b/server/utils/web/json_body.py new file mode 100644 index 00000000..03cd2a59 --- /dev/null +++ b/server/utils/web/json_body.py @@ -0,0 +1,27 @@ +from __future__ import annotations + +from typing import TYPE_CHECKING, Any + +from tornado import escape +from tornado.web import HTTPError + + +if TYPE_CHECKING: + from tornado.web import RequestHandler + + +def get_json_object(handler: RequestHandler) -> dict[str, Any]: + """Decode the request body as a JSON object. + + :param handler: The request handler whose body should be decoded. + :returns: The decoded JSON object. + :raises HTTPError: With status 400 if the body is not valid JSON, or the + decoded value is not a JSON object (dict). + """ + try: + data = escape.json_decode(handler.request.body) + except (ValueError, TypeError) as exc: + raise HTTPError(400, log_message="Invalid JSON body") from exc + if not isinstance(data, dict): + raise HTTPError(400, log_message="JSON body must be an object") + return data diff --git a/server/utils/web/web_decorators.py b/server/utils/web/web_decorators.py index 295ce37b..db2f41e3 100644 --- a/server/utils/web/web_decorators.py +++ b/server/utils/web/web_decorators.py @@ -33,8 +33,10 @@ def require_admin( ) -> Callable[..., Optional[Awaitable[None]]]: @functools.wraps(method) def wrapper(self: BaseController, *args, **kwargs): - if not self.current_user or not self.current_user["is_admin"]: - raise HTTPError(401, log_message="Not admin user") + if not self.current_user: + raise HTTPError(401, log_message="Not logged in") + if not self.current_user["is_admin"]: + raise HTTPError(403, log_message="Not admin user") return method(self, *args, **kwargs) return wrapper From 10c39b195170657eae6b98493ca8c5875ae83f8e Mon Sep 17 00:00:00 2001 From: Tim Bradgate Date: Mon, 19 Oct 2026 23:19:21 +0100 Subject: [PATCH 03/11] Make log redaction setting authoritative and default it on - WS and REST body logging redact only when log_redaction is enabled; when disabled raw content is logged for debugging - log_redaction now defaults to True (new installs and installs without a saved value) - Document the setting; move logging tests next to the code they cover Co-Authored-By: Claude Sonnet 5.5 --- docs/pages/user_config.md | 6 +++ server/controllers/ws_controller.py | 13 +++-- server/digi_server/settings.py | 8 ++- server/test/controllers/test_ws_controller.py | 19 +++++-- server/test/digi_server/test_settings.py | 11 ++++ server/test/utils/web/test_base_controller.py | 54 +++++++++++++++++++ server/test/utils/web/test_log_redaction.py | 40 -------------- server/utils/web/base_controller.py | 21 ++++---- 8 files changed, 110 insertions(+), 62 deletions(-) create mode 100644 server/test/digi_server/test_settings.py create mode 100644 server/test/utils/web/test_base_controller.py diff --git a/docs/pages/user_config.md b/docs/pages/user_config.md index 3275ba11..4fe3292c 100644 --- a/docs/pages/user_config.md +++ b/docs/pages/user_config.md @@ -76,6 +76,12 @@ Once users have been created, their permissions can be configured by clicking th RBAC configuration determines what shows a user can access and what actions they can perform within those shows. +### Log Redaction + +The **Enable Log Redaction** system setting is on by default. While it is on, passwords, tokens and other credentials are replaced with a placeholder in logged HTTP request bodies, and WebSocket messages are logged as their operation name and size only. + +Turn it off to see the raw HTTP and WebSocket content while debugging. **Warning:** with redaction off, the logs contain credentials and authentication tokens, so only disable it temporarily and treat the log files as sensitive. Installations that already had the setting saved keep their existing value. + ### Log Viewer The **Logs** tab (admin only) provides a real-time view of server and client log entries stored in the in-memory log buffer. diff --git a/server/controllers/ws_controller.py b/server/controllers/ws_controller.py index 7fd2baaf..2b211f5f 100644 --- a/server/controllers/ws_controller.py +++ b/server/controllers/ws_controller.py @@ -470,6 +470,7 @@ async def on_message(self, message: Union[str, bytes]): if self.current_username else self.request.remote_ip ) + raw = message size = len(message) try: @@ -484,10 +485,14 @@ async def on_message(self, message: Union[str, bytes]): ) return - # Never log the payload: AUTHENTICATE / REFRESH_TOKEN carry live JWTs. - get_logger().debug( - f"WebSocket message from {user_part}: {ws_op} ({size} bytes)" - ) + # With redaction on never log the payload: AUTHENTICATE / REFRESH_TOKEN + # carry live JWTs. + if self.application.digi_settings.get_sync("log_redaction"): + get_logger().debug( + f"WebSocket message from {user_part}: {ws_op} ({size} bytes)" + ) + else: + get_logger().debug(f"WebSocket message from {user_part}: {raw}") # Connection boundary: an exception escaping on_message ends Tornado's # read loop without calling on_close, leaving a handler that no longer diff --git a/server/digi_server/settings.py b/server/digi_server/settings.py index 37737242..fa5fa4cc 100644 --- a/server/digi_server/settings.py +++ b/server/digi_server/settings.py @@ -302,10 +302,14 @@ def init_settings(self): self.define( "log_redaction", bool, - False, + True, True, display_name="Enable Log Redaction", - help_text="When enabled, potentially sensitive information will be redacted from logs.", + help_text=( + "When enabled (the default), passwords, tokens and WebSocket message " + "contents are kept out of the logs. Disable only while debugging: " + "logs will then contain credentials and tokens." + ), category="Logging", ) self.define( diff --git a/server/test/controllers/test_ws_controller.py b/server/test/controllers/test_ws_controller.py index 296bb8d4..c889aa5c 100644 --- a/server/test/controllers/test_ws_controller.py +++ b/server/test/controllers/test_ws_controller.py @@ -3512,9 +3512,8 @@ async def test_non_object_and_non_string_op_frames_are_ignored(self): await self._still_responsive(ws, self.admin_id) ws.close() - @gen_test - async def test_ws_payload_and_tokens_are_not_logged(self): - """AUTHENTICATE tokens never reach the log; only op name and size do.""" + async def _auth_logs(self, redaction): + self._app.digi_settings.settings["log_redaction"].set_value(redaction) ws, _ = await self._connect_and_auth() sentinel = self._app.jwt_service.create_access_token( data={"user_id": self.admin_id} @@ -3524,10 +3523,20 @@ async def test_ws_payload_and_tokens_are_not_logged(self): json.dumps({"OP": "AUTHENTICATE", "DATA": {"token": sentinel}}) ) await self._read_until(ws, op="WS_AUTH_SUCCESS") - joined = "\n".join(captured.output) + ws.close() + return sentinel, "\n".join(captured.output) + + @gen_test + async def test_ws_payload_not_logged_when_redaction_on(self): + """With redaction on only the op name and size are logged.""" + sentinel, joined = await self._auth_logs(True) self.assertIn("AUTHENTICATE", joined) self.assertNotIn(sentinel, joined) - ws.close() + + @gen_test + async def test_ws_payload_logged_raw_when_redaction_off(self): + sentinel, joined = await self._auth_logs(False) + self.assertIn(sentinel, joined) @gen_test async def test_update_session_survives_deleted_user(self): diff --git a/server/test/digi_server/test_settings.py b/server/test/digi_server/test_settings.py new file mode 100644 index 00000000..7c8267af --- /dev/null +++ b/server/test/digi_server/test_settings.py @@ -0,0 +1,11 @@ +from test.conftest import DigiScriptTestCase + + +class TestLoggingSettingDefaults(DigiScriptTestCase): + def test_log_level_defaults_to_info(self): + self.assertEqual("INFO", self._app.digi_settings.settings["log_level"].default) + + def test_log_redaction_enabled_by_default(self): + setting = self._app.digi_settings.settings["log_redaction"] + self.assertTrue(setting.default) + self.assertTrue(setting.get_value()) diff --git a/server/test/utils/web/test_base_controller.py b/server/test/utils/web/test_base_controller.py new file mode 100644 index 00000000..82624e34 --- /dev/null +++ b/server/test/utils/web/test_base_controller.py @@ -0,0 +1,54 @@ +import json + +from tornado.httpclient import HTTPRequest +from tornado.testing import gen_test + +from test.conftest import DigiScriptTestCase + + +class TestRequestBodyLogging(DigiScriptTestCase): + def _set_redaction(self, value): + self._app.digi_settings.settings["log_redaction"].set_value(value) + + async def _post_login(self, body): + request = HTTPRequest( + self.get_url("/api/v1/auth/login"), + method="POST", + body=body, + ) + with self.assertLogs(level="DEBUG") as captured: + await self.http_client.fetch(request, raise_error=False) + return "\n".join(captured.output) + + @gen_test + async def test_secrets_hidden_when_redaction_enabled(self): + self._set_redaction(True) + joined = await self._post_login( + json.dumps( + { + "username": "nobody", + "password": "SENTINEL-PASSWORD", + "extra": {"access_token": "SENTINEL-TOKEN"}, + } + ) + ) + self.assertIn("/api/v1/auth/login", joined) + self.assertNotIn("SENTINEL-PASSWORD", joined) + self.assertNotIn("SENTINEL-TOKEN", joined) + + @gen_test + async def test_body_logged_raw_when_redaction_disabled(self): + self._set_redaction(False) + joined = await self._post_login( + json.dumps({"username": "nobody", "password": "SENTINEL-PASSWORD"}) + ) + self.assertIn("SENTINEL-PASSWORD", joined) + + @gen_test + async def test_non_json_body_follows_setting(self): + self._set_redaction(True) + self.assertNotIn( + "SENTINEL-RAW", await self._post_login("password=SENTINEL-RAW") + ) + self._set_redaction(False) + self.assertIn("SENTINEL-RAW", await self._post_login("password=SENTINEL-RAW")) diff --git a/server/test/utils/web/test_log_redaction.py b/server/test/utils/web/test_log_redaction.py index 1f25e8b5..0620b0ee 100644 --- a/server/test/utils/web/test_log_redaction.py +++ b/server/test/utils/web/test_log_redaction.py @@ -1,10 +1,5 @@ -import json import unittest -from tornado.httpclient import HTTPRequest -from tornado.testing import gen_test - -from test.conftest import DigiScriptTestCase from utils.web.log_redaction import REDACTED, redact_sensitive_keys @@ -32,38 +27,3 @@ def test_does_not_mutate_input(self): def test_scalars_pass_through(self): self.assertEqual("x", redact_sensitive_keys("x")) self.assertIsNone(redact_sensitive_keys(None)) - - -class TestRestBodyLogging(DigiScriptTestCase): - @gen_test - async def test_sentinel_secrets_never_logged(self): - body = { - "username": "nobody", - "password": "SENTINEL-PASSWORD", - "extra": {"access_token": "SENTINEL-TOKEN"}, - } - request = HTTPRequest( - self.get_url("/api/v1/auth/login"), - method="POST", - body=json.dumps(body), - ) - with self.assertLogs(level="DEBUG") as captured: - await self.http_client.fetch(request, raise_error=False) - joined = "\n".join(captured.output) - self.assertIn("/api/v1/auth/login", joined) - self.assertNotIn("SENTINEL-PASSWORD", joined) - self.assertNotIn("SENTINEL-TOKEN", joined) - - @gen_test - async def test_non_json_body_not_logged(self): - request = HTTPRequest( - self.get_url("/api/v1/auth/login"), - method="POST", - body="password=SENTINEL-RAW", - ) - with self.assertLogs(level="DEBUG") as captured: - await self.http_client.fetch(request, raise_error=False) - self.assertNotIn("SENTINEL-RAW", "\n".join(captured.output)) - - def test_default_log_level_is_info(self): - self.assertEqual("INFO", self._app.digi_settings.settings["log_level"].default) diff --git a/server/utils/web/base_controller.py b/server/utils/web/base_controller.py index e1a42f52..c9625651 100644 --- a/server/utils/web/base_controller.py +++ b/server/utils/web/base_controller.py @@ -183,22 +183,21 @@ def on_finish(self): try: body = escape.json_decode(self.request.body) except BaseException: - # Not JSON, so it cannot be redacted by key: log only its size. + if self.application.digi_settings.settings["log_redaction"].get_value(): + # Not JSON, so it cannot be redacted by key: log only its size. + logged = f"" + else: + logged = self.request.body get_logger().debug( - f"{self.request.method} {self.request.path} " - f"{user_suffix}", + f"{self.request.method} {self.request.path} {logged}{user_suffix}", extra=self._log_extra(), ) else: - if ( - redacted_data_paths - and self.application.digi_settings.settings[ - "log_redaction" - ].get_value() - ): + if self.application.digi_settings.settings["log_redaction"].get_value(): body = deepcopy(body) - redacted_data_paths.apply(body) - body = redact_sensitive_keys(body) + if redacted_data_paths: + redacted_data_paths.apply(body) + body = redact_sensitive_keys(body) log_method( f"{self.request.method} {self.request.path} {body}{user_suffix}", From cad9f115ff71deaa5480d8dec342c59d0d06dad4 Mon Sep 17 00:00:00 2001 From: Tim Bradgate Date: Mon, 19 Oct 2026 23:19:48 +0100 Subject: [PATCH 04/11] Redistribute tests into modules mirroring the code under test Co-Authored-By: Claude Sonnet 5.5 --- .../test/controllers/api/v1/auth/test_auth.py | 28 ++++ .../api/v1/show/session/test_sessions.py | 29 ++++ server/test/controllers/api/v1/test_rbac.py | 46 ++++++ .../api/v1/test_security_hygiene.py | 151 ------------------ .../test/controllers/api/v1/test_settings.py | 13 ++ .../controllers/api/v1/user/test_settings.py | 44 +++++ server/test/controllers/api/v2/test_users.py | 8 + server/test/utils/web/test_base_controller.py | 16 ++ server/test/utils/web/test_json_body.py | 35 ++++ server/test/utils/web/test_web_decorators.py | 21 +++ 10 files changed, 240 insertions(+), 151 deletions(-) delete mode 100644 server/test/controllers/api/v1/test_security_hygiene.py create mode 100644 server/test/controllers/api/v1/user/test_settings.py create mode 100644 server/test/utils/web/test_base_controller.py create mode 100644 server/test/utils/web/test_json_body.py diff --git a/server/test/controllers/api/v1/auth/test_auth.py b/server/test/controllers/api/v1/auth/test_auth.py index 46bd5a7a..7fc23457 100644 --- a/server/test/controllers/api/v1/auth/test_auth.py +++ b/server/test/controllers/api/v1/auth/test_auth.py @@ -1249,3 +1249,31 @@ def test_admin_reset_password_missing_user_id(self): self.assertEqual(400, response.code) response_body = escape.json_decode(response.body) self.assertEqual("user_id is required", response_body["message"]) + + +class TestAuthJsonBodyValidation(DigiScriptTestCase): + def _post(self, path, body, token=None, method="POST"): + return self.fetch( + path, + method=method, + body=body, + headers={"Authorization": f"Bearer {token}"} if token else {}, + raise_error=False, + ) + + def test_login_invalid_json_returns_400(self): + self.assertEqual(400, self._post("/api/v1/auth/login", "{not json").code) + + def test_login_non_object_json_returns_400(self): + self.assertEqual(400, self._post("/api/v1/auth/login", "[1, 2]").code) + + def test_login_empty_body_returns_400(self): + self.assertEqual(400, self._post("/api/v1/auth/login", "").code) + + def test_create_user_invalid_json_returns_400(self): + self.assertEqual(400, self._post("/api/v1/auth/create", "nope").code) + + def test_change_password_invalid_json_returns_400(self): + token = self._create_and_login_admin() + resp = self._post("/api/v1/auth/change-password", "nope", token, "PATCH") + self.assertEqual(400, resp.code) diff --git a/server/test/controllers/api/v1/show/session/test_sessions.py b/server/test/controllers/api/v1/show/session/test_sessions.py index d8330506..61d33153 100644 --- a/server/test/controllers/api/v1/show/session/test_sessions.py +++ b/server/test/controllers/api/v1/show/session/test_sessions.py @@ -66,3 +66,32 @@ def test_get_sessions_with_data(self): self.assertEqual(200, response.code) response_body = tornado.escape.json_decode(response.body) self.assertEqual(2, len(response_body["sessions"])) + + def test_start_session_invalid_json_returns_400(self): + """A malformed body reaches the JSON helper (a show is loaded) and gets 400.""" + token = self._create_and_login_admin() + for body in ("nope", "[1]"): + with self.subTest(body=body): + response = self.fetch( + "/api/v1/show/sessions/start", + method="POST", + body=body, + headers={"Authorization": f"Bearer {token}"}, + raise_error=False, + ) + self.assertEqual(400, response.code) + + def test_start_session_valid_json_reaches_handler(self): + """Control for the test above: a valid body gets the handler's own error.""" + token = self._create_and_login_admin() + response = self.fetch( + "/api/v1/show/sessions/start", + method="POST", + body="{}", + headers={"Authorization": f"Bearer {token}"}, + raise_error=False, + ) + self.assertEqual(400, response.code) + self.assertEqual( + "session_id missing", tornado.escape.json_decode(response.body)["message"] + ) diff --git a/server/test/controllers/api/v1/test_rbac.py b/server/test/controllers/api/v1/test_rbac.py index d99642b2..91982bd0 100644 --- a/server/test/controllers/api/v1/test_rbac.py +++ b/server/test/controllers/api/v1/test_rbac.py @@ -110,3 +110,49 @@ def test_get_objects_for_resource(self): self.assertIn("objects", response_body) # Should find the script we created self.assertEqual(1, len(response_body["objects"])) + + +class TestRBACRoleChangeValidation(DigiScriptTestCase): + def _body(self, **overrides): + body = {"resource": "shows", "object": {"id": 1}, "user": 1, "role": 1} + body.update(overrides) + return body + + def _post(self, path, body, token): + return self.fetch( + path, + method="POST", + body=body if isinstance(body, str) else tornado.escape.json_encode(body), + headers={"Authorization": f"Bearer {token}"}, + raise_error=False, + ) + + def test_grant_and_revoke_invalid_input_returns_400(self): + token = self._create_and_login_admin() + bad_bodies = [ + self._body(role=999999), + self._body(role="admin"), + self._body(user="abc"), + self._body(object="not-a-dict"), + self._body(object={"unrelated": 1}), + self._body(resource=["shows"]), + ] + for path in ( + "/api/v1/rbac/user/roles/grant", + "/api/v1/rbac/user/roles/revoke", + ): + for body in bad_bodies: + with self.subTest(path=path, body=body): + self.assertEqual(400, self._post(path, body, token).code) + + def test_grant_missing_field_returns_400(self): + token = self._create_and_login_admin() + body = self._body() + del body["role"] + resp = self._post("/api/v1/rbac/user/roles/grant", body, token) + self.assertEqual(400, resp.code) + + def test_grant_invalid_json_returns_400(self): + token = self._create_and_login_admin() + resp = self._post("/api/v1/rbac/user/roles/grant", "nope", token) + self.assertEqual(400, resp.code) diff --git a/server/test/controllers/api/v1/test_security_hygiene.py b/server/test/controllers/api/v1/test_security_hygiene.py deleted file mode 100644 index f4299e4c..00000000 --- a/server/test/controllers/api/v1/test_security_hygiene.py +++ /dev/null @@ -1,151 +0,0 @@ -"""Tests for the security hygiene hardening: JSON body parsing, admin status codes, -RBAC input validation, user settings allow-list and default security headers.""" - -from tornado import escape - -from models.user import UserSettings -from test.conftest import DigiScriptTestCase - - -class TestSecurityHygiene(DigiScriptTestCase): - def _auth(self, token): - return {"Authorization": f"Bearer {token}"} - - def _post(self, path, body, token=None, method="POST"): - return self.fetch( - path, - method=method, - body=body if isinstance(body, str) else escape.json_encode(body), - headers=self._auth(token) if token else {}, - raise_error=False, - ) - - # ─── Security headers ────────────────────────────────────────────────────── - - def test_security_headers_present(self): - resp = self.fetch("/api/v1/health", raise_error=False) - self.assertEqual("nosniff", resp.headers.get("X-Content-Type-Options")) - self.assertEqual("SAMEORIGIN", resp.headers.get("X-Frame-Options")) - self.assertEqual("same-origin", resp.headers.get("Referrer-Policy")) - - def test_security_headers_present_on_error_responses(self): - resp = self.fetch("/api/v1/rbac/user/resources", raise_error=False) - self.assertEqual(401, resp.code) - self.assertEqual("nosniff", resp.headers.get("X-Content-Type-Options")) - - # ─── JSON body helper ────────────────────────────────────────────────────── - - def test_login_invalid_json_returns_400(self): - self.assertEqual(400, self._post("/api/v1/auth/login", "{not json").code) - - def test_login_non_object_json_returns_400(self): - self.assertEqual(400, self._post("/api/v1/auth/login", "[1, 2]").code) - - def test_login_empty_body_returns_400(self): - self.assertEqual(400, self._post("/api/v1/auth/login", "").code) - - def test_create_user_invalid_json_returns_400(self): - self.assertEqual(400, self._post("/api/v1/auth/create", "nope").code) - - def test_v2_create_user_invalid_json_returns_400(self): - self.assertEqual(400, self._post("/api/v2/users", "nope").code) - - def test_change_password_invalid_json_returns_400(self): - token = self._create_and_login_admin() - resp = self._post("/api/v1/auth/change-password", "nope", token, "PATCH") - self.assertEqual(400, resp.code) - - def test_rbac_grant_invalid_json_returns_400(self): - token = self._create_and_login_admin() - self.assertEqual( - 400, self._post("/api/v1/rbac/user/roles/grant", "nope", token).code - ) - - def test_user_settings_patch_non_object_returns_400(self): - token = self._create_and_login_admin() - resp = self._post("/api/v1/user/settings", "[1]", token, "PATCH") - self.assertEqual(400, resp.code) - - def test_system_settings_patch_invalid_json_returns_400(self): - token = self._create_and_login_admin() - resp = self._post("/api/v1/settings", "nope", token, "PATCH") - self.assertEqual(400, resp.code) - - def test_session_start_invalid_json_returns_400(self): - token = self._create_and_login_admin() - resp = self._post("/api/v1/show/sessions/start", "nope", token) - # No show is loaded in this fixture, so requires_show answers first. - self.assertEqual(400, resp.code) - - # ─── require_admin status codes ──────────────────────────────────────────── - - def test_require_admin_unauthenticated_is_401(self): - resp = self._post("/api/v1/rbac/user/roles/grant", {}) - self.assertEqual(401, resp.code) - - def test_require_admin_non_admin_is_403(self): - admin_token = self._create_and_login_admin() - user_token = self._create_and_login_user(admin_token) - resp = self._post("/api/v1/rbac/user/roles/grant", {}, user_token) - self.assertEqual(403, resp.code) - - # ─── RBAC grant / revoke validation ──────────────────────────────────────── - - def _role_body(self, **overrides): - body = { - "resource": "shows", - "object": {"id": 1}, - "user": 1, - "role": 1, - } - body.update(overrides) - return body - - def test_rbac_grant_and_revoke_invalid_input_returns_400(self): - token = self._create_and_login_admin() - bad_bodies = [ - self._role_body(role=999999), - self._role_body(role="admin"), - self._role_body(user="abc"), - self._role_body(object="not-a-dict"), - self._role_body(object={"unrelated": 1}), - self._role_body(resource=["shows"]), - ] - for path in ( - "/api/v1/rbac/user/roles/grant", - "/api/v1/rbac/user/roles/revoke", - ): - for body in bad_bodies: - with self.subTest(path=path, body=body): - resp = self._post(path, body, token) - self.assertEqual(400, resp.code) - - def test_rbac_grant_missing_field_returns_400(self): - token = self._create_and_login_admin() - body = self._role_body() - del body["role"] - resp = self._post("/api/v1/rbac/user/roles/grant", body, token) - self.assertEqual(400, resp.code) - - # ─── User settings allow-list ────────────────────────────────────────────── - - def test_user_settings_patch_sets_allowed_and_ignores_others(self): - token = self._create_and_login_admin() - resp = self._post( - "/api/v1/user/settings", - { - "cue_position_right": True, - "_user_id": 999, - "metadata": "x", - "registry": "x", - "nonexistent": 1, - }, - token, - "PATCH", - ) - self.assertEqual(200, resp.code) - - with self._app.get_db().sessionmaker() as session: - settings = session.scalars(session.query(UserSettings).statement).first() - self.assertTrue(settings.cue_position_right) - self.assertNotEqual(999, settings._user_id) diff --git a/server/test/controllers/api/v1/test_settings.py b/server/test/controllers/api/v1/test_settings.py index 64a9597c..4d6f1778 100644 --- a/server/test/controllers/api/v1/test_settings.py +++ b/server/test/controllers/api/v1/test_settings.py @@ -64,3 +64,16 @@ def test_jwt_lifetime_as_json_includes_choice_labels(self): self.assertEqual( len(json_repr["choice_labels"]), len(json_repr["choice_options"]) ) + + +class TestSettingsController(DigiScriptTestCase): + def test_patch_invalid_json_returns_400(self): + token = self._create_and_login_admin() + resp = self.fetch( + "/api/v1/settings", + method="PATCH", + body="nope", + headers={"Authorization": f"Bearer {token}"}, + raise_error=False, + ) + self.assertEqual(400, resp.code) diff --git a/server/test/controllers/api/v1/user/test_settings.py b/server/test/controllers/api/v1/user/test_settings.py new file mode 100644 index 00000000..e1066aea --- /dev/null +++ b/server/test/controllers/api/v1/user/test_settings.py @@ -0,0 +1,44 @@ +"""Tests for the /api/v1/user/settings controller.""" + +from tornado import escape + +from models.user import UserSettings +from test.conftest import DigiScriptTestCase + + +class TestUserSettingsController(DigiScriptTestCase): + def _patch(self, body, token): + return self.fetch( + "/api/v1/user/settings", + method="PATCH", + body=body if isinstance(body, str) else escape.json_encode(body), + headers={"Authorization": f"Bearer {token}"}, + raise_error=False, + ) + + def test_patch_non_object_returns_400(self): + token = self._create_and_login_admin() + self.assertEqual(400, self._patch("[1]", token).code) + + def test_patch_invalid_json_returns_400(self): + token = self._create_and_login_admin() + self.assertEqual(400, self._patch("nope", token).code) + + def test_patch_sets_allowed_and_ignores_others(self): + token = self._create_and_login_admin() + resp = self._patch( + { + "cue_position_right": True, + "_user_id": 999, + "metadata": "x", + "registry": "x", + "nonexistent": 1, + }, + token, + ) + self.assertEqual(200, resp.code) + + with self._app.get_db().sessionmaker() as session: + settings = session.scalars(session.query(UserSettings).statement).first() + self.assertTrue(settings.cue_position_right) + self.assertNotEqual(999, settings._user_id) diff --git a/server/test/controllers/api/v2/test_users.py b/server/test/controllers/api/v2/test_users.py index 8d3b0a9a..1d22e0f0 100644 --- a/server/test/controllers/api/v2/test_users.py +++ b/server/test/controllers/api/v2/test_users.py @@ -310,3 +310,11 @@ def test_delete_self_rejected(self): # note: the last-admin delete guard (400 when target is the only admin) suffers the same # race-condition constraint as the PATCH guard — not reachable in a synchronous test. + + +class TestUsersV2JsonBodyValidation(DigiScriptTestCase): + def test_create_user_invalid_json_returns_400(self): + resp = self.fetch( + "/api/v2/users", method="POST", body="nope", raise_error=False + ) + self.assertEqual(400, resp.code) diff --git a/server/test/utils/web/test_base_controller.py b/server/test/utils/web/test_base_controller.py new file mode 100644 index 00000000..bf29b283 --- /dev/null +++ b/server/test/utils/web/test_base_controller.py @@ -0,0 +1,16 @@ +"""Tests for the default response headers set by BaseController.""" + +from test.conftest import DigiScriptTestCase + + +class TestBaseControllerDefaultHeaders(DigiScriptTestCase): + def test_security_headers_present(self): + resp = self.fetch("/api/v1/health", raise_error=False) + self.assertEqual("nosniff", resp.headers.get("X-Content-Type-Options")) + self.assertEqual("SAMEORIGIN", resp.headers.get("X-Frame-Options")) + self.assertEqual("same-origin", resp.headers.get("Referrer-Policy")) + + def test_security_headers_present_on_error_responses(self): + resp = self.fetch("/api/v1/rbac/user/resources", raise_error=False) + self.assertEqual(401, resp.code) + self.assertEqual("nosniff", resp.headers.get("X-Content-Type-Options")) diff --git a/server/test/utils/web/test_json_body.py b/server/test/utils/web/test_json_body.py new file mode 100644 index 00000000..279f354e --- /dev/null +++ b/server/test/utils/web/test_json_body.py @@ -0,0 +1,35 @@ +"""Tests for utils/web/json_body.py.""" + +from unittest.mock import MagicMock + +from tornado.web import HTTPError + +from test.conftest import DigiScriptTestCase +from utils.web.json_body import get_json_object + + +def _handler(body: bytes): + handler = MagicMock() + handler.request.body = body + return handler + + +class TestGetJsonObject(DigiScriptTestCase): + def test_valid_object_returned(self): + self.assertEqual({"a": 1}, get_json_object(_handler(b'{"a": 1}'))) + + def test_invalid_json_raises_400(self): + with self.assertRaises(HTTPError) as ctx: + get_json_object(_handler(b"{not json")) + self.assertEqual(400, ctx.exception.status_code) + + def test_empty_body_raises_400(self): + with self.assertRaises(HTTPError) as ctx: + get_json_object(_handler(b"")) + self.assertEqual(400, ctx.exception.status_code) + + def test_non_object_raises_400(self): + for body in (b"[1, 2]", b'"text"', b"5", b"null"): + with self.subTest(body=body), self.assertRaises(HTTPError) as ctx: + get_json_object(_handler(body)) + self.assertEqual(400, ctx.exception.status_code) diff --git a/server/test/utils/web/test_web_decorators.py b/server/test/utils/web/test_web_decorators.py index b54b9ee2..660b6e7c 100644 --- a/server/test/utils/web/test_web_decorators.py +++ b/server/test/utils/web/test_web_decorators.py @@ -116,3 +116,24 @@ def test_empty_room_passes_through(self): self.assertNotEqual(409, response.code) finally: del self._app.room_manager + + +class TestRequireAdminDecorator(DigiScriptTestCase): + """require_admin: 401 when not logged in, 403 for a logged-in non-admin.""" + + def _grant(self, token=None): + return self.fetch( + "/api/v1/rbac/user/roles/grant", + method="POST", + body="{}", + headers={"Authorization": f"Bearer {token}"} if token else {}, + raise_error=False, + ) + + def test_unauthenticated_is_401(self): + self.assertEqual(401, self._grant().code) + + def test_non_admin_is_403(self): + admin_token = self._create_and_login_admin() + user_token = self._create_and_login_user(admin_token) + self.assertEqual(403, self._grant(user_token).code) From 603542e517e72453f55624ba68090559245b923e Mon Sep 17 00:00:00 2001 From: Tim Bradgate Date: Tue, 20 Oct 2026 13:26:23 +0100 Subject: [PATCH 05/11] Reduce cognitive complexity of REST body logging and WS session op dispatch Behaviour unchanged; addresses SonarCloud S3776 findings. Co-Authored-By: Claude Sonnet 5.5 --- server/controllers/ws_controller.py | 79 ++++++++++++++++------------- server/utils/web/base_controller.py | 67 +++++++++++++++--------- 2 files changed, 89 insertions(+), 57 deletions(-) diff --git a/server/controllers/ws_controller.py b/server/controllers/ws_controller.py index 2b211f5f..2db3b21f 100644 --- a/server/controllers/ws_controller.py +++ b/server/controllers/ws_controller.py @@ -545,47 +545,58 @@ async def _handle_session_op(self, ws_op: str, message: dict) -> None: elif ws_op == "REFRESH_CLIENT": self._resume_client(message.get("DATA")) elif ws_op in self._LEADER_OP_HANDLERS: - show_session = ( - session.get(ShowSession, show.current_session_id) - if show and show.current_session_id - else None - ) - # Ownership rule: only the authenticated show user on its own row. - if self._is_leader(session, show_session): - data = message.get("DATA") - if not isinstance(data, dict): - get_logger().warning(f"Ignored {ws_op}: DATA must be an object") - return - handler = getattr(self, self._LEADER_OP_HANDLERS[ws_op]) - await handler(session, show_session, entry, data) - else: - get_logger().debug( - f"Ignored {ws_op} from client " - f"{self.__getattribute__('internal_id')} (user " - f"{self.current_user_id}): not the live-show leader" - ) + await self._handle_leader_op(session, show, entry, ws_op, message) elif ws_op == "LIVE_SHOW_JUMP_TO_PAGE": - show_session = ( - session.get(ShowSession, show.current_session_id) - if show and show.current_session_id - else None - ) - if show_session and self._user_has_show_execute_access(session, show): - await self._op_jump_to_page( - session, show_session, entry, message.get("DATA") - ) - else: - get_logger().debug( - f"Ignored {ws_op} from client " - f"{self.__getattribute__('internal_id')} (user " - f"{self.current_user_id}): no show execute access" - ) + await self._handle_jump_op(session, show, entry, ws_op, message) else: get_logger().warning( f"Unknown OP {ws_op} received from " f"WebSocket connection {self.request.remote_ip}" ) + @staticmethod + def _live_show_session(session, show: Optional[Show]) -> Optional[ShowSession]: + """Return the show's current live ShowSession, or None.""" + if show and show.current_session_id: + return session.get(ShowSession, show.current_session_id) + return None + + async def _handle_leader_op( + self, session, show: Optional[Show], entry, ws_op: str, message: dict + ) -> None: + """Run a leader-only live-show op if this connection is the leader.""" + show_session = self._live_show_session(session, show) + # Ownership rule: only the authenticated show user on its own row. + if not self._is_leader(session, show_session): + get_logger().debug( + f"Ignored {ws_op} from client " + f"{self.__getattribute__('internal_id')} (user " + f"{self.current_user_id}): not the live-show leader" + ) + return + data = message.get("DATA") + if not isinstance(data, dict): + get_logger().warning(f"Ignored {ws_op}: DATA must be an object") + return + handler = getattr(self, self._LEADER_OP_HANDLERS[ws_op]) + await handler(session, show_session, entry, data) + + async def _handle_jump_op( + self, session, show: Optional[Show], entry, ws_op: str, message: dict + ) -> None: + """Run ``LIVE_SHOW_JUMP_TO_PAGE`` if the user has show EXECUTE access.""" + show_session = self._live_show_session(session, show) + if show_session and self._user_has_show_execute_access(session, show): + await self._op_jump_to_page( + session, show_session, entry, message.get("DATA") + ) + return + get_logger().debug( + f"Ignored {ws_op} from client " + f"{self.__getattribute__('internal_id')} (user " + f"{self.current_user_id}): no show execute access" + ) + def _resume_client(self, new_uuid: Any) -> None: """Handle ``REFRESH_CLIENT``: a reconnecting page resumes its old uuid. diff --git a/server/utils/web/base_controller.py b/server/utils/web/base_controller.py index c9625651..b5cf81a3 100644 --- a/server/utils/web/base_controller.py +++ b/server/utils/web/base_controller.py @@ -21,6 +21,9 @@ if TYPE_CHECKING: from digi_server.app_server import DigiScriptServer +# Returned by BaseAPIController._loggable_body when a body is not valid JSON. +_NON_JSON = object() + class DatabaseMixin: """Mixin providing database session management (replaces tornado_sqlalchemy.SessionMixin).""" @@ -165,6 +168,39 @@ def _log_extra(self) -> dict: extra["user_id"] = self.current_user.get("id") return extra + def _redaction_enabled(self) -> bool: + """Return True if the ``log_redaction`` setting is on.""" + return bool( + self.application.digi_settings.settings["log_redaction"].get_value() + ) + + def _loggable_raw_body(self) -> Any: + """Return a non-JSON request body in the form allowed by ``log_redaction``. + + With redaction on only its size is returned, as it cannot be redacted by key. + """ + if self._redaction_enabled(): + return f"" + return self.request.body + + def _loggable_body(self, redacted_data_paths) -> Any: + """Decode the JSON request body, redacted if ``log_redaction`` is on. + + :param redacted_data_paths: Decorator-supplied patch of paths to redact, + or None. + :returns: The body, or the ``_NON_JSON`` sentinel if it is not valid JSON. + """ + try: + body = escape.json_decode(self.request.body) + except BaseException: + return _NON_JSON + if self._redaction_enabled(): + body = deepcopy(body) + if redacted_data_paths: + redacted_data_paths.apply(body) + body = redact_sensitive_keys(body) + return body + def on_finish(self): from utils.web.route import Route # noqa: PLC0415 @@ -180,28 +216,13 @@ def on_finish(self): method_name = self.request.method.lower() handler_method = getattr(self, method_name, None) redacted_data_paths = getattr(handler_method, "_redacted_data_paths", None) - try: - body = escape.json_decode(self.request.body) - except BaseException: - if self.application.digi_settings.settings["log_redaction"].get_value(): - # Not JSON, so it cannot be redacted by key: log only its size. - logged = f"" - else: - logged = self.request.body - get_logger().debug( - f"{self.request.method} {self.request.path} {logged}{user_suffix}", - extra=self._log_extra(), - ) - else: - if self.application.digi_settings.settings["log_redaction"].get_value(): - body = deepcopy(body) - if redacted_data_paths: - redacted_data_paths.apply(body) - body = redact_sensitive_keys(body) - - log_method( - f"{self.request.method} {self.request.path} {body}{user_suffix}", - extra=self._log_extra(), - ) + logged = self._loggable_body(redacted_data_paths) + if logged is _NON_JSON: + log_method = get_logger().debug + logged = self._loggable_raw_body() + log_method( + f"{self.request.method} {self.request.path} {logged}{user_suffix}", + extra=self._log_extra(), + ) super().on_finish() From 1c91d9824b56f9028843c8d3ce6cedfd8e39c2f1 Mon Sep 17 00:00:00 2001 From: Tim Bradgate Date: Tue, 20 Oct 2026 13:42:55 +0100 Subject: [PATCH 06/11] Merge dev; non-admin show delete now returns 403 Co-Authored-By: Claude Sonnet 5.5 --- server/test/controllers/api/v1/show/test_shows.py | 2 +- server/test/utils/web/test_base_controller.py | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/server/test/controllers/api/v1/show/test_shows.py b/server/test/controllers/api/v1/show/test_shows.py index e70df019..58716411 100644 --- a/server/test/controllers/api/v1/show/test_shows.py +++ b/server/test/controllers/api/v1/show/test_shows.py @@ -450,7 +450,7 @@ def test_delete_show_non_admin(self): 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) + self.assertEqual(403, response.code) with self._app.get_db().sessionmaker() as session: self.assertIsNotNone(session.get(Show, show_b_id)) diff --git a/server/test/utils/web/test_base_controller.py b/server/test/utils/web/test_base_controller.py index 98d366cf..643fd788 100644 --- a/server/test/utils/web/test_base_controller.py +++ b/server/test/utils/web/test_base_controller.py @@ -4,6 +4,7 @@ from tornado.httpclient import HTTPRequest from tornado.testing import gen_test + from test.conftest import DigiScriptTestCase From abd282e3db178844b39fb4b205c828e684540407 Mon Sep 17 00:00:00 2001 From: Tim Bradgate Date: Tue, 20 Oct 2026 13:47:00 +0100 Subject: [PATCH 07/11] Address review: fix redaction crash on absent decorated path, log malformed WS frames per setting - Key-based redaction first; tolerate decorated paths missing from the body - Malformed WS frames log exception text and content per log_redaction - Split BEGIN_INTERVAL warnings, warn on non-object script-room DATA - Redact confirmPassword generically - Tests for absent-path logging, bool interval guards, saved settings kept - Docs: note Log Level must be DEBUG to see raw content Co-Authored-By: Claude Sonnet 5.5 --- docs/pages/user_config.md | 4 +-- server/controllers/ws_controller.py | 17 +++++++++--- server/test/controllers/test_ws_controller.py | 27 ++++++++++++++++--- server/test/digi_server/test_settings.py | 23 ++++++++++++++++ server/test/utils/web/test_base_controller.py | 26 ++++++++++++++++++ server/utils/web/base_controller.py | 10 +++++-- server/utils/web/log_redaction.py | 2 ++ 7 files changed, 98 insertions(+), 11 deletions(-) diff --git a/docs/pages/user_config.md b/docs/pages/user_config.md index 4fe3292c..d22164cc 100644 --- a/docs/pages/user_config.md +++ b/docs/pages/user_config.md @@ -80,7 +80,7 @@ RBAC configuration determines what shows a user can access and what actions they The **Enable Log Redaction** system setting is on by default. While it is on, passwords, tokens and other credentials are replaced with a placeholder in logged HTTP request bodies, and WebSocket messages are logged as their operation name and size only. -Turn it off to see the raw HTTP and WebSocket content while debugging. **Warning:** with redaction off, the logs contain credentials and authentication tokens, so only disable it temporarily and treat the log files as sensitive. Installations that already had the setting saved keep their existing value. +Turn it off to see the raw HTTP and WebSocket content while debugging. Request bodies and WebSocket messages are logged at debug level, so also set **Log Level** to `DEBUG` (the default is `INFO`). **Warning:** with redaction off, the logs contain credentials and authentication tokens, so only disable it temporarily and treat the log files as sensitive. A value you have saved is never changed by a later change to a setting's default. ### Log Viewer @@ -88,7 +88,7 @@ The **Logs** tab (admin only) provides a real-time view of server and client log #### Sources -- **Server** — Application-level logs from the DigiScript backend: HTTP request access logs, request body debug logs, WebSocket connection events, and general application messages. +- **Server** — Application-level logs from the DigiScript backend: HTTP request access logs, request body and WebSocket message debug logs (only shown when the Log Level is `DEBUG`), WebSocket connection events, and general application messages. - **Client** — Logs forwarded from connected browser clients via the `/api/v1/logs/batch` endpoint. #### Username Attribution diff --git a/server/controllers/ws_controller.py b/server/controllers/ws_controller.py index 2db3b21f..7fa892eb 100644 --- a/server/controllers/ws_controller.py +++ b/server/controllers/ws_controller.py @@ -479,9 +479,13 @@ async def on_message(self, message: Union[str, bytes]): if not isinstance(ws_op, str): raise TypeError("OP must be a string") except (json.JSONDecodeError, KeyError, TypeError) as e: + if self.application.digi_settings.get_sync("log_redaction"): + content = f"<{size} bytes>" + else: + content = raw get_logger().warning( - f"Malformed WS message ({size} bytes) from " - f"{self.request.remote_ip}: {type(e).__name__}" + f"Malformed WS message from {self.request.remote_ip}: " + f"{type(e).__name__}: {e} {content}" ) return @@ -681,9 +685,13 @@ async def _op_begin_interval(self, session, show_session, entry, data) -> None: """Leader op ``BEGIN_INTERVAL``: start an interval after the given act.""" act_id = data.get("actId") length = data.get("length") + if not entry: + get_logger().warning( + "Ignored BEGIN_INTERVAL: no session entry for this connection" + ) + return if ( - not entry - or isinstance(act_id, bool) + isinstance(act_id, bool) or not isinstance(act_id, int) or isinstance(length, bool) or not isinstance(length, (int, float)) @@ -875,6 +883,7 @@ async def _handle_script_room_op(self, ws_op: str, message: dict): data = message.get("DATA") if not isinstance(data, dict): + get_logger().warning(f"{ws_op}: DATA is not an object; treating as empty") data = {} # Everything that touches the shared draft only exists in collaborative mode; diff --git a/server/test/controllers/test_ws_controller.py b/server/test/controllers/test_ws_controller.py index c889aa5c..6ccb4912 100644 --- a/server/test/controllers/test_ws_controller.py +++ b/server/test/controllers/test_ws_controller.py @@ -3496,14 +3496,35 @@ def latest(): await ws_l.write_message( json.dumps({"OP": "SCRIPT_SCROLL", "DATA": {"current_line": ["x"]}}) ) - await ws_l.write_message( - json.dumps({"OP": "BEGIN_INTERVAL", "DATA": {"actId": "a", "length": 1}}) - ) + for bad_interval in ( + {"actId": "a", "length": 1}, + {"actId": True, "length": 1}, + {"actId": 1, "length": True}, + ): + await ws_l.write_message( + json.dumps({"OP": "BEGIN_INTERVAL", "DATA": bad_interval}) + ) await ws_l.write_message(json.dumps({"OP": "LIVE_SHOW_JUMP_TO_PAGE"})) await self._still_responsive(ws_l, self.admin_id) self.assertEqual(before, latest()) + with self._app.get_db().sessionmaker() as session: + self.assertEqual([], session.scalars(select(Interval)).all()) ws_l.close() + @gen_test + async def test_malformed_frame_content_logging_follows_redaction_setting(self): + ws, _ = await self._connect_and_auth(self.admin_id) + for redaction, expect_raw in ((True, False), (False, True)): + self._app.digi_settings.settings["log_redaction"].set_value(redaction) + with self.assertLogs(get_logger(), level="WARNING") as captured: + await ws.write_message('{"NOOP": "SENTINEL-FRAME"}') + await self._still_responsive(ws, self.admin_id) + get_logger().warning("marker") + joined = "\n".join(captured.output) + self.assertIn("Malformed WS message", joined) + self.assertEqual(expect_raw, "SENTINEL-FRAME" in joined) + ws.close() + @gen_test async def test_non_object_and_non_string_op_frames_are_ignored(self): ws, _ = await self._connect_and_auth(self.admin_id) diff --git a/server/test/digi_server/test_settings.py b/server/test/digi_server/test_settings.py index 7c8267af..aa2dbd39 100644 --- a/server/test/digi_server/test_settings.py +++ b/server/test/digi_server/test_settings.py @@ -1,3 +1,5 @@ +import json + from test.conftest import DigiScriptTestCase @@ -9,3 +11,24 @@ def test_log_redaction_enabled_by_default(self): setting = self._app.digi_settings.settings["log_redaction"] self.assertTrue(setting.default) self.assertTrue(setting.get_value()) + + +class TestSavedLoggingSettingsAreKept(DigiScriptTestCase): + """A value already saved in the settings file wins over a changed default.""" + + def get_app(self): + with open(self.settings_path, encoding="UTF-8") as file_pointer: + saved = json.load(file_pointer) + saved["log_redaction"] = False + saved["log_level"] = "DEBUG" + with open(self.settings_path, "w", encoding="UTF-8") as file_pointer: + json.dump(saved, file_pointer) + return super().get_app() + + def test_saved_log_redaction_is_not_overridden(self): + setting = self._app.digi_settings.settings["log_redaction"] + self.assertFalse(setting.get_value()) + + def test_saved_log_level_is_not_overridden(self): + setting = self._app.digi_settings.settings["log_level"] + self.assertEqual("DEBUG", setting.get_value()) diff --git a/server/test/utils/web/test_base_controller.py b/server/test/utils/web/test_base_controller.py index 82624e34..730726c9 100644 --- a/server/test/utils/web/test_base_controller.py +++ b/server/test/utils/web/test_base_controller.py @@ -52,3 +52,29 @@ async def test_non_json_body_follows_setting(self): ) self._set_redaction(False) self.assertIn("SENTINEL-RAW", await self._post_login("password=SENTINEL-RAW")) + + @gen_test + async def test_decorated_path_absent_from_body_still_logs_redacted(self): + """A body missing a decorated redaction path (optional old_password) must + still be logged, with its other secrets redacted, and must not error. + """ + self._set_redaction(True) + request = HTTPRequest( + self.get_url("/api/v1/auth/change-password"), + method="PATCH", + body=json.dumps({"new_password": "SENTINEL-NEW"}), + ) + with self.assertLogs(level="DEBUG") as captured: + await self.http_client.fetch(request, raise_error=False) + joined = "\n".join(captured.output) + self.assertIn("/api/v1/auth/change-password", joined) + self.assertNotIn("SENTINEL-NEW", joined) + self.assertNotIn("Uncaught exception", joined) + + @gen_test + async def test_confirm_password_redacted_without_decorator(self): + self._set_redaction(True) + joined = await self._post_login( + json.dumps({"username": "x", "confirmPassword": "SENTINEL-CONFIRM"}) + ) + self.assertNotIn("SENTINEL-CONFIRM", joined) diff --git a/server/utils/web/base_controller.py b/server/utils/web/base_controller.py index b5cf81a3..e38a7403 100644 --- a/server/utils/web/base_controller.py +++ b/server/utils/web/base_controller.py @@ -4,6 +4,7 @@ from typing import TYPE_CHECKING, Any, Awaitable, Optional import bcrypt +from jsonpath import JSONPatchError from sqlalchemy import select from tornado import escape, httputil from tornado.ioloop import IOLoop @@ -196,9 +197,14 @@ def _loggable_body(self, redacted_data_paths) -> Any: return _NON_JSON if self._redaction_enabled(): body = deepcopy(body) - if redacted_data_paths: - redacted_data_paths.apply(body) body = redact_sensitive_keys(body) + if redacted_data_paths: + try: + redacted_data_paths.apply(body) + except JSONPatchError: + # A decorated path is absent from this body (e.g. the optional + # old_password); there is nothing at that path to redact. + pass return body def on_finish(self): diff --git a/server/utils/web/log_redaction.py b/server/utils/web/log_redaction.py index 01733751..9bbd21aa 100644 --- a/server/utils/web/log_redaction.py +++ b/server/utils/web/log_redaction.py @@ -11,6 +11,8 @@ SENSITIVE_KEYS = frozenset( { "password", + "confirmpassword", + "confirm_password", "new_password", "old_password", "current_password", From 47b5cef0de99bdd1aae3f6867e55f24c8a914df2 Mon Sep 17 00:00:00 2001 From: Tim Bradgate Date: Tue, 20 Oct 2026 23:51:50 +0100 Subject: [PATCH 08/11] Apply decorated redaction paths independently and only to dict/list bodies Co-Authored-By: Claude Sonnet 5.5 --- server/test/utils/web/test_base_controller.py | 10 ++++++++ server/test/utils/web/test_log_redaction.py | 25 ++++++++++++++++++- server/utils/web/base_controller.py | 10 ++------ server/utils/web/log_redaction.py | 24 ++++++++++++++++++ 4 files changed, 60 insertions(+), 9 deletions(-) diff --git a/server/test/utils/web/test_base_controller.py b/server/test/utils/web/test_base_controller.py index 730726c9..ca8c72f9 100644 --- a/server/test/utils/web/test_base_controller.py +++ b/server/test/utils/web/test_base_controller.py @@ -71,6 +71,16 @@ async def test_decorated_path_absent_from_body_still_logs_redacted(self): self.assertNotIn("SENTINEL-NEW", joined) self.assertNotIn("Uncaught exception", joined) + @gen_test + async def test_json_string_body_does_not_break_logging(self): + """A body that decodes to a bare JSON string is still logged and does not + raise in on_finish (the handler's own rejection of it is separate). + """ + self._set_redaction(True) + joined = await self._post_login('"{"') + self.assertIn("DEBUG:DigiScript:POST /api/v1/auth/login", joined) + self.assertNotIn("on_finish", joined) + @gen_test async def test_confirm_password_redacted_without_decorator(self): self._set_redaction(True) diff --git a/server/test/utils/web/test_log_redaction.py b/server/test/utils/web/test_log_redaction.py index 0620b0ee..a4e5fdc4 100644 --- a/server/test/utils/web/test_log_redaction.py +++ b/server/test/utils/web/test_log_redaction.py @@ -1,6 +1,12 @@ import unittest -from utils.web.log_redaction import REDACTED, redact_sensitive_keys +from jsonpath import JSONPatch + +from utils.web.log_redaction import ( + REDACTED, + apply_path_redactions, + redact_sensitive_keys, +) class TestRedactSensitiveKeys(unittest.TestCase): @@ -27,3 +33,20 @@ def test_does_not_mutate_input(self): def test_scalars_pass_through(self): self.assertEqual("x", redact_sensitive_keys("x")) self.assertIsNone(redact_sensitive_keys(None)) + + +class TestApplyPathRedactions(unittest.TestCase): + def _patch(self, *paths): + patch = JSONPatch() + for path in paths: + patch.replace(path, REDACTED) + return patch + + def test_missing_path_does_not_stop_later_paths(self): + body = {"new_password": "secret"} + apply_path_redactions(self._patch("/old_password", "/new_password"), body) + self.assertEqual({"new_password": REDACTED}, body) + + def test_non_container_body_is_ignored(self): + for body in ("{", 5, None): + apply_path_redactions(self._patch("/password"), body) diff --git a/server/utils/web/base_controller.py b/server/utils/web/base_controller.py index e38a7403..713ef4e1 100644 --- a/server/utils/web/base_controller.py +++ b/server/utils/web/base_controller.py @@ -4,7 +4,6 @@ from typing import TYPE_CHECKING, Any, Awaitable, Optional import bcrypt -from jsonpath import JSONPatchError from sqlalchemy import select from tornado import escape, httputil from tornado.ioloop import IOLoop @@ -16,7 +15,7 @@ from models.user import User from rbac.role import Role from schemas.schemas import ShowSchema, UserSchema -from utils.web.log_redaction import redact_sensitive_keys +from utils.web.log_redaction import apply_path_redactions, redact_sensitive_keys if TYPE_CHECKING: @@ -199,12 +198,7 @@ def _loggable_body(self, redacted_data_paths) -> Any: body = deepcopy(body) body = redact_sensitive_keys(body) if redacted_data_paths: - try: - redacted_data_paths.apply(body) - except JSONPatchError: - # A decorated path is absent from this body (e.g. the optional - # old_password); there is nothing at that path to redact. - pass + apply_path_redactions(redacted_data_paths, body) return body def on_finish(self): diff --git a/server/utils/web/log_redaction.py b/server/utils/web/log_redaction.py index 9bbd21aa..3db8f523 100644 --- a/server/utils/web/log_redaction.py +++ b/server/utils/web/log_redaction.py @@ -4,6 +4,8 @@ from typing import Any +from jsonpath import JSONPatch, JSONPatchError + REDACTED = "<-- REDACTED -->" @@ -46,3 +48,25 @@ def redact_sensitive_keys(value: Any) -> Any: if isinstance(value, list): return [redact_sensitive_keys(item) for item in value] return value + + +def apply_path_redactions(patch: JSONPatch, body: Any) -> None: + """Apply each operation of a redaction *patch* to *body* independently, in place. + + A path absent from the body is skipped without stopping the remaining paths + (``JSONPatch.apply`` would abort at the first failure). Bodies that are not a + dict or list are left alone: ``JSONPatch`` would treat a string as serialized + JSON and raise on it. + + :param patch: Patch of ``replace`` operations built by ``redact_data_paths``. + :param body: Decoded JSON request body, modified in place. + """ + if not isinstance(body, (dict, list)): + return + for operation in patch.ops: + single = JSONPatch() + single.ops.append(operation) + try: + single.apply(body) + except JSONPatchError: + continue From bbfb0c4a5a8ce6b2fb02231c4bb050ecebe9ddec Mon Sep 17 00:00:00 2001 From: Tim Bradgate Date: Tue, 20 Oct 2026 23:55:58 +0100 Subject: [PATCH 09/11] Test bare JSON string login body returns 400 Co-Authored-By: Claude Sonnet 5.5 --- server/test/controllers/api/v1/auth/test_auth.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/server/test/controllers/api/v1/auth/test_auth.py b/server/test/controllers/api/v1/auth/test_auth.py index 7fc23457..7125233d 100644 --- a/server/test/controllers/api/v1/auth/test_auth.py +++ b/server/test/controllers/api/v1/auth/test_auth.py @@ -1267,6 +1267,9 @@ def test_login_invalid_json_returns_400(self): def test_login_non_object_json_returns_400(self): self.assertEqual(400, self._post("/api/v1/auth/login", "[1, 2]").code) + def test_login_bare_json_string_returns_400(self): + self.assertEqual(400, self._post("/api/v1/auth/login", '"secret"').code) + def test_login_empty_body_returns_400(self): self.assertEqual(400, self._post("/api/v1/auth/login", "").code) From 6616d4f7febba82bc4099adc72b05ae974b6c8b3 Mon Sep 17 00:00:00 2001 From: Tim Bradgate Date: Wed, 21 Oct 2026 00:14:35 +0100 Subject: [PATCH 10/11] Share RBAC grant/revoke request handling to remove duplication Co-Authored-By: Claude Sonnet 5.5 --- server/controllers/api/v1/rbac.py | 69 +++++++++++-------------------- 1 file changed, 24 insertions(+), 45 deletions(-) diff --git a/server/controllers/api/v1/rbac.py b/server/controllers/api/v1/rbac.py index 19c8f683..11836809 100644 --- a/server/controllers/api/v1/rbac.py +++ b/server/controllers/api/v1/rbac.py @@ -158,8 +158,18 @@ async def get(self): await self.finish({"roles": res}) -@ApiRoute("rbac/user/roles/grant", ApiVersion.V1) -class RBACRolesGrantHandler(BaseAPIController): +class _RBACRoleChangeHandler(BaseAPIController): + """Shared request handling for granting and revoking a role.""" + + def _apply_role(self, user: User, rbac_object, role: Role) -> None: + """Apply the role change. + + :param user: The user whose role is changing. + :param rbac_object: The resource object the role applies to. + :param role: The role to grant or revoke. + """ + raise NotImplementedError + @api_authenticated @require_admin async def post(self): @@ -178,11 +188,9 @@ async def post(self): self.set_status(404) await self.finish({"message": "user not found"}) return - resource = inspect(mapper) - cols = {} - cols.update( - {col.key: rbac_object.get(col.key) for col in resource.primary_key} - ) + cols = { + col.key: rbac_object.get(col.key) for col in inspect(mapper).primary_key + } if any(v is None for v in cols.values()): self.set_status(400) await self.finish({"message": "object is missing primary key fields"}) @@ -192,49 +200,20 @@ async def post(self): self.set_status(404) await self.finish({"message": "object not found"}) return - self.application.rbac.give_role(user, rbac_object, role) + self._apply_role(user, rbac_object, role) for socket in self.application.get_all_ws(user.id): await socket.write_message( {"OP": "NOOP", "DATA": {}, "ACTION": "GET_CURRENT_RBAC"} ) -@ApiRoute("rbac/user/roles/revoke", ApiVersion.V1) -class RBACRolesRevokeHandler(BaseAPIController): - @api_authenticated - @require_admin - async def post(self): - data = get_json_object(self) - resource, rbac_object, user, role = _parse_role_change(data) +@ApiRoute("rbac/user/roles/grant", ApiVersion.V1) +class RBACRolesGrantHandler(_RBACRoleChangeHandler): + def _apply_role(self, user, rbac_object, role): + self.application.rbac.give_role(user, rbac_object, role) - mapper = self.application.get_db().get_mapper_for_table(resource) - if not mapper: - self.set_status(404) - await self.finish({"message": "resource not found"}) - return - with self.make_session() as session: - user = session.get(User, user) - if not user: - self.set_status(404) - await self.finish({"message": "user not found"}) - return - resource = inspect(mapper) - cols = {} - cols.update( - {col.key: rbac_object.get(col.key) for col in resource.primary_key} - ) - if any(v is None for v in cols.values()): - self.set_status(400) - await self.finish({"message": "object is missing primary key fields"}) - return - rbac_object = session.get(mapper, cols) - if not rbac_object: - self.set_status(404) - await self.finish({"message": "object not found"}) - return - self.application.rbac.revoke_role(user, rbac_object, role) - for socket in self.application.get_all_ws(user.id): - await socket.write_message( - {"OP": "NOOP", "DATA": {}, "ACTION": "GET_CURRENT_RBAC"} - ) +@ApiRoute("rbac/user/roles/revoke", ApiVersion.V1) +class RBACRolesRevokeHandler(_RBACRoleChangeHandler): + def _apply_role(self, user, rbac_object, role): + self.application.rbac.revoke_role(user, rbac_object, role) From dc2d20370da0a0a3eb7731d7aa68ef6fa0fde0cb Mon Sep 17 00:00:00 2001 From: Tim Bradgate Date: Wed, 21 Oct 2026 00:25:30 +0100 Subject: [PATCH 11/11] Send security headers on static routes; tighten user settings allow-list test Co-Authored-By: Claude Sonnet 5.5 --- server/digi_server/app_server.py | 11 +++-- .../controllers/api/v1/user/test_settings.py | 45 ++++++++++++++----- .../test/utils/web/test_security_headers.py | 38 ++++++++++++++++ server/utils/web/base_controller.py | 11 ++--- server/utils/web/security_headers.py | 29 ++++++++++++ 5 files changed, 110 insertions(+), 24 deletions(-) create mode 100644 server/test/utils/web/test_security_headers.py create mode 100644 server/utils/web/security_headers.py diff --git a/server/digi_server/app_server.py b/server/digi_server/app_server.py index 6eefcd6b..f8c59cb6 100644 --- a/server/digi_server/app_server.py +++ b/server/digi_server/app_server.py @@ -12,7 +12,7 @@ from alembic.runtime import migration from sqlalchemy import Column, String, delete, select from tornado.ioloop import IOLoop -from tornado.web import Application, StaticFileHandler +from tornado.web import Application from tornado_prometheus import PrometheusMixIn from controllers import controllers @@ -46,6 +46,7 @@ from utils.web.pending_disconnects import PendingDisconnects from utils.web.request_size_metrics import observe_http_request from utils.web.route import Route +from utils.web.security_headers import SecureStaticFileHandler class DigiScriptServer(PrometheusMixIn, Application): @@ -316,16 +317,18 @@ class AlembicVersion(self._db.Model): handlers = Route.routes() handlers.append(("/favicon.ico", controllers.StaticController)) handlers.append( - (r"/assets/(.*)", StaticFileHandler, {"path": static_files_path}) + (r"/assets/(.*)", SecureStaticFileHandler, {"path": static_files_path}) ) handlers.append( ( r"/ui-old/assets/(.*)", - StaticFileHandler, + SecureStaticFileHandler, {"path": ui_old_static_files_path}, ) ) - handlers.append((r"/docs/(.*)", StaticFileHandler, {"path": docs_files_path})) + handlers.append( + (r"/docs/(.*)", SecureStaticFileHandler, {"path": docs_files_path}) + ) handlers.append((r"/api/.*", controllers.ApiFallback)) handlers.append((r"/ui-old(/?.*)", controllers.RootController)) handlers.append((r"/(.*)", controllers.RootControllerV3)) diff --git a/server/test/controllers/api/v1/user/test_settings.py b/server/test/controllers/api/v1/user/test_settings.py index e1066aea..64900c6a 100644 --- a/server/test/controllers/api/v1/user/test_settings.py +++ b/server/test/controllers/api/v1/user/test_settings.py @@ -1,8 +1,11 @@ """Tests for the /api/v1/user/settings controller.""" +from unittest.mock import patch + +from sqlalchemy import select from tornado import escape -from models.user import UserSettings +from models.user import User, UserSettings from test.conftest import DigiScriptTestCase @@ -26,19 +29,37 @@ def test_patch_invalid_json_returns_400(self): def test_patch_sets_allowed_and_ignores_others(self): token = self._create_and_login_admin() - resp = self._patch( - { - "cue_position_right": True, - "_user_id": 999, - "metadata": "x", - "registry": "x", - "nonexistent": 1, - }, - token, + # Create the settings row first so its constructor is not recorded below. + self.fetch( + "/api/v1/user/settings", headers={"Authorization": f"Bearer {token}"} ) + assigned = [] + original_setattr = UserSettings.__setattr__ + + def recording_setattr(obj, name, value): + assigned.append(name) + original_setattr(obj, name, value) + + with patch.object(UserSettings, "__setattr__", recording_setattr): + resp = self._patch( + { + "cue_position_right": True, + "_user_id": 999, + "metadata": "x", + "registry": "x", + "nonexistent": 1, + }, + token, + ) self.assertEqual(200, resp.code) + # Only the allow-listed field is ever assigned; existing non-column + # attributes (metadata, registry) and protected/unknown keys are skipped. + for name in ("metadata", "registry", "_user_id", "nonexistent"): + self.assertNotIn(name, assigned) + self.assertIn("cue_position_right", assigned) + with self._app.get_db().sessionmaker() as session: - settings = session.scalars(session.query(UserSettings).statement).first() + admin = session.scalars(select(User).where(User.username == "admin")).one() + settings = session.get(UserSettings, admin.id) self.assertTrue(settings.cue_position_right) - self.assertNotEqual(999, settings._user_id) diff --git a/server/test/utils/web/test_security_headers.py b/server/test/utils/web/test_security_headers.py new file mode 100644 index 00000000..d301357d --- /dev/null +++ b/server/test/utils/web/test_security_headers.py @@ -0,0 +1,38 @@ +"""Tests for utils/web/security_headers.py.""" + +import os +import tempfile + +from tornado.testing import AsyncHTTPTestCase +from tornado.web import Application + +from utils.web.security_headers import SecureStaticFileHandler + + +class TestSecureStaticFileHandler(AsyncHTTPTestCase): + def setUp(self): + self._dir = tempfile.TemporaryDirectory() + with open(os.path.join(self._dir.name, "page.html"), "w") as f: + f.write("

hi

") + super().setUp() + + def tearDown(self): + super().tearDown() + self._dir.cleanup() + + def get_app(self): + return Application( + [(r"/docs/(.*)", SecureStaticFileHandler, {"path": self._dir.name})] + ) + + def test_static_file_response_has_security_headers(self): + resp = self.fetch("/docs/page.html") + self.assertEqual(200, resp.code) + self.assertEqual("nosniff", resp.headers.get("X-Content-Type-Options")) + self.assertEqual("SAMEORIGIN", resp.headers.get("X-Frame-Options")) + self.assertEqual("same-origin", resp.headers.get("Referrer-Policy")) + + def test_static_not_found_response_has_security_headers(self): + resp = self.fetch("/docs/missing.html") + self.assertEqual(404, resp.code) + self.assertEqual("nosniff", resp.headers.get("X-Content-Type-Options")) diff --git a/server/utils/web/base_controller.py b/server/utils/web/base_controller.py index e6f07234..48232581 100644 --- a/server/utils/web/base_controller.py +++ b/server/utils/web/base_controller.py @@ -16,6 +16,7 @@ from rbac.role import Role from schemas.schemas import ShowSchema, UserSchema from utils.web.log_redaction import apply_path_redactions, redact_sensitive_keys +from utils.web.security_headers import apply_security_headers if TYPE_CHECKING: @@ -50,14 +51,8 @@ def __init__( self.current_show: Optional[dict] = None def set_default_headers(self) -> None: - """Add baseline security headers to every response. - - No Content-Security-Policy is set: the SPA and the bundled docs rely on - inline styles/scripts, so a policy strict enough to be useful would break them. - """ - self.set_header("X-Content-Type-Options", "nosniff") - self.set_header("X-Frame-Options", "SAMEORIGIN") - self.set_header("Referrer-Policy", "same-origin") + """Add the baseline security headers to every response.""" + apply_security_headers(self) async def prepare( self, diff --git a/server/utils/web/security_headers.py b/server/utils/web/security_headers.py new file mode 100644 index 00000000..4ee88a32 --- /dev/null +++ b/server/utils/web/security_headers.py @@ -0,0 +1,29 @@ +from __future__ import annotations + +from tornado.web import RequestHandler, StaticFileHandler + + +SECURITY_HEADERS = { + "X-Content-Type-Options": "nosniff", + "X-Frame-Options": "SAMEORIGIN", + "Referrer-Policy": "same-origin", +} + + +def apply_security_headers(handler: RequestHandler) -> None: + """Set the baseline security headers on a handler's response. + + No Content-Security-Policy is set: the SPA and the bundled docs rely on + inline styles/scripts, so a policy strict enough to be useful would break them. + + :param handler: The request handler whose response should carry the headers. + """ + for name, value in SECURITY_HEADERS.items(): + handler.set_header(name, value) + + +class SecureStaticFileHandler(StaticFileHandler): + """A ``StaticFileHandler`` that also sends the baseline security headers.""" + + def set_default_headers(self) -> None: + apply_security_headers(self)