Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the elFinder library to version 2.1.66 and adds a GitHub Actions workflow for plugin checking. The update includes security improvements, bug fixes, and enhancements to the file manager functionality, along with code quality improvements across the backend and frontend.
Changes:
- Updated elFinder from version 2.1.65 to 2.1.66
- Added GitHub Actions workflow for WordPress plugin checks
- Improved code formatting and consistency across backend PHP files
- Enhanced security with better file validation and syntax checking
Reviewed changes
Copilot reviewed 86 out of 105 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tools/build_elfinder.sh | Updated build script to use pnpm and improved path handling |
| submodule/sources/elFinder | Updated elFinder submodule to version 2.1.66 |
| libs/elFinder/php/*.php | Updated elFinder PHP classes with compatibility fixes and improvements |
| libs/elFinder/js/*.js | Updated elFinder JavaScript files and CDN resources |
| libs/elFinder/css/elfinder.full.css | Updated version information in CSS file |
| frontend/src/**/*.css | Improved CSS formatting and consistency |
| frontend/src/components/utilities/Tabs/Tabs.tsx | Improved code formatting |
| file-manager.php | Added ABSPATH security check |
| composer.json | Updated dependencies to newer versions |
| backend/hooks/*.php | Reorganized imports for better code structure |
| backend/functions/*.php | Added ABSPATH security checks |
| backend/app/**/*.php | Multiple improvements: code formatting, import ordering, and logic refinements |
| .github/workflows/plugin-check.yml | Added new GitHub Actions workflow for plugin checks |
| .distignore | Added additional files to ignore in distribution |
Comments suppressed due to low confidence (1)
libs/elFinder/php/elFinderVolumeSFTPphpseclib.class.php:1
- Corrected spelling of 'substitue' to 'substitute'.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary of ChangesHello @abdul-kaioum, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on a significant update to the elFinder file manager submodule and its associated Composer dependencies. It also includes extensive backend PHP code refactoring to enhance code quality, improve error handling, and ensure better consistency across the codebase. Frontend changes are minimal, primarily addressing styling and a minor component structure adjustment. Highlights
Changelog
Ignored Files
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
🔍 WordPress Plugin Check Report
📊 Report
❌ Errors (3)📁 backend/app/Providers/FileEditValidator.php (1 error)
📁 backend/app/Providers/AccessControlProvider.php (1 error)
📁 backend/app/Providers/PhpSyntaxChecker.php (1 error)
|
| 📍 Line | 🔖 Check | 💬 Message |
|---|---|---|
216 |
WordPress.Security.NonceVerification.Recommended | Processing form data without nonce verification. |
217 |
WordPress.Security.NonceVerification.Recommended | Processing form data without nonce verification. |
217 |
WordPress.Security.ValidatedSanitizedInput.MissingUnslash | $_REQUEST['content'] not unslashed before sanitization. Use wp_unslash() or similar |
217 |
WordPress.Security.ValidatedSanitizedInput.InputNotSanitized | Detected usage of a non-sanitized input variable: $_REQUEST['content'] |
📁 backend/app/Providers/PhpSyntaxChecker.php (3 warnings)
| 📍 Line | 🔖 Check | 💬 Message |
|---|---|---|
94 |
WordPress.Security.ValidatedSanitizedInput.InputNotSanitized | Detected usage of a non-sanitized input variable: $_SERVER['PHP_AUTH_USER'] |
94 |
WordPress.Security.ValidatedSanitizedInput.InputNotSanitized | Detected usage of a non-sanitized input variable: $_SERVER['PHP_AUTH_PW'] |
100 |
Squiz.PHP.DiscouragedFunctions.Discouraged | The use of function set_time_limit() is discouraged |
📁 composer.json (1 warning)
| 📍 Line | 🔖 Check | 💬 Message |
|---|---|---|
0 |
missing_composer_json_file | The "/vendor" directory using composer exists, but "composer.json" file is missing. |
📁 backend/app/Http/Controllers/FileManagerController.php (2 warnings)
| 📍 Line | 🔖 Check | 💬 Message |
|---|---|---|
28 |
WordPress.Security.NonceVerification.Recommended | Processing form data without nonce verification. |
28 |
WordPress.Security.ValidatedSanitizedInput.InputNotValidated | Detected usage of a possibly undefined superglobal array index: $_REQUEST['cmd']. Check that the array index exists before using it. |
📁 backend/app/Views/Admin.php (3 warnings)
| 📍 Line | 🔖 Check | 💬 Message |
|---|---|---|
258 |
WordPress.Security.ValidatedSanitizedInput.InputNotValidated | Detected usage of a possibly undefined superglobal array index: $_SERVER['HTTP_USER_AGENT']. Check that the array index exists before using it. |
258 |
WordPress.Security.ValidatedSanitizedInput.MissingUnslash | $_SERVER['HTTP_USER_AGENT'] not unslashed before sanitization. Use wp_unslash() or similar |
258 |
WordPress.Security.ValidatedSanitizedInput.InputNotSanitized | Detected usage of a non-sanitized input variable: $_SERVER['HTTP_USER_AGENT'] |
📁 backend/app/Providers/FileManager/FinderConnector.php (2 warnings)
| 📍 Line | 🔖 Check | 💬 Message |
|---|---|---|
58 |
WordPress.Security.ValidatedSanitizedInput.MissingUnslash | $_SERVER['HTTP_RANGE'] not unslashed before sanitization. Use wp_unslash() or similar |
58 |
WordPress.Security.ValidatedSanitizedInput.InputNotSanitized | Detected usage of a non-sanitized input variable: $_SERVER['HTTP_RANGE'] |
📁 backend/app/Providers/PermissionsProvider.php (4 warnings)
| 📍 Line | 🔖 Check | 💬 Message |
|---|---|---|
500 |
WordPress.Security.NonceVerification.Recommended | Processing form data without nonce verification. |
501 |
WordPress.Security.NonceVerification.Recommended | Processing form data without nonce verification. |
511 |
WordPress.Security.NonceVerification.Recommended | Processing form data without nonce verification. |
512 |
WordPress.Security.NonceVerification.Recommended | Processing form data without nonce verification. |
📁 backend/app/Plugin.php (5 warnings)
| 📍 Line | 🔖 Check | 💬 Message |
|---|---|---|
416 |
WordPress.Security.NonceVerification.Recommended | Processing form data without nonce verification. |
416 |
WordPress.Security.NonceVerification.Recommended | Processing form data without nonce verification. |
416 |
WordPress.Security.ValidatedSanitizedInput.MissingUnslash | $_REQUEST['action'] not unslashed before sanitization. Use wp_unslash() or similar |
417 |
Squiz.PHP.DiscouragedFunctions.Discouraged | The use of function ini_set() is discouraged |
418 |
Squiz.PHP.DiscouragedFunctions.Discouraged | The use of function ini_set() is discouraged |
📁 backend/functions/global.php (1 warning)
| 📍 Line | 🔖 Check | 💬 Message |
|---|---|---|
63 |
WordPress.PHP.DevelopmentFunctions.error_log_error_log | error_log() found. Debug code should not normally be used in production. |
📁 backend/functions/common.php (4 warnings)
| 📍 Line | 🔖 Check | 💬 Message |
|---|---|---|
33 |
WordPress.PHP.DevelopmentFunctions.error_log_print_r | print_r() found. Debug code should not normally be used in production. |
68 |
WordPress.Security.ValidatedSanitizedInput.InputNotValidated | Detected usage of a possibly undefined superglobal array index: $_SERVER['REQUEST_URI']. Check that the array index exists before using it. |
68 |
WordPress.Security.ValidatedSanitizedInput.MissingUnslash | $_SERVER['REQUEST_URI'] not unslashed before sanitization. Use wp_unslash() or similar |
68 |
WordPress.Security.ValidatedSanitizedInput.InputNotSanitized | Detected usage of a non-sanitized input variable: $_SERVER['REQUEST_URI'] |
📁 backend/app/Http/Controllers/PermissionsController.php (1 warning)
| 📍 Line | 🔖 Check | 💬 Message |
|---|---|---|
56 |
WordPressVIPMinimum.Performance.WPQueryParams.PostNotIn_exclude | Using exclusionary parameters, like exclude, in calls to get_posts() should be done with caution, see https://wpvip.com/documentation/performance-improvements-by-removing-usage-of-post__not_in/ for more information. |
📁 readme.txt (2 warnings)
| 📍 Line | 🔖 Check | 💬 Message |
|---|---|---|
0 |
mismatched_plugin_name | Plugin name "File Manager" is different from the name declared in plugin header "Bit File Manager". |
0 |
readme_parser_warnings_no_short_description_present | The "Short Description" section is missing. An excerpt was generated from your main plugin description. |
🤖 Generated by WordPress Plugin Check Action • Learn more about Plugin Check
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Abdul Kaioum <58425930+abdul-kaioum@users.noreply.github.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request primarily focuses on updating dependencies, including elfinder, and applying various code style fixes and refactorings. The changes introduce significant improvements, such as enhancing the PHP syntax check for better security and performance, and fixing potential security vulnerabilities related to path traversal and arbitrary file deletion. The file move operation has also been made more robust. While the overall changes are positive, I've identified a few areas for further improvement, including a potential bug in path calculation and opportunities for code simplification. My specific comments are detailed below.
| } | ||
| if (empty($file['url']) && $this->URL) { | ||
| $path = str_replace($this->separator, '/', substr($this->decode($hash), strlen(rtrim($this->root, '/' . $this->separator)) + 1)); | ||
| $path = str_replace($this->separator, '/', substr($this->decode($hash), strlen(trim($this->root, '/' . $this->separator)))); |
There was a problem hiding this comment.
The change from rtrim to trim and the removal of + 1 might introduce a bug. trim will remove leading slashes from $this->root, which could lead to an incorrect path calculation in substr. For example, if $this->root is /var/www/files/, trim would make it var/www/files, and strlen would be incorrect, leading to a malformed relative path. The previous implementation with rtrim and + 1 seemed more robust for handling root paths with or without a trailing slash. I suggest reverting to a logic similar to the original to ensure correct path handling.
$path = str_replace($this->separator, '/', substr($this->decode($hash), strlen(rtrim($this->root, '/' . $this->separator)) + 1));| $validatedIds = array_map(function ($id) { | ||
| return \intval($id); | ||
| }, $request->ids); |
There was a problem hiding this comment.
While the current implementation is correct, it can be simplified. Instead of using an anonymous function with array_map, you can directly pass 'intval' as a callable string. This is more concise and generally more readable.
$validatedIds = array_map('intval', $request->ids);| return ''; | ||
| } else { | ||
| if ($path && strpos($path, $this->root) === 0) { | ||
| if (strpos($path, $this->root) === 0) { |
There was a problem hiding this comment.
Removing the $path && check could potentially lead to warnings or errors if _relpath() is ever called with a null or empty string value for $path. While the current call sites might prevent this, it's safer to keep the guard as a defensive programming measure, especially for a protected method that could be used by subclasses in unexpected ways.
if ($path && strpos($path, $this->root) === 0) {There was a problem hiding this comment.
Pull request overview
Copilot reviewed 86 out of 105 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
tools/build_elfinder.sh:1
- Duplicate echo statement "Building elFinder from submodule." appears on both lines 3 and 6. Remove the duplicate on line 6 as it's redundant after the cd command.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try { | ||
| token_get_all($content); | ||
| } catch (ParseError $e) { | ||
| $error = sprintf( | ||
| /* translators: 1: file name, 2: PHP parse error message */ | ||
| __('Syntax error in file: %1$s. Error: %2$s', 'file-manager'), | ||
| $fileName, | ||
| $e->getMessage() | ||
| ); |
There was a problem hiding this comment.
Using token_get_all() for PHP syntax validation is less robust than exec('php -l') as it only performs tokenization, not full parsing. This could miss syntax errors that would be caught by the PHP interpreter. Consider documenting this limitation or providing an option to use the more thorough validation method when available.
| try { | |
| token_get_all($content); | |
| } catch (ParseError $e) { | |
| $error = sprintf( | |
| /* translators: 1: file name, 2: PHP parse error message */ | |
| __('Syntax error in file: %1$s. Error: %2$s', 'file-manager'), | |
| $fileName, | |
| $e->getMessage() | |
| ); | |
| if (\function_exists('exec')) { | |
| $phpBinary = \defined('PHP_BINARY') ? PHP_BINARY : 'php'; | |
| $tmpFile = @\tempnam(\sys_get_temp_dir(), 'bfm_php_lint_'); | |
| if ($tmpFile !== false) { | |
| @\file_put_contents($tmpFile, $content); | |
| $cmd = \escapeshellarg($phpBinary) . ' -l ' . \escapeshellarg($tmpFile) . ' 2>&1'; | |
| $output = []; | |
| $returnVar = 0; | |
| @\exec($cmd, $output, $returnVar); | |
| @\unlink($tmpFile); | |
| if ($returnVar !== 0) { | |
| $error = sprintf( | |
| /* translators: 1: file name, 2: PHP parse error message */ | |
| __('Syntax error in file: %1$s. Error: %2$s', 'file-manager'), | |
| $fileName, | |
| \implode("\n", $output) | |
| ); | |
| } | |
| } | |
| } else { | |
| try { | |
| token_get_all($content); | |
| } catch (ParseError $e) { | |
| $error = sprintf( | |
| /* translators: 1: file name, 2: PHP parse error message */ | |
| __('Syntax error in file: %1$s. Error: %2$s', 'file-manager'), | |
| $fileName, | |
| $e->getMessage() | |
| ); | |
| } |
| public function log() | ||
| { | ||
| return $this->hasMany(Log::class, 'user_id', 'ID'); | ||
| } |
There was a problem hiding this comment.
The method name 'log' is ambiguous and could conflict with logging functionality. Consider renaming to 'logs' (plural) to better indicate it returns a collection of log records.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 92 out of 111 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| protected function encode($path) | ||
| { | ||
| if (!is_null($path) && $path !== '') { | ||
| if ($path !== '') { | ||
|
|
||
| // cut ROOT from $path for security reason, even if hacker decodes the path he will not know the root | ||
| $p = $this->relpathCE($path); | ||
| // if reqesting root dir $path will be empty, then assign '/' as we cannot leave it blank for crypt | ||
| if ($p === '') { | ||
| $p = $this->separator; | ||
| } | ||
| // change separator | ||
| if ($this->separatorForHash) { | ||
| $p = str_replace($this->separator, $this->separatorForHash, $p); | ||
| } | ||
| // TODO crypt path and return hash | ||
| $hash = $this->crypt($p); | ||
| // hash is used as id in HTML that means it must contain vaild chars | ||
| // make base64 html safe and append prefix in begining | ||
| $hash = strtr(base64_encode($hash), '+/=', '-_.'); | ||
| // remove dots '.' at the end, before it was '=' in base64 | ||
| $hash = rtrim($hash, '.'); | ||
| // append volume id to make hash unique | ||
| return $this->id . $hash; | ||
| } | ||
| //TODO: Add return statement here | ||
| } |
There was a problem hiding this comment.
encode() no longer guards against null ($path !== '' is true for null) and also has no return statement when $path is empty, which can lead to null being returned where a string hash is expected. Restore the null/empty handling and ensure the function always returns a string.
| protected function decode($hash) | ||
| { | ||
| if ($hash && strpos($hash, $this->id) === 0) { | ||
| if (strpos($hash, $this->id) === 0) { | ||
| // cut volume id after it was prepended in encode | ||
| $h = substr($hash, strlen($this->id)); | ||
| // replace HTML safe base64 to normal | ||
| $h = base64_decode(strtr($h, '-_.', '+/=')); |
There was a problem hiding this comment.
decode() now calls strpos($hash, $this->id) without checking that $hash is a non-empty string. If $hash can be null/false, PHP 8+ will throw a TypeError. Restore the previous guard (e.g. $hash && ...) or cast/validate $hash before using string functions.
| //Delete temp file paths | ||
| if (!empty($GLOBALS['elFinderTempFiles'])) { | ||
| foreach (array_keys($GLOBALS['elFinderTempFiles']) as $f) { | ||
| is_file($f) && is_writable($f) && unlink($f); | ||
| //Make sure paths are safe before deleting them | ||
| $tf = elFinder::$commonTempPath . DIRECTORY_SEPARATOR . basename($f); | ||
| is_file($tf) && is_writable($tf) && unlink($tf); | ||
| } | ||
| unset($f); | ||
| } | ||
|
|
||
| //Delete abort file paths | ||
| if (!empty($GLOBALS['elFinderAbortFiles'])) { | ||
| foreach (array_keys($GLOBALS['elFinderAbortFiles']) as $f) { | ||
| //Make sure paths are safe before deleting them | ||
| $tf = elFinder::$connectionFlagsPath . DIRECTORY_SEPARATOR . basename($f); | ||
| is_file($tf) && is_writable($tf) && unlink($tf); | ||
| } |
There was a problem hiding this comment.
onShutdown() derives deletion targets with elFinder::$commonTempPath . DIRECTORY_SEPARATOR . basename($f) / connectionFlagsPath .... If either base path is empty, this can resolve to an absolute path like /somefile and risks deleting unintended files. Add a guard to only delete when the base directory is non-empty and the resolved realpath stays within that directory.
| @@ -4492,7 +4510,7 @@ protected function itemUnlock($hash) | |||
| return true; | |||
| } | |||
| $lock = elFinder::$commonTempPath . DIRECTORY_SEPARATOR . $hash . '.lock'; | |||
| $cnt = (int)file_get_contents($lock); | |||
| $cnt = file_get_contents($lock); | |||
| if (--$cnt < 1) { | |||
| unlink($lock); | |||
There was a problem hiding this comment.
itemLock()/itemUnlock() now use file_get_contents($lock) + 1 / --$cnt without casting/handling false. If the lock file is empty/unreadable, arithmetic on false/strings can yield incorrect counters and prevent proper unlocking. Cast to (int) (and handle false explicitly) before increment/decrement.
| if ($onetime) { | ||
| $volume = null; | ||
| $tmpdir = elFinder::$commonTempPath; | ||
| if (!$tmpdir || !is_file($tmpf = $tmpdir . DIRECTORY_SEPARATOR . 'ELF' . $target)) { | ||
| if (!$tmpdir || !is_file($tmpf = $tmpdir . DIRECTORY_SEPARATOR . 'ELF' . basename($target))) { | ||
| return $a404; | ||
| } | ||
| $GLOBALS['elFinderTempFiles'][$tmpf] = true; | ||
| if ($file = json_decode(file_get_contents($tmpf), true)) { | ||
| $src = base64_decode($file['file']); | ||
| $src = $tmpdir . DIRECTORY_SEPARATOR . basename(base64_decode($file['file'])); | ||
| if (!is_file($src) || !($fp = fopen($src, 'rb'))) { | ||
| return $a404; |
There was a problem hiding this comment.
The one-time download flow now reconstructs $src as $tmpdir . DIRECTORY_SEPARATOR . basename(base64_decode($file['file'])), but the stored value appears to be an absolute path (set when creating the one-time link). This will likely 404 because the file won’t exist in $tmpdir. Either always copy the source into $tmpdir when generating the token (and store only the basename), or store a validated relative path and verify it stays inside $tmpdir when resolving.
| server.httpServer.close(() => { | ||
| server.watcher.close() | ||
| removeStoredPort() | ||
| }) |
There was a problem hiding this comment.
configureServer is calling server.httpServer.close() during startup. That will immediately shut down the dev server (and will also close the watcher), which likely makes vite dev unusable. Instead of invoking close(), register a listener (e.g. server.httpServer.once('close', ...)) or use process.on('exit'/'SIGINT') to run cleanup when the server is actually stopping.
| if (empty($file['url']) && $this->URL) { | ||
| $path = str_replace($this->separator, '/', substr($this->decode($hash), strlen(rtrim($this->root, '/' . $this->separator)) + 1)); | ||
| $path = str_replace($this->separator, '/', substr($this->decode($hash), strlen(trim($this->root, '/' . $this->separator)))); | ||
| if ($this->encoding) { |
There was a problem hiding this comment.
getContentUrl() now builds $path using trim($this->root, ...) and a different substr() offset. trim() removes characters from both ends (not just trailing separators), so the computed offset can be wrong and produce broken URLs. Consider reverting to the previous rtrim(...)+1 style (or otherwise compute the relative path safely) so URLs don’t include missing/extra path segments.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 93 out of 112 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $volume = null; | ||
| $tmpdir = elFinder::$commonTempPath; | ||
| if (!$tmpdir || !is_file($tmpf = $tmpdir . DIRECTORY_SEPARATOR . 'ELF' . $target)) { | ||
| if (!$tmpdir || !is_file($tmpf = $tmpdir . DIRECTORY_SEPARATOR . 'ELF' . basename($target))) { |
There was a problem hiding this comment.
The file path construction uses string concatenation which could be vulnerable to path traversal attacks. While basename() is used later in the file cleanup, it would be safer to validate or sanitize $target earlier to ensure it doesn't contain directory traversal sequences before constructing the file path.
| //Make sure paths are safe before deleting them | ||
| $tf = elFinder::$commonTempPath . DIRECTORY_SEPARATOR . basename($f); | ||
| is_file($tf) && is_writable($tf) && unlink($tf); |
There was a problem hiding this comment.
The temporary file cleanup logic on lines 5403-5405 only deletes files in the common temp path by reconstructing the path with basename($f). However, files may have been stored with full paths in the elFinderTempFiles array. If a file was registered with a path outside the common temp directory, it won't be properly deleted. Consider checking whether the original path is within the expected temp directory before deletion.
| $wpError = (new PhpSyntaxChecker())->check($content, $realFile); | ||
|
|
There was a problem hiding this comment.
The PHP file syntax validation has been changed from using exec('php -l') to a loopback HTTP request approach. While this is more secure (avoiding shell execution), the new PhpSyntaxChecker class performs actual file writes and HTTP requests which could impact performance significantly, especially for large files or high-traffic scenarios. Consider adding configuration options to control this behavior or implement caching for recently validated files.
| $wpError = (new PhpSyntaxChecker())->check($content, $realFile); | |
| // Allow disabling of the syntax check cache via constant if needed. | |
| $cacheEnabled = !(\defined('BFM_DISABLE_SYNTAX_CACHE') && BFM_DISABLE_SYNTAX_CACHE); | |
| $wpError = null; | |
| if ($cacheEnabled) { | |
| // Simple per-request cache keyed by file path and content hash. | |
| static $cache = []; | |
| $cacheKey = $realFile . ':' . md5($content); | |
| if (array_key_exists($cacheKey, $cache)) { | |
| $wpError = $cache[$cacheKey]; | |
| } else { | |
| $wpError = (new PhpSyntaxChecker())->check($content, $realFile); | |
| $cache[$cacheKey] = $wpError; | |
| } | |
| } else { | |
| $wpError = (new PhpSyntaxChecker())->check($content, $realFile); | |
| } |
| public function check($content, $realFile) | ||
| { | ||
| $previousContent = file_get_contents($realFile); | ||
|
|
||
| if (!is_writable($realFile)) { | ||
| return new WP_Error('file_not_writable'); | ||
| } | ||
|
|
||
| $f = fopen($realFile, 'w+'); | ||
|
|
||
| if (false === $f) { | ||
| return new WP_Error('file_not_writable'); | ||
| } | ||
|
|
||
| $written = fwrite($f, $content); | ||
| fclose($f); | ||
|
|
||
| if (false === $written) { | ||
| return new WP_Error('unable_to_write', __('Unable to write to file.')); | ||
| } | ||
|
|
||
| wp_opcache_invalidate($realFile, true); |
There was a problem hiding this comment.
The PhpSyntaxChecker writes the content to the actual file on line 39 before validation, which means invalid PHP code will temporarily exist in the production file. If a fatal error occurs between writing the file and completing the loopback validation, the file could be left in a broken state. While there's a rollback mechanism, consider additional safeguards like atomic file operations or temporary staging.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 93 out of 112 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public function check($content, $realFile) | ||
| { | ||
| $previousContent = file_get_contents($realFile); | ||
|
|
||
| if (!is_writable($realFile)) { | ||
| return new WP_Error('file_not_writable'); | ||
| } | ||
|
|
||
| // phpcs:disable WordPress.WP.AlternativeFunctions.file_system_operations_fopen, WordPress.WP.AlternativeFunctions.file_system_operations_fwrite, WordPress.WP.AlternativeFunctions.file_system_operations_fclose | ||
| $f = fopen($realFile, 'w+'); | ||
|
|
||
| if (false === $f) { | ||
| return new WP_Error('file_not_writable'); | ||
| } | ||
|
|
||
| $written = fwrite($f, $content); | ||
| fclose($f); | ||
|
|
||
| if (false === $written) { | ||
| return new WP_Error('unable_to_write', __('Unable to write to file.', 'file-manager')); | ||
| } | ||
|
|
||
| wp_opcache_invalidate($realFile, true); | ||
|
|
||
| $result = $this->loopbackRequest(); | ||
|
|
||
| if (true !== $result) { | ||
| file_put_contents($realFile, $previousContent); | ||
| wp_opcache_invalidate($realFile, true); | ||
|
|
||
| $message = isset($result['message']) | ||
| ? $result['message'] | ||
| : __('An error occurred. Please try again later.', 'file-manager'); | ||
|
|
||
| $data = $result; | ||
| unset($data['message']); | ||
|
|
||
| return new WP_Error('php_error', $message, $data); | ||
| } | ||
| } |
There was a problem hiding this comment.
The PHP syntax checker writes user-provided content directly to the real file on the filesystem before validation. If the loopback request fails or times out, the rollback mechanism (line 52) attempts to restore the previous content. However, there's a potential window of vulnerability: if the process terminates unexpectedly between writing the new content (line 40) and the rollback (line 52), the malicious/invalid code could remain on the filesystem.
Additionally, using file_get_contents/file_put_contents with phpcs:disable comments suggests awareness of WordPress coding standards but bypasses them, which may pose security concerns in a file manager context where user-provided content is written to executable PHP files.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 94 out of 113 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const VAR_PREFIX = 'bit_fm_'; | ||
|
|
||
| const VERSION = '6.8.6'; | ||
| const VERSION = '6.8.9'; |
There was a problem hiding this comment.
The Config::VERSION is set to '6.8.9' but the readme.txt shows version 6.8.7 and the file-manager.php header shows 6.8.7. All version numbers across the plugin should be consistent.
| function fileSystemAdapter() | ||
| { | ||
| global $wp_filesystem; | ||
|
|
||
| if (empty($wp_filesystem)) { | ||
| require_once ABSPATH . '/wp-admin/includes/file.php'; | ||
| WP_Filesystem(); | ||
| } | ||
|
|
||
| return $wp_filesystem; | ||
| } |
There was a problem hiding this comment.
The fileSystemAdapter function returns the global $wp_filesystem but does not handle the case where WP_Filesystem() fails to initialize. If WP_Filesystem() returns false or $wp_filesystem remains empty, this function will return an invalid value. Add error handling to check if initialization was successful.
| $result = $this->loopbackRequest(); | ||
|
|
||
| if (true !== $result) { | ||
| file_put_contents($realFile, $previousContent); |
There was a problem hiding this comment.
This code uses file_put_contents() which is flagged by the phpcs:disable comment, but then calls fileSystemAdapter()->delete() for cleanup. For consistency and to respect WordPress filesystem abstraction, the initial write operation should also use the WP_Filesystem API (e.g., $wp_filesystem->put_contents()) instead of file_put_contents().
| file_put_contents($realFile, $previousContent); | |
| global $wp_filesystem; | |
| if (!\function_exists('\WP_Filesystem')) { | |
| require_once ABSPATH . 'wp-admin/includes/file.php'; | |
| } | |
| if (!$wp_filesystem instanceof \WP_Filesystem_Base) { | |
| \WP_Filesystem(); | |
| } | |
| if ($wp_filesystem instanceof \WP_Filesystem_Base) { | |
| $wp_filesystem->put_contents($realFile, $previousContent, FS_CHMOD_FILE); | |
| } else { | |
| // phpcs:ignore WordPress.WP.AlternativeFunctions.file_system_operations_file_put_contents | |
| \file_put_contents($realFile, $previousContent); | |
| } |
| $wpError = (new PhpSyntaxChecker())->check($content, $realFile); | ||
|
|
||
| if ($wpError instanceof WP_Error) { | ||
| throw new PreCommandException($wpError->get_error_message()); |
There was a problem hiding this comment.
The removal of the exec()-based syntax checking without providing an alternative check means PHP syntax errors may not be caught before writing files. The PhpSyntaxChecker::check() method writes content to the file first, then performs a loopback HTTP request. If that request fails or times out for reasons unrelated to syntax errors (network issues, server configuration), valid code could be rejected. Consider adding fallback validation or better error messaging to distinguish between syntax errors and connectivity issues.
| $wpError = (new PhpSyntaxChecker())->check($content, $realFile); | |
| if ($wpError instanceof WP_Error) { | |
| throw new PreCommandException($wpError->get_error_message()); | |
| $syntaxOk = null; | |
| // Primary, local syntax check using "php -l" on a temporary file, if exec() is available. | |
| if (\function_exists('exec')) { | |
| $tmpFile = @tempnam(\sys_get_temp_dir(), 'bfm_php_syntax_'); | |
| if ($tmpFile !== false) { | |
| // Write the in-memory content to the temporary file without touching the real file. | |
| if (@file_put_contents($tmpFile, $content) !== false) { | |
| $phpBinary = \defined('PHP_BINARY') && PHP_BINARY ? PHP_BINARY : 'php'; | |
| $command = escapeshellarg($phpBinary) . ' -l ' . escapeshellarg($tmpFile) . ' 2>&1'; | |
| $output = []; | |
| $exitCode = 0; | |
| @exec($command, $output, $exitCode); | |
| // Ensure the temporary file is always removed. | |
| @unlink($tmpFile); | |
| if ($exitCode !== 0) { | |
| // A non-zero exit code from "php -l" indicates a syntax error. | |
| $message = trim(implode("\n", $output)); | |
| if ($message === '') { | |
| $message = __('PHP syntax check failed with an unknown error.', 'file-manager'); | |
| } | |
| throw new PreCommandException($message); | |
| } | |
| $syntaxOk = true; | |
| } else { | |
| // Clean up if the file could not be written. | |
| @unlink($tmpFile); | |
| } | |
| } | |
| } | |
| $wpError = (new PhpSyntaxChecker())->check($content, $realFile); | |
| if ($wpError instanceof WP_Error) { | |
| $originalMessage = $wpError->get_error_message(); | |
| if ($syntaxOk === true) { | |
| // Local "php -l" check passed, but the HTTP-based checker failed. | |
| // Provide clearer context so users can distinguish syntax errors from connectivity issues. | |
| $wrappedMessage = sprintf( | |
| /* translators: 1: original error message from HTTP-based syntax checker */ | |
| __('PHP syntax appears valid, but an additional syntax check failed: %1$s This may be caused by a connectivity or server configuration issue rather than a syntax error in the file.', 'file-manager'), | |
| $originalMessage | |
| ); | |
| throw new PreCommandException($wrappedMessage); | |
| } | |
| throw new PreCommandException($originalMessage); |
| @@ -1,11 +1,11 @@ | |||
| <?php | |||
|
|
|||
| if ( ! defined( 'ABSPATH' ) ) exit; | |||
There was a problem hiding this comment.
The ABSPATH check should come before any code execution, including comments. The opening PHP tag and any subsequent code should immediately be followed by the security check. Move the ABSPATH check to line 2, right after the opening PHP tag.
Pull Request
Description
Plugin check gh action added
Type of Change
Checklist
Changelog