Skip to content

Commit 7ba5654

Browse files
committed
Fix #[Security] on ExtendType fields (source-injection array_combine crash)
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.
1 parent dd1fc8b commit 7ba5654

4 files changed

Lines changed: 105 additions & 9 deletions

File tree

src/Middlewares/SecurityFieldMiddleware.php

Lines changed: 19 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,9 @@
1818

1919
use function array_combine;
2020
use function array_keys;
21+
use function array_slice;
2122
use function assert;
23+
use function count;
2224

2325
/**
2426
* A field middleware that reads "Security" Symfony annotations.
@@ -73,9 +75,15 @@ public function process(
7375
$originalResolver = $queryFieldDescriptor->getOriginalResolver();
7476

7577
$parameters = $queryFieldDescriptor->getParameters();
78+
// QueryField::fromFieldDescriptor prepends a SourceParameter to the parameters list when
79+
// `injectSource` is true, but that injection happens AFTER this middleware runs. At resolver
80+
// invocation time the runtime $args therefore includes the source as its first element while
81+
// $parameters captured here does not. Track the flag so getVariables() can strip the leading
82+
// source arg before zipping args to parameter names.
83+
$injectSource = $queryFieldDescriptor->isInjectSource();
7684

77-
$queryFieldDescriptor = $queryFieldDescriptor->withResolver(function (object|null $source, ...$args) use ($originalResolver, $securityAnnotations, $resolver, $failWith, $parameters, $queryFieldDescriptor) {
78-
$variables = $this->getVariables($args, $parameters, $originalResolver->executionSource($source));
85+
$queryFieldDescriptor = $queryFieldDescriptor->withResolver(function (object|null $source, ...$args) use ($originalResolver, $securityAnnotations, $resolver, $failWith, $parameters, $queryFieldDescriptor, $injectSource) {
86+
$variables = $this->getVariables($args, $parameters, $originalResolver->executionSource($source), $injectSource);
7987

8088
foreach ($securityAnnotations as $annotation) {
8189
try {
@@ -108,7 +116,7 @@ public function process(
108116
*
109117
* @return array<string, mixed>
110118
*/
111-
private function getVariables(array $args, array $parameters, object|null $source): array
119+
private function getVariables(array $args, array $parameters, object|null $source, bool $injectSource = false): array
112120
{
113121
$variables = [
114122
// 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
118126
'this' => $source,
119127
];
120128

129+
// Strip the source arg prepended by QueryField::fromFieldDescriptor so the remaining
130+
// user-supplied args line up positionally with the captured parameter names. The source
131+
// is always exposed via `this`, so there's no loss of information for the expression.
132+
if ($injectSource && count($args) > count($parameters)) {
133+
$args = array_slice($args, 1);
134+
}
135+
121136
$argsName = array_keys($parameters);
122-
$argsByName = array_combine($argsName, $args);
137+
$argsByName = $argsName ? array_combine($argsName, $args) : [];
123138

124139
return $variables + $argsByName;
125140
}

src/Middlewares/SecurityInputFieldMiddleware.php

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,8 @@
1515

1616
use function array_combine;
1717
use function array_keys;
18+
use function array_slice;
19+
use function count;
1820

1921
/**
2022
* A field input middleware that reads "Security" Symfony annotations.
@@ -43,9 +45,13 @@ public function process(InputFieldDescriptor $inputFieldDescriptor, InputFieldHa
4345
$originalResolver = $inputFieldDescriptor->getOriginalResolver();
4446

4547
$parameters = $inputFieldDescriptor->getParameters();
48+
// InputField::fromFieldDescriptor prepends a SourceParameter to the parameters list when
49+
// `injectSource` is true, but that injection happens AFTER this middleware runs. See the
50+
// sibling SecurityFieldMiddleware for the detailed comment.
51+
$injectSource = $inputFieldDescriptor->isInjectSource();
4652

47-
$inputFieldDescriptor = $inputFieldDescriptor->withResolver(function (object|null $source, ...$args) use ($originalResolver, $securityAnnotations, $resolver, $parameters, $inputFieldDescriptor) {
48-
$variables = $this->getVariables($args, $parameters, $originalResolver->executionSource($source));
53+
$inputFieldDescriptor = $inputFieldDescriptor->withResolver(function (object|null $source, ...$args) use ($originalResolver, $securityAnnotations, $resolver, $parameters, $inputFieldDescriptor, $injectSource) {
54+
$variables = $this->getVariables($args, $parameters, $originalResolver->executionSource($source), $injectSource);
4955

5056
foreach ($securityAnnotations as $annotation) {
5157
try {
@@ -71,18 +77,22 @@ public function process(InputFieldDescriptor $inputFieldDescriptor, InputFieldHa
7177
*
7278
* @return array<string, mixed>
7379
*/
74-
private function getVariables(array $args, array $parameters, object|null $source): array
80+
private function getVariables(array $args, array $parameters, object|null $source, bool $injectSource = false): array
7581
{
7682
$variables = [
7783
// If a user is not logged, we provide an empty user object to make usage easier
7884
'user' => $this->authenticationService->getUser(),
79-
'authorizationService' => $this->authorizationService, // Used by the is_granted expression language function.
85+
'authorizationService' => $this->authenticationService, // Used by the is_granted expression language function.
8086
'authenticationService' => $this->authenticationService, // Used by the is_logged expression language function.
8187
'this' => $source,
8288
];
8389

90+
if ($injectSource && count($args) > count($parameters)) {
91+
$args = array_slice($args, 1);
92+
}
93+
8494
$argsName = array_keys($parameters);
85-
$argsByName = array_combine($argsName, $args);
95+
$argsByName = $argsName ? array_combine($argsName, $args) : [];
8696

8797
return $variables + $argsByName;
8898
}

tests/Fixtures/Integration/Types/ExtendedContactType.php

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66

77
use TheCodingMachine\GraphQLite\Annotations\ExtendType;
88
use TheCodingMachine\GraphQLite\Annotations\Field;
9+
use TheCodingMachine\GraphQLite\Annotations\Security;
910
use TheCodingMachine\GraphQLite\Fixtures\Integration\Models\Contact;
1011

1112
use function strtoupper;
@@ -34,4 +35,17 @@ public function company(Contact $contact): string
3435
{
3536
return $contact->getName() . ' Ltd';
3637
}
38+
39+
/**
40+
* Regression: #[Security] on an ExtendType field used to blow up with
41+
* "array_combine(): ... must have the same number of elements" because
42+
* SecurityFieldMiddleware didn't mirror the source-injection that
43+
* QueryField::fromFieldDescriptor performs.
44+
*/
45+
#[Field]
46+
#[Security("user && user.bar == 42", failWith: null)]
47+
public function extendedSecretName(Contact $contact): string|null
48+
{
49+
return $contact->getName();
50+
}
3751
}

tests/Integration/EndToEndTest.php

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1338,6 +1338,63 @@ public function testEndToEndSecurityInField(): void
13381338
$this->assertSame('Access denied.', $result->toArray(DebugFlag::RETHROW_UNSAFE_EXCEPTIONS)['errors'][0]['message']);
13391339
}
13401340

1341+
/**
1342+
* Regression test for #[Security] on an #[ExtendType] field method.
1343+
*
1344+
* SecurityFieldMiddleware captures $parameters at process() time, but
1345+
* QueryField::fromFieldDescriptor prepends an implicit SourceParameter
1346+
* AFTER the middleware runs when isInjectSource() is true. At runtime the
1347+
* resolver receives N+1 args for N captured parameter names, which used
1348+
* to blow array_combine() up with "Argument #1 ($keys) and argument #2
1349+
* ($values) must have the same number of elements".
1350+
*/
1351+
public function testEndToEndSecurityOnExtendTypeField(): void
1352+
{
1353+
// No logged-in user → the Security expression `user.bar == 42` fails, failWith returns null.
1354+
$schema = $this->mainContainer->get(Schema::class);
1355+
assert($schema instanceof Schema);
1356+
1357+
$queryString = '
1358+
query {
1359+
contacts {
1360+
name
1361+
extendedSecretName
1362+
}
1363+
}
1364+
';
1365+
1366+
$result = GraphQL::executeQuery($schema, $queryString);
1367+
$data = $this->getSuccessResult($result);
1368+
$this->assertSame('Joe', $data['contacts'][0]['name']);
1369+
$this->assertNull($data['contacts'][0]['extendedSecretName']);
1370+
1371+
// Logged-in user with bar=42 → the Security expression passes and the field returns the name.
1372+
$container = $this->createContainer([
1373+
AuthenticationServiceInterface::class => static function () {
1374+
return new class implements AuthenticationServiceInterface {
1375+
public function isLogged(): bool
1376+
{
1377+
return true;
1378+
}
1379+
1380+
public function getUser(): object|null
1381+
{
1382+
$user = new stdClass();
1383+
$user->bar = 42;
1384+
return $user;
1385+
}
1386+
};
1387+
},
1388+
]);
1389+
1390+
$schema = $container->get(Schema::class);
1391+
assert($schema instanceof Schema);
1392+
1393+
$result = GraphQL::executeQuery($schema, $queryString);
1394+
$data = $this->getSuccessResult($result);
1395+
$this->assertSame('Joe', $data['contacts'][0]['extendedSecretName']);
1396+
}
1397+
13411398
public function testEndToEndUnions(): void
13421399
{
13431400
$schema = $this->mainContainer->get(Schema::class);

0 commit comments

Comments
 (0)