Login.php problems

Discussion in 'PHP' started by ViggoAvatar, Feb 17, 2016.

  1. #1
    <?php
    session_start();
    $conection = mysqli_connect("<login for database and sql)");
    if(isset($_POST["submit"]))
    
    // Define $username and $password
    $username=$_POST['username'];
    $password=$_POST['password'];
    // To protect from MySQL injection
    $username = stripslashes($username);
    $password = stripslashes($password);
    //Check username and password from database
    $result = mysqli_query($connection, "SELECT * FROM usertbl WHERE username='$username' and password='$password'");
    $row=mysqli_fetch_array($result);
    
    if(mysqli_num_rows($result) == 1)
    {
    $_SESSION['username'] = $login_user; // Initializing Session
    header("<succes>"); // Redirecting To Other Page
    }else
    {
    $error = "<failure>";
    }
    ?>
    PHP:
    this doesnt work, and at this point losing my patience (after solving a lot of problems, still got a few)


    Warning: mysqli_query() expects parameter 1 to be mysqli, null given in /home/u470788761/public_html/login.php on line 16

    Warning: mysqli_fetch_array() expects parameter 1 to be mysqli_result, null given in /home/u470788761/public_html/login.php on line 17

    Warning: mysqli_num_rows() expects parameter 1 to be mysqli_result, null given in /home/u470788761/public_html/login.php on line 19


    could somebody tell me what is wrong? (and if possible fix it)
     
    Solved! View solution.
    ViggoAvatar, Feb 17, 2016 IP
  2. PoPSiCLe

    PoPSiCLe Illustrious Member

    Messages:
    4,623
    Likes Received:
    725
    Best Answers:
    152
    Trophy Points:
    470
    #2
    Your connection variable isn't correct. It's $conection when it's defined, and (correctly) $connection when used.
    Besides that, that code is fairly bad, and completely defies the reason for using mysqli over mysql, the fact that you can use prepared queries.
     
    PoPSiCLe, Feb 17, 2016 IP
  3. ViggoAvatar

    ViggoAvatar Peon

    Messages:
    21
    Likes Received:
    0
    Best Answers:
    0
    Trophy Points:
    1
    #3
    yeah well it didnt allow me to use normal mysql (my host didnt allow it)
    And i know the code may be sucky, as im just a student trying to learn this stuff (and making mistakes along the way)
    i got this code to work at one point, redirecting to the <succes> page, but it also redirected to the <succes> page when it should be the <failed> page.
     
    ViggoAvatar, Feb 17, 2016 IP
  4. PoPSiCLe

    PoPSiCLe Illustrious Member

    Messages:
    4,623
    Likes Received:
    725
    Best Answers:
    152
    Trophy Points:
    470
    #4
    Nothing wrong with being new to this, I'm just saying there are several things wrong with your code. I'm usually not that interested in providing proper code examples (@deathshadow on the other hand, does that a lot), but for this, if you were using mysqli_ properly, it would look something like this:
    
    <?php
    session_start();
    $mysqli = new mysqli("localhost","username","password","database");
    if (mysqli_connect_errno()) {
       echo "Connect failed: ".mysqli_connect_error();
       exit();
    }
    if (isset($_POST["submit"])) {
       if ($stmt = $mysqli->prepare("SELECT * FROM usertbl WHERE username = ? AND password = ?")) {
         $stmt->bind_param('ss',$_POST['username'],$_POST['password'])
    
         if (!$stmt->execute()) {
           trigger_error('Error executing MySQL query: '.$stmt->error());
         }
         if ($stmt->num_rows() == 1) {
           $_SESSION['username'] = $_POST['username'];
           header('location: success');
         } else {
           header('location: error');
         }
       }
    
    
    }
    
    ?>
    
    PHP:
    This uses the object oriented version of mysqli, which is usually better, as it doesn't really allow you to use backwards coding like plugging user-input directly into a query and such (it's of course possible, but involves a bit more work).

    Not tested, so it might not work (I don't use mysqli_, I use PDO, which I find a bit more robust and easier to work with)
     
    PoPSiCLe, Feb 17, 2016 IP
  5. ViggoAvatar

    ViggoAvatar Peon

    Messages:
    21
    Likes Received:
    0
    Best Answers:
    0
    Trophy Points:
    1
    #5
    thanks for the rework of the code!
    it didnt provide me with any errors, it just shows a white screen with the "/login.php" line in the adressbar (after filling in all the blank spaces you left for me ofcourse)
     
    ViggoAvatar, Feb 17, 2016 IP
  6. PoPSiCLe

    PoPSiCLe Illustrious Member

    Messages:
    4,623
    Likes Received:
    725
    Best Answers:
    152
    Trophy Points:
    470
    #6
    Hm. That suggests the header-bit isn't working as it should. Do you have display_errors on?
    If not, add this to the login.php file, at the start:
    
    ini_set('display_errors',1);
    error_reporting(E_ALL);
    
    PHP:
    That should show you any php-errors that shows up.
     
    PoPSiCLe, Feb 17, 2016 IP
  7. ViggoAvatar

    ViggoAvatar Peon

    Messages:
    21
    Likes Received:
    0
    Best Answers:
    0
    Trophy Points:
    1
    #7
    nope, doesnt print any errors, just the white screen :/
     
    ViggoAvatar, Feb 17, 2016 IP
  8. #8
    Do not use isset, use empty to check your data existent.
    Here's my simpler version (not include security thingy):
    
    <?php
    
        /* connect and select database with MySQLi */
        if ($mysqli = new mysqli('localhost','root','password','database_name'));
    
        /* check connection */
        if (mysqli_connect_errno()) {
              printf("Connect failed: %s\n", mysqli_connect_error());
            exit();
        }
    
        if(empty($_POST['username']) || empty($_POST['password'])) {
            die("error message here");
        }
    
        if ($result = $mysqli->query("SELECT username, password FROM usertbl WHERE username = '$username' and password = '$password'")) { // never use *
            if($result->num_rows == 1) {
                while($row = $result->fetch_object()) {
                    // check if login sucess
                    echo $row->username;
                    // set up session
                    $_SESSION['username'] = $row->username;
                }
            }
        }
    ?>
    
    Code (markup):
    Better yet, use the PDO method. It's simpler and more secure.

    Again, I already ditched this in favor of PDO.
     
    Last edited: Feb 18, 2016
    ketting00, Feb 18, 2016 IP
  9. ViggoAvatar

    ViggoAvatar Peon

    Messages:
    21
    Likes Received:
    0
    Best Answers:
    0
    Trophy Points:
    1
    #9
    worked like a charm, many thanks!
     
    ViggoAvatar, Feb 18, 2016 IP
  10. deathshadow

    deathshadow Acclaimed Member

    Messages:
    9,732
    Likes Received:
    1,999
    Best Answers:
    253
    Trophy Points:
    515
    #10
    I gotta go with @ketting00 on this, switch to PDO.

    I would also NOT be using the username as what you store in the session. Names are strings, strings are slower to query when it's time to pull more information than just the username on database lookups. That's why you should be storing the ID.

    ONLY return the ID from the password query and try NOT to EVER return the password in any other query. You are best off keeping all password traffic mono-directional TOWARDS the database to reduce the odds of a password leak. In that same way you should probably encode the password with a hash like sha256, sha512 or whirlpool BEFORE sending it to the database, since you should NEVER store raw unencrypted reversable passwords in a DB. That's why password recovery should be about making them create a new one, NOT about recovering the old one!

    You should also check that ALL the $_POST values exist, not just the one. I'd probably have a hidden input "from" with different trigger values, but that's because I'm a "one index to rule them all" guy since that means a single entry point for ALL user accesses.

    I'd also lose that redirect asshattery -- I just don't get why people do that as if it's too hard to associate a hash with the session and invalidate it... but then AGAIN I'm a "one index" kind of guy so just what would I be redirecting to, itself?

    This is a bit loose, but roughly what I'd have:
    <?php
    
    session_start();
    session_regenerate_id(); // reduce window for MitM attack
    
    $db = new PDO(
    	'mysql:dbname=database;host=localhost',
    	'username',
    	'password'
    );
    
    if (
    	isset($_POST['from']) &&
    	$_POST['from'] == 'login' &&
    	isset($_POST['username']) &&
    	isset($_POST['password'])
    ) {
    	// NEVER return all, particularly the password!
    	$stmt = $db->prepare('
    		SELECT id
    		FROM users
    		WHERE username = :username
    		AND password = :password
    	');
    	$stmt->execute([
    		':username' => $_POST['username'],
    		':password' => hash('sha256', $_POST['password'])
    	]);
    	$_POST['password'] == ''; // prevent anything else from intercepting this!
    	if ($_SESSION['userId'] = $statement->fetchColumn()) {
    		// success handler here
    	} else {
    		// failed handler here
    	}
    } else {
    	// show info that the form was incomplete or invalid
    }
    Code (markup):
    Well, not really since I'd be trapping things a bit differently.
     
    deathshadow, Feb 18, 2016 IP
  11. ViggoAvatar

    ViggoAvatar Peon

    Messages:
    21
    Likes Received:
    0
    Best Answers:
    0
    Trophy Points:
    1
    #11
    the problem is that i dont really understand PDO, so it's kind of impossible to work with it if i would ever need to change anything.
    so, if i shouldnt redirect anywhere, what happens after a succesful login? (thats not sarcasm, i just really dont know what i should do instead of redirecting)
     
    ViggoAvatar, Feb 18, 2016 IP
  12. RapidVideo

    RapidVideo Greenhorn

    Messages:
    28
    Likes Received:
    0
    Best Answers:
    0
    Trophy Points:
    11
    #12
    Please secure your $_POST variables.... its not safe to get the data unprocessed trough mysql...

    if i were you i would do this:

    $username = preg_replace("![^@A-Za-z0-9]!is","", $_POST['username']);
    $password = md5($_POST['password']);

    or

    secure the $_POSt variables against mysql injections.
     
    RapidVideo, Feb 18, 2016 IP
  13. ketting00

    ketting00 Well-Known Member

    Messages:
    782
    Likes Received:
    28
    Best Answers:
    3
    Trophy Points:
    128
    #13
    http://php.net/manual/en/function.header.php

    That's where you should read about the redirect.

    Basically, from my code above:
    
    while($row = $result->fetch_object()) {
        // put this at the very end of the loop
        header('Location: my_profile.php');
    }
    
    Code (markup):
     
    ketting00, Feb 18, 2016 IP
  14. ketting00

    ketting00 Well-Known Member

    Messages:
    782
    Likes Received:
    28
    Best Answers:
    3
    Trophy Points:
    128
    #14
    @deathshadow ,

    Is this really working?
    I only see people do a query like this:
    
    SELECT
        id, username, password
    FROM
        users
    WHERE
        username = :username
        AND password = :password
    
    Code (markup):
    What's the benefit of this? Is it faster?

    Thanks,
     
    ketting00, Feb 18, 2016 IP
  15. deathshadow

    deathshadow Acclaimed Member

    Messages:
    9,732
    Likes Received:
    1,999
    Best Answers:
    253
    Trophy Points:
    515
    #15
    Returning less data is faster, but not the real reason. You already have the username, why pull it again? I mean if it matches, it matches. As to the password, you should NEVER pull the value stored in the database for security reasons. ANY query you build that goes anywhere NEAR that information should be mono-directional. Sending the value TO sql for verification and never even TRYING to pull the value code-side. Hence why right after running that query, I'd even delete the contents of $_POST['password'] as soon as done with it for good measure (hah, I did it in my sample above without even thinking about it!)

    It's why in logins I trap and check before I even let anything output result information.

    The reason I wouldn't redirect is that I tend to use a technique called "one index to rule them all" -- much like Gollum with the mother-honking ring, I use ONE index.php for all page requests.

    But even if I weren't something like login trapping I'd have on EVERY page of a PHP system, (one index makes this easier), so success or no success you can just push on to the task at hand rather than screw around with redirects. I wouldn't even have a separate "login.php" since my include('user.php') would trap a login attempt, if failed load the failed message and show the menu again on the SAME url, if not a login attempt see if the user is logged in via their session ID, if that's set pull any user specific information the page might need, and then just show the action/page they were already on. You don't need a redirect for that.

    The only thing a redirect might get you is avoiding an accidental resubmit, but that's where sending a random hash in the form and comparing it against one in the session, then invalidating the one in the session on every request prevents that -- and a whole host of other nasties.

    Which is done FOR YOU if you use PDO or mysqli with prepared queries -- hell that very reason is part of why you're NOT supposed to use mysql_ functions and WHY you're supposed to use prepared queries WHENEVER you are putting a variable into a query. If you add the variable to the query string, you're doing it all wrong!

    Which is why all that regex nonsense shouldn't even be needed... much less MD5? REALLY? Say hello to Mr. Rainbow Table for me!
     
    deathshadow, Feb 20, 2016 IP
    ketting00 likes this.
  16. RapidVideo

    RapidVideo Greenhorn

    Messages:
    28
    Likes Received:
    0
    Best Answers:
    0
    Trophy Points:
    11
    #16
    That was an example, sir.... But i haven't learned about that prepared queries statement yet... Thank you for that, i learned new today... I do use a central index.php and mysqli for all requests that goes in.
     
    RapidVideo, Feb 20, 2016 IP