My opinion, and some of the topics on that are very infinite in possibilities and also not fully clarified by doc, for example, but still, I think it might help:
The sniff I created actually should ignore your PHPCS annotation, because devs might think to be smart and bypass things by adding the ignore statement. So we want to see them anyway.
It is absolutely correct to add a comment.
The annotation should however not work when using the sniff shared earlier (but of course if you have your own sniff setup to not ignore those, then yes it will hide the error).
I would still recommend that this is exactly a case where you want to be very sure about what comes in.
While cap is one thing that makes things safer… the very “right” thing would be a custom kses maybe, that allows a range of things you might expect, but not others.
However, of course, the reviewer does not know what you intend to do with that input, so if it is entirely impossible to validate, I guess it would be fine (given it is only for manage option cap users)
Strictly speaking however that is not enough. I am not sure this would pass WP Repo review.
Do you expect HTML in that $var?
If not, sanitise it so that it can’t hold any.
Or, do you expect special tags, but not others? Then you can use kses to allow some, not others.
However the context would show more I guess.
It is one of those areas where I personally, if reviewing this, would likely ask someone to help me review just to be sure.
My opinion is that I must review translations, so there is no need to sanitize them but let’s follow WPCS.
This is not only your opinion, as a matter of fact this is something that seems the more wide idea here at CP. I disagree with it, but the idea is not entirely wrong.
However, as long we do not have a centralised translation management also for Plugins that gets reviewed… it should be sanitised/escaped I think - not only because of security but also because stuff might just simply by mistake hold things that can break your code. Generally speaking that is.
Here phpcs can’t know that I’ve sanitized my input elsewhere.
That is a classic one which I hate because it puts the entire “escape late, sanitise early” talk into a perspective of “how please?!”, and can be found through out the entire core code.
Often your $var has complex HTML and not even wp_kses will help you… so Yes, in those cases, escape on echo is plain simple not possible. Nothing we can do here.
The best approach I think here is to try avoiding this kind of $var output whenever possible and output chunks insetead, but it is simply not possible sometimes (hey, looking at you the_content()
!).
And yes a comment elaborating helps and should be enough in this case.
If it is not obvious from the previous few lines, an @see
annotation also helps, so the other dev can quickly see whatever other function or code does the actual escaping or sanitisation.
I would expect to get a “you forgot to sanitise” feedback from reviewers (especially the busy ones like at WP will not bother long about reading code), and then I would have to feedback with a detailed explanation as of why it is safe and OK.
That should hopefully not happen here as we are not that busy with reviews, but still might happen. So whenever delivering such code, just be prepared to have a well backed up reasoning (if not already put in code comment)
PS:
I am not the most experienced security tool here (I am a tool, but not really a security tool ).
So I have hopes that some experienced people could also feedback on it, but I think most what will be the result is “it depends on the context” (which puts me as noob back to the thinking pad trying to avoid any such case where context needs to be interpreted so I can avoid any “ambiguity”)
Hope it helps
PS2
this thread might also help about those $vars that get echoed without escaping.