TrustHosts 404 response difficult to trace

fabriciojs commented 3 years ago

Laravel Version: 8.26.1
PHP Version: 7.4.14

Description:

If the APP_ENV is set to production the TrustHosts middleware, in case of failure, will raise a SuspiciousOperationException exception which will be translated to a NotFoundHttpException exception with a clear message Bad hostname provided.

The problem is that NONE of these details reach the user, nor logs are generated, as the response is nothing else but a standard 404.

This is very, very, very misleading, at the point I decided to open this report. Is it fair to say that the expected behavior here would be a more semantic response? Like a 400. That by itself should improve observability in this situation.

The broader context here to imagine the impact is:

This is a behavior only affecting the production (or non-local) environments.
Some health check tools (i.e in my case AWS ELB) will not allow you to set a specific host to be used for the healthcheck.
In such a case the developer is very frustrated with an unhealthy status for ELB and no logs whatsoever of what was causing the Laravel application to return a non-200 response.

Steps To Reproduce:

Set the APP_ENV to production
Change APP_URL for anything different than what you are gonna use in the browser to actually access your running environment.
You should see a standard 404 page for any routes.

Proposed Solution

Can we stop translating this into a NotFoundHttpException and then make it return a 400 instead?

driesvints commented 3 years ago

Heya, since this is more of a request to change the current behavior I'm moving this to the ideas repo.

On the issue at hand: I'm a bit torn myself. I think I also more lean to making it a 400 instead. You're free to attempt a PR if you want. This would need to go into master as it's a breaking change. Please make sure you're very thorough in your PR description. Thanks

fabriciojs commented 3 years ago

Hello @driesvints !

I have been looking into this trying to format a PR. But I reached a point I believe it is best we discuss first what to do:

What we know:

The SuspiciousOperationException is not thrown anywhere in the Laravel code itself. It is something we expect might come mainly from Symfony HTTP Foundation classes.
Back in 5.8, it was this PR 28866 that introduced the "don't report" and exchange of SuspiciousOperationException to NotFoundHttpException.
Even @taylorotwell participated in that discussion and was convinced at the time it was OK to "hide" the exception for a 404, with the argument that no real user should ever see this error as the application.
Later in version 7.x the TrustHosts middleware was introduced, which became active by default in Laravel 8 (I believe). The middleware will basically proxy the app.url config to the request object.
The advent of the middleware by default working only for non-test and non-local environments makes with that real users will legitimately face this error disguised as a 404 only for production/testing deployments (like it was my case for a healthcheck request that won't support custom domain - AWS ELB). This is not an ideal Developer Experience.

Given the situation laid out above, my suggestion would be to compromise between the spirit from PR 28866 was made and the practical acknowledgment that hiding out SuspiciousOperationException behind a 404 is not ideal and more observability should be given here.

Ideas on what to do:

Let's make SuspiciousOperationException be translated to a HttpException(400, 'Bad request', $e) instead of NotFoundHttpException on Illuminate\Foundation\Exceptions\Handler::prepareException.
Let's make the TrustHosts middleware work on local environment as well - this would make developers aware of its existence and role.
Add documentation to TrustHosts (couldn't find it on 8.x docs)
Remove SuspiciousOperationException from $internalDontReport.
For default Illuminate\Foundation\Exceptions\Handler::report implementation in case shouldntReport() is true, we should still look for the previous exception for possibly reporting that one (maybe recursively).

or

Maybe forget about all of this, and make TrustHosts middleware validate the host itself instead of delegating it to later usage of Symfony Foundation internal verifications - returning a clear, very well located exception, for 400.

I am not sure about how big is the impact of this - as well as how much of a share of Laravel 8.x applications have made to the production environment already. I do believe that the semantics needs to be improved for TrustHosts to bring the value it proposes.

driesvints commented 3 years ago

Hey @fabriciojs. I've talked this over with the team and at this time we're not going to take any actions here and leave things be. In the past we've already experimented with adjusting the behavior here and it broke a few things for us on Forge. This could potential have another big impact. So we're not keen to make changes here, sorry.

We are going to try to document the usage of TrustHosts soon.

Thanks for raising this.

laravel / ideas