BUGFIX: catch InvalidHmacErrors in DefaultFormStateInitializer #180
Conversation
| $serializedFormState = $this->hashService->validateAndStripHmac($serializedFormStateWithHmac); | ||
| } catch (InvalidArgumentForHashGenerationException | InvalidHashException) { | ||
| return new FormState(); | ||
| } |
There was a problem hiding this comment.
silently catching and ignoring exceptions is almost never a good idea. IMO we should think about other ways to solve the issue..
What is the issue? That exceptions are logged due to incorrect requests to your site? Can't you detect them otherwise?
There was a problem hiding this comment.
The problem I try to solve is when somebody tinkers with the hmac in the form, the page will throw a 500er.
Sample: https://www.neos.io/submission-forms/service-provider-submission.html
When manipulating the --service-provider-form[__state] in the DOM directly by replacing the value with foobar the submission of the form will result in a error 500er page.
This attracts bots hoping to exploit this issue, resulting in thousands of requests.
I know this is not a security issue, at least I don't know how this could get exploit, but still attracts lots of attention.
There was a problem hiding this comment.
Thanks for the clarification! I agree that a 500 error is a problem in this case, but IMO it should not ignore the error but throw some 4** error
There was a problem hiding this comment.
yes you are right, the current behavior with a thrown exception is implemented in multiple functions and also tested.
Question is who should handle the InvalidArgumentForHashGenerationException and InvalidHashException.
Maybe the primary issue is, that InvalidArgumentForHashGenerationException does not set the status code to 400, like InvalidHashExceptiondoes?
There was a problem hiding this comment.
You could, in your Flow configuration, decide to ignore that kind of exceptions or handle them differently (see https://flowframework.readthedocs.io/en/stable/TheDefinitiveGuide/PartIII/ErrorAndExceptionHandling.html). Maybe that's an approach.
Otherwise the rendering part could maybe catch the exception and display a more generic error instead
There was a problem hiding this comment.
Might fix it in a single project, but I'm more looking into a general fix solving it for every project. I think that over 90% of all cases using this package, there is no custom code around this methods.
Just had a look into our sites using this package and all suffer from bots trying to exploit this exception. Fixing it in every single project instead of fixing it at the root feels wrong to me 😅
I do believe that this exception is only triggered by bad actors, so I only care in them not seeing any exceptions. For normal users this will/should never happen and work just fine.
Maybe discuss this person to person next week? :)
There was a problem hiding this comment.
I hear you. I'm just concerned that this might hide bugs in someones custom form that creates some invalid state.
OTOH maybe it's just a theoretical issue..
So definitely no -1 from me, let's see what others have to say
There was a problem hiding this comment.
Hi there :)
what do you think about an additional System error Log in the catch block? Would that be verbose enough?
I also like the 400 status idea. @Pingu501 do you know, how bots or security leak detections would react to 400er responses?
cheers
3 participants
Hey there, just saw a bot attack against one of our services trying to exploit this on-caught exception.
Not sure if such hmac-manipulations should be logged using the security-logger?