From 706b33fef80b8b0901fdd2e954e5e5ea189a528a Mon Sep 17 00:00:00 2001 From: Aymeric Augustin Date: Sat, 20 Feb 2016 20:54:18 +0100 Subject: [PATCH] Fixed #26249 -- Fixed collectstatic crash for files in STATIC_ROOT referenced by absolute URL. collectstatic crashed when: * a hashing static file storage backend was used * a static file referenced another static file located directly in STATIC_ROOT (not a subdirectory) with an absolute URL (which must start with STATIC_URL, which cannot be empty) It seems to me that the current code reimplements relative path joining and doesn't handle edge cases correctly. I suspect it assumes that STATIC_URL is of the form r'/[^/]+/'. Throwing out that code in favor of the posixpath module makes the logic easier to follow. Handling absolute paths correctly also becomes easier. --- django/contrib/staticfiles/storage.py | 71 +++++++++++-------- .../project/documents/absolute_root.css | 1 + .../project/documents/cached/absolute.css | 1 + .../project/documents/styles_root.css | 1 + tests/staticfiles_tests/test_storage.py | 15 +++- 5 files changed, 57 insertions(+), 32 deletions(-) create mode 100644 tests/staticfiles_tests/project/documents/absolute_root.css create mode 100644 tests/staticfiles_tests/project/documents/styles_root.css diff --git a/django/contrib/staticfiles/storage.py b/django/contrib/staticfiles/storage.py index 58cb847f1f..636ba0da5f 100644 --- a/django/contrib/staticfiles/storage.py +++ b/django/contrib/staticfiles/storage.py @@ -75,7 +75,7 @@ class HashedFilesMixin(object): def file_hash(self, name, content=None): """ - Returns a hash of the file with the given name and optional content. + Return a hash of the file with the given name and optional content. """ if content is None: return None @@ -119,7 +119,7 @@ class HashedFilesMixin(object): def url(self, name, force=False): """ - Returns the real URL in DEBUG mode. + Return the real URL in DEBUG mode. """ if settings.DEBUG and not force: hashed_name, fragment = name, '' @@ -147,51 +147,60 @@ class HashedFilesMixin(object): def url_converter(self, name, template=None): """ - Returns the custom URL converter for the given file name. + Return the custom URL converter for the given file name. """ if template is None: template = self.default_template def converter(matchobj): """ - Converts the matched URL depending on the parent level (`..`) - and returns the normalized and hashed URL using the url method - of the storage. + Convert the matched URL to a normalized and hashed URL. + + This requires figuring out which files the matched URL resolves + to and calling the url() method of the storage. """ matched, url = matchobj.groups() - # Completely ignore http(s) prefixed URLs, - # fragments and data-uri URLs - if url.startswith(('#', 'http:', 'https:', 'data:', '//')): + + # Ignore absolute/protocol-relative, fragments and data-uri URLs. + if url.startswith(('http:', 'https:', '//', '#', 'data:')): return matched - name_parts = name.split(os.sep) - # Using posix normpath here to remove duplicates + + # Ignore absolute URLs that don't point to a static file (dynamic + # CSS / JS?). Note that STATIC_URL cannot be empty. + if url.startswith('/') and not url.startswith(settings.STATIC_URL): + return matched + + # This is technically not useful and could be considered a bug: + # we're making changes to our user's code for no good reason. + # Removing it makes test_template_tag_denorm fail, though, and I'm + # working on another bug, so I'm going to leave it there for now. + # When someone complains that /foo/bar#a/../b gets changed to + # /foo/bar#b, just remove it, as well as test_template_tag_denorm. url = posixpath.normpath(url) - # Strip off the fragment so that a path-like fragment won't confuse - # the lookup. + + # Strip off the fragment so a path-like fragment won't interfere. url_path, fragment = urldefrag(url) - url_parts = url_path.split('/') - parent_level, sub_level = url_path.count('..'), url_path.count('/') + if url_path.startswith('/'): - sub_level -= 1 - url_parts = url_parts[1:] - if parent_level or not url_path.startswith('/'): - start, end = parent_level + 1, parent_level + # Otherwise the condition above would have returned prematurely. + assert url_path.startswith(settings.STATIC_URL) + target_name = url_path[len(settings.STATIC_URL):] else: - if sub_level: - if sub_level == 1: - parent_level -= 1 - start, end = parent_level, 1 - else: - start, end = 1, sub_level - 1 - joined_result = '/'.join(name_parts[:-start] + url_parts[end:]) - hashed_url = self.url(unquote(joined_result), force=True) - file_name = hashed_url.split('/')[-1:] - relative_url = '/'.join(url_path.split('/')[:-1] + file_name) + # We're using the posixpath module to mix paths and URLs conveniently. + source_name = name if os.sep == '/' else name.replace(os.sep, '/') + target_name = posixpath.join(posixpath.dirname(source_name), url_path) + + # Determine the hashed name of the target file with the storage backend. + hashed_url = self.url(unquote(target_name), force=True) + + transformed_url = '/'.join(url_path.split('/')[:-1] + hashed_url.split('/')[-1:]) + + # Restore the fragment that was stripped off earlier. if fragment: - relative_url += '?#%s' % fragment if '?#' in url else '#%s' % fragment + transformed_url += ('?#' if '?#' in url else '#') + fragment # Return the hashed version to the file - return template % unquote(relative_url) + return template % unquote(transformed_url) return converter diff --git a/tests/staticfiles_tests/project/documents/absolute_root.css b/tests/staticfiles_tests/project/documents/absolute_root.css new file mode 100644 index 0000000000..b35ff47c9e --- /dev/null +++ b/tests/staticfiles_tests/project/documents/absolute_root.css @@ -0,0 +1 @@ +@import url("/static/styles_root.css"); diff --git a/tests/staticfiles_tests/project/documents/cached/absolute.css b/tests/staticfiles_tests/project/documents/cached/absolute.css index 03f2ae791a..6a2040b413 100644 --- a/tests/staticfiles_tests/project/documents/cached/absolute.css +++ b/tests/staticfiles_tests/project/documents/cached/absolute.css @@ -1,4 +1,5 @@ @import url("/static/cached/styles.css"); +@import url("/static/styles_root.css"); body { background: #d3d6d8 url(/static/cached/img/relative.png); } diff --git a/tests/staticfiles_tests/project/documents/styles_root.css b/tests/staticfiles_tests/project/documents/styles_root.css new file mode 100644 index 0000000000..64512630cb --- /dev/null +++ b/tests/staticfiles_tests/project/documents/styles_root.css @@ -0,0 +1 @@ +/* see cached/absolute.css and absolute_root.css */ diff --git a/tests/staticfiles_tests/test_storage.py b/tests/staticfiles_tests/test_storage.py index f0c69cebe6..9de5a28223 100644 --- a/tests/staticfiles_tests/test_storage.py +++ b/tests/staticfiles_tests/test_storage.py @@ -96,13 +96,26 @@ class TestHashedFiles(object): def test_template_tag_absolute(self): relpath = self.hashed_file_path("cached/absolute.css") - self.assertEqual(relpath, "cached/absolute.ae9ef2716fe3.css") + self.assertEqual(relpath, "cached/absolute.df312c6326e1.css") with storage.staticfiles_storage.open(relpath) as relfile: content = relfile.read() self.assertNotIn(b"/static/cached/styles.css", content) self.assertIn(b"/static/cached/styles.bb84a0240107.css", content) + self.assertNotIn(b"/static/styles_root.css", content) + self.assertIn(b"/static/styles_root.401f2509a628.css", content) self.assertIn(b'/static/cached/img/relative.acae32e4532b.png', content) + def test_template_tag_absolute_root(self): + """ + Like test_template_tag_absolute, but for a file in STATIC_ROOT (#26249). + """ + relpath = self.hashed_file_path("absolute_root.css") + self.assertEqual(relpath, "absolute_root.f864a4d7f083.css") + with storage.staticfiles_storage.open(relpath) as relfile: + content = relfile.read() + self.assertNotIn(b"/static/styles_root.css", content) + self.assertIn(b"/static/styles_root.401f2509a628.css", content) + def test_template_tag_denorm(self): relpath = self.hashed_file_path("cached/denorm.css") self.assertEqual(relpath, "cached/denorm.c5bd139ad821.css")