From 7f6fbc906a21e9f8db36e06ace2a9b687aa26130 Mon Sep 17 00:00:00 2001 From: Aymeric Augustin Date: Tue, 23 Feb 2016 10:51:54 +0100 Subject: [PATCH] Prevented static file corruption when URL fragment contains '..'. When running collectstatic with a hashing static file storage backend, URLs referencing other files were normalized with posixpath.normpath. This could corrupt URLs: for example 'a.css#b/../c' became just 'c'. Normalization seems to be an artifact of the historical implementation. It contained a home-grown implementation of posixpath.join which relied on counting occurrences of .. and /, so multiple / had to be collapsed. The new implementation introduced in the previous commit doesn't suffer from this issue. So it seems safe to remove the normalization. There was a test for this normalization behavior but I don't think it's a good test. Django shouldn't modify CSS that way. If a developer has rendundant /s, it's mostly an aesthetic issue and it isn't Django's job to fix it. Conversely, if the user wants a series of /s, perhaps in the URL fragment, Django shouldn't destroy it. Refs #26249. --- django/contrib/staticfiles/storage.py | 8 -------- .../project/documents/cached/css/fragments.css | 2 +- .../project/documents/cached/denorm.css | 4 ---- tests/staticfiles_tests/test_storage.py | 14 ++------------ 4 files changed, 3 insertions(+), 25 deletions(-) delete mode 100644 tests/staticfiles_tests/project/documents/cached/denorm.css diff --git a/django/contrib/staticfiles/storage.py b/django/contrib/staticfiles/storage.py index 636ba0da5f6..8949969c587 100644 --- a/django/contrib/staticfiles/storage.py +++ b/django/contrib/staticfiles/storage.py @@ -170,14 +170,6 @@ class HashedFilesMixin(object): 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 a path-like fragment won't interfere. url_path, fragment = urldefrag(url) diff --git a/tests/staticfiles_tests/project/documents/cached/css/fragments.css b/tests/staticfiles_tests/project/documents/cached/css/fragments.css index e6e70494651..533d7617aae 100644 --- a/tests/staticfiles_tests/project/documents/cached/css/fragments.css +++ b/tests/staticfiles_tests/project/documents/cached/css/fragments.css @@ -1,7 +1,7 @@ @font-face { src: url('fonts/font.eot?#iefix') format('embedded-opentype'), url('fonts/font.svg#webfontIyfZbseF') format('svg'); - url('fonts/font.svg#../path/to/fonts/font.svg') format('svg'); + url('fonts/font.svg#path/to/../../fonts/font.svg') format('svg'); url('data:font/woff;charset=utf-8;base64,d09GRgABAAAAADJoAA0AAAAAR2QAAQAAAAAAAAAAAAA'); } div { diff --git a/tests/staticfiles_tests/project/documents/cached/denorm.css b/tests/staticfiles_tests/project/documents/cached/denorm.css deleted file mode 100644 index d6567b00dd6..00000000000 --- a/tests/staticfiles_tests/project/documents/cached/denorm.css +++ /dev/null @@ -1,4 +0,0 @@ -@import url("..//cached///styles.css"); -body { - background: #d3d6d8 url(img/relative.png ); -} diff --git a/tests/staticfiles_tests/test_storage.py b/tests/staticfiles_tests/test_storage.py index 9de5a282236..edb5339bcf9 100644 --- a/tests/staticfiles_tests/test_storage.py +++ b/tests/staticfiles_tests/test_storage.py @@ -85,12 +85,12 @@ class TestHashedFiles(object): def test_path_with_querystring_and_fragment(self): relpath = self.hashed_file_path("cached/css/fragments.css") - self.assertEqual(relpath, "cached/css/fragments.ef92012a8c16.css") + self.assertEqual(relpath, "cached/css/fragments.59dc2b188043.css") with storage.staticfiles_storage.open(relpath) as relfile: content = relfile.read() self.assertIn(b'fonts/font.a4b0478549d0.eot?#iefix', content) self.assertIn(b'fonts/font.b8d603e42714.svg#webfontIyfZbseF', content) - self.assertIn(b'fonts/font.b8d603e42714.svg#../path/to/fonts/font.svg', content) + self.assertIn(b'fonts/font.b8d603e42714.svg#path/to/../../fonts/font.svg', content) self.assertIn(b'data:font/woff;charset=utf-8;base64,d09GRgABAAAAADJoAA0AAAAAR2QAAQAAAAAAAAAAAAA', content) self.assertIn(b'#default#VML', content) @@ -116,16 +116,6 @@ class TestHashedFiles(object): 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") - with storage.staticfiles_storage.open(relpath) as relfile: - content = relfile.read() - self.assertNotIn(b"..//cached///styles.css", content) - self.assertIn(b"../cached/styles.bb84a0240107.css", content) - self.assertNotIn(b"url(img/relative.png )", content) - self.assertIn(b'url("img/relative.acae32e4532b.png', content) - def test_template_tag_relative(self): relpath = self.hashed_file_path("cached/relative.css") self.assertEqual(relpath, "cached/relative.b0375bd89156.css")