mondula / ticket-hub

1 stars 0 forks source link

WP_Feedback_Variables and options must be escaped when echo'd #16

Closed JonathanPultz closed 2 weeks ago

JonathanPultz commented 3 weeks ago

Much related to sanitizing everything, all variables that are echoed need to be escaped when they're echoed, so it can't hijack users or (worse) admin screens. There are many esc_*() functions you can use to make sure you don't show people the wrong data, as well as some that will allow you to echo HTML safely.

At this time, we ask you escape all $-variables, options, and any sort of generated data when it is being echoed. That means you should not be escaping when you build a variable, but when you output it at the end. We call this 'escaping late.'

Besides protecting yourself from a possible XSS vulnerability, escaping late makes sure that you're keeping the future you safe. While today your code may be only outputted hardcoded content, that may not be true in the future. By taking the time to properly escape when you echo, you prevent a mistake in the future from becoming a critical security issue.

This remains true of options you've saved to the database. Even if you've properly sanitized when you saved, the tools for sanitizing and escaping aren't interchangeable. Sanitizing makes sure it's safe for processing and storing in the database. Escaping makes it safe to output.

Also keep in mind that sometimes a function is echoing when it should really be returning content instead. This is a common mistake when it comes to returning JSON encoded content. Very rarely is that actually something you should be echoing at all. Echoing is because it needs to be on the screen, read by a human. Returning (which is what you would do with an API) can be json encoded, though remember to sanitize when you save to that json object!

There are a number of options to secure all types of content (html, email, etc). Yes, even HTML needs to be properly escaped.

https://developer.wordpress.org/apis/security/escaping/

Remember: You must use the most appropriate functions for the context. There is pretty much an option for everything you could echo. Even echoing HTML safely.

Example(s) from your plugin:

ticket-hub/shortcodes/th-form-sc.php:37

" . esc_html($field['label']) . "

"; ticket-hub/post-types/th-ticket-pt.php:148 echo "

"; ticket-hub/shortcodes/th-faqs-sc.php:33 echo apply_filters('the_content', wp_kses_post(get_post_meta(get_the_ID(), '_th_answer', true))); ticket-hub/shortcodes/th-changelog-sc.php:35 echo apply_filters('the_content', wp_kses_post(get_post_meta(get_the_ID(), '_th_log', true))); ticket-hub/shortcodes/th-tickets-sc.php:184 echo $final_output;

↳ Detected origin: json_encode(array('tickets' => $output, 'pagination' => $pagination_html))

↳ Remember to ALWAYS escape as LATE as possible as with a PROPER function for the context.

... out of a total of 11 incidences.

Note: when you need to echo a JSON, it's better to make use of the function wp_json_encode, also, make sure you are not avoiding escaping with the options passed on the second parameter. echo wp_json_encode($array_or_object); Example(s) from your plugin:

ticket-hub/shortcodes/th-tickets-sc.php:181 $final_output = json_encode(array('tickets' => $output, 'pagination' => $pagination_html));

Note: The functions _e and _ex outputs the translation without escaping, please use an alternative function that escapes the output. An alternative to _e would be esc_html_e, esc_attr_e or simply using __ wrapped by a escaping function and inside an echo. An alternative to _ex would be using _x wrapped by a escaping function and inside an echo. Examples:

Example(s) from your plugin:

ticket-hub/shortcodes/th-ticket-sc.php:58 <?php _e('Back', 'tickethub') ?> -----> _e('Back', 'tickethub')

JonathanPultz commented 2 weeks ago

Alle Vorkommen dieses Problems sollten (glaube ich) durch die Überprüfung des Plugin-Check Plugins abgedeckt werden. Siehe: Issue #20

JonathanPultz commented 2 weeks ago

Alle Vorkommen (bis auf eins, welche mit TODO markiert) sind done via #20