Fixed django#21080 -- Ignored urls inside comments during collectstatic.

Thanks Mariusz Felisiak for the review.

Co-authored-by: Nathan Gaberel <nathan@gnab.fr>

Author: 2 people

django/contrib/staticfiles/storage.py

@@ -11,6 +11,12 @@
 from django.core.files.base import ContentFile
 from django.core.files.storage import FileSystemStorage, storages
 from django.utils.functional import LazyObject
+from django.utils.regex_helper import _lazy_re_compile
+
+comment_re = _lazy_re_compile(r"\/\*[^*]*\*+([^/*][^*]*\*+)*\/", re.DOTALL)
+line_comment_re = _lazy_re_compile(
+    r"\/\*[^*]*\*+([^/*][^*]*\*+)*\/|\/\/[^\n]*", re.DOTALL
+)
 
 
 class StaticFilesStorage(FileSystemStorage):
@@ -204,7 +210,22 @@ def url(self, name, force=False):
         """
         return self._url(self.stored_name, name, force)
 
-    def url_converter(self, name, hashed_files, template=None):
+    def get_comment_blocks(self, content, include_line_comments=False):
+        """
+        Return a list of (start, end) tuples for each comment block.
+        """
+        pattern = line_comment_re if include_line_comments else comment_re
+        return [(match.start(), match.end()) for match in re.finditer(pattern, content)]
+
+    def is_in_comment(self, pos, comments):
+        for start, end in comments:
+            if start < pos and pos < end:
+                return True
+            if pos < start:
+                return False
+        return False
+
+    def url_converter(self, name, hashed_files, template=None, comment_blocks=None):
         """
         Return the custom URL converter for the given file name.
         """
@@ -222,6 +243,10 @@ def converter(matchobj):
             matched = matches["matched"]
             url = matches["url"]
 
+            # Ignore URLs in comments.
+            if comment_blocks and self.is_in_comment(matchobj.start(), comment_blocks):
+                return matched
+
             # Ignore absolute/protocol-relative and data-uri URLs.
             if re.match(r"^[a-z]+:", url) or url.startswith("//"):
                 return matched
@@ -375,7 +400,13 @@ def path_level(name):
                         if matches_patterns(path, (extension,)):
                             for pattern, template in patterns:
                                 converter = self.url_converter(
-                                    name, hashed_files, template
+                                    name,
+                                    hashed_files,
+                                    template,
+                                    self.get_comment_blocks(
+                                        content,
+                                        include_line_comments=path.endswith(".js"),
+                                    ),
                                 )
                                 try:
                                     content = pattern.sub(converter, content)

docs/ref/contrib/staticfiles.txt

@@ -349,12 +349,6 @@ argument. For example::
             manifest_storage = StaticFilesStorage(location=settings.BASE_DIR)
             super().__init__(*args, manifest_storage=manifest_storage, **kwargs)
 
-.. admonition:: References in comments
-
-    ``ManifestStaticFilesStorage`` doesn't ignore paths in statements that are
-    commented out. This :ticket:`may crash on the nonexistent paths <21080>`.
-    You should check and eventually strip comments.
-
 .. attribute:: storage.ManifestStaticFilesStorage.manifest_hash
 
 This attribute provides a single hash that changes whenever a file in the

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

@@ -8,3 +8,24 @@ body {
     background: url();
 }
 
+/* @import url("non_exist.css") */
+
+/* url("non_exist.png") */
+
+/*
+
+@import url("non_exist.css")
+
+url("non_exist.png")
+
+@import url(other.css)
+
+*/
+
+body {
+    background: #d3d6d8 /*url("does.not.exist.png")*/ url(/static/cached/img/relative.png);
+}
+
+body {
+    background: #d3d6d8 /* url("does.not.exist.png") */ url(/static/cached/img/relative.png) /*url("does.not.exist.either.png")*/;
+}

tests/staticfiles_tests/project/documents/cached/module.js

@@ -24,3 +24,14 @@ export {
     firstVar as firstVarAlias,
     secondVar as secondVarAlias
 } from "./module_test.js";
+
+// ignore block comments
+/* export * from "./module_test_missing.js"; */
+/*
+import rootConst from "/static/absolute_root_missing.js";
+const dynamicModule = import("./module_test_missing.js");
+*/
+
+// ignore line comments
+// import testConst from "./module_test_missing.js";
+// const dynamicModule = import("./module_test_missing.js");

tests/staticfiles_tests/test_storage.py

@@ -65,7 +65,7 @@ def test_template_tag_simple_content(self):
 
     def test_path_ignored_completely(self):
         relpath = self.hashed_file_path("cached/css/ignored.css")
-        self.assertEqual(relpath, "cached/css/ignored.55e7c226dda1.css")
+        self.assertEqual(relpath, "cached/css/ignored.0e15ac4a4fb4.css")
         with storage.staticfiles_storage.open(relpath) as relfile:
             content = relfile.read()
             self.assertIn(b"#foobar", content)
@@ -75,6 +75,22 @@ def test_path_ignored_completely(self):
             self.assertIn(b"chrome:foobar", content)
             self.assertIn(b"//foobar", content)
             self.assertIn(b"url()", content)
+            self.assertIn(b'/* @import url("non_exist.css") */', content)
+            self.assertIn(b'/* url("non_exist.png") */', content)
+            self.assertIn(b'@import url("non_exist.css")', content)
+            self.assertIn(b'url("non_exist.png")', content)
+            self.assertIn(b"@import url(other.css)", content)
+            self.assertIn(
+                b'background: #d3d6d8 /*url("does.not.exist.png")*/ '
+                b'url("/static/cached/img/relative.acae32e4532b.png");',
+                content,
+            )
+            self.assertIn(
+                b'background: #d3d6d8 /* url("does.not.exist.png") */ '
+                b'url("/static/cached/img/relative.acae32e4532b.png") '
+                b'/*url("does.not.exist.either.png")*/',
+                content,
+            )
         self.assertPostCondition()
 
     def test_path_with_querystring(self):
@@ -698,7 +714,7 @@ class TestCollectionJSModuleImportAggregationManifestStorage(CollectionTestCase)
 
     def test_module_import(self):
         relpath = self.hashed_file_path("cached/module.js")
-        self.assertEqual(relpath, "cached/module.4326210cf0bd.js")
+        self.assertEqual(relpath, "cached/module.eaa407b94311.js")
         tests = [
             # Relative imports.
             b'import testConst from "./module_test.477bbebe77f0.js";',
@@ -721,6 +737,15 @@ def test_module_import(self):
             b"    firstVar1 as firstVarAlias,\n"
             b"    $second_var_2 as secondVarAlias\n"
             b'} from "./module_test.477bbebe77f0.js";',
+            # Ignore block comments
+            b'/* export * from "./module_test_missing.js"; */',
+            b"/*\n"
+            b'import rootConst from "/static/absolute_root_missing.js";\n'
+            b'const dynamicModule = import("./module_test_missing.js");\n'
+            b"*/",
+            # Ignore line comments
+            b'// import testConst from "./module_test_missing.js";',
+            b'// const dynamicModule = import("./module_test_missing.js");',
         ]
         with storage.staticfiles_storage.open(relpath) as relfile:
             content = relfile.read()
@@ -730,7 +755,7 @@ def test_module_import(self):
 
     def test_aggregating_modules(self):
         relpath = self.hashed_file_path("cached/module.js")
-        self.assertEqual(relpath, "cached/module.4326210cf0bd.js")
+        self.assertEqual(relpath, "cached/module.eaa407b94311.js")
         tests = [
             b'export * from "./module_test.477bbebe77f0.js";',
             b'export { testConst } from "./module_test.477bbebe77f0.js";',