Unsanitized options results in 400-ish errors and mostly inaccessible admin settings

[Varstahl]

Unsanitized options results in 400-ish errors and mostly inaccessible admin settings

[dskrepps]

Was checking this out today and was immediately hit by a screen fulls of errors. After managing to get to settings they went away, so probably the same bug.

[Varstahl]

Looks like it, yes.

[covertnine]

I pushed an update today that I believe addresses this, but I’m having a hard time getting it to error out

I have an issue with the admin bar setting still that I’m going to work out this weekend.

Take a look

f9a5f03

[Varstahl]

From my recollection of things, this issue appears only on new installations and that commit doesn't fix it. The default return value for get_option() is false, so the line would still be evaluated as isset(<bool>['suppress_update_notice']). I would honestly mirror get_option functionality in a class function like so:

function c9_get_option($name) {
    $opt = get_option($this->plugin_name);
    return (((false !== $opt) && array_key_exists($name, $opt)) ? $opt[$name] : false);
}

At that point you could disregard most of the conditionals within your code, and instances like

c9-admin/admin/class-c9-admin.php

Line 57 in f9a5f03

if ((isset(get_option($this->plugin_name)['suppress_update_notice'])) && (true == get_option($this->plugin_name)['suppress_update_notice'])) {

would just become

if (c9_get_option('suppress_update_notice'))

Also, since mirroring the get_option behaviour, this works even if the return value is not simply true but a complex dataset.

Note: I wrote the code off the top of my head, and while it should work some test might be in order :D

[covertnine]

I’ll be honest this is definitely beyond my skillset. I’ll have to do some testing with it tomorrow, but I’ll see what I can do.

…

-T

On Nov 21, 2020, at 6:46 PM, Bruno Passeri ***@***.***> wrote:  From my recollection of things, this issue appears only on new installations and that commit doesn't fix it. The default return value for get_option() is false, so the line would still be evaluated as isset(<bool>['suppress_update_notice']). I would honestly mirror get_option functionality in a class function like so: function c9_get_option($name) { $opt = get_option($this->plugin_name); return (((false !== $opt) && array_key_exists($name, $opt)) ? $opt[$name] : false); } At that point you could disregard most of the conditionals within your code, and instances like https://github.com/covertnine/c9-admin/blob/f9a5f034ad832328c3f2c71e7d270265c125fd1a/admin/class-c9-admin.php#L57 would just become if (c9_get_option('suppress_update_notice')) Also, since mirroring the get_option behaviour, this works even if the return value is not simply true but a complex dataset. Note: I wrote the code off the top of my head, and while it should work some test might be in order :D — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

[dskrepps]

Both these solutions fix the errors except for the first, line 41.

If you could change that line in your pull request to the following all errors are gone on my end.

        if (!$this->c9_option('custom_skin')) {

But now I notice the settings page has more of the same errors in c9-admin-display.php.

[Varstahl]

oops, that line eluded me, thanks for reporting.

[covertnine]

Merged these and tested this morning. New version pushed to Wordpress.org.

…

-T

On Nov 22, 2020, at 7:06 PM, Dylan Krepps ***@***.***> wrote:  Both these solutions fix the errors except for the first, line 41. If you could change that line in your pull request to the following all errors are gone on my end. if (!$this->c9_option('custom_skin')) { But now I notice the settings page has more of the same errors in c9-admin-display.php. — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.