refactor: PHP 8 modernization and WMI transport seam - #16
Draft
somethingwithproof wants to merge 10 commits into
Draft
refactor: PHP 8 modernization and WMI transport seam#16somethingwithproof wants to merge 10 commits into
somethingwithproof wants to merge 10 commits into
Conversation
Automated fixes: - XSS: escape request variables in HTML value attributes - SQLi: convert string-concat queries to prepared statements - Deserialization: add allowed_classes=>false - Temp files: replace rand() with tempnam() Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
- Change Dependabot ecosystem from npm to composer (PHP-only repo) - Remove PHP from CodeQL paths-ignore so security PRs get analysis - Remove committed .omc session artifacts, add .omc/ to .gitignore Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Linux_WMI::clean() escaped the username, password, binary and command but left
the device hostname only trimmed and the query namespace untouched, so
getcommand() interpolated them raw into the wmic command line that exec() runs
on the Cacti server. A device-supplied hostname such as
127.0.0.1; touch /tmp/pwned #
therefore ran a command on the poller.
Escape the hostname and namespace with cacti_escapeshellarg, and on Windows
strip the cmd.exe metacharacters (" & | ^ < > ( ) %) that cmd.exe interprets
despite quoting. A standalone regression test in tests/ verifies both.
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Escape account/query/tool output on render (html_escape/__esc), bind the remaining WMI queries as prepared statements, and drop wmi_accounts.php and wmi_tools.php from the Template Editor auth augment so credential management and the live query tool stay behind the WMI Management realm. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
The default separator (|+|) contains pipe characters, so the unquoted --delimiter=|+| made exec() split the command into a shell pipeline (exit 127, no data). Quote it in getcommand() while keeping the property raw for the explode() in fetch(). issue#5 Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
…hp8-transport-modernization
…p8-transport-modernization
Type the properties and method signatures, replace var/sizeof, and move the wmic command construction behind a Wmi_Transport interface (Wmic_Shell_Transport by default, injectable) so a PowerShell/CIM backend can be added without touching the parser. Behaviour and the shell-escaping guards are unchanged; the existing injection test still passes. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Short array syntax, single-quoted literals, comment style and indentation to match the checked-in .php-cs-fixer.php. Mechanical only; no logic change. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
There was a problem hiding this comment.
🟡 Changes recommended
There are correctness and security issues in the changed code paths (notably a broken include path in script/wmi-script.php and unsafe/unescaped error rendering) that should be fixed before merge.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR modernizes the WMI plugin for PHP 8.x and introduces a transport seam around the wmic execution path so alternative backends (e.g., CIM/PowerShell) can be added without rewriting parsing/callers. It also includes the stacked security work from #13 and #14 plus mechanical formatting changes from the repo’s php-cs-fixer ruleset.
Changes:
- Refactor
Linux_WMIto use typed properties/signatures and delegate execution to aWmi_Transport(defaultWmic_Shell_Transport) via aWmi_Request. - Apply broad PHP-CS-Fixer style updates (arrays, quoting, spacing) across the plugin, plus prepared SQL in remaining places.
- Add a standalone regression test for command injection quoting/escaping behavior.
File summaries
| File | Description |
|---|---|
| wmi_tools.php | Formatting updates plus output escaping for query results and headers. |
| wmi_script.php | Minor PHP 8 cleanup (__DIR__, arrays, spacing) for the CLI entrypoint. |
| wmi_queries.php | Formatting + output escaping of stored query fields on render. |
| wmi_accounts.php | Formatting + output escaping of stored account username on render. |
| tests/WmiCommandInjectionTest.php | New standalone regression test validating hostname/namespace escaping. |
| setup.php | Formatting changes + role augment scoping; navigation text updates. |
| script/wmi-script.php | Formatting + prepared query; CLI script logic adjustments. |
| poller_wmi.php | Formatting + prepared statements for remaining SQL; minor CLI output tweaks. |
| linux_wmi.php | Main refactor: transport interface, request object, typed API, safer decode behavior. |
| functions.php | Formatting + SQL binding; minor logic cleanups. |
| CHANGELOG.md | New changelog documenting the security/behavioral items. |
| .gitignore | Ignore .omc/ artifacts. |
Review details
- Files reviewed: 11/12 changed files
- Comments generated: 7
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| @@ -37,29 +38,31 @@ function wmi_script($hostname, $host_id, $wmiquery, $cmd = '', $arg1 = '', $arg2 | |||
|
|
|||
| include_once($config['base_path'] . '/plugins/wmi/linux-wmi.php'); | |||
Comment on lines
601
to
603
| } else { | ||
| print $wmi->error; | ||
| } |
Comment on lines
+59
to
63
| foreach ($tokens as $token) { | ||
| if ($next_ic) { | ||
| $exists = db_fetch_cell("SELECT COUNT(*) FROM wmi_wql_queries WHERE query RLIKE '^FROM\s$token$+'"); | ||
| $exists = db_fetch_cell_prepared('SELECT COUNT(*) FROM wmi_wql_queries WHERE query RLIKE ?', ['^FROM\s' . $token . '$+']); | ||
| } | ||
|
|
Comment on lines
+82
to
+86
| if ($status !== 0) { | ||
| $this->error = 'ERROR: ' . implode('<br>', $output); | ||
|
|
||
| return false; | ||
| } |
Comment on lines
44
to
48
| function plugin_wmi_uninstall() { | ||
| global $config; | ||
|
|
||
| return true; | ||
|
|
||
| include_once($config['base_path'] . '/lib/api_data_source.php'); |
Comment on lines
+349
to
+351
| print "<td class='nowrap'>" . __('Command', 'wmi') . '</td>'; | ||
| print "<td><textarea class='textAreaNotes' rows='4' cols='80' id='command' value='" . html_escape_request_var('command') . "'></textarea></td>"; | ||
| print "</tr><tr>"; | ||
| print "<td class='nowrap'>" . __('Primary Key', 'wmi') . "</td>"; | ||
| print '</tr><tr>'; |
Comment on lines
323
to
326
| function wmi_draw_navigation_text($nav) { | ||
| $nav['wmi_accounts.php:'] = array( | ||
| 'title' => __('WMI Autenication', 'wmi'), | ||
| $nav['wmi_accounts.php:'] = [ | ||
| 'title' => __('WMI Autenication', 'wmi'), | ||
| 'mapping' => 'index.php:', |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Draft. Stacked on #13 and #14 (both security). Merge order: #13, then #14, then this. The diff currently includes those two PRs; once they land I will rebase onto develop so this shows only the modernization. Do not merge before them.
Linux_WMI
var, the no-op__destruct, and the dead$configwrite in exec().Wmi_Transportinterface withWmic_Shell_Transportas the default, injectable via the constructor. The wmic command construction and shell escaping move into the transport;Linux_WMIbuilds aWmi_Requestand delegates. This is Phase 1 of the CIM/PowerShell backend (Windows compatibility: server-on-Windows unsupported and modern Windows host auth #15): a second transport can be added without touching the parser.decode()returns `` on a malformed blob instead of tripping a type error; theunserializestays `allowed_classes => false` (annotated for semgrep).Whole plugin
.php-cs-fixer.phpruleset (short arrays, single-quoted literals, comment style, indentation). Mechanical; no logic change.strict_paramdoes not alter the single-argumentarray_keys()calls.Sets the effective PHP floor at 8.x (matches Cacti develop, PHP ^8.1). No schema or storage-format change.