PHP Image Upload and Validation

Question

I have a PHP script that uploads an image file, checks it for size, extension, and type. It then converts it to png, saves it to a folder outside the web root with a randomly generated name, then moves it to a specific folder inside the web root with a new filename that matches the user's username. By default, users have a generic profile image, but if they use the script to change their picture the database is updated to reflect the new filename. So far it's working without any obvious problems, but I would just like to have the script reviewed for possible issues. Also, what changes should be made to make it more secure?

        <div class="formSection">
<?php
if(isset($_FILES["image"]["name"])) {
    $imageFile = ($_FILES["image"]["name"]);
    $imageType = ($_FILES["image"]["type"]);
    $imageSize = ($_FILES["image"]["size"]);
    $validext = array("jpeg","jpg","gif","png");
    $fileExt = pathinfo($imageFile, PATHINFO_EXTENSION);
    $fileExt = strtolower($fileExt);
    $imageErrors = array();

    if($_FILES["image"]["size"] > 1048576) {
        $imageErrors[] = "<strong>ERROR!</strong> File size was too large! It must be under 1MB.";
    }

    if((($imageType == "image/jpeg") || ($imageType == "image/jpg") || ($imageType == "image/gif") || ($imageType == "image/png")) && in_array($fileExt, $validext)) {
    }else{
        $imageErrors[] = "<strong>ERROR!</strong> Not an image file! Only jpg, gif, and png allowed.";
    }

    if($_FILES["image"]["error"]) {
        $imageErrors[] = "<strong>ERROR!</strong> Looks like there was an error. " . $_FILES["image"]["error"];
    }

    if(empty($imageErrors)) {
        if(exif_imagetype($_FILES['image']['tmp_name']) ==  IMAGETYPE_GIF) {
            $newpng = 'gifto.png';
            $png = imagepng(imagecreatefromgif($_FILES['image']['tmp_name']), $newpng);
        }
        elseif(exif_imagetype($_FILES['image']['tmp_name']) ==  IMAGETYPE_JPEG) {
            $newpng = 'jpgto.png';
            $png = imagepng(imagecreatefromjpeg($_FILES['image']['tmp_name']), $newpng);
        }
        elseif(exif_imagetype($_FILES['image']['tmp_name']) == IMAGETYPE_PNG) {
            $newpng = '.png';
            $png = imagepng(imagecreatefrompng($_FILES['image']['tmp_name']), $newpng);
        }
        $temporaryPath = "../../images/" . uniqid() . $newpng;
        $sourcePath = $_FILES["image"]["tmp_name"];
        move_uploaded_file($sourcePath, $temporaryPath);
        $profilePictureMessage = '        <div class="alert alert-success">
          <strong>SUCCESS!</strong> Image file uploaded!
        </div>' . chr(13) . chr(10);
        $finalPath = "images/profilePictures/" . $username . $newpng;
        rename($temporaryPath, $finalPath);

        $query = $con->prepare("UPDATE users SET profilePic = :profilePic WHERE username=:un");
        $query->bindParam(":profilePic", $finalPath);
        $query->bindParam(":un", $username);
        $query->execute();
        unlink($newpng);
    }else{
        foreach($imageErrors as $imageError) {
            $profilePictureMessage = '        <div class="alert alert-danger">'
          .$imageError.
        '</div>' . chr(13) . chr(10);
        }
    }
}
?>
      <div class="message">
<?php echo $profilePictureMessage; ?>
      </div>
      <form action="settings.php" method="POST" enctype="multipart/form-data">
        <span class="title">Change profile picture</span>
        <div class="form-group">
          <input type="file" class="form-control-file" name="image" accept="image/png, image/gif, image/jpeg">
        </div>
        <button type="submit" class="btn btn-primary" name="saveProfilePictureButton">Save</button>
      </form>
    </div>

I welcome any and all feedback, and thanks for your time.

Answer 1

The code is pretty secure, but it could use some cleanup.

Security

Regarding security, you are doing the right thing checking the file extension.

Storing all images as png is a smart move, that should, in theory, prevent uploading PHP code inside files, as it can be done with jpg. But I wouldn't be so sure - sooner or later there will be found a way to keep the PHP code during this transformation too. I wouldn't call it much a problem though, as long as you don't have these files executed, through a web-server of via include. But to be completely 100% sure, consider having a different web server for the images, either your own or a DSN.

Code structure

The biggest flaw i see in this code so far is somewhat surprising. It's its place inside HTML. But in fact, it shouldn't have such a place. Any PHP data processing should be done before starting any output.

Moreover, when you process an HTML form, a successful processing should lead to HTTP redirect, which means there must be no HTML before form processing at all.

Storing the path

Do you really need it? given it's always constant and can be recreated from the user info, why bother storing the image path at all?

What you should really do is always use the row id instead of some other information to address a row in the database. Yes, I mean the username. Everywhere where you are using the username to identify a user, it should be the id.

Testing

Regarding the code, first of all I would like to give you a simple but often overlooked advise.

So far it's working without any obvious problems

Most people consider such a statement after testing only the good scenarios. But the real testing actually begins when you test for the bad scenarios. Hence you should really test not only with good images but with bad uploads as well.

From this point of view I would say that testing $_FILES["image"]["error"] should go first and prevent all other code from the execution as it will produce many unnecessary errors.

The same notion goes for the error echoed out. $_FILES["image"]["error"] is actually just a number and could be of some use only for a sophisticated PHP dev but not for a casual site visitor. Verbal error explanations can be found in the PHP manual so consider translating at least some errors for the user.

Code repetiton

I don't see much point in getting and testing the $imageType - don't you have the same test later on? just add an else condition to your exif_imagetype test.

There is also a lot of repetition inside this condition as well. I would rather create an array with function names and then just call it.

I also don't see much point in moving files around. Why not to create a file in one go?

Minor inconsistencies

they are minor but a good looking code shouldn't have it anyway

• there is no use for braces when you assign a variable like this ($imageFile = $_FILES["image"]["name"];)
• for some reason you assign every $_FILES array member to a distinct variable but tmp_file. Why such a discrimination?
• Avoid "magic numbers". Every hardcoded value should really go into config section, so it could be easily changed in the future or assigned from some configuration setting. This is related to the max upload size and the input name
• HTML formatting inside error messages is actually not a minor issue but a serious design flaw. Add every decoration at the output time.
• inconsistent use of nested functions. imagecreatefromjpeg is nested but strtolower is not. why?
• if with empty body is silly. Add a negation operator instead.
• you can safely write chr(13) . chr(10) as just "\n"

The code

could be something like this

if (isset($_FILES["image"]["name"])) {
    $inputName = "image";
    $imageFile = $_FILES[$inputName]["name"];
    $imageTmp  = $_FILES[$inputName]["tmp_name"];
    $imageSize = $_FILES[$inputName]["size"];
    $imageType = exif_imagetype($imageTmp);
    $fileExt = strtolower(pathinfo($imageFile, PATHINFO_EXTENSION));
    $validext = array("jpeg", "jpg", "gif", "png");
    $maxFileSize = 1024*1024;
    $finalPath = "images/profilePictures/$userid.png";
    $functoions = [
        IMAGETYPE_GIF => 'imagecreatefromgif',
        IMAGETYPE_JPEG => 'imagecreatefromjpeg',
        IMAGETYPE_PNG => 'imagecreatefrompng',
    ];

    if ($_FILES["image"]["error"]) {
        $imageError = translate_file_error($_FILES["image"]["error"]);
    }elseif ($_FILES["image"]["size"] > 1048576) {
        $imageError = "File size is too large! It must be under 1MB.";
    } elseif (!in_array($fileExt, $validext) || !isset($functoions[$imageType])) {
        $imageError = "Not an image file! Only jpg, gif, and png allowed.";
    } else {
        imagepng($functoions[$imageType]($imageTmp), $finalPath);
        header("Location: ".$_SERVER['REQUEST_URI']);
        exit;
    }
}

Answer 2

• Try to keep your processing code separated from your markup printing code. When you mix the two together and obey proper code tabbing, you end up with lots of tabbing which causes "arrowhead code" and excessive line width (horizontal scrolling isn't fun).

• Please read all of the PSR-12 Coding Standards -- especially about control structures. This will help you to write consistent and professional scripts.

• $imageFile, $imageType, and $imageSize don't need their values to be wrapped in parentheses.

• I think you should unconditionally declare a $validImageTypes array which can be used to validate submissions and to populate the accept attribute in your form.

• Your two-point image type check should not have an empty if branch and it should not have so many piped conditions -- there should be just two in_array() calls.

  if (!in_array($imageType, $validImageTypes) || !in_array($fileExt, $validext)) {
      $imageErrors[] = "<strong>ERROR!</strong> Not an image file! Only jpg, gif, and png allowed.";
  }
  

• You are making repeated calls of exif_imagetype($_FILES['image']['tmp_name']), but it would be better to use a switch to avoid redundant calls.