-1

When an user enters in a username and password, i am trying to compare this user input with data that is already stored in the database. For the purposes of just trying to get it to work, if the correct username and password are entered the screen should display "Yay" otherwise it should display "No".

Right now whenever a user enters in any information in the login (whether is be correct or not) the screen is just blank. Please advise what needs to be done to fix this, thank you.

To access the login page go to: http://www.montecarlohotel.co.nf/Membership.html Username & Password already in the database: 585791, Password1

<!DOCTYPE html>


<html>
   <head>
      <meta charset = "utf-8">
      <title>Search Results</title>
      <style type = "text/css">
         body{font-size: 300%;}
      </style>
   </head>
   <body>
      <?php



         $query = "SELECT *
                   FROM UserAccount
                   WHERE Username = '$usr' and Password = '$password'";
         $usr = $_POST['memberid'];
         $password = $_POST['password'];                  

         // Connect to MySQL
         if ( !( $database = mysql_connect( "xxxxxxxx", "xxxxxxx", "xxxxxxxx" ) ) )
            {
              die( "<p>Could not connect to database</p></body></html>" );
            }

         // open hotel database
         if ( !mysql_select_db( "1994715_hotel", $database ) )
            {
              die( "<p>Could not open Hotel database</p></body></html>" );
            }

         // query hotel database
         if ( !( $result = mysql_query( $query, $database ) ) )
            {
              print( "<p>Could not execute query!</p>" );
              die( mysql_error() . "</body></html>" );
            }



      for ($counter = 0; $row = mysql_fetch_row($result); ++$counter) 
          {

           if($result['Username'] == $usr && $result['Password'] == $password)
             {
              print("Yay");
             }

           else
             {
              print("No");
             }
          }



      ?>
   </body>
</html>
Scott Arciszewski
  • 33,610
  • 16
  • 89
  • 206
Brittany
  • 1
  • 1
  • 5
  • 1
    Please use PHP's [built-in functions](http://jayblanchard.net/proper_password_hashing_with_PHP.html) to handle password security. If you're using a PHP version less than 5.5 you can use the `password_hash()` [compatibility pack](https://github.com/ircmaxell/password_compat). – Jay Blanchard Dec 07 '15 at 20:14
  • 3
    Please [stop using `mysql_*` functions](http://stackoverflow.com/questions/12859942/why-shouldnt-i-use-mysql-functions-in-php). [These extensions](http://php.net/manual/en/migration70.removed-exts-sapis.php) have been removed in PHP 7. Learn about [prepared](http://en.wikipedia.org/wiki/Prepared_statement) statements for [PDO](http://php.net/manual/en/pdo.prepared-statements.php) and [MySQLi](http://php.net/manual/en/mysqli.quickstart.prepared-statements.php) and consider using PDO, [it's really pretty easy](http://jayblanchard.net/demystifying_php_pdo.html). – Jay Blanchard Dec 07 '15 at 20:14
  • 1
    [Your script is at risk for SQL Injection Attacks.](http://stackoverflow.com/questions/60174/how-can-i-prevent-sql-injection-in-php) – Jay Blanchard Dec 07 '15 at 20:14
  • Add error reporting to the top of your file(s) right after your opening ` – Jay Blanchard Dec 07 '15 at 20:15
  • Possibly `syntax` error somewhere. Also in your for loop, I don't see where you're getting `$result` from. `for ($counter = 0; $row = mysql_fetch_row($result); ++$counter) ` – yardie Dec 07 '15 at 20:17
  • You think @andre3wap? The OP is setting variables *after* the query. the credentials for accessing the database are exposed for all of the world to see. There is a for loop that is toally unecessary. Yep - syntax errors. – Jay Blanchard Dec 07 '15 at 20:19
  • 1
    @JayBlanchard - Agreed lol OP code is all over the place, in the wrong places ^__^ - heck, I'll re-write the code if the hotel can give me a lifetime free pass to stay there xD – yardie Dec 07 '15 at 20:21
  • the $result is coming from the 3rd IF statement @andre3wap – Brittany Dec 07 '15 at 20:46
  • @Brittany - I am not seeing where it's being stored for reuse. It should be something like `if( logics ){ $result = "Query here"; }` – yardie Dec 07 '15 at 20:49
  • Why is there a for loop with `if($result['Username'] == $usr && $result['Password'] == $password)` when your SQL query was already `WHERE Username = '$usr' and Password = '$password'";`? This code is so totally confused. – developerwjk Dec 07 '15 at 20:52
  • And if that query could ever possibly return more than 1 row, that's the end of the world, because you should have the username as `unique` in the table. So rather than a for loop, verify that there is only one record returned! – developerwjk Dec 07 '15 at 20:53
  • i was told to use a for loop so the when the query was executed it would through each row in the database to search for the same username and password the user entered @developerwjk – Brittany Dec 07 '15 at 20:56
  • can the $result just be a separate variable or does it have to be in an if statement as you've shown @andre3wap – Brittany Dec 07 '15 at 20:59
  • @Brittany the where clause already searched through it all with `WHERE Username = '$usr' and Password = '$password'` and now you want to do it a second time? Stop and THINK. Programming is about logic. Its not magic. – developerwjk Dec 07 '15 at 21:12
  • @Brittany - Yes, you can store it separately. But as Developerwjk is explaining, you need to go back to the drawing board and map out your logic properly - pretty much everything needs to be re-written. – yardie Dec 08 '15 at 13:14

2 Answers2

1

The code you've provided is very insecure:

  • It's vulnerable to SQL injection, as Jay Blanchard noted.
  • You're storing your user's password in plaintext. This is the worst thing you can do.

If you refactor to use PDO (highly recommended) and PHP's password hashing API, you can implement this feature securely and easily.

First, take these lines...

mysql_connect("localhost", "username", "password");
mysql_select_db("database");

... and rewrite them like so:

$db = new PDO("mysql:localhost;dbname=database", "username", "password");

You're going to need that $db object for every query, but it's not so bad once you get used to it.

$stmt = $db->prepare("SELECT * FROM UserAccount WHERE Username = ?");
if ($stmt->execute(array($_POST['username']))) {
    $data = $stmt->fetchAll(PDO::FETCH_ASSOC);
    $firstRow = $data[0];
    if (password_verify($_POST['password'], $firstRow['password'])) {
        // Success, mark them as logged in here.
        $_SESSION['UserId'] = $data[0]['UserId'];
    }
}

Notice in the query string I used ? instead of concatenating the $_POST['username'] variable? And instead I passed it as an array to execute()? This is how you should be writing database code. They're called prepared statements.

Note: This requires using password_hash() during sign-up rather than storing the password directly in the database. You must always use password_hash() and password_verify() for passwords.

Further reading:

And ultimately, a curated list for learning about application security. There's a rich wealth of material available for learning to write secure web applications. Please do take advantage of this and learn as much as you can.

Community
  • 1
  • 1
Scott Arciszewski
  • 33,610
  • 16
  • 89
  • 206
-1

Change this:

$query = "SELECT *
                   FROM UserAccount
                   WHERE Username = '$usr' and Password = '$password'";
         $usr = $_POST['memberid'];
         $password = $_POST['password'];

to this:

$usr = $_POST['memberid'];
$password = $_POST['password']; 
$query = "SELECT *
                   FROM UserAccount
                   WHERE Username = $usr and Password = $password";

I just fixed your syntax and I suggest that you use PDO.

momouu
  • 711
  • 4
  • 14