trjhtr

I am quite new to PHP, and with the help of a few tutorials have made this login system.

However I know its not as efficient and compact as it could be, also I am looking for ways to improve the security of this system. As I noticed when testing that If I simply changed the url from the login page, I could bypass the whole login process without having to login. I'm not really sure how I would go about fixing this, I tried creating session variables at the bottom of the script but not really sure if this is the way to go about it.

I would be very grateful for any feedback and if someone could help me improve this code, to make it more secure and maybe more efficient?

<?php

session_start();

if (isset($_POST['submit'])) 

 require_once 'config.php';

 $username = mysqli_real_escape_string($connect, $_POST['username']);
 $password = mysqli_real_escape_string($connect, $_POST['password']);

 //Error handlers
 //Check if inputs are empty
 if (empty($username) else 
 header("Location: ../index.php?login=error");
 exit();

?>

Security

You are escaping user input, which prevents SQL injection. It's good enough, but really not the recommended way to do this as it's too error-prone (whenever a project does this, it's basically guaranteed that you can find injections). Use prepared statements instead. It's not only more secure, but also leads to nicer code.

Guard Clauses

You have a lot of nested ifs, which makes it difficult to see what happens when (for example, I have to scroll all the way down to see what happens if it's a GET request).

If you use guard clauses / return early instead, your code will be much more readable. It might look like this:

if (!isset($_POST['submit'])) 
 header("Location: ../index.php?login=error");
 exit();


if (empty($username) || empty($password)) 
 header("Location: ../index.php?login=empty");
 exit();


// select the user

if (!$userExists) 
 header("Location: ../index.php?login=error");
 exit();


// verify the password

if ($validPassword) 
 header("Location: ../index.php?login=error");
 exit();


// set the session, redirect

Now it's very clear what happens when.

Duplication and functions

Following a header redirect with exit is very good practice. You can make sure that you always do this, simplify the code, and make it easier to modify later by extracting that to it's own function (eg redirectError(Error error)).

True or false

Something is either true or false. There is no reason to do an else check on true if you already did one on false.

Note though that some equals checks can be unexpected, so you should always use strong comparison (===) if you don't have a reason not to.