BUGFIX: Dont log stack trace for InvalidHashException in Production

[kdambekalns]

This configures an invalidHashExceptions exception handler rendering group and configures it to not dump stack traces in Production context. For Development context stack traces are still written to ease debugging.

See #3159

Upgrade instructions

In case you need trace dumps for InvalidHashException in production context, override the settings as needed.

Review instructions

See #3159 for ways to trigger those exceptions. Then check if a trace is dumped.

Checklist

• Code follows the PSR-2 coding style
• Tests have been created, run and adjusted as needed
• The PR is created against the lowest maintained branch
• Reviewer - PR Title is brief but complete and starts with FEATURE|TASK|BUGFIX
• Reviewer - The first section explains the change briefly for change-logs
• Reviewer - Breaking Changes are marked with !!! and have upgrade-instructions

[kdambekalns]

/cc @mrimann

[mrimann]

@kdambekalns thanks for cc'ing me - looks fine by pure reading. But did not test. Looking forward to getting this released :-) Thanks for your work!

[fcool]

Just something to think about: Wouldn't it make sense to choose a more general group name, that still makes sense, if we discover additional exception types, we dont want to write stack traces for the same reasons?

Or do you think, that we should follow the recipe here then: Add another group for the new exception type?

[kitsunet]

Mmm, fair point, could be generic as in noStacktraceExceptionGroup

[kitsunet]

Nice, agree to what @fcool wrote but I am fine either way.

[kdambekalns]

Would be fine with me, but I am too lazy to fix that now. 😬

Feel free to adjust this, you can work on this PR – just keep in mind the rendering config needs to be adjusted (the message would be misleading for other exceptions…)

[mrimann]

@kdambekalns is the review from @albe mandatory to proceed with this issue? Or could someone else review it? If so, who could be that additional reviewer?

[mhsdesign]

Im not sure that this fix works.

I tested it by tampering with the trustedProperties (2).

but via #2685 we log the throwable directly and set the content to Invalid HMAC submitted. Wasnt the idea to not log the exception?
Logged exception (Logger and Throwable storage):

"A hashed string must contain at least 40 characters, the given string was only 10 characters long."

But yes the herby introduces error group will not be used in that case, because the exception doesnt bubble up that far.
The question is when will it be used then? I played around with the other methods #3159 (comment) but i think i was to stupid to fool them :D. When modifying the referrer in the form i get to the default fusion 404 page because of an NoSuchActionException.

[kdambekalns]

A late showstopper? I need to try (again), I am pretty sure it worked, though it may not work in all cases. But when those are caught and handled differently, without creating a trace file, they do not cause the problem this is trying to solve.

[kitsunet]

Yeah this seems fine, it then is only a question of setting log levels for production

[kdambekalns]

Rebased on 7.3, changed the rendering group name to noStacktraceExceptionGroup.

Now I'll test again, to check if it actually makes a difference…

[kdambekalns]

Test protocol

• Neos & Flow 7.3
• PHP 8.1.27

Scenarios from #3159 (comment)

Unpatched sources

• calling Neos login with wrong credentials: no error, no trace ✅
• scenario 1, tampered __referrer[@controller]: 404, no error, no trace ✅
• scenario 2, tampered __trustedProperties: 400, notice logged, trace written ❌
• scenario 3, tampered __referrer[arguments]: 400, crtitical logged, trace written ❌

The notice and trace come from ActionController.processRequest (see #2685).

Patched sources

• calling Neos login with wrong credentials: no error, no trace ✅
• scenario 1, tampered __referrer[@controller]: 404, no error, no trace ✅
• scenario 2, tampered __trustedProperties: 400, notice logged, trace written ❌
• scenario 3, tampered __referrer[arguments]: 400, no error, no trace ✅

Scenario 2 needs to be fixed…

Patches sources, part two

This is with 5f29d10

• calling Neos login with wrong credentials: no error, no trace ✅
• scenario 1, tampered __referrer[@controller]: 404, no error, no trace ✅
• scenario 2, tampered __trustedProperties: 400, no error, no trace ✅
• scenario 3, tampered __referrer[arguments]: 400, no error, no trace ✅

[kdambekalns]

Whetever Psalm has been smoking…