thephpleague / oauth1-client

OAuth 1 Client
MIT License
968 stars 73 forks source link

Query strings with arrays cannot be properly signed #39

Closed nicktacular closed 8 years ago

nicktacular commented 8 years ago

I've encountered an issue whereby if you create a service that extends League\OAuth1\Client\Server\Server and use $this->getHeaders(...) with a query string that has array notation (i.e. var[]=whatever) then the URL cannot be properly signed.

Here's an example:

$this->getHeaders($this->tokenCredentials, 'GET', 'https://myservice.com/hello?a=1&b=2&c[]=3&c[]=4&d[a]=5&d[b]=6');

This generates a warning: Warning: rawurlencode() expects parameter 1 to be string, array given in /my/proj/vendor/league/oauth1-client/src/Client/Signature/HmacSha1Signature.php on line 66

Which means that the service is sending an invalid signature.

When I dug deeper, it looks like the method League\OAuth1\Client\Signature\HmacSha1Signature::baseString does not properly operate on such nested arrays. Specifically this line: $data[rawurlencode($key)] = rawurlencode($value);. The method rawurlencode doesn't know how to act on an array.

Is this a bug or did I miss another way to sign a URL here?

Client version: 1.6.1

php -a
Interactive shell

php > parse_str('a=1&b=2&c[]=3&c[]=4&d[a]=5&d[b]=6', $query);
php > var_dump($query);
array(4) {
  ["a"]=>
  string(1) "1"
  ["b"]=>
  string(1) "2"
  ["c"]=>
  array(2) {
    [0]=>
    string(1) "3"
    [1]=>
    string(1) "4"
  }
  ["d"]=>
  array(2) {
    ["a"]=>
    string(1) "5"
    ["b"]=>
    string(1) "6"
  }
}
php > foreach ($query as $key => $val) { var_dump(rawurlencode($key), rawurlencode($val)); }
string(1) "a"
string(1) "1"
string(1) "b"
string(1) "2"

Warning: rawurlencode() expects parameter 1 to be string, array given in php shell code on line 1
string(1) "c"
NULL

Warning: rawurlencode() expects parameter 1 to be string, array given in php shell code on line 1
string(1) "d"
NULL
php > 
stevenmaguire commented 8 years ago

@nicktacular Please help us understand a bit more about this issue by providing more information.

nicktacular commented 8 years ago

@stevenmaguire I responded with a few more details in #42 but I can provide more detail here.

Currently, I see there being 2 different, but related issues:

  1. The PHP method parse_str will parse [] as arrays in a query string, thereby resulting in a multi-dimensional array. The HmacSha1Signature should be aware of this and deal with this case in some way that does not issue a PHP warning about unexpected arrays: Warning: rawurlencode() expects parameter 1 to be string, array given
  2. Since OAuth spec is not specific on how to handle the issue of nested arrays, there should be either a) an implementation or b) a way to inject an implementation for a specific API.

I think we can solve in this manner. Create an interface called QueryParserInterface that allows you to override the method in which queries are parsed and sorted into a string. The default would be called DefaultQueryParser which uses parse_url and anyone that wants to modify this can contribute to an adapter implementation.

Thoughts?

stevenmaguire commented 8 years ago

Awesome, Thanks for providing this!

Can you add a bit more clarity to these responses?

The nature of the nested parameters is to pass an array-like structure to provide the necessary request parameters to the backend.

Are these parameters being used to create entities? to filter a query of existing entities?

I'm trying to make any request that uses a query string like a[b]=1&a[c]=2.

Can you provide a specific example of a request query string that is causing some problems?

I do think the two points you've made are valid, I am trying to discern the scope of the use case you are experiencing. Service providers implement OAuth (1 and 2!) inconsistently. I want to understand the instigator of this issue to research whether or not other providers, at least the ones I know of, are susceptible.

It is worth noting that another project had this same discussion and the root of the initial concern was more enlightening than the solution being proposed. https://github.com/woothemes/woocommerce/issues/7833

jtsternberg commented 8 years ago

Edit: Same issue here when passing a multidimensional array as the $bodyParameters argument to League\OAuth1\Client\Server::getHeaders().

nicktacular commented 8 years ago

@stevenmaguire - I've not had time to work on this. I will get to this sometime later this month. Thanks.

bencorlett commented 8 years ago

Hi guys, any updates on this?

bencorlett commented 8 years ago

Ignore previous comment ;)