diff --git a/composer.lock b/composer.lock index fbd26f92d..b08f92ac7 100644 --- a/composer.lock +++ b/composer.lock @@ -1102,7 +1102,7 @@ }, { "name": "plugin-check/phpcs-sniffs", - "version": "dev-1009-require-users-to-write-readmetxt-in-english", + "version": "dev-665-check-asks-users-to-editwrite-to-plugin-use-uploads-folder", "dist": { "type": "path", "url": "./phpcs-sniffs", diff --git a/includes/Checker/Checks/Plugin_Repo/Write_File_Check.php b/includes/Checker/Checks/Plugin_Repo/Write_File_Check.php new file mode 100644 index 000000000..6ceb1420f --- /dev/null +++ b/includes/Checker/Checks/Plugin_Repo/Write_File_Check.php @@ -0,0 +1,88 @@ + 'php', + 'standard' => 'PluginCheck', + 'sniffs' => 'PluginCheck.CodeAnalysis.WriteFile', + ); + } + + /** + * Gets the description for the check. + * + * Every check must have a short description explaining what the check does. + * + * @since n.e.x.t. + * + * @return string Description. + */ + public function get_description(): string { + return __( 'Detects if plugins save data in the plugin folder instead of using the uploads directory or database.', 'plugin-check' ); + } + + /** + * Gets the documentation URL for the check. + * + * Every check must have a URL with further information about the check. + * + * @since n.e.x.t. + * + * @return string The documentation URL. + */ + public function get_documentation_url(): string { + return __( 'https://developer.wordpress.org/plugins/wordpress-org/common-issues/#saving-data-in-the-plugin-folder', 'plugin-check' ); + } +} diff --git a/includes/Checker/Default_Check_Repository.php b/includes/Checker/Default_Check_Repository.php index daa258844..524b863c2 100644 --- a/includes/Checker/Default_Check_Repository.php +++ b/includes/Checker/Default_Check_Repository.php @@ -95,6 +95,7 @@ private function register_default_checks() { 'trademarks' => new Checks\Plugin_Repo\Trademarks_Check(), 'non_blocking_scripts' => new Checks\Performance\Non_Blocking_Scripts_Check(), 'offloading_files' => new Checks\Plugin_Repo\Offloading_Files_Check(), + 'write_file' => new Checks\Plugin_Repo\Write_File_Check(), 'setting_sanitization' => new Checks\Plugin_Repo\Setting_Sanitization_Check(), 'prefixing' => new Checks\Plugin_Repo\Prefixing_Check(), 'direct_db' => new Checks\Security\Direct_DB_Check(), diff --git a/phpcs-sniffs/PluginCheck/Sniffs/CodeAnalysis/WriteFileSniff.php b/phpcs-sniffs/PluginCheck/Sniffs/CodeAnalysis/WriteFileSniff.php new file mode 100644 index 000000000..d818372de --- /dev/null +++ b/phpcs-sniffs/PluginCheck/Sniffs/CodeAnalysis/WriteFileSniff.php @@ -0,0 +1,200 @@ + Key is function name, value irrelevant. + */ + protected $target_functions = array( + // File write functions - check first parameter (file path). + 'fwrite' => true, + 'fputs' => true, + 'file_put_contents' => true, + 'touch' => true, + // File copy/move functions - check second parameter (destination path). + 'copy' => true, + 'rename' => true, + 'copy_dir' => true, + 'move_dir' => true, + 'unzip_file' => true, + ); + + /** + * Parameter positions for each function. + * + * @var array + */ + private $param_positions = array( + 'fwrite' => 1, + 'fputs' => 1, + 'file_put_contents' => 1, + 'touch' => 1, + 'copy' => 2, + 'rename' => 2, + 'copy_dir' => 2, + 'move_dir' => 2, + 'unzip_file' => 2, + ); + + /** + * WordPress constants that indicate plugin directory usage. + * + * @var array + */ + private $plugin_constants = array( + 'WP_PLUGIN_DIR', + 'WP_PLUGIN_URL', + 'PLUGINDIR', + 'WPINC', + 'WP_CONTENT_DIR', + 'WP_CONTENT_URL', + ); + + /** + * WordPress functions that indicate plugin directory usage. + * + * @var array + */ + private $plugin_functions = array( + 'plugins_url', + 'plugin_dir_path', + 'plugin_dir_url', + ); + + /** + * WordPress functions that indicate safe directory usage (uploads, temp). + * + * @var array + */ + private $safe_functions = array( + 'wp_upload_dir', + 'wp_tempnam', + 'get_temp_dir', + ); + + /** + * Process the parameters of a matched function. + * + * @since 1.1.0 + * + * @param int $stackPtr The position of the current token in the stack. + * @param string $group_name The name of the group which was matched. + * @param string $matched_content The token content (function name) which was matched in lowercase. + * @param array $parameters Array with information about the parameters. + * + * @return void + */ + public function process_parameters( $stackPtr, $group_name, $matched_content, $parameters ) { + $param_position = isset( $this->param_positions[ $matched_content ] ) ? $this->param_positions[ $matched_content ] : 1; + $path_param = PassedParameters::getParameterFromStack( $parameters, $param_position, array() ); + + if ( false === $path_param ) { + return; + } + + // Get the content of the parameter. + $path_content = ''; + for ( $i = $path_param['start']; $i <= $path_param['end']; $i++ ) { + if ( isset( Tokens::$emptyTokens[ $this->tokens[ $i ]['code'] ] ) ) { + continue; + } + $path_content .= $this->tokens[ $i ]['content']; + } + + // Check if the path uses safe functions (uploads directory or temp). + foreach ( $this->safe_functions as $safe_func ) { + if ( stripos( $path_content, $safe_func ) !== false ) { + // Safe function detected, no error needed. + return; + } + } + + // Check for plugin constants. + foreach ( $this->plugin_constants as $constant ) { + if ( stripos( $path_content, $constant ) !== false ) { + $this->add_error( $stackPtr, $matched_content, $path_param['start'], 'constant ' . $constant ); + return; + } + } + + // Check for plugin functions. + foreach ( $this->plugin_functions as $func ) { + if ( stripos( $path_content, $func ) !== false ) { + $this->add_error( $stackPtr, $matched_content, $path_param['start'], 'function ' . $func . '()' ); + return; + } + } + + // Check for __FILE__ or __DIR__ magic constants. + if ( stripos( $path_content, '__FILE__' ) !== false || stripos( $path_content, '__DIR__' ) !== false ) { + $this->add_error( $stackPtr, $matched_content, $path_param['start'], '__FILE__ or __DIR__ magic constant' ); + return; + } + + // Check for ABSPATH usage (could be writing to WordPress root or plugin folder). + if ( stripos( $path_content, 'ABSPATH' ) !== false ) { + $this->phpcsFile->addWarning( + 'Writing files using ABSPATH may be problematic. Consider using wp_upload_dir() instead if storing user data or generated files.', + $path_param['start'], + 'ABSPATHDetected' + ); + return; + } + } + + /** + * Adds an error message for plugin directory write attempt. + * + * @param int $stackPtr The position of the function call. + * @param string $function_name The name of the function being called. + * @param int $error_ptr The position to report the error. + * @param string $indicator What indicated this is a plugin path. + * + * @return void + */ + private function add_error( $stackPtr, $function_name, $error_ptr, $indicator ) { + $this->phpcsFile->addError( + 'Plugin folders are deleted when upgraded. Do not save data to the plugin folder using %s(). Detected usage of %s. Use wp_upload_dir() to get the uploads directory path or save to the database instead.', + $error_ptr, + 'PluginDirectoryWrite', + array( $function_name, $indicator ) + ); + } +} diff --git a/phpcs-sniffs/PluginCheck/Tests/CodeAnalysis/WriteFileUnitTest.inc b/phpcs-sniffs/PluginCheck/Tests/CodeAnalysis/WriteFileUnitTest.inc new file mode 100644 index 000000000..1d024aa56 --- /dev/null +++ b/phpcs-sniffs/PluginCheck/Tests/CodeAnalysis/WriteFileUnitTest.inc @@ -0,0 +1,33 @@ + => + */ + public function getErrorList() { + return array( + 5 => 1, // file_put_contents with __FILE__. + 8 => 1, // fwrite with __DIR__. + 11 => 1, // fputs with plugin_dir_path(). + 14 => 1, // copy with WP_PLUGIN_DIR. + 17 => 1, // rename with plugin_dir_path(). + 20 => 1, // touch with WP_CONTENT_DIR. + 23 => 1, // file_put_contents with plugins_url(). + ); + } + + /** + * Returns the lines where warnings should occur. + * + * @return array => + */ + public function getWarningList() { + return array(); + } + + /** + * Returns the fully qualified class name (FQCN) of the sniff. + * + * @return string The fully qualified class name of the sniff. + */ + protected function get_sniff_fqcn() { + return WriteFileSniff::class; + } + + /** + * Sets the parameters for the sniff. + * + * @throws \RuntimeException If unable to set the ruleset parameters required for the test. + * + * @param Sniff $sniff The sniff being tested. + */ + public function set_sniff_parameters( Sniff $sniff ) { + } +} + diff --git a/phpcs-sniffs/PluginCheck/ruleset.xml b/phpcs-sniffs/PluginCheck/ruleset.xml index 7b6cc0ee3..acdebd754 100644 --- a/phpcs-sniffs/PluginCheck/ruleset.xml +++ b/phpcs-sniffs/PluginCheck/ruleset.xml @@ -10,6 +10,7 @@ + diff --git a/tests/phpunit/testdata/plugins/test-plugin-write-file-with-errors/file-operations.php b/tests/phpunit/testdata/plugins/test-plugin-write-file-with-errors/file-operations.php new file mode 100644 index 000000000..1b6cf2a86 --- /dev/null +++ b/tests/phpunit/testdata/plugins/test-plugin-write-file-with-errors/file-operations.php @@ -0,0 +1,24 @@ + 'value' ) ); +} + diff --git a/tests/phpunit/tests/Checker/Checks/Write_File_Check_Tests.php b/tests/phpunit/tests/Checker/Checks/Write_File_Check_Tests.php new file mode 100644 index 000000000..53db3093c --- /dev/null +++ b/tests/phpunit/tests/Checker/Checks/Write_File_Check_Tests.php @@ -0,0 +1,69 @@ +run( $check_result ); + + $errors = $check_result->get_errors(); + + // Should have errors for plugin directory writes. + $this->assertNotEmpty( $errors ); + $this->assertArrayHasKey( 'file-operations.php', $errors ); + + // Check for specific error codes. + $error_codes = array(); + foreach ( $errors['file-operations.php'] as $line => $columns ) { + foreach ( $columns as $column => $messages ) { + foreach ( $messages as $message ) { + $error_codes[] = $message['code']; + } + } + } + + // Should detect PluginDirectoryWrite errors. + $this->assertContains( 'PluginCheck.CodeAnalysis.WriteFile.PluginDirectoryWrite', $error_codes ); + + // Should have at least 6 errors (one for each bad example). + $this->assertGreaterThanOrEqual( 6, $check_result->get_error_count() ); + } + + public function test_run_without_errors() { + $write_file_check = new Write_File_Check(); + $check_context = new Check_Context( UNIT_TESTS_PLUGIN_DIR . 'test-plugin-write-file-without-errors/load.php' ); + $check_result = new Check_Result( $check_context ); + + $write_file_check->run( $check_result ); + + $errors = $check_result->get_errors(); + + // Should have no errors when using wp_upload_dir() or temp directories. + $this->assertEmpty( $errors ); + $this->assertSame( 0, $check_result->get_error_count() ); + } + + public function test_get_description() { + $check = new Write_File_Check(); + $this->assertNotEmpty( $check->get_description() ); + } + + public function test_get_documentation_url() { + $check = new Write_File_Check(); + $url = $check->get_documentation_url(); + $this->assertNotEmpty( $url ); + $this->assertStringContainsString( 'developer.wordpress.org', $url ); + } +}