From 3fa43a529bbef075364666c5122e7ad18d34de62 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Fri, 17 Nov 2023 14:04:09 +0100 Subject: [PATCH] enh(dispatcher): enforce psalm ranges in the http dispatcher - allows devs to provide int ranges for API arguments Signed-off-by: Arthur Schiwon --- lib/private/AppFramework/Http/Dispatcher.php | 22 ++++++++- .../Utility/ControllerMethodReflector.php | 33 ++++++++++--- .../lib/AppFramework/Http/DispatcherTest.php | 47 +++++++++++++++++++ .../Utility/ControllerMethodReflectorTest.php | 21 +++++++++ 4 files changed, 116 insertions(+), 7 deletions(-) diff --git a/lib/private/AppFramework/Http/Dispatcher.php b/lib/private/AppFramework/Http/Dispatcher.php index 7ff7f7a9cc0..5649060ba76 100644 --- a/lib/private/AppFramework/Http/Dispatcher.php +++ b/lib/private/AppFramework/Http/Dispatcher.php @@ -42,6 +42,7 @@ use OCP\AppFramework\Http\Response; use OCP\Diagnostics\IEventLogger; use OCP\IConfig; use OCP\IRequest; +use OutOfRangeException; use Psr\Container\ContainerInterface; use Psr\Log\LoggerInterface; @@ -197,7 +198,7 @@ class Dispatcher { private function executeController(Controller $controller, string $methodName): Response { $arguments = []; - // valid types that will be casted + // valid types that will be cast $types = ['int', 'integer', 'bool', 'boolean', 'float', 'double']; foreach ($this->reflector->getParameters() as $param => $default) { @@ -219,6 +220,7 @@ class Dispatcher { $value = false; } elseif ($value !== null && \in_array($type, $types, true)) { settype($value, $type); + $this->ensureParameterValueSatisfiesRange($param, $value); } elseif ($value === null && $type !== null && $this->appContainer->has($type)) { $value = $this->appContainer->get($type); } @@ -250,4 +252,22 @@ class Dispatcher { return $response; } + + /** + * @psalm-param mixed $value + * @throws OutOfRangeException + */ + private function ensureParameterValueSatisfiesRange(string $param, $value): void { + $rangeInfo = $this->reflector->getRange($param); + if ($rangeInfo) { + if ($value < $rangeInfo['min'] || $value > $rangeInfo['max']) { + throw new OutOfRangeException(sprintf( + 'Parameter %s must be between %d and %d', + $param, + $rangeInfo['min'], + $rangeInfo['max'], + )); + } + } + } } diff --git a/lib/private/AppFramework/Utility/ControllerMethodReflector.php b/lib/private/AppFramework/Utility/ControllerMethodReflector.php index b76b3c33c42..5a1ed0fd6ee 100644 --- a/lib/private/AppFramework/Utility/ControllerMethodReflector.php +++ b/lib/private/AppFramework/Utility/ControllerMethodReflector.php @@ -42,6 +42,7 @@ class ControllerMethodReflector implements IControllerMethodReflector { public $annotations = []; private $types = []; private $parameters = []; + private array $ranges = []; /** * @param object $object an object or classname @@ -54,26 +55,38 @@ class ControllerMethodReflector implements IControllerMethodReflector { if ($docs !== false) { // extract everything prefixed by @ and first letter uppercase preg_match_all('/^\h+\*\h+@(?P[A-Z]\w+)((?P.*))?$/m', $docs, $matches); - foreach ($matches['annotation'] as $key => $annontation) { - $annontation = strtolower($annontation); + foreach ($matches['annotation'] as $key => $annotation) { + $annotation = strtolower($annotation); $annotationValue = $matches['parameter'][$key]; if (isset($annotationValue[0]) && $annotationValue[0] === '(' && $annotationValue[\strlen($annotationValue) - 1] === ')') { $cutString = substr($annotationValue, 1, -1); $cutString = str_replace(' ', '', $cutString); - $splittedArray = explode(',', $cutString); - foreach ($splittedArray as $annotationValues) { + $splitArray = explode(',', $cutString); + foreach ($splitArray as $annotationValues) { [$key, $value] = explode('=', $annotationValues); - $this->annotations[$annontation][$key] = $value; + $this->annotations[$annotation][$key] = $value; } continue; } - $this->annotations[$annontation] = [$annotationValue]; + $this->annotations[$annotation] = [$annotationValue]; } // extract type parameter information preg_match_all('/@param\h+(?P\w+)\h+\$(?P\w+)/', $docs, $matches); $this->types = array_combine($matches['var'], $matches['type']); + preg_match_all('/@psalm-param\h+(?P\w+)<(?P(-?\d+|min)),\h*(?P(-?\d+|max))>\h+\$(?P\w+)/', $docs, $matches); + foreach ($matches['var'] as $index => $varName) { + if ($matches['type'][$index] !== 'int') { + // only int ranges are possible at the moment + // @see https://psalm.dev/docs/annotating_code/type_syntax/scalar_types + continue; + } + $this->ranges[$varName] = [ + 'min' => $matches['rangeMin'][$index] === 'min' ? PHP_INT_MIN : (int)$matches['rangeMin'][$index], + 'max' => $matches['rangeMax'][$index] === 'max' ? PHP_INT_MAX : (int)$matches['rangeMax'][$index], + ]; + } } foreach ($reflection->getParameters() as $param) { @@ -106,6 +119,14 @@ class ControllerMethodReflector implements IControllerMethodReflector { return null; } + public function getRange(string $parameter): ?array { + if (array_key_exists($parameter, $this->ranges)) { + return $this->ranges[$parameter]; + } + + return null; + } + /** * @return array the arguments of the method with key => default value */ diff --git a/tests/lib/AppFramework/Http/DispatcherTest.php b/tests/lib/AppFramework/Http/DispatcherTest.php index bb7541c4d22..d9d9eb82a68 100644 --- a/tests/lib/AppFramework/Http/DispatcherTest.php +++ b/tests/lib/AppFramework/Http/DispatcherTest.php @@ -522,4 +522,51 @@ class DispatcherTest extends \Test\TestCase { $this->assertEquals('{"text":[3,true,4,1]}', $response[3]); } + + + public function rangeDataProvider(): array { + return [ + [PHP_INT_MIN, PHP_INT_MAX, 42, false], + [0, 12, -5, true], + [-12, 0, 5, true], + [7, 14, 5, true], + [7, 14, 10, false], + [-14, -7, -10, false], + ]; + } + + /** + * @dataProvider rangeDataProvider + */ + public function testEnsureParameterValueSatisfiesRange(int $min, int $max, int $input, bool $throw): void { + $this->reflector = $this->createMock(ControllerMethodReflector::class); + $this->reflector->expects($this->any()) + ->method('getRange') + ->willReturn([ + 'min' => $min, + 'max' => $max, + ]); + + $this->dispatcher = new Dispatcher( + $this->http, + $this->middlewareDispatcher, + $this->reflector, + $this->request, + $this->config, + \OC::$server->getDatabaseConnection(), + $this->logger, + $this->eventLogger, + $this->container, + ); + + if ($throw) { + $this->expectException(\OutOfRangeException::class); + } + + $this->invokePrivate($this->dispatcher, 'ensureParameterValueSatisfiesRange', ['myArgument', $input]); + if (!$throw) { + // do not mark this test risky + $this->assertTrue(true); + } + } } diff --git a/tests/lib/AppFramework/Utility/ControllerMethodReflectorTest.php b/tests/lib/AppFramework/Utility/ControllerMethodReflectorTest.php index 5452fb853b9..2ba2f34425b 100644 --- a/tests/lib/AppFramework/Utility/ControllerMethodReflectorTest.php +++ b/tests/lib/AppFramework/Utility/ControllerMethodReflectorTest.php @@ -54,6 +54,14 @@ class MiddleController extends BaseController { public function test3() { } + + /** + * @psalm-param int<-4, 42> $rangedOne + * @psalm-param int $rangedTwo + * @return void + */ + public function test4(int $rangedOne, int $rangedTwo) { + } } class EndController extends MiddleController { @@ -234,4 +242,17 @@ class ControllerMethodReflectorTest extends \Test\TestCase { $this->assertFalse($reader->hasAnnotation('Annotation')); } + + public function testRangeDetection() { + $reader = new ControllerMethodReflector(); + $reader->reflect('Test\AppFramework\Utility\EndController', 'test4'); + + $rangeInfo1 = $reader->getRange('rangedOne'); + $this->assertSame(-4, $rangeInfo1['min']); + $this->assertSame(42, $rangeInfo1['max']); + + $rangeInfo2 = $reader->getRange('rangedTwo'); + $this->assertSame(PHP_INT_MIN, $rangeInfo2['min']); + $this->assertSame(PHP_INT_MAX, $rangeInfo2['max']); + } } -- 2.39.5