Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Apr 19, 2016

Conversation

jrfnl
Copy link
Contributor

@jrfnl jrfnl commented Jan 7, 2016

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

screenshot

*/
protected function is_core_update_page() {
// Current screen is not always available, most notably on the customizer screen.
if ( function_exists( 'get_current_screen' ) ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do an inverse check, and return false early.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@jrfnl jrfnl force-pushed the feature/no-notices-on-some-pages branch 2 times, most recently from 9c5843f to 6bf1947 Compare January 7, 2016 21:19
Prevents core/plugin/theme updates screens looking like:

![screenshot](http://snag.gy/mIFC6.jpg)
@jrfnl jrfnl force-pushed the feature/no-notices-on-some-pages branch from 6bf1947 to ddc1275 Compare January 7, 2016 21:24
@jrfnl
Copy link
Contributor Author

jrfnl commented Jan 7, 2016

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
Copy link
Member

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
Copy link
Contributor Author

jrfnl commented Jan 8, 2016

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
Copy link
Member

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

@jrfnl jrfnl added this to the 2.6.0 milestone Jan 9, 2016
@jrfnl
Copy link
Contributor Author

jrfnl commented Jan 9, 2016

@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.

@GaryJones GaryJones merged commit 06a03ad into develop Apr 19, 2016
@GaryJones GaryJones deleted the feature/no-notices-on-some-pages branch April 19, 2016 12:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants