Return to Answer

##The Security Aspect##

Good to see that you're not just checking for a file extension in the name!!

Basically, yes, it's the minimal security needed. There aren't too many steps needed to make an image uploading script safe, however, doing it wrong could present some future issues.

Here's some notes though:

• When renaming the file, what's the point in changing the name to something similar but with characters replaced? You could (not recommended) keep the original name, or you could completely alter the name to something unpredictable, and then use a look-up table (i.e. database) to point the user in the correct path. Instead of simply swapping out illegal characters with underscores, give the file a name that has no resemblance.

• The permissions on your directory do look okay. It is however, not a bad idea to simply remove the images from the public directories, place them in say var/www/uploads/ and let a script get them for you. You don't want these images being in a directory where PHP (or any other server scripts) may run.

• Simply appending $parts["extension"] onto the file seems risky to me. You did check the image type, however I'm not sure how it responds to files with multiple extensions. Someone with more knowledge here could extend this...

##Non-Security Related##

• exit(); and exit; are used throughout. Please don't! Two things: be consistent, they're aliases (along with die); don't use them, they leave the end-user hangin'! These kill the page with no clean way to display an error.

• Using names such as myFile are quite book/tutorial copy-and-pasted. Choose names that have meaning to your application.

• echo "<p>An error occurred.</p>"; Lot's of things I could say here. 1) An error!? No kidding! You kill the page right after this without even saying what the user should do to correct the error. (Doesn't matter if you're the only person using this)

• $name, $parts, $success - more unhelpful variable names!

Other than that, it's a simple piece of code, and there's already tons of people who have done this, plus more. It's usually safer to work with a community run library than to create your own!

The Security Aspect

Good to see that you're not just checking for a file extension in the name!!

Basically, yes, it's the minimal security needed. There aren't too many steps needed to make an image uploading script safe, however, doing it wrong could present some future issues.

Here's some notes though:

• When renaming the file, what's the point in changing the name to something similar but with characters replaced? You could (not recommended) keep the original name, or you could completely alter the name to something unpredictable, and then use a look-up table (i.e. database) to point the user in the correct path. Instead of simply swapping out illegal characters with underscores, give the file a name that has no resemblance.

• The permissions on your directory do look okay. It is however, not a bad idea to simply remove the images from the public directories, place them in say var/www/uploads/ and let a script get them for you. You don't want these images being in a directory where PHP (or any other server scripts) may run.

• Simply appending $parts["extension"] onto the file seems risky to me. You did check the image type, however I'm not sure how it responds to files with multiple extensions. Someone with more knowledge here could extend this...

Non-Security Related

• exit(); and exit; are used throughout. Please don't! Two things: be consistent, they're aliases (along with die); don't use them, they leave the end-user hangin'! These kill the page with no clean way to display an error.

• Using names such as myFile are quite book/tutorial copy-and-pasted. Choose names that have meaning to your application.

• echo "<p>An error occurred.</p>"; Lot's of things I could say here. 1) An error!? No kidding! You kill the page right after this without even saying what the user should do to correct the error. (Doesn't matter if you're the only person using this)

• $name, $parts, $success - more unhelpful variable names!

Other than that, it's a simple piece of code, and there's already tons of people who have done this, plus more. It's usually safer to work with a community run library than to create your own!