Security hygiene: JSON body helper, 403 for non-admin, RBAC validation, security headers

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:

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
@@ -120,33 +158,23 @@ 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):
-        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,80 +183,37 @@ 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"})
                 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"})
+                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._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 = 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
+@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, int(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}
-            )
-            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))
-            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)

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(

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)

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()