devinsays / options-framework-plugin

An Options Panel Framework to help speed theme development.
https://wptheming.com
833 stars 286 forks source link

Incorrectly sanitizing the "desc" field #191

Closed RadGH closed 10 years ago

RadGH commented 10 years ago

A problem I've had in the past which I've since worked around was brought up by a comment on the plugin at wordpress.org.

The issue: The desc field is overzealously sanitized using a strict wp_kses allowedtags array.

After some research I found the issue was already discovered and resolved for textareas. However, the issue still exists for the "desc" field.

The most important issue is that you cannot have a link in the "desc" and with the attribute "target=blank". This means your links will leave the page, and lose any changes.

The issue exists in the file "class-options-interface.php". Specifically, you need to remove validation of three occurrences of: wp_kses( $explain_value, $allowedtags)

Why is this field even sanitized? It's not user input. It's source code.

Here is the code I am using to work around the issue. This is a hack that replaces $allowedtags with $allowedposttags while Options Framework is displaying the fields. It may have unwanted side effects, but so far has been working for me.

A better solution is to either make the three wp_kses references mentioned above filterable, or remove them entirely. In any case, $allowedposttags is a better choice for the default.

function rad_of_add_better_kses() {
  add_action('optionsframework_after', 'rad_of_remove_better_kses'); // Reset our changes when OF is done

  // Hold on to $allowedtags in another variable, temporarily replacing the original value while Options Framework is displaying the interface
  global $allowedtags, $allowedposttags, $rad_temp_allowedtags;
  $rad_temp_allowedtags = $allowedtags;
  $allowedtags = $allowedposttags;
}
function rad_of_remove_better_kses() {
  // Options framework is done displaying the interface, reset $allowedtags as to not interfere with other plugins.
  global $allowedtags, $rad_temp_allowedtags;
  $allowedtags = $rad_temp_allowedtags;
}
add_action('appearance_page_options-framework', 'rad_of_add_better_kses');
devinsays commented 10 years ago

I've always errored on the side of rigorous sanitization- but agree it's a bit over-zealous here and one of the items I get questions about the most.

I just removed the sanitization for the info option and the "desc" field with this commit (https://github.com/devinsays/options-framework-plugin/commit/03a1a94cc2e00f9064a5511744ef09195bed39e5). Feel free to test and let me know if you notice any issues: