Secure upload script

Question

I am creating a social network that let's users upload a profile picture. I just want to know if this is a secure way of doing it. Thanks.

<?php

include 'includes/header.php';
include 'includes/form_handlers/settings_handler.php';

//$userPic = ''; 
$date_time = date('Y-m-d_H-i-s');

if(!empty($userLoggedIn)) {

    if (isset($_FILES['fileToUpload'])) {
        $errors= array();
        $file_name = $_FILES['fileToUpload']['name'];
        $file_size = $_FILES['fileToUpload']['size'];
        $width = 1500;
        $height = 1500;
        $file_tmp = $_FILES['fileToUpload']['tmp_name'];
        $file_type = $_FILES['fileToUpload']['type'];
        $tmp = explode('.',$_FILES['fileToUpload']['name']);
        $file_ext=strtolower (end ($tmp));

        $extensions = array( "jpeg", "jpg", "png", "gif");

        if(in_array($file_ext,$extensions)=== false){

            $errors[]="extension not allowed, please choose a JPEG or PNG file.";
        }
 
        if ($file_size > 8097152) {

            $errors[] = 'File size must be 2 MB';
        }
 
        if ($width > 1500 || $height > 1500) {

            echo"File is to large";
        }

        if(!$errors) {

            $userPic = md5($_FILES["fileToUpload"]["name"]) . $date_time . " " . $file_name;
            $profilePic = move_uploaded_file($file_tmp,"assets/images/profile_pics/" . $userPic);

            $file_path = "assets/images/profile_pics/" . $userPic;

            $stmt = $con->prepare("UPDATE users SET profile_pic = ? WHERE username = ?");
            $stmt->bind_param('ss', $file_path, $username);
            $stmt->execute();
            $stmt->close();

            header('Location: settings.php');
            exit();
 
        }
    }
} else {

    echo "Invalid Username";
}

?>

Answer 1

This is my personal opinion, but I'd say the following:

1. The code should be formatted, I'd personally look at PSR-12 as this standard should be followed when possible.
2. move_uploaded_file doesn't protect against directory traversal. You should use basename on $_FILES['fileToUpload']['tmp_name'] and some other forms of validation
3. Checking the file extension with if(in_array($file_ext,$extensions)=== false) doesn't prevent a user from uploading a malicious file they could for instance use a magic byte to trick the server into thinking it's a certain type of file. You should take a look at finfo and the first example on file upload
4. You're create an array of errors, currently that's being checked in an if statement and is then thrown away. If you aren't planning on using it you might be better just returning out of the function early rather than continuing execution.
5. Depending on how unique the filename should be you might want to use something like uniqid(mt_rand(), true)
6. move_uploaded_file will replace a file if it already exists, you might want to check that this exists before you overwrite an existing file. Depending on your naming solution it's very unlikely to occur but under high load for long periods of time this could happen more often than you think.
7. You're using UPDATE users SET profile_pic = ? WHERE username = ? I'd assume that this value exists in the database as the user needs to be logged in. However, if you aren't sure if the field exists or not (I haven't seen the database) I'd personally use: INSERT INTO users (profile_pic, username) VALUES (?,?) ON DUPLICATE KEY UPDATE profile_pic=?, username=? this will insert into the table if the row doesn't exist but will update it if it does.
8. You've set a local variable called width and height and are comparing them to the same value. I assume this was meant to check the actual file dimensions?

I hope this helps in some way :)