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

class-tkt-maintenance-admin #21

Open
joyously opened this issue Jun 20, 2021 · 3 comments
Open

class-tkt-maintenance-admin #21

joyously opened this issue Jun 20, 2021 · 3 comments
Assignees

Comments

@joyously
Copy link

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

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.

smileBeda added a commit that referenced this issue Jun 21, 2021
- Instead of checking on role, use manage_options or manage_network_options
smileBeda added a commit that referenced this issue Jun 21, 2021
@joyously
Copy link
Author

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

smileBeda commented Jun 22, 2021

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

@smileBeda smileBeda self-assigned this Jun 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants