Validation is processed before guarding

[wimski]

Describe the bug
When creating a mutation with both the @guard directive as well as the @validator directive on the input, the validation is processed before the guarding.

Expected behavior/Solution
I expect the guarding to be processed before the validation, because if someone is not authenticated, there's is no point in further handling the request with input validation.

Steps to reproduce

1. Create a mutation

extend type Mutation @guard {
    createStuff(input: StuffInput!): Stuff! @create
}

type Stuff @model(class: "App\\Models\\Stuff") {
    name: String!
}

input StuffInput @validator {
    name: String!
}

2. Create a validator

<?php

namespace App\GraphQL\Validators;

use Nuwave\Lighthouse\Validation\Validator;

class StuffInputValidator extends Validator
{
    public function rules(): array
    {
        return [
            'name' => [
                'string',
                'min:24',
            ],
        ];
    }
}

3. Create a test to check the unauthenticated error

<?php

namespace Tests;

use Illuminate\Contracts\Console\Kernel;
use Illuminate\Foundation\Application;
use Illuminate\Foundation\Testing\TestCase;
use Nuwave\Lighthouse\Testing\MakesGraphQLRequests;

class CreateStuffTest extends TestCase
{
    use MakesGraphQLRequests;

    public function createApplication(): Application
    {
        $app = require dirname(__DIR__) . DIRECTORY_SEPARATOR . 'bootstrap' . DIRECTORY_SEPARATOR . 'app.php';

        $app->make(Kernel::class)->bootstrap();

        return $app;
    }

    /**
     * @test
     */
    public function it_throws_an_error_if_the_user_is_unauthenticated(): void
    {
        $response = $this->graphQL(/** @lang GraphQL */ '
            mutation {
                createStuff(input: {
                    name: "not-long-enough"
                }) {
                    id
                }
            }
        ');
        
        static::assertContains('Unauthenticated.', $response->json('errors.*.message'));
    }
}

This test will fail, because the name attribute is not long enough. It should pass, because a user being unauthenticated should take precedence over validation.

Lighthouse Version
5.3.0

[Rubens10010]

Hi I have the following case too:

extend type Mutation @guard(with: ["api"]) { createPostulante(input: CreatePostulanteInput! @spread): Postulante @create }

The input has a @validator directive and it always goes first even if I put it after @create. That seems wrong since I want to authenticate my user before validation is attempted.

[wimski]

@spawnia I've managed to put something together that does the trick, but I'm totally not sure if this the way to go. Just thinking out loud.

New directive

<?php

namespace Nuwave\Lighthouse\Auth;

use Closure;
use GraphQL\Language\AST\DirectiveNode;
use Nuwave\Lighthouse\Schema\Directives\GuardDirective;
use Nuwave\Lighthouse\Schema\Values\FieldValue;

class AuthDirective extends GuardDirective
{
    public function handleField(FieldValue $fieldValue, Closure $next): FieldValue
    {
        if ($this->fieldHasGuardDirective($fieldValue)) {
            return parent::handleField($fieldValue, $next);
        }
    
        return $next($fieldValue);
    }
    
    protected function fieldHasGuardDirective(FieldValue $fieldValue): bool
    {
        /** @var DirectiveNode $directive */
        foreach ($fieldValue->getField()->directives as $directive) {
            if ($directive->name->value === 'guard') {
                $this->directiveNode = $directive;
                return true;
            }
        }
    
        return false;
    }
}

Config

'field_middleware' => [
    \Nuwave\Lighthouse\Schema\Directives\TrimDirective::class,
    \Nuwave\Lighthouse\Schema\Directives\SanitizeDirective::class,
    \Nuwave\Lighthouse\Auth\AuthDirective::class, // <--- before validation
    \Nuwave\Lighthouse\Validation\ValidateDirective::class,
    \Nuwave\Lighthouse\Schema\Directives\TransformArgsDirective::class,
    \Nuwave\Lighthouse\Schema\Directives\SpreadDirective::class,
    \Nuwave\Lighthouse\Schema\Directives\RenameArgsDirective::class,
],

[spawnia]

@wimski that would run @guard twice.

I think we should enhance the field middleware configuration to allow defining a default order for field middleware. That would allow interleaving optional field middleware directives such as @guard with global field middleware.

[andrewmclagan]

This is actually a security risk in many situations, exposing validation before authentication allows an attacker to gain information from the system e.g.

'email' => 'email:rfc,dns'

Allows an attacker to discover valid accounts without being authed.

[tasinttttttt]

I've come up with a workaround that uses a custom GraphQLContext:

The GraphQLContext object is referenced and can be accessed through all middleware and directives in the lighthouse pipeline.
The idea is then to add a validationError property to save the validation error and throw it after the guard/can directives have a chance to act on the request.

Schematically:

1. Add a validationError property in a custom GraphQLContext.
2. Add a custom ValidateDirective field middleware: if request contains guard or can directives, register the error in the graphqlContext, if not, throw a validation exception.
3. Add a custom Guard and Can directives: same as the default ones, with one step more: check if graphql context has a validation error, if so, throw it, if not, proceed.

Note that you have to update lighthouse.php config and make sure the custom graphqlcontext is properly registered (see AppServiceProvider.php in the gist).

Here's a link to a gist with a working implementation.

N.B.: I was hesitant to post this as it has at least one obvious flaw (having to track the project updates in order to reflect the changes in the custom files). But I thought it could show how the current pipeline can't be used to solve the issue and has to be rethought. Also: works :)

[nyelnizy]

I believe this issue has to be clearly stated somewhere in the docs and let people decide whether to use the library or not. Its a serious issue and exposes a lot of security vulnerabilities.

[yondifon]

I believe this issue has to be clearly stated somewhere in the docs and let people decide whether to use the library or not. Its a serious issue and exposes a lot of security vulnerabilities.

I disagree @nyelnizy

True that if you choose to implement like this - well that's at your own risk.

But normally there's a lot that goes on in validation (Also be consistent with Laravel Validation). With this that means even little things like validating a user changing their email is not possible except in the resolver.

[gwachhamit]

@spawnia I've managed to put something together that does the trick, but I'm totally not sure if this the way to go. Just thinking out loud.

New directive

<?php

namespace Nuwave\Lighthouse\Auth;

use Closure;
use GraphQL\Language\AST\DirectiveNode;
use Nuwave\Lighthouse\Schema\Directives\GuardDirective;
use Nuwave\Lighthouse\Schema\Values\FieldValue;

class AuthDirective extends GuardDirective
{
    public function handleField(FieldValue $fieldValue, Closure $next): FieldValue
    {
        if ($this->fieldHasGuardDirective($fieldValue)) {
            return parent::handleField($fieldValue, $next);
        }
    
        return $next($fieldValue);
    }
    
    protected function fieldHasGuardDirective(FieldValue $fieldValue): bool
    {
        /** @var DirectiveNode $directive */
        foreach ($fieldValue->getField()->directives as $directive) {
            if ($directive->name->value === 'guard') {
                $this->directiveNode = $directive;
                return true;
            }
        }
    
        return false;
    }
}

Config

'field_middleware' => [
    \Nuwave\Lighthouse\Schema\Directives\TrimDirective::class,
    \Nuwave\Lighthouse\Schema\Directives\SanitizeDirective::class,
    \Nuwave\Lighthouse\Auth\AuthDirective::class, // <--- before validation
    \Nuwave\Lighthouse\Validation\ValidateDirective::class,
    \Nuwave\Lighthouse\Schema\Directives\TransformArgsDirective::class,
    \Nuwave\Lighthouse\Schema\Directives\SpreadDirective::class,
    \Nuwave\Lighthouse\Schema\Directives\RenameArgsDirective::class,
],

@wimski how is this respected in lighthouse 6?

[wimski]

@gwachhamit I have no idea. This was just an idea that I tested out, but never actually implemented in any real project. So your guess is as good as mine. I'm still hoping on a fundamental solution within the package, because this is not something I want want to attach myself on a project basis.

[RahulDey12]

This whole issue can be solved if we can modify the @guard directive in a way that it can be used in input types