Fix SQL injection vulnerability in type array key parameter

[DawoudIO]

What Changed

Use InputUtils::filterInt() to sanitize the array key from the type POST parameter before using it in SQL queries. This prevents SQL injection attacks where malicious payloads could be injected via crafted array keys like 'type[1 AND SLEEP(5)--]=text'.

Type

• ✨ Feature
• 🐛 Bug fix
• ♻️ Refactor
• 🏗️ Build/Infrastructure
• 🔒 Security

Testing

Screenshots

Security Check

• Introduces new input validation
• Modifies authentication/authorization
• Affects data privacy/GDPR

Code Quality

• Database: Propel ORM only, no raw SQL
• No deprecated attributes (align, valign, nowrap, border, cellpadding, cellspacing, bgcolor)
• Bootstrap CSS classes used
• All CSS bundled via webpack

Pre-Merge

• Tested locally
• No new warnings
• Build passes
• Backward compatible (or migration documented)

[Copilot]

Pull request overview

This PR addresses a SQL injection vulnerability in UserEditor.php by sanitizing the array key extracted from the $_POST['type'] parameter. The fix adds InputUtils::filterInt() to prevent malicious payloads like type[1 AND SLEEP(5)--]=text from being injected into SQL queries. However, the fix is incomplete as it doesn't address the underlying issue: the code uses raw SQL with RunQuery() instead of Propel ORM, which violates ChurchCRM coding standards.

Key Changes:

• Added sanitization of array key using InputUtils::filterInt(key($type)) before using it in SQL queries
• Added inline comment explaining the security purpose of the sanitization

Important Notes:

• The same vulnerability exists in at least 3 other files: SettingsIndividual.php, SettingsUser.php, and SystemSettings.php
• The entire settings save block should be refactored to use Propel ORM (UserConfigQuery) instead of raw SQL

Comments suppressed due to low confidence (1)

src/UserEditor.php:348

• While the sanitization of the array key with InputUtils::filterInt() is correct, this entire code block (lines 265-348) violates ChurchCRM coding standards by using raw SQL with RunQuery() instead of Propel ORM.

The raw SQL queries at lines 296-297, 306-307, and 340-342 should be replaced with Propel Query classes. For example:

// Instead of raw SQL:
$sSQL = 'SELECT * FROM userconfig_ucfg WHERE ucfg_id=$id AND ucfg_per_id=$iPersonID';
$iNumRows = mysqli_num_rows(RunQuery($sSQL));

// Use Propel:
use ChurchCRM\model\ChurchCRM\UserConfigQuery;
$userConfig = UserConfigQuery::create()
    ->filterById((int)$id)
    ->filterByPeronId((int)$iPersonID)
    ->findOne();
$bRowExists = ($userConfig !== null);

This is a critical security and maintainability issue that should be addressed in this security-focused PR.

if (isset($_POST['save']) && ($iPersonID > 0)) {
    $new_value = $_POST['new_value'];
    $new_permission = $_POST['new_permission'];
    $type = $_POST['type'];
    ksort($type);
    reset($type);
    while ($current_type = current($type)) {
        // Sanitize the array key to prevent SQL injection
        $id = InputUtils::filterInt(key($type));
        // Filter Input
        if ($current_type === 'text' || $current_type === 'textarea') {
            $value = InputUtils::legacyFilterInput($new_value[$id]);
        } elseif ($current_type === 'number') {
            $value = InputUtils::legacyFilterInput($new_value[$id], 'float');
        } elseif ($current_type === 'date') {
            $value = InputUtils::legacyFilterInput($new_value[$id], 'date');
        } elseif ($current_type === 'boolean') {
            if ($new_value[$id] != '1') {
                $value = '';
            } else {
                $value = '1';
            }
        }

        if ($new_permission[$id] != 'TRUE') {
            $permission = 'FALSE';
        } else {
            $permission = 'TRUE';
        }

        // We can't update unless values already exist.
        $sSQL = 'SELECT * FROM userconfig_ucfg '
            . "WHERE ucfg_id=$id AND ucfg_per_id=$iPersonID ";
        $bRowExists = true;
        $iNumRows = mysqli_num_rows(RunQuery($sSQL));
        if ($iNumRows === 0) {
            $bRowExists = false;
        }

        if (!$bRowExists) { // If Row does not exist then insert default values.
            // Defaults will be replaced in the following Update
            $sSQL = 'SELECT * FROM userconfig_ucfg '
                . "WHERE ucfg_id=$id AND ucfg_per_id=0 ";
            $rsDefault = RunQuery($sSQL);
            $aDefaultRow = mysqli_fetch_row($rsDefault);
            if ($aDefaultRow) {
                list(
                    $ucfg_per_id,
                    $ucfg_id,
                    $ucfg_name,
                    $ucfg_value,
                    $ucfg_type,
                    $ucfg_tooltip,
                    $ucfg_permission,
                    $ucfg_cat
                ) = $aDefaultRow;

                $userConfig = new UserConfig();
                $userConfig
                    ->setPeronId($iPersonID)
                    ->setId($id)
                    ->setName($ucfg_name)
                    ->setValue($ucfg_value)
                    ->setType($ucfg_type)
                    ->setTooltip($ucfg_tooltip)
                    ->setPermission($ucfg_permission)
                    ->setCat($ucfg_cat);
                $userConfig->save();
            } else {
                echo '<br> Error on line ' . __LINE__ . ' of file ' . __FILE__;
                exit;
            }
        }

        // Save new setting
        $sSQL = 'UPDATE userconfig_ucfg '
            . "SET ucfg_value='$value', ucfg_permission='$permission' "
            . "WHERE ucfg_id='$id' AND ucfg_per_id=$iPersonID ";
        $rsUpdate = RunQuery($sSQL);
        next($type);
    }

    RedirectUtils::redirect('UserList.php');
}

[Copilot]

While InputUtils::filterInt() correctly sanitizes the array key to prevent SQL injection, this fix is incomplete. The sanitized $id variable is subsequently used in raw SQL queries (lines 296-297, 306-307, 340-342) via RunQuery(), which violates ChurchCRM's coding standard requiring Propel ORM for all database operations.

The entire settings save block should be refactored to use UserConfigQuery instead of raw SQL. This would eliminate SQL injection risks entirely through parameterized queries and improve code maintainability.