-
Notifications
You must be signed in to change notification settings - Fork 824
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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"); | ||
} | ||
Comment on lines
+850
to
852
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
|
||
// 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); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 ?? '')) | ||
); | ||
} | ||
Comment on lines
-138
to
-148
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed legacy logic for handling missing |
||
// All resources mapped directly to public/_resources/ | ||
$relativePath = Path::join(RESOURCES_DIR, $relativePath); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No traversal protection needed here because is |
||
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,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]; | ||
} | ||
|
||
|
There was a problem hiding this comment.
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.