BUG: Exception handling differs depending on "trigger point"

[kdambekalns]

Is there an existing issue for this?

• I have searched the existing issues

Current Behavior

One example is the handling of login forms:

• when entering wrong credentials, a "nice" error message is show, no exception dump is written
• when tampering with the HMAC, the output is a bare error message, an exception is logged, an exception dump is written

Other "wrong" requests show the same behaviour, e.g. when changing the controller reference of a POST request.

Expected Behavior

The error shown to the user should be consistent.

Steps To Reproduce

No response

Environment

- Flow: 7.3 & up

Anything else?

It is generally fine that Flow throws an error here. But the Neos backend login that is “tested” quite often, like any other reachable login page on the web, should in my opinion be a bit more resilient to such tampered requests. Showing the user the same or a similar error message like for a failed login would be OK. Optionally just logging this but not writing out exception dumps would be great. Optionally even ignoring that in the logs should be possible.

All the above applies to Production context. Having more verbose output in Development context is wanted/needed, of course.

Related issue from the past: #2681 (and fix in #2685)

[kdambekalns]

• Testcase: Neos-Login
• Context: Production
• Neos 7.3
• PHP 8.1

Scenarios

1. When the Neos login form is submitted with a changed value for the controller or action in __referrer[@…], a 404 is returned when changin the controller to something non-existent, when changing the action to e.g. logout, the form is shown again. No log message, no trace file. This is fine… ✅
2. When the __trustedProperties of a request is tampered with, the browser show a plain text message: Invalid HMAC submitted. A trace file is written, the log says:
   24-02-23 09:23:46 2212 NOTICE Neos.Flow Property mapping configuration failed due to HMAC errors. Exception #1320830018 in line 127 of /application/Data/Temporary/Production/SubContextBeach/SubContextInstance/Cache/Code/Flow_Object_Classes/Neos_Flow_Mvc_Controller_MvcPropertyMappingConfigurationService.php: The given string was not appended with a valid HMAC. - See also: 2024022309234484b44b.txt
3. If in scenario 3 the __referrer[arguments] (present e.g. after a failed login) is tampered with (and not the trustedProperties), a "400 Bad Request" error screen is the result. A trace file is written, the log says:
   24-02-23 09:22:01 2094 CRITICAL Exception #1320830018 in line 265 of /application/Data/Temporary/Production/SubContextBeach/SubContextInstance/Cache/Code/Flow_Object_Classes/Neos_Flow_Mvc_ActionRequest.php: The given string was not appended with a valid HMAC. - See also: 20240223092158190ae2.txt

Possible solutions

Change configuration

To supress the writing of trace files, the following settings can be used:

Neos:
  Flow:
    error:
      exceptionHandler:
        renderingGroups:
          invalidHashExceptions:
            matchingExceptionClassNames: ['Neos\Flow\Security\Exception\InvalidHashException']
            options:
              logException: false
              viewOptions:
                templatePathAndFilename: 'resource://Neos.Flow/Private/Templates/Error/Default.html'
              variables:
                errorDescription: 'Sorry, the HMAC you submitted was not valid.'

Make InvalidHashException return 400

InvalidHashException should declare 400 as it's default status code (not the inherited 500 it has now), as that is clearly a case of a "bad request". See #3234 for a PR. Merged and done, ✅

Whether the same is true for InvalidArgumentForHashGenerationException is debatable, that could also be a "proper internal server error". That is a bit unfortunate, but the same exception is not only thrown in validateAndStripHmac(), but also in generateTrustedPropertiesToken() and generateHmac().

Catch exceptions in forwardToReferringRequest()

Instead of letting the exception bubble up, the code could handle this the same way as when $this->request->getReferringRequest() returns null: by doing nothing.

This leads to a rather weird response: status 200, plain text message Validation failed while trying to call Neos\Neos\Controller\LoginController->authenticateAction(). – but the resulting log message is very short and no trace is written.

[kitsunet]

es, this looks annyoing, good that you take it on. I would definitely like to get sensible status codes and less logging if possible.

[kdambekalns]

es, this looks annyoing, good that you take it on. I would definitely like to get sensible status codes and less logging if possible.

Ok, the status code is done now. ✅

As far as the logging goes, I just realized we could disable logging of the exception (trace) for Production context by default, and keep it enabled in Development. WDYT? 🙄

When it comes to "properly handling" this for the users in a visible way (as if you had entered wrong credentials): This should not happen if you just use the system without poking around in the internals (HMACS, …). Plus, if the referrer is tampered with, it cannot be used. So, I guess that can be left as it is. 🤷

[mrimann]

(...) we could disable logging of the exception (trace) for Production context by default, and keep it enabled in Development. WDYT?

That would be the solution I've dreamed from when I initially reported this issue, as it then would not fill up disks with irrelevant traces when some bot (again) tries to fumble around with the login form for the Neos backend.

So, if I'm allowed to "vote": +1 from my side.