1
\$\begingroup\$

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";
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

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.

\$\endgroup\$
2
  • \$\begingroup\$ So I dont have to escape the string before using it in a query? \$\endgroup\$
    – remy
    Jun 5, 2013 at 17:47
  • \$\begingroup\$ No. You have just made sure it contains nothing dangerous. Escaping wouldn't change the contents of the string anyway. \$\endgroup\$ Jun 5, 2013 at 17:56

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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