Add possibility to use JS on cached pages

One of our sites just ran into this, I'll test out awolfey's patch and will provide feedback here.

I've actually hit this once or twice this month on www.jeffgeerling.com — I wonder if it's related to traffic volume and a reverse proxy, because I've never had the issue before I got things set up behind nginx's cache, and it seems to occur more frequently when I have high-traffic incidents.

++ I haven't tested the sandbox code but would love to see this functionality incorporated. Simple and elegant... Beautiful! I never thought of doing it that way before and always struggled with the caching issues.

I'd love to see this in honeypot itself!
Sometimes we have forms being shown in all pages (Login, Newsletter subscription, etc.) and having honeypot time limit enabled just prevents every page from using the internal dynamic page cache.

@geerlingguy does something speak against a JS solution for this feature? We could create a feature request and I could spend some time on it, but first I wanted to see if this way would be THE way to go for?

@geerlingguy does something speak against a JS solution for this feature?

So far, there are a few other modules which use JS-only or JS-supplemented solutions... I'm not as opposed to it today as I was a few years back when there were more than like 0.001% of devices with no javascript, but there are still some tradeoffs, and to provide the same level of protection, each of the cached pages where a form is displayed would still need to make a request back to the site for Honeypot.

But really, if you have a protected form showing on the majority of the pages on a site, Honeypot might not be the best solution (if you need more cache hits)—Honeypot was designed (and is maintained) for the use case of protecting individual forms like webforms, node forms, comment forms, and user registration/password reset type forms, not for something like a search form or views exposed block that appears everywhere on a site.

I needed that for a client. This patch does:

- 1. Add an option to enable JS protection of the form if and only if the form is on a cacheable page (drupal_page_is_cacheable() is TRUE)

- 2. Ensure that if a user without JS uses the form, they are not penalized: They however need to re-confirm the form submission.

- 3. Ensure that if the JS does not load in time, the fallback for a user without JS enabled is used.

- 4. Ensure that the JS is only used for cacheable pages. This means after a failed submission always the normal protection (without JS) is used.

- 5. Opens the way for custom / contrib modules to replace the no_js_available value with a real timestamp either in a cache module, which hooks into cache_page (or using cacheobject module) or doing it even within Fastly with hash_hmac256_base64 and ESI synthetic request.

---

I opted to create a patch as there was a) demand and b) I found no good way to avoid the disabling of the page cache without lots of custom logic. And re-enabling like the sandbox module could have bad side effects.

---

While the logic to not send an AJAX request might seem faulty and easily exploitable, the honeypot timestamps can be re-used anyway, so any robot putting in the work to hack the JS implementation can also hack the normal implementation.

--

Note: The original code had a bug / hidden feature:

@@ -284,12 +356,44 @@ function _honeypot_time_restriction_validate($element, &$form_state) {
     _honeypot_log($form_state['values']['form_id'], 'honeypot_time');
     // Get the time limit again, since it increases after first failure.
     $time_limit = honeypot_get_time_limit($form_state['values']);
+    // Ensure to update both $form_state and the $element value.
     $form_state['values']['honeypot_time'] = honeypot_get_signed_timestamp(REQUEST_TIME);
+    $element['#value'] = $form_state['values']['honeypot_time'];
     form_set_error('', t('There was a problem with your form submission. Please wait @limit seconds and try again.', array('@limit' => $time_limit)));
   }
 }
 
 /**

If you don't update $element['#value'] then the value will continue to have the original timestamp (in the form displayed in the HTML). That means if a user should have waited 5 seconds, waits only 4, submits, gets the error with 6 seconds and submits after 2 seconds again => succeed.

With the above fixed they _need_ to wait for 6 seconds before trying again. (which is what is displayed in the message).

---

The functionality is marked as experimental, so hopefully people will use some caution.

---

If logging is enabled, then submissions with JS disabled will be logged, so this incident can be monitored.

Please disregard the previous patch. I made a last minute mistake in the refactoring ...

This one works way better :D. (!== vs === mixup ...)

I tested this with some ajax enabled forms and those need a little tweak to work.

Interdiff and new patch attached.

Removed misleading left-over comment from other project :).

On first read through, I like what I see, and this checks all the boxes for things I would want in a JS implementation; haven't had time to dig in too much, but I want to run through some manual testing before merging.

Also—I'm willing to accept into 7.x first since this is a pretty killer feature and would take away one of the things which causes the most hesitation for people considering Honeypot, but do you think you'd have time to work on the port to D8 of this same feature? Otherwise, I can leave this ticket open 'to be ported' and hopefully someone can pick it up and run with it. I don't like having D7 != D8 in terms of feature parity.

Finally, I might be willing to bump a major version to 2.0 with this feature, since it does fundamentally change (and improve) the site owner experience.

On first read through, I like what I see, and this checks all the boxes for things I would want in a JS implementation; haven't had time to dig in too much, but I want to run through some manual testing before merging.

For sure, I also tested this a lot to ensure it works for all cases. :)

Also—I'm willing to accept into 7.x first since this is a pretty killer feature and would take away one of the things which causes the most hesitation for people considering Honeypot, but do you think you'd have time to work on the port to D8 of this same feature? Otherwise, I can leave this ticket open 'to be ported' and hopefully someone can pick it up and run with it. I don't like having D7 != D8 in terms of feature parity.

Nice! I can totally understand the reasoning. Unfortunately I won't have time to work on a Drupal 8 port right now.

Finally, I might be willing to bump a major version to 2.0 with this feature, since it does fundamentally change (and improve) the site owner experience.

That is potentially a good idea. Or maybe do it once it is no longer marked experimental and other users have given some feedback.

---

Little update to the patch as I messed up the logging of the form_id as plach found out.

Looks great and works as advertised in my local testing, I have only a few minor nitpicks:

1. +++ b/honeypot.admin.inc
   @@ -46,7 +46,23 @@ function honeypot_admin_form($form, &$form_state) {
   +  if (!variable_get('honeypot_use_js_for_cached_pages', FALSE)) {
   

   Shouldn't we hide also the warning about cache in the "Protect all forms with Honeypot" checkbox description?

2. +++ b/honeypot.module
   @@ -272,8 +310,42 @@ function _honeypot_time_restriction_validate($element, &$form_state) {
   +      // Update form_state and element values.
   
   @@ -284,12 +356,44 @@ function _honeypot_time_restriction_validate($element, &$form_state) {
   +    // Ensure to update both $form_state and the $element value.
   

   These comments are not very informative, what about explaining why we need to do that?

Updated patch accounting for at least #1 above, also optimizing the grammar in a few places just to make it a little more compact.

I'm testing this locally. One thing I would really like to have available (but AFAIK it's not available on drupal.org test infra?) is JS testing so I can have a couple behavioral tests for this (since the JS behavior is currently not covered by any of the module's automated testing...).

Pushed and tagged for 7.x-1.23. Even without the tests I'd like to get this in the wild and let people on D7 start testing it. I'm going to mark this as 'to be ported' to 8.x next.

New 7.x release: https://www.drupal.org/project/honeypot/releases/7.x-1.23

Any chance of PHP 5.3 support for this update? The new code uses short hand array notation ['title' => 'Javascript to support timelimit on cached pages.'] that was introduced in PHP 5.4 instead of the old long hand array('title' => 'Javascript to support timelimit on cached pages.'). The rest of the code is using long hand, so for consistency it might make sense to swap it over anyways.

Eek, yes, that should be fixed for the D7 version, sorry I let that slip (though you need to get upgraded to at least 5.6 yesterday!). Can you open a new bug issue for this?

Any plans to add this to the 8.x branch? I'd love to be able to enable the time limit feature WITH page caching enabled. Happy to contribute if this is something that would be committed.

I've made a direct port of the patch (attached). I tested two test cases manually, one to verify that submissions are blocked correctly and one to confirm that they are submitted (without being blocked) correctly. I did not test the "javascript disabled" codepath.

I'm not crazy about the implementation. The string concatenation and explosion feels a bit yucky. I'm not sure how necessary it is to concatenate the 'js_token:' and $identifier prefixes, but it works.

It could use post update hook for existing sites

You mean a hook_update_n() implementation? What would it do? I guess I could save a default value for the new config setting:

/**
 * Add default value for use_js_for_cached_pages.
 */
function honeypot_update_8101() {
  $config_factory = \Drupal::configFactory();
  $config = $config_factory->getEditable('honeypot.settings');
  $config->set('use_js_for_cached_pages', FALSE);
  $config->save(TRUE);
}

Oops. Removing patch contents from diff.

Would like to test against 2.0.x since that's the new maintained branch for D8/D9.

This looks good on first glance, one minor nit, honeypot_create_expirable_key function docs just mention it returns a string; I'd like there to be at least a one line description in the doc that explains what the function does with a tiny bit more detail than the function name.

Ok this is rolled against 2.0.x and I've cleaned up the implementation.

One last request—is there any way to be able to add a test that uses JS (versus the existing tests which don't)? I can imagine this will be very fragile if we make modifications to some of the non-JS code that break what the new JS is expecting.

honeypot_update_8101() better to replace with post update hook, because it's for 2.x branch and does not affect database

> One last request—is there any way to be able to add a test that uses JS (versus the existing tests which don't)? I can imagine this will be very fragile if we make modifications to some of the non-JS code that break what the new JS is expecting.

That should be possible but I don't think I'll be able to do it--I've run out of time for this project. If someone would like to add to my patch, please feel free!

There still seems to be an issue when you are retrying. The identifier is getting duplicated, and that leads to a failure in validation. The user is stuck if he submitted to early the first time.

js_token:js_token:js_token:Kj39ldwDCSGHHIjYki92SmE2yyVu6fevyPb2bTMoZ5w|7|23|4

This is my identifer after 3 retries. Al previous attempts are in the identifier, and the php split on pipe always returns the first failure (7 seconds)

This patch fixes the last issue for me.

This patch fixes also a fatal error if javascript is disabled. The previous version was using form_state as an array instead of an object.

Seems like the js part was missing in my patch. An updated version

Here's an updated patch based on the previous ones with fixed JS.

Wait, this was fixed in D7 almost 4 years ago but was never committed to D8/D9? That's a major bug - we've lost functionality in the current version of Drupal. This is exactly why new features should go into the latest version FIRST, and only after that be backported. If it's not done in that order, we end up like in this issue where a lot of effort was spent to add features to an almost-obsolete version of this module without benefiting the current version

Re-rolled #37 to apply to current HEAD.

I don't understand what the patch is doing here:

-      $identifier = $input['honeypot_time'];
+      [$identifier] = explode('|', str_replace('js_token:', '', $input['honeypot_time']));

What use is it to assign a value of an anonymous array that isn't/can't be used later on in the function?

It seems like this change was added in #34, but as there is no interdiff and no discussion I have no idea why this change was made.

No-one has been providing interdiffs with their patches, so it's hard to trace back all the changes that have been made in the past 4 years to figure out whether all the above concerns have been addressed.

And this functionality absolutely needs a test. We have the ability to add good functional JavaScript tests in Drupal now, so we should use that.

Forgot to attach the re-rolled patch. And I also forgot to add the new files (.libraries.yml and the .js file), so ignore this and I'll upload the correct patch in the next comment.

The tests pass, but there is no test for the new functionality yet and it's not clear how this current patch compares to the D7 feature.

This could use some reviews and some community input.

Hey , this works great for cached pages. However it didn't seem to work on uncached pages, it only seems to work if the page is cached.

Use case scenario: flush caches, test on a page on first load. First load the timer doesn't seem to protect. Subsequent page loads do though.

Wondering if we could optionally allow this to work on uncached pages as well as cached pages?

Still needs tests.

Yes, this feature should be designed to work on both cached and uncached pages. The simple act of writing the test probably would have revealed what @joseph.olstad found in #43.

Unless I'm not testing correctly, further testing on my end shows that this functionality only seems to work on a 'soft' refresh in the browser.

A soft refresh being a single press of the F5 key.
A hard refresh being pressing the ctrl->shift->R key combination.

Soft refresh seems to behave as expected;
Hard refresh doesn't work, maybe it's a limitation, not sure.

perhaps a javascript call to an ntp service could be used but then network lag would affect the accuracy probably.

it's an improvement but the patch is incomplete /doesn't cover a timer for all situations . It's a bit complex to resolve though. I was thinking perhaps an external ntp service or have the module expose a non-cached ajax REST endpoint that provides the current time on the server and to submit it polls on document ready then on submit, compares how many seconds, this would be easiest to implement probably and would probably be fairly efficient

Unfortunately the patch is not applying anymore on Drupal 9.3.

@interactivex, the patch is for honeypot 2.x not Drupal 9.3 core, perhaps apply it to honeypot instead of core

with that said, the patch needs work, it only covers a very narrow scope which is browser cached forms.

I am, of course, applying it to Honeypot, but it's not applying on the latest version I use with Drupal 9.3

@interactivex, ok so a new branch was "JUST" pushed as a recommended replacement for 2.0.x, 2.1.x and it breaks the patch.

 ╭─◀ ☕ j envy-4700u ▶ ~/drupal.org/honeypot ▶ 📂4 [f] 12 🔗0 ▶ 🔀 2.1.x ▶
 ╰❯ $ patch -p1 --dry-run < honeypot-js-2820400-37.patch 
checking file config/install/honeypot.settings.yml
checking file config/schema/honeypot.schema.yml
checking file honeypot.install
Hunk #1 succeeded at 70 (offset -2 lines).
checking file honeypot.libraries.yml
checking file honeypot.module
Hunk #1 FAILED at 168.
Hunk #2 FAILED at 187.
Hunk #3 succeeded at 178 with fuzz 2 (offset -21 lines).
Hunk #4 FAILED at 231.
Hunk #5 FAILED at 243.
Hunk #6 FAILED at 260.
5 out of 6 hunks FAILED
checking file js/honeypot_timestamp.js
can't find file to patch at input line 265
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|diff --git a/src/Controller/HoneypotSettingsController.php b/src/Controller/HoneypotSettingsController.php
|index 18794d5..2c2a1b6 100644
|--- a/src/Controller/HoneypotSettingsController.php
|+++ b/src/Controller/HoneypotSettingsController.php
--------------------------
File to patch:

In either case, the patch needs work as it needs improvements either way.

Since the patch of #40 is now failing, because of the rework that has been done to the honeypot 2.1x version, I've created a new patch that does comply with the 2.1x version. Biggest difference was moving some functions from the .module file to the service file.

Found out there was a bug created by the previous patch. This one works.

Just hit this issue and wrote https://julian.pustkuchen.com/en/beware-drupal-spam-protection-modules-k... about it.
Great to see this is planned to be fixed in Honeypot! The antibot module for example doesn't kill page cache FYI!

I'm wondering if the additional setting is really required and useful, as caching is typically used on most projects and a user would expect that it "just works" I guess. So why would I want to disable this feature? (UX)

Once this is fixed, it would really make sense to add something similar to the CAPTCHA module to load via AJAX and not break cache. Both modules are used by hundred-thousands of Drupal installations(!) ;) I guess most of them (like me till now) just don't know they're affected so hard.

@Anybody, wow ya good to know, I didn't know about the antibot module, thanks!
https://www.drupal.org/project/antibot

I might be missing something about this approach, but if we're now trusting the client side to tell us how long it has been since a form has been loaded on the page, there's nothing to stop spammer simply sending a random, large value in the form submission as the 'time since page load'? The honeypot module will validate the input and find that it's been a long time since the original form request and allow the request to proceed.

The very original approach by @awolfey did something subtly different whereby there was an AJAX request made on page load to the server that would fetch in a newly signed timestamp into the form, the client wasn't trusted.
But it looks like in #9 the version simply concatenates the 'time since page load' to the signed token, and the server implicitly trusts that time since, if the signed token is correct. That version was committed to Drupal 7 and I believe is still in the patch in #55.

I might be missing something about this approach, but if we're now trusting the client side to tell us how long it has been since a form has been loaded on the page, there's nothing to stop spammer simply sending a random, large value in the form submission as the 'time since page load'?

Yes, you're right. But it's like all the "honeypot"-like strategies. There is nothing to stop a spammer to wait 30 seconds before sending the request :) It's just a bet on spambots not willing to spend resources on waiting time, or in this case recognize that a client-side timestamp is validated and set it to a different value.

@szeidler Thanks for that, makes sense I guess.

From #56:

I'm wondering if the additional setting is really required and useful, as caching is typically used on most projects and a user would expect that it "just works" I guess. So why would I want to disable this feature? (UX)

Okay, so maybe this is the reason to make this configurable, so that if you really need server side honeypot time based protection, you can get it back?

For 2.1.3 patch honeypot-js_timelimit_for_cached_pages-2820400-53-1.patch has an issue - module already has implemented hook_update_8101, so we need to change function name to honeypot_update_8103() in the patch.

Just by checking at the code, the patch in #61 (and all previous ones) rely on jQuery Once, but this is not supported on Drupal 10 any more.

The patch is not compatible with Drupal 10.

Added an MR based on patch on #61, updated to not rely on jquery once but on core/once.