Don't check for the security on null obj on graphql

[nuryagdym]

Hi,

I have an security issue with GraphQl endpoint

example graphql query:

{
  intent(id: "/api/intents/30") {
    mainEconomicActivity {
      isMainActivity
    }
  }
}

problem here is that when intent.mainEconomicActivity is null api-platform still tries to run the security check which is causing the request to fail with, but when it is not null then the code is working properly. I enabled both RestAPI and GraphQl in my project and the problem exists only with GraphQl.

the error:

      "extensions": {
        "debugMessage": "Unable to call method \"getIntent\" of non-object \"object\".",
        "file": "/var/www/api/vendor/symfony/expression-language/Node/GetAttrNode.php",
        "line": 111,
        "trace": [
          {
            "file": "/var/www/api/vendor/symfony/expression-language/Node/GetAttrNode.php",

Resources:

<?php
namespace App\Entity;

use ApiPlatform\Metadata\ApiProperty;
use ApiPlatform\Metadata\ApiResource;
use ApiPlatform\Metadata\Get;
use ApiPlatform\Metadata\GetCollection;
use ApiPlatform\Metadata\GraphQl\Query;
use ApiPlatform\Metadata\GraphQl\QueryCollection;
use Doctrine\Common\Collections\ArrayCollection;
use Doctrine\Common\Collections\Collection;
use Doctrine\DBAL\Types\Types;
use Doctrine\ORM\Mapping as ORM;
use Gedmo\Mapping\Annotation\Blameable;
use Symfony\Component\Serializer\Annotation\Groups;

#[ApiResource(
    operations: [
        new Get(security: "object.getUserId() == user.getUserIdentifier()"),
        new GetCollection(),
    ],
    normalizationContext: ['skip_null_values' => false, 'groups' => ['intent:read']],
    graphQlOperations: [
        new Query(security: "object.getUserId() == user?.getUserIdentifier()"),
        new QueryCollection(),
    ],
)]
#[ORM\Entity]
#[ORM\Index(columns: ['user_id'])]
class Intent
{
    #[ORM\Id]
    #[ORM\Column(type: Types::INTEGER)]
    #[ORM\GeneratedValue]
    #[Groups(['intent:read'])]
    private ?int $id = null;

    #[ORM\Column]
    #[Blameable(on: 'create')]
    #[Groups(['intent:read'])]
    private string|null $userId = null;

    /**
     * @var Collection<IntentEconomicActivity>
     */
    #[ORM\OneToMany(mappedBy: 'intent', targetEntity: IntentEconomicActivity::class, cascade: ['persist', 'remove'])]
    #[Groups(['intent:read'])]
    private Collection $economicActivities;

    public function __construct()  {
        $this->economicActivities = new ArrayCollection();
    }
    public function getId(): ?int {
        return $this->id;
    }
    public function getUserId(): ?string  {
        return $this->userId;
    }

    // this is the property dynamic property that I am trying to access
     #[Groups(['intent:read'])]
    public function getMainEconomicActivity(): ?IntentEconomicActivity
    {
        $mainActivities = $this->economicActivities->filter(fn(IntentEconomicActivity $item) => $item->getIsMainActivity());
        return $mainActivities[0] ?? null;
    }
}

<?php
namespace App\Entity;

use ApiPlatform\Metadata\ApiResource;
use ApiPlatform\Metadata\Get;
use ApiPlatform\Metadata\GraphQl\Query;
use ApiPlatform\Metadata\Link;
use Doctrine\DBAL\Types\Types;
use Doctrine\ORM\Mapping as ORM;
use Symfony\Component\Serializer\Annotation\Groups;

#[ApiResource(
    uriTemplate: '/intents/{intentId}/intent_economic_activities/{id}',
    operations: [
        new Get(security: "object.getIntent().getUserId() == user.getUserIdentifier()"),
    ],
    uriVariables: [
        'intentId' => new Link(toProperty: 'intent', fromClass: Intent::class),
        'id'       => new Link(fromClass: IntentEconomicActivity::class),
    ],
    normalizationContext: ['skip_null_values' => false, 'groups' => ['intent:read']],
    graphQlOperations: [
       // this expression is almost the same as for restAPI
       // as mentioned in the error "object.getIntent()" this check is failing because object is null. 
       // it also works if I change expression to "null === object || object.getIntent().getUserId() == user?.getUserIdentifier()"
       // but I prefer when it is the same expression as in RestAPI
        new Query(security: "object.getIntent().getUserId() == user?.getUserIdentifier()"),
    ],
)]
#[ORM\Entity]
#[ORM\UniqueConstraint(columns: ['intent_id', 'economic_activity_code'])]
class IntentEconomicActivity
{
    #[ORM\Id]
    #[ORM\Column(type: Types::INTEGER)]
    #[ORM\GeneratedValue]
    #[Groups(['intent:read'])]
    private ?int $id = null;

    #[ORM\ManyToOne(targetEntity: Intent::class, inversedBy: 'economicActivities')]
    #[ORM\JoinColumn(nullable: false)]
    #[Groups(['intent:read'])]
    private Intent $intent;

    #[ORM\Column(type: Types::BOOLEAN)]
    #[Groups(['intent:read'])]
    private bool $isMainActivity = false;

    public function getId(): ?int  {
        return $this->id;
    }

    public function getIntent(): Intent  {
        return $this->intent;
    }

    public function getIsMainActivity(): bool  {
        return $this->isMainActivity;
    }
}

[nuryagdym]

some checks are failing. Is there any documentation on how to set up php cs fixer and commit linting on my local? There is not information about it in here https://github.com/api-platform/core/blob/main/CONTRIBUTING.md

the given command

php-cs-fixer.phar fix

is not available command on my lcal

[alanpoulain]

I think it should not be handled by the code but in your security expression: null !== object and (object.getUserId() == user?.getUserIdentifier())

[nuryagdym]

yes @alanpoulain changing it to null === object || object.getIntent().getUserId() == user?.getUserIdentifier() works. But should the same expressions behave in the same way in RestAPI?

[alanpoulain]

I'm not sure what you mean, could you develop ?

[nuryagdym]

firstly, why there is need a security check on a null object? secondly, isn't it better when I use the same security expression for both RestAPI and GraphQl and expect the same behaviour?

[alanpoulain]

Is REST checking if the object is not null?

[nuryagdym]

for the /api/intents/31 request REST api does not do security check for intent.mainEconomicActivity at all, even though I am fetching it as well.

It is running security check when only for sub resource request /api/intents/{intentId}/intent_economic_activities/{activityId} And if activity does not exist then HTTP 404.

GraphQl is using Resolver for all resources and sub resources and resolver passes it through security check.

[alanpoulain]

I see, REST doesn't really "traverse" the intent, so the problem does not exist. We cannot compare them. I'm not sure if it's a good idea or not to check the nullability. It could be too "magical" for a security expression. Let the other members of @api-platform/core-team decide.

[soyuka]

just add object !== null in your Expression language?

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.