From 5fe8eae2bd39e9d84a2d4954849fd92a0798ad2f Mon Sep 17 00:00:00 2001 From: Robert Sparks Date: Thu, 30 Jul 2026 15:52:57 -0500 Subject: [PATCH] fix: log exception detail instead of reflecting it in 400 responses --- ietf/api/tests.py | 12 ++++++++++++ ietf/api/views.py | 5 ++++- ietf/group/milestones.py | 7 ++++++- ietf/group/tests_info.py | 35 +++++++++++++++++++++++++++++++++++ ietf/meeting/tests_views.py | 36 +++++++++++++++++++++++++++++++++++- ietf/meeting/views.py | 12 +++++++++--- 6 files changed, 101 insertions(+), 6 deletions(-) diff --git a/ietf/api/tests.py b/ietf/api/tests.py index 87f7d684d32..5c15d32a1d4 100644 --- a/ietf/api/tests.py +++ b/ietf/api/tests.py @@ -1524,6 +1524,18 @@ def test_api_top_level(self): self.assertIn(name, resource_list, "Expected a REST API resource for %s, but didn't find one" % name) + def test_api_top_level_bad_accept_header(self): + """A malformed Accept header is rejected without reflecting its content + + The response body is served as unescaped text/html, and this is an + unauthenticated GET endpoint, so nothing derived from the request may + appear in it. + """ + payload = "" + r = self.client.get("/api/v1/", headers={"accept": payload}) + self.assertEqual(r.status_code, 400) + self.assertNotIn(payload, r.content.decode("utf-8")) + def test_all_model_resources_exist(self): client = Client(Accept='application/json') r = client.get("/api/v1") diff --git a/ietf/api/views.py b/ietf/api/views.py index 420bc396934..90fa61f8c00 100644 --- a/ietf/api/views.py +++ b/ietf/api/views.py @@ -67,7 +67,10 @@ def top_level(request): try: desired_format = determine_format(request, serializer) except BadRequest as err: - return HttpResponseBadRequest(str(err)) + # tastypie's message is a fixed string today, but don't reflect a dependency's + # exception text into an unescaped text/html body on an unauthenticated endpoint. + log.log("Bad request determining format for api top_level: %s" % err) + return HttpResponseBadRequest("Invalid Accept header") options = {} diff --git a/ietf/group/milestones.py b/ietf/group/milestones.py index 52f2eaebeee..c6380ffad76 100644 --- a/ietf/group/milestones.py +++ b/ietf/group/milestones.py @@ -21,6 +21,7 @@ from ietf.name.models import GroupMilestoneStateName from ietf.group.mails import email_milestones_changed from ietf.utils.fields import DatepickerDateField +from ietf.utils.log import log from ietf.utils.response import permission_denied class MilestoneForm(forms.Form): @@ -415,7 +416,11 @@ def reset_charter_milestones(request, acronym, group_type=None): try: milestone_ids = [int(v) for v in request.POST.getlist("milestone")] except ValueError as e: - return HttpResponseBadRequest("error in list of ids - %s" % e) + # Log the detail rather than reflecting it - the exception message from int() + # embeds the offending value verbatim, and HttpResponseBadRequest serves its + # content as unescaped text/html. + log("Invalid milestone id in reset_charter_milestones POST: %s" % e) + return HttpResponseBadRequest("error in list of ids") # delete existing for m in charter_milestones: diff --git a/ietf/group/tests_info.py b/ietf/group/tests_info.py index 97ec7ebdb15..4e0096b1859 100644 --- a/ietf/group/tests_info.py +++ b/ietf/group/tests_info.py @@ -1760,6 +1760,41 @@ def test_reset_charter_milestones(self): self.assertEqual(group.charter.docevent_set.count(), events_before + 2) # 1 delete, 1 add + def test_reset_charter_milestones_bad_ids(self): + """A non-integer milestone id is rejected without echoing the submitted value + + int() puts the offending value in its exception message and + HttpResponseBadRequest serves its content as unescaped text/html, so + reflecting the message would be an XSS vector. + """ + m1, m2, group = self.create_test_milestones() + + url = urlreverse('ietf.group.milestones.reset_charter_milestones', kwargs=dict(group_type=group.type_id, acronym=group.acronym)) + login_testing_unauthorized(self, "secretary", url) + + milestones_before = GroupMilestone.objects.count() + events_before = group.charter.docevent_set.count() + + payload = '' + for bad_id in (payload, 'not-a-number', '1.5'): + r = self.client.post(url, dict(milestone=[str(m1.pk), bad_id])) + self.assertEqual(r.status_code, 400) + content = r.content.decode('utf-8') + self.assertIn('error in list of ids', content) + self.assertNotIn(bad_id, content) + self.assertNotIn('invalid literal', content) + + # an empty id is also rejected, without the exception detail + r = self.client.post(url, dict(milestone=[str(m1.pk), ''])) + self.assertEqual(r.status_code, 400) + self.assertNotIn('invalid literal', r.content.decode('utf-8')) + + # nothing was changed + self.assertEqual(GroupMilestone.objects.count(), milestones_before) + self.assertEqual(group.charter.docevent_set.count(), events_before) + self.assertEqual(GroupMilestone.objects.get(pk=m1.pk).state_id, m1.state_id) + self.assertEqual(GroupMilestone.objects.get(pk=m2.pk).state_id, m2.state_id) + def test_edit_sort(self): group = GroupFactory(uses_milestone_dates=False) DatelessGroupMilestoneFactory(group=group,order=1) diff --git a/ietf/meeting/tests_views.py b/ietf/meeting/tests_views.py index eea08be8c77..beaaf8da8a7 100644 --- a/ietf/meeting/tests_views.py +++ b/ietf/meeting/tests_views.py @@ -18,7 +18,7 @@ from icalendar import Calendar from io import StringIO, BytesIO from bs4 import BeautifulSoup -from urllib.parse import urlparse, urlsplit +from urllib.parse import quote, urlparse, urlsplit from PIL import Image from pathlib import Path from tempfile import NamedTemporaryFile @@ -1119,6 +1119,40 @@ def _r(show=(), hide=(), showtypes=(), hidetypes=()): 'Parsed "%s" incorrectly' % qstr, ) + # Unrecognized parameters are ignored, not rejected. The ical views rely on this - + # they cannot report a parse error, so they cannot reflect one back to the client. + for qstr in ( + 'unknown=x', + 'show=a&unknown=x', + '=1', + 'show=', + ): + self.assertIsNotNone( + parse_agenda_filter_params(QueryDict(qstr)), + 'Parsing "%s" should not fail' % qstr, + ) + + def test_ical_filter_params_are_not_reflected(self): + """A query string must never be echoed into an ical view's response + + HttpResponseBadRequest serves text/html without escaping, so reflecting a + parameter name or value would be a reflected XSS on an unauthenticated GET. + """ + meeting = make_meeting_test_data() + payload = '' + for url in ( + urlreverse('ietf.meeting.views.agenda_ical', kwargs={'num': meeting.number}), + urlreverse('ietf.meeting.views.upcoming_ical'), + ): + for querystring in ( + '?%s=1' % quote(payload), + '?show=%s' % quote(payload), + '?unknown=%s' % quote(payload), + ): + r = self.client.get(url + querystring) + self.assertEqual(r.status_code, 200, 'Expected %s%s to be accepted' % (url, querystring)) + self.assertNotIn(payload, r.content.decode('utf-8')) + def do_ical_filter_test(self, meeting, querystring, expected_session_summaries): url = urlreverse('ietf.meeting.views.agenda_ical', kwargs={'num':meeting.number}) r = self.client.get(url + querystring) diff --git a/ietf/meeting/views.py b/ietf/meeting/views.py index 913c8bd3021..e2a15d3e8aa 100644 --- a/ietf/meeting/views.py +++ b/ietf/meeting/views.py @@ -2656,7 +2656,11 @@ def agenda_ical(request, num=None, acronym=None, session_id=None): try: filt_params = parse_agenda_filter_params(request.GET) except ValueError as e: - return HttpResponseBadRequest(str(e)) + # Defensive only - parse_agenda_filter_params ignores unrecognized parameters and + # does not raise. Log the detail rather than reflecting it: the query string is + # attacker-controlled and HttpResponseBadRequest serves unescaped text/html. + log("Invalid agenda filter parameters in agenda_ical: %s" % e) + return HttpResponseBadRequest("Invalid agenda filter parameters") if meeting.type_id == "ietf": return agenda_ical_ietf(meeting, filt_params, acronym, session_id) @@ -4568,8 +4572,10 @@ def upcoming_ical(request): try: filter_params = parse_agenda_filter_params(request.GET) except ValueError as e: - return HttpResponseBadRequest(str(e)) - + # Defensive only - see the corresponding handler in agenda_ical. + log("Invalid agenda filter parameters in upcoming_ical: %s" % e) + return HttpResponseBadRequest("Invalid agenda filter parameters") + today = datetime_today() # get meetings starting 7 days ago -- we'll filter out sessions in the past further down