Skip to content

fix: add path validation #3272

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.

Sign up for GitHub

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

Open
yenfryherrerafeliz wants to merge 4 commits into
base: master
Choose a base branch
from
Open

fix: add path validation #3272

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ff00554
fix: add path validation
yenfryherrerafeliz Apr 15, 2026
249d4a2
chore: add changelog entry
yenfryherrerafeliz Apr 16, 2026
c423c97
chore: update changelog entry
yenfryherrerafeliz Apr 16, 2026
d9705c8
chore: enforce destination and temp file locations
yenfryherrerafeliz Apr 20, 2026
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
7 changes: 7 additions & 0 deletions .changes/nextrelease/enhance-path-validation.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
[
{
"type": "enhancement",
"category": "S3/S3Transfer",
"description": "Enhance path validation in FileDownloadHandler and ResumableDownload implementation to avoid usage of `..` within the destination path."
}
]
23 changes: 23 additions & 0 deletions src/S3/S3Transfer/Models/ResumableDownload.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'],
Expand Down Expand Up @@ -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'"
);
}
}
}
57 changes: 56 additions & 1 deletion src/S3/S3Transfer/Utils/FileDownloadHandler.php
View file
Open in desktop

@stobrien89 stobrien89 Apr 16, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Since fromFile() knows the resume file's location, I would add a confinement check there that resolves dirname($data['destination']) against dirname($filePath) using realpath() and verify the destination's parent starts with the resume file's parent. The SDK generates the resume file at $destination.s3tmp.XXXX.resume, so they should always be in the same directory tree.

Keep the .. rejection in fromJson() as a fast check. Add the realpath() confinement in fromFile() after deserializing the data and before returning the object. fromJson() can't do confinement because it doesn't have a file path to confine against.

@yenfryherrerafeliz yenfryherrerafeliz Apr 20, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The API supports specifying a custom location for the generated resume file. Should we consider removing support for this parameter so we can add this validation and keep the flow safer?

Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -57,6 +57,18 @@ public function __construct(
$this->fixedPartSize = $fixedPartSize;
$this->handle = null;
$this->transferFailed = false;

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
);
}
}

/**
Expand Down Expand Up @@ -451,4 +463,47 @@ 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'"
);
}
}

/**
* 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."
);
}
}
}
134 changes: 134 additions & 0 deletions tests/S3/S3Transfer/Models/ResumableDownloadTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
90 changes: 90 additions & 0 deletions tests/S3/S3Transfer/Utils/FileDownloadHandlerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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';
Expand All @@ -260,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'
);
}
}