UndefinedOffset / silverstripe-nocaptcha

A spam protector and form field using Google's reCAPTCHA v2 or optionally a foundation v3 implementation
BSD 3-Clause "New" or "Revised" License
31 stars 37 forks source link

reCaptcha & HTML5 Validation #66

Closed LiamW closed 3 years ago

LiamW commented 3 years ago

The invisible mode overtakes a button's click and submits the form first without running the HTML5 validation. So the form tries to submit. Yes you can set SS required on the fields, but the page still reloads with errors. Better UX to have HTML5 validation working.

https://stackoverflow.com/questions/44021400/how-to-run-recaptcha-only-if-html5-validation-has-passed/44026198#44026198

purplespider commented 3 years ago

I would argue this is more of a bug than an enhancement, as it causes Userforms' javascript validation to be bypassed.

e.g. You have a Userform and have set a field to have a minimum length. If the user has entered fewer characters than required but clicks submit, it briefly says "Please enter at least 10 characters." but submits the form anyway:

Screenshot 2021-07-09 at 13 38 27

UndefinedOffset commented 3 years ago

ya I agree @purplespider flagged as such

UndefinedOffset commented 3 years ago

@LiamW In my local tests (Firefox and Chrome) pure html5 validation is being triggered before the submit event so nocaptcha does not appear to be involved. I'm curious what browser you were using and what mode of recaptcha (invisible, normal, v3)?

@purplespider In my local testing I do see jQuery validate being triggered that said perhaps it's a load order problem. If userforms.js is loaded after NocaptchaField.js it's possible that it's listeners are not being triggered properly.

Can both of you post what version of userforms you are using (if applicable), nocaptcha, and which version of Silverstripe you are using.

UndefinedOffset commented 3 years ago

Also any JS console errors would be helpful

purplespider commented 3 years ago

Here's an example: https://nocaptcha.pswd.biz/test-form Try submitting the form with less than 10 characters and see what happens.

This is a fresh Silverstripe 4.8.0 install on PHP 7.4, just with the latest versions of userforms and nocaptcha installed:

spam.yml:

SilverStripe\SpamProtection\Extension\FormSpamProtectionExtension:
    default_spam_protector: UndefinedOffset\NoCaptcha\Forms\NocaptchaProtector
UndefinedOffset\NoCaptcha\Forms\NocaptchaField:
    site_key: "XXXX" #Your site key (required)
    secret_key: "XXXX" #Your secret key (required)
    default_theme: "light" #Default theme color (optional, light or dark, defaults to light)
    default_size: "invisible" #Default size (optional, normal, compact or invisible, defaults to normal)
    default_badge: "bottomright" #Default badge position (bottomright, bottomleft or inline, defaults to bottomright)

I then created a user forms page, added a text field and the spam protection field, and set the "Allowed text length" on the text field to "10 to 1000".

Not seeing any errors in the console.


Here is an example page with a single field set as Required, like @LiamW's experience: https://nocaptcha.pswd.biz/test-required-form

You can see if you submit the form without completing the field, the JS validation error appears, but the form still submits, then the server-side validation error message appears.

UndefinedOffset commented 3 years ago

@purplespider So i think what's happening in your test instance is one i've seen before in other cases, basically the simple theme in this case is loading another jQuery version which is basically removing the event listeners setup in the one supplied by user forms. I wonder if this is what could be happening in both cases talked about here.

I do see the captcha trying but since the form has already begun to navigate away it's not actually being added to the form. I wouldn't be surprised to see the issue go away if one of the two jQuery versions is removed.

two-jquery-instances

LiamW commented 3 years ago

I've been away but I might be able to take a look at this later today. Just a heads up, I was having the same issue with the invisible mode and only 1 jquery version. I wasn't using simple theme, but my custom design.

Would have been latest SS and module version from when I first posted this.

I also don't use the userform module.

purplespider commented 3 years ago

I wouldn't be surprised to see the issue go away if one of the two jQuery versions is removed.

@UndefinedOffset Thanks, yep, spot on! I appreciate that this specific issue (in my case at least) isn't actually related to this module now. But would the ideal fix in this instance be to move the jQuery requirement in the "simple" theme from Page.ss to PageController::init() (so it loads before the userforms js files), and stop userforms from adding its own copy of jQuery?

If so, I'll to add a PR to userforms with a config option to disable just the jQuery requirement. (Currently, there is an option to disable all javascript reqs (block_default_userforms_js), but you can't solely disable the jQuery one without using Requirements:block and this is more likely to break with any changes to the jQuery location in future).

UndefinedOffset commented 3 years ago

@purplespider Ya that would be what I would think is probably the best bet, prevent userforms from adding it it's own jQuery version at least. I think simple is designed to work out of the box work so it kind of makes sense that it requires it's own version. Technically you can block it in PageController::init() using Requirements::block() and technically you could also do the same with UserForms as well.

LiamW commented 3 years ago

@UndefinedOffset Can I send you the site to check in an email? I don't want to post it here. It's on the live site now. I can fill out the form on your website.

UndefinedOffset commented 3 years ago

@LiamW perhaps reach out to me via the contact form on https://webbuildersgroup.com/ I honestly hardly check the email on my own site anymore. I'll grab the email when it comes into the inbox for that form, we can connect privately from there.

UndefinedOffset commented 3 years ago

@LiamW Got your email, in your case it does seem to be that the HTML5 validation is not being allowed to run for some reason.

I do have a theory... I see you have a couple attributes added to the submit button data-sitekey, data-callback, data-action and a class g-recaptcha. You shouldn't need to do that the scripting provided by this library should handle mapping the submit button properly. Which will based on my local tests allow HTML5 validation to work since it's listening on the form submit rather than the clicking of the button. Basically what's happening right now is reCaptcha's own code is kicking in because of those attributes and it is binding to the click of the button rather than the form submit. So removing what I've mentioned here should solve the issue.

I wanted to post here for visibility in case someone runs into a similar problem. But let me know if you want to continue this privately from this point.

LiamW commented 3 years ago

Ya I'm fine with discussing here as it makes sense.

As I mentioned in the email, I'm not much of a dev myself. I'm also not doing anything custom to add those attributes in myself. I'm running a super basic form. Attached a screenshot of it so you can see the code minus domain stuff I've removed.

So those attributes are being added by the modules. Either yours or spam protection I guess.

Screen Shot 2021-08-20 at 4 05 16 PM

UndefinedOffset commented 3 years ago

@LiamW there's definitely something odd going on I'm not seeing this behavior on my local test environment, do you mind posting your composer.json file? As well as your your yaml config for nocaptcha like purplespider posted (make sure to remove the keys). Basically I'm trying to figure out if something else is changing the form, because when viewing the source I'm definitely seeing something adding attributes to the submit button which this module does not do.

Edit: Module does do this when in v3 mode.

LiamW commented 3 years ago

Sure no problem.

Screen Shot 2021-08-23 at 12 15 33 PM

And

Screen Shot 2021-08-23 at 12 21 49 PM

UndefinedOffset commented 3 years ago

Ah v3 recaptcha seems to be related to that, I'll look into it and let you know what I find, explains why I wasn't seeing it :) I was testing v2 invisible.

UndefinedOffset commented 3 years ago

@LiamW can you give master a try and see if that works for you (I assume you have a test version of the affected site). If it does I'll tag a new release.

LiamW commented 3 years ago

Yup I believe that has fixed it. Works on my local test site. If I come across anything when I push live or in general, I'll let you know.

Thanks!

UndefinedOffset commented 3 years ago

Tagged as 2.1.1