From a2906cd02c01d59e021202289e9835bb3cce3331 Mon Sep 17 00:00:00 2001 From: Sergey Shevchenko Date: Tue, 9 Nov 2021 14:15:12 +1300 Subject: [PATCH] ENH Requirements_Backend::resolveCSSReferences(): Tests, config, doc, safety. * Changed to ignore absolute paths altogether * Improve tests * Added config flag * Changed docs --- src/View/Requirements_Backend.php | 25 ++- tests/php/View/RequirementsTest.php | 142 ++++++++++++++++-- .../css/deep/deeper/RequirementsTest_p.css | 17 +++ 3 files changed, 168 insertions(+), 16 deletions(-) diff --git a/src/View/Requirements_Backend.php b/src/View/Requirements_Backend.php index 9d6c79494..7b2ad48db 100644 --- a/src/View/Requirements_Backend.php +++ b/src/View/Requirements_Backend.php @@ -52,6 +52,20 @@ class Requirements_Backend */ private static $combine_in_dev = false; + /** + * Determine if relative urls in the combined files should be converted to absolute. + * + * By default combined files will be parsed for relative urls to image/font assets and those + * utls will be changed to absolute to accomodate to the fact that the combined css is placed + * in a totally different folder than the source css files. + * + * Turn this off if you see some unexpected results. + * + * @config + * @var bool + */ + private static $resolve_relative_css_refs = true; + /** * Paths to all required JavaScript files relative to docroot * @@ -1434,10 +1448,15 @@ MESSAGE */ protected function resolveCSSReferences($content, $filePath) { + $doResolving = Config::inst()->get(__CLASS__, 'resolve_relative_css_refs'); + if (!$doResolving) { + return $content; + } $fileUrl = Injector::inst()->get(ResourceURLGenerator::class)->urlForResource($filePath); $fileUrlDir = dirname($fileUrl); - $content = preg_replace_callback('#([\.]{1,2}/)+#', function ($a) use ($fileUrlDir) { - $full = $fileUrlDir . '/' . $a[0]; + $content = preg_replace_callback('#([\s\'"\(])((:?[\.]{1,2}/)+)#', function ($a) use ($fileUrlDir) { + [ $_, $prefix, $match ] = $a; + $full = $fileUrlDir . '/' . $match; $full = preg_replace('#/{2,}#', '/', $full); // ensure there's no repeated slashes while (strpos($full, './') > 0) { $post = $full; @@ -1448,7 +1467,7 @@ MESSAGE } $full = $post; } - return $full; + return $prefix . $full; }, $content); return $content; } diff --git a/tests/php/View/RequirementsTest.php b/tests/php/View/RequirementsTest.php index d3d357380..2728209b3 100644 --- a/tests/php/View/RequirementsTest.php +++ b/tests/php/View/RequirementsTest.php @@ -13,6 +13,7 @@ use Silverstripe\Assets\Dev\TestAssetStore; use SilverStripe\View\Requirements_Backend; use SilverStripe\Core\Manifest\ResourceURLGenerator; use SilverStripe\Control\SimpleResourceURLGenerator; +use SilverStripe\Core\Config\Config; use SilverStripe\View\SSViewer; use SilverStripe\View\ThemeResourceLoader; @@ -77,12 +78,12 @@ class RequirementsTest extends SapphireTest $this->assertStringContainsString('http://www.mydomain.com:3000/test.css', $html, 'Load external with port'); } - public function testResolveCSSReferences() + public function testResolveCSSReferencesDisabled() { /** @var Requirements_Backend $backend */ $backend = Injector::inst()->create(Requirements_Backend::class); $this->setupRequirements($backend); - $backend->setCombinedFilesFolder('_combinedfiles'); + Config::forClass(get_class($backend))->set('resolve_relative_css_refs', false); $backend->combineFiles( 'RequirementsTest_pc.css', @@ -96,7 +97,59 @@ class RequirementsTest extends SapphireTest // we get the file path here $allCSS = $backend->getCSS(); - $this->assertTrue(count($allCSS) == 1, 'only one combined file'); + $this->assertCount( + 1, + $allCSS, + 'only one combined file' + ); + + $files = array_keys($allCSS); + $combinedFileName = $files[0]; + $combinedFileName = str_replace('/' . ASSETS_DIR . '/', '/', $combinedFileName); + + $combinedFilePath = TestAssetStore::base_path() . $combinedFileName; + + $content = file_get_contents($combinedFilePath); + + /* DISABLED COMBINED CSS URL RESOLVER IGNORED ONE DOT */ + $this->assertStringContainsString( + ".p0 { background: url(./zero.gif); }", + $content, + 'disabled combined css url resolver ignored one dot' + ); + + /* DISABLED COMBINED CSS URL RESOLVER IGNORED DOUBLE-DOT */ + $this->assertStringContainsString( + ".p1 { background: url(../one.gif); }", + $content, + 'disabled combined css url resolver ignored double-dot' + ); + } + + public function testResolveCSSReferences() + { + /** @var Requirements_Backend $backend */ + $backend = Injector::inst()->create(Requirements_Backend::class); + $this->setupRequirements($backend); + Config::forClass(get_class($backend))->set('resolve_relative_css_refs', true); + + $backend->combineFiles( + 'RequirementsTest_pc.css', + [ + 'css/RequirementsTest_d.css', + 'css/deep/deeper/RequirementsTest_p.css' + ] + ); + + $backend->includeInHTML(self::$html_template); + + // we get the file path here + $allCSS = $backend->getCSS(); + $this->assertCount( + 1, + $allCSS, + 'only one combined file' + ); $files = array_keys($allCSS); $combinedFileName = $files[0]; $combinedFileName = str_replace('/' . ASSETS_DIR . '/', '/', $combinedFileName); @@ -111,39 +164,102 @@ class RequirementsTest extends SapphireTest $content = file_get_contents($combinedFilePath); - /* COMBINED CSS DECODED ONE DOT OKAY */ + /* COMBINED CSS URL RESOLVER DECODED ONE DOT */ $this->assertStringContainsString( ".p0 { background: url(/css/deep/deeper/zero.gif); }", $content, - 'combined css decoded one dot okay' + 'combined css url resolver decoded one dot' ); - /* COMBINED CSS DECODED ONE DOUBLE-DOT OKAY */ + /* COMBINED CSS URL RESOLVER DECODED ONE DOT WITH SINGLE QUOTES */ + $this->assertStringContainsString( + ".p0sq { background: url('/css/deep/deeper/zero-sq.gif'); }", + $content, + 'combined css url resolver decoded one dot with single quotes' + ); + + /* COMBINED CSS URL RESOLVER DECODED ONE DOT WITH DOUBLE QUOTES */ + $this->assertStringContainsString( + ".p0dq { background: url(\"/css/deep/deeper/zero-dq.gif\"); }", + $content, + 'combined css url resolver decoded one dot with double quotes' + ); + + /* COMBINED CSS URL RESOLVER DECODED ONE DOT WITH DOUBLE QUOTES AND SPACES NEW LINE */ + $this->assertStringContainsString( + "\n \"/css/deep/deeper/zero-dq-nls.gif\"\n", + $content, + 'combined css url resolver decoded one dot with double quotes and spaces new line' + ); + + /* COMBINED CSS URL RESOLVER DECODED ONE DOT WITH DOUBLE QUOTES NEW LINE */ + $this->assertStringContainsString( + "\"/css/deep/deeper/zero-dq-nl.gif\"", + $content, + 'combined css url resolver decoded one dot with double quotes new line' + ); + + /* COMBINED CSS URL RESOLVER DECODED ONE DOT WITH DOUBLE QUOTES NEW LINE WITH SPACES */ + $this->assertStringContainsString( + "\"/css/deep/deeper/zero-dq-nls.gif\"", + $content, + 'combined css url resolver decoded one dot with double quotes new line with spaces' + ); + + /* COMBINED CSS URL RESOLVER DECODED 1 DOUBLE-DOT */ $this->assertStringContainsString( ".p1 { background: url(/css/deep/one.gif); }", $content, - 'combined css decoded 1 double-dot okay' + 'combined css url resolver decoded 1 double-dot' ); - /* COMBINED CSS DECODED 2 DOUBLE-DOT OKAY */ + /* COMBINED CSS URL RESOLVER DECODED 2 DOUBLE-DOT */ $this->assertStringContainsString( ".p2 { background: url(/css/two.gif); }", $content, - 'combined css decoded 2 double-dot okay' + 'combined css url resolver decoded 2 double-dot' ); - /* COMBINED CSS DECODED 3 DOUBLE-DOT OKAY */ + /* COMBINED CSS URL RESOLVER DECODED 2 DOUBLE-DOT SINGLE QUOTES */ + $this->assertStringContainsString( + ".p2sq { background: url('/css/two-sq.gif'); }", + $content, + 'combined css url resolver decoded 2 double-dot single quotes' + ); + + /* COMBINED CSS URL RESOLVER DECODED 2 DOUBLE-DOT DOUBLE QUOTES */ + $this->assertStringContainsString( + ".p2dq { background: url(\"/css/two-dq.gif\"); }", + $content, + 'combined css url resolver decoded 2 double-dot double quotes' + ); + + /* COMBINED CSS URL RESOLVER SHOULD NOT TOUCH ABSOLUTE PATH */ + $this->assertStringContainsString( + ".p2abs { background: url(/foo/bar/../../two-abs.gif); }", + $content, + 'combined css url resolver should not touch absolute path' + ); + + /* COMBINED CSS URL RESOLVER SHOULD NOT TOUCH ABSOLUTE PATH ON NEW LINE */ + $this->assertStringContainsString( + "\n /foo/bar/../../two-abs-ln.gif\n", + $content, + 'combined css url resolver should not touch absolute path on new line' + ); + + /* COMBINED CSS URL RESOLVER DECODED 3 DOUBLE-DOT */ $this->assertStringContainsString( ".p3 { background: url(/three.gif); }", $content, - 'combined css decoded 3 double-dot okay' + 'combined css url resolver decoded 3 double-dot' ); - /* COMBINED CSS DECODED 4 DOUBLE-DOT OKAY */ + /* COMBINED CSS URL RESOLVER DECODED 4 DOUBLE-DOT */ $this->assertStringContainsString( ".p4 { background: url(/../four.gif); }", $content, - 'combined css decoded 4 double-dot okay' + 'combined css url resolver decoded 4 double-dot' ); } diff --git a/tests/php/View/SSViewerTest/public/css/deep/deeper/RequirementsTest_p.css b/tests/php/View/SSViewerTest/public/css/deep/deeper/RequirementsTest_p.css index 1e1dc91e4..291c8ea52 100644 --- a/tests/php/View/SSViewerTest/public/css/deep/deeper/RequirementsTest_p.css +++ b/tests/php/View/SSViewerTest/public/css/deep/deeper/RequirementsTest_p.css @@ -1,5 +1,22 @@ .p0 { background: url(./zero.gif); } +.p0-nl { background: url( + ./zero-nl.gif ); +} +.p0sq { background: url('./zero-sq.gif'); } +.p0dq { background: url("./zero-dq.gif"); } +.p0dq-nl { background: url( +"./zero-dq-nl.gif" +); } +.p0dq-nls { background: url( + "./zero-dq-nls.gif" +); } .p1 { background: url(../one.gif); } .p2 { background: url(../../two.gif); } +.p2abs { background: url(/foo/bar/../../two-abs.gif); } /* don't touch this */ +.p2abs-ln { background: url( + /foo/bar/../../two-abs-ln.gif +); } /* don't touch this */ +.p2sq { background: url('../../two-sq.gif'); } +.p2dq { background: url("../../two-dq.gif"); } .p3 { background: url(../../../three.gif); } .p4 { background: url(../../../../four.gif); }