Is this Secure?

[Joshua]

Trying to learn how to secure all McCodes files in my game.

Have to start somewhere so I started with a Small file. Could anyone tell me if this is secure, if I overkilled it or Underkilled it, and what I might do to fix any erros?

Again, just curious if their is anymore I can do, as I can look at any errors, upgrades you can offer and install them in other files. Thanks.

 

<?php
include "globals.php";
$_GET['ID'] = abs(@intval($_GET['ID']));
$_POST['qty']= abs(@intval($_POST['qty']));
if(!$_GET['ID'] || !$_POST['qty'])
{
print "Invalid use of file";
}
else if($_POST['qty'] <= 0)
{
print "You have been added to the delete list for trying to cheat the game.";
}
else
{
$q=$db->query(sprintf("SELECT * FROM items WHERE itmid={$_GET['ID']}");
if(mysql_num_rows($q) == 0)
{
print "Invalid item ID";
}
else
{
$itemd=$db->fetch_row($q);
if($ir['money'] < $itemd['itmbuyprice']*$_POST['qty'])
{
print "You don't have enough money to buy this item!";
$h->endpage();
exit;
}
if($itemd['itmbuyable'] == 0)
{
print "This item can't be bought!";
$h->endpage();
exit;
}
$price=($itemd['itmbuyprice']*$_POST['qty']);
item_add($userid, $_GET['ID'], $_POST['qty']);
$db->query(sprintf("UPDATE users SET money=money-$price WHERE userid=$userid");
$db->query(sprintf("INSERT INTO itembuylogs VALUES ('', $userid, {$_GET['ID']}, $price, {$_POST['qty']}, unix_timestamp(), '{$ir['username']} bought {$_POST['qty']} {$itemd['itmname']}(s) for {$price}')");
print "You bought {$_POST['qty']} {$itemd['itmname']}(s) for \$$price";
}
}
$h->endpage();
?>

[a_bertrand]

For the cleanup of the parameters yes it is. However I have a few comment to give:

Once you cleanup your data, don't put them back in the GET and POST, use variable name (so you are sure you first cleaned up the value and then use the clean value):

$itmid = abs($_GET['ID']+0);
$qty= $_POST['qty']+0;

 

Now the second one I didn't use ABS, why? Because you can now check if the quantity is <= than 0.

for your queries like: $db->query(sprintf("UPDATE users SET money=money-$price WHERE userid=$userid");

The use of sprintf is totally useless here, as sprintf would be useful if you have the %1, %2 place holders and then add the values at the end. In your case you can simply write:

$db->query("UPDATE users SET money=money-$price WHERE userid=$userid");

[Joshua]

Thanks for the input Bertrand. Im working on every single file in my site, that was..most helpful :)

I've read tons of stuff here on the forums, my problem is I can't always see exactly what they are saying.

I don't learn as fast as most, takes a few times and perhaps some annoyance. BUT once i DO learn, I tend to learn better than others :P

Once I get this down pat I'm going to work on Securing every mod here on the forums and re-releasing it ><

[a_bertrand]

please don't re-submit mods which are not OPEN SOURCE, as you may get in troubles with their owners ;-)

Otherwise you are welcome. If you have more questions you know where to find me.

[Joshua]

No, I was thinking about fetching all the Free Mods posted on Free Scripts in the McCodes section.

One at a time, Securing the be-jesus out of them :)