From 2fcc4c5ea7119e8d6f2e9416e5dfbc6cf1e13ffd Mon Sep 17 00:00:00 2001 From: Toby Such Date: Tue, 24 Dec 2024 00:15:52 +0000 Subject: [PATCH] #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 --- shifter/assets/js/timezone-utils.js | 6 ++- shifter/shifter/settings.py | 4 ++ shifter/shifter_files/forms.py | 47 ++++++++++++------- .../tests/views/test_index_view.py | 28 +++++++++++ shifter/shifter_site_settings/forms.py | 15 ++++++ shifter/shifter_site_settings/tests.py | 39 +++++++++++++++ 6 files changed, 120 insertions(+), 19 deletions(-) diff --git a/shifter/assets/js/timezone-utils.js b/shifter/assets/js/timezone-utils.js index 8bdde6b..d33cb4d 100644 --- 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 index 1805e3a..faf5b49 100644 --- 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 index 5343354..068723d 100644 --- a/shifter/shifter_files/forms.py +++ b/shifter/shifter_files/forms.py @@ -30,35 +30,48 @@ 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( @@ -66,7 +79,7 @@ def clean_expiry_datetime(self): 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 index 2401707..e3a1ca9 100644 --- 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 index e4c9890..0f85d6a 100644 --- 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 index e9512de..8698f42 100644 --- 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):