fix: Ballot return to url via url params rather than session (#7788)

* fix: #7287 ballot return params

* fix: Moving Ballot edit position ballot_edit_return_point from session to query param

* fix: tests for return_to_path param

* chore: removing Playwright tests until we can figure out a plan

* feat: return_to path utility refactoring

* fix: throw HTTP 400 in view rather than bubbling up a 500

* fix: return http400 rather than raising

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/')

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:
+    try:
-        return_to_url = request.session['ballot_edit_return_point']
+        return_to_url = parse_ballot_edit_return_point(request.GET.get('ballot_edit_return_point'), doc.name, ballot_id)
-    else:
+    except ValueError:
-        return_to_url = urlreverse("ietf.doc.views_doc.document_ballot", kwargs=dict(name=doc.name, ballot_id=ballot_id))
+        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:
+    try:
-        return_to_url = request.session['ballot_edit_return_point']
+        return_to_url = parse_ballot_edit_return_point(request.GET.get('ballot_edit_return_point'), doc.name, ballot_id)
-    else:
+    except ValueError:
-        return_to_url = urlreverse("ietf.doc.views_doc.document_ballot", kwargs=dict(name=doc.name, ballot_id=ballot_id))
+        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)

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",

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):

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 %}
                     <a class="btn btn-primary"
-                       href="{% url "ietf.doc.views_ballot.edit_position" name=doc.name ballot_id=ballot_id %}">
+                       href="{% url "ietf.doc.views_ballot.edit_position" name=doc.name ballot_id=ballot_id %}?ballot_edit_return_point={{ request.path|urlencode }}">
                         Edit position
                     </a>
                 {% endif %}

ietf/templates/doc/document_ballot_content.html

@@ -60,7 +60,7 @@
             </a>
             {% if user|can_ballot:doc %}
                 <a class="btn btn-primary"
-                   href="{% url "ietf.doc.views_ballot.edit_position" name=doc.name ballot_id=ballot.pk %}">
+                   href="{% url "ietf.doc.views_ballot.edit_position" name=doc.name ballot_id=ballot.pk %}?ballot_edit_return_point={{ request.path|urlencode }}">
                     Edit position
                 </a>
             {% endif %}

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

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"
   },