1

I have been writing a file uploader, and I want to validate the types and size, and I currently have this:

    <?php 

//SETTING UP LOCAL VARIABLES
$username = trim($_POST['username']);
$password = trim($_POST['password']);
$name = trim($_POST['name']);
$email = trim($_POST['email']);
$message = trim($_POST['message']);

//STRIPPED OUT ALL NON ASCII CHARACTERS
$username = preg_replace('/[^(\x20-\x7F)]*/','', $username);
$password = preg_replace('/[^(\x20-\x7F)]*/','', $password);
$name = preg_replace('/[^(\x20-\x7F)]*/','', $name);
//$email = preg_replace('/[^(\x20-\x7F)]*/','', $email);

//SETTING UPLOAD DIR
$upload_dir = $_SERVER['DOCUMENT_ROOT'] . "/beta_images/";

//CREATING A RANDOM HASH TO PROTECT FROM DUPLICATE FILES
$random = rand(1, 100);

$user_hash = $random . $name;

$hash = sha1($user_hash);
$hash = substr($hash, 32, $random);

foreach ($_FILES['file']['error'] as $key => $error) {

$counter++;

if (
(
    ($_FILES['file']['type'][$key] == "image/jpeg")
 || ($_FILES['file']['type'][$key] == "image/pjpeg")
 || ($file['file']['type'][$key] == "image/jpg")
)

&& ($error == UPLOAD_ERR_OK)
&& ($_FILES['file']['size'][$key] < 20971520)
) {

        $temp_name = $_FILES['file']['tmp_name'][$key];
        $image_name = $hash . '_' . $name . $counter . '.jpg';
        move_uploaded_file($temp_name, $upload_dir . $image_name);

    } else {

    $body_fail = "Someone forgot to do something and wound up on the Sorry page. You might want to contact them and try and convince them to join still. Here are the details \n\n Username: $username \n Password: $password \n Email: $email \n Name: $name \n Message: $message";

    mail("bla", "Failed beta sign up", $body_fail);

    header("Location: ../sorry.html");

    }

}

        //EMAIL INTERNAL

            $body_internal = "Success! $name has submitted a beta entry!\n\n Their username is \n $username \n Their password is \n $password \n Their email address is $email.\n\n The images are located in \n /beta_images/{$hash}_{$name}1/2/3.jpg \n\n They also wrote a little message for us: \n$message";

            mail("bla", "New Beta Sign Up", $body_internal);

        //EMAIL INTERNAL

        //EMAIL EXTERNAL

            $body_external = "Thank you for applying to join the Stylistic Gallery, we just wanted to let you know that we have received everything and will be in touch shortly.\n\n Best Wishes \n Stylistic Gallery \n\n The Stylistic Gallery, a portal for creative individuals and businesses to showcase and sell their work online";

            mail($email, "Thank you for your application", $body_external);

        //EMAIL EXTERNAL

        header("Location: ../thanks.html");

However it is still going through even if the files are not jpg or the size is bigger.

Sorry for being vague, so confused from looking at it too long at the moment :)

Thanks!

3
  • I suggest you to rewrite that condition, it will be easier to understand what is going on... Something Like: $types = array("jpeg", "jpg"); if (validImageType(types, $_FILES['file']['type'][$key])) .... and then inside the function or method validImageType check if the type of image is included in the collection.... Nov 3, 2010 at 18:20
  • Could you do a var_dump($_FILES['file']['type'][$key]) within your loop and see what turns up when you upload a non-JPEG file?
    – BoltClock
    Nov 3, 2010 at 18:33
  • @BoltClock I got string(24) "application/x-msdownload" Nov 3, 2010 at 18:37

2 Answers 2

5

Just because you issue a Location: header DOES NOT mean the script stops executing. Try placing an exit; or die; right after the redirection header.

Also, you should generally die to a small amount of content describing where to go for browsers that won't follow redirects, e.g.

die(<<< ERRORTEXT
<!doctype html>
<html><head><title>Error: Foo</title></head><body>
<h1>We're sorry, your call could not be completed as dialed.</h1>
<p><a href="../sorry.html">Click here to continue to your final destination.</a></p>
</body></html>
ERRORTEXT
);

EDIT: Nitpicking

($_FILES['file']['size'][$key] < 20971520)

What we have here is called a "magic number"; sure it's obvious now, but it won't take up a lot more time programming-wise or execution-wise to do one of the following:

($_FILES['file']['size'][$key] < 20 * 1024 * 1024)

or

define('MAX_FILE_SIZE', 20 * 1024 * 1024); // 20 MB
($_FILES['file']['size'][$key] < MAX_FILE_SIZE)
1
  • 1
    Spot-on with the Location header.
    – BoltClock
    Nov 3, 2010 at 18:34
4

You're missing some parantheses and you want to replace one || by &&.

if (
(
    ($_FILES['file']['type'] == "image/jpeg")
 || ($_FILES['file']['type'] == "image/pjpeg")
 || ($_FILES['file']['type'] == "image/jpg")
)

&& ($error == UPLOAD_ERR_OK)
&& ($_FILES['file']['size'] < 20971520)
) {

If you've got long if statements like this it's better to break it down. Here's how I would code your loop:

/* ideally you would move those messages to some text files, to make it
 * easy to change any content and eventually localize it.
 */

// mail if upload fails
$failure_mail_body_to_admin = "Someone ...";

// mail to admin after successful upload
$sucess_mail_body_to_admin = "Success! ...";

// mail to user after successful upload
$sucess_mail_body_to_user = "Thank you ...";

// mime types of allowed images, I know that those can be spoofed
$allowed_mime_types = array("image/jpeg", "image/pjpeg", "image/jpg");
$fileCount = 0;

foreach ($_FILES as $filename => $file) {
    $file_is_ok = true;
    // test general errors
    if ($file['error'] != UPLOAD_ERR_OK) {
         $file_is_ok = false;
    }

    // test size (< 20MB)
    if ($file['size'] >= 20971520) {
        $file_is_ok = false;
    }

    if (!in_array($file['type'], $allowed_mime_types) {
        $file_is_ok = false;
    }

    if ($file_is_ok) {
        $fileCount++;

        // store image
        $temp_name = $file['tmp_name'];
        $image_name = $hash . '_' . $name . $counter . '.jpg';
        move_uploaded_file($temp_name, $upload_dir . $image_name);
    }
}

if ($fileCount > 0) {
    // send confirmation mails
    mail("bla", "New Beta Sign Up", $sucess_mail_body_to_user);
    mail($email, "Thank you for your application", $sucess_mail_body_to_admin);

    // redirect user
    header("Location: ../thanks.html");
    die();
} else {
    mail("bla", "Failed beta sign up", $failure_mail_body_to_admin);

    // redirect user
    header("Location: ../sorry.html");
    die;
}
6
  • Added that to my code but I am still able to upload .exe's. Updated my post with the full code. Nov 3, 2010 at 18:23
  • @Noctine: You've also had [$key] added to each statement, which I didn't know what purpose it served. I don't think you need that. Nov 3, 2010 at 18:33
  • The only problem I have found with this code is that I want to exempt error no.4, I don't mind if there is no file. Any ideas on that one? Nov 3, 2010 at 18:47
  • @Noctine: To be honest, I don't fully understand your code. Is there a possibility of having more than 1 file uploaded at the same time? Because if yes, you're iterating over the wrong array. Nov 3, 2010 at 18:53
  • Yes, there is 1 - 3 files uploading at the same time. Its my first time doing multiple file uploads and I am confused as hell. Everything does work now, its just coded extremely badly :) Nov 3, 2010 at 18:56

Your Answer

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.