diff --git a/composer.json b/composer.json index a7fb43ee..86e76ac1 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", diff --git a/src/Filesystem/PathResolver.php b/src/Filesystem/PathResolver.php index 24661de7..d3d2ab77 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 new file mode 100644 index 00000000..df850bd4 --- /dev/null +++ b/src/Parse/Assetic/Filter/HasAllowedImportRoots.php @@ -0,0 +1,33 @@ +allowedImportRoots = $roots; + } +} diff --git a/src/Parse/Assetic/Filter/JavascriptImporter.php b/src/Parse/Assetic/Filter/JavascriptImporter.php index a0ef8bd5..d4b40724 100644 --- a/src/Parse/Assetic/Filter/JavascriptImporter.php +++ b/src/Parse/Assetic/Filter/JavascriptImporter.php @@ -3,6 +3,7 @@ use RuntimeException; use Assetic\Filter\BaseFilter; use Assetic\Contracts\Asset\AssetInterface; +use Winter\Storm\Filesystem\PathResolver; use Winter\Storm\Support\Facades\File; /** @@ -20,6 +21,8 @@ */ class JavascriptImporter extends BaseFilter { + use HasAllowedImportRoots; + /** * @var string Location of where the processed JS script resides. */ @@ -92,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); @@ -99,6 +110,24 @@ protected function directiveInclude($data, $required = false) $script = $script . '.js'; } + /* + * Only JavaScript files may be inlined. Rejecting any other extension + * (e.g. `.env`, `.php`, `.log`) neutralises disclosure of non-JS server + * files even before the path-confinement check below, and mirrors the + * combiner's CSS importer, which is restricted to `.css`. Extension-less + * includes keep working — they are resolved to `.js` immediately above, + * before this check. See GHSA-2223-f22x-24cq. + */ + if (strtolower((string) File::extension($script)) !== 'js') { + $errorMsg = sprintf("File '%s' has a disallowed extension. in %s", $script, $this->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 +139,23 @@ 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. + * 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 (!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; + } + /* * Exclude duplicates */ diff --git a/src/Parse/Assetic/Filter/LessCompiler.php b/src/Parse/Assetic/Filter/LessCompiler.php index 9713b136..6490c2c4 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 51bbbc91..035655ae 100644 --- a/src/Parse/Assetic/Filter/LessImportResolver.php +++ b/src/Parse/Assetic/Filter/LessImportResolver.php @@ -92,16 +92,10 @@ public static function makeResolver(array $allowedRoots, ?string $contextDir = n return $sentinel; } - if ($contextDir !== null && PathResolver::within($resolved, $contextDir)) { + if (PathResolver::withinAny($resolved, array_merge([$contextDir], $allowedRoots))) { return [$resolved, dirname($filename)]; } - foreach ($allowedRoots as $root) { - if (PathResolver::within($resolved, $root)) { - return [$resolved, dirname($filename)]; - } - } - return $sentinel; }; } diff --git a/tests/Filesystem/PathResolverTest.php b/tests/Filesystem/PathResolverTest.php index 486f1f61..c61db492 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( diff --git a/tests/Parse/Assetic/JavascriptImporterTest.php b/tests/Parse/Assetic/JavascriptImporterTest.php new file mode 100644 index 00000000..5ce16b5b --- /dev/null +++ b/tests/Parse/Assetic/JavascriptImporterTest.php @@ -0,0 +1,144 @@ +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"); + } + + /** + * 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"); + } +}