Secure file upload for PDF only

Question

I am trying to create a secure file upload using PHP 7+ where I only allow PDF files. I found a lot of posts on this topic on different websites but couldn't find a complete solution that ensures that no harmful files can be uploaded this way.

So far I have the following code. Can someone tell me if I am missing any important steps here or if anything should be changed or removed in my code?

(Note: I am not interested in the old x-pdf file types.)

<?php
    include 'session.php';
    include 'header.php';

    if (empty($_FILES['files'])) {
        echo json_encode(['error'=>'No files found for upload.']); 
        return;
    }

    if(!empty($_POST['csrfToken'])) {
        if(hash_equals($_SESSION['csrfToken'], $_POST['csrfToken'])) {
            $postData = $_POST;
            $files = $_FILES['files'];
            $uploadRef = preg_replace('/[^A-Za-z0-9]/', '', $_GET['uploadRef']);
            $categoryId = preg_replace('/[^A-Za-z0-9]/', '', $_GET['categoryId']);
            $tags = preg_replace('/[^A-Za-z0-9,]/', '', $_GET['tagsList']);
            $success = null;

            $paths= [];
            $filenames = $files['name'];

            for($i=0; $i < count($filenames); $i++){
                if($_FILES['file']['error'] !== UPLOAD_ERR_OK) {
                    die('Upload failed with error ' . $_FILES['file']['error']);
                }
                
                $fileTitle = $files['name'][$i];
                $fileTitle = substr($fileTitle, 0 , (strrpos($fileTitle, ".")));
                $fileExtensions = explode('.', basename($filenames[$i]));
                $fileExtension = strtolower(array_pop($fileExtensions));
                $ok = false;
                switch($fileExtension) {
                   case 'pdf':
                        $ok = true;
                   default:
                       die('Unknown/not permitted file type');
                }
                
                $finfo = finfo_open(FILEINFO_MIME_TYPE);
                $mime = finfo_file($finfo, $_FILES['file']['tmp_name']);
                $ok = false;
                switch($mime) {
                   case 'application/pdf':
                        $ok = true;
                   default:
                       die('Unknown/not permitted file type');
                }
                
                $uploadId = md5(uniqid()) . '_' . $i;
                $target = 'uploads' . DIRECTORY_SEPARATOR . $uploadId . '.' . $fileExtension;
                if(move_uploaded_file($files['tmp_name'][$i], $target)) {
                    $success = true;
                    $paths[] = $target;
                    
                    $conn = new mysqli($dbHost, $dbUser, $dbPw, $dbName);
                    if($conn->connect_error) {
                        exit($trans['errorConnectionFailedTxt'][$lang]);
                    }
                    mysqli_report(MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT);
                    $conn->set_charset('utf8mb4');
                        
                    $stmt = $conn->prepare("INSERT INTO uploads (uploadId, uploadRef, categoryId, tags, fileTitle, fileExtension) VALUES (?, ?, ?, ?, ?, ?)");
                    $stmt->bind_param("ssssss", $uploadId, $uploadRef, $categoryId, $tags, $fileTitle, $fileExtension);  
                    $stmt->execute();
                    
                    $stmt->close();
                    $conn->close();     
                } else {
                    $success = false;
                    break;
                }
            }

            if ($success === true) {
                $output = [];
            } elseif ($success === false) {
                $output = ['error'=>'Error while uploading images. Contact the system administrator'];
                foreach ($paths as $file) {
                    unlink($file);
                }
            } else {
                $output = ['error'=>'No files were processed.'];
            }

            unset($postData);
            
            echo json_encode($output);
        } else {
             echo json_encode('invalid CSRF token');
        }
    } else {
         echo json_encode('no CSRF token');
    }
?>

Answer 1

Guard statements

if(!empty($_POST['csrfToken'])) {
    if(hash_equals($_SESSION['csrfToken'], $_POST['csrfToken'])) {

I think that this should be performed in a separate method that validates tokens. Furthermore, I think that this should end in an HTTP error (as indicated by the other answer).

Above is called a "guard statement", it checks if the state or parameter is valid before the method is executed. If it isn't valid, it should terminate it. Termination can be performed locally, you definitely don't want to have to scroll through the method to find:

    } else {
         echo json_encode('invalid CSRF token');
    }
} else {
     echo json_encode('no CSRF token');
}

at the end. Furthermore, if you simply exit the method you remove a layer of indentation making the method less complex to read.

Unclear regular expressions

$uploadRef = preg_replace('/[^A-Za-z0-9]/', '', $_GET['uploadRef']);

This should also be a method, not so much because the regular expression is hard to understand (the "what"), but it isn't clear why it is performed.

Success?

 $success = null;

Success is a boolean, it should not be used as a variable with three values. Use either two variables or an enumeration. Furthermore, $success is a terrible bad name, try filesUploaded or similar.

If we'd check that count($filenames) is zero beforehand we can simply set the output beforehand and skip the rest of the execution (remember the guard statements). Programming is all about limiting complexity.

Recovering C programmer

 $ok = false;

Not required if you die anyway, right?

Not so unique

 uniqid()

"This function does not guarantee uniqueness of return value."¹ Um, right. This is just waiting to fail horribly, whatever you do with it. Using md5() on it will not accomplish anything. Extension with a counter will help unless the same folder is used by parallel processes (is that possible?).

The sequel

The SQL statements should be in a separate method, e.g. createFileUploadReport. If you make a separate class you can even use different ways of reporting, e.g. reporting to console or log file instead, so you can test your method without SQL server present.

Answer 2

Bug / Logic error

The two switch statements have one case plus the default case. In both switch statements the default case is always executed. Perhaps a better understanding of how switch statements work would be useful. Also, for a single case it doesn't make sense to use a switch statement - a simple if statement would suffice.

Inconsistent return types

In some cases an array is passed to json_encode() and used with an echo statement, yet in other cases die() is called. The json encoded arrays would have me believe this script is used in conjunction with an asynchronous loading mechanism (e.g. AJAX), but that would likely be thrown off if die() or exit() is used, unless it looks for both arrays and simple strings.

Then apparently when the upload is successful there is this code:

if ($success === true) {
     $output = [];

That doesn't seem to be very helpful for the front-end code.

It would also be wise to use HTTP response codes - e.g. 200 to indicate success, 422 for invalid input, etc.

returning early

When the first condition, i.e. if (empty($_FILES['files'])) { evaluates to true then the JSON response is returned with the appropriate error message. The other conditions that lead to error messages should follow the same fashion e.g. empty value in $_POST['csrfToken'], etc. Doing so will decrease the amount of indentation for the rest of the code. For more information on this topic see this presentation about cleaning up code where Rafael Dohms talks about many ways to keep code lean - like avoiding the else keyword (see the slides here).

Because it doesn’t depend on the loop iterator variable, this block can be moved above the for loop:

if($_FILES['file']['error'] !== UPLOAD_ERR_OK) {
    die('Upload failed with error ' . $_FILES['file']['error']);
}

checking MIME type

Instead of calling finfo_open(FILEINFO_MIME_TYPE) just to get the mime-type, the mime-content-type() function could be used.

$mime = mime-content-type($_FILES['file']['tmp_name'])

Additionally, the mime-type may be provided from the browser in $_FILES['files']['type'][$i] though it "not checked on the PHP side and therefore don't take its value for granted."¹

variable $postData - really needed?

After postData is assigned, it only appears to be used in one spot- passed to unset(). It hardly seems necessary...

variadic field name checks

While the form submitted to this script is not included one can only guess as to the fields. The script checks both $_FILES['files'] as well as $_FILES['file'] - while the former most likely allows multiple files to be uploaded, are there really two different file input fields?

database query in loop

The code currently inserts records into the database on each iteration of the loop. Consider using only one statement to insert all records. This will minimize database connections (which can increase execution time) as well as prevent invalid data from being inserted (e.g. if first file was valid but subsequent files were not).

regular expressions to replace characters

The regular expressions could perhaps be simplified using the character type \w though that includes the underscore characters i.e. _ but must those be removed? Also not that preg_replace() won't safely clean multi-byte strings so consider that if unicode strings need to be supported.