mirror of
https://github.com/Cacti/cacti.git
synced 2026-10-24 03:57:32 +09:00
develop
349
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 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> |
||
|
Thomas VincentandTheWitness
|
391c6ead18 |
Add scoped poller regressions and weekly branch backstop (develop) (#8286)
* test(poller): gate regressions by changes and run weekly scale checks Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * test(poller): satisfy workflow policy and bound cold-cache queries Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix(poller): retry failed template metadata reads Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * docs(changelog): defer release entries to final backlog PR Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> --------- Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> Co-authored-by: TheWitness <thewitness@cacti.net> |
||
|
TheWitness
|
5e870a91df |
security: batch 6 develop - path_php_binary escape (GHSA-4p7f), boost-log .log guard (GHSA-m6wx), tree lock/unlock/get_node authz (GHSA-73qf), rrd create strip (GHSA-967c) (#8148)
* security: batch 6 develop - escape path_php_binary (GHSA-4p7f), boost-log .log guard (GHSA-m6wx), tree lock/unlock/get_node authz (GHSA-73qf), confirm rrd create control-char strip (GHSA-967c) * security: cast path_php_binary before escaping in resource_cache_out (GHSA-4p7f) |
||
|
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>
|
||
|
TheWitness
|
0415606710 |
fix: skip replication-ignored files in cache_in_path() to stop poll-cycle log spam (#8127)
* fix: skip replication-ignored files in cache_in_path() to stop poll-cycle log spam cache_in_path() emitted a 'Detecting Resource Change' log and called update_db_from_path() for tooling artifacts (.gitignore, .mdlrc, tests/, ...) that update_db_from_path() then declines to cache. Because their md5 is never stored, every poll cycle re-detected them as changed, spamming the Cacti log once per minute. Guard cache_in_path() with should_ignore_from_replication() up front so these paths are skipped entirely. * test: assert cache_in_path() consults the ignore predicate before logging/updating Regression guard for the per-poll-cycle log spam fix. cache_in_path() runs change detection before update_db_from_path(), so it must short-circuit on should_ignore_from_replication() up front. Without that early return an ignored dotfile (.gitignore, .mdlrc, ...) is logged and re-processed every cycle because its md5 is never stored and it is forever re-detected as "changed". The new test pins the guard ahead of the first change-detection log line in cache_in_path(). * fix: restore literal <?php opening tag in replication exclusion test The previous commit stored the opening tag as an HTML entity, which would make the test file fail to parse. |
||
|
TheWitness
|
1f9445bb03 |
Exclude repo/tooling metadata files from remote data collector replication (#8120)
* Exclude repo/tooling metadata files from remote replication Add .gitignore, .htaccess.dist, .mdl_style.rb and .mdlrc to should_ignore_from_replication() so these development-only files are never cached into poller_resource_cache nor written to remote data collectors' base and other directories during file replication. * CHANGELOG: use numbered -issue entry in correct order |
||
|
TheWitness
|
ac2445fab5 |
Exclude 'tests' directories from resource replication (#8118)
* poller: exclude 'tests' directories from resource replication should_ignore_from_replication() now skips any path with a 'tests' segment, covering Cacti's own tests/ and every plugin's tests/ for both the cache-in (md5sum_path/update_db_from_path) and cache-out (resource_cache_out) directions. update_resource_cache()'s plugin scan also adds 'tests' to its directory exclusions so a plugin's tests/ tree is never cached in the first place. Adds a Pest unit test covering both directions and the plugin-scan exclusion. * poller: purge stale excluded rows, guard collector dirs, strip install root Address review feedback on #8118: - should_ignore_from_replication() now strips CACTI_PATH_BASE from absolute file paths before segment matching, so an installation living under a 'tests' ancestor (e.g. /srv/tests/cacti) no longer rejects ordinary files. - update_resource_cache() purges rows that are now excluded even when their source file still exists, dropping tests/* rows cached before the exclusion. - The remote collector skips excluded plugin rows before building directories, so it no longer recreates plugins/foo/tests/ from stale cache rows. Adds regression tests for the absolute-path handling and both source-level guards. |
||
|
TheWitness
|
d57431371b |
perf(poller/boost): cached per-data-source field-name lookups, remove batched prefetch (#8078)
* perf(poller/boost): replace batched field-name prefetch with cached per-data-source lookups poller_prefetch_rrd_field_names() (issue#7534) batched the unused/nt field-name lookups into large IN() lists and rebuilt them on every process_poller_output()/boost_process_local_data_ids() call, so the completion-wait loop re-ran them each pass (DB thrash) and the IN() lists risk crashing some MariaDB/MySQL releases and do not scale to multi-million-data-source installs. Option B (full): remove poller_prefetch_rrd_field_names() and replace it with two small per-data-source helpers - poller_get_unused_data_source_names() and poller_get_nt_rrd_field_names() - each a single indexed WHERE local_data_id = ? query cached by local_data_id in a caller-held static, so every data source is queried at most once per poller process. Templated field names resolve from the static poller_data_template_field_mappings cache (the Data Template dictates); only non-templated (data_template_id == 0) manual sources hit the per-instance query - a rare exception. Both process_poller_output() (lib/poller.php) and boost_process_local_data_ids() (poller_boost.php) now use the helpers. Added a WARNING documentation block to both bulk hot-path functions spelling out the query-discipline rules for future maintainers/AI (minimal queries, no query in the per-row loop, avoid large IN() lists and full-table scans, single pass, static template-dictated caching). Updated BoostProcessBugsTest assertions to the new helpers and removed the now-obsolete BoostPrefetchRrdFieldNamesTest integration file. * fix(poller/boost): split GROUP_CONCAT'd field mappings; guard orphan-name query to templated sources; add coverage * boost: normalize orphan-name guard to ternary at third call site The reset_template branch in boost_process_local_data_ids() guarded the poller_get_unused_data_source_names() call with an if/else block rather than the ternary used at the other two call sites. Normalize it so all three sites read identically and the template-id guard is uniform. |
||
|
TheWitness
|
a52368125e |
feat(auth): rename User Domains to Login Providers, add SAML2 and OpenID Connect (#8066)
* feat(auth): rename User Domains to Login Providers Collapses user_domains/user_domains_ldap into a single login_providers table with a JSON parameters column, migrated in place during the 1.3.0 upgrade. Renames the admin page, menu, breadcrumbs, and help links to match. * feat(auth): rework login providers as Cacti\Auth classes Introduces LoginProviderInterface with credential (LDAP/AD) and redirect (SAML2/OpenID) flavors, replacing the procedural domains_login_process()/domains_ldap_*() functions. Adds blank-field group membership gating shared by every provider type. * feat(auth): add SAML2 and OpenID Connect login providers Adds onelogin/php-saml and firebase/php-jwt, a new cacti_http() SSRF-hardened outbound request helper for every OpenID discovery/token/JWKS call, and login_sso.php to handle the redirect-based SSO flow. Login page now shows a button per enabled SAML2/OpenID provider, and per-provider auth cookie opt-out is enforced on both the password and SSO login paths. * chore(i18n): regenerate cacti.pot * feat(auth): add Local and Basic auth as Cacti\Auth providers LocalAuthLoginProvider (CredentialLoginProviderInterface) and BasicAuthLoginProvider (new PreAuthenticatedLoginProviderInterface, since Basic Auth has no password for Cacti to check) are thin adapters over the existing local_auth_login_process()/basic_auth_login_process() functions, which are left untouched to avoid disturbing their regression-tested lockout/timing/rehash behavior. * feat(auth): move local password verification into LocalAuthLoginProvider secpass_login_process() and local_auth_login_process() are retired; their lockout, constant-time verification, forced-password-change, and rehash-on-login logic now live end to end in the class (verifyCredential/enforcePasswordPolicy/rehashIfNeeded). secpass_check_pass() stays a shared lib/auth.php function since password change/reset/user_admin also call it outside any login flow. Local auth still has no login_providers row: isEnabled() is always true and it cannot be disabled or deleted. * feat(auth): move Login Provider CRUD into Cacti\Auth classes AbstractLoginProvider gains static deleteById()/enableById()/disableById()/makeDefaultById()/persist(); each concrete provider owns its own collectParameters(), dispatched via LoginProviderFactory. login_providers.php's form_save()/form_actions() call these instead of doing raw SQL, and the retired provider_remove()/provider_disable()/provider_enable()/provider_default()/provider_collect_parameters() procedural functions are gone. Local/Basic auth are unaffected: they never extend AbstractLoginProvider, so they structurally cannot expose delete/disable. * fix(auth): address Copilot review findings on SSO/SAML/OIDC security Fixes callback/ACS/SLS/metadata URLs that used the raw provider id instead of the login realm (broke every SSO login), adds InResponseTo correlation for SAML and sub/azp validation for OIDC, requires HTTPS for OIDC discovery/endpoints, pins cacti_http() connections against DNS rebinding via CURLOPT_RESOLVE, exempts login_sso.php from the authenticated-page redirect guard and gates it on auth_method, wraps provider dispatch in a exception boundary, adds a dedicated SAML SLO path instead of routing it through login completion, makes remember-me an explicit opt-in for SSO, fixes disabled/deleted providers not revoking existing auth cookies, restricts the default-realm action to LDAP/AD, and fixes the 1.3.0 migration silently enabling group enforcement for installs that had it off. Also updates the E2E seed script for the login_providers schema. * chore(auth): refresh CI security baselines and document no-op validation Regenerate the sink inventory baseline for the new/renamed lib/Auth/* and login_providers.php files, and add '// no-validation:' comments on the intentionally-unconstrained admin/free-text/secret fields (LDAP DNs/filters, SAML certs/keys, OIDC client credentials, provider display fields) flagged by the validator constraint ratchet. Refreshes the validator baseline to drop the deleted user_domains.php entries. * refactor(ldap): remove dead DB-lookup path from Ldap::__construct The domain_id>0 branch duplicated the JSON-parameter-to-property mapping already owned by LdapLoginProvider::buildLdap(), and nothing constructs Ldap with a real id anymore (all call sites pass 0). Drop the parameter and the branch so Ldap stays a pure connection/protocol class with no DB coupling. Also fixes a latent bug: group_require is a typed bool property, but LdapLoginProvider::authenticate() assigned it an int (1/0) from a strict_types=1 file, which throws a TypeError on every LDAP/AD login attempt. Assign a real bool instead. * fix(auth): fix LDAP login, upgrade ordering, and hardening guard failures LdapLoginProvider compared LdapError::GetErrorDetails()'s int error_num against the string '0' with strict equality, so it never matched and every LDAP/AD login was rejected regardless of valid credentials. Compare against int 0 instead. install/upgrades/1_3_0.php called login_providers_convert_1_3_0() (which reads and then drops user_domains/user_domains_ldap) before the pre-existing legacy ALTER TABLE statements that add the debug/network_timeout/bind_timeout columns those reads depend on, so upgrades failed once the tables were already gone. Moved the call to run after those ALTERs. OpenIdLoginProvider::complete() read state/code from raw \ instead of gnrv(), tripping the hardening pattern guard; switched to the request helper. * style(auth): apply php-cs-fixer formatting and normalize line endings to LF Runs php-cs-fixer across the new lib/Auth/* classes, login_providers.php, login_sso.php, and the other touched files, fixing indentation/alignment/docblock style to match project convention. Also normalizes several files that had picked up CRLF line endings back to the LF the project's .php-cs-fixer.php config (setLineEnding) and the rest of the tracked tree actually use. * fix(auth): address second Copilot review round (9 high/medium findings) - Make login_providers_convert_1_3_0() resumable: CREATE TABLE IF NOT EXISTS and INSERT ... ON DUPLICATE KEY UPDATE so an interrupted upgrade can be re-run instead of leaving a half-migrated, unrecoverable table. - SamlLoginProvider::complete() now fails closed when no saved AuthnRequest ID exists instead of passing null to processResponse(), which skipped InResponseTo validation and accepted unsolicited IdP-initiated assertions. - LoginResult gains an isCredentialFailure flag; only a genuine wrong-password LDAP bind (LdapError::Failure) counts toward the lockout counter now, not connection errors or group-membership denials. - Fixed button_label/debug being read from the JSON parameters column in OpenIdLoginProvider/SamlLoginProvider/LdapLoginProvider when they are actually top-level login_providers columns; AbstractLoginProvider now hydrates and exposes both. - Removed SamlLoginProvider's wantMessagesSigned, which silently required the outer SAML message (not just the assertion) to be signed, rejecting IdPs that only sign the assertion as the UI promises. - Added getTemplateUserId() to CredentialLoginProviderInterface and implemented it on LocalAuthLoginProvider so the contract (not just incidental AbstractLoginProvider inheritance) guarantees the method lib/auth.php calls. - cacti_http()'s CURLOPT_RESOLVE entries now bracket IPv6 literals correctly, and safe-IP validation strips parse_url()'s brackets before checking, fixing IPv6 target pinning. - login_providers.php's type dropdown now swaps the Full Name attribute default (cn/displayName) on an actual user-driven type change, not on page load, so a new Active Directory provider does not silently save the LDAP default. - Extracted auth_apply_group_login_opts() and call it from both auth_login.php and login_sso.php, so SSO logins honor a user's group-derived login_opts the same way password logins already do. * fix(auth): fix PHPStan level 8 errors and pest harness stub drift Test harness fix: the LoginProvidersLoginRealmValidationTest stub LoginResult class was missing the isCredentialFailure property added to the real class, causing 'Undefined property' warnings that broke the harness's JSON output. Added the property and set it on the simulated bad-password failure. PHPStan (48+ errors, all newly introduced by this branch's rewrite, never actually verified until now): narrowed db_fetch_row_prepared()/db_fetch_assoc()'s array|bool (including bare true, which survives a '?: []' short-circuit - fixed with explicit is_array() checks instead) at every call site across auth_login.php, login_sso.php, lib/auth.php, lib/Auth/LocalAuthLoginProvider.php, lib/Auth/LoginProviderFactory.php, and install/upgrades/1_3_0.php. Fixed OpenIdLoginProvider::discover()'s return shape to match its declared docblock instead of returning the raw decoded JSON. Fixed lib/functions.php's cacti_http() to narrow curl_exec()'s string|bool result with is_string() (not ===false, which still leaves 'true' as a residual type) and guard / as non-empty for curl_setopt_array()'s stubs. Fixed a real bug in SamlLoginProvider::processLogout(): processSLO(true) was passing 'true' as \ (the first parameter) instead of \ (the fifth), so it always redirected/exited internally with the default \=false, making Cacti's own logout.php never regain control. Now calls processSLO(keepLocalSession: true, stay: true) and is typed ': never' to match; login_sso.php's now-genuinely-unreachable break statements after processLogout()/initiate() are removed. Fixed login_providers.php passing a expression (not a variable) to inject_form_variables()'s by-reference parameter. Verified: PHPStan analyse clean (0 errors), full Pest Unit/Security suite passes (128 tests), hardening guard/sink inventory/validator ratchet all still clean. * fix(auth): fix ambiguous column and array+bool crash in user_log filter The user filter dropdown query selected a bare 'id' column after joining user_auth and login_providers, which is ambiguous now that login_providers also has an id column (unlike the old user_domains.domain_id) - MySQL rejected the query outright. Qualified as ua.id. Also narrow db_fetch_assoc()'s array|bool result before the \ + \ + \ merge, which was throwing a TypeError whenever the query failed instead of surfacing the real SQL error. * fix(auth): address third Copilot review round (5 findings + tests) - AbstractLoginProvider::persist() now disables the template account for every provider type; SAML2/OpenID also copy it via user_copy() in login_sso_complete() and were left directly usable as a local login. - cacti_http_resolve_safe_ips() now detects IPv4-mapped IPv6 literals (::ffff:0:0/96) via inet_pton() and validates the embedded IPv4 address, closing an SSRF bypass via e.g. ::ffff:169.254.169.254. - login_providers_convert_1_3_0() now tracks db_install_execute()'s status for every row and only drops the legacy user_domains/user_domains_ldap tables once every row (and the final row count) is confirmed migrated. - Ldap::Getcn() now looks up ldap_get_entries()'s always-lowercased attribute keys case-insensitively while still storing the result under the originally-requested key, fixing AD's displayName (and any other mixed-case configured attribute) always resolving to a blank full name. - SamlLoginProvider::processLogout() now redirects to the LogoutResponse URL processSLO() returns for a LogoutRequest (stay:true), instead of discarding it and always redirecting to Cacti's own logout page - federated logout was never actually acknowledged to the IdP. - Added tests/Unit/Security/Auth/OpenIdLoginProviderTest.php (13 tests) covering verifyIdToken() signature/issuer/audience/azp/nonce validation, discover()'s HTTPS enforcement, and complete()'s callback-state and UserInfo-subject-matching checks, against a locally-generated RSA keypair/JWKS. - Regenerated the sink inventory baseline for the new header_redirect call in SamlLoginProvider::processLogout(). Verified: PHPStan clean, full Pest Security/Auth suite passes (830 tests including the 13 new ones), php-cs-fixer/hardening-guard/sink-inventory/validator-ratchet all clean. |
||
|
TheWitnessandcopilot-swe-agent
|
2d516ca0a9 |
security: consolidate additional verified GHSA fixes for develop (batch 2) (#8065)
* security: contain data source paths within the RRA directory (GHSA-9qff-xccr-rrc5)
* security: escape executable path in exec_background on non-win32 (GHSA-cv7j-p6fh-j4q7)
* security: stop disabled accounts authenticating via remember-me cookie and revoke on disable (GHSA-pqrp-v752-mc4f)
* security: prefer base_url over client Host for the force_https redirect (GHSA-3466-fp7g-c9rf)
* changelog: add fragments for GHSA-9qff-xccr-rrc5, GHSA-cv7j-p6fh-j4q7, GHSA-pqrp-v752-mc4f, GHSA-3466-fp7g-c9rf
* security: address review findings on GHSA batch 2 (develop)
- lib/dsstats.php, poller_maintenance.php: route through
get_data_source_path() instead of expanding data_source_path directly,
so the RRA containment check applies to these consumers too (GHSA-9qff)
- lib/functions.php: data_source_path_within_rra() now also validates
the realpath of each existing ancestor segment, closing a symlink
pivot that the purely lexical check missed (GHSA-9qff)
- include/global.php: fail closed instead of falling back to SERVER_NAME
when base_url is unset/invalid, removing the residual open-redirect
path (GHSA-3466)
- lib/html_utility.php: cacti_force_https_host()/
cacti_build_https_redirect_url() now parse a scheme-less base_url
(matching lib/reports.php's own normalization) and preserve a
configured non-default port instead of silently dropping it (GHSA-3466)
- tests: move DataSourcePathContainmentTest.php into
tests/Unit/Security/PathTraversal/, and switch its fixtures from a
fictional CACTI_PATH_RRA path to a real temp directory so the new
realpath()-based checks (including a symlink-pivot regression case)
can actually run
* test: fix CACTI_PATH_RRA constant collision in DataSourcePathContainmentTest
CI showed 2 failures: RrdProxyProtocolTest.php runs earlier in the same
Pest process and already defines CACTI_PATH_RRA to a fictional,
non-existent '/var/lib/cacti/rra', so our own definition guard never
ran and realpath(CACTI_PATH_RRA) returned false for every assertion,
failing the two tests that expect containment to be true.
Now ensure the directory backing CACTI_PATH_RRA actually exists on
disk regardless of who defined the constant first, and gracefully
skip only the fixture-dependent assertions if this process can't
create it (e.g. no permission on a system path).
* test: refresh sink inventory baseline for exec_background rename
|
||
|
TheWitness
|
af8449e487 |
Fix RECACHE ASSERT loop caused by SNMP uptime selection diverging from spine (develop) (#8058)
* fix: match spine's unconditional engine-time preference for SNMP uptime recache (develop) PR #7982 changed cacti_snmp_select_uptime() to prefer whichever of sysUpTime and snmpEngineTime is numerically larger. Spine's own reindex-assert re-check (poller.c) has no such magnitude comparison - it always prefers a numeric engine time, only falling back to sysUpTime when the engine OID isn't present. For any device where snmpEngineTime is legitimately smaller than sysUpTime (e.g. the SNMP agent/engine restarted more recently than the OS, a common, unremarkable scenario), lib/poller.php's update_reindex_cache() now picks sysUpTime as the recache baseline while spine always re-derives its live comparison value from the smaller engine time. Every poll then reports "RECACHE ASSERT FAILED" and queues a reindex; Cacti recomputes the baseline the same way, so it never converges - an infinite RECACHE ASSERT loop that looks like a spine bug even though spine is unchanged. Fix: add an opt-in $prefer_engine_time parameter to cacti_snmp_select_uptime() that keeps the existing wall-clock rejection (issue#7342) but skips the "prefer whichever is larger" comparison, matching spine's rule exactly. Use it for the two call sites that must stay consistent with spine's own live re-check: lib/poller.php's recache baseline, and both uptime reads in cmd.php (update_system_mibs()'s host.snmp_sysUpTimeInstance update, and ping_and_reindex_check()'s own PHP-poller equivalent of spine's reindex re-check). The display-only call sites (lib/api_device.php, lib/api_automation.php) keep the existing wrap-aware "prefer larger" logic, since they aren't compared against a persisted, spine-shared baseline. Ported from the 1.2.x fix (#8057). Added tests covering the new parameter and asserting both call sites opt into it. * test(snmp): cover prefer_engine_time in the coverage-gated suite phpunit-snmp.xml's "SNMP coverage" testsuite only runs SnmpCoverageTest.php, not SnmpUptimeSelectionTest.php where the new $prefer_engine_time assertions were added. The engine-time-preference branch in cacti_snmp_select_uptime() was never executed under the 100% line coverage gate, so the "lib/snmp.php 100% coverage" check failed at 99.8%. Mirror the same prefer_engine_time assertions into SnmpCoverageTest.php so the coverage-gated suite exercises the new branch. * fix: match spine's lack of wall-clock awareness for prefer_engine_time paths The wall-clock rejection guard ran before the prefer_engine_time branch, so an OpenBSD-style agent (engine time == Unix clock) still fell back to sysUpTime on the spine-facing paths even with prefer_engine_time=true. Spine's own re-check has no wall-clock awareness at all - it unconditionally prefers any numeric engine time - so that class of device kept the RECACHE ASSERT loop. Move the prefer_engine_time short-circuit before the wall-clock check so those paths replicate spine exactly; the wall-clock rejection still protects the display-only call sites. * test(snmp): fix stale wall-clock assumption in prefer_engine_time coverage test |
||
|
Thomas Vincent
|
3f377531b7 |
fix(poller): stop the registry pid probe narrowing a wide pid to -1 (#7956)
* fix(poller): stop the registry pid probe narrowing a wide pid to -1 processes.pid is int(10) unsigned, so a corrupted row can hold 4294967295. Through PHP 8.4 posix_kill() narrows that to a 32 bit pid_t of -1 and kill(2) reads it as every process the caller may signal, so cacti_process_pid_exists() calls a dead registry row live and the two kill sites that ask it first SIGTERM the caller's whole session. From PHP 8.5 the same value raises a ValueError and ends the poller instead. Refusing it ahead of the call covers both. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * hardening: bound every process table PID before signalling it Twelve call sites read a pid out of a process table and handed it straight to posix_kill(). processes.pid is int(10) unsigned, so a corrupted row can carry 4294967295, which narrows to a 32 bit pid_t of -1 and signals every process the caller owns, or from PHP 8.5 raises a ValueError that ends the collector. They now go through cacti_process_kill(), which refuses init, anything that cannot name a process, and anything wider than pid_t. poller_recovery read its pid with a bare posix_kill($pid, 0) and treats false as "stale, clear the row and run". cacti_process_signalable() keeps exactly that meaning, including EPERM as stale, and adds the bound. Neither cacti_process_pid_exists() nor cacti_process_still_running() fits there: both report a pid live when signalling it is merely forbidden, so a recycled recovery_pid would have wedged recovery until an operator cleared the row. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * test: place process guards in poller domain Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix(poller): honor platform PID ranges Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * test(poller): cover platform signal semantics Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> --------- Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> |
||
|
Thomas Vincent
|
130f58243b | Fix SNMP uptime selection across polling paths (#7984) | ||
|
Thomas Vincent
|
a155419ff3 |
Serialize process registry admission with portable locks (#7771)
* Serialize process registry admission with Symfony Lock Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * Fix PDO process lock mode handling Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * ci: run process lock coverage on PHP 8.2 Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix: confirm a pid is still ours before signalling it #7574 added the still-running guard to dsstats but left six byte-identical siblings signalling a pid straight out of the processes table. After a crash or a power loss the row survives, the pid is recycled by an unrelated daemon, and the next start SIGTERMs a stranger. That is issue #7425, still live in rrdcheck, boost, poller_commands, float_rrdfiles, poller_reindex_hosts and rebuild_poller_cache. Wrap each kill in cacti_process_still_running(), matching the dsstats shape, and leave unregister_process() outside the guard so a stale row is still cleaned up. Cover all seven with a test that walks every kill site, so a new variant cannot reintroduce it. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * test: assert the guard plainly toContain() takes needles rather than a failure message, so the file name was being asserted as a second required substring and the test always failed. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * docs(poller): describe required lock parameters --------- Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> |
||
|
Thomas VincentandTheWitness
|
f2898730e1 |
fix: guard process registration and cleanup against PID reuse (#7425) (#7574)
* fix: guard process registration and cleanup against PID reuse A bare posix_kill($pid, 0) only proves some process owns the pid, so a recycled one blocked new tasks or drew a SIGTERM meant for something else. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * test: spawn probe children without a wrapping shell proc_open with a string command runs it via sh, so the child's /proc comm was sh and the identity check correctly refused to signal it on Linux. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * docs: append the changelog entry at the end of the issue block Matches .github/copilot-instructions.md, which appends rather than inserting in numeric order. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix: limit the process identity check to CLI callers The registry holds CLI tasks, so a web request comparing command names would never match and would report a running task dead. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * test: use the full licence header on the new test files Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * test: treat CACTI_PATH_LOG as a directory and pass ctor args to the statement class Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix: make process identity checks self-contained * fix(batchgapfix): guard forced process termination * fix(processes): preserve live pids on permission errors * Refresh security baselines after develop rebase Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * Wait for the spawned child to exec before the identity check Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * Serve fetch() from the buffered rows in the process registry fake db_table_exists() probes with SHOW TABLES, and db_fetch_row_prepared() reads a single row with fetch(); the fake drained the cursor on execute() and answered neither, so every registry query came back empty and the tests exercised the no-processes-table path instead of the guard. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix(changelog): drop entries duplicated by an earlier rebase Only the issue#7425 line belongs to this branch. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix(poller): identify a registered process by its executable, not its name Children are spawned from path_php_binary, so a versioned child and a parent started as plain php reported different command names for one interpreter. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * test(poller): skip the alias check where both paths share a command name Whether a second path to the interpreter yields a second name is the runner's business; asserting it failed where php is already the plain name. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> --------- Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> Co-authored-by: TheWitness <thewitness@cacti.net> |
||
|
Thomas VincentandTheWitness
|
edafd489a5 |
fix(poller): launch cactid without a shell (#7603)
* Launch cactid poller without a shell * Handle an unset poller PHP binary * fix(poller): reject null bytes in executable paths --------- Co-authored-by: TheWitness <thewitness@cacti.net> |
||
|
Thomas VincentandTheWitness
|
f2a5ef0d51 |
fix: cast path_php_binary before passing it to string parameters (#7576) (#7577)
* fix: cast path_php_binary before passing it to string parameters The setting has no default, so it is null on a fresh install, and PHP 9 turns the resulting coercion from a deprecation into a TypeError. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * docs: append the changelog entry at the end of the issue block Matches .github/copilot-instructions.md, which appends rather than inserting in numeric order. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * test: use the full licence header on the new test files Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix: cast path_php_binary at every site that escapes it The first pass covered three of them; a repo-wide scan found 21, and the test now fails if a new one appears. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * test: strengthen PHP binary path guard * fix: cover default-config PHP binary reads --------- Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> Co-authored-by: TheWitness <thewitness@cacti.net> |
||
|
Thomas VincentandTheWitness
|
db39178735 |
fix: harden the poller_item cache and Boost RRD staging subsystems (#7538)
* fix(boost): forward still-open-round archive rows instead of dropping them boost_process_poller_output() correctly excluded rows from an in-progress poll round when building the RRD write, but the archive-table cleanup right after deleted every row for the local_data_id anyway, with no time filter. Those rows now get INSERT IGNORE'd into the live poller_output_boost table before the archive table is cleared, so a still-open round's data survives. Also fixes the duplicate-record guard in the same function: $last_item was initialized once before the results loop and never reassigned, so the comparison against it could never be true. It is now updated after each processed row. Fixes #7519 Fixes #7523 Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix(boost): gate archive-table drop on shard completeness poller_boost.php decided whether to drop a run's archive tables using SELECT SUM(status) FROM poller_output_boost_processes, an aggregate over whatever completion rows landed. If one child crashed before writing its completion row, the sum from the surviving shards was still positive and every archive table for the run got dropped, including the crashed shard's unprocessed data. The drain loop already computes boost_completed_children() vs $expected_children to detect a crashed child; the drop now uses that same completeness check instead, and leaves the archive tables in place with a warning when it is not met. Fixes #7520 Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix(dsstats,rrdcheck): apply Boost's startup barrier to launch loops dsstats_launch_children()/rrdcheck_launch_children() fork children via exec_background() (non-blocking) and the callers then wait with a bare while ($running = ..._processes_running($type)) { sleep(2); }. If the parent evaluates that before any child has registered in the processes table, $running reads 0 and the loop body never runs, treating "not started yet" as "already done" -- the same race Boost's boost_all_children_registered() barrier was added to close. dsstats_launch_children()/rrdcheck_launch_children() now return the launched count, and dsstats_wait_for_children()/ rrdcheck_wait_for_children() wait for the running count to reach it (bounded by a deadline) before trusting the drain loop's running-count poll. Fixes #7526 Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * refactor(poller): consolidate poller_item deletes behind a shared helper Extract poller_item_delete_for_data_source() and poller_item_delete_for_host() in lib/poller.php and migrate the existing local+remote delete pairs in lib/api_device.php and lib/api_data_source.php onto them, so the local-delete-plus-remote-mirror pattern lives in one place instead of eight. Refs #7535 Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix(poller): mirror poller_item deletes to the remote poller on handoff api_data_source_change_host(), data_query_remove_disabled_items(), and data_source_to_data_template() each moved or removed a data source's poller_item row locally but never on the remote Data Collector that still owned it, so that Collector kept polling an item it no longer should. All three now resolve the owning remote poller before the local change and purge the row there too via the shared helper. Fixes #7521 Fixes #7524 Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix(poller): correct poller_item join semantics and cron reset in cmd.php poller_item and host share ~13 identically-named snmp_*/hostname columns; SELECT * across the join let PDO FETCH_ASSOC silently collapse them to host's live values instead of poller_item's cached copies. Select pi.* plus explicitly aliased host/site disabled columns instead. The now already-fetched $poller_items also makes the separate script-server-call COUNT(*) re-join redundant, replaced with an in-PHP count. Also apply the same cron_interval-aware rrd_next_step reset used by the ranged branch to the --allhost branch, which previously ignored it. Fixes #7522, #7525, #7528. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix(poller): thread known poller_id through update_poller_cache() callers update_poller_cache() re-derived poller_id via a host/data_local JOIN on every call, even when the caller already had it: repopulate_poller_cache() and push_out_data_input_method() both already select h.poller_id and discarded it, push_out_host() has the host row cached per data source, and api_device_enable_devices()/update_poller_cache_from_query() compute it once outside their loops. Add an optional $poller_id argument (and honor an already-present 'poller_id' array key) so these callers skip the redundant query. Fixes #7527. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix(poller): scope global aggregate scans and make process-leveling sargable Three poller.php queries (total_snmp_ports and the mutually-exclusive active_profiles pair) had no poller_id filter, unlike the rest of the file's central-only blocks, so every remote poller redundantly recomputed and overwrote the same global config options; the snmp_port scan is also an unindexed full table scan. Gate them behind poller_id == 1, with remote pollers reading back the value the central poller already stored. Also rewrite the process-leveling $sql_where's rrd_next_step predicate from the non-sargable `rrd_next_step - $poller_interval <= 0` to the algebraically equivalent `rrd_next_step <= $poller_interval`, restoring use of the poller_id_rrd_next_step index. Fixes #7529, #7530. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix(poller): batch update_data_query_cache()'s poller cache writes Each changed data source called update_poller_cache($data_source, true), which flushes immediately via poller_update_poller_cache_from_buffer() -- its own present=0 UPDATE, INSERT ON DUPLICATE KEY UPDATE, and stale-row DELETE -- on every iteration. Re-indexing a host where every port changed issued up to 3x as many statements as ports instead of one batched flush. Accumulate changed rows across the loop and call update_poller_cache() with commit=false, then flush once via poller_update_poller_cache_from_buffer() after the loop, matching the batched pattern already used by push_out_data_input_method(). Fixes #7531. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix(boost): add USING BTREE to poller_output_boost_local_data_ids indexes MEMORY-engine indexes default to HASH unless USING BTREE is specified, unlike the sibling poller_output/poller_output_boost tables in the same schema file. boost_output_rrd_data()'s ORDER BY/LIMIT query and boost_process_local_data_ids()'s ranged DELETE both need to scan local_data_id in order, which a HASH index can't serve, forcing a filesort on every boost worker pass. Verified against a real MariaDB 11 instance: EXPLAIN on the ORDER BY query shows "Using filesort" with the old HASH schema and no filesort with USING BTREE, using the primary key for ordering instead. Also aligns boost_prepare_process_table()'s dead CREATE TABLE IF NOT EXISTS fallback with the corrected schema, and adds an upgrade step that rebuilds both indexes as BTREE on existing installs (checked via information_schema.STATISTICS, since db_install_add_key() only compares index columns, not algorithm, and would otherwise skip an existing HASH index with the right column list). Closes #7532 Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix(boost): skip the UNION ALL wrapper for the single-archive-table case boost_process_local_data_ids() always wrapped its flush query in "SELECT * FROM (... UNION ALL ...) t ORDER BY ...", even when there was only one archive table -- the steady-state case, since old archive tables are dropped at the end of each cycle. UNION ALL results wrapped in a derived table are one of the documented cases MySQL/MariaDB's derived-table merging doesn't apply to, so every pass paid for a filesort over an indexless derived table regardless of how well-ordered the underlying archive table's rows already were. Extracts the per-table SELECT into boost_archive_select_sql() and uses it directly, without the wrapper, when there's exactly one archive table. The UNION ALL/derived-table shape is kept as the fallback for the genuinely rare case where a prior crashed run left more than one archive table behind. Verified against a real MariaDB 11 instance and via a SQLite-backed integration test that both shapes return correctly filtered and sorted rows. Closes #7533 Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix(boost): batch-prefetch RRD field name lookups instead of per-row queries Two N+1 patterns in the Boost hot paths: boost_process_local_data_ids() (poller_boost.php) re-ran the unused_data_source_names/nt_rrd_field_names lookups on every local_data_id boundary in the sorted flush result set. process_poller_output() (lib/poller.php) had the equivalent lookup, but without even a boundary check -- it re-ran per matching row whenever the output contained a ':', including consecutive rows for the same data source. Adds poller_prefetch_rrd_field_names() (lib/poller.php, shared by both callers since poller_boost.php already requires lib/poller.php) to batch-fetch these mappings once per invocation via IN()-batched queries keyed by the full set of local_data_ids in the batch, chunked at 1000 ids to match the array_chunk(..., 1000) convention used elsewhere (lib/api_data_source.php, lib/api_graph.php). Both call sites now look up the prefetched maps in-memory inside their loops. While consolidating the lookups, fixes a latent bug the old process_poller_output() code had in its non-templated fallback branch: a query referencing gti.task_item_id without joining graph_templates_item, which throws an unknown-column error whenever that path is hit. The batched query always joins correctly. Integration test proves both correctness (against a real SQLite-backed schema) and the query-count reduction: 5 local_data_ids that would previously cost up to 10 queries for these two lookups now cost exactly 3, regardless of how many ids are in the batch. Closes #7534 Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * refactor(boost): consolidate poller_output_boost inserts into one helper The INSERT INTO poller_output_boost (...) VALUES ... statement was hand-written four times: three in cmd.php (behind the same boost_redirect == 'on' && boost_rrd_update_enable == 'on' guard) using INSERT IGNORE, and once in boost_poller_on_demand() (lib/boost.php) using ON DUPLICATE KEY UPDATE. Beyond the duplication, that was a real data-integrity inconsistency between two paths writing the same shape of data: silently-drop vs last-write-wins on the same key. Since boost_redirect gates cmd.php's writes and boost_poller_on_demand() returns early when boost_redirect is on, the two paths never run concurrently for the same install; they're alternate write paths selected by that one setting, not competing writers. A genuine collision on (local_data_id, rrd_name, time) in either path represents a retry of an already-collected sample -- a batch resent after a partial failure, or two writers racing on the same rounded second -- not two independently meaningful readings, since RRD only stores one value per timestamp regardless. Standardizes on INSERT IGNORE (first write wins), matching the sibling poller_output insert this table shadows, which has always used INSERT IGNORE, and 3 of the 4 existing call sites. Extracts boost_flush_output_batch() (lib/boost.php) as the one shared helper, handling both the batched INSERT and the max_allowed_packet-safe chunking cmd.php's three sites previously didn't do at all (they only chunked by item count). Integration test exercises a genuine duplicate-key collision against a real (SQLite-backed) table and asserts the chosen first-write-wins behavior. Closes #7536 Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix(tests): resolve cross-branch test interactions found by full-suite verification Two test-only bugs surfaced only when all four fix branches for the poller_item/Boost hardening pass ran together, not when each ran in isolation: - BoostArchiveHandoffIntegrationTest extracted the archive-forward INSERT IGNORE by searching lib/boost.php for the first occurrence of "INSERT IGNORE INTO poller_output_boost". The Boost SQL-optimization branch adds a second, earlier occurrence (the new boost_flush_output_batch() helper's single-line prefix), so the naive search now grabbed the wrong snippet. Anchor on the multi-line "...poller_output_boost\n\tSELECT *" shape that only the archive-forward statement has. - PollerItemRemotePurgeHandOffTest calls push_out_host() (via api_data_source_change_host()), which needs CACTI_PATH_LIBRARY. The test never requires include/global.php (which normally defines it), and was relying on some other file in a full multi-thousand-file suite run having defined it first as a process-wide side effect. Define it directly instead of depending on unguaranteed load order. No production code changed. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * style(poller): satisfy php-cs-fixer strict_param in the script-server count Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix(boost): order the single-archive-table flush by the table's PK columns Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix(boost): key the max_allowed_packet cache per connection Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix(tests): restore the default db_* session after swapping it for a fake handle Left swapped, the fake handle answered every later read_config_option() in the run; on PHP 8.4 the extra deprecation traffic routed through CactiErrorHandler turned that into an uncaught PDOException and killed the pest process. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> --------- Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> Co-authored-by: TheWitness <thewitness@cacti.net> |
||
|
Thomas VincentandTheWitness
|
1f4461783d |
Parameterise graph and poller delete queries (#7264)
* security(graph): parameterise api_graph_remove_multi delete queries The removal loop built an id CSV by raw concatenation (no intval) and repeated the five deletes in a manual flush block. Replaced with array_chunk over intval-ed ids and array_to_sql_or, which removes the duplicated block and the unsanitised interpolation. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * security(poller): parameterise poller_output cleanup deletes The three poller_output cleanup deletes interpolated a data-id list (a DB GROUP_CONCAT and an accumulated array). Route them through array_to_sql_or on intval-ed ids. The CREATE TABLE replication and poller-id lookups in this file are left as a separate concern. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * Use safe numeric IN clauses for batch deletes * fix: harden batched graph deletion inputs --------- Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> Co-authored-by: TheWitness <thewitness@cacti.net> |
||
|
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> |
||
|
Thomas Vincent
|
b4cb892816 |
fix: remove unconditional overwrite of coerced multi-DS values (#6880)
* fix: remove unconditional overwrite of coerced multi-DS values Remove the unconditional assignment at line 643 that overwrote hex-converted and 'U' fallback values with the raw input string in the multi-DS output parsing path of process_poller_output(). The if/elseif/else block at lines 635-641 correctly coerces values (numeric passthrough, hexdec conversion, 'U' fallback), but line 643 immediately overwrote the result with $matches[1], negating the hex conversion and garbage filtering. Impact: hexadecimal 64-bit SNMP counter values (e.g., ifHCInOctets) in multi-DS output were stored as raw hex strings instead of decimal, causing RRDtool update failures. Includes 14 regression tests proving the fix: - numeric, U, hex, and garbage values coerce correctly - fixed vs buggy behavior produces different results - mutation killers for all branches - source verification confirming the overwrite line is removed Fixes #6879 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> --------- Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> |
||
|
Thomas VincentandTheWitness
|
5282549edf |
fix: three exec_with_timeout bugs causing unreliable script polling (#6748)
* cleanup: remove unreachable defensive error logs in aggregate graph item validation * fix: three bugs in exec_with_timeout causing unreliable script polling 1. Operator precedence: (int) cast applied before multiplication, so sub-second elapsed times truncated to 0 and timeout never decreased. Fix: parenthesize the multiply before the cast. 2. stderr discards stdout: any stderr output caused the function to return false, losing valid poll data. Fix: log stderr as a warning instead of aborting. 3. Orphaned children: proc_terminate only kills the direct process. Fix: use posix_kill with negative PID to kill the process group when the timeout expires. Fixes #6719, #6720, #6721. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * test: verify exec_with_timeout operator precedence and process cleanup Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix: use setsid for correct process group kill on timeout The child process inherits the parent process group, so posix_kill(-pid, 9) targets the wrong PGID. Prepending setsid makes the child its own process group leader (PID == PGID). Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix: address Copilot review feedback on exec_with_timeout - Add setsid availability check for portability - Only log stderr on non-zero exit code with cmd context - Reduce test sleep to 5s, fix test names and comments Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix: use CACTI_SERVER_OS instead of DIRECTORY_SEPARATOR Per TheWitness review feedback. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix: add CACTI_SERVER_OS stub and fix return value assertions exec_with_timeout returns end(explode("\n", ...)) which yields '' for output with trailing newlines. Check $output array instead. Add CACTI_SERVER_OS constant stub for non-Cacti test contexts. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * test: address Copilot review feedback on exec timeout tests - Reduce sleep duration in kill test to 3s to bound CI worst-case - Assert return value is null for invalid command (empty stdout path) - Restore api_aggregate.php accidentally modified by prior commit - Fix pre-existing lib/dsstats.php alignment and PHPStan bootstrap order Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix: use CACTI_SERVER_OS instead of PHP_OS in detect_cpu_cores TheWitness review: match the codebase convention of testing CACTI_SERVER_OS rather than inspecting PHP_OS directly. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> --------- Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> Co-authored-by: TheWitness <thewitness@cacti.net> |
||
|
|
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> |
||
|
Thomas VincentandTheWitness
|
5c690df5c4 |
fix: strict comparison in replicate_table_to_poller column exclusion (#6680)
* fix: column exclusion skips first element due to loose array_search comparison array_search() returns the key on match or false on no match. When the matching key is 0 (first element), the loose comparison == true evaluates 0 == true as false, so the first excluded column is never actually excluded from the ON DUPLICATE KEY UPDATE clause. Replace == true with !== false and != false with !== false. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix: skip excluded columns in VALUES tuples in replicate_table_to_poller Excluded columns were omitted from the INSERT column list but still included in the VALUES tuples, causing a column count mismatch error. Add them to $skipcols so the row-building loop skips them consistently. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix: add issue #6682 to CHANGELOG and place in numeric order Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> --------- Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> Co-authored-by: TheWitness <thewitness@cacti.net> |
||
|
Thomas Vincent
|
1a850dba44 |
Check table existence before replicating out (#6616)
When a plugin asks to replicate a table that doesn't exist locally, the SHOW COLUMNS query at the top of replicate_out_table() blows up with a warning. The empty-data path (line 1951) already handles missing remote tables, but the data-present path never checks the local side. Add a db_table_exists() guard before hitting SHOW COLUMNS and log a WARNING so the admin can see which table was skipped. Fixes #6600 |
||
|
TheWitness
|
27919c50d7 | Updating Copyrights to 2026 (#6578) | ||
|
Athos RibeiroandTheWitness
|
271c478285 |
Improve PHP 8.5 support (#6568)
* Improve PHP 8.5 support * The backtick operator is now deprecated in favor of shell_exec. * Terminating case statements with a semicolon instead of a colon has been deprecated. * xml_parser_free was a noop since PHP 8.0 and is now deprecated. * Fix case statement formatting in mib_parser.php * Fix indentation for version check in xml.php * Add whitespace handling for XML parsing --------- Co-authored-by: TheWitness <thewitness@cacti.net> |
||
|
TheWitnessandCopilot
|
b993471579 |
Fix #6539 - Allow Standard Cacti Log Files to be Rotated on Demand,PHPdoc, and Typing (#6540)
* Fix #6539 - Allow Standard Cacti Log Files to be Rotated on Demand from the Cacti UI * QA: php-cs-fixer updates and documentation for lib/database.php * QA: Update .gitignore for composer files * QA: Update .gitignore for composer files * QA: Back off .gitignore change its duplicated * QA: Fix typing of db_has_permissions * QA: Fixing typing issue in db_format_index_create * Update lib/clog_webapi.php Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * QA: Fix additional code issues * QA: Applying more defaults to add device * QA: Fix at the database layer * QA: Fixes based upon automated testing * QA: Fix at the database layer * QA: After new SQL checking * QA: Skip MacTrack oui database download warnings * QA: Fixing type issue * QA: Recommendation from Copilot * QA: Recommendation from Copilot * QA: Recommendation from Copilot * QA: Recommendation from Copilot * QA: Some minor changes to avoid errors in testing * QA: Unused variable * QA: Recommendation from Copilot * QA: Recommendation from Copilot * QA: Recommendation from Copilot * QA: Minor introduced code defect * QA: Adding some unifying space around concatination * QA: Consistent comments and new function alias' * QA: Multi-line comment corrections * QA: Fix SQL Style * QA: Missed one function alias * QA: PHPStan Level 2 and PHPDoc fixes * QA: PHPStan Level 2 and PHPDoc Fixes * QA: PHPStan Level 6 Fixes * QA: Fix lib/xml.php to PHPStan Level 6 * QA: Wrong grep -v * PHPStan Level 6 for lib/vdef.php * PHPStan Level 8 for lib/time.php * QA: Raise PHPStan to Level 6 for lib/database.php * QA: Remove @deprecated tag from grv() * QA: Switch to htmle() alias over html_escape() * QA: More addressing PHPStan issues at Level 6 * QA: Change recommended by Copilot for Readability * QA: More PHPStan fixes * QA: More PHPStan fixes * QA: More PHPStan fixes for lib/utility.php * QA: More PHPStan fixes at Level 6 * QA: Raise PHPStan to Level 6 More Functions * QA: Raise PHPStan to Level 6 More Functions * QA: Raise PHPStan to Level 6 More Functions * QA: Raise PHPStan to Level 6 More Functions * QA: Raise PHPStan to Level 6 More Functions * QA: More PHPStan fixes at Level 6 * QA: Raise PHPStan to Level 6 More Functions * QA: Raise PHPStan to Level 6 More Functions * QA: More PHPStan fixes at Level 6 * QA: More PHPStan fixes at Level 6 * QA: Raise PHPStan to Level 6 More Functions * QA: Raise PHPStan to Level 6 More Functions * QA: More PHPStan fixes at Level 6 * QA: Raise PHPStan to Level 6 More Functions * QA: Raise PHPStan to Level 6 More Functions * QA: More PHPStan fixes at Level 6 * QA: Raise PHPStan to Level 6 More Functions * QA: Raise PHPStan to Level 6 More Functions * QA: More PHPStan fixes at Level 6 * QA: More PHPStan fixes at Level 6 * QA: More PHPStan fixes at Level 6 * QA: Raise PHPStan to Level 6 More Functions * QA: More PHPStan fixes at Level 6 * QA: Some phpseclib3 fixups to update to latest * QA: More fixups for graphing for magic strings * QA: More PHPStan fixes at Level 6 * QA: Fixing order string type on get_allowed_graphs() * QA: Fixing improper use of print * QA: Optimization of color_templates * QA: Copilot recommendation * QA: Copilot recommendation * QA: Copilot recommendation * QA: Copilot recommendation * QA: Copilot recommendation * QA: Copilot recommendation * QA: Copilot recommendation * QA: Copilot recommendation * QA: Raise PHPStan to Level 6 More Functions * QA: Issues insalling Cacti * QA: Issues insalling Cacti * QA: Issues insalling Cacti * QA: Issues insalling Cacti * QA: Issues insalling Cacti * QA: Issues testing Cacti * QA: Copilot recommendation * QA: More PHPStan fixes at Level 6 * QA: More PHPStan fixes at Level 6 * QA: More PHPStan fixes at Level 6 * QA: More PHPStan fixes at Level 6 * QA: More PHPStan fixes at Level 6 * QA: Installer failing * Undo ownership changes to remove tar errors * QA: Updates after php-cs-fixer * QA: Adding code checks to syntax checking * QA Change syntax flow language * QA: Reduce the number of cures * QA: Use sequentional option * Change runner to self-hosted for check job * Fix typo in OS version in workflow file * Add Git installation step in workflow Added step to install Git if not already installed. * Add '-y' flag to apt-get install and update commands Updated apt-get commands to include '-y' for automatic confirmation. * Change workflow OS version to 'ubuntu-latest' Updated the workflow to use 'ubuntu-latest' instead of 'ubuntu:latest' and modified the 'runs-on' configuration. * Remove sequential option from phpcsfixer script * Remove PHPCS fixer step from workflow Removed PHP coding standards check from workflow. * Move code checks to before plugins Moving these core checks to before the plugins are installed as we really want to check only the base Cacti here. * Add Composer validation and caching to start of workflow * QA: Addressing PHPStan additional * QA: Restore composer lock file * QA: Addressing PHPStan additional * QA: Addressing PHPStan additional * QA: Complete LDAP Project * QA: php-cs-fixer doing it's thing * QA: Syntax is already happening in PHPStan * QA: The md5 function no longer exists * QA: Update after ldap changes * QA: Undo that last password hack * QA: Attempting to switch to MariaDB 11.8 * QA: Attempting to switch to MariaDB 11.8 * QA: Resolving some issues with PHP8.4 * QA: Resolving some issues with PHP8.4 * QA: Add PHP8.5 to the matrix * QA: Removing PHP8.5 due to dependency issues * QA: Add the DB version to the testing output * QA: Switch back to MySQL for the moment * QA: More fidelity in reporting * QA: The pipe must be mixed * QA: Moving audit_schema.php changes from 1.2.x * QA: Moving audit_schema.php changes from 1.2.x * QA: Restore mysql monitoring * QA: php-cs-fixer issues * QA: Attempt to roll back to MariaDB 10.6 * QA: Attempt to fix check_all_pages.sh * QA: Make installer functional again * QA: Fix installer php-cs-fixer issues * QA: Type to a string * QA: Get the database version correctly * QA: Fix issue with reports in check_all_pages.sh * QA: Check the server version in the right place * Feature: Support SHA256 as a new hash for passwords * QA: Function definition and return typing audit * QA: Refinement of form types * QA: Refinement of form types * QA: Consistency of comments * QA: Issues with php-cs-fixer and more comments * QA: More typing issues * QA: php-cs-fixer on merge conflict resolution --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> |
||
|
TheWitness
|
b9334a15d9 | QA: Updates for files for php-cs-fixer and some php-doc changes | ||
|
MSS
|
973f6c95da |
A DB Exec Failed!, Error: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'function, status) VALUES ('1', 'internal', 'config_arrays', '', 'plugin_config_a' at line 1 (#6211)
* A DB Exec Failed!, Error: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'function, status) VALUES ('1', 'internal', 'config_arrays', '', 'plugin_config_a' at line 1
A DB Exec Failed!, Error: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'function, status) VALUES ('1', 'internal', 'config_arrays', '', 'plugin_config_a' at line 1
* issue#6210: Fix Error: You have an error in your SQL syntax due to using table field without backtick.
A DB Exec Failed!, Error: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'function, status) VALUES ('1', 'internal', 'config_arrays', '', 'plugin_config_a' at line 1 #6210
|
||
|
TheWitness
|
44bdd44466 | Minor Lint Checking Fixes | ||
|
TheWitness
|
e5c4015e0e |
QA: Remove most references to $config
This removes most entries to $config in the base code. A few new constants were defined to achieve this: * CACTI_CONNECTION - The status of the current connection * CACTI_CSRF_SECRET - The path to the csrf secret file * CACTI_PHP_SNMP - True if php-snmp is installed * CACTI_WHITELIST - The path to the Cacti Whitelist file if defined I also moved several $config['poller_id'] to the existing POLLER_ID and move a number of $config['is_web'] to the existing CACTI_WEB constants. There remain a few artifacts in the code. They will be handled in a separate commit. |
||
|
TheWitness
|
2c779273f9 | Copyright Updates and PHP-CS-Fixer updates | ||
|
Jimmy ConnerandTheWitness
|
65ab16f5cf |
Start converting to newer PHP Syntax and Functions (#6103)
* Add Rector * Add Rector * Fix "Cashed" misspelling Also harmonize the phrase with the one below it * Fix implode error in db_table_create Only relevant when multiple primary keys * Fix auth issue, parentheses in the wrong location Was causing too many arguments to be sent to cacti_sizeof which only take 1, instead of get_allowed_devices. * Fix issue with session_var not properly using class variable * Fix pass by reference error in get_allowed_devices get_allowed_devices has a pass by reference on the "total_rows" field. It is used in several places to pass the total_rows back to a counter. Here though, we are passing a number directly to the function, which errors out with "Uncaught Error: get_allowed_devices(): Argument #4 ($total_rows) cannot be passed by reference". So we will just place a dummy variable in front to catch the pass. This wasn't caught before because the previous parentheses issue. * Fix undefined variable "field_array" in html_filter * Fix undefined function 'get_nfilter_filter_request_var' * Fix undefined variable 'resizable' I corrected the function parameter, as that was technically the incorrect spelling * Fix invalid variable $user_id * Fix undefined vars in spikekill class * Fix copy and paste error Oops * Forgot to use the dummy variables * Add missing $ Oops * Start converting to newer PHP Syntax and Functions * Test fixing the workflow by install composer * Lets not valid the composer file right now Seems we have a few things to fix it in * Validate composer * Lite formating changes * lite formatting * lite formatting * lite format changes * lite format changes * lite format changes * lite format changes * lite format changes * lite format changes * lite format changes * lite format changes --------- Co-authored-by: TheWitness <thewitness@cacti.net> |
||
|
TheWitness
|
cefbd2cfc8 |
Fix: #6001 - Extend SYSTEM STATS
* Provides more visibility into Device collection errors. |
||
|
Jimmy Conner
|
ad07471bb1 |
Lots of PHPDoc formatting and cleanup
Lots more to go |
||
|
TheWitness
|
22b85e1622 |
Fix: #5966 - Add snmp retries at the device level
* This covers all the built in scripts. Several packages will need rebuild |
||
|
TheWitness
|
fdd9cd766b |
Fix: #5963 and #5961 - Replication Issues and Database Caching
* Cacti caching for db_table_exists() and db_column_exists() is not connection aware and replication does not replicate empty table structures * Issues with Replication of Tables and Misleading Full Sync Time |
||
|
Linegod
|
6cca407d10 | typos | ||
|
TheWitness
|
3356a88691 |
Fix #2738 - Set cache rebuild process indicator for the Cacti UI
* This will run the rebuild process in background and provide a visual indicator on the rebuild page. |
||
|
TheWitness
|
493400c534 | Fixing #5840 - Verify All Fields Not working with OID/INDEX parse method | ||
|
TheWitness
|
1aa72d24c9 | QA: Update Copyright to 2024 | ||
|
TheWitness
|
3fdeda5372 | QA: Fixing #5582 - No default timeout for exec_poll() | ||
|
TheWitness
|
3c7b49a359 |
QA: Remove deprecated (very old) function
This function has not been required for going on two decades. Remove it. |
||
|
TheWitness
|
8b61937b5e |
QA: Resolving #5530 - Empty exec_background()
1.3.0 dev - Error php depreciated parameter to file_exists on initial run |
||
|
jkbzhandJose Kahan
|
d5615333e4 |
Support for PDO::MYSQL_ATTR_SSL_CAPATH and PDO::MYSQL_ATTR_SSL_VERIFY_SERVER_CERT (#5522)
* Make PDO::MYSQL_ATTR_SSL_CAPATH and PDO::MYSQL_ATTR_SSL_VERIFY_SERVER_CERT accessible thru $database_ssl_capath and $database_ssl_verify_server_cert configuration options. * typo: wrong variable name * fixing typo --------- Co-authored-by: Jose Kahan <jose.kahan@w3.org> |
||
|
TheWitness
|
ab9dd5435f |
Fixing #5384 - Untemplated Data Source Issue
* Untemplated Data Sources can fail to update due to lack of an assigned Graph * Related to #5336. We missed the case of the untemplated data source when we fixed this, and another issue in release 1.2.24. This will allow objects such as RTM's ELIM Templates to properly create RRDfiles and operate as it does today. |
||
|
TheWitness
|
8ff31b63cd |
Porting changes from #5336 - Poller Issues
Issues with certain cases of Data Input to Data Template to Graph Template Item mappings. |
||
|
TheWitness
|
3174bf4cb7 | QA: #5624 - Ensure the value cache table is replicated | ||
|
TheWitness
|
402242861f |
Porting #5259 - Issues with Data Template and Graphs
When the Data Template contains more Data Sources than the Graph Template RRDfiles won't update |