jazzband / django-pipeline

Pipeline is an asset packaging library for Django.
https://django-pipeline.readthedocs.io/
MIT License
1.52k stars 372 forks source link

URL Rewriter breaks valid CSS #615

Open dbrgn opened 7 years ago

dbrgn commented 7 years ago

I noticed that django-pipeline breaks the minified bootstrap 4 CSS file, even when using the no-op compressor. This affects at least 1.6.0, 1.6.10 and master.

I dug into the code and tried to fix it for about an hour, but that URL processing seems quite convoluted. Sorry :)

The problem is with CSS like this:

.data-url-quoted-mixed {
  background-image:url("data:image/svg+xml;charset=utf8,%3Csvg viewBox='0 0 32 32' xmlns='http://www.w3.org/2000/svg'%3E%3Cpath stroke='rgba(255, 255, 255, 0.5)' stroke-width='2' stroke-linecap='round' stroke-miterlimit='10' d='M4 8h24M4 16h24M4 24h24'/%3E%3C/svg%3E");
}

Because the URL detector regex looks like this:

URL_DETECTOR = r"""url\((['"]){0,1}\s*(.*?)["']{0,1}\)"""

...it does not ensure that both matched quotes are the same.

I tried to solve the issue with a backreference:

URL_DETECTOR = r"""url\((['"]){0,1}\s*(.*?)\1\)"""

...but that seems to break some other tests.

Anyways, I couldn't come up with a fix, but here's a test case that should cover this issue:

diff --git a/tests/assets/css/nested/nested.css b/tests/assets/css/nested/nested.css
index 8adac3c..07a54f1 100644
--- a/tests/assets/css/nested/nested.css
+++ b/tests/assets/css/nested/nested.css
@@ -4,3 +4,6 @@
 .data-url-quoted {
   background-image: url('data:image/svg+xml;charset=US-ASCII,%3C%3Fxml%20version%3D%221.0%22%20encoding%3D%22iso-8859-1%22%3F%3E%3C!DOCTYPE%20svg%20PUBLIC%20%22-%2F%2FW3C%2F%2FDTD%20SVG%201.1%2F%2FEN%22%20%22http%3A%2F%2Fwww.w3.org%2FGraphics%2FSVG%2F1.1%2FDTD%2Fsvg11.dtd%22%3E%3Csvg%20version%3D%221.1%22%20id%3D%22Layer_1%22%20xmlns%3D%22http%3A%2F%2Fwww.w3.org%2F2000%2Fsvg%22%20xmlns%3Axlink%3D%22http%3A%2F%2Fwww.w3.org%2F1999%2Fxlink%22%20x%3D%220px%22%20y%3D%220px%22%20%20width%3D%2212px%22%20height%3D%2214px%22%20viewBox%3D%220%200%2012%2014%22%20style%3D%22enable-background%3Anew%200%200%2012%2014%3B%22%20xml%3Aspace%3D%22preserve%22%3E%3Cpath%20d%3D%22M11%2C6V5c0-2.762-2.239-5-5-5S1%2C2.238%2C1%2C5v1H0v8h12V6H11z%20M6.5%2C9.847V12h-1V9.847C5.207%2C9.673%2C5%2C9.366%2C5%2C9%20c0-0.553%2C0.448-1%2C1-1s1%2C0.447%2C1%2C1C7%2C9.366%2C6.793%2C9.673%2C6.5%2C9.847z%20M9%2C6H3V5c0-1.657%2C1.343-3%2C3-3s3%2C1.343%2C3%2C3V6z%22%2F%3E%3Cg%3E%3C%2Fg%3E%3Cg%3E%3C%2Fg%3E%3Cg%3E%3C%2Fg%3E%3Cg%3E%3C%2Fg%3E%3Cg%3E%3C%2Fg%3E%3Cg%3E%3C%2Fg%3E%3Cg%3E%3C%2Fg%3E%3Cg%3E%3C%2Fg%3E%3Cg%3E%3C%2Fg%3E%3Cg%3E%3C%2Fg%3E%3Cg%3E%3C%2Fg%3E%3Cg%3E%3C%2Fg%3E%3Cg%3E%3C%2Fg%3E%3Cg%3E%3C%2Fg%3E%3Cg%3E%3C%2Fg%3E%3C%2Fsvg%3E');
 }
+.data-url-quoted-mixed {
+  background-image:url("data:image/svg+xml;charset=utf8,%3Csvg viewBox='0 0 32 32' xmlns='http://www.w3.org/2000/svg'%3E%3Cpath stroke='rgba(255, 255, 255, 0.5)' stroke-width='2' stroke-linecap='round' stroke-miterlimit='10' d='M4 8h24M4 16h24M4 24h24'/%3E%3C/svg%3E");
+}
diff --git a/tests/tests/test_compressor.py b/tests/tests/test_compressor.py
index 4291b51..6b55b2c 100644
--- a/tests/tests/test_compressor.py
+++ b/tests/tests/test_compressor.py
@@ -177,6 +177,9 @@ class CompressorTest(TestCase):
 .data-url-quoted {
   background-image: url('data:image/svg+xml;charset=US-ASCII,%3C%3Fxml%20version%3D%221.0%22%20encoding%3D%22iso-8859-1%22%3F%3E%3C!DOCTYPE%20svg%20PUBLIC%20%22-%2F%2FW3C%2F%2FDTD%20SVG%201.1%2F%2FEN%22%20%22http%3A%2F%2Fwww.w3.org%2FGraphics%2FSVG%2F1.1%2FDTD%2Fsvg11.dtd%22%3E%3Csvg%20version%3D%221.1%22%20id%3D%22Layer_1%22%20xmlns%3D%22http%3A%2F%2Fwww.w3.org%2F2000%2Fsvg%22%20xmlns%3Axlink%3D%22http%3A%2F%2Fwww.w3.org%2F1999%2Fxlink%22%20x%3D%220px%22%20y%3D%220px%22%20%20width%3D%2212px%22%20height%3D%2214px%22%20viewBox%3D%220%200%2012%2014%22%20style%3D%22enable-background%3Anew%200%200%2012%2014%3B%22%20xml%3Aspace%3D%22preserve%22%3E%3Cpath%20d%3D%22M11%2C6V5c0-2.762-2.239-5-5-5S1%2C2.238%2C1%2C5v1H0v8h12V6H11z%20M6.5%2C9.847V12h-1V9.847C5.207%2C9.673%2C5%2C9.366%2C5%2C9%20c0-0.553%2C0.448-1%2C1-1s1%2C0.447%2C1%2C1C7%2C9.366%2C6.793%2C9.673%2C6.5%2C9.847z%20M9%2C6H3V5c0-1.657%2C1.343-3%2C3-3s3%2C1.343%2C3%2C3V6z%22%2F%3E%3Cg%3E%3C%2Fg%3E%3Cg%3E%3C%2Fg%3E%3Cg%3E%3C%2Fg%3E%3Cg%3E%3C%2Fg%3E%3Cg%3E%3C%2Fg%3E%3Cg%3E%3C%2Fg%3E%3Cg%3E%3C%2Fg%3E%3Cg%3E%3C%2Fg%3E%3Cg%3E%3C%2Fg%3E%3Cg%3E%3C%2Fg%3E%3Cg%3E%3C%2Fg%3E%3Cg%3E%3C%2Fg%3E%3Cg%3E%3C%2Fg%3E%3Cg%3E%3C%2Fg%3E%3Cg%3E%3C%2Fg%3E%3C%2Fsvg%3E');
 }
+.data-url-quoted-mixed {
+  background-image:url("data:image/svg+xml;charset=utf8,%3Csvg viewBox='0 0 32 32' xmlns='http://www.w3.org/2000/svg'%3E%3Cpath stroke='rgba(255, 255, 255, 0.5)' stroke-width='2' stroke-linecap='round' stroke-miterlimit='10' d='M4 8h24M4 16h24M4 24h24'/%3E%3C/svg%3E");
+}
 """, output)

     @skipIf(sys.platform.startswith("win"), "requires posix platform")

Maybe it could help to quickly find a proper fix? I guess I just overlooked something in the codebase.

PS: How about using a proper CSS parser for this, that returns tokens with offsets?

vskh commented 7 years ago

Hey guys, @triat!

I might have a fix for this. Could you take a look into referenced pull request?

Thanks!

mike86k commented 7 years ago

I have the same problem with https://cdnjs.cloudflare.com/ajax/libs/Swiper/3.1.2/css/swiper.css

@vskh's PR solve the problem.

triat commented 7 years ago

Hey guys, thanks @vskh for his PR, I just merged it, it should be there in the next version of django-pipeline.

jwhitlock commented 7 years ago

This seems like an important fix. Preparing a 1.6.13 release.

Edit: I think we should add Django 1.11 support for the next release, which means dropping 1.6 and 1.7. Sounds like a 1.7.0 release, after a new PR.