I’m the author of events-made-easy (hosted on github). If I run the CPCS on it, I see a lot of warnings like:
9 | ERROR | Processing form data without nonce verification. (WordPress.Security.NonceVerification.Missing)
9 | ERROR | $_POST data not unslashed before sanitization. Use wp_unslash() or similar
| | (WordPress.Security.ValidatedSanitizedInput.MissingUnslash)
10659 | ERROR | [ ] Use placeholders and $wpdb->prepare(); found interpolated variable $answers_table at "SELECT * FROM
| | $answers_table WHERE related_id=%d AND type='event'" (WordPress.DB.PreparedSQL.InterpolatedNotPrepared)
10660 | ERROR | [ ] Use placeholders and $wpdb->prepare(); found $sql (WordPress.DB.PreparedSQL.NotPrepared)
9008 | ERROR | [ ] All output should be run through an escaping function (see the Security sections in the WordPress Developer
| | Handbooks), found 'eme_rss_cdata'. (WordPress.Security.EscapeOutput.OutputNotEscaped)
Now most of these are false positives:
the nonce is checked if needed, but not always needed for a GET of course)
the unslash and sanitize of $_POST data happens in a custom function (which also attempts to remove more “bad” things but also uses wp_kses and wp_unslash)
the variable $answers_table is a fixed var, set before (using wpdb prefix and a constant), but of course the prepare sees it as non prepared. Replacing all these table names by their value in each query would generate very ugly code which I try to avoid.
the $sql var is prepared (using wpdb prepare) just before the wpdb query, but wordpress wants that “all in one” (which again generates very ugly code …)
I tend to use esc_html way before an actual print (so I don’t need to do this call multiple times) but the CS requires esc_html inside a print
So … in short: is there any way to be able to attack these issues without needing to rewrite all SQL and functions? Only if that is possible, I would like to include my plugin here.
The same CS-issues caused my plugin to be booted from the WP plugins site, after being on it for more than 11 years. I had a small security issue, but wordpress refused to release the fix unless I adhered by the complete WPCS (including late escaping) standard. This resulted in the security fix not being released at all, something I never encountered in any company (not releasing a fix because you want to first have the code up to new standards …). Anyway, before I do any extra work, I want to know the CP point of view on the coding issues
Hi Frank, what I do is take the offending file and ask Perplexity AI to explain to me how I can make it PHPCS compliant without changing basically what it does (I am very junior and this helps me learn because it explains to me like I am a 3 year old in the “why” age)
That results in making only the needed changes (in your case you can I think add exceptions for PHPCS if they are false positives) without having to rewrite the whole thing.
If I have a variable that will be used in multiple places for output, I escape first and then use it in the print statements. If I would do it in each print statement, it is slower and much more ugly code (and bigger in size). The only reason I can think about is that a phpcs-thing can pick up escaping in a print statement, but not before it (like the sql prepare issue etc …).
A good code validator would detect that if escaping happened, the variable wasn’t changed afterwards and then printed, the print is safe. Maybe phpcs is a good one and just the wp-specific tests are not correctly written … I know for one thing that sanitizing array content is not being detected ok either (bug in wp-cs).
Anyway, it seems my code iis not likely to get approved here either, so I’ll stop trying to convince you
I was already very happy to see that my plugin works without code changes on classicpress, so at least there’s that
A good code validator would detect that if escaping happened, the variable wasn’t changed afterwards and then printed, the print is safe.
That’s not the point of the validator. It has no idea whether another plugin might have access to that variable (e.g. though an output buffer). If that is possible, then early escaping runs the risk of that other plugin also escaping the variable (because it needs to ensure that all content it outputs is escaped). And then the variable gets double-escaped, resulting in HTML tags and entities ending up getting printed literally to the screen.
This is an example of how coding for other people is very different from coding for yourself.
Finally, if there is something that CPCS is missing, please open a new thread and explain what the problem is and, if you have an idea, how to fix it.
That’s not the point of the validator. It has no idea whether another plugin might have access to that variable (e.g. though an output buffer). If that is possible, then early escaping runs the risk of that other plugin also escaping the variable (because it needs to ensure that all content it outputs is escaped). And then the variable gets double-escaped, resulting in HTML tags and entities ending up getting printed literally to the screen
Well ok, that might be an example and indeed would result in double escaping (which is bad, but doesn’t count as a security issue). Wordpress introduced (or started requiring) this 2,5 years ago (I think), just before I released the security fix. Then not allowing the security release because of this, is even worse. Anyhow, I’m not going to attack CP or WP here for their standards, but I teach my students to take care of CPU cycles, mem usage and code readability too, and in my opinion this late escaping is not ok (the same goes for their wpdb implementation … other implementations are much more robust and user friendly, but that’s another thing). Now browsing through 72k lines of code and fixing those is just too cumbersome
