Closed MichelleBlanchette closed 9 months ago
Well, shoot. I deliberately set the Stable Version to the currently stable version because I didn't want them to accidentally launch the updated version of Completionist before I'm ready. Alas, though, it should be fine.
Also, this seems like mostly PHPCS errors, so I should just make sure those are COMPLETELY clean (and that all ignore comments are removed) to better ensure everything is properly handled. The Plugin Check plugin should also help with ensuring I've addressed everything, as well.
This review made me want to prioritize three massive refactors that have been on my mind:
Options
class or at least improving/simplifying it to do things more The WordPress Way™
These refactors are so large and prone to regression issues, though, that I've always put it off in favor of new feature development. These components of the plugin are major as they affect every feature. Alas... it must be done, and we'll definitely be better positioned at the end of it all! Also, this is definitely the best time to do such a thorough refactor/upgrade as the literal Plugin Review Team is taking great care to thoroughly review the codebase during this time. We should definitely take full advantage of it!
Now where to find the time... 😁
Hello,
There are issues with your plugin code preventing it from being approved immediately. We have pended your submission in order to help you correct all issues so that it may be approved and published.
We ask you read this email in its entirety, address all listed issues, and reply to this email with your corrected code attached (or linked). You have three (3) months to make all corrections, before your plugin will be rejected. Even so, as long as you reply to this email, we will be able to continue with your review and eventually publish your code.
Remember in addition to code quality, security and functionality, we require all plugins adhere to our guidelines. If you have not yet, please read them:
https://developer.wordpress.org/plugins/wordpress-org/detailed-plugin-guidelines/
We know it can be long, but you must follow the directions at the end as not doing so will result in your review being delayed. It is required for you to read and reply to these emails, and failure to do so will result in significant delays with your plugin being accepted.
Finally, should you at any time wish to alter your permalink (aka the plugin slug), you must explicitly tell us what you want it to be. Just changing the display name is not sufficient, and we require to you clearly state your desired permalink. Remember, permalinks cannot be altered after approval.
Be aware that you will not be able to submit another plugin while this one is being reviewed.
Incorrect Stable Tag
In your readme, your 'Stable Tag' does not match the Plugin Version as indicated in your main plugin file.
ERROR: Readme Stable tag and header versions are different 3.10.1 != 4.0.0 readme.txt - Stable tag: 3.10.1 completionist.php - Version: 4.0.0
Your Stable Tag is meant to be the stable version of your plugin, not of WordPress. For your plugin to be properly downloaded from WordPress.org, those values need to be the same. If they're out of sync, your users won't get the right version of your code.
We recommend you use Semantic Versioning (aka SemVer) for managing versions:
https://en.wikipedia.org/wiki/Software_versioning https://semver.org/
Please note: While currently using the stable tag of trunk currently works in the Plugin Directory, it's not actually a supported or recommended method to indicate new versions and has been known to cause issues with automatic updates.
We ask you please properly use tags and increment them when you release new versions of your plugin, just like you update the plugin version in the main file. Having them match is the best way to be fully forward supporting.
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:
https://developer.wordpress.org/apis/security/sanitizing/ https://developer.wordpress.org/apis/security/escaping/
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 wp_kses_post(), 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:
completionist/src/admin/ajax/ajax-update-task.php:27 && wp_verify_nonce( $_POST['nonce'], 'ptc_completionist' ) !== FALSE//phpcs:ignore WordPress.Security.ValidatedSanitizedInput -----> wp_verify_nonce($_POST['nonce'], 'ptc_completionist') completionist/src/admin/script-save-settings.php:115 $submitted_ttl = (int) Options::sanitize( Options::CACHE_TTL_SECONDS, $_POST['asana_cache_ttl'] ); completionist/src/admin/ajax/ajax-pin-task.php:45 $the_post_id = (int) Options::sanitize( 'gid', $_POST['post_id'] );//phpcs:ignore WordPress.Security.ValidatedSanitizedInput -----> Options::sanitize('gid', $_POST['post_id']) completionist/src/admin/ajax/ajax-list-tasks.php:31 $task_gids = json_decode( stripslashes( $_POST['task_gids'] ), FALSE, 2, JSON_BIGINT_AS_STRING ); -----> stripslashes($_POST['task_gids']) completionist/src/admin/ajax/ajax-list-tasks.php:27 && wp_verify_nonce( $_POST['nonce'], 'ptc_completionist_list_task' ) !== FALSE//phpcs:ignore WordPress.Security.ValidatedSanitizedInput -----> wp_verify_nonce($_POST['nonce'], 'ptc_completionist_list_task') completionist/src/admin/ajax/ajax-update-task.php:64 $due_on = Options::sanitize( 'date', $_POST['due_on'] );//phpcs:ignore WordPress.Security.ValidatedSanitizedInput -----> Options::sanitize('date', $_POST['due_on']) completionist/src/admin/script-save-settings.php:111 && wp_verify_nonce( $_POST['asana_cache_ttl_save_nonce'], 'asana_cache_ttl_save' ) !== false completionist/src/admin/ajax/ajax-list-task.php:43 $detailed_view = filter_var( wp_unslash( $_POST['detailed'] ), FILTER_VALIDATE_BOOLEAN ); -----> wp_unslash($_POST['detailed']) completionist/src/admin/ajax/ajax-get-post-title-by-id.php:26 && wp_verify_nonce( $_POST['nonce'], 'ptc_completionist_automations' ) !== FALSE
... out of a total of 55 coincidences.
We have checked the function public static function sanitize( string $context, string $value ) : string {
And we see that some of the inputs are not being sanitized, as with
case self::ASANA_PAT:
$filtered_asana_pat = filter_var( $value, FILTER_UNSAFE_RAW, FILTER_FLAG_STRIP_LOW | FILTER_FLAG_STRIP_HIGH ); $sanitized_asana_pat = trim( preg_replace( '/[^a-z0-9\/:]+/', '', $filtered_asana_pat ) ); return (string) $sanitized_asana_pat;
So we worry about the safety of this, but we also think that you know what are you doing, so maybe you can help checking it out and giving a brief context to us.
Note: When checking a nonce using wp_verify_nonce you will need to sanitize the input using wp_unslash AND sanitize_text_field, this is because this function is pluggable, and extenders should not trust its input values.
Example: if ( ! isset( $_POST['prefix_nonce'] ) || ! wp_verify_nonce( sanitize_text_field( wp_unslash ( $_POST['prefix_nonce'] ) ) , 'prefix_nonce' ) )
Example(s) from your plugin:
completionist/src/admin/ajax/ajax-list-tasks.php:27 && wp_verify_nonce( $_POST['nonce'], 'ptc_completionist_list_task' ) !== FALSE//phpcs:ignore WordPress.Security.ValidatedSanitizedInput -----> wp_verify_nonce($_POST['nonce'], 'ptc_completionist_list_task') completionist/src/admin/ajax/ajax-get-post-title-by-id.php:26 && wp_verify_nonce( $_POST['nonce'], 'ptc_completionist_automations' ) !== FALSE completionist/src/admin/ajax/ajax-pin-task.php:28 && wp_verify_nonce( $_POST['nonce'], 'ptc_completionist' ) !== FALSE//phpcs:ignore WordPress.Security.ValidatedSanitizedInput -----> wp_verify_nonce($_POST['nonce'], 'ptc_completionist') completionist/src/admin/ajax/ajax-get-pins.php:27 && wp_verify_nonce( $_POST['nonce'], 'ptc_completionist_list_task' ) !== FALSE//phpcs:ignore WordPress.Security.ValidatedSanitizedInput -----> wp_verify_nonce($_POST['nonce'], 'ptc_completionist_list_task') completionist/src/admin/script-save-settings.php:111 && wp_verify_nonce( $_POST['asana_cache_ttl_save_nonce'], 'asana_cache_ttl_save' ) !== false
... out of a total of 21 coincidences.
Note: We strongly recommend you never attempt to process the whole $_POST/$_REQUEST/$_GET stack. This makes your plugin slower as you're needlessly cycling through data you don't need. Instead, you should only be attempting to process the items within that are required for your plugin to function.
Example(s) from your plugin:
completionist/src/admin/ajax/ajax-create-task.php:30 $task = Asana_Interface::create_task( $_POST );
Variables and options must be escaped when echo'd
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:
completionist/src/admin/ajax/ajax-get-automation.php:48 echo json_encode( $res ); completionist/src/admin/ajax/ajax-list-tasks.php:87 echo json_encode( $res ); completionist/src/admin/ajax/ajax-get-pins.php:49 echo json_encode( $res ); completionist/src/admin/templates/html-dashboard-widget.php:48 echo HTML_Builder::format_error_box( $e, 'Feature unavailable. ', false ); completionist/src/admin/templates/html-admin-dashboard.php:90 <select id="asana-tag" name="asana_tag" <?php echo $disabled; ?> required="required"> -----> echo $disabled; completionist/src/admin/ajax/ajax-pin-task.php:102 echo json_encode( $res ); completionist/src/admin/templates/html-metabox-pinned-tasks.php:108 <?php echo HTML_Builder::format_note_box_cta_button( Admin_Pages::get_settings_url(), 'Go to Settings' ); ?> completionist/src/admin/ajax/ajax-get-automation-overviews.php:56 echo json_encode( $res ); completionist/src/public/rest-api/class-attachments.php:193 print( $response_body );//phpcs:ignore
... out of a total of 24 coincidences.
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:
completionist/src/admin/ajax/ajax-get-post-options-by-title.php:64 echo json_encode( $res ); completionist/src/admin/ajax/ajax-save-automation.php:75 echo json_encode( $res ); completionist/src/admin/ajax/ajax-delete-automation.php:51 echo json_encode( $res ); completionist/src/admin/ajax/ajax-unpin-task.php:99 echo json_encode( $res ); completionist/src/admin/ajax/ajax-get-automation-overviews.php:56 echo json_encode( $res );
... out of a total of 15 coincidences.
Please note that due to the significant backlog the Plugin Review team is facing, we have only done a basic review of your plugin. Once the issues we shared above are fixed, we will do a more in-depth review that might surface other issues. In order to prevent further delays, we strongly urge you to review the guidelines again before you resubmit it.
If the corrections we requested in this initial review are not completed within 3 months (90 days), we will reject this submission in order to keep our queue manageable and you will need to resubmit the plugin from scratch.
Your next steps are:
Make all the corrections related to the issues we listed. Review your entire code following the guidelines to ensure there are no other related concerns. Attach your corrected plugin as a zip file OR provide a link to a public location (Dropbox, Github, etc) from where we can download the code. A direct link to the zip is best. Please do not send it using services where the download expires after a short period of time (such as WeTransfer-Free). Once we receive your updated code, we will re-review it from top down.
Be aware that if your zip contains javascript files, you may not be able to email it as many hosts block that in the interests of security. Keep in mind, all version control directories (like Github) will auto-generate a zip for you, so you do not need to upload a zip file to their systems. You can just link to the repository.
We again remind you that should you wish to alter your permalink (not the display name, the plugin slug), you must explicitly tell us what you want it to be. We require to you clearly state in the body of your email what your desired permalink is. Permalinks cannot be altered after approval, and we generally do not accept requests to rename should you fail to inform us during the review.
If you previously asked for a permalink change and got a reply that is has been processed, you’re all good! While these emails will still use the original display name, you don’t need to panic. If you did not get a reply that we processed the permalink, let us know immediately. While we have tried to make this review as exhaustive as possible we, like you, are humans and may have missed things. As such, we will re-review the entire plugin when you send it back to us. We appreciate your patience and understanding.
If you have questions, concerns, or need clarification, please reply to this email and just ask us.
-- WordPress Plugin Review Team | plugins@wordpress.org https://make.wordpress.org/plugins/ https://developer.wordpress.org/plugins/wordpress-org/detailed-plugin-guidelines/