If statement flow possibly disrupted by while loop PHP

I have no idea why this code cannot be executed correctly but basically, the first if statement and the following elseif statement are both executed appropriately upon condition, though the final elseif statement is not executed upon condition. I have tried both else and elseif for the final statement and both get hung up. I don't know if it's because they lie within a while loop or what, but I am stumped.

<?php
session_start();

$servername = 'redacted';
$username = 'redacted';
$password = 'redacted';
$dbname = 'redacted';

$user = $_POST[ 'User' ];
$pass = $_POST[ 'Pass' ];
	
$conn = new mysqli( $servername, $username, $password, $dbname );

if ( $conn->connect_error ) {
	die( "Connection failed: " . $conn->connect_error );
}

$lsql = "SELECT user, pass FROM userinfo WHERE user='$user';";
$lresult = $conn->query( $lsql );

if ( $lresult->num_rows > 0 ) {
	while ( $row = $lresult->fetch_assoc() ) {
		if ( ( ( $row[ 'user' ] ) == $user ) && ( ( $row[ 'pass' ] ) == $pass ) ) {
			$_SESSION[ 'signeduser' ] = $row[ 'user' ];
			$_SESSION[ 'incorrectlogin' ] = false;
			header( 'Location: redacted' );
		} 
		elseif ( ( ( $row[ 'user' ] ) == $user ) && ( ( $row[ 'pass' ] ) != $pass ) ) {
			$_SESSION[ 'incorrectlogin' ] = true;
			header( 'Location: redacted' );
		}
		elseif ( ( ( $row[ 'user' ] ) != $user ) && ( ( $row[ 'pass' ] ) != $pass ) ) {
			$rsql = "INSERT INTO userinfo (user, pass) VALUES ('$user', '$pass');";
			$conn->query( $rsql );
			$tsql = "INSERT INTO usertransfers(user, transfer) VALUES ('$user', 1); INSERT INTO usertransfers(user, transfer) VALUES ('$user', 2); INSERT INTO usertransfers(user, transfer) VALUES ('$user', 3);";
			$tresult = $conn->query($tsql);
			$_SESSION[ 'incorrectlogin' ] = false;
			$_SESSION[ 'signeduser' ] = $user;
			header( 'Location: redacted' );
		}	
	}
}
$conn->close();
?>

Open in new window

Caden PangHS Sophomore Web DeveloperAsked:
Who is Participating?
 
gr8gonzoConnect With a Mentor ConsultantCommented:
Well, the short answer is because $row["user"] will ALWAYS equal $user. There's no way to get into that loop without that being the case. The last section of code starts with "$row['user'] != $user" but that will NEVER ever be hit.

Let's walk through it and say that on line 9, $user becomes "john".

Your query on line 18 is then:
SELECT user, pass FROM userinfo WHERE user='john'

Then all of your if/elseif/elseif code is within a loop that will only run when there is a record returned, which means the "user" field that came back from the database HAS to also be "john" (otherwise the database wouldn't match the record).

Since your last elseif will only execute if $row["user"] (which will be "john") is NOT equal to $user (which will ALSO be "john"), it will never meet all the conditions and enter that section of code.

A few additional thoughts:

1. I would assume that your user field is unique (probably a bad idea otherwise), so add a "LIMIT 1" to the end of your query, and just grab the first row instead of adding code that does a while() loop.
$lsql = "SELECT user, pass FROM userinfo WHERE user='$user' LIMIT 1;";

2. That code is vulnerable to SQL injection. Make sure you escape your parameters to avoid injection, like this:
$lsql = "SELECT user, pass FROM userinfo WHERE user='" . $conn->real_escape_string($user) . "' LIMIT 1;";

3. Since you're only returning rows where you already have matched the username, there's no real purpose in returning the "user" field in your results. If the query doesn't match the username, it won't return records and won't entire your while() loop:
$lsql = "SELECT pass FROM userinfo WHERE user='" . $conn->real_escape_string($user) . "' LIMIT 1;";


4. If your username is unique, then you should only ever get one row back, so don't bother with a while() loop - just fetch the row:

if ( $lresult->num_rows > 0 )
{
 // Grab the first row
  $row = $lresult->fetch_assoc();

  ...rest of the code...
}

5. If you ever use header() to redirect the user to a new location, immediately follow that line with an exit() or die() line to prevent any additional code from processing (code that might be somewhere below the header() line):
header( 'Location: redacted' );
exit();

6. You seem to be storing the password in the database and comparing the user's entered password to what's stored in the database. This is a really insecure practice. At the bare minimum, you should be working with salted hashes, like a SHA-1 hash. Basically, it means you're converting the original password into a series of letters and numbers that CONSISTENTLY AND ACCURATELY REPRESENTS the original password ("letmein" becomes "b7a875fc1ea228b9061041b7cec4bd3c52ab3ce3").

So when you insert a record into the userinfo table, instead of inserting the original password like this:
$user = 'john';
$pass = 'letmein';
$query = "INSERT INTO userinfo (user, pass) VALUES ('$user', '$pass')";

...instead prepend the password with something that only you know and then run sha1() on the value, like this:
$user = 'john';
$pass = sha1('CadenPang!'.'letmein');
$query = "INSERT INTO userinfo (user, pass) VALUES ('$user', '$pass')";

And later when someone tries to log in, you do the same thing. So instead of:
if ($row['pass'] == $pass)

... you do this:
if ($row['pass'] == sha1('CadenPang!'.$pass))

By doing this, the original/raw password is never stored in your database, so if someone hacks your site and copies your database, they won't be able to figure out what your users' passwords are. They'll just see the salted hash values, which are useless to a hacker.

Additionally, it means that your password column only needs to be 40 characters long (that's how long every SHA-1 hash is, regardless if your original password is "Hello" or the entirety of someone's favorite poem.

7. Since you've already established that the username matches in your query, you don't need to check for it again in the code. Also, you don't need parentheses around individual fields like "( $row['pass'] )" - that makes the code harder to read and serves no purpose.


Your adjusted code after all the above recommendations (aside from the hashing) might look like this:
<?php
session_start();

$servername = 'redacted';
$username = 'redacted';
$password = 'redacted';
$dbname = 'redacted';

$user = $_POST[ 'User' ];
$pass = $_POST[ 'Pass' ];
	
$conn = new mysqli( $servername, $username, $password, $dbname );

if ( $conn->connect_error ) {
	die( "Connection failed: " . $conn->connect_error );
}

$lsql = "SELECT pass FROM userinfo WHERE user='" . $conn->real_escape_string($user) . "' LIMIT 1;";
$lresult = $conn->query( $lsql );

if ( $lresult->num_rows > 0 )
{
  // Grab the first row
  $row = $lresult->fetch_assoc();

  if ($row['pass'] == $pass)
  {
    $_SESSION[ 'signeduser' ] = $user;
    $_SESSION[ 'incorrectlogin' ] = false;
    header( 'Location: redacted' );
    exit();
  } 
  else
  {
    $_SESSION[ 'incorrectlogin' ] = true;
    header( 'Location: redacted' );
    exit();
  }
}
$conn->close();

Open in new window


Some additional reading that might help you avoid serious security vulnerabilities:
https://www.experts-exchange.com/articles/1263/5-Steps-to-Securing-Your-Web-Application.html
0
Question has a verified solution.

Are you are experiencing a similar issue? Get a personalized answer when you ask a related question.

Have a better answer? Share it in a comment.

All Courses

From novice to tech pro — start learning today.