[9.x] Allow authorization responses to specify HTTP status codes

[timacdonald]

This PR introduces the ability to set the status code for a denied authorization action in policies, gates, and wherever Auth\Access\Response or AuthorizationException are handled by the framework.

Let's take the following example policy where only the authenticated user can view themselves...

Route::get('users/{user}', fn () => UserResource::make($user))->can('view', 'user');

class UserPolicy
{
    use HandlesAuthorization;

    public function view(User $authenticated, User $user)
    {
        return $authenticated->is($user)
            ? $this->allow()
            : $this->deny();
    }
}

Given the above scenario, any user is now able to determine how many users the system currently has if the system is using auto-incrementing IDS...

actingAs($me)->get('users/9991') // 403 😏
actingAs($me)->get('users/9992') // 403 😏
actingAs($me)->get('users/9993') // 404 😮

This PR drastically improves (but does not totally solve**) this scenario by allowing developers to specify the HTTP response code that should be returned when a "forbidden" action is attempted.

class UserPolicy
{
    use HandlesAuthorization;

    public function view(User $authenticated, User $user)
    {
        return $authenticated->is($user)
            ? $this->allow()
            : $this->deny()->asNotFound(); // 404 status code
    }
}

actingAs($me)->get('users/9991') // 404 🤔
actingAs($me)->get('users/9992') // 404 🤔
actingAs($me)->get('users/9993') // 404 🤔

This is what I would consider a security "best practice" in many, but not all, scenarios. You can see it all across the web. GitHub, for example, will show a 404 for repositories that exist or not when you are not allowed to access them, for example...

https://github.com/timacdonald/config-differ

The above repository exists...

but because it is private, everyone else except for myself will see a 404 response, thus keeping its existence secret.

As you can see from my initial example and then this GitHub example, this feature is not only useful for incrementing IDs, but generally for hiding the existence of any resource**.

Full API

Using Illuminate\Auth\Access\HandlesAuthorization e.g. Policies...

use HandlesAuthorization;

public function view(User $user, Post $post)
{
    return $this->deny(/* ... */)->withStatus(404);

    return $this->deny(/* ... */)->asNotFound();

    return $this->denyWithStatus(418);

    return $this->denyWithStatus(418, 'foo', 3);

    return $this->denyAsNotFound();

    return $this->denyAsNotFound('foo', 3);
}

Returning Illuminate\Auth\Access\Response e.g. Gates...

Gate::define('implode-universe', function (User $user) {
    return Response::deny(/* ... */)->withStatus(418);

    return Response::deny(/* ... */)->asNotFound();

    return Response::denyWithStatus(418);

    return Response::denyWithStatus(418, 'foo', 3);

    return Response::denyAsNotFound();

    return Response::denyAsNotFound('foo', 3);
});

Directly throwing the exception....

throw (new AuthorizationException)->withStatus(418);

throw (new AuthorizationException)->asNotFound();

The exception is translated in the exception handler to send the appropriate HTTP status response.

Why not just use abort()

I'm glad you asked! There is an argument to just utilise the abort helper here...

public function view(User $user, Post $post)
{
    /* ... */
    
    // return $this->denyWithStatus(404);

    abort(404);
}

However this has a few downfalls.

1. It bubbles up a HTTP exception, which means in the exception handler you have lost the context of the exception. Knowing the type of exception can be important for logging and detecting bad actors, threat detection, etc. Having the AuthorizationException bubble up to the exception handler allows you to categorize and appropriately handle that type of exception as needed by your application.
2. You lose the ability to specify an exception code. The abort helper allows you to specify a HTTP status code, but not the underlying exception code. status code != exception code. Exception codes are generally utilised on applications as internal mappings to exact issues within the application. The following example is how that may be used...

public function view(User $user, Post $post)
{
    /* ... */
    
    if ($user->isInactive()) {
        return $this->deny(Errors::INACTIVE_USER['message'], Errors::INACTIVE_USER['code'])
            ->withStatus(404);
    }

    if ($user->isNotAdmin()) {
        return $this->deny(Errors::NOT_ADMIN['message'], Errors::NOT_ADMIN['code'])
            ->withStatus(404);
    }
    
    // etc...
}

This allows developers to handle the exception type while logging an exception message, exception code, and translating to an appropriate HTTP status code for security reasons.

Form Requests

Another place to perform authorization is within Form Requests. You can set a response status in the form request by using the new helpers on the exception.

class StoreUserRequest extends FormRequest
{
    /**
     * Handle a failed authorization attempt.
     *
     * @return void
     *
     * @throws \Illuminate\Auth\Access\AuthorizationException
     */
    protected function failedAuthorization()
    {
        throw (new AuthorizationException)->asNotFound();
    }

    /* ... */
}

Not supporting default status per policy

In my initial implementation, I supported a default deny status per policy. This status was then shared to all the deny responses on the policy. This worked by having the policy implement an "implicit property contract" e.g.

class PostPolicy
{
    use HandlesAuthorization;

    protected $defaultDenyStatus = 404;

    public function view(User $user, Post $post)
    {
        return $this->deny(); // gets "404"
    }

    public function viewAny(User $user)
    {
        return $this->deny()->withStatus(403); // gets "403"
    }
}

This felt really nice and I felt smart 🤓 The implementation for this was housed on the HandlesAuthorization trait. In my opinion, this starts to break down. Take the following policy...

class PostPolicy
{
    use HandlesAuthorization;

    protected $defaultDenyStatus = 404;

    public function view(User $user, Post $post)
    {
        return false;
    }

    public function viewAny(User $user)
    {
        return Response::deny();
    }

    public function create(User $user)
    {
        return $this->deny();
    }
}

Should these all get the default 404 response status? I feel like developers might feel like they should and I was worried it might create some confusion. As it stands, only the create would get the 404, as the implementation was handled in the HandlesAuthorization trait.

I could look at pushing this functionality up into the Gate, where it could apply the default status if it is denied with false, Response::deny(), or $this->deny(), but then I'm wondering if it would have to be an actual interface as I believe our implicit property contracts are usually internal and thus protected. We could make it that the property needs to be public, but that feels flakey and like it could also cause issues as people might declare it as protected.

So then you'd need to implement an interface to support the default functionality...and that feels like more work than just being explicit in the policy.

Converting to 404 globally and just handling other cases

It might be the case that developers actually want all authorization exceptions to be rendered as 404, and instead be explicit about what denied actions should show a 403 instead. This would be achievable by doing the following in the exception handler...

protected function prepareException(Throwable $e)
{
    return match(true) {
        $e instanceof AuthorizationException && ! $e->hasStatus() => new NotFoundHttpException($e->getMessage(), $e),
        default => parent::prepareException($e)
    };
}

This now converts all AuthorizationException that do not have an explicit status set to a 404 response. Developers are then able to opt back into a 403 response with the following...

class PostPolicy
{
    use HandlesAuthorization;

    public function view(User $user, Post $post)
    {
        return $this->denyWithStatus(403);
    }
}

Notes

• **This PR improves, but does not completely solve the problem, as there is still the possibility of timing attacks.
• It is possible to set the status on an "allow" action, however it is discarded in the same way that the "message" and exception "code" are discarded.

[timacdonald]

Note to self:

• show how you can globally make the default a “404” when no status is set.
• Justify why you can’t set a default per policy.

[fgaroby]

Would it be possible to disable this feature in development mode (maybe the same way we did with Model::preventLazyLoading() in AppServiceProvider@boot()) ?

[valorin]

@timacdonald This is an awesome addition! 😍

A couple of notes:

• show how you can globally make the default a “404” when no status is set.
• Justify why you can’t set a default per policy.

Is it a business decision to not support a default per policy?

It seems to me that supporting a per policy default would be easier than a global default. The policy is inheriting a trait, so you can define a parameter on the trait and have the trait use it when building the Response. The deny() method can use denyWithStatus() internally and just pass a status=null when there isn't a default set.
But I would definitely support both. If you want 404's thrown in some places, you'll likely want them everywhere.

Would it be possible to disable this feature in development mode (maybe the same way we did with Model::preventLazyLoading() in AppServiceProvider@boot()) ?

Maybe it would be better for the debug mode 404 screen to expand on the 404 context. HttpException receives the previous exception, so it could display a 404 alongside the 403 with the details of both. That way you know the 404 is working - and can assert it in tests - but still get the extra context needed.

[timacdonald]

@valorin I've just updated the original description with reasoning around why I removed the default status from the PR. See: "Not supporting default status per policy".

@fgaroby @valorin I'm still thinking on if I think the framework should offer a switch for debug mode on this. You already have the ability to handle this in your own app of course.

// in the exception handler...

protected function prepareException(Throwable $e)
{
    if (config('app.debug') && $e instanceof AuthorizationException) {
        return parent::prepareException($e->withStatus(null));
    }

    return parent::prepareException($e);
}

Gonna ponder on this today a bit more. Happy to hear any more feedback though!

[mattkingshott]

This seems like a no-brainer to me. Great work! 👍🏻

[timacdonald]

Thanks for all the feedback on this folks. If anyone has ideas on how this could be improved now the base functionality it in place, please feel free to circle back and suggest further additions. I think a default can still be achieved as well as a debug thing.

[valorin]

@valorin I've just updated the original description with reasoning around why I removed the default status from the PR. See: "Not supporting default status per policy".

Ah, very good points as to why. Thanks. 🙂

I think this is an awesome addition, nice work. 👍

[timacdonald]

Thanks for reviewing it. When it comes to security related things, you are my go to community member, so appreciate you taking the time!

[valorin]

I'm always happy to review anything security related. 😁

[taylorotwell]

@timacdonald can you send in docs for this?

[BenWalters]

@timacdonald really helpful! One thing I have picked up on that I'd appreciate your thoughts on... the AuthorizationException defaults the message to This action is unauthorized. when one isn't set. So when using denyAsNotFound you can get mixed signals. Do you think that perhaps denyAsNotFound should have a default message as to override the unauthorized message?

[johanmolen]

@timacdonald should the prepareException example not be like this:

return match (true) {
    $e instanceof AuthorizationException && ! $e->hasStatus() => new NotFoundHttpException($e->getMessage(), $e),
    default => parent::prepareException($e)
};

instead of match($e)?

[timacdonald]

Good catch @johanmolen. Updated for future reference. Thanks!

[timacdonald]

Adding the ability to set a global deny status: #43902

[jansgescheit]

return match (true) {
    $e instanceof AuthorizationException && ! $e->hasStatus() => new NotFoundHttpException($e->getMessage(), $e),
    default => parent::prepareException($e)
};

i would not use $e->getMessage() because in case of a hidden 403, you got a 404 with This action is unauthorized. message.