Page 2 of 3

Posted: Fri Dec 22, 2006 12:33 pm
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?

Posted: Fri Dec 22, 2006 1:07 pm
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];
}
?>

Posted: Fri Dec 22, 2006 4:54 pm
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.

Posted: Fri Dec 22, 2006 6:05 pm
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

Posted: Fri Dec 22, 2006 6:07 pm
by Ollie Saunders
Fixed

Posted: Fri Dec 22, 2006 6:11 pm
by jyhm
Oh,
I thought maybe it was new PHP5 syntax or coding guru stuff!

HeHe :D

Posted: Fri Dec 22, 2006 6:14 pm
by Ollie Saunders
Hehe.. nahh. Everah can't type and I can't read :)

Posted: Sat Dec 23, 2006 8:04 am
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.

Posted: Sat Dec 23, 2006 10:37 am
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.

Posted: Tue Dec 26, 2006 8:00 am
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!

Posted: Tue Dec 26, 2006 10:37 am
by RobertGonzalez
New week, new gotchas to watch for. :wink:

Here's How I would do it

Posted: Thu Dec 28, 2006 1:21 pm
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 ;)

Posted: Thu Dec 28, 2006 2:37 pm
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]);
}

Posted: Thu Dec 28, 2006 5:27 pm
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.';
}
?>

Posted: Thu Dec 28, 2006 6:11 pm
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...