1 (edited by PhaxeNor 2007-11-04 23:36)

Topic: [Mod] Project List 1.0 (Removed for fixing)

##
##     Mod title:  Project List (Extra Option)
##
##     Mod version:  1.0
##     Works on PunBB:  1.2.*
##     Release date:  2007-11-04
##
##           Author:  PhaxeNor
##     Author email:  punbb@phaxe.com
##
##     Project Team:  PunLancer.com Team (Elbekko, Quaker, Dr Jeckyl,
##                    PhaxeNor, Mark, FSX)
##     Team Email:  devteam@punlancer.com
##
##      Description:  This mod allows you to add projects or other
##              things that you need to have a list over.
##
##   Affected files:  None
##                    
##       Affects DB:  Yes
##
##            Notes:  Only the owner, admin(s) or a group with
##              permissions can edit or delete the project.
##             
##              Report bugs or problems in this topic, on
##              punlancer.com or my website (phaxe.com)
##                
##
##       DISCLAIMER:  Please note that "mods" are not officially
##              supported by PunBB. 
##                    Installation of this modification is done
##              at your own risk. Backup your forum database and
##              any and all applicable files before proceeding.
##
##

[Download]

Re: [Mod] Project List 1.0 (Removed for fixing)

A bit of feedback for you:

In the admin plugin:

    while (list($id, $set) = @each($allow_read))
    {
        $db->query('UPDATE '.$db->prefix.'groups SET g_read_project='.$set.' WHERE g_id=\''.$id.'\'') or error('Unable to change permissions.', __FILE__, __LINE__, $db->error());
    }

$set is not intval'ed like everywhere else: SQL inject

For all the code like that, $id is a number as well but is treated like a string (and not escaped properly, which allows for SQL injects).

        $less=0;
        $db->query("DELETE FROM {$db->prefix}project WHERE id>{$less}")
                or error('Unable to delete a type', __FILE__, __LINE__, $db->error());

A. Out of curiosity, what's up with the use of {}?
B. Why not just do >0 instead of defining a variable to be a constant and using it in the next line?

<th scope="row"><?php echo $cur_group['g_title'] ?></th>

XSS, pun_htmlspecialchars needs to be called on the title

---

Now, project.php

if ($is_admmod)

I see that used a lot, but I never see it initialized anywhere. Where is it initialized?

if (isset($_POST['form_add']))
{    
    $result = $db->query('SELECT * FROM '.$db->prefix.'project') or error('Unable to fetch project list', __FILE__, __LINE__, $db->error());
    $cur_todo = $db->fetch_assoc($result);
    $id = $cur_todo['id'];
if (isset($_POST['form_del']))
{    
    $result = $db->query('SELECT * FROM '.$db->prefix.'project') or error('Unable to fetch project list', __FILE__, __LINE__, $db->error());
    $cur_todo = $db->fetch_assoc($result);
    $id = $cur_todo['id'];

As far as I can see, this code is entirely unnecessary (not the if/else if, the query).

if (isset($_POST['form_edit']))
{    
    $project_id = $_POST['id'];
    
    $result = $db->query('SELECT * FROM '.$db->prefix.'project WHERE id='.$project_id.'') or error('Unable to fetch project list', __FILE__, __LINE__, $db->error());
    $cur_project = $db->fetch_assoc($result);
    $id = $cur_project['id'];

SQL inject, $project_id is never properly sanitized

$result = $db->query('SELECT id, owner FROM '.$db->prefix.'project WHERE id='.$_GET['del'].'') or error('Unable to fetch project list', __FILE__, __LINE__, $db->error());

Another SQL inject, $_GET['del'] is being used directly. You sanitize it for $del a few lines later: that should be moved up and you should use $del.

The actual form processing stuff (add, edit, delete) never checks for permission. That's only done when displaying the HTML. It needs to be done in both places.

<input type="hidden" name="delete_project" value="<?php echo $_GET['del']?>">
<h2><?php echo $lang_project['edit'] ?> - <?php echo $cur_project['subject']?></h2>
<input type="hidden" name="id" value="<?php echo $_GET['edit']?>"/>
            if ($project_data['owner'] == $pun_user['id'] || ($pun_user['g_id'] == PUN_ADMIN || ($pun_group['g_project_edit'] == '1'))){
                echo '<p><a href="project.php?edit='.$_GET['adv'].'">'.$lang_project['edit'].'</a></p>';
            }
            if ($project_data['owner'] == $pun_user['id'] || ($pun_user['g_id'] == PUN_ADMIN || ($pun_group['g_project_delete'] == '1'))){
                echo '<p><a href="project.php?del='.$_GET['adv'].'">'.$lang_project['delete'].'</a></p>';
            }
<h2><span><?php echo $lang_project['current work'].' - '.$project_data['subject'] ?></span></h2>
<div class="block" style="width:15%;">
    <h2><?php echo $lang_project['workers'] ?></h2>
    <div class="box">
        <div class="inbox">
            <div>
            <?php        
            echo '<p>'.$project_data['workers'].'</p>';
            ?>
            </div>
        </div>
    </div>
</div>
<td class="tcl"><h3 class="toggler introduction"><a><?php echo $project_data['subject'] ?></a></h3></td>
<td><?php echo $project_data['workers'];?></td>
<p><?php echo "$newtext\n"; ?></p>
            <td class="tcl"><h3 class="toggler introduction"><a><?php echo $project_data['subject'] ?></a></h3></td>
            <td><?php echo $project_data['workers']; ?></td>
<p><strong><?php echo $lang_project['description'] ?>:</strong></p>
<p><?php echo "$project_info_short\n"; ?></p>

XSS

I don't think I caught everything, so don't take this as a comprehensive review wink
I also think you could benefit from splitting project.php into several files (create_project.php, edit_project.php, delete_project.php, etc).

Re: [Mod] Project List 1.0 (Removed for fixing)

wow.. thanks Smartys big_smile

I'm not a good coder, but feedback helps smile

Will work on the things that you have noted down.

Btw could you also help me? (IRC)