Secure PHP login with SQL injection prevention
Clash Royale CLAN TAG#URR8PPP
.everyoneloves__top-leaderboard:empty,.everyoneloves__mid-leaderboard:empty margin-bottom:0;
up vote
3
down vote
favorite
I am working on a project where users have to log in then if it is correct it creates a cookie to process requests. I would like to find a way to auto-renew sessions every day without affecting the user or requiring the user to log in again.
I would like to also know if I overlooked any security flaws such as SQL injection or session-hijacking and all those, are there any other one that I could prevent or improve?
The login page is
<?php
//Login.php
if(isset($_POST['username']) && isset($_POST['password']))
$username = filter_input(INPUT_POST, "username", FILTER_SANITIZE_ENCODED, FILTER_FLAG_STRIP_HIGH); //Changes to unicode characters to prevent injection
$password = $_POST['password']; //Get password
$result = LoginManager::Login($username, $password); //Run login method
if($result == "invalid_username")
echo 'Invalid Username!';
if($result == "invalid_login")
echo 'Invalid Login!';
$expires = gmdate('Y-m-d', strtotime($Date. ' + 1 days')); //sessions expire in 1 day
DataBase::query("INSERT INTO sessions VALUES(:session, :userid, :expires)", array(':session' => $result, ':userid' => $userid, ':expires' => $expires)); //insert into database
setcookie("session", $result); //set as cookie
return;
echo '<form action="Login.php" method="post">
<p>Username: <input type="text" name="username" placeholder="Username"/></p>
<p>Password: <input type="password" name="password" placeholder="Password"/></p>
<p><input type="submit" name="login" value="Login"/></p>
</form>';
?>
The LoginManager class
<?php
//LoginManager.php
public static function Login($username, $password) strlen($password) < 5) //usernames less than 16, password from 5-50 char
return 'invalid_bounds';
$sql = "SELECT * from accounts WHERE username = :username";
$users = Database::query($sql, array(':username' => $username)); //get account
if(sizeof($users) == 0) //no accounts found
return 'invalid_username';
$userdata = $users[0];
$salt = '-45dfeHK/__yu349@-/klF21-1_/4JkUP/4'; //salt for password
if(password_verify($password . $salt, $userdata['password'])) //check if password is correct
$session = com_create_guid(); //create unique session
return $session;
else
return 'invalid_login';
?>
And the main page / feed
<?php
//Main.php
$session = $_COOKIE['session']; //get cookie session
$sessions = DataBase::query('SELECT * FROM sessions WHERE id=:id', array(':id' => $session));
if(sizeof($sessions) == 0)
//Redirect to login page
return;
//get first session
$session = $sessions[0];
$date = new DateTime($session['expires']);
$now = new DateTime();
if($date < $now)
//Expired session : Redirect to login
return;
//Display feed
?>
php sql mysql
add a comment |Â
up vote
3
down vote
favorite
I am working on a project where users have to log in then if it is correct it creates a cookie to process requests. I would like to find a way to auto-renew sessions every day without affecting the user or requiring the user to log in again.
I would like to also know if I overlooked any security flaws such as SQL injection or session-hijacking and all those, are there any other one that I could prevent or improve?
The login page is
<?php
//Login.php
if(isset($_POST['username']) && isset($_POST['password']))
$username = filter_input(INPUT_POST, "username", FILTER_SANITIZE_ENCODED, FILTER_FLAG_STRIP_HIGH); //Changes to unicode characters to prevent injection
$password = $_POST['password']; //Get password
$result = LoginManager::Login($username, $password); //Run login method
if($result == "invalid_username")
echo 'Invalid Username!';
if($result == "invalid_login")
echo 'Invalid Login!';
$expires = gmdate('Y-m-d', strtotime($Date. ' + 1 days')); //sessions expire in 1 day
DataBase::query("INSERT INTO sessions VALUES(:session, :userid, :expires)", array(':session' => $result, ':userid' => $userid, ':expires' => $expires)); //insert into database
setcookie("session", $result); //set as cookie
return;
echo '<form action="Login.php" method="post">
<p>Username: <input type="text" name="username" placeholder="Username"/></p>
<p>Password: <input type="password" name="password" placeholder="Password"/></p>
<p><input type="submit" name="login" value="Login"/></p>
</form>';
?>
The LoginManager class
<?php
//LoginManager.php
public static function Login($username, $password) strlen($password) < 5) //usernames less than 16, password from 5-50 char
return 'invalid_bounds';
$sql = "SELECT * from accounts WHERE username = :username";
$users = Database::query($sql, array(':username' => $username)); //get account
if(sizeof($users) == 0) //no accounts found
return 'invalid_username';
$userdata = $users[0];
$salt = '-45dfeHK/__yu349@-/klF21-1_/4JkUP/4'; //salt for password
if(password_verify($password . $salt, $userdata['password'])) //check if password is correct
$session = com_create_guid(); //create unique session
return $session;
else
return 'invalid_login';
?>
And the main page / feed
<?php
//Main.php
$session = $_COOKIE['session']; //get cookie session
$sessions = DataBase::query('SELECT * FROM sessions WHERE id=:id', array(':id' => $session));
if(sizeof($sessions) == 0)
//Redirect to login page
return;
//get first session
$session = $sessions[0];
$date = new DateTime($session['expires']);
$now = new DateTime();
if($date < $now)
//Expired session : Redirect to login
return;
//Display feed
?>
php sql mysql
I'm no expert on this but I think people usually employ prepared statements to avoid SQL injections.
â yuri
Apr 30 at 8:33
@yuri so he does
â Your Common Sense
Apr 30 at 9:01
Where does $userid come from?
â Your Common Sense
Apr 30 at 9:06
add a comment |Â
up vote
3
down vote
favorite
up vote
3
down vote
favorite
I am working on a project where users have to log in then if it is correct it creates a cookie to process requests. I would like to find a way to auto-renew sessions every day without affecting the user or requiring the user to log in again.
I would like to also know if I overlooked any security flaws such as SQL injection or session-hijacking and all those, are there any other one that I could prevent or improve?
The login page is
<?php
//Login.php
if(isset($_POST['username']) && isset($_POST['password']))
$username = filter_input(INPUT_POST, "username", FILTER_SANITIZE_ENCODED, FILTER_FLAG_STRIP_HIGH); //Changes to unicode characters to prevent injection
$password = $_POST['password']; //Get password
$result = LoginManager::Login($username, $password); //Run login method
if($result == "invalid_username")
echo 'Invalid Username!';
if($result == "invalid_login")
echo 'Invalid Login!';
$expires = gmdate('Y-m-d', strtotime($Date. ' + 1 days')); //sessions expire in 1 day
DataBase::query("INSERT INTO sessions VALUES(:session, :userid, :expires)", array(':session' => $result, ':userid' => $userid, ':expires' => $expires)); //insert into database
setcookie("session", $result); //set as cookie
return;
echo '<form action="Login.php" method="post">
<p>Username: <input type="text" name="username" placeholder="Username"/></p>
<p>Password: <input type="password" name="password" placeholder="Password"/></p>
<p><input type="submit" name="login" value="Login"/></p>
</form>';
?>
The LoginManager class
<?php
//LoginManager.php
public static function Login($username, $password) strlen($password) < 5) //usernames less than 16, password from 5-50 char
return 'invalid_bounds';
$sql = "SELECT * from accounts WHERE username = :username";
$users = Database::query($sql, array(':username' => $username)); //get account
if(sizeof($users) == 0) //no accounts found
return 'invalid_username';
$userdata = $users[0];
$salt = '-45dfeHK/__yu349@-/klF21-1_/4JkUP/4'; //salt for password
if(password_verify($password . $salt, $userdata['password'])) //check if password is correct
$session = com_create_guid(); //create unique session
return $session;
else
return 'invalid_login';
?>
And the main page / feed
<?php
//Main.php
$session = $_COOKIE['session']; //get cookie session
$sessions = DataBase::query('SELECT * FROM sessions WHERE id=:id', array(':id' => $session));
if(sizeof($sessions) == 0)
//Redirect to login page
return;
//get first session
$session = $sessions[0];
$date = new DateTime($session['expires']);
$now = new DateTime();
if($date < $now)
//Expired session : Redirect to login
return;
//Display feed
?>
php sql mysql
I am working on a project where users have to log in then if it is correct it creates a cookie to process requests. I would like to find a way to auto-renew sessions every day without affecting the user or requiring the user to log in again.
I would like to also know if I overlooked any security flaws such as SQL injection or session-hijacking and all those, are there any other one that I could prevent or improve?
The login page is
<?php
//Login.php
if(isset($_POST['username']) && isset($_POST['password']))
$username = filter_input(INPUT_POST, "username", FILTER_SANITIZE_ENCODED, FILTER_FLAG_STRIP_HIGH); //Changes to unicode characters to prevent injection
$password = $_POST['password']; //Get password
$result = LoginManager::Login($username, $password); //Run login method
if($result == "invalid_username")
echo 'Invalid Username!';
if($result == "invalid_login")
echo 'Invalid Login!';
$expires = gmdate('Y-m-d', strtotime($Date. ' + 1 days')); //sessions expire in 1 day
DataBase::query("INSERT INTO sessions VALUES(:session, :userid, :expires)", array(':session' => $result, ':userid' => $userid, ':expires' => $expires)); //insert into database
setcookie("session", $result); //set as cookie
return;
echo '<form action="Login.php" method="post">
<p>Username: <input type="text" name="username" placeholder="Username"/></p>
<p>Password: <input type="password" name="password" placeholder="Password"/></p>
<p><input type="submit" name="login" value="Login"/></p>
</form>';
?>
The LoginManager class
<?php
//LoginManager.php
public static function Login($username, $password) strlen($password) < 5) //usernames less than 16, password from 5-50 char
return 'invalid_bounds';
$sql = "SELECT * from accounts WHERE username = :username";
$users = Database::query($sql, array(':username' => $username)); //get account
if(sizeof($users) == 0) //no accounts found
return 'invalid_username';
$userdata = $users[0];
$salt = '-45dfeHK/__yu349@-/klF21-1_/4JkUP/4'; //salt for password
if(password_verify($password . $salt, $userdata['password'])) //check if password is correct
$session = com_create_guid(); //create unique session
return $session;
else
return 'invalid_login';
?>
And the main page / feed
<?php
//Main.php
$session = $_COOKIE['session']; //get cookie session
$sessions = DataBase::query('SELECT * FROM sessions WHERE id=:id', array(':id' => $session));
if(sizeof($sessions) == 0)
//Redirect to login page
return;
//get first session
$session = $sessions[0];
$date = new DateTime($session['expires']);
$now = new DateTime();
if($date < $now)
//Expired session : Redirect to login
return;
//Display feed
?>
php sql mysql
asked Apr 30 at 6:35
Dan
1191
1191
I'm no expert on this but I think people usually employ prepared statements to avoid SQL injections.
â yuri
Apr 30 at 8:33
@yuri so he does
â Your Common Sense
Apr 30 at 9:01
Where does $userid come from?
â Your Common Sense
Apr 30 at 9:06
add a comment |Â
I'm no expert on this but I think people usually employ prepared statements to avoid SQL injections.
â yuri
Apr 30 at 8:33
@yuri so he does
â Your Common Sense
Apr 30 at 9:01
Where does $userid come from?
â Your Common Sense
Apr 30 at 9:06
I'm no expert on this but I think people usually employ prepared statements to avoid SQL injections.
â yuri
Apr 30 at 8:33
I'm no expert on this but I think people usually employ prepared statements to avoid SQL injections.
â yuri
Apr 30 at 8:33
@yuri so he does
â Your Common Sense
Apr 30 at 9:01
@yuri so he does
â Your Common Sense
Apr 30 at 9:01
Where does $userid come from?
â Your Common Sense
Apr 30 at 9:06
Where does $userid come from?
â Your Common Sense
Apr 30 at 9:06
add a comment |Â
2 Answers
2
active
oldest
votes
up vote
2
down vote
Magic variables
You are using invalid_username
, invalid_login
and some more magic names that are directly related to the login procedure. Factor those out and put them as static properties on your LoginManager
class, so the actual contents are irrelevant and you can simply check against LoginManager::INVALID_LOGIN
to see if that is the state of the session.
Factor out your session management
Creation and renewing of sessions should happen in their own class. Other code doesn't need to know how to make sessions, just that a session exists and who is authenticated by that session.
Factor out the salt
Move the salt to a config file somewhere. You don't want to have to dig through your code when you have to change the salt. If you do not want to have a config file, at least put it as a static or private variable in your LoginManager.
Odd sessions
You are making a session even if the login was invalid. Later you only check if a session exists, not if that session was valid. This allows someone to log in by entering a wrong username, or a right username with a wrong password.
Your LoginManager should also not disqualify usernames or passwords based on length, as this is a thing that should be enforced when setting a password or username. The login will always fail in those cases, so don't worry about it. You want to do the same amount of work for every login attempt to prevent an attacker gathering information about valid usernames/passwords based on the time it takes for a request to return.
Timezones
You are creating a date with gmdate(..)
which is a UTC date. Later you are parsing it with new DateTime(..)
as a local date and comparing it with a local date. This causes sessions to last longer or shorter based on where the server is.
Undefined variables
You are making sessions where the $userid
is undefined. This means you cannot do anything useful with the session, as you lose information about who is logged in.
You are also setting the expiration date with $Date
, a misspelled variable that is not defined either.
Queries can return false
You should always keep in mind that queries may fail. If your username query fails, it will return false
, and sizeof(false) === 1
. Your code would then error out on $userdata['password']
, which is not that great.
Expired sessions
You are currently not doing anything with your expired sessions. That means they are not removed from the database after they expired.
Various
You specifically asked about sql injection and session hijacking. I believe your code to be reasonably safe on both accounts. If you are using truely prepared statements (not a setting that uses sprintf behind the scenes), you should be safe from sql injection. Your queries may still fail, but you should not be able to execute unintended sql queries.
Your sessions are based around com_create_guid
, which I believe is cryptographically secure. The generated string is also long enough that guessing a session guid is practically impossible. You are making sessions less secure by essentially wanting a session to last forever, increasing the time they could be used by some malicious third party. I do not believe this to be a great risk in this case though.
add a comment |Â
up vote
1
down vote
One thing to note with your SQLstatements, you are retrieving more fields than you need to. This results in more memory usage and slower performance. While it may not make a huge difference for you, it's still best practice to only select the fields you need.
Change
'SELECT * FROM sessions WHERE id=:id'
to
'SELECT expires FROM sessions WHERE id=:id'
and
$sql = "SELECT * from accounts WHERE username = :username";
to
$sql = "SELECT password from accounts WHERE username = :username";
You will also want to make sure that the username field is unique, as otherwise you would get back the first password for that username but not necessarily the correct one.
add a comment |Â
2 Answers
2
active
oldest
votes
2 Answers
2
active
oldest
votes
active
oldest
votes
active
oldest
votes
up vote
2
down vote
Magic variables
You are using invalid_username
, invalid_login
and some more magic names that are directly related to the login procedure. Factor those out and put them as static properties on your LoginManager
class, so the actual contents are irrelevant and you can simply check against LoginManager::INVALID_LOGIN
to see if that is the state of the session.
Factor out your session management
Creation and renewing of sessions should happen in their own class. Other code doesn't need to know how to make sessions, just that a session exists and who is authenticated by that session.
Factor out the salt
Move the salt to a config file somewhere. You don't want to have to dig through your code when you have to change the salt. If you do not want to have a config file, at least put it as a static or private variable in your LoginManager.
Odd sessions
You are making a session even if the login was invalid. Later you only check if a session exists, not if that session was valid. This allows someone to log in by entering a wrong username, or a right username with a wrong password.
Your LoginManager should also not disqualify usernames or passwords based on length, as this is a thing that should be enforced when setting a password or username. The login will always fail in those cases, so don't worry about it. You want to do the same amount of work for every login attempt to prevent an attacker gathering information about valid usernames/passwords based on the time it takes for a request to return.
Timezones
You are creating a date with gmdate(..)
which is a UTC date. Later you are parsing it with new DateTime(..)
as a local date and comparing it with a local date. This causes sessions to last longer or shorter based on where the server is.
Undefined variables
You are making sessions where the $userid
is undefined. This means you cannot do anything useful with the session, as you lose information about who is logged in.
You are also setting the expiration date with $Date
, a misspelled variable that is not defined either.
Queries can return false
You should always keep in mind that queries may fail. If your username query fails, it will return false
, and sizeof(false) === 1
. Your code would then error out on $userdata['password']
, which is not that great.
Expired sessions
You are currently not doing anything with your expired sessions. That means they are not removed from the database after they expired.
Various
You specifically asked about sql injection and session hijacking. I believe your code to be reasonably safe on both accounts. If you are using truely prepared statements (not a setting that uses sprintf behind the scenes), you should be safe from sql injection. Your queries may still fail, but you should not be able to execute unintended sql queries.
Your sessions are based around com_create_guid
, which I believe is cryptographically secure. The generated string is also long enough that guessing a session guid is practically impossible. You are making sessions less secure by essentially wanting a session to last forever, increasing the time they could be used by some malicious third party. I do not believe this to be a great risk in this case though.
add a comment |Â
up vote
2
down vote
Magic variables
You are using invalid_username
, invalid_login
and some more magic names that are directly related to the login procedure. Factor those out and put them as static properties on your LoginManager
class, so the actual contents are irrelevant and you can simply check against LoginManager::INVALID_LOGIN
to see if that is the state of the session.
Factor out your session management
Creation and renewing of sessions should happen in their own class. Other code doesn't need to know how to make sessions, just that a session exists and who is authenticated by that session.
Factor out the salt
Move the salt to a config file somewhere. You don't want to have to dig through your code when you have to change the salt. If you do not want to have a config file, at least put it as a static or private variable in your LoginManager.
Odd sessions
You are making a session even if the login was invalid. Later you only check if a session exists, not if that session was valid. This allows someone to log in by entering a wrong username, or a right username with a wrong password.
Your LoginManager should also not disqualify usernames or passwords based on length, as this is a thing that should be enforced when setting a password or username. The login will always fail in those cases, so don't worry about it. You want to do the same amount of work for every login attempt to prevent an attacker gathering information about valid usernames/passwords based on the time it takes for a request to return.
Timezones
You are creating a date with gmdate(..)
which is a UTC date. Later you are parsing it with new DateTime(..)
as a local date and comparing it with a local date. This causes sessions to last longer or shorter based on where the server is.
Undefined variables
You are making sessions where the $userid
is undefined. This means you cannot do anything useful with the session, as you lose information about who is logged in.
You are also setting the expiration date with $Date
, a misspelled variable that is not defined either.
Queries can return false
You should always keep in mind that queries may fail. If your username query fails, it will return false
, and sizeof(false) === 1
. Your code would then error out on $userdata['password']
, which is not that great.
Expired sessions
You are currently not doing anything with your expired sessions. That means they are not removed from the database after they expired.
Various
You specifically asked about sql injection and session hijacking. I believe your code to be reasonably safe on both accounts. If you are using truely prepared statements (not a setting that uses sprintf behind the scenes), you should be safe from sql injection. Your queries may still fail, but you should not be able to execute unintended sql queries.
Your sessions are based around com_create_guid
, which I believe is cryptographically secure. The generated string is also long enough that guessing a session guid is practically impossible. You are making sessions less secure by essentially wanting a session to last forever, increasing the time they could be used by some malicious third party. I do not believe this to be a great risk in this case though.
add a comment |Â
up vote
2
down vote
up vote
2
down vote
Magic variables
You are using invalid_username
, invalid_login
and some more magic names that are directly related to the login procedure. Factor those out and put them as static properties on your LoginManager
class, so the actual contents are irrelevant and you can simply check against LoginManager::INVALID_LOGIN
to see if that is the state of the session.
Factor out your session management
Creation and renewing of sessions should happen in their own class. Other code doesn't need to know how to make sessions, just that a session exists and who is authenticated by that session.
Factor out the salt
Move the salt to a config file somewhere. You don't want to have to dig through your code when you have to change the salt. If you do not want to have a config file, at least put it as a static or private variable in your LoginManager.
Odd sessions
You are making a session even if the login was invalid. Later you only check if a session exists, not if that session was valid. This allows someone to log in by entering a wrong username, or a right username with a wrong password.
Your LoginManager should also not disqualify usernames or passwords based on length, as this is a thing that should be enforced when setting a password or username. The login will always fail in those cases, so don't worry about it. You want to do the same amount of work for every login attempt to prevent an attacker gathering information about valid usernames/passwords based on the time it takes for a request to return.
Timezones
You are creating a date with gmdate(..)
which is a UTC date. Later you are parsing it with new DateTime(..)
as a local date and comparing it with a local date. This causes sessions to last longer or shorter based on where the server is.
Undefined variables
You are making sessions where the $userid
is undefined. This means you cannot do anything useful with the session, as you lose information about who is logged in.
You are also setting the expiration date with $Date
, a misspelled variable that is not defined either.
Queries can return false
You should always keep in mind that queries may fail. If your username query fails, it will return false
, and sizeof(false) === 1
. Your code would then error out on $userdata['password']
, which is not that great.
Expired sessions
You are currently not doing anything with your expired sessions. That means they are not removed from the database after they expired.
Various
You specifically asked about sql injection and session hijacking. I believe your code to be reasonably safe on both accounts. If you are using truely prepared statements (not a setting that uses sprintf behind the scenes), you should be safe from sql injection. Your queries may still fail, but you should not be able to execute unintended sql queries.
Your sessions are based around com_create_guid
, which I believe is cryptographically secure. The generated string is also long enough that guessing a session guid is practically impossible. You are making sessions less secure by essentially wanting a session to last forever, increasing the time they could be used by some malicious third party. I do not believe this to be a great risk in this case though.
Magic variables
You are using invalid_username
, invalid_login
and some more magic names that are directly related to the login procedure. Factor those out and put them as static properties on your LoginManager
class, so the actual contents are irrelevant and you can simply check against LoginManager::INVALID_LOGIN
to see if that is the state of the session.
Factor out your session management
Creation and renewing of sessions should happen in their own class. Other code doesn't need to know how to make sessions, just that a session exists and who is authenticated by that session.
Factor out the salt
Move the salt to a config file somewhere. You don't want to have to dig through your code when you have to change the salt. If you do not want to have a config file, at least put it as a static or private variable in your LoginManager.
Odd sessions
You are making a session even if the login was invalid. Later you only check if a session exists, not if that session was valid. This allows someone to log in by entering a wrong username, or a right username with a wrong password.
Your LoginManager should also not disqualify usernames or passwords based on length, as this is a thing that should be enforced when setting a password or username. The login will always fail in those cases, so don't worry about it. You want to do the same amount of work for every login attempt to prevent an attacker gathering information about valid usernames/passwords based on the time it takes for a request to return.
Timezones
You are creating a date with gmdate(..)
which is a UTC date. Later you are parsing it with new DateTime(..)
as a local date and comparing it with a local date. This causes sessions to last longer or shorter based on where the server is.
Undefined variables
You are making sessions where the $userid
is undefined. This means you cannot do anything useful with the session, as you lose information about who is logged in.
You are also setting the expiration date with $Date
, a misspelled variable that is not defined either.
Queries can return false
You should always keep in mind that queries may fail. If your username query fails, it will return false
, and sizeof(false) === 1
. Your code would then error out on $userdata['password']
, which is not that great.
Expired sessions
You are currently not doing anything with your expired sessions. That means they are not removed from the database after they expired.
Various
You specifically asked about sql injection and session hijacking. I believe your code to be reasonably safe on both accounts. If you are using truely prepared statements (not a setting that uses sprintf behind the scenes), you should be safe from sql injection. Your queries may still fail, but you should not be able to execute unintended sql queries.
Your sessions are based around com_create_guid
, which I believe is cryptographically secure. The generated string is also long enough that guessing a session guid is practically impossible. You are making sessions less secure by essentially wanting a session to last forever, increasing the time they could be used by some malicious third party. I do not believe this to be a great risk in this case though.
answered May 7 at 21:19
Sumurai8
2,260315
2,260315
add a comment |Â
add a comment |Â
up vote
1
down vote
One thing to note with your SQLstatements, you are retrieving more fields than you need to. This results in more memory usage and slower performance. While it may not make a huge difference for you, it's still best practice to only select the fields you need.
Change
'SELECT * FROM sessions WHERE id=:id'
to
'SELECT expires FROM sessions WHERE id=:id'
and
$sql = "SELECT * from accounts WHERE username = :username";
to
$sql = "SELECT password from accounts WHERE username = :username";
You will also want to make sure that the username field is unique, as otherwise you would get back the first password for that username but not necessarily the correct one.
add a comment |Â
up vote
1
down vote
One thing to note with your SQLstatements, you are retrieving more fields than you need to. This results in more memory usage and slower performance. While it may not make a huge difference for you, it's still best practice to only select the fields you need.
Change
'SELECT * FROM sessions WHERE id=:id'
to
'SELECT expires FROM sessions WHERE id=:id'
and
$sql = "SELECT * from accounts WHERE username = :username";
to
$sql = "SELECT password from accounts WHERE username = :username";
You will also want to make sure that the username field is unique, as otherwise you would get back the first password for that username but not necessarily the correct one.
add a comment |Â
up vote
1
down vote
up vote
1
down vote
One thing to note with your SQLstatements, you are retrieving more fields than you need to. This results in more memory usage and slower performance. While it may not make a huge difference for you, it's still best practice to only select the fields you need.
Change
'SELECT * FROM sessions WHERE id=:id'
to
'SELECT expires FROM sessions WHERE id=:id'
and
$sql = "SELECT * from accounts WHERE username = :username";
to
$sql = "SELECT password from accounts WHERE username = :username";
You will also want to make sure that the username field is unique, as otherwise you would get back the first password for that username but not necessarily the correct one.
One thing to note with your SQLstatements, you are retrieving more fields than you need to. This results in more memory usage and slower performance. While it may not make a huge difference for you, it's still best practice to only select the fields you need.
Change
'SELECT * FROM sessions WHERE id=:id'
to
'SELECT expires FROM sessions WHERE id=:id'
and
$sql = "SELECT * from accounts WHERE username = :username";
to
$sql = "SELECT password from accounts WHERE username = :username";
You will also want to make sure that the username field is unique, as otherwise you would get back the first password for that username but not necessarily the correct one.
answered May 11 at 18:33
Charlie Brumbaugh
1727
1727
add a comment |Â
add a comment |Â
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
StackExchange.ready(
function ()
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f193243%2fsecure-php-login-with-sql-injection-prevention%23new-answer', 'question_page');
);
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
I'm no expert on this but I think people usually employ prepared statements to avoid SQL injections.
â yuri
Apr 30 at 8:33
@yuri so he does
â Your Common Sense
Apr 30 at 9:01
Where does $userid come from?
â Your Common Sense
Apr 30 at 9:06