Fix S3 safe_join() to allow colons

Combine the identical s3boto3 and s3boto implementations of safe_join()
and its tests to reduce code duplication.

Fixes #248

storages/backends/s3boto3.py

@@ -22,7 +22,7 @@
     raise ImproperlyConfigured("Could not load Boto3's S3 bindings.\n"
                                "See https://github.com/boto/boto3")
 
-from storages.utils import setting
+from storages.utils import setting, safe_join
 
 boto3_version_info = tuple([int(i) for i in boto3_version.split('.')])
 
@@ -31,39 +31,6 @@
                                "higher.\nSee https://github.com/boto/boto3")
 
 
-def safe_join(base, *paths):
-    """
-    A version of django.utils._os.safe_join for S3 paths.
-
-    Joins one or more path components to the base path component
-    intelligently. Returns a normalized version of the final path.
-
-    The final path must be located inside of the base path component
-    (otherwise a ValueError is raised).
-
-    Paths outside the base path indicate a possible security
-    sensitive operation.
-    """
-    base_path = force_text(base)
-    base_path = base_path.rstrip('/')
-    paths = [force_text(p) for p in paths]
-
-    final_path = base_path
-    for path in paths:
-        final_path = urlparse.urljoin(final_path.rstrip('/') + "/", path)
-
-    # Ensure final_path starts with base_path and that the next character after
-    # the final path is '/' (or nothing, in which case final_path must be
-    # equal to base_path).
-    base_path_len = len(base_path)
-    if (not final_path.startswith(base_path) or
-            final_path[base_path_len:base_path_len + 1] not in ('', '/')):
-        raise ValueError('the joined path is located outside of the base path'
-                         ' component')
-
-    return final_path.lstrip('/')
-
-
 @deconstructible
 class S3Boto3StorageFile(File):
 

storages/utils.py

@@ -3,7 +3,6 @@
 from django.conf import settings
 from django.core.exceptions import ImproperlyConfigured
 from django.utils.encoding import force_text
-from django.utils.six.moves.urllib import parse as urlparse
 
 
 def setting(name, default=None, strict=False):
@@ -63,16 +62,15 @@ def safe_join(base, *paths):
     base_path = base_path.rstrip('/')
     paths = [force_text(p) for p in paths]
 
-    final_path = base_path
-    for path in paths:
-        final_path = urlparse.urljoin(final_path.rstrip('/') + '/', path)
+    final_path = posixpath.normpath(posixpath.join(base_path + '/', *paths))
+    # posixpath.normpath() strips the trailing /. Add it back.
+    if paths[-1].endswith('/'):
+        final_path += '/'
 
     # Ensure final_path starts with base_path and that the next character after
-    # the final path is '/' (or nothing, in which case final_path must be
-    # equal to base_path).
+    # the final path is /.
     base_path_len = len(base_path)
-    if (not final_path.startswith(base_path) or
-            final_path[base_path_len:base_path_len + 1] not in ('', '/')):
+    if (not final_path.startswith(base_path) or final_path[base_path_len] != '/'):
         raise ValueError('the joined path is located outside of the base path'
                          ' component')
 

tests/test_s3boto3.py

@@ -18,55 +18,13 @@
 
 from storages.backends import s3boto3
 
-__all__ = (
-    'SafeJoinTest',
-    'S3Boto3StorageTests',
-)
-
 
 class S3Boto3TestCase(TestCase):
     def setUp(self):
         self.storage = s3boto3.S3Boto3Storage()
         self.storage._connection = mock.MagicMock()
 
 
-class SafeJoinTest(TestCase):
-    def test_normal(self):
-        path = s3boto3.safe_join("", "path/to/somewhere", "other", "path/to/somewhere")
-        self.assertEqual(path, "path/to/somewhere/other/path/to/somewhere")
-
-    def test_with_dot(self):
-        path = s3boto3.safe_join("", "path/./somewhere/../other", "..",
-                                 ".", "to/./somewhere")
-        self.assertEqual(path, "path/to/somewhere")
-
-    def test_base_url(self):
-        path = s3boto3.safe_join("base_url", "path/to/somewhere")
-        self.assertEqual(path, "base_url/path/to/somewhere")
-
-    def test_base_url_with_slash(self):
-        path = s3boto3.safe_join("base_url/", "path/to/somewhere")
-        self.assertEqual(path, "base_url/path/to/somewhere")
-
-    def test_suspicious_operation(self):
-        self.assertRaises(ValueError,
-                          s3boto3.safe_join, "base", "../../../../../../../etc/passwd")
-
-    def test_trailing_slash(self):
-        """
-        Test safe_join with paths that end with a trailing slash.
-        """
-        path = s3boto3.safe_join("base_url/", "path/to/somewhere/")
-        self.assertEqual(path, "base_url/path/to/somewhere/")
-
-    def test_trailing_slash_multi(self):
-        """
-        Test safe_join with multiple paths that end with a trailing slash.
-        """
-        path = s3boto3.safe_join("base_url/", "path/to/" "somewhere/")
-        self.assertEqual(path, "base_url/path/to/somewhere/")
-
-
 class S3Boto3StorageTests(S3Boto3TestCase):
 
     def test_clean_name(self):

tests/test_utils.py

@@ -1,3 +1,5 @@
+import datetime
+
 from django.test import TestCase
 from django.conf import settings
 from django.core.exceptions import ImproperlyConfigured
@@ -65,8 +67,10 @@ def test_base_url_with_slash(self):
         self.assertEqual(path, "base_url/path/to/somewhere")
 
     def test_suspicious_operation(self):
-        self.assertRaises(ValueError,
-                          utils.safe_join, "base", "../../../../../../../etc/passwd")
+        with self.assertRaises(ValueError):
+            utils.safe_join("base", "../../../../../../../etc/passwd")
+        with self.assertRaises(ValueError):
+            utils.safe_join("base", "/etc/passwd")
 
     def test_trailing_slash(self):
         """
@@ -79,5 +83,10 @@ def test_trailing_slash_multi(self):
         """
         Test safe_join with multiple paths that end with a trailing slash.
         """
-        path = utils.safe_join("base_url/", "path/to/" "somewhere/")
+        path = utils.safe_join("base_url/", "path/to/", "somewhere/")
         self.assertEqual(path, "base_url/path/to/somewhere/")
+
+    def test_datetime_isoformat(self):
+        dt = datetime.datetime(2017, 5, 19, 14, 45, 37, 123456)
+        path = utils.safe_join('base_url', dt.isoformat())
+        self.assertEqual(path, 'base_url/2017-05-19T14:45:37.123456')