current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite

Is it? And maybe theres a better way to do this?

$allowed = array("name_desc", "name_asc", "level_desc", "level_asc", "vocation_desc", "vocation_asc");
$order = isset($_GET['order']) ? $_GET['order'] : "name";
$order = in_array($order, $allowed) ? str_replace("_", " ", $order) : "name";


$query = "SELECT * FROM players 
          WHERE status = 0 AND group_id < 3 ORDER BY $order";
share|improve this question

1 Answer 1

up vote 1 down vote accepted

It is safe. You have a white list of allowed values and ensure that the user provided input is on the list.

But I'd still do the white list checking as the very first thing, and then do the other processing later, so it would be:

$field = 'name';
$direction = 'asc';
if(preg_match('^([a-z]+)_(asc|desc)$', $_GET['order'], $matches)) {
    if(in_array($matches[0], array("name", "level", "vocation"))) {
        $field = $matches[0];
        $direction = $matches[1];
    }
}

What happens here is that the block splits up and validates the input. You can be sure that once you're past this block, $field and $direction can be trusted. An important point is to use $_GET['order'] exactly once, since accessing it multiple times may lead to errors that will leak non-validated data.

share|improve this answer
    
So I dont have to escape the string before using it in a query? – remy Jun 5 '13 at 17:47
    
No. You have just made sure it contains nothing dangerous. Escaping wouldn't change the contents of the string anyway. – Michael Zedeler Jun 5 '13 at 17:56

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.