2
\$\begingroup\$

I'm trying to develop some templates for common PHP tasks I've been dealing with. One of which is a general image file upload handler.

So far I'm using the following reusable code which seems to be working fine without any noticeable bug:

<?php

    if ( !isset($_POST['submit']) ) {
        goto page_content;}

    if ( $_FILES['file_upload']['error']===4 ) {
        echo 'No file uploaded';
        goto page_content;}

    if ( $_FILES['file_upload']['error']===1 || $_FILES['file_upload']['error']===2 ) {
        echo 'File exceeds maximum size limit';
        goto page_content;}

    if ( $_FILES['file_upload']['error']!==0 ) {
        echo 'Failed to upload the file';
        goto page_content;}

    if ( !is_uploaded_file($_FILES['file_upload']['tmp_name']) ) {
        echo 'Failed to upload the file';
        goto page_content;}

    require_once('imageResize.php');
    $err = imageResize($_FILES['file_upload']['tmp_name'], 'random.png' );
    if ( $err !== 0 ) {
        echo 'Invalid image format';
        goto page_content;}

    echo 'Image uploaded successfully';

    page_content:
?>
<form action="filename.php" method="POST" enctype="multipart/form-data">

    <input type="hidden" name="MAX_FILE_SIZE" value="1000000">
    <input type="file" name="file_upload" accept="image/*">
    <input type="submit" name="submit">

</form>

Additional file imageResize.php:

<?php
    // image resize
    function imageResize($source, $target){

        $size = getimagesize($source);
        if ($size === false) {return 1;} // invalid image format

        $sourceImg = @imagecreatefromstring(@file_get_contents($source));
        if ($sourceImg === false) {return 2;} //invalid image format

        $width = imagesx($sourceImg);
        $height = imagesy($sourceImg);
        $sidelenght = min($width,$height);
        $targetImg = imagecreatetruecolor(100, 100);
        imagecopyresampled($targetImg, $sourceImg, 0, 0, ($width-$sidelenght)/2, ($height-$sidelenght)/2, 100, 100, $sidelenght, $sidelenght);
        imagedestroy($sourceImg);
        imagepng($targetImg, $target);
        imagedestroy($targetImg);

        return 0;           
    }
?>

Some main characteristics of this code are:

  • provides messages for the most common errors that can happened during the upload process
  • it allows the client to upload an image file up to 1Mb size
  • resizes all images to a standard 100x100 px size
  • save all images to a standard PNG format

Questions

  1. Is this code safe? Or are there any vulnerability that could be exploited by an malicious client? In this case, how can I solve it?
  2. To avoid several nested if-then-else conditions (which can become hard to read), I'm currently using goto (which can become a bad control structure practice). Is there a better alternative?
\$\endgroup\$
  • \$\begingroup\$ Following others suggestion, this is a repost from StackOverflow \$\endgroup\$ – Mark Messa Jul 4 '18 at 17:30
2
\$\begingroup\$

DO NOT use GOTO. They have been criticised since 1960s.

  1. Your indentation is broken for the first snippet. It is also inconsistent with your second snippet. Keep it consistent throughout a project. Try looking at some linters or follow a style guide.
  2. Do not suppress your errors by using @. Let the function throw any and all errors it encounters. You should instead make use of try-catch blocks.
  3. While you could replace the error lookups by error code to a switch-case block, I'd suggest that you use an associated array (or a hashtable) to keep a mapping from ERROR_CODE => "ERROR MESSAGE". This will go in a separate errors.php maybe, and referenced as needed.
  4. Define the target image resolution as a constant, instead of placing magic numbers.
  5. You could also have a client side javascript snippet for checking file size, so that users do not waste their bandwidth and time waiting for upload process.
\$\endgroup\$
  • \$\begingroup\$ I'm surprised there is a GOTO in PHP. I've learned something. \$\endgroup\$ – KIKO Software Jul 5 '18 at 15:42
  • \$\begingroup\$ @KIKOSoftware Just for the records, the GOTO available at PHP is not full unrestricted. \$\endgroup\$ – Mark Messa Jul 5 '18 at 19:04

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.