fix(security): replace str(e) and user-input echoes in HTTP responses with static messages by rtibblesbot · Pull Request #5918 · learningequality/studio

rtibblesbot · 2026-05-16T20:23:57Z

Summary

Fix CodeQL-flagged py/stack-trace-exposure and py/reflective-xss alerts across contentcuration/. Exception text and user-input echoes in HTTP response bodies are replaced with static messages; diagnostic detail is retained server-side via existing handle_server_error() or new logger.exception() calls.

Three patterns swept across 10 files (~25 sites in views/internal.py, smaller clusters in viewsets/base.py, viewsets/contentnode.py, viewsets/channel.py, viewsets/user.py, views/base.py, views/users.py, views/nodes.py, utils/pagination.py):

str(e) in HttpResponse* bodies → static "Internal server error" with content_type="text/plain"
User-input formatted into HttpResponse* bodies → static message with content_type="text/plain"
str(e) in change["errors"] lists in viewset mixins → "Internal server error"

References

Fixes #5916

Reviewer guidance

Three areas warrant careful review:

views/internal.py — wide sweep: Each of the ~25 except blocks was independently modified. Spot-check that every site retains either handle_server_error(...) or logger.warning/exception(...) before the static response.
views/internal.py:check_user_is_editor: A raise HttpResponseBadRequest(...) (raising a response object as an exception — a pre-existing bug) was converted to return. Verify the caller is not catching an exception here that now won't be raised.
viewsets/contentnode.py:move(): Return type changed from str(ValidationError(...)) to a plain string literal. The caller wraps the return value in change["errors"] = [move_error] — confirm the list-wrapping still works correctly.

AI usage

Implemented with Claude Code working from an autonomous implementation plan. Each commit was scoped to one pattern/file group.

@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly

How was this generated?

Ran pre-flight CI checks (lint, format, tests) and verified all pass
Rebased onto the target branch and resolved any conflicts
Reorganized commit history into clean, logical commits
Audited the diff to ensure only issue-relevant files are changed
Built PR body from the repository's PR template with evidence blocks

…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 <noreply@anthropic.com>

…y 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 <noreply@anthropic.com>

… from views/base, views/users, views/nodes Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…and JSON error body with static messages (patterns 1+3) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

… 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 <noreply@anthropic.com>

…ent exception text leakage Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…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 <noreply@anthropic.com>

rtibbles

This is looking mostly right - don't need the Python tests.

rtibbles · 2026-05-17T01:27:45Z

        self.assertEqual(response.status_code, 200, response.content)
+
+
+class Pattern1StaticErrorBodyTestCase(StudioTestCase):


These added tests seem pointless as long as we continue to review CodeQL alerts. please remove them.

Removed — all five test classes and their imports are gone.

rtibbles · 2026-05-17T01:30:37Z

-            )
-            raise NotFound(msg)
+        except InvalidPage:
+            raise NotFound("Invalid page: {}".format(page_number))


We should continue to use the self.invalid_message template here - we just need to change the message to not be the exception. We should also raise from the exc for the NotFound exception.

Fixed: restored self.invalid_page_message.format(page_number=page_number, message="Invalid page") and chained with raise NotFound(msg) from exc.

Fixed — kept self.invalid_page_message.format(page_number=page_number, message="Invalid page") and added raise NotFound(msg) from exc.

rtibbles · 2026-05-17T01:32:10Z

-        except SuspiciousOperation as e:
-            return HttpResponseBadRequest(str(e))
+        except SuspiciousOperation:
+            logger.warning("SuspiciousOperation in api_file_upload", exc_info=True)


I did not know about the exc_info kwarg, handy!

Glad it's useful — it's the standard way to include the traceback in a warning-level log without using logger.exception (which logs at ERROR).

rtibbles · 2026-05-17T01:35:51Z

            except Exception as e:
                log_sync_exception(e, user=self.request.user, change=change)
-                change["errors"] = [str(e)]
+                change["errors"] = ["Internal server error"]


For all of these exceptions, I think this is probably the right call - and actual serialization validation errors would not fall through to this.

Agreed — DRF ValidationError carries programmer-authored messages (not raw exception text), so propagating its detail is the right call for validation failures vs generic server errors.

rtibbles · 2026-05-17T01:37:07Z

+            logger.exception("_handle_relationship_changes IntegrityError")
            for change in valid_changes:
-                change.update({"errors": str(e)})
+                change.update({"errors": ["Internal server error"]})


Can we be a bit more specific when we have a specific class of error? We know this is because of trying to create something that already exists, usually?

Changed to "Relationship already exists" — more specific to the IntegrityError cause in this context.

Changed to "Relationship already exists" — IntegrityErrors in this path are duplicate relationship inserts.

…, improve IntegrityError message - 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 <noreply@anthropic.com>

rtibblesbot and others added 7 commits May 16, 2026 13:20

fix(security): add content_type=text/plain and remove user-input echo…

827d8b1

… from views/base, views/users, views/nodes Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

fix(security): replace str(e) in viewsets/base.py change error lists …

f8310f8

…and JSON error body with static messages (patterns 1+3) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

fix(security): drop str(exc) from pagination NotFound message to prev…

171456e

…ent exception text leakage Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

rtibblesbot marked this pull request as ready for review May 16, 2026 21:12

rtibbles requested changes May 17, 2026

View reviewed changes

		self.assertEqual(response.status_code, 200, response.content)


		class Pattern1StaticErrorBodyTestCase(StudioTestCase):

Uh oh!

Conversation

rtibblesbot commented May 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

References

Reviewer guidance

AI usage

Uh oh!

rtibbles left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rtibblesbot commented May 16, 2026 •

edited

Loading