Run on_spam callback if timestamp triggers but passes through

[bb]

I'd like to allow letting pass a request in on_timespan_spam.

However, when I decide to allow a request in my on_timespan_spam callback, the honeypot_spam detection is never run. The reason is obviously the elsif in

invisible_captcha/lib/invisible_captcha/controller_ext.rb

Line 24 in 6a19a93

elsif honeypot_spam?(options) || spinner_spam?

.

This PR changes the controller extension so that the on_spam detection and callback are still performed, even though the timestamp_spam? triggered but on_timespan_spam callback doesn't render/redirect.

Use cases

• enable on_timespan_spam only for logging to get insights how well it performs
• use on_timespan_spam callback only to set instance variables for later, e.g. require an additional confirmation
• allow requests if there are strong reasons, e.g. having certain preconditions met in Ahoy current_visit.events

Specs

All existing specs continue to pass unchanged.

Two new examples were added.
The first one succeeds with the new controller extension code but fails with the old controller extension code.
The second one succeeds also with the old code, but for the wrong reason.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (6a19a93) 98.39% compared to head (f5ae177) 98.44%.

❗ Current head f5ae177 differs from pull request most recent head ee48c63. Consider uploading reports for the commit ee48c63 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #132      +/-   ##
==========================================
+ Coverage   98.39%   98.44%   +0.05%     
==========================================
  Files          22       22              
  Lines         435      449      +14     
==========================================
+ Hits          428      442      +14     
  Misses          7        7              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[markets]

Thanks @bb! I think that change makes sense 🤔 but I'm a bit worried if this can potentially break any app during the upgrade.

[bb]

Let's analyze the code paths to see whats happening to see if we find any potentially breaking codepaths:

• If no on_timespan_spam callback option is provided, the default callback redirects which leads to performed?becoming true and not running honeypot_spam?(options) || spinner_spam? (nor the body of the elsif).
• if on_timespan_spam is provided, the code there may
  • either render/redirect, resulting in early exist, as previously
  • do nothing which is considered performed? by ActionController which would now call honeypot_spam? and possibly also spinner_spam?.

So, we only need to look at the last bullet. Next question is: Has this been a supported code path? If no: we could break it. If yes: why wasn't it in the tests/specs?

Now let's look at the use case an app could have implemented: if and only if timespan spam is detected, skip both the honeypot spam check and the spinner check and let the action be performed. Is this something which you currently want to have supported and don't want to change? Personally, I was surprised why the honeypot check was not performed when I out-commented code in my timespan callback.

Did I cover everything? Does this reasoning convince you?

If it doesn't, are you considering this for the next major version? Or would you prefer to make the change opt-in?

[markets]

Thanks for such detailed analysis 👍🏼

If yes: why wasn't it in the tests/specs?

You know, having 100% mutant coverage testing would be the perfect situation in the perfect world, but at the end of the day this is just a gem I built and maintained for free for more than 10 years, so yeah probably there are things that aren't covered. But after your reasoning, I now think that probably this is not that breaking 😅 ... On the other hand, I don't have the exact numbers, but probably most of the people are using the defaults.

We can also introduce a new opt-in setting, but this gem already has a lot of flags and this makes things harder in the long-run. So, I'm now in favor of merging and releasing as it is. Let's add a Changelog entry, so at least if anybody has a problem with that change, they can track it by reading the Changelog file.

[bb]

I'm happy you didn't choose the opt-in option 😅
Should I write a Changelog entry or do you want to do it, maybe part of a new release?

[markets]

If you don't mind, let's add a new "unreleased" section at the top of the file, describing the behavoir change. It will be released soon, probably as v2.3.

[bb]

done

[markets]

Thanks @bb