search

TGMPA / TGM-Plugin-Activation

TGM Plugin Activation is a PHP library that allows you to easily require or recommend plugins for your WordPress themes (and plugins). It allows your users to install, update and even automatically activate plugins in singular or bulk fashion using native WordPress classes, functions and interfaces. You can reference bundled plugins, plugins from the WordPress Plugin Repository or even plugins hosted elsewhere on the internet.
http://tgmpluginactivation.com/
GNU General Public License v2.0
1.76k stars 430 forks source link

check_admin_referer fails if plugin activatin is attempted via TGMPA #621

Open caraffa opened 8 years ago

caraffa commented 8 years ago

My onPluginActivation hook looks like this.

    if ( ! current_user_can( 'activate_plugins' ) )
        wp_die(__("Insufficent privileg for the requested operation"));
    $plugin = isset( $_REQUEST['plugin'] ) ? $_REQUEST['plugin'] : '';
    check_admin_referer( "activate-plugin_{$plugin}" );

    // Call child class Activation code
    static::onActivateInstance();

While this code is working flawlessly using Wordpress' usual plugin's admin panel, check_admin_referer() fails if I try to activate my plugin through the TGMPA panel. I've also tried modifing the code like this:

    $plugin = isset( $_REQUEST['plugin'] ) ? $_REQUEST['plugin'] : '';
    if ( isset($_REQUEST['tgmpa-nonce']) ) {
        check_admin_referer( "activate-plugin_{$plugin}", 'tgmpa-nonce' );
    } else {
        check_admin_referer( "activate-plugin_{$plugin}" );
    }

but to no avail. For the moment, the only hack to let my plugins be activated via TGMPA is to bypass check_admin_referer() completely, which I don't think is a good idea. Is there any other way to modify my call to check_admin_referer so that it works?

Thanks

jrfnl commented 8 years ago

IMHO the checking for whether the plugin can be activated, should be done by the code handling the activation, i.e. WP Core or TGMPA and is not something you should do in your own plugin activation routine. Your check should just be that the user has the right privileges to run the actions needed on activation and should be run on the appropriate activation hook - which will only be called if the plugin was successfully activated, i.e. after either WP Core or TGMPA has already checked the nonce for the activation action.

caraffa commented 8 years ago

Well, I don't have the necessary in-depth knowledge of WP_Core to object to your opinion, so I can assume your are right. Nevertheless, googling around about good programming habits while programming plugins, it seems to be an accepted opinion to include some additional sort of security check during the activation/deactivation/uninstallation process. In any, case, even assuming that WP_Core is doing a check_admin_referer, upon activation/deactivation, the additional check in my activation hook succedes. Actually, if you look at the generated HTML source page (the error page generated upon check failure), you can see that the hidden input wpnonce value matches the wpnonce query arg in the referer, whereas the hidden input tgmpa__nonce value doesn't match its corresponding argument in the referrer.

jrfnl commented 8 years ago

whereas the hidden input tgmpa__nonce value doesn't match its corresponding argument in the referrer.

That is curious - will have to have a look at that when I have some time. Thanks for pointing this out.