feat(security): consolidated security hardening — forward-port from 1.2.x + DOM XSS + JS fixes#7055
Open
somethingwithproof wants to merge 58 commits intoCacti:developfrom
Open
Conversation
Centralized shell command gateway routing all execution through cacti_exec() with argv-array input, per-argument escaping, optional timeout via proc_open + stream_select, and automatic credential redaction in logs. Migrated structure_rra_paths.php to use bounded GET_LOCK retry with exponential backoff and shutdown-function release guard. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Add early SAPI !== cli rejection in cli_check.php returning 404. Tighten Content-Security-Policy from default-src * to default-src 'self' with explicit per-directive sources, add HSTS on HTTPS, and add COOP/CORP headers. Add XML size-limit checks and structured parse-error logging in install/functions.php and lib/import.php. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Centralized outbound HTTP fetch with reserved-IP rejection via FILTER_FLAG_NO_RES_RANGE, TLS peer verification by default, redirect-following disabled, and configurable timeout. Prevents SSRF by resolving hostnames and checking all A records against RFC 1918/6598/loopback/link-local ranges before connecting. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Add cacti_ldap_filter() for safe LDAP filter construction using ldap_escape() with LDAP_ESCAPE_FILTER per substitution variable. Replaces raw string interpolation in search filter templates. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Add get_request_sort() for whitelist-only column/direction extraction, get_request_ids() for integer-only ID array parsing, and db_qstr_like() for safe SQL LIKE clause construction with percent/underscore escaping. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Add cacti_dispatch() declarative action table replacing ad-hoc switch/case on \$_REQUEST['action']. Enforces HTTP method, realm permission, and optional object-level ACL callback before dispatch. Logs unknown actions and method mismatches. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Apply html_escape() to unescaped description fields in managers.php SNMP notification tooltip and three locations in html_form.php where field_array description was rendered as raw HTML in formFieldDescription spans and display_tooltip() calls. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
…ntion Add cacti_auth_transition() to lib/auth.php for safe privilege-level changes. Regenerates session ID, checks lockout status, rotates remember-me cookie token, and invalidates cached permission data. Intended for login, password change, and role-switch call sites. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Document current csrf-magic state, known gaps, and short/medium/long term migration plan toward explicit token embedding and SameSite cookie enforcement. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Add phpstan.neon targeting new security gateway files at level 5 with ignore rules for Cacti global functions not visible to PHPStan. Add GitHub Actions workflow running PHPStan on PHP 8.1 for push/PR to 1.2.x branch. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Add Pest source-scanning tests verifying all 9 security architecture items: exec gateway, HTTP SSRF protection, LDAP filter escaping, request helpers, action dispatcher, XSS escaping, auth transition, CSP/HSTS headers, and XML hardening. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Atomic write with realpath containment. Rejects absolute paths, traversal segments, and symlinks that escape the allowed base directories. Post-write verification via realpath. Eliminates the file-write/path-traversal vulnerability class in package imports. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
basename() on plugin name, realpath containment check against the plugins directory. Eliminates the LFI/include-path vulnerability class across all 5 plugin include sites in lib/plugins.php. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
All 4 include_once sites in lib/plugins.php now use cacti_plugin_path() which enforces basename + realpath containment. Rejects traversal attempts and paths that resolve outside the plugins directory. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Force SHA-256 for package signature verification. The legacy SHA-1 key path is commented out. Packages signed with the old key must be re-signed with the SHA-256 key to import. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
When graph.php is loaded without a valid local_graph_id the existing $exists check at line 66 can pass due to MySQL implicit type coercion (WHERE local_graph_id = '' matches id 0). The subsequent db_fetch_row at line 105 returns false, and line 114 dereferences $graph['graph_template_id'] on a non-array, producing PHP 8.x 'Undefined array key' warnings for graph_template_id, rows, steps, and id. Add a cacti_sizeof guard after the graph fetch and redirect to graph_view.php with raise_message() so the user sees a clear error instead of a stack of PHP warnings. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Pest source-scan tests confirming the cacti_sizeof($graph) guard exists after db_fetch_row_prepared, fires raise_message with redirect to graph_view.php, and exits before any $graph['...'] dereference. All syntax is PHP 7.4 compatible (no arrow functions, no match expressions, no named arguments). Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
…ipts Prevent null array dereferences when db_fetch_row_prepared returns an empty result for a missing or deleted record. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
…ures Per TheWitness: guard-failure redirects should return the user to where they came from, not to a hardcoded page head. Use validate_redirect_url with HTTP_REFERER and a safe fallback to the file's own list view. Applies to all 16 guard sites added in 34cdca9 plus the graph.php guard from ddc5c35. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
The graph.php guard now redirects via validate_redirect_url with HTTP_REFERER instead of a hardcoded Location header. Update the test assertion to match. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Setting cache_directory to the Cacti install root itself passed the strpos prefix check because equality was not rejected. Tighten to require strict subdirectory (base + DIRECTORY_SEPARATOR prefix). Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
tempnam() files persist until removed. The import flow created a temp file but never cleaned it up, leaking one file per import operation. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
index.php and script_server.php had identical normalization + prefix blocks for dynamic-include confinement. Replace both with the new cacti_path_is_within helper in lib/functions.php. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Source-scan tests for cacti_csv_safe (tab/CR in dangerous list, ltrim applied), cacti_path_is_within usage in index.php and script_server.php, db_replace redaction of snmp_auth_passphrase and rsa_private_key, and boost cache-directory equality rejection. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
…ion/purge actions Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Csrf-magic fallback cookie now sets Secure, HttpOnly, SameSite=Strict. Token comparisons use hash_equals() instead of raw ===. GET logout documented as risk-accepted (annoyance-only, SameSite mitigates). Test coverage for all three changes. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
* Improve IPv6 support in RRDtool proxy and ping utilities
- Fix RRDtool proxy to dynamically detect IPv6 addresses and use
AF_INET6 sockets instead of hardcoded AF_INET (IPv4-only), enabling
connections to IPv6 RRDtool proxy servers including backup servers
- Strip brackets from IPv6 addresses before passing to socket_connect
- Properly close failed sockets before creating new ones for failover
- Replace fragile str_contains(':') IPv6 detection in ping with
filter_var(FILTER_FLAG_IPV6) for more robust address validation
https://claude.ai/code/session_01SbuDigvAkYvPKvdcougsdo
* Fix JS code quality: implicit globals, deprecated APIs, loose equality
- Add var declarations to prevent implicit globals in install.js
(element, enabled, button, buttonCheck)
- Remove console.log debug output left in production (install.js)
- Replace deprecated jQuery .unbind() with .off() (layout.js)
- Fix "depreciated" typo to "deprecated" in deprecation warnings
- Convert == / != to === / !== for null, boolean, string, typeof,
and numeric comparisons across install.js and realtime.js
https://claude.ai/code/session_01SbuDigvAkYvPKvdcougsdo
* Replace deprecated jQuery shorthand event methods in layout.js
Replace .click(), .keyup(), .keydown(), .mousedown(), .mouseenter(),
.mouseleave(), .submit(), .resize() with .on() equivalents. Replace
.focus(), .change() trigger calls with .trigger(). These shorthands
were deprecated in jQuery 3.5.
https://claude.ai/code/session_01SbuDigvAkYvPKvdcougsdo
* Replace deprecated jQuery methods in realtime.js
Replace .bind() with .on() and .change() trigger calls with
.trigger('change'). .bind() was deprecated in jQuery 3.0 and
shorthand triggers in jQuery 3.5.
https://claude.ai/code/session_01SbuDigvAkYvPKvdcougsdo
* Replace deprecated jQuery methods in install.js and fix ===/!== errors
Replace .click(), .change(), .focus() with .on()/.trigger() equivalents.
Also fix !=== and ==== operators that were incorrectly introduced by a
prior replace-all of == to === within existing !== and === expressions.
https://claude.ai/code/session_01SbuDigvAkYvPKvdcougsdo
* Restore == null where needed to catch both null and undefined
In JavaScript, == null matches both null and undefined, which is an
intentional idiom. The prior === null conversion broke cases where
values come from jQuery .val(), .data(), $.urlParam(), or object
property access that may return undefined rather than null. Revert
those specific cases while keeping === null where variables are
explicitly initialized to null.
https://claude.ai/code/session_01SbuDigvAkYvPKvdcougsdo
* Replace deprecated jQuery methods in all theme main.js files
Replace .unbind().click() with .off('click').on('click'), convert
.hover() to .on('mouseenter').on('mouseleave'), replace .change(),
.scroll(), .click() shorthands with .on() equivalents, and .blur()
with .trigger('blur') across all 10 theme files.
https://claude.ai/code/session_01SbuDigvAkYvPKvdcougsdo
* Convert string concatenation to template literals
Replace 'str' + var + 'str' patterns with ES6 template literals
in realtime.js and install.js. Improves readability especially for
URL construction and HTML building. Also replace $.parseJSON() with
native JSON.parse() in realtime.js.
https://claude.ai/code/session_01SbuDigvAkYvPKvdcougsdo
* Convert var to const for single-assignment variables
Replace var with const where the variable is assigned once and never
reassigned within its scope, in install.js and realtime.js. Keeps var
for variables that are conditionally reassigned (e.g. size, url).
https://claude.ai/code/session_01SbuDigvAkYvPKvdcougsdo
---------
Co-authored-by: Claude <noreply@anthropic.com>
* Improve IPv6 support in RRDtool proxy and ping utilities
- Fix RRDtool proxy to dynamically detect IPv6 addresses and use
AF_INET6 sockets instead of hardcoded AF_INET (IPv4-only), enabling
connections to IPv6 RRDtool proxy servers including backup servers
- Strip brackets from IPv6 addresses before passing to socket_connect
- Properly close failed sockets before creating new ones for failover
- Replace fragile str_contains(':') IPv6 detection in ping with
filter_var(FILTER_FLAG_IPV6) for more robust address validation
https://claude.ai/code/session_01SbuDigvAkYvPKvdcougsdo
* Fix JS code quality: implicit globals, deprecated APIs, loose equality
- Add var declarations to prevent implicit globals in install.js
(element, enabled, button, buttonCheck)
- Remove console.log debug output left in production (install.js)
- Replace deprecated jQuery .unbind() with .off() (layout.js)
- Fix "depreciated" typo to "deprecated" in deprecation warnings
- Convert == / != to === / !== for null, boolean, string, typeof,
and numeric comparisons across install.js and realtime.js
https://claude.ai/code/session_01SbuDigvAkYvPKvdcougsdo
* Replace deprecated jQuery shorthand event methods in layout.js
Replace .click(), .keyup(), .keydown(), .mousedown(), .mouseenter(),
.mouseleave(), .submit(), .resize() with .on() equivalents. Replace
.focus(), .change() trigger calls with .trigger(). These shorthands
were deprecated in jQuery 3.5.
https://claude.ai/code/session_01SbuDigvAkYvPKvdcougsdo
* Replace deprecated jQuery methods in realtime.js
Replace .bind() with .on() and .change() trigger calls with
.trigger('change'). .bind() was deprecated in jQuery 3.0 and
shorthand triggers in jQuery 3.5.
https://claude.ai/code/session_01SbuDigvAkYvPKvdcougsdo
* Replace deprecated jQuery methods in install.js and fix ===/!== errors
Replace .click(), .change(), .focus() with .on()/.trigger() equivalents.
Also fix !=== and ==== operators that were incorrectly introduced by a
prior replace-all of == to === within existing !== and === expressions.
https://claude.ai/code/session_01SbuDigvAkYvPKvdcougsdo
* Restore == null where needed to catch both null and undefined
In JavaScript, == null matches both null and undefined, which is an
intentional idiom. The prior === null conversion broke cases where
values come from jQuery .val(), .data(), $.urlParam(), or object
property access that may return undefined rather than null. Revert
those specific cases while keeping === null where variables are
explicitly initialized to null.
https://claude.ai/code/session_01SbuDigvAkYvPKvdcougsdo
* Replace deprecated jQuery methods in all theme main.js files
Replace .unbind().click() with .off('click').on('click'), convert
.hover() to .on('mouseenter').on('mouseleave'), replace .change(),
.scroll(), .click() shorthands with .on() equivalents, and .blur()
with .trigger('blur') across all 10 theme files.
https://claude.ai/code/session_01SbuDigvAkYvPKvdcougsdo
* Convert string concatenation to template literals
Replace 'str' + var + 'str' patterns with ES6 template literals
in realtime.js and install.js. Improves readability especially for
URL construction and HTML building. Also replace $.parseJSON() with
native JSON.parse() in realtime.js.
https://claude.ai/code/session_01SbuDigvAkYvPKvdcougsdo
* Convert var to const for single-assignment variables
Replace var with const where the variable is assigned once and never
reassigned within its scope, in install.js and realtime.js. Keeps var
for variables that are conditionally reassigned (e.g. size, url).
https://claude.ai/code/session_01SbuDigvAkYvPKvdcougsdo
---------
Co-authored-by: Claude <noreply@anthropic.com>
Add cactiEscapeHtml() utility and fix 8 DOM XSS findings where text read from the DOM was reinterpreted as HTML without escaping. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Fix 2 DOM XSS findings: breadcrumb category/rubric text and column title text were inserted as HTML without escaping. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
Reject phar://, php://filter, expect://, data://, glob://, ssh2:// before prepending compress.zlib://. Prevents nested wrapper injection that could lead to deserialization or information disclosure on older PHP runtimes. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Strip non-alphanumeric/underscore characters from column keys before backtick-quoting in INSERT/UPDATE construction. Addresses issue Cacti#7028. Current callers use hardcoded keys but the sink was unprotected. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Forward-port of the 1.2.x security-architecture hardening work into the mainline codebase, plus additional DOM XSS/JS fixes, CI/static-analysis additions, and related defense-in-depth changes across PHP, JS, shell tooling, and localization.
Changes:
- Adds new centralized security gateway libraries (exec/http/request/dispatch/safe file writes/plugin path) and accompanying Pest source-scan tests.
- Updates multiple UI/JS surfaces to address DOM XSS findings and JS runtime errors.
- Adjusts CI/tooling (PHPStan workflow, pre-commit hooks, CodeQL/dependabot configs) and applies broad hardening edits across core PHP pages.
Reviewed changes
Copilot reviewed 129 out of 138 changed files in this pull request and generated 25 comments.
Show a summary per file
| File | Description |
|---|---|
| user_group_admin.php | Replaces str_contains usage with strpos checks when parsing realm display strings. |
| user_admin.php | Same realm parsing change as user_group_admin.php. |
| tree.php | JS tree editing behavior adjustments; renames drag helper; minor text change. |
| tests/tools/install_packages.sh | Alters template import loop implementation. |
| tests/tools/copyright_year.sh | Modifies exclusion lists/arg parsing for copyright scanning. |
| tests/tools/check_docker.sh | Updates required PHP extensions list for docker checks. |
| tests/tools/check_cli_version.sh | Adjusts CLI file discovery and webuser detection logic. |
| tests/tools/check_all_pages.sh | Large refactor of shellcheck suppressions/quoting/logic in recursive page checker. |
| tests/Unit/SecurityArchitectureTest.php | New: source-scan tests validating presence of security helper patterns. |
| tests/Unit/GraphInvalidLocalGraphIdTest.php | New: regression tests for db_fetch_row guard behavior in graph.php. |
| tests/Unit/DefenseInDepthFollowupTest.php | New: additional defense-in-depth regression/source checks. |
| tests/Unit/DbFetchRowGuardTest.php | New: scans multiple files to ensure db_fetch_row results are guarded before deref. |
| tests/Unit/CsrfHardeningTest.php | New: source checks for CSRF hardening behaviors. |
| scripts/ss_webseer.php | Changes return semantics for “unknown” values. |
| scripts/ss_gexport.php | Changes return semantics for “unknown” values. |
| scripts/ss_apache_stats.php | Edits header comment text. |
| scripts/query_host_cpu.php | Removes explicit “U” output on missing index. |
| scripts/diskfree.sh | Changes argument quoting behavior. |
| remote_agent.php | Removes reverse-DNS forward-confirmation and relaxes SNMP OID validation. |
| poller_dsstats.php | Edits CLI help text. |
| poller_boost.php | Edits a comment. |
| poller_automation.php | Edits log message text. |
| poller.php | Moves poller heartbeat check function and adds notification logic for heartbeat status. |
| plugins.php | Alters server->JS variable emission for sorting/state and DnD logic. |
| phpstan.neon | New: PHPStan config focused on new gateway files. |
| phpstan-baseline.neon | Removes baseline entries. |
| permission_denied.php | Changes referer handling for “Return” link. |
| package_import.php | Alters SSL context behavior for direct URL repository fetches. |
| oauth2.php | Switches request handling to direct $_GET for OAuth code/state flow. |
| locales/po/zh-CN.po | Updates percent-escaping and translation strings. |
| locales/po/uk-UA.po | Adds translations for previously empty strings. |
| locales/po/ru-RU.po | Adds translations/metadata cleanup. |
| locales/po/ka-GE.po | Adjusts header formatting. |
| locales/po/fr-FR.po | Adds translations and fuzzy markers/metadata cleanup. |
| locales/po/bg-BG.po | Adds translations for warning strings. |
| link.php | Changes referer loop prevention and session storage behavior. |
| lib/utility.php | Edits recommendation strings and doc comments. |
| lib/time.php | Replaces str_contains with strpos logic in month_shift. |
| lib/snmp.php | Modifies OID validation logic. |
| lib/rrdcheck.php | Edits doc comment text. |
| lib/rrd.php | Significant changes: IPv6 proxy socket handling; command-building/escaping changes; theme-path handling; graph logic tweaks. |
| lib/reports.php | Comment/doc string edits. |
| lib/poller.php | Changes exec_background signature/escaping behavior and poller output parsing. |
| lib/plugins.php | Changes include/require behavior for plugin hook files and install/uninstall flows. |
| lib/ping.php | Changes IPv6 detection logic for ping variants. |
| lib/installer.php | Alters default mode handling and doc spelling. |
| lib/html_utility.php | Changes ORDER BY string construction and removes validate_redirect_url implementation. |
| lib/html_graph.php | Alters PHP->JS emission and removes graph_list sanitization helper. |
| lib/html_filter.php | Changes rendered form class attribute. |
| lib/html.php | Alters auth footer error rendering behavior. |
| lib/functions.php | Reworks sanitize_uri and loosens some typing/guard logic. |
| lib/dbparallel.php | Comment text edit. |
| lib/data_query.php | Log message grammar edit. |
| lib/cacti_safe_write.php | New: safe file write helper enforcing allowed base dirs. |
| lib/cacti_request.php | New: request helper functions for sorting/IDs/LIKE escaping. |
| lib/cacti_plugin_path.php | New: safe plugin-path resolver. |
| lib/cacti_http.php | New: SSRF-safe-ish HTTP fetch helper with reserved-IP rejection. |
| lib/cacti_exec.php | New: centralized shell execution gateway with redaction and timeout support. |
| lib/cacti_dispatch.php | New: centralized action dispatcher with method/realm/ACL enforcement. |
| lib/auth.php | Comment/doc spelling edits. |
| lib/api_device.php | Removes replicate_out hook invocation; changes SNMP down-detection logic. |
| lib/api_aggregate.php | Adjusts aggregate total-type behavior and comments. |
| lib/aggregate.php | Reworks totalling SQL logic and logging; changes to prepared-vs-raw SQL usage. |
| install/upgrades/1_2_19.php | Edits upgrade SQL string content. |
| install/upgrades/1_1_20.php | Comment spelling edit. |
| install/upgrades/1_1_11.php | Comment spelling edit. |
| install/upgrades/1_0_0.php | Adds/edits upgrade logic and comment spelling. |
| include/themes/sunrise/main.js | Modernizes event binding (off/on) and hover handlers. |
| include/themes/raspberry/main.js | Same jQuery event binding modernization. |
| include/themes/paw/main.js | Same jQuery event binding modernization. |
| include/themes/paper-plane/main.js | Same jQuery event binding modernization. |
| include/themes/modern/main.js | Same jQuery event binding modernization. |
| include/themes/hollyberry/main.js | Same jQuery event binding modernization. |
| include/themes/dark/main.js | Same jQuery event binding modernization. |
| include/themes/carrot/main.js | Same jQuery event binding modernization. |
| include/themes/cacti/main.js | Same jQuery event binding modernization. |
| include/realtime.js | Refactors string building and equality checks in realtime graph JS. |
| include/js/navigationTree.jstree.js | New: midwinter navigation tree JS implementation. |
| include/global_settings.php | Comment and description string edits. |
| include/global_languages.php | Switches from request helper usage to direct $_REQUEST mutation. |
| include/global_form.php | Comment and description string edits. |
| include/global_arrays.php | Message text edits. |
| include/global.php | Switches many require_once calls to include_once and tweaks request checks. |
| include/css/jquery.contextMenu.css | Removes tabler icon pseudo-element styling. |
| include/auth.php | Adjusts unauthenticated-page exemptions and referer redirect handling. |
| graphs_new.php | Changes referer/returnto handling logic and parsing. |
| graphs.php | Comment spelling edit. |
| docs/csrf-migration.md | New: CSRF migration plan documentation. |
| data_templates.php | Edits UI footer text. |
| data_sources.php | Whitespace-only SQL string edits; changes html_start_box title argument. |
| data_queries.php | Switches move_item_* calls to string WHERE clauses; comment grammar edit. |
| data_debug.php | Changes escaping behavior for displayed values. |
| composer.json | Adjusts oauth2-keycloak version constraint and Composer script aliases. |
| color_templates_items.php | New: color template item admin page/actions. |
| cmd_realtime.php | Changes pipe-close guard logic. |
| cli/sqltable_to_php.php | Help text edits. |
| cli/splice_rrd.php | Comment case/spelling edit. |
| cli/audit_database.php | Switches require_once to include_once for plugin setup includes. |
| cli/add_site.php | Renames help/version functions; refactors some output and error handling. |
| cactid.php | Comment spelling edit. |
| automation_templates.php | String literal changes in descriptions (quote escaping). |
| auth_resetpassword.php | Changes JS onClick URL emission from json_encode to raw interpolation. |
| auth_profile.php | Changes tab handling and referer persistence; changes JS currentTab emission. |
| auth_changepassword.php | Refactors unauthenticated redirect and return URL handling. |
| aggregate_graphs.php | Refactors referer handling and changes error message text. |
| about.php | Edits HTML markup/structure in developer/thanks sections. |
| CHANGELOG | Removes a changelog line. |
| .pre-commit-config.yaml | Refactors hook entrypoints and file-matching patterns. |
| .github/workflows/syntax.yml | CI workflow edits (action versions/commands/quoting). |
| .github/workflows/phpstan.yml | New: PHPStan workflow. |
| .github/workflows/codeql.yml | New: CodeQL workflow definition. |
| .github/dependabot.yml | New: dependabot configuration. |
| include/themes/midwinter/main.css | Removes imports and extra whitespace. |
| include/themes/midwinter/default/style.css | Adjusts jsTree styling and transitions/shadows. |
| include/themes/midwinter/css/pre/helper.css | Quote style change for font-family. |
| include/themes/midwinter/css/pre/colors.css | Removes disabled text color variables. |
| include/themes/midwinter/css/media/core.css | Removes/adjusts multiple style blocks and comments. |
| include/themes/midwinter/css/media/compact-landscape.css | Enables/adjusts several previously commented style blocks and tooltip behavior. |
Comments suppressed due to low confidence (4)
lib/rrd.php:3005
- graph_theme is used directly to build a filesystem path that is then included (
/themes/{$graph_theme}/rrdtheme.php) without basename/containment checks. Because graph_theme can be influenced by request variables (e.g., remote_agent graph_theme), this can become a local file include/path traversal vector. Please restore basename + allowlist validation (or reuse cacti_path_is_within) before including theme files.
remote_agent.php:273 - OID validation for the remote_agent SNMP helpers has been removed. Since
oidis request-controlled and is passed into SNMP calls, please restore a strict OID format check (e.g.,cacti_snmp_validate_oid()for numeric OIDs) and return a safe'U'on rejection to avoid abuse and unexpected SNMP library behavior.
data_debug.php:818 - $value_title is now assigned from raw
$valueand later used as a cell title/tooltip. If $value contains HTML/metacharacters, this can lead to XSS in the debug UI. Please keep escaping for tooltip/title contexts (e.g., htmle((string)$value)) and only use raw values when guaranteed safe.
$value_title = $value;
if (strlen($value) > 100) {
$value = substr($value, 0, 100);
}
form_selectable_ecell($value, $i, '', '', $value_title);
form_selectable_cell($icon, $i);
link.php:53
$refereris set from raw HTTP_REFERER and later used inheader('Location: ' . $referer)without a same-origin validation. This reintroduces an open redirect and can be abused via a crafted Referer header. Please restore validate_redirect_url() semantics (same-host + scheme rejection) before persisting or redirecting to referers.
Comment on lines
1353
to
1366
| function get_order_string() : string { | ||
| $page = get_order_string_page(true); | ||
| $sort_column = get_nfilter_request_var('sort_column'); | ||
| $sort_dir = strtoupper(get_nfilter_request_var('sort_direction')); | ||
| $page = get_order_string_page(); | ||
|
|
||
| if ($sort_dir !== 'ASC' && $sort_dir !== 'DESC') { | ||
| $sort_dir = 'ASC'; | ||
| } | ||
|
|
||
| if (strpos($sort_column, '(') === false && strpos($sort_column, '`') === false) { | ||
| if (!str_contains(get_request_var('sort_column'), '(') && !str_contains(get_request_var('sort_column'), '`')) { | ||
| $del = '`'; | ||
| } else { | ||
| $del = ''; | ||
| } | ||
|
|
||
| /** | ||
| * Allowlist: identifiers are word chars and dots only; anything else could escape | ||
| * backtick quoting. Validate direction to prevent SQL keyword injection. | ||
| */ | ||
| if (!preg_match('/^[a-zA-Z][a-zA-Z0-9_.()]*$/', $sort_column)) { | ||
| $sort_column = ''; | ||
| } | ||
|
|
||
| if (isset($_SESSION['sort_string'][$page])) { | ||
| return $_SESSION['sort_string'][$page]; | ||
| } else { | ||
| return 'ORDER BY ' . $del . implode($del . '.' . $del, explode('.', get_request_var('sort_column'))) . $del . ' ' . get_request_var('sort_direction'); | ||
| } |
There was a problem hiding this comment.
get_order_string() now builds an ORDER BY clause directly from request variables (sort_column/sort_direction) without the prior allowlist/regex validation. Because the column name and direction are concatenated into SQL (even with backticks), this reintroduces SQL injection risk and allows invalid direction keywords. Please restore strict validation (identifier regex/allowlist) and normalize direction to ASC/DESC before building the session sort string / ORDER BY clause.
Contributor
Author
There was a problem hiding this comment.
Valid concern — get_order_string() does need allowlist/regex validation restored. Tracking as follow-up.
Comment on lines
+3
to
5
| for file in $(ls -1 install/templates/*.gz);do | ||
| php cli/import_package.php --filename=$file | ||
| done |
There was a problem hiding this comment.
This loop uses command substitution over ls and passes $file unquoted. That breaks on filenames with spaces/newlines and can cause unexpected behavior. Please revert to a plain glob loop (for file in install/templates/*.gz; do) and keep $file quoted when passing it to PHP.
Contributor
Author
There was a problem hiding this comment.
Valid — will restore quoting and replace ls with find -print0 or array expansion.
Comment on lines
686
to
709
| function api_plugin_install(string $plugin) : bool { | ||
| if (!defined('IN_CACTI_INSTALL')) { | ||
| define('IN_CACTI_INSTALL', 1); | ||
| define('IN_PLUGIN_INSTALL', 1); | ||
| } | ||
|
|
||
| $message = ''; | ||
|
|
||
| $dependencies = api_plugin_get_dependencies($plugin); | ||
|
|
||
| $proceed = api_plugin_can_install($plugin, $message); | ||
|
|
||
| if (!$proceed) { | ||
| $message .= '<br><br>' . __('Plugin cannot be installed.'); | ||
|
|
||
| raise_message('dependency_check', $message, MESSAGE_LEVEL_ERROR); | ||
|
|
||
| header('Location: plugins.php'); | ||
|
|
||
| exit; | ||
| } | ||
|
|
||
| if (!file_exists(CACTI_PATH_PLUGINS . "/$plugin/setup.php")) { | ||
| cacti_log('ERROR: Plugin \'' . preg_replace('/[^a-zA-Z0-9_\-]/', '', $plugin) . '\' setup.php not found, cannot install', false, 'PLUGIN'); | ||
| raise_message('plugin_missing', __('Plugin setup file not found.'), MESSAGE_LEVEL_ERROR); | ||
| header('Location: plugins.php'); | ||
| exit; | ||
| } | ||
|
|
||
| require_once(CACTI_PATH_PLUGINS . "/$plugin/setup.php"); | ||
| include_once(CACTI_PATH_PLUGINS . "/$plugin/setup.php"); | ||
|
|
There was a problem hiding this comment.
api_plugin_install() now unconditionally include_once's the plugin setup.php without checking for existence and without the previous error handling/redirect. If setup.php is missing or unreadable, this will emit warnings and continue with partially-initialized state. Please restore the explicit file_exists check and the user-facing error/redirect path before including.
Contributor
Author
There was a problem hiding this comment.
Valid — the file_exists guard and error redirect should be restored.
Comment on lines
+1013
to
+1026
| $referer_url = parse_url($_SERVER['HTTP_REFERER']); | ||
|
|
||
| if ($_SERVER['SERVER_NAME'] != $referer_url['host']) { | ||
| // Potential security exploit 1 | ||
| srv('returnto', 'host.php'); | ||
| } elseif (!str_contains($_SERVER['HTTP_REFERER'], 'graphs_new')) { | ||
| srv('returnto', basename($_SERVER['HTTP_REFERER'])); | ||
| } else { | ||
| srv('returnto', 'host.php'); | ||
| } | ||
| } elseif (isrv('returnto') && gnrv('returnto') != '') { | ||
| $returnto_url = parse_url(gnrv('returnto')); | ||
|
|
||
| if ($_SERVER['SERVER_NAME'] != $returnto_url['host']) { |
There was a problem hiding this comment.
graphs_new.php now uses parse_url($_SERVER['HTTP_REFERER']) and then immediately reads $referer_url['host'] without checking that parse_url() succeeded or that host exists. This can trigger warnings and also mis-handle relative referers. Please guard for false/missing host and use validate_redirect_url() (or same-origin checks) before storing returnto; also avoid basename() on full URLs since it retains query strings.
Suggested change
| $referer_url = parse_url($_SERVER['HTTP_REFERER']); | |
| if ($_SERVER['SERVER_NAME'] != $referer_url['host']) { | |
| // Potential security exploit 1 | |
| srv('returnto', 'host.php'); | |
| } elseif (!str_contains($_SERVER['HTTP_REFERER'], 'graphs_new')) { | |
| srv('returnto', basename($_SERVER['HTTP_REFERER'])); | |
| } else { | |
| srv('returnto', 'host.php'); | |
| } | |
| } elseif (isrv('returnto') && gnrv('returnto') != '') { | |
| $returnto_url = parse_url(gnrv('returnto')); | |
| if ($_SERVER['SERVER_NAME'] != $returnto_url['host']) { | |
| $referer_url = parse_url($_SERVER['HTTP_REFERER']); | |
| $referer_host = (is_array($referer_url) && isset($referer_url['host']) ? $referer_url['host'] : ''); | |
| $referer_path = (is_array($referer_url) && isset($referer_url['path']) ? $referer_url['path'] : ''); | |
| $referer_query = (is_array($referer_url) && isset($referer_url['query']) && $referer_url['query'] != '' ? '?' . $referer_url['query'] : ''); | |
| $referer_returnto = ltrim($referer_path, '/\\') . $referer_query; | |
| if ($referer_host != '' && $_SERVER['SERVER_NAME'] != $referer_host) { | |
| // Potential security exploit 1 | |
| srv('returnto', 'host.php'); | |
| } elseif ($referer_path != '' && !str_contains($referer_path, 'graphs_new') && validate_redirect_url($referer_returnto)) { | |
| srv('returnto', $referer_returnto); | |
| } else { | |
| srv('returnto', 'host.php'); | |
| } | |
| } elseif (isrv('returnto') && gnrv('returnto') != '') { | |
| $returnto_url = parse_url(gnrv('returnto')); | |
| $returnto_host = (is_array($returnto_url) && isset($returnto_url['host']) ? $returnto_url['host'] : ''); | |
| if (($returnto_host != '' && $_SERVER['SERVER_NAME'] != $returnto_host) || !validate_redirect_url(gnrv('returnto'))) { |
Contributor
Author
There was a problem hiding this comment.
Valid — will add parse_url success + host key presence checks.
Comment on lines
+143
to
145
| "-S" | "-S") | ||
| shift | ||
| EXC_FILES="$1" |
There was a problem hiding this comment.
The case pattern for the -s/-S option is broken ("-S" | "-S") so the lowercase -s flag will no longer work. Please restore the intended "-S" | "-s" pattern.
Contributor
Author
There was a problem hiding this comment.
Fixed — will restore the -S | -s pattern.
| # +-------------------------------------------------------------------------+ | ||
|
|
||
| df -k "$1" | grep -v Filesystem| awk '{printf "megabytes:" $4 " percent:" int($5)}' | ||
| df -k $1 | grep -v Filesystem| awk '{printf "megabytes:" $4 " percent:" int($5)}' |
There was a problem hiding this comment.
The script now expands $1 unquoted. This will break paths containing spaces and can allow unintended option injection into df if $1 begins with -. Please restore quoting (df -k "$1") and consider using -- before the path to stop option parsing.
Contributor
Author
There was a problem hiding this comment.
Fixed — restore double-quote around $1.
Comment on lines
+107
to
+116
| $host = $parsed['host']; | ||
|
|
||
| /* resolve hostname and check against reserved ranges */ | ||
| $ips = gethostbynamel($host); | ||
|
|
||
| if ($ips === false) { | ||
| cacti_log('ERROR: cacti_http_fetch: DNS resolution failed for ' . $host, false, 'SYSTEM'); | ||
|
|
||
| return false; | ||
| } |
There was a problem hiding this comment.
cacti_http_fetch() uses gethostbynamel(), which only resolves IPv4 A records and will miss IPv6 AAAA records (and also won’t handle IP-literal hosts). This weakens reserved-IP SSRF protection for IPv6. Please resolve both A and AAAA (e.g., dns_get_record) and validate IP-literal hosts directly with cacti_is_reserved_ip().
Contributor
Author
There was a problem hiding this comment.
Valid — dns_get_record A+AAAA + IP-literal handling tracked.
| refreshFunction = 'refreshGraphs()'; | ||
|
|
||
| var graph_list_array = <?php print json_encode($graph_list_js); ?>; | ||
| var graph_list_array = new Array(<?php print grv('graph_list'); ?>); |
There was a problem hiding this comment.
graph_list_array is constructed from raw grv('graph_list') inside JavaScript (new Array(...)). Since graph_list is request-controlled, this allows arbitrary JS injection (e.g. graph_list=1);alert(1);//). Please keep the previous server-side sanitization and emit the array via json_encode.
Contributor
Author
There was a problem hiding this comment.
Valid and important — raw grv() in JS array literal is XSS. Will json_encode().
Comment on lines
4
to
+22
| - id: lintcheck | ||
| name: PHP Lint Checking | ||
| entry: sh tests/tools/pre-commit-check.sh lint | ||
| entry: composer run-script lint | ||
| language: system | ||
| types: [php] | ||
| files: ^app/ | ||
| always_run: true | ||
|
|
||
| - id: phpcsfixer | ||
| name: PHP CS Fixer Validation | ||
| entry: sh tests/tools/pre-commit-check.sh phpcsfixer | ||
| entry: composer run-script phpcsfixer | ||
| language: system | ||
| types: [php] | ||
| files: ^app/ | ||
| always_run: true | ||
|
|
||
| - id: phpstan | ||
| name: PHPStan Validation | ||
| entry: sh tests/tools/pre-commit-check.sh phpstan | ||
| entry: composer run-script phpstan | ||
| language: system | ||
| types: [php] | ||
| files: ^app/ |
There was a problem hiding this comment.
The pre-commit hooks are restricted to files: ^app/, but this repository’s PHP sources are primarily at the repo root and in lib/, include/, etc. As a result, these checks won’t run on most changes. Please adjust the files: regex (or use types: [php]) so lint/phpcsfixer/phpstan run on the actual codebase paths.
Contributor
Author
There was a problem hiding this comment.
Acknowledged — pre-commit config needs path scope updated to match PHP source layout.
Comment on lines
92
to
+110
| @@ -99,15 +99,15 @@ | |||
| // Check given state against previously stored one to mitigate CSRF attack | |||
| } | |||
|
|
|||
| if (isempty_request_var('state') || (isset($_SESSION['oauth2state']) && (grv('state') !== $_SESSION['oauth2state']))) { | |||
| if (empty($_GET['state']) || (isset($_SESSION['oauth2state']) && ($_GET['state'] !== $_SESSION['oauth2state']))) { | |||
| unset($_SESSION['oauth2state']); | |||
|
|
|||
| exit('Invalid state'); | |||
| } else { // Try to get an access token (using the authorization code grant) | |||
| $token = $provider->getAccessToken( | |||
| 'authorization_code', | |||
| [ | |||
| 'code' => grv('code') | |||
| 'code' => $_GET['code'] | |||
There was a problem hiding this comment.
This page now reads OAuth2 parameters directly from $_GET, bypassing the request helper layer (gfrv/grv) used throughout the codebase for filtering/caching. Please revert to request helpers (with appropriate validation) for code and state to keep consistent input handling and reduce the risk of unexpected types/encodings.
Contributor
Author
There was a problem hiding this comment.
Valid — switch to gfrv()/grv() to match codebase conventions.
… temp, exec_redirect, csrf, db_in Tier 1 (audit-derived): - cacti_path_is_within: realpath containment check (replaces 3 duplicated blocks) - cacti_remote_url: URL builder with rawurlencode (eliminates HPP) - cacti_redirect: validated redirect with forced exit (eliminates open-redirect class) - cacti_fetch_by_id: safe single-row fetch with intval + cacti_sizeof (eliminates undefined-key class) - cacti_redact_sensitive: recursive array redaction for log/debug output - cacti_temp_file: RAII-style temp file with guaranteed unlink in finally Tier 2 (built on Tier 1): - cacti_exec_with_redirect: proc_open with file descriptors instead of shell > redirect - cacti_csrf_rotate: session regen + cookie rotation + audit log at privilege boundaries - cacti_db_in: safe IN-clause builder with intval cast Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
10 raw RLIKE string-concat sites in graph_view.php, data_debug.php, graphs.php, and data_sources.php replaced with db_qstr_rlike() which uses PDO quote() for proper escaping. Addresses GHSA-69gg (critical pre-auth SQLi) and GHSA-gp82 (pre-auth SQLi via FILTER_VALIDATE_REGEXP). graph_image.php and graph_json.php graph_theme parameter now validated with basename() + is_dir() against installed themes directory. Addresses GHSA-cx5r (pre-auth LFI via graph_theme). Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
graph_name_regexp and search_key from stored DB values were concatenated into REGEXP clauses without escaping. All 5 sites now use db_qstr_rlike() for proper PDO quoting. Addresses GHSA-xrh3 (stored SQLi via graph_name_regexp). Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
… LDAP injection, param pollution, TLS, ORDER BY, lockout bypass Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
…ion to http context Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
… null check Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
…, hi-IN, it-IT, ja-JP, ko-KR, lv-LV, pl-PL, ru-RU, uk-UA Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
…acti_validate_sort_column Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
…lidation.php, remove duplicate helper Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
… duplicate Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
…ggregate and reports Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
… failures Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
…xport, query_host_cpu Local branch diverged from upstream before PR Cacti#6941 was merged. Restore the scripts from upstream/develop so DatasourceScriptErrorReturnTest passes. 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>
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.
Forward-port of #7054 (architectural security helpers) plus DOM XSS fixes and JS error corrections.
35 commits covering: cacti_exec, cacti_http_fetch, cacti_ldap_filter, cacti_safe_write, cacti_plugin_path, cacti_dispatch, cacti_auth_transition, typed request helpers, db_fetch_row guards, CSRF hardening, CSP/HSTS, DOM XSS escaping, JS null-deref fixes, SHA-256 enforcement, PHPStan CI.
Consolidates and supersedes: #7049, #7053.
Signed-off-by: Thomas Vincent thomasvincent@gmail.com