From dafc8b299fefc35dd575dc9efbb08174acb21dff Mon Sep 17 00:00:00 2001 From: Jacob Thomason Date: Fri, 17 Apr 2026 21:27:25 -0400 Subject: [PATCH 1/6] Fix #[Security] on ExtendType fields (source-injection array_combine crash) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SecurityFieldMiddleware captures $parameters in process() before QueryField::fromFieldDescriptor prepends a SourceParameter when isInjectSource() is true. At resolver invocation time $args then includes the source as its first element while $parameters (captured earlier) does not, so array_combine() blows up with: Argument #1 ($keys) and argument #2 ($values) must have the same number of elements In practice this masks as a generic "Internal server error" from any #[Security]-decorated field on an #[ExtendType], which isn't discovered until someone tries to guard an ExtendType field by role. AuthorizationFieldMiddleware (@Logged / @Right) sidesteps the issue by using `function (...\$args)` and passing args through transparently — it doesn't need to map args to parameter names because it has no expression language context. The Security middlewares are the only ones that zip args and parameters together, so they're the only ones that need the fix. The same bug exists in SecurityInputFieldMiddleware for input field factories where source is similarly injected by InputField::fromFieldDescriptor. Fix: pass isInjectSource() through to getVariables() and slice the leading source arg off \$args before the array_combine. The source is still available via `this` in the expression context, so no information is lost for Security expressions. Regression test: #[Security] on ExtendedContactType::extendedSecretName exercises the ExtendType + Security combination. Before the fix the test throws the array_combine TypeError; after, both the failWith null path and the authorized path return the expected values. --- src/Middlewares/SecurityFieldMiddleware.php | 23 ++++++-- .../SecurityInputFieldMiddleware.php | 18 ++++-- .../Integration/Types/ExtendedContactType.php | 14 +++++ tests/Integration/EndToEndTest.php | 57 +++++++++++++++++++ 4 files changed, 104 insertions(+), 8 deletions(-) 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); From 0099ce23a5475b2492939da4aaa7bb5a8ae444d3 Mon Sep 17 00:00:00 2001 From: Jacob Thomason Date: Fri, 17 Apr 2026 22:12:40 -0400 Subject: [PATCH 2/6] ci: drop PHP 8.1, pin PHPUnit to 11.x, pin webpack for Docusaurus build MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PHP 8.1 reached end-of-life in 2025 so drop it from the test matrix and bump the runtime requirement to >=8.2. On the matrix side this also clears a pre-existing red CI caused by PHPUnit 12.x (PHP >=8.3) now resolving over PHPUnit 11.x on 8.1. Pin `phpunit/phpunit` to `^11.0` explicitly — PHPUnit 11 is the latest major that supports PHP 8.2, and 12.x's PHP >=8.3 requirement is what was breaking composer resolution for every PR opened after PHPUnit 12.5.22 shipped. Acknowledge advisory PKSA-5jz8-6tcw-pbk4 (GHSA-qrr6-mg7r-m243) with a targeted audit-ignore carrying the threat-model rationale. The advisory describes argument injection via newlines in PHP INI values forwarded to child processes; phpunit is require-dev only and the attack surface is phpunit config + CLI args authored by maintainers/CI, which carry the same trust boundary as any other committed code. No fix has been backported to PHPUnit 10.x or 11.x. Revisit when a backport ships or when we bump min PHP to 8.3 and can move to ^12.5.22. For the Docusaurus docs workflow, pin webpack to 5.88.2 via a package.json `resolutions` block. Webpack versions newer than 5.88.x tightened ProgressPlugin schema validation and reject options that webpackbar@5 (transitively pinned by @docusaurus/core 2.4.3) passes through, producing the "options has an unknown property 'name' / 'color' / 'reporters' / 'reporter'" build failure on every PR. --- .github/workflows/continuous_integration.yml | 4 ++-- composer.json | 9 +++++++-- website/package.json | 3 +++ 3 files changed, 12 insertions(+), 4 deletions(-) diff --git a/.github/workflows/continuous_integration.yml b/.github/workflows/continuous_integration.yml index 7aa72bb6dd..1c374ac378 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'] 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..3bac8dd8aa 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", @@ -35,7 +35,7 @@ "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/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%", From 1c6078449c2ea092d748725c52aaebd4b0084ebc Mon Sep 17 00:00:00 2001 From: Jacob Thomason Date: Fri, 17 Apr 2026 22:25:04 -0400 Subject: [PATCH 3/6] ci: add PHP 8.5 to the test matrix --- .github/workflows/continuous_integration.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/continuous_integration.yml b/.github/workflows/continuous_integration.yml index 1c374ac378..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.2', '8.3', '8.4'] + php-version: ['8.2', '8.3', '8.4', '8.5'] fail-fast: false steps: From 1f47699a7b29ca015a05401d9c7cd8e9d9021358 Mon Sep 17 00:00:00 2001 From: Jacob Thomason Date: Fri, 17 Apr 2026 22:31:07 -0400 Subject: [PATCH 4/6] fix: satisfy PHPStan 8.5 strict nullable-offset analysis MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PHPStan on PHP 8.5 is stricter about array-offset access when the key type includes null. Three pre-existing call sites tripped the new offsetAccess.invalidOffset rule; all three are safe to tighten without behavior change on any supported PHP version (8.2+). FieldsBuilder::mapDocBlock — skip @param tags with no variable name (phpdocumentor returns null there) instead of silently coercing null into an empty-string key. GlobTypeMapperCache::registerAnnotations — GlobAnnotationsCache's withType() sets typeClassName and typeName together, so inside the `typeClassName !== null` branch typeName is guaranteed non-null. Add an assert() to document the invariant for the analyser. IteratorTypeMapper::splitIteratorFromOtherTypes — restructure so the unset/return happens inside the loop where $key has a concrete array-key type, instead of carrying a nullable $key across the loop/early-return boundary. Also drops the unused null initializer. --- src/FieldsBuilder.php | 8 +++++++- src/Mappers/GlobTypeMapperCache.php | 4 ++++ src/Mappers/Root/IteratorTypeMapper.php | 18 ++++++------------ 3 files changed, 17 insertions(+), 13 deletions(-) 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..4afc7f1cc1 100644 --- a/src/Mappers/GlobTypeMapperCache.php +++ b/src/Mappers/GlobTypeMapperCache.php @@ -48,7 +48,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; } } From bbb17146998d83b209235b30a68581d003f8fbc5 Mon Sep 17 00:00:00 2001 From: Jacob Thomason Date: Fri, 17 Apr 2026 22:35:25 -0400 Subject: [PATCH 5/6] fix: coding-standard follow-ups for the 8.2 cs-check run - Add missing `use function assert;` import in GlobTypeMapperCache so the new assertion satisfies the Slevomat no-fallback-global-function rule. - Exclude SlevomatCodingStandard.TypeHints.ClassConstantTypeHint: the rule expects PHP 8.3 typed class constants, but our minimum supported PHP is 8.2 so the rule can't be satisfied. Re-enable when min PHP lifts to 8.3. --- phpcs.xml.dist | 3 +++ src/Mappers/GlobTypeMapperCache.php | 1 + 2 files changed, 4 insertions(+) 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/Mappers/GlobTypeMapperCache.php b/src/Mappers/GlobTypeMapperCache.php index 4afc7f1cc1..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 From a6223ecdc547658b2f006c011e217a83c46135a5 Mon Sep 17 00:00:00 2001 From: Jacob Thomason Date: Fri, 17 Apr 2026 22:38:44 -0400 Subject: [PATCH 6/6] ci: pin doctrine/coding-standard to ^13.0 only The ClassConstantTypeHint rule we exclude lives only in v13; on `--prefer-lowest` composer was picking v12 and phpcs errored with "Referenced sniff ... does not exist" when it hit the exclude. --- composer.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/composer.json b/composer.json index 3bac8dd8aa..5adda6bc52 100644 --- a/composer.json +++ b/composer.json @@ -28,7 +28,7 @@ }, "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",