Interoperability issue with 500 response code caused by ApplicationException

[lex0r]

Winter CMS Build

dev-develop

PHP Version

8.1

Database engine

MySQL/MariaDB

Plugins installed

No response

Issue description

In certain setups that rely on a frontend web proxy (like CloudFront or CloudFlare) to deliver HTML generated by a Winter CMS application, it is possible for the proxy to intercept 500 response which is considered an application error. These proxies typically will show a custom HTML page when a 500 response is received, and for a good reason - not all applications behave correctly and are able to show a meaningful error page when an internal server error occurs.

Winter CMS has a mechanism based on throwing ApplicationException in the application in order for some admin UI JavaScript handlers to produce a non-browser-native alert (popup) with the error message matching the string used in ApplicationException constructor. This is simple an efficient in case a quick error message not related to any form is needed. Unfortunately, it doesn't work well with the above web proxy logic - the 500 response's message is ignored and replaced with an error page's HTML.

It is not always possible to override the proxy's behaviour. Also, logically, it's not clear why ApplicationException which is meant to be a quick (and yes, dirty) way of delivering a message even for normal error cases (like a field repeater's minimum number of items not reached), relies on code 500. If the server is able to produce a response that has a meaning then it's not an internal server error, but some kind of client error (4xx), For example, validation errors produce 416, so why an ApplicationException sticks to 500?

Steps to replicate

1. Run a Winter CMS app through a web proxy that has custom error handling support and allows redefining 500 error responses with a custom HTML page
2. Throw ApplicationException

Expected result: an popup shows whatever message was passed as ApplicationException constructor argument
Actual result: the proxy thinks the app is defunct and substitutes the response with the custom 500 page.

Workaround

No response

[LukeTowers]

It's not Winter's responsibility to care about what proxies are doing to the response. In the provided example of a repeater's minimum items not being reached then yes, that would probably be more correct to be handled with a ValidationException instead, but that's up to the developer writing the logic to decide which exception is most relevant.

Do you have a suggestion for more relevant status codes / exceptions to use instead of 500 & an ApplicationException? The ApplicationException is intended for when the application cannot proceed with processing, but it's not because of a coding error or something that should be fixed by a developer.

[lex0r]

It's not Winter's responsibility to care about what proxies are doing to the response.

An application doesn't operate in a vacuum, there are 3rd parties and they all rely on some common standards that Web has to offer. When an app doesn't adhere to them for a good reason then it's hard to blame it, however when there's no good reason to give a 500 response for an error which is not fatal, then this behaviour should be changed. It's not only about the environment, it's about providing a logically correct behaviour.

but that's up to the developer writing the logic to decide which exception is most relevant.

I would like to know what led to that decision. On the surface it looks like the developer took an opportunity to come up with a quick error handling mechanism which is already there, and just inherited the specifics of it. But this issue is raised exactly for this reason - many developers may want to use it so more pressure is put on making it right and compatible.

Do you have a suggestion for more relevant status codes / exceptions to use instead of 500 & an ApplicationException? The ApplicationException is intended for when the application cannot proceed with processing, but it's not because of a coding error or something that should be fixed by a developer.

Right, this class of errors in HTTP terms fall into 4xx category - client error (as opposed to 5xx when server fails to fulfil an apparently valid [i.e. inputs are correct and match the context] request).

I did some custom implementation by redefining ErrorHandler via Event::listen('exception.beforeRender') and used HTTP code 400. To make it fully compatible with ApplicationException I also changed the detailed error message to just the message used in the exception constructor. Can share some snippets if that helps...

[LukeTowers]

I wouldn't be opposed to a PR to specify the HTTP error code to be used along with providing the most common error codes as constants on the class because sometimes a 422 is more relevant.

however when there's no good reason to give a 500 response for an error which is not fatal, then this behaviour should be changed. It's not only about the environment, it's about providing a logically correct behaviour.

I would argue that an ApplicationException is fatal. Unlike a stateful server application, the ApplicationException might not indicate that things are now permanently broken, but for the purposes of the request being made when the ApplicationException is thrown the code is saying "I've reached a state where I cannot help you anymore; here's why".

What changes would you like to propose? I'm not opposed to an audit of the existing uses of ApplicationException vs SystemException in the core and improving our documentation / developer education resources to better communicate when each exception type should be used, we just need to figure out what changes specifically we want to make and then make them :)

[bennothommo]

I'm (apologetically) going to have to hard disagree on this, @lex0r. An Exception is most certainly fatal unless it is caught and handled differently, and PHP's default behaviour when an exception is thrown is to return a HTTP 500 when run as an Apache module or through FPM. HTTP 500 is most definitely the standard response for when the server cannot complete a request/response - an exception would certainly fit in that scenario.

We can certainly argue about the usage of exceptions in general throughout Winter and the plugins when it relates to exceptions caused by client input - as you said, ideally, if it's a client issue, it shouldn't be an exception (or it should be an exception that returns a different HTTP code), but I certainly don't agree with changing the behaviour of exceptions wholesale to account for proxies.

If your proxy doesn't allow you to change its behavior when it encounters a HTTP 500, and can't be configured to not intercept 500 errors being returned by AJAX queries, then it might be worth finding a better proxy.

EDIT: I don't mind if you want to change the behavior of this through a plugin - I'm only drawing a hard line when it comes to core code. :)