2

I'm trying to develop some templates for common PHP tasks I've been dealing with. One of which is a general 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. Does this code safe? Or are there any vulnerability that could be exploited by an malicious client? In this case, how to 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?
  3. Any other idea to improve it?
Improve this question
8
  • Code Review is what you're looking for Commented Jul 4, 2018 at 16:25
  • @Samuel Is it possible to transfer it? Commented Jul 4, 2018 at 17:10
  • I'm not sure, I would just copy+paste it on there. They are a pretty nice community, and every time you have working code which you think can be improved they are extremely helpful. Commented Jul 4, 2018 at 17:11
  • 1
    @Samuel Posts can be migrated between Stack Exchange community and sister sites. No need for creating cross-site duplicates. Just flag the post as off-topic with custom reason that it should get migrated to [codereview.se], which will get converted to a comment as: "it should get migrated to Code Review". Moderators and other users agreeing to your flag will make it happen :) Commented Jul 4, 2018 at 21:03
  • 1
    I'm voting to close this question as off-topic because it now has a cross site duplicate at codereview.stackexchange.com/q/197826/12240 (and is better suited there). Commented Jul 4, 2018 at 21:04

1 Answer 1

Reset to default
1

Really, look into putting this code into functions (maybe even a class), and instead of goto's just use return. This will allow you to better structure and separate logic where it needs to be separated.

Look at this example:

function upload_image($file)
{
  if( $err = check_error($file['error']) ) return $err;
  if( !is_uploaded_file($file['tmp_name']) ) return 'Failed to upload the file';
  $resize = imageResize($file['tmp_name'], 'random.png');
  if( $resize !== 0 )
  {
    return 'Invalid image format';
  }

  return true;
}

For error checking look into using the switch function. It will be more organized (in my opinion).

I would also check for the numerical upload errors in a separate function, this will allow for the individual action to be easily distinguished.

function check_error($err)
{
  if($err === 0)
  {
    return false; // no errors
  }
  $response = false;
  switch($err)
  {
    case 1:
    case 2:
      $response = 'File exceeds maximum size limit';
      break;
    case 4:
      $response = 'No file uploaded';
      break;
    default:
      $response = 'Unkown error';
  }
  return $response;
}

Then just call the function and display an error at the top if any:

$upload = upload_image($_FILE['file_upload']);
if( $upload === true ):
  echo 'Image uploaded successfully!';
else:
  echo $upload;
?>
<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>
<?php endif; ?>
Improve this answer
Sign up to request clarification or add additional context in comments.

7 Comments

Totally agree (code into functions, switch and numerical upload errors). Helpful insights. Thks!
Although I see your point and liked most of your ideas, there are still some issues with your code: 1) missing equal sign at $response = 'Failed to upload the file';, 2) missing equal sign at $response 'File exceeds maximum size limit';, .....
3) try to execute PHP file and upload any standar file, you will always get the following output: Failed to upload the file
4) case 0: If the value is identical to 0, then there is no error and the file uploaded with success (Error Messages Explained).
5) since the switch-case is not set to do identical comparisons (as in the original post), it is another source of bugs.
|

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.