2

This bit of code is not working for me. My list.php shows all the members. If I click on edit for a row I goto my update.php which shows 'update.php?id=# and I have a small piece of code to display the number which is always the correct one. But I cannot display the persons info. Here is what I have

<?
    $id = $_GET['id'];
     echo "<print>" .$id."</print>";
    mysql_query("SET NAMES utf8"); 
    mysql_query("SET CHARACTER_SET utf8");
    header('Content-type: text/html;charset=utf-8');   
    $order = 'SELECT * FROM `person` WHERE `PersonID` = $id';  
    $result = mysql_query($order);
?>

It should display the persons info in my table as it dose in my list.php but nothing shows up? If I replace 'PersonID' = 1234 for example, I then get some info. Why is the code not using $id and placing it after 'PersonID' = .?

tadman
  • 208,517
  • 23
  • 234
  • 262
Bobby
  • 23
  • 5
  • 4
    PHP's MySQL library is deprecated, so you should really be learning to use MySQLi or PDO with prepared statements – Mark Baker Jul 23 '13 at 17:48
  • 1
    Your (resulting) script may be vulnerable to SQL injections. You should [do something against it](http://stackoverflow.com/q/60174/53114). – Gumbo Jul 23 '13 at 17:49
  • You can remove the backticks from your query... you are not using any reserved word – Barranka Jul 23 '13 at 17:51

3 Answers3

7

You're passing the contents of $_GET['id'] directly to the database - what if some one passes in a variable of 1'? At best that will break the query; at worst, someone can do nasty things to your database. At the least, you need to sanitise your data.

mysql_real_escape_stringwill let you escape a string so you can do this; ifPersonID` is an int, you can also use intval to get the integer value of a string, and stop invalid characters being passed that way.

The actual problem you're seeing is because PHP treats a string with single quotes as a string literal; it doesn't parse it to see if there are variables that need replacing. It's treating $id as a string. To do what you're trying to do, you need to use double quotes:

$id = intval($_GET['id']);
$order = "SELECT * FROM `person` WHERE `PersonID` = $id";

You should also look at moving away from mysql_* functions, as they are being deprecated. To avoid that, look at moving to mysqli_* or PDO instead. Both of those will help you write code that's considerably more secure, by using prepared statements and binding variables to them.

andrewsi
  • 10,807
  • 132
  • 35
  • 51
  • Without `mysql_real_escape_string`, this code is career-destroyingly bad. – tadman Jul 23 '13 at 17:54
  • Or he could stop using mysql_, as the last half of the answer points out. – andrewsi Jul 23 '13 at 17:56
  • 1
    WOW....OK this is very useful information... and it worked as suggested. I see why this could be a security problem. If only one person has access to this page is it still a big issue? About the $_GET['id'] and using mysqli_* or PDO this is new to me. Could you suggest where I might learn about this? – Bobby Jul 23 '13 at 18:00
  • @andrewsi I know you say that, but it's easily ignored, and your code is **dangerously** bad. At the very least it needs to be escaped. – tadman Jul 23 '13 at 18:02
  • WOW... Very good advice. I will look into this. Your answer and others worked as suggested. Where is the best place to get info on mysqli_* or PDO? – Bobby Jul 23 '13 at 18:03
  • Moving to mysqli won't make your insecure code any more secure - **any** DB library will allow you to execute arbitrary SQL, and therefore cannot stop you adding unescaped variables to that SQL. For heaps of information on this, see http://stackoverflow.com/questions/60174/how-to-prevent-sql-injection-in-php?lq=1 – IMSoP Jul 23 '13 at 18:04
  • The PDO manual is at: http://php.net/manual/en/book.pdo.php - it's a little scary at first, but it's definitely worth using PDO. The problem with assuming that it's one user accessing the page is that you can't guarantee that no one else will ever access it. If the page accidentally gets index by google, then anyone might stumble across it. – andrewsi Jul 23 '13 at 18:04
  • 2
    Thanks guys..I found the code on the web and it appears that it old school.. Just learning here and all comments are appreciated. #Starting over.. – Bobby Jul 23 '13 at 18:07
  • 1
    @tadman - that's an excellent point. I do tend to assume that people will read everything, instead of just skimming. I've re-done the answer and bumped the scary part to the top. – andrewsi Jul 23 '13 at 18:11
  • I have removed all the files from the server and will start over. I had the following error popup the other day and I want to know what folks think. – Bobby Jul 23 '13 at 18:33
  • ERROR MSG: Your PHP MySQL library version 5.0.51a differs from your MySQL server version 5.1.61. This may cause unpredictable behavior. Could this be an issue for me? – Bobby Jul 23 '13 at 18:33
  • @Bobby Please ask other unrelated questions as a separate thing. – tadman Jul 23 '13 at 19:16
2

Variables within strings that are single-quoted are not interpolated. Use double quotes or string concatenation:

$id = mysql_real_escape_string($id);
$order = "SELECT * FROM `person` WHERE `PersonID` = $id";

Or:

$order = 'SELECT * FROM `person` WHERE `PersonID` = ' . mysql_real_escape_string($id);

However, as mentioned by many others, this query is prone to SQL injection. Almost all queries using string interpolation or string concatenation are prone to SQL injection due to the variable being a user input value. A parametrized query would look like this:

$order = 'SELECT * FROM `person` WHERE `PersonID` = ?';

Alternative, if you're using PDO, the parameters can be named, like this:

$order = 'SELECT * FROM `person` WHERE `PersonID` = :id;';

Then you would bind the parameter to the query with the appropriate method and execute it. Using PDO, it would be something like (of course you'd have to have code prior to this to create the database connection for $db):

$statement = $db->prepare($order);
$statement->bindValue(':id', $id, PDO::PARAM_INT);
$statement->execute();

This is just a start to using parametrized queries. Read more about PDO in the PHP manual.

rink.attendant.6
  • 44,500
  • 61
  • 101
  • 156
  • **NO**. Why does everyone insist on switching to concatenation? It completely ignores the massive SQL injection problem. – tadman Jul 23 '13 at 17:53
  • I added to my answer about using parametrized queries and PDO. – rink.attendant.6 Jul 23 '13 at 18:00
  • 1
    The original example could be made correct and working if you just add an escape call around `$id`. Good PDO example, though. Sorry, but you can't just leave that there as a "fixed" version of the code. – tadman Jul 23 '13 at 18:03
  • Your `mysql_real_escape_string` solution isn’t secure. – Gumbo Jul 23 '13 at 19:24
  • @Gumbo No, it's just a working solution. The secure solution is with PDO (or mysqli, if you prefer that road). – rink.attendant.6 Jul 23 '13 at 19:32
  • Why would you use `mysql_real_escape_string` if not for a MySQL string literal? – Gumbo Jul 23 '13 at 19:34
-1

Try 

$order = "SELECT * FROM `person` WHERE `PersonID` = '$id'";

Had to do this in my own project , nothing else worked , just put ' ' around your id and make your php string start and end with "

Florian Braun
  • 973
  • 9
  • 16