From 8d6a20b656ff3fa18e36954668a44a831c2f6ddd Mon Sep 17 00:00:00 2001 From: Sarah Boyce <42296566+sarahboyce@users.noreply.github.com> Date: Tue, 16 Jul 2024 10:41:20 +0200 Subject: [PATCH] Fixed #35604, Refs #35326 -- Made FileSystemStorage.exists() behaviour independent from allow_overwrite. Partially reverts 0b33a3abc2ca7d68a24f6d0772bc2b9fa603744e. Storage.exists(name) was documented to "return False if the name is available for a new file." but return True if the file exists. This is ambiguous in the overwrite file case. It will now always return whether the file exists. Thank you to Natalia Bidart and Josh Schneier for the review. --- django/core/files/storage/base.py | 10 +++++-- django/core/files/storage/filesystem.py | 19 +++++++------ docs/ref/files/storage.txt | 3 +- tests/file_storage/test_generate_filename.py | 6 ++++ tests/file_storage/tests.py | 30 +++++++++++++++----- 5 files changed, 48 insertions(+), 20 deletions(-) diff --git a/django/core/files/storage/base.py b/django/core/files/storage/base.py index 55285bc23a5e..31ecbd209ab9 100644 --- a/django/core/files/storage/base.py +++ b/django/core/files/storage/base.py @@ -51,6 +51,10 @@ def save(self, name, content, max_length=None): validate_file_name(name, allow_relative_path=True) return name + def is_name_available(self, name, max_length=None): + exceeds_max_length = max_length and len(name) > max_length + return not self.exists(name) and not exceeds_max_length + # These methods are part of the public API, with default implementations. def get_valid_name(self, name): @@ -82,11 +86,11 @@ def get_available_name(self, name, max_length=None): validate_file_name(file_name) file_ext = "".join(pathlib.PurePath(file_name).suffixes) file_root = file_name.removesuffix(file_ext) - # If the filename already exists, generate an alternative filename - # until it doesn't exist. + # If the filename is not available, generate an alternative + # filename until one is available. # Truncate original name if required, so the new filename does not # exceed the max_length. - while self.exists(name) or (max_length and len(name) > max_length): + while not self.is_name_available(name, max_length=max_length): # file_ext includes the dot. name = os.path.join( dir_name, self.get_alternative_name(file_root, file_ext) diff --git a/django/core/files/storage/filesystem.py b/django/core/files/storage/filesystem.py index ed752cc06296..310a0ed0dee8 100644 --- a/django/core/files/storage/filesystem.py +++ b/django/core/files/storage/filesystem.py @@ -4,7 +4,6 @@ from urllib.parse import urljoin from django.conf import settings -from django.core.exceptions import SuspiciousFileOperation from django.core.files import File, locks from django.core.files.move import file_move_safe from django.core.signals import setting_changed @@ -192,14 +191,18 @@ def delete(self, name): # concurrently. pass - def exists(self, name): - try: - exists = os.path.lexists(self.path(name)) - except SuspiciousFileOperation: - raise + def is_name_available(self, name, max_length=None): + if self._allow_overwrite: + return not (max_length and len(name) > max_length) + return super().is_name_available(name, max_length=max_length) + + def get_alternative_name(self, file_root, file_ext): if self._allow_overwrite: - return False - return exists + return f"{file_root}{file_ext}" + return super().get_alternative_name(file_root, file_ext) + + def exists(self, name): + return os.path.lexists(self.path(name)) def listdir(self, path): path = self.path(path) diff --git a/docs/ref/files/storage.txt b/docs/ref/files/storage.txt index e912bcc4129a..f7c290a15098 100644 --- a/docs/ref/files/storage.txt +++ b/docs/ref/files/storage.txt @@ -129,8 +129,7 @@ The ``Storage`` class .. method:: exists(name) Returns ``True`` if a file referenced by the given name already exists - in the storage system, or ``False`` if the name is available for a new - file. + in the storage system. .. method:: get_accessed_time(name) diff --git a/tests/file_storage/test_generate_filename.py b/tests/file_storage/test_generate_filename.py index 9631705fc8e2..483115e09cc9 100644 --- a/tests/file_storage/test_generate_filename.py +++ b/tests/file_storage/test_generate_filename.py @@ -80,11 +80,14 @@ def test_storage_dangerous_paths(self): ("", ""), ] s = FileSystemStorage() + s_overwrite = FileSystemStorage(allow_overwrite=True) msg = "Could not derive file name from '%s'" for file_name, base_name in candidates: with self.subTest(file_name=file_name): with self.assertRaisesMessage(SuspiciousFileOperation, msg % base_name): s.get_available_name(file_name) + with self.assertRaisesMessage(SuspiciousFileOperation, msg % base_name): + s_overwrite.get_available_name(file_name) with self.assertRaisesMessage(SuspiciousFileOperation, msg % base_name): s.generate_filename(file_name) @@ -98,11 +101,14 @@ def test_storage_dangerous_paths_dir_name(self): ("\\tmp\\..\\path", "/tmp/.."), ] s = FileSystemStorage() + s_overwrite = FileSystemStorage(allow_overwrite=True) for file_name, path in candidates: msg = "Detected path traversal attempt in '%s'" % path with self.subTest(file_name=file_name): with self.assertRaisesMessage(SuspiciousFileOperation, msg): s.get_available_name(file_name) + with self.assertRaisesMessage(SuspiciousFileOperation, msg): + s_overwrite.get_available_name(file_name) with self.assertRaisesMessage(SuspiciousFileOperation, msg): s.generate_filename(file_name) diff --git a/tests/file_storage/tests.py b/tests/file_storage/tests.py index 38d87dc7f2fc..868b18dd2c06 100644 --- a/tests/file_storage/tests.py +++ b/tests/file_storage/tests.py @@ -95,18 +95,18 @@ def test_file_access_options(self): """ Standard file access options are available, and work as expected. """ - self.assertFalse(os.path.exists(os.path.join(self.temp_dir, "storage_test"))) + self.assertFalse(self.storage.exists("storage_test")) f = self.storage.open("storage_test", "w") f.write("storage contents") f.close() - self.assertTrue(os.path.exists(os.path.join(self.temp_dir, "storage_test"))) + self.assertTrue(self.storage.exists("storage_test")) f = self.storage.open("storage_test", "r") self.assertEqual(f.read(), "storage contents") f.close() self.storage.delete("storage_test") - self.assertFalse(os.path.exists(os.path.join(self.temp_dir, "storage_test"))) + self.assertFalse(self.storage.exists("storage_test")) def _test_file_time_getter(self, getter): # Check for correct behavior under both USE_TZ=True and USE_TZ=False. @@ -275,10 +275,10 @@ def test_file_save_with_path(self): """ Saving a pathname should create intermediate directories as necessary. """ - self.assertFalse(os.path.exists(os.path.join(self.temp_dir, "path/to"))) + self.assertFalse(self.storage.exists("path/to")) self.storage.save("path/to/test.file", ContentFile("file saved with path")) - self.assertTrue(os.path.exists(os.path.join(self.temp_dir, "path/to"))) + self.assertTrue(self.storage.exists("path/to")) with self.storage.open("path/to/test.file") as f: self.assertEqual(f.read(), b"file saved with path") @@ -692,12 +692,12 @@ def test_save_overwrite_behavior(self): stored_name_1 = self.storage.save(name, f_1) try: self.assertEqual(stored_name_1, name) - self.assertTrue(os.path.exists(os.path.join(self.temp_dir, name))) + self.assertTrue(self.storage.exists(name)) with self.storage.open(name) as fp: self.assertEqual(fp.read(), content_1) stored_name_2 = self.storage.save(name, f_2) self.assertEqual(stored_name_2, name) - self.assertTrue(os.path.exists(os.path.join(self.temp_dir, name))) + self.assertTrue(self.storage.exists(name)) with self.storage.open(name) as fp: self.assertEqual(fp.read(), content_2) finally: @@ -729,6 +729,22 @@ def test_save_overwrite_behavior_temp_file(self): finally: self.storage.delete(name) + def test_file_name_truncation(self): + name = "test_long_file_name.txt" + file = ContentFile(b"content") + stored_name = self.storage.save(name, file, max_length=10) + self.addCleanup(self.storage.delete, stored_name) + self.assertEqual(stored_name, "test_l.txt") + self.assertEqual(len(stored_name), 10) + + def test_file_name_truncation_extension_too_long(self): + name = "file_name.longext" + file = ContentFile(b"content") + with self.assertRaisesMessage( + SuspiciousFileOperation, "Storage can not find an available filename" + ): + self.storage.save(name, file, max_length=5) + class DiscardingFalseContentStorage(FileSystemStorage): def _save(self, name, content):