Page 1 of 2

List Meny PREDEFINED Values validation..

Posted: Wed Jun 20, 2007 11:56 am
by NTGr
Hello.
its the first time i m trying to secure what user ender in form field..so please be gentle with me :oops:
Lets say that i got a simple form...

Code: Select all

$test='<form  action="" method="post"  name="upl_form">
	<select name="categoryboth">
	<option value="">Pls Select One Option Of This Field</option>
			'.$photo_category_list.'
	</select>
	'.$error_cat.'
	<input  name="submit" type="submit"  value="Submit" />';
where $categoryboth...

Code: Select all

//Building Category
				 $photo_category_list  = "";
				 $allowed_category = array();
				 $result = mysql_query( "SELECT category_id,category_name FROM gallery_category ORDER BY category_name" );
				 while( $row = mysql_fetch_array( $result ) )
				 {
					  $photo_category_list .= '<option value="'.$row[0].'|'.$row[1].'" '.($row[0].'|'.$row[1]== $_POST["categoryboth"]? "selected=\"selected\"":"").'>'.$row[1].'</option>';
					  $allowed_category[]= $row[0].'|'.$row[1] ;
				 }
				 mysql_free_result( $result );
that create a list menu like this..

Code: Select all

<option value="">Pls Select One Option Of This Field</option>
<option value="2|EuroDance" >EuroDance</option>
<option value="7|Hip-Hop" >Hip-Hop</option>
<option value="1|Italo" >Italo</option>
Im trying also to create an allowed values array at the same time...seems that is working..but is secure???

OK now the validation part...

Code: Select all

$categoryboth=$_POST['categoryboth'];
	$error_msg='';
	$categorybothA=(is_array($allowed_category) && !in_array($categoryboth,$allowed_category) ? true : false);
	$error_cat=(isset($categoryboth) && $categoryboth=="" && $categorybothA==true ? '<font color="red"> Error in <strong>Category</strong></font>' : " "  );
	$error_msg=(isset($categoryboth) && $categoryboth=="" && $categorybothA==true ?  $error_msg+1  : $error_msg  ) ;
and...

Code: Select all

if ($error_msg!='')       ///<<<<<<<<<<<<<<<<<<< E r r o r s Display Form Again 
{
 redisplay form
}
 else
{
//       Insert values in my DB

  $query = "INSERT INTO database('categoryboth', ....etc....) VALUES ('my_quote($categoryboth)',  ....etc...) ";
	$result = mysql_query($query);
	if (!$result)
	{
		echo "<br>Error with query is ".mysql_error();
	}
	else
	{ 
		echo " CONGRATULATION !!!!!......etc........";
	} 
}
where my_quote()...

Code: Select all

function my_quote( $value )
{
    if( get_magic_quotes_gpc() )
    {
          $value = stripslashes( $value );
    }
    //check if this function exists
    if( function_exists( "mysql_real_escape_string" ) )
    {
          $value = mysql_real_escape_string( $value );
    }
    //for PHP version < 4.3.0 use addslashes
    else
    {
          $value = addslashes( $value );
    }
    return $value;
}
I m doing something wrong??
My validation is safe ??
Thanks.....

Posted: Wed Jun 20, 2007 1:10 pm
by superdezign
Your method of creating an "allowed categories" array is intuitive. There are other ways to do it, but most of them require querying the database later on to check if the choice was valid.

Everything looks solid aside from you not checking if $_POST['categoryboth'] exists before using it, but you've probably already done that beyond the code we see here.

Posted: Wed Jun 20, 2007 2:21 pm
by Mordred
This won't work:

Code: Select all

" (snip) VALUES ('my_quote($categoryboth)', (snip) "
should be:

Code: Select all

" (snip) VALUES ('" . my_quote($categoryboth) ."', (snip) "
And anyway, my_quote() is an evil (and stupid) function, and you are advised not to use it, as it will introduce subtle bugs (not security threats though) under magic_quotes on. I know it's been in the PHP documentation, but there you go, it's still stupid. It is better to check for magic quotes in the beginning of your script, stripslashes all GPC variables if so, and then go on with a "normal" escaping routine, namely mysql_real_escape_string()

Code: Select all

if (!$result) 
        { 
                echo "<br>Error with query is ".mysql_error(); 
        }
This is unwise and unsecure. The same effect can be achieved with trigger_error, and in a production environment you should not display the error messages, but log them in a file.


As for your implementation, I suggest you use only the category_id, the category_name should be used only for display purposes.

Posted: Wed Jun 20, 2007 2:53 pm
by NTGr
superdezign..thank you for the answer....
Your method of creating an "allowed categories" array is intuitive
what you mean..my dictionary is not giving me the right meaning of the word intuitive :)
aside from you not checking if $_POST['categoryboth'] exists before using it
You mean something like this???

Code: Select all

$categoryboth=(isset($_POST['categoryboth']) ? $_POST['categoryboth'] : "" );
Mordred thank you too ...
should be:
Code:
" (snip) VALUES ('" . my_quote($categoryboth) ."', (snip) "
yes you r right..Thank you
And anyway, my_quote() is an evil (and stupid) function, and you are advised not to use it
8O when i submited this post i was only sure that my_quote() was OK ..seems that i must reconsider this
This is unwise and unsecure. The same effect can be achieved with trigger_error,
yes i know that..but right now i want to learn how to validate correctly my forms...

Thank you both once again
:D

CORRECTION...

Posted: Wed Jun 20, 2007 4:29 pm
by NTGr
seems that sould be...

Code: Select all

$categoryboth=(isset($_POST['categoryboth']) ? $_POST['categoryboth'] : "" );
$error_msg='';
$categorybothA=(is_array($allowed_category) && !in_array($categoryboth,$allowed_category) ? true : false);
$error_cat=($categorybothA==true ? '<font color="red"> Error in <strong>Category</strong></font>' : " "  );
$error_msg=($categorybothA==true ?  $error_msg+1  : $error_msg  ) ;
i think is BETTER NOW...
IS IT :?:

Posted: Wed Jun 20, 2007 5:05 pm
by superdezign
I meant for you to check if it's set before even trying to perform any operations on it. Why waste time processing nothing?


And your "my_quote" function isn't.. normal. Use mysql_real_escape_string instead and if magic quotes are on, clean them elsewhere or turn them off. (Basically what Mordred just said, but some repetition may keep it in mind).

better ??

Posted: Thu Jun 21, 2007 4:35 am
by NTGr

Code: Select all

if(!isset( $_POST['categoryboth']) || $_POST['categoryboth']=== '') 
{
	$error_cat='<font color="red"> Error in <strong>Category</strong></font>' ;
	$error_msg=$error_msg+1 ;
}
else
{
	$categoryboth=(isset($_POST['categoryboth']) ? $_POST['categoryboth'] : "" );
	$error_msg='';
	$categorybothA=(is_array($allowed_category) && !in_array($categoryboth,$allowed_category) ? true : false);
	$error_cat=($categorybothA==true ? '<font color="red"> Error in <strong>Category</strong></font>' : " "  );
	$error_msg=($categorybothA==true ?  $error_msg+1  : $error_msg  ) ;
}
and....

Code: Select all

function my_quote( $value )
{
	 if (get_magic_quotes_gpc()) 
	 {
		$value = stripslashes($value);
	 }
	else
	{
	 $value = " . mysql_real_escape_string($value) . ";
	 }
	return $value;
}
BETTER NOW??? :?

Re: better ??

Posted: Thu Jun 21, 2007 5:48 am
by superdezign
Firstly, this:
NTGr wrote:

Code: Select all

if(!isset( $_POST['categoryboth']) || $_POST['categoryboth']=== '')
Can be rewritten as this:

Code: Select all

if(empty($_POST['categoryboth']))
However, if you're not checking at all before that, then even if people don't get a chance to make an entry, they'll get an error message.

And we've said multiple times... my_quote shouldn't be there at all. If you have magic_quotes enabled, clean them upon load (in all of GPC) instead of just going into your database, and use mysql_real_escape_string to escape data going into the database.

Posted: Thu Jun 21, 2007 6:33 am
by NTGr
thank you oce again superdezign !!!!!!!

Reading in
http://www.php.net/manual/en/security.m ... .php#55935
to turn of magic quotes put the following line into the .htaccess file:

php_flag magic_quotes_gpc off
So i think that is what you wanted to say ...
Can be rewritten as this:

if(empty($_POST['categoryboth']))
Thank you for the correction.
However, if you're not checking at all before that, then even if people don't get a chance to make an entry, they'll get an error message.
Sure ...

Code: Select all

if(isset($submit))
{
  validation goes here...etc...
}
I hope that his time i v made all things correctly !!!

Posted: Thu Jun 21, 2007 7:23 am
by feyd
I wrote:Looking for the submit button is not proper. Why? Because it's quite possible to submit the form (normally) without the submit button. Look for more specific data, that must be there or use $_SERVER['REQUEST_METHOD']. You still must use isset()/array_key_exists()/empty() however to detect which fields are there or not just to make sure you don't drop errors.
;)

Posted: Thu Jun 21, 2007 2:15 pm
by superdezign
NTGr wrote:Reading in
http://www.php.net/manual/en/security.m ... .php#55935
to turn of magic quotes put the following line into the .htaccess file:

php_flag magic_quotes_gpc off
So i think that is what you wanted to say ...
Not quite what I said, but that is a valid method.

Posted: Thu Jun 21, 2007 4:48 pm
by NTGr
Thank you superdezign....once again!!!
Can you please be more specific ???
A sample code maybe ???
I dont want to give me the exact answer BUT at least give me a way to discover what you mean...
I know,you explain this several times ..but i dont get it....sorry :oops:

Posted: Thu Jun 21, 2007 4:58 pm
by superdezign

Code: Select all

if(get_magic_quotes_gpc())
{
	function strip_gpc_slashes(&$array)
	{
		if(!is_array($array))
		{
			return;
		}
		foreach($array as $key => $val)
		{
			is_array($array[$key]) ? strip_gpc_slashes($array[$key]) : ($array[$key] = stripslashes($array[$key]));
        }
	}
	
	$gpc = array(&$_GET, &$_POST, &$_COOKIE, &$_REQUEST, &$_FILES);
	strip_gpc_slashes($gpc);
}
I didn't write this, but I always use it. The .htaccess way is perfectly valid as well. I just prefer not to clog up mine, that's all. Yours is probably already clean.

Posted: Fri Jun 22, 2007 3:16 am
by NTGr
Thank you superdezign :D
I didn't write this, but I always use it. The .htaccess way is perfectly valid as well. I just prefer not to clog up mine, that's all. Yours is probably already clean.
Yes its clean sicnce i havent touch it yet !!!!
Thank you once again 8)

feydwrote..
Looking for the submit button is not proper. Why? Because it's quite possible to submit the form (normally) without the submit button. Look for more specific data, that must be there or use $_SERVER['REQUEST_METHOD']. You still must use isset()/array_key_exists()/empty() however to detect which fields are there or not just to make sure you don't drop errors.
You mean that i must create an array with the submited values and check if all submited???
A hidden field is OK to check for , insted of $submit??
Thank you.

Posted: Fri Jun 22, 2007 6:36 am
by feyd
NTGr wrote:You mean that i must create an array with the submited values and check if all submited???
A hidden field is OK to check for , insted of $submit??
Thank you.
Don't blindly use submission values. Check for their existence before use. There's no exceptions to this really.