616 Commits
Author SHA1 Message Date
Thomas Vincent 2761fdc226 fix(rrd): [develop] retain source identity after CF fallback (#8339)
* fix(rrd): [develop] retain source identity after CF fallback

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

* test: measure native consolidation fallback coverage

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

---------

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
2026-10-23 10:37:54 -06:00
1997691de0 Render RRDtool Tune Info as balanced div boxes and POST the Data Source repair actions (#8412)
* data_sources: render RRDtool Tune Info as balanced div boxes

rrdtool_tune() emitted an unbalanced mix of html_header() and
html_end_box() calls wrapped around raw table rows inside the single
box opened by data_sources.php.  The premature and extra closing tags
corrupted the DOM, which (depending on theme) clipped the Save button
at the bottom of the Data Source edit page while in Data Source Info
mode.

- Separate the execute, CLI and HTML code paths in rrdtool_tune() and,
  for HTML, render each section as a self contained, balanced div box.
- Extract and rename the helper print_leaves() to the standalone
  rrdtool_tune_print_leaves(), matching the rrdtool_tune_* naming
  convention, and emit div rows instead of table rows.
- Switch the data_sources.php wrapper box to div mode to match.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* data_debug: require POST for the Data Source repair actions

Applying RRDfile repairs and rerunning the debugger are state changing
operations, so they should not be reachable with a GET.  Driving them
through a GET also meant the follow up redirect was pulled into the
content area as a full page, losing the theme and navigation on
rerender.

Register data_debug.php run_repair and run_debug in the
cactiIsPostActionUrl() allowlist so loadUrl() upgrades them to a
token-bearing POST, and add the matching csrf_require_post() guard so
the browser transport and server side guard stay in sync.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* rrd: split rrdtool_tune into testable render and execute helpers

Break rrdtool_tune() into small single-purpose helpers so each path can
be exercised directly by unit tests (the web display path is otherwise
unreachable under CACTI_CLI):

- rrdtool_tune_print_leaves() takes an explicit $html flag instead of
  branching on CACTI_CLI, so both the div and plain-text branches are
  testable.
- rrdtool_tune_execute() applies the tune/resize commands for a repair.
- rrdtool_tune_print_cli() prints the source commands as plain text.
- rrdtool_tune_print_html() renders the balanced div boxes.
- rrdtool_tune() becomes a thin dispatcher over the three.

Add tests/Unit/RrdTuneRenderTest.php covering the leaf printer, the HTML
and CLI printers, the execute path and the dispatcher, asserting the
output is balanced div markup with no table rows.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* review: post Data Source Troubleshooter actions via cactiPostAction

Address PR review feedback:

- Switch the Apply Suggested Fixes and Rerun Analysis links to the
  existing cactiPostAction/data-url idiom (as data_sources.php already
  does for ds_repair) instead of upgrading a loadUrl() GET through the
  cactiIsPostActionUrl allowlist. The allowlist path replayed the
  query-free mutation URL into browser history, so a refresh reopened
  the list instead of the selected Data Source; a full cactiPostAction
  submission follows the server redirect to the read-only view URL and
  records that in history. The run_repair / run_debug csrf_require_post
  guards are retained, and the data_debug.php allowlist entry (and its
  now redundant click handlers) are removed.

- Add a callable $executor test seam to rrdtool_tune_execute() (matching
  rrdtool_dump_document) so the unit tests assert the tune and resize
  commands are actually executed, with resize.rrd produced only when the
  resize command runs, rather than passing on the rename alone.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* review: move RrdTuneRenderTest into the Unit/Core/Rrd category folder

Per AGENTS.md, new unit tests belong in a category subfolder rather than
directly under tests/Unit. Move RrdTuneRenderTest there and update the
UnitStubs include depth accordingly.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

---------

Co-authored-by: TheWitness <copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
2026-10-16 11:23:07 -04:00
Thomas Vincent 3bcb4e4f49 fix(graphs): [develop] clear empty query fields in rendered text (#8349)
* fix(graphs): clear empty query fields in rendered text

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

* ci(coverage): enforce changed PHP statement coverage

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

* ci: share changed-code coverage contracts and enforce shell policy

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

* fix(ci): run request coverage with portable path matching

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

* ci: retain bootstrap coverage and validate request dispatch

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

* test: synchronize native coverage fixture dispatch

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

* test: measure coverage at the exact PR revision

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

---------

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
2026-10-15 20:38:03 -04:00
32467ddcab perf: reuse graph option arrays and document validation results (#8361)
* perf: reuse bootstrapped graph option arrays and document validation

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

* test: record the installed RRDtool version in profiles

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

* docs: record final validation scope and profiling runtime limits

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

* docs: clarify the scope of changed-code coverage

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

* ci: repair relocated PHPUnit configs and the deletion-only coverage gate

- Point the five relocated configs that still carried a contributor's local
  autoload path (#8376) at ../include/vendor/autoload.php.
- Pass --configuration=tests/phpunit.xml to the CSV window workflow and the
  README example now that phpunit.xml no longer lives in the repository root.
- Let the changed-lines gate pass deletion-only PHP changes. Without added
  lines the generated config has no source filter, PHPUnit writes no clover
  report, and the gate failed on the missing file instead of reporting zero
  executable changed lines.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: TheWitness <thewitness@cacti.net>

* ci: keep the missing request-report check for deletion-only changes

Only short-circuit the changed-lines gate when there are no added PHP lines
and PHPUnit wrote no clover report, so an empty request-coverage directory
is still rejected.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: TheWitness <thewitness@cacti.net>

---------

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: TheWitness <thewitness@cacti.net>
Co-authored-by: TheWitness <thewitness@cacti.net>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
2026-10-14 22:45:42 -04:00
TheWitness 75f92c2d86 Detect and repair RRDfile Data Source count mismatches via RRDtune (#8368)
* Detect and repair RRDfile Data Source count mismatches via RRDtune

Report when a Graph requires more or fewer Data Sources than its RRDfile
contains, generate the rrdtool tune DS:/DEL: command to correct it, and run
it when the RRDfile is writable.  Applies to data_debug (rrdtool_cacti_compare)
and the RRDchecker batch process (do_rrdcheck).  The decision logic lives in
pure helpers (rrdtool_cacti_compare_ds, rrdcheck_ds_findings) with unit tests.

Resolves #8366

* rrdcheck: only probe local writability for local storage

Under the storage proxy the RRDfile is remote, so is_resource_writable()
checks the wrong filesystem.  Skip the local probe when using the proxy and
let it enforce remote permissions and report failures via its response.

* rrdcheck: validate tune args, enforce RRA containment, fix failure detection

Validate every tune specification against RRDtool's documented DS grammar
(ds-name 1-19 [a-zA-Z0-9_], known DST, integer heartbeat, numeric-or-U min/max)
and escape it before use.  For local storage require the RRDfile to resolve
inside the RRA directory via data_source_path_within_rra().  Detect failures
from the real return value (false/null/empty or missing OK) instead of casting
to string, so a failed tune is no longer logged as success.

* rrd: normalize a 0/0 data source range to 0:U when building a tune add spec

rrdtool_function_info_from_ds() passes min/max through unchanged, so a source
stored as 0/0 produced DS:name:type:hb:0:0 which RRDtool rejects.  Mirror RRD
creation and emit 0:U so the repair paths can restore those sources.
2026-10-14 20:55:52 -04:00
Thomas Vincent 4d11070ced feat: expose RRDtool 1.11 graph axis controls (#8356)
* feat: expose RRDtool 1.11 graph axis controls

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

* fix: constrain axis template inheritance checkboxes

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

* test(rrd): exercise native aggregate validation and enforce coverage

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

* ci: share changed-code coverage contracts and enforce shell policy

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

* fix(ci): run request coverage with portable path matching

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

* ci: retain bootstrap coverage and validate request dispatch

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

* test: synchronize native coverage fixture dispatch

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

* test: measure coverage at the exact PR revision

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

---------

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
2026-10-14 12:10:28 -04:00
Thomas Vincent a12fe59627 fix(poller): [develop] retain output until bounded storage batches succeed (#8332)
* fix(poller): retain output until bounded storage batches succeed

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

* test(poller): refresh SQL audit after exact-key acknowledgements

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

* test(boost): preserve handoff contracts with explicit acknowledgements

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

* fix(poller): retain failed timestamp ordering across batches

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

* fix(poller): acknowledge complete timestamp groups together

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

* fix(poller): retry failed source metadata reads on develop

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

* fix(poller): preload standalone script exclusions once per process

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

* fix(poller): bound metadata failures and validate acknowledgement queries

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

---------

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
2026-10-12 15:41:53 -07:00
TheWitnessandci 9facd9b500 security: batch 7 develop - consolidate remaining develop-only GHSA advisories (12 fixes + 8 confirmations) (#8149)
* security: batch 7 develop - consolidate remaining develop-only GHSA advisories (12 fixes + 8 regression confirmations)

Fixes: f7jw (automation preview XSS), rp5g (db_column_exists LIKE), vx2m (automation pivot CASE WHEN), h5wg (plugin DDL identifiers), 929h (aggregate LIKE filter), mfw8 (add_graphs REGEXP), m49v (user_admin sort_column allowlist), mpfm (rrd error-image theme allowlist), m3fh (SNMPv3 control chars), cg45 (remember-me row revoke), xq26 (remember-me retention window), m67r (import xml_path containment).

Confirmed already fixed on develop, pinned with regression tests: 4gph, 977w, mgcq, 5p79, cfhh, wfcf, pjq5, wp33.

Excluded: 227p (1.2.x-only), mx5c (maintainer private branch).

* security: address batch-7 review feedback (lexical-first xml_path containment, session-wide sort allowlist, connection-aware db_column_exists quoting, reject-not-normalize plugin DDL identifiers, behavior-based test names)

* style: apply php-cs-fixer single-quote fix to api_automation pivot builders; add WSL dev-environment note to copilot instructions

* fix phpstan string typing

* test: refresh SQL interpolation baseline for db_column_exists db_qstr change (database.php 9->8)

---------

Co-authored-by: ci <ci@local>
2026-10-02 15:27:34 -04:00
TheWitnessandTheWitness 3ab4c69fb7 PHPStan Level 8 - Phase 1 Conversion (#8135)
* PHPStan Level 8 - Phase 1 Conversion

Reduces the real (no-baseline) PHPStan level 8 error count from ~2,724 to 425
and regenerates phpstan-baseline.neon accordingly (8,941 -> 1,921 lines).

Core type-safety changes
- lib/database.php: narrow db_fetch_row()/db_fetch_row_prepared() to return
  array (was bool|array) and db_fetch_assoc()/db_fetch_assoc_prepared() to
  return array (was mixed); the _prepared workers coerce the connection-failure
  path to an empty array so the declared type is honest. empty()/cacti_sizeof()
  checks are unaffected.
- lib/database.php: narrow $db_conn guards from is_object() to instanceof PDO in
  the transaction/quote/insert-id helpers so the PDO calls resolve.
- lib/mib_cache.php: the fluent MibCache methods mib()/object()/table()/row()
  now return self instead of a bare object so chained calls type-check.

Type-safety casts and guards (mechanical, behaviour-preserving)
- Cast string|false/|null builtin results feeding string parameters
  (json_decode, base64_encode, trim, htmle, preg_replace, explode, etc.).
- Cast int|string / int|false values feeding int id/pid parameters.
- Guard resource|false fopen()/popen() handles before fwrite/fclose/fread, and
  wrap foreach over array|false/list|false results with `?: []`.

Genuine bugs found and fixed
- cli/add_tree.php / lib/api_tree.php: site tree nodes could not be created via
  the CLI - the --site-id option was never parsed, $nodeTypes had no 'site'
  entry (passing null to api_tree_item_save(int $type) -> TypeError), and
  api_tree_item_save() never persisted site_id. All three fixed.
- tree.php: leaves_exist() is typed : int but returned db_fetch_assoc_prepared()
  (an array of rows) for a SELECT COUNT(*); now uses db_fetch_cell_prepared()
  and returns the scalar count.
- lib/mib_cache.php: $entries_per_object was a float (division / ceil) used as
  an array key (PHP 8.1 implicit float-to-int key deprecation); now cast to int.

Config
- .phpstan.neon: add notIdentical.alwaysTrue and booleanAnd.alwaysFalse to the
  existing always-true/false ignore family (same "PHPStan gets confused"
  dynamic-constant cause already covered by identical.alwaysTrue,
  notEqual.alwaysTrue, booleanAnd.leftAlwaysTrue, etc.).
- phpstan-baseline.neon: regenerated; the remaining 425 entries are the
  untouched long tail for Phase 2.

* Remove redundant @phpstan-ignore comments

Follow-up cleanup for the level 8 Phase 1 work. Removes 14 inline
@phpstan-ignore comments that no longer suppress any reported error:

- cli/repair_database.php: `@phpstan-ignore booleanAnd.alwaysFalse` is now
  covered by the global booleanAnd.alwaysFalse ignore added to .phpstan.neon.
- 13 others (host.php, include/global.php, include/session.php,
  lib/api_automation.php x2, lib/functions.php x2, lib/html_utility.php x2,
  lib/installer.php, lib/package.php, lib/reports.php, lib/rrd.php) no longer
  match any reported error after the type-safety fixes (e.g. the
  db_fetch_row_prepared() return-type narrowing removed the offset-assign
  errors the api_automation ignores were suppressing).

Verified with reportUnmatchedIgnoredErrors enabled: 0 unmatched ignores
remain and the full level 8 run still reports 0 errors.

* ci: satisfy validator ratchet and refresh security sink baselines

- lib/api_tree.php: give the new tree-item site_id a real numeric
  constraint ('^[-0-9]+$', matching html_reports.php) instead of an
  empty regex, so the validator constraint ratchet passes without
  bumping its baseline.
- tests/security/baselines: refresh the three sink/hotspot signatures
  whose code text changed from the (string) casts added in this branch
  (lib/poller.php setsid, cli/rrdresize.php json_encode, lib/import.php
  simplexml_load_string).

* ci(js): allow DOMPurify 3.4.16 and update sync_js pin test

The dependabot bump (#8134) moved package.json/package-lock.json to
DOMPurify 3.4.16 on develop but left tests/js/sync_js.test.mjs pinned to
3.4.15, so the PR merge commit fails the 'DOMPurify is pinned' contract.
Bring package.json/package-lock.json to 3.4.16 (matching develop) and
update the pin test to expect 3.4.16.

* test: update sql.php null-handling assertion for the (string) cast

This branch changed scripts/sql.php from trim(\ ?? '') to
trim((string) \) (the phpstan-safe form that coerces both null and
false from shell_exec()). Update SqlScriptsTest's null-return assertion
to match the new cast instead of the removed '?? ''.

* fix: address Copilot review - ownership IDs, fopen guards, mib_cache bound

- chown()/chgrp(): pass integer UID/GID from fileowner()/filegroup()
  (guarding the false lookup) instead of string-casting them, which made
  them resolve account/group names like '1001' and fail. Covers
  lib/api_device.php, lib/boost.php and poller_maintenance.php (x2).
- fopen() failure now short-circuits instead of reporting success:
  lib/package.php returns false, lib/clog_webapi.php and utilities.php
  render an error, and snmpagent_mibcachechild.php skips the cache write
  and sentinel unlink so a concurrent writer's lock is preserved.
- lib/mib_cache.php: keep the float quotient as the loop bound and cast
  only the array offset, so a non-even entries/columns split no longer
  truncates the loop and drops a cached row.

* revert: keep db_fetch_* returning false on error; baseline Level 8 instead

Per review, restoring the intentional fail-safe: db_fetch_row[_prepared]()
return bool|array and db_fetch_assoc[_prepared]() return mixed again, so a
genuine SQL error (not just a missing connection) is signalled as false and
the === false checks in cdef_is_in_use(), boost_process_* and aggregate.php
work as before. Dropped the is_array() ? : [] coercion.

Level 8 stays green by regenerating phpstan-baseline.neon to capture the
re-introduced bool|array/mixed call-site errors (the ~1,850 the narrowing
had eliminated). Tightening those callers to drop them from the baseline is
Phase 2 work.

---------

Co-authored-by: TheWitness <copilot@users.noreply.github.com>
2026-10-02 10:08:56 -04:00
Thomas VincentandTheWitness d3d99bb08a test(cdef): enforce resolver coverage (#7974)
* test(cdef): enforce resolver coverage

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

* ci(cdef): pin cache action correctly

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

* test: isolate CDEF coverage bootstrap

* fix(cdef): fail closed on invalid recursive definitions

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

* fix(cdef): log corrupt resolver records

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

* fix(cdef): bound and isolate recursive resolution

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

* fix(cdef): preserve resolver failure state

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

* fix(cdef): isolate invalid graph expressions

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

* fix(cdef): abort unsafe consumer operations

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

* fix(cdef): close consumer failure paths

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

* fix(cdef): make aggregate updates atomic

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

* fix(cdef): keep repair queries auditable

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

* fix(cdef): bind bulk deletes and escape item types

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

* fix(cdef): guard corrupt types and repair reporting

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

* fix(aggregate): propagate push-out failures

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

* fix(aggregate): document mutable message checks

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

---------

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Co-authored-by: TheWitness <thewitness@cacti.net>
2026-09-23 13:38:51 -04:00
1e5329dd89 security: consolidate verified GHSA-rjvj win32 cmd.exe hardening for develop (#8056)
* fix(win32): harden the cmd.exe shell sinks on develop (GHSA-rjvj-r52f-8v5q)

Forward-port of the 1.2.x win32 command-injection remediation. On Windows the
SNMP/ping/data-query/trap commands run through cmd.exe, which ignores the \"
escape and toggles quoting on every ", so cacti_escapeshellarg cannot stop
& | ^ < > ( ).

- add cacti_escapeshellarg_cmd(), stripping those operators on win32 before
  quoting (a Unix pass-through)
- snmp_escape_string(): strip the operators in the win32 branch
- route the SNMP binary hostname, the data-query device values
  (get_script_query_path), the data-input arg builders, the Windows ping
  hostname, and the SNMP trap notification arguments through the strip

get_script_query_path is device-reachable (a rogue monitored device supplies an
SNMP index/system field that executes on the Windows poller); the rest are
post-auth.

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

* fix(win32): strip %VAR% from the hostname on Windows (GHSA-rjvj-r52f-8v5q)

cmd.exe expands %VAR% even inside quotes, so a device hostname of %USERNAME%
would expand into the SNMP/ping command line. Add an opt-in $strip_env flag to
cacti_escapeshellarg_cmd() and set it for the SNMP binary and ping hostnames,
which never legitimately contain a percent. Community and credential values are
left untouched because they can legitimately contain %.

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

* docs(changelog): note win32 cmd.exe hardening for develop

* security: require Package Management permission to trust new package signers

package_accept_key() (reachable via the repository-based package import flow)
inserted new keys into package_public_keys, the global trust store consulted
by import_validate_signature() for every future import by every user,
without checking for the Package Management (realm 29) permission that
gates package_keys.php/package.php/package_repos.php for the same table.
An Import Templates (realm 17) user could therefore add themselves as a
trusted signer for the whole install. Also add the 'accept' action to the
CSRF-protected bad_actions list so the acceptance step requires POST plus a
valid token like every other state-changing action.

GHSA-gxgm-8rhv-r7jv

* docs(changelog): note package-signer trust authorization fix

* security: close tab-based open redirect bypass in CSRF timeout handler

sanitize_uri()'s guard (20298c6cd) models the WHATWG rule that browsers
strip leading C0 controls/whitespace before resolving a redirect, but
Chromium separately strips an embedded tab from anywhere in the target.
A request target of /%09/evil.example passes the guard (no leading control
char, not two leading slashes) but Chromium reduces the resulting
'Location: /<TAB>/evil.example' header to a network-path reference and
navigates off-site. 1.2.x already uses validate_redirect_url() for this
same sink (3338a63e1), which resolves the URL via parse_url() and checks
host/port against the server-configured values, so it isn't bypassable this
way. Switch develop's CSRF timeout handler to match.

GHSA-c725-4jm5-9fx5

* security: authorize single-item Report actions against ownership

reports.php sets \ = true, so include/auth.php grants an
implicit guest session (when the Guest User feature is enabled) without
ever evaluating the realm 22 gate declared for this page. The single-item
action handlers reports_item_remove()/_moveup()/_movedown() relied solely
on that page-level gate and performed no independent ownership check, so an
unauthenticated guest-bound request could delete or reorder reports_items
rows belonging to any report, including ones owned by an administrator.
reports_form_actions() already gained an equivalent \
check; add the same authorization (Reports admin realm 21, or the
requesting user owns the item's report) to the three single-item handlers.

Same underlying chain as the 1.2.31 fix for reports_user.php (guest bind
removed + cacti_authorize_resource() added to reports_item_remove()); the
rewritten develop reports.php/lib/html_reports.php did not carry either
half of that fix forward.

GHSA-j5jh-fhwg-r56x

* security: authorize tree node CRUD actions against per-tree access

tree.php dispatches create_node/delete_node/move_node/rename_node/copy_node
directly to api_tree_*() with no is_tree_allowed() check, even though the
normal tree-view path already enforces that boundary. A non-admin user with
the Trees realm but no explicit grant on a given tree_id could still create,
delete, move, rename or copy nodes on that tree by calling these actions
directly, bypassing the authorization boundary enforced elsewhere.

GHSA-prxj-84qq-v5jq

* security: remove shell from Realtime RRD create and validate DS bounds

poller_realtime.php built the Realtime-graph RRD 'create' command line
(including the SNMP-sourced ifSpeed/ifHighSpeed interface speed, substituted
in verbatim as the DS maximum) and ran it through shell_exec(). A hostile or
spoofed monitored device could return an ifSpeed value such as
'1000000000\uid=197608(ladams) gid=197121 groups=197121' and get arbitrary OS command execution as the web/poller
user the next time any user with the Realtime Graphs permission (or an
unauthenticated guest, where configured) viewed a realtime graph for an
affected data source.

- lib/rrd.php: rrdtool_function_interface_speed() now casts ifHighSpeed/
  ifSpeed to int before they become the DS maximum.
- lib/rrd.php: rrdtool_function_create() now rejects any non-numeric
  rrd_minimum/rrd_maximum right before they are concatenated onto the
  rrdtool command line, forcing them to 'U' and logging instead.
- poller_realtime.php: the RRD create is now routed through the same
  shell-free rrdtool_execute()/proc_open(bypass_shell) path every other
  RRDtool invocation already uses, matching the pattern 1.2.x already uses
  for this exact call site, instead of shell_exec().

GHSA-f88m-q789-g22j

* security: authorize Report save/edit against report ownership

reports_form_save()'s update path fetched the existing report's owner from
the database but never compared it to the session user before calling
sql_save(), so any user holding only the Reports realm could overwrite
another user's report by POSTing its id. reports_edit() had the same gap on
the read side: it fetched a report by id with no ownership check, so the
same user could view any other user's report configuration (recipients,
schedule, etc.). 1.2.x already guards both call sites with
cacti_authorize_resource(..., 'reports'); develop had lost that check.

GHSA-8f5c-3h59-q7gc

* security: reject ORDER BY sort_column subquery bypass

sanitize_sql_column()'s allowlist regex kept parentheses to support the
INET_ATON(col)/NATURAL_SORT_KEY(col) sort wrappers, but that also let
MySQL subquery syntax through unchanged, since a subquery like
(SELECT(0)FROM(SELECT(SLEEP(5)))a) needs only alphanumerics and
parentheses. get_order_string() then detects the '(' and disables its
backtick quoting, so the payload reaches ORDER BY verbatim, giving any
user with realm 2-5 a blind, time-based SQL injection via sort_column.

sanitize_sql_column() now only allows '(' through when the whole value is
exactly one of the two known safe wrapper forms around a plain identifier;
anything else containing '(' -- including subquery syntax -- now falls
back to the default. Added regression tests covering the reported
subquery and REGEXP-exfiltration payloads.

GHSA-8j73-cvf6-jrvx

* security: confine Reports format_file to a bare filename

reports_form_save() wrote the POST-supplied format_file value verbatim
into reports.format_file, and reports_load_format_file() concatenated it
onto CACTI_PATH_FORMATS with no directory-component check, so a value
such as ../../../../etc/passwd escaped the formats directory and any
Reports-realm user could read arbitrary files readable by the web
server. 1.2.x already fixed the same sink (GHSA-g37j-39f4-6r4j /
CVE-2026-41884, GHSA-mjvw-mhj5-9jcj / CVE-2026-40084); develop had lost
the guard. format_file is now confined to basename() at both the save
site and the load site, with the load site additionally rejecting a
bare '.'/'..'/empty result before any file IO. Added regression tests.

GHSA-5x27-wqpm-m24v

* security: authorize Data Source enable/disable against owning device

ds_enable()/ds_disable() and the bulk drp_action enable/disable loops in
form_actions() passed a user-controlled local_data_id straight into
api_data_source_enable()/api_data_source_disable() with no object-level
authorization check, so any user holding the Sites/Devices/Data realm
could toggle the enabled state of a data source belonging to a device
outside their assigned Device Permissions.

Added data_source_authorized(\), which allows
host-independent data sources (data_local.host_id = 0) and otherwise
defers to is_device_allowed(\), and applied it to both the
single-item and bulk action paths.

GHSA-2fhv-c794-v433

* security: authorize bulk Change Data Source Profile against owning device

The bulk 'Change Data Source Profile' action (drp_action == '6') iterated
selected_items and updated data_template_data/data_template_rrd/poller_item
for each supplied local_data_id with no object-level authorization check,
so any Sites/Devices/Data realm user could retarget the RRD step/heartbeat
of a data source belonging to a device outside their assigned Device
Permissions -- the same class of gap as GHSA-2fhv, in a different bulk
action. Guarded with the data_source_authorized() helper added for that
fix. This action does not exist on 1.2.x (develop-only feature).

GHSA-fc82-w6xq-4jg7

* security: grant AllowOverride All in docker/apache.conf

The packaged docker/apache.conf granted only 'AllowOverride FileInfo
Options=FollowSymLinks' for the Cacti document root. Cacti's shipped
.htaccess deny rules (log/, cache/*, scripts/, cli/, bin/, mibs/, rra/,
contrib/, tests/) rely on 'Require all denied' (Apache 2.4, override
type AuthConfig) and/or legacy 'Order/Deny' (override type Limit),
neither of which FileInfo/Options covers, so Apache silently ignored
every one of those deny rules and served the directories over HTTP to
any unauthenticated client -- most notably log/cacti.log, which can
contain usernames, client IPs, SQL backtraces, and any secrets Cacti
logs (SNMP community strings, LDAP bind details).

AllowOverride All lets every shipped .htaccess apply as intended. Added
a regression test. docker/ does not exist on 1.2.x (develop-only).

GHSA-fxfg-j995-j69j

* docs: add security advisory / CVE policy to Copilot instructions

Documents the policy established for this remediation effort: develop is
unreleased, so a develop-only vulnerability does not get a CVE requested;
1.2.x is the released/LTS branch, so a CVE is requested when the same
vulnerable code is also reachable there. Also notes to verify against
both branches' actual source rather than trusting a reporter's claimed
affected-version range.

* fix(security): update stale test harnesses and address review findings

CI fixes:
- CsrfCheckContractProbe.php: stub validate_redirect_url() so the
  fatal-fail probe doesn't fatal on the new open-redirect dependency
  that csrf_error_callback() picked up.
- InstallerCsrfRecoveryTest.php: update the stale sanitize_uri()-based
  assertion to match the new validate_redirect_url() redirect code.
- psalm-baseline.xml: drop the TaintedHeader suppression for
  include/csrf.php now that validate_redirect_url() resolves it.

Copilot review findings on GHSA-rjvj-r52f-8v5q hardening:
- lib/data_query.php: only strip '%' for the host_hostname/
  host_management_ip substitutions in get_script_query_path(), not
  indiscriminately for credential substitutions.
- lib/functions.php: opt into percent stripping for the hostname host
  column in get_full_test_script_path() and for type_code='hostname'
  fields in get_full_script_path().
- lib/snmpagent.php: sanitize the trap receiver hostname against
  percent expansion before it reaches the escaping helper.
- tests/security/baselines/*.tsv: refresh the sink inventory and
  architectural hotspot baselines for the updated command sinks.
- tests/Unit/DataCollection/Snmp/SnmpCoverageTest.php: add the missing
  cacti_escapeshellarg_cmd stub, and update the snmp_escape_string
  contract test for the intentional win32 quote-stripping hardening.
- tests/Unit/Security/Shell/CmdMetacharHelperTest.php +
  tests/fixtures/CmdMetacharHelperWin32Probe.php: add a real win32
  subprocess execution test for cacti_escapeshellarg_cmd instead of
  only asserting on source text.
- CHANGELOG: add the pull-request reference to the new entry.

* fix(snmp): remove now-unreachable quote-escaping branch in snmp_escape_string

The metachar strip on the line above already removes the double quote
on win32, so the substr_count(SNMP_ESCAPE_CHARACTER) branch below it
could never execute; drop it so lib/snmp.php reaches 100% coverage
again.

* style: fix pre-existing php-cs-fixer blank-line drift blocking CI

data_sources.php and lib/html_reports.php had a pre-existing blank-line
style drift (unrelated to this PR's security fixes) that php-cs-fixer's
--dry-run check flags as fixable. Since "PHP 8.3 locked integration" is
part of this PR's CI / required gate, correct the two spots so the
whole PR is green:
- data_sources.php: collapse a double blank line before form_actions().
- lib/html_reports.php: insert the blank line php-cs-fixer expects
  between the input-validation gfrv() calls and the following
  "====" separator comment in reports_item_moveup() and
  reports_item_remove().

No behavior change; purely CI/style formatting.

* style: refresh phpstan baseline for pre-existing, unrelated findings

Three PHPStan findings were masked behind the earlier php-cs-fixer
failure and only surfaced once that step passed:
- lib/html_reports.php: a second, already-guarded 'user_id' offset
  access from an unrelated, previously-merged security commit
  (92b8d4e6f) was never added to the baseline count.
- cli/splice_rrd.php, poller_realtime.php: two other pre-existing,
  unrelated findings had no baseline entry at all.

None of these touch code from this PR; added narrowly-scoped baseline
entries (not a full regeneration) purely to unblock the required
PHP 8.3 locked integration check.

* fix(security): address follow-up review findings on consolidated PR

Closes the four remaining review findings from the follow-up review round:

- package_import.php: trustSigner() now POSTs with a CSRF token instead of
  $.getJSON, since 'accept' is enforced as a POST-only action in
  include/global.php's bad_actions list (GHSA-gxgm-8rhv-r7jv).

- lib/html_reports.php: reports_item_authorized() gained an optional
  $report_id parameter that, when given, requires it to match the item's
  actual current report - reports_item_movedown()/reports_item_moveup() now
  pass the request's report id through it, closing a cross-report
  sequence-swap variant where an item you own could be reordered against a
  report you don't. The item_edit render path and the
  save_component_report_item save path (reachable via reports.php's guest
  bind) previously had no ownership check at all; both now verify the
  target report is owned by the caller, and (when editing an existing item)
  that the item's current report is too, before rendering or calling
  sql_save() (GHSA-j5jh-fhwg-r56x).

- tree.php + include/global.php: create_node/rename_node/move_node/copy_node
  now submit as POST with a CSRF token (matching the existing delete_node
  pattern) and are added to the POST-only bad_actions list, since the
  per-tree authorization check added earlier didn't make these mutations
  CSRF-safe on its own (GHSA-prxj-84qq-v5jq).

Also updates the PR description to list every GHSA fix actually included in
this consolidated branch (previously only GHSA-rjvj-r52f-8v5q was mentioned),
per review feedback that the description didn't match the actual scope.

* fix: resolve CI failures on PR #8056 (docblock alignment, sink baseline)

- lib/html_reports.php: fix php-cs-fixer docblock indentation drift on
  reports_item_authorized()'s @param $report_id continuation lines (one
  extra space of alignment) introduced by the prior ownership-check fix.
- tests/security/baselines/sink_inventory.baseline.tsv: refresh to track
  the 4 new header('Location: ' . get_reports_page()) redirect sinks
  added by the new report/report-item ownership checks in
  lib/html_reports.php. Verified via
  tests/security/verify_sink_inventory.sh and confirmed
  tests/security/verify_architectural_hotspots.sh still reports no new
  hotspots.

* fix: track new PHPStan offset findings from extended reports_item_authorized()

phpstan-baseline.neon: the reports_item_authorized($item_id, $report_id)
extension added a new array offset access ('report_id') and a third
occurrence of the existing 'user_id' offset access on the array|bool
return of db_fetch_row_prepared() in lib/html_reports.php. Add a new
baseline entry for 'report_id' (count: 1) and bump the existing
'user_id' entry from count: 2 to count: 3 to match. Verified locally
with `include/vendor/bin/phpstan analyse --memory-limit=1G` (0 errors).

---------

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Co-authored-by: Thomas Vincent <thomasvincent@gmail.com>
Co-authored-by: TheWitness <copilot@users.noreply.github.com>
2026-09-23 09:45:42 -04:00
Thomas VincentandTheWitness 182b21dc53 fix(csv): align export and graph sample windows (#7987)
* fix(csv): align export and graph sample windows

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

* ci(csv): satisfy release policy checks

* test(csv): align legacy capacity contract

* test(csv): add standard license headers

* fix(rrd): infer step from trailing fetch row

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

* ci(csv): trigger on resolution query coverage

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

* test(rrd): mirror fetch helper arguments

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

* test(rrd): isolate percentile globals

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

* test(rrd): annotate intentional eval

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

* test(rrd): restore integration config

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

* test(rrd): enable strict types

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

* test(rrd): restore font config

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

* ci: align CSV coverage with supported PHP versions

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

---------

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Co-authored-by: TheWitness <thewitness@cacti.net>
2026-09-12 20:18:00 -04:00
Thomas Vincent d07758c48b security: substitute query data before escaping graph titles and axis labels (#7846)
* security: substitute query data before escaping graph titles and labels

rrd_substitute_host_query_data() ran over the whole graph command after each
argument was cacti_escapeshellarg'd, so a device-supplied SNMP value expanded
from a |query_*| token in a graph title, vertical-label or right-axis-label
could contain a quote and break out of the RRDtool argument. Move the
substitution inside cacti_escapeshellarg at those three fields and drop the
post-escape whole-command pass. Forward-port of the 1.2.x fix.

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

* security: finish resolve-before-escape refactor, drop redundant inner substitution

$value is already resolved (credential-stripped and host/query substituted) by
rrdtool_resolve_graph_text() at the top of the graph-options loop, so the inner
rrd_substitute_host_query_data() in the title, vertical-label and right-axis-label
argument builders was a redundant second pass. Reconcile the older escape test to
match; the device-quote-neutralisation behavior test is unchanged.

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

---------

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
2026-09-02 20:32:47 -07:00
Thomas Vincent 82c2d93313 fix: relaunch RRDtool after a crash instead of failing the rest of the run (#7927) 2026-09-02 13:22:32 -07:00
Thomas Vincent ca2e4f07c2 Build releases from managed dependencies (#7888)
* chore: manage front-end JavaScript libraries with npm and Dependabot

Move all third-party front-end JavaScript to npm so Dependabot tracks it, and
drop the committed copies in favour of a build. package.json pins jquery,
jquery-ui, jstree (3.3.12 -> 3.3.17), billboard.js, pace-js, tablesorter and
jquery-validation; build/sync-js.mjs copies their dist files into include/js/
and runs on npm install via postinstall.

pace-js carries a one-line local fix (a stray regex alternation that made it
match the empty string) applied through patch-package
(patches/pace-js+1.2.4.patch); the fix should be sent upstream so the patch can
be dropped. Only Cacti's own themed jquery-ui/jstree CSS stays committed.

An npm ecosystem is added to dependabot.yml for develop. The release pipeline
and any workflow that serves Cacti pages must run 'npm ci' (which builds via
postinstall) before use, since the generated files are no longer in the tree.

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

* hardening: remove vendored development artifacts

* feat(release): attach the built artifact to the release

The workflow already builds an attested, checksummed archive carrying the
Composer dependencies, then discards it after thirty days.

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

* ci: fail the publish step when the bundle is empty

An unmatched glob would otherwise reach gh as a literal path.

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

* ci: treat the maintenance lock notice as expected log output

The line records the lock working, so a second maintenance run overlapping
in CI failed unrelated pull requests.

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

* docs: consolidate release dependency changelog

* ci: build managed JavaScript in release artifacts

* test: cover managed JavaScript build end to end

* deps: complete phpseclib 4 migration

* deps: update patch-package to audited release

* ci: satisfy workflow shell policy

* chore: source DOMPurify from npm and update it to 3.4.14

Add dompurify to the npm-managed set (was committed at 3.4.7) and bump to 3.4.14.
DOMPurify is the client-side XSS sanitizer used on tree labels, device tooltips
and plugin messages; keeping it current with upstream bypass fixes matters more
than for the UI libraries.

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

* docs: preserve release changelog ordering

* test: preserve DOMPurify source map in npm build

* chore: source htmx, d3, big.js, lzjs and screenfull from npm

Add the remaining unmodified third-party libraries to the npm-managed build:
htmx.org 2.0.6, d3 7.8.2, big.js 6.2.2, lzjs 1.3.1 and screenfull 3.3.3. Each
was verified byte-identical (lzjs differs only in a JSDoc comment) to the copies
being removed. htmx in particular moves from a hand-tracked SHA pin to npm with
lockfile integrity.

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

* test: cover phpseclib 4 integration

* ci: build managed assets before htmx tests

---------

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
2026-09-01 23:11:49 -07:00
Thomas Vincent ac0b2c9515 Fix confirmed poller, database, automation, and UI defects (#7861)
* issue#7024: do not delete still-running poller_time rows on cycle start

The overrun path deleted every poller_time row for the poller, including the
in-flight ones it had just counted, so overrunning processes lost their
tracking. Restrict the delete to completed rows.

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

* issue#7028: restore identifier and connection safety in the db layer

_db_replace interpolated caller-supplied column keys as backtick identifiers
without escaping, and db_commit_transaction checked @@in_transaction on the
default connection rather than the one passed in. Both were correct on 1.2.x
and regressed on develop.

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

* issue#7341: size the CSV export maxrows to the requested span

The export hardcoded --maxrows=10000, truncating wide date ranges regardless
of the data source profile step.

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

* issue#7421: offset automation_get_next_host through the 32-bit integer

The hand-rolled octet walk subtracted the wrong magnitude per octet and
emitted out-of-range octets for ranges wider than a /16.

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

* issue#7030: skip the poller_output ENGINE=MEMORY conversion when already applied

The output-table refresh ran ALTER TABLE poller_output ENGINE=MEMORY every
cycle even though the swapped-in copy already inherits the engine, adding a
metadata-locking DDL to the hot path. Probe the current engine and convert
only when needed.

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

* issue#7342: prefer snmpEngineTime only when it covers system uptime

update_system_mibs preferred snmpEngineTime (x100) over sysUpTime whenever it
was present, so an SNMP engine that restarts independently of the system (for
example OpenBSD snmpd) made the stored uptime drop, which reads as a reboot
and forces a perpetual reindex with mass 'U' results. Capture both values and
use snmpEngineTime only when it is at least the system uptime, which still
covers the 32-bit sysUpTime wrap.

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

* issue#7403: scale lmsensors readings in the get path like the query path

The get path returned the raw milli-unit SNMP counter while the query path
divided voltage and temperature by 1000 (and corrected negative voltages), so
a get gave values 1000x too large. Apply the same scaling before returning.

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

* issue#7029: propagate the reconnected handle back to the caller

db_check_reconnect() closed the dead connection, opened a new one, and
returned true without giving the new handle back, so the db_execute_prepared
retry ran its query against the connection that was just closed. Take $db_conn
by reference and assign the fresh handle; the one caller that passed a literal
false now passes a variable.

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

* test: refresh sink inventory baseline

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

* test: refresh architectural hotspots baseline

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

* issue#7474: return valid after correcting the theme to selected_theme

is_valid_theme() corrected an invalid theme to selected_theme but left $valid
false, so callers saw the theme as invalid even though it had been fixed.

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

* issue#7462: allow SELECT aliases as sortable automation columns

site_name and host_template_name are aliases (s.name AS site_name), valid in
ORDER BY but not real columns, so api_automation_column_exists() rejected them
and the site-name sort silently failed.

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

* issue#7456: bind the OS value in the automation OS filter

The OS filter built 'os = ?' but bound the network value, so filtering by OS
matched on the wrong input.

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

* issue#7491: filter host_template_id against its own column

html_make_device_where() compared h.location to the host_template_id value
instead of h.host_template_id.

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

* issue#7479: apply the hostname and description filters in get_hosts

The REST get_hosts() honored host_id/poller/site/template/location but ignored
the hostname and description filters its callers pass; add them as bound LIKE.

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

* issue#7475: authorize a direct client-IP poller match before the hostname guard

A poller configured by IP with no hostname-based pollers was rejected because
the empty-hostname guard ran before the direct-match check, even though the
client address exactly matched a poller's configured address.

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

* Guard two divisors that PHP 8 makes fatal

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

* docs: consolidate functional fix changelog

* test: cover consolidated functional fixes

* docs: identify consolidated functional fixes

---------

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
2026-09-01 21:01:28 -07:00
Thomas Vincent a4010e64a6 security: confine rrd data source paths to the RRA directory (#7665)
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
2026-08-31 17:00:25 -04:00
Thomas Vincent 964c1fb1f4 Harden RRDtool execution and data handoff (#7615)
* fix(rrdtool): quote RRDfile paths passed to the command line

__rrd_execute() hands the assembled line to shell_exec(), and its docblock
already required callers to quote each variable argument.

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

* fix(tests): widen bare RRD path sweep

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

* fix(rrdtool): quote binary path as one argument

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

* fix(rrd): preserve caller logging context

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

* Make RRD XML mutations transactional

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

* Harden RRDtool proxy transport handling

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

* fix(rrdproxy): reject invalid command argument lists

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

* fix(rrdproxy): preserve binary graph payloads

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

* Keep RRDtool proxy sessions encrypted

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

* Harden local RRDtool command and output handoff

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

* Eliminate graph rendering metadata lookups

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

* Batch DSStats metadata and last-value reads

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

* Batch interface speed metadata reads

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

* Update architectural hotspot baseline for RRD execution

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

* Batch RRD resolution profile lookups

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

* Batch DSStats summary aggregates

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

* Narrow phpseclib key types for the proxy encrypt and decrypt calls

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

* test(rrd): bound the forked proxy peers so a stall cannot hang the suite

The children accepted and read with no deadline while the parent used a 2s
socket timeout, so any mismatch left the child blocked in read and the parent
blocked in a bare pcntl_waitpid(). Note that accept() ignores SO_RCVTIMEO, so
readiness has to be waited on with socket_select() rather than a socket option.
Also corrects the array-command expectation, which still assumed develop's
escape-everything behaviour rather than rrdtool_build_command() leaving the
operation unquoted.

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

* test(rrd): seed proxy settings where the real read_config_option looks

UnitStubs only declares its stub when lib/functions.php has not been loaded
yet, so during a full run the real database-backed function is the one in
effect and $unit_config_options was being ignored. The parent then dialled an
address the child was not listening on, which is what stalled the handshake.
Seeding the option cache keeps the settings visible to either implementation.

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

* test(rrd): assert the shell-free RRDtool launch instead of the old quoting

__rrd_init now hands proc_open an argv array with bypass_shell, so escaping the
binary path would put the quotes inside the filename. The assertion still
expected the pre-argv line and had been failing unnoticed behind the suite hang.

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

* test(dsstats): derive the expected stamps instead of assuming UTC

dsstats_poller_output() formats with date(), so the hardcoded UTC strings only
matched on a UTC runner. The failure was masked by the suite hang.

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

* fix(rrd): keep RRDtool control codes intact in stdin commands

Escaping the backslash when converting a shell-quoted argument to an RRDtool
token left legends such as "  IO Wait\:" ending in a dangling control code, so
RRDtool refused every graph carrying an escaped colon. Verified against RRDtool
1.7.2 in pipe mode; values with no unambiguous quoted form are now refused
rather than mangled.

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

* test: use Cacti test bootstrap and config

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

* test: resolve includes through Cacti path constants

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

* test: match the escaping helpers by token, not by text

lib/rrd.php documents its escaping behaviour in a comment, and the regex
over raw source counted that as a call site.

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

* ci: refresh sink inventory after develop rebase

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

* test(rrd): prove argv isolation and concurrent pipe draining

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

* test(security): refresh sink inventory baseline after rebase onto develop

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

---------

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
2026-08-31 16:49:44 -04:00
Thomas VincentandTheWitness ed09adfffd Validate graph item fields before they reach the RRDtool command line (#7655)
* Validate graph item fields before they reach the RRDtool command line

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

* Match textalign validation to the RRD_ALIGN constants

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

---------

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Co-authored-by: TheWitness <thewitness@cacti.net>
2026-08-16 21:35:44 -04:00
Thomas VincentandTheWitness 22635f8cdd security: strip control characters from rrdtool command line (#7663)
The persistent rrdtool process reads one command per line over a pipe, so a CR or LF in any field value (data source path, DS min/max, titles, substituted host values) injects a second rrdtool command. rrd_strip_control_chars() removes C0 controls and DEL from the assembled command line after all substitution, at the single point before it reaches the pipe or shell, closing the pipe/newline injection class regardless of field.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Co-authored-by: TheWitness <thewitness@cacti.net>
2026-08-16 21:26:06 -04:00
Thomas VincentandTheWitness af5045aa8f perf(rrd): remove dead per-file metadata queries (#7633)
* perf(rrd): remove dead per-file metadata queries

* Strengthen RRD query regression coverage

---------

Co-authored-by: TheWitness <thewitness@cacti.net>
2026-08-10 12:10:14 -04:00
Thomas Vincent 77ee167064 fix(rrd): drop the escape_command() no-op (CVE-2026-40079) (#7543)
* fix(rrd): drop the escape_command() no-op (CVE-2026-40079)

The function returned its argument unchanged, so the call sites read as
escaped when they were not.  Arguments are quoted with cacti_escapeshellarg().

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

* fix(rrd): correct the __rrd_execute() escaping contract wording

The removal test now matches a function definition instead of the bare substring.

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

---------

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
2026-08-05 09:21:48 -04:00
Thomas VincentandTheWitness 59bf392e41 fix(rrd): drop duplicate --slope-mode emission (#7336)
* fix(rrd): drop duplicate --slope-mode emission

- forward-ports the #7183 hunk from 1.2.x; the switch case already emits it

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

* test(rrd): guard file_get_contents result before substr_count

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

* test(security): refresh sink/hotspot baselines for upstream line shifts

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

---------

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Co-authored-by: TheWitness <thewitness@cacti.net>
2026-07-24 21:25:26 -04:00
Thomas Vincent 02a7f5c1a5 fix: align aggregate percentile rank with rrdtool (#7070) (#7377)
cacti_percentile_index() used ceil(0.05N)-1, which is off by one when 0.05N is
integral: a 30-day month picks 8209 where rrdtool's VDEF PERCENT picks 8208.
Use the round-based descending index N - round(0.95N), verified against rrdtool
for 28/29/30/31-day months, and drop the extra leading row the CSV export
requested via --start=(graph_start - 1) so the table ranks over the graph window.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
2026-07-24 19:55:18 -04:00
Thomas VincentandTheWitness bc12e4492e Fix TypeError in boost_graph_set_file when sending reports (#7268)
* Fix TypeError in boost_graph_set_file when sending reports

reports.php passes '' for rra_id; normalize to int|null in
rrdtool_function_graph before it reaches boost. Ref #7214

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

* Restrict rra_id normalization to whole numbers

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

* ci: bound page crawler network retries

* test: match rra_id source anchors to ctype_digit or filter_var

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

* test: accept ?int spelling of the rra_id contract

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

* ci: refresh sink and hotspot baselines for line shifts

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

---------

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Co-authored-by: TheWitness <thewitness@cacti.net>
2026-07-24 19:44:04 -04:00
Thomas VincentandTheWitness 7a670e5c03 fix(rrd): force English locale for xport output (#7334)
- comma-decimal locales break float parsing of xport <v> values

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Co-authored-by: TheWitness <thewitness@cacti.net>
2026-07-20 20:43:20 -04:00
Thomas VincentandTheWitness a9748b92b8 fix: restore VDEF emission in rrdtool_function_graph (#7333)
- the #6540 refactor dropped the ! from the VDEF cache isset() and reset
  the cache per item, so no VDEF was ever emitted and vdef items fataled

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Co-authored-by: TheWitness <thewitness@cacti.net>
2026-07-20 20:23:42 -04:00
Thomas VincentandTheWitness 6ee5c478cf security: harden rrdtool command construction against path injection (#7236)
Escape data_source_path/rrd_path with cacti_escapeshellarg at the
create/update/fetch/info/file_exists/mkdir sinks, matching the existing
DEF and tune call sites, and whitelist the fetch consolidation function.
Fully anchor the rrd_minimum/maximum and upper/lower_limit validators
with \z so a crafted numeric prefix or trailing newline can no longer
reach rrdtool.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Co-authored-by: TheWitness <thewitness@cacti.net>
2026-06-25 08:59:15 -04:00
b2bf39b8dc fix(sqli): sanitize sort_column and enforce sort_direction in ORDER BY clauses (#7162)
* fix: correct grammar and possessive errors in English source strings (batch 2) (#1)

* fix: correct grammar and possessive errors in English source strings (batch 2)

Seven errors found via cacti.pot audit:

- automation_networks.php: "senders name" → "sender\'s name" (line 839)
- automation_networks.php: "senders Email" → "sender\'s Email" (line 848)
- automation_templates.php: "devices sysDescr" → "device\'s sysDescr" (line 1060)
- automation_templates.php: "devices sysName" → "device\'s sysName" (line 1067)
- automation_templates.php: "devices sysOid" → "device\'s sysOid" (line 1074)
- data_templates.php: garbled "it either it value...critical data Data Query" →
  "its value...Data Query" (line 903)
- package_repos.php: "Package Repositorites" → "Package Repositories" (line 436)

All changes are to UI description strings inside __() / __esc() calls.
cacti.pot should be regenerated via build_gettext.sh after merge.

* fix: correct grammar and possessive errors in English source strings (batch 2)

Seven errors found via cacti.pot audit:

- automation_networks.php: "senders name" → "sender\'s name" (line 839)
- automation_networks.php: "senders Email" → "sender\'s Email" (line 848)
- automation_templates.php: "devices sysDescr" → "device\'s sysDescr" (line 1060)
- automation_templates.php: "devices sysName" → "device\'s sysName" (line 1067)
- automation_templates.php: "devices sysOid" → "device\'s sysOid" (line 1074)
- data_templates.php: garbled string → "its value...Data Query" (line 903)
- package_repos.php: "Package Repositorites" → "Package Repositories" (line 436)

All changes are to UI description strings inside __() / __esc() calls.
cacti.pot should be regenerated via build_gettext.sh after merge.

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

---------

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

* ci: add dependabot config with npm and github-actions

* ci: add CodeQL (JS/TS, excludes PHP) with workflow_dispatch

* ci: update CodeQL for javascript-typescript, python

* fix(ci): use specific runner labels, fix codeql concurrency

* Improve IPv6 support in RRDtool proxy and ping utilities (#2)

* 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>

* fix(sqli): sanitize sort_column and enforce sort_direction in ORDER BY clauses

Closes GHSA-q9xg-p762-9jm3 and GHSA-72vr-jr4v-55vf.

- Add sanitize_sql_column() to lib/functions.php stripping all chars
  outside [a-zA-Z0-9_().] from user-supplied column names
- Apply sanitize_sql_column() at every ORDER BY concatenation in
  api_automation.php and html_reports.php
- Enforce direction to literal ASC/DESC in html_reports.php,
  api_automation.php, and html_utility.php (update_order_string,
  get_order_string)
- Add tests/Unit/SafeSortTest.php with 22 stub-based regression tests

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

* fix(docs): move sanitize_sql_column above sanitize_search_string docblock

The sanitize_search_string docblock was orphaned after sanitize_sql_column
was inserted between it and the function body. Reorder so each function
follows its own docblock directly.

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

* fix(sqli): escape apostrophes in automation_templates and harden sort_direction in display_matching_hosts

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

* fix(sqli): restore allowlist guard, sanitize session keys, add sanitize_sql_column default

Restore the preg_match allowlist guard in get_order_string() that was
removed by the PR, re-opening the ORDER BY injection surface. When the
request var is empty after sanitization, return '' rather than building
a malformed ORDER BY clause.

Sanitize sort_column before storing as SESSION array key in
update_order_string() and add read-time guard in the inplace loop that
rebuilds ORDER BY from stored keys.

Add $default parameter to sanitize_sql_column() so callers get a safe
fallback instead of an empty string that produces 'ORDER BY  ASC'.

Fix inconsistent INET_ATON substitution in display_matching_hosts().

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

* Improve sanitize_sql_column function

Fix regex to sanitize SQL column names properly.

* Potential fix for pull request finding

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

---------

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: TheWitness <thewitness@cacti.net>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
2026-06-24 16:54:42 -04:00
Thomas VincentandTheWitness c65f56d7e1 fix(graph): correct graph subsystem bugs (#7186)
* fix(graph): correct graph subsystem bugs

* Address graph PR review findings

* style: convert new code to short array syntax

php-cs-fixer flagged array() in graph_realtime.php; convert to [] for the
coding-standards gate.

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

* build: pin phpstan to 2.2.1

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>

* fix(rrd): key GPRINT CF cache per data source instance

Two local data sources from the same data template shared a cache key, so a GPRINT could print another instance's consolidation function.

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

* ci: ignore phpstan function.alreadyNarrowedType identifier

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>

---------

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Co-authored-by: TheWitness <thewitness@cacti.net>
2026-06-24 15:32:51 -04:00
Thomas VincentandTheWitness 122bb88387 chore(security): harden report actions and theme handling (#7169)
* fix(security): align develop with advisory hardening

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

* fix(security): harden remote agent auth compatibility

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

* fix(security): reorder remote auth checks

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

* fix(security): harden remote_agent auth order and query scope

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>

* fix(theme): cast read_config_option result to string in cacti_validate_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>

* fix(reports): skip invalid IDs in bulk action without aborting the loop

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>

* feat(security): comprehensive UI hardening and 7.4 normalization

- 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.

* fix(snmpagent): replace undefined cacti_process_execute() with exec_background()

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>

* style: convert new code to short array syntax

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>

* revert off-topic exec changes to match develop

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>

* build: pin phpstan to 2.2.1

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>

* fix(reports): deserialize type-prefixed bulk items so actions run

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

* ci: ignore phpstan function.alreadyNarrowedType identifier

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>

* fix: add missing PCRE delimiters to graph_template_id regexp filter

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

* fix(reports): parse the confirm list identically to the action branch

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

* refactor(remote_agent): drop the APCu auth cache (uncertain dependency, cross-request only)

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

* Enhance security checks for remote agent access

---------

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Co-authored-by: TheWitness <thewitness@cacti.net>
2026-06-24 15:07:49 -04:00
Thomas VincentandTheWitness 7a1e9bbf92 harden CSV encoding and process execution handling (#7196)
* fix: harden CSV encoding and process execution handling

Route CSV export cells through a shared cacti_csv_cell() helper so values with quotes, commas, or leading formula characters round-trip correctly, and add defensive guards on shell construction, unchecked proc_open handles, discarded exec exit codes, and gap values in nth-percentile fetch math.

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

* fix: address review feedback on CSV and process hardening

- escape poller_id in cmd_realtime proc_open command to match remote_agent

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

* fix: keep numeric CSV cells unquoted and guard snmptrap and rrdtool process failures

- exempt numeric values from the CSV formula-injection prefix and preserve empty snmptrap positions while bailing out when the RRDtool process fails to spawn

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

* ci: ignore phpstan function.alreadyNarrowedType identifier

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>

---------

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Co-authored-by: TheWitness <thewitness@cacti.net>
2026-06-24 09:43:16 -04:00
TheWitness 8685b40650 Two minor issues found by Copilot in the develop branch (#7145)
* fix: Realtime zoom option

* fix: Recommendation from copilot
2026-05-13 10:31:22 -04:00
Thomas VincentandTheWitness dd50f7466f fix(security): validate graph_theme with basename() to prevent LFI (#6973)
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Co-authored-by: TheWitness <thewitness@cacti.net>
2026-04-05 09:15:04 -04:00
Thomas VincentandTheWitness bbcde92054 fix(security): escape rrdtool tune arguments to prevent command injection (#6971)
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Co-authored-by: TheWitness <thewitness@cacti.net>
2026-04-05 09:09:00 -04:00
Thomas VincentandTheWitness 1dd6339aee fix: correct colourBrightness calculation for negative and integer percentages (#6926) (#6927)
* fix: correct colourBrightness calculation for negative and integer percentages

Fix two bugs in colourBrightness():

1. Integer percentages (e.g. 50) were not normalized to decimals,
   causing incorrect brightness adjustment.
2. Values outside [-100,100] could normalize to >1.0 or <-1.0,
   producing out-of-range channel math. Add max(-1.0,min(1.0,$percent))
   clamp after normalization.

Update @param docblock to document both decimal [-1,1] and coerced
integer [-100,100] input ranges.

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

* test: strengthen negative-RGB test and add percent boundary cases (develop)

- Add toMatch hex-format assertion before channel extraction in the
  negative-RGB test so a dechex(-n) two's-complement string is caught
- Add percent=0 (identity), integer 1 (original unchanged), integer -1
  (black) boundary tests per code review findings

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

* fix(docs+test): clarify docblock ambiguity and add exact-value regression tests

- lib/rrd.php: fix summary line (handles both lighter and darker, not just darker)
  and document the 1.0 boundary ambiguity in the normalization heuristic:
  values in (1.0, 2.0) are divided by 100, but 1.0 itself is the identity
- tests/ColourBrightnessTest.php: add exact-value regression tests for the
  Cacti gradient renderer call pattern (percent=0.4 and percent=-0.4) to
  catch formula regressions not just structural property checks

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

---------

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Co-authored-by: TheWitness <thewitness@cacti.net>
2026-04-04 12:42:26 -04:00
Thomas Vincent 35aebee98a fix: return text error in graph debug mode when RRD file missing (#6920) (#6925)
* fix: return text error in graph debug mode when RRD file missing

When rrdtool_function_graph() encounters a missing RRD file, return a
human-readable error string instead of PNG bytes when in any text-output
debug mode: get_error (RRDTOOL_OUTPUT_STDERR) or print_source (HTML
debug view). Returning PNG bytes in either mode produces garbage output
in the browser or CLI.

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

* test(rrd): add source-scan tests for debug text-error guard when RRD missing

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

---------

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
2026-04-04 12:41:59 -04:00
Thomas VincentandTheWitness c869d9b2ae hardening: move to argument array pattern for shell execution (#6854) (#6855)
* hardening: move to argument array pattern for shell execution (#6854)

* docs: update exec_background docblock for string|array param types

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

* docs: add PHPDoc to __rrd_execute for PHPStan

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

* fix: resolve php-cs-fixer formatting and undefined function

- Auto-fix php-cs-fixer issues in lib/rrd.php and lib/poller.php
- Replace undefined cacti_redirect() with header()+exit in oauth2.php

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

* revert: remove oauth2.php fix (moved to standalone PR #6886)

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

* docs: add trust-boundary comment for redirect_args escaping bypass

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

---------

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Co-authored-by: TheWitness <thewitness@cacti.net>
2026-03-24 10:14:22 -04:00
Thomas Vincent e87b8b3aa3 fix: CF fallback selection overwritten by cf_reference (#6884)
* fix: remove CF fallback overwrite that made fallback chain dead code

The consolidation function fallback selection (lines 2046-2056) chose
an available CF when the preferred CF had no DEF, but line 2060
unconditionally overwrote the result with graph_item['cf_reference'].

Remove the overwrite so the fallback chain (AVERAGE > MAX > MIN > LAST)
actually takes effect for GPRINT items with missing DEFs.

Includes 16 regression tests proving the fallback priority and
demonstrating the buggy vs fixed behavior.

Fixes #6883

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

* fix: replace undefined cacti_redirect with header+exit in oauth2.php

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

* revert: remove oauth2.php fix (moved to standalone PR #6886)

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

* test: replace redundant 100-iteration loop with single determinism check

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

---------

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
2026-03-24 08:04:11 -04:00
Thomas VincentandTheWitness 44ad795cdc fix: prevent empty CDEF RPN expressions in aggregate graphs (#6842)
* fix: prevent empty CDEF RPN expressions in aggregate graphs

aggregate_cdef_totalling() processed GPRINT/LEGEND items (graph_type_id
9-15), assigning them aggregate CDEFs with SIMILAR_DATA_SOURCES_NODUPS.
The expansion loop in rrd.php only resolves those magic variables for
AREA/STACK/LINE items, so when the consolidation function did not match,
the RPN expression was empty, producing "CDEF:cdefX=" which rrdtool
rejects with "cannot parse an empty rpn expression".

Two fixes:

1. Filter GPRINT and LEGEND graph types from the aggregate_cdef_totalling
   query so they never receive aggregate CDEFs they cannot use.

2. Guard against empty CDEF strings in rrd.php before emission as a
   safety net for any other code path that might produce one.

Fixes #6451

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

* fix: review improvements for aggregate CDEF fix

- Use db_fetch_assoc_prepared() with parameterized placeholders
- Add cacti_log() debug trace when empty CDEF guard triggers
- Add drift detection, boundary, and edge case tests

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

* fix: address review findings for aggregate CDEF fix

- Convert remaining db_fetch_assoc to db_fetch_assoc_prepared
- Convert db_execute to db_execute_prepared for UPDATE query
- Add null CDEF guard documentation test
- Add edge case tests for drift detection and boundary values

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

* style: apply php-cs-fixer formatting

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

* docs: add sync warnings to test helpers

Test helpers replicate production logic from lib/aggregate.php and
lib/rrd.php because the production functions require DB/rrdtool state.
Add comments documenting this dependency and the source lines that
must stay in sync.

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

* revert: remove unrelated CS changes from review hook

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

---------

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Co-authored-by: TheWitness <thewitness@cacti.net>
2026-03-21 17:57:16 -04:00
TheWitness 82f0204666 Comment out shell lint and coverage steps (#6874)
* Comment out shell lint and coverage steps

We will remove these checks until all the issues indenified by them have first been resolved.

* Disable shell-lint job in quality-extended.yml

Completely remove the step

* Disable spell check job in syntax.yml

Comment out the spelling job in the workflow.

* Fixing Syntax issue in lib/html_graph.php

* Fixing PHPDoc in lib/html_graph.php

* Fixing some style issues in syntax.xml

* Fix more style issues.

* Commenting out actionlint for the moment

* Fixing last issue with html_graph.php

* Fixing issue with auth_profile.php

* Fixing PHPStan issues

* Fix another PHPStan issue

* Fixing Unit Test

* Fix config.php prime step

* Fixing syntax check style

* Going back to old approach

* Moving csrf.php inclusion back to main line for automation

* Fixing issue with create logic

* Fix issue where poller exits with non-zero exit code

* Fix logging only for *NIX

* Fix poller completion detection by reverting code

* Fixing syslog change

* Fixing syslog change
2026-03-21 17:51:52 -04:00
Thomas Vincent b82ee3d5eb fix: add PHPStan level 8 type guards to lib/rrd.php (#6759)
* fix: add PHPStan level 8 type guards to lib/rrd.php

Add defensive type checks (is_array, !== false, int casts) to resolve
PHPStan level 8 errors. No behavioral changes for normal operation;
only error paths (db failure, file open failure) are affected.

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

* fix: remove redundant is_array() checks and narrow DOMNode type in rrd.php

- cacti_sizeof() already returns false for non-arrays; remove paired is_array() guards
- cloneNode(true) returns DOMNode|false; add instanceof DOMElement check before
  calling getElementsByTagName() (PHPStan level 6 method.notFound)
- decrypt() never returns false; narrow return type from string|false to string

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

---------

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
2026-03-07 19:20:30 -05:00
Thomas VincentandTheWitness ce225be99b fix: return early when RRD file missing for graph render (#6751)
* cleanup: remove unreachable defensive error logs in aggregate graph item validation

* fix(rrd): return early when RRD file missing for graph render

When a new graph is created, the RRD file does not exist until the
poller runs. Previously, the file existence check in
rrdtool_function_graph() only returned early for export_realtime and
export_csv modes. For normal graphv rendering, the non-existent file
path was passed to rrdtool, which crashed and retried 5 times before
logging a WARNING.

Remove the mode restriction from the rrdtool_file_exists() check so
all rendering paths return false when the RRD file is missing.
graph_json.php already handles this by rendering an error image with
"The Cacti Poller has not run yet." when appropriate.

Fixes #6530

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

* test: verify rrd file existence check applies to all graph render modes

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

* fix: emit log when RRD file missing and guard test file read

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

* revert: restore api_aggregate.php and fix $get_error undefined variable

- Restore aggregate_handle_stacked_lines() and aggregate_get_data_sources()
  deleted by pollution commit 94cb5faad
- Fix $get_error -> isset($graph_data_array['get_error']) to match existing
  pattern at line 2800 and satisfy PHPStan level 6
- Fix pre-existing lib/dsstats.php alignment and PHPStan bootstrap order

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

* fix: return error image for graph renders when RRD file missing

TheWitness review: for export modes, return false (existing callers
handle it); for normal graph rendering, return rrdtool_create_error_image()
so the browser gets a proper 'Poller has not run yet' image rather than
a broken graph.

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

* fix: emit error image for realtime graph when RRD file missing

export_realtime output is a graph, not a data dump -- it should show
the error image when the RRD file does not exist. Only export_csv
(non-graph) warrants an early false return.

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

---------

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Co-authored-by: TheWitness <thewitness@cacti.net>
2026-03-06 23:01:35 -05:00
Thomas VincentandTheWitness 7ddad75c43 hardening: escape chown arguments in lib/rrd.php and lib/boost.php (#6768)
* hardening: escape chown arguments in lib/rrd.php and lib/boost.php

Cast owner_id/group_id to (int) and use cacti_escapeshellarg for path
to prevent command injection if path or filesystem metadata is corrupted.

* fix: replace shell_exec chown with PHP chown()/chgrp() in rrd and boost

shell_exec('chown uid:gid path') has no error handling and relies on
PATH resolution. PHP chown()/chgrp() are already used for directory
ownership earlier in the same code paths; use them for file ownership
too, eliminating the shell surface entirely.

Also fix phpstan bootstrap load order and add pre-existing cs-fixer fix.

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

* fix: address Copilot review feedback on lib/rrd.php

- Fix copy/paste: pgroup_id used fileowner() instead of filegroup()
- Guard against empty/false rrdp_public_key before RSA loadPublicKey call
- Return false from decrypt() on base64_decode failure; update return type
- Clone DOM node per loop iteration in rrd_append_cdp_prep_ds (appendChild moves)
- Clone DOM node per loop iteration in rrd_append_value (same DOM move issue)
- Guard mktime() return against false before passing to date()

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

---------

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Co-authored-by: TheWitness <thewitness@cacti.net>
2026-03-06 22:48:24 -05:00
Thomas VincentandTheWitness 6ad201cc4f fix: remove deprecated curl_close() and imagedestroy() for PHP 8.5 (#6772)
* fix: remove deprecated curl_close() and imagedestroy() for PHP 8.5

PHP 8.5 deprecates curl_close(), imagedestroy(), and xml_parser_free().
Handles are automatically freed when they go out of scope.

- cli/add_site.php: remove 2 curl_close() calls
- lib/auth.php: remove curl_close() in secpass_check_pwned()
- lib/plugins.php: remove curl_close() in plugin fetch
- lib/reports.php: remove 2 imagedestroy() calls in jpeg2png/png2gif
- lib/rrd.php: remove 3 imagedestroy() calls in graph rendering

No behavioral change; resources are freed implicitly.

Made-with: Cursor

* test: add Pest tests for PHP 8.5 curl_close/imagedestroy removal

- Php85DeprecationsTest: static checks that no curl_close or imagedestroy
  calls remain in the 5 changed files
- ReportsImageConversionTest: png2jpeg/png2gif produce valid output
  without explicit imagedestroy (resources freed on scope exit)

Made-with: Cursor

---------

Co-authored-by: TheWitness <thewitness@cacti.net>
2026-03-06 08:55:54 -05:00
f0b5baad90 fix: xport empty data handling + add Pest unit tests for sanitization and validation (#6683)
* cleanup: remove unreachable defensive error logs in aggregate graph item validation

* fix: log and surface error when rrdtool xport returns empty data

Add error logging in rrdtool_function_xport() when xport returns
no data. Update graph_xport.php to handle empty results gracefully.

Add Pest unit and integration tests for graph xport error handling,
string functions, serialization security, and HTML escaping.

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

* fix: escape shell args in xport integration tests

Use escapeshellarg() for all interpolated paths in rrdtool commands
to prevent breakage on systems with spaces in temp directory paths.

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

* chore: drop unrelated lib/api_aggregate changes

Keep PR scope focused on the issue for this branch.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

* chore: narrow xport PR to xport-focused changes

Drop unrelated shared test and harness updates so this branch only carries xport handling and its focused unit coverage.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

---------

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: TheWitness <thewitness@cacti.net>
2026-03-05 20:07:51 -05:00
5180e7974b Refactor/consolidate gprint cases (#6704)
* cleanup: remove unreachable defensive error logs in aggregate graph item validation

* refactor: consolidate duplicate GPRINT switch cases in rrd.php

Merge GRAPH_ITEM_TYPE_GPRINT_AVERAGE and GRAPH_ITEM_TYPE_GPRINT_LAST
into a single fall-through case since they share identical logic.

Add Pest tests verifying GPRINT case consolidation behavior.

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

* chore: drop unrelated lib/api_aggregate changes

Keep PR scope focused on the issue for this branch.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

---------

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: TheWitness <thewitness@cacti.net>
2026-03-05 20:02:37 -05:00
3e1dc3c886 fix: replace strtolower/strtoupper with locale-independent wrappers (#6717)
* cleanup: remove unreachable defensive error logs in aggregate graph item validation

* fix: replace strtolower/strtoupper with locale-independent wrappers

PHP 8.5 deprecates locale-sensitive strtolower() and strtoupper().
Add cacti_strtolower() and cacti_strtoupper() wrappers that use
mb_strtolower/mb_strtoupper with explicit UTF-8 encoding, then
replace all 185 call sites across 65 files.

Existing mb_strtolower() calls (email normalization) are left as-is
since they already specify encoding explicitly.

Includes 18 Pest tests covering ASCII, UTF-8, protocol names, config
values, hex strings, NaN detection, and file extension patterns.

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

* fix: correct docblock references and month_shift() strpos check

Fix docblocks to reference strtolower()/strtoupper() (the deprecated
PHP builtins) instead of the wrapper names. Fix month_shift() to use
strpos() !== false instead of > 0 so 'month' at position 0 is matched.

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

* chore: drop unrelated lib/api_aggregate changes

Keep PR scope focused on the issue for this branch.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

---------

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: TheWitness <thewitness@cacti.net>
2026-03-05 19:51:02 -05:00
TheWitness 214ea7e58d Fixing rrdtool export issues in test (#6673) 2026-02-27 10:53:55 -05:00
TheWitness 27919c50d7 Updating Copyrights to 2026 (#6578) 2026-01-28 16:11:10 -05:00