From f4d5652875af4dffd5e35f1dcb71082eb5fe930e Mon Sep 17 00:00:00 2001 From: Aaron Weeden Date: Thu, 5 Oct 2023 20:03:07 -0400 Subject: [PATCH] Fix validation of REST array parameters. --- .../Controllers/BaseControllerProvider.php | 15 +- classes/Rest/XdmodApplicationFactory.php | 6 +- tests/integration/lib/BaseTest.php | 138 ++++++++++--- .../lib/Controllers/MetricExplorerTest.php | 118 +++++++++++ .../UserControllerProviderTest.php | 33 +++ .../AuthenticationControllerProviderTest.php | 44 ++++ .../Rest/DashboardControllerProviderTest.php | 63 ++++++ .../Rest/WarehouseControllerProviderTest.php | 189 +++++++++++++++++- .../WarehouseExportControllerProviderTest.php | 35 ++++ 9 files changed, 600 insertions(+), 41 deletions(-) create mode 100644 tests/integration/lib/Rest/AuthenticationControllerProviderTest.php diff --git a/classes/Rest/Controllers/BaseControllerProvider.php b/classes/Rest/Controllers/BaseControllerProvider.php index 55b50dfbb7..ac4cc50c4c 100644 --- a/classes/Rest/Controllers/BaseControllerProvider.php +++ b/classes/Rest/Controllers/BaseControllerProvider.php @@ -284,12 +284,25 @@ private function getParam(Request $request, $name, $mandatory, $default, $filter } } + // If the parameter is an array, throw an exception. + $invalidMessage = ( + "Invalid value for $name. Must be a(n) $expectedValueType." + ); + if (is_array($value)) { + throw new BadRequestHttpException($invalidMessage); + } + // Run the found parameter value through the given filter. + if (array_key_exists('flags', $filterOptions)) { + $filterOptions['flags'] |= FILTER_NULL_ON_FAILURE; + } else { + $filterOptions['flags'] = FILTER_NULL_ON_FAILURE; + } $value = filter_var($value, $filterId, $filterOptions); // If the value is invalid, throw an exception. if ($value === null) { - throw new BadRequestHttpException("Invalid value for $name. Must be a(n) $expectedValueType."); + throw new BadRequestHttpException($invalidMessage); } // Return the filtered value. diff --git a/classes/Rest/XdmodApplicationFactory.php b/classes/Rest/XdmodApplicationFactory.php index 77ee61d4ee..f91dc9056c 100644 --- a/classes/Rest/XdmodApplicationFactory.php +++ b/classes/Rest/XdmodApplicationFactory.php @@ -108,7 +108,11 @@ public static function getInstance() // Extracting any POST variables provided in the Request. $post = array(); foreach($request->request->getIterator() as $key => $value) { - $post[$key] = json_decode($value, true); + $post[$key] = ( + is_string($value) + ? json_decode($value, true) + : null + ); } // Calculate the amount of time that has elapsed serving this request. diff --git a/tests/integration/lib/BaseTest.php b/tests/integration/lib/BaseTest.php index e544f9d19f..b62c2bb9ea 100644 --- a/tests/integration/lib/BaseTest.php +++ b/tests/integration/lib/BaseTest.php @@ -331,6 +331,11 @@ protected static function assertRequiredKeys( * be present for other tests here to succeed. * - 'int_params' — array of parameters that * will each be tested for invalid integer values. + * - 'string_params' — array of parameters that + * will each be tested for invalid string values. + * - 'unix_ts_params' — array of parameters that + * will each be tested for invalid Unix timestamp + * values. * - 'date_params' — array of parameters that will * each be tested for invalid ISO 8601 date values. * @return array of arrays of test data, each of which contains a string @@ -361,16 +366,23 @@ protected function provideRestEndpointTests( $options['additional_params'] ); } + // Set up the custom error body validator. + $errorBodyValidator = null; + if (array_key_exists('error_body_validator', $options)) { + $errorBodyValidator = $options['error_body_validator']; + } $tests = []; $this->provideRestEndpointAuthenticationTests( $tests, $options, - $validInputWithAdditionalParams + $validInputWithAdditionalParams, + $errorBodyValidator ); $runAs = $this->provideRestEndpointAuthorizationTests( $tests, $options, - $validInputWithAdditionalParams + $validInputWithAdditionalParams, + $errorBodyValidator ); // Set the role for running the tests. if (!isset($runAs)) { @@ -394,7 +406,8 @@ protected function provideRestEndpointTests( $validInput, $paramSource, $runAs, - $tokenAuth + $tokenAuth, + $errorBodyValidator ); $this->provideRestEndpointInvalidParamTests( $tests, @@ -402,7 +415,8 @@ protected function provideRestEndpointTests( $validInputWithAdditionalParams, $paramSource, $runAs, - $tokenAuth + $tokenAuth, + $errorBodyValidator ); return $tests; } @@ -413,13 +427,18 @@ protected function provideRestEndpointTests( * the given name was not provided in the request. * * @param string $name + * @param callable|null $bodyValidator if provided, overrides the default + * body validator. * @return array */ - protected function validateMissingRequiredParameterResponse($name) - { + protected function validateMissingRequiredParameterResponse( + $name, + $bodyValidator = null + ) { return $this->validateBadRequestResponse( "$name is a required parameter.", - 0 + 0, + $bodyValidator ); } @@ -430,13 +449,19 @@ protected function validateMissingRequiredParameterResponse($name) * * @param string $name * @param string $type + * @param callable|null $bodyValidator if provided, overrides the default + * body validator. * @return array */ - protected function validateInvalidParameterResponse($name, $type) - { + protected function validateInvalidParameterResponse( + $name, + $type, + $bodyValidator = null + ) { return $this->validateBadRequestResponse( "Invalid value for $name. Must be a(n) $type.", - 0 + 0, + $bodyValidator ); } @@ -447,15 +472,21 @@ protected function validateInvalidParameterResponse($name, $type) * * @param string $message * @param int $code + * @param callable|null $bodyValidator if provided, overrides the default + * body validator. * @return array */ - protected function validateBadRequestResponse($message, $code) - { + protected function validateBadRequestResponse( + $message, + $code, + $bodyValidator = null + ) { return [ 'status_code' => 400, 'body_validator' => $this->validateErrorResponseBody( $message, - $code + $code, + $bodyValidator ) ]; } @@ -465,10 +496,14 @@ protected function validateBadRequestResponse($message, $code) * validates authorization error responses with the given HTTP status code. * * @param int $statusCode + * @param callable|null $bodyValidator if provided, overrides the default + * body validator. * @return array */ - protected function validateAuthorizationErrorResponse($statusCode) - { + protected function validateAuthorizationErrorResponse( + $statusCode, + $bodyValidator = null + ) { return [ 'status_code' => $statusCode, 'body_validator' => $this->validateErrorResponseBody( @@ -476,7 +511,8 @@ protected function validateAuthorizationErrorResponse($statusCode) 'An error was encountered while attempting to process the' . ' requested authorization procedure.' ), - 0 + 0, + $bodyValidator ) ]; } @@ -488,10 +524,18 @@ protected function validateAuthorizationErrorResponse($statusCode) * * @param string $message * @param int $code + * @param callable|null $bodyValidator if provided, overrides the default + * body validator. * @return callable */ - protected function validateErrorResponseBody($message, $code) - { + protected function validateErrorResponseBody( + $message, + $code, + $bodyValidator = null + ) { + if (!is_null($bodyValidator)) { + return $bodyValidator($message, $code); + } return function ($body, $assertMessage) use ($message, $code) { parent::assertEquals( [ @@ -662,7 +706,8 @@ private static function getEndpointTestData( private function provideRestEndpointAuthenticationTests( array &$tests, array $options, - array $validInputWithAdditionalParams + array $validInputWithAdditionalParams, + $errorBodyValidator ) { if ( array_key_exists('authentication', $options) @@ -672,7 +717,10 @@ private function provideRestEndpointAuthenticationTests( 'unauthenticated', 'pub', $validInputWithAdditionalParams, - $this->validateAuthorizationErrorResponse(401) + $this->validateAuthorizationErrorResponse( + 401, + $errorBodyValidator + ) ]; } } @@ -683,7 +731,8 @@ private function provideRestEndpointAuthenticationTests( private function provideRestEndpointAuthorizationTests( array &$tests, array $options, - array $validInputWithAdditionalParams + array $validInputWithAdditionalParams, + $errorBodyValidator ) { if (array_key_exists('authorization', $options)) { foreach (self::getBaseRoles() as $role) { @@ -692,7 +741,10 @@ private function provideRestEndpointAuthorizationTests( 'unauthorized', $role, $validInputWithAdditionalParams, - $this->validateAuthorizationErrorResponse(403) + $this->validateAuthorizationErrorResponse( + 403, + $errorBodyValidator + ) ]; } } @@ -733,7 +785,8 @@ private function provideRestEndpointMissingRequiredParamTests( array $validInput, $paramSource, $runAs, - $tokenAuth + $tokenAuth, + $errorBodyValidator ) { foreach (array_keys($validInput[$paramSource]) as $param) { $input = $validInput; @@ -743,7 +796,10 @@ private function provideRestEndpointMissingRequiredParamTests( $runAs, $tokenAuth, $input, - $this->validateMissingRequiredParameterResponse($param) + $this->validateMissingRequiredParameterResponse( + $param, + $errorBodyValidator + ) ); } } @@ -757,24 +813,40 @@ private function provideRestEndpointInvalidParamTests( array $validInputWithAdditionalParams, $paramSource, $runAs, - $tokenAuth + $tokenAuth, + $errorBodyValidator ) { $types = [ 'int_params' => 'integer', + 'string_params' => 'string', + 'unix_ts_params' => 'Unix timestamp', 'date_params' => 'ISO 8601 Date' ]; + $values = [ + 'string' => 'foo', + 'array' => ['foo' => 'bar'] + ]; foreach ($types as $key => $type) { if (array_key_exists($key, $options)) { foreach ($options[$key] as $param) { $input = $validInputWithAdditionalParams; - $input[$paramSource][$param] = 'foo'; - $tests[] = self::getEndpointTestData( - $param . '_string', - $runAs, - $tokenAuth, - $input, - $this->validateInvalidParameterResponse($param, $type) - ); + foreach ($values as $id => $value) { + // Strings can be strings, so skip that test. + if ('string_params' !== $key || 'string' !== $id) { + $input[$paramSource][$param] = $value; + $tests[] = self::getEndpointTestData( + $param . '_' . $id, + $runAs, + $tokenAuth, + $input, + $this->validateInvalidParameterResponse( + $param, + $type, + $errorBodyValidator + ) + ); + } + } } } } diff --git a/tests/integration/lib/Controllers/MetricExplorerTest.php b/tests/integration/lib/Controllers/MetricExplorerTest.php index 720e605003..a74011f48f 100644 --- a/tests/integration/lib/Controllers/MetricExplorerTest.php +++ b/tests/integration/lib/Controllers/MetricExplorerTest.php @@ -550,4 +550,122 @@ public function chartDataProvider() array($emptyChart) ); } + + /** + * @dataProvider provideCreateQueryParamValidation + */ + public function testCreateQueryParamValidation( + $id, + $role, + $input, + $output + ) { + parent::authenticateRequestAndValidateJson( + $this->helper, + $role, + $input, + $output + ); + } + + public function provideCreateQueryParamValidation() + { + $validInput = [ + 'path' => 'rest/metrics/explorer/queries', + 'method' => 'post', + 'params' => null, + 'data' => ['data' => 'foo'] + ]; + // Run some standard endpoint tests. + return parent::provideRestEndpointTests( + $validInput, + [ + 'authentication' => true, + 'string_params' => ['data'], + 'error_body_validator' => $this->validateQueryErrorBody( + 'creatQuery' + ) + ] + ); + } + + private function validateQueryErrorBody($action) + { + // This function is passed to parent::provideRestEndpointTests(). + return function ($message) use ($action) { + // This function is passed to parent::validateErrorResponseBody(). + return function ($body, $assertMessage) use ($message, $action) { + return parent::assertEquals( + [ + 'success' => false, + 'message' => $message, + 'action' => $action + ], + $body, + $assertMessage + ); + }; + }; + } + + /** + * @dataProvider provideUpdateQueryByIdParamValidation + */ + public function testUpdateQueryByIdParamValidation( + $id, + $role, + $input, + $output + ) { + // Get a query ID. + $chartSettings = $this->chartDataProvider()[0][0]; + $settings = [ + 'name' => 'test', + 'ts' => microtime(true), + 'config' => $chartSettings + ]; + $this->helper->authenticate('usr'); + $response = $this->helper->post( + 'rest/metrics/explorer/queries', + null, + ['data' => json_encode($settings)] + ); + $this->helper->logout(); + $id = $response[0]['data']['recordid']; + $path = "rest/metrics/explorer/queries/$id"; + $input['path'] .= $path; + // Run the test. + parent::authenticateRequestAndValidateJson( + $this->helper, + $role, + $input, + $output + ); + // Delete the query ID. + $this->helper->authenticate('usr'); + $this->helper->delete($path); + $this->helper->logout(); + } + + public function provideUpdateQueryByIdParamValidation() + { + $validInput = [ + 'path' => null, // set in provideUpdateQueryByIdParamValidation(). + 'method' => 'post', + 'params' => null, + 'data' => [] + ]; + // Run some standard endpoint tests. + return parent::provideRestEndpointTests( + $validInput, + [ + 'authentication' => true, + 'string_params' => ['data', 'name', 'config'], + 'unix_ts_params' => ['ts'], + 'error_body_validator' => $this->validateQueryErrorBody( + 'updateQuery' + ) + ] + ); + } } diff --git a/tests/integration/lib/Controllers/UserControllerProviderTest.php b/tests/integration/lib/Controllers/UserControllerProviderTest.php index 360dc91e26..f4fcc9d5f6 100644 --- a/tests/integration/lib/Controllers/UserControllerProviderTest.php +++ b/tests/integration/lib/Controllers/UserControllerProviderTest.php @@ -119,6 +119,39 @@ public function provideGetCurrentUser() return $tests; } + /** + * @dataProvider provideUpdateCurrentUser + */ + public function testUpdateCurrentUser($id, $role, $input, $output) + { + parent::authenticateRequestAndValidateJson( + $this->helper, + $role, + $input, + $output + ); + } + + public function provideUpdateCurrentUser() + { + $validInput = [ + 'path' => 'rest/users/current', + 'method' => 'patch', + 'params' => null, + 'data' => [] + ]; + // Run some standard endpoint tests. + return parent::provideRestEndpointTests( + $validInput, + ['string_params' => [ + 'first_name', + 'last_name', + 'email_address', + 'password' + ]] + ); + } + /** * Test expected successes and failures at creating, getting, and revoking * API tokens. diff --git a/tests/integration/lib/Rest/AuthenticationControllerProviderTest.php b/tests/integration/lib/Rest/AuthenticationControllerProviderTest.php new file mode 100644 index 0000000000..f247361eb0 --- /dev/null +++ b/tests/integration/lib/Rest/AuthenticationControllerProviderTest.php @@ -0,0 +1,44 @@ + 'rest/auth/idpredirect', + 'method' => 'get', + 'params' => [], + 'data' => null + ]; + // Run some standard endpoint tests. + return parent::provideRestEndpointTests( + $validInput, + ['string_params' => ['returnTo']] + ); + } +} diff --git a/tests/integration/lib/Rest/DashboardControllerProviderTest.php b/tests/integration/lib/Rest/DashboardControllerProviderTest.php index 9e6bc53372..d8027de806 100644 --- a/tests/integration/lib/Rest/DashboardControllerProviderTest.php +++ b/tests/integration/lib/Rest/DashboardControllerProviderTest.php @@ -7,6 +7,36 @@ class DashboardControllerProviderTest extends BaseUserAdminTest { + /** + * @dataProvider provideSetLayout + */ + public function testSetLayout($id, $role, $input, $output) + { + parent::authenticateRequestAndValidateJson( + $this->helper, + $role, + $input, + $output + ); + } + public function provideSetLayout() + { + $validInput = [ + 'path' => 'rest/dashboard/layout', + 'method' => 'post', + 'params' => null, + 'data' => ['data' => 'foo'] + ]; + // Run some standard endpoint tests. + return parent::provideRestEndpointTests( + $validInput, + [ + 'authentication' => true, + 'string_params' => ['data'] + ] + ); + } + /** * Exercises the `dashboard/statistics` REST endpoint. * @@ -121,6 +151,39 @@ public function provideTestGetStatistics() ); } + /** + * @dataProvider provideGetStatisticsParamValidation + */ + public function testGetStatisticsParamValidation( + $id, + $role, + $input, + $output + ) { + parent::requestAndValidateJson($this->helper, $input, $output); + } + + public function provideGetStatisticsParamValidation() + { + $validInput = [ + 'path' => 'rest/dashboard/statistics', + 'method' => 'get', + 'params' => [ + 'start_date' => 'foo', + 'end_date' => 'foo' + ], + 'data' => null + ]; + // Run some standard endpoint tests. + return parent::provideRestEndpointTests( + $validInput, + [ + 'run_as' => 'pub', + 'string_params' => ['start_date', 'end_date'] + ] + ); + } + private function recursivelyFilter(array $data, array $keys) { foreach ($data as $key => $value) { diff --git a/tests/integration/lib/Rest/WarehouseControllerProviderTest.php b/tests/integration/lib/Rest/WarehouseControllerProviderTest.php index ab12833bdb..3af0df127a 100644 --- a/tests/integration/lib/Rest/WarehouseControllerProviderTest.php +++ b/tests/integration/lib/Rest/WarehouseControllerProviderTest.php @@ -41,7 +41,8 @@ public function provideGetSearchHistory() $validInput, [ 'authentication' => true, - 'int_params' => ['nodeid', 'infoid', 'jobid', 'recordid'] + 'int_params' => ['nodeid', 'infoid', 'jobid', 'recordid'], + 'string_params' => ['tsid', 'realm', 'title'] ] ); } @@ -75,7 +76,135 @@ public function provideCreateSearchHistory() $validInput, [ 'authentication' => true, - 'int_params' => ['recordid'] + 'int_params' => ['recordid'], + 'string_params' => ['realm', 'data'] + ] + ); + } + + /** + * @dataProvider provideGetHistoryById + */ + public function testGetHistoryById($id, $role, $input, $output) + { + parent::authenticateRequestAndValidateJson( + self::$helper, + $role, + $input, + $output + ); + } + + public function provideGetHistoryById() + { + $validInput = [ + 'path' => 'rest/warehouse/search/history/0', + 'method' => 'get', + 'params' => ['realm' => 'Jobs'], + 'data' => null + ]; + // Run some standard endpoint tests. + return parent::provideRestEndpointTests( + $validInput, + [ + 'authentication' => true, + 'string_params' => ['realm'] + ] + ); + } + + /** + * @dataProvider provideUpdateHistory + */ + public function testUpdateHistory($id, $role, $input, $output) + { + parent::authenticateRequestAndValidateJson( + self::$helper, + $role, + $input, + $output + ); + } + + public function provideUpdateHistory() + { + $validInput = [ + 'path' => 'rest/warehouse/search/history/0', + 'method' => 'post', + 'params' => null, + 'data' => [ + 'realm' => 'Jobs', + 'data' => '{"text":"foo"}' + ] + ]; + // Run some standard endpoint tests. + return parent::provideRestEndpointTests( + $validInput, + [ + 'authentication' => true, + 'string_params' => ['realm', 'data'] + ] + ); + } + + /** + * @dataProvider provideDeleteHistory + */ + public function testDeleteHistory($id, $role, $input, $output) + { + parent::authenticateRequestAndValidateJson( + self::$helper, + $role, + $input, + $output + ); + } + + public function provideDeleteHistory() + { + $validInput = [ + 'path' => 'rest/warehouse/search/history/0', + 'method' => 'delete', + 'params' => ['realm' => 'Jobs'], + 'data' => null + ]; + // Run some standard endpoint tests. + return parent::provideRestEndpointTests( + $validInput, + [ + 'authentication' => true, + 'string_params' => ['realm'] + ] + ); + } + + /** + * @dataProvider provideDeleteAllHistory + */ + public function testDeleteAllHistory($id, $role, $input, $output) + { + parent::authenticateRequestAndValidateJson( + self::$helper, + $role, + $input, + $output + ); + } + + public function provideDeleteAllHistory() + { + $validInput = [ + 'path' => 'rest/warehouse/search/history', + 'method' => 'delete', + 'params' => ['realm' => 'Jobs'], + 'data' => null + ]; + // Run some standard endpoint tests. + return parent::provideRestEndpointTests( + $validInput, + [ + 'authentication' => true, + 'string_params' => ['realm'] ] ); } @@ -113,7 +242,13 @@ public function provideSearchJobs() $validInput, [ 'authentication' => true, - 'int_params' => ['start', 'limit'] + 'int_params' => ['start', 'limit'], + 'string_params' => [ + 'realm', + 'params', + 'start_date', + 'end_date' + ] ] ); } @@ -147,7 +282,8 @@ public function provideSearchJobsByPeers() $validInput, [ 'authentication' => true, - 'int_params' => ['jobid', 'start', 'limit'] + 'int_params' => ['jobid', 'start', 'limit'], + 'string_params' => ['realm'] ] ); } @@ -191,6 +327,13 @@ public function provideSearchJobsByTimeseries() 'width', 'height', 'font_size' + ], + 'string_params' => [ + 'realm', + 'tsid', + 'format', + 'scale', + 'show_title' ] ] ); @@ -267,7 +410,8 @@ public function aggregateDataMalformedRequestProvider() $validInput, [ 'authentication' => true, - 'int_params' => ['start', 'limit'] + 'int_params' => ['start', 'limit'], + 'string_params' => ['config'] ] ); // Test bad request parameters. @@ -397,6 +541,37 @@ public function testGetAggregateWithFilters() self::$helper->logout(); } + /** + * @dataProvider provideGetDimensions + */ + public function testGetDimensions($id, $role, $input, $output) + { + parent::authenticateRequestAndValidateJson( + self::$helper, + $role, + $input, + $output + ); + } + + public function provideGetDimensions() + { + $validInput = [ + 'path' => 'rest/warehouse/dimensions', + 'method' => 'get', + 'params' => [], + 'data' => null + ]; + // Run some standard endpoint tests. + return parent::provideRestEndpointTests( + $validInput, + [ + 'authentication' => true, + 'string_params' => ['realm'] + ] + ); + } + /** * @dataProvider provideGetDimensionValues */ @@ -423,7 +598,8 @@ public function provideGetDimensionValues() $validInput, [ 'authentication' => true, - 'int_params' => ['offset', 'limit'] + 'int_params' => ['offset', 'limit'], + 'string_params' => ['search_text', 'realm'] ] ); } @@ -464,6 +640,7 @@ public function provideGetRawData() [ 'token_auth' => true, 'int_params' => ['offset'], + 'string_params' => ['realm', 'fields'], 'date_params' => ['start_date', 'end_date'] ] ); diff --git a/tests/integration/lib/Rest/WarehouseExportControllerProviderTest.php b/tests/integration/lib/Rest/WarehouseExportControllerProviderTest.php index 54ed430f37..3151d527d8 100644 --- a/tests/integration/lib/Rest/WarehouseExportControllerProviderTest.php +++ b/tests/integration/lib/Rest/WarehouseExportControllerProviderTest.php @@ -265,6 +265,41 @@ public function testCreateRequest($role, array $params, $httpCode, $schema) $this->validateAgainstSchema($content, $schema); } + /** + * @dataProvider provideCreateRequestParamValidation + */ + public function testCreateRequestParamValidation( + $id, + $role, + $input, + $output + ) { + parent::requestAndValidateJson(self::$helpers[$role], $input, $output); + } + + public function provideCreateRequestParamValidation() + { + $validInput = [ + 'path' => 'rest/warehouse/export/request', + 'method' => 'post', + 'params' => null, + 'data' => [ + 'realm' => 'Jobs', + 'start_date' => '2017-01-01', + 'end_date' => '2017-01-01' + ] + ]; + // Run some standard endpoint tests. + return parent::provideRestEndpointTests( + $validInput, + [ + 'authentication' => true, + 'string_params' => ['realm', 'format'], + 'date_params' => ['start_date', 'end_date'] + ] + ); + } + /** * Test getting the list of export requests. *