Check that the input is a string in wp_strip_all_tags()

[chocofc1]

I found my error log full of:

PHP Warning: strip_tags() expects parameter 1 to be string, array given in /wp-includes/formatting.php on line 5386

And it results that the developer of the template I was using, was causing this error message.
Off course the logical thing to do would be to fix the template code, but this is one of those badly designed templates with the code hard to read, ofuscated, splited, mixed, brain-melting, so I ended up having to add 1 line to WordPress file:

/wp-includes/formatting.php line 5385

to make sure the @string is a string

Would be great if WordPress took care of this in future versions

This PR builds on patch 56434.diff.

Trac ticket: https://core.trac.wordpress.org/ticket/56434

This PR provides an alternate approach to ​PR 3132.

Split out to make it easier to review changes against Core and to compare the approaches.

Trac ticket: https://core.trac.wordpress.org/ticket/56434

[jrf]

At the request of @costdev, I've had a look at both ​GH #3132 as well as ​GH #3133.

First things I noticed:

• Both are missing a test for null.
• ​GH #3133 has tests with floats, ​GH #3132 does not.
• Neither has a test with a negative numeric value.
• Also missing: a resource, but to be honest, I'm fine with not testing that.

Realistically, this is not something which should be fixed in WP, but it should be fixed at the point of origin (where this function is called) instead.

All the same, I understand the desire to prevent the PHP 8.1 "passing null to non-nullable" deprecation notice and the PHP 8.0 "passing array to string" warning.

If you want my opinion, I would combine the two approaches:

Basically use the patch from ​GH #3133, but add the _doing_it_wrong() from ​GH #3132 within the if ( ! is_string( $string ) ) { condition before returning.
I would not special case null, but treat it the same as other non-scalar values.

Leaving the _doing_it_wrong() out, as @peterwilsoncc ​suggests, would be hiding an error which should be fixed instead, which is bad practice and not helping devs.

Given the ease with which user submitted data can be turned in to an array by the user and the frequency with which is normalized to a non string by developers $thing = ! empty( $_POST['t'] ) ? wp_unslash( $_POST['t'] ) : false;

To me, that's all the more reason for a _doing_it_wrong() as we need to "teach" devs better input sanitation practices, not just to prevent these kind of PHP errors, but also to keep the users safe from hacks.

Off course the logical thing to do would be to fix the template code, but this is one of those badly designed templates with the code hard to read, ofuscated, splited, mixed, brain-melting, so I ended up having to add 1 line to WordPress file:

@chocofc1 I understand and hear your frustration, but the fact that the template is bad is a reason to improve the template, not to try and fix a non-bug in WP.

[SergeyBiryukov]

Replying to jrf:

If you want my opinion, I would combine the two approaches:

Basically use the patch from ​GH #3133, but add the _doing_it_wrong() from ​GH #3132 within the if ( ! is_string( $string ) ) { condition before returning.
I would not special case null, but treat it the same as other non-scalar values.

Thanks! That makes sense to me.

[costdev]

Alright, so after adding back the _doing_it_wrong(), including for null, other tests began failing because of an unexpected _doing_it_wrong().

After some investigating, there's some fun about throwing _doing_it_wrong() when $string is null.

---

1. Comments: wp_handle_comment_submission() can send a comment without an author_url, prompting the following path:
   • wp_new_comment()
   • wp_allow_comment()
   • wp_filter_comment()
   • apply_filters( 'pre_comment_author_url' )
   • default-filters.php adds wp_strip_all_tags() as a callback.
   • 💥 _doing_it_wrong() is thrown on null.
2. Login: By default, the username is empty (read: null).
   • Visit the login page.
   • wp_signon()
   • wp_authenticate()
   • sanitize_user()
   • wp_strip_all_tags()
   • 💥 _doing_it_wrong() is thrown on null.

These are just two of the paths where null can be passed by Core to wp_strip_all_tags().

---

So, at the moment, I see some possible options:

1. We evaluate all 25 calls/callbacks of wp_strip_all_tags() in Core and lock down all potential edge cases.
   • This will take some time and a couple of contributors to do it correctly.
2. We don't throw a _doing_it_wrong() on null, and simply return an empty string for the sake of BC.
   • This is bad for input validation/sanitization. Essentially, Core hasn't validated the input to a function used for input sanitization, and Core itself has been passing the wrong type in certain circumstances.

I'm curious to hear what others think about the two options above, as well as other options you might have in mind.

---

Additional information:

• There are ​~10.5k plugins that call wp_strip_all_tags().
• There are ​~2300 themes that call wp_strip_all_tags().
• The type has been documented as string since the function was introduced in WordPress 2.9.

[peterwilsoncc]

Replying to jrf:

Leaving the _doing_it_wrong() out, as @peterwilsoncc ​suggests, would be hiding an error which should be fixed instead, which is bad practice and not helping devs.

I disagree:

• WordPress needs to make sanitization of data as easy as possible. Rather than assisting site owners and developers here, logging a notice here would add noise for a security function that is handling the use case
• the WPCS security sniffs recommend unslashing and an isset/empty check. They do not recommend a type check.

Adding a notice would require developers change change the code:

<?php
$thing = ! empty( $_POST['t'] ) ? wp_strip_all_tags( wp_unslash( $_POST['t'] ) ) : '';

to

<?php
if ( ! empty( $POST['t'] ) && is_string( $POST['t'] ) {
  $thing = wp_strip_all_tags( wp_unslash( $_POST['t'] ) );
} else {
  $this = ''; // or false or whatever works.
}

An alarming number of extenders already fail to sanitize correctly, making it more difficult for them to do so is not going to help improve the situation.

[jrf]

@peterwilsoncc There are two parts of your argumentation which don't sit well with me:

1. WPCS is not the right tool to verify whether a supported type is being passed to a function. So, saying that WPCS is *not* demanding something is a non-argument here. If you want to improve type-safety as a dev: use PHPStan or Psalm.
2. Your code sample comparison doesn't make sense as everything received from $_POST will always be a string, so no changes are needed for that code.

What's left is your concern for error logs. Sorry, but with the default settings of WP, these kind of notices would not show and not be logged. A user/dev would have to actively turn notices on to see them, which is exactly what plugin/theme devs should do and why adding the notice should have no effect on end-users.

[peterwilsoncc]

$_POST and $_GET can be either a string or an array and it's beyond the control of the developer. As a visitor I can change ?t=thing to ?t[]=thing and the type will be an array.

I am not saying WPCS is the right tool for determining type. I am saying WordPress is the right tool to make sanitization of user data as simple as possible for extenders in order to encourage them to use it.

As WPCS encourages users to make sure the data is set, WordPress should make things as easy as possible to do the right thing from there. Calling sanitization functions is, unambiguously, the right thing to do so telling developers they are doing the wrong thing does not help.

[dd32]

I would hesitant to suggest that a _doing_it_wrong() is an appropriate notice for passing unexpected data-types. If a sanitization function only operates on scalar, and a non-scalar is provided, either the function should handle it (ie. array) or return false IMHO. I would note that wp_strip_all_tags() is not a sanitization function IMHO though..

That seems especially true to me for functions which are intended to run on user-provided data via POST/GET/etc, due to the significant number of plugins (and Core!) which simply do not validate whether a parameter is the expected data-type.

I would argue that type-checking of the data is best left to the point it's accessed, and in the case of POST/GET would probably be benefited through a solution to #22325 at some point.

However, to step back from my point of view, the modern PHP way of doing things is to throw a TypeError fatal when strict types are used, or when a parameter data type is specified.. so in that sense, _doing_it_wrong() is kind of the PHP way of thinking, although a lot less quiet than PHPs fatal errors.

I guess to me, _doing_it_wrong() is appropriate when a developer will actually see the warning during development, and can act appropriately to change how they're calling something, but if it's just passing junk to a function and not handling the PHP Notice / Warning / Fatal / Invalid return value, the _doing_it_wrong() notice wouldn't have actually helped them in the first place.

[costdev]

@peterwilsoncc Calling sanitization functions is, unambiguously, the right thing to do so telling developers they are doing the wrong thing does not help.

_doing_it_wrong() tells developers that they are doing a thing incorrectly, not that they are doing the wrong thing altogether. i.e. a function/method was called incorrectly, not "you tried to sanitize, bad dev".

Letting developers know when they've made a mistake is definitely helpful, especially if they're trying to do a good thing, but a mistake makes it ineffective. It would remain ineffective if the function failed silently.

@dd32 I would note that wp_strip_all_tags() is not a sanitization function IMHO though..

I'd say it's context-dependent - e.g. _sanitize_text_fields() and sanitize_user() use it. It's probably safe to say that it's at least a helper for sanitization if not used as a sanitization function itself.

---

Some thoughts on the discussion:

• Returning false for non-scalar makes sense. However, note that this will change the function's return type to string|false, and as truthy/falsy/empty() checks are inappropriate here (consider <b>0</b>), it would break BC.
• Silently handling incorrect usage doesn't help developers track down why they're getting unexpected output, particularly when there's a long trace.

IMO, the appropriate solution is the following:

Scalar - Continue to cast to string.
Non-scalar (except null) - Call _doing_it_wrong() and return ''.
null - see comment 7. Need opinions.

• Protects BC for scalar.
• Protects sites from a fatal error for non-scalar in PHP 8+.
• Informs developers of mistakes, without terminating execution in PHP 8+.
• Only those who want to see debug messages will see them.

[jrf]

@peterwilsoncc @dd32 The problem I have with this kind of handling is that WP is slowly turning into a "every function handles every type of input"-engine instead of supporting the documented functionality.

This will only set us up for failure in the future what with PHP getting stricter all the time.

I'm not advocating that WP should become as strict as PHP, but at the very least, let's only support the intended/documented functionality and not make every function a catch-all for any input types.

[audrasjb]

With WP 6.1 RC 1 scheduled today (Oct 11, 2022), there is not much time left to address this ticket. Let's move it to the next milestone as it still needs to be discussed.

Ps: if you were about to send a patch and if you feel it is realistic to commit it in the next few hours, please feel free to move this ticket back to milestone 6.1.

[peterwilsoncc]

The original ​linked pull request, #3132 is been worked on to closely mirror the PHP < 8 behavior

• implicit conversion for scalar values
• warning, return empty string for non-scalar values

This retains backcompat and avoids fatal errors.

Closing this off as it appears the approach of retaining 7.0 compatibility has wider support.

Am I missing something or is the null test case (which started the issue) still not being tested ?

Actually, adding some additional tests (which were included in #3133) may not be a bad idea anyway.

[peterwilsoncc]

In 55245:

Formatting: Guard wp_strip_all_tags() against fatal errors.

Check the input of wp_strip_all_tags() before passing it to strip_tags(). This protects against fatal errors introduced in PHP 8, retaining the E_USER_WARNING from PHP 7, and prevents a PHP 8.1 deprecation notice when passing null.

Props chocofc1, costdev, jrf, dd32, audrasjb, peterwilsoncc.
Fixes #56434.

Merged in https://core.trac.wordpress.org/changeset/55245 / ​https://github.com/WordPress/wordpress-develop/commit/d46dc08342c8e90e16289ae850097d6a31118cd6