2
\$\begingroup\$

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;
}

?>
\$\endgroup\$
1
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.