2
\$\begingroup\$

Can you check my code if I wrote like a professional?

  1. connection page.
  2. form sign page.
  3. plan page.
  4. log out page.

First I create the database and tables, then the connection file, then the form sign in page, then the plan page, finally the log out page

Database educate (create table users):

CREATE TABLE users(
id INT AUTO_INCREMENT,
username VARCHAR(50) NOT NULL,
password VARCHAR(100) NOT NULL,
PRIMARY KEY(id)
);

Conection.php:

<?php
$servername = 'localhost';
$username = 'root';
$password = '';
$db = 'educate';
//I added @ to delete error message that comming from  MySQL 
$conn = @new mysqli($servername,$username,$password,$db);
if($conn->connect_error) die('Connection faild');
?>

Form signin.php:

<?php
//Check the cookie
if(!empty($_COOKIE['user'])) { header ('location: plan.php'); }
?>

<?php
//Check the inputs
function test_input($data) {
$data = trim($data);  
//return a $data with whitespace stripped from the beginning and end of    
$data
$data = stripslashes($data); //emoves backslashes
$data = htmlspecialchars($data); 
//converts some predefined characters to  HTML entities
return $data;
}
?>

<?php
$username = $password = $password_hash = '';
$usernameErr = $passwordErr = '';

//Connection file
require ('connection.php');

if($_SERVER['REQUEST_METHOD'] == 'POST'){

if(empty($_POST['username'])){
$usernameErr = 'Username cannot be empty';
}else{
$username = test_input($_POST['username']);
}

if(empty($_POST['password'])){
$passwordErr = 'Password cannot be empty';
}else{
$password = test_input($_POST['password']);
$password_hash = md5($password);
}



//Select the data from DB('educate')
$sql = " SELECT username, password FROM users WHERE username='$username' 
AND  password='$password_hash' ";

$result = $conn->query($sql);

if($result->num_rows === 0) {
$usernameErr = 'Username is error';
$password = 'Password is error';
} elseif($result->num_rows === 1) {
//Set cookie (one hour) and moved to  another    page
SETCOOKIE('user', $username, time() + (60*60) );
header('location: plan.php');
}


}
?>

<form action="#" method="post">
Username: <input type="text" name="username" placeholder="username"> 
<?php echo $usernameErr; ?>
<br/><br/>
Password: <input type="password" name="password" placeholder="password">
<?php echo $passwordErr; ?>
<br/><br/>
<input type="submit" value="Login">
</form>

Logout.php:

<?php
setcookie('user',"", time() - (30*30) );
header('location: form signin.php');
?>
\$\endgroup\$
2
\$\begingroup\$

Did you know that anyone can set a cookie whenever they like? Imagine someone will set the "user" cookie to "admin".

Use any of popular PHP frameworks instead. They're here for a reason.

I'd also advise to use prepared statement if you absolutely have to write this kind of code yourself.

$query = "SELECT username, password FROM users WHERE username=? AND password=?";
if ($pst = $conn->prepare($query)) {
  $pst->bind_param("ss", $username, $password_hash);
  $pst->execute();
  if ($pst->num_rows) { 
    // logged in
  }
}
\$\endgroup\$
  • \$\begingroup\$ Can you explain more ? \$\endgroup\$ – Mohamed Ahmed Jul 1 '16 at 10:05
1
\$\begingroup\$

Use PDO and prepared statements to connect to your database. It is safer. An example of doing a database query:

<?php
  // This is using a PDO connection
  $con = new PDO('mysql:host='. $servername .';dbname=' . $db .';charset=utf8', $username, $password);

  // First you do the prepared statement
  $con->prepare('SELECT * FROM USERS WHERE username=:username');

  // Then you enter the variable(s)
  $con->bindValue(':username', $username, PDO::PARAM_STR);

  // Then you retrieve the database row
  $con->fetch();
  // or in case of multiple
  $con->fetchAll();

Use the built in PHP function password() instead of md5(). It is much safer. Every time you use password() on the same password, it generates a different string. To compare a password to the password_hash stored in the database you use password_verify()

<?php
  // Making a password hash during registration
  $password_hash = password_hash($password);

  // During login, check if password is correct::
  if(password_verify($password_fromlogin, $password_hash_fromdatabase))
  {
    // The password is correct
  }
  else
  {
    // The password is not correct
  }

Whenever a user signs in, I suggest using a token. Example of creating a token:

$login_token = hash('sha256', mt_rand());

Store the token inside your users database and cookie/session. (example of a users table in this scenario: username, password_hash, login_token, ...). Whenever you want to check if someone is logged in, compare the login_token from the database with the login_token inside the cookie/session. And when a user logs out, set the login_token in the database and cookie/session to 0.

\$\endgroup\$
0
\$\begingroup\$

Security

There are very many security issues with your code:

  • SQL Injection: Others have mentioned that prepared statements are safer, but really, it's not "safer" than what you have, you are completely vulnerable right now. Anyone can log in as anyone, steal all the data in the database, and possibly even execute code on your server.
  • test_input does nothing to protect against SQL injection (it's a very bad function in general, see eg here and the linked post there; it is most definitely not the right approach to defend against XSS).
  • Faulty Authentication: Cookies are 100% user controlled and cannot be used as you are doing it here to see if someone is logged in. You should instead store some random token in the cookie and compare it against a value stored in the database. Or just use sessions.
  • CSRF: If you are not defending against it somewhere else, you are vulnerable to login (and logout) CSRF. CSRF means that an attacker can perform requests for a victim, which is a very serious issue. Login CSRF is a bit more limited, but it does mean that an attacker can log a victim into an account they control, to either exploit XSS issues in the user area, or in the hope that a user divulges confidential information that the attacker can then see.
  • Bad Hashing: md5 is broken, way too fast (since at the very least 15 years), and you are missing salts. Use bcrypt instead, it's very easy to use.
  • Missing Cookie Flags: Your cookie is neither set as secure, nor as httpOnly, both of which are strongly recommended.
  • you also never die after a header redirect. This may be a problem if your redirect is used to prevent access, as users do not have to follow them. It is best-practice to always die after a header redirect.
  • You didn't post your plan.php file, but I'm hoping that it also checks that a user is authenticated. Just relying on the fact that a user will not guess the filename would not be enough.

If your code is based on some tutorial - which I am assuming - you should choose a different one. This one obviously does not follow any best-practices regarding security. You might also want to check out the OWASP top 10, a list of the most common web application security issues, to get a quick overview.

Comments

You have way too many comments, which don't actually add all that much value. They can in fact draw attention away from comments that are actually important.

Comments shouldn't just repeat what the code already told a reader (except function/class level PHPDoc comments, but they are meant as documentation).

You can get rid of most of them by simply introducing more functions, which would also make your code more structured, easier to maintain, and test. You can - and should - add PHPDoc style comments to your functions.

\$\endgroup\$
  • 1
    \$\begingroup\$ Your feedback has tremendous value but I'm worried that it won't fulfil its potential b/c it's of a level too high for the OP I think. In particular, your CSRF paragraph on its own would only help someone who already knows what that is. I disagree with your take on comments (that the number of comments is too great). They are not meant just for another reader. Comments, especially when you're getting started, can help you learn by forcing you to think through what you're doing, and in the future helping your pick up meaning that a more experience coder would get easily from your code. \$\endgroup\$ – BeetleJuice Jul 1 '16 at 17:23
  • \$\begingroup\$ @BeetleJuice Yes, it's mainly meant as a list of keywords which the OP can search on google to learn more about, implement, and then maybe post a new question with revised code to get more input on issues that they didn't solve correctly. But I added a bit more information and links, I hope it's clearer now. I can see the benefit of comments for beginners, but the question was if this is how professionals would write code, and I would say that the answer to that would be no. \$\endgroup\$ – tim Jul 1 '16 at 17:42
0
\$\begingroup\$

Don't come out of PHP mode unless you're trying to send content to the browser. Doing...

return $data;
}
?>

<?php
$username = $password = $password_hash = '';

Will cause the new lines and blank spaces between PHP blocks to be echoed to the browser. Since this is not your intent, it can result in unintended and difficult to fix problems. Change to:

return $data;
}
$username = $password = $password_hash = '';

For the same reason, do not close the PHP tag at the end of the file unless you're trying to send more content to the browser. In signin.php for example, it makes sense to have the closing tag ?> because you need to send the HTML template below it to the browser. But the end of connection.php and logout.php should not have ?> since you're not trying to send more blank space to the browser.

I understood, what about the code?

I didn't give feedback on the logic because the other answers covered it well. The things that struck me the most were:

  1. You take the existence of a cookie as proof that the user is logged in. Nooooo! Anyone can send any cookie named anything to any site. There are a few good alternatives, but at a minimum your solution must ensure whatever you get from the user (eg: cookie, token) is vetted before you do anything else.
  2. You insert what the user provided directly in your MySQL queries. This is death if anybody wants to hurt you. Never trust content supplied by the browser as safe even if you wrote the website/client application. Look up and use parameterized queries.
  3. You use really weak hashing of your passwords. This is too easy to break. Use PHP's built in functions password_hash and password_verify
  4. You were smart enough to get started and build a working program, and insightful enough to hash your passwords and to realize that you probably missed some things. You'll be alright :-)
\$\endgroup\$
  • \$\begingroup\$ Thanks BeetleJuice:), I understood, what about the code؟ \$\endgroup\$ – Mohamed Ahmed Jul 1 '16 at 17:49
  • \$\begingroup\$ expanded my answer a bit \$\endgroup\$ – BeetleJuice Jul 1 '16 at 18:02

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.