WordPress.org

Make WordPress Core

Opened 5 years ago

Last modified 9 months ago

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

Download all attachments as: .zip

Change History (32)

#1 @Viper007Bond
5 years ago

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

#2 follow-up: @nacin
5 years ago

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

#3 in reply to: ↑ 2 @Viper007Bond
5 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
5 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
5 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
5 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
5 years ago

Optionally encode parameters to add_query_arg

@kurtpayne
5 years ago

Unit test for add_query_arg and remove_query_arg

#8 @kurtpayne
5 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
5 years ago

Sweet. :)

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

@kurtpayne
5 years ago

s/foreach/array_map/

#10 in reply to: ↑ 9 @kurtpayne
5 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
5 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
5 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
5 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
5 years ago

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

#14 @kurtpayne
5 years ago

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

#15 @bpetty
4 years ago

  • Keywords needs-patch removed

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

#16 @greenshady
4 years ago

  • Cc justin@… added

@kurtpayne
3 years ago

Updated for 3.6

@kurtpayne
3 years ago

Updated for new phpunit framework

#17 @kurtpayne
3 years ago

  • Keywords 3.6-early added

#18 follow-up: @Viper007Bond
3 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
3 years ago

Use 'empty' instead of a silence operator

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

Updated patch to use empty instead of silence operator.

#20 @phill_brown
3 years ago

  • Cc phill@… added

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

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

#23 @chriscct7
10 months 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 months 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 months 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
Note: See TracTickets for help on using tickets.