slove05 Posted February 26, 2016 Share Posted February 26, 2016 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. Quote Link to comment Share on other sites More sharing options...
Barand Posted February 26, 2016 Share Posted February 26, 2016 desc is a reserved word, it needs to be in backticks (or, better, renamed) Quote Link to comment Share on other sites More sharing options...
slove05 Posted February 26, 2016 Author Share Posted February 26, 2016 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? Quote Link to comment Share on other sites More sharing options...
slove05 Posted February 26, 2016 Author Share Posted February 26, 2016 Sorry Edit. it gave me this error object(mysqli)#1 (0) { } mysqli_prepare() failed: Quote Link to comment Share on other sites More sharing options...
mac_gyver Posted February 27, 2016 Share Posted February 27, 2016 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. Quote Link to comment Share on other sites More sharing options...
Jacques1 Posted February 27, 2016 Share Posted February 27, 2016 Use mysqli_report(MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT); on top of your script to make mysqli report all query problems. Quote Link to comment Share on other sites More sharing options...
slove05 Posted March 1, 2016 Author Share Posted March 1, 2016 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. Quote Link to comment Share on other sites More sharing options...
Jacques1 Posted March 1, 2016 Share Posted March 1, 2016 (edited) 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 Quote Link to comment Share on other sites More sharing options...
mac_gyver Posted March 1, 2016 Share Posted March 1, 2016 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. Quote Link to comment Share on other sites More sharing options...
Barand Posted March 1, 2016 Share Posted March 1, 2016 The SET clause indicates which columns to modify and the values they should be given. SET colname1 = :value1, colname2=:value2, ..... Quote Link to comment Share on other sites More sharing options...
slove05 Posted March 1, 2016 Author Share Posted March 1, 2016 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? Quote Link to comment Share on other sites More sharing options...
Jacques1 Posted March 1, 2016 Share Posted March 1, 2016 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. Quote Link to comment Share on other sites More sharing options...
mac_gyver Posted March 1, 2016 Share Posted March 1, 2016 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. Quote Link to comment Share on other sites More sharing options...
Jacques1 Posted March 1, 2016 Share Posted March 1, 2016 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. Quote Link to comment Share on other sites More sharing options...
Recommended Posts
Join the conversation
You can post now and register later. If you have an account, sign in now to post with your account.