From 2ace51c3a18597f2f27fdef855a51ca0b338247a Mon Sep 17 00:00:00 2001 From: Matthieu Napoli Date: Wed, 31 Jan 2024 12:38:12 +0100 Subject: [PATCH 1/4] Add test case to reproduce invalid body parsing with malformed body --- tests/Event/Http/CommonHttpTest.php | 18 +++++++ ...-body-form-multipart-arrays-malformed.json | 53 +++++++++++++++++++ ...-body-form-multipart-arrays-malformed.json | 41 ++++++++++++++ 3 files changed, 112 insertions(+) create mode 100644 tests/Event/Http/Fixture/ag-v1-body-form-multipart-arrays-malformed.json create mode 100644 tests/Event/Http/Fixture/ag-v2-body-form-multipart-arrays-malformed.json diff --git a/tests/Event/Http/CommonHttpTest.php b/tests/Event/Http/CommonHttpTest.php index b6cc0e34e..63fd9b4ed 100644 --- a/tests/Event/Http/CommonHttpTest.php +++ b/tests/Event/Http/CommonHttpTest.php @@ -357,6 +357,24 @@ public function test POST request with multipart form data containing ar ]); } + /** + * @dataProvider provide API Gateway versions + */ + public function test POST request with malformed multipart form data(int $version) + { + $this->fromFixture(__DIR__ . "/Fixture/ag-v$version-body-form-multipart-arrays-malformed.json"); + + $this->assertContentType('multipart/form-data; boundary=testBoundary'); + $body = "--testBoundary\r +Content-Disposition: form-data; name=\"key0[key1][key2][\"\r +\r +123\r +--testBoundary--\r +"; + $this->assertBody($body); + $this->assertParsedBody(['key0' => ['key1' => ['key2' => '123']]]); + } + /** * @dataProvider provide API Gateway versions */ diff --git a/tests/Event/Http/Fixture/ag-v1-body-form-multipart-arrays-malformed.json b/tests/Event/Http/Fixture/ag-v1-body-form-multipart-arrays-malformed.json new file mode 100644 index 000000000..c5fb11bb2 --- /dev/null +++ b/tests/Event/Http/Fixture/ag-v1-body-form-multipart-arrays-malformed.json @@ -0,0 +1,53 @@ +{ + "version": "1.0", + "resource": "/path", + "path": "/path", + "httpMethod": "POST", + "headers": { + "Accept": "*/*", + "Accept-Encoding": "gzip, deflate", + "Cache-Control": "no-cache", + "Content-Type": "multipart/form-data; boundary=testBoundary", + "Host": "example.org", + "User-Agent": "PostmanRuntime/7.20.1", + "X-Amzn-Trace-Id": "Root=1-ffffffff-ffffffffffffffffffffffff", + "X-Forwarded-For": "1.1.1.1", + "X-Forwarded-Port": "443", + "X-Forwarded-Proto": "https" + }, + "queryStringParameters": null, + "pathParameters": null, + "stageVariables": null, + "requestContext": { + "resourceId": "xxxxxx", + "resourcePath": "/path", + "httpMethod": "PUT", + "extendedRequestId": "XXXXXX-xxxxxxxx=", + "requestTime": "24/Nov/2019:18:55:08 +0000", + "path": "/path", + "accountId": "123400000000", + "protocol": "HTTP/1.1", + "stage": "dev", + "domainPrefix": "dev", + "requestTimeEpoch": 1574621708700, + "requestId": "ffffffff-ffff-4fff-ffff-ffffffffffff", + "identity": { + "cognitoIdentityPoolId": null, + "accountId": null, + "cognitoIdentityId": null, + "caller": null, + "sourceIp": "1.1.1.1", + "principalOrgId": null, + "accessKey": null, + "cognitoAuthenticationType": null, + "cognitoAuthenticationProvider": null, + "userArn": null, + "userAgent": "PostmanRuntime/7.20.1", + "user": null + }, + "domainName": "example.org", + "apiId": "xxxxxxxxxx" + }, + "body": "--testBoundary\r\nContent-Disposition: form-data; name=\"key0[key1][key2][\"\r\n\r\n123\r\n--testBoundary--\r\n", + "isBase64Encoded": false +} diff --git a/tests/Event/Http/Fixture/ag-v2-body-form-multipart-arrays-malformed.json b/tests/Event/Http/Fixture/ag-v2-body-form-multipart-arrays-malformed.json new file mode 100644 index 000000000..67209fbb6 --- /dev/null +++ b/tests/Event/Http/Fixture/ag-v2-body-form-multipart-arrays-malformed.json @@ -0,0 +1,41 @@ +{ + "version": "2.0", + "routeKey": "ANY /path", + "rawPath": "/path", + "rawQueryString": "", + "headers": { + "Accept": "*/*", + "Accept-Encoding": "gzip, deflate", + "Cache-Control": "no-cache", + "Content-Type": "multipart/form-data; boundary=testBoundary", + "Host": "example.org", + "User-Agent": "PostmanRuntime/7.20.1", + "X-Amzn-Trace-Id": "Root=1-ffffffff-ffffffffffffffffffffffff", + "X-Forwarded-For": "1.1.1.1", + "X-Forwarded-Port": "443", + "X-Forwarded-Proto": "https" + }, + "queryStringParameters": null, + "stageVariables": null, + "requestContext": { + "accountId": "123400000000", + "apiId": "xxxxxxxxxx", + "domainName": "example.org", + "domainPrefix": "0000000000", + "http": { + "method": "POST", + "path": "/path", + "protocol": "HTTP/1.1", + "sourceIp": "1.1.1.1", + "userAgent": "PostmanRuntime/7.20.1" + }, + "requestId": "JTHoQgr2oAMEPMg=", + "routeId": "47matwk", + "routeKey": "ANY /path", + "stage": "$default", + "time": "24/Nov/2019:18:55:08 +0000", + "timeEpoch": 1574621708700 + }, + "body": "--testBoundary\r\nContent-Disposition: form-data; name=\"key0[key1][key2][\"\r\n\r\n123\r\n--testBoundary--\r\n", + "isBase64Encoded": false +} From 350788de12880b6fd64c4c318ba995388bec840e Mon Sep 17 00:00:00 2001 From: Matthieu Napoli Date: Wed, 31 Jan 2024 14:59:12 +0100 Subject: [PATCH 2/4] Support malformed multipart body For example body containing broken array keys like `key0[key1][key2][` --- src/Event/Http/Psr7Bridge.php | 59 ++++++++++++++--------- tests/Event/Http/CommonHttpTest.php | 4 +- tests/Event/Http/HttpRequestEventTest.php | 2 +- tests/Event/Http/Psr7BridgeTest.php | 2 +- 4 files changed, 40 insertions(+), 27 deletions(-) diff --git a/src/Event/Http/Psr7Bridge.php b/src/Event/Http/Psr7Bridge.php index 667282016..7694fdfd4 100644 --- a/src/Event/Http/Psr7Bridge.php +++ b/src/Event/Http/Psr7Bridge.php @@ -93,36 +93,49 @@ public static function convertResponse(ResponseInterface $response): HttpRespons return new HttpResponse($body, $response->getHeaders(), $response->getStatusCode()); } + /** + * @return array{0: array, 1: array|null} + */ private static function parseBodyAndUploadedFiles(HttpRequestEvent $event): array { - $bodyString = $event->getBody(); - $files = []; - $parsedBody = null; $contentType = $event->getContentType(); - if ($contentType !== null && $event->getMethod() === 'POST') { - if (str_starts_with($contentType, 'application/x-www-form-urlencoded')) { - parse_str($bodyString, $parsedBody); - } else { - $document = new Part("Content-type: $contentType\r\n\r\n" . $bodyString); - if ($document->isMultiPart()) { - $parsedBody = []; - foreach ($document->getParts() as $part) { - if ($part->isFile()) { + if ($contentType === null || $event->getMethod() !== 'POST') { + return [[], null]; + } + + if (str_starts_with($contentType, 'application/x-www-form-urlencoded')) { + $parsedBody = []; + parse_str($event->getBody(), $parsedBody); + return [[], $parsedBody]; + } + + // Parse the body as multipart/form-data + $document = new Part("Content-type: $contentType\r\n\r\n" . $event->getBody()); + if (!$document->isMultiPart()) { + return [[], null]; + } + $files = []; + $queryString = ''; + foreach ($document->getParts() as $part) { + if ($part->isFile()) { $tmpPath = tempnam(sys_get_temp_dir(), self::UPLOADED_FILES_PREFIX); - if ($tmpPath === false) { - throw new RuntimeException('Unable to create a temporary directory'); - } - file_put_contents($tmpPath, $part->getBody()); - $file = new UploadedFile($tmpPath, filesize($tmpPath), UPLOAD_ERR_OK, $part->getFileName(), $part->getMimeType()); - - self::parseKeyAndInsertValueInArray($files, $part->getName(), $file); - } else { - self::parseKeyAndInsertValueInArray($parsedBody, $part->getName(), $part->getBody()); - } - } + if ($tmpPath === false) { + throw new RuntimeException('Unable to create a temporary directory'); } + file_put_contents($tmpPath, $part->getBody()); + $file = new UploadedFile($tmpPath, filesize($tmpPath), UPLOAD_ERR_OK, $part->getFileName(), $part->getMimeType()); + self::parseKeyAndInsertValueInArray($files, $part->getName(), $file); + } else { + // Temporarily store as a query string so that we can use PHP's native parse_str function to parse keys + $queryString .= urlencode($part->getName()) . '=' . urlencode($part->getBody()) . '&'; } } + if ($queryString !== '') { + $parsedBody = []; + parse_str($queryString, $parsedBody); + } else { + $parsedBody = null; + } return [$files, $parsedBody]; } diff --git a/tests/Event/Http/CommonHttpTest.php b/tests/Event/Http/CommonHttpTest.php index 63fd9b4ed..737653633 100644 --- a/tests/Event/Http/CommonHttpTest.php +++ b/tests/Event/Http/CommonHttpTest.php @@ -401,7 +401,7 @@ public function test POST request with multipart file uploads(int $version --testBoundary--\r "; $this->assertBody($body); - $this->assertParsedBody([]); + $this->assertParsedBody(null); $this->assertUploadedFile( 'foo', 'lorem.txt', @@ -554,7 +554,7 @@ abstract protected function assertUri(string $expected): void; abstract protected function assertHasMultiHeader(bool $expected): void; - abstract protected function assertParsedBody(array $expected): void; + abstract protected function assertParsedBody(array|null $expected): void; abstract protected function assertSourceIp(string $expected): void; diff --git a/tests/Event/Http/HttpRequestEventTest.php b/tests/Event/Http/HttpRequestEventTest.php index d1f2f516d..74259e12f 100644 --- a/tests/Event/Http/HttpRequestEventTest.php +++ b/tests/Event/Http/HttpRequestEventTest.php @@ -112,7 +112,7 @@ protected function assertSourceIp(string $expected): void $this->assertEquals($expected, $this->event->getSourceIp()); } - protected function assertParsedBody(array $expected): void + protected function assertParsedBody(array|null $expected): void { // Not applicable here since the class doesn't parse the body } diff --git a/tests/Event/Http/Psr7BridgeTest.php b/tests/Event/Http/Psr7BridgeTest.php index e2ab8de1a..d60a69eb9 100644 --- a/tests/Event/Http/Psr7BridgeTest.php +++ b/tests/Event/Http/Psr7BridgeTest.php @@ -123,7 +123,7 @@ protected function assertHasMultiHeader(bool $expected): void // Not applicable here } - protected function assertParsedBody(array $expected): void + protected function assertParsedBody(array|null $expected): void { $this->assertEquals($expected, $this->request->getParsedBody()); } From 30a3e4745859de6c710816df513a6789b89a42b5 Mon Sep 17 00:00:00 2001 From: Matthieu Napoli Date: Wed, 31 Jan 2024 15:02:40 +0100 Subject: [PATCH 3/4] Fix code formatting --- src/Event/Http/Psr7Bridge.php | 2 +- tests/Event/Http/CommonHttpTest.php | 2 +- tests/Event/Http/HttpRequestEventTest.php | 2 +- tests/Event/Http/Psr7BridgeTest.php | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Event/Http/Psr7Bridge.php b/src/Event/Http/Psr7Bridge.php index 7694fdfd4..2021ae6e8 100644 --- a/src/Event/Http/Psr7Bridge.php +++ b/src/Event/Http/Psr7Bridge.php @@ -111,7 +111,7 @@ private static function parseBodyAndUploadedFiles(HttpRequestEvent $event): arra // Parse the body as multipart/form-data $document = new Part("Content-type: $contentType\r\n\r\n" . $event->getBody()); - if (!$document->isMultiPart()) { + if (! $document->isMultiPart()) { return [[], null]; } $files = []; diff --git a/tests/Event/Http/CommonHttpTest.php b/tests/Event/Http/CommonHttpTest.php index 737653633..1c4805901 100644 --- a/tests/Event/Http/CommonHttpTest.php +++ b/tests/Event/Http/CommonHttpTest.php @@ -554,7 +554,7 @@ abstract protected function assertUri(string $expected): void; abstract protected function assertHasMultiHeader(bool $expected): void; - abstract protected function assertParsedBody(array|null $expected): void; + abstract protected function assertParsedBody(array | null $expected): void; abstract protected function assertSourceIp(string $expected): void; diff --git a/tests/Event/Http/HttpRequestEventTest.php b/tests/Event/Http/HttpRequestEventTest.php index 74259e12f..925a324f9 100644 --- a/tests/Event/Http/HttpRequestEventTest.php +++ b/tests/Event/Http/HttpRequestEventTest.php @@ -112,7 +112,7 @@ protected function assertSourceIp(string $expected): void $this->assertEquals($expected, $this->event->getSourceIp()); } - protected function assertParsedBody(array|null $expected): void + protected function assertParsedBody(array | null $expected): void { // Not applicable here since the class doesn't parse the body } diff --git a/tests/Event/Http/Psr7BridgeTest.php b/tests/Event/Http/Psr7BridgeTest.php index d60a69eb9..a6d0f921c 100644 --- a/tests/Event/Http/Psr7BridgeTest.php +++ b/tests/Event/Http/Psr7BridgeTest.php @@ -123,7 +123,7 @@ protected function assertHasMultiHeader(bool $expected): void // Not applicable here } - protected function assertParsedBody(array|null $expected): void + protected function assertParsedBody(array | null $expected): void { $this->assertEquals($expected, $this->request->getParsedBody()); } From 56df95ef8039fddec7a005e0dfa277a8243ed793 Mon Sep 17 00:00:00 2001 From: Matthieu Napoli Date: Wed, 31 Jan 2024 17:32:09 +0100 Subject: [PATCH 4/4] Support malformed multipart body for uploaded files too --- src/Event/Http/Psr7Bridge.php | 60 ++++++++--------------------------- 1 file changed, 14 insertions(+), 46 deletions(-) diff --git a/src/Event/Http/Psr7Bridge.php b/src/Event/Http/Psr7Bridge.php index 2021ae6e8..1745fdd86 100644 --- a/src/Event/Http/Psr7Bridge.php +++ b/src/Event/Http/Psr7Bridge.php @@ -114,11 +114,11 @@ private static function parseBodyAndUploadedFiles(HttpRequestEvent $event): arra if (! $document->isMultiPart()) { return [[], null]; } + $parsedBody = null; $files = []; - $queryString = ''; foreach ($document->getParts() as $part) { if ($part->isFile()) { - $tmpPath = tempnam(sys_get_temp_dir(), self::UPLOADED_FILES_PREFIX); + $tmpPath = tempnam(sys_get_temp_dir(), self::UPLOADED_FILES_PREFIX); if ($tmpPath === false) { throw new RuntimeException('Unable to create a temporary directory'); } @@ -126,16 +126,12 @@ private static function parseBodyAndUploadedFiles(HttpRequestEvent $event): arra $file = new UploadedFile($tmpPath, filesize($tmpPath), UPLOAD_ERR_OK, $part->getFileName(), $part->getMimeType()); self::parseKeyAndInsertValueInArray($files, $part->getName(), $file); } else { - // Temporarily store as a query string so that we can use PHP's native parse_str function to parse keys - $queryString .= urlencode($part->getName()) . '=' . urlencode($part->getBody()) . '&'; + if ($parsedBody === null) { + $parsedBody = []; + } + self::parseKeyAndInsertValueInArray($parsedBody, $part->getName(), $part->getBody()); } } - if ($queryString !== '') { - $parsedBody = []; - parse_str($queryString, $parsedBody); - } else { - $parsedBody = null; - } return [$files, $parsedBody]; } @@ -144,42 +140,14 @@ private static function parseBodyAndUploadedFiles(HttpRequestEvent $event): arra */ private static function parseKeyAndInsertValueInArray(array &$array, string $key, mixed $value): void { - if (! str_contains($key, '[')) { - $array[$key] = $value; - - return; - } - - $parts = explode('[', $key); // files[id_cards][jpg][] => [ 'files', 'id_cards]', 'jpg]', ']' ] - $pointer = &$array; - - foreach ($parts as $k => $part) { - if ($k === 0) { - $pointer = &$pointer[$part]; - - continue; - } - - // Skip two special cases: - // [[ in the key produces empty string - // [test : starts with [ but does not end with ] - if ($part === '' || ! str_ends_with($part, ']')) { - // Malformed key, we use it "as is" - $array[$key] = $value; - - return; - } - - $part = substr($part, 0, -1); // The last char is a ] => remove it to have the real key - - if ($part === '') { // [] case - $pointer = &$pointer[]; - } else { - $pointer = &$pointer[$part]; - } - } - - $pointer = $value; + $parsed = []; + // We use parse_str to parse the key in the same way PHP does natively + // We use "=mock" because the value can be an object (in case of uploaded files) + parse_str(urlencode($key) . '=mock', $parsed); + // Replace `mock` with the actual value + array_walk_recursive($parsed, fn (&$v) => $v = $value); + // Merge recursively into the main array to avoid overwriting existing values + $array = array_merge_recursive($array, $parsed); } /**