Update reCAPTCHA to use reCAPTCHA API version 2.0 #2152
Conversation
This change introduces new class Jetpack_ReCaptcha that handles verification and rendering HTML of reCAPTCHA widget. Previous lib, recaptchalib.php, is replaced by Jetpack_ReCaptcha. Initial test for Jetpack_ReCaptcha is also included in this change.
Also reset reCAPTCHA when verification of user response failed.
It's too early to define this filter without knowing whether users want it or not.
The official PHP client uses that, however this parameter is not required and undocumented. Google might be used this to recognize their own client version. In that case using Jetpack version will disrupt their expectation.
| $recaptcha = new Jetpack_ReCaptcha( RECAPTCHA_PUBLIC_KEY, RECAPTCHA_PRIVATE_KEY ); | ||
| $response = ''; | ||
| if ( isset( $_POST['g-recaptcha-response'] ) ) { | ||
| $response = sanitize_text_field( $_POST['g-recaptcha-response'] ); |
There was a problem hiding this comment.
Is sanitizing necessary here? What if we end up stripping or modifying characters that reCAPTCHA expects? (e.g. <)
There was a problem hiding this comment.
We don't store the value of g-recaptcha-response so I think sanitizing is not needed. There won't be < — it will break the reCAPTCHA markup as the value is populated in invisible textarea.
I've removed the sanitize_text_field in gedex@755d68a.
Sanitizing the value could end up stripping user's response. This will makes verification failed. Since value is not persisted, it's safe to skip sanitize_text_field.
There's `script_async` config that can be passed to constructor to override default behaviour.
| * | ||
| * @var array | ||
| */ | ||
| private $error_codes = array( |
There was a problem hiding this comment.
Should we translate these and any other strings in this class? Even though we're not using them directly now, another module could use them in the future.
There was a problem hiding this comment.
For error messages done in gedex@19d9af4
This makes it easier to follow from top to bottom.
jeherve
added
[Type] Enhancement
| $result = $this->recaptcha->verify( '', '127.0.0.1' ); | ||
| $this->assertInstanceOf( 'WP_Error', $result ); | ||
|
|
||
| // TODO: test succeed and failed respones. Make sure to mock wp_remote_post. |
There was a problem hiding this comment.
Can we actually implement this TODO, since it's probably the most useful set of tests to have?
Method verify calls wp_remote_post to verify user response with reCAPTCHA server. To be able to mock the server response we neeed to short-circuit the request via pre_http_request filter.
|
@mjangda is this ready to merge? If so, could you please add "Ready to Merge" label? I don't have permission to alter the labels. |
kraftbj
added
the
[Status] Ready to Merge
|
@kraftbj Thanks! |
| esc_attr( $this->config['tag_attributes']['theme'] ), | ||
| esc_attr( $this->config['tag_attributes']['type'] ), | ||
| esc_attr( $this->config['tag_attributes']['tabindex'] ), | ||
| esc_attr( $this->config['language'] ), |
There was a problem hiding this comment.
It would be better to use rawurlencode instead of esc_attr since it's used as a URL param.
|
@gedex spotted two more small things. Other than that, looks good to go! 🎆 |
The language param is used as query string URL of script src attribute. The usage of rawurlencode is more appropriate than esc_attr as it's not passed as full value of attribute.
We're mocking the response so there's no real API call that would use those keys.
|
Thanks @mjangda ! I've fixed those two things. |
|
Tested and read through code. Nice work @gedex, thank you :) |
Update reCAPTCHA to use reCAPTCHA API version 2.0
dereksmart
removed
the
[Status] Ready to Merge
This change introduces new class
Jetpack_ReCaptchathat handles verification and rendering HTML of reCAPTCHA widget. Previous lib,recaptchalib.php, is replaced byJetpack_ReCaptcha. Initial test forJetpack_ReCaptchais also included in this change.This fixes #1368.