WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 2 years ago

Last modified 2 years ago

#41450 closed defect (bug) (fixed)

sanitize_text_field() assumes the field is a string

Reported by: johnbillion Owned by: pento
Milestone: 5.1 Priority: low
Severity: normal Version: 2.9
Component: Formatting Keywords:
Focuses: Cc:

Description

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.

Attachments (2)

41450.diff (972 bytes) - added by Mte90 3 years ago.
patch with unit test
41450.2.diff (559 bytes) - added by Nick_theGeek 2 years ago.
Use is_array/object check and typecast other $str values.

Download all attachments as: .zip

Change History (25)

#1 @NathanAtmoz
3 years ago

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
3 years ago

patch with unit test

#2 @Mte90
3 years ago

  • Keywords has-patch dev-feedback added; needs-patch 2nd-opinion removed

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

#3 @johnbillion
3 years ago

  • Milestone changed from Awaiting Review to 5.0
  • Owner set to johnbillion
  • Status changed from new to reviewing

#4 @johnbillion
2 years ago

  • Milestone changed from 5.0 to 5.1

#5 @pento
2 years ago

  • Owner changed from johnbillion to pento
  • Status changed from reviewing to accepted

#6 @pento
2 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

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.

#7 @jadpm
2 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

#8 @ocean90
2 years ago

@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".

#9 @azaozz
2 years ago

...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.

#10 @jadpm
2 years ago

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.

#11 @pento
2 years ago

  • Keywords dev-feedback removed
  • Resolution set to fixed
  • Status changed from reopened to closed

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.

This ticket was mentioned in Slack in #core by peterwilsoncc. View the logs.


2 years ago

#13 @Nick_theGeek
2 years ago

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.

#14 @pento
2 years ago

  • Keywords needs-patch added; has-patch removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

#15 @pento
2 years ago

  • Keywords needs-patch removed

@Nick_theGeek
2 years ago

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

#16 @Nick_theGeek
2 years ago

@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.

#17 @pento
2 years ago

Hah, I was just writing the same patch. 😄

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

#18 @pento
2 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

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.

#19 follow-up: @fclaussen
2 years ago

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.

#20 in reply to: ↑ 19 @fclaussen
2 years ago

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.

#21 follow-up: @iCaleb
2 years ago

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 '';
}
Last edited 2 years ago by iCaleb (previous) (diff)

#22 in reply to: ↑ 21 @fclaussen
2 years ago

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?

#23 @fclaussen
2 years ago

A new ticket with the enhancement was created #46329

Note: See TracTickets for help on using tickets.