From 92e93704be82d30a759e7cf3b58c2be9ac0c2e53 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Fri, 26 Jan 2018 17:02:54 +0100 Subject: [PATCH 1/8] SCSS files are only cached if their size is > 0 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl --- lib/private/Template/SCSSCacher.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/private/Template/SCSSCacher.php b/lib/private/Template/SCSSCacher.php index a460442554..dd3870b861 100644 --- a/lib/private/Template/SCSSCacher.php +++ b/lib/private/Template/SCSSCacher.php @@ -153,8 +153,9 @@ class SCSSCacher { return false; } } + return true; } - return true; + return false; } catch(NotFoundException $e) { return false; } From 5eae4819bfaf69c66957dc1d723608b457fd4106 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Fri, 26 Jan 2018 17:03:22 +0100 Subject: [PATCH 2/8] Make sure that injected variables do not break the CSS generation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl --- lib/private/Template/SCSSCacher.php | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/lib/private/Template/SCSSCacher.php b/lib/private/Template/SCSSCacher.php index dd3870b861..1c4650ea1b 100644 --- a/lib/private/Template/SCSSCacher.php +++ b/lib/private/Template/SCSSCacher.php @@ -63,6 +63,9 @@ class SCSSCacher { /** @var ICache */ protected $depsCache; + /** @var null|string */ + protected $injectedVariables = null; + /** * @param ILogger $logger * @param Factory $appDataFactory @@ -268,10 +271,22 @@ class SCSSCacher { * @return string SCSS code for variables from OC_Defaults */ private function getInjectedVariables() { + if ($this->injectedVariables !== null) + return $this->injectedVariables; $variables = ''; foreach ($this->defaults->getScssVariables() as $key => $value) { $variables .= '$' . $key . ': ' . $value . ';'; } + + // check for valid variables / otherwise fall back to defaults + try { + $scss = new Compiler(); + $scss->compile($variables); + $this->injectedVariables = $variables; + } catch (ParserException $e) { + $this->logger->error($e, ['app' => 'core']); + } + return $variables; } From 60e601f4abb530d0503e43c6dae52b4e63d203ee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Fri, 26 Jan 2018 17:04:04 +0100 Subject: [PATCH 3/8] Only override image styles if the theming values are set MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl --- apps/theming/css/theming.scss | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/apps/theming/css/theming.scss b/apps/theming/css/theming.scss index 63d466542e..3cb8ee2584 100644 --- a/apps/theming/css/theming.scss +++ b/apps/theming/css/theming.scss @@ -91,18 +91,20 @@ } /* override styles for login screen in guest.css */ -#header .logo { - background-image: url(#{$image-logo}); - @if $theming-logo-mime != '' { +@if variable_exists('theming-logo-mime') { + #header .logo { + background-image: url(#{$image-logo}); background-size: contain; } } -#body-login, -#firstrunwizard .firstrunwizard-header, -#theming-preview { - background-image: url(#{$image-login-background}); - background-color: $color-primary; +@if variable_exists('theming-background-mime') { + #body-login, + #firstrunwizard .firstrunwizard-header, + #theming-preview { + background-image: url(#{$image-login-background}); + background-color: $color-primary; + } } input.primary, From 68db09ddcef57faa58b5747e480347e867d03175 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Fri, 26 Jan 2018 19:07:30 +0100 Subject: [PATCH 4/8] Clear injectect variables when resetting the cache MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl --- lib/private/Template/SCSSCacher.php | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/private/Template/SCSSCacher.php b/lib/private/Template/SCSSCacher.php index 1c4650ea1b..391138255b 100644 --- a/lib/private/Template/SCSSCacher.php +++ b/lib/private/Template/SCSSCacher.php @@ -254,6 +254,7 @@ class SCSSCacher { * We need to regenerate all files when variables change */ private function resetCache() { + $this->injectedVariables = null; $appDirectory = $this->appData->getDirectoryListing(); if(empty($appDirectory)){ return; From 67702136494b9b355e99c0960ee36accd5e97b85 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Fri, 26 Jan 2018 19:08:45 +0100 Subject: [PATCH 5/8] Do not rewrite absolute URLs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl --- apps/theming/lib/ThemingDefaults.php | 4 ++-- lib/private/Template/SCSSCacher.php | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/apps/theming/lib/ThemingDefaults.php b/apps/theming/lib/ThemingDefaults.php index 9dcc981817..94abb4e288 100644 --- a/apps/theming/lib/ThemingDefaults.php +++ b/apps/theming/lib/ThemingDefaults.php @@ -242,8 +242,8 @@ class ThemingDefaults extends \OC_Defaults { 'theming-background-mime' => "'" . $this->config->getAppValue('theming', 'backgroundMime', '') . "'" ]; - $variables['image-logo'] = "'".$this->urlGenerator->getAbsoluteURL($this->getLogo())."'"; - $variables['image-login-background'] = "'".$this->urlGenerator->getAbsoluteURL($this->getBackground())."'"; + $variables['image-logo'] = "'".$this->getLogo()."'"; + $variables['image-login-background'] = "'".$this->getBackground()."'"; $variables['image-login-plain'] = 'false'; if ($this->config->getAppValue('theming', 'color', null) !== null) { diff --git a/lib/private/Template/SCSSCacher.php b/lib/private/Template/SCSSCacher.php index 391138255b..c6473ead09 100644 --- a/lib/private/Template/SCSSCacher.php +++ b/lib/private/Template/SCSSCacher.php @@ -298,7 +298,7 @@ class SCSSCacher { * @return string */ private function rebaseUrls($css, $webDir) { - $re = '/url\([\'"]([\.\w?=\/-]*)[\'"]\)/x'; + $re = '/url\([\'"]([^\/][\.\w?=\/-]*)[\'"]\)/x'; $subst = 'url(\''.$webDir.'/$1\')'; return preg_replace($re, $subst, $css); } From 5ec7296f6d19eab46291f724c38557b4e8e6b235 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Fri, 26 Jan 2018 19:56:13 +0100 Subject: [PATCH 6/8] Add tests for URL rewriting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl --- tests/lib/Template/SCSSCacherTest.php | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/tests/lib/Template/SCSSCacherTest.php b/tests/lib/Template/SCSSCacherTest.php index fca9500810..7b5300b99a 100644 --- a/tests/lib/Template/SCSSCacherTest.php +++ b/tests/lib/Template/SCSSCacherTest.php @@ -351,11 +351,21 @@ class SCSSCacherTest extends \Test\TestCase { $this->assertFalse($actual); } - public function testRebaseUrls() { + public function dataRebaseUrls() { + return [ + ['#id { background-image: url(\'../img/image.jpg\'); }','#id { background-image: url(\'/apps/files/css/../img/image.jpg\'); }'], + ['#id { background-image: url("../img/image.jpg"); }','#id { background-image: url(\'/apps/files/css/../img/image.jpg\'); }'], + ['#id { background-image: url(\'/img/image.jpg\'); }','#id { background-image: url(\'/img/image.jpg\'); }'], + ['#id { background-image: url("http://example.com/test.jpg"); }','#id { background-image: url("http://example.com/test.jpg"); }'], + ]; + } + + /** + * @dataProvider dataRebaseUrls + */ + public function testRebaseUrls($scss, $expected) { $webDir = '/apps/files/css'; - $css = '#id { background-image: url(\'../img/image.jpg\'); }'; - $actual = self::invokePrivate($this->scssCacher, 'rebaseUrls', [$css, $webDir]); - $expected = '#id { background-image: url(\'/apps/files/css/../img/image.jpg\'); }'; + $actual = self::invokePrivate($this->scssCacher, 'rebaseUrls', [$scss, $webDir]); $this->assertEquals($expected, $actual); } From e4c58fc4733ffd2386044469c2be7f6dceab1d71 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Fri, 26 Jan 2018 20:11:37 +0100 Subject: [PATCH 7/8] Add typehinting an fix some minor cleanup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl --- lib/private/Template/SCSSCacher.php | 40 +++++++++++++++++------------ 1 file changed, 24 insertions(+), 16 deletions(-) diff --git a/lib/private/Template/SCSSCacher.php b/lib/private/Template/SCSSCacher.php index c6473ead09..8830a651f3 100644 --- a/lib/private/Template/SCSSCacher.php +++ b/lib/private/Template/SCSSCacher.php @@ -57,6 +57,9 @@ class SCSSCacher { /** @var IConfig */ protected $config; + /** @var \OC_Defaults */ + private $defaults; + /** @var string */ protected $serverRoot; @@ -64,7 +67,7 @@ class SCSSCacher { protected $depsCache; /** @var null|string */ - protected $injectedVariables = null; + private $injectedVariables; /** * @param ILogger $logger @@ -93,12 +96,14 @@ class SCSSCacher { /** * Process the caching process if needed + * * @param string $root Root path to the nextcloud installation * @param string $file * @param string $app The app name * @return boolean + * @throws NotPermittedException */ - public function process($root, $file, $app) { + public function process(string $root, string $file, string $app): bool { $path = explode('/', $root . '/' . $file); $fileNameSCSS = array_pop($path); @@ -126,7 +131,7 @@ class SCSSCacher { * @param $fileName * @return ISimpleFile */ - public function getCachedCSS($appName, $fileName) { + public function getCachedCSS(string $appName, string $fileName): ISimpleFile { $folder = $this->appData->getFolder($appName); return $folder->getFile($this->prependBaseurlPrefix($fileName)); } @@ -137,7 +142,7 @@ class SCSSCacher { * @param ISimpleFolder $folder * @return boolean */ - private function isCached($fileNameCSS, ISimpleFolder $folder) { + private function isCached(string $fileNameCSS, ISimpleFolder $folder) { try { $cachedFile = $folder->getFile($fileNameCSS); if ($cachedFile->getSize() > 0) { @@ -151,7 +156,7 @@ class SCSSCacher { } $deps = json_decode($deps, true); - foreach ($deps as $file=>$mtime) { + foreach ((array)$deps as $file=>$mtime) { if (!file_exists($file) || filemtime($file) > $mtime) { return false; } @@ -168,7 +173,7 @@ class SCSSCacher { * Check if the variables file has changed * @return bool */ - private function variablesChanged() { + private function variablesChanged(): bool { $injectedVariables = $this->getInjectedVariables(); if($this->config->getAppValue('core', 'scss.variables') !== md5($injectedVariables)) { $this->resetCache(); @@ -180,14 +185,16 @@ class SCSSCacher { /** * Cache the file with AppData + * * @param string $path * @param string $fileNameCSS * @param string $fileNameSCSS * @param ISimpleFolder $folder * @param string $webDir * @return boolean + * @throws NotPermittedException */ - private function cache($path, $fileNameCSS, $fileNameSCSS, ISimpleFolder $folder, $webDir) { + private function cache(string $path, string $fileNameCSS, string $fileNameSCSS, ISimpleFolder $folder, string $webDir) { $scss = new Compiler(); $scss->setImportPaths([ $path, @@ -261,7 +268,7 @@ class SCSSCacher { } foreach ($appDirectory as $folder) { foreach ($folder->getDirectoryListing() as $file) { - if (substr($file->getName(), -3) === "css" || substr($file->getName(), -4) === "deps") { + if (substr($file->getName(), -3) === 'css' || substr($file->getName(), -4) === 'deps') { $file->delete(); } } @@ -271,9 +278,10 @@ class SCSSCacher { /** * @return string SCSS code for variables from OC_Defaults */ - private function getInjectedVariables() { - if ($this->injectedVariables !== null) + private function getInjectedVariables(): string { + if ($this->injectedVariables !== null) { return $this->injectedVariables; + } $variables = ''; foreach ($this->defaults->getScssVariables() as $key => $value) { $variables .= '$' . $key . ': ' . $value . ';'; @@ -297,7 +305,7 @@ class SCSSCacher { * @param string $webDir * @return string */ - private function rebaseUrls($css, $webDir) { + private function rebaseUrls(string $css, string $webDir): string { $re = '/url\([\'"]([^\/][\.\w?=\/-]*)[\'"]\)/x'; $subst = 'url(\''.$webDir.'/$1\')'; return preg_replace($re, $subst, $css); @@ -309,20 +317,20 @@ class SCSSCacher { * @param string $fileName * @return string */ - public function getCachedSCSS($appName, $fileName) { + public function getCachedSCSS(string $appName, string $fileName): string { $tmpfileLoc = explode('/', $fileName); $fileName = array_pop($tmpfileLoc); $fileName = $this->prependBaseurlPrefix(str_replace('.scss', '.css', $fileName)); - return substr($this->urlGenerator->linkToRoute('core.Css.getCss', array('fileName' => $fileName, 'appName' => $appName)), strlen(\OC::$WEBROOT) + 1); + return substr($this->urlGenerator->linkToRoute('core.Css.getCss', ['fileName' => $fileName, 'appName' => $appName]), strlen(\OC::$WEBROOT) + 1); } /** * Prepend hashed base url to the css file - * @param $cssFile + * @param string$cssFile * @return string */ - private function prependBaseurlPrefix($cssFile) { + private function prependBaseurlPrefix(string $cssFile): string { $frontendController = ($this->config->getSystemValue('htaccess.IgnoreFrontController', false) === true || getenv('front_controller_active') === 'true'); return substr(md5($this->urlGenerator->getBaseUrl() . $frontendController), 0, 8) . '-' . $cssFile; } @@ -335,7 +343,7 @@ class SCSSCacher { * @param string $webRoot the nextcloud installation root path * @return string the webDir */ - private function getWebDir($path, $appName, $serverRoot, $webRoot) { + private function getWebDir(string $path, string $appName, string $serverRoot, string $webRoot): string { // Detect if path is within server root AND if path is within an app path if ( strpos($path, $serverRoot) === false && $appWebPath = \OC_App::getAppWebPath($appName)) { // Get the file path within the app directory From 5dbf73339542977698e1f3ae6d22624fab74fe3e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Fri, 26 Jan 2018 20:13:28 +0100 Subject: [PATCH 8/8] Fix theming tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl --- apps/theming/tests/ThemingDefaultsTest.php | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/apps/theming/tests/ThemingDefaultsTest.php b/apps/theming/tests/ThemingDefaultsTest.php index d0dc6587f7..1a4679a299 100644 --- a/apps/theming/tests/ThemingDefaultsTest.php +++ b/apps/theming/tests/ThemingDefaultsTest.php @@ -517,18 +517,12 @@ class ThemingDefaultsTest extends TestCase { ['theming.Theming.getLoginBackground', [], 'custom-background'], ]); - $this->urlGenerator->expects($this->exactly(2)) - ->method('getAbsoluteURL') - ->willReturnCallback(function ($path) { - return 'absolute-' . $path; - }); - $expected = [ 'theming-cachebuster' => '\'0\'', 'theming-logo-mime' => '\'jpeg\'', 'theming-background-mime' => '\'jpeg\'', - 'image-logo' => "'absolute-custom-logo?v=0'", - 'image-login-background' => "'absolute-custom-background?v=0'", + 'image-logo' => "'custom-logo?v=0'", + 'image-login-background' => "'custom-background?v=0'", 'color-primary' => $this->defaults->getColorPrimary(), 'color-primary-text' => '#ffffff', 'image-login-plain' => 'false',