Implementing a login on index page

Question

I am trying to implement a login in my index page. The code runs through with no errors. My issue is after a user submits they are not being logged in. I appreciate everyone's comments, I am new to stackoverflow, not to coding, so I am still trying to grasp the community and how it works..

<?php
session_start()
if($_POST['submit']=='Login')
{
 //Login form submitted??

$err = array();
 //Hold errors


if(!$_POST['username'] || !$_POST['password'])
    $err[] = 'All the fields must be filled in!';

if(!count($err))
{
    $_POST['username'] = mysql_real_escape_string($_POST['username']);
    $_POST['password'] = mysql_real_escape_string($_POST['password']);


     //Data escaping

    $row = mysql_fetch_assoc(mysql_query("SELECT id,usr FROM tz_members WHERE usr='{$_POST['username']}' AND pass='".md5($_POST['password'])."'"));

    if($row['usr'])
    {
         //If kosher, Login

        $_SESSION['usr']=$row['usr'];
        $_SESSION['id'] = $row['id'];


         //Session storing data

        setcookie('tzRemember');
    }
    else $err[]='Wrong username and/or password!';
}
if($err)
$_SESSION['msg']['login-err'] = implode('<br />',$err);
 //Save errors in session

header("Location: login.php/");
exit;
}

I feel I am overlooking something, simple. I have been putting this off for a while. — jmr333, Jun 05 '14 at 22:01
How am I wasting your time, I had a question, came to Stackoverflow for some guidance and get insulted... — jmr333, Jun 05 '14 at 22:09
there is actually a forum starting up about how you insult people with very valid questions.. — jmr333, Jun 05 '14 at 22:13
Hey, I see no probs with your question, except that I'd edit it and write "and the problem is that even after this, the user is not logged in"... just so people are clear (when reading the question) what the issue is (without having to read all the comments too). — Taryn East, Jun 06 '14 at 01:23

score 3 · Accepted Answer · answered Jun 05 '14 at 22:04

you have many errors (for me) in your code. First, if you do not open the session_start (), sessions will never work. Second, the condition of "if" I do not think is correct because not have comparison method. I hope it can give you a hand

<?php
    if($_POST['submit']=='Login')
    {
     //Login form submitted??

    $err = array();
    // Hold errors


    if(!$_POST['username'] || !$_POST['password'])
        $err[] = 'All the fields must be filled in!';

    if(!count($err))
    {
        $_POST['username'] = mysql_real_escape_string($_POST['username']);
        $_POST['password'] = mysql_real_escape_string($_POST['password']);


         //Data escaping

        $row = mysql_fetch_assoc(mysql_query("SELECT id,usr FROM tz_members WHERE usr='{$_POST['username']}' AND pass='".md5($_POST['password'])."'"));

        if(!empty($row['usr']))
        {
           //  If kosher, Login
            session_start();
            $_SESSION['usr']=$row['usr'];
            $_SESSION['id'] = $row['id'];


             //Session storing data

            setcookie('tzRemember');
        }
        else $err[]='Wrong username and/or password!';
    }
    if(!empty($err)){
    session_start();
    $_SESSION['msg']['login-err'] = implode('<br />',$err);
    }
     //Save errors in session

    header("Location: login.php/");
    exit;
    }?>

score 3 · Answer 2 · edited Jun 20 '20 at 09:12

Despite you not asking a distinct question and rather just asking for a code review (much better to do this on codereview) here is my opinion.

You has a couple of obvious issues,

Firstly like napster3world said you really need to start your session or nothing of much use will come of your error reporting.
Secondly you are wide open for all sort of malicious attacks this kinda undermining having a secure login.

While by no means exhaustive the following contains a couple of suggestions to help make it a little securer.

Detach your variables from the POST global - making it more obvious in the input is tainted.
Using a PDO database connection as this is much more secure method of connecting to your database than mysql_connect especially when using bound parameters to protect against sql injection (See the discussion here)

Code

The following is a quick ready that hopefully will point you in the right direction and improve some of you security.

    // Start the session for storing your errors
    session_start();

    // Check that the button was clicked on the form
    if (isset($post)) {

    // Array for storing any errors
    $err = array();

    // Extract details from POST global
    $_username = $_POST['username'];
    $_password = $_POST['password'];

    /*
    You may want to consider some filtering here
     */
    
    // Did the user fill in the username field?
    if (empty($_username)) {
        $err['username'] = "User name not provided";
    }

    // Did the user fill in the password field?
    if (empty($_password)) {
        $err['password'] = "Password not provided";
    } else {
        // Yes so hash it for the database check
        $hashedPassword = md5($_password);
    }

    if (empty($err)) {

        // Establish database connection
        try {
            $dsn = "mysql:host{$host};port={$port};dbname={$database}";
            $connection = new PDO($dsn, $dbUsername, $dbPassword, array(PDO::MYSQL_ATTR_INIT_COMMAND => "SET NAMES utf8"));
        } catch (PDOException $e) {
            throw new \Exception("Unable to connect to the Database"); 
        }
        
        // Build SQL query and run on PDO connection
        $sql = "SELECT id, usr FROM tz_members WHERE usr = :username AND pass = :hashedPassword";
        
        try {
            $stmt = $connection->prepare($sql);
            // Bid your parameters to prevent sql injection
            $stmt->bindParam(':username', $_username);
            $stmt->bindParam(':hashedPassword', $hashedPassword);
            $stmt->execute();
        } catch (PDOException $e) {
            throw new Exception("Error with executing query: {$e->getMessage()}");   
        }
        
        // Fetch your results 
        $row = $stmt->fetch(PDO::FETCH_ASSOC);

        if (!empty($row)) {

            // Fill the session up with users details
            $_SESSION['id'] = $row['id'];
            $_SESSION['usr'] = $row['usr'];

            // Head back to the login page - surely you wan to head to your protected page?
            header("Location: login.php/");
            return;
        }

        // Login failed
        $err['login'] = "Wrong username and/or password!";
    }

    // Head back to the login page
    $_SESSION['errors'] = $err;
    header("Location: login.php/");
    return;
}

Implementing a login on index page

2 Answers2

Code

Further reading