WordPress / plugin-check

A repository for the new Plugin Check plugin from the WordPress Performance and Plugins Team.
https://wordpress.org/plugins/plugin-check/
GNU General Public License v2.0
266 stars 53 forks source link

Warning about "Processing form data without nonce verification" (uninstall.php) #803

Closed joho1968 closed 2 days ago

joho1968 commented 2 days ago

For the file uninstall.php, I'm seeing this:

WARNING WordPress.Security.NonceVerification.Recommended Processing form data without nonce verification.

But if I read this correctly: https://core.trac.wordpress.org/ticket/38661, nonces need not be checked in uninstall.php since core has already done that ... 🤔

The actual code "processing" form data (in uninstall.php) is this:

// If action is not to uninstall, then exit
if ( empty( $_REQUEST['action'] ) || $_REQUEST['action'] !== 'delete-plugin' ) {
    if ( defined( 'MYPLUGIN_UNINSTALL_TRACE' ) ) {
        error_log( 'myplugin-uninstall: REQUEST["action"] is not delete-plugin' );
    }
    exit;
}
// If it's not us, then exit
if ( empty( $_REQUEST['slug'] ) || $_REQUEST['slug'] !== 'myslug' ) {
    if ( defined( 'MYPLUGIN_UNINSTALL_TRACE' ) ) {
        error_log( 'myplugin-uninstall: REQUEST["slug"] is not myslug' );
    }
    exit;
}
swissspidy commented 2 days ago

That‘s not really how you should check whether you‘re in the install process. You should check whether the uninstall constant is defined. Your code breaks if the plugin is uninstalled via WP-CLI for example.

joho1968 commented 2 days ago

So I guess I shouldn't be doing this then either?

if ( ! current_user_can( 'manage_options' ) || ! current_user_can( 'delete_plugins' ) ) {
    if ( defined( 'MYPLUGIN_UNINSTALL_TRACE' ) ) {
        error_log( 'myplugin-uninstall: User is not allowed to manage/uninstall plugins' );
    }
    exit;
}
swissspidy commented 2 days ago

Correct. Here‘s how you do it: https://developer.wordpress.org/plugins/plugin-basics/uninstall-methods/