What would be a better way of doing the following

Question

if(get_magic_quotes_gpc())
{
  $location_name = trim(mysql_real_escape_string(trim(stripslashes($_GET['location_name']))));
}
else
{
  $location_name = trim(mysql_real_escape_string(trim($_GET['location_name'])));
}

That's the code I have so far. seems to me this code is fundamentally ... OK. Do you think I can safely remove the inner trim(). Please try not a spam me with endless version of this, I want to try to learn how to do this better.

UPDATE

So after reading some of the responses, I think I have come to understand a good method for safely getting data from a user, storing it and then displaying it back.

When you first load the page

$foo = trim($_GET['foo']);
if(get_magic_quotes_gpc())
{
  $foo = stripslashes($foo);
}

Then when you come to use this variable as part of a SQL string, even if not storing the data in the database, you should escape it.

 mysql_real_escape_string($foo);

And finally, if reading data from the database and wanting to display it as HTML, such a post on a blog or forum, you should pass the variable using htmlspecialchars

echo(htmlspecialchars($bar));

Would any one like to suggest a better set of functions to use? other then obviously wrapping these functions to make them simpler to call.

Answer 1

Here:

$location_name = trim($_GET['location_name']);
if(get_magic_quotes_gpc()) $location_name=stripslashes($location_name);

Then there is also the SQL injection protection, but don't do this until the very last moment before sticking this var in an SQL query. And even then don't apply the changes to the var itself, but rather a copy. You might want to show $location_name to the user afterwards (for example if the form fails). So

$sql="UPDATE whatever(location) VALUES('" . mysql_real_escape_string($location_name) . "')"

I'm assuming of course that $location_name will end up in the database; otherwise you don't need mysql_real_escape_string.

Finally you want to use htmlspecialchars if you're going to display $location_name on your page somewhere.

Edit: You want to use htmlspecialchars() just before displaying the data (definately don't save data that has already been transformed via htmlspecialchars in your database). In general you want to use escaping functions at the last moment and then on a copy of your var. That way you know that at any point during the script the var is the original one and is not carrying some random escape characters from a transformation that happened somewhere before.

You also know where your escape functions are/should be. sql escaping is near/at your sql query. XSS escaping (htmlspecialchars) is near the part where you display data in a web page.

Finally once you get the grip of things, you could always forego SQL escaping by using PHP's PDO functions. Also in the future you might want to take at look at this: http://stackoverflow.com/questions/110575/do-htmlspecialchars-and-mysql-real-escape-string-keep-my-php-code-safe-from-injec

Answer 2

I am sorry to say but everything in your question is wrong.

First, it has nothing to do with performance, by any means. these functions never become a bottleneck and never cause any performance issue.

Next, You've choose wrong place to get rid of magic quotes. Magic quotes is input data related, not database related. It it is better to make a distinct function and place it in your configuration file, being included into every script. You can use one from here

So, the code become like this:

$location_name = mysql_real_escape_string(trim($_GET['location_name']));

But i strongly advise you not to mix database escaping with anything else, as anything else is optional while database escaping is strict and unconditional.