Good Code or Awful Hack?

[createErrorMsg]

I need to strip the slashes out of many variables being passed between multiple forms. Unfortunately, one of those variables is an array, so the script was choking on the array to string conversion. To work around this, I coded the script to...

(a) copy the array from $_POST into a variable.
(b) set the array in $_POST to an empty string.
(c) strip the slashes from everything in $_POST and copy to a portable array.
(d) copy the variable from (a) into a slot in the portable array from (c).
(e) send the portable array on it's way.

This worked. No errors. The slashes are gone. The variable and array values are moving.

But I'm afraid I'm not experienced enough with PHP to know whether this is a stable/reliable/safe way to handle this situation. Are there easier or better ways to accomplish this using one of PHP's many functions of which I am blissfully unaware?

Here's the code for the function. Thanks in advance for any replies.

function fetch_postvalues() {
$cats = $_POST['cats'];
$_POST['cats'] = '';
$entry_values = get_magic_quotes_gpc()? array_map('stripslashes', $_POST) : $_POST;
$entry_values['cats'] = $cats;
return $entry_values;
}

cEM

[mincklerstraat]

I'd just ditch making this a function and use the single line:

$entry_values = get_magic_quotes_gpc()? array_map('stripslashes', $_POST) : $_POST;

This basically just asks the question, is magic quotes on? If it is, it sets $entry_values with $_POST that's been stripped of its slashes; if not, it just sets it to $_POST.

The reason you do something like this is for code portability. If for whatever reason magic quotes are turned on or off, your code will still work.

The reason I'd just use the line instead of the function is basically, why make a function out of it? With the function you do have the added value of the 'original' $_POST array there as one of the elements. But under what conditions would you really need it? I end up adding stuff I don't need to my code all the time, just adds a bit to the bloat. Plan on using the conditionally stripped version all the time. Yeah, you have to add slashes again when making queries, but I like to make sure I always, always add slashes to queries when it's coming from relatively unchecked user input. You could check again to see if magic quotes are on, and if they are, use the unstripped version in queries, but there you have an extra level of complexity on top of your script. Also, making this a function gives you that tiny extra bit of overhead/processor time - why bother when you can settle for just one line?

Another potential problem with this function / the line of code: it'll choke if you've got an array in your $_POST variables. That *is* a situation in which you might want to have a function. Lessee, digging out something similar from a recent script ...

function conditional_strip_slashes($var, $level=0, $type=''){ 
  static $set; 
  if(!isset($set)) { 
     if(!get_magic_quotes_gpc()) return $var; 
     $set = 1; 
  } 
  $maxlevel = 10; 
  if($level > $maxlevel) die('exceeded maxlevel on '.__FILE__.': '.__LINE__); 
  if(is_array($var) & $type == 1) die ('var can\'t be an array in this case'); 
  if(is_array($var)){ 
     foreach($var as $k => $v) { 
       $var[(stripslashes($k))] = conditional_strip_slashes($v, $level + 1); 
     }  
  }  
  else { 
     $var = stripslashes($var); 
  } 
  return $var; 
} 
 

Here's a function that I have in a script I'm writing now - I didn't really think much about it, if I had, I probably would have made it less complicated and thus more efficient, it's a little over-built for what it's supposed to do.

You use it like:

$stripped = conditional_strip_slashes($_POST);

That third parameter is a bit of bloat that I never use that lets you make sure something's not an array, if you really can't have an array there. Like if you have only room in the inn for one cat, and you want to make sure your neighborhood cat lady isn't trying to sneak in a whole array of cats -

$strippedcat = conditional_strip_slashes($_POST['cat'], 0, 1);

Our neighborhood cat lady doesn't have an internet connection, so I still have never bothered with this 'feature.'

If you have an array in $_POST and you feed it to this function, it won't matter, as long as it doesn't go into 10 levels of keys. It just recurses.

If you have big, huge honking $_POST arrays, it'd be worthwhile changing this to make it more efficient, otherwise 'it just works'.

For stripping arrays in $_POST, it really does make sense to have a function (since this lets you recurse), unless you do the stripping 'manually' by setting up a foreach for your $_POST array. In most cases, though, you won't have any arrays inside of $_POST, so the few times you do, you can just 'manually' foreach that one element and strip slashes as necessary.

So probably if I were you, I'd just go with that one line, unless I was making a form with an array inside of $_POST, and then either use this function or else strip slashes inside of a loop.

[edited by: ergophobe at 7:22 pm (utc) on Feb. 5, 2005]

[ergophobe]

Same idea, but without limiting recursion levels

function strip_post_array($array) 
{ 
  if (!get_magic_quotes_gpc()) 
  { 
    return $array; 
  } 
  if (is_string($array)) 
  { 
    return StripSlashes($array); 
  }  
  $new = array(); 
  foreach ($array as $key => $val) 
  { 
    if (is_array($val)) 
    { 
      $new[$key] = strip_post_array($val); 
    } 
    elseif (is_string($val)) 
    { 
     $new[$key] = StripSlashes($val); 
    } 
  } 
  return $new; 
} 

This calls the get_magic_quotes_gpc() function more times, but is this a huge amount of overhead? It only calls it when it recurses, so I don't think it's a bid deal compared with the overall overhead of recursing.

The is_string() test is not strictly necessary if you know that only POST data will be sent, since ultimately it's all strings, but if you forget later and send an object to the function, it's nice to check.

If you want to be able to process objects by design, you would need another condition that would check for objects and go through them with get_object_vars() I guess.

Limiting number of recursion levels would be just a matter of adding a flag as Minck did. If you have an infinite recursion problem though, it won't show up here, it would show up with a timeout or overflow problem when you create the array, so I think the level checking is redundant here - it should be done and need only be done when the array is created. If it were possible for a user to send you an array directly, that would be another issue.

As for the third flag, I don't think that really belongs here. This function should strip slashes from all array elements. Whether or not an array is allowed under certain circumstances or not is a job that properly belongs elsewhere, no?

[edited by: ergophobe at 7:44 pm (utc) on Feb. 5, 2005]

[createErrorMsg]

One of the $_POST values is an array, but it's not an array that will ever need stripping (all values in the array are returned from a series of input checkboxes whose values are integers). Would you recommend using a stripping function in that case, or is it acceptable to just pull the array out of $_POST and strip everything else?

cEM

[mincklerstraat]

cEM>> If you have an array that's all integers, it depends really on what you'll be doing with it. Even if all the options that can be selected are integers, it's still possible for nasty users to forge requests with different values, and get you all screwed up, so even user input that's supposed to be integers has got to be checked.

When the day's over, optimization for speed and processor time only makes so much sense. Is this a page that's going to be hit hard, lots of visitors all the time? The page is a major contributor to your server load? Then it makes sense to shave off a few milliseconds here or there. Is the array just garagantually huge? Are you having users fill out forms that contain thousands or tens of thousands of bits of info? Then there's a chance that some optimization here might shave off some time that's large enough to be significant. But then maybe not. Just looping through a few thousand entities to strip slashes isn't something likely to slow down PHP all that much, even in a recursive function. In most cases, I'd guess, if 'it just works' here, go for the option that works. So don't bother copying the array and having one branch that's not checked - not worth your time and effort.

Ergophobe's completely right about the function I posted - level not really needed, and neither is the 'type' check to see if it's just a simple non-array varaible. In some script it might possibly come in handy, but I've never used it. This function is also an example of misguided speed optimization. Creating and seting a static variable just to see if magic quotes is on just isn't worth the programmer's time to avoid calling get_magic_quotes() each time. I'd guess that this would save no time at all, or at most a few microseconds for a huge POST array. The simpler option is preferable. Ditto for the recursion level check. I put level checks in every recursive function I write as a matter of habit, just to make sure things don't go apescheiss. But here's one instance where you really don't need to check for recursion at all, if it's a POST variable. If somehow you end up mistakenly feeding it GLOBALS, well then you would have a problem. But writing a function that's safe for even such an unlikely eventuality is really 'over-coding.'

That said, I knew right after I wrote this function that all this extra business was really a bit of paranoid lunacy. But it works, and removing the extra stuff for the script's sake would also have been wasted time, since it works anyways and no time would be saved in cleaning it up.

But for the sake of this board, where others might copy it, or try to learn from it, ergophobe is quite right to clean out the bloat - why put that stuff in if you don't need it? Ergophobe's function also have a feature which to me does make sense in terms of speed optimization.

foreach ($array as $key => $val) 
 { 
  if (is_array($val)) 
  { 
   $new[$key] = strip_post_array($val); 
  } 
  elseif (is_string($val)) 
  { 
   $new[$key] = StripSlashes($val); 
  } 
 } 

When my function hits an array, it calls itself for each element. Calling a function is something which takes a bit of overhead, and this is something that gets repeated for *every* last element. However, in ergophobe's version, if the element isn't an array, it just does strip_slashes instead of calling the larger function. This does indeed make sense.

[createErrorMsg]

mincklerstraat, thanks again for the lucid explanation. Considering your points, I think it's smart to check everything for a possible need to strip. I also found that pulling the array out of the larger array, just to avoid stripping it, was getting tiresomely complicated. When all was said and done, between stripping slashes for html output and adding slashes for queries, my workaround was using more lines of code than ergophobe's function(s).

I'm using the function to strip everything (and another to addslashes) and it's working great.

Thanks to you both!

cEM

[dmmh]

mysql_real_escape_string() is safer then addslashes() afaik :)

[ergophobe]

In fact, if I really want to make sure the data goes through mysql_real_escape_string() instead of the regular AddSlashes() via magic_quotes or not, what I usually do is escape data in queries using

function db_safe_escape_string($string)
{
if (get_magic_quotes_gpc())
{
$query_string = StripSlashes($string);
}
return mysql_real_escape_string($string);
}

and I just use it as

"SELECT field1 FROM table1 WHERE field1 LIKE '" . db_safe_escape_string($where_ value) . "'";

No need to go through the whole array.

[mincklerstraat]

My solutions usually depend on what kind of script I'm writing, and a little bit, what mood I'm in. I'd like to 'standardize' this sometime since if you know, 'I always write my code like this', you don't waste time on those tiny decisions, and when you go back to your old code you don't have to double-check. Until now, the little variances in situations has left me using different options - but really, the extra efficiency I get out of using differing options is probably negligible.

For example, one I'm writing now - I can ask the user to stick to alphanumeric characters and - _ , I just check at the beginning of the script if the vars match what's asked. It's not for a site, so I don't have to super-friendly up the users - if output is bad at the beginning, you just get an error message and a link back to the form. What's nice about this is for the rest I can just use the $_POST vars as $_POST and this is a nice reminder that these are unmodified user input that's passed the filter. Also, no extra memory consumption from setting extra variables (as if that 0.000001% of memory added is any big deal).

What I usually do in scripts where being nice to the user is more important is something like this:

$desiredvars = array('this parameter', 'thatparameter', 'someotherparameter'); 
if(magic_quotes_gpc()){ 
foreach($desiredvars as $v){ 
if(isset($_POST[$v])) $$v = stripslashes($_POST[$v]); 
} 
} else { 
foreach($desiredvars as $v){ 
if(isset($_POST[$v])) $$v = $_POST[$v]; 
} 
} 

When I add a variable to the form, it's just a matter of adding it to the array $desiredvars, no work at all. Won't work of course with arrays as $_POST elements.

Probably using some kind of naming convention would be a good compromise - just always name my desiredvars things like $p_name, $p_address etc.

I like this solution since I really *always* want my user variables back in the form they were input - I never want the slashes, and add slashes on the spot with the appropriate function. Otherwise I can get those slashes in form values, etc. Yes, dmmh, quite right about mysql_real_escape_string when it's for mysql.