chore(security): harden report actions and theme handling#7169
Open
somethingwithproof wants to merge 13 commits into
Open
chore(security): harden report actions and theme handling#7169somethingwithproof wants to merge 13 commits into
somethingwithproof wants to merge 13 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Hardens remote agent authentication, validates graph theme inputs to prevent path traversal, and adds ownership/authorization checks to report bulk actions.
Changes:
- Rewrites
remote_client_authorized()to validate against poller hostnames first (with APCu-backed result caching), and adds forward/reverse DNS checks only as a secondary path. - Adds a new
cacti_validate_theme()helper and applies it everywhere graph themes are read from user input. - Adds an owner/admin authorization check to the reports bulk-action handler and switches to
sanitize_unserialize_selected_items().
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| remote_agent.php | Reworks remote agent auth with poller list pre-filtering and APCu cache; validates theme input. |
| lib/rrd.php | Uses cacti_validate_theme() when resolving the rrdtheme.php path. |
| lib/html_reports.php | Adds per-report ownership check in bulk actions; switches to sanitized unserialize. |
| lib/functions.php | Adds cacti_validate_theme() helper that scans the themes directory. |
| graph_json.php | Validates graph_theme request value via cacti_validate_theme(). |
| graph_image.php | Validates graph_theme request value via cacti_validate_theme(). |
Files not reviewed (1)
- include/js/purify.js: Language not supported
TheWitness
previously approved these changes
May 28, 2026
This was referenced May 30, 2026
somethingwithproof
force-pushed
the
fix/security-ghsa-parity-develop
branch
from
7abc0bd to
7fb1982
Compare
Contributor
Author
|
Rebased onto develop (picks up #7188) and reverted the off-topic exec changes in poller_spikekill / script_server / snmpagent_persist / snmpagent_mibcache / support. Those had been switched to 1.2.x-style |
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Move whitelist check before the single-poller guard so single-poller installs relying on the whitelist are not incorrectly rejected. Narrow SELECT * to SELECT hostname; the loop only reads hostname. Drop function_exists guards around md5/hash; both are PHP core and the raw $cache_seed fallback is unsafe as an APCu key. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
…e_theme read_config_option can return false or null when the setting is absent. Without the cast, a strict comparison to '' would not match, leaving $default unset and risking a null-typed value flowing into callers. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Replace input_validate_input_number (which calls die_html_input_error on bad input, aborting the entire bulk action) with an inline guard. Cast $report_id to int at call sites where the signature requires int. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
- Refactored poller_spikekill.php to use array-based execution and sanitized SQL IN clauses. - Refactored script_server.php Windows process check to use cacti_exec. - Hardened snmpagent background process lifecycles. - Standardized version checks in support.php and utilities.php. - Replaced short array syntax [] with array() and verified 7.4 compatibility. - Synchronized all hardened files to primary developer path.
…ackground() cacti_process_execute() was called but never defined, causing a PHP fatal. Replaced with exec_background() from lib/poller.php. Also fix pre-existing parse error in snmpagent_persist.php ([ closed with ) instead of ]). Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
php-cs-fixer (array_syntax/list_syntax short) flagged array() introduced on this branch; convert to [] so the coding-standards gate passes. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
poller_spikekill, script_server, snmpagent_persist/mibcache and support had been switched to 1.2.x-style cacti_exec(), which is undefined on develop and failed phpstan. Restore develop's native exec() idiom; these files are outside this PR's scope (report actions and theme handling). Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
phpstan 2.2.2 added a stricter is_numeric() always-true inference that fails on pre-existing develop code (lib/data_query.php); pin until that is addressed upstream so CI is reproducible. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
somethingwithproof
force-pushed
the
fix/security-ghsa-parity-develop
branch
from
3dc14e7 to
ea87cce
Compare
Clears the level 6 alreadyNarrowedType error on lib/data_query.php:2591 that develop currently emits, matching the sibling narrowing identifiers already ignored. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
reports_form_actions()with per-item authorization checks before bulk mutationscacti_validate_theme()and apply it to graph theme inputs and RRD theme resolution paths3.4.7)Notes
1.2.x