The module is actually using the following code, to add the time restriction form field.

    $form['honeypot_time'] = [
      '#type' => 'hidden',
      '#title' => t('Timestamp'),
      '#default_value' => honeypot_get_signed_timestamp(time()),
      '#element_validate' => ['_honeypot_time_restriction_validate'],
      '#cache' => [
        'max-age' => 0,
      ],
    ];

Since the value is passed to the browser, and it could be changed, the module needs to join the timestamp with its HMAC value, to be sure it is not changed. There is no need to use the HMAC, to be sure the value is not changed: It would be enough to use value as type of the form element. In this way, the value would not be passed to the browser, and its value could not be changed from the user who submits the form.

    $form['honeypot_time'] = [
      '#type' => 'value',
      '#value' => time(),
      '#cache' => [
        'max-age' => 0,
      ],
    ];

If the form field is created as hidden to verify the user changes its value, and reject the form submission in that case, the module could be use two form fields: The module would check the hidden field is not changed, and then use the value field to see if the time restriction has been respected.

CommentFileSizeAuthor
#25 use_type_value-2672826-25.patch3.81 KBgeerlingguy
#22 use_type_value-2672826-22.patch3.84 KBmr.baileys
#9 use_type_value-2672826-9-D8-revert.patch5.78 KBmr.baileys
#9 use_type_value-2672826-9-D8-test-only.patch1.43 KBmr.baileys
#5 use_type_value-2672826-5.patch4.97 KBmr.baileys
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kiamlaluno created an issue. See original summary.

apaderno’s picture

apaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
CreditAttribution: apaderno as a volunteer commented

As side note, since both the timestamp and its HMAC are visible from the browser, could not a user get the value from a form, and use it for other forms he submits? A bot could do that, I guess.

apaderno’s picture

apaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
CreditAttribution: apaderno as a volunteer commented
Issue summary: View changes
mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
CreditAttribution: mr.baileys at Calibrate commented
Status: Active » Needs review

This makes a lot of sense, and simplifies the module's logic without trade-offs with regards to security.

Patch attached with switches to #type => 'value', rips out the timestamp signing logic and removes time limit from validation (since the value is no longer posted back, it cannot be tampered with and no validation should be necessary).

Tests will fail (2 fails) since testing is currently broken (#2673060: Tests are currently failing.)

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
CreditAttribution: mr.baileys at Calibrate commented
FileSize
use_type_value-2672826-5.patch4.97 KB

Forgot the patch.

geerlingguy’s picture

geerlingguy CreditAttribution: geerlingguy as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Works well for me. Will merge in a minute.

geerlingguy’s picture

geerlingguy CreditAttribution: geerlingguy as a volunteer commented
Version: 8.x-1.x-dev » 7.x-1.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

This makes the code a bit simpler (always a nice thing), and I'm not quite sure why I didn't go this route originally when building the module... backport to D7 shouldn't be too difficult; much of the structure is the same. I'll get to it if I get a chance, otherwise, more internet points to @mr.baileys if you want to do it :)

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
CreditAttribution: mr.baileys at Calibrate commented
Version: 7.x-1.x-dev » 8.x-1.x-dev
Status: Patch (to be ported) » Needs review
FileSize
use_type_value-2672826-9-D8-test-only.patch1.43 KB
use_type_value-2672826-9-D8-revert.patch5.78 KB

I'm not 100% sure yet, but we might have to revert this change.

While porting this to D7, I noticed that I removed the #default_value from the timestamp element completely instead of leaving time(). This caused the time protection to always be triggered:

// Make sure current time - (time_limit + form time value) is greater than 0.
// If not, throw an error.
  if (!$honeypot_time || time() < ($honeypot_time + $time_limit)) {
}

$honeypot_time was always '', since I removed the #default_value.

This also means that there is no test coverage for a "not-too-fast-form-submission-when-time-limit-enabled"-scenario, and I set about writing that test.

I'm unable to get the test to pass with #type => 'value' though, and it seems there are subtle differences between 'value' and 'hidden': 'Hidden' allows you to set a #default_value, which serves as initial value for the field. 'Value' only has '#value', not '#default_value', and that value seems to be reset/refreshed each time the form is loaded. This means that even though we set the timestamp for honeypot_time on initial form retrieval, the value is updated just prior to validation, and each submission is considered too fast.

Attached is a patch containing the amended test that *should* pass (but is expected to fail against HEAD at the moment), as well as a combined revert+test patch.

The last submitted patch, 9: use_type_value-2672826-9-D8-test-only.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 9: use_type_value-2672826-9-D8-revert.patch, failed testing.

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
CreditAttribution: mr.baileys at Calibrate commented
Status: Needs work » Needs review

Huh. Version was changed to D8, but seems the testbot still tried to apply the patch against D7.

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
CreditAttribution: mr.baileys at Calibrate commented
Category: Feature request » Bug report
Priority: Normal » Major

Switching to bug since we've introduced a bug here, switching to major since this breaks time limit protected forms.

apaderno’s picture

apaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
CreditAttribution: apaderno as a volunteer commented

Notice that #element_validate cannot be used for a value element; the only attribute it accepts is #value.

geerlingguy’s picture

geerlingguy CreditAttribution: geerlingguy as a volunteer commented

K... We can revert to stop the bleeding, then after that; is there any way we could still use value successfully? I do like the aspect of the value never hitting the end user in any way.

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
CreditAttribution: mr.baileys at Calibrate commented

@kiamlaluno: interesting, the validation handler is actually invoked though.
@geerlinguy: I spent some time troubleshooting this today, but was unable to have the value *not* refresh prior to validation. I'll take a closer look later tonight to see if I can come up with a viable solution...

apaderno’s picture

apaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
CreditAttribution: apaderno as a volunteer commented

The code currently in the development snapshot is the following.


  // Build the time restriction element (if it's not disabled).
  if (in_array('time_restriction', $options) && \Drupal::config('honeypot.settings')->get('time_limit') != 0) {
    // Set the current time in a hidden value to be checked later.
    $form['honeypot_time'] = [
      '#type' => 'value',
      '#title' => t('Timestamp'),
      '#element_validate' => ['_honeypot_time_restriction_validate'],
      '#cache' => [
        'max-age' => 0,
      ],
    ];

    // Disable page caching to make sure timestamp isn't cached.
    $account = \Drupal::currentUser();
    if ($account->id() == 0) {
      // TODO D8 - Use DIC? See: http://drupal.org/node/1539454
      // Should this now set 'omit_vary_cookie' instead?
      Drupal::service('page_cache_kill_switch')->trigger();
    }
  }

The form element is wrong, since it doesn't have any #value. The correct one is the following one.

    $form['honeypot_time'] = [
      '#type' => 'value',
      '#value' => time(),
      '#cache' => [
        'max-age' => 0,
      ],
    ];

You cannot add an element validation handler, so a form validation handler should be added. I am not sure #cache is accepted from that kind of form element.

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
CreditAttribution: mr.baileys at Calibrate commented

The dev-snapshot is indeed incorrect, but I've been running some tests locally with the correct definitions, and am still running into test failures. This seems related, although old so I don't know if it is still releveant: [#169020]

geerlingguy’s picture

geerlingguy CreditAttribution: geerlingguy as a volunteer commented

Relevant snippet from that forum topic:

When the page is loaded, the form build function is called, which defines the form values again. The form build function is also called upon form submission, and then the validation and submit hooks are called after the form build.

so dynamically created values will be re-dynamically-recreated?

Will need to look into Form API to confirm, but that sounds potentially true of the existing system... I've never used it for a value that is dynamic.

geerlingguy’s picture

geerlingguy CreditAttribution: geerlingguy as a volunteer commented
Category: Bug report » Feature request
Status: Needs review » Needs work

Merged the revert patch, so now back to needs work/feature request... I'm not sure if this will be possible if the FAPI is doing what that forum topic suggests.

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
CreditAttribution: mr.baileys at Calibrate commented
Status: Needs work » Needs review
Issue tags: +drupaldevdays
FileSize
use_type_value-2672826-22.patch3.84 KB

I've picked this up again at the Drupal Dev Days.

From a Form API perspective, I don't think what we are trying to do (saving state for the form on the initial GET request) is possible. Drupal Core actively avoids persisting form and form_state for a GET request, regardless of whether you are logged in or not. This seems by design, see for example #2502785: Remove support for $form_state->setCached() for GET requests. This also causes the code that assigns the timestamp to run again, defeating it's purpose.

However, we can still avoid the process of signing/tamper-proofing the timestamp in the form by storing the timestamp server-side ourselves. Patch attached generates a unique id per form (we cannot use form_build_id for this, since it is not yet available when initially rendering the form), and stores the timestamp in \Drupal\Core\KeyValueStore\KeyValueStoreExpirableInterface. When submitting, the timestamp is retrieved from that store to check whether enough time has elapsed for the form submission to pass the Honeypot time restriction. If no timestamp can be found, the submission is rejected.

geerlingguy’s picture

geerlingguy CreditAttribution: geerlingguy as a volunteer commented

I'm going to try to do some manual testing just to make sure I'm not missing something here, but I like the approach.

geerlingguy’s picture

geerlingguy CreditAttribution: geerlingguy as a volunteer commented

New patch attached (needed reroll after merging #2853205: Field is not hidden if big pipe is used). There are a couple bits of code I want to also touch up.

geerlingguy’s picture

geerlingguy CreditAttribution: geerlingguy as a volunteer commented
FileSize
use_type_value-2672826-25.patch3.81 KB

Patch attached this time.

  • geerlingguy committed 62df55e on 8.x-1.x 
    Issue #2672826 by mr.baileys, geerlingguy: Use '#type' => 'value' for...
geerlingguy’s picture

geerlingguy CreditAttribution: geerlingguy as a volunteer commented
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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