WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 2 months ago

#17923 new defect (bug)

add_query_arg() should encode values

Reported by: Viper007Bond Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 3.2
Component: General Keywords: has-patch 3.6-early
Focuses: Cc:

Description

One (or me at least) would expect that the result of

add_query_arg( 'foobar', 'this&that', 'index.php' )

would be

index.php?foobar=this%26that

since the whole purpose of the function is to build a URL. However the actual result is

index.php?foobar=this&that

You're asking to the function to create a URL in which foobar is this&that but instead it creates a URL in which foobar is set to only this. You shouldn't have to pre-encode values -- the function should take care of it for you.

The function to "blame" for this is our build_query() which for some reason does not encode by default.

Semi-related: #16943

Attachments (7)

17923.patch (1.2 KB) - added by kurtpayne 2 years ago.
Optionally encode parameters to add_query_arg
17923_unit_test.patch (3.4 KB) - added by kurtpayne 2 years ago.
Unit test for add_query_arg and remove_query_arg
17923.2.patch (1.1 KB) - added by kurtpayne 2 years ago.
s/foreach/array_map/
17923.3.patch (1.1 KB) - added by Viper007Bond 2 years ago.
This is what I meant, although whether it's better or not is debatable
17923.4.patch (1000 bytes) - added by kurtpayne 15 months ago.
Updated for 3.6
17923_unit_test.2.patch (3.4 KB) - added by kurtpayne 15 months ago.
Updated for new phpunit framework
17923.5.patch (1000 bytes) - added by kurtpayne 15 months ago.
Use 'empty' instead of a silence operator

Download all attachments as: .zip

Change History (29)

comment:1 Viper007Bond3 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release
  • Version set to 3.2

comment:2 follow-up: nacin3 years ago

I imagine this might break some things. Probably a lot of things.

comment:3 in reply to: ↑ 2 Viper007Bond3 years ago

Replying to nacin:

I imagine this might break some things. Probably a lot of things.

Specifically where things are already being pre-encoded using (raw)urlencode()? I thought of that -- we need a rawurlencode() that doesn't re-encode or something.

comment:4 azaozz3 years ago

Perhaps better to leave the urlencoding to the function that uses add_query_arg() as having & or = in the name or value is uncommon. Example:

add_query_arg( '_wp_http_referer', urlencode( stripslashes($_SERVER['REQUEST_URI']) ) )

comment:5 dd322 years ago

I've run into this many times, caused a few core bugs due to me expecting it to encode too.
Unfortunately, as pointed out, changing this would lead to pretty much every case of people who are using it correctly breaking..

comment:6 Viper007Bond2 years ago

Then perhaps a new arg for the function could be added? I end up doing this before I call it usually and it's annoying to have to do.

$args = array_map( 'rawurlencode', $args );

kurtpayne2 years ago

Optionally encode parameters to add_query_arg

kurtpayne2 years ago

Unit test for add_query_arg and remove_query_arg

comment:8 kurtpayne2 years ago

  • Cc kpayne@… added
  • Keywords has-patch added

17923.patch doesn't affect current unit tests. The same tests pass / fail before and after the patch. Light testing of the admin / front-end show no change in behavior.

comment:9 follow-up: Viper007Bond2 years ago

Sweet. :)

Rather than doing a foreach() though, array_map() is easier (see comment 6).

kurtpayne2 years ago

s/foreach/array_map/

comment:10 in reply to: ↑ 9 kurtpayne2 years ago

Replying to Viper007Bond:

Sweet. :)

Rather than doing a foreach() though, array_map() is easier (see comment 6).

TIL that array_map preserves key/value relationships.

comment:11 follow-up: Viper007Bond2 years ago

Just noticed something else -- why the change in methods between the array vs strings?

For arrays, you leave it where the value gets assigned to a variable and then check to see if encoding is enabled and if so, you then encode the value.

However for strings, you do both at once.

In short, wouldn't this be better?

(first line already exists in core, second and third lines would be new)

$qs[func_get_arg( 0 )] = func_get_arg( 1 );
if ( true === @func_get_arg( 3 ) )
	$qs[func_get_arg( 0 )] = rawurlencode( $qs[func_get_arg( 0 )] );

Wow, I am sure being nitpicky today. Sorry. lol

comment:12 in reply to: ↑ 11 kurtpayne2 years ago

Replying to Viper007Bond:

Just noticed something else -- why the change in methods between the array vs strings?

For arrays, you leave it where the value gets assigned to a variable and then check to see if encoding is enabled and if so, you then encode the value.

However for strings, you do both at once.

In short, wouldn't this be better?

(first line already exists in core, second and third lines would be new)

$qs[func_get_arg( 0 )] = func_get_arg( 1 );
if ( true === @func_get_arg( 3 ) )
	$qs[func_get_arg( 0 )] = rawurlencode( $qs[func_get_arg( 0 )] );

Wow, I am sure being nitpicky today. Sorry. lol

Very valid question, actually. The function should only encode new arguments. Anything that's currently in the URL may already be encoded and should be left alone. If the $qs array is encoded after it's split, things may be re-encoded and yield funny results. Example:

$url = '/wordpress/';
$url = add_query_arg('a', 'b&c', $url, true);  # /wordpress/?a=b%26c
$url = add_query_arg('a', 'b&c', $url, true);  # /wordpress/?a=b%2526c

As you may suspect, I found this out the hard way.

comment:13 Viper007Bond2 years ago

Check out my code again. ;) I wasn't suggesting running the whole $qs array through rawurlencode(), just the new arg.

I was suggesting a code simplification by setting the element in the array regardless and then if encoding is enabled, going back and encoding it, much like is done with the array format.

Viper007Bond2 years ago

This is what I meant, although whether it's better or not is debatable

comment:14 kurtpayne2 years ago

Viper007Bond, your patch looks good. I understand now. Sorry for the miscommunication. Thanks for the attention to detail!

comment:15 bpetty20 months ago

  • Keywords needs-patch removed

Patch has been attached, just removing "needs-patch" keyword.

comment:16 greenshady15 months ago

  • Cc justin@… added

kurtpayne15 months ago

Updated for 3.6

kurtpayne15 months ago

Updated for new phpunit framework

comment:17 kurtpayne15 months ago

  • Keywords 3.6-early added

comment:18 follow-up: Viper007Bond15 months ago

Silencing errors via @ should be avoided if possible. Since it's a variable now, instead of this:

if ( true === @$args[3] ) 

We can just do this:

if ( ! empty( $args[3] ) )

kurtpayne15 months ago

Use 'empty' instead of a silence operator

comment:19 in reply to: ↑ 18 kurtpayne15 months ago

Updated patch to use empty instead of silence operator.

comment:20 phill_brown14 months ago

  • Cc phill@… added

comment:21 DeanMarkTaylor9 months ago

  • Cc deanmarktaylor@… added

Both keys and values should be encoded, perhaps something like:

$kayvees = array_combine(
	array_map( 'rawurlencode', array_keys( $kayvees ) ),
	array_map( 'rawurlencode', array_values( $kayvees ) )
);

Perhaps WP::build_query_string needs reviewing too regarding encoding keys?

Last edited 9 months ago by DeanMarkTaylor (previous) (diff)

comment:22 Viper007Bond2 months ago

I keep running into this issue, especially when trying to improve code security. You have to be really careful with this function if you don't want to create bad stuff.

Maybe a better solution is to just introduce a new function, such as add_encoded_query_arg() that encodes and then calls add_query_arg(). That way we don't have to worry about passing yet another argument, especially when we would otherwise only pass 2 arguments. This also has the advantage of being able to deprecate add_query_arg() down the road if we so wish.

Last edited 2 months ago by Viper007Bond (previous) (diff)
Note: See TracTickets for help on using tickets.