Context Navigation

← Previous Ticket
Next Ticket →

#56119 new defect (bug)

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

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

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
4 years 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
4 years 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
4 years 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 side effects.

Last edited 4 years ago by SergeyBiryukov (previous) (diff)

Note: See TracTickets for help on using tickets.

Download in other formats:

Make WordPress Core