Escaping MySQL Query issue

Question

I'm terribly bad at keeping MySQL queries straight, but that aside I have one query working for some data input, but not all. My guess is quotation marks getting escaped where they should be.

I have the entire query string get escaped at the same time. Is this bad practice or does it really matter?

Here's the query:

"INSERT INTO bio_manager_pubs(userid,category,citation,date,link,requests) VALUES ( ".     
$userid.",'".
$_POST['category']."', '".
htmlentities($_POST['pub'])."',
FROM_UNIXTIME(".strtotime($_POST['date'])."),'".
$_POST['link']."',
0)"

In query:

• Userid and requests are ints
• Link and Category are Tiny Text (not sure if that's appropriate, but max is 255 char, so would VarChar be better?)
• Date is a date (is it better to reformat with php or reformat with mysql?)
• Citation is a text field

Any ideas?

Thanks

EDIT: The answer to this question was posted four times there abouts where the issue was me escaping the entire query.

What was left out, and cause some confusion was the code surrounding the query. It was like this

$db->query($query)

This where the function query was:

public function query($SQL)
{
    $this->SQL = $this->mysqli->real_escape_string($SQL);
    $this->result = $this->mysqli->query($SQL);

    if ($this->result == true)
    {
        return true;
    }
    else
    {
        printf("<b>Problem with SQL:</b> %s\n", $this->SQL);
        exit;
    }
}

I just found a class that made life a bit simpler on smaller projects and stuck with it. Now, the issue I'm running into is removing $this->mysqli->real_escape_string($SQL); and adding in escapes elsewhere in the code.

Answer 1

I really don't see any sanitizing of your $_POST data, and there is really no need to run htmlentities before you insert into the database, that should be done when you take that data and display it on the page. Make sure to sanitize your posts!! Using mysql_real_escape_string() or preferably PDO with prepared statements.

If you are running mysql_real_escape_string() on this whole query, after you build it, than that is what is breaking it.

Use it on the individual posts, and / or cast variables that should only ever be numbers to integers.

Heres what I would change it to in your case:

$posted = $_POST;

foreach($posted as &$value)
    $value = mysql_real_escape_string($value);

$date = strtotime($posted['date']);


$q = "INSERT INTO bio_manager_pubs(userid,category,citation,date,link,requests) VALUES
(
'{$userid}',
'{$posted['category']}',
'{$posted['pub'])}', 
FROM_UNIXTIME({$posted['date']}),
'{$posted['link']}',
'0'
)";

Answer 2

I believe it is considered bad practice to build the entire query and then escape the whole thing. You should sanitize the inputs as soon as they enter the code, not after you've started using them to build your database interactions.

You'd want to sanitize each input, kind of like this:

$category = mysql_real_escape_string($_POST['category'])

And then you'd use the local variables, not the inputs, to build your SQL command(s).

Also, you may want to look into something like PDO for your data access, which manages a lot of the details for you.

Answer 3

I think you need to wrap each of your inputs in mysql_real_escape_string (only once!), not the whole query. Other than that it looks OK to me.

"INSERT INTO bio_manager_pubs(userid,category,citation,date,link,requests) VALUES ( ".     
mysql_real_escape_string($userid).",'".
mysql_real_escape_string($_POST['category'])."', '".
mysql_real_escape_string(htmlentities($_POST['pub']))."',
FROM_UNIXTIME(".mysql_real_escape_string(strtotime($_POST['date']))."),'".
mysql_real_escape_string($_POST['link'])."',
0)"

Answer 4

Instead of escaping the entire SQL query (which can run the risk of breaking things), just escape the user's input:

$userid = mysql_real_escape_string($userid);
$cat    = mysql_real_escape_string($_POST['category']);
$pub    = mysql_real_escape_string($_POST['pub']);
$date   = strtotime($_POST['date']);
$link   = mysql_real_escape_string($_POST['link']);
$query  = "INSERT INTO bio_manager_pubs(userid,   category,  citation,  date,   link,   requests)"
                             ." VALUES ($userid, '$cat',    '$pub',     $date, '$link', 0       );";

Answer 5

Well for a start you should avoid using data from external sources directly in a query, so I would rewrite the code so as not to use $_POST in your query. Even better if you can to use PDO or similar to escape your data. And I would avoid converting text with htmlentities before inserting it into your database. You're better off doing that after you pull it from the database as you will then be able to use that data in other (non-HTML) output contexts.

But in terms of inline code, do you have magic_quotes on?

Try something like this

if (get_magic_quotes_gpc()) {
    $category = stripslashes($_POST['category']);
    $pub = stripslashes($_POST['pub']);
    $link = stripslashes($_POST['link']);
} else {
    $category = $_POST['category'];
    $category = $_POST['category'];
    $category = $_POST['category'];
}
$category = mysql_escape_string( $category );
$pub = mysql_escape_string( $pub );
$link = mysql_escape_string( $link );
$sql = "
   INSERT INTO bio_manager_pubs(userid,category,citation,date,link,requests) VALUES (      
   ".  $userid.",
   '$category', 
   '$pub',
   FROM_UNIXTIME(".strtotime($_POST['date'])."),
   '$link',
   0
)";

Answer 6

Turn off magic_quotes_gpc and use prepared statements.

With magic_quotes_gpc disabled, you don't end up with automatic escaping of input - and magic_quotes_gpc is deprecated anyway.

Use parameter binding prepared statements to avoid SQL injection rather than escaping characters. I personally suggest using PDO or MDB2 to talk to your db, but you can also do prepared statements with the mysqli driver. Note that the mysql driver is on the chopping block as well, so you soon will be forced to either use mysqli or an abstraction layer like MDB2.

I bet though that magic_quotes_gpc is your problem.