0
\$\begingroup\$

I made this function to delete a directory with all its contents recursively, will it work as expected in a production environment ? is it safe ? I don't want to wake up one day with /home contents is gone :D

public static function delTree($dir) {
    if(!is_dir($dir)){return false;};

    $files = scandir($dir);if(!$files){return false;}
    $files = array_diff($files, array('.','..'));

    foreach ($files as $file) {
        (is_dir("$dir/$file")) ? SELF::delTree("$dir/$file") : unlink("$dir/$file");
    }

    return rmdir($dir);
}

Note: I use this function internally, meaning there are no client parameters like directory names is taken from the client before I call it, so there is no chance for traversal attacks, and I check the base path with another function before I call it, for example to delete a client folder I do something like this

$clientsFolderPath = $_SERVER['DOCUMENT_ROOT'] . "/../clients"
$clientFolderPath = "$clientsFolderPath/$clientId";

$realBase = realpath($clientsFolderPath);
$realClientDir = realpath($clientFolderPath);
if ( !$realBase || !$realClientDir || strpos($realClientDir, $realBase) !== 0 ){
    //error, log , and exit;
} else {
    ExtendedSystemModel::delTree($clientFolderPath);
}
\$\endgroup\$
  • \$\begingroup\$ It seems you have already found stackoverflow.com/q/3349753/2943403 There is plenty of discussion about security / safety concerns and performance. \$\endgroup\$ – mickmackusa Apr 22 at 6:54
  • \$\begingroup\$ @mickmackusa oh, I was having this function in my files for 3 years, and now I wanted to use it in a production environment, I thought I wrote it in the past, but it looks like I took it exactly from here and forgot. Sorry for that, 3 years in coding should had impact my memory :) . Any way I don't know should I delete the question or what, if it got reviews, that still can help me as I'm using it now. \$\endgroup\$ – Accountant م Apr 22 at 7:34
1
\$\begingroup\$

As indicated by the scattered comments on https://stackoverflow.com/q/3349753/2943403, your approach is trustworthy.

scandir() has an advantage over glob() (which is normally handy when trying to ignore . and ..) because glob() will not detect hidden files.

The RecursiveIterator methods are powerful, but it is my opinion that fewer developers possess the ability to instantaneously comprehend all of the calls and flags (and I believe that should weigh in on your decision).

As for your snippet, I would like to clean it up a little.

public static function delTree($dir) {
    if (!is_dir($dir)) {
        return false;
    }

    $files = scandir($dir);
    if (!$files) {
        return false;
    }
    $files = array_diff($files, ['.', '..']);

    foreach ($files as $file) {
        if (is_dir("$dir/$file")) {
            SELF::delTree("$dir/$file");
        } else {
            unlink("$dir/$file");
        }
    }

    return rmdir($dir);
}

I don't use ternary operators when I am not assigning something in that line. For this reason, a classic if-else is cleaner in my opinion.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.