From 32c5fb1d6c0eaf1897d9e0cd6fbf293dc7c37e22 Mon Sep 17 00:00:00 2001 From: Guy Sartorelli Date: Fri, 13 Sep 2024 12:02:31 +1200 Subject: [PATCH] API Remove Path class in favour of Symfony's Path class --- src/Control/Director.php | 44 ++++--- src/Control/SimpleResourceURLGenerator.php | 32 ++--- src/Core/Manifest/Module.php | 35 +++-- src/Core/Manifest/ModuleResource.php | 45 ++----- src/Core/Path.php | 61 --------- src/Core/TempFolder.php | 1 + src/View/Requirements_Backend.php | 5 +- src/View/ThemeResourceLoader.php | 11 +- src/i18n/Messages/YamlWriter.php | 6 +- src/i18n/TextCollection/i18nTextCollector.php | 18 ++- tests/php/Core/PathTest.php | 122 ------------------ 11 files changed, 97 insertions(+), 283 deletions(-) delete mode 100644 src/Core/Path.php delete mode 100644 tests/php/Core/PathTest.php diff --git a/src/Control/Director.php b/src/Control/Director.php index 119d4e746d0..db740699e94 100644 --- a/src/Control/Director.php +++ b/src/Control/Director.php @@ -2,6 +2,7 @@ namespace SilverStripe\Control; +use InvalidArgumentException; use SilverStripe\CMS\Model\SiteTree; use SilverStripe\Control\Middleware\CanonicalURLMiddleware; use SilverStripe\Control\Middleware\HTTPMiddlewareAware; @@ -11,11 +12,11 @@ use SilverStripe\Core\Injector\Injectable; use SilverStripe\Core\Injector\Injector; use SilverStripe\Core\Kernel; -use SilverStripe\Core\Path; use SilverStripe\Versioned\Versioned; use SilverStripe\View\Requirements; use SilverStripe\View\Requirements_Backend; use SilverStripe\View\TemplateGlobalProvider; +use Symfony\Component\Filesystem\Path; /** * Director is responsible for processing URLs, and providing environment information. @@ -841,28 +842,16 @@ public static function is_site_url($url) /** * Given a filesystem reference relative to the site root, return the full file-system path. - * - * @param string $file - * - * @return string */ - public static function getAbsFile($file) + public static function getAbsFile(string $file): string { - // If already absolute - if (Director::is_absolute($file)) { - return $file; - } + $path = Director::getAbsolutePathForFile($file); - // If path is relative to public folder search there first - if (Director::publicDir()) { - $path = Path::join(Director::publicFolder(), $file); - if (file_exists($path ?? '')) { - return $path; - } + if (!Path::isBasePath(Director::baseFolder(), $path)) { + throw new InvalidArgumentException("'$file' must not be outside the project root"); } - // Default to base folder - return Path::join(Director::baseFolder(), $file); + return $path; } /** @@ -1123,4 +1112,23 @@ protected static function currentRequest(HTTPRequest $request = null) } return $request; } + + private static function getAbsolutePathForFile(string $file): string + { + // If already absolute + if (Director::is_absolute($file)) { + return $file; + } + + // If path is relative to public folder search there first + if (Director::publicDir()) { + $path = Path::join(Director::publicFolder(), $file); + if (file_exists($path ?? '')) { + return $path; + } + } + + // Default to base folder + return Path::join(Director::baseFolder(), $file); + } } diff --git a/src/Control/SimpleResourceURLGenerator.php b/src/Control/SimpleResourceURLGenerator.php index c41f68ef36c..6ad4253008f 100644 --- a/src/Control/SimpleResourceURLGenerator.php +++ b/src/Control/SimpleResourceURLGenerator.php @@ -8,7 +8,7 @@ use SilverStripe\Core\Manifest\ManifestFileFinder; use SilverStripe\Core\Manifest\ModuleResource; use SilverStripe\Core\Manifest\ResourceURLGenerator; -use SilverStripe\Core\Path; +use Symfony\Component\Filesystem\Path; /** * Generate URLs assuming that BASE_PATH is also the webroot @@ -77,7 +77,7 @@ public function urlForResource($relativePath) if (strpos($relativePath ?? '', '?') !== false) { list($relativePath, $query) = explode('?', $relativePath ?? ''); } - + list($exists, $absolutePath, $relativePath) = $this->resolvePublicResource($relativePath); } if (!$exists) { @@ -135,17 +135,8 @@ protected function resolveModuleResource(ModuleResource $resource) $exists = $resource->exists(); $absolutePath = $resource->getPath(); - // Rewrite to _resources with public directory - if (Director::publicDir()) { - // All resources mapped directly to _resources/ - $relativePath = Path::join(RESOURCES_DIR, $relativePath); - } elseif (stripos($relativePath ?? '', ManifestFileFinder::VENDOR_DIR . DIRECTORY_SEPARATOR) === 0) { - // If there is no public folder, map to _resources/ but trim leading vendor/ too (4.0 compat) - $relativePath = Path::join( - RESOURCES_DIR, - substr($relativePath ?? '', strlen(ManifestFileFinder::VENDOR_DIR ?? '')) - ); - } + // All resources mapped directly to public/_resources/ + $relativePath = Path::join(RESOURCES_DIR, $relativePath); return [$exists, $absolutePath, $relativePath]; } @@ -160,13 +151,17 @@ protected function resolveModuleResource(ModuleResource $resource) protected function inferPublicResourceRequired(&$relativePath) { // Normalise path - $relativePath = Path::normalise($relativePath, true); + $relativePath = Path::normalize($relativePath); // Detect public-only request $publicOnly = stripos($relativePath ?? '', 'public' . DIRECTORY_SEPARATOR) === 0; if ($publicOnly) { $relativePath = substr($relativePath ?? '', strlen(Director::publicDir() . DIRECTORY_SEPARATOR)); } + + // Trim slashes + $relativePath = trim($relativePath, '/'); + return $publicOnly; } @@ -181,10 +176,14 @@ protected function resolvePublicResource($relativePath) { // Determine if we should search both public and base resources, or only public $publicOnly = $this->inferPublicResourceRequired($relativePath); + $publicDir = Director::publicFolder(); // Search public folder first, and unless `public/` is prefixed, also private base path - $publicPath = Path::join(Director::publicFolder(), $relativePath); + $publicPath = Path::join($publicDir, $relativePath); if (file_exists($publicPath ?? '')) { + if (!Path::isBasePath($publicDir, $publicPath)) { + throw new InvalidArgumentException("'$relativePath' must not be outside the public root"); + } // String is a literal url committed directly to public folder return [true, $publicPath, $relativePath]; } @@ -194,6 +193,9 @@ protected function resolvePublicResource($relativePath) if (!$publicOnly && file_exists($privatePath ?? '')) { // String is private but exposed to _resources/, so rewrite to the symlinked base $relativePath = Path::join(RESOURCES_DIR, $relativePath); + if (!Path::isBasePath(RESOURCES_DIR, $privatePath)) { + throw new InvalidArgumentException("'$relativePath' must not be outside the resources root"); + } return [true, $privatePath, $relativePath]; } diff --git a/src/Core/Manifest/Module.php b/src/Core/Manifest/Module.php index b335dbc2df5..cfc1d3a7c69 100644 --- a/src/Core/Manifest/Module.php +++ b/src/Core/Manifest/Module.php @@ -4,7 +4,7 @@ use Exception; use InvalidArgumentException; -use SilverStripe\Core\Path; +use Symfony\Component\Filesystem\Path; /** * Abstraction of a PHP Package. Can be used to retrieve information about Silverstripe CMS modules, and other packages @@ -14,17 +14,13 @@ class Module { /** * Full directory path to this module with no trailing slash - * - * @var string */ - protected $path = null; + protected string $path; /** * Base folder of application with no trailing slash - * - * @var string */ - protected $basePath = null; + protected string $basePath; /** * Cache of composer data @@ -46,10 +42,10 @@ class Module * @param string $path Absolute filesystem path to this module * @param string $basePath base path for the application this module is installed in */ - public function __construct($path, $basePath) + public function __construct(string $path, string $basePath) { - $this->path = Path::normalise($path); - $this->basePath = Path::normalise($basePath); + $this->path = rtrim(Path::normalize($path), '/'); + $this->basePath = rtrim(Path::normalize($basePath), '/'); $this->loadComposer(); } @@ -118,7 +114,7 @@ public function getShortName() } // Base name of directory - return basename($this->path ?? ''); + return basename($this->path); } /** @@ -156,7 +152,7 @@ public function getRelativePath() if ($this->path === $this->basePath) { return ''; } - return substr($this->path ?? '', strlen($this->basePath ?? '') + 1); + return substr($this->path, strlen($this->basePath) + 1); } public function __serialize(): array @@ -175,14 +171,14 @@ public function __unserialize(array $data): void $this->composerData = $data['composerData']; $this->resources = []; } - + /** * Activate _config.php for this module, if one exists */ public function activate() { $config = "{$this->path}/_config.php"; - if (file_exists($config ?? '')) { + if (file_exists($config)) { requireFile($config); } } @@ -194,9 +190,9 @@ protected function loadComposer() { // Load composer data $path = "{$this->path}/composer.json"; - if (file_exists($path ?? '')) { - $content = file_get_contents($path ?? ''); - $result = json_decode($content ?? '', true); + if (file_exists($path)) { + $content = file_get_contents($path); + $result = json_decode($content, true); if (json_last_error()) { $errorMessage = json_last_error_msg(); throw new Exception("$path: $errorMessage"); @@ -208,12 +204,11 @@ protected function loadComposer() /** * Get resource for this module * - * @param string $path * @return ModuleResource */ - public function getResource($path) + public function getResource(string $path) { - $path = Path::normalise($path, true); + $path = trim(Path::normalize($path), '/'); if (empty($path)) { throw new InvalidArgumentException('$path is required'); } diff --git a/src/Core/Manifest/ModuleResource.php b/src/Core/Manifest/ModuleResource.php index e89b90ac547..71539e91c9b 100644 --- a/src/Core/Manifest/ModuleResource.php +++ b/src/Core/Manifest/ModuleResource.php @@ -4,7 +4,7 @@ use InvalidArgumentException; use SilverStripe\Core\Injector\Injector; -use SilverStripe\Core\Path; +use Symfony\Component\Filesystem\Path; /** * This object represents a single resource file attached to a module, and can be used @@ -12,17 +12,12 @@ */ class ModuleResource { - /** - * @var Module - */ - protected $module = null; + protected Module $module; /** * Path to this resource relative to the module (no leading slash) - * - * @var string */ - protected $relativePath = null; + protected string $relativePath; /** * Nested resources for this parent resource @@ -31,43 +26,33 @@ class ModuleResource */ protected $resources = []; - /** - * ModuleResource constructor. - * - * @param Module $module - * @param string $relativePath - */ - public function __construct(Module $module, $relativePath) + public function __construct(Module $module, string $relativePath) { $this->module = $module; - $this->relativePath = Path::normalise($relativePath, true); + $this->relativePath = trim(Path::normalize($relativePath), '/'); if (empty($this->relativePath)) { throw new InvalidArgumentException("Resource cannot have empty path"); } } /** - * Return the full filesystem path to this resource. + * Return the full filesystem path to this resource with no trailing slash.. * * Note: In the case that this resource is mapped to the `_resources` folder, this will * return the original rather than the copy / symlink. - * - * @return string Path with no trailing slash E.g. /var/www/module */ - public function getPath() + public function getPath(): string { return Path::join($this->module->getPath(), $this->relativePath); } /** - * Get the path of this resource relative to the base path. + * Get the path of this resource relative to the base path with no trailing or leading slash. * * Note: In the case that this resource is mapped to the `_resources` folder, this will * return the original rather than the copy / symlink. - * - * @return string Relative path (no leading /) */ - public function getRelativePath() + public function getRelativePath(): string { // Root module $parent = $this->module->getRelativePath(); @@ -104,20 +89,16 @@ public function Link() /** * Determine if this resource exists - * - * @return bool */ - public function exists() + public function exists(): bool { return file_exists($this->getPath() ?? ''); } /** * Get relative path - * - * @return string */ - public function __toString() + public function __toString(): string { return $this->getRelativePath(); } @@ -132,12 +113,10 @@ public function getModule() /** * Get nested resource relative to this. - * Note: Doesn't support `..` or `.` relative syntax * - * @param string $path * @return ModuleResource */ - public function getRelativeResource($path) + public function getRelativeResource(string $path) { // Defer to parent module $relativeToModule = Path::join($this->relativePath, $path); diff --git a/src/Core/Path.php b/src/Core/Path.php deleted file mode 100644 index 7d06a1e8516..00000000000 --- a/src/Core/Path.php +++ /dev/null @@ -1,61 +0,0 @@ - 1) { throw new InvalidArgumentException("Invalid theme identifier {$identifier}"); } - return Path::normalise($identifier, true); + return trim(Path::normalize($identifier), '/'); } // If there is no slash / colon it's a legacy theme @@ -158,7 +158,7 @@ public function getPath($identifier) } // Join module with subpath - return Path::normalise($modulePath . $subpath, true); + return trim(Path::normalize($modulePath . $subpath), '/'); } /** @@ -238,7 +238,7 @@ public function findTemplate($template, $themes = null) // Join path $pathParts = [ $this->base, $themePath, 'templates', $head, $type, $tail ]; try { - $path = Path::join($pathParts) . '.ss'; + $path = Path::join(...$pathParts) . '.ss'; if (file_exists($path ?? '')) { $this->getCache()->set($cacheKey, $path); return $path; @@ -328,6 +328,9 @@ public function findThemedResource($resource, $themes = null) foreach ($paths as $themePath) { $relativePath = Path::join($themePath, $resource); $absolutePath = Path::join($this->base, $relativePath); + if (!Path::isBasePath($themePath, $relativePath) || !Path::isBasePath($this->base, $absolutePath)) { + continue; + } if (file_exists($absolutePath ?? '')) { return $relativePath; } diff --git a/src/i18n/Messages/YamlWriter.php b/src/i18n/Messages/YamlWriter.php index dbaf2d07c7a..8d7cc60287d 100644 --- a/src/i18n/Messages/YamlWriter.php +++ b/src/i18n/Messages/YamlWriter.php @@ -2,12 +2,13 @@ namespace SilverStripe\i18n\Messages; +use InvalidArgumentException; use SilverStripe\Assets\Filesystem; -use SilverStripe\Core\Path; use SilverStripe\i18n\i18n; use Symfony\Component\Yaml\Dumper; use SilverStripe\i18n\Messages\Symfony\ModuleYamlLoader; use LogicException; +use Symfony\Component\Filesystem\Path; /** * Write yml files compatible with ModuleYamlLoader @@ -55,6 +56,9 @@ public function write($messages, $locale, $path) // Open the English file and write the Master String Table $langFile = Path::join($langFolder, $locale . '.yml'); + if (!Path::isBasePath($langFolder, $langFile)) { + throw new InvalidArgumentException("Language file must be inside '$langFolder'"); + } if ($fh = fopen($langFile ?? '', "w")) { fwrite($fh, $content ?? ''); fclose($fh); diff --git a/src/i18n/TextCollection/i18nTextCollector.php b/src/i18n/TextCollection/i18nTextCollector.php index 3d3d6fa6f8f..86458e888ce 100644 --- a/src/i18n/TextCollection/i18nTextCollector.php +++ b/src/i18n/TextCollection/i18nTextCollector.php @@ -3,6 +3,7 @@ namespace SilverStripe\i18n\TextCollection; use Exception; +use InvalidArgumentException; use LogicException; use SilverStripe\Core\ClassInfo; use SilverStripe\Core\Config\Config; @@ -12,7 +13,6 @@ use SilverStripe\Core\Manifest\ClassLoader; use SilverStripe\Core\Manifest\Module; use SilverStripe\Core\Manifest\ModuleLoader; -use SilverStripe\Core\Path; use SilverStripe\Dev\Debug; use SilverStripe\Control\Director; use ReflectionClass; @@ -22,6 +22,7 @@ use SilverStripe\i18n\Messages\Reader; use SilverStripe\i18n\Messages\Writer; use SilverStripe\ORM\DataObject; +use Symfony\Component\Filesystem\Path; /** * SilverStripe-variant of the "gettext" tool: @@ -113,9 +114,10 @@ class i18nTextCollector */ public function __construct($locale = null) { - $this->defaultLocale = $locale - ? $locale - : i18n::getData()->langFromLocale(i18n::config()->uninherited('default_locale')); + if (!$locale) { + $locale = i18n::getData()->langFromLocale(i18n::config()->uninherited('default_locale')); + } + $this->setDefaultLocale($locale); $this->basePath = Director::baseFolder(); $this->baseSavePath = Director::baseFolder(); $this->setWarnOnEmptyDefault(i18n::config()->uninherited('missing_default_warning')); @@ -473,8 +475,9 @@ private function getModulesAndThemes(): array } if (!empty($themes)) { foreach ($themes as $theme) { - if (is_dir(Path::join(THEMES_PATH, $theme))) { - $modules[i18nTextCollector::THEME_PREFIX . $theme] = new Module(Path::join(THEMES_PATH, $theme), BASE_PATH); + $themeDir = Path::join(THEMES_PATH, $theme); + if (is_dir($themeDir)) { + $modules[i18nTextCollector::THEME_PREFIX . $theme] = new Module($themeDir, BASE_PATH); } } } @@ -1075,6 +1078,9 @@ public function getDefaultLocale() public function setDefaultLocale($locale) { + if ($locale && str_contains($locale, '..')) { + throw new InvalidArgumentException('Locale must not contain ".."'); + } $this->defaultLocale = $locale; } diff --git a/tests/php/Core/PathTest.php b/tests/php/Core/PathTest.php deleted file mode 100644 index 10dff0e8d98..00000000000 --- a/tests/php/Core/PathTest.php +++ /dev/null @@ -1,122 +0,0 @@ -assertEquals($expected, $joined); - } - - /** - * List of tests for testJoinPaths - * - * @return array - */ - public function providerTestJoinPaths() - { - $tests = [ - // Single arg - [['/'], '/'], - [['\\'], '/'], - [['base'], 'base'], - [['c:/base\\'], 'c:/base'], - // Windows paths - [['c:/', 'bob'], 'c:/bob'], - [['c:/', '\\bob/'], 'c:/bob'], - [['c:\\basedir', '/bob\\'], 'c:/basedir/bob'], - // Empty-ish paths to clear out - [['/root/dir', '/', ' ', 'next/', '\\'], '/root/dir/next'], - [['/', '/', ' ', '/', '\\'], '/'], - [['/', '', '',], '/'], - [['/root', '/', ' ', '/', '\\'], '/root'], - [['', '/root', '/', ' ', '/', '\\'], '/root'], - [['', 'root', '/', ' ', '/', '\\'], 'root'], - [['\\', '', '/root', '/', ' ', '/', '\\'], '/root'], - // join blocks of paths - [['/root/dir', 'another/path\\to/join'], '/root/dir/another/path/to/join'], - // Double dot is fine if it's not attempting directory traversal - [['/root/my..name/', 'another/path\\to/join'], '/root/my..name/another/path/to/join'], - ]; - - // Rewrite tests for other filesystems (output arg only) - if (DIRECTORY_SEPARATOR !== '/') { - foreach ($tests as $index => $test) { - $tests[$index][1] = str_replace('/', DIRECTORY_SEPARATOR, $tests[$index][1] ?? ''); - } - } - return $tests; - } - - /** - * Test that joinPaths give the appropriate error - * - * @dataProvider providerTestJoinPathsErrors - * @param array $args Arguments to pass to Filesystem::joinPath() - * @param string $error Expected path - */ - public function testJoinPathsErrors($args, $error) - { - $this->expectException(\InvalidArgumentException::class); - $this->expectExceptionMessage($error); - Path::join($args); - } - - public function providerTestJoinPathsErrors() - { - return [ - [['/base', '../passwd'], 'Can not collapse relative folders'], - [['/base/../', 'passwd/path'], 'Can not collapse relative folders'], - [['../', 'passwd/path'], 'Can not collapse relative folders'], - [['..', 'passwd/path'], 'Can not collapse relative folders'], - [['base/..', 'passwd/path'], 'Can not collapse relative folders'], - ]; - } - - /** - * @dataProvider providerTestNormalise - * @param string $input - * @param string $expected - */ - public function testNormalise($input, $expected) - { - $output = Path::normalise($input); - $this->assertEquals($expected, $output, "Expected $input to be normalised to $expected"); - } - - public function providerTestNormalise() - { - $tests = [ - // Windows paths - ["c:/bob", "c:/bob"], - ["c://bob", "c:/bob"], - ["/root/dir/", "/root/dir"], - ["/root\\dir\\\\sub/", "/root/dir/sub"], - [" /some/dir/ ", "/some/dir"], - ["", ""], - ["/", ""], - ["\\", ""], - ]; - - // Rewrite tests for other filesystems (output arg only) - if (DIRECTORY_SEPARATOR !== '/') { - foreach ($tests as $index => $test) { - $tests[$index][1] = str_replace('/', DIRECTORY_SEPARATOR, $tests[$index][1] ?? ''); - } - } - return $tests; - } -}