Correctly validate from/to column index in redirect import

- fixes #8813
- relates to #6913 #8814
- add file hash check to ensure that no content can be tampered with mid-request after form submis

CHANGELOG.txt

@@ -86,6 +86,7 @@ Changelog
  * Extract filtering code from ReportView to generic IndexView (Sage Abdullah)
  * Retain other query params in header search behaviour (Sage Abdullah)
  * Remove `is_parent` kwarg in various page button hooks as this approach is no longer required (Paarth Agarwal)
+ * Improve security of redirect imports by adding a file hash (signature) check for so that any tampering of file contents between requests will throw a `BadSignature` error (Jaap Roes)
  * Fix: Typo in `ResumeWorkflowActionFormatter` message (Stefan Hammer)
  * Fix: Throw a meaningful error when saving an image to an unrecognised image format (Christian Franke)
  * Fix: Remove extra padding for headers with breadcrumbs on mobile viewport (Steven Steinwand)
@@ -116,6 +117,7 @@ Changelog
  * Fix: Ensure ModelAdmin single selection lists show correctly with Django 4.0 form template changes (Coen van der Kamp)
  * Fix: Ensure icons within help blocks have accessible contrasting colours, and links have a darker colour plus underline to indicate they are links (Paarth Agarwal)
  * Fix: Ensure consistent sidebar icon position whether expanded or collapsed (Scott Cranfill)
+ * Fix: Avoid redirects import error if the file had lots of columns (Jaap Roes)
 
 
 3.0.1 (16.06.2022)

docs/releases/4.0.md

@@ -115,6 +115,7 @@ In Wagtail 2.16, we introduced support for Windows High Contrast mode (WHCM). Th
  * Add shortcut for accessing StreamField blocks by block name with new [`blocks_by_name` and `first_block_by_name` methods on `StreamValue`](streamfield_retrieving_blocks_by_name) (Tidiane Dia, Matt Westcott)
  * Add HTML-aware max_length validation on RichTextField and RichTextBlock (Matt Westcott)
  * Remove `is_parent` kwarg in various page button hooks as this approach is no longer required (Paarth Agarwal)
+ * Improve security of redirect imports by adding a file hash (signature) check for so that any tampering of file contents between requests will throw a `BadSignature` error (Jaap Roes)
 
 ### Bug fixes
 
@@ -146,6 +147,7 @@ In Wagtail 2.16, we introduced support for Windows High Contrast mode (WHCM). Th
  * Ensure ModelAdmin single selection lists show correctly with Django 4.0 form template changes (Coen van der Kamp)
  * Ensure icons within help blocks have accessible contrasting colours, and links have a darker colour plus underline to indicate they are links (Paarth Agarwal)
  * Ensure consistent sidebar icon position whether expanded or collapsed (Scott Cranfill)
+ * Avoid redirects import error if the file had lots of columns (Jaap Roes)
 
 
 ## Upgrade considerations

wagtail/contrib/redirects/forms.py

@@ -1,6 +1,5 @@
-import os
-
 from django import forms
+from django.core.signing import BadSignature, Signer
 from django.utils.translation import gettext_lazy as _
 
 from wagtail.admin.widgets import AdminPageChooser
@@ -70,7 +69,36 @@ class ImportForm(forms.Form):
         self.fields["import_file"].help_text = help_text
 
 
-class ConfirmImportForm(forms.Form):
+class ConfirmImportManagementForm(forms.Form):
+    """
+    Store the import file name and input format in the form so that it can be used in the next step
+
+    The initial values are signed, to prevent them from being tampered with.
+    """
+
+    import_file_name = forms.CharField(widget=forms.HiddenInput())
+    input_format = forms.CharField(widget=forms.HiddenInput())
+
+    def __init__(self, *args, **kwargs):
+        self.signer = Signer()
+        initial = kwargs.get("initial", {})
+        for key in {"import_file_name", "input_format"}:
+            if key in initial:
+                # Sign initial data so it cannot be tampered with
+                initial[key] = self.signer.sign(initial[key])
+        super().__init__(*args, **kwargs)
+
+    def clean(self):
+        cleaned_data = super().clean()
+        for key in {"import_file_name", "input_format"}:
+            try:
+                cleaned_data[key] = self.signer.unsign(cleaned_data[key])
+            except BadSignature as e:
+                raise forms.ValidationError(e.message)
+        return cleaned_data
+
+
+class ConfirmImportForm(ConfirmImportManagementForm):
     from_index = forms.ChoiceField(
         label=_("From field"),
         choices=(),
@@ -86,9 +114,6 @@ class ConfirmImportForm(forms.Form):
         empty_label=_("All sites"),
     )
     permanent = forms.BooleanField(initial=True, required=False)
-    import_file_name = forms.CharField(widget=forms.HiddenInput())
-    original_file_name = forms.CharField(widget=forms.HiddenInput())
-    input_format = forms.CharField(widget=forms.HiddenInput())
 
     def __init__(self, headers, *args, **kwargs):
         super().__init__(*args, **kwargs)
@@ -101,8 +126,3 @@ class ConfirmImportForm(forms.Form):
 
         self.fields["from_index"].choices = choices
         self.fields["to_index"].choices = choices
-
-    def clean_import_file_name(self):
-        data = self.cleaned_data["import_file_name"]
-        data = os.path.basename(data)
-        return data

wagtail/contrib/redirects/tests/files/example_offset_columns.csv

@@ -0,0 +1,4 @@
+ID,UUID,Type,Requested by,Granted,Imported,Site,User,Date,Time,Permanent,Notes,Checked,Category,Protocol,Domain,From,To
+1,4aaafa99-9362-4170-9949-d628482f847f,Redirect,Ann Smith,TRUE,FALSE,1,Admin User,06/07/2022,14:46,FALSE,…,TRUE,Offsite,HTTPS,www.example.com,/goodbye,https://www.example.com/cya/
+2,713e00d5-599e-4f9d-bd1b-be5ff3ab627c,Redirect,Ngaio Bailo,TRUE,FALSE,1,Admin User,06/07/2022,14:43,FALSE,…,TRUE,Offsite,HTTPS,www.example.com,/hello,https://www.example.com/hi/
+3,0e9e4f6a-cb56-4ea4-9301-61dff68b9961,Redirect,Cheng Lee,TRUE,FALSE,1,Admin User,06/07/2022,14:43,FALSE,…,TRUE,Samesite,HTTPS,,/welcome,/hello

wagtail/contrib/redirects/tests/test_import_admin_views.py

@@ -114,6 +114,35 @@ class TestImportAdminViews(TestCase, WagtailTestUtils):
             )
             self.assertEqual(Redirect.objects.all().count(), 2)
 
+    def test_import_step_with_offset_columns(self):
+        f = "{}/files/example_offset_columns.csv".format(TEST_ROOT)
+        (_, filename) = os.path.split(f)
+
+        with open(f, "rb") as infile:
+            upload_file = SimpleUploadedFile(filename, infile.read())
+
+            self.assertEqual(Redirect.objects.all().count(), 0)
+
+            response = self.post(
+                {
+                    "import_file": upload_file,
+                }
+            )
+
+            import_response = self.post_import(
+                {
+                    **response.context["form"].initial,
+                    "from_index": 16,
+                    "to_index": 17,
+                    "permanent": True,
+                },
+            )
+
+            self.assertTemplateUsed(
+                import_response, "wagtailredirects/import_summary.html"
+            )
+            self.assertEqual(Redirect.objects.all().count(), 2)
+
     def test_permanent_setting(self):
         f = "{}/files/example.csv".format(TEST_ROOT)
         (_, filename) = os.path.split(f)

wagtail/contrib/redirects/views.py

@@ -1,6 +1,6 @@
 import os
 
-from django.core.exceptions import PermissionDenied
+from django.core.exceptions import PermissionDenied, SuspiciousOperation
 from django.core.paginator import Paginator
 from django.db import transaction
 from django.db.models import Q
@@ -18,9 +18,13 @@ from wagtail.admin.auth import PermissionPolicyChecker
 from wagtail.admin.forms.search import SearchForm
 from wagtail.admin.views.reports import ReportView
 from wagtail.contrib.redirects import models
-from wagtail.contrib.redirects.base_formats import DEFAULT_FORMATS
 from wagtail.contrib.redirects.filters import RedirectsReportFilterSet
-from wagtail.contrib.redirects.forms import ConfirmImportForm, ImportForm, RedirectForm
+from wagtail.contrib.redirects.forms import (
+    ConfirmImportForm,
+    ConfirmImportManagementForm,
+    ImportForm,
+    RedirectForm,
+)
 from wagtail.contrib.redirects.permissions import permission_policy
 from wagtail.contrib.redirects.utils import (
     get_file_storage,
@@ -259,9 +263,10 @@ def start_import(request):
         )
         return redirect("wagtailredirects:start_import")
 
+    # This data is needed in the processing step, so it is stored in
+    # hidden form fields as signed strings (signing happens in the form).
     initial = {
-        "import_file_name": file_storage.name,
-        "original_file_name": import_file.name,
+        "import_file_name": os.path.basename(file_storage.name),
         "input_format": get_import_formats().index(import_format_cls),
     }
 
@@ -281,49 +286,44 @@ def process_import(request):
     supported_extensions = get_supported_extensions()
     from_encoding = "utf-8"
 
-    form_kwargs = {}
-    form = ConfirmImportForm(
-        DEFAULT_FORMATS, request.POST or None, request.FILES or None, **form_kwargs
-    )
+    management_form = ConfirmImportManagementForm(request.POST)
+    if not management_form.is_valid():
+        # Unable to unsign the hidden form data, or the data is missing, that's suspicious.
+        raise SuspiciousOperation(
+            f"Invalid management form, data is missing or has been tampered with:\n"
+            f"{management_form.errors.as_text()}"
+        )
 
-    is_confirm_form_valid = form.is_valid()
-
-    import_formats = get_import_formats()
-    input_format = import_formats[int(form.cleaned_data["input_format"])]()
+    input_format = get_import_formats()[
+        int(management_form.cleaned_data["input_format"])
+    ]()
 
     FileStorage = get_file_storage()
-    file_storage = FileStorage(name=form.cleaned_data["import_file_name"])
-
-    if not is_confirm_form_valid:
-        data = file_storage.read(input_format.get_read_mode())
-        if not input_format.is_binary() and from_encoding:
-            data = force_str(data, from_encoding)
-        dataset = input_format.create_dataset(data)
-
-        initial = {
-            "import_file_name": file_storage.name,
-            "original_file_name": form.cleaned_data["import_file_name"],
-        }
-
-        return render(
-            request,
-            "wagtailredirects/confirm_import.html",
-            {
-                "form": ConfirmImportForm(
-                    dataset.headers,
-                    request.POST or None,
-                    request.FILES or None,
-                    initial=initial,
-                ),
-                "dataset": dataset,
-            },
-        )
+    file_storage = FileStorage(name=management_form.cleaned_data["import_file_name"])
 
     data = file_storage.read(input_format.get_read_mode())
     if not input_format.is_binary() and from_encoding:
         data = force_str(data, from_encoding)
     dataset = input_format.create_dataset(data)
 
+    # Now check if the rest of the management form is valid
+    form = ConfirmImportForm(
+        dataset.headers,
+        request.POST,
+        request.FILES,
+        initial=management_form.cleaned_data,
+    )
+
+    if not form.is_valid():
+        return render(
+            request,
+            "wagtailredirects/confirm_import.html",
+            {
+                "form": form,
+                "dataset": dataset,
+            },
+        )
+
     import_summary = create_redirects_from_dataset(
         dataset,
         {