diff --git a/ietf/api/tests.py b/ietf/api/tests.py
index 204d2120f59..f7fa1aff02c 100644
--- a/ietf/api/tests.py
+++ b/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 = "
"
+ 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
diff --git a/ietf/api/views.py b/ietf/api/views.py
index 1c62d8a1ea2..9c4743c152a 100644
--- a/ietf/api/views.py
+++ b/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 = {}
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