Escaping ORDER BY clause

Question

Possible Duplicate:
mysql_fetch_array() expects parameter 1 to be resource, boolean given in select

i need some help here.

I have this query:

$order = isset($_GET['order']) ? mysql_real_escape_string($_GET['order']) : 'title';
$query = mysql_query("SELECT * FROM entry ORDER BY $order ASC");

You can either order by title, date or author.

But if someone gives $order something else it goes:

Warning: mysql_fetch_assoc() expects parameter 1 to be resource, boolean given in C:\wamp\www\entries.php on line 20

How do I get rid of this error message?

score 7 · Answer 1 · answered Jul 14 '10 at 15:53

You cannot have something like colu\'nname in an order by clause -- which means mysql_real_escape_string is not the solution here.

Instead, you should check that $order actually contains the name of one of the columns of your table -- and not run the query if it doesn't.

For example, you could use something like this :

if (!in_array($_GET['order'], array('column1', 'title', 'id', 'other_column'))) {
    // deal with the problem
    // and don't run the query
    // because $_GET['order'] is not one of the allowed values
}

score 2 · Answer 2 · answered Jul 14 '10 at 15:52

Just not offer any order options to the user, that doesn't. If you are using a text input, replace it with a select. As long as the column exists, you shouldn't get an error.

You might hardcode each option in a switch-case:

switch($_GET['order']){
    case 'date' : 
         $order = 'date';
         break;
    case 'author' : 
         $order = 'author';
         break;
    default
         $order = 'title';
}

That will also prevent SQL-Injections.

score 2 · Accepted Answer · answered Jul 14 '10 at 16:03

You shouldn't treat column identifiers the same as string literals. In your example, you could end up with SQL like this:

SELECT * FROM entry ORDER BY O\'Hare ASC

Which would result in a syntax error in any SQL parser.

I have a few tips:

Put your SQL into a variable, don't try to build it inside the call to mysql_query(). If you use a variable, you can now inspect the SQL string, which makes errors like the above easier to catch.
```
$sql = "SELECT * FROM entry ORDER BY $order ASC";
// here you can log $sql, or output to Firebug, etc.
$query = mysql_query($sql);
```
Check that the return value does not indicate an error. You need to check for error states, because they can occur for many reasons.
```
$query = mysql_query($sql);
if ($query === false) {
  die(mysql_error());
}
```

Use mysql_real_escape_string() for strings -- not column names, table names, SQL keywords, etc. What I use instead is an associative array that maps the $_GET input to a valid column name, so I know it's safe. This also allows you to use different values in your app parameters than the names of columns.

$ordercolumns = array(
  "t" => "title",
  "d" => "date"
);
$order = "title"; // the default
if (isset($_GET["order"]) && isset($ordercolumns[$_GET["order"]])) {
  $order = $ordercolumns[$_GET["order"]];
}
// now we know $order can only be 'title' or 'date', 
// so there's no need to escape it.
$sql = "SELECT * FROM entry ORDER BY $order ASC";

Great answer! I learned something new :) I will bookmark this ;P — Jolabero, Jul 14 '10 at 16:18

score 0 · Answer 4 · answered Jul 14 '10 at 15:52

0

It expects a column name in $order and if it isn't one it will fail. Try echoing the $query.

answered Jul 14 '10 at 15:52

Jan

2,295
1
17
16

score 0 · Answer 5 · answered Jul 14 '10 at 15:52

0

You should be checking the value of $_GET['order'] from a possible acceptable list of values.

You should also be checking if ($query) to determine if the query worked, before trying to fetch a row.

answered Jul 14 '10 at 15:52

Fosco

38,138
7
87
101

score 0 · Answer 6 · answered Jul 14 '10 at 15:53

0

$checker = array('title', 'date', 'author')

$order = isset($_GET['order']) ? mysql_real_escape_string($_GET['order']) : 'title';

if (in_array($_GET['order'], $checker) {
  $query = mysql_query("SELECT * FROM entry ORDER BY $order ASC");
}

answered Jul 14 '10 at 15:53

karlw

668
4
13

score 0 · Answer 7 · answered Jul 14 '10 at 15:54

You should check for valid values and present an error to the user if something isn't as you expect.

A simple example,

$valid_values = array("title", "date", "author");

if (in_array($_GET['order'], $valid_values))
{
    // Your db stuff
}
else
{
    echo "The order value you gave wasn't valid. Please try another.";
}

score 0 · Answer 8 · answered Jul 14 '10 at 15:54

You also need to make sure $_GET["order"] contains a valid column.
Your problem comes from getting an invalid column name, that you put in the query. The mysql_query() function return false on error, and your error message tells you passed it to a mysql_fetch_assoc call.

Try something like

$columns = array("id", "name", "title");
if (false === ($key = array_search($_GET["column"], $columns))) {
  $column = "title";
} else {
  $column = $columns[$key];
}
$query = "SELECT * FROM entry ORDER BY {$column};";

I didn't use mysql_real_escape_string because this method also checks the input to be in the list of valid options.

Josh K · Answer 9 · 2010-07-14T16:32:44.100

0

Check to see $order is a column name before you run the query

$order = isset($_GET['order']) ? mysql_real_escape_string($_GET['order']) : 'title';
//Run a check here, before the query
$query = mysql_query("SELECT * FROM entry ORDER BY $order ASC");

The mysql_real_escape_string also looks to be in an improper location. Just a heads up

edited Jul 14 '10 at 16:32

answered Jul 14 '10 at 15:54

Josh K

562
1
4
11

This code doesn't do any checking if the column name is valid... – Blair McMillan Jul 14 '10 at 16:13
I was just showing the OP where to do the checking. After you set the var and before you attempt a query... – Josh K Jul 14 '10 at 16:32

Escaping ORDER BY clause

9 Answers9