Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Robust query string encoding #129

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

duncanjbrown
Copy link
Contributor

The plugin previously relied on a hand-rolled function that tried to do its own urlencoding.

This meant it broke on params with special chars like [] (issue #34).

A fix was previously proposed by @thiago-negri at #79. This fixed signatures for one-dimensional arrays like filter[day]=10 but did not work for 2D arrays like filter[post__not_in][]=100&filter[post__not_in][]=200.

This commit moves query string encoding to the standard php function http_build_query. It also double-encodes the generated query because it has to, to work with the rest of the plugin. I wonder if #65 may fix that?

I have some tests for this function鈥攚ill attach if you give me somewhere to put them 馃檹.

@thiago-negri
Copy link
Contributor

Awesome. :)

Would you mind to change code a bit just to follow calling convention of the rest of the code?

Parameters should have a space to separate from parens.

http_build_query( $params );
preg_replace( '/%5B[0-9]+%5D/simU', '%5B%5D', $query );

Also, it would greatly help future developers if you could extend the "double-encode" comment giving a reason to why it has to do it.

This previously relied on a hand-rolled function that tried to do
its own urlencoding.

This commit moves query string encoding to the standard function
http_build_query.
@duncanjbrown
Copy link
Contributor Author

Updated

@kosso
Copy link
Contributor

kosso commented Apr 9, 2016

馃憤
Thanks!
This fixed my issue when using &filter[cat]=n

@kosso
Copy link
Contributor

kosso commented Apr 10, 2016

Hmm.. however it seems to have broken media uploads. (I will double-check how my client is doing things, but it was working fine with the previous method. )

@kosso
Copy link
Contributor

kosso commented Apr 10, 2016

update: OK. It appears my client is fine. Debugging this method compared to the existing methods shows some double-encoding of spaces in the text.

eg: Let's say I'm uploading a media file with the caption uploaded by kosso

working: existing create_signature_string() method:

caption%3Duploaded%2520by%2520kosso%26oauth_consumer_key%3D...

not working: proposed create_signature_string() method (notice double encoding of percent symbol from %20 spaces)

caption%3Duploaded%252520by%252520kosso%26oauth_consumer_key%3D...

So, I found that adding:

$replaced = preg_replace( '/%25/simU', '%', $replaced );

..seems to fix it for all my requests.

giving:

public function create_signature_string( $params ) {
    $query = http_build_query( $params );
    // http_build_query will attach numeric indices for array values, eg
    // filter[post__not_in][0]=1 instead of filter[post__not_in][]=1.
    //
    // Clients issue requests in the form filter[post__not_in][]=1 so
    // we should compare against that. This regex will strip out
    // the numeric indices.
    //
    // cf. http://php.net/manual/en/function.http-build-query.php
    // cf. http://stackoverflow.com/a/11996686/751089
    $replaced = preg_replace( '/%5B[0-9]+%5D/simU', '%5B%5D', $query );
    // http_build_query has urlencoded the parameters, but our calling function
    // expects a double-encoded return value
    // replace encoded percent symbols. 
    $replaced = preg_replace( '/%25/simU', '%', $replaced );
    return urlencode( $replaced );
}

@duncanjbrown
Copy link
Contributor Author

Thanks @kosso. I'm afraid I'm away this week so can't reply properly - sorry! Don't mind if you want to re-raise with commits of your own.

@rachelbaker, would you be open to a PR adding unit tests to the plugin? Then all could be robust and reliable 馃檪

@francoisjacquet
Copy link

francoisjacquet commented Jan 19, 2017

Had a similar issue with the Wordpress Example OAuth Client which uses (an old version... of) https://github.com/thephpleague/oauth1-client/, see this issue about nested arrays.

Just wanted to say that to fix the brackets encoding problem, simply changing class-wp-rest-oauth1.php line 736 to:

$param_key = $key . '[' . $param_key . ']'; // Handle multi-dimensional array

did the trick :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants