Skip to content
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

ensure error when deleting entire storage or deleting directory by delete method instead of deleteDirectory #1737

Closed
wants to merge 11 commits into from
Closed
2 changes: 1 addition & 1 deletion composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
"google/cloud-storage": "^1.23",
"async-aws/s3": "^1.5 || ^2.0",
"async-aws/simple-s3": "^1.1 || ^2.0",
"sabre/dav": "^4.3.1"
"sabre/dav": "^4.6.0"
},
"conflict": {
"async-aws/core": "<1.19.0",
Expand Down
47 changes: 47 additions & 0 deletions src/AdapterTestUtilities/FilesystemAdapterTestCase.php
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
use League\Flysystem\FileAttributes;
use League\Flysystem\FilesystemAdapter;
use League\Flysystem\StorageAttributes;
use League\Flysystem\UnableToDeleteFile;
use League\Flysystem\UnableToMoveFile;
use League\Flysystem\UnableToProvideChecksum;
use League\Flysystem\UnableToReadFile;
Expand Down Expand Up @@ -871,4 +872,50 @@ public function cannot_get_checksum_for_directory(): void

$adapter->checksum('dir', new Config());
}

/**
* @test
*/
public function cannot_delete_directory_over_delete(): void
{
$this->runScenario(function () {
$adapter = $this->adapter();

$adapter->write(
'test/text.txt',
'contents',
new Config()
);

$this->assertTrue($adapter->fileExists('test/text.txt'));

$this->expectException(UnableToDeleteFile::class);
$adapter->delete('test/');

$this->assertTrue($adapter->fileExists('test/text.txt'));
});
}

/**
* @test
*/
public function cannot_delete_with_empty_path(): void
{
$this->runScenario(function () {
$adapter = $this->adapter();

$adapter->write(
'test/text.txt',
'contents',
new Config()
);

$this->assertTrue($adapter->fileExists('test/text.txt'));

$this->expectException(UnableToDeleteFile::class);
$adapter->delete('');

$this->assertTrue($adapter->fileExists('test/text.txt'));
});
}
}
38 changes: 22 additions & 16 deletions src/Ftp/FtpAdapter.php
Original file line number Diff line number Diff line change
Expand Up @@ -213,10 +213,25 @@ public function delete(string $path): void
*/
private function deleteFile(string $path, $connection): void
{
if (empty($path)) {
throw UnableToDeleteFile::atLocation($path);
}

$fileExists = $this->fileExists($path);

if ($fileExists === false) {
if ($this->directoryExists($path)) {
throw UnableToDeleteFile::atLocation($path);
}

return;
}

$location = $this->prefixer()->prefixPath($path);

$success = @ftp_delete($connection, $location);

if ($success === false && ftp_size($connection, $location) !== -1) {
if ($success === false && $this->fileExists($path)) {
throw UnableToDeleteFile::atLocation($path, 'the file still exists');
}
}
Expand Down Expand Up @@ -275,7 +290,7 @@ private function fetchMetadata(string $path, string $type): FileAttributes

$object = @ftp_raw($this->connection(), 'STAT ' . $location);

if (empty($object) || count($object) < 3 || substr($object[1], 0, 5) === "ftpd:") {
if (empty($object) || count($object) < 3 || str_starts_with($object[1], "ftpd:")) {
throw UnableToRetrieveMetadata::create($path, $type, error_get_last()['message'] ?? '');
}

Expand Down Expand Up @@ -450,7 +465,7 @@ private function normalizeUnixObject(string $item, string $base): StorageAttribu

private function listingItemIsDirectory(string $permissions): bool
{
return substr($permissions, 0, 1) === 'd';
return str_starts_with($permissions, 'd');
}

private function normalizeUnixTimestamp(string $month, string $day, string $timeOrYear): int
Expand All @@ -459,14 +474,12 @@ private function normalizeUnixTimestamp(string $month, string $day, string $time
$year = $timeOrYear;
$hour = '00';
$minute = '00';
$seconds = '00';
} else {
$year = date('Y');
[$hour, $minute] = explode(':', $timeOrYear);
$seconds = '00';
}

$dateTime = DateTime::createFromFormat('Y-M-j-G:i:s', "{$year}-{$month}-{$day}-{$hour}:{$minute}:{$seconds}");
$dateTime = DateTime::createFromFormat('Y-M-j-G:i:s', "{$year}-{$month}-{$day}-{$hour}:{$minute}:00");

return $dateTime->getTimestamp();
}
Expand All @@ -484,19 +497,14 @@ private function normalizePermissions(string $permissions): int
$parts = str_split($permissions, 3);

// convert the groups
$mapper = function ($part) {
$mapper = static function ($part) {
return array_sum(str_split($part));
};

// converts to decimal number
return octdec(implode('', array_map($mapper, $parts)));
}

/**
* @inheritdoc
*
* @param string $directory
*/
private function listDirectoryContentsRecursive(string $directory): Generator
{
$location = $this->prefixer()->prefixPath($directory);
Expand Down Expand Up @@ -583,9 +591,6 @@ private function ensureParentDirectoryExists(string $path, ?string $visibility):
$this->ensureDirectoryExists($dirname, $visibility);
}

/**
* @param string $dirname
*/
private function ensureDirectoryExists(string $dirname, ?string $visibility): void
{
$connection = $this->connection();
Expand Down Expand Up @@ -634,9 +639,10 @@ private function hasFtpConnection(): bool

public function directoryExists(string $path): bool
{
$location = $this->prefixer()->prefixPath($path);
$connection = $this->connection();

return @ftp_chdir($connection, $path) === true;
return @ftp_chdir($connection, $location) === true;
}

/**
Expand Down
34 changes: 19 additions & 15 deletions src/InMemory/InMemoryFilesystemAdapter.php
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
use League\Flysystem\FileAttributes;
use League\Flysystem\FilesystemAdapter;
use League\Flysystem\UnableToCopyFile;
use League\Flysystem\UnableToDeleteFile;
use League\Flysystem\UnableToMoveFile;
use League\Flysystem\UnableToReadFile;
use League\Flysystem\UnableToRetrieveMetadata;
Expand All @@ -19,11 +20,10 @@

use function array_keys;
use function rtrim;
use function strpos;

class InMemoryFilesystemAdapter implements FilesystemAdapter
{
const DUMMY_FILE_FOR_FORCED_LISTING_IN_FLYSYSTEM_TEST = '______DUMMY_FILE_FOR_FORCED_LISTING_IN_FLYSYSTEM_TEST';
public const DUMMY_FILE_FOR_FORCED_LISTING_IN_FLYSYSTEM_TEST = '______DUMMY_FILE_FOR_FORCED_LISTING_IN_FLYSYSTEM_TEST';

/**
* @var InMemoryFile[]
Expand Down Expand Up @@ -82,17 +82,21 @@ public function readStream(string $path)

public function delete(string $path): void
{
if (empty($path) || $this->directoryExists($path)) {
throw UnableToDeleteFile::atLocation($path);
}

unset($this->files[$this->preparePath($path)]);
}

public function deleteDirectory(string $prefix): void
public function deleteDirectory(string $path): void
{
$prefix = $this->preparePath($prefix);
$prefix = $this->preparePath($path);
$prefix = rtrim($prefix, '/') . '/';

foreach (array_keys($this->files) as $path) {
if (strpos($path, $prefix) === 0) {
unset($this->files[$path]);
foreach (array_keys($this->files) as $filePath) {
if (str_starts_with($filePath, $prefix)) {
unset($this->files[$filePath]);
}
}
}
Expand All @@ -108,8 +112,8 @@ public function directoryExists(string $path): bool
$prefix = $this->preparePath($path);
$prefix = rtrim($prefix, '/') . '/';

foreach (array_keys($this->files) as $path) {
if (strpos($path, $prefix) === 0) {
foreach (array_keys($this->files) as $filePath) {
if (str_starts_with($filePath, $prefix)) {
return true;
}
}
Expand Down Expand Up @@ -184,9 +188,9 @@ public function listContents(string $path, bool $deep): iterable
$prefixLength = strlen($prefix);
$listedDirectories = [];

foreach ($this->files as $path => $file) {
if (substr($path, 0, $prefixLength) === $prefix) {
$subPath = substr($path, $prefixLength);
foreach ($this->files as $filePath => $file) {
if (str_starts_with($filePath, $prefix)) {
$subPath = substr($filePath, $prefixLength);
$dirname = dirname($subPath);

if ($dirname !== '.') {
Expand All @@ -208,12 +212,12 @@ public function listContents(string $path, bool $deep): iterable
}

$dummyFilename = self::DUMMY_FILE_FOR_FORCED_LISTING_IN_FLYSYSTEM_TEST;
if (substr($path, -strlen($dummyFilename)) === $dummyFilename) {
if (str_ends_with($filePath, $dummyFilename)) {
continue;
}

if ($deep === true || strpos($subPath, '/') === false) {
yield new FileAttributes(ltrim($path, '/'), $file->fileSize(), $file->visibility(), $file->lastModified(), $file->mimeType());
if ($deep === true || ! str_contains($subPath, '/')) {
yield new FileAttributes(ltrim($filePath, '/'), $file->fileSize(), $file->visibility(), $file->lastModified(), $file->mimeType());
}
}
}
Expand Down
18 changes: 17 additions & 1 deletion src/PhpseclibV3/SftpAdapter.php
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
use League\Flysystem\UnableToCheckFileExistence;
use League\Flysystem\UnableToCopyFile;
use League\Flysystem\UnableToCreateDirectory;
use League\Flysystem\UnableToDeleteFile;
use League\Flysystem\UnableToMoveFile;
use League\Flysystem\UnableToReadFile;
use League\Flysystem\UnableToRetrieveMetadata;
Expand Down Expand Up @@ -176,9 +177,24 @@ public function readStream(string $path)

public function delete(string $path): void
{
if (empty($path)) {
throw UnableToDeleteFile::atLocation($path);
}

$fileExists = $this->fileExists($path);

if ($fileExists === false) {
if ($this->directoryExists($path)) {
throw UnableToDeleteFile::atLocation($path);
}

return;
}

$location = $this->prefixer->prefixPath($path);
$connection = $this->connectionProvider->provideConnection();
$connection->delete($location);

$connection->delete($location, false);
}

public function deleteDirectory(string $path): void
Expand Down
5 changes: 4 additions & 1 deletion src/WebDAV/UrlPrefixingClientStub.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,10 @@

class UrlPrefixingClientStub extends Client
{
public function propFind($url, array $properties, $depth = 0)
/**
* @param string $url
*/
public function propFind($url, array $properties, $depth = 0): array
{
$response = parent::propFind($url, $properties, $depth);

Expand Down
10 changes: 10 additions & 0 deletions src/WebDAV/WebDAVAdapter.php
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,16 @@ public function readStream(string $path)

public function delete(string $path): void
{
$fileExists = $this->fileExists($path);

if ($fileExists === false) {
if ($this->directoryExists($path)) {
throw UnableToDeleteFile::atLocation($path);
}

return;
}

$location = $this->encodePath($this->prefixer->prefixPath($path));

try {
Expand Down
2 changes: 1 addition & 1 deletion src/WebDAV/composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
"require": {
"php": "^8.0.2",
"league/flysystem": "^3.6.0",
"sabre/dav": "^4.3.1"
"sabre/dav": "^4.6.0"
},
"license": "MIT",
"authors": [
Expand Down
16 changes: 15 additions & 1 deletion src/ZipArchive/ZipArchiveAdapter.php
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,20 @@ public function readStream(string $path)

public function delete(string $path): void
{
if (empty($path) || \str_ends_with($path, '/')) {
throw UnableToDeleteFile::atLocation($path);
}

$fileExists = $this->fileExists($path);

if ($fileExists === false) {
if ($this->directoryExists($path)) {
throw UnableToDeleteFile::atLocation($path);
}

return;
}

$prefixedPath = $this->pathPrefixer->prefixPath($path);
$zipArchive = $this->zipArchiveProvider->createZipArchive();
$success = $zipArchive->locateName($prefixedPath) === false || $zipArchive->deleteName($prefixedPath);
Expand All @@ -155,7 +169,7 @@ public function deleteDirectory(string $path): void

$itemPath = $stats['name'];

if (strpos($itemPath, $prefixedPath) !== 0) {
if ( ! str_starts_with($itemPath, $prefixedPath)) {
continue;
}

Expand Down
Loading