[xor-logic]

Hi there.

I just finished a weekend project. The concept is pretty simple: it's like 'hot or not', except instead of rating the hotness of scantily clad women, you rate babies on cuteness.

Client-side form verification is done with javascript for user convenience, then it gets validated for real on the server side with PHP.

Please have a look, poke around, and let me know if anything is out of place, broken, or could be better.
I'd like to know how I've done in the areas of appearance, content, code, usability and security.

Currently, there are only two photos of my son since they're supposed to be uploaded by users.

How Cute Is My Baby

Thanks for helping.

[icedd]

I am no designer, but you sure hit the "baby" theme. The bottle star ranking system is a simple yet clever idea. Though I have disable Java Script and it ceases to work, and their is no alternative. Nowadays that's not so much a problem, but its still nice to provide thorough code to cover for things like users not having Java Script enabled.

On the submit form, the JS alert pop ups are sort of an annoying way to state the required fields aren't filled. I am a fan of Ajax messages on screen. Example... Before validation the email field might say "Email", and after it failed it might say "Email (Required)" with Required in red. My point is they are many, smoother ways to achieve the same message. Also, when I turn of JS again for the same form there is no indication that is has worked or been denied. The fields clear, that it. I'm not sure if it's spamming you messages, or if its just not working. (Sorry for the spam if it did ) You could flash a simple approved or error page on result to make things clearer.

[xor-logic]

Thanks for the feedback. I've never used Ajax - vanilla javascript is what I know; although I could quite easily use javascript to replace the pop ups with a notice at the top of the form if they mess up a field.

But wouldn't Ajax stop working if they turned javascript off? I thought it was more or less advanced javascript.

And no, it didn't spam me. The form uses javascript to do a quick check, then submit. So if js is turned off, no submit.

Also, I received my automated email that someone had uploaded an image, so I went and looked and found a jpg that wouldn't display. First thing I did was download it and open it in notepad, and I got this:

Quote

This is called a double barreled extension, and is a method of intrusion. I don't know much about how it works, but it could be worth some research to prevent.
I'm not sure the process but it does something along the lines of using ".jpg" to get past validation and then drops the ".jpg" and can effectivly work as a ".php" file now.

Was that you that uploaded this? It's cool. I was actually aware of this possibility before I wrote the scripts, but I forgot to code it in. It seems to me that a valid image file name will only have one dot in it, so I could search the file name for that character and reject it if two or more are found. Alternately I could split the string, and if there are more than two tokens, reject it.

I also wonder what would have happened if I had approved this file and my php script on the front page had grabbed it and tried to display it.

I would like it however if whoever uploaded this file could pm me - I have some questions about what you actually uploaded (i.e. I'd like very much to see the file in the state in which it was uploaded).

Thanks.

[Atli]

About the whole "uploading PHP via an Image" attack vector. The simplest way to avoid this is to use the getimagesize() function on the uploaded image. If the image is not a valid image, it will return false. It will also provide the correct mime type of the image via the "mime" element of the returned array.

Quote

It seems to me that a valid image file name will only have one dot in it

Actually no. This is a perfectly valid image file name:
- my.php.example.image.jpg

You can't validate an image by it's file name. You also can't trust the file name or mime type sent by the browser. It can very easily be forged/changed to look like something else.

Quote

But wouldn't Ajax stop working if they turned javascript off? I thought it was more or less advanced javascript.

Yes, it would. AJAX is just a Javascript API for making HTTP requests. It's not "advanced" Javascript, it's a part of Javascript.

This post has been edited by Atli: 15 April 2012 - 08:27 AM

[xor-logic]

I also just realized that there was no verification that votes coming in from the front page were valid votes. For example, sub() is the method I use to submit votes from the main page, and which anyone can very easily determine from looking at the HTML - I popped "javascript: sub('100');" into the browser and bounced the stats way up high.

I think someone may have already done this as I saw some negative stats earlier. Or there was a glitch in my script somewhere.

Either way, the flaw above definitely needs to be fixed. I'll modify the php script to just drop the vote if it's not in the acceptable bounds.

[icedd]

xor

I uploaded the double barrelled extension file.

As Atli and your self stated. Ajax will not work if JS is turned off. What I was trying to get at is the JS alert message is annoying, and while your already using JS maybe something on page would be a better soloution. Ajax was an example also lookup Jquery Ajax, they have made it pretty simple.

And I was playing with your rating system. I forgot to mention that it went in the negatives. (oops). There are several ways to get around this. As you mentioned you could pre-process the query and if its above 10 of below 0 reject the input. My suggestion would be to write it with an "average" base equation. So like... Say there are 4 votes.. (4,8,2,9) just write a function to take the average. 5.75. So maybe you would add all the value in the array together, get array length and divide by array length, then round to nearest whole number. This would solve your problem, because there is no way of custom inputs since you have pre-defined 1-10, no one can be outside of those parameters. Hence it would be impossible to have an average above 10 or below 0. :D

I prefer to keep communication here so others may benefit. But if you want to PM me about more detail of the site that is fine too

This post has been edited by icedd: 15 April 2012 - 09:04 AM

[icedd]

Oh 2 additional comments

1: It's bad practice to have form submits RELY on JS. If someone had JS off, then they effectively can't use your website. Typically you'd have an underlying php validation that would determine sending or not. And you would use JS ONLY as a tool to make your validation seem "pretty". The js should NOT effect the validation process what so ever, just used as a nice visual touch. so like "Fake aesthetic validation for the front end user"


2:..... I have forgotten now

[Atli]

icedd, on 15 April 2012 - 04:16 PM, said:

If someone had JS off, then they effectively can't use your website.

If someone has JS off, they effectively can't use the internet

I'd agree that you should typically have a fallback, but these days it's not unreasonable to expect users to have JS enabled. If you've made the decision to use Javascript heavily on your site, you shouldn't go out of your way to support users without it enabled. (It's a very very slight minority in any case, and they are more than likely aware that their ability to use modern website is out the window.)

[xor-logic]

Well, the voting problem has been fixed (I believe). The php now only accepts numeric input between 1 and 10 inclusive. If it's outside that range, or not a number, the entire script gets skipped and it just reloads the home page.

Atli, on 15 April 2012 - 03:25 PM, said:

About the whole "uploading PHP via an Image" attack vector. The simplest way to avoid this is to use the getimagesize() function on the uploaded image. If the image is not a valid image, it will return false. It will also provide the correct mime type of the image via the "mime" element of the returned array.

How exactly would I go about doing this? I tried getimagesize($_FILES['photo']['name']);, but that just threw errors no matter what I uploaded.

[Atli]

xor-logic, on 15 April 2012 - 05:02 PM, said:

How exactly would I go about doing this? I tried getimagesize($_FILES['photo']['name']);, but that just threw errors no matter what I uploaded.

The $_FILES["photo"]["name"] element only contains the name of the file. You'll need to use the $_FILES["photo"]["tmp_name"] element to get the location of the uploaded file.

[xor-logic]

Atli, on 15 April 2012 - 05:09 PM, said:

xor-logic, on 15 April 2012 - 05:02 PM, said:

How exactly would I go about doing this? I tried getimagesize($_FILES['photo']['name']);, but that just threw errors no matter what I uploaded.

The $_FILES["photo"]["name"] element only contains the name of the file. You'll need to use the $_FILES["photo"]["tmp_name"] element to get the location of the uploaded file.

I tried that too before asking, and it gave me an error message about a blank argument. Maybe I did it wrong, I'll try again in a bit.

EDIT: It occurred to me on re-reading this: what maybe? Of course I did it wrong - it didn't work, did it? Some days I'm not too bright. The rest of the time I'm a moron.

This post has been edited by xor-logic: 15 April 2012 - 10:16 AM

[Atli]

OK, this is an example of how to process an image upload correctly. Something I've used before. Normally I wouldn't just give you a working example (where's the fun in that?!) but this is sort of a delicate operation that you don't want to get wrong. Better to see how to do it correctly right away. - There is one minor "bug" in this code though. See if you can spot it

<?php
/**
 * Processes a image upload, making sure the image is valid
 * and that it will end in the proper extension for it's type.
 * @param string $name The name of the HTML input element.
 * @param string $uploadDir The path to the upload dir.
 * @return string The location where the image is moved to.
 * @throws Exception For all errors.
 */
function processImageUpload($name, $uploadDir="./uploads/") {
	// Make sure there was actually a file uploaded.
	if (isset($_FILES[$name])) {
		// Make sure it uploaded without error.
		if ($_FILES[$name]["error"] == 0) {
			// Check that the image is in fact an image.
			$imgInfo = getimagesize($_FILES[$name]["tmp_name"]);
			if ($imgInfo) {
				// Create a list of allowed image types.
				$mimeTypes = array(
					"jpeg" => "image/jpeg",
					"png" => "image/png",
					"gif" => "image/gif"
				);
				
				// Make sure the mime type of the uploaded image is valid.
				if (in_array($imgInfo["mime"], $mimeTypes)) {
					// Compile the new location for the image.
					$uploadPath = $uploadDir . $_FILES[$name]["name"];
					
					// Make sure the path has the correct extension.
					$imgExt = pathinfo($uploadPath, PATHINFO_EXTENSION);
					$expectedExt = array_search($imgInfo["mime"], $mimeTypes);
					if ($imgExt != $expectedExt) {
						// Add the correct extension to the path.
						$uploadPath .= "." . $expectedExt;
					}
					
					// Check if a file already exists with that name
					// and add a (N) after the file name, to avoid
					// overwriting the existing file.
					if (file_exists($uploadPath)) {
						// Count the number of files that already exist.
						$nameTpl = substr_replace($uploadPath, "(*).", strrpos($uploadPath, "."), 1);
						$files = glob($nameTpl);
						$fcount = count($files) + 1;
						
						// Modify the upload path to include the count.
						$uploadPath = str_replace("(*)", "({$fcount})", $nameTpl);
					}
					
					// Move the file to it's new location.
					if (move_uploaded_file($_FILES[$name]["tmp_name"], $uploadPath)) {
						return $uploadPath;
					}
					else {
						throw new Exception("Failed to move the uploaded file.");
					}
				}
				else {
					throw new Exception("Only JPEG, PNG and GIF images are allowed!");
				}
			}
			else {
				throw new Exception("File is not an image!");
			}
		}
		else {
			throw new Exception("Upload failed with error #" . $_FILES[$name]["error"]);
		}
	}
	else {
		throw new Exception("No file upload found.");
	}
}

[xor-logic]

Atli, on 15 April 2012 - 06:08 PM, said:

There is one minor "bug" in this code though. See if you can spot it

Nope, I couldn't. Had a read through, but I'm pretty new to PHP, which is why we're having this conversation in the first place. In any case, I took a look at the code you provided and worked it into my current page. So now it looks something like this:

$valid = false;

if(isset($_FILES)) {
  if($_FILES != 0) {
    $info = getimagesize($_FILES);
     if($info) {
       $mimeTypes = array("jpeg" => "image/jpeg","png" => "image/png","gif" => "image/gif");
       if(in_array($info,$mimetypes)) {
         $valid = true;
       )
     }
  }
}
/*I wrote it this way so that I could just enclose 
the entire rest of the script in an if statement, 
rather than modifying the rest of the validation script
since if it fails here, it's no good anyways.*/

I haven't tested it yet though. Going to go do that in a second. If there are any issues, I'll see if I can sort them out, and if not I'll put them up here.

EDIT:
Something in this line is throwing an error, even with valid files. Unfortunately, this is the part of the code you provided that I don't really understand.

       $mimeTypes = array("jpeg" => "image/jpeg","png" => "image/png","gif" => "image/gif");

I looked up php arrays, but I'm still lost.

This post has been edited by xor-logic: 15 April 2012 - 04:00 PM

[Lemur]

I'm not a fan of the design. If I instinctively twitch and reach for the middle mouse button to close a tab it's really not a good thing.

The site has no real contrast, no direction for my eyes to immediately follow. The white shadow on the text blurs it to the background and makes it extremely hard to read the text and impossible to ghet an immediate feel for the site.

I see no real necessity with the flowers other than you randomly added them to the logo. You have to remember that every element needs to have a large amount of relevance to the overall site design otherwise it detracts from the point and really just kills the flow of the site. If the flow is obstructed then the entire site is crippled.

You have under two seconds to catch and keep a visitor. They need to immediately know what they're using or they're out. Really, it has to almost be instant anymore. Any more than a 2 MS lag time in load and you'll be gone.

Speed and clarity is the name of the game.

[Atli]

xor-logic, on 15 April 2012 - 10:46 PM, said:

Something in this line is throwing an error, even with valid files.

Actually, I would guess that the two lines above the one you posted are the problem. Compare the getimagesize() call in your code to the one in mine and you should see it. The way you are doing it, you are passing the whole $_FILES array into the function, where you should only be passing a single element from within the array (the location of the image file) to it.

Also, this doesn't make sense. An array will never be 0.

if ($_FILES != 0)

I'm guessing you'r trying to see if there was an error during the upload, in which case you need to check the "error" element of a specific image, like I do in my code: $_FILES["photo"]["error"]. (And you want to make sure it is zero. Zero, in this instance, means "success".)