Validate Numerical Input

[nodirtyrockstar]

I have a form which contains a dynamic number of text fields that are intended for shoppers to input a positive number only. I separated the validation into a function. It is working well for me so far, but given that security is such a major concern I thought I would ask for comments from the forum. Here's the function:

 

   function validate($array){
       if (count($array) > 0) {
           foreach($array as $product){
               if(is_numeric($product[0]) && $product[0] >= 0){
                   return true;
               }
               else{
                   return false;
               }
           }
       }
   }

 

When the form is sent, the returning page does a little bit of it's own configuring; functions relevant to the page itself and the user's context. Then the script checks to see if input was sent. Then it validates the data before attempting to use it. There are even checks further down in the script that continue to compare it as though it is numeric, and that it is greater than or equal to zero.

 

Is this sufficient?

Edited October 10, 2012 by nodirtyrockstar

[Pikachu2000]

is_numeric() will validate 12.3948E491, and 0xAC4DF5 as valid numbers. If you want only positive whole numbers, use ctype_digit. If you need to allow decimals, you can str_replace the decimal point out of it for the ctype_digit() validation.

[nodirtyrockstar]

No decimals needed! This is a very helpful tip, thank you. Other than making sure to use the correct function to validate the numbers, can you (or anyone else on here) see any other security concerns?

[Pikachu2000]

You could also type cast each value as an integer in the loop. I'd validate that the array is !empty instead of using count(), but that isn't really a security concern. With ctype_digit(), you can eliminate the check for >= 0 too, since it won't validate a negative sign as a valid digit.

[nodirtyrockstar]

Okay. I took all of your suggestions and now I have this:

 

   function validate($array){
       if (!empty($array)) {
           foreach($array as $product){
               if(ctype_digit($product[0])){
                   $product[0] = (int)$product[0];
                   return true;
               }
               else{
                   return false;
               }
           }
       }
   }

[Pikachu2000]

I see an issue in the fact that each iteration of the loop will overwrite $product[0]. What is the structure of the $array you're starting with?

[nodirtyrockstar]

I see what you're getting at, but I am working with a 2D array, and each subarray contains only one entry.

[Christian F.]

Since you're working with a generic validation function, it's best to ensure that you are sure it'll either handle an array with more than 1 element in the second dimension, or more dimensions.

A good tip in general, is to utilize the "exit early" principle. To avoid your code becoming too nested, and thus harder to read than necessary.

 

I've edit your code a bit, to show you what I mean:

function validate ($array) {
// Exit early, if this is not an array.
if (empty ($array)) {
	return false;
}

foreach ($array as $product) {
	// If it's a multi-dimensional array, validate it recursively and skip to next item.
	if (is_array ($product)) {
		if (!validate ($product)) {
			return false;
		}
		continue;
	}

	// Exit early if not a digit.
	if (!ctype_digit ($product)) {
		return false;
	}

	// Type cast to ensure validation isn't compromised.
	$product = (int) $product;
}

// All elements of the array has been validated successfully.
return true;
}

[Pikachu2000]

There are a couple other things I noticed too. The return in the foreach loop will exit the function prematurely, and you never return the valid data from the function.

 

 

[nodirtyrockstar]

@Christian - While I appreciate the design principle lesson, I don't really think you simplified my function. If the array is empty, the function finishes. If the array is not empty, and the input passes validation, it is cast as an integer and the function returns true. Furthermore, the array that this function will return is a list of products, and the only thing stored in the subarray is the desired quantity. Therefore, the array will NEVER have more than one index.

 

@Pikachu2000 - The function is not intended to return data. I will cast my data as int elsewhere.

 

I really appreciate all of the coding advice, but I am really interested in security at this point. Is there another forum I should post to which specializes in security?

[kicken]

I really appreciate all of the coding advice, but I am really interested in security at this point. Is there another forum I should post to which specializes in security?

 

The only two "security" related issues regarding the function have been addressed:

- is_numeric allows more than you want, so use ctype_digit

- You are returning too early, and thus possibly not checking all entries

 

You should have a return false; at the end of the function (incase the !empty condition is false), but it won't cause any type of "security" problems either.

 

One can't really talk about whether something is "secure" or not by just looking at one function. whether something is secure isn't just about how well you do or don't validate input data, it's about the entire process of how the data is used.

[Christian F.]

So, if I've understood you correctly, you have an array containing a number of sub-arrays. Each of those sub arrays contain only one, and always one, element? If so, then I would have done away with the second dimension completely, and saved the elements directly into the first dimension. Anything more is quite useless.

 

Secondly, I didn't say that I (only) simplified your function. I also said that I would have added functionality, to make it more generic and suit the name you gave it. If you don't want the second dimension in the array, as stated above, then just remove the entire recursive-block (if (is_array...) from the function.

[nodirtyrockstar]

Thanks for all of the help with answering my question about security for this one very simple function!