Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[9.x] Support a global deny http status for Gates / Policies #43902

Closed
wants to merge 1 commit into from

Conversation

timacdonald
Copy link
Member

@timacdonald timacdonald commented Aug 29, 2022

We currently have the ability to set the HTTP response status code for individual gate / policy functions. I believe it would be useful and good idea for many applications to default to a 404 deny status, and then on a case by case basis opt into the 403 status.

Making it more "secure by default" than "opt-in".

// service provider...

public function boot()
{
    Gate::useDefaultDenyStatus(404);

    // ...
}


// Use the default behaviour...

Gate::define('defaultStatus', fn () => Response::deny());

// Override the default status...

Gate::define('customStatus', fn () => Response::denyWithStatus(403));

Alternatively, you could also set this in a middleware to limit to specific routes / users:

class Middleware
{
    public function handle($request, $next)
    {
        if ($request->user()->isNotAdmin()) {
            Gate::useDefaultDenyStatus(404);
        }
        
        return $next($request);
    }
}

You should ensure that this middleware runs before the authorization middleware.

Form requests

This status does not apply to FormRequests. I'm sure I can make it so that it does, but I was not sure if it made sense, as the Form Request mechanism doesn't interact with gates at all.

Would love to know your thoughts on if this default status should be propagated to form request's "deny" functionality.

Documentation: laravel/docs#8169

@timacdonald timacdonald force-pushed the global-deny-status branch 2 times, most recently from 524040b to 5bbb847 Compare August 29, 2022 01:45
@timacdonald
Copy link
Member Author

Leaving this in draft until #43901 is merged, as I will add some assertions against the returned message as well.

@garygreen
Copy link
Contributor

garygreen commented Aug 29, 2022

Hmm not sure about this one. Most authorization routes aren't particularly sensitive - in our app it doesn't really mater that they could see the route exists and got a sensible http forbidden response by default. Although for very specific routes like "/admin" that only 0.01% of our users should know about we may hide those behind a 404. Most other authorization exceptions it is useful to know that it was specifically forbidden, especially for API based requests. So 404 by default doesn't seem like a sensible default.

Also need to consider that it's useful from a analytical perspective to see those authorization exceptions in your error tracking software, rather than some generic 404. It helps highlight bugs, and vulnerability attacks happening. If it just 404 for most policy exceptions you wouldn't have any way of knowing that - especially as those http responses aren't logged by default.

@timacdonald
Copy link
Member Author

timacdonald commented Aug 29, 2022

Hey @garygreen, thank you for the feedback.

I don't agree that "Most authorization routes aren't particularly sensitive" as a blanket rule. That is gonna depend on the application, architecture, and type of information the system holds and exposes.

You have said that your app defaults for 404 for admin routes, which is a great use-case for this feature e.g. a middleware:

class Admin
{
    public function handle($request, $next)
    {
        Gate::useDefaultDenyStatus(404);

        return $next($request);
    }
}

You should of course ensure that this runs before the Authorization middleware. The opposite could be true for applications as well - where public areas should 404 by default.

Note: I've updated the initial description to include the Middleware example.

Regarding:

Also need to consider that it's useful from a analytical perspective to see those authorization exceptions in your error tracking software, rather than some generic 404. It helps highlight bugs, and vulnerability attacks happening. If it just 404 for most policy exceptions you wouldn't have any way of knowing that - especially as those http responses aren't logged by default.

Nothing changes here from within Laravel. The exception handler still gets the same exception. So the logging / analytics still see an AuthorizationException. This is only concerned with how the AuthorizationException is rendered to the user.

If your infrastructure is tracking things via the response code sent from Laravel, then yes that changes. But that is a tradeoff you have to make whether you use this feature, the per-cases feature - or push all the handling of this kinda thing to your infra layer and manually handle this there on a route basis or globally.

@garygreen
Copy link
Contributor

garygreen commented Aug 29, 2022

I don't agree that "Most authorization routes aren't particularly sensitive". That is gonna depend on the application, architecture, and type of information the system holds and exposes.

This is obviously opinionated stuff, and this is just our perspective on it. An example could help here.

Lets say a user is in the process of buying a ticket, they took a while to buy and the event sells out - lets assume there is a policy which checks to see if there are available tickets and if there isn't it'll come back with a deny message saying it's sold out:

public function buy(User $user, Ticket $ticket)
{
   if (! $ticket->available) {
     return $this->deny('Ticket has sold out');
   }
}

Then let's say the http exception handles showing a 403 with that friendly message saying it's sold out - that is more preferable and sensible default from our perspective than to simply 404 which makes it feel like something broke. Also I think this could encourage quite confusing and poorly designed APIs.

@timacdonald
Copy link
Member Author

Totally agree that there are no truths in here, and all our opinions ❤️

If your application wanted to opt into a default deny status (again, an optional switch), you may still specify when 404 is not what you want:

public function buy(User $user, Ticket $ticket)
{
   if (! $ticket->available) {
     return $this->denyWithStatus(409, 'Tim bought this ticket before you. Be quicker next time 😈');
   }
}

I take your point that things can be used poorly, but used "correctly" (whatever that looks like), this feature can be useful IMO.

@timacdonald
Copy link
Member Author

I'm gonna close this one off as I feel that it is a bit inconsistent not also capturing the FormRequest responses and adjusting their status.

I might re-visit this and see if I can come up with a more full featured approach.

@timacdonald timacdonald closed this Sep 4, 2022
@timacdonald timacdonald deleted the global-deny-status branch September 4, 2022 23:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants