diff --git a/src/Core/CHANGELOG.md b/src/Core/CHANGELOG.md index 603e36241..1966e9cf9 100644 --- a/src/Core/CHANGELOG.md +++ b/src/Core/CHANGELOG.md @@ -2,6 +2,10 @@ ## NOT RELEASED +### Fixed + +- SignerV4: fix sort of query parameters to build correct canoncal query string + ## 1.27.0 ### Added diff --git a/src/Core/phpunit.xml.dist b/src/Core/phpunit.xml.dist index 9894ce353..3185bb954 100644 --- a/src/Core/phpunit.xml.dist +++ b/src/Core/phpunit.xml.dist @@ -1,17 +1,5 @@ - - - - ./src - - + @@ -20,4 +8,9 @@ ./tests/ + + + ./src + + diff --git a/src/Core/src/Signer/SignerV4.php b/src/Core/src/Signer/SignerV4.php index 602999076..2d371de2a 100644 --- a/src/Core/src/Signer/SignerV4.php +++ b/src/Core/src/Signer/SignerV4.php @@ -321,7 +321,9 @@ private function buildCanonicalQuery(Request $request): string return ''; } - ksort($query); + uksort($query, static function (string $a, string $b): int { + return strcmp(rawurlencode($a), rawurlencode($b)); + }); $encodedQuery = []; foreach ($query as $key => $values) { if (!\is_array($values)) { diff --git a/src/Core/tests/Unit/Signer/SignerV4Test.php b/src/Core/tests/Unit/Signer/SignerV4Test.php index 495dfb70b..18acb9353 100644 --- a/src/Core/tests/Unit/Signer/SignerV4Test.php +++ b/src/Core/tests/Unit/Signer/SignerV4Test.php @@ -78,7 +78,7 @@ public function testSignsRequests($rawRequest, $rawExpected) self::assertEquals($expected, $request); } - public function provideRequests() + public static function provideRequests() { return [ // POST headers should be signed. @@ -140,7 +140,58 @@ public function provideRequests() ]; } - private function parseRequest(string $req): Request + /** + * @dataProvider provideRequestsWithQueryParams + */ + public function testSignsRequestsWithArrayInQueryParams($rawRequestWithoutQuery, $rawExpected, $queryParams) + { + $request = $this->parseRequest($rawRequestWithoutQuery, $queryParams); + + $signer = new SignerV4('host', 'us-east-1'); + $context = new RequestContext(['currentDate' => new \DateTimeImmutable('20110909T233600Z')]); + $credentials = new Credentials('AKIDEXAMPLE', 'wJalrXUtnFEMI/K7MDENG+bPxRfiCYEXAMPLEKEY'); + + $signer->sign($request, $credentials, $context); + + $expected = $this->parseRequest($rawExpected, $queryParams); + + self::assertEquals($expected, $request); + } + + public static function provideRequestsWithQueryParams() + { + return [ + // GET Case with array in query params + [ + "GET / HTTP/1.1\r\nHost: host.foo.com:443\r\n\r\n", + "GET / HTTP/1.1\r\nHost: host.foo.com:443\r\nX-Amz-Date: 20110909T233600Z\r\nAuthorization: AWS4-HMAC-SHA256 Credential=AKIDEXAMPLE/20110909/us-east-1/host/aws4_request, SignedHeaders=host;x-amz-date, Signature=7bd9c6fed0473be1f7ea96ff34c7d79e71a92932054a24b4210b3a56e4ab46d3\r\n\r\n", + [ + 'foos[0]' => '834127', + 'foos[1]' => '59', + 'foos[2]' => '90123', + 'foos[3]' => '4708', + 'foos[4]' => '120001', + 'foos[5]' => '333', + 'foos[6]' => '78005', + 'foos[7]' => '2', + 'foos[8]' => 'string value', + 'foos[9]' => '40617', + 'foos[10]' => '715', + ], + ], + // GET Simple case with query params (copy of one of the cases above from testSignsRequests) + [ + "GET / HTTP/1.1\r\nHost: host.foo.com:443\r\n\r\n", + "GET / HTTP/1.1\r\nHost: host.foo.com:443\r\nX-Amz-Date: 20110909T233600Z\r\nAuthorization: AWS4-HMAC-SHA256 Credential=AKIDEXAMPLE/20110909/us-east-1/host/aws4_request, SignedHeaders=host;x-amz-date, Signature=1c3274381ae12d8817336268d7da17672bd57e7348e39b7b9c567280f73742af\r\n\r\n", + [ + 'a' => 'foo', + 'b' => 'foo', + ], + ], + ]; + } + + private function parseRequest(string $req, array $queryParams = []): Request { $lines = explode("\r\n", $req); [$method, $path] = explode(' ', array_shift($lines)); @@ -156,6 +207,12 @@ private function parseRequest(string $req): Request $req = new Request($method, '/', [], $headers, StringStream::create(implode("\n", $lines))); $req->setEndpoint('https://' . $headers['Host'] . $path); + + // currently the library cannot properly parse query params if they contain arrays, so we need to set them manually + foreach ($queryParams as $k => $v) { + $req->setQueryAttribute($k, $v); + } + // Ensure that the memoized property is filled, so that comparison works consistently. $req->getEndpoint();