Skip to content

BUGFIX: catch InvalidHmacErrors in DefaultFormStateInitializer #180

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Pingu501 wants to merge 1 commit into
base: 5.4
Choose a base branch
from
Open

BUGFIX: catch InvalidHmacErrors in DefaultFormStateInitializer #180

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion Classes/FormState/DefaultFormStateInitializer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
use Neos\Flow\Annotations as Flow;
use Neos\Flow\Mvc\ActionRequest;
use Neos\Flow\Security\Cryptography\HashService;
use Neos\Flow\Security\Exception\InvalidArgumentForHashGenerationException;
use Neos\Flow\Security\Exception\InvalidHashException;
use Neos\Form\Core\Model\FormDefinition;
use Neos\Form\Core\Runtime\FormState;

Expand All @@ -31,7 +33,11 @@ public function initializeFormState(FormDefinition $formDefinition, ActionReques
{
$serializedFormStateWithHmac = $actionRequest->getInternalArgument('__state');
if ($serializedFormStateWithHmac !== null) {
$serializedFormState = $this->hashService->validateAndStripHmac($serializedFormStateWithHmac);
try {
$serializedFormState = $this->hashService->validateAndStripHmac($serializedFormStateWithHmac);
} catch (InvalidArgumentForHashGenerationException | InvalidHashException) {
return new FormState();
}
Copy link
Copy Markdown
Member

@bwaidelich bwaidelich Sep 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

@Pingu501 Pingu501 Sep 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

@bwaidelich bwaidelich Sep 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

@Pingu501 Pingu501 Sep 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Member

@bwaidelich bwaidelich Sep 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

@Pingu501 Pingu501 Sep 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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? :)

Copy link
Copy Markdown
Member

@bwaidelich bwaidelich Sep 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown

@erickloss erickloss Sep 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/** @noinspection UnserializeExploitsInspection The unserialize call is safe because of the HMAC check above */
return unserialize(base64_decode($serializedFormState));
}
Expand Down