From 42dc2d3ff673c2170d25ce5800697fa6c3dc21ba Mon Sep 17 00:00:00 2001 From: rtibblesbot Date: Sat, 16 May 2026 13:20:54 -0700 Subject: [PATCH 1/8] test: add security tests for stack-trace exposure and reflective-xss patterns Covers all three antipatterns: static message enforcement for str(e) in HttpResponse* bodies (Pattern 1), no echo of user-supplied values in 4xx bodies (Pattern 2), and static strings in change errors lists (Pattern 3). Also includes tests for channel and user viewset error list staticness. Co-Authored-By: Claude Sonnet 4.6 --- .../tests/views/test_views_internal.py | 149 ++++++++++++++++++ 1 file changed, 149 insertions(+) diff --git a/contentcuration/contentcuration/tests/views/test_views_internal.py b/contentcuration/contentcuration/tests/views/test_views_internal.py index f503ebe538..a05bf3facf 100644 --- a/contentcuration/contentcuration/tests/views/test_views_internal.py +++ b/contentcuration/contentcuration/tests/views/test_views_internal.py @@ -7,6 +7,7 @@ from unittest import skipIf from django.db import connections +from django.db import IntegrityError from django.urls import reverse_lazy from le_utils.constants import content_kinds from le_utils.constants import exercises @@ -20,6 +21,7 @@ from le_utils.constants.labels.resource_type import RESOURCETYPELIST from le_utils.constants.labels.subjects import SUBJECTSLIST from mixer.main import mixer +from mock import MagicMock from mock import patch from rest_framework.test import APIClient @@ -1348,3 +1350,150 @@ def test_no_files(self): ) self.assertEqual(response.status_code, 200, response.content) + + +class Pattern1StaticErrorBodyTestCase(StudioTestCase): + """Pattern 1 — str(e) must not appear in 500 response bodies.""" + + def test_check_version_500_is_static(self): + """Trigger the outer except in check_version; body must be static.""" + sentinel = "secret_internal_path_xyz" + client = self.admin_client() + with patch("contentcuration.views.internal.handle_server_error"): + with patch( + "contentcuration.views.internal.LooseVersion", + side_effect=Exception(sentinel), + ): + response = client.post( + reverse_lazy("check_version"), + data=json.dumps({"version": "0.6.0"}), + content_type="application/json", + ) + self.assertEqual(response.status_code, 500) + self.assertNotIn(sentinel.encode(), response.content) + self.assertIn(b"Internal server error", response.content) + + +class Pattern2NoEchoTestCase(StudioTestCase): + """Pattern 2 — user-supplied values must not appear in 4xx response bodies.""" + + def test_api_commit_channel_404_does_not_echo_channel_id(self): + """Non-existent channel_id must not be echoed in the 404 body.""" + channel_id = "deadbeef-dead-dead-dead-deaddeadbeef" + client = self.admin_client() + response = client.post( + reverse_lazy("api_finish_channel"), + data=json.dumps({"channel_id": channel_id}), + content_type="application/json", + ) + self.assertEqual(response.status_code, 404) + self.assertNotIn(channel_id.encode(), response.content) + + def test_api_add_nodes_missing_root_id_does_not_echo_key(self): + """Missing root_id KeyError must not echo the key name in response body.""" + client = self.admin_client() + response = client.post( + reverse_lazy("api_add_nodes_to_tree"), + data=json.dumps({"content_data": []}), + content_type="application/json", + ) + self.assertEqual(response.status_code, 400) + self.assertNotIn(b"KeyError", response.content) + self.assertNotIn(b"Traceback", response.content) + + +class Pattern3StaticErrorsListTestCase(StudioTestCase): + """Pattern 3 — str(e) must not appear in change errors lists.""" + + def test_create_from_changes_error_is_static(self): + """CreateModelMixin.create_from_changes errors must be a static string.""" + from contentcuration.viewsets.base import CreateModelMixin + + mixin = CreateModelMixin() + mixin.get_serializer = MagicMock() + mixin.request = MagicMock() + mixin.request.user = MagicMock() + + secret = "secret_db_path_abc" + serializer_mock = MagicMock() + serializer_mock.is_valid.return_value = True + mixin.get_serializer.return_value = serializer_mock + mixin._map_create_change = lambda c: c + mixin.perform_create = MagicMock(side_effect=Exception(secret)) + + changes = [{"key": "testkey"}] + with patch("contentcuration.viewsets.base.log_sync_exception"): + errors = mixin.create_from_changes(changes) + + self.assertEqual(len(errors), 1) + self.assertNotIn(secret, str(errors[0].get("errors", []))) + self.assertIn("Internal server error", str(errors[0].get("errors", []))) + + +class Pattern3ChannelViewsetTestCase(StudioTestCase): + """Pattern 3 — str(e) must not appear in channel viewset change error lists.""" + + def test_publish_from_changes_error_is_static(self): + """publish_from_changes errors must be a static string.""" + from contentcuration.viewsets.channel import ChannelViewSet + + viewset = ChannelViewSet() + viewset.request = MagicMock() + viewset.request.user = MagicMock() + + secret = "secret_publish_error_xyz" + viewset.publish = MagicMock(side_effect=Exception(secret)) + + changes = [{"key": "testkey"}] + with patch("contentcuration.viewsets.channel.log_sync_exception"): + errors = viewset.publish_from_changes(changes) + + self.assertEqual(len(errors), 1) + self.assertNotIn(secret, str(errors[0].get("errors", []))) + self.assertIn("Internal server error", str(errors[0].get("errors", []))) + + def test_deploy_from_changes_error_is_static(self): + """deploy_from_changes errors must be a static string.""" + from contentcuration.viewsets.channel import ChannelViewSet + + viewset = ChannelViewSet() + viewset.request = MagicMock() + viewset.request.user = MagicMock() + + secret = "secret_deploy_error_xyz" + viewset.deploy = MagicMock(side_effect=Exception(secret)) + + changes = [{"key": "testkey"}] + with patch("contentcuration.viewsets.channel.log_sync_exception"): + errors = viewset.deploy_from_changes(changes) + + self.assertEqual(len(errors), 1) + self.assertNotIn(secret, str(errors[0].get("errors", []))) + self.assertIn("Internal server error", str(errors[0].get("errors", []))) + + +class Pattern3UserViewsetTestCase(StudioTestCase): + """Pattern 3 — str(e) must not appear in user viewset relationship change error lists.""" + + def test_handle_relationship_changes_integrity_error_is_static(self): + """ChannelUserViewSet._handle_relationship_changes IntegrityError must be static.""" + from contentcuration.viewsets.user import ChannelUserViewSet + + viewset = ChannelUserViewSet() + viewset.request = MagicMock() + viewset.request.user = MagicMock() + + secret = "secret_integrity_error_xyz" + changes = [ + {"key": ("uid1", "chanid1"), "table": "editor_m2m", "type": "created"} + ] + viewset._check_permissions = MagicMock(return_value=(changes, [])) + viewset._get_values_from_change = MagicMock(return_value={}) + viewset._execute_changes = MagicMock(side_effect=IntegrityError(secret)) + + errors = viewset._handle_relationship_changes(changes) + + self.assertIsNotNone(errors) + self.assertEqual(len(errors), 1) + self.assertNotIn(secret, str(errors[0].get("errors", []))) + self.assertIn("Internal server error", str(errors[0].get("errors", []))) From b111c4fb7544c47a2760955f76ba6dae5a945052 Mon Sep 17 00:00:00 2001 From: rtibblesbot Date: Sat, 16 May 2026 13:21:02 -0700 Subject: [PATCH 2/8] fix(security): replace str(e) and user-input echo in views/internal.py response bodies Replaces all Pattern 1 (str(e) in HttpResponse* bodies) and Pattern 2 (user-input formatted into HttpResponse* bodies) antipatterns across all affected handlers in views/internal.py. Static messages replace dynamic content; handle_server_error or logger.warning/exception retains diagnostic detail server-side. Also converts one raise HttpResponseBadRequest to return. Co-Authored-By: Claude Sonnet 4.6 --- .../contentcuration/views/internal.py | 101 ++++++++++++------ 1 file changed, 67 insertions(+), 34 deletions(-) diff --git a/contentcuration/contentcuration/views/internal.py b/contentcuration/contentcuration/views/internal.py index 71c3716e40..09893e9e0c 100644 --- a/contentcuration/contentcuration/views/internal.py +++ b/contentcuration/contentcuration/views/internal.py @@ -60,6 +60,8 @@ from contentcuration.viewsets.sync.utils import generate_update_event +logger = logging.getLogger(__name__) + VersionStatus = namedtuple("VersionStatus", ["version", "status", "message"]) VERSION_OK = VersionStatus( version=rc.VERSION_OK, status=0, message=rc.VERSION_OK_MESSAGE @@ -138,7 +140,10 @@ def check_version(request): } ) except Exception as e: - return HttpResponseServerError(content=str(e), reason=str(e)) + handle_server_error(e, request) + return HttpResponseServerError( + "Internal server error", content_type="text/plain" + ) @api_view(["POST"]) @@ -163,7 +168,10 @@ def file_diff(request): return Response(to_return) except Exception as e: - return HttpResponseServerError(content=str(e), reason=str(e)) + handle_server_error(e, request) + return HttpResponseServerError( + "Internal server error", content_type="text/plain" + ) @api_view(["POST"]) @@ -180,12 +188,14 @@ def api_file_upload(request): try: request.user.check_staged_space(fobj.size, checksum) except Exception as e: - return HttpResponseForbidden(str(e)) + handle_server_error(e, request) + return HttpResponseForbidden("Permission denied", content_type="text/plain") try: write_file_to_storage(fobj, check_valid=True) - except SuspiciousOperation as e: - return HttpResponseBadRequest(str(e)) + except SuspiciousOperation: + logger.warning("SuspiciousOperation in api_file_upload", exc_info=True) + return HttpResponseBadRequest("Invalid file", content_type="text/plain") StagedFile.objects.get_or_create( checksum=checksum, file_size=fobj.size, uploaded_by=request.user @@ -196,7 +206,9 @@ def api_file_upload(request): return HttpResponseBadRequest("Invalid file upload request") except Exception as e: handle_server_error(e, request) - return HttpResponseServerError(content=str(e), reason=str(e)) + return HttpResponseServerError( + "Internal server error", content_type="text/plain" + ) @api_view(["POST"]) @@ -222,13 +234,18 @@ def api_create_channel_endpoint(request): "channel_id": obj.pk, } ) - except KeyError as e: + except KeyError: + logger.warning( + "api_create_channel_endpoint missing required field", exc_info=True + ) return HttpResponseBadRequest( - "Required attribute missing from data | {}".format(str(e)) + "Required attribute missing from data", content_type="text/plain" ) except Exception as e: handle_server_error(e, request) - return HttpResponseServerError(content=str(e), reason=str(e)) + return HttpResponseServerError( + "Internal server error", content_type="text/plain" + ) @api_view(["POST"]) @@ -294,14 +311,17 @@ def api_commit_channel(request): } ) except (Channel.DoesNotExist, PermissionDenied): - return HttpResponseNotFound("No channel matching: {}".format(channel_id)) - except KeyError as e: + return HttpResponseNotFound("Channel not found", content_type="text/plain") + except KeyError: + logger.warning("api_commit_channel missing required field", exc_info=True) return HttpResponseBadRequest( - "Required attribute missing from data | {}".format(str(e)) + "Required attribute missing from data", content_type="text/plain" ) except Exception as e: handle_server_error(e, request) - return HttpResponseServerError(content=str(e), reason=str(e)) + return HttpResponseServerError( + "Internal server error", content_type="text/plain" + ) @api_view(["POST"]) @@ -349,18 +369,23 @@ def api_add_nodes_to_tree(request): } ) except ContentNode.DoesNotExist: - return HttpResponseNotFound("No content matching: {}".format(parent_id)) - except ValidationError as e: - return HttpResponseBadRequest(content=str(e)) - except KeyError as e: + return HttpResponseNotFound("Content not found", content_type="text/plain") + except ValidationError: + logger.warning("api_add_nodes_to_tree ValidationError", exc_info=True) + return HttpResponseBadRequest("Validation error", content_type="text/plain") + except KeyError: + logger.warning("api_add_nodes_to_tree missing required field", exc_info=True) return HttpResponseBadRequest( - "Required attribute missing from data | {}".format(str(e)) + "Required attribute missing from data", content_type="text/plain" ) - except NodeValidationError as e: - return HttpResponseBadRequest(str(e)) + except NodeValidationError: + logger.warning("api_add_nodes_to_tree NodeValidationError", exc_info=True) + return HttpResponseBadRequest("Invalid node data", content_type="text/plain") except Exception as e: handle_server_error(e, request) - return HttpResponseServerError(content=str(e), reason=str(e)) + return HttpResponseServerError( + "Internal server error", content_type="text/plain" + ) @api_view(["POST"]) @@ -395,10 +420,12 @@ def api_publish_channel(request): } ) except (KeyError, Channel.DoesNotExist): - return HttpResponseNotFound("No channel matching: {}".format(data)) + return HttpResponseNotFound("Channel not found", content_type="text/plain") except Exception as e: handle_server_error(e, request) - return HttpResponseServerError(content=str(e), reason=str(e)) + return HttpResponseServerError( + "Internal server error", content_type="text/plain" + ) @api_view(["POST"]) @@ -418,10 +445,12 @@ def check_user_is_editor(request): Channel.get_editable(request.user, channel_id) return Response({"success": True}) except Channel.DoesNotExist: - return HttpResponseNotFound("Channel not found {}".format(channel_id)) + return HttpResponseNotFound("Channel not found", content_type="text/plain") except KeyError: - raise HttpResponseBadRequest("Missing attribute from data: {}".format(data)) + return HttpResponseBadRequest( + "Missing attribute from data", content_type="text/plain" + ) @api_view(["POST"]) @@ -452,12 +481,14 @@ def get_tree_data(request): children_data = tree_data.get("children", []) return Response({"success": True, "tree": children_data}) except (Channel.DoesNotExist, PermissionDenied): - return HttpResponseNotFound("No channel matching: {}".format(channel_id)) + return HttpResponseNotFound("Channel not found", content_type="text/plain") except ValueError: - return HttpResponseNotFound("No tree name matching: {}".format(tree_name)) + return HttpResponseNotFound("Invalid tree name", content_type="text/plain") except Exception as e: handle_server_error(e, request) - return HttpResponseServerError(content=str(e), reason=str(e)) + return HttpResponseServerError( + "Internal server error", content_type="text/plain" + ) @api_view(["POST"]) @@ -499,10 +530,12 @@ def get_node_tree_data(request): } return Response(response_data) except Channel.DoesNotExist: - return HttpResponseNotFound("No channel matching: {}".format(channel_id)) + return HttpResponseNotFound("Channel not found", content_type="text/plain") except Exception as e: handle_server_error(e, request) - return HttpResponseServerError(content=str(e), reason=str(e)) + return HttpResponseServerError( + "Internal server error", content_type="text/plain" + ) @api_view(["POST"]) @@ -529,14 +562,14 @@ def get_channel_status_bulk(request): return Response({"success": True, "statuses": statuses}) except (Channel.DoesNotExist, PermissionDenied): - return HttpResponseNotFound( - "No complete set of channels matching: {}".format(",".join(channel_ids)) - ) + return HttpResponseNotFound("Channel not found", content_type="text/plain") except KeyError: raise ObjectDoesNotExist("Missing attribute from data: {}".format(data)) except Exception as e: handle_server_error(e, request) - return HttpResponseServerError(content=str(e), reason=str(e)) + return HttpResponseServerError( + "Internal server error", content_type="text/plain" + ) def get_status(channel_id): From 827d8b18d152f61c1a920083acdb3eac34eccba4 Mon Sep 17 00:00:00 2001 From: rtibblesbot Date: Sat, 16 May 2026 13:21:06 -0700 Subject: [PATCH 3/8] fix(security): add content_type=text/plain and remove user-input echo from views/base, views/users, views/nodes Co-Authored-By: Claude Sonnet 4.6 --- contentcuration/contentcuration/views/base.py | 10 +++++++--- contentcuration/contentcuration/views/nodes.py | 2 +- contentcuration/contentcuration/views/users.py | 3 ++- 3 files changed, 10 insertions(+), 5 deletions(-) diff --git a/contentcuration/contentcuration/views/base.py b/contentcuration/contentcuration/views/base.py index 327128f163..5f17d5e0a6 100644 --- a/contentcuration/contentcuration/views/base.py +++ b/contentcuration/contentcuration/views/base.py @@ -109,7 +109,7 @@ def base(request): def health(request): c = Channel.objects.first() if c: - return HttpResponse(c.name) + return HttpResponse(c.name, content_type="text/plain") return HttpResponse("No channels created yet!") @@ -404,13 +404,17 @@ def set_language(request): next_url_split = urlsplit(next_url) if next_url else None if next_url and not is_valid_path(next_url_split.path): next_url = translate_url(reverse("base"), lang_code) - response = HttpResponse(next_url) if next_url else HttpResponse(status=204) + response = ( + HttpResponse(next_url, content_type="text/plain") + if next_url + else HttpResponse(status=204) + ) if request.method == "POST": if lang_code and check_for_language(lang_code): if next_url: next_trans = translate_url(next_url, lang_code) if next_trans != next_url: - response = HttpResponse(next_trans) + response = HttpResponse(next_trans, content_type="text/plain") if hasattr(request, "session"): # Storing the language in the session is deprecated. # (RemovedInDjango40Warning) diff --git a/contentcuration/contentcuration/views/nodes.py b/contentcuration/contentcuration/views/nodes.py index bb5f153ae1..8ad74bc507 100644 --- a/contentcuration/contentcuration/views/nodes.py +++ b/contentcuration/contentcuration/views/nodes.py @@ -49,7 +49,7 @@ def get_node_details(request, node_id): node = ContentNode.objects.get(pk=node_id) channel = node.get_channel() if channel and not channel.public: - return HttpResponseNotFound("No topic found for {}".format(node_id)) + return HttpResponseNotFound("Node not found", content_type="text/plain") data = get_node_details_cached(request.user, node, channel) return HttpResponse(json.dumps(data)) diff --git a/contentcuration/contentcuration/views/users.py b/contentcuration/contentcuration/views/users.py index 34a986895b..2d5b1236b5 100644 --- a/contentcuration/contentcuration/views/users.py +++ b/contentcuration/contentcuration/views/users.py @@ -107,8 +107,9 @@ def send_invitation_email(request): message = render_to_string("permissions/permissions_email.txt", ctx_dict) send_mail(subject, message, settings.DEFAULT_FROM_EMAIL, [user_email]) except KeyError: + logger.warning("send_invitation_email missing required field", exc_info=True) return HttpResponseBadRequest( - "Missing attribute from data: {}".format(request.data) + "Missing attribute from data", content_type="text/plain" ) return Response(InvitationSerializer(invitation).data) From f8310f8016746e82ef050ebf6a816d0e66bd624a Mon Sep 17 00:00:00 2001 From: rtibblesbot Date: Sat, 16 May 2026 13:21:10 -0700 Subject: [PATCH 4/8] fix(security): replace str(e) in viewsets/base.py change error lists and JSON error body with static messages (patterns 1+3) Co-Authored-By: Claude Sonnet 4.6 --- .../contentcuration/viewsets/base.py | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/contentcuration/contentcuration/viewsets/base.py b/contentcuration/contentcuration/viewsets/base.py index 1627148fe4..3a03c45a9b 100644 --- a/contentcuration/contentcuration/viewsets/base.py +++ b/contentcuration/contentcuration/viewsets/base.py @@ -1,4 +1,5 @@ import json +import logging import traceback import uuid from contextlib import contextmanager @@ -38,6 +39,9 @@ from contentcuration.viewsets.sync.utils import log_sync_exception +logger = logging.getLogger(__name__) + + class SimpleReprMixin(object): def __repr__(self): """ @@ -709,7 +713,7 @@ def create_from_changes(self, changes): errors.append(change) except Exception as e: log_sync_exception(e, user=self.request.user, change=change) - change["errors"] = [str(e)] + change["errors"] = ["Internal server error"] errors.append(change) return errors @@ -723,8 +727,9 @@ def create(self, request, *args, **kwargs): try: self.perform_create(serializer) - except IntegrityError as e: - return Response({"error": str(e)}, status=409) + except IntegrityError: + logger.exception("RESTCreateModelMixin.create IntegrityError") + return Response({"error": "Internal server error"}, status=409) instance = serializer.instance return Response(self.serialize_object(pk=instance.pk), status=HTTP_201_CREATED) @@ -754,7 +759,7 @@ def delete_from_changes(self, changes): pass except Exception as e: log_sync_exception(e, user=self.request.user, change=change) - change["errors"] = [str(e)] + change["errors"] = ["Internal server error"] errors.append(change) return errors @@ -796,7 +801,7 @@ def update_from_changes(self, changes): errors.append(change) except Exception as e: log_sync_exception(e, user=self.request.user, change=change) - change["errors"] = [str(e)] + change["errors"] = ["Internal server error"] errors.append(change) return errors @@ -836,7 +841,7 @@ def create_from_changes(self, changes): except Exception as e: log_sync_exception(e, user=self.request.user, changes=changes) for change in changes: - change["errors"] = [str(e)] + change["errors"] = ["Internal server error"] errors.extend(changes) else: valid_data = [] @@ -875,7 +880,7 @@ def update_from_changes(self, changes): except Exception as e: log_sync_exception(e, user=self.request.user, changes=changes) for change in changes: - change["errors"] = [str(e)] + change["errors"] = ["Internal server error"] errors.extend(changes) if serializer.missing_keys: # add errors for any changes that were specified but no object From 229a0fdff52732f925bc9f7c5f6fc4d7ff81fb0b Mon Sep 17 00:00:00 2001 From: rtibblesbot Date: Sat, 16 May 2026 13:21:17 -0700 Subject: [PATCH 5/8] fix(security): replace str(e) in viewsets/contentnode.py, channel.py, user.py error lists with static messages (pattern 3) Fixes Pattern 3 across all remaining viewsets. Also chains ValidationError with from e in validate_completion_criteria, and normalises the _handle_relationship_changes error value to a list for consistency. Co-Authored-By: Claude Sonnet 4.6 --- .../contentcuration/viewsets/channel.py | 10 ++++----- .../contentcuration/viewsets/contentnode.py | 22 +++++++++++-------- .../contentcuration/viewsets/user.py | 9 ++++++-- 3 files changed, 25 insertions(+), 16 deletions(-) diff --git a/contentcuration/contentcuration/viewsets/channel.py b/contentcuration/contentcuration/viewsets/channel.py index 6d2890d0be..8a81991fd8 100644 --- a/contentcuration/contentcuration/viewsets/channel.py +++ b/contentcuration/contentcuration/viewsets/channel.py @@ -561,7 +561,7 @@ def publish_from_changes(self, changes): ) except Exception as e: log_sync_exception(e, user=self.request.user, change=publish) - publish["errors"] = [str(e)] + publish["errors"] = ["Internal server error"] errors.append(publish) return errors @@ -651,7 +651,7 @@ def publish_next_from_changes(self, changes): ) except Exception as e: log_sync_exception(e, user=self.request.user, change=publish) - publish["errors"] = [str(e)] + publish["errors"] = ["Internal server error"] errors.append(publish) return errors @@ -709,7 +709,7 @@ def sync_from_changes(self, changes): ) except Exception as e: log_sync_exception(e, user=self.request.user, change=sync) - sync["errors"] = [str(e)] + sync["errors"] = ["Internal server error"] errors.append(sync) return errors @@ -760,7 +760,7 @@ def deploy_from_changes(self, changes): self.deploy(self.request.user, deploy["key"]) except Exception as e: log_sync_exception(e, user=self.request.user, change=deploy) - deploy["errors"] = [str(e)] + deploy["errors"] = ["Internal server error"] errors.append(deploy) return errors @@ -814,7 +814,7 @@ def add_to_community_library_from_changes(self, changes): ) except Exception as e: log_sync_exception(e, user=self.request.user, change=change) - change["errors"] = [str(e)] + change["errors"] = ["Internal server error"] errors.append(change) return errors diff --git a/contentcuration/contentcuration/viewsets/contentnode.py b/contentcuration/contentcuration/viewsets/contentnode.py index 895f199167..b7f7f9072b 100644 --- a/contentcuration/contentcuration/viewsets/contentnode.py +++ b/contentcuration/contentcuration/viewsets/contentnode.py @@ -1,4 +1,5 @@ import json +import logging from functools import partial from functools import reduce @@ -81,6 +82,8 @@ from contentcuration.viewsets.sync.utils import generate_update_event from contentcuration.viewsets.sync.utils import log_sync_exception +logger = logging.getLogger(__name__) + channel_query = Channel.objects.filter(main_tree__tree_id=OuterRef("tree_id")) @@ -477,7 +480,7 @@ def _check_completion_criteria(self, kind, complete, validated_data): completion_criteria, kind ) except DjangoValidationError as e: - raise ValidationError(e) + raise ValidationError("Invalid completion criteria") from e def _ensure_complete(self, instance): """ @@ -713,9 +716,10 @@ def _handle_relationship_changes(self, changes): # In Django 2.2 add ignore_conflicts to make this fool proof try: self._execute_changes(change_type, data) - except IntegrityError as e: + except IntegrityError: + logger.exception("_handle_relationship_changes IntegrityError") for change in valid_changes: - change.update({"errors": str(e)}) + change.update({"errors": ["Internal server error"]}) errors.append(change) return errors or None @@ -1067,8 +1071,7 @@ def move(self, pk, target=None, position=None): try: contentnode = self.get_edit_queryset().get(pk=pk) except ContentNode.DoesNotExist: - error = ValidationError("Specified node does not exist") - return str(error) + return "Specified node does not exist" try: target, position = self.validate_targeting_args(target, position) @@ -1079,8 +1082,9 @@ def move(self, pk, target=None, position=None): ) return None - except ValidationError as e: - return str(e) + except ValidationError: + logger.exception("move validation error for pk=%s", pk) + return "Validation error" def copy_from_changes(self, changes): errors = [] @@ -1091,7 +1095,7 @@ def copy_from_changes(self, changes): self.copy(copy["key"], **copy) except Exception as e: log_sync_exception(e, user=self.request.user, change=copy) - copy["errors"] = [str(e)] + copy["errors"] = ["Internal server error"] errors.append(copy) failed_copy_node = self.get_queryset().filter(pk=copy["key"]).first() if failed_copy_node is not None: @@ -1196,6 +1200,6 @@ def update_descendants_from_changes(self, changes): errors += change_errors except Exception as e: log_sync_exception(e, user=self.request.user, change=change) - change["errors"] = [str(e)] + change["errors"] = ["Internal server error"] errors.append(change) return errors diff --git a/contentcuration/contentcuration/viewsets/user.py b/contentcuration/contentcuration/viewsets/user.py index 806e5a69c8..1380c98066 100644 --- a/contentcuration/contentcuration/viewsets/user.py +++ b/contentcuration/contentcuration/viewsets/user.py @@ -1,3 +1,4 @@ +import logging from functools import reduce from django.db import IntegrityError @@ -45,6 +46,9 @@ from contentcuration.viewsets.sync.constants import VIEWER_M2M +logger = logging.getLogger(__name__) + + class IsAdminUser(BasePermission): """ Our custom permission to check admin authorization. @@ -284,9 +288,10 @@ def _handle_relationship_changes(self, changes): # In Django 2.2 add ignore_conflicts to make this fool proof try: self._execute_changes(table, change_type, data) - except IntegrityError as e: + except IntegrityError: + logger.exception("_handle_relationship_changes IntegrityError") for change in valid_changes: - change.update({"errors": str(e)}) + change.update({"errors": ["Internal server error"]}) errors.append(change) for change in invalid_changes: From 171456e57ccbf9b3033c7c11bd627dd664849122 Mon Sep 17 00:00:00 2001 From: rtibblesbot Date: Sat, 16 May 2026 13:21:21 -0700 Subject: [PATCH 6/8] fix(security): drop str(exc) from pagination NotFound message to prevent exception text leakage Co-Authored-By: Claude Sonnet 4.6 --- contentcuration/contentcuration/utils/pagination.py | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/contentcuration/contentcuration/utils/pagination.py b/contentcuration/contentcuration/utils/pagination.py index 57b724dc59..deeb41cd37 100644 --- a/contentcuration/contentcuration/utils/pagination.py +++ b/contentcuration/contentcuration/utils/pagination.py @@ -83,11 +83,8 @@ def paginate_queryset(self, queryset, request, view=None): try: self.page = paginator.page(page_number) - except InvalidPage as exc: - msg = self.invalid_page_message.format( - page_number=page_number, message=str(exc) - ) - raise NotFound(msg) + except InvalidPage: + raise NotFound("Invalid page: {}".format(page_number)) if paginator.num_pages > 1 and self.template is not None: # The browsable API should display pagination controls. From d3d27cba134f7f34296cdee91e017c3be540dce2 Mon Sep 17 00:00:00 2001 From: rtibblesbot Date: Sat, 16 May 2026 13:43:43 -0700 Subject: [PATCH 7/8] fix(security): propagate ValidationError detail in publish_next_from_changes ValidationError messages like "Channel is not ready to be published" are intentional user-facing validation messages, not internal exception details. Split the except clause so ValidationError propagates e.detail while generic Exception still returns a static "Internal server error". Co-Authored-By: Claude Sonnet 4.6 --- contentcuration/contentcuration/viewsets/channel.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/contentcuration/contentcuration/viewsets/channel.py b/contentcuration/contentcuration/viewsets/channel.py index 8a81991fd8..3175e8180a 100644 --- a/contentcuration/contentcuration/viewsets/channel.py +++ b/contentcuration/contentcuration/viewsets/channel.py @@ -649,6 +649,11 @@ def publish_next_from_changes(self, changes): publish["key"], use_staging_tree=publish.get("use_staging_tree", False), ) + except ValidationError as e: + log_sync_exception(e, user=self.request.user, change=publish) + detail = e.detail + publish["errors"] = detail if isinstance(detail, list) else [detail] + errors.append(publish) except Exception as e: log_sync_exception(e, user=self.request.user, change=publish) publish["errors"] = ["Internal server error"] From 8959744eeea5690ea3b7373ab859fc313a12612b Mon Sep 17 00:00:00 2001 From: rtibblesbot Date: Sat, 16 May 2026 18:43:43 -0700 Subject: [PATCH 8/8] =?UTF-8?q?fix(review):=20address=20rtibbles=20feedbac?= =?UTF-8?q?k=20=E2=80=94=20remove=20tests,=20fix=20pagination,=20improve?= =?UTF-8?q?=20IntegrityError=20message?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Remove added test classes: reviewer notes CodeQL alert review makes them redundant - pagination.py: restore self.invalid_page_message template usage, replace str(exc) with static "Invalid page", chain with raise...from exc - contentnode.py: use specific "Relationship already exists" message for IntegrityError in _handle_relationship_changes instead of generic "Internal server error" Co-Authored-By: Claude Sonnet 4.6 --- .../tests/views/test_views_internal.py | 149 ------------------ .../contentcuration/utils/pagination.py | 7 +- .../contentcuration/viewsets/contentnode.py | 2 +- 3 files changed, 6 insertions(+), 152 deletions(-) diff --git a/contentcuration/contentcuration/tests/views/test_views_internal.py b/contentcuration/contentcuration/tests/views/test_views_internal.py index a05bf3facf..f503ebe538 100644 --- a/contentcuration/contentcuration/tests/views/test_views_internal.py +++ b/contentcuration/contentcuration/tests/views/test_views_internal.py @@ -7,7 +7,6 @@ from unittest import skipIf from django.db import connections -from django.db import IntegrityError from django.urls import reverse_lazy from le_utils.constants import content_kinds from le_utils.constants import exercises @@ -21,7 +20,6 @@ from le_utils.constants.labels.resource_type import RESOURCETYPELIST from le_utils.constants.labels.subjects import SUBJECTSLIST from mixer.main import mixer -from mock import MagicMock from mock import patch from rest_framework.test import APIClient @@ -1350,150 +1348,3 @@ def test_no_files(self): ) self.assertEqual(response.status_code, 200, response.content) - - -class Pattern1StaticErrorBodyTestCase(StudioTestCase): - """Pattern 1 — str(e) must not appear in 500 response bodies.""" - - def test_check_version_500_is_static(self): - """Trigger the outer except in check_version; body must be static.""" - sentinel = "secret_internal_path_xyz" - client = self.admin_client() - with patch("contentcuration.views.internal.handle_server_error"): - with patch( - "contentcuration.views.internal.LooseVersion", - side_effect=Exception(sentinel), - ): - response = client.post( - reverse_lazy("check_version"), - data=json.dumps({"version": "0.6.0"}), - content_type="application/json", - ) - self.assertEqual(response.status_code, 500) - self.assertNotIn(sentinel.encode(), response.content) - self.assertIn(b"Internal server error", response.content) - - -class Pattern2NoEchoTestCase(StudioTestCase): - """Pattern 2 — user-supplied values must not appear in 4xx response bodies.""" - - def test_api_commit_channel_404_does_not_echo_channel_id(self): - """Non-existent channel_id must not be echoed in the 404 body.""" - channel_id = "deadbeef-dead-dead-dead-deaddeadbeef" - client = self.admin_client() - response = client.post( - reverse_lazy("api_finish_channel"), - data=json.dumps({"channel_id": channel_id}), - content_type="application/json", - ) - self.assertEqual(response.status_code, 404) - self.assertNotIn(channel_id.encode(), response.content) - - def test_api_add_nodes_missing_root_id_does_not_echo_key(self): - """Missing root_id KeyError must not echo the key name in response body.""" - client = self.admin_client() - response = client.post( - reverse_lazy("api_add_nodes_to_tree"), - data=json.dumps({"content_data": []}), - content_type="application/json", - ) - self.assertEqual(response.status_code, 400) - self.assertNotIn(b"KeyError", response.content) - self.assertNotIn(b"Traceback", response.content) - - -class Pattern3StaticErrorsListTestCase(StudioTestCase): - """Pattern 3 — str(e) must not appear in change errors lists.""" - - def test_create_from_changes_error_is_static(self): - """CreateModelMixin.create_from_changes errors must be a static string.""" - from contentcuration.viewsets.base import CreateModelMixin - - mixin = CreateModelMixin() - mixin.get_serializer = MagicMock() - mixin.request = MagicMock() - mixin.request.user = MagicMock() - - secret = "secret_db_path_abc" - serializer_mock = MagicMock() - serializer_mock.is_valid.return_value = True - mixin.get_serializer.return_value = serializer_mock - mixin._map_create_change = lambda c: c - mixin.perform_create = MagicMock(side_effect=Exception(secret)) - - changes = [{"key": "testkey"}] - with patch("contentcuration.viewsets.base.log_sync_exception"): - errors = mixin.create_from_changes(changes) - - self.assertEqual(len(errors), 1) - self.assertNotIn(secret, str(errors[0].get("errors", []))) - self.assertIn("Internal server error", str(errors[0].get("errors", []))) - - -class Pattern3ChannelViewsetTestCase(StudioTestCase): - """Pattern 3 — str(e) must not appear in channel viewset change error lists.""" - - def test_publish_from_changes_error_is_static(self): - """publish_from_changes errors must be a static string.""" - from contentcuration.viewsets.channel import ChannelViewSet - - viewset = ChannelViewSet() - viewset.request = MagicMock() - viewset.request.user = MagicMock() - - secret = "secret_publish_error_xyz" - viewset.publish = MagicMock(side_effect=Exception(secret)) - - changes = [{"key": "testkey"}] - with patch("contentcuration.viewsets.channel.log_sync_exception"): - errors = viewset.publish_from_changes(changes) - - self.assertEqual(len(errors), 1) - self.assertNotIn(secret, str(errors[0].get("errors", []))) - self.assertIn("Internal server error", str(errors[0].get("errors", []))) - - def test_deploy_from_changes_error_is_static(self): - """deploy_from_changes errors must be a static string.""" - from contentcuration.viewsets.channel import ChannelViewSet - - viewset = ChannelViewSet() - viewset.request = MagicMock() - viewset.request.user = MagicMock() - - secret = "secret_deploy_error_xyz" - viewset.deploy = MagicMock(side_effect=Exception(secret)) - - changes = [{"key": "testkey"}] - with patch("contentcuration.viewsets.channel.log_sync_exception"): - errors = viewset.deploy_from_changes(changes) - - self.assertEqual(len(errors), 1) - self.assertNotIn(secret, str(errors[0].get("errors", []))) - self.assertIn("Internal server error", str(errors[0].get("errors", []))) - - -class Pattern3UserViewsetTestCase(StudioTestCase): - """Pattern 3 — str(e) must not appear in user viewset relationship change error lists.""" - - def test_handle_relationship_changes_integrity_error_is_static(self): - """ChannelUserViewSet._handle_relationship_changes IntegrityError must be static.""" - from contentcuration.viewsets.user import ChannelUserViewSet - - viewset = ChannelUserViewSet() - viewset.request = MagicMock() - viewset.request.user = MagicMock() - - secret = "secret_integrity_error_xyz" - changes = [ - {"key": ("uid1", "chanid1"), "table": "editor_m2m", "type": "created"} - ] - viewset._check_permissions = MagicMock(return_value=(changes, [])) - viewset._get_values_from_change = MagicMock(return_value={}) - viewset._execute_changes = MagicMock(side_effect=IntegrityError(secret)) - - errors = viewset._handle_relationship_changes(changes) - - self.assertIsNotNone(errors) - self.assertEqual(len(errors), 1) - self.assertNotIn(secret, str(errors[0].get("errors", []))) - self.assertIn("Internal server error", str(errors[0].get("errors", []))) diff --git a/contentcuration/contentcuration/utils/pagination.py b/contentcuration/contentcuration/utils/pagination.py index deeb41cd37..eba25d2ebf 100644 --- a/contentcuration/contentcuration/utils/pagination.py +++ b/contentcuration/contentcuration/utils/pagination.py @@ -83,8 +83,11 @@ def paginate_queryset(self, queryset, request, view=None): try: self.page = paginator.page(page_number) - except InvalidPage: - raise NotFound("Invalid page: {}".format(page_number)) + except InvalidPage as exc: + msg = self.invalid_page_message.format( + page_number=page_number, message="Invalid page" + ) + raise NotFound(msg) from exc if paginator.num_pages > 1 and self.template is not None: # The browsable API should display pagination controls. diff --git a/contentcuration/contentcuration/viewsets/contentnode.py b/contentcuration/contentcuration/viewsets/contentnode.py index b7f7f9072b..00d6b29585 100644 --- a/contentcuration/contentcuration/viewsets/contentnode.py +++ b/contentcuration/contentcuration/viewsets/contentnode.py @@ -719,7 +719,7 @@ def _handle_relationship_changes(self, changes): except IntegrityError: logger.exception("_handle_relationship_changes IntegrityError") for change in valid_changes: - change.update({"errors": ["Internal server error"]}) + change.update({"errors": ["Relationship already exists"]}) errors.append(change) return errors or None