backport: fix: Refactor flawed add to collection XSS redirect sanitis…

…ation added (#203)

* Cherry picked: 141018d

CHANGELOG.rst

@@ -4,7 +4,8 @@ Changelog
 
 Unreleased
 ==========
-* Pin the following packages to a python 3.6 compatible version matching this release stream: django-admin-sortable2, djangocms-text-ckeditor, djangocms-version-locking and djangocms-versioning.  
+* fix: Refactor flawed add to collection XSS redirect sanitisation added in 1.0.26
+* Pin the following packages to a python 3.6 compatible version matching this release stream: django-admin-sortable2, djangocms-text-ckeditor, djangocms-version-locking and djangocms-versioning.
 
 1.0.28 (2021-10-18)
 ===================

djangocms_moderation/views.py

@@ -1,12 +1,14 @@
 from __future__ import unicode_literals
 
+from urllib.parse import quote
+
 from django.contrib import admin, messages
 from django.db import transaction
 from django.http import Http404, HttpResponseRedirect
 from django.shortcuts import get_object_or_404, render
 from django.urls import reverse
 from django.utils.decorators import method_decorator
-from django.utils.http import is_safe_url, urlquote
+from django.utils.http import is_safe_url
 from django.utils.translation import ugettext_lazy as _, ungettext
 from django.views.generic import FormView
 
@@ -88,7 +90,9 @@ def _get_success_redirect(self):
                 allowed_hosts=self.request.get_host(),
                 require_https=self.request.is_secure(),
             )
-            return_to_url = urlquote(return_to_url)
+            # Protect against refracted XSS attacks
+            # Allow : in http://, ?=& for GET parameters
+            return_to_url = quote(return_to_url, safe='/:?=&')
             if not url_is_safe:
                 return_to_url = self.request.path
             return HttpResponseRedirect(return_to_url)

tests/test_views.py

@@ -6,7 +6,7 @@
 from django.urls import reverse
 
 from cms.test_utils.testcases import CMSTestCase
-from cms.utils.urlutils import add_url_parameters
+from cms.utils.urlutils import add_url_parameters, admin_reverse
 
 from djangocms_versioning.test_utils.factories import PageVersionFactory
 
@@ -550,15 +550,20 @@ def test_adding_page_not_by_the_author_doesnt_trigger_nested_collection_mechanis
             ModerationRequestTreeNode.objects.filter(moderation_request=mr1.first()).count(), 0
         )
 
-    def test_collection_with_redirect_url_query_string_parameter_sanitisation(self):
+    def test_collection_with_redirect_url_query_redirect_sanitisation(self):
+        """
+        Reflected XSS Protection by ensuring that harmful characters are encoded
+
+        When a collection is successful a redirect occurs back to the grouper in versioning,
+        this functionality should continue to function even when sanitised!
+        """
         user = self.get_superuser()
         collection = ModerationCollectionFactory(author=user)
         page1_version = PageVersionFactory(created_by=user)
         page2_version = PageVersionFactory(created_by=user)
-
-        redirect_to_url = f"""{reverse(
-            "admin:djangocms_moderation_moderationcollection_changelist"
-        )}?=<script>alert('attack!')</script>"""
+        opts = page1_version.versionable.version_model_proxy._meta
+        redirect_to_url = admin_reverse(f"{opts.app_label}_{opts.model_name}_changelist")
+        redirect_to_url += f"?page={page1_version.content.page.id}&<script>alert('attack!')</script>"
 
         url = add_url_parameters(
             get_admin_url(
@@ -578,8 +583,19 @@ def test_collection_with_redirect_url_query_string_parameter_sanitisation(self):
                 follow=False,
             )
 
+        messages = [message.message for message in list(get_messages(response.wsgi_request))]
+
         self.assertEqual(response.status_code, 302)
-        self.assertIn("%3F%3D%3Cscript%3Ealert%28%27attack%21%27%29%3C/script%3E", response.url)
+        self.assertIn("%3Cscript%3Ealert%28%27attack%21%27%29%3C/script%3E", response.url)
+        self.assertIn(f"?page={page1_version.content.page.id}", response.url)
+        self.assertIn(
+            "2 items successfully added to moderation collection",
+            messages,
+        )
+        self.assertNotIn(
+            "Perhaps it was deleted",
+            messages,
+        )
 
 
 class CollectionItemsViewTest(CMSTestCase):