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 429 forks source link

Prevent TGMPA notice showing on WP core update pages. #512

Closed jrfnl closed 8 years ago

jrfnl commented 8 years ago

Prevents core/plugin/theme updates screens looking like this:

screenshot

jrfnl commented 8 years ago

I got push-back from WPCS about not checking the nonce for the $_POST - WordPress.CSRF.NonceVerification.NoNonceVerification. I don't think this opens up any CSRF issues in this case, so have chosen to whitelist, because: a) it's not 'our' nonce to check. b) we're not actually using the value c) if for whatever nefarious reason the action variable is set & not empty, it would only cause the notice not to display, rather lessening any security exposure, rather than heightening it.

I did change the isset() to a ! empty() as an empty action should not be a reason not to display the notice. Let me know if you disagree.

GaryJones commented 8 years ago

It's not our nonce, but if there is a nonce, we should still check it, since we're still wanting to confirm that we're on the plugins / update page, and that an action was posted from within that page. My making an external POST to example.com/wp-admin/plugins.php should not trigger TGMPA into doing anything.

We may not be using the value, but we are using the fact the key exists, to make an assumption.

Whitelisting is the wrong decision here IMO.

jrfnl commented 8 years ago

My making an external POST to example.com/wp-admin/plugins.php should not trigger TGMPA into doing anything.

That's the whole point why I did whitelist. In that case, we don't want to expose TGMPA and therefore shouldn't show the notice.

The alternative would be to check the nonce + rest and if ok, not show the notice and if not ok, also not show the notice, so the end result would be exactly the same, just with more code.

GaryJones commented 8 years ago

Whitelisting doesn't affect exposure. Using the nonce does. The nonce is the right way to go here.

jrfnl commented 8 years ago

@GaryJones Just been looking into this, but there are something like 8 different nonces to check. This is going to be a maintenance nightmare, quite apart from the fact that I stand by my point that it's totally useless to do this as - I repeat-:

The alternative would be to check the nonce + rest and if ok, not show the notice and if not ok, also not show the notice, so the end result would be exactly the same, just with more code.