API Remove Path class in favour of Symfony's Path class #11380
Conversation
| public static function getAbsFile($file) | ||
| public static function getAbsFile(string $file): string |
There was a problem hiding this comment.
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 (!Path::isBasePath(Director::baseFolder(), $path)) { | ||
| throw new InvalidArgumentException("'$file' must not be outside the project root"); | ||
| } |
There was a problem hiding this comment.
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.
| // 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 ?? '')) | ||
| ); | ||
| } |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
No traversal protection needed here because is $relativepath comes from module resource, which we should be able to trust.
| if (!Path::isBasePath(RESOURCES_DIR, $privatePath)) { | ||
| throw new InvalidArgumentException("'$relativePath' must not be outside the resources root"); | ||
| } |
There was a problem hiding this comment.
Added traversal protection here because it seems likely for the return value to be directly used to read files at the returned path, and we have no idea where the relative path is coming from.
src/Core/Manifest/Module.php
Outdated
There was a problem hiding this comment.
We don't have any logic that would allow unsafe relative paths to be passed to this class, so no need for path traversal protection.
src/Core/Manifest/ModuleResource.php
Outdated
There was a problem hiding this comment.
We don't have any logic that would allow unsafe relative paths to be passed to this class, so no need for path traversal protection.
src/Core/TempFolder.php
Outdated
There was a problem hiding this comment.
No need for path traversal protection here because there's no way for an attacker to inject unsafe values here without already owning your server
| $substitute = FilesystemPath::canonicalize(FilesystemPath::join($fileUrlDir, $relativePath)); | ||
| $substitute = Path::join($fileUrlDir, $relativePath); |
There was a problem hiding this comment.
join() already does canonicalize() so removed the unnecessary call.
There was a problem hiding this comment.
No need for path traversal protection here because the only call to the old Path::join() is for matching css relative paths which should explicitly allow path traversal (and couldn't be altered by an attacker without owning the server anyway)
| if (!Path::isBasePath($themePath, $relativePath) || !Path::isBasePath($this->base, $absolutePath)) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Path traversal outside of the theme path or base dir means the resource doesn't exist.
Not throwing an exception because this is consistent with what happens if the file just doesn't exist anyway.
GuySartorelli
force-pushed
the
pulls/6/symfony-path
branch
from
cd482e7 to
25a5a86
Compare
| if (!Path::isBasePath($langFolder, $langFile)) { | ||
| throw new InvalidArgumentException("Language file must be inside '$langFolder'"); | ||
| } |
There was a problem hiding this comment.
Added protection here because we don't know where $locale is coming from
| if ($locale && str_contains($locale, '..')) { | ||
| throw new InvalidArgumentException('Locale must not contain ".."'); | ||
| } |
There was a problem hiding this comment.
No locale has .. in its name, so this is easy to do and protects against... well, probably not much because this can't really be run by arbitrary users. But it seems sensible anyway.
GuySartorelli
force-pushed
the
pulls/6/symfony-path
branch
from
25a5a86 to
32c5fb1
Compare
GuySartorelli
force-pushed
the
pulls/6/symfony-path
branch
from
32c5fb1 to
9cd1802
Compare
There was a problem hiding this comment.
I'm not keen on the fact that we could be inadvertently introducing security issues by removing directory traversal protection. This makes these PR's hard to peer review as there's a need to consider all the possible scenarios where we may have introduced a new attack vector.
This is also removing a layer of protection for projects that use the Silverstripe Path class path
I'm not sure that if (!Path::isBasePath(Director::baseFolder(), $path)) { is protection enough against directory traversal as you still access files that you shouldn't be able to
Seems like the main motivation for this work was that both the Silverstripe and Symfony classes were called Path. I think we should simply rename the Path to something like SecurePath and ditch probably ditch the normlize() method off it.
Issue
Pathclass with SymfonyPathclass #11340