Skip to content

Commit 94bb656

Browse files
rtibblesbotclaude
rtibblesbot
and
claude
authored
fix(security): replace str(e) and user-input echoes in HTTP responses with static messages (#5918)
* 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * fix(security): drop str(exc) from pagination NotFound message to prevent exception text leakage Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * fix(review): address rtibbles feedback — remove tests, fix pagination, 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> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent e3bbe30 commit 94bb656

9 files changed

Lines changed: 121 additions & 64 deletions

File tree

‎contentcuration/contentcuration/utils/pagination.py‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -85,9 +85,9 @@ def paginate_queryset(self, queryset, request, view=None):
8585
self.page = paginator.page(page_number)
8686
except InvalidPage as exc:
8787
msg = self.invalid_page_message.format(
88-
page_number=page_number, message=str(exc)
88+
page_number=page_number, message="Invalid page"
8989
)
90-
raise NotFound(msg)
90+
raise NotFound(msg) from exc
9191

9292
if paginator.num_pages > 1 and self.template is not None:
9393
# The browsable API should display pagination controls.

‎contentcuration/contentcuration/views/base.py‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -109,7 +109,7 @@ def base(request):
109109
def health(request):
110110
c = Channel.objects.first()
111111
if c:
112-
return HttpResponse(c.name)
112+
return HttpResponse(c.name, content_type="text/plain")
113113
return HttpResponse("No channels created yet!")
114114

115115

@@ -404,13 +404,17 @@ def set_language(request):
404404
next_url_split = urlsplit(next_url) if next_url else None
405405
if next_url and not is_valid_path(next_url_split.path):
406406
next_url = translate_url(reverse("base"), lang_code)
407-
response = HttpResponse(next_url) if next_url else HttpResponse(status=204)
407+
response = (
408+
HttpResponse(next_url, content_type="text/plain")
409+
if next_url
410+
else HttpResponse(status=204)
411+
)
408412
if request.method == "POST":
409413
if lang_code and check_for_language(lang_code):
410414
if next_url:
411415
next_trans = translate_url(next_url, lang_code)
412416
if next_trans != next_url:
413-
response = HttpResponse(next_trans)
417+
response = HttpResponse(next_trans, content_type="text/plain")
414418
if hasattr(request, "session"):
415419
# Storing the language in the session is deprecated.
416420
# (RemovedInDjango40Warning)

‎contentcuration/contentcuration/views/internal.py‎

Lines changed: 67 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,8 @@
6060
from contentcuration.viewsets.sync.utils import generate_update_event
6161

6262

63+
logger = logging.getLogger(__name__)
64+
6365
VersionStatus = namedtuple("VersionStatus", ["version", "status", "message"])
6466
VERSION_OK = VersionStatus(
6567
version=rc.VERSION_OK, status=0, message=rc.VERSION_OK_MESSAGE
@@ -138,7 +140,10 @@ def check_version(request):
138140
}
139141
)
140142
except Exception as e:
141-
return HttpResponseServerError(content=str(e), reason=str(e))
143+
handle_server_error(e, request)
144+
return HttpResponseServerError(
145+
"Internal server error", content_type="text/plain"
146+
)
142147

143148

144149
@api_view(["POST"])
@@ -163,7 +168,10 @@ def file_diff(request):
163168

164169
return Response(to_return)
165170
except Exception as e:
166-
return HttpResponseServerError(content=str(e), reason=str(e))
171+
handle_server_error(e, request)
172+
return HttpResponseServerError(
173+
"Internal server error", content_type="text/plain"
174+
)
167175

168176

169177
@api_view(["POST"])
@@ -180,12 +188,14 @@ def api_file_upload(request):
180188
try:
181189
request.user.check_staged_space(fobj.size, checksum)
182190
except Exception as e:
183-
return HttpResponseForbidden(str(e))
191+
handle_server_error(e, request)
192+
return HttpResponseForbidden("Permission denied", content_type="text/plain")
184193

185194
try:
186195
write_file_to_storage(fobj, check_valid=True)
187-
except SuspiciousOperation as e:
188-
return HttpResponseBadRequest(str(e))
196+
except SuspiciousOperation:
197+
logger.warning("SuspiciousOperation in api_file_upload", exc_info=True)
198+
return HttpResponseBadRequest("Invalid file", content_type="text/plain")
189199

190200
StagedFile.objects.get_or_create(
191201
checksum=checksum, file_size=fobj.size, uploaded_by=request.user
@@ -196,7 +206,9 @@ def api_file_upload(request):
196206
return HttpResponseBadRequest("Invalid file upload request")
197207
except Exception as e:
198208
handle_server_error(e, request)
199-
return HttpResponseServerError(content=str(e), reason=str(e))
209+
return HttpResponseServerError(
210+
"Internal server error", content_type="text/plain"
211+
)
200212

201213

202214
@api_view(["POST"])
@@ -222,13 +234,18 @@ def api_create_channel_endpoint(request):
222234
"channel_id": obj.pk,
223235
}
224236
)
225-
except KeyError as e:
237+
except KeyError:
238+
logger.warning(
239+
"api_create_channel_endpoint missing required field", exc_info=True
240+
)
226241
return HttpResponseBadRequest(
227-
"Required attribute missing from data | {}".format(str(e))
242+
"Required attribute missing from data", content_type="text/plain"
228243
)
229244
except Exception as e:
230245
handle_server_error(e, request)
231-
return HttpResponseServerError(content=str(e), reason=str(e))
246+
return HttpResponseServerError(
247+
"Internal server error", content_type="text/plain"
248+
)
232249

233250

234251
@api_view(["POST"])
@@ -294,14 +311,17 @@ def api_commit_channel(request):
294311
}
295312
)
296313
except (Channel.DoesNotExist, PermissionDenied):
297-
return HttpResponseNotFound("No channel matching: {}".format(channel_id))
298-
except KeyError as e:
314+
return HttpResponseNotFound("Channel not found", content_type="text/plain")
315+
except KeyError:
316+
logger.warning("api_commit_channel missing required field", exc_info=True)
299317
return HttpResponseBadRequest(
300-
"Required attribute missing from data | {}".format(str(e))
318+
"Required attribute missing from data", content_type="text/plain"
301319
)
302320
except Exception as e:
303321
handle_server_error(e, request)
304-
return HttpResponseServerError(content=str(e), reason=str(e))
322+
return HttpResponseServerError(
323+
"Internal server error", content_type="text/plain"
324+
)
305325

306326

307327
@api_view(["POST"])
@@ -349,18 +369,23 @@ def api_add_nodes_to_tree(request):
349369
}
350370
)
351371
except ContentNode.DoesNotExist:
352-
return HttpResponseNotFound("No content matching: {}".format(parent_id))
353-
except ValidationError as e:
354-
return HttpResponseBadRequest(content=str(e))
355-
except KeyError as e:
372+
return HttpResponseNotFound("Content not found", content_type="text/plain")
373+
except ValidationError:
374+
logger.warning("api_add_nodes_to_tree ValidationError", exc_info=True)
375+
return HttpResponseBadRequest("Validation error", content_type="text/plain")
376+
except KeyError:
377+
logger.warning("api_add_nodes_to_tree missing required field", exc_info=True)
356378
return HttpResponseBadRequest(
357-
"Required attribute missing from data | {}".format(str(e))
379+
"Required attribute missing from data", content_type="text/plain"
358380
)
359-
except NodeValidationError as e:
360-
return HttpResponseBadRequest(str(e))
381+
except NodeValidationError:
382+
logger.warning("api_add_nodes_to_tree NodeValidationError", exc_info=True)
383+
return HttpResponseBadRequest("Invalid node data", content_type="text/plain")
361384
except Exception as e:
362385
handle_server_error(e, request)
363-
return HttpResponseServerError(content=str(e), reason=str(e))
386+
return HttpResponseServerError(
387+
"Internal server error", content_type="text/plain"
388+
)
364389

365390

366391
@api_view(["POST"])
@@ -395,10 +420,12 @@ def api_publish_channel(request):
395420
}
396421
)
397422
except (KeyError, Channel.DoesNotExist):
398-
return HttpResponseNotFound("No channel matching: {}".format(data))
423+
return HttpResponseNotFound("Channel not found", content_type="text/plain")
399424
except Exception as e:
400425
handle_server_error(e, request)
401-
return HttpResponseServerError(content=str(e), reason=str(e))
426+
return HttpResponseServerError(
427+
"Internal server error", content_type="text/plain"
428+
)
402429

403430

404431
@api_view(["POST"])
@@ -418,10 +445,12 @@ def check_user_is_editor(request):
418445
Channel.get_editable(request.user, channel_id)
419446
return Response({"success": True})
420447
except Channel.DoesNotExist:
421-
return HttpResponseNotFound("Channel not found {}".format(channel_id))
448+
return HttpResponseNotFound("Channel not found", content_type="text/plain")
422449

423450
except KeyError:
424-
raise HttpResponseBadRequest("Missing attribute from data: {}".format(data))
451+
return HttpResponseBadRequest(
452+
"Missing attribute from data", content_type="text/plain"
453+
)
425454

426455

427456
@api_view(["POST"])
@@ -452,12 +481,14 @@ def get_tree_data(request):
452481
children_data = tree_data.get("children", [])
453482
return Response({"success": True, "tree": children_data})
454483
except (Channel.DoesNotExist, PermissionDenied):
455-
return HttpResponseNotFound("No channel matching: {}".format(channel_id))
484+
return HttpResponseNotFound("Channel not found", content_type="text/plain")
456485
except ValueError:
457-
return HttpResponseNotFound("No tree name matching: {}".format(tree_name))
486+
return HttpResponseNotFound("Invalid tree name", content_type="text/plain")
458487
except Exception as e:
459488
handle_server_error(e, request)
460-
return HttpResponseServerError(content=str(e), reason=str(e))
489+
return HttpResponseServerError(
490+
"Internal server error", content_type="text/plain"
491+
)
461492

462493

463494
@api_view(["POST"])
@@ -499,10 +530,12 @@ def get_node_tree_data(request):
499530
}
500531
return Response(response_data)
501532
except Channel.DoesNotExist:
502-
return HttpResponseNotFound("No channel matching: {}".format(channel_id))
533+
return HttpResponseNotFound("Channel not found", content_type="text/plain")
503534
except Exception as e:
504535
handle_server_error(e, request)
505-
return HttpResponseServerError(content=str(e), reason=str(e))
536+
return HttpResponseServerError(
537+
"Internal server error", content_type="text/plain"
538+
)
506539

507540

508541
@api_view(["POST"])
@@ -529,14 +562,14 @@ def get_channel_status_bulk(request):
529562

530563
return Response({"success": True, "statuses": statuses})
531564
except (Channel.DoesNotExist, PermissionDenied):
532-
return HttpResponseNotFound(
533-
"No complete set of channels matching: {}".format(",".join(channel_ids))
534-
)
565+
return HttpResponseNotFound("Channel not found", content_type="text/plain")
535566
except KeyError:
536567
raise ObjectDoesNotExist("Missing attribute from data: {}".format(data))
537568
except Exception as e:
538569
handle_server_error(e, request)
539-
return HttpResponseServerError(content=str(e), reason=str(e))
570+
return HttpResponseServerError(
571+
"Internal server error", content_type="text/plain"
572+
)
540573

541574

542575
def get_status(channel_id):

‎contentcuration/contentcuration/views/nodes.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ def get_node_details(request, node_id):
4949
node = ContentNode.objects.get(pk=node_id)
5050
channel = node.get_channel()
5151
if channel and not channel.public:
52-
return HttpResponseNotFound("No topic found for {}".format(node_id))
52+
return HttpResponseNotFound("Node not found", content_type="text/plain")
5353
data = get_node_details_cached(request.user, node, channel)
5454
return HttpResponse(json.dumps(data))
5555

‎contentcuration/contentcuration/views/users.py‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -107,8 +107,9 @@ def send_invitation_email(request):
107107
message = render_to_string("permissions/permissions_email.txt", ctx_dict)
108108
send_mail(subject, message, settings.DEFAULT_FROM_EMAIL, [user_email])
109109
except KeyError:
110+
logger.warning("send_invitation_email missing required field", exc_info=True)
110111
return HttpResponseBadRequest(
111-
"Missing attribute from data: {}".format(request.data)
112+
"Missing attribute from data", content_type="text/plain"
112113
)
113114

114115
return Response(InvitationSerializer(invitation).data)

‎contentcuration/contentcuration/viewsets/base.py‎

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import json
2+
import logging
23
import traceback
34
import uuid
45
from contextlib import contextmanager
@@ -38,6 +39,9 @@
3839
from contentcuration.viewsets.sync.utils import log_sync_exception
3940

4041

42+
logger = logging.getLogger(__name__)
43+
44+
4145
class SimpleReprMixin(object):
4246
def __repr__(self):
4347
"""
@@ -709,7 +713,7 @@ def create_from_changes(self, changes):
709713
errors.append(change)
710714
except Exception as e:
711715
log_sync_exception(e, user=self.request.user, change=change)
712-
change["errors"] = [str(e)]
716+
change["errors"] = ["Internal server error"]
713717
errors.append(change)
714718

715719
return errors
@@ -723,8 +727,9 @@ def create(self, request, *args, **kwargs):
723727
try:
724728
self.perform_create(serializer)
725729

726-
except IntegrityError as e:
727-
return Response({"error": str(e)}, status=409)
730+
except IntegrityError:
731+
logger.exception("RESTCreateModelMixin.create IntegrityError")
732+
return Response({"error": "Internal server error"}, status=409)
728733
instance = serializer.instance
729734
return Response(self.serialize_object(pk=instance.pk), status=HTTP_201_CREATED)
730735

@@ -754,7 +759,7 @@ def delete_from_changes(self, changes):
754759
pass
755760
except Exception as e:
756761
log_sync_exception(e, user=self.request.user, change=change)
757-
change["errors"] = [str(e)]
762+
change["errors"] = ["Internal server error"]
758763
errors.append(change)
759764
return errors
760765

@@ -796,7 +801,7 @@ def update_from_changes(self, changes):
796801
errors.append(change)
797802
except Exception as e:
798803
log_sync_exception(e, user=self.request.user, change=change)
799-
change["errors"] = [str(e)]
804+
change["errors"] = ["Internal server error"]
800805
errors.append(change)
801806
return errors
802807

@@ -836,7 +841,7 @@ def create_from_changes(self, changes):
836841
except Exception as e:
837842
log_sync_exception(e, user=self.request.user, changes=changes)
838843
for change in changes:
839-
change["errors"] = [str(e)]
844+
change["errors"] = ["Internal server error"]
840845
errors.extend(changes)
841846
else:
842847
valid_data = []
@@ -875,7 +880,7 @@ def update_from_changes(self, changes):
875880
except Exception as e:
876881
log_sync_exception(e, user=self.request.user, changes=changes)
877882
for change in changes:
878-
change["errors"] = [str(e)]
883+
change["errors"] = ["Internal server error"]
879884
errors.extend(changes)
880885
if serializer.missing_keys:
881886
# add errors for any changes that were specified but no object

0 commit comments

Comments
 (0)