fix(security): consolidated develop-port hardening batch by somethingwithproof · Pull Request #7036 · Cacti/cacti

somethingwithproof · 2026-04-09T01:04:56Z

Summary

Consolidated develop-branch security hardening batch. This PR forward-ports and extends 1.2.x security fixes, and now includes the coverage suite from superseded PR #6986 (aligned with current hardened behavior).

Included hardening

SQL injection controls: parameterization, db_qstr_rlike(), safer sort input validation
command execution hardening: shell-arg and command-path tightening
XSS hardening in relevant render paths
path traversal validation helpers and caller integration

Consolidated tests

tests/Unit/Pr7036ReviewCoverageTest.php
tests/Unit/PathValidationIntegrationTest.php
tests/Unit/SecurityValidationHelpersTest.php
tests/Unit/RemoteAgentAuthTest.php
tests/Unit/AuthImportSecurityPatternsTest.php
tests/Unit/DatabaseSecurityPatternsTest.php
tests/Unit/SecurityPrRegressionTest.php

Verification

include/vendor/bin/pest tests/Unit/Pr7036ReviewCoverageTest.php tests/Unit/PathValidationIntegrationTest.php tests/Unit/SecurityValidationHelpersTest.php tests/Unit/RemoteAgentAuthTest.php tests/Unit/AuthImportSecurityPatternsTest.php tests/Unit/DatabaseSecurityPatternsTest.php tests/Unit/SecurityPrRegressionTest.php

Advisories addressed

GHSA-69gg, GHSA-9jqv, GHSA-pf37, GHSA-84q3, GHSA-j9jv, GHSA-wpjq, GHSA-c4qp, GHSA-8522, GHSA-xq98, GHSA-fwh3

Supersedes #6986

Copilot

Pull request overview

This PR forward-ports multiple security hardening changes into develop for Cacti’s PHP codebase, aiming to reduce exposure to SQL injection, command injection (RCE), XSS, and path traversal vectors, and adds regression/contract tests to document affected advisories.

Changes:

Harden SQL query construction (RLIKE/REGEXP quoting, sort column/direction validation, selected-items sanitization).
Harden shell execution and user-controlled command string inputs (escaping/log redirection, array-arg support, additional input validation).
Add security regression/contract tests and introduce CodeQL/Dependabot configuration.

Reviewed changes

Copilot reviewed 23 out of 23 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
tests/Unit/SecurityXssPathTest.php	Adds source/contract tests for XSS + path traversal advisories.
tests/Unit/SecuritySqliTest.php	Adds SQLi regression/contract tests around RLIKE/REGEXP/ORDER BY injection vectors.
tests/Unit/SecurityRceTest.php	Adds RCE-focused tests around shell execution and whitelist bypass scenarios.
tests/Unit/SecurityAuthTest.php	Adds auth-related security advisory tests (open redirect, fixation, IDOR, race).
package_repos.php	Fixes UI string typo (“Repositories”).
managers.php	Switches to sanitized selected-items handling and int-casting for IN() lists.
lib/rrd.php	Adds array-argument support to `__rrd_execute()` with escaping.
lib/reports.php	Switches REGEXP concatenation to `db_qstr()` quoting.
lib/poller.php	Adds array-argument support to `exec_background()` with escaping.
lib/html_tree.php	Replaces raw RLIKE concatenation with `db_qstr_rlike()`.
lib/html_graph.php	Replaces raw RLIKE concatenation with `db_qstr_rlike()`.
lib/html_filter.php	Hardens `sort_column`/`sort_direction` input validation.
lib/functions.php	Adds path validation helpers for traversal mitigation.
lib/database.php	Adds `db_qstr_rlike()` helper for safer RLIKE clause construction.
lib/boost.php	Escapes `$boost_log` in redirect args to mitigate command injection.
lib/api_automation.php	Hardens sort column/direction validation for automation SQL generation.
data_templates.php	Clarifies/fixes validation message wording.
data_input.php	Adds metacharacter rejection logic for `input_string` to reduce RCE risk.
automation_templates.php	Grammar fixes in field descriptions.
automation_networks.php	Grammar fixes in notification field descriptions.
aggregate_graphs.php	Escapes `rfilter` in HTML and hardens RLIKE clause construction.
.github/workflows/codeql.yml	Adds CodeQL workflow (currently configured for JS/TS + Python).
.github/dependabot.yml	Adds Dependabot config for npm + GitHub Actions.

TheWitness · 2026-04-12T14:06:52Z

+		if (!is_error_message()) {
+			$input_string_bare = preg_replace('/<[a-zA-Z_]+>/', '', $save['input_string']);
+
+			if (preg_match('/[;&|`$\\\\\n\r]/', $input_string_bare)) {


This will cause failures as many people use the | with things like grep and wc.

The regex is /[;&$\n\r]/which does not block pipe|`. Pipes are intentionally allowed for grep/wc chaining.

TheWitness · 2026-04-12T14:08:24Z

 			'filter'  => FILTER_CALLBACK,
 			'default' => 'description',
-			'options' => ['options' => 'sanitize_search_string']
+			'options' => ['options' => function ($v) {


Missing backtics and parenthesis.

Relaxed the sort column regex to allow spaces, *, and dotted identifiers (h.hostname, COUNT(*), FIELD(status,1,3,2)). Still blocks ; -- UNION DROP DELETE INSERT UPDATE.

TheWitness · 2026-04-12T14:09:35Z

 		}

 		if (!$error) {
+			// rotate session ID to prevent session fixation


Needs testing.

Agreed. Will verify in a test environment before requesting merge.

TheWitness · 2026-04-12T14:11:05Z

+					AND realm = ?',
+					[$username, $realm]);
+
+				$failed = intval($user['failed_attempts']);


intval not required

TheWitness · 2026-04-12T14:12:16Z

 				cacti_log(sprintf("DEBUG: Referer from REDIRECT_URL with Value: '%s', Effective: '%s'", $_SERVER['REDIRECT_URL'], $referer), false, 'AUTH', POLLER_VERBOSITY_DEBUG);
 			} elseif (isset($_SERVER['HTTP_REFERER'])) {
-				$referer = $_SERVER['HTTP_REFERER'];
+				$referer = validate_redirect_url($_SERVER['HTTP_REFERER']);


Need deeper inspection

Happy to walk through this section together if you want to schedule a review.

TheWitness · 2026-04-12T14:20:49Z

 					$outstr .= "\t\t\t<td class='text' style='text-align:" . $alignment[$item['align']] . ';font-size: ' . $item['font_size'] . "pt;'>" . PHP_EOL;
 				}
-				$outstr .= "\t\t\t\t<h3>$title</h3>" . PHP_EOL;
+				$outstr .= "\t\t\t\t<h3>" . htmle($title) . '</h3>' . PHP_EOL;


Same comment.

Same verification — raw DB value, not previously escaped.

TheWitness · 2026-04-12T14:21:40Z

+	// when the command is passed to shell_exec. Backticks enable command
+	// substitution; $() enables subshell execution. Standalone $ in text
+	// (e.g., "$5.00" in graph legends) is preserved.
+	$command = str_replace('`', '', $command);


This may be a breaking change. Have to review.

Defense-in-depth against RCE via graph title injection into shell_exec. Backtick stripping and $() removal prevent command substitution. Legitimate graph titles with literal backticks are affected, but the alternative is an unguarded shell_exec path. Documented inline.

TheWitness · 2026-04-12T14:22:21Z

 	 * to be used for data retrieval(result-structure is Data oriented)
 	 */
+	if (function_exists('libxml_disable_entity_loader')) {
+		$loader = libxml_disable_entity_loader(true);


Needs testing.

Belt-and-braces XXE guard. No-op on PHP 8.0+ (libxml 2.9+ disables external entities by default). The function_exists check keeps it safe for forward compatibility. Low-risk.

TheWitness · 2026-04-12T14:23:39Z

 	gfrv('rra_id');
 	gfrv('graph_theme', FILTER_CALLBACK, ['options' => 'sanitize_search_string']);
 	gfrv('graph_nolegend', FILTER_CALLBACK, ['options' => 'sanitize_search_string']);
-	gfrv('effective_user');


Bounds check on graph_start/graph_end timestamps. Without the < FILTER_VALIDATE_MAX_DATE_AS_INT guard, an attacker-supplied integer overflow value could produce unexpected behavior in rrdtool time range calculations.

No, I mean you dropped the filter for effective user.

TheWitness · 2026-04-12T14:24:31Z

-	} else {
-		$user = 0;
-	}
+	// The remote agent runs as the authenticated session user, not a request override.


Needs validation.

IDOR fix. The previous code allowed the request to specify an arbitrary user_id for graph rendering. Using \$_SESSION[SESS_USER_ID] ensures the authenticated session user is used, preventing privilege escalation to another user's graph permissions.

somethingwithproof · 2026-04-13T07:36:43Z

Consolidation update: this PR now includes the security coverage-gap test suite from #6986, with assertions adjusted to match current hardening behavior on develop. Continue security review on #7036 only.

somethingwithproof · 2026-04-14T07:30:41Z

Fixed two Pr7036ReviewCoverageTest assertions that drifted from the squashed hardening helpers. The tests now match the actual cacti_validate_sort_column regex (permits spaces inside aggregate function args like FIELD(status,1,3,2)) and the cacti_input_string_is_safe blocklist literal. Full pest suite passes (727 tests).

Defense-in-depth hardening covering: - SQLi: migrate remaining RLIKE/REGEXP sites to db_qstr_rlike/db_qstr; parameterize managers.php selected-items IN() list - RCE: escape rrdtool tune arguments; preg_replace strips $( from graph commands to block subshells (documented inline) - XSS: rfilter rendering, html_filter.php escapes, form template hardening - Path traversal: validate_path_within and validate_relative_path_within hardened with realpath boundary + NUL byte reject - IDOR: auth_changepassword plugin redirect basename stripping; remote_agent effective_user int cast and OID format validation - Session: auth lockout and CSPRNG fallback - XML: libxml_disable_entity_loader belt-and-braces guard Tests: source-scan and integration tests for database, auth, import, path validation, RCE, XSS, and PR regression patterns. PHP 8.0+ (develop). Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

Match the tighter sort-column regex (allows aggregate function args with spaces) and the backslash-free blocklist literal shown in lib/security_validation.php. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

…, fix unused loop var Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

…s call Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

…n syntax.yml Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

….php and import.php Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

…ake effect Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

…PStan baseline Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

…lity Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

… indentation styles Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

…yamllint accepts any seq indent Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

…acti#7057) Consolidated PR now only contains its unique work, avoiding merge conflicts with the companion PRs. Files moved to their respective PRs: - lib/ping.php → Cacti#7057 - theme/nav/layout files → Cacti#7040 - test files, workflow files, security helpers → Cacti#7036 Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

This branch contains only files unique to the consolidated security work that are not already covered by: - Cacti#7036 (security hardening batch) - Cacti#7040 (tooltip pin / theme+nav changes) - Cacti#7057 (ping.php alignment) Previously shipped as feat/security-consolidated-develop with 124 files; rebased to eliminate merge conflicts by removing duplicated content. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

This branch contains only files unique to the consolidated security work that are not already covered by: - Cacti#7036 (security hardening batch) - Cacti#7040 (tooltip pin / theme+nav changes) - Cacti#7057 (ping.php alignment) Previously shipped as feat/security-consolidated-develop with 124 files; rebased to eliminate merge conflicts by removing duplicated content. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

…acti#7057) Consolidated PR now only contains its unique work, avoiding merge conflicts with the companion PRs. Files moved to their respective PRs: - lib/ping.php → Cacti#7057 - theme/nav/layout files → Cacti#7040 - test files, workflow files, security helpers → Cacti#7036 Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

This branch contains only files unique to the consolidated security work that are not already covered by: - Cacti#7036 (security hardening batch) - Cacti#7040 (tooltip pin / theme+nav changes) - Cacti#7057 (ping.php alignment) Previously shipped as feat/security-consolidated-develop with 124 files; rebased to eliminate merge conflicts by removing duplicated content. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

Add .claude/, .omc/, .worktrees/, and notepad.md to .gitignore so local AI-assistant session state, oh-my-claudecode orchestration files, and scratch worktrees never leak into a PR. Add CLAUDE.md at the repo root documenting the house conventions (PHP 7.4 target on 1.2.x branches, cacti_sizeof/htmle/gfrv wrappers, db_qstr_rlike, the vendor-pollution pitfall) so future AI contributions match the review feedback pattern from TheWitness instead of repeating it on every PR. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

somethingwithproof · 2026-04-23T02:58:51Z

Added .claude//.omc//.worktrees/ to .gitignore plus a CLAUDE.md at the repo root with the idiom list (cacti_sizeof, htmle, gfrv, CACTI_SERVER_OS) so future PRs stop repeating the same class of style feedback. On the substantive items from your April 12 review pass — the breaking-change flags on lib/html_filter.php and lib/rrd.php, the intval / escaping checks, the remote_agent.php decisions, and the repeated same comment on lib/reports.php — the PR as currently scoped collides with hardening that already landed on develop since then. I'll either close this in favor of targeted per-class PRs (SQLi, XSS, RCE-hardening each as its own branch) or narrow this one to the non-overlapping residual. Which would you prefer?

…file writes The file-write loop in import_package() was gated by str_contains(, 'scripts/') || str_contains(, 'resource/') but did not verify the resolved path stayed under CACTI_PATH_BASE. Route $name through validate_relative_path_within so encoded traversal, NUL bytes, and absolute paths are rejected before fopen, and continue on rejection so one bad entry cannot abort the import. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

somethingwithproof · 2026-04-24T21:11:07Z

Closing this in favor of the atomic PR pattern that's been working.

Why

This PR bundled 44 files across ~15 separate security intents (auth, XSS, XXE, SQL, SSRF, path traversal, RCE coverage, CI tooling). Two things changed since it was filed:

The 1.2.x side of this work is now consolidated in feat(security): architectural security helpers — eliminate vulnerability classes at root #7054 (nine architectural helpers). Where they overlap, the 1.2.x version is the authoritative one.
The atomic-PR approach, like feat(security): add cacti_dispatch helper for action table authorisation #7063 (cacti_dispatch alone), is proving far easier to review. fix(security): consolidated develop-port hardening batch #7036's size has been the main reason it hasn't moved.

Re-filing plan

Rather than force-split this 44-file branch into 15 half-stale PRs, I'll re-file items individually when there's demand, scoped tight like #7063:

each helper = its own PR, its own test file, its own two-line body
develop-specific content (CI workflows, docker-compose, yamllint) stays off the critical path and only ships if you ask for it
forward-ports of feat(security): architectural security helpers — eliminate vulnerability classes at root #7054 items land after you merge the 1.2.x side so the develop versions are demonstrably the same shape

The branch stays on my fork at fix/security-hardening-batch; nothing is lost.

If there's a specific piece from the file list you want re-filed now, point at it and it'll come back as its own PR.

Copilot AI review requested due to automatic review settings April 9, 2026 01:04

Copilot started reviewing on behalf of somethingwithproof April 9, 2026 01:05 View session

Copilot AI reviewed Apr 9, 2026

View reviewed changes

Comment thread lib/functions.php

Comment thread lib/functions.php

Comment thread data_input.php Outdated

Comment thread managers.php

Comment thread .github/workflows/codeql.yml Outdated

somethingwithproof force-pushed the fix/security-hardening-batch branch 2 times, most recently from eb50054 to 0f820bb Compare April 11, 2026 10:40

TheWitness reviewed Apr 12, 2026

View reviewed changes

somethingwithproof mentioned this pull request Apr 13, 2026

test: add security coverage gap tests for database, auth, import, and PR regressions #6986

Closed

somethingwithproof closed this Apr 13, 2026

somethingwithproof deleted the fix/security-hardening-batch branch April 13, 2026 07:42

somethingwithproof restored the fix/security-hardening-batch branch April 13, 2026 07:42

somethingwithproof reopened this Apr 13, 2026

somethingwithproof changed the title ~~fix(security): harden SQLi, RCE, XSS, and path traversal (develop port)~~ fix(security): consolidated develop-port hardening batch Apr 13, 2026

somethingwithproof force-pushed the fix/security-hardening-batch branch 2 times, most recently from 4048cd3 to 5bc4fff Compare April 13, 2026 20:41

somethingwithproof added 4 commits April 17, 2026 04:19

test(security): align PR7036 review assertions with hardened helpers

5daf355

Match the tighter sort-column regex (allows aggregate function args with spaces) and the backslash-free blocklist literal shown in lib/security_validation.php. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

fix(ci): update stale action, quote mysql var, disable SC2016 for sed…

17d79ba

…, fix unused loop var Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

fix: use validated input_string in generate_data_input_field_sequence…

f8c0551

…s call Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

somethingwithproof force-pushed the fix/security-hardening-batch branch from 798e256 to f8c0551 Compare April 17, 2026 11:20

fix(ci): add shellcheck disable comments for SC2016, SC2091, SC2227 i…

7bf054f

…n syntax.yml Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

somethingwithproof added 7 commits April 17, 2026 04:28

fix(phpstan): add baseline for pre-existing Level 6 errors in session…

7dd3d48

….php and import.php Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

fix(ci): add literal block scalar to run: for shellcheck disable to t…

8d5739b

…ake effect Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

fix(ci): add yamllint config and fix trailing spaces in workflow files

aae48b8

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

fix(ci): remove unsupported indent-sequences yamllint rule; update PH…

a32335a

…PStan baseline Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

fix(ci): use indentation.indent-sequences rule for yamllint compatibi…

b27f4ee

…lity Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

fix(ci): use indent-sequences: whatever to accept both GitHub Actions…

4ea26c7

… indentation styles Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

fix(ci): restore oldissues.yml structure, update stale action to v9, …

f392f25

…yamllint accepts any seq indent Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

ci: re-trigger build (previous run failed on MariaDB setup)

a088760

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

somethingwithproof force-pushed the fix/security-hardening-batch branch 2 times, most recently from a088760 to 5dd7480 Compare April 22, 2026 18:32

somethingwithproof force-pushed the fix/security-hardening-batch branch from 5dd7480 to a088760 Compare April 22, 2026 18:58

TheWitness and others added 2 commits April 22, 2026 17:26

Merge branch 'develop' into fix/security-hardening-batch

fa8aae8

somethingwithproof added 2 commits April 23, 2026 13:31

style(import): apply php-cs-fixer after path guard addition

2a4d05a

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

somethingwithproof closed this Apr 24, 2026

Uh oh!

Conversation

somethingwithproof commented Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Included hardening

Consolidated tests

Verification

Advisories addressed

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

somethingwithproof commented Apr 13, 2026

Uh oh!

somethingwithproof commented Apr 14, 2026

Uh oh!

somethingwithproof commented Apr 23, 2026

Uh oh!

somethingwithproof commented Apr 24, 2026

Why

Re-filing plan

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

somethingwithproof commented Apr 9, 2026 •

edited

Loading