3
\$\begingroup\$

This is something that I initially came up with to protect a bunch of scripts on my personal webserver, but am now planning on using with a few public projects too.

login.php:

<?php
session_start();

//log out if ?logout=true
if($_GET["logout"]){
    $_SESSION["loggedIn"]=false;
    session_destroy();
    header('Location: '. $_SERVER['SCRIPT_NAME']);
    exit();
}

$password = '8f434346648f6b96df89dda901c5176b10a6d83961dd3c1ac88b59b2dc327aa4';

if(empty($_SESSION["loggedIn"])){
    if(isset($_POST["password"])){
        if(hash('sha256', $_POST['password'])===$password){
                session_regenerate_id(true);
                $_SESSION["loggedIn"]=true;
        } else{
            echo "Incorrect password! <br>";
        }   
    }
}

if($_SESSION["loggedIn"] == true){
print '
    You are logged in!<br>
    <a href="?logout=true">Logout</a>
    <!-- HTML page goes here-->
';
} else{
print ' <h3>Login</h3>
      <form action="#" method="post">
        Password: <input type="password" name="password"><br>
      <input type="submit" value="Login!" />
    </form>
';
}
?>

For other files which should only be accessible if the user is logged in:

OtherFiles.php:

<?php
session_start();
if($_SESSION["loggedIn"]==false){
    session_destroy();
    header('Location: login.php');
    exit();
} else {
    //stuff goes here
}

Yes, I know, sha-256 hashes aren't recommended for password storage, but that's only for testing, when deployed it'll be using proper salting for password storage. What I need feedback on is the login script itself.

\$\endgroup\$
  • 2
    \$\begingroup\$ Not a review but == loosely comparison can become quite dangerous when working with hashes it is better to use strict comparison ===. See whitehatsec.com/blog/magic-hashes \$\endgroup\$ – Ludisposed Aug 17 '18 at 13:42
  • \$\begingroup\$ @Ludisposed Yes, that slipped my mind. Thanks \$\endgroup\$ – rahuldottech Aug 17 '18 at 15:39
0
\$\begingroup\$

Perhaps use some strategies to counter CSRF as well. Take a look at this comprehensive guide: https://www.owasp.org/index.php/Cross-Site_Request_Forgery_(CSRF)_Prevention_Cheat_Sheet

\$\endgroup\$
0
\$\begingroup\$

session_start(); being set and called on a per-script basis is a poor approach. Prone to problems like forgetting to set it, setting it more than once when there are multiple layers of files being included().

The "system" should have services (and other "things" that are re-used) invoked before it gets to things such as login.php.

Though this is more a limitation of writing procedural code and not using modular setup with OOP etc.


You should choose a standard, like PSR2 that is more globally recognised. Consistency is absolute key, but the standards such as PSR2 are quite logical in their decisions and makes for better readability.

Such as:

// Yours
if($_GET["logout"]){

// PSR2
if ($_GET["logout"]) {

Why is "login.php" handling "logout"?
Even using procedural (instead of nice classes) doesn't mean you wont benefit from things like SRP.

You should have a "logout.php" file and "login.php" file.

Though, you should use classes really. You say you're going to use this in some public projects, but you have no namespaces so would possibly have name clashes with others' code. And people don't want to require() your file they want to inject it in an OOP fashion.


You have no namespaces, so this is all in global namespace. Any other file in that name space (probably all of your other code) has potential for naming clashes. Which not only potentially causes issues, but ones that can be very hard to find and debug.


This seems very prone to things going wrong - 404 and possible other issues.

header('Location: '. $_SERVER['SCRIPT_NAME']);

I'd consider a good resource handler, but again this would really need classes otherwise you're going to be stuck including things.


As with all of your code (missing some kind of nice coding standard), this can be simplified and tidied to be more readable:

if(empty($_SESSION["loggedIn"])){
    if(isset($_POST["password"])){
        if(hash('sha256', $_POST['password'])===$password){
                session_regenerate_id(true);
                $_SESSION["loggedIn"]=true;
        } else{
            echo "Incorrect password! <br>";
        }   
    }
}

To this:

if (
    empty($_SESSION["loggedIn"])
    && isset($_POST["password"])
    && hash('sha256', $_POST['password']) === $password
) {
       session_regenerate_id(true);
       $_SESSION["loggedIn"] = true;

   } else {
       echo "Incorrect password! <br>";
   }
}

Yes, I know, sha-256 hashes aren't recommended for password storage, but that's only for testing, when deployed it'll be using proper salting for password storage

Calling a variable that stores the username $get_da_user_name_like is "not recommended". Using SHA-256 or similar is not "not recommended" it's entirely insecure.

Also, this approach means that once you're done writing your code and have it all tested, you'll... start writing your code and start testing again to add this other thing. Or won't because something else will be needed.

Honestly, this is a pointless thing to "put off" - it's 2 single PHP functions, built in.

// Store this in the DB
$hashed_password = password_hash($password);

// Check their login pass with the one stored in the DB
if (password_verify($passwordFromLoginForm, $passwordFromDb)) {

if($_SESSION["loggedIn"] == true){

Using a loose comparison == instead of strict === means this will be "true" for many values (almost everything, basically barring a few values it'll just be set and so true). It might not matter but is usually better to be strict in these matters. Even if not for security and just for clear intent.


Echoing out in a PHP file is not ideal. Even with procedural (no classes) treat PHP files as "controllers" that do the system things, and "view" files that output.

It's an old approach but set a variable to the desired output, then at the end call a new file (e.g. loginView.php) and have that display the HTML/CSS and data from the controller (login.php).

Then you can also call this file early and exit, eg when there are errors.


I will just presume this is for testing purposes:

$password = '8f434346648f6b96df89dda901c5176b10a6d83961dd3c1ac88b59b2dc327aa4';

I said it before but honestly, switch to using classes. Writing procedural code seems easier on face value, but in the long run it will without doubt cost you more time, headaches, bugs, and limitations.

\$\endgroup\$
  • \$\begingroup\$ Namespaces has nothing to do here as there are no classes. There are no "possible 404 or other issues". Login is handling logout because it's the same authentication process. You aren't using a distinct file for the every method in a class using your beloved OOP? Same here. Talking about OOP without giving an example is a waste of time. And an off topic too. Why the password hash is hardcoded in the script is clearly explained in the question \$\endgroup\$ – Your Common Sense Aug 21 '18 at 7:36
  • \$\begingroup\$ As a general rule, try to stay closer to the topic and have a proof for the every statement you make. \$\endgroup\$ – Your Common Sense Aug 21 '18 at 7:47
  • \$\begingroup\$ @YourCommonSense The lacking of namespaces have everything to do with a code review, because the lacking of them causes massive problems in codebases. I see potential issues due to no namespaces - is this not a review? Especially as they stated they might add this code to public projects. The login/logout is missing SoC and SRP, and while more of an issue with classes, whether a class, function, or file doesn't matter. It's just good organisational approach and good practice to have things laid out in an organised manner. Also as per: codereview.stackexchange.com/help/how-to-answer \$\endgroup\$ – James Aug 21 '18 at 9:40
  • \$\begingroup\$ "you aren't using a distinct file for the every method in a class using your beloved OOP?" File = class. "login.php/logout.php" = "class Login / class Logout" what happens in that class (methods) or that file (procedural code) should relate to the class or file. \$\endgroup\$ – James Aug 21 '18 at 9:41
  • \$\begingroup\$ I'd like to know why I was downvoted. This took me nearly an hour to go through their code and write out. I made some good suggestions on how their code is and could be better. It would be interesting to know why someone thinks I did not or this answer is bad for some reason. Again, before downvoting please see codereview.stackexchange.com/help/how-to-answer "In addition to criticisms, pointing out good practices in the code is also a form of helpful feedback." \$\endgroup\$ – James Aug 21 '18 at 9:52

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.