diff --git a/ietf/doc/tests_ballot.py b/ietf/doc/tests_ballot.py index 034ba6f4b..2403b6ef5 100644 --- a/ietf/doc/tests_ballot.py +++ b/ietf/doc/tests_ballot.py @@ -20,6 +20,7 @@ from ietf.doc.factories import (DocumentFactory, IndividualDraftFactory, Individ BallotPositionDocEventFactory, BallotDocEventFactory, IRSGBallotDocEventFactory) from ietf.doc.templatetags.ietf_filters import can_defer from ietf.doc.utils import create_ballot_if_not_open +from ietf.doc.views_ballot import parse_ballot_edit_return_point from ietf.doc.views_doc import document_ballot_content from ietf.group.models import Group, Role from ietf.group.factories import GroupFactory, RoleFactory, ReviewTeamFactory @@ -1451,3 +1452,32 @@ class BallotContentTests(TestCase): self._assertBallotMessage(q, balloters[0], 'No discuss send log available') self._assertBallotMessage(q, balloters[1], 'No comment send log available') self._assertBallotMessage(q, old_balloter, 'No ballot position send log available') + +class ReturnToUrlTests(TestCase): + def test_invalid_return_to_url(self): + self.assertRaises( + Exception, + lambda: parse_ballot_edit_return_point('/doc/', 'draft-ietf-opsawg-ipfix-tcpo-v6eh', '998718'), + ) + self.assertRaises( + Exception, + lambda: parse_ballot_edit_return_point('/a-route-that-does-not-exist/', 'draft-ietf-opsawg-ipfix-tcpo-v6eh', '998718'), + ) + self.assertRaises( + Exception, + lambda: parse_ballot_edit_return_point('https://example.com/phishing', 'draft-ietf-opsawg-ipfix-tcpo-v6eh', '998718'), + ) + + def test_valid_default_return_to_url(self): + self.assertEqual(parse_ballot_edit_return_point( + None, + 'draft-ietf-opsawg-ipfix-tcpo-v6eh', + '998718' + ), '/doc/draft-ietf-opsawg-ipfix-tcpo-v6eh/ballot/998718/') + + def test_valid_return_to_url(self): + self.assertEqual(parse_ballot_edit_return_point( + '/doc/draft-ietf-opsawg-ipfix-tcpo-v6eh/ballot/998718/', + 'draft-ietf-opsawg-ipfix-tcpo-v6eh', + '998718' + ), '/doc/draft-ietf-opsawg-ipfix-tcpo-v6eh/ballot/998718/') diff --git a/ietf/doc/views_ballot.py b/ietf/doc/views_ballot.py index 83ccb07be..357bd2fa2 100644 --- a/ietf/doc/views_ballot.py +++ b/ietf/doc/views_ballot.py @@ -8,13 +8,14 @@ import datetime, json from django import forms from django.conf import settings -from django.http import HttpResponse, HttpResponseRedirect, Http404 +from django.http import HttpResponse, HttpResponseRedirect, Http404, HttpResponseBadRequest from django.shortcuts import render, get_object_or_404, redirect from django.template.defaultfilters import striptags from django.template.loader import render_to_string from django.urls import reverse as urlreverse from django.views.decorators.csrf import csrf_exempt from django.utils.html import escape +from urllib.parse import urlencode as urllib_urlencode import debug # pyflakes:ignore @@ -39,6 +40,7 @@ from ietf.message.utils import infer_message from ietf.name.models import BallotPositionName, DocTypeName from ietf.person.models import Person from ietf.utils.fields import ModelMultipleChoiceField +from ietf.utils.http import validate_return_to_path from ietf.utils.mail import send_mail_text, send_mail_preformatted from ietf.utils.decorators import require_api_key from ietf.utils.response import permission_denied @@ -185,11 +187,11 @@ def edit_position(request, name, ballot_id): balloter = login = request.user.person - if 'ballot_edit_return_point' in request.session: - return_to_url = request.session['ballot_edit_return_point'] - else: - return_to_url = urlreverse("ietf.doc.views_doc.document_ballot", kwargs=dict(name=doc.name, ballot_id=ballot_id)) - + try: + return_to_url = parse_ballot_edit_return_point(request.GET.get('ballot_edit_return_point'), doc.name, ballot_id) + except ValueError: + return HttpResponseBadRequest('ballot_edit_return_point is invalid') + # if we're in the Secretariat, we can select a balloter to act as stand-in for if has_role(request.user, "Secretariat"): balloter_id = request.GET.get('balloter') @@ -209,9 +211,14 @@ def edit_position(request, name, ballot_id): save_position(form, doc, ballot, balloter, login, send_mail) if send_mail: - qstr="" + query = {} if request.GET.get('balloter'): - qstr += "?balloter=%s" % request.GET.get('balloter') + query['balloter'] = request.GET.get('balloter') + if request.GET.get('ballot_edit_return_point'): + query['ballot_edit_return_point'] = request.GET.get('ballot_edit_return_point') + qstr = "" + if len(query) > 0: + qstr = "?" + urllib_urlencode(query, safe='/') return HttpResponseRedirect(urlreverse('ietf.doc.views_ballot.send_ballot_comment', kwargs=dict(name=doc.name, ballot_id=ballot_id)) + qstr) elif request.POST.get("Defer") and doc.stream.slug != "irtf": return redirect('ietf.doc.views_ballot.defer_ballot', name=doc) @@ -337,11 +344,11 @@ def send_ballot_comment(request, name, ballot_id): balloter = request.user.person - if 'ballot_edit_return_point' in request.session: - return_to_url = request.session['ballot_edit_return_point'] - else: - return_to_url = urlreverse("ietf.doc.views_doc.document_ballot", kwargs=dict(name=doc.name, ballot_id=ballot_id)) - + try: + return_to_url = parse_ballot_edit_return_point(request.GET.get('ballot_edit_return_point'), doc.name, ballot_id) + except ValueError: + return HttpResponseBadRequest('ballot_edit_return_point is invalid') + if 'HTTP_REFERER' in request.META: back_url = request.META['HTTP_REFERER'] else: @@ -1302,3 +1309,15 @@ def rsab_ballot_status(request): # Possible TODO: add a menu item to show this? Maybe only if you're in rsab or an rswg chair? # There will be so few of these that the general community would follow them from the rswg docs page. # Maybe the view isn't actually needed at all... + + +def parse_ballot_edit_return_point(path, doc_name, ballot_id): + get_default_path = lambda: urlreverse("ietf.doc.views_doc.document_ballot", kwargs=dict(name=doc_name, ballot_id=ballot_id)) + allowed_path_handlers = { + "ietf.doc.views_doc.document_ballot", + "ietf.doc.views_doc.document_irsg_ballot", + "ietf.doc.views_doc.document_rsab_ballot", + "ietf.iesg.views.agenda", + "ietf.iesg.views.agenda_documents", + } + return validate_return_to_path(path, get_default_path, allowed_path_handlers) diff --git a/ietf/doc/views_doc.py b/ietf/doc/views_doc.py index bd4927508..0ef132c21 100644 --- a/ietf/doc/views_doc.py +++ b/ietf/doc/views_doc.py @@ -1538,7 +1538,6 @@ def document_ballot(request, name, ballot_id=None): top = render_document_top(request, doc, ballot_tab, name) c = document_ballot_content(request, doc, ballot.id, editable=True) - request.session['ballot_edit_return_point'] = request.path_info return render(request, "doc/document_ballot.html", dict(doc=doc, @@ -1556,8 +1555,6 @@ def document_irsg_ballot(request, name, ballot_id=None): c = document_ballot_content(request, doc, ballot_id, editable=True) - request.session['ballot_edit_return_point'] = request.path_info - return render(request, "doc/document_ballot.html", dict(doc=doc, top=top, @@ -1575,8 +1572,6 @@ def document_rsab_ballot(request, name, ballot_id=None): c = document_ballot_content(request, doc, ballot_id, editable=True) - request.session['ballot_edit_return_point'] = request.path_info - return render( request, "doc/document_ballot.html", diff --git a/ietf/iesg/views.py b/ietf/iesg/views.py index b67ef04a0..a92d617ac 100644 --- a/ietf/iesg/views.py +++ b/ietf/iesg/views.py @@ -209,7 +209,6 @@ def agenda(request, date=None): urlreverse("ietf.iesg.views.telechat_agenda_content_view", kwargs={"section": "minutes"}) )) - request.session['ballot_edit_return_point'] = request.path_info return render(request, "iesg/agenda.html", { "date": data["date"], "sections": sorted(data["sections"].items(), key=lambda x:[int(p) for p in x[0].split('.')]), @@ -398,7 +397,7 @@ def agenda_documents(request): "sections": sorted((num, section) for num, section in sections.items() if "2" <= num < "5") }) - request.session['ballot_edit_return_point'] = request.path_info + return render(request, 'iesg/agenda_documents.html', { 'telechats': telechats }) def past_documents(request): diff --git a/ietf/templates/doc/ballot_popup.html b/ietf/templates/doc/ballot_popup.html index 2a04ffab6..2c0863ab6 100644 --- a/ietf/templates/doc/ballot_popup.html +++ b/ietf/templates/doc/ballot_popup.html @@ -27,7 +27,7 @@ {% if editable and user|has_role:"Area Director,Secretariat,IRSG Member,RSAB Member" %} {% if user|can_ballot:doc %} + href="{% url "ietf.doc.views_ballot.edit_position" name=doc.name ballot_id=ballot_id %}?ballot_edit_return_point={{ request.path|urlencode }}"> Edit position {% endif %} diff --git a/ietf/templates/doc/document_ballot_content.html b/ietf/templates/doc/document_ballot_content.html index 803ed84a3..e0feb78bc 100644 --- a/ietf/templates/doc/document_ballot_content.html +++ b/ietf/templates/doc/document_ballot_content.html @@ -60,7 +60,7 @@ {% if user|can_ballot:doc %} + href="{% url "ietf.doc.views_ballot.edit_position" name=doc.name ballot_id=ballot.pk %}?ballot_edit_return_point={{ request.path|urlencode }}"> Edit position {% endif %} diff --git a/ietf/utils/http.py b/ietf/utils/http.py index 6e6409e31..cda51680a 100644 --- a/ietf/utils/http.py +++ b/ietf/utils/http.py @@ -1,6 +1,8 @@ -# Copyright The IETF Trust 2023, All Rights Reserved +# Copyright The IETF Trust 2023-2024, All Rights Reserved # -*- coding: utf-8 -*- +from django.urls import resolve as urlresolve, Resolver404 + def is_ajax(request): """Checks whether a request was an AJAX call @@ -8,3 +10,25 @@ def is_ajax(request): exact reproduction of the deprecated method suggested there. """ return request.headers.get("x-requested-with") == "XMLHttpRequest" + +def validate_return_to_path(path, get_default_path, allowed_path_handlers): + if path is None: + path = get_default_path() + + # we need to ensure the path isn't used for attacks (eg phishing). + # `path` can be used in HttpResponseRedirect() which could redirect to Datatracker or offsite. + # Eg http://datatracker.ietf.org/...?ballot_edit_return_point=https://example.com/phish + # offsite links could be phishing attempts so let's reject them all, and require valid Datatracker + # routes + try: + # urlresolve will throw if the url doesn't match a route known to Django + match = urlresolve(path) + # further restrict by whether it's in the list of valid routes to prevent + # (eg) redirecting to logout + if match.url_name not in allowed_path_handlers: + raise ValueError("Invalid return to path not among valid matches") + pass + except Resolver404: + raise ValueError("Invalid return to path doesn't match a route") + + return path diff --git a/playwright/package.json b/playwright/package.json index 874faa824..5e3ba9478 100644 --- a/playwright/package.json +++ b/playwright/package.json @@ -3,6 +3,7 @@ "install-deps": "playwright install --with-deps", "test": "playwright test", "test:legacy": "playwright test -c playwright-legacy.config.js", + "test:legacy:visual": "playwright test -c playwright-legacy.config.js --headed --workers=1", "test:visual": "playwright test --headed --workers=1", "test:debug": "playwright test --debug" },