#427 Fix error on index page when setting max expiry setting too high (…

…#429) * Add max values to site settings * Fix issue where max expiry would create too large date object
TobySuch · Dec 24, 2024 · 2fcc4c5 · 2fcc4c5
1 parent e7d97c4
commit 2fcc4c5
Show file tree

Hide file tree

Showing 6 changed files with 120 additions and 19 deletions.
diff --git a/shifter/assets/js/timezone-utils.js b/shifter/assets/js/timezone-utils.js
@@ -36,8 +36,10 @@ function convertDateTimeLocalFormElementToLocalTime(input) {
   const minVal = new Date(input.getAttribute("data-min-iso"));
   input.min = convertDateToFormFieldFormat(minVal);
 
-  const maxVal = new Date(input.getAttribute("data-max-iso"));
-  input.max = convertDateToFormFieldFormat(maxVal);
+  if (input.getAttribute("data-max-iso") != null) {
+    const maxVal = new Date(input.getAttribute("data-max-iso"));
+    input.max = convertDateToFormFieldFormat(maxVal);
+  }
 }
 
 document.addEventListener("DOMContentLoaded", function () {

diff --git a/shifter/shifter/settings.py b/shifter/shifter/settings.py
@@ -262,11 +262,15 @@
         "default": 24 * 14,  # 2 weeks
         "label": "Default Expiry Offset (hours)",
         "field_type": forms.IntegerField,
+        "min_value": 0,
+        "max_value": 2147483647,
     },
     "max_expiry_offset": {
         "default": 24 * 365 * 5,  # 5 years
         "label": "Maximum Expiry Offset (hours)",
         "field_type": forms.IntegerField,
+        "min_value": 0,
+        "max_value": 2147483647,
     },
 }
 

diff --git a/shifter/shifter_files/forms.py b/shifter/shifter_files/forms.py
@@ -30,43 +30,56 @@ def __init__(self, *args, **kwargs):
         )
         exp_date_str = exp_date.strftime(settings.DATETIME_INPUT_FORMATS[0])
         self.fields["expiry_datetime"].initial = exp_date_str
+        self.fields["expiry_datetime"].widget.attrs["data-initial-iso"] = (
+            exp_date.isoformat()
+        )
+
         exp_date_min = timezone.now()
         exp_date_min_str = exp_date_min.strftime(
             settings.DATETIME_INPUT_FORMATS[0]
         )
         self.fields["expiry_datetime"].widget.attrs["min"] = exp_date_min_str
-        exp_date_max = timezone.now() + timedelta(
-            hours=int(SiteSetting.get_setting("max_expiry_offset"))
-        )
-        exp_date_max_str = exp_date_max.strftime(
-            settings.DATETIME_INPUT_FORMATS[0]
-        )
-        self.fields["expiry_datetime"].widget.attrs["max"] = exp_date_max_str
-        self.fields["expiry_datetime"].widget.attrs["data-initial-iso"] = (
-            exp_date.isoformat()
-        )
         self.fields["expiry_datetime"].widget.attrs["data-min-iso"] = (
             exp_date_min.isoformat()
         )
-        self.fields["expiry_datetime"].widget.attrs["data-max-iso"] = (
-            exp_date_max.isoformat()
-        )
+
+        try:
+            exp_date_max = timezone.now() + timedelta(
+                hours=int(SiteSetting.get_setting("max_expiry_offset"))
+            )
+            exp_date_max_str = exp_date_max.strftime(
+                settings.DATETIME_INPUT_FORMATS[0]
+            )
+            self.fields["expiry_datetime"].widget.attrs["max"] = (
+                exp_date_max_str
+            )
+            self.fields["expiry_datetime"].widget.attrs["data-max-iso"] = (
+                exp_date_max.isoformat()
+            )
+        except OverflowError:
+            # If the max expiry offset is too large, don't set a max expiry
+            # It is too far in the future to matter.
+            pass
 
     def clean_expiry_datetime(self):
         expiry_datetime = self.cleaned_data["expiry_datetime"]
         current_datetime = timezone.now()
         max_expiry_offset = SiteSetting.get_setting("max_expiry_offset")
-        max_expiry_time = current_datetime + timedelta(
-            hours=int(max_expiry_offset)
-        )
+        dont_validate_max_expiry = False
+        try:
+            max_expiry_time = current_datetime + timedelta(
+                hours=int(max_expiry_offset)
+            )
+        except OverflowError:
+            dont_validate_max_expiry = True
 
         if expiry_datetime < current_datetime:
             raise ValidationError(
                 "You can't upload a file with an expiry time in the past.",
                 code="expiry-time-past",
             )
 
-        if expiry_datetime > max_expiry_time:
+        if not dont_validate_max_expiry and expiry_datetime > max_expiry_time:
             raise ValidationError(
                 "You can't upload a file with an expiry time more than "
                 f"{max_expiry_offset} hours in the future.",

diff --git a/shifter/shifter_files/tests/views/test_index_view.py b/shifter/shifter_files/tests/views/test_index_view.py
@@ -179,3 +179,31 @@ def test_file_upload_unauthenticated(self):
         )
         self.assertEqual(response.status_code, 302)
         self.assertEqual(FileUpload.objects.count(), 0)
+
+    # #427 - Setting max expiry date in hours to large number causes 500 error
+    # when accessing file upload page.
+    def test_site_setting_max_expiry_too_big(self):
+        SiteSetting.objects.create(
+            name="max_expiry_offset",
+            value="2147483647",
+        )
+
+        client = Client()
+        client.login(email=TEST_USER_EMAIL, password=TEST_USER_PASSWORD)
+        url = reverse("shifter_files:index")
+        response = client.get(url)
+        self.assertEqual(response.status_code, 200)
+
+        # Also test uploading a file
+        expiry_datetime = datetime.datetime(5000, 1, 1)
+        test_file = SimpleUploadedFile(TEST_FILE_NAME, TEST_FILE_CONTENT)
+        response = client.post(
+            reverse("shifter_files:index"),
+            {
+                "expiry_datetime": expiry_datetime.isoformat(
+                    sep=" ", timespec="minutes"
+                ),
+                "file_content": test_file,
+            },
+        )
+        self.assertEqual(response.status_code, 200)
diff --git a/shifter/shifter_site_settings/forms.py b/shifter/shifter_site_settings/forms.py
@@ -1,5 +1,6 @@
 from django import forms
 from django.conf import settings
+from django.core.validators import MaxValueValidator, MinValueValidator
 
 from .models import SiteSetting
 
@@ -19,3 +20,17 @@ def __init__(self, *args, **kwargs):
                 self.fields[
                     f"setting_{setting.name}"
                 ].help_text = setting_config["tooltip"]
+            if "min_value" in setting_config:
+                self.fields[f"setting_{setting.name}"].validators.append(
+                    MinValueValidator(
+                        setting_config["min_value"],
+                        "Minimum value: %(limit_value)s",
+                    )
+                )
+            if "max_value" in setting_config:
+                self.fields[f"setting_{setting.name}"].validators.append(
+                    MaxValueValidator(
+                        setting_config["max_value"],
+                        "Maximum value: %(limit_value)s",
+                    )
+                )
diff --git a/shifter/shifter_site_settings/tests.py b/shifter/shifter_site_settings/tests.py
@@ -114,6 +114,45 @@ def test_standard_user_cannot_access(self):
         response = client.get(url)
         self.assertEqual(response.status_code, 403)
 
+    # #427 - Setting max expiry date in hours to large number causes 500 error
+    # when accessing file upload page.
+    def test_max_expiry_limits(self):
+        # Test submitting the form changes the settings values
+        client = Client()
+        client.login(email=TEST_USER_EMAIL, password=TEST_USER_PASSWORD)
+        url = reverse("shifter_site_settings:site-settings")
+        response = client.get(url)
+        self.assertEqual(response.status_code, 200)
+
+        # Change setting values
+        old_setting_max_expiry_offset = SiteSetting.get_setting(
+            "max_expiry_offset"
+        )
+        setting_max_expiry_offset = "99999999999999999999999999999999999999"
+        response = client.post(
+            url,
+            {
+                "setting_max_file_size": "5120MB",
+                "setting_default_expiry_offset": "336",
+                "setting_max_expiry_offset": setting_max_expiry_offset,
+            },
+        )
+        self.assertEqual(response.status_code, 200)
+        # Check response contains error
+        self.assertContains(
+            response,
+            (
+                "Maximum value: "
+                f"{settings.SITE_SETTINGS["max_expiry_offset"]["max_value"]}"
+            ),
+        )
+
+        # Check that the setting was not changed
+        self.assertEqual(
+            SiteSetting.get_setting("max_expiry_offset"),
+            old_setting_max_expiry_offset,
+        )
+
 
 class SiteSettingsSiteInformationTest(TestCase):
     def setUp(self):