-3

I am writing a login/register system for my first php project and I am facing problems in login.php. Here is my login.php:

<?php
   include("db.php");
   session_start();
   if(isset($_SESSION['login_user'])){
      header("Location: welcome.php");
   }

   $error = '';
   if (isset($_POST['submit'])) {
      if (empty($_POST['username']) or empty($_POST['password'])) {
           $error = "Please enter your login details";
     } else {

        $username = mysqli_real_escape_string($conn, $_POST['username']);
        $password = md5($_POST['password']);



        $query = mysqli_prepare($conn, "SELECT password FROM workers WHERE user_name=?");
        mysqli_stmt_bind_param($query,"s",$username);
        mysqli_stmt_execute($query);
        mysqli_stmt_bind_result($query,$pass);
        if(mysqli_stmt_fetch($query)){
            if ($password == $pass){
                header('Location:welcome.php');
                $_SESSION['login_user'] = $username;

            }else {
                $error = "You typed the wrong password";
                unset($username, $password);
            }
        }else{
            $error = "User Login doesn't exists";
            unset($username,$password);
        }

    }
}
?>

The problem is that after entering existing username/password it just refreshes the page instead of heading to the welcome.php. I checked with an incorrect username/password and it displayed an error as expected, so I don't think the problem is in the MySQL queries. How can I find the problem with a code and explain why it happened?

halfer
  • 19,824
  • 17
  • 99
  • 186
ProPall
  • 137
  • 1
  • 13
  • 3
    are you intending on going live with this, or is this purely for academic purposes? – Funk Forty Niner Jul 28 '17 at 18:35
  • 3
    ***You really shouldn't use [MD5 password hashes](http://security.stackexchange.com/questions/19906/is-md5-considered-insecure)*** and you really should use PHP's [built-in functions](http://jayblanchard.net/proper_password_hashing_with_PHP.html) to handle password security. Make sure you [don't escape passwords](http://stackoverflow.com/q/36628418/1011527) or use any other cleansing mechanism on them before hashing. Doing so *changes* the password and causes unnecessary additional coding. – Jay Blanchard Jul 28 '17 at 18:37
  • Location redirects should also be followed immediately by `exit()` - that's a race condition bug right there. – halfer Jul 28 '17 at 18:41
  • 1
    **WARNING**: Writing your own access control layer is not easy and there are many opportunities to get it severely wrong. Please, do not write your own authentication system when any modern [development framework](http://codegeekz.com/best-php-frameworks-for-developers/) like [Laravel](http://laravel.com/) comes with a robust [authentication system](https://laravel.com/docs/5.4/authentication) built-in. At the absolute least follow [recommended security best practices](http://www.phptherightway.com/#security) and **never store passwords with a uselessly weak hash like SHA1 or MD5**. – tadman Jul 28 '17 at 18:43
  • You're using placeholder values in prepared statements, which is great, but you're *also* using the manual escaping functions which is not only not necessary but harmful as it mangles your data and introduces backslashes and other characters where none should exist. – tadman Jul 28 '17 at 18:44
  • @tadman Sorry but "use a framework" is one of the worst pieces of advice to give a beginner. It's the reason we have so many people in the Laravel section asking so many simple questions on basic PHP functions and syntax. Nobody should ever use any frameworks or pre-built code until they know enough to do anything the framework can do in pure PHP. – WheatBeak Jul 28 '17 at 18:47
  • 3
    That, unfortunately, is unrealistic @WheatBeak I am not an advocate of frameworks, but so many beginners get themselves and their users in trouble when they try to code access control layers for their applications because they are trying to achieve a goal, not learn to code. – Jay Blanchard Jul 28 '17 at 18:52
  • @WheatBeak Frameworks are more beginner friendly than hacking away on your own half-baked authentication system that anyone can can-opener in seconds with off-the-shelf tools. It's not a bad thing that Laravel people are asking basic PHP questions, *that means they're learning*, which I hope you can agree is a good thing. That they're not learning things in the order you'd prefer is irrelevant. – tadman Jul 28 '17 at 19:02
  • @WheatBeak This PHP culture of "kill your own food" is really damaging to beginners. We don't force people to build their own car before learning to drive. We don't teach people to grow wheat before teaching them how to bake bread. You have to start *somewhere* in the abstraction spectrum, and given the risks of doing an authentication system badly, using a canned one that's well documented and tested is better than the alternative. – tadman Jul 28 '17 at 19:04
  • @JayBlanchard Exactly. If you want to learn PHP from the ground up there's better places to start than writing a login system. If you want to secure your website then doing it while learning PHP from the ground up is counter-productive. Your client only cares that your site is secure, not that you learned a lot while doing it, as learning often means a lot billable time spent re-inventing the wheel, and often dong it badly. Knowing the fundamentals of any language you're using helps you be more effective, but you can pick that up over time as you need to. – tadman Jul 28 '17 at 19:05
  • I guess we'll have to agree to disagree. I feel the same about it as I feel about people who download WordPress, buy a white-label theme, and call themselves a developer. A third of my business is rescuing people from 'developers' who got in over their head and created an inferior product. Although I guess technically that's a good thing for me :P – WheatBeak Jul 28 '17 at 19:31
  • @Fred purely for academic purposes.Currently learning Web Development languages so I just making it to practice in PHP. – ProPall Jul 28 '17 at 19:49

2 Answers2

1

no code should be executed after header was sent.

$_SESSION['login_user'] = $username;
header('Location:welcome.php');
exit();

well, and consider those comments good people took time to post ;)

Inna Tichman
  • 606
  • 5
  • 12
  • I did it , It redirects to an empty page with the same url a.k.a login.php. It doesn't change to the welcome.php and doesn't display contents of the welcome.php. – ProPall Jul 29 '17 at 07:20
0
I will suggest code like this

<?php
   include("db.php");
   session_start();
   $error = '';
   if(!isset($_SESSION['login_user'])){
      if (isset($_POST['submit'])) {
      if (empty($_POST['username']) or empty($_POST['password'])) {
           $error = "Please enter your login details";
     } else {

        $username = mysqli_real_escape_string($conn, $_POST['username']);
        $password = md5($_POST['password']);



        $query = mysqli_prepare($conn, "SELECT password FROM workers WHERE user_name=?");
        mysqli_stmt_bind_param($query,"s",$username);
        mysqli_stmt_execute($query);
        mysqli_stmt_bind_result($query,$pass);
        if(mysqli_stmt_fetch($query)){
            if ($password == $pass){
                header('Location:welcome.php');
                $_SESSION['login_user'] = $username;

            }else {
                $error = "You typed the wrong password";
                unset($username, $password);
            }
        }else{
            $error = "User Login doesn't exists";
            unset($username,$password);
        }

    }
}
   }else
   {
       header("Location: welcome.php");
   }
?>

In your heade function is used in top and when it check it always gives header location error.
Remove this by taking whole your code in !isset.