patternphpMinor
PHP Login Check
Viewed 0 times
phpchecklogin
Problem
I use this block of code to check if the user is logged in (login stored in cookies as a plain text and pass MD5 crypted). As I'm a novice in PHP, I'm not sure that this is a correct way. Are there other easier ways of checking that you use?
if(isset($_COOKIE['login']) AND isset($_COOKIE['pass'])){
$login = $_COOKIE['login'];
$pass = $_COOKIE['pass'];
if($qw->query("SELECT count(id) FROM users where password = '".$pass."' and email = '".$login."';")->fetchColumn() == 0){
setcookie('login','');
setcookie('pass','');
header('Location:auth.php');
}
} else {
setcookie('login','');
setcookie('pass','');
header('Location:auth.php');
die();
}Solution
Are there other easier ways of checking that you use?
Any particular reason why you're using cookies to persist a user's authentication through every request?
I recommend using sessions to keep track of user authentications by storing its identifier and checking for its presence to know whether a user is already authenticated.
Using the
Did you know that PHP 5.5+ has the
-
I would use
-
When using
-
Be consistent when writing code. If you like your SQL keywords uppercased, then uppercase them all, not just "most of them". It prevents potential confusion and it makes for code that's easier to read.
-
-
Early exits will more often then not make your code cleaner and avoid nested if-statements that might hurt your code's readability.
Why are you setting the login and password cookies to an empty string regardless if the authentication has been successful or not? I would keep the cookies if the authentication was successful and ignore them if otherwise.
My 2 cents for now.
Any particular reason why you're using cookies to persist a user's authentication through every request?
I recommend using sessions to keep track of user authentications by storing its identifier and checking for its presence to know whether a user is already authenticated.
Using the
MD5 algorithm for passwords is a bad idea, not because of its cryptographic weaknesses, but because it's fast. This means that an attacker can try billions of candidate passwords per second on a single GPU.Did you know that PHP 5.5+ has the
password_hash() function that takes care of all the troublesome details for you when it comes to creating secure password hashes?-
I would use
&& as a convention instead of and if you're not utilizing its lower precedence. Better yet, you can do isset($_COOKIE['login'], $_COOKIE['pass']);. See the isset() manual.-
When using
PDO::query($statement) it requires you to properly escape all data to avoid SQL Injections and other issues. Use prepared statements to get rid of that.-
Be consistent when writing code. If you like your SQL keywords uppercased, then uppercase them all, not just "most of them". It prevents potential confusion and it makes for code that's easier to read.
-
header(): "HTTP/1.1 requires an absolute URI as argument to » Location: including the scheme, hostname and absolute path, but some clients accept relative URIs.". So if you want to be safe you should use absolute paths.-
Early exits will more often then not make your code cleaner and avoid nested if-statements that might hurt your code's readability.
Why are you setting the login and password cookies to an empty string regardless if the authentication has been successful or not? I would keep the cookies if the authentication was successful and ignore them if otherwise.
My 2 cents for now.
Context
StackExchange Code Review Q#64063, answer score: 3
Revisions (0)
No revisions yet.