Providers are executed before any security check

[Cocray]

API Platform version(s) affected: >=3.1.0

Description

I don't really know if it's a bug, but in the AccessCheckerProvider, the provider is executed before any security check. I may have missed something, but IMHO, we may have:

• a security failure if the provider is executing something the user is not allowed to do.
• a perf killer if the provider is executing long stuff whereas in the end, the response will be "you can't access this endpoint"

So I have to rewrite the security in some of my providers to check the access rights in order to avoid what I mentioned above

How to reproduce

Any code such as this small one:

#[ApiResource(
    operations: [
        new GetCollection(
            security: 'is_granted("TEST")',
            provider: TestProvider:class,
        ),
    ]
)]
class Test
{
}

Possible Solution

In AccessCheckerProvider.php, this line is executed before any security check:
$body = $this->decorated->provide($operation, $uriVariables, $context);

By the way, I really don't know the impacts for this part a few lines below :(
if (!$this->resourceAccessChecker->isGranted($operation->getClass(), $isGranted, $resourceAccessCheckerContext)) {

Additional Context

[soyuka]

As the resource access checker has an object param we need to fetch the data. As the data is never sent (in case of a falsy security check) I don't see the security issue. Also please if you find a security issue report it on https://github.com/api-platform/core/security/advisories/new thanks!

You can also create your provider that decorates ours applying some security checks before if needed.

[Cocray]

Thanks @soyuka for your answer, I understand the need.

By the way, when I said it could be a security issue, I don't mean the endpoint response, for the reason you explained, but anything the developer could do in the provider. This is not my case, but I have in mind something like that:

• In the provider, I identify the email adress of the current user and send an email with some data returned
• If the user is not granted by voters, he'll receive the email anyway

I think that's just a good point to know to be careful of what we should do / shouldn't do in the provider because a developer could think that the security part blocks any users not granted.

I close this (false) issue :)

[soyuka]

Sending an email should be done inside processors instead.

[soyuka]

@Miras4207 @Cocray you're very welcome to update our documentation with this. It's also quite easy to decorate the ReadProvider https://api-platform.com/docs/core/extending/#decoration-example and adding your security layer there, let me know if you need more assistance.

[Miras4207]

@soyuka

How is this not considered to be a security issue? The attribute argument is literally called security - and it checks the security condition after the operation is already executed. It does not matter the data are not sent in a response, the problem is that an unauthorized client executed an operation that should be accessible to authorized clients only.

The State Provider is supposed to be used for GET operations - and it's not always just about simply obtaining and providing data. There could be a lot more going on, i.e. resource or time expensive operations, generating or persisting data and other wanted side effects for various reasons. Some examples were given here, others were given in my issue post.

The State Processor is not supposed to be used for GET operations (correct me if I am wrong). That is meant for operations such as POST. However, some operations are supposed to be GET and at the same time manipulate data or have other side effects - as a wild example, you may want to have GET /api/sequence-number endpoint that increments number in a database and returns the new value. The current implementation would allow unauthorized clients to increment this sequence, even if the new value is not sent in a response.

I understand that you may want to use object in the security expression, but due to such security implications, this should not be allowed within security argument. Instead, there should be something like securityPostProvider (or better name) that would allow to perform security check based on data returned by the provider.

Having to use Symfony Firewall or extension points to actually achieve security undermines the utility of the security argument.

Please give this matter a second thought.