diff --git a/.github/workflows/continuous_integration.yml b/.github/workflows/continuous_integration.yml index 7aa72bb6dd..583c97b199 100644 --- a/.github/workflows/continuous_integration.yml +++ b/.github/workflows/continuous_integration.yml @@ -22,7 +22,7 @@ jobs: strategy: matrix: install-args: ['', '--prefer-lowest'] - php-version: ['8.1', '8.2', '8.3', '8.4'] + php-version: ['8.2', '8.3', '8.4', '8.5'] fail-fast: false steps: @@ -73,7 +73,7 @@ jobs: run: "composer phpstan" - name: "Run coding standard checks with squizlabs/php_codesniffer on minimum supported PHP version" - if: matrix.php-version == '8.1' + if: matrix.php-version == '8.2' run: composer cs-check - name: "Archive code coverage results" diff --git a/composer.json b/composer.json index 00ccdb862c..5adda6bc52 100644 --- a/composer.json +++ b/composer.json @@ -10,7 +10,7 @@ } ], "require": { - "php": ">=8.1", + "php": ">=8.2", "ext-json": "*", "composer/package-versions-deprecated": "^1.8", "phpdocumentor/reflection-docblock": "^5.4", @@ -28,14 +28,14 @@ }, "require-dev": { "beberlei/porpaginas": "^2.0", - "doctrine/coding-standard": "^12.0 || ^13.0", + "doctrine/coding-standard": "^13.0", "ecodev/graphql-upload": "^7.0", "laminas/laminas-diactoros": "^3.5", "myclabs/php-enum": "^1.6.6", "php-coveralls/php-coveralls": "^2.7", "phpstan/extension-installer": "^1.4", "phpstan/phpstan": "^2.0", - "phpunit/phpunit": "^10.5 || ^11.0 || ^12.0", + "phpunit/phpunit": "^11.0", "symfony/var-dumper": "^6.4" }, "suggest": { @@ -68,6 +68,11 @@ "composer/package-versions-deprecated": true, "dealerdirect/phpcodesniffer-composer-installer": true, "phpstan/extension-installer": true + }, + "audit": { + "ignore": { + "PKSA-5jz8-6tcw-pbk4": "PHPUnit argument-injection via newline in INI values forwarded to child PHP processes (GHSA-qrr6-mg7r-m243). phpunit is a require-dev dependency; the attack surface is INI values in phpunit config or CLI args, which are authored by maintainers/CI and carry the same trust as any other committed code. No fix available for PHPUnit 10.x or 11.x yet — revisit when backport ships or when we bump min PHP to 8.3 and can move to ^12.5.22." + } } } } diff --git a/phpcs.xml.dist b/phpcs.xml.dist index 2a564ed45b..691b89f249 100644 --- a/phpcs.xml.dist +++ b/phpcs.xml.dist @@ -29,6 +29,9 @@ + + diff --git a/src/FieldsBuilder.php b/src/FieldsBuilder.php index ea8688a18e..8ed8c87a57 100644 --- a/src/FieldsBuilder.php +++ b/src/FieldsBuilder.php @@ -878,7 +878,13 @@ private function mapParameters( $docBlockTypes = []; foreach ($paramTags as $paramTag) { - $docBlockTypes[$paramTag->getVariableName()] = $paramTag->getType(); + $variableName = $paramTag->getVariableName(); + // Skip malformed @param tags with no variable name (phpdocumentor returns null for + // those). PHPStan 8.5 rejects the implicit null-to-empty-string coercion. + if ($variableName === null) { + continue; + } + $docBlockTypes[$variableName] = $paramTag->getType(); } $parameterAnnotationsPerParameter = $this->annotationReader->getParameterAnnotationsPerParameter($refParameters); diff --git a/src/Mappers/GlobTypeMapperCache.php b/src/Mappers/GlobTypeMapperCache.php index 032f0f51c9..2339fea83b 100644 --- a/src/Mappers/GlobTypeMapperCache.php +++ b/src/Mappers/GlobTypeMapperCache.php @@ -7,6 +7,7 @@ use ReflectionClass; use function array_keys; +use function assert; /** * The cached results of a GlobTypeMapper @@ -48,7 +49,11 @@ public function registerAnnotations(ReflectionClass|string $sourceClass, GlobAnn $this->mapClassToTypeArray[$objectClassName] = $className; } + // GlobAnnotationsCache::withType() sets typeClassName and typeName together, so + // inside this branch typeName is guaranteed non-null. Assert the invariant for PHPStan + // 8.5+, which is stricter about nullable array offsets. $typeName = $globAnnotationsCache->getTypeName(); + assert($typeName !== null); $this->mapNameToType[$typeName] = $className; } diff --git a/src/Mappers/Root/IteratorTypeMapper.php b/src/Mappers/Root/IteratorTypeMapper.php index 767f0489f6..3d51c54a98 100644 --- a/src/Mappers/Root/IteratorTypeMapper.php +++ b/src/Mappers/Root/IteratorTypeMapper.php @@ -206,8 +206,6 @@ private function toGraphQLType(Compound $type, Closure $topToGraphQLType, bool $ */ private function splitIteratorFromOtherTypes(array &$types): Type|null { - $iteratorType = null; - $key = null; foreach ($types as $key => $singleDocBlockType) { if (! ($singleDocBlockType instanceof Object_)) { continue; @@ -216,21 +214,17 @@ private function splitIteratorFromOtherTypes(array &$types): Type|null /** @var class-string $fqcn */ $fqcn = (string) $singleDocBlockType->getFqsen(); $refClass = new ReflectionClass($fqcn); - // Note : $refClass->isIterable() is only accessible in PHP 7.2 if (! $refClass->implementsInterface(Iterator::class) && ! $refClass->implementsInterface(IteratorAggregate::class)) { continue; } - $iteratorType = $singleDocBlockType; - break; - } - if ($iteratorType === null) { - return null; - } + // One of the classes in the compound is an iterator. Remove it and let the caller + // test the remaining values as potential subTypes. + unset($types[$key]); - // One of the classes in the compound is an iterator. Let's remove it from the list and let's test all other values as potential subTypes. - unset($types[$key]); + return $singleDocBlockType; + } - return $iteratorType; + return null; } } diff --git a/src/Middlewares/SecurityFieldMiddleware.php b/src/Middlewares/SecurityFieldMiddleware.php index f9d744afa7..920071c02d 100644 --- a/src/Middlewares/SecurityFieldMiddleware.php +++ b/src/Middlewares/SecurityFieldMiddleware.php @@ -18,7 +18,9 @@ use function array_combine; use function array_keys; +use function array_slice; use function assert; +use function count; /** * A field middleware that reads "Security" Symfony annotations. @@ -73,9 +75,15 @@ public function process( $originalResolver = $queryFieldDescriptor->getOriginalResolver(); $parameters = $queryFieldDescriptor->getParameters(); + // QueryField::fromFieldDescriptor prepends a SourceParameter to the parameters list when + // `injectSource` is true, but that injection happens AFTER this middleware runs. At resolver + // invocation time the runtime $args therefore includes the source as its first element while + // $parameters captured here does not. Track the flag so getVariables() can strip the leading + // source arg before zipping args to parameter names. + $injectSource = $queryFieldDescriptor->isInjectSource(); - $queryFieldDescriptor = $queryFieldDescriptor->withResolver(function (object|null $source, ...$args) use ($originalResolver, $securityAnnotations, $resolver, $failWith, $parameters, $queryFieldDescriptor) { - $variables = $this->getVariables($args, $parameters, $originalResolver->executionSource($source)); + $queryFieldDescriptor = $queryFieldDescriptor->withResolver(function (object|null $source, ...$args) use ($originalResolver, $securityAnnotations, $resolver, $failWith, $parameters, $queryFieldDescriptor, $injectSource) { + $variables = $this->getVariables($args, $parameters, $originalResolver->executionSource($source), $injectSource); foreach ($securityAnnotations as $annotation) { try { @@ -108,7 +116,7 @@ public function process( * * @return array */ - private function getVariables(array $args, array $parameters, object|null $source): array + private function getVariables(array $args, array $parameters, object|null $source, bool $injectSource = false): array { $variables = [ // If a user is not logged, we provide an empty user object to make usage easier @@ -118,8 +126,15 @@ private function getVariables(array $args, array $parameters, object|null $sourc 'this' => $source, ]; + // Strip the source arg prepended by QueryField::fromFieldDescriptor so the remaining + // user-supplied args line up positionally with the captured parameter names. The source + // is always exposed via `this`, so there's no loss of information for the expression. + if ($injectSource && count($args) > count($parameters)) { + $args = array_slice($args, 1); + } + $argsName = array_keys($parameters); - $argsByName = array_combine($argsName, $args); + $argsByName = $argsName ? array_combine($argsName, $args) : []; return $variables + $argsByName; } diff --git a/src/Middlewares/SecurityInputFieldMiddleware.php b/src/Middlewares/SecurityInputFieldMiddleware.php index c2e77c207b..8c9a5632ce 100644 --- a/src/Middlewares/SecurityInputFieldMiddleware.php +++ b/src/Middlewares/SecurityInputFieldMiddleware.php @@ -15,6 +15,8 @@ use function array_combine; use function array_keys; +use function array_slice; +use function count; /** * A field input middleware that reads "Security" Symfony annotations. @@ -43,9 +45,13 @@ public function process(InputFieldDescriptor $inputFieldDescriptor, InputFieldHa $originalResolver = $inputFieldDescriptor->getOriginalResolver(); $parameters = $inputFieldDescriptor->getParameters(); + // InputField::fromFieldDescriptor prepends a SourceParameter to the parameters list when + // `injectSource` is true, but that injection happens AFTER this middleware runs. See the + // sibling SecurityFieldMiddleware for the detailed comment. + $injectSource = $inputFieldDescriptor->isInjectSource(); - $inputFieldDescriptor = $inputFieldDescriptor->withResolver(function (object|null $source, ...$args) use ($originalResolver, $securityAnnotations, $resolver, $parameters, $inputFieldDescriptor) { - $variables = $this->getVariables($args, $parameters, $originalResolver->executionSource($source)); + $inputFieldDescriptor = $inputFieldDescriptor->withResolver(function (object|null $source, ...$args) use ($originalResolver, $securityAnnotations, $resolver, $parameters, $inputFieldDescriptor, $injectSource) { + $variables = $this->getVariables($args, $parameters, $originalResolver->executionSource($source), $injectSource); foreach ($securityAnnotations as $annotation) { try { @@ -71,7 +77,7 @@ public function process(InputFieldDescriptor $inputFieldDescriptor, InputFieldHa * * @return array */ - private function getVariables(array $args, array $parameters, object|null $source): array + private function getVariables(array $args, array $parameters, object|null $source, bool $injectSource = false): array { $variables = [ // If a user is not logged, we provide an empty user object to make usage easier @@ -81,8 +87,12 @@ private function getVariables(array $args, array $parameters, object|null $sourc 'this' => $source, ]; + if ($injectSource && count($args) > count($parameters)) { + $args = array_slice($args, 1); + } + $argsName = array_keys($parameters); - $argsByName = array_combine($argsName, $args); + $argsByName = $argsName ? array_combine($argsName, $args) : []; return $variables + $argsByName; } diff --git a/tests/Fixtures/Integration/Types/ExtendedContactType.php b/tests/Fixtures/Integration/Types/ExtendedContactType.php index 2b9e8eb437..10d69db74f 100644 --- a/tests/Fixtures/Integration/Types/ExtendedContactType.php +++ b/tests/Fixtures/Integration/Types/ExtendedContactType.php @@ -6,6 +6,7 @@ use TheCodingMachine\GraphQLite\Annotations\ExtendType; use TheCodingMachine\GraphQLite\Annotations\Field; +use TheCodingMachine\GraphQLite\Annotations\Security; use TheCodingMachine\GraphQLite\Fixtures\Integration\Models\Contact; use function strtoupper; @@ -34,4 +35,17 @@ public function company(Contact $contact): string { return $contact->getName() . ' Ltd'; } + + /** + * Regression: #[Security] on an ExtendType field used to blow up with + * "array_combine(): ... must have the same number of elements" because + * SecurityFieldMiddleware didn't mirror the source-injection that + * QueryField::fromFieldDescriptor performs. + */ + #[Field] + #[Security("user && user.bar == 42", failWith: null)] + public function extendedSecretName(Contact $contact): string|null + { + return $contact->getName(); + } } diff --git a/tests/Integration/EndToEndTest.php b/tests/Integration/EndToEndTest.php index 27503359b1..58085f32ec 100644 --- a/tests/Integration/EndToEndTest.php +++ b/tests/Integration/EndToEndTest.php @@ -1338,6 +1338,63 @@ public function testEndToEndSecurityInField(): void $this->assertSame('Access denied.', $result->toArray(DebugFlag::RETHROW_UNSAFE_EXCEPTIONS)['errors'][0]['message']); } + /** + * Regression test for #[Security] on an #[ExtendType] field method. + * + * SecurityFieldMiddleware captures $parameters at process() time, but + * QueryField::fromFieldDescriptor prepends an implicit SourceParameter + * AFTER the middleware runs when isInjectSource() is true. At runtime the + * resolver receives N+1 args for N captured parameter names, which used + * to blow array_combine() up with "Argument #1 ($keys) and argument #2 + * ($values) must have the same number of elements". + */ + public function testEndToEndSecurityOnExtendTypeField(): void + { + // No logged-in user → the Security expression `user.bar == 42` fails, failWith returns null. + $schema = $this->mainContainer->get(Schema::class); + assert($schema instanceof Schema); + + $queryString = ' + query { + contacts { + name + extendedSecretName + } + } + '; + + $result = GraphQL::executeQuery($schema, $queryString); + $data = $this->getSuccessResult($result); + $this->assertSame('Joe', $data['contacts'][0]['name']); + $this->assertNull($data['contacts'][0]['extendedSecretName']); + + // Logged-in user with bar=42 → the Security expression passes and the field returns the name. + $container = $this->createContainer([ + AuthenticationServiceInterface::class => static function () { + return new class implements AuthenticationServiceInterface { + public function isLogged(): bool + { + return true; + } + + public function getUser(): object|null + { + $user = new stdClass(); + $user->bar = 42; + return $user; + } + }; + }, + ]); + + $schema = $container->get(Schema::class); + assert($schema instanceof Schema); + + $result = GraphQL::executeQuery($schema, $queryString); + $data = $this->getSuccessResult($result); + $this->assertSame('Joe', $data['contacts'][0]['extendedSecretName']); + } + public function testEndToEndUnions(): void { $schema = $this->mainContainer->get(Schema::class); diff --git a/website/package.json b/website/package.json index cc21e6d0e1..520980cd14 100644 --- a/website/package.json +++ b/website/package.json @@ -16,6 +16,9 @@ "react": "^17.0.1", "react-dom": "^17.0.1" }, + "resolutions": { + "webpack": "5.88.2" + }, "browserslist": { "production": [ ">0.5%",