fix: Ballot return to url via url params rather than session

ietf/doc/views_ballot.py

@@ -12,9 +12,11 @@
 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.urls import reverse as urlreverse, resolve as urlresolve, Resolver404
 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
 
@@ -185,9 +187,9 @@ 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 = request.GET.get("ballot_edit_return_point")
+
+    if return_to_url is None:
         return_to_url = urlreverse("ietf.doc.views_doc.document_ballot", kwargs=dict(name=doc.name, ballot_id=ballot_id))
 
     # if we're in the Secretariat, we can select a balloter to act as stand-in for
@@ -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=dict()
                 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,24 @@ 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))
+    return_to_url = request.GET.get('ballot_edit_return_point')
 
+    if return_to_url is not None:
+        # we need to ensure return_to_url isn't used for phishing attacks.
+        # return_to_url is used in HttpResponseRedirect() which could redirect to Datatracker or offsite.
+        # Eg http://datatracker.ietf.org/?ballot_edit_return_point=https://example.com/phishing-attack
+        # offsite links could be phishing attempts so let's reject them all, and require valid Datatracker
+        # routes
+        try:
+            urlresolve(return_to_url)
+            # if it doesn't throw then it's a valid route
+            pass
+        except Resolver404:
+            raise ValueError(f"Invalid ballot_edit_return_point doesn't match a route: {return_to_url}")
+
+    if return_to_url is None:
+        return_to_url = urlreverse("ietf.doc.views_doc.document_ballot", kwargs=dict(name=doc.name, ballot_id=ballot_id))
+
     if 'HTTP_REFERER' in request.META:
         back_url = request.META['HTTP_REFERER']
     else:

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 %}

playwright/helpers/session.js

@@ -0,0 +1,16 @@
+module.exports = {
+  login: async (page, baseURL, username, password) => {
+    await page.goto('/accounts/login/')
+    await page.getByLabel('Username').fill(username)
+    await page.getByLabel('Password').fill(password)
+    await page.getByRole('button', { name: 'Sign in' }).click()
+    // Wait until the page receives the cookies.
+    //
+    // Theoretically login flow could set cookies in the process of several
+    // redirects.
+    // Wait for the final URL to ensure that the cookies are actually set.
+    await page.waitForURL(
+      new URL('/accounts/profile/', baseURL).toString()
+    )
+  }
+}

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

playwright/tests-legacy/ballot-edit.spec.js

@@ -0,0 +1,53 @@
+const { test, expect } = require('@playwright/test')
+const viewports = require('../helpers/viewports')
+const session = require('../helpers/session')
+
+// ====================================================================
+// BALLOT - EDITING
+// ====================================================================
+
+test.describe('ballot edit position ', () => {
+  test.beforeEach(async ({ page, baseURL }) => {
+    await page.setViewportSize({
+      width: viewports.desktop[0],
+      height: viewports.desktop[1]
+    })
+    // Is there an AD user for local testing?
+    await session.login(page, baseURL, '[email protected]', 'password')
+  })
+
+  test('redirect back to ballot_edit_return_point param' , async ({ page, baseURL }, workerInfo) => {
+    await page.goto('/doc/draft-ietf-opsawg-ipfix-tcpo-v6eh/ballot/')
+    const editPositionButton = await page.getByRole('link', { name: 'Edit position' })
+    const href = await editPositionButton.getAttribute('href')
+    // The href's query param 'ballot_edit_return_point' should point to the current page
+    const hrefUrl = new URL(href, baseURL)
+
+    const entryPageUrl = new URL(page.url(), baseURL)
+    await expect(hrefUrl.searchParams.get('ballot_edit_return_point')).toBe(entryPageUrl.pathname)
+    await editPositionButton.click()
+    await page.waitForURL('**/position')
+
+    // The position page has several ways through it so we'll test each one to see if 'ballot_edit_return_point' works
+
+    // Option 1. Try the default 'Save' button
+    await page.getByRole('button', { name: 'Save' })
+    await page.waitForURL(entryPageUrl.pathname)
+
+    // Option 2. Try the 'Save & send email' button
+    await page.getByRole('link', { name: 'Edit position' }).click()
+    await page.waitForURL('**/position')
+    await page.getByRole('button', { name: 'Save & send email' }).click()
+    await page.waitForURL('**/emailposition/')
+    await page.getByRole('button', { name: 'Send' }).click()
+    await page.waitForURL(entryPageUrl.pathname)
+
+    // TODO: Option 3. Try the 'Defer ballot' button
+    // This doens't yet work.
+    // await page.getByRole('link', { name: 'Edit position' }).click()
+    // await page.waitForURL('**/position')
+    // await page.getByRole('button', { name: 'Defer ballot' }).click()
+    // await page.waitForURL('**/deferballot/')
+  })
+})
+