-2

I'm a complete neophyte when it comes to PHP, so I suspect this question has a simple answer that I'm just not able to find yet.

I'm using PHP Code Sniffer with WordPress Coding Standards to put together a basic wordpress plugin. A bunch of tutorial code I've encountered encouraged echoing content along the lines of:

echo $before_widget . $before_title . $title . $after_title;

Which works fine, but which PHP Code Sniffer raises a warning before each variable echoed of expected next thing to be an escaping function not $VariableName.Intuitively, I don't want to escape these variables - HTML contained in them should render properly, and it's hard to see how an attacker could have changed the instance $args object where $before_widget etc. come from to introduce XSS vulnerabilities. But as I said, I'm new to PHP, WordPress, etc. and I'm not aware of what will have had access to these variables in full before I get them.

Long story short: Given I want the HTML to be rendered as HTML (I do not want to escape it), how should I either prepare them such that I am protected from any XSS issues I'm not aware of, or inform PHP Code Sniffer that these are not user input, and therefore are safe?

tobriand
  • 1,095
  • 15
  • 29
  • That question can hardly be answered without you first of all _specifying_ what you would consider HTML that is “okay” to be output, and at what point it would become XSS … – CBroe May 11 '17 at 08:37
  • Fair point. If a site owner, or someone they (implicitly) trust (say, the author of a theme who has set up a widget area, which is typically where these values come from) has provided that HTML, then I would consider it trusted (even if trust has been misplaced). If a site user has managed to adjust those values via some mechanism that calls `add_filter()` with user content on one of them, then it is now most assuredly not. Since I'm new to PHP and WordPress dev, I don't know what such circumstances might be. But I do not yet trust that they do not exist since phpcs seems to thing they might! – tobriand May 11 '17 at 09:22
  • Equally, if they don't, and the appropriate response is just "leave the warning there", or "put in whatever the PHP equivalent of a `#pragma` directive is to suppress it", that's fine with me. But I don't want to ignore a potential issue when I don't yet know that I know better than the coding tools! – tobriand May 11 '17 at 09:23
  • Such tools can only ever give hints and point out _potential_ issues. It still needs someone with the appropriate technical knowledge to review each single instance. – CBroe May 11 '17 at 09:31
  • Yes, of course. Rather what I'm saying is that since I *don't* yet have that knowledge because I'm new to the environment, I'm hoping that since there are lots of people who write WordPress themes and plugins, someone can tell me if this is such a case, given that the `$args['before_widget']` etc. where my html is coming from are pretty well standardised. As I say, if the answer is "Yup, that should just be ignored" then that *is* fine (I suspect that is the case). I'm asking the question because it is not obvious - precisely because of the structure of the system - whether that is the case. – tobriand May 11 '17 at 09:59
  • 1
    Well in regard to this specific instance: The before and after stuff comes from the sidebar configuration, and since that usually contains HTML elements to structure the output, escaping would not make much sense here. The title however gets input by the admin user in the backend - so that could easily contain characters that need escaping. Does not even have to be XSS, can be simple stuff like the title containing an `&`, which would have to become `&` if you still want the output to be valid HTML. So for that value, `sanitize_title` or at least `esc_html` should probably be used. – CBroe May 11 '17 at 10:38
  • Awesome - that makes sense. Presumably I then will need to find a way to suppress the warnings for the rest of the content? You wouldn't happen to know what that is would you? – tobriand May 11 '17 at 11:06

2 Answers2

0

This will prevent the warning

$a = "";

$a .= $before_widget; 
$a .= $before_title;
$a .= $title;
$a .= $after_title;

echo $a;

or

echo $before_widget ."". $before_title ."". $title ."". $after_title;
Subi
  • 107
  • 8
  • Unfortunately, neither seem to remove the warning for me. I also get a new one stating `a string does not need double quotes; use single quotes` too. – tobriand May 11 '17 at 08:39
  • Try to replace all double quotes to single quotes – Subi May 11 '17 at 08:42
  • Eliminates the double-quote warning, and I'm now down to one warning on the echo argument needing escaping, but I still have it (when I use the fist approach, that is) – tobriand May 11 '17 at 09:10
  • 1
    try this in first approach ** echo htmlentities($a); ** – Subi May 11 '17 at 09:33
  • Still doesn't work to eliminate the warning. I've a suspicion the eventual solution is close to that provided by CBroe's comment above, i.e. don't escape anything except title. The missing piece, then, becomes telling phpcs not to moan for the remainder of those elements I should be echoing un-filtered. – tobriand May 11 '17 at 15:40
  • Hey, it's all useful. Even if they don't work in the end, they're logical things to try :). I was a little surprised it didn't pick up the `htmlentities()` call as indicating that "this is HTML and you shouldn't worry about that", really... Can't shake the sensation that it's basically just not the PHP/WordPress way to output HTML, though. – tobriand May 11 '17 at 15:49
0

Ok, the answer it looks like I was after was to make use of the wp_kses() function on the output prior to printing it. According to the documentation, this

makes sure that only the allowed HTML element names, attribute names and attribute values plus only sane HTML entities will occur in $string

It also makes PHP Code Sniffer stop moaning about echoing a sting without any escaping taking place.

Without digging into the implementation, I can't say for sure that this covers all bases (sane HTML entities could be interpreted in a number of ways, after all), but intuitively, it feels like the kind of thing I might want to do to code that's come in from - albeit trusted - external sources in case they, you know, take down the webpage due to malforming.

Final code ended up looking like:

$html = '';
$html .= $args['before_widget'];
$html .= $args['before_title'];
$html .= esc_html( $title );
$html .= $args['after_title'];
$allowed_tags = wp_kses_allowed_html( 'post' );
echo wp_kses( $html, $allowed_tags );

As a note, it's worth noting that in my travels identifying kses, I found a number of places suggesting it's a pretty slow function to pass content through. For the purpose, it seems right, but I'll listen if anyone knows a lighter weight approach.

tobriand
  • 1,095
  • 15
  • 29