From 2c6207aa1cb9158221c5373457166cb1f862b59d Mon Sep 17 00:00:00 2001 From: Luke Towers Date: Fri, 25 Sep 2026 20:06:30 -0600 Subject: [PATCH 1/3] Apply the combiner's allowed import roots to the SCSS compiler wintercms/storm#245 gave ScssCompiler the same allowed-import-roots support as the LESS and JavaScript compilers, defaulting to the asset's own directory. CombineAssets already passes its roots (themes, plugins and modules) to those siblings but registered the SCSS compiler without them, so SCSS imports that reach into another of those trees would stop resolving. It now receives the same roots. --- modules/system/classes/CombineAssets.php | 7 +- .../tests/classes/CombineAssetsTest.php | 83 +++++++++++++++++++ 2 files changed, 88 insertions(+), 2 deletions(-) diff --git a/modules/system/classes/CombineAssets.php b/modules/system/classes/CombineAssets.php index bb185bad4a..50676e97b0 100644 --- a/modules/system/classes/CombineAssets.php +++ b/modules/system/classes/CombineAssets.php @@ -135,7 +135,7 @@ public function init() // explicit roots, a writable asset could disclose arbitrary server-readable // files: `@import (inline) ""` in a .less file (GHSA-58fp-mcx6-7qf9), // `=include ../../../.env` in a .js file (GHSA-2223-f22x-24cq), or an - // `@import` traversal in a .css file. The asset's own source directory is + // `@import` traversal in a .scss or .css file. The asset's own source directory is // always allowed implicitly; this list adds the cross-tree roots that // legitimate themes/plugins/modules actually import from (e.g. a plugin // importing a module asset, or a theme importing its own ../vendor). @@ -170,7 +170,10 @@ public function init() $lessCompiler = new LessCompiler; $lessCompiler->setAllowedImportRoots($allowedImportRoots); $this->registerFilter('less', $lessCompiler); - $this->registerFilter('scss', new ScssCompiler); + + $scssCompiler = new ScssCompiler; + $scssCompiler->setAllowedImportRoots($allowedImportRoots); + $this->registerFilter('scss', $scssCompiler); /* * Minification filters diff --git a/modules/system/tests/classes/CombineAssetsTest.php b/modules/system/tests/classes/CombineAssetsTest.php index 7491fa1cc4..2cb34d65bd 100644 --- a/modules/system/tests/classes/CombineAssetsTest.php +++ b/modules/system/tests/classes/CombineAssetsTest.php @@ -237,6 +237,89 @@ public function testJavascriptImporterAllowsSameTreeInclude() } } + /** + * SCSS `@import` resolution must stay within the allowed import roots, the same + * as the LESS and JavaScript importers above. + */ + public function testScssCompilerBlocksRelativeTraversalImport() + { + $themeDir = $this->makeTempThemeDir(); + $secretPath = dirname($themeDir) . '/scss-secret-' . bin2hex(random_bytes(4)) . '.scss'; + file_put_contents($secretPath, '.leak { content: "combine-leak-canary"; }'); + file_put_contents( + $themeDir . '/assets/poc.scss', + '@import "../../' . basename($secretPath, '.scss') . '"; .x { color: red; }' + ); + + try { + $css = $this->compileScssTo($themeDir, 'assets/poc.scss'); + $this->assertStringNotContainsString('combine-leak-canary', $css); + } finally { + @unlink($secretPath); + \File::deleteDirectory($themeDir); + } + } + + /** + * Legitimate same-tree `@import "partial"` must still resolve, including a + * partial that imports another partial of its own. + */ + public function testScssCompilerAllowsLegitimatePartial() + { + $themeDir = $this->makeTempThemeDir(); + mkdir($themeDir . '/assets/sub', 0777, true); + file_put_contents($themeDir . '/assets/sub/_deep.scss', '.deep-marker { color: purple; }'); + file_put_contents($themeDir . '/assets/_partial.scss', '@import "sub/deep"; .partial-marker { color: orange; }'); + file_put_contents($themeDir . '/assets/main.scss', '@import "partial"; .main-marker { color: blue; }'); + + try { + $css = $this->compileScssTo($themeDir, 'assets/main.scss'); + $this->assertStringContainsString('deep-marker', $css); + $this->assertStringContainsString('partial-marker', $css); + $this->assertStringContainsString('main-marker', $css); + } finally { + \File::deleteDirectory($themeDir); + } + } + + /** + * Imports from outside the asset's own directory must keep working when they + * land in one of the roots CombineAssets allows (themes, plugins, modules). + */ + public function testScssCompilerAllowsImportFromAllowedRoot() + { + $themeDir = $this->makeTempThemeDir(); + $sharedDir = themes_path('scss-import-root-test-' . bin2hex(random_bytes(4))); + mkdir($sharedDir, 0777, true); + file_put_contents($sharedDir . '/_shared.scss', '.shared-marker { color: green; }'); + // SCSS string literals treat a backslash as an escape, so use forward slashes + // for the absolute path on Windows. + file_put_contents( + $themeDir . '/assets/main.scss', + '@import "' . str_replace('\\', '/', $sharedDir) . '/shared"; .main-marker { color: blue; }' + ); + + try { + $css = $this->compileScssTo($themeDir, 'assets/main.scss'); + $this->assertStringContainsString('shared-marker', $css); + $this->assertStringContainsString('main-marker', $css); + } finally { + \File::deleteDirectory($sharedDir); + \File::deleteDirectory($themeDir); + } + } + + protected function compileScssTo(string $themeDir, string $relativeAsset): string + { + $dest = sys_get_temp_dir() . '/winter-combine-out-' . bin2hex(random_bytes(4)) . '.css'; + try { + CombineAssets::instance()->combineToFile([$relativeAsset], $dest, $themeDir); + return file_get_contents($dest) ?: ''; + } finally { + @unlink($dest); + } + } + protected function compileJsTo(string $themeDir, string $relativeAsset): string { $dest = sys_get_temp_dir() . '/winter-combine-out-' . bin2hex(random_bytes(4)) . '.js'; From 093a5568441dc7ce80ab89ac68647f650d8eade0 Mon Sep 17 00:00:00 2001 From: Luke Towers Date: Fri, 25 Sep 2026 20:20:18 -0600 Subject: [PATCH 2/3] Tighten the remaining asset combiner import and error handling - The CSS import validator refuses targets that CssImportFilter would load as a URL (any scheme, or a protocol-relative `//`) rather than passing them to the path check, which cannot judge them. They are left in the output for the browser to resolve. - ColorPicker validates saved values against the same normalised format list it renders with. An unrecognised `formats` value falls back to "hex" instead of skipping validation, an array of formats accepts only the formats it lists, and "custom" is the explicit way to opt out. - The combine route only returns compiler error messages in debug mode, since they quote source lines and paths. Otherwise it logs the exception and returns a generic message. - Adds end-to-end combiner coverage for nested LESS imports and `data-uri()`, nested JavaScript includes, and CSS imports. --- modules/backend/formwidgets/ColorPicker.php | 40 +-- .../tests/formwidgets/ColorPickerTest.php | 44 +++ modules/system/classes/CombineAssets.php | 8 + modules/system/classes/SystemController.php | 7 + modules/system/lang/en/lang.php | 1 + .../tests/classes/CombineAssetsTest.php | 303 ++++++++++++++++++ .../tests/classes/SystemControllerTest.php | 59 ++++ 7 files changed, 438 insertions(+), 24 deletions(-) create mode 100644 modules/system/tests/classes/SystemControllerTest.php diff --git a/modules/backend/formwidgets/ColorPicker.php b/modules/backend/formwidgets/ColorPicker.php index 248b078a2d..7183268a81 100644 --- a/modules/backend/formwidgets/ColorPicker.php +++ b/modules/backend/formwidgets/ColorPicker.php @@ -54,8 +54,8 @@ class ColorPicker extends FormWidgetBase /** * @var string|array Color format(s) to allow for the resulting color value. Specify "all" as a string to allow all - * formats. - * Allowed values: 'cmyk', 'hex', 'hsl', 'rgb', 'all' + * formats, or "custom" to accept the value as-is without format validation. + * Allowed values: 'cmyk', 'hex', 'hsl', 'rgb', 'all', 'custom' */ public $formats = 'hex'; @@ -258,29 +258,21 @@ public function getSaveValue($value) return null; } - switch (is_array($this->formats) ? 'all' : $this->formats) { - case 'cmyk': - case 'hex': - case 'hsl': - case 'rgb': - if (!preg_match($this->validationPatterns[$this->formats], $value)) { - throw new ApplicationException(Lang::get('backend::lang.field.colors_invalid_input')); - } - break; - case 'all': - $valid = false; - foreach ($this->validationPatterns as $pattern) { - if (preg_match($pattern, $value)) { - $valid = true; - break; - } - } - if (!$valid) { - throw new ApplicationException(Lang::get('backend::lang.field.colors_invalid_input')); - } - break; + // "custom" is the explicit opt-out of format validation, for fields that + // store something the built-in patterns cannot express. + if ($this->formats === 'custom') { + return $value; } - return $value; + // Validate against the same normalised format list the field renders with, so a + // `formats` value that names no known format falls back to "hex" rather than + // accepting anything. + foreach ($this->getFormats() as $format) { + if (preg_match($this->validationPatterns[$format], $value)) { + return $value; + } + } + + throw new ApplicationException(Lang::get('backend::lang.field.colors_invalid_input')); } } diff --git a/modules/backend/tests/formwidgets/ColorPickerTest.php b/modules/backend/tests/formwidgets/ColorPickerTest.php index c97200441a..3848077940 100644 --- a/modules/backend/tests/formwidgets/ColorPickerTest.php +++ b/modules/backend/tests/formwidgets/ColorPickerTest.php @@ -121,6 +121,50 @@ public function testAllowCustomSaveValue(): void $this->assertEquals('Test(51.9, 152, 219, 1)', $widget->getSaveValue('Test(51.9, 152, 219, 1)')); } + /** + * A `formats` value that names no known format renders the field as "hex", so + * "hex" is what must be enforced on save rather than accepting any value. + */ + public function testUnrecognisedFormatFallsBackToHexValidation(): void + { + $widget = $this->makeWidget([ + 'formats' => 'hexadecimal', + ]); + + $this->assertEquals('#3498DB', $widget->getSaveValue('#3498DB')); + + $this->expectException(ApplicationException::class); + $widget->getSaveValue('not a colour'); + } + + /** + * An array of formats must accept only the formats it lists. + */ + public function testArrayFormatAcceptsOnlyListedFormats(): void + { + $widget = $this->makeWidget([ + 'formats' => ['hex'], + ]); + + $this->assertEquals('#3498DB', $widget->getSaveValue('#3498DB')); + + $this->expectException(ApplicationException::class); + $widget->getSaveValue('rgba(51.9, 152, 219, 1)'); + } + + /** + * ...but every format it does list keeps working. + */ + public function testArrayFormatAcceptsEachListedFormat(): void + { + $widget = $this->makeWidget([ + 'formats' => ['hex', 'rgb'], + ]); + + $this->assertEquals('#3498DB', $widget->getSaveValue('#3498DB')); + $this->assertEquals('rgba(51.9, 152, 219, 1)', $widget->getSaveValue('rgba(51.9, 152, 219, 1)')); + } + protected function makeWidget(array $config = []): ColorPicker { return new ColorPicker(new Controller(), new FormField('test', 'Test'), $config); diff --git a/modules/system/classes/CombineAssets.php b/modules/system/classes/CombineAssets.php index 50676e97b0..9980f6d2cc 100644 --- a/modules/system/classes/CombineAssets.php +++ b/modules/system/classes/CombineAssets.php @@ -10,6 +10,7 @@ use Config; use Request; use Response; +use Str; use Assetic\Asset\FileAsset; use Assetic\Asset\AssetCache; use Assetic\Asset\AssetCollection; @@ -158,7 +159,14 @@ public function init() $cssImportFilter = new CssImportFilter; // Assetic's CssImportFilter resolves `@import` targets relative to the source // with `..` traversal allowed; confine the resolved path to the allowed roots. + // Targets it would load as a URL instead (anything with a scheme, or a + // protocol-relative `//`) are not paths the root check can judge, so they are + // refused and left in the output for the browser to resolve. $cssImportFilter->setImportValidator(function ($path) use ($allowedImportRoots) { + if (Str::contains($path, '://') || Str::startsWith($path, '//')) { + return false; + } + $resolved = PathResolver::resolve($path); return $resolved !== false diff --git a/modules/system/classes/SystemController.php b/modules/system/classes/SystemController.php index 2b430ceb21..1589be6bee 100644 --- a/modules/system/classes/SystemController.php +++ b/modules/system/classes/SystemController.php @@ -38,6 +38,13 @@ public function combine($name) return $combiner->getContents($cacheId); } catch (Exception $ex) { + // Compiler errors quote source lines and absolute paths, so only show them in debug mode. + if (!Config::get('app.debug', false)) { + report($ex); + + return Response::make('/* '.e(Lang::get('system::lang.combiner.error')).' */', 500); + } + return Response::make('/* '.e($ex->getMessage()).' */', 500); } } diff --git a/modules/system/lang/en/lang.php b/modules/system/lang/en/lang.php index 8e1d24f732..4ecd491598 100644 --- a/modules/system/lang/en/lang.php +++ b/modules/system/lang/en/lang.php @@ -58,6 +58,7 @@ ], 'combiner' => [ 'not_found' => "The combiner file ':name' is not found.", + 'error' => 'The combined file could not be generated.', ], 'system' => [ 'name' => 'System', diff --git a/modules/system/tests/classes/CombineAssetsTest.php b/modules/system/tests/classes/CombineAssetsTest.php index 2cb34d65bd..655a85e72a 100644 --- a/modules/system/tests/classes/CombineAssetsTest.php +++ b/modules/system/tests/classes/CombineAssetsTest.php @@ -148,6 +148,66 @@ public function testLessCompilerBlocksRelativeTraversalImport() } } + /** + * A `.less` file imported from a subdirectory is parsed with its own directory as + * the current one, so its imports must be confined as well as the entry asset's. + */ + public function testLessCompilerBlocksTraversalFromNestedImport() + { + [$themeDir, $secretPath] = $this->setupNestedLessLeakFixture( + '@import (inline) "../../../../%SECRET%"; .x { color: red; }' + ); + + try { + $css = $this->compileLessTo($themeDir, 'assets/less/poc.less'); + $this->assertStringNotContainsString('APP_KEY', $css); + $this->assertStringNotContainsString('combine-leak-canary', $css); + } finally { + $this->teardownLessLeakFixture($themeDir, $secretPath); + } + } + + /** + * `data-uri()` reads the file it is given and inlines it into the compiled CSS, so + * it must be confined the same way as `@import`, including from a nested file. + */ + public function testLessCompilerBlocksDataUriFileRead() + { + [$themeDir, $secretPath] = $this->setupNestedLessLeakFixture( + '.x { background: data-uri("text/plain", "../../../../%SECRET%"); }' + ); + + try { + $css = $this->compileLessTo($themeDir, 'assets/less/poc.less'); + $this->assertStringNotContainsString('APP_KEY', $css); + $this->assertStringNotContainsString('combine-leak-canary', $css); + } finally { + $this->teardownLessLeakFixture($themeDir, $secretPath); + } + } + + /** + * Legitimate multi-level partial chains (an asset importing a partial from a + * subdirectory, which imports its own neighbour) must keep resolving. + */ + public function testLessCompilerAllowsNestedPartialChain() + { + $themeDir = $this->makeTempThemeDir(); + mkdir($themeDir . '/assets/less/sub'); + file_put_contents($themeDir . '/assets/less/main.less', '@import "sub/child.less"; .main-marker { color: blue; }'); + file_put_contents($themeDir . '/assets/less/sub/child.less', '@import "deeper.less"; .child-marker { color: green; }'); + file_put_contents($themeDir . '/assets/less/sub/deeper.less', '.deeper-marker { color: purple; }'); + + try { + $css = $this->compileLessTo($themeDir, 'assets/less/main.less'); + $this->assertStringContainsString('main-marker', $css); + $this->assertStringContainsString('child-marker', $css); + $this->assertStringContainsString('deeper-marker', $css); + } finally { + \File::deleteDirectory($themeDir); + } + } + /** * Legitimate same-tree `@import "partial.less"` must still resolve through * the gate, otherwise we've broken every theme that uses partials. @@ -219,6 +279,33 @@ public function testJavascriptImporterBlocksDisallowedExtension() } } + /** + * An `=include` inside an included file must be confined the same way as one in + * the entry asset. + */ + public function testJavascriptImporterBlocksTraversalFromNestedInclude() + { + $themeDir = $this->makeTempThemeDir(); + mkdir($themeDir . '/assets/sub', 0777, true); + $secretPath = dirname($themeDir) . '/js-secret-' . bin2hex(random_bytes(4)) . '.js'; + file_put_contents($secretPath, 'var LEAK = "combine-leak-canary";'); + file_put_contents($themeDir . '/assets/poc.js', "/*\n=include sub/child.js\n*/\nvar MAIN = 1;"); + file_put_contents( + $themeDir . '/assets/sub/child.js', + "/*\n=include ../../../" . basename($secretPath) . "\n*/\nvar CHILD = 1;" + ); + + try { + $js = $this->compileJsTo($themeDir, 'assets/poc.js'); + $this->assertStringNotContainsString('combine-leak-canary', $js); + // The legitimate half of the chain must still have been inlined. + $this->assertStringContainsString('var CHILD = 1;', $js); + } finally { + @unlink($secretPath); + \File::deleteDirectory($themeDir); + } + } + /** * Legitimate same-tree `=include partial.js` must still resolve, otherwise the * hardening would break every asset that composes its own bundle. @@ -237,6 +324,148 @@ public function testJavascriptImporterAllowsSameTreeInclude() } } + /** + * A `file://` import is not a filesystem path the root check can judge, so it must + * be left for the browser rather than loaded by the combiner. + */ + public function testCssImportFilterBlocksFileSchemeImport() + { + [$themeDir, $secretPath] = $this->setupCssLeakFixture( + '@import url("file://%SECRET%");' . "\n" . '.x { color: red; }' + ); + + try { + $css = $this->compileCssTo($themeDir, 'assets/poc.css'); + $this->assertStringNotContainsString('combine-leak-canary', $css); + } finally { + $this->teardownLessLeakFixture($themeDir, $secretPath); + } + } + + /** + * Likewise for any other stream wrapper, such as `php://`. + */ + public function testCssImportFilterBlocksPhpFilterSchemeImport() + { + [$themeDir, $secretPath] = $this->setupCssLeakFixture( + '@import url("php://filter/read=convert.base64-encode/resource=%SECRET%");' . "\n" . '.x { color: red; }' + ); + + try { + $css = $this->compileCssTo($themeDir, 'assets/poc.css'); + $this->assertStringNotContainsString('combine-leak-canary', $css); + $this->assertStringNotContainsString(base64_encode('APP_KEY=combine-leak-canary'), $css); + } finally { + $this->teardownLessLeakFixture($themeDir, $secretPath); + } + } + + /** + * A remote `@import` must be left in the output for the browser to resolve, not + * fetched and inlined by the server. Assetic replaces a fetched import with the + * response body (empty on failure), so the URL surviving shows it was not fetched. + */ + public function testCssImportFilterDoesNotFetchRemoteImport() + { + $themeDir = $this->makeTempCssThemeDir(); + // Port 1 on loopback: no listener, so a fetch attempt fails fast and silently + // (HttpAsset is constructed with $ignoreErrors) rather than hanging on DNS. + file_put_contents( + $themeDir . '/assets/poc.css', + '@import url("http://127.0.0.1:1/remote.css");' . "\n" . '.x { color: red; }' + ); + + try { + $css = $this->compileCssTo($themeDir, 'assets/poc.css'); + $this->assertStringContainsString('http://127.0.0.1:1/remote.css', $css); + } finally { + \File::deleteDirectory($themeDir); + } + } + + /** + * A protocol-relative `//host/path` import is also a URL, even when its path part + * happens to look like a location inside an allowed root. + */ + public function testCssImportFilterDoesNotFetchProtocolRelativeImport() + { + $themeDir = $this->makeTempCssThemeDir(); + $target = '/' . str_replace('\\', '/', $themeDir) . '/assets/partial.css'; + file_put_contents($themeDir . '/assets/partial.css', '.partial-marker { color: orange; }'); + file_put_contents($themeDir . '/assets/poc.css', '@import url("' . $target . '");' . "\n" . '.x { color: red; }'); + + try { + $css = $this->compileCssTo($themeDir, 'assets/poc.css'); + $this->assertStringContainsString($target, $css); + } finally { + \File::deleteDirectory($themeDir); + } + } + + /** + * Relative `..` traversal out of the allowed roots must be refused. + */ + public function testCssImportFilterBlocksRelativeTraversalImport() + { + [$themeDir, $secretPath] = $this->setupCssLeakFixture( + '@import url("' . str_repeat('../', 20) . '%SECRET_RELATIVE%");' . "\n" . '.x { color: red; }' + ); + + try { + $css = $this->compileCssTo($themeDir, 'assets/poc.css'); + $this->assertStringNotContainsString('combine-leak-canary', $css); + } finally { + $this->teardownLessLeakFixture($themeDir, $secretPath); + } + } + + /** + * The same holds for an `@import` inside an imported stylesheet. + */ + public function testCssImportFilterBlocksTraversalFromNestedImport() + { + $themeDir = $this->makeTempCssThemeDir(); + mkdir($themeDir . '/assets/sub', 0777, true); + $secretPath = tempnam(sys_get_temp_dir(), 'wn-sec-'); + file_put_contents($secretPath, "APP_KEY=combine-leak-canary\n"); + file_put_contents($themeDir . '/assets/poc.css', '@import url("sub/child.css");'); + file_put_contents( + $themeDir . '/assets/sub/child.css', + '@import url("' . str_repeat('../', 20) . ltrim($secretPath, '/') . '");' . "\n" . '.child-marker { color: red; }' + ); + + try { + $css = $this->compileCssTo($themeDir, 'assets/poc.css'); + $this->assertStringNotContainsString('combine-leak-canary', $css); + $this->assertStringContainsString('child-marker', $css); + } finally { + @unlink($secretPath); + \File::deleteDirectory($themeDir); + } + } + + /** + * Legitimate same-tree `@import` must still be inlined, otherwise the hardening + * has broken every stylesheet that composes itself from partials. + */ + public function testCssImportFilterAllowsSameTreeImport() + { + $themeDir = $this->makeTempCssThemeDir(); + file_put_contents($themeDir . '/assets/partial.css', '.partial-marker { color: orange; }'); + file_put_contents( + $themeDir . '/assets/main.css', + '@import url("partial.css");' . "\n" . '.main-marker { color: blue; }' + ); + + try { + $css = $this->compileCssTo($themeDir, 'assets/main.css'); + $this->assertStringContainsString('partial-marker', $css); + $this->assertStringContainsString('main-marker', $css); + } finally { + \File::deleteDirectory($themeDir); + } + } + /** * SCSS `@import` resolution must stay within the allowed import roots, the same * as the LESS and JavaScript importers above. @@ -309,6 +538,56 @@ public function testScssCompilerAllowsImportFromAllowedRoot() } } + /** + * Writes a `.css` proof-of-concept asset plus a secret file outside every allowed + * import root. `%SECRET%` is substituted with the secret's absolute path and + * `%SECRET_RELATIVE%` with its path relative to the filesystem root, for building + * `..`-traversal payloads. + * + * @return array{0:string,1:string} [theme dir, secret path] + */ + protected function setupCssLeakFixture(string $pocTemplate): array + { + $themeDir = $this->makeTempCssThemeDir(); + // The name carries no canary: a rejected `@import` is emitted verbatim, so a + // recognisable filename in the statement would look like a leak of the file's + // contents. + $secretPath = tempnam(sys_get_temp_dir(), 'wn-sec-'); + file_put_contents($secretPath, "APP_KEY=combine-leak-canary\n"); + + $poc = str_replace( + ['%SECRET%', '%SECRET_RELATIVE%'], + [$secretPath, ltrim($secretPath, '/')], + $pocTemplate + ); + file_put_contents($themeDir . '/assets/poc.css', $poc); + + return [$themeDir, $secretPath]; + } + + /** + * A theme directory under the real `themes_path()`, so that the combiner's CSS + * import validator — which confines imports to themes/plugins/modules — sees the + * fixture the way it sees a real theme asset. + */ + protected function makeTempCssThemeDir(): string + { + $themeDir = themes_path('wn-sec-test-' . bin2hex(random_bytes(4))); + mkdir($themeDir . '/assets', 0777, true); + return $themeDir; + } + + protected function compileCssTo(string $themeDir, string $relativeAsset): string + { + $dest = sys_get_temp_dir() . '/winter-combine-out-' . bin2hex(random_bytes(4)) . '.css'; + try { + CombineAssets::instance()->combineToFile([$relativeAsset], $dest, $themeDir); + return file_get_contents($dest) ?: ''; + } finally { + @unlink($dest); + } + } + protected function compileScssTo(string $themeDir, string $relativeAsset): string { $dest = sys_get_temp_dir() . '/winter-combine-out-' . bin2hex(random_bytes(4)) . '.css'; @@ -350,6 +629,30 @@ protected function setupLessLeakFixture(string $pocTemplate): array return [$themeDir, $secretPath]; } + /** + * Builds a theme whose `assets/less/poc.less` imports `assets/less/sub/child.less`, + * with `$childTemplate` as the child's contents and `%SECRET%` replaced by the + * basename of a canary file written just outside the theme tree (and outside every + * allowed import root). + * + * @return array{0:string,1:string} [theme dir, secret path] + */ + protected function setupNestedLessLeakFixture(string $childTemplate): array + { + $themeDir = $this->makeTempThemeDir(); + $secretPath = dirname($themeDir) . '/less-secret-' . bin2hex(random_bytes(4)) . '.txt'; + file_put_contents($secretPath, "APP_KEY=combine-leak-canary\n"); + + mkdir($themeDir . '/assets/less/sub'); + file_put_contents($themeDir . '/assets/less/poc.less', '@import "sub/child.less";'); + file_put_contents( + $themeDir . '/assets/less/sub/child.less', + str_replace('%SECRET%', basename($secretPath), $childTemplate) + ); + + return [$themeDir, $secretPath]; + } + protected function teardownLessLeakFixture(string $themeDir, string $secretPath): void { @unlink($secretPath); diff --git a/modules/system/tests/classes/SystemControllerTest.php b/modules/system/tests/classes/SystemControllerTest.php new file mode 100644 index 0000000000..2cffa8ac58 --- /dev/null +++ b/modules/system/tests/classes/SystemControllerTest.php @@ -0,0 +1,59 @@ +themeDir = base_path('storage/framework/cache/combine-tests/theme-' . bin2hex(random_bytes(4))); + mkdir($this->themeDir . '/assets', 0777, true); + file_put_contents($this->themeDir . '/assets/broken.scss', '.x { color: red; '); + } + + public function tearDown(): void + { + \File::deleteDirectory($this->themeDir); + + parent::tearDown(); + } + + public function testCombineHidesCompilerErrorsOutsideDebugMode() + { + Config::set('app.debug', false); + + $response = (new SystemController)->combine($this->combineBrokenAsset()); + + $this->assertSame(500, $response->getStatusCode()); + $this->assertStringContainsString(trans('system::lang.combiner.error'), $response->getContent()); + $this->assertStringNotContainsString('unclosed block', $response->getContent()); + } + + public function testCombineShowsCompilerErrorsInDebugMode() + { + Config::set('app.debug', true); + + $response = (new SystemController)->combine($this->combineBrokenAsset()); + + $this->assertSame(500, $response->getStatusCode()); + $this->assertStringContainsString('unclosed block', $response->getContent()); + } + + /** + * Registers a combined asset that fails to compile and returns its combiner file name. + */ + protected function combineBrokenAsset(): string + { + return basename(CombineAssets::combine(['assets/broken.scss'], $this->themeDir)); + } +} From 6af920ce6e9f6fe75da9b97a95b994fd0964529c Mon Sep 17 00:00:00 2001 From: Luke Towers Date: Fri, 25 Sep 2026 20:28:04 -0600 Subject: [PATCH 3/3] Leave ColorPicker format validation as it was Unrecognised formats values passing through unvalidated is the intended behaviour from #1020, and core only uses the validated hex and all formats. The change is out of scope here. --- modules/backend/formwidgets/ColorPicker.php | 40 ++++++++++------- .../tests/formwidgets/ColorPickerTest.php | 44 ------------------- 2 files changed, 24 insertions(+), 60 deletions(-) diff --git a/modules/backend/formwidgets/ColorPicker.php b/modules/backend/formwidgets/ColorPicker.php index 7183268a81..248b078a2d 100644 --- a/modules/backend/formwidgets/ColorPicker.php +++ b/modules/backend/formwidgets/ColorPicker.php @@ -54,8 +54,8 @@ class ColorPicker extends FormWidgetBase /** * @var string|array Color format(s) to allow for the resulting color value. Specify "all" as a string to allow all - * formats, or "custom" to accept the value as-is without format validation. - * Allowed values: 'cmyk', 'hex', 'hsl', 'rgb', 'all', 'custom' + * formats. + * Allowed values: 'cmyk', 'hex', 'hsl', 'rgb', 'all' */ public $formats = 'hex'; @@ -258,21 +258,29 @@ public function getSaveValue($value) return null; } - // "custom" is the explicit opt-out of format validation, for fields that - // store something the built-in patterns cannot express. - if ($this->formats === 'custom') { - return $value; + switch (is_array($this->formats) ? 'all' : $this->formats) { + case 'cmyk': + case 'hex': + case 'hsl': + case 'rgb': + if (!preg_match($this->validationPatterns[$this->formats], $value)) { + throw new ApplicationException(Lang::get('backend::lang.field.colors_invalid_input')); + } + break; + case 'all': + $valid = false; + foreach ($this->validationPatterns as $pattern) { + if (preg_match($pattern, $value)) { + $valid = true; + break; + } + } + if (!$valid) { + throw new ApplicationException(Lang::get('backend::lang.field.colors_invalid_input')); + } + break; } - // Validate against the same normalised format list the field renders with, so a - // `formats` value that names no known format falls back to "hex" rather than - // accepting anything. - foreach ($this->getFormats() as $format) { - if (preg_match($this->validationPatterns[$format], $value)) { - return $value; - } - } - - throw new ApplicationException(Lang::get('backend::lang.field.colors_invalid_input')); + return $value; } } diff --git a/modules/backend/tests/formwidgets/ColorPickerTest.php b/modules/backend/tests/formwidgets/ColorPickerTest.php index 3848077940..c97200441a 100644 --- a/modules/backend/tests/formwidgets/ColorPickerTest.php +++ b/modules/backend/tests/formwidgets/ColorPickerTest.php @@ -121,50 +121,6 @@ public function testAllowCustomSaveValue(): void $this->assertEquals('Test(51.9, 152, 219, 1)', $widget->getSaveValue('Test(51.9, 152, 219, 1)')); } - /** - * A `formats` value that names no known format renders the field as "hex", so - * "hex" is what must be enforced on save rather than accepting any value. - */ - public function testUnrecognisedFormatFallsBackToHexValidation(): void - { - $widget = $this->makeWidget([ - 'formats' => 'hexadecimal', - ]); - - $this->assertEquals('#3498DB', $widget->getSaveValue('#3498DB')); - - $this->expectException(ApplicationException::class); - $widget->getSaveValue('not a colour'); - } - - /** - * An array of formats must accept only the formats it lists. - */ - public function testArrayFormatAcceptsOnlyListedFormats(): void - { - $widget = $this->makeWidget([ - 'formats' => ['hex'], - ]); - - $this->assertEquals('#3498DB', $widget->getSaveValue('#3498DB')); - - $this->expectException(ApplicationException::class); - $widget->getSaveValue('rgba(51.9, 152, 219, 1)'); - } - - /** - * ...but every format it does list keeps working. - */ - public function testArrayFormatAcceptsEachListedFormat(): void - { - $widget = $this->makeWidget([ - 'formats' => ['hex', 'rgb'], - ]); - - $this->assertEquals('#3498DB', $widget->getSaveValue('#3498DB')); - $this->assertEquals('rgba(51.9, 152, 219, 1)', $widget->getSaveValue('rgba(51.9, 152, 219, 1)')); - } - protected function makeWidget(array $config = []): ColorPicker { return new ColorPicker(new Controller(), new FormField('test', 'Test'), $config);