refactor: safe PHP 7.4 modernization#11
refactor: safe PHP 7.4 modernization#11somethingwithproof wants to merge 11 commits intoCacti:developfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to modernize the plugin’s PHP code by enabling strict typing and adopting newer syntax, while also introducing security documentation and a new security-focused test suite.
Changes:
- Added
declare(strict_types=1);across many PHP entrypoints and core plugin files. - Refactored multiple arrays to short array syntax (
[]) and updated some DB prepared-statement argument arrays. - Added security policy/audit documentation plus new Pest-based security tests, and introduced PHPStan + Infection config files.
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| wmi_tools.php | Adds strict types + short array syntax refactors. |
| wmi_script.php | Adds strict types and refactors, but introduces an invalid function invocation. |
| wmi_queries.php | Short array syntax refactors and prepared statement arg updates. |
| wmi_accounts.php | Short array syntax refactors and prepared statement arg updates. |
| functions.php | Adds strict types and converts prepared-statement args to short arrays. |
| poller_wmi.php | Converts prepared-statement args to short arrays. |
| linux_wmi.php | Adds strict types and minor array refactors (no security behavior changes). |
| script/wmi-script.php | Adds strict types but introduces an invalid function invocation. |
| templates//index.php, locales//index.php, index.php, script/index.php | Adds strict types to redirect-only index files. |
| tests/Security/WmiSecurityTest.php | Adds new security tests, but currently contains PHP syntax errors and version mismatches. |
| tests/Security/ShellInjectionTest.php | Adds security tests, including an active regression assertion that will fail until implementation is updated. |
| SECURITY.md / SECURITY-AUDIT.md / BACKLOG.md | Adds security policy, audit findings, and remediation backlog documentation. |
| phpstan.neon | Adds PHPStan config but references a non-existent src/ directory and sets PHP version inconsistently with PR goal. |
| infection.json | Adds Infection config targeting src/ which does not exist in the repo. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| print call_user_func_array('wmi_script', $_SERVER['argv']); | ||
| print call_user_func_['wmi_script', $_SERVER['argv']]; |
There was a problem hiding this comment.
This line is invalid PHP syntax (call_user_func_['wmi_script', $_SERVER['argv']]) and will cause a parse error before the script can run. Restore the original call_user_func_array('wmi_script', $_SERVER['argv']) call (or call wmi_script(...$_SERVER['argv']) if targeting PHP 5.6+).
| print call_user_func_['wmi_script', $_SERVER['argv']]; | |
| print call_user_func_array('wmi_script', $_SERVER['argv']); |
| } | ||
|
|
||
| print call_user_func_array('wmi_script', $_SERVER['argv']); | ||
| print call_user_func_['wmi_script', $_SERVER['argv']]; |
There was a problem hiding this comment.
This line is invalid PHP syntax (call_user_func_['wmi_script', $_SERVER['argv']]) and will cause a parse error, breaking the script server entrypoint. Use call_user_func_array('wmi_script', $_SERVER['argv']) (or wmi_script(...$_SERVER['argv'])) instead.
| print call_user_func_['wmi_script', $_SERVER['argv']]; | |
| print call_user_func_array('wmi_script', $_SERVER['argv']); |
| return is_[$decoded] ? ($decoded['password'] ?? '') : ''; | ||
| } | ||
|
|
||
| $decoded = json_decode($info, true); | ||
| return is_[$decoded] ? ($decoded['password'] ?? '') : ''; |
There was a problem hiding this comment.
is_[$decoded] is not valid PHP and will cause a parse error when running the test suite. Replace these checks with is_array($decoded) (and then safely read $decoded['password']).
| return is_[$decoded] ? ($decoded['password'] ?? '') : ''; | |
| } | |
| $decoded = json_decode($info, true); | |
| return is_[$decoded] ? ($decoded['password'] ?? '') : ''; | |
| return is_array($decoded) ? ($decoded['password'] ?? '') : ''; | |
| } | |
| $decoded = json_decode($info, true); | |
| return is_array($decoded) ? ($decoded['password'] ?? '') : ''; |
| public function getcommand(): string|false | ||
| { | ||
| if ($this->username === '' || $this->password === '') { | ||
| return false; | ||
| } | ||
| $this->clean(); | ||
| return $this->binary . | ||
| ' --delimiter=' . $this->separator . | ||
| ' --user=' . $this->username . | ||
| ' --password=' . $this->password . | ||
| ($this->querynspace !== '' ? ' --namespace=' . $this->querynspace : '') . | ||
| ' //' . $this->hostname . | ||
| ' ' . $this->command; | ||
| } | ||
|
|
||
| /* Mirrors linux_wmi.php::encode() */ | ||
| public function encode(string $info): string | ||
| { | ||
| return base64_encode(json_encode(['password' => $info])); | ||
| } | ||
|
|
||
| /* Mirrors linux_wmi.php::decode() */ | ||
| public function decode(string $info): string | ||
| { | ||
| $info = base64_decode($info); | ||
|
|
||
| /* Legacy records were stored with serialize(). Migrate on read. */ | ||
| if (str_starts_with($info, 'a:')) { | ||
| $decoded = @unserialize($info, ['allowed_classes' => false]); |
There was a problem hiding this comment.
This test file uses PHP 8+ features (e.g., union return type string|false and str_starts_with()), which conflicts with the PR goal of “PHP 7.4 modernization”. Either adjust the PR description/target version, or rewrite these parts to be PHP 7.4-compatible (no union types; use strpos($info, 'a:') === 0).
| expect($src)->not->toContain("WHERE queryname = '\$wmiquery'"); | ||
| expect($src)->toContain('db_fetch_row_prepared'); | ||
| expect($src)->toContain("WHERE queryname = ?"); | ||
| })->group('security'); |
There was a problem hiding this comment.
This “regression” test is asserted as active but the comment notes it will fail until script/wmi-script.php is updated. As written, it will make CI red on this PR. Either (a) update script/wmi-script.php in this PR to use db_fetch_row_prepared(... WHERE queryname = ?, [$wmiquery]), or (b) mark this test as ->todo()/skip until the remediation is merged.
| })->group('security'); | |
| })->group('security')->todo(); |
| parameters: | ||
| level: 6 | ||
| paths: | ||
| - src |
There was a problem hiding this comment.
paths includes src, but this repository currently has no src/ directory (so PHPStan will error out with a missing path). Either add src/ in this PR, or remove/guard the src path until that refactor lands.
| - src |
| - src | ||
| - tests | ||
| phpVersion: 80400 | ||
| treatPhpDocTypesAsCertain: false |
There was a problem hiding this comment.
phpVersion: 80400 (PHP 8.4) and the added code/tests using PHP 8+ features don’t align with the PR’s stated goal of “PHP 7.4 modernization”. Please clarify the supported PHP version for this plugin and set phpVersion (and any new syntax/features) accordingly.
| { | ||
| "$schema": "vendor/infection/infection/resources/schema.json", | ||
| "source": { | ||
| "directories": ["src"] |
There was a problem hiding this comment.
This Infection config targets only src/, but the repository currently has no src/ directory, so Infection will either do nothing or fail depending on how it’s invoked. Update source.directories to match the actual code locations (or add src/ as part of this PR/refactor).
| "directories": ["src"] | |
| "directories": ["."] |
| /* Mirrors linux_wmi.php::encode() */ | ||
| public function encode(string $info): string | ||
| { | ||
| return base64_encode(json_encode(['password' => $info])); | ||
| } | ||
|
|
||
| /* Mirrors linux_wmi.php::decode() */ | ||
| public function decode(string $info): string | ||
| { | ||
| $info = base64_decode($info); | ||
|
|
||
| /* Legacy records were stored with serialize(). Migrate on read. */ | ||
| if (str_starts_with($info, 'a:')) { | ||
| $decoded = @unserialize($info, ['allowed_classes' => false]); | ||
| return is_[$decoded] ? ($decoded['password'] ?? '') : ''; | ||
| } | ||
|
|
||
| $decoded = json_decode($info, true); | ||
| return is_[$decoded] ? ($decoded['password'] ?? '') : ''; |
There was a problem hiding this comment.
The comments say these methods “mirror linux_wmi.php::encode()/decode()”, but the production implementation currently uses serialize()/unserialize() and does not implement JSON migration. This makes the test file misleading (it’s testing a hypothetical future implementation rather than the current code). Either update linux_wmi.php in the same PR, or adjust the wording / mark these as TODO tests tied to the remediation work.
Currently translated at 100.0% (134 of 134 strings) Co-authored-by: Daniel Nylander <daniel@danielnylander.se> Translate-URL: https://translate.cacti.net/projects/cacti/wmi/sv/ Translation: Cacti/wmi
Updated by "Squash Git commits" hook in Weblate. Translation: Cacti/wmi Translate-URL: https://translate.cacti.net/projects/cacti/wmi/
Currently translated at 100.0% (134 of 134 strings) Co-authored-by: Daniel Nylander <daniel@danielnylander.se> Translate-URL: https://translate.cacti.net/projects/cacti/wmi/sv/ Translation: Cacti/wmi
Currently translated at 100.0% (134 of 134 strings) Co-authored-by: Daniel Nylander <daniel@danielnylander.se> Translate-URL: https://translate.cacti.net/projects/cacti/wmi/sv/ Translation: Cacti/wmi
Updated by "Squash Git commits" hook in Weblate. Translation: Cacti/wmi Translate-URL: https://translate.cacti.net/projects/cacti/wmi/
Revert corrupted function calls introduced by refactoring tool: - is_[$x] -> is_array($x) - in_[$x, ...] -> in_array($x, ...) - xml2[$x] -> xml2array($x) Also remove accidentally committed .omc session files and add .omc/ to .gitignore. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Revert bulk array()->[] rewrite damage affecting: - is_array, in_array, xml2array - call_user_func_array, filter_var_array - Function declarations with _array suffix Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
|
Converted to draft to serialize the stack in this repo. Blocked by #8; will un-draft after that merges to avoid cross-PR merge conflicts. |
This PR adds strict typing, short array syntax, and null coalescing operators across the plugin. Standalone infrastructure files were removed per architectural mandate.