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

Post by Mightywayne »

Yeah, I know, I don't really like caps though. They annoy me. I'm one to instantly ignore spammers regardless.

And... defines can't be changed by hackers. I don't know how they hack, but post values are supposed to be defined so they can't be manipulated. That's what I learned. =/ I'm ready to experience slight slowness for security, you see.

Edit: And, if defines are really that bad... then what would be appropriate to use them for?
User avatar
RobertGonzalez
Site Administrator
Posts: 14293
Joined: Tue Sep 09, 2003 6:04 pm
Location: Fremont, CA, USA

Post by RobertGonzalez »

Mightywayne wrote:And... defines can't be changed by hackers. I don't know how they hack, but post values are supposed to be defined so they can't be manipulated. That's what I learned.
Where did you learn this? Constants cannot be changed after they are defined. That is true. But why in the world would you need to define your POST vars? If a hacker is going to hack you, they are more than likely going to do it at the entry point (where the post data comes in) at which point you are hosed because you just made malicious data unchangeable.
Mightywayne wrote:Edit: And, if defines are really that bad... then what would be appropriate to use them for?
It depends on your needs. I use them for URL params sometimes, like this...

Code: Select all

<?php
define('PAGENAME', 'p');
?>
Then in my code, when I need to pass a get var or check for one, I do this...

Code: Select all

<?php
$link_to = 'fatpenguinpicures';
echo '<a href="index.php>' . PAGENAME . '=' . $link_to . '" title="See penguins!">Show me the penguins!</a>';
?>

Code: Select all

<?php
if (isset($_GET[PAGENAME]))
{
    $page = $_GET[PAGENAME];
}
?>
User avatar
Ollie Saunders
DevNet Master
Posts: 3179
Joined: Tue May 24, 2005 6:01 pm
Location: UK

Post by Ollie Saunders »

Everah's example demonstrates two advantages to constants. Whilst...

Code: Select all

echo '<a href="index.php?p=' . $link_to . '"
does achieve the same result as

Code: Select all

echo '<a href="index.php?' . PAGENAME . '=' . $link_to . '"
the use of the constant is preferred as the meaning of p is made clear and if you mistype a constant you get an error telling you about it instead of mistyping the parameter itself where you would get a silent bug.
Last edited by Ollie Saunders on Fri Dec 22, 2006 6:07 pm, edited 1 time in total.
User avatar
jyhm
Forum Contributor
Posts: 228
Joined: Tue Dec 19, 2006 10:08 pm
Location: Connecticut, USA
Contact:

Post by jyhm »

Hmm just a curiosity,

You guys don't use "?" to identify vars in th URL :?:

i.e.

Code: Select all

$wutevervals=$_POST ["wutevah"];
$urlString="http://www.yoursite.com/index.php";
$addQuest="?"; // In its own var just for demonstration purposes
$daVars="variable=";

$setURLvars= $urlString . $addQuest . $daVars . $wutevervals;

//$setURLvars Would equal "http://www.yoursite.com/index.php?variable=10" if I set $wutevah to 10
User avatar
Ollie Saunders
DevNet Master
Posts: 3179
Joined: Tue May 24, 2005 6:01 pm
Location: UK

Post by Ollie Saunders »

Fixed
User avatar
jyhm
Forum Contributor
Posts: 228
Joined: Tue Dec 19, 2006 10:08 pm
Location: Connecticut, USA
Contact:

Post by jyhm »

Oh,
I thought maybe it was new PHP5 syntax or coding guru stuff!

HeHe :D
User avatar
Ollie Saunders
DevNet Master
Posts: 3179
Joined: Tue May 24, 2005 6:01 pm
Location: UK

Post by Ollie Saunders »

Hehe.. nahh. Everah can't type and I can't read :)
User avatar
RobertGonzalez
Site Administrator
Posts: 14293
Joined: Tue Sep 09, 2003 6:04 pm
Location: Fremont, CA, USA

Post by RobertGonzalez »

ole wrote:Hehe.. nahh. Everah can't type and I can't read :)
Given the nature of the code monkeying I have been doing, I would say that little omission was a small oversight :D . Thanks ole, at this point in the month, I can honestly agree with you.
Small happening at work yesterday wrote:me: Why does my error trigger keep throwing this friggin error?!?!?!?!?
coworker: Maybe if you added a second equal sign when checking the equality?
me: %$$@#&T%T*@%$*&(&^%@@))*@()*&$^^@#^*(@)$()_*^*(@^)(_@_((*@$. I'm going home.
User avatar
Ollie Saunders
DevNet Master
Posts: 3179
Joined: Tue May 24, 2005 6:01 pm
Location: UK

Post by Ollie Saunders »

Everah wrote:
Small happening at work yesterday wrote:me: Why does my error trigger keep throwing this friggin error?!?!?!?!?
coworker: Maybe if you added a second equal sign when checking the equality?
me: %$$@#&T%T*@%$*&(&^%@@))*@()*&$^^@#^*(@)$()_*^*(@^)(_@_((*@$. I'm going home.
lol. We all have our gotchas; happens to the best of us.
Mightywayne
Forum Contributor
Posts: 237
Joined: Sat Dec 09, 2006 6:46 am

Post by Mightywayne »

LOL thanks guys, and everah, my friend said that... thing is, he's hardly wrong with this stuff, but I'm trusting you over him because of how uh... 'pro' you are. xP So, thanks for all the help guys, and hope you all had a nice Christmas.

And watch those equal signs!
User avatar
RobertGonzalez
Site Administrator
Posts: 14293
Joined: Tue Sep 09, 2003 6:04 pm
Location: Fremont, CA, USA

Post by RobertGonzalez »

New week, new gotchas to watch for. :wink:
timclaason
Forum Commoner
Posts: 77
Joined: Tue Dec 16, 2003 9:06 am
Location: WI

Here's How I would do it

Post by timclaason »

Here's how I would do it procedurally (in reality, I'd do it with objects/classes AND I'd make the methods a bit different, but I was just kind of converting your code, as opposed to writing it from scratch):

I'd start out with the functions:

Code: Select all

// Function List
function findSymbol($address, $symbol) {
	if($symbol == "AT")
		$returnVal = strpos($address, "@");
	elseif($symbol == "DOT")
		$returnVal = strpos($address, ".");
	else
		$returnVal = 0;
	return $returnVal;

}

function checkForBlanks($field, $input) {
	if(!$input) {
		print("You are missing input for " . $field . ".<BR><a href='....'>Click Here</A> to fix it");
		$errorCount = 1;
		return $errorCount;
	}
}

function checkAllInput($P_password, $P_cpassword, $atSymbol, $dotSymbol) {
	if($P_password != $P_cpassword) {
		print("Passwords Do Not Match");
		$errorCount = 1;
		return $errorCount;
	}
	elseif(!$atSymbol || !$dotSymbol) {
		print("Invalid Email Address");
		$errorCount = 1;
		return $errorCount;
	}
}

function preventDuplicates($field, $val) {
	if($field == "USERNAME") {
		$query = "SELECT username FROM...";
		$SQL = mysql_query($query, $dbLink);		
	}

	elseif($field == "EMAIL") {
		$query = "SELECT email FROM...";
		$SQL = mysql_query($query, $dbLink);
	}
	if(@mysql_num_rows($SQL) { 
		print("That " . $field . " already exists");
		$errorCount = 1;
		return $errorCount;
	}

}
Then I'd make the code simpler:

Code: Select all

//Start Code

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

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

	$un		= $_POST['username'];
	$pw		= $_POST['password'];
	$cpw		= $_POST['cpassword'];
	$email		= $_POST['email'];




//Checking for typo's and stuff now.

$findat = findSymbol($email, AT);
$finddot = findSymbol($email, DOT);

if(	!checkForBlanks(USERNAME, $un) &&
	!checkForBlanks(PASSWORD, $pw) &&
	!checkForBlanks(CONFIRM_PASSWORD, $cpw) &&
	!checkForBlanks(EMAIL, $email) &&
	!checkAllInput($pw, $cpw, $findat, $finddot) &&
	!checkForUniqueEmail($email) &&
	!preventDuplicates(USERNAME, $un) &&
	!preventDuplicates(EMAIL, $email)
   ) {

	mysql_select_db("burnttoa_monbre", $con);


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

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

	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.";
} //End if statement
PHP Gurus: Feel free to crucify me with "That's bad coding..." comments ;)
User avatar
Ollie Saunders
DevNet Master
Posts: 3179
Joined: Tue May 24, 2005 6:01 pm
Location: UK

Post by Ollie Saunders »

PHP Gurus: Feel free to crucify me with "That's bad coding..." comments
Am I guru enough? Because, you know, I just can't resist such invitations :P
I'm just going to critique findSymbol() only I think.
  • No docblock comment. What is findSymbol() supposed to do?
  • findSymbol() seems to be a very simple wrapper for strpos() to restrict what needle you can use, is there really any point to its existence at all?
  • Personally I don't like any if, else, elseif, for, while etc. without braces ( {} ); unbraced control structures are more prone to bugs.
  • Usage of double quoted strings when not using double quote functionality. They are slightly slower to process and generally I like to reserve them for when I'm using escapes or inline variable substitution so when I do such things it is more obvious.
  • Temp variable $returnVal makes code complex. return, continue and break are your friends and often really go a long way to improving readability.
  • Future modifications to findSymbol to search for other symbols would require you to add more conditions. Array?
Given all previous comments here's my rewrite of findSymbol:

Code: Select all

/**
 * Wrapper to strpos() designed to restrict to certain hard-coded needles.
 *
 * @param string $haystack
 * @param string $needleName name of hard-coded needle to use
 * @param int $limit as in strpos()
 * @return int
 */
function findSymbol($hayStack, $needleName, $limit = null)
{
    static $needles = array('AT' => '@', 'DOT' => '.');
    if (!isset($needles[$needleName])) {
        return false;
    }
    if (is_int($limit)) {
        return strpos($hayStack, $needles[$needleName], $limit);
    }
    return strpos($hayStack, $needles[$needleName]);
}
User avatar
RobertGonzalez
Site Administrator
Posts: 14293
Joined: Tue Sep 09, 2003 6:04 pm
Location: Fremont, CA, USA

Post by RobertGonzalez »

Alright, here's my take on it...

Code: Select all

<?php
// Connect to the database server
if (!$con = mysql_connect('localhost','****','****'))
{
    die('Could not connect: ' . mysql_error());
}

// Select our database
if (!mysql_select_db('mydatabase', $con))
{
    die('Could not select the database: ' . mysql_error());
}

// Default values, to prevent uninitialized variables
$username = '';
$password = '';
$cpassword = '';
$email = '';

if (!empty($_POST))
{
    $username = $_POST['username'];
    $password = $_POST['password'];
    $cpassword = $_POST['cpassword'];
    $email = $_POST['email'];
    
    if (empty($username) || empty($password) || empty($cpassword) || empty($email)) 
    {
        die('You must fill out all fields! Go back and fix it.');
    }

    // This is not the most effective way to check email addresses
    // But this is moreso to show you an easier way to do what you were doing
    if (!strpos($email, '@') || !strpos('.', $email))
    {
        // Yes, there are better ways to handle bad 
        // information, but I'll let you figure those ways out 
        die('There is an error in your email address.');
    }
    
    // Check password/confirm match
    if ($password !=  $cpassword)
    {
        die('Your password confirmation did not match your password.');
    }
    
    // Check username and emails in the table
    $sql = "SELECT `ID` 
            FROM `user` 
            WHERE `name` = '$username' 
            OR `email` = '$email'";
    
    if (!$result = mysql_query($sql))
    {
        die('There was an error in the query: ' . mysql_error());
    }
    
    if (mysql_num_rows($result) > 0)
    {
        // There is a record somewhere with that username or email
        die('The email or username you entered is not available.');
    }
    
    // We need to enter the user if they get this far
    $sql = "INSERT INTO user 
                (`ID`, `name`, `pword`, `email`, `money`, `ranch`, `barn`, `happy`, `tlevel`, `verified`)
            VALUES 
                ('', '$username', '$password', '$email', 3000, 1, 1, 70, 1, 'no')";
    
    if (!mysql_query($sql))
    {
        die('Could not insert the user: ' . mysql_error());
    }

    $sql = "INSERT INTO `items` (`userbagID`) VALUES ('')";
    if (!mysql_query($sql))
    {
        die('Could not insert the bag: ' . mysql_error());
    }

    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.';
}
?>
timvw
DevNet Master
Posts: 4897
Joined: Mon Jan 19, 2004 11:11 pm
Location: Leuven, Belgium

Post by timvw »

Everah wrote:

Code: Select all

if (!empty($_POST))
{
    $username = $_POST['username'];
    $password = $_POST['password'];
    $cpassword = $_POST['cpassword'];
    $email = $_POST['email'];
    
    if (empty($username) || empty($password) || empty($cpassword) || empty($email)) 
    {
        die('You must fill out all fields! Go back and fix it.');
    }
You blindfully assume they will exist in $_POST...
Everah wrote:

Code: Select all

$sql = "SELECT `ID` 
            FROM `user` 
            WHERE `name` = '$username' 
            OR `email` = '$email'";

    $sql = "INSERT INTO user 
                (`ID`, `name`, `pword`, `email`, `money`, `ranch`, `barn`, `happy`, `tlevel`, `verified`)
            VALUES 
                ('', '$username', '$password', '$email', 3000, 1, 1, 70, 1, 'no')";
You did not prepare your variables for use in a MySQL query.

Why do you assign a link to the db resource $con if you're not going to use it for your following mysql_* calls?
Everah wrote:

Code: Select all

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.';
?>
You haven't prepared the data for use in a html context...
Post Reply