fix: log exception detail instead of reflecting it in 400 responses

ietf/api/tests.py

@@ -1663,6 +1663,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 = "<img src=x onerror=alert(1)>"
+        r = self.client.get("/api/v1/", headers={"accept": payload})
+        self.assertEqual(r.status_code, 400)
+        self.assertNotIn(payload, r.content.decode("utf-8"))
+
     def _assert_filter_is_bad_request(self, querystring, leaked):
         """Assert a filter the database rejects gives a 400 that leaks nothing
 

ietf/api/views.py

@@ -68,7 +68,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 = {}
 

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:

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 = '<img src=x onerror=alert(1)>'
+        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)

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',
+            '<img src=x onerror=alert(1)>=1',
+            'show=<img src=x onerror=alert(1)>',
+        ):
+            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 = '<img src=x onerror=alert(1)>'
+        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)

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