From ff00554f281ef5d21d55eecad33e686ee8907142 Mon Sep 17 00:00:00 2001 From: Yenfry Herrera Feliz Date: Wed, 15 Apr 2026 15:42:31 -0700 Subject: [PATCH 1/4] fix: add path validation --- .../S3Transfer/Models/ResumableDownload.php | 23 +++ .../S3Transfer/Utils/FileDownloadHandler.php | 25 +++- .../Models/ResumableDownloadTest.php | 134 ++++++++++++++++++ .../Utils/FileDownloadHandlerTest.php | 78 ++++++++++ 4 files changed, 259 insertions(+), 1 deletion(-) diff --git a/src/S3/S3Transfer/Models/ResumableDownload.php b/src/S3/S3Transfer/Models/ResumableDownload.php index 90138747d3..a54487caff 100644 --- a/src/S3/S3Transfer/Models/ResumableDownload.php +++ b/src/S3/S3Transfer/Models/ResumableDownload.php @@ -157,6 +157,11 @@ public static function fromJson(string $json): self } } + self::validatePathSafety($data['destination'], 'destination'); + if ($data['temporaryFile'] !== null) { + self::validatePathSafety($data['temporaryFile'], 'temporaryFile'); + } + return new self( $data['resumeFilePath'], $data['requestArgs'], @@ -297,4 +302,22 @@ public function markPartCompleted(int $partNumber): void { $this->partsCompleted[$partNumber] = true; } + + /** + * Validate that a path does not contain directory traversal segments. + * + * @param string $path The path to validate + * @param string $fieldName The field name for error messaging + * + * @throws S3TransferException If path traversal is detected + */ + private static function validatePathSafety(string $path, string $fieldName): void + { + $segments = preg_split('/[\\/\\\\]/', $path); + if (in_array('..', $segments, true)) { + throw new S3TransferException( + "Invalid resume file: path traversal detected in '$fieldName'" + ); + } + } } diff --git a/src/S3/S3Transfer/Utils/FileDownloadHandler.php b/src/S3/S3Transfer/Utils/FileDownloadHandler.php index 13cd4881eb..11c892eca6 100644 --- a/src/S3/S3Transfer/Utils/FileDownloadHandler.php +++ b/src/S3/S3Transfer/Utils/FileDownloadHandler.php @@ -16,7 +16,7 @@ final class FileDownloadHandler extends AbstractDownloadHandler private const MAX_UNIQUE_ID_ATTEMPTS = 100; /** @var string */ - private string $destination; + private readonly string $destination; /** @var bool */ private bool $failsWhenDestinationExists; @@ -57,6 +57,11 @@ public function __construct( $this->fixedPartSize = $fixedPartSize; $this->handle = null; $this->transferFailed = false; + + self::validatePathSafety($this->destination, 'destination'); + if ($this->temporaryFilePath !== null) { + self::validatePathSafety($this->temporaryFilePath, 'temporaryFilePath'); + } } /** @@ -451,4 +456,22 @@ public function getFixedPartSize(): int { return $this->fixedPartSize; } + + /** + * Validates that a path does not contain directory traversal segments. + * + * @param string $path The path to validate + * @param string $fieldName The field name for error messaging + * + * @throws FileDownloadException If path traversal is detected + */ + private static function validatePathSafety(string $path, string $fieldName): void + { + $segments = preg_split('/[\\/\\\\]/', $path); + if (in_array('..', $segments, true)) { + throw new FileDownloadException( + "path traversal detected in '$fieldName'" + ); + } + } } diff --git a/tests/S3/S3Transfer/Models/ResumableDownloadTest.php b/tests/S3/S3Transfer/Models/ResumableDownloadTest.php index 7f13a54950..2ec64c9cc8 100644 --- a/tests/S3/S3Transfer/Models/ResumableDownloadTest.php +++ b/tests/S3/S3Transfer/Models/ResumableDownloadTest.php @@ -337,6 +337,140 @@ public function testIsResumeFileReturnsFalseForNonExistentFile(): void $this->assertFalse(ResumableDownload::isResumeFile('/non/existent.resume')); } + /** + * Preservation: fromJson() with legitimate paths (no '..' segments) must + * continue to return valid ResumableDownload objects. + */ + #[\PHPUnit\Framework\Attributes\DataProvider('legitimateDestinationPathsProvider')] + public function testFromJsonPreservesLegitimateDestinationPaths( + string $destination, + ): void { + $json = $this->buildResumeJson([ + 'destination' => $destination, + ]); + + $result = ResumableDownload::fromJson($json); + + $this->assertEquals($destination, $result->getDestination()); + } + + public static function legitimateDestinationPathsProvider(): array + { + return [ + 'absolute path' => [ + '/home/user/downloads/file.dat', + 'Legitimate absolute path', + ], + 'relative path' => [ + 'downloads/file.dat', + 'Relative path without traversal', + ], + 'dots in filename' => [ + '/tmp/file.v2.0.tar.gz', + 'Dots in filename must NOT be falsely rejected', + ], + 'single-dot segment in path' => [ + '/tmp/./file.dat', + 'Single-dot segment must NOT be falsely rejected', + ], + ]; + } + + /** + * Preservation: fromJson() with null temporaryFile must continue to + * return a valid ResumableDownload object. + */ + public function testFromJsonPreservesNullTemporaryFile(): void + { + $json = json_encode([ + 'version' => '1.0', + 'resumeFilePath' => $this->tempDir . 'download.resume', + 'requestArgs' => ['Bucket' => 'my-bucket', 'Key' => 'my-key'], + 'config' => ['target_part_size_bytes' => 8388608], + 'currentSnapshot' => ['transferred_bytes' => 0, 'total_bytes' => 5000], + 'initialRequestResult' => ['ContentLength' => 5000, 'ETag' => '"abc123"'], + 'partsCompleted' => [], + 'totalNumberOfParts' => 3, + 'temporaryFile' => null, + 'eTag' => '"abc123"', + 'objectSizeInBytes' => 5000, + 'fixedPartSize' => 8388608, + 'destination' => '/tmp/destination.dat', + ]); + + $result = ResumableDownload::fromJson($json); + + $this->assertNull($result->getTemporaryFile()); + } + + /** + * fromJson() with destination containing '..' segments + * should throw S3TransferException with "path traversal" message. + */ + public function testFromJsonThrowsOnDestinationPathTraversal(): void + { + $json = $this->buildResumeJson([ + 'destination' => '/downloads/../../etc/malicious', + ]); + + $this->expectException(S3TransferException::class); + $this->expectExceptionMessage('path traversal'); + ResumableDownload::fromJson($json); + } + + /** + * fromJson() with temporaryFile containing '..' segments + * should throw S3TransferException with "path traversal" message. + */ + public function testFromJsonThrowsOnTemporaryFilePathTraversal(): void + { + $json = $this->buildResumeJson([ + 'temporaryFile' => '/tmp/../../var/www/shell.php', + ]); + + $this->expectException(S3TransferException::class); + $this->expectExceptionMessage('path traversal'); + ResumableDownload::fromJson($json); + } + + /** + * fromJson() with both destination and temporaryFile + * containing '..' segments should throw S3TransferException. + */ + public function testFromJsonThrowsOnBothFieldsPathTraversal(): void + { + $json = $this->buildResumeJson([ + 'destination' => '../../../var/www/shell.php', + 'temporaryFile' => '../../etc/passwd', + ]); + + $this->expectException(S3TransferException::class); + $this->expectExceptionMessage('path traversal'); + ResumableDownload::fromJson($json); + } + + /** + * Build a valid resume JSON string with optional field overrides. + */ + private function buildResumeJson(array $overrides = []): string + { + return json_encode([ + 'version' => '1.0', + 'resumeFilePath' => $overrides['resumeFilePath'] ?? $this->tempDir . 'download.resume', + 'requestArgs' => $overrides['requestArgs'] ?? ['Bucket' => 'my-bucket', 'Key' => 'my-key'], + 'config' => $overrides['config'] ?? ['target_part_size_bytes' => 8388608], + 'currentSnapshot' => $overrides['currentSnapshot'] ?? ['transferred_bytes' => 0, 'total_bytes' => 5000], + 'initialRequestResult' => $overrides['initialRequestResult'] ?? ['ContentLength' => 5000, 'ETag' => '"abc123"'], + 'partsCompleted' => $overrides['partsCompleted'] ?? [], + 'totalNumberOfParts' => $overrides['totalNumberOfParts'] ?? 3, + 'temporaryFile' => $overrides['temporaryFile'] ?? '/tmp/download.tmp', + 'eTag' => $overrides['eTag'] ?? '"abc123"', + 'objectSizeInBytes' => $overrides['objectSizeInBytes'] ?? 5000, + 'fixedPartSize' => $overrides['fixedPartSize'] ?? 8388608, + 'destination' => $overrides['destination'] ?? '/tmp/destination.dat', + ]); + } + private function createResumableDownload(array $overrides = []): ResumableDownload { return new ResumableDownload( diff --git a/tests/S3/S3Transfer/Utils/FileDownloadHandlerTest.php b/tests/S3/S3Transfer/Utils/FileDownloadHandlerTest.php index fb93b89c12..978eff33ae 100644 --- a/tests/S3/S3Transfer/Utils/FileDownloadHandlerTest.php +++ b/tests/S3/S3Transfer/Utils/FileDownloadHandlerTest.php @@ -8,9 +8,11 @@ use Aws\S3\S3Transfer\Utils\FileDownloadHandler; use Aws\Test\TestsUtility; use GuzzleHttp\Psr7\Utils; +use PHPUnit\Framework\Attributes\CoversClass; use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\TestCase; +#[CoversClass(FileDownloadHandler::class)] final class FileDownloadHandlerTest extends TestCase { private string $tempDir; @@ -238,6 +240,82 @@ public function testFailsOnChecksumMismatch(): void $handler->bytesTransferred([AbstractTransferListener::PROGRESS_SNAPSHOT_KEY => $snapshot]); } + /** + * Preservation: FileDownloadHandler constructed with legitimate paths + * (no '..' segments) must succeed without throwing. + */ + #[DataProvider('legitimatePathsProvider')] + public function testConstructorAcceptsLegitimatePaths( + string $destination, + ?string $temporaryFilePath, + string $description + ): void { + $handler = new FileDownloadHandler( + $destination, + false, + false, + $temporaryFilePath + ); + + $this->assertEquals($destination, $handler->getDestination()); + } + + public static function legitimatePathsProvider(): array + { + return [ + 'absolute path' => [ + '/home/user/downloads/file.dat', + null, + 'Legitimate absolute path succeeds', + ], + 'relative path' => [ + 'downloads/file.dat', + null, + 'Relative path succeeds', + ], + 'dots in filename' => [ + '/tmp/file.v2.0.tar.gz', + null, + 'Dots in filename must NOT be falsely rejected', + ], + 'null temporaryFilePath' => [ + '/tmp/destination.dat', + null, + 'Null temporaryFilePath succeeds', + ], + ]; + } + + /** + * FileDownloadHandler with destination containing '..' + * segments should throw FileDownloadException with "path traversal" message. + */ + public function testConstructorThrowsOnDestinationPathTraversal(): void + { + $this->expectException(FileDownloadException::class); + $this->expectExceptionMessage('path traversal'); + new FileDownloadHandler( + '/tmp/../../etc/cron.d/job', + false + ); + } + + /** + * FileDownloadHandler with temporaryFilePath containing + * '..' segments should throw FileDownloadException with "path traversal" message. + */ + public function testConstructorThrowsOnTemporaryFilePathTraversal(): void + { + $this->expectException(FileDownloadException::class); + $this->expectExceptionMessage('path traversal'); + new FileDownloadHandler( + $this->tempDir . 'safe-destination.txt', + false, + true, + '../../var/malicious' + ); + } + public function testCleansUpResourcesAfterFailure(): void { $destination = $this->tempDir . 'file.txt'; From 249d4a21fdd0cba78585043978b8faf931618ddc Mon Sep 17 00:00:00 2001 From: Yenfry Herrera Feliz Date: Thu, 16 Apr 2026 07:27:20 -0700 Subject: [PATCH 2/4] chore: add changelog entry --- .changes/nextrelease/enhance-path-validation.json | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 .changes/nextrelease/enhance-path-validation.json diff --git a/.changes/nextrelease/enhance-path-validation.json b/.changes/nextrelease/enhance-path-validation.json new file mode 100644 index 0000000000..f92237d1bc --- /dev/null +++ b/.changes/nextrelease/enhance-path-validation.json @@ -0,0 +1,7 @@ +[ + { + "type": "api-change", + "category": "", + "description": "Enhance path validation in FileDownloadHandler and ResumableDownload implementation to avoid usage of `..` within the destination path." + } +] \ No newline at end of file From c423c97c522aa4c68e4adae904147dfc99d7a991 Mon Sep 17 00:00:00 2001 From: Yenfry Herrera Feliz Date: Thu, 16 Apr 2026 15:08:29 -0700 Subject: [PATCH 3/4] chore: update changelog entry --- .changes/nextrelease/enhance-path-validation.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.changes/nextrelease/enhance-path-validation.json b/.changes/nextrelease/enhance-path-validation.json index f92237d1bc..1078e1f031 100644 --- a/.changes/nextrelease/enhance-path-validation.json +++ b/.changes/nextrelease/enhance-path-validation.json @@ -1,7 +1,7 @@ [ { - "type": "api-change", - "category": "", + "type": "enhancement", + "category": "S3/S3Transfer", "description": "Enhance path validation in FileDownloadHandler and ResumableDownload implementation to avoid usage of `..` within the destination path." } ] \ No newline at end of file From d9705c8398e30d0d83bd8fe0c4098ef9e2fa606e Mon Sep 17 00:00:00 2001 From: Yenfry Herrera Feliz Date: Mon, 20 Apr 2026 08:37:22 -0700 Subject: [PATCH 4/4] chore: enforce destination and temp file locations --- .../S3Transfer/Utils/FileDownloadHandler.php | 34 ++++++++++++++++++- .../Utils/FileDownloadHandlerTest.php | 12 +++++++ 2 files changed, 45 insertions(+), 1 deletion(-) diff --git a/src/S3/S3Transfer/Utils/FileDownloadHandler.php b/src/S3/S3Transfer/Utils/FileDownloadHandler.php index 11c892eca6..3478e7cec1 100644 --- a/src/S3/S3Transfer/Utils/FileDownloadHandler.php +++ b/src/S3/S3Transfer/Utils/FileDownloadHandler.php @@ -60,7 +60,14 @@ public function __construct( self::validatePathSafety($this->destination, 'destination'); if ($this->temporaryFilePath !== null) { + // Validate temporary path has no directory traversal segments self::validatePathSafety($this->temporaryFilePath, 'temporaryFilePath'); + + // Validate temporary path and destination are both on the same directory + self::validateTemporaryFileIsOnSameDestinationDirectory( + $this->destination, + $this->temporaryFilePath + ); } } @@ -465,7 +472,10 @@ public function getFixedPartSize(): int * * @throws FileDownloadException If path traversal is detected */ - private static function validatePathSafety(string $path, string $fieldName): void + private static function validatePathSafety( + string $path, + string $fieldName + ): void { $segments = preg_split('/[\\/\\\\]/', $path); if (in_array('..', $segments, true)) { @@ -474,4 +484,26 @@ private static function validatePathSafety(string $path, string $fieldName): voi ); } } + + /** + * Validates that destination and temporary path are on the same directory. + * + * @param string $destinationPath + * @param string $temporaryFilePath + * + * @return void + */ + private static function validateTemporaryFileIsOnSameDestinationDirectory( + string $destinationPath, + string $temporaryFilePath + ): void + { + $destinationDir = dirname($destinationPath); + $temporaryFileDir = dirname($temporaryFilePath); + if ($destinationDir !== $temporaryFileDir) { + throw new FileDownloadException( + "The destination and temporary paths must be on same directory." + ); + } + } } diff --git a/tests/S3/S3Transfer/Utils/FileDownloadHandlerTest.php b/tests/S3/S3Transfer/Utils/FileDownloadHandlerTest.php index 978eff33ae..a08801a684 100644 --- a/tests/S3/S3Transfer/Utils/FileDownloadHandlerTest.php +++ b/tests/S3/S3Transfer/Utils/FileDownloadHandlerTest.php @@ -338,4 +338,16 @@ public function testCleansUpResourcesAfterFailure(): void $this->assertFileDoesNotExist($tempFile); } + + public function testConstructorThrowsWhenDestinationAndTemporaryDirsAreNotTheSame(): void + { + $this->expectException(FileDownloadException::class); + $this->expectExceptionMessage('The destination and temporary paths must be on same directory.'); + new FileDownloadHandler( + '/opt/temp/myfile.txt', + false, + false, + '/opt/temp/anotherdir/myfile.s3tmp.12345678' + ); + } }