[9.x] Default 404 message on denyAsNotFound

[guilledll]

404 Default message when using denyAsNotFound response.

I'm currently developing an API Laravel App and I realized that the denyAsNotFound method on the HandlesAuthorization trait does return a 404 response status but no a "Not found" message by default.

In the docs is mentioned how the 404 response is used as a common practice, what i see here that laravel does wrong is returning the 404 status but not a "Not found." message by default.

Indeed the default message is 'This action is unauthorized.' from the AuthorizationException

public function __construct($message = null, $code = null, Throwable $previous = null)
{
    parent::__construct($message ?? 'This action is unauthorized.', 0, $previous);
    // ...
}

This is partially good but does not comply with the 404 status because we do not found nothing by status but we say "unauthorized" which means there is somthing but you can't do it.

How this impreve other devs experience

I think this aproach help to avoid reapeating setting a default 404 message in all requests that need to check that gate, also gives the function a corresponding message of what it's actually does.

My code example:

class CompanyPolicy
{
    use HandlesAuthorization;

    public function before(User $user)
    {
        return $user->hasSuperRole() ?: $this->denyAsNotFound(); // <-- Now '404 not found.' by default
    }

    public function viewAny(User $user)
    {
        return $user->isPartOfComany() ?: $this->denyAsNotFound(); // <-- Now '404 not found.' by default too
    }
}

Compatibility

• This will change the default response status for every policy or gate that uses the denyAsNotFound method without giving a $message parameter.
• All denyAsNotFound methods that have a $message won't be affected

Hope it helps, I'm open to opinions 😄

[garygreen]

Presumably the "404 deny as not found" feature was added to hide the fact that a sensitive route exists, but was secretly/discreetly forbidden. So exposing the http message as "This action is unauthorized" is actually exposing the fact that the route exists and could be a security issue. So a definite 👍 from me.

[timacdonald]

Thanks for the PR to fix this one and I apologise for the oversight on that one!

May I suggest a different approach that will handle all status codes, not just 404:

<?php

namespace Illuminate\Auth\Access;

use Illuminate\Contracts\Support\Arrayable;
+ use Illuminate\Http\Response as HttpResponse;

class Response implements Arrayable
{
    /* ... */

    public function authorize()
    {
        if ($this->denied()) {

+           $message = $this->message() ?? HttpResponse::$statusTexts[$this->status] ?? null;
+
-           throw (new AuthorizationException($this->message(), $this->code()))
+           throw (new AuthorizationException($message, $this->code()))
                ->setResponse($this)
                ->withStatus($this->status);
        }

        return $this;
    }
}

We could house all of this in function message(), however then the outside world would not be able to tell if the message was intentionally set by the user or if it was set by the status code, so I feel like the above is the most flexible approach.

[guilledll]

@timacdonald one final thing, a test is failing with status 418 and message "I'm a teapot" how can we approach that? I don't think changing the test to assert that will be a good option...

But.... wait.... not so wrong at all 😂

[timacdonald]

After thinking on this over lunch, I realised that this locks the developer into a specific way of handling things. If we handle this in the exception handler instead, the developer can then customise this handling to suit their needs. The framework could do this:

    /**
     * Prepare exception for rendering.
     *
     * @param  \Throwable  $e
     * @return \Throwable
     */
    protected function prepareException(Throwable $e)
    {
        return match (true) {
            // ...
-           $e instanceof AuthorizationException && $e->hasStatus() => new HttpException($e->status(), $e->getMessage(), $e),
+           $e instanceof AuthorizationException && $e->hasStatus() => new HttpException(
+               $e->status(), ($e->response()?->message() ?: Response::$statusTexts[$e->status()]) ?? $e->getMessage(), $e
+           ),
            $e instanceof AuthorizationException && ! $e->hasStatus() => new AccessDeniedHttpException($e->getMessage(), $e),
            // ...
        };
    }

The precedence of message returned would be:

1. The $message from the Gate / Policy with Response::deny($message)->withStatus(321).
2. The HTTP status code e.g. "Not Found."
3. The exception message "The action is unauthorized".

This is nice because a developer may want to change this in their app to always render the HTTP status code text. They could then do the following in their own app:

/**
 * Prepare exception for rendering.
 *
 * @param  \Throwable  $e
 * @return \Throwable
 */
protected function prepareException(Throwable $e)
{
    if ($e instanceof AuthorizationException && $e->hasStatus()) {
        return new HttpException($e->status(), Response::$statusTexts[$e->status()], $e);
    }

    return parent::prepareException($e);
}

This allows for greater flexibility. It also means that the original exception message is not lost and can be logged, but the HTTP status code message can be rendered.

We could even make this a configurable option in the framework so that you can just call something in the service provider to decide what message is rendered - but we don't have to as the developer does have complete control via the handler method.

Note: I haven't tested the above code. I just threw it together in the editor - but I believe something like that should be achievable and preferable IMO.

[taylorotwell]

Please mark as ready for review when this is settled.

[guilledll]

@timacdonald i'm trying with your proposed changes, the problem with is that the AuthorizationException default message is setted before the Handler.prepareException so the message precedence you mentionied is not possible to achieve, at least with my knowledge.

$e instanceof AuthorizationException && $e->hasStatus() => new HttpException(
    $e->status(),
    ($e->response()?->message() ?: // will allways have the value $message or "This action is unauthorized."
        Response::$statusTexts[$e->status()]  // this sentence will never be reached
    ) ?? $e->getMessage(), // this sentence will never be reached
    $e
),

My actual work is now commited.

[timacdonald]

Note that 'Whoops, looks like something went wrong.' is only presented to the user when:

1. The deny action did not receive a message.
2. The status code in unknown to Symfony.

Also, this message is really on for JSON responses, as Symfony will show this message no matter what we change this too. For example, doing the following on a fresh application:

// .env:

APP_DEBUG=false


// Route:

Route::get('/', function () {
    throw new HttpException(399, 'foo');
});

So using this messaging actually creates consistency between the HTML response and the JSON response.

[timacdonald]

@guilledll I've pushed some additional changes and tests to support my proposed solution. This approach will allow developers to customize their handling if the default precedence is not preferred.

Thanks for raising this issue and sending through a fix ❤️

[garygreen]

@timacdonald This PR still exposes policy message even for 404 when policy comes back with custom message. Is that intentional?

[timacdonald]

This was intentional. Before this PR, we had a bug that I 100% agree needed fixing. This is what the response to the user would look like:

/**
 * Before:
 */

Response::deny();
// 403: This action is unauthorized.
// ✅

Response::deny('This ticket has sold.');
// 403: This ticket has sold.
// ✅

Response::deny()->withStatus(409);
// 409: This action is unauthorized.
// ❌

Response::deny('This ticket has sold.')->withStatus(409);
// 409: This ticket has sold.
// ✅

/**
 * After:
 */

Response::deny();
// 403: This action is unauthorized.
// ✅

Response::deny('This ticket has sold.');
// 403: This ticket has sold.
// ✅

Response::deny()->withStatus(409);
// 409: Conflict
// ✅

Response::deny('This ticket has sold.')->withStatus(409);
// 409: This ticket has sold.
// ✅

What we are talking about now is changing the intention of the $message so that it is no longer presented to the user, even when explicitly set:

Response::deny('This ticket has sold.')->withStatus(409);
// 409: Conflict

IMO if you don't want to expose the message to the user, you should just not set a message. When the message is set it is to be communicated to the user:

Response::denyWithStatus(409);
// 409: Conflict

Response::denyWithStatus(409, 'The ticket has sold.');
// 409: The ticket has sold.

This is similar to how abort(409, 'The ticket has sold.') works.

Status: 409

Body:
{
    "message": "The ticket has sold."
}

The $code is there to communicate the exception reasoning to your exception reporting.

[garygreen]

IMO if you don't want to expose the message to the user, you should just not set a message.

Though the whole premise of sending a 404 or some other exception is because you want to hide the route and it's output, right? How does that work with your draft #43902 - if that sets a default 404 then you would have to ensure all of your policies don't return a message, otherwise it's kinda pointless sending 404 in the first place.

Personally I think exposing the policy deny message in the http status should be opt-in rather than doing it by default. Just seems safer and more sensible. Especially as some people could send some arbitrary HTML or json as the message:

Response::deny('You cannot <strong>create tickets</strong>');

...not that it would be recommended doing that, but either way that message cannot be relied upon to be the right format. And it's not obvious it's used as a http status message.

[timacdonald]

I guess I already see it as opt-in. If you set a message, you opt in.

// default...

Response::deny();

// opt-in...

Response::deny('foo');

The documentation also states that the message is propagated to the HTTP response.

I'm not saying I'm right about all this - just sharing my perspective. Will ask for some input from the team and get some other voices in the mix.

Thanks for asking and pushing on these points Gary!

[jessarcher]

I think it would be inconsistent and confusing if the "status" methods changed the behaviour of the message when it is specified.

I agree that if you're using this feature to obfuscate, then showing a message could leak knowledge, but I don't think that's the responsibility of these methods. There are other use cases where you may want to change the status code and still show a message.

If the method name was more opinionated, such as "silentlyDeny" or "hide", then I agree the message should not be shown and it probably shouldn't even be a parameter. For those method names, I wouldn't even expect to pass a status code.