Some advice on this is overkill or a better less process intensive way..

[yourichi]

Ok bit of a longwinded one but basically on page I will have a standard form

 

 <form name"lala" action=post.php?transfer=yes type="POST"> 
<input type="textbox" name="scouts1">
<input type="select" name="fromscouts1">
<input type="select" name="toscouts1">
<input type="button" name="transfer">
</form>

(not gonna type it all, but you get the idea)

 

Now its actually posting to itself. concept is, towards the top of the page, there is a like a header box,

that has

<?php onloadfindpostdata(); ?> 

 

now in the database connection resource file that i use for majority of my functions is

 

Now this was recommended to me by a friend who was doing similar things with forms and recommended this function he got from a guide on protecting your php forms from SQL injection etc.

 

function cleanQuery($string)
{
  if(get_magic_quotes_gpc())  // prevents duplicate backslashes
  {
    $string = stripslashes($string);
  }
  if (phpversion() >= '4.3.0')
  {
    $string = mysql_real_escape_string($string);
  }
  else
  {
    $string = mysql_escape_string($string);
  }
  return $string;
} 

 

 

Now This is the function thats called when the page is loaded, to pick up any form entrys and changes they wish to do

<?php
function onloadfindpostdata()


{


if (isset($_GET['transfer'])) $linkchoice=$_GET['transfer']; 
else $linkchoice=''; 

switch($linkchoice){ 

case 'yes' : 
    
if(!$_POST)

    { echo "failed to recieve data for transfer";
} else
{
	//$post = array_map(“cleanQuery”, $_POST);

$noscout = cleanQuery($_POST['scouts1']);
    if(!is_numeric($noscout)){
	$noscout = "Invalid number";
}

$fromscouts1 = cleanQuery($_POST['fromscouts1']);
    //if()){
	switch($fromscouts1){ 

case 'lions' :
	$fromscouts1 = "lions";
	break;
case 'griffins' :
       	$fromscouts1 = "griffins";
	break;
case 'home' :
      $fromscouts1 = "home";
  break;
  default: 
  echo "Stop screwing around";
  exit;	
}

$toscouts1 = cleanQuery($_POST['toscouts1']);
    //if()){
	switch($toscouts1){ 

case 'lions' :
	$toscouts1 = "lions";
	break;
case 'griffins' :
       	$toscouts1 = "griffins";
	break;
case 'home' :
      $roscouts1 = "home";
  break;
  
  default: 
  echo "Stop screwing around";
  exit;	
}

 

Now the leading question is, is this sufficent validating to prevent abuse of the form, as the eventual form will have 4 more sets of data sets on the form like this, I already have that in place, but felt it was only nessecary to show one of the sets of data,

 

The data once validated will construct a query that looks something like

 

if ($toscouts1 == "griffins") AND ($fromscouts1 == 'lions'){
$query = "SELECT `scouts`,`alphascouts`,`betascouts` FROM troops WHERE id='_SESSION[user_id]'";
  mysql bla bla bla or die etc
list($alphascouts,$betascouts) ($results) etc etc

$newfrom = "$alphascouts - $noscouts";
  $newto = "$betascouts + $noscouts";
  
$query  = "UPDATE `troops` SET `alphascouts` = '$newfrom', `betascouts` ='$newto' WHERE id ='_SESSION[user_id]`";
}

Oh and about the case GET statement, reason im posting to separate case GET's is ill have separate forms for performing a different task not concerning this particular function, and wanted to be identify different post data from a different form.

otherwise it seems a little redundant i know. Also setting defined case options, was hoping to prevent misuse of url entrys into GET requests. although im sure i need to be filtering the get request also clarify please?

 

made that up on the fly so didnt really type it out in full correctly, skipped the obvious query parts. made it as a general idea of what im trying to achieve and how im using the user data.

 

So in short are the validations sufficient for what im trying to achieve.

As this project is starting to move towards ready for testing although not ready to go onto a production enviroment, it will be readily available. At this stage I want to test not to have it crushed before it even gets off the ground, so im starting to go back round and tightening up the obvious entry points on forms etc.

 

Voices, opinions? (yes i know my coding is sloppy, thats not what i was asking )

 

 

Thanks

Regards, Yourichi~

[Psycho]

OK, I didn't read through all of your post, but I think I read enough to at least provide some suggestions.

 

I think the function cleanQuery() is problematic for several reasons:

 

First off, if magic quotes are turned on, then you need to be running strip_slashes on the input regardless if you are going to use it in a query or not. You state that you are having the form post to itself. I do that all the time because I can then repopulate the form with the user's input if there was a validation error. If magic quotes are on you need to unescape the input but NOT run it through the mysql_real_escape_string(). The way I look at it - is there any reason you want the escaped value that is generated via magic quotes? I can't think of any. Therefore, I prefer to put a function that is called on load of every page to unescape ALL input (POST, GET, etc.) if magic quotes is turned on. Then you are always dealing with the raw user input.

 

Second, I see no reason to try and support mysql_escape_string(). That function was introduced in 4.0.1 (the year 2000) and mysql_real_escape_string() was introduced in 4.3.0 (the year 2002). If anyone is running a version of PHP that doesn't support mysql_real_escape_string() they should upgrade. Trying to be backward compatible to a decade old technology is not worthwhile.

 

Third, mysql_real_escape_string() is for 'string' data. You should not rely upon it for any other type of input: date and/or time, ints, floats, etc. Running values through  mysql_real_escape_string() that are supposed to be numeric is a pointless process. I see you are running $_POST['scouts1'] through the cleanQuery() function and then checking if the value is numeric. If the value is numeric then the cleanQuery() wouldn't do anything to the value anyway. So, you just performed an operation that did nothing. Also, I don't think is_numeric() is the right validation. I assume the value is the primary index for a db value? If so, you should be checking that the value is an integer. is_numeric() will return true for negative and float values.

 

Lastly, unless you have a good reason not to (on a case y case basis) you should always be trimming the user input to remove leading/trailing white space.

 

So, I suggest having an initial process that runs strip_slashes() on the initial input in case you need to repopulate the form with the submitted data or use it in another manner outside of a query. Then use a second process to escape/validate the data for use in a db query. You can create individual functions for each type of data you need to process (string, date, int, float, etc.) or you can create one function that you pass the value and a second parameter to determine the correct processing.

[yourichi]

I appriciate the input, like I said that particular cleanquery was passed to me from a friend who'd had problems with people manipulating forms  and various other issues, and suggested that would be a good fix for validating.

 

The backwards compatibility parts of it are like you said fairly obsolete and isn't really my end goal on a production level. As for certain I will be working with php 5~ install.

Wasnt really the key issue I was looking at.

 

Your point about the numeric value is duely noted, that was something I was wondering about myself, hence I asked.

Ive never been a big fan of client sided form validation as its always so easily broken. I was merely trying to enforce that particular form field could only be passing numbers. Wasnt looking for anything fancy like float values etc.

its just purely meant for handling whole round numbers. As an easy way to prevent any kind of data corruption or intentional breaking, or maniupulation of sql injection etc.

 

the numeric value isnt actually an index of any kind, its just purely a physical value that will go into a few maths questions to alter a few fields decided by the select lists.

 

obviously ensuring that the list data maintains its legitamacy is important as it tells the query - the numeric value from field $a and + to field $b

as both field a and b are stored in db as INT.

 

This particular task will not require any repopulation, altho I can think of another page where I will need to do such, so Its useful to keep in mind for reference.

 

[Psycho]

Your point about the numeric value is duely noted, that was something I was wondering about myself, hence I asked.

Ive never been a big fan of client sided form validation as its always so easily broken. I was merely trying to enforce that particular form field could only be passing numbers. Wasnt looking for anything fancy like float values etc.

its just purely meant for handling whole round numbers. As an easy way to prevent any kind of data corruption or intentional breaking, or maniupulation of sql injection etc.

 

Where did client-side validation come into this discussion? But since you bring it up. There's nothing "wrong" with client-side validation. In fact, I like implementing client-side validation - it gives the user immediate feedback without having to submit the form. It's just that you should never "rely" upon client-side validation. You always have to validate server-side.

 

You say you are not "looking for anything fancy like float values", but that is the validation you are doing now. By your explanation you want an integer value. So, use the correct validation: is_int().

[yourichi]

Dont get me wrong, Im not saying I dont implement it, its just like i said i dont go overboard on it as its an unreliable validator to any wannabe mischief makers.

Think i represented what i was looking for poorly, as it feels like you think im disagreeing with you, which isnt the case. and the numeric factor like i said i noted, an will use int rather than numeric, that was my lack of knowledge on the true purpose of that.

It was one of the things i was looking for

[Psycho]

Dont get me wrong, Im not saying I dont implement it, its just like i said i dont go overboard on it as its an unreliable validator to any wannabe mischief makers.

 

Well, just to clarify my position. I wouldn't say that client-side validation is "unreliable" to prevent malicious input - I would say it is completely worthless and should never be relied upon. Clint-side validation is a good way to give the user immediate feedback and a more interactive experience. It should never be used as a replacement for server-side validation to prevent malicious input.

[yourichi]

So a revised segment of code would look something along the lines of this?

 

if (isset($_GET['transfer'])) $linkchoice=$_GET['transfer']; 
else $linkchoice=''; 

switch($linkchoice){ 

case 'yes' : 
    
if(!$_POST)

    { echo "failed to recieve data for transfer";
} else
{
	//$post = array_map(“cleanQuery”, $_POST);

$noscout = strip_slashes($_POST['scouts1']);
    if(!is_int($noscout)){
	$noscout = "Invalid number";
}

$fromscouts1 = strip_slashes($_POST['fromscouts1']);
    //if()){
	switch($fromscouts1){

case : etc etc etc 

[Psycho]

No, that is running strip_slashes() regardless of whether magic quotes are enabled or not.You should only run it is magic quotes are enabled. Otherwise you would remove slashes that the user entered.

 

The manual has a function that you can run on page load that will run strip_slashes() on all user input if magic quotes is enabled. I use something similar and put it in one of the config files that I load for every page.

 

http://php.net/manual/en/security.magicquotes.disabling.php (see example #2)

[yourichi]

Ok, So whats the best way to provide protection against the string based inputs such as the form selection boxs?

the case statement that id already created to filter anything that wasnt in the predefined options?

 

[Psycho]

Ok, So whats the best way to provide protection against the string based inputs such as the form selection boxs?

the case statement that id already created to filter anything that wasnt in the predefined options?

 

I'm not totally understanding that second sentence. For a SELECT input I always validate against the predefined values. In many cases a select list is generated from a list defined in the database. So, I would test the received value against the database (but first 'cleaning' the value based upon it's type: string, int, date, etc.). Although, if the value is 'hard-coded' then I usually have it stored in an array. Then I just check the submitted value against the array.

[yourichi]

sorry that second sentance was meant to say "does the case statement filter.." in the form of a question.

must have missed the word lol

 

 

[yourichi]

You say you are not "looking for anything fancy like float values", but that is the validation you are doing now. By your explanation you want an integer value. So, use the correct validation: is_int().

 

heh just spent 30minutes trying to work out why the hell i couldnt validate correctly with is_int.

 

found the answer on good ol php.net lol

 

is_int — Find whether the type of a variable is integer

 

Report a bug

Description

 

bool is_int ( mixed $var )

Finds whether the type of the given variable is integer.

 

Note:

 

To test if a variable is a number or a numeric string (such as form input, which is always a string), you must use is_numeric().

 

kinda explains why it was always returning as invalid -.- so using numeric was correct after all -.-

 

[Psycho]

kinda explains why it was always returning as invalid -.- so using numeric was correct after all -.-

 

Not if you want to know if the value is an integer. Yes, I was incorrect in suggesting to use is_int() against a string that is supposed to represent an int value. But, that doesn't mean is_numeric() is the correct function to check for a string that should be an integer.

 

is_numeric() will return true for "1.2345" or "-123", which are not "valid" if you are looking for a positive integer. Typically, the validation is to ensure the value is a positive (non-zero) integer. So, one solution is to use int_val() to get the integer value or use "(int) $value" to convert the value to an integer. Then test that the value is greater than 0. Or, there is also ctype_digit() which wil validate that every character in the string is a digit (does not allow neg sign or decimal).

[yourichi]

ooo ctype why didnt i think of that! >.<!

 

anyhow going back on topic of the sql injection etc, a standard filter using mysql real escape should be sufficient combined with standard data validation ensuring that only the limited answers are accepted and else error everything else so no action is taken upon any dodgy input?