diff --git a/server/controllers/api/v1/auth/user.py b/server/controllers/api/v1/auth/user.py index fe6999af9..bf7d64510 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 a36df59b8..118368098 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 @@ -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) diff --git a/server/controllers/api/v1/settings.py b/server/controllers/api/v1/settings.py index 94a8f984e..56fe472ec 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 c84e6cc91..63fac7b70 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 dbf3fdd57..3cddd3276 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 71860c35d..a583aeb28 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/digi_server/app_server.py b/server/digi_server/app_server.py index 6eefcd6ba..f8c59cb69 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/auth/test_auth.py b/server/test/controllers/api/v1/auth/test_auth.py index af589061e..7125233dd 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""" @@ -1249,3 +1249,34 @@ 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_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) + + 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 d83305066..61d33153d 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/show/test_shows.py b/server/test/controllers/api/v1/show/test_shows.py index e70df019a..58716411f 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/controllers/api/v1/test_db_backups.py b/server/test/controllers/api/v1/test_db_backups.py index 4aa53fec4..9827da3d1 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 974aa6f8e..c721a8d00 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_rbac.py b/server/test/controllers/api/v1/test_rbac.py index d99642b20..91982bd01 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_settings.py b/server/test/controllers/api/v1/test_settings.py index 64a9597c4..4d6f1778e 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/test_system_info.py b/server/test/controllers/api/v1/test_system_info.py index 3c89428db..2585140a4 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 aa49990bf..59ea545e6 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/v1/user/test_settings.py b/server/test/controllers/api/v1/user/test_settings.py new file mode 100644 index 000000000..64900c6aa --- /dev/null +++ b/server/test/controllers/api/v1/user/test_settings.py @@ -0,0 +1,65 @@ +"""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 User, 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() + # 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: + admin = session.scalars(select(User).where(User.username == "admin")).one() + settings = session.get(UserSettings, admin.id) + self.assertTrue(settings.cue_position_right) diff --git a/server/test/controllers/api/v2/test_users.py b/server/test/controllers/api/v2/test_users.py index decdbb720..1d22e0f07 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() @@ -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 index ca8c72f9d..a52641604 100644 --- a/server/test/utils/web/test_base_controller.py +++ b/server/test/utils/web/test_base_controller.py @@ -6,6 +6,19 @@ 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")) + + class TestRequestBodyLogging(DigiScriptTestCase): def _set_redaction(self, value): self._app.digi_settings.settings["log_redaction"].set_value(value) 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 000000000..279f354e2 --- /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_security_headers.py b/server/test/utils/web/test_security_headers.py new file mode 100644 index 000000000..d301357d8 --- /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/test/utils/web/test_web_decorators.py b/server/test/utils/web/test_web_decorators.py index b54b9ee20..660b6e7ca 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) diff --git a/server/utils/web/base_controller.py b/server/utils/web/base_controller.py index 713ef4e1f..482325813 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: @@ -49,6 +50,10 @@ def __init__( self.application: DigiScriptServer = self.application self.current_show: Optional[dict] = None + def set_default_headers(self) -> None: + """Add the baseline security headers to every response.""" + apply_security_headers(self) + 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 000000000..03cd2a59f --- /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/security_headers.py b/server/utils/web/security_headers.py new file mode 100644 index 000000000..4ee88a325 --- /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) diff --git a/server/utils/web/web_decorators.py b/server/utils/web/web_decorators.py index 295ce37be..db2f41e3f 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