TukuToi / tukutoi-maintenance

Enable and Control a Custom Maintenance Mode for your WordPress Website.
https://www.tukutoi.com/program/tukutoi-maintenance/
GNU General Public License v2.0
0 stars 1 forks source link

class-tkt-maintenance-admin #21

Open joyously opened 3 years ago

joyously commented 3 years ago

I think the settings should be one array instead of individual fields. But what is going on with $this->plugin_short . '_active', and $this->plugin_short . '_dequeue_styles_scripts', using validate_number as a sanitization callback? They look like booleans. https://developer.wordpress.org/reference/functions/wp_validate_boolean/ Also, for non-negative integers: https://developer.wordpress.org/reference/functions/absint/

https://github.com/TukuToi/tkt-maintenance/blob/18c552f0089296be71e39d1c9521a61df0f6d6e1/admin/class-tkt-maintenance-admin.php#L224-L227

In the template, there are no script tags output, so the user would have to put them in here, but then they would be escaped. The CSS field doesn't have that problem, but the esc_html function is probably not the right one for sanitizing.

https://github.com/TukuToi/tkt-maintenance/blob/18c552f0089296be71e39d1c9521a61df0f6d6e1/admin/class-tkt-maintenance-admin.php#L507

This should be current_user_can( 'manage_options' ) and/or 'unfiltered_upload' or 'manage_network_options'.

smileBeda commented 3 years ago

Those are not booleans, those are checkbox 1 or 0. It can't be treated as a boolean, neither as a 1 or 0 numeric because 0 will mean false in PHP if checking on boolean.

Thus, I created this custom function that does exactly what I need (the options API supports a custom validation callback).

Is this wrong? I wanted to avoid creating 2 different validation methods.


The template must feature script tags, this is a bug of mine, I am fixing that in the template. esc_html is the only one I could find that would remotely escape something like CSS or JS even if it is for HTML. Is there something one can use to actually stereo JS or CSS safely in the database? I could not find a thing in the Codex. Online reading, and studying existing plugins in repo I can see either no sanitisation used at all (bad) or esc_html Thus, that is what I used, but also feel awkward with it, because it is for HTML, not CSS or JS. @joyously - Do you now what we can use here?


I disagree with This should be current_user_can( 'manage_options' ) and/or 'unfiltered_upload' or 'manage_network_options'. We can set manage options or network options to any user using a plugin or code to any user role. And they would immediately get access to this. Also in 8 years I have not once heard this is not allowed, however, you are right! DOC states

While checking against particular roles in place of a capability is supported in part, this practice is discouraged as it may produce unreliable results.

Would be good to put there why it actually is discouraged, but, anyway. Corrected this to current_user_can( 'manage_options') || current_user_can( 'manage_network_options'). Should good now.

joyously commented 3 years ago

Those are not booleans, those are checkbox 1 or 0.

Uh, last I checked, that is a boolean.

Thus, I created this custom function

I pointed you to the wp_validate_boolean function. I use it in my theme for all my checkboxes.

Is there something one can use to actually stereo JS or CSS safely in the database?

"stereo"? I think I mentioned in another issue that core uses strip_tags() on the Additional CSS option.

We can set manage options or network options to any user using a plugin or code to any user role. And they would immediately get access to this.

Yes! So you could give that one capability to a lesser(or custom) role. Since you can also remove capabilities from an admin user, it's best to always check the capability instead of the role.

smileBeda commented 3 years ago

Keeping this open for the checkbox.

The problem I encountered with validating like that is that the check box option does not store booleans. It stores 0 or 1 as numeric string. At least that is what I recall from when I initially created my own checkbox option boilerplate.

I need to revise this as maybe I either did something wrong or looked wrong, and use your suggested approach.

The rest here became obsolete as JS and CSS are gone.

PS: stereo is a typo, should have been store