Help with prepared update statement

[slove05]

I am wanting to update some information in my events using a prepared statement but my mysqli_prepare fails. I think I am missing something silly. Below is the code I am running

if (isset($_POST['create'])) {
                
                $title = $_POST['title'];
                $date = $_POST['date'];
                $time = $_POST['time'];
				$desc = $_POST['desc'];
				$presenter = $_POST['business'];
				$id = $_POST['id'];
				
							
                $sql = "UPDATE events SET title=?, date=?, time=?, desc=? presenter=? WHERE id=?)";
$stmt = mysqli_prepare($con, $sql);
if ( false===$stmt ) {
  die('mysqli_prepare() failed: ' . htmlspecialchars($mysqli->error));
}
mysqli_stmt_bind_param($con, "sssssi", $title, $date, $time, $desc, $presenter, $id);
mysqli_stmt_execute($stmt); 

				echo "Success";
		 
    }

The error message I receive is 

 

mysqli_prepare() failed:

 

Any guidance or better error checking would be appreciated.

[Barand]

desc is a reserved word, it needs to be in backticks (or, better, renamed)

[slove05]

I fixed that. and added

var_dump($con);

directly after 

$stmt = mysqli_prepare($con, $sql);

which gave me this... which I assume means my database is not connected? 

[slove05]

Sorry  Edit.  it gave me this error  

object(mysqli)#1 (0) { } mysqli_prepare() failed:

[mac_gyver]

you are actually using several of the statements with the wrong or non-existent variables. if you had php's error_reporting set to E_ALL and display_errors set to ON (the best place to set these is in the php.ini on your development system), you would be getting several php error messages due to the incorrect usage.

 

if $con is your database connection, you would use that in the mysqli_prepare() and the $con->error. you would not use it in the mysqli_stmt_bind_param(). the mysqli_stmt_bind_param() uses the $stmt.

 

next, you should have error handling for all the database statements that can fail (connection. prepare, execute), so that you don't run following dependent statements when an earlier one has failed. this would catch the case where the connection didn't work. you would never get to the point of trying to run code that depends on the connection. the easiest way of universally adding error handling for all the database statements, is to use exceptions. by using exceptions, your main code only has to deal with error free database statements. you don't have to write conditional logic in your code at each statement that can fail.

 

lastly, are you open to using the PDO extension, rather than the mysqli extension? when using prepared queries, the PDO extension results in the cleanest code.

[Jacques1]

Use

mysqli_report(MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT);

on top of your script to make mysqli report all query problems.

[slove05]

Ok I have the error reporting giving me a better idea of what is erroring out. I am now getting the following error message.

 

Fatal error: Uncaught exception 'mysqli_sql_exception' with message '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 'Event with Upload edit, 2015-02-03, 11:00:00, Testing the event with upload. Mak' at line 1' in /home/content/60/8676960/html/membersadmin/edit_test.php:31 Stack trace: #0 /home/content/60/8676960/html/membersadmin/edit_test.php(31): mysqli->prepare('UPDATE events S...') #1 {main} thrown in /home/content/60/8676960/html/membersadmin/edit_test.php on line 31

 

Lines 30 and 31 look as follows.

$sql = "UPDATE events SET $title, $date, $time, $descrip, $presenter WHERE id = '{$id}')";
$stmt = $con->prepare($sql);

The form I have to process looks like this...

<form method="POST" action="<?php echo basename($_SERVER['PHP_SELF']); ?>" enctype="multipart/form-data">
  
  <?php if (mysqli_num_rows($result) > 0) {?> 
<?php while($row = mysqli_fetch_assoc($result)) {;?>

<input type="text" name="id" id="id" value="<?php echo $row['id']?>" />


    <p>
      <label for="title">Event Title:</label>
      <input type="text" name="title" id="title" value="<?php echo $row['title']?>">
    </p>
    <p>
      <label for="date">Date:</label>
      <input type="text" name="date" id="date" value="<?php echo $row['date']?>">        
    please format YYYY-MM-DD<p>
      <label for="time">Time:</label>
      <input type="text" name="time" id="time" value="<?php echo $row['time']?>" >
    please format HH:MM:SS<p>
      <label for="descrip">Description:</label>
      <input type="text" name="descrip" id="descrip" value="<?php echo $row['descrip']?>"> 
                     
    <p>
    <input type="text" name="presenter" id="presenter" value="<?php echo $row['presenter']?>">
    

   </p>
      <input type="submit" name="create" value="create"> 
    </p>
 
 <?php }/*End Loop*/ ?>
<?php } else { ?>
<h2>Nothing to display.</h2>
<?php }/*End Rows Checking*/ ?> 
  </form>

My statement preparation looks as follows. 

if (isset($_POST['create'])) {
                
				$id = $_POST['id'];
                $title = $_POST['title'];
                $date = $_POST['date'];
                $time = $_POST['time'];
				$descrip = $_POST['descrip'];
				$presenter = $_POST['presenter'];
				
	try {			
							
                $sql = "UPDATE events SET $title, $date, $time, $descrip, $presenter WHERE id = '{$id}')";
$stmt = $con->prepare($sql);
$stmt->execute();
  echo $stmt->rowCount() . " records UPDATED successfully";
    }
catch(PDOException $e)
    {
    echo $sql . "<br>" . $e->getMessage();
    }
}

It is telling my my syntax is no correct but I do not see why. I am obviously new so help with my Update statement is what I believe I need help with now.

[Jacques1]

This isn't a prepared statement, it's a collection of SQL injection vulnerabilities. And appearently you've managed to perform an injection attack against your own site.

 

The PDO manual explains prepared statements in great detail, including multiple examples. I recommend you read that.

 

It also seems you're randomly mixing mysqli functions with PDO features. You cannot do that. They're two entirely different database interfaces which are not compatible in any way.

Edited March 1, 2016 by Jacques1

[mac_gyver]

in the first post in this thread, you had the correct syntax for an UPDATE query, just the desc column needed special handling since it is a reserved keyword.

 

why have you now completely changed the syntax?

 

the following is the syntax definition for an UPDATE query (from the mysql documentation) -

 

UPDATE [LOW_PRIORITY] [iGNORE] table_reference

    SET col_name1={expr1|DEFAULT} [, col_name2={expr2|DEFAULT}] ...

    [WHERE where_condition]

    [ORDER BY ...]

    [LIMIT row_count]

 

 

the red parts are what is commonly used.

[Barand]

The SET clause indicates which columns to modify and the values they should be given.

SET colname1 = :value1, colname2=:value2, .....

[slove05]

I was originally using a prepared statement and only switched to PDO at mac_gyver's urging. Is a mysqli prepared statement enough to overcome sql infection vulnerabilities? I had the update function working with the following 

if (isset($_POST['create'])) {
      
$title = $_POST['title'];
$date = $_POST['date'];
$time = $_POST['time'];
$descrip = $_POST['descrip'];
$presenter = $_POST['presenter'];
$id = $_POST['id'];

$statement = $con->prepare("UPDATE events SET title=?, date=?, time=?, descrip=?, presenter=? WHERE id=?");

//bind parameters for markers, where (s = string, i = integer, d = double,  b = blob)
$statement->bind_param('sssssi', $title, $date, $time, $descrip, $presenter, $id);
$results =  $statement->execute();
if($results){
    print 'Success! record updated'; 
}else{
    print 'Error : ('. $mysqli->errno .') '. $mysqli->error;
}          
				
}

I assume this is open to attack because I am posting the id in a hidden field?

[Jacques1]

This is safe from SQL injection attacks, because now you're actually using a prepared statement. All values are passed to the statement parameters, which means they cannot alter the query structure. Injections only happen when you insert PHP strings straight into the query.

 

You should get rid of the $mysqli->errno stuff, though. This will leak information about your server and is also completely unnecessary if you use mysqli_report() (since mysqli will automatically report all errors).

 

 

 

I was originally using a prepared statement and only switched to PDO at mac_gyver's urging.

 

PDO is superior to mysqli in every aspect, so this is indeed the recommended interface. But if you somehow like mysqli more, you can of course stick to it.

[mac_gyver]

I assume this is open to attack because I am posting the id in a hidden field?

 

 

stopping sql injection is just one part of making a secure application. your query can be safe from sql injection, but your application can still be open to misuse.

 

should all the users on your site be able to submit data to this code and update any record having any id value? if not, you would need a user permission system to control who can perform any action (an update query for events, in this case) or view any content (the 'edit/update' button part of an events list and the events update form for a particular record) and if they are restricted to only affecting records they are the 'owner'/creator of, or do they have permission to update any event record.

 

to allow a user to pick which record to update, you would end up passing the id as a hidden form field value, which is what you are doing now. you would need to determine if the user has permission to run the update code at all and if he has permission to affect the record with the id value that was submitted.

[Jacques1]

And let's not forget CSRF attacks. You can check permissions all day long: If anybody can “tunnel” request through your admin accounts, permissions are meaningless.

 

So first fix the SQL injection vulnerabilities (this should be done by now), then solve the CSRF issues with the Synchronizer Token Pattern and finally take care of authentication and authorization. When in doubt, open a new topic.