diff --git a/django/core/files/storage.py b/django/core/files/storage.py index 2d7c0c78e9..90ab1be27a 100644 --- a/django/core/files/storage.py +++ b/django/core/files/storage.py @@ -1,8 +1,11 @@ -import os -import errno from datetime import datetime +import errno +from inspect import getargspec +import os +import warnings from django.conf import settings +from django.core.exceptions import SuspiciousFileOperation from django.core.files import locks, File from django.core.files.move import file_move_safe from django.utils.crypto import get_random_string @@ -13,6 +16,7 @@ from django.utils.six.moves.urllib.parse import urljoin from django.utils.text import get_valid_filename from django.utils._os import safe_join, abspathu from django.utils.deconstruct import deconstructible +from django.utils.deprecation import RemovedInDjango20Warning __all__ = ('Storage', 'FileSystemStorage', 'DefaultStorage', 'default_storage') @@ -33,7 +37,7 @@ class Storage(object): """ return self._open(name, mode) - def save(self, name, content): + def save(self, name, content, max_length=None): """ Saves new content to the file specified by name. The content should be a proper File object or any python file-like object, ready to be read @@ -46,7 +50,18 @@ class Storage(object): if not hasattr(content, 'chunks'): content = File(content) - name = self.get_available_name(name) + args, varargs, varkw, defaults = getargspec(self.get_available_name) + if 'max_length' in args: + name = self.get_available_name(name, max_length=max_length) + else: + warnings.warn( + 'Backwards compatibility for storage backends without ' + 'support for the `max_length` argument in ' + 'Storage.get_available_name() will be removed in Django 2.0.', + RemovedInDjango20Warning, stacklevel=2 + ) + name = self.get_available_name(name) + name = self._save(name, content) # Store filenames with forward slashes, even on Windows @@ -61,7 +76,7 @@ class Storage(object): """ return get_valid_filename(name) - def get_available_name(self, name): + def get_available_name(self, name, max_length=None): """ Returns a filename that's free on the target storage system, and available for new content to be written to. @@ -71,10 +86,25 @@ class Storage(object): # If the filename already exists, add an underscore and a random 7 # character alphanumeric string (before the file extension, if one # exists) to the filename until the generated filename doesn't exist. - while self.exists(name): + # 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): # file_ext includes the dot. name = os.path.join(dir_name, "%s_%s%s" % (file_root, get_random_string(7), file_ext)) - + if max_length is None: + continue + # Truncate file_root if max_length exceeded. + truncation = len(name) - max_length + if truncation > 0: + file_root = file_root[:-truncation] + # Entire file_root was truncated in attempt to find an available filename. + if not file_root: + raise SuspiciousFileOperation( + 'Storage can not find an available filename for "%s". ' + 'Please make sure that the corresponding file field ' + 'allows sufficient "max_length".' % name + ) + name = os.path.join(dir_name, "%s_%s%s" % (file_root, get_random_string(7), file_ext)) return name def path(self, name): diff --git a/django/db/models/fields/files.py b/django/db/models/fields/files.py index 8ca8532113..301244bbef 100644 --- a/django/db/models/fields/files.py +++ b/django/db/models/fields/files.py @@ -1,5 +1,7 @@ import datetime +from inspect import getargspec import os +import warnings from django import forms from django.db.models.fields import Field @@ -11,6 +13,7 @@ from django.db.models import signals from django.utils.encoding import force_str, force_text from django.utils import six from django.utils.translation import ugettext_lazy as _ +from django.utils.deprecation import RemovedInDjango20Warning class FieldFile(File): @@ -85,7 +88,19 @@ class FieldFile(File): def save(self, name, content, save=True): name = self.field.generate_filename(self.instance, name) - self.name = self.storage.save(name, content) + + args, varargs, varkw, defaults = getargspec(self.storage.save) + if 'max_length' in args: + self.name = self.storage.save(name, content, max_length=self.field.max_length) + else: + warnings.warn( + 'Backwards compatibility for storage backends without ' + 'support for the `max_length` argument in ' + 'Storage.save() will be removed in Django 2.0.', + RemovedInDjango20Warning, stacklevel=2 + ) + self.name = self.storage.save(name, content) + setattr(self.instance, self.field.name, self.name) # Update the filesize cache diff --git a/docs/howto/custom-file-storage.txt b/docs/howto/custom-file-storage.txt index 9b217b8031..3d2c96e2bd 100644 --- a/docs/howto/custom-file-storage.txt +++ b/docs/howto/custom-file-storage.txt @@ -50,7 +50,7 @@ typically have to be overridden: * :meth:`Storage.size` * :meth:`Storage.url` -Note however that not all these methods are required and may be deliberately +Note however that not all these methods are required and may be deliberately omitted. As it happens, it is possible to leave each method unimplemented and still have a working Storage. @@ -87,7 +87,6 @@ instead). .. method:: get_valid_name(name) - Returns a filename suitable for use with the underlying storage system. The ``name`` argument passed to this method is the original filename sent to the server, after having any path information removed. Override this to customize @@ -96,21 +95,28 @@ how non-standard characters are converted to safe filenames. The code provided on ``Storage`` retains only alpha-numeric characters, periods and underscores from the original filename, removing everything else. -.. method:: get_available_name(name) +.. method:: get_available_name(name, max_length=None) Returns a filename that is available in the storage mechanism, possibly taking the provided filename into account. The ``name`` argument passed to this method will have already cleaned to a filename valid for the storage system, according to the ``get_valid_name()`` method described above. -.. versionchanged:: 1.7 +The length of the filename will not exceed ``max_length``, if provided. If a +free unique filename cannot be found, a :exc:`SuspiciousFileOperation +` exception is raised. - If a file with ``name`` already exists, an underscore plus a random 7 - character alphanumeric string is appended to the filename before the - extension. +If a file with ``name`` already exists, an underscore plus a random 7 character +alphanumeric string is appended to the filename before the extension. + +.. versionchanged:: 1.7 Previously, an underscore followed by a number (e.g. ``"_1"``, ``"_2"``, etc.) was appended to the filename until an available name in the destination directory was found. A malicious user could exploit this deterministic algorithm to create a denial-of-service attack. This change was also made in Django 1.6.6, 1.5.9, and 1.4.14. + +.. versionchanged:: 1.8 + + The ``max_length`` argument was added. diff --git a/docs/internals/deprecation.txt b/docs/internals/deprecation.txt index 345202545e..fe392cff97 100644 --- a/docs/internals/deprecation.txt +++ b/docs/internals/deprecation.txt @@ -143,6 +143,10 @@ details on these changes. * Support for the ``=`` comparison operator in the ``if`` template tag will be removed. +* The backwards compatibility shims to allow ``Storage.get_available_name()`` + and ``Storage.save()`` to be defined without a ``max_length`` argument will + be removed. + .. _deprecation-removed-in-1.9: 1.9 diff --git a/docs/ref/files/storage.txt b/docs/ref/files/storage.txt index a16b6b6c9d..88da416dcf 100644 --- a/docs/ref/files/storage.txt +++ b/docs/ref/files/storage.txt @@ -114,12 +114,17 @@ The Storage Class in the storage system, or ``False`` if the name is available for a new file. - .. method:: get_available_name(name) + .. method:: get_available_name(name, max_length=None) Returns a filename based on the ``name`` parameter that's free and available for new content to be written to on the target storage system. + The length of the filename will not exceed ``max_length``, if provided. + If a free unique filename cannot be found, a + :exc:`SuspiciousFileOperation + ` exception will be raised. + If a file with ``name`` already exists, an underscore plus a random 7 character alphanumeric string is appended to the filename before the extension. @@ -133,6 +138,10 @@ The Storage Class attack. This change was also made in Django 1.6.6, 1.5.9, and 1.4.14. + .. versionchanged:: 1.8 + + The ``max_length`` argument was added. + .. method:: get_valid_name(name) Returns a filename based on the ``name`` parameter that's suitable @@ -165,17 +174,24 @@ The Storage Class standard ``open()``. For storage systems that aren't accessible from the local filesystem, this will raise ``NotImplementedError`` instead. - .. method:: save(name, content) + .. method:: save(name, content, max_length=None) Saves a new file using the storage system, preferably with the name specified. If there already exists a file with this name ``name``, the storage system may modify the filename as necessary to get a unique name. The actual name of the stored file will be returned. + The ``max_length`` argument is passed along to + :meth:`get_available_name`. + The ``content`` argument must be an instance of :class:`django.core.files.File` or of a subclass of :class:`~django.core.files.File`. + .. versionchanged:: 1.8 + + The ``max_length`` argument was added. + .. method:: size(name) Returns the total size, in bytes, of the file referenced by ``name``. diff --git a/docs/releases/1.8.txt b/docs/releases/1.8.txt index c4e4e34894..122064971c 100644 --- a/docs/releases/1.8.txt +++ b/docs/releases/1.8.txt @@ -317,7 +317,15 @@ Email File Storage ^^^^^^^^^^^^ -* ... +* :meth:`Storage.get_available_name() + ` and + :meth:`Storage.save() ` + now take a ``max_length`` argument to implement storage-level maximum + filename length constraints. Filenames exceeding this argument will get + truncated. This prevents a database error when appending a unique suffix to a + long filename that already exists on the storage. See the :ref:`deprecation + note ` about adding this argument to your custom + storage classes. File Uploads ^^^^^^^^^^^^ @@ -1432,6 +1440,17 @@ loader that inherits ``BaseLoader``, you must inherit ``Loader`` instead. Private API ``django.test.utils.TestTemplateLoader`` is deprecated in favor of ``django.template.loaders.locmem.Loader``. +.. _storage-max-length-update: + +Support for the ``max_length`` argument on custom ``Storage`` classes +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +``Storage`` subclasses should add ``max_length=None`` as a parameter to +:meth:`~django.core.files.storage.Storage.get_available_name` and/or +:meth:`~django.core.files.storage.Storage.save` if they override either method. +Support for storages that do not accept this argument will be removed in +Django 2.0. + ``qn`` replaced by ``compiler`` ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ diff --git a/tests/file_storage/models.py b/tests/file_storage/models.py index 126cd202aa..2f58c53ddd 100644 --- a/tests/file_storage/models.py +++ b/tests/file_storage/models.py @@ -13,6 +13,19 @@ from django.db import models from django.core.files.storage import FileSystemStorage +class OldStyleFSStorage(FileSystemStorage): + """ + Storage backend without support for the ``max_length`` argument in + ``get_available_name()`` and ``save()``; for backward-compatibility and + deprecation testing. + """ + def get_available_name(self, name): + return name + + def save(self, name, content): + return super(OldStyleFSStorage, self).save(name, content) + + temp_storage_location = tempfile.mkdtemp(dir=os.environ['DJANGO_TEST_TEMP_DIR']) temp_storage = FileSystemStorage(location=temp_storage_location) @@ -31,3 +44,9 @@ class Storage(models.Model): random = models.FileField(storage=temp_storage, upload_to=random_upload_to) default = models.FileField(storage=temp_storage, upload_to='tests', default='tests/default.txt') empty = models.FileField(storage=temp_storage) + limited_length = models.FileField(storage=temp_storage, upload_to='tests', max_length=20) + extended_length = models.FileField(storage=temp_storage, upload_to='tests', max_length=300) + old_style = models.FileField( + storage=OldStyleFSStorage(location=temp_storage_location), + upload_to='tests', + ) diff --git a/tests/file_storage/tests.py b/tests/file_storage/tests.py index e1938e9ea5..8e8b0e0a32 100644 --- a/tests/file_storage/tests.py +++ b/tests/file_storage/tests.py @@ -8,6 +8,7 @@ import sys import tempfile import time import unittest +import warnings from datetime import datetime, timedelta try: @@ -16,7 +17,7 @@ except ImportError: import dummy_threading as threading from django.core.cache import cache -from django.core.exceptions import SuspiciousOperation +from django.core.exceptions import SuspiciousOperation, SuspiciousFileOperation from django.core.files.base import File, ContentFile from django.core.files.storage import FileSystemStorage, get_storage_class from django.core.files.uploadedfile import (InMemoryUploadedFile, SimpleUploadedFile, @@ -407,7 +408,7 @@ class FileStorageTests(unittest.TestCase): class CustomStorage(FileSystemStorage): - def get_available_name(self, name): + def get_available_name(self, name, max_length=None): """ Append numbers to duplicate files rather than underscores, like Trac. """ @@ -433,7 +434,7 @@ class CustomStorageTests(FileStorageTests): self.storage.delete(second) -class FileFieldStorageTests(unittest.TestCase): +class FileFieldStorageTests(SimpleTestCase): def tearDown(self): shutil.rmtree(temp_storage_location) @@ -503,6 +504,69 @@ class FileFieldStorageTests(unittest.TestCase): for o in objs: o.delete() + def test_file_truncation(self): + # Given the max_length is limited, when multiple files get uploaded + # under the same name, then the filename get truncated in order to fit + # in _(7 random chars). When most of the max_length is taken by + # dirname + extension and there are not enough characters in the + # filename to truncate, an exception should be raised. + objs = [Storage() for i in range(2)] + filename = 'filename.ext' + + for o in objs: + o.limited_length.save(filename, ContentFile('Same Content')) + try: + # Testing truncation. + names = [o.limited_length.name for o in objs] + self.assertEqual(names[0], 'tests/%s' % filename) + six.assertRegex(self, names[1], 'tests/fi_%s.ext' % FILE_SUFFIX_REGEX) + + # Testing exception is raised when filename is too short to truncate. + filename = 'short.longext' + objs[0].limited_length.save(filename, ContentFile('Same Content')) + self.assertRaisesMessage( + SuspiciousFileOperation, 'Storage can not find an available filename', + objs[1].limited_length.save, *(filename, ContentFile('Same Content')) + ) + finally: + for o in objs: + o.delete() + + def test_extended_length_storage(self): + # Testing FileField with max_length > 255. Most systems have filename + # length limitation of 255. Path takes extra chars. + filename = 251 * 'a' # 4 chars for extension. + obj = Storage() + obj.extended_length.save('%s.txt' % filename, ContentFile('Same Content')) + self.assertEqual(obj.extended_length.name, 'tests/%s.txt' % filename) + self.assertEqual(obj.extended_length.read(), b'Same Content') + obj.extended_length.close() + + def test_old_style_storage(self): + # Testing backward-compatibility with old-style storage backends that + # don't take ``max_length`` parameter in ``get_available_name()`` + # and save(). A deprecation warning should be raised. + obj = Storage() + with warnings.catch_warnings(record=True) as warns: + warnings.simplefilter('always') + obj.old_style.save('deprecated_storage_test.txt', ContentFile('Same Content')) + self.assertEqual(len(warns), 2) + self.assertEqual( + str(warns[0].message), + 'Backwards compatibility for storage backends without support for ' + 'the `max_length` argument in Storage.save() will be removed in ' + 'Django 2.0.' + ) + self.assertEqual( + str(warns[1].message), + 'Backwards compatibility for storage backends without support for ' + 'the `max_length` argument in Storage.get_available_name() will ' + 'be removed in Django 2.0.' + ) + self.assertEqual(obj.old_style.name, 'tests/deprecated_storage_test.txt') + self.assertEqual(obj.old_style.read(), b'Same Content') + obj.old_style.close() + def test_filefield_default(self): # Default values allow an object to access a single file. temp_storage.save('tests/default.txt', ContentFile('default content'))