From 0cce80c63292a7db9e80aaf7131f4111c23cd2ea Mon Sep 17 00:00:00 2001 From: Luke Towers Date: Sun, 26 Jul 2026 12:19:13 -0600 Subject: [PATCH 1/5] Confine JavaScript importer paths to allowed roots (GHSA-2223-f22x-24cq) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The JS combiner `=include`/`=require` directives resolved their target with `realpath()` and no confinement check, so a `.js` asset containing `=include ../../../.env` inlined arbitrary server-readable files into combined output that `System\Classes\SystemController@combine` serves unauthenticated — an authenticated local file disclosure (APP_KEY, DB credentials, etc.). This is the JavaScript sibling of the LESS import flaw hardened for GHSA-58fp-mcx6-7qf9; that fix gated the LESS filter but left `JavascriptImporter` exploitable. Harden `JavascriptImporter::directiveInclude()` with two layers: - Restrict inlined files to the `.js` extension, rejecting `.env`/`.php`/`.log` and the like before any path math. Extension-less includes are unaffected — they are still resolved to `.js` first — and this mirrors the CSS importer, which only inlines `.css`. - Confine the resolved path to the including file's own directory subtree or a caller-configured allowed root, via a new shared `ImportGuard` policy. Extract the confinement decision shared with the LESS importer into `ImportGuard::isAllowed()`, and the allowed-roots storage into a `HasAllowedImportRoots` trait, so both filters enforce one identical policy. `System\Classes\CombineAssets` configures the roots (themes/plugins/modules). Co-Authored-By: Claude Fable 5 --- .../Assetic/Filter/HasAllowedImportRoots.php | 32 +++++ src/Parse/Assetic/Filter/ImportGuard.php | 51 +++++++ .../Assetic/Filter/JavascriptImporter.php | 33 +++++ src/Parse/Assetic/Filter/LessCompiler.php | 26 +--- .../Assetic/Filter/LessImportResolver.php | 9 +- .../Parse/Assetic/JavascriptImporterTest.php | 131 ++++++++++++++++++ 6 files changed, 250 insertions(+), 32 deletions(-) create mode 100644 src/Parse/Assetic/Filter/HasAllowedImportRoots.php create mode 100644 src/Parse/Assetic/Filter/ImportGuard.php create mode 100644 tests/Parse/Assetic/JavascriptImporterTest.php diff --git a/src/Parse/Assetic/Filter/HasAllowedImportRoots.php b/src/Parse/Assetic/Filter/HasAllowedImportRoots.php new file mode 100644 index 000000000..b8e09525e --- /dev/null +++ b/src/Parse/Assetic/Filter/HasAllowedImportRoots.php @@ -0,0 +1,32 @@ +allowedImportRoots = $roots; + } +} diff --git a/src/Parse/Assetic/Filter/ImportGuard.php b/src/Parse/Assetic/Filter/ImportGuard.php new file mode 100644 index 000000000..6f81fdafe --- /dev/null +++ b/src/Parse/Assetic/Filter/ImportGuard.php @@ -0,0 +1,51 @@ +scriptFile); + if ($required) { + throw new RuntimeException($errorMsg); + } + + $result .= '/* ' . $errorMsg . ' */' . PHP_EOL; + continue; + } + $scriptPath = realpath($this->scriptPath . '/' . $script); if (!File::isFile($scriptPath)) { $errorMsg = sprintf("File '%s' not found. in %s", $script, $this->scriptFile); @@ -110,6 +130,19 @@ protected function directiveInclude($data, $required = false) continue; } + /* + * Confine the resolved path to the including file's own directory subtree + * or a caller-configured allowed root, mirroring the LESS importer gate + * (see ImportGuard). Without this, `=include ../../../.env` would resolve + * via `realpath()` traversal and inline an arbitrary server-readable file + * into public combiner output. See GHSA-2223-f22x-24cq. + */ + if (!ImportGuard::isAllowed($scriptPath, $this->scriptPath, $this->allowedImportRoots)) { + $errorMsg = sprintf("File '%s' is outside the allowed import paths. in %s", $script, $this->scriptFile); + $result .= '/* ' . $errorMsg . ' */' . PHP_EOL; + continue; + } + /* * Exclude duplicates */ diff --git a/src/Parse/Assetic/Filter/LessCompiler.php b/src/Parse/Assetic/Filter/LessCompiler.php index 9713b1363..6490c2c47 100644 --- a/src/Parse/Assetic/Filter/LessCompiler.php +++ b/src/Parse/Assetic/Filter/LessCompiler.php @@ -18,39 +18,17 @@ */ class LessCompiler extends BaseFilter implements HashableInterface, DependencyExtractorInterface { + use HasAllowedImportRoots; + protected $presets = []; protected $lastHash; - /** - * Additional roots beyond the asset's own source directory that `@import` - * directives are allowed to resolve into. Configured by the caller (typically - * `System\Classes\CombineAssets`) to permit legitimate cross-tree imports - * (e.g. a plugin asset importing a module asset). Defaults to none — the - * asset's own directory subtree is always allowed implicitly via the - * resolver's contextDir rule. - * - * @var string[] - */ - protected array $allowedImportRoots = []; - public function setPresets(array $presets) { $this->presets = $presets; } - /** - * Configure additional roots that `@import` directives may resolve into. The - * source file's own directory is always allowed; this list adds cross-tree - * destinations. - * - * @param string[] $roots - */ - public function setAllowedImportRoots(array $roots): void - { - $this->allowedImportRoots = $roots; - } - public function filterLoad(AssetInterface $asset) { $parser = new Less_Parser(); diff --git a/src/Parse/Assetic/Filter/LessImportResolver.php b/src/Parse/Assetic/Filter/LessImportResolver.php index 51bbbc91c..a7136d060 100644 --- a/src/Parse/Assetic/Filter/LessImportResolver.php +++ b/src/Parse/Assetic/Filter/LessImportResolver.php @@ -1,7 +1,6 @@ tmpRoot = sys_get_temp_dir() . '/storm-js-importer-' . bin2hex(random_bytes(4)); + mkdir($this->tmpRoot . '/assets/sub', 0777, true); + mkdir($this->tmpRoot . '/allowed', 0777, true); + + // Secrets a level above the asset tree — the disclosure targets. + file_put_contents($this->tmpRoot . '/secret.env', 'APP_KEY=leak-env'); + file_put_contents($this->tmpRoot . '/secret.js', 'var SECRET_JS = "leak-js";'); + + // Legitimate includes. + file_put_contents($this->tmpRoot . '/allowed/lib.js', 'var LIB = 1;'); + file_put_contents($this->tmpRoot . '/assets/partial.js', 'var PARTIAL = 1;'); + file_put_contents($this->tmpRoot . '/assets/sub/nested.js', 'var NESTED = 1;'); + + $this->tmpReal = realpath($this->tmpRoot); + } + + protected function tearDown(): void + { + (new \Winter\Storm\Filesystem\Filesystem())->deleteDirectory($this->tmpRoot); + parent::tearDown(); + } + + /** + * Run a JS entry file's contents through the importer and return the combined output. + * + * @param string[] $allowedRoots + */ + protected function combine(string $entryContents, array $allowedRoots = []): string + { + $entry = $this->tmpReal . '/assets/entry.js'; + file_put_contents($entry, $entryContents); + + $filter = new JavascriptImporter(); + $filter->setAllowedImportRoots($allowedRoots); + + $asset = new FileAsset($entry, [$filter], $this->tmpReal . '/assets', 'entry.js'); + $asset->load(); + + return $asset->dump(); + } + + public function testAllowsSameDirectoryInclude() + { + $out = $this->combine("/*\n=include partial.js\n*/\n"); + + $this->assertStringContainsString('var PARTIAL = 1;', $out); + } + + public function testAllowsSubdirectoryInclude() + { + $out = $this->combine("/*\n=include sub/nested.js\n*/\n"); + + $this->assertStringContainsString('var NESTED = 1;', $out); + } + + /** + * Extension-less targets must continue to resolve with `.js` appended, so existing + * assets that rely on the auto-append keep working after the hardening. + */ + public function testExtensionlessIncludeStillResolvesToJs() + { + $out = $this->combine("/*\n=include partial\n*/\n"); + + $this->assertStringContainsString('var PARTIAL = 1;', $out); + } + + public function testBlocksEnvTraversal() + { + $out = $this->combine("/*\n=include ../secret.env\n*/\n"); + + $this->assertStringNotContainsString('leak-env', $out); + $this->assertStringContainsString('disallowed extension', $out); + } + + public function testBlocksJsTraversalOutsideAllowedRoots() + { + $out = $this->combine("/*\n=include ../secret.js\n*/\n"); + + $this->assertStringNotContainsString('leak-js', $out); + $this->assertStringContainsString('outside the allowed import paths', $out); + } + + public function testAllowsJsIncludeWithinExplicitlyAllowedRoot() + { + $out = $this->combine( + "/*\n=include ../allowed/lib.js\n*/\n", + [$this->tmpReal . '/allowed'] + ); + + $this->assertStringContainsString('var LIB = 1;', $out); + } + + public function testBlocksJsIncludeIntoRootNotWhitelisted() + { + // Same include as above, but without granting the `allowed` root. + $out = $this->combine("/*\n=include ../allowed/lib.js\n*/\n"); + + $this->assertStringNotContainsString('var LIB = 1;', $out); + $this->assertStringContainsString('outside the allowed import paths', $out); + } + + public function testRequireThrowsOnDisallowedExtension() + { + $this->expectException(\RuntimeException::class); + + $this->combine("/*\n=require ../secret.env\n*/\n"); + } +} From 028e2b7dd8f0e4d17953ae9f6d8ece773523ff71 Mon Sep 17 00:00:00 2001 From: Luke Towers Date: Sun, 26 Jul 2026 12:50:08 -0600 Subject: [PATCH 2/5] Replace ImportGuard with a generic PathResolver::withinAny() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The import confinement check was a single static method on a dedicated ImportGuard class. Fold it into PathResolver as a general-purpose `withinAny($path, $directories)` — a natural sibling of `within()` — which removes the one-method class and is reusable beyond the asset importers. The "source directory always allowed" semantics become just another entry in the directory list: callers pass the context dir (which may be null — non-string and empty entries are ignored) alongside the configured roots. JavascriptImporter and LessImportResolver call it directly; no behaviour change. Co-Authored-By: Claude Fable 5 --- src/Filesystem/PathResolver.php | 20 ++++++++ .../Assetic/Filter/HasAllowedImportRoots.php | 3 +- src/Parse/Assetic/Filter/ImportGuard.php | 51 ------------------- .../Assetic/Filter/JavascriptImporter.php | 11 ++-- .../Assetic/Filter/LessImportResolver.php | 3 +- 5 files changed, 30 insertions(+), 58 deletions(-) delete mode 100644 src/Parse/Assetic/Filter/ImportGuard.php diff --git a/src/Filesystem/PathResolver.php b/src/Filesystem/PathResolver.php index 24661de77..d3d2ab779 100644 --- a/src/Filesystem/PathResolver.php +++ b/src/Filesystem/PathResolver.php @@ -122,6 +122,26 @@ public static function within(string $path, string $directory): bool || starts_with($path, rtrim($directory, '/\\') . '/'); } + /** + * Determines whether a path is within any of the given directories. + * + * Returns true as soon as {@see within()} matches one of $directories. Empty and + * non-string entries are ignored, so callers may pass an optional context + * directory (which may be null) alongside a list of roots without pre-filtering. + * + * @param string[] $directories + */ + public static function withinAny(string $path, array $directories): bool + { + foreach ($directories as $directory) { + if (is_string($directory) && $directory !== '' && static::within($path, $directory)) { + return true; + } + } + + return false; + } + /** * Join two paths, making sure they use the correct directory separators. */ diff --git a/src/Parse/Assetic/Filter/HasAllowedImportRoots.php b/src/Parse/Assetic/Filter/HasAllowedImportRoots.php index b8e09525e..df850bd47 100644 --- a/src/Parse/Assetic/Filter/HasAllowedImportRoots.php +++ b/src/Parse/Assetic/Filter/HasAllowedImportRoots.php @@ -6,7 +6,8 @@ * * Configured by the caller (typically `System\Classes\CombineAssets`) to permit * legitimate cross-tree imports — e.g. a plugin asset importing a module asset — while - * still confining everything else. Consumed together with {@see ImportGuard}. + * still confining everything else. The stored roots are enforced via + * {@see \Winter\Storm\Filesystem\PathResolver::withinAny()}. */ trait HasAllowedImportRoots { diff --git a/src/Parse/Assetic/Filter/ImportGuard.php b/src/Parse/Assetic/Filter/ImportGuard.php deleted file mode 100644 index 6f81fdafe..000000000 --- a/src/Parse/Assetic/Filter/ImportGuard.php +++ /dev/null @@ -1,51 +0,0 @@ -scriptPath, $this->allowedImportRoots)) { + if (!PathResolver::withinAny($scriptPath, array_merge([$this->scriptPath], $this->allowedImportRoots))) { $errorMsg = sprintf("File '%s' is outside the allowed import paths. in %s", $script, $this->scriptFile); $result .= '/* ' . $errorMsg . ' */' . PHP_EOL; continue; diff --git a/src/Parse/Assetic/Filter/LessImportResolver.php b/src/Parse/Assetic/Filter/LessImportResolver.php index a7136d060..035655aef 100644 --- a/src/Parse/Assetic/Filter/LessImportResolver.php +++ b/src/Parse/Assetic/Filter/LessImportResolver.php @@ -1,6 +1,7 @@ Date: Sun, 26 Jul 2026 12:59:50 -0600 Subject: [PATCH 3/5] Require assetic/framework ^3.2.1 for CssImportFilter validator The combiner's CSS import confinement relies on `CssImportFilter::setImportValidator()`, added in assetic/framework v3.2.1. Raise the minimum so the method is guaranteed present. Co-Authored-By: Claude Fable 5 --- composer.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/composer.json b/composer.json index a7fb43ee0..86e76ac13 100644 --- a/composer.json +++ b/composer.json @@ -35,7 +35,7 @@ "ext-pdo": "*", "ext-zip": "*", - "assetic/framework": "~3.0", + "assetic/framework": "^3.2.1", "doctrine/dbal": "^2.6", "enshrined/svg-sanitize": "~0.16", "laravel/framework": "^9.49", From 7d76f2dfe3ce113ca509c2f84a46be37046565ff Mon Sep 17 00:00:00 2001 From: Luke Towers Date: Sun, 26 Jul 2026 13:05:51 -0600 Subject: [PATCH 4/5] Add direct unit coverage for PathResolver::withinAny() withinAny() was only exercised transitively through the JS/LESS importer tests. Add a dedicated test covering multi-directory matching, the no-match and empty-list cases, and the null/empty/non-string skipping that lets callers pass a nullable context directory alongside the roots list. Co-Authored-By: Claude Fable 5 --- tests/Filesystem/PathResolverTest.php | 39 +++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/tests/Filesystem/PathResolverTest.php b/tests/Filesystem/PathResolverTest.php index 486f1f618..c61db4926 100644 --- a/tests/Filesystem/PathResolverTest.php +++ b/tests/Filesystem/PathResolverTest.php @@ -383,6 +383,45 @@ public function testWithinDirectoryRejectsPrefixCollision() } } + /** + * {@see PathResolver::withinAny()} returns true if the path is within any of the + * given directories. Null, empty and non-string entries are ignored, so callers + * can pass an optional (nullable) context directory alongside a roots list + * without pre-filtering — the behaviour the JS/LESS importers rely on. + */ + public function testWithinAny() + { + $base = sys_get_temp_dir() . '/pathresolver-withinany-' . bin2hex(random_bytes(4)); + mkdir($base . '/allowed/inside', 0777, true); + mkdir($base . '/other', 0777, true); + mkdir($base . '/outside', 0777, true); + file_put_contents($base . '/allowed/inside/file', 'x'); + + try { + $target = $base . '/allowed/inside/file'; + + // Matches when the containing directory appears anywhere in the list. + $this->assertTrue(PathResolver::withinAny($target, [$base . '/allowed'])); + $this->assertTrue(PathResolver::withinAny($target, [$base . '/other', $base . '/allowed'])); + + // No directory in the list contains the path. + $this->assertFalse(PathResolver::withinAny($target, [$base . '/other', $base . '/outside'])); + + // Empty list never matches. + $this->assertFalse(PathResolver::withinAny($target, [])); + + // Null, empty-string and non-string entries are skipped; a valid later + // entry still matches (the nullable context-dir usage in the importers). + $this->assertTrue(PathResolver::withinAny($target, [null, '', $base . '/allowed'])); + $this->assertFalse(PathResolver::withinAny($target, [null, '', $base . '/outside'])); + + // A list of only skippable entries never matches. + $this->assertFalse(PathResolver::withinAny($target, [null, '', false])); + } finally { + (new \Winter\Storm\Filesystem\Filesystem())->deleteDirectory($base); + } + } + public function testWithOpenBaseDirRestrictions() { $this->markTestSkipped( From 4ae7d40bda7c9aa7e0d2733d5bdf19d9d5763839 Mon Sep 17 00:00:00 2001 From: Luke Towers Date: Sun, 26 Jul 2026 13:29:17 -0600 Subject: [PATCH 5/5] Throw on required JS includes outside allowed import roots Make the path-confinement guard in JavascriptImporter::directiveInclude() honour $required: a mandatory =require into a disallowed path now throws a RuntimeException, matching the not-found and disallowed-extension guards. The optional =include still degrades to an error comment. Also hoist the allowed-roots array out of the per-include loop. Adds a regression test. Co-Authored-By: Claude Fable 5 --- src/Parse/Assetic/Filter/JavascriptImporter.php | 14 +++++++++++++- tests/Parse/Assetic/JavascriptImporterTest.php | 13 +++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/src/Parse/Assetic/Filter/JavascriptImporter.php b/src/Parse/Assetic/Filter/JavascriptImporter.php index 147786d13..d4b407245 100644 --- a/src/Parse/Assetic/Filter/JavascriptImporter.php +++ b/src/Parse/Assetic/Filter/JavascriptImporter.php @@ -95,6 +95,14 @@ protected function directiveInclude($data, $required = false) $require = explode(',', $data); $result = ""; + /* + * The set of roots an include may resolve into: the including file's own + * directory subtree plus any caller-configured cross-tree roots. Computed + * once here rather than per-iteration; $this->scriptPath is stable across + * the loop (nested parsing saves and restores it below). + */ + $allowedRoots = array_merge([$this->scriptPath], $this->allowedImportRoots); + foreach ($require as $script) { $script = trim($script); @@ -138,8 +146,12 @@ protected function directiveInclude($data, $required = false) * traversal and inline an arbitrary server-readable file into public * combiner output. See GHSA-2223-f22x-24cq. */ - if (!PathResolver::withinAny($scriptPath, array_merge([$this->scriptPath], $this->allowedImportRoots))) { + if (!PathResolver::withinAny($scriptPath, $allowedRoots)) { $errorMsg = sprintf("File '%s' is outside the allowed import paths. in %s", $script, $this->scriptFile); + if ($required) { + throw new RuntimeException($errorMsg); + } + $result .= '/* ' . $errorMsg . ' */' . PHP_EOL; continue; } diff --git a/tests/Parse/Assetic/JavascriptImporterTest.php b/tests/Parse/Assetic/JavascriptImporterTest.php index a36aee243..5ce16b5b8 100644 --- a/tests/Parse/Assetic/JavascriptImporterTest.php +++ b/tests/Parse/Assetic/JavascriptImporterTest.php @@ -128,4 +128,17 @@ public function testRequireThrowsOnDisallowedExtension() $this->combine("/*\n=require ../secret.env\n*/\n"); } + + /** + * A mandatory `=require` for a `.js` file outside the allowed roots must fail hard, + * mirroring the not-found and disallowed-extension guards rather than silently + * degrading to a comment the way the optional `=include` does. + */ + public function testRequireThrowsOnPathOutsideAllowedRoots() + { + $this->expectException(\RuntimeException::class); + $this->expectExceptionMessage('outside the allowed import paths'); + + $this->combine("/*\n=require ../secret.js\n*/\n"); + } }