diff --git a/composer.json b/composer.json index 7f273a30..5a0b6ebd 100644 --- a/composer.json +++ b/composer.json @@ -35,7 +35,7 @@ "ext-pdo": "*", "ext-zip": "*", - "assetic/framework": "^3.2.2", + "assetic/framework": "^3.2.3", "doctrine/dbal": "^2.6", "enshrined/svg-sanitize": "~0.16", "laravel/framework": "^9.49", diff --git a/src/Parse/Assetic/Filter/LessImportResolver.php b/src/Parse/Assetic/Filter/LessImportResolver.php index 035655ae..3762e591 100644 --- a/src/Parse/Assetic/Filter/LessImportResolver.php +++ b/src/Parse/Assetic/Filter/LessImportResolver.php @@ -18,6 +18,12 @@ * authoritative resolver, we have to collide-and-override the auto-added entry by * using its exact normalised key (`buildImportDirs()` does this). * + * That auto-added entry is re-created for *every* file less.php parses, not just the + * entry file, and it is also consulted by `data-uri()` / `image-size()`. Overriding + * only the entry file's directory therefore leaves any `.less` imported from another + * directory ungated. `makeResolver()` closes that by registering a resolver for each + * directory it admits, so the collision follows the import graph. + * * Usage shapes: * * // parseFile()-based caller (e.g. theme asset compilation): @@ -93,6 +99,12 @@ public static function makeResolver(array $allowedRoots, ?string $contextDir = n } if (PathResolver::withinAny($resolved, array_merge([$contextDir], $allowedRoots))) { + // less.php is about to make this file's directory "current", which + // re-adds an unconfined path-form import dir for it. Claim that key + // now so the gate keeps applying to the file's own imports and to + // any data-uri() / image-size() call it makes. + self::registerDir(dirname($resolved), array_merge([$contextDir], $allowedRoots)); + return [$resolved, dirname($filename)]; } @@ -117,16 +129,40 @@ public static function buildImportDirs(string $sourceFile, array $allowedRoots): $resolvedSource = realpath($sourceFile); $sourceDir = $resolvedSource !== false ? dirname($resolvedSource) : dirname($sourceFile); - // less.php normalises its auto-added currentDirectory key by running - // it through `WinPath()` (backslash -> forward slash) before storing, - // then SetImportDirs() applies `rtrim('/\\') . '/'`. We must reproduce - // the *exact same* normalisation here or PHP `array_merge`'s - // string-key collision won't happen on Windows and the gate becomes - // non-authoritative for relative-traversal attacks (the auto-added - // path-form entry would still match first via file_exists). This is - // not just a test issue — it's a security regression on Windows. - $key = rtrim((new Filesystem())->normalizePath($sourceDir), '/') . '/'; + return [self::importDirKey($sourceDir) => self::makeResolver($allowedRoots, $sourceDir)]; + } + + /** + * Register a resolver for `$dir` directly on the parser's import-dir list, so it + * collides with the path-form entry less.php auto-adds while that directory is + * the current one. Existing entries are left alone: the first resolver to claim + * a directory is the one that admitted it, and re-registering would only widen + * the allowed set. + * + * @param string[] $allowedRoots + */ + public static function registerDir(string $dir, array $allowedRoots): void + { + $key = self::importDirKey($dir); + + if (!isset(\Less_Parser::$options['import_dirs'][$key])) { + \Less_Parser::$options['import_dirs'][$key] = self::makeResolver($allowedRoots, $dir); + } + } - return [$key => self::makeResolver($allowedRoots, $sourceDir)]; + /** + * Reproduce the exact key less.php uses for a directory in its import-dir list. + * + * It normalises the key by running the file through `AbsPath()`/`WinPath()` + * (backslash -> forward slash) and `dirname()`-ing it with a trailing slash, then + * `SetImportDirs()` applies `rtrim('/\\') . '/'`. Reproducing that normalisation + * exactly is what makes PHP `array_merge` string-key collision replace the + * auto-added entry with our callable. Getting it wrong doesn't fail loudly — it + * silently leaves the auto-added path-form entry matching first via `file_exists`, + * which is a security regression, and it differs by platform (Windows). + */ + public static function importDirKey(string $dir): string + { + return rtrim((new Filesystem())->normalizePath($dir), '/') . '/'; } } diff --git a/src/Parse/Assetic/Filter/ScssCompiler.php b/src/Parse/Assetic/Filter/ScssCompiler.php index 59b0d5ae..b3db689a 100644 --- a/src/Parse/Assetic/Filter/ScssCompiler.php +++ b/src/Parse/Assetic/Filter/ScssCompiler.php @@ -5,6 +5,7 @@ use Assetic\Contracts\Asset\AssetInterface; use Assetic\Contracts\Filter\HashableInterface; use Assetic\Contracts\Filter\DependencyExtractorInterface; +use Winter\Storm\Filesystem\PathResolver; use Winter\Storm\Support\Facades\Event; /** @@ -15,12 +16,38 @@ */ class ScssCompiler extends ScssphpFilter implements HashableInterface, DependencyExtractorInterface { + use HasAllowedImportRoots; + protected $currentFiles = []; protected $variables = []; protected $lastHash; + /** + * Import paths configured on this filter, mirrored from the parent so that they + * can be treated as allowed import roots. The parent stores them privately. + * + * @var array + */ + protected $configuredImportPaths = []; + + /** + * Directory of the asset currently being compiled. Always an allowed import root, + * so same-tree `@import "partial"` keeps working without configuration. + * + * @var string|null + */ + protected $sourceDirectory = null; + + /** + * Whether getChildren() is already running. The parent recurses through the + * override for each child, and only the outermost call may set the root. + * + * @var bool + */ + protected $resolvingChildren = false; + public function __construct() { Event::listen('cms.combiner.beforePrepare', function ($compiler, $assets) { @@ -30,6 +57,13 @@ public function __construct() } } }); + + // Confine `@import` resolution to the compiled asset's own directory subtree + // plus any caller-configured roots, matching the LESS and JavaScript + // compilers. Without it, scssphp resolves imports against the importing + // file's own directory with `..` traversal allowed, so resolution is not + // bounded to the asset tree. + $this->setImportValidator([$this, 'isImportAllowed']); } public function setPresets(array $presets) @@ -47,12 +81,68 @@ public function addVariable($variable) $this->variables[] = $variable; } + public function setImportPaths(array $paths) + { + $this->configuredImportPaths = $paths; + + parent::setImportPaths($paths); + } + + public function addImportPath($path) + { + $this->configuredImportPaths[] = $path; + + parent::addImportPath($path); + } + + /** + * Determines whether scssphp may inline the file it resolved an `@import` to. + * + * Passed to {@see ScssphpFilter::setImportValidator()} and called with the + * resolved filesystem path of every candidate import. + */ + public function isImportAllowed(string $path): bool + { + $resolved = PathResolver::resolve($path); + + if ($resolved === false) { + return false; + } + + // withinAny() skips non-string entries, so callable import paths (which + // scssphp also accepts) are simply not treated as roots. + return PathResolver::withinAny($resolved, array_merge( + [$this->sourceDirectory], + $this->configuredImportPaths, + $this->allowedImportRoots + )); + } + public function filterLoad(AssetInterface $asset) { + $this->sourceDirectory = $asset->getSourceDirectory(); + parent::setVariables($this->variables); parent::filterLoad($asset); } + public function getChildren(AssetFactory $factory, $content, $loadPath = null) + { + // Nested calls keep the entry asset's directory as the root, as filterLoad() does. + if ($this->resolvingChildren) { + return parent::getChildren($factory, $content, $loadPath); + } + + $this->resolvingChildren = true; + $this->sourceDirectory = $loadPath; + + try { + return parent::getChildren($factory, $content, $loadPath); + } finally { + $this->resolvingChildren = false; + } + } + public function setHash($hash) { $this->lastHash = $hash; diff --git a/tests/Parse/Assetic/LessCompilerTest.php b/tests/Parse/Assetic/LessCompilerTest.php index 3c764b97..1730257a 100644 --- a/tests/Parse/Assetic/LessCompilerTest.php +++ b/tests/Parse/Assetic/LessCompilerTest.php @@ -102,6 +102,96 @@ public function testBlocksCrossTreeImportWhenRootIsNotWhitelisted() $this->assertStringNotContainsString('cross-tree-marker', $css); } + /** + * less.php re-creates the unconfined path-form import dir for every file it + * parses, keyed by that file's own directory. Confining only the entry asset's + * directory therefore left anything imported from a subdirectory ungated. + */ + public function testBlocksTraversalFromAnImportedSubdirectoryFile() + { + mkdir($this->tmpReal . '/theme/assets/less/sub', 0777, true); + $main = $this->tmpReal . '/theme/assets/less/main.less'; + file_put_contents($main, '@import "sub/child.less"; .main { color: blue; }'); + file_put_contents( + $this->tmpReal . '/theme/assets/less/sub/child.less', + '@import (inline) "../../../../secret.env"; .child { color: red; }' + ); + + $css = $this->compile($main); + + $this->assertStringNotContainsString('APP_KEY', $css); + $this->assertStringNotContainsString('do-not-leak-me', $css); + } + + /** + * `data-uri()` resolves through the same import-dir list as `@import` and + * inlines the file's bytes, so the gate has to cover it too. + */ + public function testBlocksDataUriFileReadFromAnImportedSubdirectoryFile() + { + mkdir($this->tmpReal . '/theme/assets/less/sub', 0777, true); + $main = $this->tmpReal . '/theme/assets/less/main.less'; + file_put_contents($main, '@import "sub/child.less"; .main { color: blue; }'); + file_put_contents( + $this->tmpReal . '/theme/assets/less/sub/child.less', + '.x { background: data-uri("text/plain", "../../../../secret.env"); }' + ); + + $css = $this->compile($main); + + $this->assertStringNotContainsString('APP_KEY', $css); + $this->assertStringNotContainsString('do-not-leak-me', $css); + } + + /** + * A legitimate multi-level partial chain inside the asset tree must keep + * resolving — the gate follows the import graph rather than blocking it. + */ + public function testAllowsNestedPartialChain() + { + mkdir($this->tmpReal . '/theme/assets/less/sub', 0777, true); + $main = $this->tmpReal . '/theme/assets/less/main.less'; + file_put_contents($main, '@import "sub/child.less"; .main-marker { color: blue; }'); + file_put_contents( + $this->tmpReal . '/theme/assets/less/sub/child.less', + '@import "deeper.less"; .child-marker { color: green; }' + ); + file_put_contents( + $this->tmpReal . '/theme/assets/less/sub/deeper.less', + '.deeper-marker { color: purple; }' + ); + + $css = $this->compile($main); + + $this->assertStringContainsString('main-marker', $css); + $this->assertStringContainsString('child-marker', $css); + $this->assertStringContainsString('deeper-marker', $css); + } + + /** + * A file admitted from a subdirectory must still be able to import from the entry + * asset's tree above it, not just from its own directory downwards. + */ + public function testAllowsImportedSubdirectoryFileToImportFromEntryDirectory() + { + mkdir($this->tmpReal . '/theme/assets/less/sub', 0777, true); + $main = $this->tmpReal . '/theme/assets/less/main.less'; + file_put_contents($main, '@import "sub/child.less"; .main-marker { color: blue; }'); + file_put_contents( + $this->tmpReal . '/theme/assets/less/sub/child.less', + '@import "../variables.less"; .child-marker { color: green; }' + ); + file_put_contents( + $this->tmpReal . '/theme/assets/less/variables.less', + '.variables-marker { color: purple; }' + ); + + $css = $this->compile($main); + + $this->assertStringContainsString('child-marker', $css); + $this->assertStringContainsString('variables-marker', $css); + } + protected function compile(string $sourceFile, ?LessCompiler $compiler = null): string { $compiler ??= new LessCompiler(); @@ -110,5 +200,4 @@ protected function compile(string $sourceFile, ?LessCompiler $compiler = null): $compiler->filterLoad($asset); return $asset->getContent(); } - } diff --git a/tests/Parse/Assetic/ScssCompilerTest.php b/tests/Parse/Assetic/ScssCompilerTest.php new file mode 100644 index 00000000..e6652d00 --- /dev/null +++ b/tests/Parse/Assetic/ScssCompilerTest.php @@ -0,0 +1,157 @@ +tmpRoot = sys_get_temp_dir() . '/storm-scss-compiler-' . bin2hex(random_bytes(4)); + mkdir($this->tmpRoot . '/theme/assets/scss/sub', 0777, true); + mkdir($this->tmpRoot . '/cross-tree', 0777, true); + $this->tmpReal = realpath($this->tmpRoot); + + // Emits a rule, so a successful import is visible in the compiled output. + file_put_contents($this->tmpReal . '/secret.scss', '.leaked { content: "do-not-leak-me"; }'); + } + + protected function tearDown(): void + { + (new \Winter\Storm\Filesystem\Filesystem())->deleteDirectory($this->tmpRoot); + parent::tearDown(); + } + + public function testBlocksRelativeTraversalImport() + { + $main = $this->tmpReal . '/theme/assets/scss/main.scss'; + file_put_contents($main, '@import "../../../secret"; .x { color: red; }'); + + $this->assertStringNotContainsString('do-not-leak-me', $this->compile($main)); + } + + public function testBlocksAbsolutePathImport() + { + $main = $this->tmpReal . '/theme/assets/scss/main.scss'; + // SCSS string literals treat a backslash as an escape, so a Windows path has + // to be written with forward slashes to survive as a usable import target. + $secret = str_replace('\\', '/', $this->tmpReal) . '/secret'; + file_put_contents($main, '@import "' . $secret . '"; .x { color: red; }'); + + // A refused import leaves scssphp with nothing to resolve. Depending on the + // platform it either emits the statement verbatim or raises a compile error; + // both are refusals, and neither may inline the file. + try { + $css = $this->compile($main); + } catch (\ScssPhp\ScssPhp\Exception\CompilerException $e) { + $css = ''; + } + + $this->assertStringNotContainsString('do-not-leak-me', $css); + } + + /** + * scssphp resolves a nested `@import` against the importing file's own + * directory, so confinement has to apply to imported files too. + */ + public function testBlocksTraversalFromAnImportedSubdirectoryFile() + { + $main = $this->tmpReal . '/theme/assets/scss/main.scss'; + file_put_contents($main, '@import "sub/child"; .main { color: blue; }'); + file_put_contents( + $this->tmpReal . '/theme/assets/scss/sub/_child.scss', + '@import "../../../../secret"; .child { color: red; }' + ); + + $this->assertStringNotContainsString('do-not-leak-me', $this->compile($main)); + } + + public function testAllowsLegitimateSameTreePartial() + { + $main = $this->tmpReal . '/theme/assets/scss/main.scss'; + file_put_contents($this->tmpReal . '/theme/assets/scss/_partial.scss', '.partial-marker { color: green; }'); + file_put_contents($main, '@import "partial"; .main-marker { color: blue; }'); + + $css = $this->compile($main); + + $this->assertStringContainsString('partial-marker', $css); + $this->assertStringContainsString('main-marker', $css); + } + + public function testAllowsNestedPartialChain() + { + $main = $this->tmpReal . '/theme/assets/scss/main.scss'; + file_put_contents($main, '@import "sub/child"; .main-marker { color: blue; }'); + file_put_contents( + $this->tmpReal . '/theme/assets/scss/sub/_child.scss', + '@import "deeper"; .child-marker { color: green; }' + ); + file_put_contents( + $this->tmpReal . '/theme/assets/scss/sub/_deeper.scss', + '.deeper-marker { color: purple; }' + ); + + $css = $this->compile($main); + + $this->assertStringContainsString('main-marker', $css); + $this->assertStringContainsString('child-marker', $css); + $this->assertStringContainsString('deeper-marker', $css); + } + + public function testAllowsCrossTreeImportWhenRootIsWhitelisted() + { + $main = $this->tmpReal . '/theme/assets/scss/main.scss'; + file_put_contents($this->tmpReal . '/cross-tree/_cross.scss', '.cross-tree-marker { color: green; }'); + file_put_contents($main, '@import "cross"; .main { color: blue; }'); + + $compiler = new ScssCompiler(); + $compiler->addImportPath($this->tmpReal . '/cross-tree'); + + $this->assertStringContainsString('cross-tree-marker', $this->compile($main, $compiler)); + } + + /** + * getChildren() recurses once per child, so an import that follows a subdirectory + * import must still be validated against the entry asset's directory. + */ + public function testGetChildrenFindsSiblingImportAfterSubdirectoryImport() + { + $dir = $this->tmpReal . '/theme/assets/scss'; + file_put_contents($dir . '/sub/_child.scss', '.child { color: red; }'); + file_put_contents($dir . '/_sibling.scss', '.sibling { color: red; }'); + + $children = (new ScssCompiler())->getChildren( + new AssetFactory($dir), + '@import "sub/child"; @import "sibling";', + $dir + ); + + $this->assertCount(2, $children); + } + + protected function compile(string $sourceFile, ?ScssCompiler $compiler = null): string + { + $compiler ??= new ScssCompiler(); + $asset = new FileAsset($sourceFile, [], dirname($sourceFile), basename($sourceFile)); + $asset->load(); + $compiler->filterLoad($asset); + return $asset->getContent(); + } +}