Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added self-hosted GDPR compliant captcha module #109

Open
wants to merge 55 commits into
base: main
Choose a base branch
from
Open

Conversation

fballiano
Copy link
Contributor

@fballiano fballiano commented Feb 17, 2025

This PR adds a new Maho_Captcha module, which implements self-hosted GDPR compliant captcha based on https://altcha.org. Research was done but Altcha seems to be the most active other open source PoW based captcha project.

At the moment the implementation is almost the same as my Turnstile module: https://github.com/fballiano/openmage-cloudflare-turnstile with a lot of observers and a "css selectors" settings that (IMHO) allows for maximum flexibility.

I called it Maho_Captcha cause I think Maho should provide a basic captcha module and, since this one doesn't rely on 3rd party services (like cloudflare/recaptcha) it seems the perfect candidate.

Questions:

  • I used maho_captcha.(xml|csv) naming instead of just captcha.(xml|csv) because I didn't want it to collide with the old Mage_Captcha. Is this a good choice?
  • I used maho/captcha/footer.html as folder structure for templates. I don't like that it differs from maho_captcha but at the same time made more sense to have all modules under the maho/ folder. Is this a good choice?
  • Should we apply this naming (or the final one that we decide) to New feature: blog #103 too? We should have a standard for every new development.
  • Since the module positions the captcha widget "just before the ending" of the form, this position may not be perfectly aligned, is this a dealbreaker? ideas on how to make it better? activating the "floating" catpcha if works perfectly

}

$request = Mage::app()->getRequest();
if ($request->getActionName() == 'prelogin' || !$request->isPost()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like this line...

@fballiano fballiano marked this pull request as draft February 22, 2025 10:45
@fballiano
Copy link
Contributor Author

Converted to draft because of altcha-org/altcha#92

@fballiano fballiano marked this pull request as ready for review February 24, 2025 11:16
@fballiano
Copy link
Contributor Author

I found a way to work around the limitation of a single captcha per page so I think this PR is testable

@fballiano
Copy link
Contributor Author

As per https://altcha.org/docs/server-integration/

Replay attacks

To prevent the vulnerability of “replay attacks,” where a client resubmits the same solution multiple times, the server should implement measures that invalidate previously solved challenges.

The server should maintain a registry of solved challenges and reject any submissions that attempt to reuse a challenge that has already been successfully solved.

we have to create a database table to store solved challanges and a cleanup routing after a few days (I'd use 7)

@fballiano
Copy link
Contributor Author

hey @justinbeaty, I think the PR is at a point that it finally works 🤞🤞🤞

@justinbeaty
Copy link
Contributor

hey @justinbeaty, I think the PR is at a point that it finally works 🤞🤞🤞

Sweet -- I'll test now.

@justinbeaty
Copy link
Contributor

I really like this implementation! However I did find a few issues if you submit the form while the captcha is validating (either the first time, or revalidating). You'd get a JS alert that says "please wait..." but actually you need to close the alert and re-submit the form. If you wait as instructed you'll be waiting forever...

image

After you dismiss the alert, you also get a JS error An invalid form control with name='' is not focusable., which should be fixed upstream (I'll create an issue in their repo later).

In any case, what I tried to do (and think it's working) is to add an event listener for form submission, and if the captcha is still validating, show a "loader" and then once it's validated submit the form programmatically.

In app/code/core/Maho/Captcha/Helper/Data.php, you can set the maxNumber option to 2500000 to be able to test this more easily.

Also note, I updated the "CSS Selectors" to include #page-login #loginForm so that the same implementation is shared across frontend / backend. You'll need to add this in your config. If you're locked out, you can add this in public/js/maho-captcha.js:

this.frontendSelectors += ', #page-login #loginForm';

I actually have to leave the house very shortly, so normally I'd write more and test more thoroughly before pushing. If there's a problem as always we can revert or modify.

@fballiano
Copy link
Contributor Author

I've changed a few things like auto-verification, lazy-load, backend selector.

I don't love that the js became a bit too complex in my opinion and it's a separate http request but probably it's good cause it can be cached.

I think it should be testable again.

@justinbeaty
Copy link
Contributor

+1 to all the changes. Yes, sorry the JS became more complicated. The form submit issue was just annoying UX wise and the fix was more complicated to keep it inline.

I have a few more things to review if that's okay. One thing I was thinking of, could we replace the mysql table (and thus model, resource model, and install scripts) by instead using Mage's cache? Something like this (not tested).

    protected function checkLoggedChallenge(string $payload): bool
    {
        Mage::app()->getCache()->test($payload) !== false;
    }

    protected function logChallenge(string $payload): void
    {
        Mage::app()->getCache()->save(time(), $payload, ['maho_captcha'], 3600);
    }

@fballiano
Copy link
Contributor Author

I have a few more things to review if that's okay.

no hurry

One thing I was thinking of, could we replace the mysql table (and thus model, resource model, and install scripts) by instead using Mage's cache? Something like this (not tested).

I thought about it but I think somebody could need to monitor the size of that table (to see if there are attacks or something like that) and magento's cache is impossible to monitor or, if it grows too much, could it create problems (for example redis with limite resources or file system cache)?

@fballiano
Copy link
Contributor Author

with the "check for verification" feature, for example the newsletter form gets stuck this way:

Screenshot 2025-03-09 alle 20 10 11

doesn't the feature replace the original submit handler for the form?

Another thing is that it seems not to work in the checkout if I click the "continue" button without first focusing the form, I get an "server returned an invalid json" :-\ I'll try to do more debug asap

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants