From 1cc2dfc02066ed919b01e3da8b01d87bfb381642 Mon Sep 17 00:00:00 2001 From: Pavel Bystritsky Date: Thu, 23 Jul 2020 13:05:28 +0300 Subject: [PATCH 1/6] magento/security-package#222: Add validation for error message. --- ReCaptchaAdminUi/Model/ErrorMessageConfig.php | 57 ++++++++++++++++ ReCaptchaAdminUi/etc/adminhtml/di.xml | 2 + ReCaptchaAdminUi/etc/adminhtml/system.xml | 15 +++++ ReCaptchaAdminUi/etc/config.xml | 19 ++++++ .../Model/AjaxLogin/ErrorProcessor.php | 66 +++++++++++++++++- .../Observer/AjaxLoginObserver.php | 6 +- .../Model/ErrorMessageConfig.php | 57 ++++++++++++++++ ReCaptchaFrontendUi/etc/adminhtml/system.xml | 28 ++++++++ ReCaptchaFrontendUi/etc/config.xml | 19 ++++++ ReCaptchaFrontendUi/etc/frontend/di.xml | 3 +- .../Model/ErrorMessageConfigInterface.php | 30 +++++++++ ReCaptchaUi/Model/RequestHandler.php | 62 +++++++++++++++-- ReCaptchaUser/Observer/LoginObserver.php | 67 ++++++++++++++++++- ReCaptchaValidation/Model/Validator.php | 7 +- .../Api/Data/ValidationConfigInterface.php | 3 +- .../Model/ErrorMessagesProvider.php | 4 +- .../Model/ValidationErrorMessagesProvider.php | 40 +++++++++++ ReCaptchaValidationApi/etc/di.xml | 8 ++- .../etc/adminhtml/system.xml | 10 --- .../etc/adminhtml/system.xml | 10 --- .../etc/adminhtml/system.xml | 10 --- 21 files changed, 474 insertions(+), 49 deletions(-) create mode 100644 ReCaptchaAdminUi/Model/ErrorMessageConfig.php create mode 100644 ReCaptchaAdminUi/etc/config.xml create mode 100644 ReCaptchaFrontendUi/Model/ErrorMessageConfig.php create mode 100644 ReCaptchaFrontendUi/etc/adminhtml/system.xml create mode 100644 ReCaptchaFrontendUi/etc/config.xml create mode 100644 ReCaptchaUi/Model/ErrorMessageConfigInterface.php create mode 100644 ReCaptchaValidationApi/Model/ValidationErrorMessagesProvider.php diff --git a/ReCaptchaAdminUi/Model/ErrorMessageConfig.php b/ReCaptchaAdminUi/Model/ErrorMessageConfig.php new file mode 100644 index 00000000..c1ee222b --- /dev/null +++ b/ReCaptchaAdminUi/Model/ErrorMessageConfig.php @@ -0,0 +1,57 @@ +scopeConfig = $scopeConfig; + } + + /** + * @inheritdoc + */ + public function getTechnicalFailureMessage(): string + { + return $this->scopeConfig->getValue( + self::XML_PATH_TECHNICAL, + ScopeInterface::SCOPE_STORE + ); + } + + /** + * @inheritdoc + */ + public function getValidationFailureMessage(): string + { + return $this->scopeConfig->getValue( + self::XML_PATH_VALIDATION, + ScopeInterface::SCOPE_STORE + ); + } +} diff --git a/ReCaptchaAdminUi/etc/adminhtml/di.xml b/ReCaptchaAdminUi/etc/adminhtml/di.xml index 547a2a93..9924803e 100644 --- a/ReCaptchaAdminUi/etc/adminhtml/di.xml +++ b/ReCaptchaAdminUi/etc/adminhtml/di.xml @@ -9,4 +9,6 @@ xsi:noNamespaceSchemaLocation="urn:magento:framework:ObjectManager/etc/config.xsd"> + diff --git a/ReCaptchaAdminUi/etc/adminhtml/system.xml b/ReCaptchaAdminUi/etc/adminhtml/system.xml index f3cfdea0..ff5a7838 100644 --- a/ReCaptchaAdminUi/etc/adminhtml/system.xml +++ b/ReCaptchaAdminUi/etc/adminhtml/system.xml @@ -27,6 +27,21 @@ Magento\ReCaptchaAdminUi\Block\Adminhtml\System\Config\Form\Field\Notice + + + + + + + + + + + +
+ + + + + + reCAPTCHA verification failed. + Something went wrong with reCAPTCHA, please contact Store owner. + + + + + diff --git a/ReCaptchaCustomer/Model/AjaxLogin/ErrorProcessor.php b/ReCaptchaCustomer/Model/AjaxLogin/ErrorProcessor.php index 13602b0f..64bdd682 100644 --- a/ReCaptchaCustomer/Model/AjaxLogin/ErrorProcessor.php +++ b/ReCaptchaCustomer/Model/AjaxLogin/ErrorProcessor.php @@ -9,8 +9,12 @@ use Magento\Framework\App\Action\Action; use Magento\Framework\App\ActionFlag; +use Magento\Framework\App\ObjectManager; use Magento\Framework\App\ResponseInterface; use Magento\Framework\Serialize\SerializerInterface; +use Magento\ReCaptchaUi\Model\ErrorMessageConfigInterface; +use Magento\ReCaptchaValidationApi\Model\ValidationErrorMessagesProvider; +use Psr\Log\LoggerInterface; /** * Process error during ajax login @@ -29,27 +33,72 @@ class ErrorProcessor */ private $serializer; + /** + * @var LoggerInterface + */ + private $logger; + + /** + * @var ErrorMessageConfigInterface + */ + private $errorMessageConfig; + + /** + * @var ValidationErrorMessagesProvider + */ + private $validationErrorMessagesProvider; + /** * @param ActionFlag $actionFlag * @param SerializerInterface $serializer + * @param ErrorMessageConfigInterface|null $errorMessageConfig + * @param ValidationErrorMessagesProvider|null $validationErrorMessagesProvider */ public function __construct( ActionFlag $actionFlag, - SerializerInterface $serializer + SerializerInterface $serializer, + ?LoggerInterface $logger = null, + ?ErrorMessageConfigInterface $errorMessageConfig = null, + ?ValidationErrorMessagesProvider $validationErrorMessagesProvider = null ) { $this->actionFlag = $actionFlag; $this->serializer = $serializer; + $this->logger = $logger + ?? ObjectManager::getInstance()->get(LoggerInterface::class); + $this->errorMessageConfig = $errorMessageConfig + ?? ObjectManager::getInstance()->get(ErrorMessageConfigInterface::class); + $this->validationErrorMessagesProvider = $validationErrorMessagesProvider + ?? ObjectManager::getInstance()->get(ValidationErrorMessagesProvider::class); } /** * Set "no dispatch" flag and error message to Response * * @param ResponseInterface $response - * @param string $message + * @param array $errorMessages + * @param string $sourceKey * @return void */ - public function processError(ResponseInterface $response, string $message): void + public function processError(ResponseInterface $response, array $errorMessages, string $sourceKey): void { + $validationErrorText = $this->errorMessageConfig->getValidationFailureMessage(); + $technicalErrorText = $this->errorMessageConfig->getTechnicalFailureMessage(); + + $message = $errorMessages ? $validationErrorText : $technicalErrorText; + + foreach ($errorMessages as $errorMessageCode => $errorMessageText) { + if (!$this->isValidationError($errorMessageCode)) { + $message = $technicalErrorText; + $this->logger->error( + __( + 'reCAPTCHA \'%1\' form error: %2', + $sourceKey, + $errorMessageText + ) + ); + } + } + $this->actionFlag->set('', Action::FLAG_NO_DISPATCH, true); $jsonPayload = $this->serializer->serialize([ @@ -58,4 +107,15 @@ public function processError(ResponseInterface $response, string $message): void ]); $response->representJson($jsonPayload); } + + /** + * Check if error code present in validation errors list. + * + * @param string $errorMessageCode + * @return bool + */ + private function isValidationError(string $errorMessageCode): bool + { + return $errorMessageCode !== $this->validationErrorMessagesProvider->getErrorMessage($errorMessageCode); + } } diff --git a/ReCaptchaCustomer/Observer/AjaxLoginObserver.php b/ReCaptchaCustomer/Observer/AjaxLoginObserver.php index 5171f397..b682c0be 100644 --- a/ReCaptchaCustomer/Observer/AjaxLoginObserver.php +++ b/ReCaptchaCustomer/Observer/AjaxLoginObserver.php @@ -100,7 +100,8 @@ public function execute(Observer $observer): void $this->logger->error($e); $this->errorProcessor->processError( $response, - $validationConfig->getValidationFailureMessage() + [], + $key ); return; } @@ -109,7 +110,8 @@ public function execute(Observer $observer): void if (false === $validationResult->isValid()) { $this->errorProcessor->processError( $response, - $validationConfig->getValidationFailureMessage() + $validationResult->getErrors(), + $key ); } } diff --git a/ReCaptchaFrontendUi/Model/ErrorMessageConfig.php b/ReCaptchaFrontendUi/Model/ErrorMessageConfig.php new file mode 100644 index 00000000..47234e8d --- /dev/null +++ b/ReCaptchaFrontendUi/Model/ErrorMessageConfig.php @@ -0,0 +1,57 @@ +scopeConfig = $scopeConfig; + } + + /** + * @inheritdoc + */ + public function getTechnicalFailureMessage(): string + { + return $this->scopeConfig->getValue( + self::XML_PATH_TECHNICAL, + ScopeInterface::SCOPE_STORE + ); + } + + /** + * @inheritdoc + */ + public function getValidationFailureMessage(): string + { + return $this->scopeConfig->getValue( + self::XML_PATH_VALIDATION, + ScopeInterface::SCOPE_STORE + ); + } +} diff --git a/ReCaptchaFrontendUi/etc/adminhtml/system.xml b/ReCaptchaFrontendUi/etc/adminhtml/system.xml new file mode 100644 index 00000000..e97642ff --- /dev/null +++ b/ReCaptchaFrontendUi/etc/adminhtml/system.xml @@ -0,0 +1,28 @@ + + + + +
+ + + + + + + + + + + +
+ + diff --git a/ReCaptchaFrontendUi/etc/config.xml b/ReCaptchaFrontendUi/etc/config.xml new file mode 100644 index 00000000..3f32365f --- /dev/null +++ b/ReCaptchaFrontendUi/etc/config.xml @@ -0,0 +1,19 @@ + + + + + + + reCAPTCHA verification failed. + Something went wrong with reCAPTCHA, please contact Store owner. + + + + + diff --git a/ReCaptchaFrontendUi/etc/frontend/di.xml b/ReCaptchaFrontendUi/etc/frontend/di.xml index 5f00c9a8..56f11f03 100644 --- a/ReCaptchaFrontendUi/etc/frontend/di.xml +++ b/ReCaptchaFrontendUi/etc/frontend/di.xml @@ -9,7 +9,8 @@ xsi:noNamespaceSchemaLocation="urn:magento:framework:ObjectManager/etc/config.xsd"> - + diff --git a/ReCaptchaUi/Model/ErrorMessageConfigInterface.php b/ReCaptchaUi/Model/ErrorMessageConfigInterface.php new file mode 100644 index 00000000..e3173735 --- /dev/null +++ b/ReCaptchaUi/Model/ErrorMessageConfigInterface.php @@ -0,0 +1,30 @@ +captchaResponseResolver = $captchaResponseResolver; $this->validationConfigResolver = $validationConfigResolver; @@ -73,6 +89,10 @@ public function __construct( $this->messageManager = $messageManager; $this->actionFlag = $actionFlag; $this->logger = $logger; + $this->errorMessageConfig = $errorMessageConfig + ?? ObjectManager::getInstance()->get(ErrorMessageConfigInterface::class); + $this->validationErrorMessagesProvider = $validationErrorMessagesProvider + ?? ObjectManager::getInstance()->get(ValidationErrorMessagesProvider::class); } /** @@ -90,27 +110,59 @@ public function execute( $reCaptchaResponse = $this->captchaResponseResolver->resolve($request); } catch (InputException $e) { $this->logger->error($e); - $this->processError($response, $validationConfig->getValidationFailureMessage(), $redirectOnFailureUrl); + $this->processError($response, [], $redirectOnFailureUrl, $key); return; } $validationResult = $this->captchaValidator->isValid($reCaptchaResponse, $validationConfig); if (false === $validationResult->isValid()) { - $this->processError($response, $validationConfig->getValidationFailureMessage(), $redirectOnFailureUrl); + $this->processError($response, $validationResult->getErrors(), $redirectOnFailureUrl, $key); } } /** + * Process errors from reCAPTCHA response. + * * @param HttpResponseInterface $response - * @param string $message + * @param array $errorMessages * @param string $redirectOnFailureUrl + * @param string $sourceKey * @return void */ - private function processError(HttpResponseInterface $response, string $message, string $redirectOnFailureUrl): void + private function processError(HttpResponseInterface $response, array $errorMessages, string $redirectOnFailureUrl, string $sourceKey): void { + $validationErrorText = $this->errorMessageConfig->getValidationFailureMessage(); + $technicalErrorText = $this->errorMessageConfig->getTechnicalFailureMessage(); + + $message = $errorMessages ? $validationErrorText : $technicalErrorText; + + foreach ($errorMessages as $errorMessageCode => $errorMessageText) { + if (!$this->isValidationError($errorMessageCode)) { + $message = $technicalErrorText; + $this->logger->error( + __( + 'reCAPTCHA \'%1\' form error: %2', + $sourceKey, + $errorMessageText + ) + ); + } + } + $this->messageManager->addErrorMessage($message); $this->actionFlag->set('', Action::FLAG_NO_DISPATCH, true); $response->setRedirect($redirectOnFailureUrl); } + + /** + * Check if error code present in validation errors list. + * + * @param string $errorMessageCode + * @return bool + */ + private function isValidationError(string $errorMessageCode): bool + { + return $errorMessageCode !== $this->validationErrorMessagesProvider->getErrorMessage($errorMessageCode); + } } diff --git a/ReCaptchaUser/Observer/LoginObserver.php b/ReCaptchaUser/Observer/LoginObserver.php index 9ed191db..9c08659e 100644 --- a/ReCaptchaUser/Observer/LoginObserver.php +++ b/ReCaptchaUser/Observer/LoginObserver.php @@ -7,6 +7,7 @@ namespace Magento\ReCaptchaUser\Observer; +use Magento\Framework\App\ObjectManager; use Magento\Framework\App\RequestInterface; use Magento\Framework\Event\Observer; use Magento\Framework\Event\ObserverInterface; @@ -14,9 +15,11 @@ use Magento\Framework\Exception\LocalizedException; use Magento\Framework\Exception\Plugin\AuthenticationException; use Magento\ReCaptchaUi\Model\CaptchaResponseResolverInterface; +use Magento\ReCaptchaUi\Model\ErrorMessageConfigInterface; use Magento\ReCaptchaUi\Model\IsCaptchaEnabledInterface; use Magento\ReCaptchaUi\Model\ValidationConfigResolverInterface; use Magento\ReCaptchaValidationApi\Api\ValidatorInterface; +use Magento\ReCaptchaValidationApi\Model\ValidationErrorMessagesProvider; use Psr\Log\LoggerInterface; /** @@ -59,6 +62,16 @@ class LoginObserver implements ObserverInterface */ private $logger; + /** + * @var ErrorMessageConfigInterface|null + */ + private $errorMessageConfig; + + /** + * @var ValidationErrorMessagesProvider|null + */ + private $validationErrorMessagesProvider; + /** * @param CaptchaResponseResolverInterface $captchaResponseResolver * @param ValidationConfigResolverInterface $validationConfigResolver @@ -75,7 +88,9 @@ public function __construct( IsCaptchaEnabledInterface $isCaptchaEnabled, RequestInterface $request, LoggerInterface $logger, - string $loginActionName + string $loginActionName, + ?ErrorMessageConfigInterface $errorMessageConfig = null, + ?ValidationErrorMessagesProvider $validationErrorMessagesProvider = null ) { $this->captchaResponseResolver = $captchaResponseResolver; $this->validationConfigResolver = $validationConfigResolver; @@ -84,6 +99,10 @@ public function __construct( $this->request = $request; $this->loginActionName = $loginActionName; $this->logger = $logger; + $this->errorMessageConfig = $errorMessageConfig + ?? ObjectManager::getInstance()->get(ErrorMessageConfigInterface::class); + $this->validationErrorMessagesProvider = $validationErrorMessagesProvider + ?? ObjectManager::getInstance()->get(ValidationErrorMessagesProvider::class); } /** @@ -103,13 +122,55 @@ public function execute(Observer $observer): void $reCaptchaResponse = $this->captchaResponseResolver->resolve($this->request); } catch (InputException $e) { $this->logger->error($e); - throw new AuthenticationException(__($validationConfig->getValidationFailureMessage())); + $this->processError([], $key); } $validationResult = $this->captchaValidator->isValid($reCaptchaResponse, $validationConfig); if (false === $validationResult->isValid()) { - throw new AuthenticationException(__($validationConfig->getValidationFailureMessage())); + $this->processError($validationResult->getErrors(), $key); } } } + + /** + * Process errors from reCAPTCHA response. + * + * @param array $errorMessages + * @param string $sourceKey + * @return void + * @throws AuthenticationException + */ + private function processError(array $errorMessages, string $sourceKey): void + { + $validationErrorText = $this->errorMessageConfig->getValidationFailureMessage(); + $technicalErrorText = $this->errorMessageConfig->getTechnicalFailureMessage(); + + $message = $errorMessages ? $validationErrorText : $technicalErrorText; + + foreach ($errorMessages as $errorMessageCode => $errorMessageText) { + if (!$this->isValidationError($errorMessageCode)) { + $message = $technicalErrorText; + $this->logger->error( + __( + 'reCAPTCHA \'%1\' form error: %2', + $sourceKey, + $errorMessageText + ) + ); + } + } + + throw new AuthenticationException(__($message)); + } + + /** + * Check if error code present in validation errors list. + * + * @param string $errorMessageCode + * @return bool + */ + private function isValidationError(string $errorMessageCode): bool + { + return $errorMessageCode !== $this->validationErrorMessagesProvider->getErrorMessage($errorMessageCode); + } } diff --git a/ReCaptchaValidation/Model/Validator.php b/ReCaptchaValidation/Model/Validator.php index 4508938c..a8f544d9 100644 --- a/ReCaptchaValidation/Model/Validator.php +++ b/ReCaptchaValidation/Model/Validator.php @@ -70,9 +70,14 @@ public function isValid( $validationErrors = []; if (false === $result->isSuccess()) { foreach ($result->getErrorCodes() as $errorCode) { - $validationErrors[] = $this->errorMessagesProvider->getErrorMessage($errorCode); + $validationErrors[$errorCode] = $this->errorMessagesProvider->getErrorMessage($errorCode); + } + // 'score-threshold-not-met' error is present in response even if some technical issue happened. + if (count($validationErrors) > 1) { + unset($validationErrors[ReCaptcha::E_SCORE_THRESHOLD_NOT_MET]); } } + return $this->validationResultFactory->create(['errors' => $validationErrors]); } } diff --git a/ReCaptchaValidationApi/Api/Data/ValidationConfigInterface.php b/ReCaptchaValidationApi/Api/Data/ValidationConfigInterface.php index 35142416..824012af 100644 --- a/ReCaptchaValidationApi/Api/Data/ValidationConfigInterface.php +++ b/ReCaptchaValidationApi/Api/Data/ValidationConfigInterface.php @@ -29,8 +29,9 @@ public function getPrivateKey(): string; public function getRemoteIp(): string; /** - * Get validation failure message + * Get validation failure message TODO * + * @deprecated use TODO * @return string */ public function getValidationFailureMessage(): string; diff --git a/ReCaptchaValidationApi/Model/ErrorMessagesProvider.php b/ReCaptchaValidationApi/Model/ErrorMessagesProvider.php index 5ecadb9d..ec83f9af 100644 --- a/ReCaptchaValidationApi/Model/ErrorMessagesProvider.php +++ b/ReCaptchaValidationApi/Model/ErrorMessagesProvider.php @@ -8,9 +8,9 @@ namespace Magento\ReCaptchaValidationApi\Model; /** - * Extension point for adding reCAPTCHA validation errors + * Extension point for adding reCAPTCHA technical errors. * - * @api Class name should be used in DI for adding new validation errors + * @api Class name should be used in DI for adding new technical errors. */ class ErrorMessagesProvider { diff --git a/ReCaptchaValidationApi/Model/ValidationErrorMessagesProvider.php b/ReCaptchaValidationApi/Model/ValidationErrorMessagesProvider.php new file mode 100644 index 00000000..0b3df540 --- /dev/null +++ b/ReCaptchaValidationApi/Model/ValidationErrorMessagesProvider.php @@ -0,0 +1,40 @@ +errors = $errors; + } + + /** + * Get error label + * + * @param string $key + * @return string + */ + public function getErrorMessage(string $key): string + { + return $this->errors[$key] ?? $key; + } +} diff --git a/ReCaptchaValidationApi/etc/di.xml b/ReCaptchaValidationApi/etc/di.xml index e3ff3274..a70521e1 100644 --- a/ReCaptchaValidationApi/etc/di.xml +++ b/ReCaptchaValidationApi/etc/di.xml @@ -18,7 +18,6 @@ Expected hostname did not match. Expected APK package name did not match. Expected action did not match. - Score threshold not met. Challenge timeout. The secret parameter is missing. The secret parameter is invalid or malformed. @@ -28,4 +27,11 @@ + + + + Score threshold not met. + + + diff --git a/ReCaptchaVersion2Checkbox/etc/adminhtml/system.xml b/ReCaptchaVersion2Checkbox/etc/adminhtml/system.xml index f2c5124e..935c813d 100644 --- a/ReCaptchaVersion2Checkbox/etc/adminhtml/system.xml +++ b/ReCaptchaVersion2Checkbox/etc/adminhtml/system.xml @@ -45,11 +45,6 @@ supported Language Codes. ]]> - - - -
@@ -89,11 +84,6 @@ supported Language Codes. ]]> - - - -
diff --git a/ReCaptchaVersion2Invisible/etc/adminhtml/system.xml b/ReCaptchaVersion2Invisible/etc/adminhtml/system.xml index 66ca5d88..7a36dac7 100644 --- a/ReCaptchaVersion2Invisible/etc/adminhtml/system.xml +++ b/ReCaptchaVersion2Invisible/etc/adminhtml/system.xml @@ -45,11 +45,6 @@ supported Language Codes. ]]> - - - -
@@ -89,11 +84,6 @@ supported Language Codes. ]]> - - - -
diff --git a/ReCaptchaVersion3Invisible/etc/adminhtml/system.xml b/ReCaptchaVersion3Invisible/etc/adminhtml/system.xml index eaa206bd..b2c48c9d 100644 --- a/ReCaptchaVersion3Invisible/etc/adminhtml/system.xml +++ b/ReCaptchaVersion3Invisible/etc/adminhtml/system.xml @@ -55,11 +55,6 @@ supported Language Codes. ]]> - - - -
@@ -109,11 +104,6 @@ supported Language Codes. ]]> - - - -
From 809e8d90b85b3cf2dce9585c410b74d08768774e Mon Sep 17 00:00:00 2001 From: Pavel Bystritsky Date: Fri, 7 Aug 2020 16:20:33 +0300 Subject: [PATCH 2/6] magento/security-package#222: Add validation for error message - test fix. --- ReCaptchaAdminUi/composer.json | 3 ++- ReCaptchaAdminUi/etc/config.xml | 3 +-- ReCaptchaContact/Test/Integration/ContactFormTest.php | 2 +- ReCaptchaCustomer/Model/AjaxLogin/ErrorProcessor.php | 1 + ReCaptchaCustomer/Observer/AjaxLoginObserver.php | 5 ++--- .../Test/Integration/AjaxLoginFormTest.php | 7 ++++++- .../Test/Integration/CreateCustomerFormTest.php | 2 +- .../Test/Integration/ForgotPasswordFormTest.php | 2 +- ReCaptchaCustomer/Test/Integration/LoginFromTest.php | 2 +- ReCaptchaFrontendUi/etc/config.xml | 3 +-- .../Test/Integration/NewsletterFormTest.php | 2 +- ReCaptchaReview/Test/Integration/ReviewFormTest.php | 2 +- .../Test/Integration/SendFriendFormTest.php | 2 +- ReCaptchaUi/Model/RequestHandler.php | 10 ++++++++-- ReCaptchaUser/Observer/LoginObserver.php | 9 ++++++--- .../Test/Integration/ForgotPasswordFormTest.php | 2 +- ReCaptchaUser/Test/Integration/LoginFormTest.php | 2 +- 17 files changed, 36 insertions(+), 23 deletions(-) diff --git a/ReCaptchaAdminUi/composer.json b/ReCaptchaAdminUi/composer.json index 607ae8a9..a7286247 100644 --- a/ReCaptchaAdminUi/composer.json +++ b/ReCaptchaAdminUi/composer.json @@ -5,7 +5,8 @@ "php": "~7.3.0||~7.4.0", "magento/framework": "*", "magento/module-config": "*", - "magento/module-re-captcha-ui": "*" + "magento/module-re-captcha-ui": "*", + "magento/module-store": "*" }, "type": "magento2-module", "license": "OSL-3.0", diff --git a/ReCaptchaAdminUi/etc/config.xml b/ReCaptchaAdminUi/etc/config.xml index a66fbc46..8f371674 100644 --- a/ReCaptchaAdminUi/etc/config.xml +++ b/ReCaptchaAdminUi/etc/config.xml @@ -11,8 +11,7 @@ reCAPTCHA verification failed. - Something went wrong with reCAPTCHA, please contact Store owner. - + Something went wrong with reCAPTCHA. Please contact the store owner. diff --git a/ReCaptchaContact/Test/Integration/ContactFormTest.php b/ReCaptchaContact/Test/Integration/ContactFormTest.php index 9e963fc8..18f3470a 100644 --- a/ReCaptchaContact/Test/Integration/ContactFormTest.php +++ b/ReCaptchaContact/Test/Integration/ContactFormTest.php @@ -210,7 +210,7 @@ private function checkFailedPostResponse(array $postValues = []): void $this->makePostRequest($postValues); $this->assertSessionMessages( - $this->equalTo(['reCAPTCHA verification failed']), + $this->equalTo(['Something went wrong with reCAPTCHA. Please contact the store owner.']), MessageInterface::TYPE_ERROR ); } diff --git a/ReCaptchaCustomer/Model/AjaxLogin/ErrorProcessor.php b/ReCaptchaCustomer/Model/AjaxLogin/ErrorProcessor.php index 64bdd682..aabc9014 100644 --- a/ReCaptchaCustomer/Model/AjaxLogin/ErrorProcessor.php +++ b/ReCaptchaCustomer/Model/AjaxLogin/ErrorProcessor.php @@ -51,6 +51,7 @@ class ErrorProcessor /** * @param ActionFlag $actionFlag * @param SerializerInterface $serializer + * @param LoggerInterface|null $logger * @param ErrorMessageConfigInterface|null $errorMessageConfig * @param ValidationErrorMessagesProvider|null $validationErrorMessagesProvider */ diff --git a/ReCaptchaCustomer/Observer/AjaxLoginObserver.php b/ReCaptchaCustomer/Observer/AjaxLoginObserver.php index b682c0be..0fe159bd 100644 --- a/ReCaptchaCustomer/Observer/AjaxLoginObserver.php +++ b/ReCaptchaCustomer/Observer/AjaxLoginObserver.php @@ -20,7 +20,7 @@ use Psr\Log\LoggerInterface; /** - * AjaxLoginObserver + * Observer of ajax login. */ class AjaxLoginObserver implements ObserverInterface { @@ -79,8 +79,7 @@ public function __construct( } /** - * @param Observer $observer - * @return void + * @inheritdoc * @throws LocalizedException */ public function execute(Observer $observer): void diff --git a/ReCaptchaCustomer/Test/Integration/AjaxLoginFormTest.php b/ReCaptchaCustomer/Test/Integration/AjaxLoginFormTest.php index 7a65d351..bc60aa3b 100644 --- a/ReCaptchaCustomer/Test/Integration/AjaxLoginFormTest.php +++ b/ReCaptchaCustomer/Test/Integration/AjaxLoginFormTest.php @@ -218,7 +218,12 @@ private function checkFailedPostResponse(array $postValues = []): void { $this->makePostRequest($postValues); - $expected = json_encode(['errors' => true, 'message' => 'reCAPTCHA verification failed']); + $expected = json_encode( + [ + 'errors' => true, + 'message' => 'Something went wrong with reCAPTCHA. Please contact the store owner.' + ] + ); $this->assertEquals( $expected, diff --git a/ReCaptchaCustomer/Test/Integration/CreateCustomerFormTest.php b/ReCaptchaCustomer/Test/Integration/CreateCustomerFormTest.php index d453cd16..d4440267 100644 --- a/ReCaptchaCustomer/Test/Integration/CreateCustomerFormTest.php +++ b/ReCaptchaCustomer/Test/Integration/CreateCustomerFormTest.php @@ -238,7 +238,7 @@ private function checkFailedPostResponse(array $postValues = []): void // Customer should not be created } $this->assertSessionMessages( - self::equalTo(['reCAPTCHA verification failed']), + self::equalTo(['Something went wrong with reCAPTCHA. Please contact the store owner.']), MessageInterface::TYPE_ERROR ); } diff --git a/ReCaptchaCustomer/Test/Integration/ForgotPasswordFormTest.php b/ReCaptchaCustomer/Test/Integration/ForgotPasswordFormTest.php index 59265f51..3504ce38 100644 --- a/ReCaptchaCustomer/Test/Integration/ForgotPasswordFormTest.php +++ b/ReCaptchaCustomer/Test/Integration/ForgotPasswordFormTest.php @@ -227,7 +227,7 @@ private function checkFailedPostResponse(array $postValues = []): void $this->assertRedirect(self::equalTo($this->url->getRouteUrl('customer/account/forgotpassword'))); $this->assertSessionMessages( - self::equalTo(['reCAPTCHA verification failed']), + self::equalTo(['Something went wrong with reCAPTCHA. Please contact the store owner.']), MessageInterface::TYPE_ERROR ); diff --git a/ReCaptchaCustomer/Test/Integration/LoginFromTest.php b/ReCaptchaCustomer/Test/Integration/LoginFromTest.php index e5a61344..ee0df5ae 100644 --- a/ReCaptchaCustomer/Test/Integration/LoginFromTest.php +++ b/ReCaptchaCustomer/Test/Integration/LoginFromTest.php @@ -227,7 +227,7 @@ private function checkFailedPostResponse(array $postValues = []): void $this->assertRedirect(self::stringStartsWith($this->url->getRouteUrl('customer/account/login'))); $this->assertSessionMessages( - self::equalTo(['reCAPTCHA verification failed']), + self::equalTo(['Something went wrong with reCAPTCHA. Please contact the store owner.']), MessageInterface::TYPE_ERROR ); diff --git a/ReCaptchaFrontendUi/etc/config.xml b/ReCaptchaFrontendUi/etc/config.xml index 3f32365f..81154a13 100644 --- a/ReCaptchaFrontendUi/etc/config.xml +++ b/ReCaptchaFrontendUi/etc/config.xml @@ -11,8 +11,7 @@ reCAPTCHA verification failed. - Something went wrong with reCAPTCHA, please contact Store owner. - + Something went wrong with reCAPTCHA. Please contact the store owner. diff --git a/ReCaptchaNewsletter/Test/Integration/NewsletterFormTest.php b/ReCaptchaNewsletter/Test/Integration/NewsletterFormTest.php index ad496c47..fb27ee92 100644 --- a/ReCaptchaNewsletter/Test/Integration/NewsletterFormTest.php +++ b/ReCaptchaNewsletter/Test/Integration/NewsletterFormTest.php @@ -227,7 +227,7 @@ private function checkFailedPostResponse(array $postValues = []): void $this->makePostRequest($postValues); $this->assertSessionMessages( - self::equalTo(['reCAPTCHA verification failed']), + self::equalTo(['Something went wrong with reCAPTCHA. Please contact the store owner.']), MessageInterface::TYPE_ERROR ); self::assertEmpty( diff --git a/ReCaptchaReview/Test/Integration/ReviewFormTest.php b/ReCaptchaReview/Test/Integration/ReviewFormTest.php index a7cbe8e3..1047fc50 100644 --- a/ReCaptchaReview/Test/Integration/ReviewFormTest.php +++ b/ReCaptchaReview/Test/Integration/ReviewFormTest.php @@ -221,7 +221,7 @@ private function checkFailedPostResponse(array $postValues = []): void $this->makePostRequest($postValues); $this->assertSessionMessages( - self::equalTo(['reCAPTCHA verification failed']), + self::equalTo(['Something went wrong with reCAPTCHA. Please contact the store owner.']), MessageInterface::TYPE_ERROR ); self::assertEquals(0, $this->reviewResourceModel->getTotalReviews(1)); diff --git a/ReCaptchaSendFriend/Test/Integration/SendFriendFormTest.php b/ReCaptchaSendFriend/Test/Integration/SendFriendFormTest.php index e87f9faf..12e1d1d2 100644 --- a/ReCaptchaSendFriend/Test/Integration/SendFriendFormTest.php +++ b/ReCaptchaSendFriend/Test/Integration/SendFriendFormTest.php @@ -247,7 +247,7 @@ private function checkFailedPostResponse(array $postValues = []): void $this->makePostRequest($postValues); $this->assertSessionMessages( - self::equalTo(['reCAPTCHA verification failed']), + self::equalTo(['Something went wrong with reCAPTCHA. Please contact the store owner.']), MessageInterface::TYPE_ERROR ); self::assertEmpty($this->transportMock->getSentMessage()); diff --git a/ReCaptchaUi/Model/RequestHandler.php b/ReCaptchaUi/Model/RequestHandler.php index b774442f..52c4b8ab 100644 --- a/ReCaptchaUi/Model/RequestHandler.php +++ b/ReCaptchaUi/Model/RequestHandler.php @@ -20,6 +20,8 @@ /** * @inheritdoc + * + * @SuppressWarnings(PHPMD.CouplingBetweenObjects) */ class RequestHandler implements RequestHandlerInterface { @@ -129,8 +131,12 @@ public function execute( * @param string $sourceKey * @return void */ - private function processError(HttpResponseInterface $response, array $errorMessages, string $redirectOnFailureUrl, string $sourceKey): void - { + private function processError( + HttpResponseInterface $response, + array $errorMessages, + string $redirectOnFailureUrl, + string $sourceKey + ): void { $validationErrorText = $this->errorMessageConfig->getValidationFailureMessage(); $technicalErrorText = $this->errorMessageConfig->getTechnicalFailureMessage(); diff --git a/ReCaptchaUser/Observer/LoginObserver.php b/ReCaptchaUser/Observer/LoginObserver.php index 9c08659e..37277b8a 100644 --- a/ReCaptchaUser/Observer/LoginObserver.php +++ b/ReCaptchaUser/Observer/LoginObserver.php @@ -23,7 +23,9 @@ use Psr\Log\LoggerInterface; /** - * LoginObserver + * Observer of login. + * + * @SuppressWarnings(PHPMD.CouplingBetweenObjects) */ class LoginObserver implements ObserverInterface { @@ -80,6 +82,8 @@ class LoginObserver implements ObserverInterface * @param RequestInterface $request * @param LoggerInterface $logger * @param string $loginActionName + * @param ErrorMessageConfigInterface|null $errorMessageConfig + * @param ValidationErrorMessagesProvider|null $validationErrorMessagesProvider */ public function __construct( CaptchaResponseResolverInterface $captchaResponseResolver, @@ -106,8 +110,7 @@ public function __construct( } /** - * @param Observer $observer - * @return void + * @inheritdoc * @throws AuthenticationException * @throws LocalizedException */ diff --git a/ReCaptchaUser/Test/Integration/ForgotPasswordFormTest.php b/ReCaptchaUser/Test/Integration/ForgotPasswordFormTest.php index 3ae29679..465cfbd8 100644 --- a/ReCaptchaUser/Test/Integration/ForgotPasswordFormTest.php +++ b/ReCaptchaUser/Test/Integration/ForgotPasswordFormTest.php @@ -203,7 +203,7 @@ private function checkFailedPostResponse(array $postValues = []): void $this->makePostRequest($postValues); $this->assertSessionMessages( - self::equalTo(['reCAPTCHA verification failed']), + self::equalTo(['Something went wrong with reCAPTCHA. Please contact the store owner.']), MessageInterface::TYPE_ERROR ); self::assertEmpty($this->transportMock->getSentMessage()); diff --git a/ReCaptchaUser/Test/Integration/LoginFormTest.php b/ReCaptchaUser/Test/Integration/LoginFormTest.php index 8595c861..ea8e7524 100644 --- a/ReCaptchaUser/Test/Integration/LoginFormTest.php +++ b/ReCaptchaUser/Test/Integration/LoginFormTest.php @@ -214,7 +214,7 @@ private function checkFailedPostResponse(array $postValues = []): void // Location header is different than in the successful case $this->assertRedirect(self::equalTo($this->backendUrl->getUrl('admin'))); $this->assertSessionMessages( - self::equalTo(['reCAPTCHA verification failed']), + self::equalTo(['Something went wrong with reCAPTCHA. Please contact the store owner.']), MessageInterface::TYPE_ERROR ); self::assertFalse($this->auth->isLoggedIn()); From c9e6df375bdd72086995ddcd9121330b4888aaf5 Mon Sep 17 00:00:00 2001 From: Pavel Bystritsky Date: Fri, 7 Aug 2020 18:39:05 +0300 Subject: [PATCH 3/6] magento/security-package#222: Add validation for error message - test fix. --- ReCaptchaCustomer/Observer/AjaxLoginObserver.php | 1 + ReCaptchaCustomer/Test/Integration/EditFromTest.php | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/ReCaptchaCustomer/Observer/AjaxLoginObserver.php b/ReCaptchaCustomer/Observer/AjaxLoginObserver.php index 0fe159bd..10b54cce 100644 --- a/ReCaptchaCustomer/Observer/AjaxLoginObserver.php +++ b/ReCaptchaCustomer/Observer/AjaxLoginObserver.php @@ -80,6 +80,7 @@ public function __construct( /** * @inheritdoc + * * @throws LocalizedException */ public function execute(Observer $observer): void diff --git a/ReCaptchaCustomer/Test/Integration/EditFromTest.php b/ReCaptchaCustomer/Test/Integration/EditFromTest.php index b62e0983..885c7e66 100644 --- a/ReCaptchaCustomer/Test/Integration/EditFromTest.php +++ b/ReCaptchaCustomer/Test/Integration/EditFromTest.php @@ -236,7 +236,7 @@ private function checkFailedPostResponse(array $postValues = []): void $this->assertRedirect(self::stringStartsWith($this->url->getRouteUrl('customer/account/edit'))); $this->assertSessionMessages( - self::equalTo(['reCAPTCHA verification failed']), + self::equalTo(['Something went wrong with reCAPTCHA. Please contact the store owner.']), MessageInterface::TYPE_ERROR ); From e6ea7c650c7f2eaa9a09393df0fb82a422f49b44 Mon Sep 17 00:00:00 2001 From: Pavel Bystritsky Date: Mon, 10 Aug 2020 18:14:09 +0300 Subject: [PATCH 4/6] magento/security-package#222: Add validation for error message - admin config scope fix. --- ReCaptchaAdminUi/etc/adminhtml/system.xml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ReCaptchaAdminUi/etc/adminhtml/system.xml b/ReCaptchaAdminUi/etc/adminhtml/system.xml index ff5a7838..01e6bb50 100644 --- a/ReCaptchaAdminUi/etc/adminhtml/system.xml +++ b/ReCaptchaAdminUi/etc/adminhtml/system.xml @@ -33,12 +33,12 @@ + showInWebsite="0" showInStore="0" canRestore="1"> + showInWebsite="0" showInStore="0" canRestore="1"> From 5451642f1d39afa1827d930d7c4b2daa48230b7d Mon Sep 17 00:00:00 2001 From: Pavel Bystritsky Date: Mon, 10 Aug 2020 18:16:30 +0300 Subject: [PATCH 5/6] magento/security-package#222: Add validation for error message - PayPal fix. --- ReCaptchaPaypal/Observer/PayPalObserver.php | 71 +++++++++++++++++++-- 1 file changed, 66 insertions(+), 5 deletions(-) diff --git a/ReCaptchaPaypal/Observer/PayPalObserver.php b/ReCaptchaPaypal/Observer/PayPalObserver.php index d299050f..75c4a4c9 100644 --- a/ReCaptchaPaypal/Observer/PayPalObserver.php +++ b/ReCaptchaPaypal/Observer/PayPalObserver.php @@ -9,6 +9,7 @@ use Magento\Framework\App\Action\Action; use Magento\Framework\App\ActionFlag; +use Magento\Framework\App\ObjectManager; use Magento\Framework\App\ResponseInterface; use Magento\Framework\Event\Observer; use Magento\Framework\Event\ObserverInterface; @@ -16,9 +17,11 @@ use Magento\Framework\Exception\LocalizedException; use Magento\Framework\Serialize\SerializerInterface; use Magento\ReCaptchaUi\Model\CaptchaResponseResolverInterface; +use Magento\ReCaptchaUi\Model\ErrorMessageConfigInterface; use Magento\ReCaptchaUi\Model\IsCaptchaEnabledInterface; use Magento\ReCaptchaUi\Model\ValidationConfigResolverInterface; use Magento\ReCaptchaValidationApi\Api\ValidatorInterface; +use Magento\ReCaptchaValidationApi\Model\ValidationErrorMessagesProvider; use Psr\Log\LoggerInterface; /** @@ -61,6 +64,16 @@ class PayPalObserver implements ObserverInterface */ private $logger; + /** + * @var ErrorMessageConfigInterface + */ + private $errorMessageConfig; + + /** + * @var ValidationErrorMessagesProvider + */ + private $validationErrorMessagesProvider; + /** * @param CaptchaResponseResolverInterface $captchaResponseResolver * @param ValidationConfigResolverInterface $validationConfigResolver @@ -69,6 +82,8 @@ class PayPalObserver implements ObserverInterface * @param SerializerInterface $serializer * @param IsCaptchaEnabledInterface $isCaptchaEnabled * @param LoggerInterface $logger + * @param ErrorMessageConfigInterface|null $errorMessageConfig + * @param ValidationErrorMessagesProvider|null $validationErrorMessagesProvider */ public function __construct( CaptchaResponseResolverInterface $captchaResponseResolver, @@ -77,7 +92,9 @@ public function __construct( ActionFlag $actionFlag, SerializerInterface $serializer, IsCaptchaEnabledInterface $isCaptchaEnabled, - LoggerInterface $logger + LoggerInterface $logger, + ?ErrorMessageConfigInterface $errorMessageConfig = null, + ?ValidationErrorMessagesProvider $validationErrorMessagesProvider = null ) { $this->captchaResponseResolver = $captchaResponseResolver; $this->validationConfigResolver = $validationConfigResolver; @@ -86,6 +103,10 @@ public function __construct( $this->serializer = $serializer; $this->isCaptchaEnabled = $isCaptchaEnabled; $this->logger = $logger; + $this->errorMessageConfig = $errorMessageConfig + ?? ObjectManager::getInstance()->get(ErrorMessageConfigInterface::class); + $this->validationErrorMessagesProvider = $validationErrorMessagesProvider + ?? ObjectManager::getInstance()->get(ValidationErrorMessagesProvider::class); } /** @@ -110,24 +131,53 @@ public function execute(Observer $observer): void $reCaptchaResponse = $this->captchaResponseResolver->resolve($request); } catch (InputException $e) { $this->logger->error($e); - $this->processError($response, $validationConfig->getValidationFailureMessage()); + $this->processError( + $response, + [], + $key + ); return; } $validationResult = $this->captchaValidator->isValid($reCaptchaResponse, $validationConfig); if (false === $validationResult->isValid()) { - $this->processError($response, $validationConfig->getValidationFailureMessage()); + $this->processError( + $response, + $validationResult->getErrors(), + $key + ); } } } /** + * Process errors from reCAPTCHA response. + * * @param ResponseInterface $response - * @param string $message + * @param array $errorMessages + * @param string $sourceKey * @return void */ - private function processError(ResponseInterface $response, string $message): void + private function processError(ResponseInterface $response, array $errorMessages, string $sourceKey): void { + $validationErrorText = $this->errorMessageConfig->getValidationFailureMessage(); + $technicalErrorText = $this->errorMessageConfig->getTechnicalFailureMessage(); + + $message = $errorMessages ? $validationErrorText : $technicalErrorText; + + foreach ($errorMessages as $errorMessageCode => $errorMessageText) { + if (!$this->isValidationError($errorMessageCode)) { + $message = $technicalErrorText; + $this->logger->error( + __( + 'reCAPTCHA \'%1\' form error: %2', + $sourceKey, + $errorMessageText + ) + ); + } + } + $this->actionFlag->set('', Action::FLAG_NO_DISPATCH, true); $jsonPayload = $this->serializer->serialize([ @@ -137,4 +187,15 @@ private function processError(ResponseInterface $response, string $message): voi ]); $response->representJson($jsonPayload); } + + /** + * Check if error code present in validation errors list. + * + * @param string $errorMessageCode + * @return bool + */ + private function isValidationError(string $errorMessageCode): bool + { + return $errorMessageCode !== $this->validationErrorMessagesProvider->getErrorMessage($errorMessageCode); + } } From 6f1b3131a13ef9f9b6ad069d7f93bf38f9eb64c9 Mon Sep 17 00:00:00 2001 From: Pavel Bystritsky Date: Fri, 14 Aug 2020 14:48:43 +0300 Subject: [PATCH 6/6] magento/security-package#222: Static tests fix. --- ReCaptchaPaypal/Observer/PayPalObserver.php | 4 +++- ReCaptchaUser/Observer/LoginObserver.php | 2 ++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/ReCaptchaPaypal/Observer/PayPalObserver.php b/ReCaptchaPaypal/Observer/PayPalObserver.php index 75c4a4c9..ec047819 100644 --- a/ReCaptchaPaypal/Observer/PayPalObserver.php +++ b/ReCaptchaPaypal/Observer/PayPalObserver.php @@ -25,7 +25,9 @@ use Psr\Log\LoggerInterface; /** - * AjaxLoginObserver + * reCaptcha support for PayPal Payflow Pro Integration. + * + * @SuppressWarnings(PHPMD.CouplingBetweenObjects) */ class PayPalObserver implements ObserverInterface { diff --git a/ReCaptchaUser/Observer/LoginObserver.php b/ReCaptchaUser/Observer/LoginObserver.php index 37277b8a..7c0dbfed 100644 --- a/ReCaptchaUser/Observer/LoginObserver.php +++ b/ReCaptchaUser/Observer/LoginObserver.php @@ -126,6 +126,8 @@ public function execute(Observer $observer): void } catch (InputException $e) { $this->logger->error($e); $this->processError([], $key); + + return; } $validationResult = $this->captchaValidator->isValid($reCaptchaResponse, $validationConfig);