Commit 706b33fe authored by Aymeric Augustin's avatar Aymeric Augustin
Browse files

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.
parent c6280796

django/contrib/staticfiles/storage.py

+40 −31
Original line number Diff line number Diff line
@@ -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



            # 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

            name_parts = name.split(os.sep)

            # Using posix normpath here to remove duplicates



            # 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

            else:

                if sub_level:

                    if sub_level == 1:

                        parent_level -= 1

                    start, end = parent_level, 1

                # Otherwise the condition above would have returned prematurely.

                assert url_path.startswith(settings.STATIC_URL)

                target_name = url_path[len(settings.STATIC_URL):]

            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

tests/staticfiles_tests/project/documents/absolute_root.css

0 → 100644
+1 −0
Original line number Diff line number Diff line 
@import url("/static/styles_root.css");

tests/staticfiles_tests/project/documents/cached/absolute.css

+1 −0
Original line number Diff line number Diff line 
@import url("/static/cached/styles.css");

@import url("/static/styles_root.css");

body {

    background: #d3d6d8 url(/static/cached/img/relative.png);

}

tests/staticfiles_tests/project/documents/styles_root.css

0 → 100644
+1 −0
Original line number Diff line number Diff line 
/* see cached/absolute.css and absolute_root.css */

tests/staticfiles_tests/test_storage.py

+14 −1
Original line number Diff line number Diff line
@@ -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")