[SOLVED] form validation not working + some tips on good coding practice please

farkewie · December 1, 2007

Hello, the only thing not working on this form is the username and email check, it still lets me insert new user when the username or email are the same as the database,

also, i can normally get by with the code but i want to learn to make sure im in the habit of doing things the right way, eg; security and script speed, any tips on this one would be great.

here is my code


<?php 
include ("inc/sql.php");
include ("header.php");


//Check the form has been submitted and is the register form
if ((isset($_POST['button'])) && (($_POST['button']) == "Register")) {
$number = md5($_POST['email']);
// check the username is not in the database

$query = "SELECT * FROM users";
if ($result = mysql_query($query)) {
if ($row = mysql_fetch_array($result)){
	if (($row['uname']) == ($_POST['uname'])) {
	$error = "<li>Username Already Exists!!</li>";
	}
	if (($row['email']) == ($_POST['email'])) {
	$error .= "<li>Email Already Exists!!</li>";
	}
}
}

//Check passwords Match
if (($_POST['password']) !== ($_POST['password_confirm'])){
$error .= "<li>Passwords Do Not Match!!</li>";
}
//Check emails match
if (($_POST['email']) !== ($_POST['email_confirm'])){
$error .= "<li>Email Addresses Do Not Match!!</li>";
}
if (empty($error)){
$sql = "INSERT INTO users VALUES ('','".addslashes($_POST['uname'])."','".$_POST['fname']."','".$_POST['lname']."','".md5($_POST['password'])."','yes','".date("d-m-Y")."','$number')";
	if (!$result = mysql_query($sql)){
		$error = "There Was An Error Proccessing Your Request<br><strong>Error Details:  </Strong>".mysql_error().""; 
}
	else {

$message = "User:   <strong>".$_POST['uname']." </strong>Has Been Added!";
}

}


}
?>

Thanks for your help

farkewie · December 1, 2007

Ok so i solved it but i would still love some feedback on this code if anyone has the time?


<?php 
include ("inc/sql.php");
include ("header.php");


//Check the form has been submitted and is the register form
if ((isset($_POST['button'])) && (($_POST['button']) == "Register")) {
$number = md5($_POST['email']);
// check the username is not in the database

$query = "SELECT * FROM users WHERE uname='".addslashes($_POST['uname'])."'";
if ($result = mysql_query($query)) {
if ($row = mysql_fetch_assoc($result)){
	$error = "<li>Username Already Exists!!</li>";
}
else { $error .= "".mysql_error()."";}		
}
$query2 = "SELECT * FROM users WHERE email='".addslashes($_POST['email'])."'";
if ($result2 = mysql_query($query2)) {
if ($row2 = mysql_fetch_assoc($result2)){
	$error .= "<li>Email Already Exists!!</li>";

}
else { $error .= "".mysql_error()."";}	
}
//Check passwords Match
if (($_POST['password']) !== ($_POST['password_confirm'])){
$error .= "<li>Passwords Do Not Match!!</li>";
}
//Check emails match
if (($_POST['email']) !== ($_POST['email_confirm'])){
$error .= "<li>Email Addresses Do Not Match!!</li>";
}
if (empty($error)){
$sql = "INSERT INTO users VALUES ('','".addslashes($_POST['uname'])."','".$_POST['fname']."','".$_POST['lname']."','".addslashes($_POST['email'])."','".md5($_POST['password'])."','yes','".date("d-m-Y")."','$number')";
	if (!$result = mysql_query($sql)){
		$error = "There Was An Error Proccessing Your Request<br><strong>Error Details:  </Strong>".mysql_error().""; 
}
	else {

$message = "User:   <strong>".$_POST['uname']." </strong>Has Been Added!";
}

}


}
?>

PHP_PhREEEk · December 1, 2007

1. Before including anything, let's see if we are even going to let them in.

2. If POST['button'] does not need to be checked with isset. If it equals anything, it's set. Don't work with a variable (in this case, EMAIL) until you need that variable. Scattered variables get lost in the code, making it harder to trouble shoot. Keep variables near the processes that will be using them.

3. If POST['button'] == 'Register', then let's make sure required fields have data in them. We'll hold off including the header.php file, because I assume it's just HTML, right? We don't send any HTML until we are committed to doing so. This will save you HEADER errors down the road, if and when you end up using them in a script. Remember that... do NOT send any HTML until we have everything we need to send the entire document! I know it doesn't matter in this test script, but it will matter down the road.

4. Best to get away from addslashes and use the mysql_real_escape_string function (MRES). Let's sanitize the variables being checked and assign them new variable names. This saves typing down the road. Let's discuss what we're doing. The inner-most parens get worked on first, so we'll discuss operations from the inside back out.

<?php
$uname2 = trim ( substr ( mysql_real_escape_string( $_POST['uname'] ), 0, 25 ) );

First, we do the MRES on the POST variable. Then we force a truncate of anything greater than the third argument to substr (I used 25 arbitrarily). Why do this? Even if you use maxlength in your Form, it can be overridden by hackers. If you don't substr down to the size of your database field, MySQL will generate a error on INSERT. If a hacker can intentionally cause your script to error out at will, that's a bad thing... We also truncate after the call to MRES because if MRES added any slashes, that increases our character count. Then we issue a trim on the string, which truncates any white-space(s) from both the front and end of the string. Lastly, we assign the result to $uname2, which can now safely use in place of $_POST['uname'] whenever we need it. We do likewise operations on email and password.

There's always 10 ways to do something... the above code works, but we could also do the following:

<?php
if ( eregi( "[^a-z0-9_-]", $_POST['uname'] ) ) {
    $error . = 'The Username entered has invalid characters!
                <br>
                Valid Username characters are alpha letters, numbers, underscore and hyphen.
                <br>
                Please re-enter a valid Username and try again!';
}
$uname2 = substr( $_POST['uname'], 0, 25 );

We're using a character class with ereg (case insensitive) to error out on any characters we don't want by specifying the ones we do want. If there's no characters except what we want, we do not need MRES. In my ereg, spaces are not allowed, so we do not need to trim either. We can just truncate with substr and move on. Most coders will gravitate towards the alternative I've just shown you when dealing with usernames, because we like control over every single incoming character if we can. For larger forms with varying types of data, you will many times use both methods. As you can see, each has its strength and weaknesses.

Here's a rewrite of your script. There may be parse errors... I didn't run it. These changes are just general and meant to accomodate your current level of logic and understanding. I would do a few things differently, but that's beyond the scope of this post.

PhREEEK

<?php
//Check the form has been submitted
if ( $_POST['button']) == "Register" ) {
    // check obvious required fields are populated
    if ( empty($_POST['uname']) || empty($_POST['email'] || empty($_POST['password'] ) {
        $error = 'You must enter a Username, Email Address, and Password!';
    }
    chk_errors($error);
    //Check passwords and emails match
    if ( $_POST['password'] !== $_POST['password_confirm'] ) {
        $error = 'Passwords Do Not Match!!<br>';
    }
    if ( $_POST['email'] !== $_POST['email_confirm'] ) {
        $error .= 'Email Addresses Do Not Match!!';
    }
    chk_errors($error);
    // so far, so good, let's connect to the database
    include ("inc/sql.php");
    $uname2 = trim( substr( mysql_real_escape_string( $_POST['uname'] ), 0, 25 ) );
    $email2 = md5( substr( mysql_real_escape_string( $_POST['email'] ), 0, 30 ) );
    $user_pass = md5( substr( mysql_real_escape_string( $_POST['password'] ), 0, 25 ) );
    $query = "SELECT `uname`, `email` FROM `users`
              WHERE uname='" . $uname2 . "'
              OR email='" . $email2 . "' 
             ";
    if ( !$result = mysql_query($query) ) {
        die('MySQL error: ' . mysql_error());
    }
    if ( mysql_num_rows($result) > 0 ) {
        while ( $row = mysql_fetch_assoc($result) ) {
            if ( $row['uname'] == $uname2 ) {
                $error .= 'The username ' . $uname2 . ' is already taken!<br>';
            }
            if ( $row['email'] == $email2 ) {
                $error .= 'The email ' . $email2 . ' is already registered!<br>';
            }
        }
    }
    chk_errors($error);
    // If we made it this far, username, email, and password are clear
    // fname and lname need sanitized like above. No idea where $number comes from. 
    $sql = "INSERT INTO `users`
            VALUES ('', '$uname2', '" . $_POST['fname'] . "', '" . $_POST['lname'] . "', '$email2',
                    '$user_pass', 'yes', '" . date("d-m-Y") . "', '$number')
           ";
    if ( !$result = mysql_query($sql) ) {
        die('There Was An Error Proccessing Your Request<br><strong>Error Details:  </Strong>' . mysql_error()); 
    }
    // Houston! We are committed to sending HTML!
    include("header.php");
    echo 'User: <strong>' . $uname2 . '</strong> has been added!';
    // Where's our footer??
    // include("footer.php");
} else {
    // Houston! We are committed to sending HTML!
    $error = 'You cannot access this page directly!';
    chk_errors($error);
}

function chk_errors($error = false) {
    // If any errors, let's display them - there are cleaner ways of doing this, this is an example
    if ( $error ) {
        // Houston! We are committed to sending HTML!
        include("header.php");
        echo $error;
        // Where's our footer??
        // include("footer.php");
        die();
    }

}

?>

farkewie · December 1, 2007

Thanks you so much i never expected such a detailed reply!!

i just though i would get a couple of good links to look at.

i did make some changes for the following reasons.

1. i am running the script on the samepage as the for , ie form action is $_SERVER['PHP_SELF']

2. i added the fname & lname sanitizing as you advised,

3. because i wanted my errors displayed in a list and the form text fields to be filled in still on error i took away the chk_errros function and added an "if" statment to the sql insert.

4. the md5($_POST['email']) is a confirm key for email verification.

here is the whole script if you get a chance from what i can see i dont think i destroyed your advise, let me know what you think if i have



<?php 
include("inc/sql.php");
//Check the form has been submitted
if ( ($_POST['button']) == "Register" ) {
    // check obvious required fields are populated
    if ( empty($_POST['uname'])){ 
        $error = "<li>You must enter a Username</li>";
    }
if (empty($_POST['email'])){ 
	$error .= "<li>You must enter an Email</li>";
}	
if (empty($_POST['password'])) {
	$error .= "<li>You must enter a Password</li>";
}
if (empty($_POST['fname'])) {
	$error .= "<li>You must enter your First Name</li>";
}
if (empty($_POST['lname'])) {
	$error .= "<li>You must enter your Last Name</li>";
}
    
    //Check passwords and emails match
    if ( $_POST['password'] !== $_POST['password_confirm'] ) {
        $error .= '<li>Passwords Do Not Match!!</li>';
    }
    if ( $_POST['email'] !== $_POST['email_confirm'] ) {
        $error .= '<li>Email Addresses Do Not Match!!</li>';
    }
    // so far, so good, let's connect to the database
    include ("inc/sql.php");
    $uname2 = trim( substr( mysql_real_escape_string( $_POST['uname'] ), 0, 25 ) );
    $email2 =  substr( mysql_real_escape_string( $_POST['email'] ), 0, 30 ) ;
    $user_pass = md5( substr( mysql_real_escape_string( $_POST['password'] ), 0, 25 ) );
    $query = "SELECT `uname`, `email` FROM `users`
              WHERE uname='" . $uname2 . "'
              OR email='" . $email2 . "' 
             ";
    if ( !$result = mysql_query($query) ) {
	include("head.php");
        die('MySQL error: ' . mysql_error());
    }
    if ( mysql_num_rows($result) > 0 ) {
        while ( $row = mysql_fetch_assoc($result) ) {
            if ( $row['uname'] == $uname2 ) {
                $error .= '<li>The username ' . $uname2 . ' is already taken!</li>';
            }
            if ( $row['email'] == $email2 ) {
                $error .= '<li>The email ' . $email2 . ' is already registered!</li>';
            }
        }
    }
    // If we made it this far, username, email, and password are clear
    // fname and lname need sanitized like above. No idea where $number comes from. 
if (empty($error)) {
	$fname2 = substr( mysql_real_escape_string( $_POST['fname'] ), 0, 25 ) ;
	$lname2 = substr( mysql_real_escape_string( $_POST['lname'] ), 0, 25 ) ;
	$confirmkey = md5($_POST['email']); // this is here for later if i want to send a confirmation email 
	$sql = "INSERT INTO `users`
			VALUES ('', '$uname2', '$fname2', '$lname2', '$email2',
					'$user_pass', 'yes', '" . date("d-m-Y") . "','', '$confirmkey')
		   ";
		if ( !$result = mysql_query($sql) ) {
			include("head.php");
			die('There Was An Error Proccessing Your Request<br><strong>Error Details:  </Strong>' . mysql_error()); 
    		}
		else {
			$message =  'User: <strong>' . $uname2 . '</strong> has been added!';
		}
}
} 
include ("head.php");
include ("header.php");
?>


<script src="SpryAssets/SpryValidationTextField.js" type="text/javascript"></script>
<link href="SpryAssets/SpryValidationTextField.css" rel="stylesheet" type="text/css">
<form name="form1" method="post" action="<?php echo $_SERVER['PHP_SELF']; ?>">
  <table width="728" height="382" align="center">
    <tr>
      <td height="42" colspan="2" align="left" valign="bottom"><h3>Registration form</h3>
      <?php if (isset($_POST['button'])) { if (!empty($error)) {echo "<blockquote><ol>"; echo "<font color=\"red\">".$error."</font>"; echo "</ol></blockquote>";} if (!empty($message)) { echo "<blockquote><font color=\"green\">".$message."</font></blockquote>";}} ?>
         
        </td>
    </tr>
    <tr>
      <td width="177" height="44" align="left" valign="bottom"><label>First Name:</label></td>
      <td width="539" align="left" valign="bottom"><span id="spryfname">
        <label>
      <input name="fname" type="text" id="fname" value="<?php if ((isset($_POST['fname'])) && (!empty($error))) { echo stripslashes($_POST['fname']); } ?>">
      <span class="textfieldRequiredMsg">Your First name is Required.</span><span class="textfieldMinCharsMsg">Minimum number of characters not met.</span><span class="textfieldMaxCharsMsg">To Many Characters.</span></label>
      </label>
      </span></td>
    </tr>
    <tr>
      <td height="39" align="left" valign="bottom"><label>Last Name:</label></td>
      <td align="left" valign="bottom"><span id="sprylname">
        <label>
        <input type="text" name="lname" value="<?php if ((isset($_POST['lname'])) && (!empty($error))) { echo stripslashes($_POST['lname']); } ?>" id="lname">
        <span id="sprylname2"><span class="textfieldMaxCharsMsg"></span></span><span id="sprylname3"><span class="textfieldMaxCharsMsg"></span></span><span id="sprylname4"><span class="textfieldMaxCharsMsg"></span></span><span class="textfieldMaxCharsMsg">To Many Characters.</span><span class="textfieldRequiredMsg">Your Last Name is Required.</span><span class="textfieldMinCharsMsg">Minimum number of characters not met.</span></label>
      </span></td>
    </tr>
    <tr>
      <td height="39" align="left" valign="bottom"><label>Desired Username:</label></td>
      <td align="left" valign="bottom"><span id="spryuname">
      <label>
      <input type="text" name="uname" value="<?php if ((isset($_POST['uname'])) && (!empty($error))) { echo stripslashes($_POST['uname']); } ?>" id="uname">
      <span class="textfieldRequiredMsg">A username is required.</span><span class="textfieldMinCharsMsg">Minimum number of characters not met.</span><span class="textfieldMaxCharsMsg"> Username Exceeded maximum number of characters.</span></label>
      </span></td>
    </tr>
    <tr>
      <td height="39" align="left" valign="bottom"><label>Password:</label></td>
      <td align="left" valign="bottom"><span id="sprypassword">
      <label>
      <input type="password" name="password" id="password">
      <span class="textfieldMaxCharsMsg">Password to long.</span><span class="textfieldMinCharsMsg">password to short.</span><span class="textfieldRequiredMsg">You need a password.</span></label>
      </span></td>
    </tr>
    <tr>
      <td height="39" align="left" valign="bottom"><label>Cofirm Password:</label></td>
      <td align="left" valign="bottom"><span id="sprypassword_confirm">
      <label>
      <input type="password" name="password_confirm" value="" id="password_confirm">
      <span class="textfieldMaxCharsMsg">password to long.</span><span class="textfieldMinCharsMsg">Password to short.</span><span class="textfieldRequiredMsg">Please Confirm Password.</span></label>
      </span></td>
    </tr>
    <tr>
      <td height="39" align="left" valign="bottom"><label>E-mail Address:</label></td>
      <td align="left" valign="bottom"><span id="spryemail">
        <label>
        <input type="text" name="email" value="<?php if ((isset($_POST['email'])) && (!empty($error))) { echo stripslashes($_POST['email']); } ?>" id="email">
        <span class="textfieldInvalidFormatMsg">Invalid E-mail Address!!.</span><span class="textfieldRequiredMsg">An Email address is required.</span></label>
      </span></td>
    </tr>
    <tr>
      <td height="39" align="left" valign="bottom"><label>Confirm E-mail Address:</label></td>
      <td align="left" valign="bottom"><span id="spryemail_confirm">
      <label>
      <input type="text" name="email_confirm" value="<?php if ((isset($_POST['email_confirm'])) && (!empty($error))) { echo stripslashes($_POST['email_confirm']); } ?>" id="email_confirm">
      <span class="textfieldInvalidFormatMsg">Invalid E-mail Address!!.</span><span class="textfieldRequiredMsg">Please confirm Email address.</span></label>
      </span></td>
    </tr>
    <tr>
      <td height="40" align="left" valign="bottom"> </td>
      <td align="left" valign="bottom"><label for="button">
        
        <div align="left">
          <input type="submit" name="button" id="button" value="Register">
        </div>
      </label></td>
    </tr>
  </table>
</form>
<script type="text/javascript">
<!--
var sprytextfield1 = new Spry.Widget.ValidationTextField("spryfname", "none", {minChars:1, maxChars:25, validateOn:["blur"]});
var sprytextfield2 = new Spry.Widget.ValidationTextField("sprylname", "none", {minChars:1, maxChars:25, validateOn:["blur"]});
var sprytextfield3 = new Spry.Widget.ValidationTextField("spryuname", "none", {validateOn:["blur", "change"], minChars:2, maxChars:25});
var sprytextfield4 = new Spry.Widget.ValidationTextField("sprypassword", "none", {minChars:4, maxChars:25, validateOn:["blur"]});
var sprytextfield5 = new Spry.Widget.ValidationTextField("sprypassword_confirm", "none", {validateOn:["blur"], minChars:4, maxChars:25});
var sprytextfield6 = new Spry.Widget.ValidationTextField("spryemail", "email", {validateOn:["change"]});
var sprytextfield7 = new Spry.Widget.ValidationTextField("spryemail_confirm", "email", {validateOn:["change"]});
//-->
</script>
<?php include ("footer.php")

?>

Once again thank you so much for your help.

mlin · December 1, 2007

if $_POST['uname'] is not empty, but any of your other required fields are empty your trying to concantenate a string to a string that doesn't exist. It may work, but it's at least throwing a warning everytime. You could save your error log by setting $error = ''; before all of you if empty conditionals.

I really don't think you ought to truncate email addresses or passwords. If a user enters a 30 char password (paranoid) and you truncate the last 5 chars without letting them know, their hash will never match. Also, MySQL will truncate for you. If you send a 30 char value to a varchar(12) field, it will save the 1st 12 chars.

It looks good, I'm sure it's running just fine

PHP_PhREEEk · December 1, 2007

if $_POST['uname'] is not empty, but any of your other required fields are empty your trying to concantenate a string to a string that doesn't exist. It may work, but it's at least throwing a warning everytime. You could save your error log by setting $error = ''; before all of you if empty conditionals.

I really don't think you ought to truncate email addresses or passwords. If a user enters a 30 char password (paranoid) and you truncate the last 5 chars without letting them know, their hash will never match. Also, MySQL will truncate for you. If you send a 30 char value to a varchar(12) field, it will save the 1st 12 chars.

It looks good, I'm sure it's running just fine

If you have a fixed length field for the password in your form (which you should for every field), and the submitted variable exceeds that, then truncate. You're being rather picky about something that costs almost nil processing time but provides robust precision. I agree in principal with the password issue, since the result of the md5 function will result in a fixed length string anyways, but still, if you request a max field length for anything, be it password or whatever, the user should not be able to override it, period.

PhREEEk

farkewie · December 1, 2007

Thanks for our replys,

i have set

$error = '';

You are right PhREEEk, all my fields your ajax to: make sure a field is filled out, min and max chars, email is a valid email so a lot of this error checking is here as an extra mesure and also incase the login form ever gets changed.

once again thanks.

Sign In

[SOLVED] form validation not working + some tips on good coding practice please

Recommended Posts

farkewie

Link to comment

Share on other sites

farkewie

Link to comment

Share on other sites

PHP_PhREEEk

Link to comment

Share on other sites

farkewie

Link to comment

Share on other sites

mlin

Link to comment

Share on other sites

PHP_PhREEEk

Link to comment

Share on other sites

farkewie

Link to comment

Share on other sites

Join the conversation

Browse

Activity

Important Information