Avoid side-effects from mutating argument default value objects

[er1z]

Hello,

if PHP is configured via FPM, then it's very hard to spot. Recently, I moved with the application to FrankenPHP (but I bet other worker-alike runtimes will reveal similar behavior). Let's start with a resolve function:

'items' => [
    'type' => Type::listOf(Type::string()),
    'args' => [
        'filter' => [
            'type'         => $filterType,
            'defaultValue' => new Filter(),
        ],
    ],
    'resolve' => function ($root, array $args): array {
        static $counter = 0;

        $filter = $args['filter'];

        if ($counter++ == 0) {
            $args['filter']->status = 'polluted';
        }

        return [
            "status={$filter->status}",
            "limit={$filter->limit}",
        ];
    },
],

This is a little bit synthetic proof of concept, but I want to set the status to polluted only for a first response. Other should return a default value that's held as a public property default value of Filter class.

class Filter
{
    public function __construct(
        public string $status = 'active',
        public int $limit = 10,
    ) {}
}

But:

$schema = new Schema(['query' => $queryType]);

$gql = '{ items }';

echo "=== Request 1: ===" . PHP_EOL;
$result = GraphQL::executeQuery($schema, $gql);
print_r($result->toArray());

echo PHP_EOL . "=== Request 2: ===" . PHP_EOL;
$result = GraphQL::executeQuery($schema, '{ items }');
print_r($result->toArray());

Results in:

=== Request 1: ===
Array
(
    [data] => Array
        (
            [items] => Array
                (
                    [0] => status=polluted
                    [1] => limit=10
                )
        )
)

=== Request 2: ===
Array
(
    [data] => Array
        (
            [items] => Array
                (
                    [0] => status=polluted
                    [1] => limit=10
                )
        )
)

I dug what's going on and it turns out if the defaultValue is an object, it's always passed as a reference:

The fix is an oneliner (ReferenceExecutor):

$args = $this->fieldArgsCache[$fieldDef][$fieldNode] ??= $argsMapper(Values::getArgumentValues(
    $fieldDef,
    $fieldNode,
    $this->exeContext->variableValues,
    $this->exeContext->schema,
), $fieldDef, $fieldNode, $contextValue);

$args = unserialize(serialize($args));

return $resolveFn($rootValue, $args, $contextValue, $info);

And then everything works fine:

If you are fine with a solution with unserialize/serialize, then I'll prepare a pull request with a fix.

[mfn]

Does defaultValue accept a closure? You could then return a new Filter object from it, maybe that works?

[er1z]

Checked, didn't work.

[spawnia]

I think it is reasonable to expect that mutating objects may have side effects in PHP. Avoid mutating args I guess? Always copying user-defined values seems like a shotgun approach to paper over issues with the language itself and your code in particular and would be pretty wasteful and unnecessary for most uses.

[er1z]

@spawnia I guess it's not expected that a resolving callback pollutes library config and degrades DX to find out what's going on: why defaultValue object is being changed by very last resolve callback?

[jkobus]

@spawnia the problematic part here is that graphql-php passes the exact same object instance from the schema definition into resolver args. From the resolver perspective, $args looks request-local, but mutating it can silently mutate schema-level state and affect later requests.

This is almost invisible under PHP-FPM, but becomes a real production issue under FrankenPHP worker mode, RoadRunner, Swoole, etc.

I agree that serialize/unserialize is probably not the right fix. But I think the library should either:

1. explicitly document that object defaultValue is not safe and must be immutable / cloned by userland, or
2. clone object default values before passing them to resolver args, or
3. reject object default values if they cannot be safely copied.

FrankenPHP worker mode is a very relevant deployment model for GraphQL APIs because schema construction and type loading are expensive. This makes the issue more than theoretical: the library currently allows a very subtle cross-request state leak.

[spawnia]

Okay, I can see how a schema-level configuration item bleeding into the request lifecycle is unwanted for long-running deployment options. Can we somehow determine if an argument's default value is safe to clone?

I put the label needs reproduction for now, as I think the next step towards a solution is to implement test cases that demonstrate the problem and evaluate a potential fix from there.

[er1z]

On my way, will provide a test case.

[er1z]

Here it goes: master...er1z:graphql-php:default-value-pollution