Uploading multiple files to server with PHP and JavaScript

Question

I wrote some code to upload multiple files to a server with PHP and JavaScript. I'm happy with the form and JavaScript, below.

<form method="post" enctype="multipart/form-data">
    <input type="file" name="files[]" multiple>
    <input type="submit" value="Upload File" name="submit">
</form>

<script>
    const url = 'process.php';
    const submit = document.querySelector('[type=submit]');

    submit.addEventListener('click', e => {
        e.preventDefault();

        const files = document.querySelector('[type=file]').files;
        const formData = new FormData();

        for (let i = 0; i < files.length; i++) {
            let file = files[i];

            formData.append('files[]', file);
        }

        fetch(url, {
            method: 'POST',
            body: formData
        }).then(response => {
            console.log(response);
        });
    });
</script>

I would appreciate a review of the PHP for handling the $_POST request. I tried to make it as simple as possible, but I'm not sure if there are any major security concerns I'm overlooking.

<?php 
if ($_SERVER["REQUEST_METHOD"] === 'POST') {
    if (isset($_FILES['files'])) {
        $errors = [];
        $path = 'uploads/';
        $extensions = ['jpg', 'jpeg', 'png', 'gif', 'html', 'txt', 'pdf', 'doc'];

        $allFiles = count($_FILES['files']['tmp_name']);

        for ($i = 0; $i < $allFiles; $i++) {  
            try {
                $file_name = $_FILES['files']['name'][$i];
                $file_tmp = $_FILES['files']['tmp_name'][$i];
                $file_type = $_FILES['files']['type'][$i];
                $file_ext = strtolower(end(explode('.', $_FILES['files']['name'][$i])));

                $file = $path . $file_name;

                if (!in_array($file_ext, $extensions)) {
                    throw new Exception('Extension not allowed: ' . $file_name . ' ' . $file_type . "\n");
                }

                if ($file_size > 2097152) {
                    // 2MB
                    throw new Exception('File size exceeds limit: ' . $file_name . ' ' . $file_type. "\n");
                }
                move_uploaded_file($file_tmp, $file);
            } catch (Exception $e) {
                error_log($e->getMessage(), 3, '/path/to/error_log');
            }
        }
    }
}

Answer 1

The approach

In the beginning of this answer I would like to make a distinct note which is a bit off the actual code review.

You need to change your priorities in writing a code. For the moment you are focused more on the SEO part, trying to provide PHP users with reference answers to common questions. That's a good goal per se, either for the users and for your own good, but it shouldn't overshadow the main purpose of the code - to perform a certain task. In short, you need more practice with using a code you share with other people. I would even say that your intent should be first to solve the actual problem you have at hand and then share your experience, not just decide to write a reference answer out of the blue.

I took a liberty to make this note because it makes the actual impact on the code you write, particularly in regard of the error handling.

Error handling

There are two kinds of errors, "business logic errors" that aren't actually errors but rather regular cases handled by the application, and "real" errors that occur unexpectedly and have to be fixed by a programmer.

Such errors like 'Extension not allowed' or 'File size exceeds limit' are not real errors. That's just business logic-related conditions, whose results have to be conveyed back to the user in order to let them to fix the issue on their side. But you are bluntly catching them and writing into error log where they hardly help anyone. I assume this logic is the direct consequence of your approach. Were you try to use your own code in a real life application, this flawed error handling would become immediately apparent.

So in order fo fix this issue first create a distinct kind of exception

class FileUploadUserException extends Exception {}

it will let you to catch only this particular kind of exception:

       } catch (FileUploadUserException $e) {
            $errors[] = $e->getMessage();
       }

whereas real errors have to be handled elsewhere, either in the dedicated handler or at least a global try catch around the entire application code as it makes no sense to write a repeated error_log call in every distinct chunk of code.

Another approach would be to avoid exceptions at all and use that $errors array you were intended to use at first.

Security

I would remove htm files from the list as it's a clear XSS attack on your own site.

I would also consider not using the original filenames as they would create a real mess with all sorts of weird characters and possible overwrite each other. The simplest solution would be just md5() from the file contents.

Usability

There is one simple trick that could greatly simplify the handling code. Instead of naming your field name as files[], make them enumerated, like files1, files2, ...

it will let you to handle them the most natural way using foreach, eliminating the need of count() call, a counter and even distinct variables to hold certain values from $_FILES array:

$extensions = ['jpg', 'jpeg', 'png', 'gif', 'html', 'txt', 'pdf', 'doc'];

foreach ($_FILES as $file) {
    $file_ext = strtolower(end(explode('.', $file['name'])));
    $destination = $path . $file['name'];
    ...
    move_uploaded_file($file['tmp_name'], $destination);
}

Answer 2

Just wanted to emphazise what @Your Common Sense mentions about the Keep in mind that you have a big validation error in move_uploaded_file (which can be any value a malicious person could want).

A malicious person using crafted input such as '../../init.jpg' could cause the files to end up in different places from where you would expect them.

Handling file upload names

1. Using unique id

Unique id could be generated using uniqid() filename.

$file_name = $_FILES['files']['name'][$i];
$ext = pathinfo($path, PATHINFO_EXTENSION);
$savedFilename = uniqueid() . $ext;

Here you would save $savedFilename in the database.

2. Sanitize using a regex list, and check for existing files

You basically remove everything that's not a-z 0-9 . - _

This does open the doors to problems if you upload a file with the same name twice. You could check if the file exists and then try to look for the filename with a digit behind it, to check if it exists. (this is why the unique id method is a bit easier).

// somewhere before    
$path = 'uploads';

$file_name = $_FILES['files']['name'][$i];
$ext = pathinfo($path, PATHINFO_EXTENSION);
$basename = pathinfo($path, PATHINFO_BASENAME);
$sanitized = preg_replace('/[^a-zA-Z0-9\-\._]/','', $basename);

// add numbers behind the filename until you have a unique filename
$foundFile = false;
$postfix = '';
$increment = 0;
$filename = '';
while ($foundFile === false) {
    $filename = $path . $basename . $postfix . $ext;
    if (file_exists($filename)
    {
         $postfix = $increment++;
    }
    $foundFile = true;
}

move_uploaded_file($tmp_name, $filename);