Gaaah! Error codes and everything!

PHP programming forum. Ask questions or help people concerning PHP code. Don't understand a function? Need help implementing a class? Don't understand a class? Here is where to ask. Remember to do your homework!

Moderator: General Moderators

Mightywayne
Forum Contributor
Posts: 237
Joined: Sat Dec 09, 2006 6:46 am

Gaaah! Error codes and everything!

Post 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)
User avatar
feyd
Neighborhood Spidermoddy
Posts: 31559
Joined: Mon Mar 29, 2004 3:24 pm
Location: Bothell, Washington, USA

Post by feyd »

Are you asking us to read through all your code and figure it out for you?
User avatar
iknownothing
Forum Contributor
Posts: 337
Joined: Sun Dec 17, 2006 11:53 pm
Location: Sunshine Coast, Australia

Post 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.");
      }
Mightywayne
Forum Contributor
Posts: 237
Joined: Sat Dec 09, 2006 6:46 am

Post 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. :(
Last edited by Mightywayne on Mon Dec 18, 2006 9:26 pm, edited 1 time in total.
chakhar86
Forum Commoner
Posts: 45
Joined: Mon Jun 05, 2006 1:36 am
Contact:

Re: Gaaah! Error codes and everything!

Post by chakhar86 »

Mightywayne wrote:$usernamecheck = @mysql_fetch_array($query);
Hey, what is meaning of @ here, cannot find in PHP manual
Mightywayne
Forum Contributor
Posts: 237
Joined: Sat Dec 09, 2006 6:46 am

Post 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.
User avatar
John Cartwright
Site Admin
Posts: 11470
Joined: Tue Dec 23, 2003 2:10 am
Location: Toronto
Contact:

Post 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.
User avatar
feyd
Neighborhood Spidermoddy
Posts: 31559
Joined: Mon Mar 29, 2004 3:24 pm
Location: Bothell, Washington, USA

Post by feyd »

.. and we often say "booooo" to it's usage. :) Proper error handling is better.
User avatar
daedalus__
DevNet Resident
Posts: 1925
Joined: Thu Feb 09, 2006 4:52 pm

Post by daedalus__ »

The error suppression operator is also very slow.
afs
Forum Newbie
Posts: 5
Joined: Tue Nov 07, 2006 7:21 pm

use mysql

Post 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;
}
User avatar
RobertGonzalez
Site Administrator
Posts: 14293
Joined: Tue Sep 09, 2003 6:04 pm
Location: Fremont, CA, USA

Post 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!';
}
?>
User avatar
Ollie Saunders
DevNet Master
Posts: 3179
Joined: Tue May 24, 2005 6:01 pm
Location: UK

Post 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.
Mightywayne
Forum Contributor
Posts: 237
Joined: Sat Dec 09, 2006 6:46 am

Post 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.
User avatar
Ollie Saunders
DevNet Master
Posts: 3179
Joined: Tue May 24, 2005 6:01 pm
Location: UK

Post 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.
User avatar
RobertGonzalez
Site Administrator
Posts: 14293
Joined: Tue Sep 09, 2003 6:04 pm
Location: Fremont, CA, USA

Post 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.
Post Reply