Validate badge selection before input is saved.

[ghost]

Fixes OpenRA/OpenRAWeb#396.

[ghost]

Added an error when somebody tries to manipulate the form to revoke keys of other users.

[ghost]

Updated again. Now using the the build_query function and type casting for integers.

[ghost]

Updated.

[pchote]

Can you instead do something like this to simplify the logic and reduce the query count? Note: I haven't tested this at all, so there may be syntax or logic errors.

if ($submitmarked)
{
    // Check form key.
    if (!check_form_key($form_key))
    {
        trigger_error($this->language->lang('FORM_INVALID') . '<br><br>' . $this->user->lang('RETURN_UCP', '<a href="' . $this->u_action . '">', '</a>'));
    }

    $delete_sql = empty($markedbadges) ? '' : ' AND ' . $this->db->sql_in_set('badge_id', $markedbadges, true); // true for NOT IN, false (default) for IN

    // Delete all user badges that are NOT IN the array of marked badges or all if none is selected.
    $sql = 'DELETE FROM ' . $user_badge_table . '
        WHERE user_id = ' . $this_user_id . "
        $delete_sql";
    $this->db->sql_query($sql);

    // Query valid badges
    $sql = "SELECT badge_id
        FROM $badge_table
        WHERE badge_default = TRUE";
    if (!($result = $this->db->sql_query($sql)))
    {
        trigger_error($this->user->lang('UCP_AVAIL_BADGES_QUERY_FAILED') . '<br /><br />' . $this->user->lang('RETURN_UCP', '<a href="' . $this->u_action . '">', '</a>'));
    }
    $default_badges = $this->db->sql_fetchrowset($result);

    $sql = "SELECT badge_id
        FROM $badge_avail_table
        WHERE user_id = $this_user_id";                     
    if (!($result = $this->db->sql_query($sql)))
    {
        trigger_error($this->user->lang('UCP_AVAIL_BADGES_QUERY_FAILED') . '<br /><br />' . $this->user->lang('RETURN_UCP', '<a href="' . $this->u_action . '">', '</a>'));
    }
    $custom_badges = $this->db->sql_fetchrowset($result);

    $sql_ary = array();
    foreach ($markedbadges as $markedbadge)
    {
        // Validate requested badges
        if (!in_array($markedbadge, $default_badges) && !in_array($markedbadge, $custom_badges))
            continue;

        $sql_ary[] = array(
            'user_id' => $this_user_id,
            'badge_id' => $markedbadge,
            'badge_order' => null // Order value will be validated after loop.
        );
    }

    if (!$db->sql_multi_insert($user_badge_table, $sql_ary))
    {
        trigger_error($this->user->lang('UCP_DUPLICATE_USER_BADGE_QUERY_FAILED') . '<br /><br />' . $this->user->lang('RETURN_UCP', '<a href="' . $this->u_action . '">', '</a>'));
    }

    if (!$this->core->validate_badge_order($this_user_id))
    {
        trigger_error($this->user->lang('UCP_BADGE_ORDER_QUERY_FAILED') . '<br /><br />' . $this->user->lang('RETURN_UCP', '<a href="' . $this->u_action . '">', '</a>'));
    }
    unset($markedbadges);
    meta_refresh(5, $this->u_action);
    trigger_error($this->user->lang('UCP_SELECTED_BADGES_SAVED') . '<br /><br />' . $this->user->lang('RETURN_UCP', '<a href="' . $this->u_action . '">', '</a>'));
}

[ghost]

Thank you for your help, I have updated the commit with the changes you suggested, however I had to change some things too avoid duplicates and wrong error messages.

I want to rework the ordering code and would push that in a separate commit.

[ghost]

Updated, hope this is now ready to get merged. Will squash the commits before.

should hopefully be able to apply the same pattern, assuming it works, to the other cases too

I would like to focus in this pr on the security issues and do further optimizations in separate PRs, like refactoring other parts of the code to reduce the query count or phase out duplicate queries into functions.

[pchote]

Looks like the query for the list can similarly simplify down. I may not have time to look at that tonight, but will try in the next couple of evenings if not.

[pchote]

The queries for building the selection table can similarly simplify down:

// Query badges for the table view
$sql_ary = array(
    'SELECT'    => 'b.badge_id, b.badge_label, b.badge_icon_24, bt.badge_type_name, bu.badge_id IS NOT NULL as badge_selected',

    'FROM'      => array(
        $badge_table        => 'b',
        $badge_type_table   => 'bt'
    ),

    'LEFT_JOIN' => array(
        array(
            'FROM'  => array($user_badge_table => 'bu'),
            'ON'    => 'b.badge_id = bu.badge_id AND bu.user_id = ' . $this_user_id
        ),
        array(
            'FROM'  => array($badge_avail_table => 'ba'),
            'ON'    => 'b.badge_id = ba.badge_id AND ba.user_id = ' . $this_user_id
        ),
    ),

    // Badges are available if either:
    //  - a default badge (b.badge_default)
    //  - have been awarded to the user (ba.user_id)
    'WHERE'     => 'b.badge_type_id = bt.badge_type_id AND (b.badge_default = TRUE OR ba.user_id = ' . $this_user_id . ')',
    'ORDER_BY'  => 'bt.badge_type_name'
);

$sql = $this->db->sql_build_query('SELECT', $sql_ary);

if (!($result = $this->db->sql_query($sql)))
{
    trigger_error($this->user->lang('UCP_AVAIL_BADGES_QUERY_FAILED') . '<br /><br />' . $this->user->lang('RETURN_UCP', '<a href="' . $this->u_action . '">', '</a>'));
}

// Prepare an array to group badges by types.
$typelist = array();
while ($row = $this->db->sql_fetchrow($result))
{
    $s_badgerow = array(
        'S_SELECTED' => $row['badge_selected'],
        'BADGE_ICON_URL' => $row['badge_icon_24'],
        'BADGE_LABEL' => $row['badge_label'],
        'ID' => $row['badge_id'],
    );
    // Check if the type name is in the typelist, if not, add it to the list and to the blockvars for the current row.
    if (!in_array($row['badge_type_name'], $typelist))
    {
        $typelist[] = $row['badge_type_name'];
        $s_badgerow['BADGE_TYPE'] = $row['badge_type_name'];
    }
    $this->template->assign_block_vars('s_badgerow', $s_badgerow);
}

$this->db->sql_freeresult($result);

// Assign template vars for mode
$this->template->assign_vars(array(
    'S_UCP_MODE_SELECT_BADGES' => true,
    'S_AVAIL_BADGES' => sizeof($typelist) > 0
));

and I think the in_2d_array helper definition can go away after this too.

[ghost]

This looks really good and will help in a lot of other places too. It's worth doing this now and I'll update the pr quickly.

[ghost]

Updated.

Aside from applying your suggestions, I removed all query-fail-errors, type-casted all request vars and fixed an issue that the image mouseover displayed the image URL.

I'm not sure if there are some spots where we want to have errors, besides from error messages that reflect functionalities.

[ghost]

Updated, entirely removed the $action part where it is not required.

Edit: Small fixup,

$moveup = (bool)$action == 'move_up' ? true : false;
$movedown = (bool)$action == 'move_down' ? true : false;

always evaluated to true.

Edit#2: Another fixup,

confirm_box(false, $this->user->lang['CONFIRM_OPERATION'], build_hidden_fields(array(
    'start' => $start,
    'markedkeys' => $markedkeys,
    'i' => $id,
    'mode' => $mode,
    'rev_marked' => $revokemark,
    'rev_all' => $revokeall
))

always returned true for rev_marked and rev_all regardless of the values $revokemark, $revokeall.

[AMHOL]

$moveup = (bool)$action == 'move_up' ? true : false;
$movedown = (bool)$action == 'move_down' ? true : false;

No need for casting here, value will always be a boolean, been a while since I did any PHP but pretty sure the following would suffice:

$moveup = $action == 'move_up';
$movedown = $action == 'move_down';

Also can use the following markdown to get syntax highlighting when posting code in Github comments:

```php // Code here ```

[ghost]

Updated, also removed unused css definitions. Also removed

$sql_in = array();
foreach ($markedkeys as $marked)
{
    $sql_in[] = $marked;
}

because $markedkeys is already casted to an array of integers. The original code includes the code above, but IMO it is safe to skip this.

All changes are tested and when I removed casting, I ensured that the auto-casting works by dumping the variables.

@AMHOL You are right, these don't need to be casted. Even worse, the code quoted above always evaluates as true, regardless of the result of the check.

[ghost]

Updated again, am now happy with the changes.

[ghost]

Updated and tested, fix-up requests without reply addressed. Since we generally want to make clear that something is safe, I included the code below (based on phpBB) again which I removed earlier.

foreach ($markedkeys as $markedkey)
{
    $sql_in[] = (int)$markedkey;
}
$marked_keys_sql = ' AND ' . $this->db->sql_in_set('item_id', $sql_in);
unset($sql_in);

Edit: Had to fix a style error.

[ghost]

Updated.

[ghost]

Have tested what is intended to be done in the module again and couldn't find any issues. I tried to manipulate the user input in the html or url but didn't notice any unexpected behavior. Also tested it against the current bleed and playtest in cnc and only noticed some other things that do not seem to be related to this pr.

• While the tooltip badge data updates instantly, the profile shown when in the main menu does not update.
• When only changing the URL for Profile to my local forum, it does not display badge icons