Refactor codebase for maintainability and efficiency improvements#202
Open
Refactor codebase for maintainability and efficiency improvements#202
Conversation
Introduce `CommandRunner` to standardize PHP shell execution (proc_open, popen, shell_exec) with consistent argument escaping, logging, and auditable execution points. Migrate existing shell command calls across the codebase to use `CommandRunner`, ensuring improved maintainability and security.
… and centralized reset Implement static methods to cache WMI and WScript.Shell COM objects, reducing object creation overhead and centralizing error recovery with `resetConnections`. Update existing methods to utilize cached connections for improved efficiency and maintainability.
…intainability and reduced redundancy.
Contributor
Review Summary by QodoCentralize command execution, optimize COM caching, and refactor service lookup logic
WalkthroughsDescription• Introduce CommandRunner class for centralized, auditable shell execution with consistent argument escaping and logging • Implement COM connection caching in Win32Native to reduce object creation overhead and improve efficiency • Refactor service-specific logic across multiple action classes to use getBinByName() method for reduced redundancy • Remove unnecessary stripslashes from GET and POST input sanitization in class.util.php Diagramflowchart LR
A["Shell Execution<br/>proc_open, popen,<br/>shell_exec"] -->|"Unified via"| B["CommandRunner<br/>Class"]
B -->|"Provides"| C["exec()<br/>stream()<br/>shellExec()<br/>background()"]
C -->|"Logs to"| D["Batch Log"]
E["Service Lookup<br/>Multiple if/elseif<br/>chains"] -->|"Refactored to"| F["getBinByName()"]
F -->|"Used by"| G["Action Classes<br/>changePort, checkPort,<br/>enable, switchVersion"]
H["COM Objects<br/>WMI, WScript.Shell"] -->|"Cached via"| I["Win32Native<br/>Static Methods"]
I -->|"Provides"| J["getWmiCimv2()<br/>getWscriptShell()<br/>resetConnections()"]
File Changes1. core/classes/class.commandrunner.php
|
Contributor
Code Review by Qodo
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+18
to
+75
| // ======================================================================== | ||
| // COM Connection Cache | ||
| // Each COM object is created once per PHP process and reused across calls. | ||
| // Call resetConnections() if a COM operation fails and you need a fresh object. | ||
| // ======================================================================== | ||
|
|
||
| /** @var COM|null Cached WMI connection to root/cimv2 (process/service queries) */ | ||
| private static ?COM $wmiCimv2 = null; | ||
|
|
||
| /** @var COM|null Cached WMI connection to root/default:StdRegProv (registry key checks) */ | ||
| private static ?COM $wmiStdRegProv = null; | ||
|
|
||
| /** @var COM|null Cached WScript.Shell object (registry values, shortcuts, special folders) */ | ||
| private static ?COM $wscriptShell = null; | ||
|
|
||
| /** | ||
| * Returns the cached WMI cimv2 connection, creating it on first use. | ||
| */ | ||
| private static function getWmiCimv2(): COM | ||
| { | ||
| if (self::$wmiCimv2 === null) { | ||
| self::$wmiCimv2 = new COM("winmgmts://./root/cimv2"); | ||
| } | ||
| return self::$wmiCimv2; | ||
| } | ||
|
|
||
| /** | ||
| * Returns the cached WMI StdRegProv connection, creating it on first use. | ||
| */ | ||
| private static function getWmiStdRegProv(): COM | ||
| { | ||
| if (self::$wmiStdRegProv === null) { | ||
| self::$wmiStdRegProv = new COM("winmgmts://./root/default:StdRegProv"); | ||
| } | ||
| return self::$wmiStdRegProv; | ||
| } | ||
|
|
||
| /** | ||
| * Returns the cached WScript.Shell object, creating it on first use. | ||
| */ | ||
| private static function getWscriptShell(): COM | ||
| { | ||
| if (self::$wscriptShell === null) { | ||
| self::$wscriptShell = new COM("WScript.Shell"); | ||
| } | ||
| return self::$wscriptShell; | ||
| } | ||
|
|
||
| /** | ||
| * Clears all cached COM connections. | ||
| * Call this after a COM operation fails so the next call gets a fresh connection. | ||
| */ | ||
| public static function resetConnections(): void | ||
| { | ||
| self::$wmiCimv2 = null; | ||
| self::$wmiStdRegProv = null; | ||
| self::$wscriptShell = null; | ||
| } |
Contributor
There was a problem hiding this comment.
1. Poisoned com cache 🐞 Bug ☼ Reliability
Win32Native caches COM objects (WMI/WScript.Shell) in static properties and reuses them across calls, but COM exceptions do not clear the cached instance. After a transient COM/WMI failure, subsequent operations may keep reusing the same broken COM object and repeatedly fail until the PHP process restarts.
Agent Prompt
### Issue description
`Win32Native` now caches COM connections (WMI cimv2, StdRegProv, WScript.Shell). If any COM call throws, the cached object remains in the static property and will be reused, potentially causing repeated failures for the remainder of the PHP process.
### Issue Context
Caching COM objects is fine for performance, but it needs a clear invalidation strategy on failure. Currently, COM-using methods catch exceptions and return without clearing the corresponding cached COM instance.
### Fix Focus Areas
- Add cache invalidation in COM exception paths (either `Win32Native::resetConnections()` or targeted nulling of the specific cached object) in each method that uses cached COM.
- file: core/classes/class.win32native.php[18-75]
- file: core/classes/class.win32native.php[84-129]
### Suggested implementation approach
- In each `catch (Exception $e)` block that indicates a COM/WMI failure, call `self::resetConnections()` (simple + safe), then log and return.
- Optionally, be more granular (e.g., only clear `self::$wmiCimv2` when cimv2 queries fail, only clear `self::$wscriptShell` for RegRead/RegWrite failures).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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
1 participant
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.
Summary
This pull request refactors and optimizes various parts of the codebase for improved maintainability, efficiency, and security.
Details of Changes
getBinByNamefor better maintainability and reduced redundancy.stripslashesfromGETandPOSTinputs inclass.util.php.Win32Native, reducing overhead and improving efficiency.CommandRunnerclass for consistent PHP shell execution. Refactored existing shell command calls to utilize this class, improving maintainability, security, and logging.Checklist