diff --git a/lib/private/AppFramework/Http/Dispatcher.php b/lib/private/AppFramework/Http/Dispatcher.php index 2fa6bd8f1cb..a6b44d9fb96 100644 --- a/lib/private/AppFramework/Http/Dispatcher.php +++ b/lib/private/AppFramework/Http/Dispatcher.php @@ -16,6 +16,7 @@ use OC\DB\ConnectionAdapter; use OCP\App\IAppManager; use OCP\AppFramework\Controller; use OCP\AppFramework\Http\DataResponse; +use OCP\AppFramework\Http\InvalidStringParameterException; use OCP\AppFramework\Http\ParameterOutOfRangeException; use OCP\AppFramework\Http\Response; use OCP\Diagnostics\IEventLogger; @@ -160,6 +161,8 @@ class Dispatcher { } elseif ($value !== null && \in_array($type, $types, true)) { settype($value, $type); $this->ensureParameterValueSatisfiesRange($param, $value, $default); + } elseif ($value !== null && $type === 'string' && \is_string($value)) { + $this->ensureParameterValueSatisfiesStringConstraint($param, $value); } elseif ($value === null && $type !== null && $this->appContainer->has($type)) { $value = $this->appContainer->get($type); } @@ -225,4 +228,13 @@ class Dispatcher { } } } + + /** + * @throws InvalidStringParameterException + */ + private function ensureParameterValueSatisfiesStringConstraint(string $param, string $value): void { + if (!$this->reflector->satisfiesStringConstraint($param, $value)) { + throw new InvalidStringParameterException($param, $this->reflector->getStringConstraint($param)); + } + } } diff --git a/lib/private/AppFramework/Utility/ControllerMethodReflector.php b/lib/private/AppFramework/Utility/ControllerMethodReflector.php index e09756a957b..3c2a7d01f61 100644 --- a/lib/private/AppFramework/Utility/ControllerMethodReflector.php +++ b/lib/private/AppFramework/Utility/ControllerMethodReflector.php @@ -20,6 +20,7 @@ class ControllerMethodReflector implements IControllerMethodReflector { private array $types = []; private array $parameters = []; private array $ranges = []; + private array $stringConstraints = []; private int $startLine = 0; private string $file = ''; private ?\ReflectionMethod $reflectionMethod = null; @@ -38,6 +39,7 @@ class ControllerMethodReflector implements IControllerMethodReflector { $this->types = []; $this->parameters = []; $this->ranges = []; + $this->stringConstraints = []; $this->reflectionMethod = new \ReflectionMethod($object, $method); $this->startLine = $this->reflectionMethod->getStartLine(); $this->file = $this->reflectionMethod->getFileName(); @@ -79,6 +81,23 @@ class ControllerMethodReflector implements IControllerMethodReflector { 'max' => $matches['rangeMax'][$index] === 'max' ? PHP_INT_MAX : (int)$matches['rangeMax'][$index], ]; } + + // extract psalm int aliases that imply a fixed range + preg_match_all('/@(?:psalm-)?param\h+(\?)?(?Ppositive-int|non-negative-int|negative-int|non-positive-int)(\|null)?\h+\$(?P\w+)/', $docs, $matches); + foreach ($matches['var'] as $index => $varName) { + $this->ranges[$varName] = match ($matches['type'][$index]) { + 'positive-int' => ['min' => 1, 'max' => PHP_INT_MAX], + 'non-negative-int' => ['min' => 0, 'max' => PHP_INT_MAX], + 'negative-int' => ['min' => PHP_INT_MIN, 'max' => -1], + 'non-positive-int' => ['min' => PHP_INT_MIN, 'max' => 0], + }; + } + + // extract psalm scalar string types + preg_match_all('/@(?:psalm-)?param\h+(\?)?(?Pnon-empty-lowercase-string|non-falsy-string|non-empty-string|lowercase-string|numeric-string)(\|null)?\h+\$(?P\w+)/', $docs, $matches); + foreach ($matches['var'] as $index => $varName) { + $this->stringConstraints[$varName] = $matches['type'][$index]; + } } foreach ($this->reflectionMethod->getParameters() as $param) { @@ -120,6 +139,25 @@ class ControllerMethodReflector implements IControllerMethodReflector { return null; } + public function getStringConstraint(string $parameter): ?string { + return $this->stringConstraints[$parameter] ?? null; + } + + /** + * Whether $value satisfies the psalm string type annotated for $parameter, + * or true if none was annotated + */ + public function satisfiesStringConstraint(string $parameter, string $value): bool { + return match ($this->getStringConstraint($parameter)) { + 'non-empty-string' => $value !== '', + 'non-empty-lowercase-string' => $value !== '' && $value === strtolower($value), + 'lowercase-string' => $value === strtolower($value), + 'non-falsy-string' => $value !== '' && $value !== '0', + 'numeric-string' => is_numeric($value), + default => true, + }; + } + /** * @return array the arguments of the method with key => default value */ diff --git a/lib/public/AppFramework/Http/InvalidStringParameterException.php b/lib/public/AppFramework/Http/InvalidStringParameterException.php new file mode 100644 index 00000000000..16d92eec469 --- /dev/null +++ b/lib/public/AppFramework/Http/InvalidStringParameterException.php @@ -0,0 +1,41 @@ +parameterName, $this->constraint) + ); + } + + /** + * @since 35.0.0 + */ + public function getParameterName(): string { + return $this->parameterName; + } + + /** + * @since 35.0.0 + */ + public function getConstraint(): string { + return $this->constraint; + } +} diff --git a/tests/lib/AppFramework/Http/DispatcherTest.php b/tests/lib/AppFramework/Http/DispatcherTest.php index 3e90108c60e..af261131543 100644 --- a/tests/lib/AppFramework/Http/DispatcherTest.php +++ b/tests/lib/AppFramework/Http/DispatcherTest.php @@ -16,6 +16,7 @@ use OC\AppFramework\Utility\ControllerMethodReflector; use OCP\AppFramework\Controller; use OCP\AppFramework\Http; use OCP\AppFramework\Http\DataResponse; +use OCP\AppFramework\Http\InvalidStringParameterException; use OCP\AppFramework\Http\JSONResponse; use OCP\AppFramework\Http\ParameterOutOfRangeException; use OCP\AppFramework\Http\Response; @@ -586,4 +587,45 @@ class DispatcherTest extends \Test\TestCase { $this->assertTrue(true); } } + + public static function stringConstraintDataProvider(): array { + return [ + [true, null, false], + [true, 'non-empty-string', false], + [false, 'non-empty-string', true], + ]; + } + + #[\PHPUnit\Framework\Attributes\DataProvider('stringConstraintDataProvider')] + public function testEnsureParameterValueSatisfiesStringConstraint(bool $satisfies, ?string $constraint, bool $throw): void { + $this->reflector = $this->createMock(ControllerMethodReflector::class); + $this->reflector->expects($this->any()) + ->method('satisfiesStringConstraint') + ->willReturn($satisfies); + $this->reflector->expects($this->any()) + ->method('getStringConstraint') + ->willReturn($constraint); + + $this->dispatcher = new Dispatcher( + $this->http, + $this->middlewareDispatcher, + $this->reflector, + $this->request, + $this->config, + Server::get(IDBConnection::class), + $this->logger, + $this->eventLogger, + $this->container, + ); + + if ($throw) { + $this->expectException(InvalidStringParameterException::class); + } + + self::invokePrivate($this->dispatcher, 'ensureParameterValueSatisfiesStringConstraint', ['myArgument', '']); + 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 1fd89039807..618f3feef9c 100644 --- a/tests/lib/AppFramework/Utility/ControllerMethodReflectorTest.php +++ b/tests/lib/AppFramework/Utility/ControllerMethodReflectorTest.php @@ -63,6 +63,41 @@ class MiddleController extends BaseController { */ public function test5(int $rangedOne, int $rangedTwo, ?int $rangedThree, ?int $rangedFour) { } + + /** + * @psalm-param positive-int $positive + * @psalm-param non-negative-int $nonNegative + * @psalm-param negative-int $negative + * @psalm-param non-positive-int $nonPositive + * @psalm-param positive-int|null $positiveOrNull + * @psalm-param ?non-negative-int $nonNegativeOrNull + * @return void + */ + public function test6(int $positive, int $nonNegative, int $negative, int $nonPositive, ?int $positiveOrNull, ?int $nonNegativeOrNull) { + } + + /** + * @psalm-param non-empty-string $nonEmpty + * @psalm-param non-empty-lowercase-string $nonEmptyLowercase + * @psalm-param lowercase-string $lowercase + * @psalm-param non-falsy-string $nonFalsy + * @psalm-param numeric-string $numeric + * @psalm-param non-empty-string|null $nonEmptyOrNull + * @psalm-param ?non-empty-string $nonEmptyOrNullPrefix + * @psalm-param string $plain + * @return void + */ + public function test7( + string $nonEmpty, + string $nonEmptyLowercase, + string $lowercase, + string $nonFalsy, + string $numeric, + ?string $nonEmptyOrNull, + ?string $nonEmptyOrNullPrefix, + string $plain, + ) { + } } class EndController extends MiddleController { @@ -275,4 +310,86 @@ class ControllerMethodReflectorTest extends \Test\TestCase { $this->assertSame(-70, $rangeInfo3['min']); $this->assertSame(-30, $rangeInfo3['max']); } + + public function testRangeDetectionIntAliases(): void { + $reader = new ControllerMethodReflector(Server::get(LoggerInterface::class)); + $reader->reflect('Test\AppFramework\Utility\EndController', 'test6'); + + $positive = $reader->getRange('positive'); + $this->assertSame(1, $positive['min']); + $this->assertSame(PHP_INT_MAX, $positive['max']); + + $nonNegative = $reader->getRange('nonNegative'); + $this->assertSame(0, $nonNegative['min']); + $this->assertSame(PHP_INT_MAX, $nonNegative['max']); + + $negative = $reader->getRange('negative'); + $this->assertSame(PHP_INT_MIN, $negative['min']); + $this->assertSame(-1, $negative['max']); + + $nonPositive = $reader->getRange('nonPositive'); + $this->assertSame(PHP_INT_MIN, $nonPositive['min']); + $this->assertSame(0, $nonPositive['max']); + + $positiveOrNull = $reader->getRange('positiveOrNull'); + $this->assertSame(1, $positiveOrNull['min']); + $this->assertSame(PHP_INT_MAX, $positiveOrNull['max']); + + $nonNegativeOrNull = $reader->getRange('nonNegativeOrNull'); + $this->assertSame(0, $nonNegativeOrNull['min']); + $this->assertSame(PHP_INT_MAX, $nonNegativeOrNull['max']); + } + + public function testStringConstraintDetection(): void { + $reader = new ControllerMethodReflector(Server::get(LoggerInterface::class)); + $reader->reflect('Test\AppFramework\Utility\EndController', 'test7'); + + $this->assertSame('non-empty-string', $reader->getStringConstraint('nonEmpty')); + $this->assertSame('non-empty-lowercase-string', $reader->getStringConstraint('nonEmptyLowercase')); + $this->assertSame('lowercase-string', $reader->getStringConstraint('lowercase')); + $this->assertSame('non-falsy-string', $reader->getStringConstraint('nonFalsy')); + $this->assertSame('numeric-string', $reader->getStringConstraint('numeric')); + $this->assertSame('non-empty-string', $reader->getStringConstraint('nonEmptyOrNull')); + $this->assertSame('non-empty-string', $reader->getStringConstraint('nonEmptyOrNullPrefix')); + $this->assertNull($reader->getStringConstraint('plain')); + } + + public static function stringConstraintDataProvider(): array { + return [ + ['non-empty-string', '', false], + ['non-empty-string', 'a', true], + ['non-empty-lowercase-string', '', false], + ['non-empty-lowercase-string', 'ABC', false], + ['non-empty-lowercase-string', 'abc', true], + ['lowercase-string', '', true], + ['lowercase-string', 'ABC', false], + ['lowercase-string', 'abc', true], + ['non-falsy-string', '', false], + ['non-falsy-string', '0', false], + ['non-falsy-string', '0.0', true], + ['non-falsy-string', 'a', true], + ['numeric-string', 'abc', false], + ['numeric-string', '42', true], + ['numeric-string', '4.2', true], + [null, '', true], + [null, 'anything', true], + ]; + } + + #[\PHPUnit\Framework\Attributes\DataProvider('stringConstraintDataProvider')] + public function testSatisfiesStringConstraint(?string $constraint, string $value, bool $expected): void { + $reader = new ControllerMethodReflector(Server::get(LoggerInterface::class)); + $reader->reflect('Test\AppFramework\Utility\EndController', 'test7'); + + $parameter = match ($constraint) { + 'non-empty-string' => 'nonEmpty', + 'non-empty-lowercase-string' => 'nonEmptyLowercase', + 'lowercase-string' => 'lowercase', + 'non-falsy-string' => 'nonFalsy', + 'numeric-string' => 'numeric', + default => 'plain', + }; + + $this->assertSame($expected, $reader->satisfiesStringConstraint($parameter, $value)); + } }