Make WordPress Core

Opened 13 years ago

Last modified 5 years ago

#17923 reopened defect (bug)

add_query_arg() should encode values

Reported by: viper007bond's profile Viper007Bond Owned by:
Milestone: Priority: normal
Severity: normal Version: 3.2
Component: Formatting Keywords: has-patch needs-refresh
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 13 years ago.
Optionally encode parameters to add_query_arg
17923_unit_test.patch (3.4 KB) - added by kurtpayne 13 years ago.
Unit test for add_query_arg and remove_query_arg
17923.2.patch (1.1 KB) - added by kurtpayne 13 years ago.
s/foreach/array_map/
17923.3.patch (1.1 KB) - added by Viper007Bond 13 years ago.
This is what I meant, although whether it's better or not is debatable
17923.4.patch (1000 bytes) - added by kurtpayne 12 years ago.
Updated for 3.6
17923_unit_test.2.patch (3.4 KB) - added by kurtpayne 12 years ago.
Updated for new phpunit framework
17923.5.patch (1000 bytes) - added by kurtpayne 12 years ago.
Use 'empty' instead of a silence operator

Download all attachments as: .zip

Change History (33)

#1 @Viper007Bond
13 years ago

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

#2 follow-up: @nacin
13 years ago

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

#3 in reply to: ↑ 2 @Viper007Bond
13 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.

#4 @azaozz
13 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']) ) )

#5 @dd32
13 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..

#6 @Viper007Bond
13 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 );

@kurtpayne
13 years ago

Optionally encode parameters to add_query_arg

@kurtpayne
13 years ago

Unit test for add_query_arg and remove_query_arg

#8 @kurtpayne
13 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.

#9 follow-up: @Viper007Bond
13 years ago

Sweet. :)

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

@kurtpayne
13 years ago

s/foreach/array_map/

#10 in reply to: ↑ 9 @kurtpayne
13 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.

#11 follow-up: @Viper007Bond
13 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

#12 in reply to: ↑ 11 @kurtpayne
13 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.

#13 @Viper007Bond
13 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.

@Viper007Bond
13 years ago

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

#14 @kurtpayne
13 years ago

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

#15 @bpetty
12 years ago

  • Keywords needs-patch removed

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

#16 @greenshady
12 years ago

  • Cc justin@… added

@kurtpayne
12 years ago

Updated for 3.6

@kurtpayne
12 years ago

Updated for new phpunit framework

#17 @kurtpayne
12 years ago

  • Keywords 3.6-early added

#18 follow-up: @Viper007Bond
12 years 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] ) )

@kurtpayne
12 years ago

Use 'empty' instead of a silence operator

#19 in reply to: ↑ 18 @kurtpayne
12 years ago

Updated patch to use empty instead of silence operator.

#20 @phill_brown
11 years ago

  • Cc phill@… added

#21 @DeanMarkTaylor
11 years 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 11 years ago by DeanMarkTaylor (previous) (diff)

#22 @Viper007Bond
10 years 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 10 years ago by Viper007Bond (previous) (diff)

#23 @chriscct7
9 years ago

  • Keywords 3.6-early removed
  • Milestone Future Release deleted
  • Resolution set to wontfix
  • Status changed from new to closed

Per https://make.wordpress.org/plugins/2015/04/20/fixing-add_query_arg-and-remove_query_arg-usage/ permalinks should be run through esc_url() before being printed to the page. The new function isn't a bad idea but should be discussed in a new ticket. Closing as wontfix.

#24 @Viper007Bond
9 years ago

  • Milestone set to Future Release
  • Resolution wontfix deleted
  • Status changed from closed to reopened

Escaping the output of add_query_arg() is completely unrelated to this issue. :)

Here's a demo:

echo '<a href="' . esc_url( add_query_arg( 'message', 'Make sure you do A & B!' ) ) . '">clicky</a>';
var_dump( $_GET );

Load that up and click the link. It doesn't work like you'd expect:

array (size=2)
  'message' => string 'MakesureyoudoA' (length=14)
  'B!' => string '' (length=0)

#25 @johnbillion
9 years ago

  • Keywords needs-refresh added

I'd be interested to know what the following code in add_query_arg() does. It appears to encode the URL's query arguments, but evidently doesn't:

wp_parse_str( $query, $qs );
$qs = urlencode_deep( $qs ); // this re-URL-encodes things that were already in the query string

#26 @SergeyBiryukov
5 years ago

  • Component changed from General to Formatting

Related: #46966

Note: See TracTickets for help on using tickets.