Very simple login script review

Question

I'm relatively new to PHP and have recently created a small website for friends and family to upload and share photos, protected by a very simple login system.

While I'm aware what I'm doing isn't best practice and you'd never use this in a "published" site, I'd like to use this as learning experience to improve my coding, and would appreciate any pointers as to best practice and efficiency/security improvements.

The website contains no sensitive information and the login was purely to prevent unwanted uploading of files. The username isn't checked against anything and is only used to log who's uploaded a file. The password is shared for all users.

This script, login.php, is included at the top of each page and checks if the user is logged in - if not then redirect to homepage where the login form is displayed. If already logged in, then display some internal links instead of form.

if(!isset($_SESSION)){ session_start(); }

/***********************************************************/
//check if home page
if (/*code to check if outside root directory*/) }{$notHomePage = true;}

if($notHomePage and ($_SESSION['allow'] == false or !isset($_SESSION['allow'])) ) {
    $_SESSION['error'] = 'Please enter your name and password!';
    header('Location: index.php');
    exit();
   }

/***********************************************************/
function readCookie() {
    if (isset($_COOKIE["username"])) {
        $name = $_COOKIE["username"];
    }
    return $name;
}

/***********************************************************/
function loginForm() {
    if($_SESSION['allow']) {
        //display internal links and logout link
        echo "<p><a href=\"login.php?logout\">Logout</a></p>";
    }
    else {
        if(isset($_SESSION['error'])) {
            echo "<div>" . $_SESSION['error'] . "</div>";
            unset($_SESSION['error']);
        }

        $name = readCookie();
        echo "<form method=\"post\" action=\"login.php\">
            <input type=\"text\" value=\"$name\" name=\"name\"><label>Name</label>
            <input type=\"password\" name=\"password\"><label>Password</label>
            <button type=\"submit\" name=\"submit\">Login</button>
        </form>";
    }
}

/***********************************************************/
function login() {
    $days = 30;
    if(trim($_POST['name']) == "" || trim($_POST['password']) == "") {
        $_SESSION['error'] = 'Please enter your name and password!';
        header('Location: index.php');
        exit();
    }

    $name = $_POST['name'];
    $password = $_POST['password'];

    $pass = "password"; //passsword goes here

    if ($password != $pass) {
        $_SESSION['error'] = 'Please enter the correct password!';
        header('Location: index.php');
        exit();
    }
    else {
        $_SESSION['allow'] = true;
        header('Location: index.php');
    }

    if($days) $expire = time()+60*60*24*$days;
    setcookie("username", $name, $expire,"/");
}
/***********************************************************/
if($_SERVER['REQUEST_METHOD'] == "POST") {
   login();
}
if($_SERVER['QUERY_STRING'] == "logout") {
    $_SESSION['allow'] = false;
    header("Location: index.php");
    exit;
}

?>

Answer 1

I'd like to think security as a thing that at first refuses everything, and after that permits something specific. Your code does the opposite at the moment. It checks that if the user is NOT logged in, and if the condition does not pass, the user is permitted to view the page.

I'd do the opposite. I would first check that if the user IS logged in, and only after that permit the user to view the page if the condition pass. It is more strict when it comes to potential bugs, which means that if something goes wrong, the user will not be permitted anyhow.

Second thing, you really do not want to tell the browser the user name in the cookie you set. Instead you should hash it with some secret salt if it really is necessary to send it to the browser. Remember, every detail you send to the browser is a potential security risk that someone (man-in-the-middle attack, viruses, other malware) might abuse.

And then, if that is a php script you include at the top of every page, why do you include the login form there? It seems to me that you only display it when you are on index.php. It is not needed elsewhere. Of course this is not a big issue, but it's the tiny little seed of bloated software that will grow in time if not treated carefully. And by this I mean that if you begin to add lots and lots of thing that is being used in only one place, it will make the script run so much slower.

Also, I see a potential bug there. If you ever wanted to add a POST form (say, a guestbook or chat or shoutbox or your own forum) on your website, you will trigger the login() function every time when ANY of those POST forms get submitted. The login() function will not then find the correct data in the POST variable, and will refuse to display the page.

Answer 2

1.) I see you using a mix of single and double quotes, but it looks rather arbitrary.

The difference between the two is, that you can use variables in double quoted strings (String Interpolation):

$username = 'Jon';
echo "Hello, $username. See what's new:";

This isn't possible with single quoted strings. Therefor single quoted string have slightly better performance and are more readable (since you already know that theres not going to be a variable hidden inside).

So you should generally use single quouted strings as a default and double quoted strings for interpolation.

But there's more:

echo "<form method=\"post\" action=\"login.php\">
            <input type=\"text\" value=\"$name\" name=\"name\"><label>Name</label>
            <input type=\"password\" name=\"password\"><label>Password</label>
            <button type=\"submit\" name=\"submit\">Login</button>
      </form>";

See how you need to escape every single double quotation mark? Now try the same with single quotes:

echo '<form method="post" action="login.php">
            <input type="text" value="'.$name.'" name="name"><label>Name</label>
            <input type="password" name="password"><label>Password</label>
            <button type="submit" name="submit">Login</button>
      </form>';

It's much cleaner, eventhough you now need to concatenate $name.

2.) Also, I'd recommend using the strict comparision ===, see: PHP equality vs. identity comparision.

3.) What does readCookie return, if the cookie isn't set? I think using the PHP null coalescing operator ?? to specify a default is cleaner:

function readCookie() {
    return $_COOKIE["username"] ?? '<UNKNOWN>';
}