Make WordPress Core

Opened 17 months ago

Last modified 17 months ago

#56119 new defect (bug)

`wp_unslash()` and `wp_slash()` do not (un)slash the same data.

Reported by: peterwilsoncc's profile peterwilsoncc Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: Formatting Keywords: reporter-feedback 2nd-opinion
Focuses: Cc:

Description

The wp_unslash() and wp_slash() functions do not do exactly the opposite thing, ie running $data = wp_unslash( wp_slash( $data ) ) may modify the data.

The issue occurs with the treatment of objects, wp_unslash() uses map_deep() and therefore accounts for objects; whereas wp_slash() does not traverse objects.

<?php
/**
 * @ticket 56119
 */
public function test_slash_unslash_reversable() {
        $expected = (object) array( 'data' => 'sl\sh' );
        $actual = wp_unslash( wp_slash( clone( $expected ) ) );

        $this->assertSame( $expected->data, $actual->data );
}

/**
 * @ticket 56119
 */
public function test_unslash_slash_reversable() {
        $expected = (object) array( 'data' => 'sl\sh' );
        $actual = wp_slash( wp_unslash( clone( $expected ) ) );

        $this->assertSame( $expected->data, $actual->data );
}

Change History (3)

#1 @costdev
17 months ago

  • Keywords reporter-feedback 2nd-opinion added

It seems that using the following from the deprecated wp_slash_strings_only Ref does the trick here:

<?php
function wp_slash( $value ) {
        return map_deep( $value, 'addslashes_strings_only' );
}

addslashes_strings_only() Ref is also deprecated, but this might mean it's a candidate for undeprecating?

I modified the tests to use slashable characters, as wp_slash() has no way to know that the second "s" should be slashed in slsh:

<?php
/**
 * @ticket 56119
 */
public function test_slash_unslash_reversable() {
        $expected = (object) array( 'data' => 'sl"sh' );
        $actual   = wp_unslash( wp_slash( clone( $expected ) ) );

        $this->assertSame( $expected->data, $actual->data );
}

/**
 * @ticket 56119
 */
public function test_unslash_slash_reversable() {
        $expected = (object) array( 'data' => 'sl\"sh' );
        $actual   = wp_slash( wp_unslash( clone( $expected ) ) );

        $this->assertSame( $expected->data, $actual->data );
}

What do you think @peterwilsoncc?

#2 @johnbillion
17 months ago

It's worth noting that the parameter type of both wp_slash() and wp_unslash() is documented as string|array even though the latter handles objects via its use of map_deep().

#3 @SergeyBiryukov
17 months ago

I think wp_slash_strings_only() was deprecated because it appeared to be the same as wp_slash() as of [48433], though the difference in object handling was probably not considered.

Good catch on the inconsistency in object handling! Looks like that was the case ever since wp_slash() and wp_unslash() were both introduced in [23555].

Looking at #21767, it seems that:

  • wp_slash() was introduced as a replacement for addslashes() and add_magic_quotes().
  • wp_unslash() was introduced as a replacement for stripslashes() and an alias for stripslashes_deep().
  • wp_unslash() intentionally unslashes objects: comment:53:ticket:21767.

I guess the question to answer would be: do we need to slash objects in wp_slash(), or would that be unexpected? This comment seems tangentially related: comment:17:ticket:21767, and this commit too: [24731].

I think wp_slash() and wp_unslash() may not necessarily have been intended to be the opposite, but that would probably make sense if there are no unexpected site effects.

Version 1, edited 17 months ago by SergeyBiryukov (previous) (next) (diff)
Note: See TracTickets for help on using tickets.