Confine asset compiler imports consistently across LESS and SCSS

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",

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), '/') . '/';
     }
 }

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<int, string|callable>
+     */
+    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;

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();
     }
-
 }