mirror of
https://github.com/Cacti/plugin_thold.git
synced 2026-10-25 12:36:53 +09:00
develop
7
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 TheWitnessandCopilot
|
9b217e0e3a |
INFO: set compat to 1.2.29 (#838)
* INFO: set compat to 1.2.32 * docs: update Copilot instructions Cacti baseline to 1.2.32 (matches INFO compat) * INFO: back off compat to 1.2.29 * Add manifest.json + upgrade-time file pruning * Prune phpunit.xml on upgrade and ignore markdown-lint configs phpunit.xml ships in the repo for CI but is a dev-only test artifact, so it is removed from manifest.json 'expected', pruned from installs on upgrade (like tests/), and excluded from the manifest drift check. The retired markdown-lint configs (.mdlrc, .md_style.rb) are deleted, and .md* is now ignored like .git*: protected from pruning and excluded from drift if it reappears. * Move thold_functions.php and thold_webapi.php into includes/; flatten css The main threshold library becomes includes/functions.php and the web API helper becomes includes/webapi.php; every caller (setup.php, includes/*, the CLI/poller/notify entry points and the test suite) loads them from includes/. Each per-theme stylesheet css/<theme>/main.css becomes css/<theme>.css so users can drop their own css/<theme>.css into the css base, and the page_head hook looks there. Old paths are tombstoned so existing installs drop the stale copies on upgrade. * Regenerate cacti.pot for the includes/ file relocations Moving the plugin's library files into includes/ changed the source-reference lines in the translation template; regenerate it so the 'Verify translation template' CI check passes. * Harden the hook-include test and make RPN integer ops explicit The HookFunctionsIncludeTest invokes real hooks with placeholder input purely to execute their relocated include_once lines; it now swallows engine notices/warnings/deprecations during those calls (and restores the error handler and config afterward) so it cannot trip the suite's failOnDeprecation or leak state into other tests. The RPN % and ^ operators now cast their operands to int explicitly, matching their long-standing integer-truncation behaviour while silencing the implicit float-to-int conversion deprecation. * Harden prune against tampered-manifest path traversal The upgrade-time prune now rejects any tombstone containing '.'/'..' segments (which could escape the plugin directory or resolve to its root) and treats ancestors of whitelist entries as protected, so a tombstone on a parent directory can no longer delete a whitelisted file beneath it. * docs: update and align the Project Structure block Point the structure tree at the relocated files (libraries now under includes/, data files under docs/, stylesheets under css/) and align the trailing '# ...' comments to a single column so they no longer drift right. * docs/tests: standardize test header and correct manifest-contract notes - Use the full license header (from the plugin's own setup.php) in tests/Unit/PruneFilesTest.php instead of the abbreviated copyright banner. - Document that the manifest drift check and the upgrade-time prune also handle phpunit.xml and .md* files, matching the implemented behavior. * refactor: rename prune helpers to the plugin-slug function prefix The repository naming contract reserves the plugin_<name>_ prefix for plugin lifecycle / hook-registration functions; all other functions use the plain <name>_ prefix. Rename the internal upgrade helpers accordingly: plugin_<name>_prune_files() -> <name>_prune_files() plugin_<name>_rmtree() -> <name>_rmtree() The call site, unit tests, and the copilot-instructions.md references are updated to match. No behavioral change. * chore: set the Cacti compatibility floor to 1.2.29 Align the compat metadata (and the contributor-guide references) with the team decision to standardize the minimum supported Cacti version at 1.2.29; the previously proposed 1.2.32 floor was not adopted. --------- Co-authored-by: Copilot <copilot@users.noreply.github.com> |
||
|
TheWitness
|
899aa06028 |
chore: harmonize CI workflow, templates, and test structure (#825)
* chore: harmonize CI workflow, templates, and test structure
Part of the fleet-wide plugin_* harmonization effort.
- Pin actions/checkout, shivammathur/setup-php, actions/upload-artifact to
current release commit SHAs (was floating @v4/@v2).
- Stop logging the MySQL root password in CI output (cat ~/.my.cnf ->
chmod 600).
- Harden MySQL bootstrap: quoted --defaults-file, grants added for both
'cactiuser'@'localhost' and 'cactiuser'@'127.0.0.1'.
- Add "Restore vendor ownership for Pest" step after Composer install.
- Switch the PHP syntax-check step to the vendor-excluding, null-delimited
find/xargs pattern used in plugin_audit.
- Remove the unused "Configure Apache" step (apache2/libapache2-mod-php are
not installed and not needed).
- Simplify the Pest invocation to rely solely on phpunit.xml's <testsuites>.
- No CodeQL workflow added: this repo has no JavaScript/Python/Ruby content
(PHP only), so there is nothing for CodeQL to scan.
- Consolidate tests/Security/Php74CompatibilityTest.php into
tests/Security/PhpCompatibilityTest.php, now checking for PHP 8.3/8.4-only
syntax (this plugin's floor is PHP 8.2 per the CI matrix).
- Remove tests/TestCase.php and its require in tests/bootstrap-unit.php: not
referenced by any uses(TestCase::class) call in this plugin's tests.
- Add .github/ISSUE_TEMPLATE/{bug_report,feature_request}.md and
.github/PULL_REQUEST_TEMPLATE.md, styled after Cacti/cacti's own templates.
- Document CI/dependency baselines in copilot-instructions.md.
* chore: add i18n build/verify workflow and bump MariaDB floor to 11.8
- Document the locales/build_gettext.sh + cacti.pot workflow in
copilot-instructions.md (Weblate owns per-language .po/.mo sync).
- Add a CI step that regenerates cacti.pot and fails if it is stale,
ignoring the POT-Creation-Date timestamp.
- Mark locales/build_gettext.sh executable.
- Bump the CI MariaDB service image floor to 11.8 (from mariadb:10.6 /
mysql:8.0).
* fix: mariadb:11.8 healthcheck and i18n step permissions
- mariadb:11.8 no longer ships mysqladmin; use mariadb-admin for the
service container healthcheck.
- Run chmod/build_gettext.sh with sudo in the i18n verification step,
since locales/ is owned by www-data by the time it runs.
* chore: regenerate locales/po/cacti.pot with current source strings
Ran the real locales/build_gettext.sh (xgettext/msgmerge/msgfmt) to
refresh the translation template against current source. Per the i18n
workflow documented in copilot-instructions.md, only the regenerated
cacti.pot is committed here; Weblate owns syncing the per-language
.po/.mo files from it.
* fix: restore tests/TestCase.php, still used by classic PHPUnit-style tests
An earlier harmonization commit deleted tests/TestCase.php as
apparently-unused dead code, but ~28 test classes across
tests/Unit use classic 'extends TestCase' inheritance rather than
Pest's uses() helper, so the earlier usage check missed them. This
broke bootstrap for every Pest run with 'Class TestCase not found'.
* fix: satisfy php-cs-fixer short-array/single-quote rules in PhpCompatibilityTest
Matches the project's PSR coding standard (short array syntax, single
quotes for non-interpolated strings) that php-cs-fixer enforces in CI.
* fix: make locales/build_gettext.sh string extraction order deterministic
`find` without `sort` returns filesystem/readdir-order results, which
can differ between machines (e.g. a local regen vs. a GitHub Actions
runner), producing a spurious reordering diff in cacti.pot even when
no strings actually changed. Pipe through `sort` and regenerate
cacti.pot with the now-deterministic order.
* fix: correct TestCase.php encoding (was UTF-16, must be UTF-8)
The restored file was accidentally written as UTF-16LE (a Windows
PowerShell redirection default), so PHP never recognized the
opening <?php tag and the class was never defined, still leaving
'Class TestCase not found' even after the file's restoration.
* fix(ci): remove broken 'Import Thold Plugin Sample Data' step
This step invoked cli/cli_import.php, which does not exist anywhere
in Cacti core (the real script is cli/import_template.php, and even
that expects standard Cacti template XML, not this fixture's custom
thold_hi/thold_low/etc. row format). Nothing in the test suite
actually consumes this imported data, so remove the broken step
along with its non-functional script and fixture rather than build
an importer for a feature with no consumer.
* fix: Address Copilot review findings on the harmonization PR
- tests/Security/PhpCompatibilityTest.php: fixed the #[Override]/#[Deprecated]
attribute regexes to also match the fully qualified (`#[\Override]`,
`#[\Deprecated]`) form, added a realpath() false-guard for the plugin root,
included the relative file path in both RuntimeException messages, added
typed-class-constant and dynamic-class-constant-fetch checks (PHP 8.3), and
restored each()/create_function() removed-in-PHP-8.0 guards that the
consolidation had dropped. Also excludes include/vendor/ (not just vendor/)
from the recursive source scan.
- .github/workflows/plugin-ci-workflow.yml: the "Create MySQL Config" step's
root password is now masked via `::add-mask::` before it's echoed into
~/.my.cnf, so it no longer appears in plaintext in the Actions log (chmod
600 alone only protected the file after creation, not the command's own
echoed source in the log).
* fix: Address remaining Copilot review findings
- CHANGELOG.md: added an entry documenting the CI/template harmonization
change (was previously missing per review feedback).
- locales/po/cacti.pot (+ .po/.mo): regenerated via locales/build_gettext.sh
so the new "Verify translation template is up to date" CI check actually
passes against the current source tree instead of comparing against a
stale committed POT.
- locales/build_gettext.sh: added --from-code=UTF-8 to the xgettext
invocation - without it, xgettext aborts outright on any non-ASCII string
in the source (e.g. a "…" character), silently leaving the previous
cacti.pot in place and defeating the up-to-date check entirely.
- tests/Security/PhpCompatibilityTest.php: dropped the PHP 8.3-only checks
(kept only the PHP 8.4-only ones) and corrected the floor comment, since
this repo's actual CI matrix only runs PHP 8.3/8.4 (no 8.2) - it tracks
Cacti's `develop` branch, not `1.2.x`.
- tests/Security/PhpCompatibilityTest.php: renamed the local
plugin_test_read_source_file() helper to schema_test_read_source_file(),
matching this repo's own naming convention (schema_ prefix for non-
lifecycle helpers).
- tests/Helpers/ThresholdScenario.php: fixed a flush-left statement inside
inMaintenance() to use tab indentation, matching the surrounding code.
- README.md / tests/README.md / copilot-instructions.md: corrected stale
PHP-floor references (7.4/8.1 -> 8.2) and a stale test-filename reference
(Php82CompatibilityTest.php -> the consolidated PhpCompatibilityTest.php)
left over from the harmonization pass.
* fix(test): don't flag jQuery .each()/->each() as removed PHP each()
The each()-removed-in-PHP-8.0 check matched jQuery's $.each(/.each(
calls too, which have nothing to do with the removed PHP global
function. Exclude any each( preceded by . or > via a negative
lookbehind.
|
||
|
Thomas VincentandTheWitness
|
38dec82d55 |
fix: keep counter sample time pairs synchronized (#817)
* fix: preserve counter sample time pairs Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix: preserve samples in daemon and poller paths Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * test: exercise poller sample persistence Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix: bound counter gap recovery Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * style: format counter recovery tests Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix: treat unusable counter baselines as unknown Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix: preserve unknown counter maximum semantics Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * test: cover validated poller interval fallback Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix: retain trustworthy counter baselines Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix: propagate unknown rate samples safely Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix: validate effective counter step Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix: preserve alerts across unknown rate gaps Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix: preserve unknown values through transforms Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix: fail closed on missing expression sources Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix: keep unavailable thresholds eligible Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix: preserve cached expression rates Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix: deduplicate backward-clock warnings Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * test: adopt shared Cacti plugin CI workflow, add Security/Integration coverage - Replace plugin-ci-workflow.yml with the Cacti/plugin_evidence-based template: pinned actions, CACTI/COMPOSER_ALLOW_SUPERUSER global env vars, sudo on every composer command, and an actual Pest execution step (previously missing - this workflow never ran Pest at all). - Remove php-unit-tests.yml; its Docker-based Pest run is now redundant with plugin-ci-workflow.yml's Pest step. - Preserve this plugin's existing, more advanced tests/bootstrap-unit.php, TestCase.php, CactiStubs helper and pinned tests/.cacti-version (1.2.31, strict match) rather than overwriting with plugin_evidence's simpler generic versions - only additive stubs (api_plugin_register_hook/realm, get_current_page, plugin_test_read_source) were appended. - Add tests/Security (SetupStructureTest, Php82CompatibilityTest) and tests/Integration (PluginLifecycleTest covering plugin_thold_version/uninstall/upgrade gating) to round out the Security/Unit/Integration directory convention used across other plugins. - Update phpunit.xml to run all three suites and include setup.php as covered source. * ci: retrigger checks * fix: address Copilot review findings - includes/polling.php: stop manufacturing a numeric zero for an unavailable current sample when thold_consider_unknown_zero is enabled - it was bypassing thold_check_threshold()'s new unavailable-sample guard entirely. Preserve the informational NOTE/debug logging. - thold_functions.php: thold_build_cdef() now fails closed (returns '') when a |query_...| CDEF operand cannot be resolved, instead of silently continuing arithmetic on a manufactured zero and returning a fabricated numeric result. Added TholdBuildCdefTest.php covering the literal, unresolved-operand, and resolved-operand paths. - tests/Unit/TholdCalculateLowerUpperTest.php: updated to match thold_calculate_lower_upper()'s current fail-closed contract (returns '' when the high word is absent/out of range, previously asserted stale pass-through values) and its float return type; added coverage for a non-numeric current value and an out-of-range high word. - .github/workflows/plugin-ci-workflow.yml: drop PHP 8.1 from the matrix (Pest 3 requires PHP >= 8.2) and add a pcov coverage driver to the setup-php step so --coverage-clover does not fail with 'No code coverage driver available'. * ci: replace plugin-ci-workflow.yml whole-cloth from plugin_evidence A prior merge did not cleanly bring in plugin_evidence's current CI template, leaving this file in an inconsistent state. Replaced it entirely with plugin_evidence's .github/workflows/plugin-ci-workflow.yml, with only the light corrections needed for this plugin: - evidence -> thold in checkout path, plugin_manage --plugin, .cacti-version path, and coverage upload path - Install System Dependencies keeps apache2 (needed by the later Configure Apache step; evidence's own template omits it) - Run Pest Unit Tests step drops the trailing tests/Security path arg since thold's phpunit.xml testsuite already covers tests/Security, tests/Unit, and tests/Integration - Actions pinned to their latest release commit SHA (actions/checkout, actions/upload-artifact, shivammathur/setup-php), COMPOSER_ALLOW_SUPERUSER env var and an explicit coverage: xdebug driver added, and sudo added to the composer validate/lint/phpcsfixer/ phpstan invocations for consistency with the rest of the workflow * fix(tests): tolerate the moving 1.2.x/develop branch in bootstrap-unit.php The new workflow's 'Set expected Cacti version' step writes the literal branch name (env.CACTI, e.g. '1.2.x') into tests/.cacti-version, but this file's version check compared it verbatim against the checked-out cacti_version file (e.g. '1.2.32'), throwing on every run. Matches the exception plugin_evidence's bootstrap-unit.php already uses. * fix(ci): run composer unprivileged so Pest's vendor cache is writable Setup Permissions chowns the checkout to www-data:runner (group-writable) before Composer runs. Running composer install as root via sudo left include/vendor owned by root, so the later non-root 'Run Pest Unit Tests' step couldn't create pestphp/pest-plugin-mutate's cache directory (mkdir(): Permission denied). Composer no longer needs sudo here: the runner user is already a member of the runner group with write access from Setup Permissions, so dropping sudo leaves vendor/ owned by runner and writable by the later Pest step. Applied the same fix to the (currently dead, CACTI==1.2.x-gated) lint/phpcsfixer/phpstan steps for consistency. * fix(ci): restore vendor ownership for Pest instead of running composer unprivileged Running composer without sudo failed differently: Setup Permissions chowns the whole checkout to www-data:runner before Composer runs, and composer's own file-touching (utime) on files it does not own fails even with group-write access (touch(): Utime failed: Operation not permitted) - group write only covers content changes, not mtime updates, which require file ownership. Revert to running composer as root (matching Setup Permissions/Install Cacti via CLI's existing pattern) and add an explicit 'Restore vendor ownership for Pest' step that chowns include/vendor and composer.lock to runner:runner right after Composer finishes, so the later non-root 'Run Pest Unit Tests' step can write pestphp/pest-plugin-mutate's cache directory and Pest's own result cache under vendor/. * fix(thold): bind graph_id parameter, quote shell tag substitutions Root-caused and fixed the actual source bugs behind ~40 of the 55 failing Pest tests (the rest are pre-existing/unrelated or test-stub mismatches fixed separately): - get_allowed_thresholds()/get_allowed_threshold_logs(): $graph_id was still interpolated directly into the WHERE clause (`gl.id=$graph_id`) and the row/count queries used unprepared db_fetch_assoc()/ db_fetch_cell(), even though GetAllowedThresholdsTest and OptionalCoreFunctionTest were already written expecting a bound `gl.id = ?` placeholder via db_fetch_assoc_prepared()/db_fetch_cell_prepared(). Added a new $sql_params parameter for the caller's own WHERE-clause bindings (appended before the graph_id placeholder), matching the tests' expected bind order and query text exactly. No existing caller passes $graph_id, so this is behavior-preserving for them. - thold_replace_threshold_tags(): added the $shell parameter TholdReplaceThresholdTagsTest/TholdCommandExecutionTest already call it with. Device/threshold-derived values (DESCRIPTION, HOSTNAME, LOCATION, SITE, CURRENTVALUE, NOTES, DNOTES/DEVICENOTE, EXTERNALID, THRESHOLDNAME) are now escapeshellarg()'d when $shell is true; email/HTML callers are unaffected. thold_command_execution() now passes $shell=true when building the exec()/queued command for all three breach directions - the exec() call sites already carried a comment claiming "cacti_escapeshellarg protection" that did not actually exist. - NotificationQueueClaimTest: cacti_log() calls are now recorded into CactiStubs::$calls (added elsewhere in this PR, needed by TholdGetCurrentvalTest), which added an extra 'cacti_log' entry to 3 assertions that expected an exact list/emptiness of $calls. Updated the 3 expectations to include it. - NotificationEmailDeduplicationTest: stubbed the wrong function (db_fetch_assoc instead of db_fetch_assoc_prepared, which is what process_device_notifications() actually calls), so the fixture record was never returned and no mail was ever built. Fixed the stub name; the sha256 dedup path it asserts on was already implemented correctly. Verified locally against a real Cacti 1.2.x checkout with a Windows PHP CLI (GetAllowedThresholdsTest, OptionalCoreFunctionTest, NotificationEmailDeduplicationTest, TholdReplaceThresholdTagsTest all green; TholdCommandExecutionTest and 3 NotificationQueueClaimTest cases only fail locally on Windows due to escapeshellarg()'s CRT-quoting convention and missing /bin/*, posix_*, LOG_LOCAL0 - all Linux-only, matching the actual CI environment). * fix(thold): correct command-output log topic, harden THOLDTYPE test isolation - thold_command_execution() called thold_process_command_output() with topic 'thold' but the function's switch only matches 'thold_cmd'/ 'thold_dhost_cmd'/'thold_uhost_cmd', so cacti_log() was silently never called for inline (non-queued) command execution. Fixed the topic string, restoring the exit-status/output logging TholdCommandExecutionTest's inline-execution tests check for. - TholdReplaceThresholdTagsTest::setUpBeforeClass() loaded includes/arrays.php via the shared require_once-based test loader, which is a no-op if an earlier test class in the same run already loaded the same real path, so \ is not guaranteed to reach this class's global scope. Force a fresh include as a fallback so <THOLDTYPE> substitution is not dependent on suite run order. * fix(thold): fix RPN evaluator crashes and silent bad-result pushes - The v1==0 && v2==0 && operator=='/' special case (0/0 -> 0) had a stray break that exited the switch before the array_push() below it ran, so the special-cased result was popped off but never pushed back, leaving the stack empty instead of [0]. - The zero-divisor guard only covered '/', so '%' with a zero divisor fell through to eval('... % 0'), which throws an uncaught DivisionByZeroError on PHP 8 instead of flagging rpn_error. Extended the guard to also cover modulo. - Unary functions (SQRT, LOG, etc.) evaled their operand without checking it was numeric first, so a non-numeric operand (e.g. the 'U'/unknown sentinel) produced an undefined-constant Error instead of flagging rpn_error. Added the same is_numeric guard the binary path already had. - Unary functions that produce a mathematically undefined result (sqrt of a negative, log of zero/a negative) silently pushed NAN/-INF onto the stack instead of flagging an error, which would have compared as false against every threshold. Now flags rpn_error and drops the result instead of pushing it. * fix(thold): log time-based restoral even when it does not cross the retrigger threshold thold_check_threshold()'s time-based arm (case 2) only logged a restoral (ST_NOTIFYRS) inside the two branches that also send a notification, each gated on the freshly re-queried failure/warning-failure counts reaching their trigger. When a threshold had been alerting but the fresh counts did not reach trigger again (the common case, since those counts measure consecutive alert rows in the log rather than the prior persisted state), execution fell into the bare final else, which reset the fail counters but never logged the restoral or cleared thold_alert - so an operator never saw the all-clear in the log and the threshold stayed marked as alerting. Added a branch that logs ST_NOTIFYRS and resets thold_alert to normal whenever the threshold was previously alerting, without sending a notification, matching this arm's documented behavior of logging every restoral while only mailing on retrigger. * fix: address remaining Copilot review findings on PR #817 - get_current_value(): return missing_value instead of round()-ing a non-numeric ('U'/'nan'/blank or fail-closed CDEF) sample, avoiding a PHP 8 TypeError and manufactured-zero alerting. - PluginLifecycleTest: reset CactiStubs in beforeEach so assertions on global call history/queued returns do not depend on suite order. - README: restore the notification-queue retry/worker-claim docs that were dropped instead of retained alongside the new sampling section. * security: fail-closed RPN evaluator errors; shell-escape thold_expand_string() thold_functions.php: - thold_calculate_expression() converted an RPN evaluation error (unbalanced expression, unsupported token, divide/modulo-by-zero) into a numeric 0 return. 0 passes is_numeric(), so includes/polling.php's fail-closed "unavailable sample" guard (which only catches non-numeric values) never saw it, and the poller could alert/recover on a manufactured zero instead of treating the reading as unavailable. Both error returns now return '' instead, matching every other early-return failure path already in this function. - thold_expand_string() gained a $shell parameter (mirroring thold_replace_threshold_tags()'s existing $q()/escapeshellarg() pattern). It previously called Cacti core's expand_title() unconditionally, which substitutes |host_*|/|query_*| tokens (hostname, SNMP community/password, sysDescr/sysContact/sysLocation, polled data-query fields, etc) with no shell escaping. Any of those values can be set by the polled device itself, so thold_command_execution() calling thold_expand_string() after thold_replace_threshold_tags($shell=true) reintroduced shell injection despite the initial hardening, since expand_title() ran unescaped on the already-quoted command string. In $shell mode, each |host_*|/|query_*| token in the string is now resolved in isolation through the same core substitute_host_data()/ substitute_snmp_query_data() helpers expand_title() itself uses (so the resolved value matches), then escapeshellarg()'d before it's substituted into the command. The plugin's own direct substitutions (data source name/description, graph title, interface speed) and its thold_substitute_custom_data()/thold_substitute_host_data() helpers are now shell-aware the same way. Non-shell callers (email/HTML rendering) are unaffected; thold_command_execution()'s three exec() paths now pass $shell = true. tests/Unit/TholdCalculateExpressionTest.php: updated the two existing error-path assertions from 0 to '', and added divide-by-zero/modulo-by-zero coverage. tests/Unit/TholdExpandStringTest.php + tests/bootstrap-unit.php: added substitute_host_data()/null_out_substitutions() stubs and a test asserting a malicious |host_description| value is escapeshellarg()'d in shell mode and left untouched otherwise. * security: shell-escape the <URL> tag in trigger commands The <a href='...'>...</a> markup substituted for <URL> contains literal single quotes. In \ mode this landed in the command unquoted, so the quotes could terminate the shell argument early (and any metacharacters in the configured base_url would remain executable) despite the rest of thold_replace_threshold_tags()'s hardening. The whole substituted value is now passed through the same \() helper as every other tag, quoting it as one token in shell mode while leaving email/HTML output unchanged. Added a regression test. * fix: address remaining Copilot review findings on PR #817 (round 2) - Guard modulo against fractional zero divisors: the % operator truncates both operands to int, so a divisor like 0.5 became 0 and reached eval(), throwing an uncaught DivisionByZeroError instead of failing closed. - Shell-escape <DSNAME> in thold_replace_threshold_tags(); every other substituted value already went through the $q() helper in shell mode. - Apply the same lastchanged/acknowledgment restoral side effects in the early-restoral branch of thold_check_threshold() as the two full restoral branches above it, so an acknowledged time-based alert that recovers before either failure count retriggers no longer stays acknowledged. - Log daemon sample-persistence failures unconditionally via thold_cacti_log() instead of the --debug-gated thold_daemon_debug(), so sample loss is visible in normal daemon operation. - Restore the changed-line coverage gate the shared CI workflow dropped when it replaced the old Docker-based Pest workflow: fetch full history for the plugin checkout and run tests/bin/patch-coverage.php against the PR base SHA. Allowlist thold_process.php (daemon entry point, cannot be required into the isolated unit test process). - Add/extend unit tests covering each of the above. * fix(ci): measure includes/polling.php for changed-line coverage phpunit.xml's <source><include> only listed thold_functions.php and setup.php, so Clover never recorded hits for includes/polling.php even though it has no top-level side effects and is already require()'d directly by PollerSchedulingTest. patch-coverage.php then correctly flagged this PR's changes to thold_poller_output() (the new thold_polling_sample_row()/status-row insert path, rrd_heartbeat column) as changed-but-unmeasured production lines and failed the build. Add includes/polling.php to the source include list, and add tests exercising thold_poller_output()'s non-daemon path for both outcomes: a numeric reading persisted as a sample_row, and an unavailable reading persisted as a status-only row, plus the post-write cleanup call. * test: cover remaining changed lines flagged by patch-coverage Adds/extends unit tests for lines the coverage gate flagged as changed but unexercised: - includes/polling.php: the thold_consider_unknown_zero "would have been zero" debug/log branch in thold_poller_output()'s non-daemon path (still preserves the unavailable state either way). - thold_functions.php: thold_substitute_host_data()'s cache-hit and build-fresh (|host_management_ip|) paths, and thold_substitute_custom_data()'s shell-escaped custom value - neither had any direct test coverage before. New TholdSubstituteHostDataTest. - thold_functions.php: thold_expand_string()'s |query_ifSpeed| fallback, the SNMP-query-resolved host/query token branch in $shell mode, and the top-level |host_management_ip| fallback that reads $device_id from Cacti's real lib/variables.php (a side effect of an include() inside the function's own scope). The last of those needs its own one-off library_path fixture rather than the shared one, since include_once() means the shared fixture's injected value would only ever take effect on the first call across the whole test run. - thold_functions.php: get_current_value()'s fail-closed return for a non-numeric stored RRD value (matches the existing empty-value-series contract already covered next to it). * fix: gate sample pair advancement on interval eligibility; resolve tags before quoting; add complete headers * test: cover rrd_heartbeat/rrd_step fallback branches in sample eligibility * fix: re-anchor stale rate baselines and stop re-scanning substituted values thold_sample_persistence() froze lasttime/oldvalue once a sample arrived after a heartbeat-exceeding gap, but thold_get_currentval() already fails that cycle's rate closed using the pair as it stood before this call, so freezing bought no extra protection while permanently disqualifying every later poll (whose elapsed time is measured against that same frozen lasttime and only grows). Always re-anchor to the current sample instead; thold_sample_interval_eligible() now only decides whether to log the re-anchor. thold_substitute_custom_data() and thold_expand_string() (both its host/query token loop and its |graph_title|/|data_source_description|/ |data_source_name| substitutions) quoted each value and then applied it with str_replace(), one token at a time. A later str_replace() call re-scans the whole string, including text just inserted by an earlier one, so a value that happens to contain another token's literal placeholder could be substituted a second time outside of its own quoting. Build a token => quoted-value map per substitution site and apply it in a single strtr() pass instead, same as thold_replace_threshold_tags(). Also switched thold_expand_string()'s and thold_substitute_custom_data()'s shell-escaping to the established cacti_escapeshellarg() wrapper instead of raw escapeshellarg(), matching every other quoting site in this file. Added regression coverage: a second poll recovering a real rate after a stale-gap re-anchor, and a re-scan test for each of the three affected substitution sites. * fix: close cross-phase/cross-function shell re-scan and log flooding thold_expand_string()'s per-phase strtr() passes were each individually re-scan-safe, but a value resolved in one phase (host/query, custom data) could still contain another phase's token literal text (e.g. a device description of |data_source_name|) and get re-substituted by that later phase, breaking out of the earlier phase's quoting. Separately, thold_command_execution() called thold_replace_threshold_tags() (quoting <TAG> values) and then passed the result into thold_expand_string(), which re-scans its input for |pipe| tokens - a <TAG> value containing a literal pipe token (e.g. a description of |host_hostname|) was expanded a second time with the real, unescaped device value spliced inside the tag's quoting. Introduce thold_defer_placeholder(): every substitution that needs shell quoting now inserts an opaque, non-token placeholder instead of its quoted value immediately, and records the real value in a map. Thread that map through thold_expand_string(), thold_substitute_custom_data(), thold_substitute_host_data(), and thold_replace_threshold_tags(); each resolves its own map internally when called standalone (preserving existing behavior/tests), or accepts a caller-supplied map by reference to defer resolution further. thold_command_execution() now expands pipe tokens first, then threshold tags, sharing one deferred map across both calls and resolving every placeholder in a single final strtr() only after all token scanning for the command is complete. Also stop thold_sample_persistence()'s stale-gap re-anchor warning from firing on every poll of a persistently stale source: it now only logs on the transition (prior lastread was still numeric), matching thold_log_unavailable_transition()'s own transition-only pattern. Renamed testStaleGapDoesNotAdvanceARateBearingPairButOtherCasesStillAdvance to testStaleGapReanchorsARateBearingPairAndAllOtherCasesStillAdvance, since the pair now always re-anchors. Added regression coverage for the cross-phase reorder within thold_expand_string(), the cross-function reorder in thold_command_execution(), and the transition-only re-anchor warning. * fix: preserve unavailable sentinel, prepared sample inserts, PHP 8.4 nullability thold_calculate_expression()'s pipe-token branch converted an empty (failed) thold_expand_string() result to '0' after the resolution attempt, so an unresolved |query_*| or host token became a manufactured valid zero instead of failing the expression closed like every other branch in this function. Drop that fallback; an empty operand now falls through the RPN loop's unsupported-field case and returns '' as intended. thold_polling_sample_row() built its sample row as a raw SQL value tuple string (id, tcheck, quoted lastread, FROM_UNIXTIME(lasttime), quoted oldvalue) for direct concatenation into an unprepared db_execute() call, unlike the status-only row it introduced alongside it. It now returns the same structured fields as the status row, and includes/polling.php builds '(?, ?, ?, FROM_UNIXTIME(?), ?)' placeholders with a flattened parameter list for db_execute_prepared(), matching the status-only path. rray & = null is an implicitly nullable parameter, deprecated in PHP 8.4; phpunit.xml enables failOnDeprecation, so loading this file could fail the PHP 8.4 CI job. Declared explicitly as ?array in all 4 functions that take it. Added a regression test for the unresolved-pipe-token fail-closed fix, and updated the sample-row assertions in TholdGetCurrentvalTest and PollerSchedulingTest for the new structured/prepared-statement shape. --------- Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> Co-authored-by: TheWitness <thewitness@cacti.net> |
||
|
|
aba15fd779 |
security: replace eval() in RPN binary math with safe dispatch (GHSA-vr4v-qvqm-gm9j) (#807)
* security: replace eval() in RPN math binary ops with safe dispatch (S1523)
Replace the eval()-based binary arithmetic evaluation at line 378
with a new thold_rpn_math_binary() helper function that uses a
switch statement for +, -, *, /, %, ^ operators.
The operator was already whitelisted via in_array(, , true)
and operands validated with is_numeric(), but removing eval() eliminates
the code injection risk entirely (CWE-95).
Addresses SonarQube finding: php:S1523 at thold_functions.php:378
* ci: retry transient dependency downloads
* test(rpn): cover safe binary dispatch
* fix: restore SQL binding, trigger-command quoting, and RPN safety fixes
Same set of pre-existing gaps as develop (covered by tests but never
implemented in thold_functions.php), plus one bug specific to this PR:
- get_allowed_thresholds()/get_allowed_threshold_logs(): bind graph_id and
caller-supplied params instead of interpolating them into the SQL text,
and use the *_prepared fetchers.
- thold_replace_threshold_tags(): add a \ flag that quotes
device/threshold free-text values with cacti_escapeshellarg() when the
substituted text is a trigger command.
- thold_command_execution(): pass shell=true for the three trigger
commands, and fix a topic-string typo ('thold' vs 'thold_cmd') that
silently suppressed exit-status logging for inline command runs.
- thold_expression_math_rpn(): remove a stray break that exited the
operator switch before pushing the 0/0 result, flag modulo-by-zero
instead of letting PHP throw DivisionByZeroError, reject non-numeric
unary operands before eval(), and flag SQRT/LOG results that are NAN or
INF instead of pushing them onto the stack (unary math here is still
eval()-based; unrelated to this PR's binary-math dispatcher).
- thold_rpn_math_binary(): this PR's new dispatcher mapped '^' to pow(),
silently changing it from the pre-existing bitwise XOR behaviour
(eval('... ^ ...') always computed XOR). Restored XOR to avoid breaking
existing user thresholds, and corrected this PR's own
TholdRpnBinaryMathTest 'power' dataset, which had asserted the
incorrect pow() behaviour.
* fix(tests): stop includes/arrays.php loading from silently no-op'ing
thold_functions.php includes includes/arrays.php with a plain include()
(not include_once()) from several of its own functions (e.g. thold_log()),
keyed off \['base_path']. Once any test in the shared Pest process
exercised one of those call sites, PHP's include-once registry considered
the file already included: a later thold_test_load() (require_once) on the
same resolved path silently no-op'd and never (re)published \
to \, depending on which order the test files happened to run in.
This intermittently broke every test that reads \
(TholdReplaceThresholdTagsTest's THOLDTYPE substitution) as well as tests
several call frames downstream of thold_log()/thold_check_threshold()
(NotificationEmailDeduplicationTest, ThresholdTimeBasedCharacterizationTest),
depending on file execution order.
Add thold_test_load_always()/loadPluginSourceAlways(), which use a plain
require() instead of require_once(), for plugin files that only assign
file-scope variables (no function/class declarations) and are therefore
safe to load more than once. Switch the four test classes that load
includes/arrays.php to the new helper.
* debug: temporary CI diagnostics for notification/restoral failures
* Revert "debug: temporary CI diagnostics for notification/restoral failures"
This reverts commit
|
||
|
|
dac4a8684e |
security: replace eval() in RPN unary math functions with safe dispatch (GHSA-4mmp-mv2x-m9f6) (#808)
* security: replace eval() in RPN unary math functions with safe dispatch (S1523)
Replace the eval()-based unary math function evaluation at line 403
with a new thold_rpn_math_unary() helper function that uses a switch
statement to dispatch SIN, COS, TAN, ATAN, SQRT, FLOOR, CEIL, DEG2RAD,
RAD2DEG, ABS, EXP, LOG to their native PHP equivalents.
The operator was already whitelisted via in_array(, , true)
and the operand validated with is_numeric(), but removing eval() eliminates
the code injection risk entirely (CWE-95).
Addresses SonarQube finding: php:S1523 at thold_functions.php:403
* ci: retry transient dependency downloads
* test(rpn): cover safe unary dispatch
* fix: restore SQL binding, trigger-command quoting, and RPN safety fixes
Same set of pre-existing gaps as develop (covered by tests but never
implemented in thold_functions.php):
- get_allowed_thresholds()/get_allowed_threshold_logs(): bind graph_id and
caller-supplied params instead of interpolating them into the SQL text,
and use the *_prepared fetchers.
- thold_replace_threshold_tags(): add a \ flag that quotes
device/threshold free-text values with cacti_escapeshellarg() when the
substituted text is a trigger command.
- thold_command_execution(): pass shell=true for the three trigger
commands, and fix a topic-string typo ('thold' vs 'thold_cmd') that
silently suppressed exit-status logging for inline command runs.
- thold_expression_math_rpn(): remove a stray break that exited the
operator switch before pushing the 0/0 result, flag modulo-by-zero
instead of letting PHP throw DivisionByZeroError, and reject
non-numeric binary operands the same way for '%' as for '/' (binary
math here is still eval()-based; unrelated to this PR's unary-math
dispatcher).
- thold_rpn_math_unary()/thold_expression_math_rpn(): reject non-numeric
operands before dispatching, and flag SQRT/LOG results that are NAN or
INF instead of pushing them onto the stack.
* fix(tests): stop includes/arrays.php loading from silently no-op'ing
thold_functions.php includes includes/arrays.php with a plain include()
(not include_once()) from several of its own functions (e.g. thold_log()),
keyed off \['base_path']. Once any test in the shared Pest process
exercised one of those call sites, PHP's include-once registry considered
the file already included: a later thold_test_load() (require_once) on the
same resolved path silently no-op'd and never (re)published \
to \, depending on which order the test files happened to run in.
This intermittently broke every test that reads \
(TholdReplaceThresholdTagsTest's THOLDTYPE substitution) as well as tests
several call frames downstream of thold_log()/thold_check_threshold()
(NotificationEmailDeduplicationTest, ThresholdTimeBasedCharacterizationTest),
depending on file execution order.
Add thold_test_load_always()/loadPluginSourceAlways(), which use a plain
require() instead of require_once(), for plugin files that only assign
file-scope variables (no function/class declarations) and are therefore
safe to load more than once. Switch the four test classes that load
includes/arrays.php to the new helper.
---------
Co-authored-by: Thomas Vincent <thomasvincent@gmail.com>
Co-authored-by: TheWitness <thewitness@cacti.net>
|
||
|
Thomas Vincent
|
860be297c1 |
fix(notify): recover unfinished queue claims (#813)
* fix: recover unfinished notification claims Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * test: cover notification claim recovery branches Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix: harden notification worker recovery Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix: heartbeat notification queue loops Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix: fail closed on notification worker recovery Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix: bound notification worker recovery Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * style: format notification CLI validation Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix: prove notification claim ownership at completion Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix: bound notification completion failures Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix: recover unverifiable stale workers Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * test: keep disabled exec path measurable Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix: verify stale worker age without wall clocks Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix: fail closed on unverifiable live workers Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * test: cover portable process age fallback Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * test: reject invalid process age output Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * Retry failed queued email notifications (#811) * Retry failed queued email notifications * test: cover queued delivery retry paths * style: document notification fixture parameters * fix: batch grouped notification delivery updates * fix: align notification retry status output * refactor: centralize notification retry updates * fix: retain retry queue ownership Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> --------- Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> |
||
|
Thomas Vincent
|
4dd0f87f24 |
refactor(thold): lift the evaluation settings out of the threshold check (#800)
* test: pin the hi/low threshold evaluator's behaviour thold_check_threshold() has no return value: everything it decides is a side effect through seven global Cacti functions. ThresholdScenario builds the fixture those need and runs one poll; ThresholdOutcome reads back what was emitted, so a test asserts on behaviour rather than on the SQL text. No production file is touched. Several assertions record behaviour that is wrong rather than intended, each with a comment saying so, so that fixing it later is a deliberate edit here. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * test: pin the time-based threshold evaluator's behaviour Records where this arm has drifted from the hi/low one, notably that a restoral writes the log row and clears the state but sends no mail, so an operator sees the alert and never the all-clear. Cacti's cell fetchers return false rather than '' when a query matches no row. The stub now does the same: on PHP 8 the difference is a TypeError in the re-alert arithmetic, so the old default invented a failure production does not have. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * test: follow Cacti's test layout and composer scripts Adopts the conventions from Cacti core: tests/bootstrap-unit.php, tests/Helpers for the stubs, a phpunit.xml carrying error_reporting -1 and CACTI_TEST_BOOTSTRAP, and composer lint / test / test:coverage scripts so CI runs the same commands a developer does. The dev toolchain is Cacti's, pinned to the same platform php 8.1.0. Cacti core runs Pest and this suite does not, because pest ^2 does not resolve on PHP 8.1 -- the platform Cacti's own composer.json pins. Releases up to v2.36.0 conflict with phpunit 10.5.62 and later, every earlier 10.x release is blocked by advisory PKSA-z3gr-8qht-p93v, and v2.36.1, which does resolve, requires PHP 8.2. The stack installs on 8.2 and above; 8.1 is the floor this plugin's CI matrix targets. The tests are written in the plain PHPUnit class style that Cacti's tests/Pest.php explicitly supports, so they run unchanged under Pest wherever it is installable. * test: pin the baseline threshold evaluator's behaviour This arm compares the reading against statistics rrdtool reports for a reference window, so the scenario supplies those rather than static bounds. Reaching them means answering the three rrdtool calls thold makes -- file existence, an info block describing the data sources and consolidation functions, and a graph command whose printed values are decoded by position -- which the helper now does. Completes the three arms, so Phase 2 can start moving code. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * perf(thold): drop eight write-only locals from the threshold evaluator Each was assigned and never read. One of them, $baseu, ran a query against graph_templates_graph and discarded the result, so every threshold on every poll cycle paid for a round trip that fed nothing. $show_datasource likewise called thold_datasource_required() for a value nobody used. The others are $global_alert_address, $deadnotify (issue #751), $suffix, $show_units, $units_suffix and $decimals. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * refactor(thold): lift the evaluation settings out of the threshold check The preamble computed the settings the three threshold arms read, so reaching any of that logic meant driving a whole poll. It moves to thold_evaluation_context(), which derives everything from the threshold row and the Cacti settings and decides nothing, and the check unpacks what it needs. Pure motion. The characterization tests from the previous commits pass unchanged, which is what they were written for, and the rules that were buried in the preamble now have direct tests: trigger fallback, whether alerts also reach the warning recipients, and when a graph is attached. thold_check_threshold() goes from 1432 lines to 1378. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * test: load branch helpers without plugin Composer autoload * ci: keep plugin PR integration checks on pinned Cacti * test: tighten characterization fixture metadata and permissions * test: match the optional Cacti database stub signature * ci: bound package index refreshes --------- Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> |