Just wanted to run through my PHP session handling and get some feedback and tips with regard to what is good (if anything!), what could be better and what is either plain wrong, or using out of date methods.
Just as a bit of background, I have only ever dabbled with PHP (and many years ago that was) and want to try and embrace the OOP side of it.
Every page that is required to be part of a session has the following at the top:
<?php
include_once 'Session.php';
$session = new Session();
$session->authenticate();
?>
Session.php is as follows:
class Session {
const password = "5baa61e4c9b93f3f0682250b6cf8331b7ee68fd8";
private $sessionInfo;
public function __construct() {
session_start();
if (!isset($_SESSION['sessionInfo'])) {
$sessionInfo = new SessionInfo();
$_SESSION['sessionInfo'] = $sessionInfo;
} else {
$sessionInfo = $_SESSION['sessionInfo'];
}
}
public function authenticate() {
if (isset($_SESSION['loggedin'])) {
if ($_SESSION['loggedin'] === true) {
return;
}
}
header('Location: login.php');
}
public function logIn($password) {
if (sha1($password) === self::password) {
$_SESSION['loggedin'] = true;
header('Location: index.php');
}
}
public function logOut() {
unset($_SESSION['loggedin']);
session_destroy();
header('Location: login.php');
}
public function getSessionInfo() {
return $this->sessionInfo;
}
}
?>
SessionInfo is currently just a blank class, it's intended purpose is to act as an information store for the current session.
login.php:
<?php
include_once 'Session.php';
if(isset($_POST['submit'])) {
if(isset($_POST['password'])) {
$session = new Session();
$session->logIn($_POST['password']);
}
}
?>
<html>
<body>
<form method="post" action="<?php echo $_SERVER['PHP_SELF'];?>">
<input type="password" name="password" />
<input type="submit" name="submit" />
</form>
</body>
</html>
Do I need to call die() (or exit() seeing as they are identical) after a failed authenticate to terminate any page which may attempt to load? Or do I need to return a boolean and then terminate from the calling page?
Please disregard the use of SHA1 and the hashed password in the file, I will be extending this to use a MySQL background, and hope to incorporate the use of a salted hash. I will be using prepared statements, will this be sufficient to protect from SQL injection and allow me to not bother filtering $_POST[...]?
Also, I realise that the location headers may better be stored as a class constant in case I wish to change the locations.
2 Answers 2
Let's see the code:
Your
Sessionclass does too many things:- Session management
- Login
- Data validation
- Logout
- Authentication
- Inclusion management
Your class has no clean way to destroy a session.
- You don't verify if a session is active.
- Your constructor seems a little off. Sessions are a singleton. Use a single
static finalclass. - You don't need to repeat
publicon every method; - You are storing a class on a
$_SESSION. I would avoid that like plague. You should start and end the session. And you should have a method to save the session (that runssession_write_close()on shutdown or at any given time). - There's no way to check if a session was opened before or not, or if it was saved already or not.
Fixing these points and splitting your code into classes that do each thing in separate will make your code better in every conceivable way.
A while ago I've used JSON to store the session data. It generates files a bit longer, but it is faster to save. Consider making your own session management system, based out on PHP's way.
Since HDD is cheaper than CPU cycles, I would consider using such way to load/save your sessions.
Completely outside the scope of this review, you could use a RAMdisk with the /tmp partition to squeeze every tiny piece of performance you get. Use only 10% of your RAM for such purpose.
Before doing anything to store your password, I recommend a good read:
How to securely hash passwords?
One of the points is that you shouldn't use SHA1, but instead use bcrypt or password_verify (PHP5.6 and up) in the future.
Also, you should be extremely careful with SQL injection and other possible attacks, especially evolving different encodings.
I sound paranoid, and I know that. But I just want a safe code.
Some Notes:
- Your use of
privatevariables to hide the inner workings of your class is a very good idea. This is one of the most founding principles of OOP, and you understand that part very well it seems. :) - The
Sessionclass seems responsible for more than just a session, though with the small size of this programme it's hard to differentiate it without much overhead. I would consider moving some things out in the future, or renaming it. - I'm not sure how far into best-practices this delves (I'm not a PHP guy, really. Just a novice) I would definitely recommend against directly changing URL destinations in methods like that.
I.e.:
public function authenticate() {
if (isset($_SESSION['loggedin'])) {
if ($_SESSION['loggedin'] === true) {
return;
}
}
header('Location: login.php');
}
Could be rewritten as:
public function isAuthenticated() {
if (isset($_SESSION['loggedin'])) {
if ($_SESSION['loggedin'] === true) {
return true;
}
}
return false;
}
This will allow you to do more with the function much more easily. It allows you to use the same function to, instead of redirecting the user, show a modal instead. It's a much more reusable class at that point. (Which is the idea here, is it not? :)) Part of the purpose of building something with an OOP mindset is to allow many sections of that programme to be reusable in many different ways. By strictly using header("Location: ?") you restrict the number of ways you can use it, to 1.
You could of course simplify this further (credit to Ismael for a chat message) into:
public function isAuthenticated() {
return isset($_SESSION['loggedin']) && $_SESSION['loggedin'];
}
This keeps the LoC down, and increases maintainability. It uses short-circuiting, so if the left-hand side evaluates false, the right-hand side will not even be hit.
Do not use
include_oncefor something like this. Therequire_oncehandle is much more appropriate. Others may recommend usinginclude_onceorinclude, but I would personally recommend therequire_onceover it.Concerning:
Also, I realise that the location headers may better be stored as a class constant in case I wish to change the locations.
I would really not do that. This class should not be responsible for knowing that, this class is all about authentication, not redirection/authorization. One class, one responsibility. Instead, I would make those paths part of a Configuration class, or a SQL table that stores that type of data. This allows you to easily and concisely change them in one location for everything. (Who knows where else you might need to know that Login.php is the gateway?)
- You state:
Please disregard the use of SHA1 and the hashed password in the file, I will be extending this to use a MySQL background, and hope to incorporate the use of a salted hash. I will be using prepared statements, will this be sufficient to protect from SQL injection and allow me to not bother filtering
$_POST[...]?
While this is technically true, prepared statements will not protect you against people entering blatantly invalid data. You should still do some filtering. (Though you need worry less about filtering against SQL injection, and more about filtering against data tampering.)
- You also state:
Do I need to call
die()(orexit()seeing as they are identical) after a failed authenticate to terminate any page which may attempt to load? Or do I need to return a boolean and then terminate from the calling page?
Personally (and I use this concept in my ASP.NET work as well): if I encounter an error with authentication/authorization, instead of throwing an exception (or die/exit in your case), I have a custom error handler class that will send that data to a SQL database, the Event Viewer, and submit the user to a custom error page. Part of the security concerns are that throwing a default die/exit message
The following bit(s) deviate into ideas, rather than strictly commenting on the code:
- I would consider using a single
session_idas the main key for your session, and instead of storing all of your$sessionInfodirectly in the session, store them in a SQL (et. al.) table. A nice by-product of this is that it's very difficult for people to change session data. (Hijacking, on the other hand, is a different story.)
Those are all the notes I have now, I may come back later and add more. :)
5baa61e4c9b93f3f0682250b6cf8331b7ee68fd8the hash of any important password? If it is, CHANGE IT NOW! \$\endgroup\$