Something wrong with the quotations

Question

Sorry I'm new here, after you asked, I have added the full code: And I know my code is bad :)

if ((($_FILES["image_name"]["type"] == "image/gif")
   ($_FILES["image_name"]["type"] == "image/jpeg")
   ($_FILES["image_name"]["type"] == "image/png")
   ($_FILES["image_name"]["type"] == "image/pjpeg")))
  {
  $year = date('y');
  $month = date('m');
  $date = date('d');
    if(file_exists('./uploads/'.$year)){ 
        if(file_exists('./uploads/'.$year.'/'.$month)){
            if(file_exists('./uploads/'.$year.'/'.$month.'/'.$date)){ 
                $target_path='./uploads/'.$year.'/'.$month.'/'.$date.'/'.$_FILES["image_name"][‌"name"];
            }
            else{
                mkdir('./uploads/'.$year.'/'.$month.'/'.$date);
            }
        }
        else{
            mkdir('./uploads/'.$year.'/'.$month);
        }     
}
else{
    mkdir('./uploads/'.$year);
    mkdir('./uploads/'.$year.'/'.$month);
    mkdir('./uploads/'.$year.'/'.$month.'/'.$date);
}
  if ($_FILES['image_name']['error'] > 0)
    {
    echo 'Return Code: ' . $_FILES['image_name']['error'] . '
';
    }
    else
      {
      move_uploaded_file($_FILES['image_name']['tmp_name'],
      './uploads/'.$year.'/'.$month.'/'.$date.'/'.$_FILES['image_name']['name']);
      $target_path='./uploads/'.$year.'/'.$month.'/'.$date.'/'.$_FILES["image_name"][‌"name"];
      }
  }

The $target_path variable should print like this:

uploads/13/03/15/image_name.jpg

but it prints:

s/13/03/15/image_name.jpg

What's wrong?

DaveRandom · Accepted Answer

First, an important note on file uploads in general: The type and name keys are unsafe to use. This is because they are defined by the client, and they are a potential mechanism for injecting malicious code into your site. Consider what would happen if I set the file name to ../../../../../index.php, or if I set the MIME type to image/gif but uploaded a PHP file instead?

Next, an important note on image uploads specifically: You cannot trust image data uploaded by the client. It is also possible to embed malicious code in something that looks like an image. You need to copy the pixel data out of file and create a new one. This is usually accomplished with the GD extension.

Next, on mkdir() - it has a third argument, if you pass true to this third argument it creates the directory tree recursively, so you don't need to create each level in a separate operation. Also note that (like many things) it is possible for mkdir() to fail, if this happens it will return false, and you should check for this.

Now, to answer the actual question (and ignoring the aforementioned security issues for a moment), here is how I would simplify your code:

// Configuration
$allowedTypes = array(
    "image/gif", "image/jpeg", "image/png", "image/pjpeg"
);
$baseDir = './uploads';

// Check file was uploaded successfully
if ($_FILES['image_name']['error'] > 0) {
    exit('Return Code: ' . $_FILES['image_name']['error'] . '
');
}

// Check file type
if (!in_array($_FILES["image_name"]["type"], $allowedTypes)) {
    exit('Invalid file type: ' . $_FILES['image_name']['type'] . '
');
}

// Check/create target directory
list($year, $month, $day) = explode('-', date('y-m-d'));
$targetDir = $baseDir . '/' . $year . '/' . $month . '/' . $day;
if (!is_dir($targetDir)) {
    if (!mkdir($targetDir, 0644, true)) {
        exit('Failed to create destination directory
');
    }
}

// Store the uploaded file permanently
$targetPath = $targetDir . '/' . .$_FILES['image_name']['name'];
if (!move_uploaded_file($_FILES['image_name']['tmp_name'], $targetPath)) {
    exit('Failed to move temporary file
');
}

However, I wouldn't do this.

File upload is a very common task, and the generic code I use looks something like this. Looks complicated doesn't it? Well, that's because handling file uploads isn't simple. However, what are that complexity does is provide a nice straightforward way to address the security concerns I outlined above. It has built in support for images, including options to resize then in a clean and easy way.

Let's have a look at how we might use it in your code:

$baseDir = './uploads';

// Very simple autoloader for demo purposes
spl_autoload_register(function($class) {
    require strtolower(basename($class)).'.php';
});

// When you instantiate this the $_FILES superglobal is destroyed
// You must access all uploaded files via this API from this point onwards
$uploadManager = new \Upload\Manager;

// Fetches a FileCollection associated with the named form control
$control = $uploadManager->getControl('image_name');

// getControl returns NULL if there are no files associated with that name
if (!isset($control)) {
    exit('No file was uploaded in the image_name control');
}

// Check/create target directory
// You still need to do this, it's not magic ;-)
list($year, $month, $day) = explode('-', date('y-m-d'));
$targetDir = $baseDir . '/' . $year . '/' . $month . '/' . $day;
if (!is_dir($targetDir)) {
    if (!mkdir($targetDir, 0644, true)) {
        exit('Failed to create destination directory');
    }
}

// This also handles multiple uploads in a single control, so we need to loop
foreach ($control as $image) {
    // You need to determine a file name to use, most likely not from user
    // input. This is a high-entropy low collision-risk random approach.
    $targetFile = $targetDir . '/' . uniquid('upload-', true);

    try {
       $image->save($targetFile, true, IMAGETYPE_ORIGINAL);
    } catch (\Exception $e) {
       exit("Oh noes! Something went badly wrong: ".$e->getMessage());
    }
}

This does a lot of things in the background to address the security concerns I outlined earlier. It automatically detects that the image is of a valid, recognised type and applies the correct file extension to the saved file as well.

Something wrong with the quotations

Answers (2)

Related Questions