0
\$\begingroup\$

I'm trying to clean some incoming $_GET parameters. I've not done this before, so I'd love feedback.

I'm especially concerned with the array. While all the other parameters control simple logic, the array will be saved into the database and potentially output to users.

Please also feel free to point out any redundancies.

<?php
// int: only want a positive value
// array: wont know what this could be but it could be anything urlencoded
// string: three possible outcomes: foo, bar, fudge

( isset( $_GET["val1"] ) ? (bool) $val1 = true                              : (unset) $val1 );
( isset( $_GET["val2"] ) ? (int) $val2 = sanitize_absint( $_GET["val2"] )   : (unset) $val2 );
( isset( $_GET["val3"] ) ? (array) $val3 = sanitize_array($_GET["val3"])        : (unset) $val3 );
( isset( $_GET["val4"] ) ? (string) $val4 = sanitize_string($_GET["val4"])  : (unset) $val4 );

// further refine val4 string
val4_strip($val_4);


// sanitize
function gfahp_sanitize_absint( $int ) {
    $int = filter_var( $int, FILTER_SANITIZE_NUMBER_INT );
    $int = abs( intval( $int ) ); // positive number
    return $int;
}
function gfahp_sanitize_string( $string ) {
    // is this enough to prevent anything icky?
    $string = filter_var( $string, FILTER_SANITIZE_STRING, FILTER_FLAG_STRIP_LOW );
    return $string;
}
function gfahp_sanitize_array( $array ) {
     $array = array_walk_recursive( $array, "gfahp_sanitize_string" );
     return $array;
}

function val4_strip($val_4){
// strip down val4
    if (!empty($val4){
        switch ($val4) {
            case 'foo':
                $val4 = 'foo';
            break;
            case 'bar':
                $val4 = 'bar';
            break;
            case 'fudge':
                $val4 = 'fudge';
            break;      
            default:
            $val4 = (unset) $val4
            break;
        }
    }
    return $val4;
}
\$\endgroup\$
0
2
\$\begingroup\$

My 2 cents:

$int = abs( intval( $int ) ); // positive number

Silently fixing input values is evil, perhaps required, but still evil.

function gfahp_sanitize_array( $array ) {
     $array = array_walk_recursive( $array, "gfahp_sanitize_string" );
     return $array;
}

You assign and then return, I would just return immediately

function gfahp_sanitize_array( $array ) 
{
     return array_walk_recursive( $array, "gfahp_sanitize_string" );
}

Also, what is up with gfahp ? Hungarian notation of functions with acronyms is a slappable offence.

As for

function val4_strip($val_4){
// strip down val4
    if (!empty($val4){
        switch ($val4) {
            case 'foo':
                $val4 = 'foo';
            break;
            case 'bar':
                $val4 = 'bar';
            break;
            case 'fudge':
                $val4 = 'fudge';
            break;      
            default:
            $val4 = (unset) $val4
            break;
        }
    }

}

You should probably go for a generic function that can look up a value in a set and assign it if found, otherwise return blank. ( Also that function does not return anything? ) (Also you are unsetting var4 in places ). Something like

$val4_set = array( "foo" , "bar" , "fudge" );

( isset( $_GET["val4"] ) ? (string) $val4 = sanitize_string_from_set($_GET["val4"], $val4_set)  : (unset) $val4 );

function sanitize_string_from_set( $string , $set )
{
  return array_key_exists( $string, $set )?$string:""; 
}
\$\endgroup\$
6
  • \$\begingroup\$ Hungarian?, slapable? buuuuhahahahahah -- sorry I may have passed my Balmer peak. Wonderfull feedback truly many thanks! \$\endgroup\$
    – orionrush
    Aug 1 '13 at 19:02
  • \$\begingroup\$ As for $int = abs( intval( $int ) ); how would you do it differently? Mind you someone could try to submit a neg number and it would still fail. \$\endgroup\$
    – orionrush
    Aug 1 '13 at 19:10
  • \$\begingroup\$ I would catch it in the UI before submitting, if someone tries to hack I would check whether the value < 0 and throw an error in that case. \$\endgroup\$
    – konijn
    Aug 1 '13 at 19:12
  • \$\begingroup\$ good point re <0. No ui on this, its a service that returns GravityForms output via get params. Im just tring to keep script kiddies from imputing undeseriable XSS. \$\endgroup\$
    – orionrush
    Aug 1 '13 at 19:17
  • \$\begingroup\$ Oh just to round trip your comments re Hungairian Notaion, those are actually function prefixes - which are a good thing, no? \$\endgroup\$
    – orionrush
    Aug 2 '13 at 9:01

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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