tags:

views:

140

answers:

4

I have written a very simple function:

function editCategory() {
    $ID         = urlencode($_GET['id']);
    $cname   = mysql_fix_string($_POST['cname']);
    $kabst   = mysql_fix_string($_POST['kabst']);
    $kselect    = $_POST['kselect'];
    $subsl      = $_POST['subsl'];
    $kradio     = $_POST['kradio'];
    $ksubmit    = $_POST['ksubmit'];

    if (isset($ksubmit)) {
        $query = "UPDATE category SET name = '$cname', description = '$kabst', published = '$kselect',  home = '$kradio', subcat = '$subsl'  WHERE id = $ID ";

        $result = mysql_query($query);
        if (mysql_affected_rows () == 1) {
            echo "ok";
        }
        else{
            echo mysql_error();
        }
    }
}

function mysql_fix_string($string)
{
    if (get_magic_quotes_gpc())
        $string = stripslashes(($string));
    return mysql_real_escape_string($string);
}

Error:

You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '' at line 1

What is wrong?

+5  A: 
$ID         = intval($_GET['id']); //using urlencode here is weird
$cname      =  mysql_real_escape_string($_POST['cname']); 
//and the same for the rest ALL.
$kradio     = mysql_real_escape_string($_POST['kradio']); 

Also,

$ksubmit    = $_POST['ksubmit']; 
if (isset($ksubmit)) { 

is senseless. $ksubmit would be always set it should be

if (isset($_POST['ksubmit'])) { 

To be sure you have all variables, please add these lines at the top of the script:

ini_set('display_errors',1);
error_reporting(E_ALL);
Col. Shrapnel
Better yet `if(true != empty($_POST['ksubmit']))`
Chacha102
function mysql_fix_string($string) { if (get_magic_quotes_gpc()) $string = stripslashes(($string)); return mysql_real_escape_string($string); }The same problem...
@jasmine But you are. 1. this function is wrong. it must be mysql_real_escape_string and nothing else. 2. it must be applied to all fields which you have enclosed in quotes in the query, not only two. 3. Do you have $_GET['id'] variable? See addon to my post.
Col. Shrapnel
@Chacha102 You do not understand meaning of this operator. Your addition is just useless in every way
Col. Shrapnel
Notice: Undefined index: id in /home/***/public_html/cms/dashboard/includes/database.php on line 255
@jasmine quod erat demonstrandum, which means "that which was to be demonstrated". Pascal were right, you don't pass id wia GET method to your post. It's either in the POST or weren't sent at all
Col. Shrapnel
@Col Empty values are generally useless. I prefer to not waste my time with a second statement asking if the value is empty or not. 9 times out of 10, if it is an empty value, it is just as useful as not being set at all. And most certainly, I do understand the meaning of the operator.
Chacha102
@Chacha102 no, you don't :) 1. using `true` constant here is senseless. `if(empty($_POST['ksubmit']))` is enough. 2. we're checking merely if button were pressed, not it's value. So, isset is enough in every way.
Col. Shrapnel
A: 

Sounds like an empty variable.

And do something against SQL injection, everybody can hack your database. With a little luck, you're the one killing your database... Use mysql_real_escape_string() for all user input into your queries.

Frank Heikens
function mysql_fix_string($string) { if (get_magic_quotes_gpc()) $string = stripslashes(($string)); return mysql_real_escape_string($string); }this is the same with mysql_real_escape_string() ;)and "Im not" the one killing "my" database
And why do you protect just a few parameters against SQL injection and leave the others vulnerable? You're still killing your database, sorry for the bad news.
Frank Heikens
I didnot know that it is necessary for select and radio buttons. Thanks for bad news.
@jasmine it is necessary for the every field you put in the query, no matter from where it came. it 's not user input matter but SQL matter. Every value you enclose in quotes in the query, must be passed through mysql_real_escape_string. No exceptions. No conditions. No matter it came from the radiobutton or a text file.
Col. Shrapnel
+1  A: 

You have to make sure that :

  • For fields that are strings (varchar/char) in the DB :
    • the values you're passing are properly enclosed with quotes
    • the content of the values you're passing must be escaped : if there is a quote in what the user POSTed, it must be escaped -- see mysql_real_escape_string
  • For fields that are integers in the DB :
    • You must pass integer values
    • which can be ensured by calling intval on the values POSTed by the user


Here, you should probably :

  • Use intval() on $_GET['id']
  • Use mysql_real_escape_string on some other fields.
    • JUdging from the query, in which all fields, except id are enclosed with single-quotes, I'd say you have to use mysql_real_escape_string on all fields -- except id, of course.


As a sidenote :

  • You are using $_GET for id
  • And $_POST for everything else.

Is that on purpose ?

Pascal MARTIN
function mysql_fix_string($string) { if (get_magic_quotes_gpc()) $string = stripslashes(($string)); return mysql_real_escape_string($string); }But the same problem with intval() and mysql_real_escape_string
A: 

Here is an example of a very simple CRUD application, just to show how to pass an id:

<?
mysql_connect();
mysql_select_db("new");
$table="test";
if($_SERVER['REQUEST_METHOD']=='POST') { //form handler part:
  $name = mysql_real_escape_string($_POST['name']);
  if ($id=intval($_POST['id'])) {
    $query="UPDATE $table SET name='$name' WHERE id=$id";
  } else {
    $query="INSERT INTO $table SET name='$name'";
  }
  mysql_query($query) or trigger_error(mysql_error()." in ".$query);
  header("Location: http://".$_SERVER['HTTP_HOST'].$_SERVER['PHP_SELF']);
  exit;
}
if (!isset($_GET['id'])) { //listing part:
  $LIST=array();
  $query="SELECT * FROM $table";
  $res=mysql_query($query);
  while($row=mysql_fetch_assoc($res)) $LIST[]=$row;
  include 'list.php';
} else { // form displaying part:

  if ($id=intval($_GET['id'])) {
    $query="SELECT * FROM $table WHERE id=$id";
    $res=mysql_query($query);
    $row=mysql_fetch_assoc($res);
    foreach ($row as $k => $v) $row[$k]=htmlspecialchars($v);
  } else {
    $row['name']='';
    $row['id']=0;
  }
  include 'form.php';
}
?>

File form.php:

<form method="POST">
<input type="text" name="name" value="<?=$row['name']?>"><br>
<input type="hidden" name="id" value="<?=$row['id']?>">
<input type="submit"><br>
<a href="?">Return to the list</a>
</form>

File list.php:

<a href="?id=0">Add item</a>
<? foreach ($LIST as $row): ?>
<li><a href="?id=<?=$row['id']?>"><?=$row['name']?></a>
<? endforeach ?>
Col. Shrapnel
@Col. ShrapnelYour example solved my problem. I learned to get id from post not urlquery. Thanks for great help.
@jasmine though it can be passed via urlquery too. you have just to put it there. In my example it would be `<form method="POST" action="?id="<form method="POST">">` but I prefer to use urlquery to control my app's flow.
Col. Shrapnel