ababba2 Posted December 27, 2015 Share Posted December 27, 2015 Could someone please help me? <?php if ( $_POST['option'] == "com_content" && $_POST['view'] == "video" && is_numeric($_POST['id'])) { // connect to the database include_once("../configuration.php"); $cg = new JConfig; $con = mysqli_connect($cg->host,$cg->user,$cg->password,$cg->db); if (mysqli_connect_errno()){ die('n/a'); } // grab the new hit count $query = "SELECT times_viewed FROM ".$cg->dbprefix."hdflv_upload WHERE `id` = " . $_POST['id'] . " LIMIT 1; "; $new_hits = mysqli_fetch_assoc(mysqli_query($con,$query)); // add new hit $addone = $new_hits['times_viewed']+1; $queryadd = "UPDATE ".$cg->dbprefix."hdflv_upload SET times_viewed=".$addone." WHERE `id` = " . $_POST['id'] . "; "; mysqli_query($con,$queryadd); // close the connection to the database mysqli_close($con); echo $addone; } ?> Quote Link to comment Share on other sites More sharing options...
NotionCommotion Posted December 27, 2015 Share Posted December 27, 2015 Your danger is WHERE `id` = " . $_POST['id'] . " Your best bet is using PDO's prepared statements. If not, using something like http://php.net/manual/en/mysqli.real-escape-string.php. Quote Link to comment Share on other sites More sharing options...
ababba2 Posted December 27, 2015 Author Share Posted December 27, 2015 (edited) Could you please help me understand if now the danger is fixed? <?php if ( $_POST['option'] == "com_content" && $_POST['view'] == "video" && is_numeric($_POST['id'])) { // connect to the database include_once("../configuration.php"); $cg = new JConfig; $con = mysqli_connect($cg->host,$cg->user,$cg->password,$cg->db); if (mysqli_connect_errno()){ die('n/a'); } $idpos = mysqli_real_escape_string($_POST['id']); // grab the new hit count $query = "SELECT times_viewed FROM ".$cg->dbprefix."hdflv_upload WHERE `id` = " . $idpos . " LIMIT 1; "; $new_hits = mysqli_fetch_assoc(mysqli_query($con,$query)); // add new hit $addone = $new_hits['times_viewed']+1; $queryadd = "UPDATE ".$cg->dbprefix."hdflv_upload SET times_viewed=".$addone." WHERE `id` = " . $idpos . "; "; mysqli_query($con,$queryadd); // close the connection to the database mysqli_close($con); echo $addone; } ?> Edited December 27, 2015 by ababba2 Quote Link to comment Share on other sites More sharing options...
NotionCommotion Posted December 27, 2015 Share Posted December 27, 2015 You should be fine. After looking at your original script, I see you have is_numeric($_POST['id']) in your IF statement, so technically you might not have needed to escaped it. That being said, you are almost always better off doing so just so you don't change the code and forget. I still think PDO is an easier to maintain solution as well. Quote Link to comment Share on other sites More sharing options...
mac_gyver Posted December 27, 2015 Share Posted December 27, 2015 (edited) because this value is not a string and is not being treated as a string in the sql statement, using any escape string function on it does NOT protect against sql injection. you can inject sql using a hexadecimal value (that encodes some sql syntax) that contains no sql special characters, that the escape string function has no affect on, and mysql will happily convert the hexadecimal value back to the original encoded string. this is made worse by the posted code because is_numeric() allows a hexadecimal value. you need to either validate/cast each value as the CORRECT data type that it is or use prepared sql queries. if the $idpos is expected to be an integer, you must validate/cast it as ONLY an integer value. unfortunately, using any php code that treats the value as an integer will limit the value to php's maximum integer value, which varies depending on the bit length supported on your hardware/operating system. making sure the value only contains numeric characters, see ctype_digit(), will at least limit it to an integer value, including zero. as has already been posted, using PDO for prepared sql queries is more consistent and simpler than using msyqli_ for prepared queries. also, you don't need to SELECT data in order to UPDATE it and in fact there's a race condition present where you will loose counts when there are multiple concurrent instances of your code running. you should be using one INSERT ... ON DUPLICATE KEY UPDATE ... query to do this. Edited December 27, 2015 by mac_gyver Quote Link to comment Share on other sites More sharing options...
ababba2 Posted December 27, 2015 Author Share Posted December 27, 2015 (edited) So adding this should be fine? $idpos = intval(mysql_real_escape_string($_POST['id'])); and could you please code for me the last part, I didn't understood this: also, you don't need to SELECT data in order to UPDATE it and in fact there's a race condition present where you will loose counts when there are multiple concurrent instances of your code running. you should be using one INSERT ... ON DUPLICATE KEY UPDATE ... query to do this. Edited December 27, 2015 by ababba2 Quote Link to comment Share on other sites More sharing options...
ababba2 Posted December 27, 2015 Author Share Posted December 27, 2015 ^Because I'm using Ajax.If I remove select query then the counter begone. Even using INSERT seems to be useless Quote Link to comment Share on other sites More sharing options...
Jacques1 Posted December 27, 2015 Share Posted December 27, 2015 So adding this should be fine? $idpos = intval(mysql_real_escape_string($_POST['id'])); No, it's not fine, because mysql_real_escape_string() is for quoted SQL string literals only (hence the name). It's completely useless for numbers. intval() is likely to truncate the number if it's too big to be represented by a PHP integer. As multiple people have said multiple times already: Use a prepared statement. That's the only sensible solution. Quote Link to comment Share on other sites More sharing options...
NotionCommotion Posted December 27, 2015 Share Posted December 27, 2015 ^Because I'm using Ajax. If I remove select query then the counter begone. Even using INSERT seems to be useless Sorry, maybe not the answer you were looking for, but ajax has nothing to do with the database. You are sending a request to the PHP server, the server validates and saves the data, and replies with typically HTML if a standard request, or JSON or similar if an ajax request. Quote Link to comment Share on other sites More sharing options...
mac_gyver Posted December 27, 2015 Share Posted December 27, 2015 after you figure out if you are going to use prepared queries or not, forget about the INSERT ... ON DUPLICATE part of a single query that i mentioned. you already have the data inserted and an id assigned that corresponds to what is being viewed, you would just use a single UPDATE query. the following code and query will both update the view count by one and retrieve and echo the updated count value - $query = "UPDATE ".$cg->dbprefix."hdflv_upload SET times_viewed = LAST_INSERT_ID(times_viewed+1) WHERE id = ?"; // use a bound parameter in a prepared query for the $idpos value // mysqli prepared query $stmt = $con->prepare($query); $stmt->bind_param("i", $idpos); $stmt->execute(); $addone = mysqli_insert_id($con); // retrieve the updated times_viewed value echo $addone; Quote Link to comment Share on other sites More sharing options...
ababba2 Posted December 28, 2015 Author Share Posted December 28, 2015 Thank you 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.