Page 1 of 3

Gaaah! Error codes and everything!

Posted: Mon Dec 18, 2006 3:46 pm
by Mightywayne
Everah | Please do not post actual database connection credentials.

I'm thinking it's cuz I'm drained (been learnin' this all day and such) but I can just not, for the life of me, figure out what's wrong with this.

Code: Select all

<?php

$con = mysql_connect("localhost","****","****");

if (!$con)
{
die('Could not connect: ' . mysql_error());
}

define('username',$_POST['username']);
define('password',$_POST['password']);
define('cpassword',$_POST['cpassword']);
define('email',$_POST['email']);




//Checking for typo's and stuff now.

$findat = strpos(email, "@");
$finddot = strpos(email, ".");


//Now Check Email Is ---Entered---.
//$findat & $finddot Are Defined Earlier.

if ((username == "") || (password == "") || (cpassword == "") || (email == "")) {
die("You must fill out all fields! Go back and fix it.";
}


//(If Pass1 Is Unequal To Pass 2) And (If "@" Not Found Or "." Not Found)

if ((password!=cpassword) && ($findat == false) || ($finddot == false)) 
{
echo "The passwords you entered do not match, and your email was incorrect. Go back and fix it!";
}


//Check Passwords Are Equal.

elseif ((password!=cpassword)) {
echo "The passwords you entered do not match!";
echo 'Go back and fix it!';
}


//Now Check Email Is ---Valid---.

elseif ((($findat == false) || ($finddot == false)) && ($mail != "")) {
echo "The email you entered is invalid!";
echo 'Go back and fix it!';
}


mysql_select_db("burnttoa_monbre", $con);


//To prevent people from having two of the same name.
$query = mysql_query("SELECT name FROM user");
$usernamecheck = @mysql_fetch_array($query);

if (($usernamecheck = $usernamecheck['name']) && !($usernamecheck['name'] = "")) {
	die("Sorry, that name's already in use! You'll have to pick another.");
      }

//To prevent people from having two of the same email.
$querytwo = mysql_query("SELECT email FROM user");
$emailcheck = @mysql_fetch_array($querytwo);

if (($emailcheck = $emailcheck['email']) && !($emailcheck['email'] = "")) {
	die("Sorry, that email's already in use! You'll have to pick another.");
	}

mysql_query("INSERT INTO user (ID, name, pword, email, money, ranch, barn, happy, tlevel, verified)
VALUES ('', '".username."', '".password."', '".email."', '3000', '1', '1', '70', '1', 'no')")
or die(mysql_error());

mysql_query("INSERT INTO items (userbagID)
VALUES ('')")
or die(mysql_error());

if (($findat == true) && ($finddot == true))
{
echo "Welcome to Monbre, ".username."! Soon you will get an email that requires your email adress to be verified before actually allowing acess to the game.";
}

?>
For the most part I'm pretty sure it's okay security-wise, but now I tried to be professional here, with this whole "your password AND email are wrong" and I think I'm just effing it up somehow.

The first problem, is that when they typo their email, it will no matter what say that they typo'd their second password also. (as in, "you have typo'd your password AND your email is wrong")

The second is that no matter what, in this code:

Code: Select all

//To prevent people from having two of the same name.
$query = mysql_query("SELECT name FROM user");
$usernamecheck = @mysql_fetch_array($query);

if (($usernamecheck = $usernamecheck['name']) && !($usernamecheck['name'] = "")) {
	die("Sorry, that name's already in use! You'll have to pick another.");
      }

//To prevent people from having two of the same email.
$querytwo = mysql_query("SELECT email FROM user");
$emailcheck = @mysql_fetch_array($querytwo);

if (($emailcheck = $emailcheck['email']) && !($emailcheck['email'] = "")) {
	die("Sorry, that email's already in use! You'll have to pick another.");
	}
Even if they have the same double email, it will still tell them their NAME is already in use. :(

What can you guys do for me?! (also if you happen to notice any other errors, I'm open)

Posted: Mon Dec 18, 2006 4:12 pm
by feyd
Are you asking us to read through all your code and figure it out for you?

Posted: Mon Dec 18, 2006 4:45 pm
by iknownothing
Maybe use == (Is equal to) , as you are just changing the value of $usernamecheck there...

Code: Select all

if (($usernamecheck == $usernamecheck['name']) && !($usernamecheck['name'] = "")) {
        die("Sorry, that name's already in use! You'll have to pick another.");
      }

Posted: Mon Dec 18, 2006 9:05 pm
by Mightywayne
feyd wrote:Are you asking us to read through all your code and figure it out for you?
Good point. I'll edit the useless stuff out.

Edit: Wait. Uhm. Then yeah, I guess I am, cuz I can't find anything that wouldn't help people analyze the code.


And knownothing, I'll try that now.

Edit: And no. :(

Re: Gaaah! Error codes and everything!

Posted: Mon Dec 18, 2006 9:18 pm
by chakhar86
Mightywayne wrote:$usernamecheck = @mysql_fetch_array($query);
Hey, what is meaning of @ here, cannot find in PHP manual

Posted: Mon Dec 18, 2006 9:22 pm
by Mightywayne
Actually my friend helped me with that, and it's supposed to send something in a more organized way. I guess it's only good for things involving arrays, or, it could be needed for arrays.

Posted: Mon Dec 18, 2006 10:03 pm
by John Cartwright
Mightywayne wrote:Actually my friend helped me with that, and it's supposed to send something in a more organized way. I guess it's only good for things involving arrays, or, it could be needed for arrays.
Choose your friends wisely, because he is wrong :wink:. The @ is the error supression operator.

Posted: Mon Dec 18, 2006 11:19 pm
by feyd
.. and we often say "booooo" to it's usage. :) Proper error handling is better.

Posted: Tue Dec 19, 2006 12:01 am
by daedalus__
The error suppression operator is also very slow.

use mysql

Posted: Wed Dec 20, 2006 6:34 pm
by afs
Why not use mysql to check ie:

select id from user where name = '$name'

if mysql_num_row > 0 then the name is taken.


Also email check not too good. try;

function IsEMail($e)
{
if(eregi("^[a-zA-Z0-9]+[_a-zA-Z0-9-]*
(\.[_a-z0-9-]+)*@[a-zöäüÖÄÜ0-9]+
(-[a-zöäüÖÄÜ0-9]+)*(\.[a-zöäüÖÄÜ0-9-]+)*
(\.[a-z]{2,4})$", $e))
{
return TRUE;
}
return FALSE;
}

Posted: Wed Dec 20, 2006 7:57 pm
by RobertGonzalez
How about actually using some variable delimiters (dollar signs $)? Sup with this?!?!?!?!?

Code: Select all

<?php
if ((username == "") || (password == "") || (cpassword == "") || (email == "")) {
die("You must fill out all fields! Go back and fix it.";
}


//(If Pass1 Is Unequal To Pass 2) And (If "@" Not Found Or "." Not Found)

if ((password!=cpassword) && ($findat == false) || ($finddot == false))
{
echo "The passwords you entered do not match, and your email was incorrect. Go back and fix it!";
}


//Check Passwords Are Equal.

elseif ((password!=cpassword)) {
echo "The passwords you entered do not match!";
echo 'Go back and fix it!';
}
?>

Posted: Wed Dec 20, 2006 8:38 pm
by Ollie Saunders
Major problems with your code:
  • Defining tainted data as constant. All input is tainted until you prove otherwise
  • SQL is not escaped. Have a look at SQL injection and mysql's escaping function
  • Very poor validation of email address. Use a regular expression, one has been posted on this thread already but if you search the board you will find other solutions.
  • Use of error suppression operator is bad practise (My opinion on that thread is a bit too hard lined but you'll get the picture) and often makes life harder for yourself.
  • Where you are successfully handling errors you are just printing them unconditionally. This exposes information about your form to hackers and generally makes it easier for them. Also it doesn't look very professional when it happens. Here's one possible solution:

    Code: Select all

    error_reporting(E_ALL);
    if ($_SERVER['HTTP_HOST'] == 'localhost') { // or development server name here
        ini_set('display_errors', true);
    } else {
       ini_set('display_errors', false);
    }
    
    $result = mysql_query($q) or trigger_error(mysql_error());
    if you don't separate development and production servers then you'll have to base your condition on something else.
On a side-note your error messages aren't polite...

Code: Select all

echo 'Go back and fix it!';
...and are likely to scare or annoy your users. These things have been psychologically tested and proven to have a real effect on peoples perception of the quality of the interface. In short -- if you leave them like that people will think your form is crap and will resent using it.

Posted: Thu Dec 21, 2006 5:38 am
by Mightywayne
Holy. o.o

I guess I've got quite a lot to fix.

Thank you.. folks. :oops:

Edit: And Everah, apparently it's bad to NOT "define" things for the POST value stuff.

Posted: Thu Dec 21, 2006 6:20 am
by Ollie Saunders
What purpose does making $_POST vars constant serve? Defines aren't particularly fast so you have to have a reason to use them.

Posted: Thu Dec 21, 2006 9:44 am
by RobertGonzalez
Constants are usually defined into UPPERCASE_CONSTANT_NAMES. It is not gospel, but widely accepted as the norm in PHP Development circles.

For what you are doing you do not need constants. Just assign your $_POST array vars to independent variable and go from there.