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

Fix base string generation for array params #148

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

gauthierm
Copy link

@gauthierm gauthierm commented Oct 22, 2022

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.

 - when duplicate params are passed (like ['test' => [ '123', '456' ]]), the signature should not add numeric indices to the base string
 - when duplicate params are passed, the values need to be sorted using string comparison
 - restore parse_str behavior when using a URI containing a query string
 - improve tests
@gauthierm gauthierm force-pushed the fix-signature-base-string branch from b433348 to 09a3f74 Compare October 22, 2022 01:43
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.

1 participant