Fix #[Security] on ExtendType fields (source-injection array_combine crash)#792
Merged
oojacoboo merged 6 commits intothecodingmachine:masterfrom Apr 18, 2026
Conversation
…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.
oojacoboo
force-pushed
the
fix/security-field-middleware-extend-type-source-injection
branch
from
7ba5654 to
dafc8b2
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #792 +/- ##
============================================
- Coverage 95.72% 94.78% -0.95%
- Complexity 1773 1858 +85
============================================
Files 154 175 +21
Lines 4586 4888 +302
============================================
+ Hits 4390 4633 +243
- Misses 196 255 +59 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
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.
- 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.
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.
Collaborator
Author
|
This PR removes support for PHP 8.1, following official EOL security support. Additionally, PHP 8.5 is now officially supported and added to the test matrix. |
oojacoboo
deleted the
fix/security-field-middleware-extend-type-source-injection
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
#[Security]on an#[ExtendType]field method throws at runtime with:In most setups 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.Root cause
SecurityFieldMiddleware::process()captures\$parametersbeforeQueryField::fromFieldDescriptorprepends aSourceParameter(it does this whenisInjectSource()is true). At resolver invocation time\$argstherefore includes the source as its first element while\$parameters(captured earlier) does not, so thearray_combine(\$argsName, \$args)call fails.AuthorizationFieldMiddleware(@Logged/@Right) sidesteps the issue by usingfunction (...\$args) use (...)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
SecurityInputFieldMiddlewarefor input field factories where source is similarly injected byInputField::fromFieldDescriptor.Fix
Pass
isInjectSource()through togetVariables()and slice the leading source arg off `$args` before thearray_combine. The source is still available viathisin the expression context, so no information is lost for Security expressions.Tests
New regression test in `EndToEndTest::testEndToEndSecurityOnExtendTypeField` — adds a `#[Security]`-decorated field to the existing `ExtendedContactType` fixture and verifies both the `failWith: null` path (no user logged in) and the authorized path (user.bar == 42) resolve correctly. Before the fix the test throws the `array_combine` `TypeError`; after, it passes.
All existing tests continue to pass (494/494).
Checklist