mysqli查询空洞

Question

I am making a php registration script and registration works fine and everything is imputed into the database successfully and the activation email works just fine, but whenever I use a mysqli query to select or update info, it doesn't work.

For instance, when I log in with an account I know is in the database, it tells me the username doesn't exist, and when clicking the activation link in the email, the query fails to update the database in the same way.

I'm sure this is a super simple error I'm overlooking in my newbishness, but I couldn't find a suitable answer after a few hours of looking. I'm not really sure what the issue is.

Activate.php

require "/functions.php";

if (isset($_GET['user'])) {
  $user = $_GET['user'];
}
if (isset($_GET['key']) && (strlen($_GET['key']) == 32)){
  $key = $_GET['key'];
}

if (isset($user) && isset($key)) {
$sql = <<<SQL
  UPDATE users
  SET validation = NULL
  WHERE username='$user'
  AND validation='$key',
  user_group = 'member'
SQL;

$count = $db->affected_rows;

if ($count == 1)
{
    echo '<div>Your account is now active. You may now <a href="login.php">Log in</a></div>';

} else {
 echo '<div>Oops! Your account could not be activated. Please recheck the    link or contact the system administrator.</div>';

}
ob_end_flush();

} else {
  echo '<div>Error Occured .</div>';
}

?>

login.php

// Globals & error variable
require "/functions.php";

    session_start();
    $error = "";

if (isset( $_POST['Submit'])) {
    if (empty($_POST['username']) || empty($_POST['password'])) {
        $error = "Please fill in all fields!";
}
else {

    $username=$_POST['username']; 
    $password=$_POST['password']; 

    // Injection-protection!
    $username = stripslashes($username);
    $password = stripslashes($password);
    $username = mysqli_real_escape_string($db, $username);
    $password = mysqli_real_escape_string($db, $password);

$sql = <<<SQL
    SELECT *
    FROM `users`
    WHERE `username`='$username'
SQL;

    $result = $db->query($sql);
    $count->num_rows;

    if($count==1){
        while ($row = mysqli_fetch_array($result)) {
            $hash = $row['password'];
            $ug = $row['user_group'];
        }

        salt();

        $options=['salt'=>$bcrypt_salt, 'cost'=>12];
        $password=$argv[1];

        if (crypt($password,$hash) == $hash) {
            $_SESSION['login_user']= $username;
            $_SESSION['user_group']= $ug;
            header("location:index.php");
        }
        else {
            $error = "Username or password is invalid!";
        }
    }
    else {
        $error = "That username doesn't exist!";
    }
    ob_end_flush();
    }
}

functions.php

 // db connect
 $db = new mysqli('HOST', 'USER', 'PASS', 'DB');

if($db->connect_errno > 0){
   die('Unable to connect to database [' . $db->connect_error . ']');
}

On a side note, feel free to bring up any glaringly obvious bad-practice things you guys see. I'm new, but I don't want to develop bad habits!

Answer 1

Besides what's already been stated in comments that you weren't executing that query (something I did spot though, before reading through), and now you are, there's this block of code, besides the extra comma in comments which you said was removed:

WHERE username='$user'
AND validation='$key',
user_group = 'member'

It's missing an AND --- AND validation='$key' AND user_group = 'member'

Checking for errors would have signaled that syntax error http://php.net/manual/en/mysqli.error.php

You should also add exit; after header, otherwise your code may want to continue executing/running.

Sidenote:

• Keeping in mind what was already stated in comments about $count->num_rows; and using $count = $result->num_rows; instead and not executing the query $db->query($sql); for the activation file.

---

Add error reporting to the top of your file(s) which will help find errors.

<?php 
error_reporting(E_ALL);
ini_set('display_errors', 1);

// rest of your code

Sidenote: Error reporting should only be done in staging, and never production.

---

Footnotes:

Your present code for the activation file is open to SQL injection. Use prepared statements, or PDO with prepared statements, they're much safer.

---

Edit:

• Please read the following over very carefully and there are notes included in my test code with commented instructions/information.

Using the code that follows below, proved to be successful for me.

Make sure that:

• The column for the validation key is VARCHAR
• Its length is 50, as a test but 32 may not be enough.
• Use a minimum of 40 by altering the column's length first before new tests.
• That the "validation" column accepts a NULL value.
• You've chosen the correct database and table.
• The validation key does not contain any trailing space(s).

MD5 produces a string length of 32, so you may have to increase it for your column, to say 40 or 50. My test used 50.

Your WHERE clause will depend on it.

If the string sent via Email contains a space, make sure there are no trailing spaces.

• Use trim()
• Make sure the entered key in the table doesn't initially contain a trailing space.
• You will have to start over.

I.e.:

$key = $_GET['key'];
$key = trim($key);

• Affected rows will not work if your table still contains "test" and the "b5bad6f02c247458e3d888f94b568819" test key that you are using.
• In order for "affected_rows()" to truly work, you will need to start over with a clear table for the "user" and "key".
• So, clear those out before running a new test.

I would also get rid of this conditional statement if (isset($user) && isset($key)) { along with its accompanying closing brace }.

• It may be doing more harm than good.

You are already using a similar method in this block:

if (isset($_GET['user'])) {
  $user = $_GET['user'];
}
if (isset($_GET['key']) && (strlen($_GET['key']) == 32)){
  $key = $_GET['key'];
}

However, I would modify that to: (and to test)

if (isset($_GET['user'])) {
  $user = $_GET['user'];
}
else{ 
   echo "User is not set"; 
exit; // Stop the entire process, it's not set
}

if (isset($_GET['key']) && (strlen($_GET['key']) == 32)){
  $key = $_GET['key'];
}
else{ 
   echo "Key is not set"; 
exit; // Stop the entire process, it's not set
}

Sidenote: Instead of isset(), you may be better off using !empty() instead.

---

My test codes:

Sidenote: You said you removed user_group = 'member' make sure that doesn't alter the process.

<?php 
$DB_HOST = 'xxx'; // Change those
$DB_USER = 'xxx'; // to your
$DB_PASS = 'xxx'; // own
$DB_NAME = 'xxx'; // credentials

$db = new mysqli($DB_HOST, $DB_USER, $DB_PASS, $DB_NAME);
if($db->connect_errno > 0) {
  die('Connection failed [' . $db->connect_error . ']');
}

$activation = md5(uniqid(rand(), true));

echo $activation . "<br>";

$user = "test";
$key = "b5bad6f02c247458e3d888f94b568819 "; 
// deliberate space added at the end of the string, remove it from this when testing.

$key = trim($key); // trims off the space at the end of the key string.

echo strlen($key); // yep, gotten 32

$sql = <<<SQL
  UPDATE users
  SET validation = NULL
  WHERE username='$user'
  AND validation='$key'

SQL;

$result = $db->query($sql);

if($result) {
  echo "Success" . "<br>"; 
// This does not always mean it was truly successful. Use affected_rows()
}

else{
  echo "Failed" . mysqli_error($db);
}

$affected = $db->affected_rows;

if($affected){
  echo "Affected yes";
}

else{
  echo "Not affected";
}

Should this not work for you, then I don't know what else I can say or do that will be of any further help; I'm sorry but I tried.

I sincerely wish you well and I hope this matter gets resolved.