wp-shortcake / shortcake

Shortcake makes using WordPress shortcodes a piece of cake.
GNU General Public License v2.0
664 stars 142 forks source link

Add a nonce to the bulk shortcode ajax handler #743

Closed johnpbloch closed 7 years ago

johnpbloch commented 7 years ago

A 10up-Third-Party PHPCS run pointed out that Shortcode_UI::handle_ajax_bulk_do_shortcode() was processing user data without a nonce check. This adds the preview nonce already in the code to the request and checks it before processing data.

I also noticed that each shortcode also sends a nonce to the ajax handler along with its data, although that nonce is never checked. I'm not really familiar with what side effects we'd see by either removing the nonce or checking it on a shortcode-by-shortcode basis. Let me know if I should go ahead and remove those extra nonce fields or rework where the nonces are checked.

szepeviktor commented 7 years ago

Thank you.

goldenapples commented 7 years ago

I'm not really familiar with what side effects we'd see by either removing the nonce or checking it on a shortcode-by-shortcode basis. Let me know if I should go ahead and remove those extra nonce fields or rework where the nonces are checked.

Oh, now that the nonce is being checked in the bulk_do_shortcode ajax handler, it's completely unnecessary in each of the individual shortcodes and can be removed.

For a while after introducing the bulk endpoint, we were trying to preserve endpoints to preview individual shortcodes as well. But these aren't needed any more, so there's no need to be passing these unneeded nonces.

goldenapples commented 7 years ago

I'll merge this as-is and follow it up with a PR that removes the redundant nonces in the preview request payload.