Open alantygel opened 4 years ago
cornelRaiu
commented
4 years ago Hi @alantygel , these are some of the things I am working on regarding the template.
I am developing a plugin based on this template now and preparing a PR soon with some of the changes required to update the code base to the latest WP Standards.
carl-alberto
commented
4 years ago Cool, I also have pushed some updates to address these security concerns, would love to help out If you don't an open PR yet, I can push some of my updated codes to address them if you want @cornelRaiu @alantygel ? :)
Hi,
First of all, thank you very much for developing this template. And sorry for not being able to help as much as I could.
Recently I've submit a (very simple) plugin [1] to Wordpress repo based on this template. Reviewers noted some issues related to the template, which I transcribe here. Maybe we should later on open one issue for each.
I certainly can help on solving these issues, I just want to be sure it is from interest of the developers.
Data Must be Sanitized, Escaped, and Validated
When you include POST/GET/REQUEST/FILE calls in your plugin, it's important to sanitize, validate, and escape them. The goal here is to prevent a user from accidentally sending trash data through the system, as well as protecting them from potential security issues.
SANITIZE: Data that is input (either by a user or automatically) must be sanitized as soon as possible. This lessens the possibility of XSS vulnerabilities and MITM attacks where posted data is subverted.
VALIDATE: All data should be validated, no matter what. Even when you sanitize, remember that you don’t want someone putting in ‘dog’ when the only valid values are numbers.
ESCAPE: Data that is output must be escaped properly when it is echo'd, so it can't hijack admin screens. There are many esc_*() functions you can use to make sure you don't show people the wrong data.
To help you with this, WordPress comes with a number of sanitization and escaping functions. You can read about those here:
Remember: You must use the most appropriate functions for the context. If you’re sanitizing email, use sanitize_email(), if you’re outputting HTML, use esc_html(), and so on.
An easy mantra here is this:
Sanitize early Escape Late Always Validate
Clean everything, check everything, escape everything, and never trust the users to always have input sane data. After all, users come from all walks of life.
Example(s) from your plugin:
limit-excerpt/includes/class-limit-excerpt-settings.php:233: $current_section = $_GET['tab']; limit-excerpt/includes/class-limit-excerpt-settings.php:305: $tab .= $_GET['tab'];
Don’t use esc_ functions to sanitize
When sanitizing data, it’s important to use sanitization functions, not escape functions. The two work together, but are not interchangeable.
Functions like esc_attr() do NOT sanitize anything, and should never be used for that purpose.
The sole exception to this is URLs, which can use esc_url() or esc_url_raw() when being saved.
Please review this document for help finding the most appropriate sanitization functions: https://developer.wordpress.org/plugins/security/securing-input/
Example(s) from your plugin:
limit-excerpt/includes/lib/class-limit-excerpt-admin-api.php:349: update_post_meta( $post_id, $field['id'], $this->validate_field( $_REQUEST[ $field['id'] ], $field['type'] ) ); //phpcs:ignore
Looking at the function it incorrectly uses esc_ functions:
public function validate_field( $data = '', $type = 'text' ) {
switch ( $type ) { case 'text': $data = esc_attr( $data ); break; case 'url': $data = esc_url( $data ); break; case 'email': $data = is_email( $data ); break; }
return $data; }
[1] gitlab.com/eita/limit-excerpt