Merge pull request #41578 from nextcloud/enh/noid/dispatcher-test-argument-range

Enable AppFramework dispatcher to enforce integer ranges

6 files changed, 119 insertions, 9 deletions

diff --git a/apps/dashboard/lib/Controller/DashboardApiController.php b/apps/dashboard/lib/Controller/DashboardApiController.php
index 35db52225da..c95979e667d 100644
--- a/apps/dashboard/lib/Controller/DashboardApiController.php
+++ b/apps/dashboard/lib/Controller/DashboardApiController.php
@@ -127,7 +127,8 @@ class DashboardApiController extends OCSController {
 	 * Get the items for the widgets
 	 *
 	 * @param array<string, string> $sinceIds Array indexed by widget Ids, contains date/id from which we want the new items
-	 * @param int $limit Limit number of result items per widget
+	 * @param int $limit Limit number of result items per widget, not more than 30 are allowed
+	 * @psalm-param int<1, 30> $limit
 	 * @param string[] $widgets Limit results to specific widgets
 	 * @return DataResponse<Http::STATUS_OK, array<string, DashboardWidgetItems>, array{}>
 	 *
diff --git a/apps/dashboard/openapi.json b/apps/dashboard/openapi.json
index 7cbb959cc7e..138f06df83b 100644
--- a/apps/dashboard/openapi.json
+++ b/apps/dashboard/openapi.json
@@ -360,7 +360,7 @@
                     {
                         "name": "limit",
                         "in": "query",
-                        "description": "Limit number of result items per widget",
+                        "description": "Limit number of result items per widget, not more than 30 are allowed",
                         "schema": {
                             "type": "integer",
                             "format": "int64",
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<annotation>[A-Z]\w+)((?P<parameter>.*))?$/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<type>\w+)\h+\$(?P<var>\w+)/', $docs, $matches);
 			$this->types = array_combine($matches['var'], $matches['type']);
+			preg_match_all('/@psalm-param\h+(?P<type>\w+)<(?P<rangeMin>(-?\d+|min)),\h*(?P<rangeMax>(-?\d+|max))>\h+\$(?P<var>\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<min, max> $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']);
+	}
 }