From 9990e6214c9b6c2ef7aa2902bfeee9c80f0a3c27 Mon Sep 17 00:00:00 2001 From: Christoph Wurst Date: Mon, 5 Dec 2022 11:13:49 +0100 Subject: [PATCH 1/4] chore(app framework)!: catch missing non-optional controller parameter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Missing controller parameters cause an unhandled TypeError that results in a generic HTTP500. By reflecting on the params more clever we can determine if a parameter can be filled by its default value or if the request is invalid. A missing parameter in a JSON request, for example, could now be answered by a HTTP422 – unprocessible entity. Signed-off-by: Christoph Wurst --- .../Exceptions/ParameterMissingException.php | 39 +++++++++++++++++++ lib/private/AppFramework/Http/Dispatcher.php | 33 ++++++++++++---- 2 files changed, 65 insertions(+), 7 deletions(-) create mode 100644 lib/private/AppFramework/Exceptions/ParameterMissingException.php diff --git a/lib/private/AppFramework/Exceptions/ParameterMissingException.php b/lib/private/AppFramework/Exceptions/ParameterMissingException.php new file mode 100644 index 0000000000000..ed498e54edb30 --- /dev/null +++ b/lib/private/AppFramework/Exceptions/ParameterMissingException.php @@ -0,0 +1,39 @@ + + * + * @author 2022 Christoph Wurst + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + */ + +namespace OC\AppFramework\Exceptions; + +use Exception; + +class ParameterMissingException extends Exception { + + public function __construct(string $controllerName, string $methodName, string $parameterName) { + parent::__construct("Parameter $parameterName missing for $controllerName::$methodName"); + $this->controllerName = $controllerName; + $this->methodName = $methodName; + $this->parameterName = $parameterName; + } + +} diff --git a/lib/private/AppFramework/Http/Dispatcher.php b/lib/private/AppFramework/Http/Dispatcher.php index c1a203a7165f8..0dba99da06c1a 100644 --- a/lib/private/AppFramework/Http/Dispatcher.php +++ b/lib/private/AppFramework/Http/Dispatcher.php @@ -32,6 +32,7 @@ */ namespace OC\AppFramework\Http; +use OC\AppFramework\Exceptions\ParameterMissingException; use OC\AppFramework\Http; use OC\AppFramework\Middleware\MiddlewareDispatcher; use OC\AppFramework\Utility\ControllerMethodReflector; @@ -43,6 +44,10 @@ use OCP\IConfig; use OCP\IRequest; use Psr\Log\LoggerInterface; +use ReflectionMethod; +use ReflectionNamedType; +use TypeError; +use function get_class; /** * Class to dispatch the request to the middleware dispatcher @@ -192,20 +197,30 @@ public function dispatch(Controller $controller, string $methodName): array { */ private function executeController(Controller $controller, string $methodName): Response { $arguments = []; + $reflection = new ReflectionMethod($controller, $methodName); // valid types that will be casted $types = ['int', 'integer', 'bool', 'boolean', 'float']; - foreach ($this->reflector->getParameters() as $param => $default) { + foreach ($reflection->getParameters() as $param) { + $paramName = $param->name; // try to get the parameter from the request object and cast - // it to the type annotated in the @param annotation - $value = $this->request->getParam($param, $default); - $type = $this->reflector->getType($param); + // it to the type annotated in the @paramName annotation + $defaultValue = null; + if ($param->isDefaultValueAvailable()) { + $defaultValue = $param->getDefaultValue(); + } + $value = $this->request->getParam($paramName, $defaultValue); + $type = $param->getType(); + $typeName = null; + if ($type instanceof ReflectionNamedType) { + $typeName = $type->getName(); + } // if this is submitted using GET or a POST form, 'false' should be // converted to false - if (($type === 'bool' || $type === 'boolean') && + if (($typeName === 'bool' || $typeName === 'boolean') && $value === 'false' && ( $this->request->method === 'GET' || @@ -214,8 +229,12 @@ private function executeController(Controller $controller, string $methodName): ) ) { $value = false; - } elseif ($value !== null && \in_array($type, $types, true)) { - settype($value, $type); + } elseif ($value !== null && \in_array($typeName, $types, true)) { + settype($value, $typeName); + } + + if ($value === null && !$param->allowsNull()) { + throw new ParameterMissingException(get_class($controller), $methodName, $paramName); } $arguments[] = $value; From 9b9dd6f9633ff7974597de8794234788efd6d24c Mon Sep 17 00:00:00 2001 From: Christoph Wurst Date: Wed, 14 Dec 2022 17:58:41 +0100 Subject: [PATCH 2/4] fixup! chore(app framework)!: catch missing non-optional controller parameter Signed-off-by: Christoph Wurst --- core/Middleware/ApiErrorMiddleware.php | 53 +++++++++++++ lib/composer/composer/autoload_classmap.php | 3 + lib/composer/composer/autoload_static.php | 3 + .../DependencyInjection/DIContainer.php | 3 + .../Exceptions/ParameterMissingException.php | 8 +- lib/private/Http/ApiResponse.php | 77 +++++++++++++++++++ 6 files changed, 145 insertions(+), 2 deletions(-) create mode 100644 core/Middleware/ApiErrorMiddleware.php create mode 100644 lib/private/Http/ApiResponse.php diff --git a/core/Middleware/ApiErrorMiddleware.php b/core/Middleware/ApiErrorMiddleware.php new file mode 100644 index 0000000000000..605b8270879ab --- /dev/null +++ b/core/Middleware/ApiErrorMiddleware.php @@ -0,0 +1,53 @@ + + * + * @author 2022 Christoph Wurst + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + */ + +namespace OC\Core\Middleware; + +use Exception; +use OC\AppFramework\Exceptions\ParameterMissingException; +use OC\Http\ApiResponse; +use OCP\AppFramework\Http; +use OCP\AppFramework\Middleware; +use OCP\IRequest; + +class ApiErrorMiddleware extends Middleware { + private IRequest $request; + + public function __construct(IRequest $request) { + $this->request = $request; + } + + public function afterException($controller, $methodName, Exception $exception) { + $sendsJson = $this->request->getHeader('Content-Type') === 'application/json'; + $acceptsJson = $this->request->getHeader('Accept') === 'application/json'; + if (($sendsJson || $acceptsJson) && $exception instanceof ParameterMissingException) { + return ApiResponse::fail( + ['error' => 'Required parameter ' . $exception->getParameterName() . ' missing'], + Http::STATUS_UNPROCESSABLE_ENTITY, + ); + } + return parent::afterException($controller, $methodName, $exception); + } +} diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index 90e7fe51fa74a..23d8c3ab0655a 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -678,6 +678,7 @@ 'OC\\AppFramework\\Bootstrap\\ServiceFactoryRegistration' => $baseDir . '/lib/private/AppFramework/Bootstrap/ServiceFactoryRegistration.php', 'OC\\AppFramework\\Bootstrap\\ServiceRegistration' => $baseDir . '/lib/private/AppFramework/Bootstrap/ServiceRegistration.php', 'OC\\AppFramework\\DependencyInjection\\DIContainer' => $baseDir . '/lib/private/AppFramework/DependencyInjection/DIContainer.php', + 'OC\\AppFramework\\Exceptions\\ParameterMissingException' => $baseDir . '/lib/private/AppFramework/Exceptions/ParameterMissingException.php', 'OC\\AppFramework\\Http' => $baseDir . '/lib/private/AppFramework/Http.php', 'OC\\AppFramework\\Http\\Dispatcher' => $baseDir . '/lib/private/AppFramework/Http/Dispatcher.php', 'OC\\AppFramework\\Http\\Output' => $baseDir . '/lib/private/AppFramework/Http/Output.php', @@ -1021,6 +1022,7 @@ 'OC\\Core\\Exception\\LoginFlowV2NotFoundException' => $baseDir . '/core/Exception/LoginFlowV2NotFoundException.php', 'OC\\Core\\Exception\\ResetPasswordException' => $baseDir . '/core/Exception/ResetPasswordException.php', 'OC\\Core\\Listener\\BeforeTemplateRenderedListener' => $baseDir . '/core/Listener/BeforeTemplateRenderedListener.php', + 'OC\\Core\\Middleware\\ApiErrorMiddleware' => $baseDir . '/core/Middleware/ApiErrorMiddleware.php', 'OC\\Core\\Middleware\\TwoFactorMiddleware' => $baseDir . '/core/Middleware/TwoFactorMiddleware.php', 'OC\\Core\\Migrations\\Version13000Date20170705121758' => $baseDir . '/core/Migrations/Version13000Date20170705121758.php', 'OC\\Core\\Migrations\\Version13000Date20170718121200' => $baseDir . '/core/Migrations/Version13000Date20170718121200.php', @@ -1290,6 +1292,7 @@ 'OC\\Hooks\\Emitter' => $baseDir . '/lib/private/Hooks/Emitter.php', 'OC\\Hooks\\EmitterTrait' => $baseDir . '/lib/private/Hooks/EmitterTrait.php', 'OC\\Hooks\\PublicEmitter' => $baseDir . '/lib/private/Hooks/PublicEmitter.php', + 'OC\\Http\\ApiResponse' => $baseDir . '/lib/private/Http/ApiResponse.php', 'OC\\Http\\Client\\Client' => $baseDir . '/lib/private/Http/Client/Client.php', 'OC\\Http\\Client\\ClientService' => $baseDir . '/lib/private/Http/Client/ClientService.php', 'OC\\Http\\Client\\DnsPinMiddleware' => $baseDir . '/lib/private/Http/Client/DnsPinMiddleware.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index 8cef29efc39bc..8de9ea6ab0263 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -711,6 +711,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 'OC\\AppFramework\\Bootstrap\\ServiceFactoryRegistration' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Bootstrap/ServiceFactoryRegistration.php', 'OC\\AppFramework\\Bootstrap\\ServiceRegistration' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Bootstrap/ServiceRegistration.php', 'OC\\AppFramework\\DependencyInjection\\DIContainer' => __DIR__ . '/../../..' . '/lib/private/AppFramework/DependencyInjection/DIContainer.php', + 'OC\\AppFramework\\Exceptions\\ParameterMissingException' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Exceptions/ParameterMissingException.php', 'OC\\AppFramework\\Http' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Http.php', 'OC\\AppFramework\\Http\\Dispatcher' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Http/Dispatcher.php', 'OC\\AppFramework\\Http\\Output' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Http/Output.php', @@ -1054,6 +1055,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 'OC\\Core\\Exception\\LoginFlowV2NotFoundException' => __DIR__ . '/../../..' . '/core/Exception/LoginFlowV2NotFoundException.php', 'OC\\Core\\Exception\\ResetPasswordException' => __DIR__ . '/../../..' . '/core/Exception/ResetPasswordException.php', 'OC\\Core\\Listener\\BeforeTemplateRenderedListener' => __DIR__ . '/../../..' . '/core/Listener/BeforeTemplateRenderedListener.php', + 'OC\\Core\\Middleware\\ApiErrorMiddleware' => __DIR__ . '/../../..' . '/core/Middleware/ApiErrorMiddleware.php', 'OC\\Core\\Middleware\\TwoFactorMiddleware' => __DIR__ . '/../../..' . '/core/Middleware/TwoFactorMiddleware.php', 'OC\\Core\\Migrations\\Version13000Date20170705121758' => __DIR__ . '/../../..' . '/core/Migrations/Version13000Date20170705121758.php', 'OC\\Core\\Migrations\\Version13000Date20170718121200' => __DIR__ . '/../../..' . '/core/Migrations/Version13000Date20170718121200.php', @@ -1323,6 +1325,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 'OC\\Hooks\\Emitter' => __DIR__ . '/../../..' . '/lib/private/Hooks/Emitter.php', 'OC\\Hooks\\EmitterTrait' => __DIR__ . '/../../..' . '/lib/private/Hooks/EmitterTrait.php', 'OC\\Hooks\\PublicEmitter' => __DIR__ . '/../../..' . '/lib/private/Hooks/PublicEmitter.php', + 'OC\\Http\\ApiResponse' => __DIR__ . '/../../..' . '/lib/private/Http/ApiResponse.php', 'OC\\Http\\Client\\Client' => __DIR__ . '/../../..' . '/lib/private/Http/Client/Client.php', 'OC\\Http\\Client\\ClientService' => __DIR__ . '/../../..' . '/lib/private/Http/Client/ClientService.php', 'OC\\Http\\Client\\DnsPinMiddleware' => __DIR__ . '/../../..' . '/lib/private/Http/Client/DnsPinMiddleware.php', diff --git a/lib/private/AppFramework/DependencyInjection/DIContainer.php b/lib/private/AppFramework/DependencyInjection/DIContainer.php index 298b9f000e36e..bea877294ce4e 100644 --- a/lib/private/AppFramework/DependencyInjection/DIContainer.php +++ b/lib/private/AppFramework/DependencyInjection/DIContainer.php @@ -300,6 +300,9 @@ public function __construct($appName, $urlParams = [], ServerContainer $server = $c->get(OC\Security\RateLimiting\Limiter::class) ) ); + $dispatcher->registerMiddleware( + $c->get(\OC\Core\Middleware\ApiErrorMiddleware::class) + ); $dispatcher->registerMiddleware( new OC\AppFramework\Middleware\PublicShare\PublicShareMiddleware( $c->get(IRequest::class), diff --git a/lib/private/AppFramework/Exceptions/ParameterMissingException.php b/lib/private/AppFramework/Exceptions/ParameterMissingException.php index ed498e54edb30..7f47f272a80cd 100644 --- a/lib/private/AppFramework/Exceptions/ParameterMissingException.php +++ b/lib/private/AppFramework/Exceptions/ParameterMissingException.php @@ -29,11 +29,15 @@ class ParameterMissingException extends Exception { + private string $parameterName; + public function __construct(string $controllerName, string $methodName, string $parameterName) { parent::__construct("Parameter $parameterName missing for $controllerName::$methodName"); - $this->controllerName = $controllerName; - $this->methodName = $methodName; $this->parameterName = $parameterName; } + public function getParameterName(): string { + return $this->parameterName; + } + } diff --git a/lib/private/Http/ApiResponse.php b/lib/private/Http/ApiResponse.php new file mode 100644 index 0000000000000..6a8ceff2e700d --- /dev/null +++ b/lib/private/Http/ApiResponse.php @@ -0,0 +1,77 @@ + + * + * @author 2022 Christoph Wurst + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + */ + +namespace OC\Http; + +use OCP\AppFramework\Http; +use OCP\AppFramework\Http\JSONResponse; + +/** + * JSend-link API response + * + * @see https://github.com/omniti-labs/jsend + */ +class ApiResponse extends JSONResponse { + + public static function success($data = null, int $statusCode = Http::STATUS_OK): self { + return new self( + [ + 'status' => 'success', + 'data' => $data, + ], + $statusCode + ); + } + + public static function fail($data = null, int $statusCode = Http::STATUS_BAD_REQUEST): self { + return new self( + [ + 'status' => 'fail', + 'data' => $data, + ], + $statusCode + ); + } + + public static function error(string $message, + int $statusCode = Http::STATUS_INTERNAL_SERVER_ERROR, + int $code = null, + $data = null): self { + return new self( + [ + 'status' => 'error', + 'message' => $message, + 'code' => $code, + 'data' => $data, + ], + $statusCode + ); + } + + private function __construct($data = [], $statusCode = Http::STATUS_OK) { + parent::__construct($data, $statusCode); + } + +} From 0a2b7c4788ad863b157a875c8b2e8710b001cd36 Mon Sep 17 00:00:00 2001 From: Christoph Wurst Date: Thu, 15 Dec 2022 09:25:28 +0100 Subject: [PATCH 3/4] fixup! chore(app framework)!: catch missing non-optional controller parameter Signed-off-by: Christoph Wurst --- lib/private/AppFramework/Http/Dispatcher.php | 4 +-- .../lib/AppFramework/Http/DispatcherTest.php | 31 +++++++++++++++++++ 2 files changed, 33 insertions(+), 2 deletions(-) diff --git a/lib/private/AppFramework/Http/Dispatcher.php b/lib/private/AppFramework/Http/Dispatcher.php index 0dba99da06c1a..0faa7cbfab64b 100644 --- a/lib/private/AppFramework/Http/Dispatcher.php +++ b/lib/private/AppFramework/Http/Dispatcher.php @@ -46,7 +46,6 @@ use Psr\Log\LoggerInterface; use ReflectionMethod; use ReflectionNamedType; -use TypeError; use function get_class; /** @@ -213,9 +212,10 @@ private function executeController(Controller $controller, string $methodName): } $value = $this->request->getParam($paramName, $defaultValue); $type = $param->getType(); - $typeName = null; if ($type instanceof ReflectionNamedType) { $typeName = $type->getName(); + } else { + $typeName = $this->reflector->getType($paramName); } // if this is submitted using GET or a POST form, 'false' should be diff --git a/tests/lib/AppFramework/Http/DispatcherTest.php b/tests/lib/AppFramework/Http/DispatcherTest.php index 8f591f26e58a6..4c9ff7dd2d318 100644 --- a/tests/lib/AppFramework/Http/DispatcherTest.php +++ b/tests/lib/AppFramework/Http/DispatcherTest.php @@ -62,6 +62,9 @@ public function exec($int, $bool, $test = 4, $test2 = 1) { return [$int, $bool, $test, $test2]; } + public function execTyped(int $int, bool $bool, string $str, ?int $opt = 42): array { + return [$int, $bool, $str, $opt]; + } /** * @param int $int @@ -370,7 +373,35 @@ public function testControllerParametersInjectedDefaultOverwritten() { $this->assertEquals('[3,true,4,7]', $response[3]); } + public function testTypedControllerParameters(): void { + $this->request = new Request( + [ + 'post' => [ + 'int' => '3', + 'bool' => 'false', + 'str' => '7', + ], + 'method' => 'POST', + ], + $this->createMock(IRequestId::class), + $this->createMock(IConfig::class) + ); + $this->dispatcher = new Dispatcher( + $this->http, $this->middlewareDispatcher, $this->reflector, + $this->request, + $this->config, + \OC::$server->getDatabaseConnection(), + $this->logger, + $this->eventLogger + ); + $controller = new TestController('app', $this->request); + + // reflector is supposed to be called once + $this->dispatcherPassthrough(); + $response = $this->dispatcher->dispatch($controller, 'execTyped'); + $this->assertEquals('[3,true,"7",42]', $response[3]); + } public function testResponseTransformedByUrlFormat() { $this->request = new Request( From ebe040d1f7ca4d1061ebf4d90e5387bf87673bd6 Mon Sep 17 00:00:00 2001 From: Christoph Wurst Date: Thu, 15 Dec 2022 09:29:24 +0100 Subject: [PATCH 4/4] fixup! chore(app framework)!: catch missing non-optional controller parameter Signed-off-by: Christoph Wurst --- .../AppFramework/Exceptions/ParameterMissingException.php | 2 -- lib/private/Http/ApiResponse.php | 2 -- 2 files changed, 4 deletions(-) diff --git a/lib/private/AppFramework/Exceptions/ParameterMissingException.php b/lib/private/AppFramework/Exceptions/ParameterMissingException.php index 7f47f272a80cd..11d9e6e21784a 100644 --- a/lib/private/AppFramework/Exceptions/ParameterMissingException.php +++ b/lib/private/AppFramework/Exceptions/ParameterMissingException.php @@ -28,7 +28,6 @@ use Exception; class ParameterMissingException extends Exception { - private string $parameterName; public function __construct(string $controllerName, string $methodName, string $parameterName) { @@ -39,5 +38,4 @@ public function __construct(string $controllerName, string $methodName, string $ public function getParameterName(): string { return $this->parameterName; } - } diff --git a/lib/private/Http/ApiResponse.php b/lib/private/Http/ApiResponse.php index 6a8ceff2e700d..928bbcdfc9b04 100644 --- a/lib/private/Http/ApiResponse.php +++ b/lib/private/Http/ApiResponse.php @@ -34,7 +34,6 @@ * @see https://github.com/omniti-labs/jsend */ class ApiResponse extends JSONResponse { - public static function success($data = null, int $statusCode = Http::STATUS_OK): self { return new self( [ @@ -73,5 +72,4 @@ public static function error(string $message, private function __construct($data = [], $statusCode = Http::STATUS_OK) { parent::__construct($data, $statusCode); } - }