From ee56e3b165b47a60bbd2d3bd82c62afef139a2db Mon Sep 17 00:00:00 2001 From: Arif Hoque Date: Wed, 9 Sep 2026 23:18:15 +0600 Subject: [PATCH] Fix #4 done: path traversal in Storage --- src/Phaseolies/Support/Storage/FileSystem.php | 49 +++++++++++++-- .../Support/Storage/LocalFileSystem.php | 6 +- .../Support/Storage/PublicFileSystem.php | 8 +-- tests/Storage/FileSystemTest.php | 59 +++++++++++++++++++ tests/Storage/LocalFileSystemTest.php | 41 +++++++++++++ tests/Storage/PublicFileSystemTest.php | 23 ++++++++ 6 files changed, 175 insertions(+), 11 deletions(-) diff --git a/src/Phaseolies/Support/Storage/FileSystem.php b/src/Phaseolies/Support/Storage/FileSystem.php index 0b14ffed..6d8a1617 100644 --- a/src/Phaseolies/Support/Storage/FileSystem.php +++ b/src/Phaseolies/Support/Storage/FileSystem.php @@ -26,6 +26,46 @@ public function __construct(string $filePath, string $fileName = '') $this->fileName = $fileName; } + /** + * Resolve a caller-supplied relative path against the storage root, + * rejecting any path that would escape outside the root + * + * @param string $relativePath + * @return string + * @throws \InvalidArgumentException + */ + protected function resolvePath(string $relativePath): string + { + foreach (preg_split('#[/\\\\]#', $relativePath) as $segment) { + if ($segment === '..' || $segment === '.') { + throw new \InvalidArgumentException( + "Invalid path \"{$relativePath}\": path traversal is not allowed." + ); + } + } + + $root = rtrim($this->filePath, '/\\'); + $fullPath = $root . '/' . ltrim($relativePath, '/\\'); + + $resolvedTarget = realpath($fullPath); + + if ($resolvedTarget !== false) { + $resolvedRoot = realpath($root); + $withinRoot = $resolvedRoot !== false && ( + $resolvedTarget === $resolvedRoot || + str_starts_with($resolvedTarget, $resolvedRoot . DIRECTORY_SEPARATOR) + ); + + if (!$withinRoot) { + throw new \InvalidArgumentException( + "Invalid path \"{$relativePath}\": resolves outside the storage root." + ); + } + } + + return $fullPath; + } + /** * move file in local file system * @@ -117,10 +157,10 @@ private function getStreamSize($stream) */ public function isDirectoryExists(string $path): string { - $realPath = $this->storeageBasePath(); + $fullPath = $this->resolvePath($path); - if (! $this->isDirectory($realPath . '/' . trim($path, '/'))) { - $this->makeDirectory($realPath . '/' . $path, 0755, true); + if (! $this->isDirectory($fullPath)) { + $this->makeDirectory($fullPath, 0755, true); } return $path; } @@ -140,11 +180,12 @@ public function isDirectory(string $directory): bool * create the destination full path according in os * * @param string $path + * @param string $fileName * @return string */ public function destinationFile(string $path, $fileName): string { - return $this->filePath . '/' . $path . '/' . $fileName; + return $this->resolvePath($path . '/' . $fileName); } /** diff --git a/src/Phaseolies/Support/Storage/LocalFileSystem.php b/src/Phaseolies/Support/Storage/LocalFileSystem.php index dcd3c6bf..3c3985f5 100644 --- a/src/Phaseolies/Support/Storage/LocalFileSystem.php +++ b/src/Phaseolies/Support/Storage/LocalFileSystem.php @@ -37,7 +37,7 @@ public function storeageBasePath(): string */ public function get(string $path): ?string { - $fullPath = $this->filePath . '/' . $path; + $fullPath = $this->resolvePath($path); if ($this->isFile($fullPath)) { return $fullPath; @@ -55,7 +55,7 @@ public function get(string $path): ?string */ public function content($path) { - $fullPath = $this->filePath . '/' . $path; + $fullPath = $this->resolvePath($path); if ($this->isFile($fullPath)) { $file = new \SplFileObject($fullPath, 'r'); @@ -80,7 +80,7 @@ public function delete(string|array $path): bool $success = true; foreach ((array) $path as $filePath) { - $fullPath = $this->filePath . '/' . $filePath; + $fullPath = $this->resolvePath($filePath); if (file_exists($fullPath)) { if (!unlink($fullPath)) { diff --git a/src/Phaseolies/Support/Storage/PublicFileSystem.php b/src/Phaseolies/Support/Storage/PublicFileSystem.php index 99c31bf8..6a6cc40b 100644 --- a/src/Phaseolies/Support/Storage/PublicFileSystem.php +++ b/src/Phaseolies/Support/Storage/PublicFileSystem.php @@ -39,7 +39,7 @@ public function storeageBasePath(): string */ public function get(string $path): ?string { - $fullPath = $this->filePath . '/' . $path; + $fullPath = $this->resolvePath($path); if ($this->isFile($fullPath)) { return $fullPath; @@ -58,10 +58,10 @@ public function get(string $path): ?string */ public function content($path) { - $fullPath = $this->filePath . '/' . $path; + $fullPath = $this->resolvePath($path); if ($this->isFile($fullPath)) { - $file = new \SplFileObject($path, 'r'); + $file = new \SplFileObject($fullPath, 'r'); $contents = ''; while (!$file->eof()) { $contents .= $file->fgets(); @@ -83,7 +83,7 @@ public function delete(string|array $path): bool $success = true; foreach ((array) $path as $filePath) { - $fullPath = $this->filePath . '/' . $filePath; + $fullPath = $this->resolvePath($filePath); if (file_exists($fullPath)) { if (!unlink($fullPath)) { diff --git a/tests/Storage/FileSystemTest.php b/tests/Storage/FileSystemTest.php index 05737331..cde83abf 100644 --- a/tests/Storage/FileSystemTest.php +++ b/tests/Storage/FileSystemTest.php @@ -216,4 +216,63 @@ public function testPutWritesUsingConfiguredCustomFileName() $this->assertTrue($customFs->put('uploads', $file)); $this->assertFileExists($this->tmpDir . '/uploads/avatar.jpeg'); } + + // ==================== PATH TRAVERSAL TESTS ==================== + + public function testDestinationFileRejectsPathTraversalInPath() + { + $this->expectException(\InvalidArgumentException::class); + + $this->fs->destinationFile('../../etc', 'passwd'); + } + + public function testDestinationFileRejectsPathTraversalInFileName() + { + $this->expectException(\InvalidArgumentException::class); + + $this->fs->destinationFile('uploads', '../../etc/passwd'); + } + + public function testIsDirectoryExistsRejectsPathTraversal() + { + $this->expectException(\InvalidArgumentException::class); + + $this->fs->isDirectoryExists('../outside'); + } + + public function testDestinationFileRejectsBackslashPathTraversal() + { + // Windows-style separators must be caught too, not just "/". + $this->expectException(\InvalidArgumentException::class); + + $this->fs->destinationFile('uploads', '..\\..\\windows\\win.ini'); + } + + public function testDestinationFileHandlesRootConfiguredWithTrailingBackslash() + { + // Simulates a Windows-style config value like "C:\storage\". + $winStyleFs = new FileSystem(rtrim($this->tmpDir, '/') . '\\'); + + $result = $winStyleFs->destinationFile('uploads', 'image.jpg'); + + $this->assertEquals($this->tmpDir . '/uploads/image.jpg', $result); + } + + public function testResolvePathRejectsSymlinkEscapingRoot() + { + $outside = sys_get_temp_dir() . '/doppar_fs_outside_' . uniqid(); + mkdir($outside, 0755, true); + file_put_contents($outside . '/secret.txt', 'top secret'); + + symlink($outside, $this->tmpDir . '/escape'); + + try { + $this->expectException(\InvalidArgumentException::class); + $this->fs->destinationFile('escape', 'secret.txt'); + } finally { + unlink($this->tmpDir . '/escape'); + unlink($outside . '/secret.txt'); + rmdir($outside); + } + } } diff --git a/tests/Storage/LocalFileSystemTest.php b/tests/Storage/LocalFileSystemTest.php index a650c906..65dcab8f 100644 --- a/tests/Storage/LocalFileSystemTest.php +++ b/tests/Storage/LocalFileSystemTest.php @@ -195,4 +195,45 @@ public function testContentExceptionContainsResolvedFullPath() $this->assertStringContainsString($this->tmpDir . '/' . $missing, $e->getMessage()); } } + + // ==================== PATH TRAVERSAL TESTS ==================== + + public function testGetRejectsPathTraversal() + { + $this->expectException(\InvalidArgumentException::class); + + $this->fs->get('../../etc/passwd'); + } + + public function testContentRejectsPathTraversal() + { + $this->expectException(\InvalidArgumentException::class); + + $this->fs->content('../../etc/passwd'); + } + + public function testDeleteRejectsPathTraversal() + { + $this->expectException(\InvalidArgumentException::class); + + $this->fs->delete('../../etc/passwd'); + } + + public function testDeleteRejectsPathTraversalInAnyArrayEntry() + { + $this->createTempFile('legit.txt'); + + $this->expectException(\InvalidArgumentException::class); + + $this->fs->delete(['legit.txt', '../../etc/passwd']); + } + + public function testGetCannotEscapeRootViaAbsoluteLookingPath() + { + $result = $this->fs->get('/etc/passwd'); + + // Treated as a subpath of the storage root, not an absolute + // filesystem path, so it does not resolve to the real /etc/passwd. + $this->assertNull($result); + } } diff --git a/tests/Storage/PublicFileSystemTest.php b/tests/Storage/PublicFileSystemTest.php index 76c81388..4f768df6 100644 --- a/tests/Storage/PublicFileSystemTest.php +++ b/tests/Storage/PublicFileSystemTest.php @@ -82,4 +82,27 @@ public function testDeleteReturnsFalseWhenFileDoesNotExist(): void $this->assertFalse($result); } + + // ==================== PATH TRAVERSAL TESTS ==================== + + public function testGetRejectsPathTraversal(): void + { + $this->expectException(\InvalidArgumentException::class); + + $this->fs->get('../../etc/passwd'); + } + + public function testContentRejectsPathTraversal(): void + { + $this->expectException(\InvalidArgumentException::class); + + $this->fs->content('../../etc/passwd'); + } + + public function testDeleteRejectsPathTraversal(): void + { + $this->expectException(\InvalidArgumentException::class); + + $this->fs->delete('../../etc/passwd'); + } }