2
\$\begingroup\$

I've been working on a simple forum website as a toy project, to start learning PHP and SQL. I've implemented all of the functionality I want, but I thought I'd run the code by the Code Review community to see how I could improve it.

<?php
session_start();
class database{
	protected static $connection;
	protected $state=0; //0 is fine, 1 is connection error, 2 is query error

	public function connect(){
		if(isset(self::$connection)){
			return self::$connection;
		}
		else{
			self::$connection=new mysqli('localhost','root','','mydummydatabase');
			if(self::$connection===false){
				$this -> state=1;
				return false;
			}
			return self::$connection;
		}
	}
	public function query($query){
		$connection=$this -> connect();
		$result=$connection -> query($query);
		if($result===false){
			$this -> state=2;
		}
		return $result;
	}
	public function sanitise($input){
		$connection=$this -> connect();
		return "'".$connection -> real_escape_string($input)."'";
	}
	public function get_first_row($result){
		if($result){
			return $result -> fetch_assoc();

		}
		else{
			return false;
		}
	}
	public function state(){
		return $this->state;
	}

}
$db=new database();
$_SESSION['email-exists-in-database']=false;
$_SESSION['something-weird-happened']=false;
if(isset($_POST['email'])){
	$firstname=$db->sanitise($_POST['firstname']);
	$lastname=$db->sanitise($_POST['lastname']);
	$email=$db->sanitise($_POST['email']);
	$hash=password_hash($_POST['password'],PASSWORD_DEFAULT);
	$result=$db -> query("SELECT `user-id` FROM `users` WHERE email=".$email);
	$record=$db->get_first_row($result);
	if($record!=false){
		$_SESSION['email-exists-in-database']=true;

	}
	else{
		$db -> query("INSERT INTO `users` (firstname,lastname,email,passwordhash) VALUES (".$firstname.",".$lastname.",".$email.",'".$hash."')");

		if(!($db->state())){
			$result=$db -> query("SELECT `user-id` FROM `users` WHERE email=".$email);
			$record=$db->get_first_row($result);
			$_SESSION['id']=$record['user-id'];
			header('Location: welcome.php');
			exit;
		}
		else{
			$_SESSION['something-weird-happened']=true;

		}
	}
}
?>
<html>
<head>
<link rel="stylesheet" media="screen" href="styles2.css"/>
</head>
<body>
<div  class="input-form">
<img src="account-logo.png" id="account-logo">
<form action="<?php echo $_SERVER['PHP_SELF'];?>" method="post">
<input type="text" maxlength="30" name="firstname" placeholder="First name"class="sign-in-box" required/>
<input type="text" maxlength="60" name="lastname" placeholder="Last name"class="sign-in-box" required/>
<input type="email" maxlength="100" name="email" placeholder="Email"class="sign-in-box" required/>
<input type="password" maxlength="200" name="password" placeholder="Password"class="sign-in-box" required/>

<input type="submit" name="Submit" value="Sign up"class="submit-button"/>
</form>
</div>
<?php
if($_SESSION['email-exists-in-database']){
	echo("<div class='error-message'>We already have an account associated with that email.</div>");
}
if($_SESSION['something-weird-happened']){
	echo("<div class='error-message'>Something went wrong, please try again.</div>");
}?>
</body>
</html>

This is the Register page for the website. Some issues I'm concerned about are security, and good practice.

Any thoughts?

\$\endgroup\$
3
\$\begingroup\$

About your HTML

DOCTYPE

You are missing a DOCTYPE. You probably want to use

<!DOCTYPE html>

title element

The title element is a required element (in the very most cases). It has to be added to the head:

<head>
  <title>Register …</title>
</head>

alt attribute

The alt attribute is required for img elements.

If you account-logo.png is just decoration, the alt value should be empty:

<img src="account-logo.png" id="account-logo" alt="">

label elements and placeholder attributes

You shouldn’t use the placeholder attribute to "label" input elements. Add label elements instead.

If you want to use the placeholder attribute, it should show an example of what should be entered, or a description of the expected format. See placeholder definition in HTML 5.1.

<label for="email">Email</label>
<input type="email" maxlength="100" name="email" id="email" placeholder="alice@example.com" class="sign-in-box" required />

Usability: maxlength

Do you really want a max length here?

Because there are certainly names longer than 30/60 chars, and email addresses longer than 100 chars. Those users would have to abbreviate their names or use a different email address.

strong for warnings

You could make use of the strong element for the warnings/errors.

A heading

Your page has no headings. You’ll probably add site-wide stuff (like a header, navigation etc.) that contains headings, but even your form (i.e., this page’s main content) should get a heading.

meta-charset

Not required if you provide it via HTTP headers, but it’s typically a good idea to provide a meta-charset element in the head (as first element), e.g. if you use UTF-8:

<head>
  <meta charset="utf-8" />
  <!-- 'title' and 'link' follow here -->
</head>
\$\endgroup\$
2
\$\begingroup\$

Security

You are vulnerable to XSS:

<form action="<?php echo $_SERVER['PHP_SELF'];?>" method="post">

An attack may be:

http://example.com/register.php/"><img src=x onerror=alert(1)>

You should HTML encode all output - ideally by default - to defend against XSS. In some contexts, you need further defenses against XSS.

Note also that your approach to SQL injection is not recommended. You are not currently vulnerable, but when using escaping, it's only a matter of time until you introduce a vulnerability. Escaping is theoretically less secure, it is more difficult to use correctly, it is more difficult to check for errors, and results in code that is just less readable than when using the prepared statements.

Formatting

Your formatting is mostly consistent, which is always good. Personally, I would prefer more whitespace. I would add spaces around = and . to increase readability, as well as newlines before function definitions. Your spacing around -> is not consistent. I would suggest to remove the existing spaces.

Error handling

Your connect functions returns false if it cannot connect, but you never check the return value.

Using a state field as error holder is also less than optimal (it's not really clear where the error comes from or what it represents). But if you do, you should check the actual content of the field.

Personally, I would recommend to throw an exception instead, thus forcing the calling code to actually handle errors, instead of giving the opportunity to silently ignore them.

Database class

Your database class currently establishes a connection and provides very thin wrappers for query, real_escape_string, and fetch_assoc. You are not really adding any new functionality, but essentially just renaming well-known functions and slightly changing their functionality. You also add an additional connect call to each call to sanitize and query which is unnecessary.

I would completely remove the class, as it doesn't provide any real benefit.

Misc

  • fields should be private if there is no good reason for them not to be.
  • class names should be uppercase.
\$\endgroup\$

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.