Hello,
Show the full code of the line, so we can help you to diagnose. Be aware that could be false positives in the review.
Here are some examples (out of many) from the checker plugin:
FILE: odp_config_files/default_cssForm.php
122 51 ERROR WordPress.Security.EscapeOutput.OutputNotEscaped All output should be run through an escaping function (see the Security sections in the WordPress Developer Handbooks), found ‘CONFIG_BLOKT_MAINCAT_H2’.
123 41 ERROR WordPress.Security.EscapeOutput.OutputNotEscaped All output should be run through an escaping function (see the Security sections in the WordPress Developer Handbooks), found ‘CONFIG_BLOKT_MAINCAT_H2_MESSAGE’.
125 224 ERROR WordPress.Security.EscapeOutput.OutputNotEscaped All output should be run through an escaping function (see the Security sections in the WordPress Developer Handbooks), found ‘$currentodp2DyMainCatH2’.
And my plugin’s code that generates the error
123 … <?php echo CONFIG_BLOKT_MAINCAT_H2; ?>
124 … <?php echo CONFIG_BLOKT_MAINCAT_H2_MESSAGE; ?>
125 … <?php if(isset($currentodp2DyMainCatH2)) {echo $currentodp2DyMainCatH2;}?>
And similar code in my plugin that “fixed” the reported error:
<?php if(isset($currentMainBackgroundColor)) {echo esc_attr($currentMainBackgroundColor);}?>
So the checker requires “esc attr” on every output whether it is from a trusted source or not because (as the documentation says) there might be problems in the future from outputting nontrusted sources. This looks like a false positive to me? And there are literally hundreds of them generated?
100% trusted content from my own database.
You cannot trust DB data when it comes to escaping output. Someone in theory could inject malicious code into your DB via some other vulnerability. By escaping, you at least protect your end users even though your own protections proved to be inadequate.
By “hardcoded”, it is understood to be something like echo 'Hello world!';. Data from your DB is not hardcoded data. This example realistically doesn’t need to be escaped as it is inviolate, but there’s little harm in escaping it anyway. Constants may or may not be hardcoded, it depends on how they are defined.
@bcworkz (@bcworkz) If I “cannot trust DB data” as you say I need to shut down my site and secure the data! I definitely don’t care about displaying “escaped” and false information FROM my own database! Thanks though.
Hello,
But actually we do care about that DB, so in order to have secure code, you have to change the lines to:
123 … <?php echo esc_html( CONFIG_BLOKT_MAINCAT_H2 ); ?>
124 … <?php echo esc_html( CONFIG_BLOKT_MAINCAT_H2_MESSAGE ); ?>
125 … <?php if(isset($currentodp2DyMainCatH2)) {echo esc_html( $currentodp2DyMainCatH2 );}?>
If you have HTML inside that variables, you could use wp_kses_post.
Regards.
It looks to me that non-escaped output should simply be reported as a “WARNING” rather than an “error” . A similar situation I am running into is a warning about “WordPress.Security.NonceVerification.Missing” that is reported on an “included” (i.e. a file pulled into the active file from the plugin’s folder). If the included file also contained a form then it certainly could use the nonce to prevent cross site scripting but I don’t see nonces as necessary for each and every “included” page.
I guess my question then is whether the team reviewing plugins considers non-escaped output as an absolute “no-no” or something that should be scrutinized carefully (depending on the source of the displayed content [i.e. trusted or not])?
Yes, we do consider that we have to escape everything even if we have trusted sources. We use warnings if we find that sometimes could give false positives. But it’s not the case.
Bob
(@prasunsen)
“But actually we do care about that DB, so in order to have secure code, you have to change the lines to…”
There are thousands of cases when the site admins and authors have to output HTML and JS. It is the essence of every CMS – be it with shortcodes or templates it doesn’t matter. An authorized person is allowed to insert whatever the site owner has decided that is allowed. You should make sure that the data inserted in the DB accepts only what the specific person is allowed to embed, but you shouldn’t disallow all HTML and JS on the output making no difference where it comes from “just because”.
-
This reply was modified 1 year, 6 months ago by
Bob.
Thanks Bob, You put it very well and I appreciate it. The policy made no sense to me and I don’t believe the weak defense of them was anything “technical”.
