Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have a small function for filtering data from users. I would be grateful if someone could show me what logic mistakes I made and if I'm somewhat protected using it. And sorry for bad english i'm using gtranslate.

public static function filtru($str, $filtre = []){

    if(is_array($str)){//verifica daca datele trimise spre curatare sant array
        $str_curat = [];
        foreach($str as $key => $value){//selectam fiecare valoare din array-ul trimis spre curatare
            foreach($filtre as $fitru){//aplicam fiecare filtru, daca se folosesc mai multe

                switch($fitru){

                    case 'trim':
                        $value = htmlentities(trim(strip_tags($value)), ENT_QUOTES, 'utf-8');
                        break;

                    case 'int':
                        $value = intval($value);
                        break;

                    case 'nu':
                        $text = $value;$striptags = true;
                        $search = ["40","41","58","65","66","67","68","69","70",
                            "71","72","73","74","75","76","77","78","79","80","81",
                            "82","83","84","85","86","87","88","89","90","97","98",
                            "99","100","101","102","103","104","105","106","107",
                            "108","109","110","111","112","113","114","115","116",
                            "117","118","119","120","121","122"
                        ];
                        $replace = ["(",")",":","a","b","c","d","e","f","g","h",
                            "i","j","k","l","m","n","o","p","q","r","s","t","u",
                            "v","w","x","y","z","a","b","c","d","e","f","g","h",
                            "i","j","k","l","m","n","o","p","q","r","s","t","u",
                            "v","w","x","y","z"
                        ];
                        $entities = count($search);
                        for ($i=0; $i < $entities; $i++) {
                            $text = preg_replace("#(&\#)(0*".$search[$i]."+);*#si", $replace[$i], $text);
                        }
                        $text = preg_replace('#(&\#x)([0-9A-F]+);*#si', "", $text);
                        $text = preg_replace('#(<[^>]+[/\"\'\s])(onmouseover|onmousedown|onmouseup|onmouseout|onmousemove|onclick|ondblclick|onfocus|onload|xmlns)[^>]*>#iU', ">", $text);
                        $text = preg_replace('#([a-z]*)=([\`\'\"]*)script:#iU', '$1=$2nojscript...', $text);
                        $text = preg_replace('#([a-z]*)=([\`\'\"]*)javascript:#iU', '$1=$2nojavascript...', $text);
                        $text = preg_replace('#([a-z]*)=([\'\"]*)vbscript:#iU', '$1=$2novbscript...', $text);
                        $text = preg_replace('#(<[^>]+)style=([\`\'\"]*).*expression\([^>]*>#iU', "$1>", $text);
                        $text = preg_replace('#(<[^>]+)style=([\`\'\"]*).*behaviour\([^>]*>#iU', "$1>", $text);
                        if ($striptags) {
                            do {
                                $thistext = $text;
                                $text = preg_replace('#</*(applet|meta|xml|blink|link|style|script|embed|object|iframe|frame|frameset|ilayer|layer|bgsound|title|base|body)[^>]*>#i', "", $text);
                            } while ($thistext != $text);
                        }
                        $value = $text;
                        $value = preg_replace('#(alert|cmd|passthru|eval|exec|expression|system|fopen|fsockopen|file|file_get_contents|file_put_contents|readfile|unlink|shell_exec)(\s*)\((.*?)\)#si', "\\1\\2&#40;\\3&#41;", $value);
                        break;


                    default:
                        $value = $fitru($value);
                }// switch
            }//bucla pt fiecare filtru
            $str_curat[$key] = $value;
        }//bucla pt fiecare valoare
    }else{
        foreach($filtre as $fitru){

            switch($fitru){

                case 'trim':
                    $str = htmlentities(trim(strip_tags($str)), ENT_QUOTES, 'utf-8');
                    break;

                case 'int':
                    $str = intval($str);
                    break;

                case 'nu':
                    $text = $str;$striptags = true;
                    $search = ["40","41","58","65","66","67","68","69","70",
                        "71","72","73","74","75","76","77","78","79","80","81",
                        "82","83","84","85","86","87","88","89","90","97","98",
                        "99","100","101","102","103","104","105","106","107",
                        "108","109","110","111","112","113","114","115","116",
                        "117","118","119","120","121","122"
                    ];
                    $replace = ["(",")",":","a","b","c","d","e","f","g","h",
                        "i","j","k","l","m","n","o","p","q","r","s","t","u",
                        "v","w","x","y","z","a","b","c","d","e","f","g","h",
                        "i","j","k","l","m","n","o","p","q","r","s","t","u",
                        "v","w","x","y","z"
                    ];
                    $entities = count($search);
                    for ($i=0; $i < $entities; $i++) {
                        $text = preg_replace("#(&\#)(0*".$search[$i]."+);*#si", $replace[$i], $text);
                    }
                    $text = preg_replace('#(&\#x)([0-9A-F]+);*#si', "", $text);
                    $text = preg_replace('#(<[^>]+[/\"\'\s])(onmouseover|onmousedown|onmouseup|onmouseout|onmousemove|onclick|ondblclick|onfocus|onload|xmlns)[^>]*>#iU', ">", $text);
                    $text = preg_replace('#([a-z]*)=([\`\'\"]*)script:#iU', '$1=$2nojscript...', $text);
                    $text = preg_replace('#([a-z]*)=([\`\'\"]*)javascript:#iU', '$1=$2nojavascript...', $text);
                    $text = preg_replace('#([a-z]*)=([\'\"]*)vbscript:#iU', '$1=$2novbscript...', $text);
                    $text = preg_replace('#(<[^>]+)style=([\`\'\"]*).*expression\([^>]*>#iU', "$1>", $text);
                    $text = preg_replace('#(<[^>]+)style=([\`\'\"]*).*behaviour\([^>]*>#iU', "$1>", $text);
                    if ($striptags) {
                        do {
                            $thistext = $text;
                            $text = preg_replace('#</*(applet|meta|xml|blink|link|style|script|embed|object|iframe|frame|frameset|ilayer|layer|bgsound|title|base|body)[^>]*>#i', "", $text);
                        } while ($thistext != $text);
                    }
                    $str = $text;
                    $str = preg_replace('#(alert|cmd|passthru|eval|exec|expression|system|fopen|fsockopen|file|file_get_contents|file_put_contents|readfile|unlink|shell_exec)(\s*)\((.*?)\)#si', "\\1\\2&#40;\\3&#41;", $str);
                    break;



                default:
                    $str = $fitru($value);
            }// switch
        }//foreach pt filtre
        $str_curat = $str;
    }
return $str_curat;
}//end filtru

LE: If i try to separate the logic into 2 functions like this it's better? Each applied filter will be a separate function.

/**
 * @param $str
 * @param array $filtre
 * @return array|sanitized
 */
public static function filtru($str, $filtre = []){
   if(is_array($str)){
       for($i = 0, $j = count($str)-1; $i <= $j; $i++){
           $str[$i] = self::filtre($str[$i], $filtre);
       }
   }else{
       $str = self::filtre($str, $filtre);
   }
    return $str;
}
/**
 * @param $str
 * @param array $filtre
 * @return sanitized str
 */
private static function filtre($str, $filtre = []){
    foreach($filtre as $filtru){
        if(in_array($filtru, self::$filtre_str)){
            $str = call_user_func(['clean',$filtru], $str);
        }else{
            die('Function '.$filtru.' it's not defiend.');
        }
    }
    return $str;
}
share|improve this question
2  
This function isn't small; by modern standards, it's huge and should be split up. –  codesparkle Nov 11 '12 at 23:12
add comment

3 Answers

  • Your function should be broken into many more functions. Each different type of escaping should be its own function.
  • You should either use separate functions where the array version calls the non-array version, or youo should try to fold your array and non array logic into one
    • $wasArray = is_array($str); if (!$wasArray) { $str = array($str); } foreach ($str as $s) { ... } return ($wasArray) ? $str : array_shift($str);
  • Your trim method is a lie -- it doesn't just trim. What if you want to trim without html escaping?
  • If you did leave them all in one method, consider using class constants instead of strings to control the behavior. That's a lot less error prone, and it makes it easier to trace usage through code (and if you rename something later, it will be a million times easier).
  • What is your nu sanitation doing? It's like it's processing HTML, PHP and JavaScript?
    • With HTML, you're typically better off with a whitelist than trying to sanitize, and then escaping everything else.
    • With PHP, you're typically better off just not running code that you don't 100% trust
    • JS falls into the HTML category. If HTML is whitelisted, it should be impossible for a user to get JS through.

You should consider if you really need all of this. In a lot of situations, it's sufficient to simply escape and not sanitize (though one could argue that escape is a form of sanitizing).

(There are of course situations where you really do have to process the hell out of user input though. Consider the StackExchange questions/answers/comments. They allow a very limited subset of HTML which means that their processor has to be aware of what to escape, what to leave alone.)

share|improve this answer
    
I will remove each filter as a separate function, i avoid using constants knowing that they are slower. Nice explanation with array_shift i'll use it.I need to rethink all logic. Thank you very much for response –  danutz0501 Nov 12 '12 at 10:43
add comment

DRY - Write less handle more

You can make your function half the size by designing it to work with scalar vars only, and handling arrays with a loop:

public static function filtru($var) {
    if (is_array($var)) {
        foreach ($var as &$val) {
            $val = static::filtru($val);
        }   
        return $var;
    }   

    // $var is not an array - it is number/string etc.
    ...

    return $var;        
}

In the above example, an array, a scalar or even a nested array can all be handled by the same function.

Use PHP's built in filters

Rather than write your own filters, you're better off using PHP's sanitization filters. For common cases, your code becomes therefore (assuming this is implemented):

$arrayIntOut = Foo::filtru($arrayMixedIn, FILTER_SANITIZE_NUMBER_INT);

But, especially with the above example, it's more work to use the filtru function than it is to just write:

$arrayIntOut = array_map('intval', $arrayMixedIn);

Putting all your logic in one place is not necessarily the best idea, when the alternative is to write concise (and more efficient) code using php's function directly in functions. As such, consider 'just' using PHP instead of wrapping php functions in some more logic.

Make "nu" a separate function

Each case you want to handle should really be a protected specific function. Once you do that, consider deleting any functions that are just one line of code as it's better to just use php's own functions. The 'nu' function however, is likely to warrants it's own function.

It looks like the main purpose is to strip out XSS attacks. Unless you have exhaustive tests for this logic, it is very likely it doesn't cater for all possibilities - therefore make it a separate function and if you haven't already it'd be in your interest to write some tests to at least cover the most common XSS attacks.

share|improve this answer
    
that's a really long list :D and it's just the common ones –  danutz0501 Nov 12 '12 at 11:04
    
actually it's probably quite complete being owasp, but there can never truly be a complete xss list as new attacks are found/invented as time goes by. –  AD7six Nov 12 '12 at 11:47
add comment
  • First, break your function into pieces. One function should be doing one thing and correctly.
  • Try to give functions, variables a meaningful name. Here the function name "filteru" doesn't make any sense. same goes for "$str_curat".
  • you have huge if block
    if (is_array($str)) {
     // lot of code...
    }
    
    Instead you can write a simple check at the beginning of the function
    if (!is_array($str)) { return; } 
    // rest of the code goes here...
    
    This way you can avoid a huge if block.
  • At the end of the curly braces you have comments like
     }// switch 
     }//foreach pt filtre 
     }//end filtru 
    These are useless.
  • you have two foreach blocks. one foreach inside another foreach
    foreach($str as $key => $value) {
     foreach($filtre as $fitru) {
      // code ...
     }
    }
    
    Try rewriting the logic so that you can avoid this nested loop.
share|improve this answer
1  
The variable names and comments do make sense in romanian. –  Diego Agulló Nov 12 '12 at 7:18
1  
Then It is fine if the variable names make sense in romanian. For the comments, what I wanted to say is that it is useless to comment the end of curly braces the way it is done here - "} //switch", "}//foreach pt filtre" etc –  Kinjal Nov 12 '12 at 7:30
    
I used a poorly text editor when i made the function. It didn't had matching curly braces. I forgot to remove comments I'll rethink the function(logic). str_curat = clean string –  danutz0501 Nov 12 '12 at 10:50
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.