0

My backend login screen logs in when I fill something in, literally, something. It doesn't make sense, I connect to my database, I check if the form has been submitted, I then perform my query and I even have a session who checks if I am logged in. Maybe it is that I am using POST instead of GET? I have no idea to be honest. What am I doing wrong?

<?php
if( !isset( $_SESSION ) ) session_start();

$msg='';
*db information*
$conn = new mysqli( $dbhost, $dbuser, $dbpass, $dbname );
if ( $conn->connect_error ) die("Connection failed");


if( isset( $_POST['submit'] ) ) {

$uname = $_POST['username'];
$wwoord = $_POST['wachtwoord'];
$query = "SELECT * FROM Medewerkers WHERE medewerker_username='$uname' && medewerker_password='$wwoord'";

$result = $conn->query( $query );

if( $result ) {
    $_SESSION['ingelogd'] = true;
    header("location: adminpanel.php");
} else {
    $msg="Inloggegevens incorrect.";
}
$conn->close();
}
?>

<html lang="en">
<head>
<meta charset="UTF-8">
<title>Admin login</title>
<link rel="stylesheet" type="text/css" href="tables.css">
</head>
<body>
<div id="content">
<ul>
    <li><a href="index.php">Admin panel</a></li>
</ul>
<h1>Admin login</h1>
<?php
echo $msg;
?>
<form role="form" method="post" action="index.php" class="contactForm">
    <table>
        <tr>
            <td><label for="username">Username</label></td>
            <td><input type="text" name="username" class="" id="username">  <br><br></td>
        </tr>
        <tr>
            <td><label for="wachtwoord">Wachtwoord</label></td>
            <td><input type="password" name="wachtwoord" class="" id="wachtwoord"><br><br></td>
        </tr>
        <tr>
            <td><button type="submit" name="submit"     class="button">Inloggen</button><br></td>
        </tr>
    </table>
</form>
</div>
</body>
</html>
lucafj2j282j
  • 879
  • 3
  • 13
  • 32
  • 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 Feb 11 '16 at 13:13
  • You need to do some basic error checking, you're assuming your query is working. – Jay Blanchard Feb 11 '16 at 13:15
  • it is because you do not test the validity of $result – Professor Abronsius Feb 11 '16 at 13:15
  • You are checking if the query executed, it did, it just didn't find any results. – Epodax Feb 11 '16 at 13:17
  • `if( !isset( $_SESSION ) ) session_start();` will give you issues. Just do `session_start();` – Jay Blanchard Feb 11 '16 at 13:17
  • I am not really an expert in PHP so forgive me for my mistakes. Oh how bad is this, @RamRaider you are so right! I am not checking result.. – lucafj2j282j Feb 11 '16 at 13:18
  • @JayBlanchard someone on StackOverflow told me to use 'if(!isset( $_SESSION) ) session_start();' instead of 'session_start();' – lucafj2j282j Feb 11 '16 at 13:20
  • It costs nothing to just start the session. If the session is not started `$_SESSION` will not be set. – Jay Blanchard Feb 11 '16 at 13:20
  • I don't see why that should cause issues – Professor Abronsius Feb 11 '16 at 13:20
  • If you want to use sessions, you should aways start with `session_start()`, if you omit it, no session data will be stored. – ar34z Feb 11 '16 at 13:21
  • [Your script is at risk for SQL Injection Attacks.](http://stackoverflow.com/questions/60174/how-can-i-prevent-sql-injection-in-php) Learn about [prepared](http://en.wikipedia.org/wiki/Prepared_statement) statements for [MySQLi](http://php.net/manual/en/mysqli.quickstart.prepared-statements.php). – Jay Blanchard Feb 11 '16 at 13:24
  • I hate when people say *"I'm not that far along..."* or *"This site will not be public..."* or *"It's only for school, so security doesn't matter..."*. If teachers and professors are not talking about security from day one, they're doing it wrong. Challenge them. They're teaching sloppy and dangerous coding practices which students will have to unlearn later. I also hate it when folks say, *"I'll add security later..."*. If you don't have time to do it right the first time, when will you find the time to add it later? – Jay Blanchard Feb 11 '16 at 13:25
  • @JayBlanchard Will the `if( !isset( $_SESSION ) ) session_start();` not allow the script to be included / required in other files that might all ready have startet the session? (While still being used independently)? – Epodax Feb 11 '16 at 13:27
  • @JayBlanchard you are right. My teacher tought us a lot about security and I find it really interesting! For example; I know I should use real_escape_string so I am less vulnerable to SQL injection. – lucafj2j282j Feb 11 '16 at 13:28
  • Add error reporting to the top of your file(s) right after your opening PHP tag for example ` – Funk Forty Niner Feb 11 '16 at 13:29
  • @Epodax there is that possibility. It is also possible that without the session already being started that you cannot test the session array at all. – Jay Blanchard Feb 11 '16 at 13:31

2 Answers2

3

I think you need to check within the php after you have executed the sql to make sure you have returned a valid recordset rather than a boolean $result.

<?php
    if( !isset( $_SESSION ) ) session_start();
    if( isset( $_SESSION['ingelogd'] ) ) header("location: adminpanel.php");

    $msg='';

    $conn = new mysqli( $dbhost, $dbuser, $dbpass, $dbname );
    if ( $conn->connect_error ) die("Connection failed");


    if( isset( $_POST['submit'] ) ) {

        $uname = $_POST['username'];
        $wwoord = $_POST['wachtwoord'];
        $query = "SELECT * FROM `Medewerkers` WHERE `medewerker_username`='$uname' and `medewerker_password`='$wwoord' limit 1";

        $result = $conn->query( $query );

        if( $result ) {

            /* check that there is a valid recordset with exactly 1 record */
            if( $result->num_rows==1 ) {
                $_SESSION['ingelogd'] = true;
                header("location: adminpanel.php");
            }
            $msg='login failed';
        } else {
            $msg="Inloggegevens incorrect.";
        }
        $conn->close();
    }
?>


<?php
    /*
        if sql injection is a concern ( which it is ) then rather than embedding
        variables directly within the sql it is better to utilise prepared statements.
    */


    session_start();
    if( isset( $_SESSION['ingelogd'] ) ) header("location: adminpanel.php");

    $msg='';
    $conn = new mysqli( $dbhost, $dbuser, $dbpass, $dbname );
    if ( $conn->connect_error ) die("Connection failed");


    if( isset( $_POST['submit'] ) ) {

        $uname = $_POST['username'];
        $wwoord = $_POST['wachtwoord'];

        /* to avoid sql injection use prepared statements - add ? as placeholders for variables */
        $sql='select * from `medewerkers` where `medewerker_username`=? and `medewerker_password`=? limit 1';

        /* prepare the sql statement for execution */
        $stmt=$conn->prepare( $sql );

        /* If there is a problem preparing the statement it will return false */
        if( $stmt ) {

            /* assign variables to the placeholders */
            $stmt->bind_param( 'ss', $uname, $wwoord );

            /* execute the query */
            $result=$stmt->execute();

            /* The statement must have executed successfully, test recordset next */
            if( $result ) {

                /* store recordset so we can use access other functions / properties */
                $stmt->store_result();

                /* check that there is a valid recordset with exactly 1 record */
                if( $stmt->num_rows==1 ) {

                    /* we have a valid logon - one record retrieved */
                    $conn->close();

                    $_SESSION['ingelogd'] = true;
                    header("location: adminpanel.php");
                }
                $msg='login failed';
            } else {
                $msg="Inloggegevens incorrect.";
            }
            $conn->close();
        }
    }
?>
Professor Abronsius
  • 33,063
  • 5
  • 32
  • 46
  • You need to fix that session check at the top of the code. – Jay Blanchard Feb 11 '16 at 13:19
  • true enough - though I have found when including files or calling files over ajax that strange things happened with sessions, this seemed to resolve that. Anyway, that is besides the point - the op needs to check for the result from the recordset rather than acting on the boolean value of $result – Professor Abronsius Feb 11 '16 at 13:25
  • *Sigh*, true, but the code, as it exapnds, could become problematic later on. If the test fails for any reason all of the login functionality gets skipped. – Jay Blanchard Feb 11 '16 at 13:28
  • @RamRaider I just tried your code and it works! Edit: But Jay Blanchard told me that I should replace if( !isset( $_SESSION ) ) session_start(); for session_start(); – lucafj2j282j Feb 11 '16 at 13:33
  • ah, because you didn't check with num_rows() @Lucafraser as RamRaider added `if( $result->num_rows==1 )` - I just noticed now, the difference between yours and his. – Funk Forty Niner Feb 11 '16 at 13:34
  • @JayBlanchard is correct - it isn't really needed here in this situation so ignore the chitchat between he and I and simply use `session_start()` ~ avoids any ambiguity or possibility for mistooks – Professor Abronsius Feb 11 '16 at 13:38
  • @Lucafraser Personally, I'd do `session_start(); if( isset( $_SESSION['ingelogd'] ) ){ header("location: adminpanel.php"); exit; } else { // do something else }` and get rid of `if( !isset( $_SESSION ) )` and using proper bracing. Not doing so, could cause problems. – Funk Forty Niner Feb 11 '16 at 13:39
  • @Fred-ii- ~ any repercussions about that dubious question/answer last night? – Professor Abronsius Feb 11 '16 at 13:42
  • @RamRaider Not yet. I'll bet they're all talking about it over coffee and donuts and will probably mark my flag as helpful, while keeping the question open. *Mark my words* ;-) Something really really stinks in there. There's no history of who/when the question was reopened. Time will tell ;-) – Funk Forty Niner Feb 11 '16 at 13:44
0

You're only checking if the query is working, but not if it returns rows.

Instead of:

//...
$result = $conn->query( $query );

if( $result ) {
    $_SESSION['ingelogd'] = true;
//...

Do this:

//...
$result = $conn->query( $query );

if($result->num_rows == 1) {
    $_SESSION['ingelogd'] = true;
//...

Of course, there are other things to be improved in your code, like the SQL Injection part where you get the POST data without escaping it, but I'm focusing in your exact problem.

Phiter
  • 14,570
  • 14
  • 50
  • 84