Database exception if honeypot_time form element is tampered with

Without tests for this there is no way to tell whether the patch fixes the problem. We need a test that demonstrates the failure with the current core - that test will also prove that the patch corrects the failure and will prevent future changes to the code from introducing a similar problem.

The questions in #10 still need to be addressed.

Another variation on the same problem; if the value of honeypot_time is tampered with and a value containing one or more non-ASCII characters is submitted, we can end up with errors like this:

Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[HY000]: General error: 1267 Illegal mix of collations (ascii_general_ci,IMPLICIT) and (utf8mb4_general_ci,COERCIBLE) for operation '=': SELECT 1 AS "expression" 
FROM
"key_value_expire" "key_value_expire"
WHERE ("name" = :db_condition_placeholder_0) AND ("collection" = :db_condition_placeholder_1); Array
(
    [:db_condition_placeholder_0] => Nri1jWuZkdjkgoeelod86uyftmtfv6v59Y8ʼ
    [:db_condition_placeholder_1] => honeypot_time_restriction
)
in Drupal\Core\KeyValueStore\DatabaseStorageExpirable->doSetWithExpire() (line 114 of /path/to/drupal/core/lib/Drupal/Core/KeyValueStore/DatabaseStorageExpirable.php).

This is because the schema for the key_value* tables uses varchar_ascii for the name field, so in MySQL we end up with something like:

`name` varchar(128) CHARACTER SET ascii COLLATE ascii_general_ci NOT NULL DEFAULT ''

If honeypot is going to validate the input from the form, it should take this into account too.

The suggestion in #10 of using a value rather than hidden form element seems like a better alternative to validation though.

This changes the hidden element to a value, and uses getValues() instead of getUserInput() per #10.

I was going to add a test, but if the element no longer appears in the HTML for the form, we don't need to test what happens when the value is tampered with (in fact, we cannot) unless I'm missing something.

Erm, and now the patch :)

The last submitted patch, 17: 3008583-17.patch, failed testing. View results →

Looks like the problem is that when \Drupal\honeypot\HoneypotService::addFormProtection() is called from honepot_form_alter() the honeypot_time element doesn't show up in either $form or $form_state if it's of type value AFAICS.

That means that a new identifier with a new timestamp is created on form submission, and the test fails because the submission seems to have been too quick and is rejected on validation.

I'm not sure if this is a quirk of the Form API or whether I'm doing something wrong.

For what it's worth:

https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Render%21...

...says:

Unlike \Drupal\Core\Render\Element\Hidden, this information is not sent to the browser in a hidden form field, but only stored in the form array for use in validation and submit processing.

As far as I can see form_alter is invoked near the end of \Drupal\Core\Form\FormBuilder::prepareForm and when the form is submitted that happens before \Drupal\Core\Form\FormBuilder::processForm calls \Drupal\Core\Form\FormBuilder::doBuildForm which is where the elements (including value) are processed.

I think that means the current logic where honeypot uses form_alter to add the honeypot_time identifier when the form is being prepared, and then checks whether it's there in the form's inputs when the form is submitted won't work if we change the element to be a value instead... because the honeypot_time's not there in the input submitted to the form, honeypot will add a new one to the form and validation will most likely fail because the wrong timestamp is being used in \Drupal\honeypot\HoneypotService::validateTimeRestriction.

So I suppose the options are (or at least include):

• Keep putting the identifier into the form HTML as a hidden value, but then honeypot needs to validate the submitted value carefully.
• Change the overall implementation of this feature; e.g. perhaps the form_build_id could be used to store the timestamp instead of generating a new identifier (not tested or even well thought through!)

I would say that "#value" will not work when caching is disabled. In Form API, enabling cache means two things: caching the form but also persist the form. So, when caching is disabled there are no values saved into the database and no values can be retrieved from previous form builds.

I think here we need to let the value pass through the client side. But this is not safe. How to secure it? Using cryptographic functions,

I think the way to go is (based on the previous patches):
- On form alter, generate an identifier to store the expire time on the key/value storage.
- On form alter, add a hidden value with the identifier encoded using a cryptographic function and a secret. This secret is the same for all the forms.
- Send the form to the user and wait.
- On form submissions validate the hidden value: decode the value using the secret to get the identifier. With that identifier check the key/value storage for the expire time.

Thanks for looking at this; makes sense that the value element type seems ideal for this but might not actually work in practice.

The cryptographic signing of the tokens etc.. sounds like a good architectural solution, but if I were the maintainer of this module I'd perhaps not want to introduce that in a minor update. Would sites need to set up new keys, for example (or would an integration with another module such as encrypt make sense)?

In the meantime, here's a first pass at introducing validation of the identifier which is the value of the honeypot_time form input.

I've included tests, so removing that tag.

The last submitted patch, 23: 3008583-23.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Just for the record: once this issue is fixed (seems stalled, though) forms can persist data without enabling caching: https://www.drupal.org/project/drupal/issues/2183275 →

TIL we need to use a trick to manipulate hidden form fields, like in \Drupal\Tests\file\Functional\FileManagedFileElementTest::testManagedFile (found via #2767655: Allow WebAssert to inspect also hidden fields → ).

Made a mistake in an earlier change to the logic; the submissions should pass if the identifier is left in tact, so they need to happen after the configured time limit has passed; don't like putting sleep in tests, but I'm not sure there's a better alternative here.

Hmm should have uploaded a test only patch.

Here's one, with #27 again so that the failing test isn't the last.

The last submitted patch, 28: 3008583-28_test_only.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Anyone happy to RTBC this now that we have tests validating the fix?

Maintainers, please add credit for @tunic and @mglaman who both helped dig into the Form API aspects of this issue.

TR → credited mglaman → .

Fixed some coding standards problems.

This patch looks pretty good to me.

The last submitted patch, 33: 3008583-33-test-only.patch, failed testing. View results →

A few more minor changes and one functional change:

Because we generated the identifier with Crypt::randomBytesBase64();, it should always be a Base64-encoded value. Base64 is a subset of ASCII, so testing to see if the submitted identifier is Base64 is a stronger than just testing for ASCII.

The last submitted patch, 35: 3008583-35-test-only.patch, failed testing. View results →

The last submitted patch, 35: 3008583-35.patch, failed testing. View results →

Hmm, I broke something with the change in #35.

Reverting just the base64-related change from #35, along a tiny tweak to comment wording.

Test-only is unchanged.

The last submitted patch, 39: 3008583-39-test-only.patch, failed testing. View results →

Erm, oops - an interdiff is not a patch; apparently my muscle memory forgot.

Everything else looks okay though.

@TR are you happy with this patch?

@TR do we need anything else to get this committed?

Would you like an MR, or is the patch sufficient?

Sorry, it dropped off my radar. Yes, if you could make a MR against 2.2.x that would be great, since patches don't get tested any more on drupal.org.

Changes still apply.

Neat that cspell spotted a little typo in the test, which I've fixed.

@TR let me know if you need anything else to get this reviewed and committed (other than an 8th day of the week etc.. :)

Thanks!

Reviewd, tested, and merged. Thanks for all your hard work.

Automatically closed - issue fixed for 2 weeks with no activity.