Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API Remove Path class in favour of Symfony's Path class #11380

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 26 additions & 18 deletions src/Control/Director.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

namespace SilverStripe\Control;

use InvalidArgumentException;
use SilverStripe\CMS\Model\SiteTree;
use SilverStripe\Control\Middleware\CanonicalURLMiddleware;
use SilverStripe\Control\Middleware\HTTPMiddlewareAware;
Expand All @@ -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.
Expand Down Expand Up @@ -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
Comment on lines -849 to +846
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strong typing added here because the symfony Path class has strong typing - so we can't allow other types to come in anymore.

I've added similar typing changes in other methods across the PRs.

{
// 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");
}
Comment on lines +850 to 852
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added traversal protection here because it seems likely for the return value of Director::getAbsFile() to be directly used to read files at the returned path.


// Default to base folder
return Path::join(Director::baseFolder(), $file);
return $path;
}

/**
Expand Down Expand Up @@ -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);
}
}
35 changes: 20 additions & 15 deletions src/Control/SimpleResourceURLGenerator.php
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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 ?? ''))
);
}
Comment on lines -138 to -148
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed legacy logic for handling missing public folder. That folder was enforced in 5.0.0 so this is dead code.

// All resources mapped directly to public/_resources/
$relativePath = Path::join(RESOURCES_DIR, $relativePath);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No traversal protection needed here because is $relativepath comes from module resource, which we should be able to trust.

return [$exists, $absolutePath, $relativePath];
}

Expand All @@ -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;
}

Expand All @@ -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];
}
Expand All @@ -194,6 +193,12 @@ 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, $relativePath)) {
throw new InvalidArgumentException("'$relativePath' must not be outside the resources root");
}
if (!Path::isBasePath(Director::baseFolder(), $privatePath)) {
throw new InvalidArgumentException("'$privatePath' must not be outside the project root");
}
return [true, $privatePath, $relativePath];
}

Expand Down
35 changes: 15 additions & 20 deletions src/Core/Manifest/Module.php
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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();
}

Expand Down Expand Up @@ -118,7 +114,7 @@ public function getShortName()
}

// Base name of directory
return basename($this->path ?? '');
return basename($this->path);
}

/**
Expand Down Expand Up @@ -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
Expand All @@ -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);
}
}
Expand All @@ -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");
Expand All @@ -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');
}
Expand Down
45 changes: 12 additions & 33 deletions src/Core/Manifest/ModuleResource.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,25 +4,20 @@

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
* as a reference to this to be later turned into either a URL or file path.
*/
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
Expand All @@ -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();
Expand Down Expand Up @@ -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();
}
Expand All @@ -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);
Expand Down
Loading
Loading