Fix plugin verification to detect directories without main PHP files#143
Fix plugin verification to detect directories without main PHP files#143
Conversation
Co-authored-by: swissspidy <841956+swissspidy@users.noreply.github.com>
Co-authored-by: swissspidy <841956+swissspidy@users.noreply.github.com>
Co-authored-by: swissspidy <841956+swissspidy@users.noreply.github.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
This comment was marked as resolved.
This comment was marked as resolved.
Co-authored-by: swissspidy <841956+swissspidy@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
Co-authored-by: swissspidy <841956+swissspidy@users.noreply.github.com>
Co-authored-by: swissspidy <841956+swissspidy@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
Co-authored-by: swissspidy <841956+swissspidy@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
…plicitly Co-authored-by: swissspidy <841956+swissspidy@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Code Review
This pull request effectively addresses a security vulnerability where plugin directories without a main PHP file were skipped during checksum verification. The changes are well-implemented, introducing fallbacks to detect such 'shadow' plugins by scanning the filesystem. The new logic in get_all_plugin_names and detect_version_from_directory is sound, and the UnfilteredPlugin fetcher correctly identifies plugins even without their main file. The addition of a Behat test scenario ensures this fix is properly verified. I have a couple of suggestions to improve code quality and maintainability, but overall this is an excellent contribution.
| if ( is_dir( $plugin_dir ) && is_readable( $plugin_dir ) ) { | ||
| $dirs = @scandir( $plugin_dir ); // phpcs:ignore WordPress.PHP.NoSilencedErrors.Discouraged | ||
| if ( false !== $dirs ) { | ||
| foreach ( $dirs as $dir ) { | ||
| // Skip special directories and files | ||
| if ( '.' === $dir || '..' === $dir ) { | ||
| continue; | ||
| } | ||
|
|
||
| $full_path = $plugin_dir . '/' . $dir; | ||
| // Only include real directories, not symlinks or files | ||
| if ( is_dir( $full_path ) && ! is_link( $full_path ) && ! in_array( $dir, $names, true ) ) { | ||
| $names[] = $dir; | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider refactoring this block to use DirectoryIterator. This provides a more modern, object-oriented approach to iterating over filesystem entries and handles errors through exceptions, which is cleaner than suppressing warnings with @scandir. It also simplifies the loop by providing methods like isDot(), isDir(), and isLink(), making the code more readable and robust.
if ( is_dir( $plugin_dir ) && is_readable( $plugin_dir ) ) {
try {
foreach ( new DirectoryIterator( $plugin_dir ) as $fileinfo ) {
if ( $fileinfo->isDot() || ! $fileinfo->isDir() || $fileinfo->isLink() ) {
continue;
}
$dir = $fileinfo->getFilename();
if ( ! in_array( $dir, $names, true ) ) {
$names[] = $dir;
}
}
} catch ( UnexpectedValueException $e ) {
WP_CLI::warning( "Could not scan plugin directory '{$plugin_dir}': " . $e->getMessage() );
}
}| } | ||
| } | ||
|
|
||
| return array_unique( $names ); |
There was a problem hiding this comment.
There was a problem hiding this comment.
Pull request overview
This PR fixes a security vulnerability where wp plugin verify-checksums would skip plugin directories that lack valid main PHP files, allowing malware to hide undetected in these "shadow" directories. The fix enhances plugin detection to scan the filesystem for all plugin directories and verify them against WordPress.org checksums, regardless of whether they have valid plugin headers.
Changes:
- Enhanced
get_all_plugin_names()to scan the filesystem for all plugin directories, not just those with valid headers, with security measures including symlink exclusion - Modified
UnfilteredPlugin::get()to detect plugin directories even when main files are missing - Added
detect_version_from_directory()fallback method to find version information from any PHP file in a plugin directory
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/WP_CLI/Fetchers/UnfilteredPlugin.php | Adds fallback logic to detect plugin directories without main files by checking filesystem directly |
| src/Checksum_Plugin_Command.php | Enhances plugin detection with filesystem scanning, adds version detection fallback, includes security measures and user warnings |
| features/checksum-plugin.feature | Adds test scenario for verifying plugins with missing/renamed main files |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| // Try scanning PHP files for Version header using WordPress's get_file_data() | ||
| $files = glob( $plugin_path . '/*.php' ); |
There was a problem hiding this comment.
The glob pattern does not escape special glob characters in the plugin directory path. If a plugin directory contains characters like *, ?, [, or ], the glob function could match unintended files or fail unexpectedly. Consider using glob() with the GLOB_BRACE flag or pre-escaping the path with a function that escapes glob metacharacters, or use an alternative approach like scandir() followed by filtering.
| When I try `wp plugin verify-checksums --all --format=json` | ||
| Then STDOUT should contain: | ||
| """ | ||
| "plugin_name":"duplicate-post" | ||
| """ | ||
| And STDERR should contain: | ||
| """ | ||
| Warning: Plugin duplicate-post main file is missing: duplicate-post/duplicate-post.php | ||
| """ |
There was a problem hiding this comment.
The test scenario for verifying a plugin directory with --all flag (lines 232-240) doesn't specify whether version detection succeeds or fails. If the version cannot be auto-detected from the renamed file, the plugin would be skipped with a "Could not retrieve the version" warning, not verified. The test should either verify that version detection succeeds from other PHP files in the directory, or it should expect the version warning and skip message. Consider adding explicit assertions about the version detection outcome.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Summary
Fixes security issue where
wp plugin verify-checksumsskipped plugin directories without valid main plugin files, allowing malware to hide in these "shadow" directories.Changes
1. Enhanced Plugin Detection (
get_all_plugin_names())2. Enhanced Plugin Fetcher (
UnfilteredPlugin::get())3. Version Detection Fallback (
detect_version_from_directory())get_file_data()function.phpfiles (standard PHP extension)get_file_data()handles file reading efficiently (reads only 8KB)4. User Warnings
5. Test Coverage
--versionflag when main file is missing to ensure verification proceedsSecurity Impact
Before: Attackers could hide malicious files in plugin directories by removing/renaming the main plugin file. These directories were completely ignored during checksum verification.
After: All plugin directories are verified against WordPress.org checksums, regardless of whether they have valid plugin headers. Malware cannot hide using this technique.
Usage Note
When a plugin's main file is missing or renamed and version cannot be auto-detected from other PHP files in the directory, users can explicitly provide the version using the
--versionflag:Backward Compatibility
✅ Fully backward compatible - existing plugins with valid headers work exactly as before
✅ Only affects detection of plugin directories without valid main files (previously ignored)
✅ No breaking changes to API or command syntax
Files Changed
src/Checksum_Plugin_Command.php: +77 linessrc/WP_CLI/Fetchers/UnfilteredPlugin.php: +11 linesfeatures/checksum-plugin.feature: +34 lines (test)Total: 122 lines added, 21 lines removed
Testing Status
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.