9
\$\begingroup\$

I just wrote the session class below. I humbly request for a review on how to make this even more secured/ how sessions are normally written. The code below works, but I would love to make it even more secure and robust.

<?php
session_start();

require_once('classes/function.php');
require_once('classes/user.php');

class Session{

    private $logged_in = FALSE;
    private $id ;
    private $user_data ;
    private $email ;

public function logout(){
    session_start();
    unset($_SESSION['id']);
    unset($this->id);
    session_destroy();
    $logged_in = FALSE;
    Misc::redirect('index.php');
}

public static function is_logged_in(){
    if(isset($_SESSION['id'])){
        $logged_in = TRUE;
        Misc::redirect('127.0.0.1/users/index.php');
    }
}

public static function login($email, $password){
    global $user;
    global $misc;

    if($misc->check_form()){
        echo $this->error;
    }
    if (!MIsc::check_email_format($email)) {
            echo "Email format is invalid";
          }
          $login_user = $user->authenticate($email, $password);
          if($login_user){
            $_SESSION['id'] = $this->id = $user->user_data['id'];
            $logged_in = TRUE;
            Misc::redirect('127.0.0.1/users/index.php');
          }
}
}

$session = new Session;
?>
\$\endgroup\$
  • 2
    \$\begingroup\$ Don't change the Question to match the answers. when the code is done being reviewed post a follow up question with all the changes \$\endgroup\$ – Malachi May 23 '14 at 15:07
10
\$\begingroup\$

You don't post the code that interacts with database, there is the main security concern. I hope you use PDO prepared statements, which will keep you safe from SQL Injections.

As for non-security-related problems, there are quite some of them.

Consider creating multiple functions for form submit processing, for example:

function process_form_submit($_POST)
{
    $input = $this->validate_user_input($_POST);
    if ($this->user->authenticate($input['email'], $input['password'])) {
        $this->log_user_in($this->user->getId());
    }
}

Then your functions

  1. do one thing per function
  2. are testable
  3. are able to throw exceptions properly

As for exceptions, you don't just echo problems, do things like

throw new FormSubmitException('Email format is invalid');

and call process_form_submit like this:

try {
    $session->process_form_submit($_POST);
} catch (Exception $e) {
    echo $e->getMessage(); // or any other error handling
}

Why? I suppose your code is in the middle of the page (since you just echo all the errors), what if you want to move it to the top in order to change page title if login fails. Then your echo will show the error above page design and you will have to look for every single echo in all of classes used in order to fix this. Code like I provided will let you move it up and replace a single echo with $error = and the task will be done painlessly. Not even mentioning test-ability improvement.

Don't use global $var;, use Dependency injection instead in constructor or factory:

$session = new Session($user, $misc);

If you don't do that than your session class keeps depending on them implicitly and it makes you unable to just copy it into some other project. If you try - you will get errors here and there saying it is missing something that is not clear from it's initialization. Would not it be better to write a good class once in such a way that you can then use it in other projects?

As a last hint, start documenting your code with phpDocumentor standards in mind. It is a good habit and the sooner you start doing it, the easier it will be for yourself to work with code you write today in a few years, when you won't remember a thing about how it works.

\$\endgroup\$
  • 1
    \$\begingroup\$ Nice, Well Formed Answer \$\endgroup\$ – Malachi May 27 '14 at 13:59
7
\$\begingroup\$

First thing to do is use proper indentation for your class.


Remove indentation on the code after your second if statement in your login function.


you probably don't need to start the session in the logout function

public function logout(){
    session_start();
    unset($_SESSION['id']);
    unset($this->id);
    session_destroy();
    $logged_in = FALSE;
    Misc::redirect('index.php');
}

it doesn't make sense, the session should have been started on login after authentication.


This isn't a simple is_logged_in function it is also redirecting the user, I would never think to look here for a redirect setting.

public static function is_logged_in(){
    if(isset($_SESSION['id'])){
        $logged_in = TRUE;
        Misc::redirect('127.0.0.1/users/index.php');
    }
}

you don't even use this function in your code, from what I can see.

is it extra code floating around? If so get rid of it, extra code is not good in terms of security, it gives hackers extra code to play with.

\$\endgroup\$
  • \$\begingroup\$ I use is_logged_in to check if $_SESSION['id'] isset, if so, redirect to a different page. This is called on the login page/register page. If this is not good enough, how can i make it better? \$\endgroup\$ – hello May 23 '14 at 15:04
  • \$\begingroup\$ you should us it in your login function, if you are using it then it is okay, but you could minimize code if you used it in the login function \$\endgroup\$ – Malachi May 23 '14 at 15:06
  • 1
    \$\begingroup\$ @dotman14 I think that what Malachi is trying to say is that the function is called "is_logged_in" but it does more than just checking if the user is logged in or not. I would expect the method to be named "redirect_if_logged_in". \$\endgroup\$ – Simon Forsberg May 27 '14 at 15:29
  • \$\begingroup\$ @SimonAndréForsberg I will make changes and post a new code for review. Thanks. \$\endgroup\$ – hello May 27 '14 at 15:35
  • 1
    \$\begingroup\$ @dotman14 be sure to comment on your post here with a link to the new question so we can follow along. \$\endgroup\$ – Malachi May 27 '14 at 16:17

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.