Skip to content

hardening: prepared statements, PHP 7.4 idioms, and security fixes #90

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
somethingwithproof wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

hardening: prepared statements, PHP 7.4 idioms, and security fixes #90

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fce0526
refactor(php74): use null coalescing (??) and ??= operators
somethingwithproof Apr 9, 2026
a1d3d9b
chore: add copilot instructions and CI workflow
somethingwithproof Apr 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions .github/copilot-instructions.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
# Cacti mikrotik Plugin AI Instructions

## Project Overview
This is a Cacti plugin. It integrates with the Cacti monitoring platform via the plugin hook architecture.

## Technology Stack
- PHP 7.4+ (targeting Cacti 1.2.x compatibility)
- MySQL/MariaDB via Cacti's DB abstraction layer
- PSR-12 coding standards

## Key Rules
- Use prepared statements (db_execute_prepared, db_fetch_row_prepared, etc.) for ALL queries with variables
- Use get_request_var() / get_filter_request_var() for ALL user input, never raw $_REQUEST/$_GET/$_POST
- Use html_escape() / htmlspecialchars() for ALL output of DB/user values in HTML context
- Use cacti_escapeshellarg() for ALL shell command arguments
- No PHP 8.0+ features (str_contains, match, union types, named args) - target PHP 7.4
- Use ?? and ??= operators (PHP 7.4) instead of isset() ternary patterns
- All unserialize() calls must use allowed_classes => false

## Testing
- Tests in tests/ directory
- Use Pest PHP or PHPUnit
- php -l lint check required before commit
225 changes: 225 additions & 0 deletions .github/workflows/plugin-ci-workflow.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,225 @@
# +-------------------------------------------------------------------------+
# | Copyright (C) 2004-2026 The Cacti Group |
# | |
# | This program is free software; you can redistribute it and/or |
# | modify it under the terms of the GNU General Public License |
# | as published by the Free Software Foundation; either version 2 |
# | of the License, or (at your option) any later version. |
# | |
# | This program is distributed in the hope that it will be useful, |
# | but WITHOUT ANY WARRANTY; without even the implied warranty of |
# | MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the |
# | GNU General Public License for more details. |
# +-------------------------------------------------------------------------+
# | Cacti: The Complete RRDtool-based Graphing Solution |
# +-------------------------------------------------------------------------+
# | This code is designed, written, and maintained by the Cacti Group. See |
# | about.php and/or the AUTHORS file for specific developer information. |
# +-------------------------------------------------------------------------+
# | http://www.cacti.net/ |
# +-------------------------------------------------------------------------+

name: Plugin Integration Tests

on:
push:
branches:
- main
- develop
pull_request:
branches:
- main
- develop

jobs:
integration-test:
runs-on: ${{ matrix.os }}

strategy:
fail-fast: false
matrix:
php: ['8.1', '8.2', '8.3', '8.4']
os: [ubuntu-latest]

services:
mariadb:
image: mariadb:10.6
env:
MYSQL_ROOT_PASSWORD: cactiroot
MYSQL_DATABASE: cacti
MYSQL_USER: cactiuser
MYSQL_PASSWORD: cactiuser
ports:
- 3306:3306
options: >-
--health-cmd="mysqladmin ping"
--health-interval=10s
--health-timeout=5s
--health-retries=3

name: PHP ${{ matrix.php }} Integration Test on ${{ matrix.os }}

steps:
- name: Checkout Cacti
uses: actions/checkout@v4
with:
repository: Cacti/cacti
path: cacti

- name: Checkout mikrotik Plugin
uses: actions/checkout@v4
with:
path: cacti/plugins/mikrotik

- name: Install PHP ${{ matrix.php }}
uses: shivammathur/setup-php@v2
with:
php-version: ${{ matrix.php }}
extensions: intl, mysql, gd, ldap, gmp, xml, curl, json, mbstring
ini-values: "post_max_size=256M, max_execution_time=60, date.timezone=America/New_York"

- name: Check PHP version
run: php -v

- name: Run apt-get update
run: sudo apt-get update

- name: Install System Dependencies
run: sudo apt-get install -y apache2 snmp snmpd rrdtool fping libapache2-mod-php${{ matrix.php }}

- name: Start SNMPD Agent and Test
run: |
sudo systemctl start snmpd
sudo snmpwalk -c public -v2c -On localhost .1.3.6.1.2.1.1

- name: Setup Permissions
run: |
sudo chown -R www-data:runner ${{ github.workspace }}/cacti
sudo find ${{ github.workspace }}/cacti -type d -exec chmod 775 {} \;
sudo find ${{ github.workspace }}/cacti -type f -exec chmod 664 {} \;
sudo chmod +x ${{ github.workspace }}/cacti/cmd.php
sudo chmod +x ${{ github.workspace }}/cacti/poller.php

- name: Create MySQL Config
run: |
echo -e "[client]\nuser = root\npassword = cactiroot\nhost = 127.0.0.1\n" > ~/.my.cnf
cat ~/.my.cnf

- name: Initialize Cacti Database
env:
MYSQL_AUTH_USR: '--defaults-file=~/.my.cnf'
run: |
mysql $MYSQL_AUTH_USR -e 'CREATE DATABASE IF NOT EXISTS cacti;'
mysql $MYSQL_AUTH_USR -e "CREATE USER IF NOT EXISTS 'cactiuser'@'localhost' IDENTIFIED BY 'cactiuser';"
mysql $MYSQL_AUTH_USR -e "GRANT ALL PRIVILEGES ON cacti.* TO 'cactiuser'@'localhost';"
mysql $MYSQL_AUTH_USR -e "GRANT SELECT ON mysql.time_zone_name TO 'cactiuser'@'localhost';"
mysql $MYSQL_AUTH_USR -e "FLUSH PRIVILEGES;"
mysql $MYSQL_AUTH_USR cacti < ${{ github.workspace }}/cacti/cacti.sql
mysql $MYSQL_AUTH_USR -e "INSERT INTO settings (name, value) VALUES ('path_php_binary', '/usr/bin/php')" cacti

- name: Validate composer files
run: |
cd ${{ github.workspace }}/cacti
if [ -f composer.json ]; then
composer validate --strict || true
fi

- name: Install Composer Dependencies
run: |
cd ${{ github.workspace }}/cacti
if [ -f composer.json ]; then
sudo composer install --prefer-dist --no-progress
fi

- name: Create Cacti config.php
run: |
cat ${{ github.workspace }}/cacti/include/config.php.dist | \
sed -r "s/localhost/127.0.0.1/g" | \
sed -r "s/'cacti'/'cacti'/g" | \
sed -r "s/'cactiuser'/'cactiuser'/g" | \
sed -r "s/'cactiuser'/'cactiuser'/g" > ${{ github.workspace }}/cacti/include/config.php
sudo chmod 664 ${{ github.workspace }}/cacti/include/config.php

- name: Configure Apache
run: |
cat << 'EOF' | sed 's#GITHUB_WORKSPACE#${{ github.workspace }}#g' > /tmp/cacti.conf
<VirtualHost *:80>
ServerAdmin webmaster@localhost
DocumentRoot GITHUB_WORKSPACE/cacti

<Directory GITHUB_WORKSPACE/cacti>
Options Indexes FollowSymLinks
AllowOverride All
Require all granted
</Directory>

ErrorLog ${APACHE_LOG_DIR}/error.log
CustomLog ${APACHE_LOG_DIR}/access.log combined
</VirtualHost>
EOF
sudo cp /tmp/cacti.conf /etc/apache2/sites-available/000-default.conf
sudo systemctl restart apache2

- name: Install Cacti via CLI
run: |
cd ${{ github.workspace }}/cacti
sudo php cli/install_cacti.php --accept-eula --install --force

- name: Install mikrotik Plugin
run: |
cd ${{ github.workspace }}/cacti
sudo php cli/plugin_manage.php --plugin=mikrotik --install --enable

# - name: import mikrotik Plugin Sample Data
# run: |
# cd ${{ github.workspace }}/cacti/plugins/mikrotik
# sudo php cli_import.php --filename=.github/workflows/mikrotik_sample_data.xml
# if [ $? -ne 0 ]; then
# echo "Failed to import Thold sample data"
# exit 1
# fi

- name: Check PHP Syntax for Plugin
run: |
cd ${{ github.workspace }}/cacti/plugins/mikrotik
if find . -name '*.php' -exec php -l {} 2>&1 \; | grep -iv 'no syntax errors detected'; then
echo "Syntax errors found!"
exit 1
fi

- name: Remove the plugins directory exclusion from the .phpstan.neon
run: sed '/plugins/d' -i .phpstan.neon
working-directory: ${{ github.workspace }}/cacti

- name: Mark composer scripts executable
run: sudo chmod +x ${{ github.workspace }}/cacti/include/vendor/bin/*

- name: Run Linter on base code
run: composer run-script lint ${{ github.workspace }}/cacti/plugins/mikrotik
working-directory: ${{ github.workspace }}/cacti

- name: Checking coding standards on base code
run: composer run-script phpcsfixer ${{ github.workspace }}/cacti/plugins/mikrotik
working-directory: ${{ github.workspace }}/cacti

# - name: Run PHPStan at Level 6 on base code outside of Composer due to technical issues
# run: ./include/vendor/bin/phpstan analyze --level 6 ${{ github.workspace }}/cacti/plugins/mikrotik
# working-directory: ${{ github.workspace }}/cacti

- name: Run Cacti Poller
run: |
cd ${{ github.workspace }}/cacti
sudo php poller.php --poller=1 --force --debug
if ! grep -q "SYSTEM STATS" log/cacti.log; then
echo "Cacti poller did not finish successfully"
cat log/cacti.log
exit 1
fi

- name: View Cacti Logs
if: always()
run: |
if [ -f ${{ github.workspace }}/cacti/log/cacti.log ]; then
echo "=== Cacti Log ==="
sudo cat ${{ github.workspace }}/cacti/log/cacti.log
fi
4 changes: 2 additions & 2 deletions RouterOS/routeros_api.class.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ public function parseResponse($response) {
if ($MATCHES[0][0] == 'ret') {
$singlevalue = $MATCHES[0][1];
}
$CURRENT[$MATCHES[0][0]] = (isset($MATCHES[0][1]) ? $MATCHES[0][1] : '');
$CURRENT[$MATCHES[0][0]] = ($MATCHES[0][1] ?? '');
}
}
}
Expand Down Expand Up @@ -215,7 +215,7 @@ public function parseResponse4Smarty($response) {
if ($MATCHES[0][0] == 'ret') {
$singlevalue = $MATCHES[0][1];
}
$CURRENT[$MATCHES[0][0]] = (isset($MATCHES[0][1]) ? $MATCHES[0][1] : '');
$CURRENT[$MATCHES[0][0]] = ($MATCHES[0][1] ?? '');
}
}
}
Expand Down
11 changes: 9 additions & 2 deletions mikrotik_users.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,10 @@ function form_actions() {

/* if we are to save this form, instead of display it */
if (isset_request_var('selected_items')) {
$selected_items = unserialize(stripslashes(get_request_var('selected_items')));
$selected_items = unserialize(stripslashes(get_request_var('selected_items')), ['allowed_classes' => false]);
if (!is_array($selected_items)) {
$selected_items = [];
}
Comment on lines +63 to +66
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unserialize() on request data can emit warnings/notices on malformed input (e.g., "Error at offset...") even though you later fall back to []. Since selected_items is user-controlled, consider validating that the value is a plausible serialized array before deserializing, or otherwise handling deserialization errors in a way that avoids noisy logs (while keeping allowed_classes => false).

Copilot uses AI. Check for mistakes.

if (get_request_var('drp_action') == '1') { /* delete */
if (!isset_request_var('delete_type')) { set_request_var('delete_type', 2); }
Expand Down Expand Up @@ -105,7 +108,11 @@ function form_actions() {

api_graph_remove_multi($graphs_to_act_on);

db_execute("DELETE FROM plugin_mikrotik_users WHERE name IN ('" . implode("','", $devices_to_act_on) . "')");
// Issue QUEUE-6: use prepared statement for IN() — prevents name-column SQL injection
if (!empty($devices_to_act_on)) {
$placeholders = implode(',', array_fill(0, count($devices_to_act_on), '?'));
db_execute_prepared("DELETE FROM plugin_mikrotik_users WHERE name IN ($placeholders)", $devices_to_act_on);
}
Comment on lines +111 to +115
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new if (!empty($devices_to_act_on)) { ... } guard changes behavior when the selection is empty: previously the code would still execute a DELETE with an empty IN ('') list, now it performs no DELETE. This is probably safer, but it contradicts the PR description’s claim of “zero behavioral impact”; either adjust the description or preserve the previous semantics explicitly.

Copilot uses AI. Check for mistakes.
}

header('Location: mikrotik_users.php?header=false');
Expand Down
42 changes: 21 additions & 21 deletions poller_mikrotik.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1045,7 +1045,7 @@ function mikrotik_dateParse($value) {
$value[1] = substr($value[1], 0, strpos($value[1], '.'));
}

$date1 = trim($value[0] . ' ' . (isset($value[1]) ? $value[1]:''));
$date1 = trim($value[0] . ' ' . ($value[1] ?? ''));
if (strtotime($date1) === false) {
$value = date('Y-m-d H:i:s');
} else {
Expand Down Expand Up @@ -1304,10 +1304,10 @@ function collect_list_details(&$host) {
if (cacti_sizeof($array) && $noServer === false) {
foreach($array as $row) {
$list['host_id'] = $host['id'];
$list['dynamic'] = isset($row['dynamic']) ? $row['dynamic']:'0';
$list['disabled']= isset($row['disabled']) ? $row['disabled']:'0';
$list['list'] = isset($row['list']) ? $row['list']:'N/A';
$list['address'] = isset($row['address']) ? $row['address']:'';
$list['dynamic'] = $row['dynamic'] ?? '0';
$list['disabled']= $row['disabled'] ?? '0';
$list['list'] = $row['list'] ?? 'N/A';
$list['address'] = $row['address'] ?? '';
$list['created'] = isset($row['creation-time']) ? date('Y-m-d H:i:s', strtotime(str_replace('/', ' ', $row['creation-time']))):date('Y-m-d H:i:s');
$list['timeout'] = isset($row['timeout']) ? mikrotik_parse_ttl($row['timeout']) :'-1';

Expand Down Expand Up @@ -1405,11 +1405,11 @@ function collect_dns_details(&$host) {
foreach($array as $row) {
if (isset($row['type']) && $row['type'] != '-1') {
$dns['host_id'] = $host['id'];
$dns['type'] = isset($row['type']) ? $row['type']:'-1';
$dns['data'] = isset($row['data']) ? $row['data']:'-';
$dns['name'] = isset($row['name']) ? $row['name']:'';
$dns['type'] = $row['type'] ?? '-1';
$dns['data'] = $row['data'] ?? '-';
$dns['name'] = $row['name'] ?? '';
$dns['ttl'] = isset($row['ttl']) ? mikrotik_parse_ttl($row['ttl']) :'0';
$dns['static'] = isset($row['static']) ? $row['static']:'false';
$dns['static'] = $row['static'] ?? 'false';

$sql[] = '(' .
$dns['host_id'] . ',' .
Expand Down Expand Up @@ -1552,7 +1552,7 @@ function collect_dhcp_details(&$host) {

if (cacti_sizeof($array) && $noServer === false) {
foreach($array as $row) {
$mac_address = isset($row['mac-address']) ? $row['mac-address']:'-99';
$mac_address = $row['mac-address'] ?? '-99';
if (isset($entries[$mac_address]) && $mac_address != 99) {
$dhcp = $entries[$mac_address];
}
Expand All @@ -1564,20 +1564,20 @@ function collect_dhcp_details(&$host) {
}

$dhcp['host_id'] = $host['id'];
$dhcp['address'] = isset($row['address']) ? $row['address']:'N/A';
$dhcp['mac_address'] = isset($row['mac-address']) ? $row['mac-address']:'N/A';
$dhcp['client_id'] = isset($row['client-id']) ? $row['client-id']:'';
$dhcp['address_lists'] = isset($row['address-lists']) ? $row['address-lists']:'N/A';
$dhcp['server'] = isset($row['server']) ? $row['server']:'N/A';
$dhcp['dhcp_option'] = isset($row['dhcp-option']) ? $row['dhcp-option']:'N/A';
$dhcp['status'] = isset($row['status']) ? $row['status']:'N/A';
$dhcp['address'] = $row['address'] ?? 'N/A';
$dhcp['mac_address'] = $row['mac-address'] ?? 'N/A';
$dhcp['client_id'] = $row['client-id'] ?? '';
$dhcp['address_lists'] = $row['address-lists'] ?? 'N/A';
$dhcp['server'] = $row['server'] ?? 'N/A';
$dhcp['dhcp_option'] = $row['dhcp-option'] ?? 'N/A';
$dhcp['status'] = $row['status'] ?? 'N/A';
$dhcp['expires_after'] = isset($row['expires-after']) ? uptimeToSeconds($row['expires-after']):0;
$dhcp['last_seen'] = isset($row['last-seen']) ? uptimeToSeconds($row['last-seen']):0;
$dhcp['active_address'] = isset($row['active_address']) ? $row['active-address']:'';
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$dhcp['active_address'] checks isset($row['active_address']) but then reads $row['active-address']. Since this data comes from parseResponse() (hyphenated keys like mac-address), the isset() check will never pass, so active_address will be recorded as an empty string even when active-address is present. Use the same key on both sides (e.g., read via $row['active-address'] ?? '', or switch to the underscore-normalized response consistently).

Suggested change
$dhcp['active_address'] = isset($row['active_address']) ? $row['active-address']:'';
$dhcp['active_address'] = $row['active-address'] ?? '';

Copilot uses AI. Check for mistakes.
$dhcp['active_mac_address'] = isset($row['active-mac-address']) ? $row['active-mac-address']:'';
$dhcp['active_client_id'] = isset($row['active-client-id']) ? $row['active-client-id']:'';
$dhcp['active_server'] = isset($row['active-server']) ? $row['active-server']:'';
$dhcp['hostname'] = isset($row['host-name']) ? $row['host-name']:'';
$dhcp['active_mac_address'] = $row['active-mac-address'] ?? '';
$dhcp['active_client_id'] = $row['active-client-id'] ?? '';
$dhcp['active_server'] = $row['active-server'] ?? '';
$dhcp['hostname'] = $row['host-name'] ?? '';
$dhcp['radius'] = isset($row['radius']) ? ($row['radius'] == 'true' ? 1:0):0;
$dhcp['dynamic'] = isset($row['dynamic']) ? ($row['dynamic'] == 'true' ? 1:0):0;
$dhcp['blocked'] = isset($row['blocked']) ? ($row['blocked'] == 'true' ? 1:0):0;
Expand Down
Loading