Conversation
…ble challenge generation.
demiankatz
left a comment
There was a problem hiding this comment.
Thanks for getting the ball rolling on this, @crhallberg. I see there's still a lot of TODO work underway, so I haven't tried it yet... but see below for some comments from an initial review. Most of what I've highlighted is comment/documentation stuff, so not exactly high priority... but might as well prevent copy-and-paste errors at the earliest opportunity! ;-)
Co-authored-by: Demian Katz <demian.katz@villanova.edu>
Co-authored-by: Demian Katz <demian.katz@villanova.edu>
Co-authored-by: Demian Katz <demian.katz@villanova.edu>
demiankatz
left a comment
There was a problem hiding this comment.
Thanks, @crhallberg, I've given this another look (but still haven't tried hands-on yet). See below for some further suggestions.
…lute certainty that I forgot to update the config.ini and maybe - horror upon horrors - committed my local config (mercifully, no).
demiankatz
left a comment
There was a problem hiding this comment.
Thanks, @crhallberg -- see below for a few thoughts!
|
@demiankatz in response to all the translation strings needed - consider them debug for now. Altcha does translation so the easiest solution might just be to use Altcha and remove my custom code solution. The only benefits would be file size, customization, and increasing the number of open-source projects we're responsible for. Also, sorry for making you review a file I meant to delete (captcha-altcha.js). |
demiankatz
left a comment
There was a problem hiding this comment.
Thanks for the progress, @crhallberg (and on your day off, no less). See below for a few minor new things.
| /** | ||
| * Constructor | ||
| * | ||
| * @param AltchaOrg\Altcha\Altcha $altcha Required HMAC key for challenge calculation and solution verification. |
There was a problem hiding this comment.
This comment needs to be updated to reflect the new value (and aligned with the values below).
Co-authored-by: Demian Katz <demian.katz@villanova.edu>
…to pow-captcha
|
We had hoped that this privacy-friendly version of the proof-of-work captcha would already be included in version 11. A GDPR-compliant version with Altcha would be much more popular in Germany than an external version with Cloudflare. Thank you very much for your work on this feature so far! We eagerly await its release :). |
|
@felixlohmeier, I haven't been able to give this PR much attention because it wasn't slated for the 11.0 release, and the things that are already scheduled for that milestone have been taking up all of my time. I think we can easily aim for 11.1 with this one -- and if you have time to test and review in the meantime, getting this done in time for 11.0 is not completely impossible... but I do still have a few other things I need to give priority to first. |
demiankatz
left a comment
There was a problem hiding this comment.
Thanks, @crhallberg, this is off to a very good start!
See below for a couple of modernization suggestions. I've also pushed up a couple of adjustments directly (resolving conflicts, fixing outdated license comments, and preventing a fatal error if the user intentionally garbles the CAPTCHA input sent to the server).
You have quite a few to-do checkboxes here. Which of these do you consider necessary for a mergeable minimum viable product, and which do you think should be deferred until later?
| ->get(\VuFind\Config\PluginManager::class) | ||
| ->get('config'); |
There was a problem hiding this comment.
\VuFind\Config\PluginManager has been deprecated. This should become:
| ->get(\VuFind\Config\PluginManager::class) | |
| ->get('config'); | |
| ->get(\VuFind\Config\ConfigManager::class) | |
| ->getConfigArray('config'); |
| $secret = $config->Captcha->altcha_secret ?? null; | ||
|
|
||
| if (empty($secret)) { | ||
| throw new \Exception('Secret key needed for Altcha. See config.ini.'); | ||
| } | ||
|
|
||
| $algorithm = Algorithm::from($config->Captcha->altcha_algorithm ?? 'SHA-256'); | ||
| $maxNumber = $config->Captcha->altcha_max_number ?? 100000; | ||
| $saltLength = $config->Captcha->altcha_salt_len ?? 12; | ||
| $expiresInterval = $config->Captcha->altcha_expires_interval ?? null; | ||
| $params = $config->Captcha->altcha_params ?? []; |
There was a problem hiding this comment.
...and then we need to convert all the object notation to array notation:
| $secret = $config->Captcha->altcha_secret ?? null; | |
| if (empty($secret)) { | |
| throw new \Exception('Secret key needed for Altcha. See config.ini.'); | |
| } | |
| $algorithm = Algorithm::from($config->Captcha->altcha_algorithm ?? 'SHA-256'); | |
| $maxNumber = $config->Captcha->altcha_max_number ?? 100000; | |
| $saltLength = $config->Captcha->altcha_salt_len ?? 12; | |
| $expiresInterval = $config->Captcha->altcha_expires_interval ?? null; | |
| $params = $config->Captcha->altcha_params ?? []; | |
| $secret = $config['Captcha']['altcha_secret'] ?? null; | |
| if (empty($secret)) { | |
| throw new \Exception('Secret key needed for Altcha. See config.ini.'); | |
| } | |
| $algorithm = Algorithm::from($config['Captcha']['altcha_algorithm'] ?? 'SHA-256'); | |
| $maxNumber = $config['Captcha']['altcha_max_number'] ?? 100000; | |
| $saltLength = $config['Captcha']['altcha_salt_len'] ?? 12; | |
| $expiresInterval = $config['Captcha']['altcha_expires_interval'] ?? null; | |
| $params = $config['Captcha']['altcha_params'] ?? []; |
|
Interesting. We integrated Altcha in our firewall script that we load in a modified index.php -- you can find this among other things in our SUBHH core module. We choose the "firewall approach" to avoid the heavy lifting associated with the laminas service manager. This integrated solution has been successfully tested and is scheduled to get rolled out in the next weeks. |
|
@dmj, I think there are reasons to consider both approaches -- as a general bot blocker, your firewall approach is definitely more efficient. But this PR offers the possibility of enabling altcha to protect specific forms and functionality within the application, and that may be helpful for different use cases. |
TODO
[ ] Translations(handled by Altcha)[ ] Visuals (or no)(handled by Altcha)[ ] Turnstyle functionality(future work)TESTS
Projects that inspired this work
Future Work
Altcha support/replacement.Homegrown Solution
I started with a bespoke solution before discovering Altcha's PHP integration. I have removed my handmade version, but I have preserved it for future reference or educational reasons as a GitHub Gist.
Process
Hashes
Proof of Work (PoW) is a good way to make bot traffic more expensive, slowing and discouraging crawling. While there are better hashes for PoW, I started with natively supported hashes (SHA family).