just how bad is my php/mysql.....

Soldato
Joined
6 Feb 2004
Posts
20,862
Location
England
i tried making a user login/registration script and posted it another forum and i got flamed by a mod saying it was the worst he had ever seen. now i admitted up front i was a noob but i don't think it's that bad. any glaring holes (apart from it has no anti-spam protection)

PHP:
<?php
session_start();
include 'db_connect.php';
$here = 'http://'.$_SERVER["SERVER_NAME"].$_SERVER["SCRIPT_NAME"];
if($_GET['do'] == 'logout') {
	unset($_SESSION['isloggedin']);
	unset($_SESSION['username']);
	header("Location: $here");
	exit;
}
function sql_clean($value) {
	if(get_magic_quotes_gpc()) {
		$value = stripslashes($value);
        }
	$value = mysql_real_escape_string($value);
	return $value;
}
if($_POST['register']) {
	$username = sql_clean(trim($_POST['username']));
	$email = sql_clean(trim($_POST['email']));
	if(strlen($username) < 5) {
		$error[] = 'Your username must be at least 5 characters.';
	}
	if((strlen($_POST['password']) < 5) || (strlen($_POST['confirm']) < 5)) {
		$error[] =  'Your password must be at least 5 characters.';
	} elseif ($_POST['password'] != $_POST['confirm']) {
		$error[] = 'Your passwords do not match.';
	} else {
		$password = md5($_POST['password']);
	}
	if(!preg_match( "/^(([^<>()[\]\\\\.,;:\s@\"]+(\.[^<>()[\]\\\\.,;:\s@\"]+)*)|(\"([^\"\\\\\r]|(\\\\[\w\W]))*\"))@((\[([0-9]{1,3}\.){3}[0-9]{1,3}\])|(([a-z\-0-9áàäçéèêñóòôöüæøå]+\.)+[a-z]{2,}))$/i", $email)) {
    		$error[] = 'That doesn\'t appear to be a valid email address.';
	}
	if(is_null($error)) {
		mysql_query("INSERT INTO users(username, password, email) VALUES('$username', '$password', '$email')");
		if(mysql_affected_rows() == 1) {
			$_SESSION['isloggedin'] = true;
			$_SESSION['username'] = $username;
			header("Location: $here");
			exit;
		} else {
			$error[] = 'The username and/or the email address you specified is already in use.';
		}
	}
}
if($_POST['login']) {
	$username = sql_clean(trim($_POST['username']));
	$password = md5($_POST['password']);
	$result = mysql_query("SELECT username FROM users WHERE username = '$username' AND password = '$password'");
	if (mysql_num_rows($result) == 1) {
		$_SESSION['isloggedin'] = true;
		$_SESSION['username'] = $username;
		header("Location: $here");
	} else { 
		$error[] = 'Wrong username/password. Please try again.';
	}
}
?>
<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN"
"http://www.w3.org/TR/html4/strict.dtd">
<html>
<head>
<title></title>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<style type="text/css">
label {
width: 8em;
float: left;
text-align: right;
margin-right: 0.5em;
display: block;
}
</style>
</head>
<body>
<?php
if($_SESSION['isloggedin'] === true) {
	echo '<p>Welcome '.$_SESSION['username'].'</p>';
	echo '<p>At this point we should re-direct the page or something...</p>';
	echo '<p><a href="'.$here.'?do=logout">Logout</a></p>';
} else {
	if(is_array($error)) {
		echo "<p>The following errors were found -</p>";
		foreach($error as $value) {
			echo '<p>'.$value.'</p>';
		}
	}
	if($_GET['do'] == 'register') {
		echo '<form action="" method="post">';
		echo '<p><label>Username:</label><input type="text" name="username" value="'.$_POST['username'].'"></p>';
		echo '<p><label>Password:</label><input type="password" name="password"></p>';
		echo '<p><label>Confirm password:</label><input type="password" name="confirm"></p>';
		echo '<p><label>Your email:</label><input type="text" name="email" value="'.$_POST['email'].'"></p>';
		echo '<p><label>&nbsp;</label><input type="submit" name="register" value="Submit"></p>';
		echo '</form>';
	} else {
		echo '<form action="" method="post">';
		echo '<p><label>Username:</label><input type="text" name="username" value="'.$_POST['username'].'"></p>';
		echo '<p><label>Password:</label><input type="password" name="password"></p>';
		echo '<p><label>&nbsp</label><input type="submit" name="login" value="Submit"></p>';
		echo '</form>';
		echo '<p>Don\'t have an account? Click <a href="'.$here.'?do=register">here</a> to register.';
	}
}
?>
</body>
</html>

sorry no comments but it's fairly basic i think. :)
 
Last edited:
From a security point of view it looks OK for the most part, although you should be sanitising your user input before printing it (using htmlspecialchars() or something).
 
Does php implement regular expressions? If so then you should be using them instead of numerous clauses to validate user and password rules.
 
It's certainly....terse

I'd use more descriptive variable names, you're password-checking function could be tidied up a bit and you could make use of double-quotes in your echo statements. Also the regexp you've used to validate email addresses is quite possibly the ugliest one I've ever laid eyes on :)

But other than that, if it works....
 
M0KUJ1N said:
It's certainly....terse

good. that's my aim. :)

and i've now come to the conclusion the person who flamed me is a prat. he was waffling about how cookies are vulnerable to hacking via javascript. wtf..... i'm not even using cookies. i think the php sessions i'm using are secure? :)

and the regex was googled, i wouldn't know where to being creating my own. :o

i just thought the comments i got were a little harsh. i'm not aiming to make anything pretty. it's just that i see so many forms without the most basic protection against sql injection that made me post it on this other forum. wish i didn't bother now.... :p

more descriptive variable names. you've got me there? what else can i do? :confused:
 
From what I can see its pretty good mate. :D

I'm in the same boat as you. I've only just (over the last 6 months) started to really get into php coding.

I, personally wouldn't have organised the code any different to the way you have. Although I'm sure Augmented will shortly be suggesting an include file with user registration and authentication classes ;)

The more I work on scripts like this, the more I surrender myself to Drupal to do it all for me so I can get on with actually making the site :p

Nice work, keep it up :)
Freakish_05
 
Freakish_05 said:

don't you swear at me. i figure with the progress i've made the last couple of months, it's going to take me about another 72 years before i get to work on classes. :)
 
I've no idea what your skill-level is so I'll go through everything I can see at a glance: apologies if you've already spotted it.

  1. You're setting the session username to your SQL-escaped string, which I should imagine you don't want to do: use the username you just fetched from the query. That way if someone registers a nickname with apostrophes in it—which is half the reason you're escaping your input, the other half being security—they won't have their name displayed as "Foo\'s Beard" everywhere.
  2. This:

    Code:
    if(mysql_affected_rows() == 1) {
    	$_SESSION['isloggedin'] = true;
    	$_SESSION['username'] = $username;
    	header("Location: $here");
    	exit;
    } else {
    	$error[] = 'The username and/or the email address you specified is already in use.';
    }

    ...will only work if you have a UNIQUE constraint on your username field (or if the username is the primary key), so make sure you do.
  3. If someone registers with HTML in their username, which you allow, you print their username without sanitising it—allowing them to inject arbitrary and potentially malicious HTML into your script. Change:

    Code:
    echo '<p>Welcome '.$_SESSION['username'].'</p>';

    ...to:

    Code:
    echo '<p>Welcome ' . htmlentities($_SESSION['username']) . '</p>';

Other than that, top-notch.

(The person who said it was the worst they'd ever seen obviously hasn't seen the sort of scripts I have to deal with every day... eurgh.)
 
1) ok
2) yup, my username field was already set to unique.
3) oops. that's nasty.... ok done

i only mess about with php for something to do - my skills are very limited indeed. i've only just figured out how to use functions. classes are long way off for me.

and i only posted this on this other forum to counteract stuff i was seeing like this.....

PHP:
mysql_query("INSERT INTO `users`(`user_name`,`password`) VALUES ('".$_POST['user_name']."','".$_POST['password']."')") or die('Cannot insert data to db: '.mysql_error());

:eek:

cheers for the replies everyone and especially rob for the help. :)
 
Last edited:
Back
Top Bottom