IF
logic
All encompassing if
s
Right off the bat you have two levels of nested if
statements before we get to the main body of the code...
if (!isset($_SESSION['id'])) {
if (isset($_COOKIE['info'])) {
// Main code...
}
// Additional code...
}
...which is unnecessary; you could just as easily merge them into one statement:
if (!isset($_SESSION['id']) && isset($_COOKIE['info'])) {
// Code here...
}
Repeated if
s
The first of the duplicated if
statements isn't needed. A simplified version of your code would look like the following...
if (true) {
if (true) {
// Do "task"
}
// Do "task"
}
...notice that if
the first condition is true
then the task
is carried out regardless of the state of the second condition. Which means that you don't need to repeat the code in the nested if
statement because when that code block ends the task
is completed anyway!
The only reason that it isn't completed twice in your case is because you exit
the script manually after it runs the first time.
String comparison
There's nothing wrong with using strcmp
to compare two strings; however it is not needed in this scenario. You don't care whether one string is greater or lesser than the other you simply want to know if they are identical. The ===
comparison operator does exactly that: the values are either identical (true
; equal and of same type) or they aren't (false
). For example:
if ($username === $db_username && $password === $db_password) {
// Username and Password are correct...
}
Additionally, you're using a loose comparison: it's better practice to use a strict comparison as your default choice as you have done later on in the script. Especially here when the output you're looking for is an integer.
Whilst it may be true in this circumstance that the only false
value output is (int) 0
reusing the same logic somewhere else could lead to unexpected behaviour.
Lastly, you compare both the username and the password. You don't need to do that. You've already compared the username in your query; if it didn't match then the record would never have been returned in the first place so the check is redundant.
Checking the $result
Again, technically, there's nothing wrong here. And you are using the strict comparison operators mentioned earlier. But in this case, there is no need: the result of fetch_assoc
will either be a mysqli result set or null
. Which is equivalent to true
or false
so a strict comparison to null
isn't required.
Instead you can simply use:
if ($result) {
After all, if any false
equivalent value was returned you wouldn't want to proceed. If you did then, at minimum, you would gain a bunch of Notices
in your error log from trying to access non-existent array indexes.
Sessions
To put it bluntly, your first if
statement never succeeds! You know this (I think) because you have implemented a work around with the duplicated if
statements at the bottom of the code.
If you think about it: you'd never end up in a loop because after the first time the code is called the $_SESSION["id"]
is set and therefore the opening if
statement will evaluate to false
on subsequent page loads.
So then the question is, why does it loop?
The answer is simple: the $_SESSION
super global doesn't exist until after session_start();
is called. Up until that point...
$_SESSION === null;
// And...
isset($_SESSION) === false;
...no matter how many indexes you've assigned to it.
So in your if
statement isset(...)
evaluates to false
and is then flipped with the preceding !
to true
and therefore the code bock is executed.
The solution then is to make sure session_start();
is at the very top of your file.
<?php
session_start();
if (!isset($_SESSION["id"])) {
// Do something...
}
Getting this right means you can do away with both of the duplicated if
statements as now you won't have that endless loop.
Cookie management
You must delete the cookie if it doesn't match. You don't want random expired, useless, cookies sitting around. They may cause unexpected behaviour.
Cookies should not contain the users password (especially not in plain text form). We don't really want user details like (e.g. username and email address) stored in cookies either. In the event that a cookie is hijacked
it would make using that information much easier.
Leaving both the username and password in the cookie means that if it were stolen
then the attacker would have access to the account until the user changes their password worse still the attacker could change the password and lock out the real user!
Misc.
Variable names
Names like $connection
aren't very clear to a reader unfamiliar with your code (connection to what?). Much better to use a name with clear meaning such as $mysqli
(because it's a mysqli object) or $mysqliConnection
.
Storing credentials in $_SESSION
You don't need to store anything apart from the id
. Anything else can be looked up in the database based on the id
because it is (or should be) a PRIMARY KEY
.
Database and SQL
Password storage
You must not store a users password in plain text form. There are so many reasons why it's hard to know where to begin, but here are two of the key reasons:
- If your database is leaked those passwords are accessible to the world
- Anyone with access to the database can see ALL your users' passwords
- Inevitably a percentage of your users will reuse the same (or similar) passwords elsewhere which means anyone with access to the DB can now access other accounts (i.e. other websites) belonging to your users!
PHP has in built functions to hash and check passwords:
// Hash the password; on account creation
$password = "mySuperStrongPassword"; // User enters
$passwordHash = password_hash($password, PASSWORD_DEFAULT); // DB stores
// Check the password against the DBs hash; on login
if (password_verify($password, $passwordHash)) {
// Succesfully matched the password!
}
SQL in prepare
For short queries it's okay to put them directly into the prepare
method. It's good practice though to keep them in a separate variable - especially for longer and more complex ones. Keeping them separate means you have more flexibility in formatting the query without confusing the flow of the page.
Table names
It's good practice to not user plurals as database table names. There's no major flaw here but it makes your code nicer/easier to read because usually we're working on one individual record at a time:
// In SQL queries...
SELECT user.email FROM user WHERE user.id = 123;
SELECT users.email FROM users WHERE users.id = 123;
// In PHP..
while ( $user = $query->fetchObject() ) {
echo $user->email;
}
SELECT *
There's no harm in using the *
operator to SELECT
all records - unless you're selecting a large number of rows, the table consists of a large number of columns, or the columns pack large volumes of data.
For example, let's assume you're selecting all employees (e.g. 50 records) and each record has a blob
for a profile picture. All of a sudden that's a much larger chunk of memory compared to the email address fields we were actually after!
So, it's much better to be explicit about what fields you want returned when you can be. In your case we're only interested in the password
and id
columns:
SELECT id, password FROM `users` WHERE `username` = ?
Persistent logon
DB Structure
If you only wanted a user to be active on one device at a time then you could simply add a field (or two) to the users
table.
In most circumstances though it's preferred that users can logon on multiple devices; so we need to implement a one to many relationship in the database with the addition of a new session
table. Which, on a simplistic level, will look something like:
CREATE TABLE session (
id int AUTO_INCREMENT PRIMARY KEY,
user_id int,
cookie_id varchar(250),
hash varchar(250),
expires datetime
);
Code Example
Normal logon:
<?php
session_start();
require_once(__DIR__ . '/../dbh/dbh.php');
$username = $_POST["username"] ?? null;
$password = $_POST["password"] ?? null;
$persistent = $_POST["persistent"] ?? null;
if ($username && $password) {
$logonSQL = "SELECT id, password FROM user WHERE username = ?";
$logonQuery = $mysqli->prepare($logonSQL);
$logonQuery->bind_param("s", $username);
$logonQuery->execute();
$user = $logonQuery->get_result()->fetch_assoc();
if ($user) {
if (password_verify($password, $user["password"])) {
// Logged on
$_SESSION["id"] = $user["id"];
// Create cookie, if persistent is checked
if ($persistent) {
// Create a session cookie
$cookieId = bin2hex(random_bytes(50));
$hash = bin2hex(random_bytes(50));
$dbHash = password_hash($hash, PASSWORD_DEFAULT);
$expires = date("Y-m-d H:i:s", strtotime("+30 days"));
$sessionSQL = "
INSERT INTO session
(user_id, cookie_id, hash, expires)
VALUES
(?, ?, ?, ?)
";
$sessionQuery = $mysqli->prepare($sql);
$sessionQuery->bind_param("isss", $user["id"], $cookieId, $dbHash, $expires);
$sessionQuery->execute();
setcookie("rememberme", $cookieId.$hash, strtotime("+30 days"));
}
} else {
// Wrong credentials...
}
}
}
Logon with cookie:
<?php
session_start();
if (!isset($_SESSION["id"]) && isset($_COOKIE["rememberme"])) {
list($cookieId, $hash) = str_split($_COOKIE["rememberme"], 100);
$sessionSQL = "
SELECT id, user_id, hash, expires
FROM session
WHERE cookieId = ?
";
$sessionQuery = $mysqli->prepare($sessionSQL);
$sessionQuery->bind_param("s", $cookieId);
$sessionQuery->execute();
$session = $sessionQuery->get_result->fetch_assoc();
if (
password_verify($hash, $session["hash"]) &&
$session["expires"] > date("Y-m-d H:i:s")
) {
// Successful login with cookies
$_SESSION["id"] = $session["user_id"];
$hash = bin2hex(random_bytes(50));
$dbHash = password_hash($hash, PASSWORD_DEFAULT);
$expires = date("Y-m-d H:i:s", strtotime("+30 days"));
/**
* Regenerating the hash
*
* You don't have to do this _every_ time but frequently enough
* that the hash doesn't become _stale_.
*
* Regenerating the hash means that even if your cookie is
* hijacked the next time the _real_ user logs on the session
* will be scrubbed and the _attacker_ won't be able to log on
* with the stolen cookie again.
*
*/
setcookie("rememberme", $cookieId.$hash, strtotime("+30 days"));
$updateSQL = "
UPDATE session
SET
expires = ?,
hash = ?
WHERE id = ?
";
$updateQuery = $mysqli->prepare($updateSQL);
$updateQuery->bind_param("ssi", $expires, $hash, $session["id"]);
} else {
// Session expired or logon credentials incorrect...
// Delete cookie and session record
setcookie("rememberme", "", 1);
unset($_SESSION["id"]);
$deleteSQL = "DELETE FROM session WHERE id = ?";
$deleteQuery = $mysqli->prepare($deleteSQL)
$deleteQuery->bind_param("i", $session["id"]);
$deleteQuery->execute();
}
}
Log off:
if ($_GET["logout"] ?? null) {
if (isset($_COOKIE["rememberme"])) {
$cookieId = substr($_COOKIE["rememberme"], 0, 100);
$deleteSQL = "DELETE FROM session WHERE cookie_id = ?";
$deleteQuery = $mysqli->prepare($deleteSQL)
$deleteQuery->bind_param("s", $cookieId);
$deleteQuery->execute();
setcookie("rememberme", "", 1);
}
unset($_SESSION["id"]);
header("location: /safeplace.html");
}
Notes
- The above code is untested, there's likely a few typos etc.
- The code also doesn't check to ensure that the
cookie_id
is unique in the DB
- Very unlikely to happen on a small site
- Not a security problem, even if it did, because the worst case scenario is a session gets removed
- Unless the
1 in 64^100
occurs and the hash/cookie_id are the same
- Perhaps something to look at to ensure a consistent user experience
- Implementing this code you'd probably want to
logout
any active user before trying to logon
with new credentials
- You may also want to factor in things like garbage collection in the
session
table to remove expired sessions
- It's common practice that a
remember me
session doesn't have total access to the system
- For example, you wouldn't let someone change their password without challenging them for the old password first!
- This helps prevent problems if/when cookies are stolen
md5
\$\endgroup\$