Make WordPress Core

Opened 11 years ago

Closed 4 years ago

#24106 closed defect (bug) (fixed)

Simplify wp_slash()

Reported by: tobiasbg's profile TobiasBg Owned by: whyisjake's profile whyisjake
Milestone: 5.5 Priority: normal
Severity: normal Version: 3.6
Component: Formatting Keywords: has-patch needs-unit-tests dev-feedback
Focuses: Cc:

Description

[23416] added the function wp_slash() for the slashing sanitization in #21767.

According to ryan, it has been modelled after $wpdb->prepare(), and therefore uses a custom foreach loop with an if-check in it.
I suggest to instead model it after stripslashes_deep() and urlencode_deep() to simplify the function and make it better readable.

The attached patch also makes it clearer that this function works in a recursive manner.

Attachments (4)

24106.patch (1.0 KB) - added by TobiasBg 11 years ago.
Simplify wp_slash()
24106.2.patch (2.1 KB) - added by SergeyBiryukov 10 years ago.
24106.test.php (1.3 KB) - added by SergeyBiryukov 10 years ago.
24106.wp_slash-unittest.diff (1.6 KB) - added by devesine 10 years ago.

Download all attachments as: .zip

Change History (18)

@TobiasBg
11 years ago

Simplify wp_slash()

#1 @SergeyBiryukov
11 years ago

  • Version set to trunk

#2 @markoheijnen
10 years ago

Isn't this something we can add in 3.8?

#3 @SergeyBiryukov
10 years ago

  • Keywords commit added
  • Milestone changed from Awaiting Review to 3.8

#4 @SergeyBiryukov
10 years ago

24106.2.patch includes a basic test for wp_slash() (adapted from Tests_Formatting_StripSlashesDeep).

#5 follow-up: @SergeyBiryukov
10 years ago

Wrote a quick script to test the performance of the suggested change with 1,000,000 iterations: 24106.test.php.

The unpatched version appears to be about 25% faster:

Simple arrayNested arrays
7.121s — unpatched19.524s — unpatched
9.754s — 24106.2.patch26.051s — 24106.2.patch

Not sure if performance is crucial for this function though. stripslashes_deep() uses array_map() too, so I guess it would make sense for wp_slash() to be consistent.

#6 @ryan
10 years ago

Some of this slash/unslash code was simply cut-and-pasted from old sources without alteration to avoid any chance of back compat problems. We promised to write a slew of the needed test cases when we finally got around to cleaning up the code. Cue the present and the slew. :-)

We need to inventory all of the different depth and object slashing behaviors our varied rogues gallery of slash/unslash functions exhibit and write unit tests for everything.

Last edited 10 years ago by ryan (previous) (diff)

#7 @nacin
10 years ago

  • Keywords commit removed
  • Milestone changed from 3.8 to Future Release

We need to inventory all of the different depth and object slashing behaviors our varied rogues gallery of slash/unslash functions exhibit and write unit tests for everything.

The unpatched version appears to be about 25% faster

#8 @devesine
10 years ago

Attached 24106.wp_slash-unittest.diff is a unit test for slashing behavior that includes datatype sanctity (compare the StripSlashesDeep test). As it stands, calling wp_slash followed by wp_unslash causes datatype loss.

This makes wp_slash rather unsuitable for use when preparing (for instance) postmeta to pass to add_metadata.

Last edited 10 years ago by devesine (previous) (diff)

#9 @TobiasBg
10 years ago

  • Keywords needs-unit-tests added

devesine, I'm not sure what you mean with datatype loss here. wp_slash()/wp_unslash() (both the patched and the unpatched versions) will only work with strings and (nested) arrays of strings, which basically is a result of PHP's stripslashes() only accepting and returning a string.

#10 in reply to: ↑ 5 @devesine
10 years ago

Replying to TobiasBg:

devesine, I'm not sure what you mean with datatype loss here. wp_slash()/wp_unslash() (both the patched and the unpatched versions) will only work with strings and (nested) arrays of strings, which basically is a result of PHP's stripslashes() only accepting and returning a string.

wp_unslash() uses stripslashes_deep() (source) , which handles strings, arrays, and objects (via get_object_vars()), and the tests for stripslashes_deep() (and by extension wp_unslash()) test for its ability to touch objects without turning them into arrays.

Unpatched wp_slash() only handles strings and arrays. The test I attached uses the same logic as the existing test for stripslashes_deep(), but on the slashing rather than the unslashing side of things.

wp_unslash() has a docblock that indicates it only works on strings or arrays of strings, but it is currently inaccurate.

#11 @chriscct7
8 years ago

  • Keywords dev-feedback added

#12 @jrf
6 years ago

Came across the code of wp_slash() today and just couldn't help but wonder why a recursive function wasn't properly implemented as recursive.

Searching led me to this ticket and #42195. /cc @johnbillion

I've prepared a patch which would fix both tickets and which unifies and adds to the unit tests uploaded in previous patches for this ticket by @SergeyBiryukov and @devesine.

Before I upload the new patch, I'd like to get some clarification on how objects should be handled.

In my opinion, object property values should never be slashed, but I saw some unit tests in the previous uploads which don't seem to agree with that.

Reaching a consensus on how objects should be handled by wp_slash() is a prerequisite for any patch dealing with this and #42195.

@pento Got an opinion on this ?

#13 @whyisjake
4 years ago

  • Milestone changed from Future Release to 5.5
  • Owner set to whyisjake
  • Status changed from new to accepted
  • Type changed from enhancement to defect (bug)

Bringing the changes here over with #42195.

#14 @whyisjake
4 years ago

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

In 48433:

Formatting: Prevent wp_slash from returning non-strings as strings.

If a bool/float/int is passed into wp_slash it will be coerced into a string.

This changes the behavior to only slash strings. At the same time, handles recursion a little nicer by calling array_map for arrays.

Fixes #42195, #24106.

Props johnbillion, andizer, jrf, ryotasakamoto, SergeyBiryukov, donmhico, TobiasBg, markoheijnen, ryan, nacin, devesine, whyisjake.

Note: See TracTickets for help on using tickets.