php/sql query is returning the wrong response

Question

ok so i am running a reset password script, its supposed to replace the password with the sha1 hashed version of password01 if successful it should return the success message if not then the failed message, but its returning the failure message weather it works or not which it always does! any ideas? i have inserted the code below:

<?php 
session_start();
$host= $_SESSION["dbhost"]; 
$username= $_SESSION["dbuser"]; 
$password= $_SESSION["dbpass"]; 
$db_name= $_SESSION["dbname"];
$tbl_name="users"; // Table name 

mysql_connect("$host", "$username", "$password")or die("cannot connect"); 
mysql_select_db("$db_name")or die("cannot select DB");

$userid = $_POST['id'];
$password = 'password01';    
$hashpass = sha1($password);

//$result = mysql_query("SELECT password FROM $tbl_name WHERE id='$userid'");
$sql=mysql_query("UPDATE $tbl_name SET password='$hashpass' where id='$userid'"); 

if (mysql_query($sql)) {
echo "<ul id='breadcrumbs-one'>";
echo "<li><a href=''>Users</a></li>";
echo "<li><a href='' class='current'>Reset</a></li>";
echo "</ul>";
echo "<div class='tn-box tn-box-color-3 mcenterlow'>";
echo "<p>The Opperation was Sucessful!<br><a href='?users'>Add Another?</a></p>";
echo "<div class='tn-progress'></div>";
echo "</div>";
}
else {
echo "<ul id='breadcrumbs-one'>";
echo "<li><a href=''>Users</a></li>";
echo "<li><a href='' class='current'>Reset</a></li>";
echo "</ul>";
echo "<div class='tn-box tn-box-color-1 mcenterlow'>";
echo "<p>The Opperation was Not Sucessful!<br><a href='?users'>Try Again?</a></p>";
echo "<div class='tn-progress'></div>";
echo "</div>";
}
?>

Side note: it is usually considered a bad practice to mix application logic with presentation code. — Ethan
– Ethan, Commented Jan 31, 2013 at 2:15
Call mysql_error() in your else {} case to see why it failed. Note that this is vulnerable to SQL injection. At a minimum, call mysql_real_escape_string() on $userid. — Michael Berkowski
– Michael Berkowski, Commented Jan 31, 2013 at 2:15
Apart from the sql injection, any user can change the password of any other user. You should store the user ID in the session after a successful login. Unless only the administrator can run this script of course... — jeroen
– jeroen, Commented Jan 31, 2013 at 2:17
You should be using PHP's crypt (blowfish) functionality for passwords, unsalted sha1's are a very bad idea. — mmattax
– mmattax, Commented Jan 31, 2013 at 2:45
yeah im aware of the injection attack vulnerability, i am writing this as part of a practice program for myself locally, i know the basics of php, but im really trying to expand my knowledge. appreciate the input tho, and the program code is because this is called by my mvc code so the code is run within a template wrapper kind of thing. kinda a merge of several practice projects. @jeroen, only the admin can run this, it comes from a user list with a reset button that submits the form with the userid. — JL Griffin
– JL Griffin, Commented Jan 31, 2013 at 3:40

jeroen · Accepted Answer · 2013-01-31 02:19:55Z

1

Apart from the comments below your question, the reason your if is failing, is that you are using mysql_query on the result of another call to mysql_query.

To solve that, you should change your condition to:

if ($sql) {

(for an UPDATE query, mysql_query return true on success and false on failure)

And you should really switch to PDO (or mysqli) with prepared statements and bound variables to get rid of the sql injection problem you have.

answered Jan 31, 2013 at 2:19

jeroen

91.8k22 gold badges117 silver badges132 bronze badges

Sign up to request clarification or add additional context in comments.

Comments

CXJ · Accepted Answer · 2013-01-31 02:19:42Z

1

Your code is calling mysql_query() twice, once with the actual SQL string, and again with the returned result value from the first call! Hence, the second call which is checked by the "if" always fails; it's not valid SQL.

I think you want to say something like

$sql = "UPDATE $tbl_name SET password='$hashpass' where id='$userid'";

if (mysql_query($sql)) {
    // etc.

answered Jan 31, 2013 at 2:19

CXJ

4,5073 gold badges38 silver badges66 bronze badges

1 Comment

JL Griffin Over a year ago

yup that was the ide! looked over it too many times and just missed the error! haha! thanks!

Matt Runion · Accepted Answer · 2013-01-31 04:24:59Z

1

You are setting $sql equal to the result of mysql_query("UPDATE $tbl_name SET password='$hashpass' where id='$userid'");. On an UPDATE statement, mysql_query returns a boolean. You are THEN called mysql_quert($sql) in the if statement. This is logically incorrect, as you are checked the result of mysql_query(<bool>); because $sql is the RESULT of a query.

Either:

$sql=mysql_query("UPDATE $tbl_name SET password='$hashpass' where id='$userid'"); 

if ($sql) {

or

$sql = "UPDATE $tbl_name SET password='$hashpass' where id='$userid'"; 

if (mysql_query($sql)) {

Matt

edited Jan 31, 2013 at 4:24

answered Jan 31, 2013 at 2:21

Matt Runion

1,0717 silver badges13 bronze badges

Collectives™ on Stack Overflow

php/sql query is returning the wrong response

3 Answers 3

Comments

1 Comment

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

Comments

1 Comment

Comments

Your Answer

Sign up or log in

Post as a guest

Related