-1

My Log in form works even with the wrong username and password. I have tried searching for answers but I can't find my mistake. Here's the code:

<?php
include 'database.php'; ?>

<?php

    if($_SERVER["REQUEST_METHOD"] == "POST"){
        $username = mysqli_real_escape_string($connect,$_POST['username']);
        $password = mysqli_real_escape_string($connect,$_POST['password']);

        $query = "SELECT username, password FROM tbl_membership WHERE username = 
        '$username' AND password = '$password'";
        $result = mysqli_query($connect, $query);
        $row = mysqli_fetch_array($result,MYSQLI_ASSOC);
        $active = $row['active'];

        $count = mysqli_num_rows($result);

        if(mysqli_num_rows($query) > 0){
            $_SESSION["username"] = $username;
            $_SESSION["password"] = $password;
            header("location: welcome.php");
            exit();
        }if (mysqli_num_rows($query) != 0){
            echo "Invalid.";
        }
    }
?>
Paul Karam
  • 4,052
  • 8
  • 30
  • 53
Via
  • 1
  • 2
  • **Never store passwords in clear text!**. Only store password hashes! Use PHP's [`password_hash()`](http://php.net/manual/en/function.password-hash.php) and [`password_verify()`](http://php.net/manual/en/function.password-verify.php) . If you're running a PHP version lower than 5.5 (which I _really_ hope you aren't), you can use the [password_compat library](https://github.com/ircmaxell/password_compat) to get the same functionallity. – M. Eriksson Jun 27 '17 at 05:48
  • Learn about prepared statements to prevent SQL injection – Jens Jun 27 '17 at 05:48
  • Use $count instead of mysqli_num_rows($query) in if – Jens Jun 27 '17 at 05:50
  • `mysqli_num_rows($query)`.. `$query` is a string... – M. Eriksson Jun 27 '17 at 05:50
  • Have you checked your error log? A good idea is also to turn `display_errors` on in your local PHP environment. Read more here: [How do I get PHP errors to display?](http://stackoverflow.com/questions/1053424/how-do-i-get-php-errors-to-display). That will most likely give you some errors and warnings. – M. Eriksson Jun 27 '17 at 05:52
  • You should actually rewrite your code using prepared statements (as @Jens suggests) and properly hashing the passwords before you continue. That will drastically change your code so your current issue will most likely not be relevant anymore. – M. Eriksson Jun 27 '17 at 05:54

2 Answers2

1

1:

Take a look at this line:

if(mysqli_num_rows($query) > 0) {

The variable $query is a string here. You will need to use the $count variable you've just set.

// Check if we have a result and just one result
if($count == 1) {

2:

You will only need to get the row contents if it has one result.

$count = mysqli_num_rows($result);
if ($count == 1) {
    $row = mysqli_fetch_array($result,MYSQLI_ASSOC);
    $active = $row['active'];
}

3:

Try to use prepared statements to avoid SQL injection. Escaping your strings is a good start in this case.

$stmt = $conn->prepare("SELECT username, password FROM tbl_membership WHERE username =? AND password =?");

$stmt->bind_param('ss', $username, $password);

$stmt->execute();
$get_result = $stmt->get_result();
$row_count = $get_result->num_rows;
if ($row_count == 1) {
    $row = $get_result->fetch_assoc();
    $active = $row['active'];
    $_SESSION["username"] = $username;
    header("location: welcome.php");
    exit();
} else {
    echo "Invalid.";
}

4:

Don't save the passwords as plain text. This is strongly advised against and creates security issues for your customers/visitors. Use some form of hashing instead. Read the documentation for more info

Peter
  • 8,776
  • 6
  • 62
  • 95
JYoThI
  • 11,977
  • 1
  • 11
  • 26
0
if($_SERVER["REQUEST_METHOD"] == "POST"){
    $username = mysqli_real_escape_string($connect,$_POST['username']);
    $password = mysqli_real_escape_string($connect,$_POST['password']);

    $query = "SELECT username, password FROM tbl_membership WHERE username = 
    '$username' AND password = '$password'";
    $result = mysqli_query($connect, $query);
    $row = mysqli_fetch_assoc($result);       

    $count = mysqli_num_rows($result);

    if($count > 0){
        $_SESSION["username"] = $row['username'];
        $_SESSION["password"] = $row['password'];//no need to use password in session
        header("location: welcome.php");

    }else{
        echo "Invalid.";
    }
}
Anandh L
  • 1
  • 1
    No explanation at all? A good answer includes an explanation about what you've changed, why and how it would solve the issue. Don't just post a block of code since it's not constructive. Btw, the OP shouldn't use this either way since it's not using prepared statements and is still relying on using clear text passwords. – M. Eriksson Jun 27 '17 at 06:05
  • @MagnusEriksson My teacher told me there were no bad questions and good answers. An answer is valid when it 'answers' the question. Some explanation is pretty usefull though :) – Peter Jun 27 '17 at 06:46
  • @Peter Not to be _that_ guy, but I strongly disagree with your teacher. :-) – M. Eriksson Jun 27 '17 at 06:48