sanitize_text_field() assumes the field is a string

[johnbillion]

The sanitize_text_field() sanitisation function is used to sanitize text input, but the function actually assumes the field is a string. If an array is passed in, for example, then it'll raise PHP errors.

This function should gracefully handle not string data, probably by returning an empty string.

[NathanAtmoz]

sanitize_textarea_field has the same issue. Should both functions check whether the input is a string independently, or should _sanitize_text_fields do it?

[Mte90]

patch with unit test

[Mte90]

This patch contain a check with is_string with unit test for a string that is not a string.

[pento]

In 44618:

Formatting: Add type checking to _sanitize_text_fields().

When a non-string value is passed, return an empty string.

Props Mte90.
Fixes #41450.

[jadpm]

I kindly ask to revert this change as it has unexpected side effects that can potentially affect a great number of plugins.

If I get the chain right, sanitize_text_field calls _sanitize_text_fields which calls wp_check_invalid_utf8, and this last one does a casting to string on the variable passed. I would suggest providing more data about the PHP errors that happen when passing an array, if any, and if so finding alternatives that do not introduce breaking chnages.

The change introduced here will make that any boolean sanitized with this function becomes false by design, which is unexpected and a breaking change that can have a potentially huge impact.

[ocean90]

@jadpm I'm assuming that you mean the boolean true since false was already returning an empty string. So the change is that sanitize_text_field( true ) no longer returns "1".

[azaozz]

...is used to sanitize text input, but the function actually assumes the field is a string.

Right, a "text input" is always a string. This is clearly documented in the docblock :)

I'm thinking this should be wontfix. A well-documented function expecting an argument of specific type should not be checking if that argument is actually of that type (unless we want to throw a "Doing it wrong?).

At most we can probably cast to a string to prevent warnings, although these warnings may be helpful to notice errors while developing plugins.

[jadpm]

Yes, I mean passing a true boolean.

I am not against strong typing, and I do agree that a function to sanitize a string should get a string as source, but I know this function is used to sanitize wild data posted by forms on AJAXed and non AJAXes environments, to save settings, and gets data from text inputs as well as booleans on checkbox fields checked status.

And of course I think such a change so late in a besta stage has the potential of afecting several third parties.

Anyway, we are adjusting our codebase in case this does not get reverted or reviewed.

[pento]

Given that sanitize_text_field() is intended to take user input and sanitise it, I'm inclined to [44618] as is. I much prefer returning an empty value rather than an undefined value, the latter could potentially be a vector for security issues.

[Nick_theGeek]

I see this is closed and maybe I should start a new ticket for this, but the current change is a breaking change that could potentially affect a large number of plugins and themes. I recently ran into this with a filter that could potentially have an integer passed to the function, which results in an empty string being returned. This was exactly happening and the value was being returned empty, thus an option was not updating when expected.

I'm wondering if it might be better to check to see if the value is an array or object and return an empty string then typecast the value to a string. This avoids errors but also allows for expected behavior with less breakage.

[pento]

Good idea, thanks @Nick_theGeek!

For types that can be safely cast to a string, we can pretty easily do that, which should help avoid the back compat issues.

[Nick_theGeek]

Use is_array/object check and typecast other $str values.

[Nick_theGeek]

@pento thanks. I've done a patch. I believe I did this correct but it is my first :)

The test change looks like it should still work for this so I didn't update the test, though we could expand it to test int, bool, and float for expected behavior.

[pento]

Hah, I was just writing the same patch. 😄

I've written some tests as well, I'll commit it all in a moment.

[pento]

In 44731:

Formatting: Loosen the type checking in _sanitize_text_fields().

[44618] added strict type checking to _sanitize_text_fields(), which has caused some compat issues with plugins.

We can loosen the type checking to only reject objects and arrays, and cast other types to string.

Props Nick_theGeek, pento.
Fixes #41450.

[fclaussen]

I am still not sure how, but this broken an important function I have.
I am trying to determine why, since I am passing a string to sanitize_text_field and it is being converted to an empty string.

If I ever figure this out, I'll update here.

[fclaussen]

Replying to fclaussen:

I am still not sure how, but this broken an important function I have.
I am trying to determine why, since I am passing a string to sanitize_text_field and it is being converted to an empty string.

If I ever figure this out, I'll update here.

Ok. I figure out the issue I was having.
I read from xml, sanitize this string and then create a post with the string.

Problem is that what was considered a "string" before, is actually an SimpleXMLElement object.
Switching to sanitize_text_field( (string) $myvar->title ) did the trick.

[iCaleb]

I ran into the same as the above with the SimpleXMLElement. It has a magic __toString() method that gets called when you do that type conversion.

Perhaps an extra check should be added for better backwards compatibility?

if ( ( is_object( $str ) && ! method_exists( $str, '__toString' ) ) || is_array( $str ) ) {
  return '';
}

[fclaussen]

Replying to iCaleb:

I ran into the same as the above with SimpleXMLElement. It has a magic __toString() method that gets called when you do that type conversion.

Perhaps an extra check should be added for better backwards compatibility?

if ( ( is_object( $str ) && ! method_exists( $str, '__toString' ) ) || is_array( $str ) ) {
  return '';
}

Hi Caleb, I am cc'ed on the ticket where you saw this issue haha. The moment I figured out what was happening I went to update the ticket and your reply was there with the same explanation.

I like your suggestion. Should we reopen this for 5.1.1 or create a new one?

[fclaussen]

A new ticket with the enhancement was created #46329