search

wp-shortcake / shortcake

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

Rotten Denmark: Odd issue with ACF fields on Setting pages #722

Closed zedejose closed 7 years ago

zedejose commented 7 years ago

First of all, I suspect that this might be a way too specific use-case, in which case feel free to tell me to bugger off. Also, this is more of a doubt/request for clarification than a bug report.

So, the situation:

I went to take a look at the render_shortcode_for_preview function in the Shortcode_UI class, and here's what I'm not sure I understand:

It starts by testing if the user can edit posts, i.e. if ( ! current_user_can( 'edit_post', $post_id ) ) {} , which, in a settings page will return false, since $post_idis not set. The function then returns the stench of Denmark, and exits.

Stranger to me is that immediately after, the function seems to be ok with $post_idnot being set, and skips the whole post getting bit, before going on as normal, and rendering the module anyway.

My question is whether that first test (user perms) shouldn't add a && ! empty ( $post_id ) or maybe even be inside the second one (when checking for an empty $post_id).

The above works, but again I am not 100% aware of the larger picture in the remaining code, and am hence trying to thread very very lightly :)

szepeviktor commented 7 years ago

Could you clarify what a settings page is in WP terms?

zedejose commented 7 years ago

Sorry, I meant Options not Settings: It's a subpage of the Options menu (i.e. options-general.php)

zedejose commented 7 years ago

Come to think of it, this might not be ACF Pro specific. It could well happen with any custom Options subpage (added via CMB2 or similar)

szepeviktor commented 7 years ago

The problem cloud be Options page ➕ TinyMCE editor.

goldenapples commented 7 years ago

This is definitely an issue. I've noted it before in #599.

I wouldn't want to do something as simple as just allowing the preview to render if there's no post id specified, for fear of opening up all kinds of security vulnerabilities. We can't get in the habit of thinking shortcodes are safe to run, because there are always going to be people using shortcodes like Inline PHP, and exposing the ability to run arbitrary PHP code by unauthorized users would be a huge problem.

I think any solution we come up with would have to take into account several different common scenarios where shortcode previews would be used:

Probably a better option would be to require the user to declare the capability check required for each editor that Shortcake would be enabled for. Then we could send a capability check along with the js request to preview the shortcode, and check it when doing bulk preview....

szepeviktor commented 7 years ago

I usually use a separate CPT for small content chunks. Actually the whole website is built of these blokks https://github.com/blokk-people/blokk

szepeviktor commented 7 years ago

Duplicate of #599

goldenapples commented 7 years ago

I usually use a separate CPT for small content chunks.

Yeah, this is a good way of getting around the current shortcomings of this plugin. It'd be nice to figure out a way of fixing those shortcomings so that the plugin works well with other admin pages with TinyMCE editors.

szepeviktor commented 7 years ago

The future is editor content blocks: https://make.wordpress.org/core/2017/01/17/editor-technical-overview/ https://make.wordpress.org/design/2017/02/16/core-editor-meeting-notes-2017-02-15/

zedejose commented 7 years ago

I usually use a separate CPT for small content chunks.

It's an approach, sure. Not on this project, though.

The future is editor content blocks

I am sure it is. Sadly my issue is in the present.

I'll work something out, thanks for the help.

szepeviktor commented 7 years ago

I am sure it is. Sadly my issue is in the present.

I'll work something out, thanks for the help.

The quickest solution seems to be to start using https://github.com/blokk-people/blokk and change TinyMCE field to a "post select" field which is limited to blokks.

carasmo commented 6 years ago

Any updates on this? Thanks!