I am in the process of selecting a php/mysql programmer for my project. To help me decide between two programmers, I asked them to code a working version of this page:

http://22u.com/php/sample.html


They both provided me with working versions of the upload page, but I am having trouble deciding between the two.

PHP coders out there, can you please tell me which of the following is better?

http://22u.com/php/1.txt

http://22u.com/php/2.txt

I am interested in evaluating the quality of this code, which includes efficiency, clarity, brilliance, etc.

Overall, which is better? Why?

Thanks for helping me out!

Neither is better or worse.

One basically is storing your uploads in a database (which has it's merits) and the other is directly on the server as a file.

I suppose the real question has to be, what are you trying to achieve. I would only really store them in a database if you need to do something complicated or advanced sorting on them.

However you should note that neither is PHP 4.1+ compliant with the new super global arrays. i.e. they won't work if register_globals is set to off (as it is by default in 4.2.0 onwards).

Originally posted by Rich2k

However you should note that neither is PHP 4.1+ compliant with the new super global arrays. i.e. they won't work if register_globals is set to off (as it is by default in 4.2.0 onwards).


Thanks for the reply.

What does that mean? Are there any problems if register_globals is set to on, which I assume would be needed for these pieces of code? Can you point me to an explanation of what super global arrays are, and why I might need them?

Thanks!

Basically in older versions of php you could call a cookie, post var, get var, etc by simply $varname

now you need $_COOKIE['varname'] $_POST['varname'] and $_GET['varname']

For more see php.net
http://www.php.net/manual/en/language.variables.predefined.php

Thanks very much.

One more question--

I asked one of them the screen the files, to only accept image files.

Is javascript a good way to achieve this, or can you think of a better way off the top of your head?


<script name="javascript">
function check_it()
{
$im = new Array('.gif','.jpg','.jpeg','.bmp','.tif');
$str=document.forms.data.pic1.value.substr(document.forms.data.pic1.value.lastIndexOf("."), document.forms.data.pic1.value.length);
$newstr=$str.toLowerCase()
flag_go=0;
for(i=0;i<$im.length;i++)
{
if ($im[i]==$newstr) flag_go=1;
}
if(flag_go==1) {
// document.forms.data.submit();
document.forms.data.smb.value="Uploading...";
document.forms.data.smb.disabled="true";
document.forms.data.submit();
}
else
alert("You should upload only pictures.")
}
</script>

I would look in the header of the uploaded file, as javascript can be bypassed.

For example jpeg images would be uploaded as either image/jpeg or image/pjpeg MIME type

To get the MIME type use this $_FILES['userfile']['type'] where userfile is the name of the upload field name on your form.

Thanks again.

Maybe I should hire you as a consultant to keep my coder in line. ;)

The mime-type of an uploaded file is supplied by the client side, just like the filename itself. A regular browser won't give a false mime-type, but an HTTP request could be manually crafted with a false mime-type, so one could sneak a .php or .htaccess file onto the server disguised in the upload process as image/gif or something.

When dealing with user uploaded files, you should make sure the file extension is safe, and determine the validity of the file by examining the content. In the case of an image file, I recommend using getimagesize (http://www.php.net/manual/en/function.getimagesize.php). It will let you know if the file is a valid image file, what its dimensions are, and what the mime-type of the image is by file format, not by extension. (If the file was uploaded with another extension, it would be a good idea to correct the file extension using that info... or treat it as an error case)

Also, when dealing with uploaded files in php, is_uploaded_file (http://www.php.net/manual/en/function.is-uploaded-file.php) and/or move_uploaded_file (http://www.php.net/manual/en/function.move-uploaded-file.php) should be used, for security reasons.

And, like Rich2k pointed out, both of those code chucks are assuming registor_globals is on (which is potentially dangerous), and variables in those scritps aren't very carefully handled (again, dangerous).

Of course there is nothing wrong in having register globals set to on, so long as you take precautions to filter where your variables come from

I will point something else out, 2.txt commented their code while 1.txt did not.

Commenting code is very important, especially if you do not intend to keep the consultant around for the life of your program (which you most likely do not).

Non-commented code is not a huge problem with short code however it becomes a big problem with larger or more complex programs.

Since both programmers knew their code would be evaluated before a decision was made and programmer 1.txt still decided not to use comments I would lean towards programmer 2.txt.

Both scripts will likely encounter problems if safe mode is enabled for PHP (off by default) because they do not use PHP's built in upload copy functions (like another poster mentioned).

Additionally programmer 1.txt uses double quotes (") for non PHP code, this is bad style because PHP parses double quoted strings while it does not parse single quotes (') so for strings that you do not need PHP to evaluate you should use single quotes.

Neither programmer does too much input validation and neither programmer does error checking on their unlink() function calls, if PHP does not have write access to the files or the file does not exist PHP will display an error message.

Like others meantioned both assume register globals is enabled, it is trivial to do an ini_get to ensure it is on or off and generally register globals should be avoided wherever possible (all new code).

JavaScript is good for initial input validation, it gives feedback before the form is submitted but many users disable javascript and an attacker almost certainly will.

As far as PHP inside a image file it would be possible however in order for the attack to occur you would have to be parsing images as PHP files and I can not think of many good reasons to do that (one would be to hide PHP but it still is not a good idea).

Another idea to test a programmer is to ask them to write a PHP script that allows a website visitor to input text into a <textarea> box, then have them display the content back on the screen in such a way it will not interfere with the HTML layout (they must do any and all appropiate input validation).

Coding style should not be the determining factor for hiring a consultant but rather an influencing factor.

Originally posted by Quickfoot

Coding style should not be the determining factor for hiring a consultant but rather an influencing factor.

I was torn up about this decision in part because #2 commented his code. For me, who does not understand this stuff very well, this was something that made me a lot more comfortable. On the other hand, #1 communicated better with me overall, expressing thoughtfulness and enthusiasm, in addition to providing a very strong answer to a question that I asked about encrypting credit card numbers.

I received 47(!) bids for this project on rentacoder, so it was a serious task to narrow down the bids. First, I narrowed them down to 9, asked for some specific work examples, and asked the encryption question (which is explained well by theFish, http://www.webhostingtalk.com/showthread.php?s=&threadid=95984 ). From these responses I narrowed it down to the final two bidders that you see above. (I asked them to each build the page for me.)

Who did I pick? Number 1. Maybe I can get one of you guys to save my ass if they screw up. ;)

Thanks for all your input.

I like #2's code format much better, but then again I'm a big advocate of clean and clear code.