tags:

views:

53

answers:

5

I am having trouble making this seemingly simple MySql query work. Can anyone spot the problem?

<?php
include "config.php";

$offerid = $_POST["offerid"];
$ip = $_SERVER["REMOTE_ADDR"];

mysql_query("INSERT INTO voted (offerid,ip) VALUES (".$offerid.",".$ip.")");
?>
+5  A: 

You probably want some single quotes:

 "INSERT INTO voted (offerid,ip) VALUES ('" . $offerid . "','" . $ip . "')"

You should also use intval and mysql_real_escape_string to avoid SQL injection vulnerabilities:

 $sql = "INSERT INTO voted (offerid,ip) VALUES (" .
        intval($offerid). ", '" .
        mysql_real_escape_string($ip) . "')";

Another alternative which may be easier to read is to use sprintf:

 $sql = sprintf("INSERT INTO voted (offerid, ip) VALUES (%d, '%s')",
                $offerid, mysql_real_escape_string($ip));
Mark Byers
Just to add emphasis: if you do not use `mysql_real_escape_string` or prepared statements or something to prevent hacking, **you will be hacked**.
Matchu
@Mark Byers - should the single quotes be there if you're passing in an integer, anyway?
Matchu
@Matchu it is not hacking prevention. It's merely syntax.
Col. Shrapnel
@Matchu: It will work with single quotes round an integer, but they aren't necessary. I can remove them from the second example, but I'll leave them in the first because it's not explicitly clear that $offerid is in fact an integer.
Mark Byers
@Col. Shrapnel - I realize that, but "you will be hacked" is scarier than "you will have invalid syntax" to people who don't, and therefore increases the chances of implementation :)
Matchu
@Matchu Ah I see, it's social engineering at it's best :)
Col. Shrapnel
The query works now. Thanks! However, $offerid is allways set to 0, even though the offerid URL parameter is passed correctly. Any idea why?
Espen Arnoy
@Espen Arnoy: Is it an integer? Give an example of a possible value it can have.
Mark Byers
its allways an integer with max 4 digits (345,3435,644)
Espen Arnoy
@Col. Shrapnel - though I do remember someone who didn't want to do escaping because it would mess people up with quotes in their passwords, when really the opposite was true. So the syntax point *is* important.
Matchu
@Espen Arnoy - that's weird. `intval` usually turns things into `0` if they're *not* integers. Do they sometimes have leading spaces or something?
Matchu
Yes, there was a space in between. Thanks a million for your help!
Espen Arnoy
A: 

my guess would be with quotes

mysql_query("INSERT INTO voted (offerid,ip) VALUES (\"".$offerid."\",\"".$ip."\")");
Grumpy
+1  A: 

To place a string value into query, you must perform 2 actions on it:

  • enclose it in quotes

  • and escape special characters.

So, query must be like this:

INSERT INTO voted (text) VALUES ('I\'m a programmer')

Armed with this knowledge, you can easily write a code to make valid query:

$offerid = mysql_real_escape_string($_POST["offerid"]);
$ip = mysql_real_escape_string($_SERVER["REMOTE_ADDR"]);

$sql = "INSERT INTO voted (offerid,ip) VALUES ('$offerid','$ip')"
mysql_query($sql) or trigger_error(mysql_error().$sql);

Note the trigger_error part.
It will provide you with comprehensive information on any error

Col. Shrapnel
+1 for error handling.
Mark Byers
A: 
<?php
include "config.php";

$offerid = $_POST["offerid"];
$ip = $_SERVER["REMOTE_ADDR"];

mysql_query("INSERT INTO voted (offerid,ip) VALUES ('".mysql_real_escape_string  ($offerid)."','".mysql_real_escape_string  ($ip)."')");
?>

This adds the single quote marks around the strings you are inserting - as well as mysql_real_escape_string php function that will escape (add a backslash infront of) any security risk characters.

Matt Clements
A: 

In addition to using intval(...) and mysql_real_escape_string(...) you could use parameterized statements (or placeholders) using PEAR::DB or PEAR::MDB2:

$dsn = "mysqli://testuser:testpass@localhost/test";
$conn =& DB::connect ($dsn); // using PEAR::DB, though it's been superseded
if (DB::isError ($conn)) {
    die ("Cannot connect: " . $conn->getMessage () . "\n");
}

$result =& $conn->query ("INSERT INTO voted (offerid,ip) VALUES (?,?)", array($_POST["offerid"], $_SERVER["REMOTE_ADDR"]));
if (DB::isError ($result)) {
    die ("INSERT failed: " . $result->getMessage () . "\n");
}

Using placeholders and parameters is pretty common on platforms other than PHP, so it's not a bad idea to understand the basic premise behind them.

If you're interested in using DB modules like these, I'd recommend checking out Writing Scripts with PHP's PEAR DB Module by Paul DuBois. Again, the module it describes is superseded, but I find it's nonetheless interesting and informative.

LeguRi