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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Query strings with arrays cannot be properly signed #39

Closed
nicktacular opened this issue Nov 16, 2015 · 7 comments
Closed

Query strings with arrays cannot be properly signed #39

nicktacular opened this issue Nov 16, 2015 · 7 comments

Comments

@nicktacular
Copy link

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
Copy link
Member

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

  • Who is the service provider you are attempting to authorize? Did you build it? Does it provide documentation?
  • What is the nature of the nested parameters?
  • What action are you trying to take?

@nicktacular
Copy link
Author

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

  • The specific API I'm working with is https://oauth-api.beatport.com/ - I did not build this but I do work for Beatport as a software engineer.
  • The nature of the nested parameters is to pass an array-like structure to provide the necessary request parameters to the backend.
  • I'm trying to make any request that uses a query string like a[b]=1&a[c]=2.

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
Copy link
Member

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. woocommerce/woocommerce#7833

@jtsternberg
Copy link
Contributor

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

@nicktacular
Copy link
Author

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

jtsternberg added a commit to jtsternberg/oauth1-client that referenced this issue Feb 9, 2016
jtsternberg added a commit to jtsternberg/oauth1-client that referenced this issue Feb 10, 2016
@bencorlett
Copy link
Member

Hi guys, any updates on this?

@bencorlett
Copy link
Member

Ignore previous comment ;)

bencorlett added a commit that referenced this issue Jul 5, 2016
Allow mult-dimension arrays when generating base string for HMAC-SHA1 signature. Fixes #39
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 a pull request may close this issue.

4 participants