20 Examples of Bad phpBB MODs

All of these are real examples from real MODs which you can download at any place other than the Official phpBB MOD archive. All of these flaws are present in the most recent major version listed of the MOD.

Improper Template Usage
if($mode == "main" || !$mode)
		{
	echo "<table width='100%' border='0' class='forumline' cellspacing='2' align='center' valign='middle'>";
	echo "	<tr>";
	echo "		<th class='thHead' colspan='2'>";
	echo "			". $lang['admin_cat_1'];
	echo "		</th>";
	echo "	</tr>";
	echo "</table>";
	echo "<br><br>";

This MOD is not using templates at all! Also, note the $mode == "main" part. That is also wrong. Double quotes should only be used when the string contains a variable, else you should use single quotes for correctness and speed.

Improper SQL Usage
	$q = "INSERT INTO ". INA_CATEGORY ."
		  VALUES ('', '". str_replace("\'", "''", $cat) ."', '". str_replace("\'", "''", $des) ."', '". $img ."')";
	$r	= $db -> sql_query($q);

This query does not have the column names specified, which is a requirement of the phpBB Coding guidelines. If the table schema changes, this query may no longer work.

Design Flaw and Improper SQL Usage
    $sql = "SELECT player_id, score FROM " . iNA_SCORES . "
      WHERE game_name = '". $this->game_name ."'
        ORDER BY score " . $sort;
    if($result = $db->sql_query($sql))
    {
      $scores = $db->sql_fetchrowset($result);

      $total_scores = count($scores);
  
      if($total_scores > $game_info['highscore_limit'])
      {
        $remove_count = $total_scores - $game_info['highscore_limit'];
      
        for($i = 0; $i < $remove_count; $i++)
        {
          $sql = "DELETE FROM ". iNA_SCORES . "
             WHERE game_name = '" . $this->game_name . "'
             AND player_id = " . $scores[$i]['player_id'] . "
            AND score = " . $scores[$i]['score'];
          $remove_scores = $db->sql_fetchrowset($db->sql_query($sql));
        }
      }
    }

Notice how the second query which deletes extra game highscores, a DELETE statement, is within a loop. This is both improper SQL usage and a major design flaw. Imagine if this game has hundreds of high scores, then that's hundreds of queries. This is an example of why you need to spend time planning out all the details of your code before you start programming. The functionality of this MOD needs a change to store the score's placement in the table.

XSS Vulnerability, Missing Input Validation and Global variables
// [START] Fix for register_globals! Thanks alot Mav! ^_^
if(is_array($_GET))
{
extract($_GET, EXTR_PREFIX_SAME, "get");
}
if(is_array($_POST))
{
extract($_POST, EXTR_PREFIX_SAME, "post");
}
// [END] Fix for register_globals!

[snip...]

if($action == "change")
{
	$bgsound = '
			<SCRIPT TYPE="text/javascript">
			<!-- 
			var filename="'.$phpbb_root_path . $midi_path.'/'.$midi.'";
			if (navigator.appName == "Microsoft Internet Explorer")
			{ document.writeln (\'<BGSOUND SRC="\' + filename + \'" loop="infinite">\'); }
			else if (navigator.appName == "Netscape")
			{ document.writeln (\'<EMBED SRC="\' + filename + \'" AUTOSTART="TRUE" hidden="true" loop="true">\'); }
			// -->
			</SCRIPT>
			<NOSCRIPT>
			<BGSOUND SRC="'.$phpbb_root_path . $midi_path.'/'.$midi.'">
			</NOSCRIPT>
	';
}

This one's a triple whammy! Not only is this MOD using some code which emulates register_globals (Hint: the "Fix" for register_globals is to not use them!), not only is there absolutely no input validation, but you can also insert arbitrary Javascript code like so:

http://127.0.0.1/forums/qm_midi.php?action=change&midi=";alert('Dog+Cow+p0wns+j00!');//

And that would bring up a Javascript alert with the message. Of course, a real hacker would insert a cookie stealer or other nasty code and start spreading around the URL. All it takes is one click and he's got you.

Moral of the story: DON'T use register_globals, and ALWAYS validate user input!

Improper SQL Usage
$sqlconfig = "SELECT * FROM ".$table_prefix."nulavatar_settings WHERE config_name = 'avatar_height' ";
if ( !($resultconfig = $db->sql_query($sqlconfig)) )
{ message_die(GENERAL_MESSAGE, "<b>Fatal Error!</b><br />".mysql_error()); }
$config_row = mysql_fetch_array($resultconfig); 
$height = $config_row['config_value'];

$sqlconfig = "SELECT * FROM ".$table_prefix."nulavatar_settings WHERE config_name = 'avatar_width' ";
if ( !($resultconfig = $db->sql_query($sqlconfig)) )
{ message_die(GENERAL_MESSAGE, "<b>Fatal Error!</b><br />".mysql_error()); }
$config_row = mysql_fetch_array($resultconfig); 
$width = $config_row['config_value'];

// Get RPG System
$sqlconfig = "SELECT * FROM ".$table_prefix."nulavatar_settings WHERE config_name = 'rpgsystem' ";
if ( !($resultconfig = $db->sql_query($sqlconfig)) )
{ message_die(GENERAL_MESSAGE, "<b>Fatal Error!</b><br />".mysql_error()); }
$config_row = mysql_fetch_array($resultconfig); 
$rpgsystem = $config_row['config_value'];

// Get Image Type
$sqlconfig = "SELECT * FROM ".$table_prefix."nulavatar_settings WHERE config_name = 'imagetype' ";
if ( !($resultconfig = $db->sql_query($sqlconfig)) )
{ message_die(GENERAL_MESSAGE, "<b>Fatal Error!</b><br />".mysql_error()); }
$config_row = mysql_fetch_array($resultconfig); 
$imagetype = $config_row['config_value'];
$imagecreate = "imagecreatefrom".$imagetype;

// Get Display Mode
$sqlconfig = "SELECT * FROM ".$table_prefix."nulavatar_settings WHERE config_name = 'display' ";
if ( !($resultconfig = $db->sql_query($sqlconfig)) )
{ message_die(GENERAL_MESSAGE, "<b>Fatal Error!</b><br />".mysql_error()); }
$config_row = mysql_fetch_array($resultconfig); 
$show_shelves = $config_row['config_value'];

// Get items per shelf
$sqlconfig = "SELECT * FROM ".$table_prefix."nulavatar_settings WHERE config_name = 'eamountofitems' ";
if ( !($resultconfig = $db->sql_query($sqlconfig)) )
{ message_die(GENERAL_MESSAGE, "<b>Fatal Error!</b><br />".mysql_error()); }
$config_row = mysql_fetch_array($resultconfig); 
$items_per_shelf = $config_row['config_value'];

This is just wrong. Seriously. It's querying the same table 6 times to get the config data when all it takes is one query to return an array.

SQL Injection vulnerability, Missing Input Validation
			$insert_values .= $row['name']." = '".$HTTP_POST_VARS[$row['name']]."'";

			//
			// [START] Check for DONTSHOW layers :)
			//
			$IMsql = "SELECT * FROM ".$table_prefix."nulavatar_images WHERE image = '".$HTTP_POST_VARS[$row['name']]."' ";
			if ( !($IMresult = $db->sql_query($IMsql)) )
			{ message_die(GENERAL_MESSAGE, "<b>Fatal Error!</b><br><br>".mysql_error()); }

NEVER use super globals without doing input sanitization. In this code, a malicious user can easily rewrite the SQL query. Don't assume that because you're using POST method that your variables are safe and can't be tampered with; they're not, and that assumption is wrong. Same for cookies.

Inefficient Code - searching an array
#==== Start: Only authorized admins can view this
	$allowed = $allowed_admins = '';
	$allowed_admins = array(2, 874);
	for ($x = 0; $x < count($allowed_admins); $x++)
		{
		if ($userdata['user_id'] == $allowed_admins[$x])
			$allowed = TRUE;
		}
		
	if (!$allowed)
		message_die(GENERAL_ERROR, $lang['db_unauthed']);
#==== End: Only authorized admins can view this

This is not efficient. You need to read the PHP manual so you can familiarize yourself with the built-in functions. In this case, in_array() is applicable. Change to this:

#==== Start: Only authorized admins can view this
	$allowed_admins = array(2, 874);
	if (!in_array($userdata['user_id'], $allowed_admins))
	{
		message_die(GENERAL_ERROR, $lang['db_unauthed']);
	}
#==== End: Only authorized admins can view this

Inefficient code - loop counter

  for($i = 0; $i < count($error_log); $i++)

NEVER put a count() or sizeof() or any other function in a loop counter, because it will have to run on every single iteration. This is even in the phpBB Coding Guidelines! Make sure you read that document carefully.

Instead, do this: 
$count = count($error_log);
  for($i = 0; $i < $count; $i++)

Or, this method is even better:

  for($i = 0; $i < $count = count($error_log); $i++)

Improper SQL usage
$sql = "SELECT p.*, c.*
		FROM ". ALBUM_TABLE ." AS p, ". ALBUM_CAT_TABLE ." AS c
		WHERE p.pic_id = '$pic_id'
			AND c.cat_id = p.pic_cat_id";

Avoid selecting all columns by using *. This forces MySQL to scan the entire table, plus it will probably return columns you don't end up using. This slows down the query a bit and uses up memory. Instead, select just the columns you need.

Improper SQL Usage
	$sql = "select * from phpbb_shops where shopname='".addslashes($row['shop'])."' and shoptype!='special' and shoptype!='admin_only'";

First of all, a constant should be defined for the table name, second of all, mysql_real_escape_string() or the appropriate escaping function for your DBMS should be used in favor of addslashes(), and third, the correct symbol for "not equals" is <>. This third point is even mentioned in the phpBB Coding guidelines.

Improper SQL Usage - no DBAL
	for ($er = 0; $er < mysql_num_rows($result); $er++)
	{
		$row = mysql_fetch_array($result);

This code is not using the phpBB DBAL (DataBase Abstraction Layer). In fact, MODs which don't use the DBAL aren't even allowed anywhere on phpbb.com.

Querying on un-indexed columns
#
# Pull user items into an ARRAY to use later
#
$sql = "SELECT *
	FROM " . USER_ITEMS_TABLE . "
	WHERE user_id = {$userdata['user_id']}
		AND worn = 0
		AND no_give = 0";

This query will run OK with a small table, but on tables with more than 1,000 rows, you should either add a multi-column index, or even better, remove the AND clauses, then skip the 'worn' and 'no_give' items when building the array.

Quoting numbers
// Referral Plus
if ( ($userdata['session_logged_in']) && ($board_config['rp_enable'] =='1') && ($board_config['rp_invite'] =='1') )
{
	$template->assign_block_vars('switch_user_rp_center', array());
}
else
{
if ( ($board_config['rp_enable'] == '0') )
{
	$template->assign_block_vars('switch_user_rp_no_center', array());
	}
}
if ( ($userdata['session_logged_in']) && ($board_config['rp_enable']) && ($board_config['rp_header'] == '1') )
{
	$template->assign_block_vars('switch_user_rp_header', array());
}
else
{
if ( ($board_config['rp_header'] == '0') )
{
	$template->assign_block_vars('switch_user_rp_no_header', array());
	}
}
// Referral Plus

Don't quote numbers when you want to use the numeric value. When you quote them, that's changing the number into a string. PHP will compensate for this, but you shouldn't make it have to. The best method is to either use the constants true/false or define your own constants, as explained in the phpBB Coding Guidelines.

Database Design Error - normalization
	$sql = "INSERT INTO " . TRANSACTION_TABLE . " (trans_date, trans_from, trans_to, trans_amount) 
		VALUES (" . time() . ", '" . str_replace("'", "\'", $userdata['username']) . "', 'Deposited to bank', $deposit)"; 
	if( !$db->sql_query($sql) ) 
	{ 
		message_die(GENERAL_ERROR, "Could not insert bank transaction record.", '', __LINE__, __FILE__, $sql); 
	}

This table schema does not follow the principles of database normalization. The user_id should instead be stored, since it is the proper foreign key which should never change. On the other hand, the username should NOT be used as a reference since it can change any time. As an integer, the user_id will also take up less disk space and RAM.

Database Design Error - wrong column types
mysql> describe phpbb_transactions;
+-------------+--------------+------+-----+---------+----------------+
| Field       | Type         | Null | Key | Default | Extra          |
+-------------+--------------+------+-----+---------+----------------+
| id          | int(10)      | NO   | PRI | NULL    | auto_increment | 
| user_id     | int(10)      | NO   |     | 0       |                | 
| target_id   | int(10)      | NO   |     | 0       |                | 
| target_name | varchar(32)  | NO   |     |         |                | 
| type        | varchar(32)  | NO   |     |         |                | 
| action      | varchar(32)  | NO   |     |         |                | 
| value       | varchar(255) | NO   |     |         |                | 
| misc        | text         | NO   |     | NULL    |                | 
| ip          | varchar(16)  | NO   |     |         |                | 
| timestamp   | int(32)      | YES  |     | NULL    |                | 
+-------------+--------------+------+-----+---------+----------------+

id should be mediumint(8) unless you anticipate having more than 16.7 million records (unlikely!)
user_id and target_id should be mediumint(8)
target_name is superfluous
type and action should be tinyint(1) with the data stored as numeric constants.
ip should be varchar(8), with the data stored as hex values
timestamp should be int(11), not null, default 0

Read the MySQL manual and the phpBB Coding guidelines. There is no excuse for sloppy database design. Performance of your script will suffer without proper design!

Not using the redirect() function
// Sorry , only logged users ...
if ( !$userdata['session_logged_in'] )
{
	$redirect = "adr_character.$phpEx";
	$redirect .= ( isset($user_id) ) ? '&user_id=' . $user_id : '';
	header('Location: ' . append_sid("login.$phpEx?redirect=$redirect", true));
}

This code is incompatible with some web servers, hence the redirect() function which was added around phpBB version 2.0.3. Use it instead of header().

Superfluous Input Validation
if(isset($HTTP_POST_VARS['userid']))
{
	$user_id = intval(trim(htmlspecialchars($HTTP_POST_VARS['userid']))) + 0;
} 
else if(isset($HTTP_GET_VARS['userid']))
{
	$user_id = intval(trim(htmlspecialchars($HTTP_GET_VARS['userid']))) + 0;
}

The trim() and htmlspecialchars() functions are uncessary, so is the '+0' added at the end. A simple intval() will suffice.

Missing Input Validation
// read the post from PayPal system and add 'cmd'
$req = 'cmd=_notify-validate';

foreach ($HTTP_POST_VARS as $key => $value) {
	$value = urlencode(stripslashes($value));
	$req .= "&$key=$value";
}

[snip...]

// post back to PayPal system to validate
$header .= "POST /cgi-bin/webscr HTTP/1.0\r\n";
$header .= "Content-Type: application/x-www-form-urlencoded\r\n";
$header .= "Content-Length: " . strlen($req) . "\r\n\r\n";
$fp = fsockopen ('www.paypal.com', 80, $errno, $errstr, 30);
if (!$fp) {
// HTTP ERROR
} else {
	fputs ($fp, $header . $req);

This code will take any and all user input, without checking or sanitizing it, and send it to PayPal. This means that anyone can send megabytes' worth of data to PayPal's server using your hosting company's IP address. I should assume that PayPal frowns upon this, and may even block you. Instead, only allow input variables which you have defined, especially when they are being sent to a server which you do not control.

SQL Injection vulnerability, missing Input Validation
		$category_id = $HTTP_GET_VARS['cat'];
		$category = get_kb_cat($category_id);	

here is the function:

function get_kb_cat($id)
{
    global $db;
	
	$sql = "SELECT *  
      	FROM " . KB_CATEGORIES_TABLE . " 
		WHERE category_id = $id";
	 if ( !($result = $db->sql_query($sql)) )
	 {
		message_die(GENERAL_ERROR, "Could not obtain category data", '', __LINE__, __FILE__, $sql);
	 }
	 
	 $row = $db->sql_fetchrow($result);
	 
	 return $row;
}

Notice how there is absolutely no input validation at all! Without putting intval() around the super global, anyone can inject arbitrary SQL into the query.

Cumbersome Concatenation
		#
		# Add up new total & insert into database
		#
		$sql = "UPDATE " . USERS_TABLE . "
			SET " . $currency_db . " = " . $currency_db . " + $pool
			WHERE user_id='{$row2['user_id']}'";

Instead of chopping up the string like that, it should be written like this instead:
		$sql = 'UPDATE ' . USERS_TABLE . "
			SET $currency_db = $currency_db + $pool
			WHERE user_id = " .$row2['user_id'];

There are now single quotes where no variable interpolation is required, and double quotes where variables are mixed in. This too is covered in the phpBB Coding Guidelines.

Literals in place of named Constants
		if($item['item_type_use'] == '11')
		{
			// Grab details for Elemental infos
			$elemental = adr_get_element_infos($opponent_element);

[snip...]

 		elseif($item['item_type_use'] == '12')
		{
			// Create message
			$battle_message .= sprintf($lang['Adr_pvp_spell_defensive_success'], $current_name, adr_get_lang($item['item_name']), $current_name, $power).'<br />';

[snip...]

				if($item['item_type_use'] == '15')
				{
					// Create message
					$power = ($power > ($current_hp_max - $current_hp)) ? ($current_hp_max - $current_hp) : $power;
					
[snip...]

				elseif($item['item_type_use'] == '16')
				{
					// Create message

As quoted from the phpBB Coding Guidelines, "It's not ok to assign some special meaning to a number and then use it everywhere as a literal. This hurts readability AND maintainability." In this case, the numbers 11, 12, 15, and 16 (which, incidentally should not be quoted) should be defined as named constants and used in place of the literals. As an example:

		if($item['item_type_use'] == ITEM_TYPE_A) , elseif($item['item_type_use'] == ITEM_TYPE_B) , etc.

Which MODs were these?
In order, these MODs are:
1- Activity Plus v1.1.0 by aUsTiN
2- Activity Plus v1.1.0 by aUsTiN
3- Arcade v2.2.0 by dEfEndEr
4- Quest MOD v2.5.0 by Nuladion
5- Nulavatar v1.3.1 by Nuladion
6- BB DB Admin v1.0.0 by aUsTiN
7- Arcade v2.2.0 by dEfEndEr
8- Photo Album v2.x by Smartor, et al
9- Shop v2.6.0 by Zarath
10- Shop v2.6.0 by Zarath
11- Shop v3.1.0 by Zarath
12- Referral Plus v1.4.0 by MentalMaze
13- Bank v2.0.1 by Zarath
14- Bank v2.0.1 by Zarath
15- ADR v0.4.4 by Seteo-Bloke
16- Loewen Enterprise Donation MOD v1.0.1 by sandodo
17- Loewen Enterprise Donation MOD v1.0.1 by sandodo
18- Knowledge Base v0.7.6 beta by wGEric
19 - Lottery v2.1.0 by Zarath
20- ADR v0.4.4 by Seteo-Bloke

The mistakes featured in this document are the errors which new and/or inexperienced programmers often make. Most examples here aren't just inefficient code, but are wrong and in some cases, dangerous. For guides which offer tips on PHP and MySQL optimization, try doing a web search on the terms.

Many inexperienced programmers aren't aware of how their scripts can be misused. Others don't apply proper input validation which can lead to XSS or SQL injection attacks. Some even assume that "hidden" form variables or POST variables cannot be exploited. ALWAYS program assuming every user is a hacker out to get you! Never trust any input, and make sure all GET, POST, and COOKIE inputs are sanitized. Look at others' code to pick up good tips and programming habits. Search the web for 'php security' to find informative articles. Also search for SQL injection and XSS methods. Learn how it's done. As a programmer, you should have a knowledge of hacking and the methods and vectors used. Otherwise, how else are you going to defend yourself and write good scripts? Make sure to read the MySQL and PHP manual, as well as the phpBB Coding Guidelines document. Every single one of the examples shown above is in violation of the phpBB Coding Guidelines.