thephpleague / oauth1-client

OAuth 1 Client
MIT License
968 stars 73 forks source link

Fix base string generation for array params #148

Open gauthierm opened 2 years ago

gauthierm commented 2 years ago

Problem

When duplicate params are passed (like ['test' => [ '123', '456' ]]), the signature should not add numeric indices to the generated base string.

The default behavior of EncodesUrl::baseString() is to parse the URL query string using parse_str. This works well for most cases, but in the case where clients repeat param names with different values (which is valid and accounted for in RFC 5849 Section 3.4.1.3.2 item 2) it does not work properly.

// Loses all but last value when duplicate params are included, signature generation will fail
parse_str('test=1234&test=5678', $params); // [ 'test' => '5678' ]

// Strips index from param name, indices need to be re-added during signature generation.
// One version of the other will fail. Currently, the first form can not be properly signed.
parse_str('test[]=1234&test[]=5678', $params); // [ 'test' => [ '1234', '5678' ] ]
parse_str('test[0]=1234&test[1]=5678', $params); // [ 'test' => [ '1234', '5678' ] ]

The Guzzle PSR-7 package provides Query::parse() that can handle query strings containing duplicate params. Calling code can strip the query string from the passed URL and the baseString() method can receive these as additional parameters:

// no information lost using Guzzle PSR-7
$params = Query::parse('test=1234&test=5678'); // [ 'test' => [ '1234', '5678' ] ]
$params = Query::parse('test[]=1234&test[]=5678'); // [ 'test[]' => [ '1234', '5678' ] ]
$params = Query::parse('test[0]=1234&test[1]=5678'); // [ 'test[0]' => '1234', 'test[1]' => '5678' ]

$oauthParams = ... ;
$queryParams = Query::parse($uri->getQuery(), PHP_QUERY_RFC3986);
$signature->sign((string) $uri->withQuery(''), array_merge($oauthParams, $queryParams), 'GET');

This should work, but the base string generation adds additional index numbers to the param name, causing signature generation to fail:

// correct
&test=1234&test=5678

// incorrect
&test[0]=1234&test[1]=5678

Solution

This PR changes the following:

  1. When additional params are passed, sequential arrays do not get bracket suffixes added to param names. This allows the existing parse_str to work as before, but also allows using another library (like Guzzle) to parse query params and pass them in to baseString().
  2. Params are sorted before being imploded. Sorting is done after the key-value pair strings are generated so it will sort first by param name and then by param value as per the RFC.

A new protected method paramsFromData() is added. The existing protected method queryStringFromData() is maintained, although it is now unused.

Background

Why does this matter? Multi-value parameters are an edge case, and for folks who can control all sides of the request signing, they can work within the confines of parse_str or modify requests to work. Folks attempting to integrate with producers/consumers using spec-compliant implementations written in other languages can't make these changes, and need this implementation to work per the RFC.