diff --git a/CHANGELOG.md b/CHANGELOG.md index 035502d..927a9c3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,11 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ## [Unreleased] +### Fixed +- `ReCaptchaTrait::validateReCaptchaFromRequest()` treats a missing or empty + `g-recaptcha-response` as an invalid captcha (returns `false`) instead of passing `null` to the + non-nullable `validateReCaptcha(string ...)`, which raised a `TypeError` (HTTP 500). A tokenless + request — e.g. a programmatic JSON client — now fails the reCaptcha check cleanly. ## [10.1.2] - 2025-09-26 - Github social login diff --git a/src/Traits/ReCaptchaTrait.php b/src/Traits/ReCaptchaTrait.php index be4bba8..0cc2b20 100644 --- a/src/Traits/ReCaptchaTrait.php +++ b/src/Traits/ReCaptchaTrait.php @@ -36,7 +36,13 @@ public function validateReCaptchaFromRequest(ServerRequestInterface $request): b throw new BadMethodCallException('Request must be an instance of ServerRequest'); } $data = $request->getParsedBody(); - $captcha = $data['g-recaptcha-response'] ?? null; + $captcha = is_array($data) ? ($data['g-recaptcha-response'] ?? null) : null; + // A missing/empty token is simply an invalid captcha. Guard here so a request + // without the field (e.g. a programmatic JSON client) fails validation cleanly + // instead of hitting validateReCaptcha()'s non-nullable string param (TypeError -> 500). + if (!is_string($captcha) || $captcha === '') { + return false; + } return $this->validateReCaptcha( $captcha, diff --git a/tests/TestCase/Authenticator/FormAuthenticatorTest.php b/tests/TestCase/Authenticator/FormAuthenticatorTest.php index 9e30092..5843c54 100644 --- a/tests/TestCase/Authenticator/FormAuthenticatorTest.php +++ b/tests/TestCase/Authenticator/FormAuthenticatorTest.php @@ -299,6 +299,66 @@ public function testAuthenticateInvalidRecaptcha() $this->assertNull($result->getData()); } + /** + * A request without a g-recaptcha-response token, with reCaptcha enabled and valid + * credentials, must fail as FAILURE_INVALID_RECAPTCHA through the real + * validateReCaptchaFromRequest() guard (no token never reaches validateReCaptcha(), + * so this asserts the guard end-to-end and that it no longer raises a TypeError). + * + * @return void + */ + public function testAuthenticateTokenlessRecaptchaFailsCleanly() + { + $identifiers = new IdentifierCollection([ + 'Authentication.Password', + ]); + + $BaseAuthenticator = $this->getMockBuilder(CakeFormAuthenticator::class) + ->setConstructorArgs([$identifiers]) + ->onlyMethods(['authenticate']) + ->getMock(); + $request = ServerRequestFactory::fromGlobals( + ['REQUEST_URI' => '/testpath', 'REMOTE_ADDR' => '127.0.0.1'], + [], + ['username' => 'marcelo', 'password' => 'password'] + ); + + $baseResult = new Result( + [ + 'id' => '42', + 'username' => 'marcelo', + 'role' => 'user', + ], + Result::SUCCESS + ); + $BaseAuthenticator->expects($this->once()) + ->method('authenticate') + ->with($request) + ->will($this->returnValue($baseResult)); + + // Only the base authenticator is mocked; validateReCaptcha() is left real and + // must never be reached because the token is absent. + $Authenticator = $this->getMockBuilder(FormAuthenticator::class)->setConstructorArgs([ + $identifiers, + [ + 'fields' => [ + AbstractIdentifier::CREDENTIAL_USERNAME => 'email', + AbstractIdentifier::CREDENTIAL_PASSWORD => 'password', + ], + ], + ])->onlyMethods(['createBaseAuthenticator'])->getMock(); + + Configure::write('Users.reCaptcha.login', true); + $Authenticator->expects($this->once()) + ->method('createBaseAuthenticator') + ->will($this->returnValue($BaseAuthenticator)); + + $result = $Authenticator->authenticate($request); + $this->assertInstanceOf(Result::class, $result); + $this->assertEquals(FormAuthenticator::FAILURE_INVALID_RECAPTCHA, $result->getStatus()); + $this->assertNull($result->getData()); + } + /** * test getBaseAuthenticator * diff --git a/tests/TestCase/Traits/ReCaptchaTraitTest.php b/tests/TestCase/Traits/ReCaptchaTraitTest.php new file mode 100644 index 0000000..b214e92 --- /dev/null +++ b/tests/TestCase/Traits/ReCaptchaTraitTest.php @@ -0,0 +1,111 @@ + '127.0.0.1']); + + return $request->withParsedBody($parsedBody); + } + + /** + * A subject that uses the trait and stubs the actual verification so the tests + * never reach _getReCaptchaInstance()/the google/recaptcha dependency and can + * observe whether the guard short-circuited. + * + * @param bool $return value the stubbed validateReCaptcha() returns + * @return object + */ + protected function subject(bool $return = true): object + { + return new class ($return) { + use ReCaptchaTrait; + + public bool $validatorCalled = false; + public array $validatorArgs = []; + + public function __construct(protected bool $return) + { + } + + public function validateReCaptcha(string $recaptchaResponse, string $clientIp): bool + { + $this->validatorCalled = true; + $this->validatorArgs = [$recaptchaResponse, $clientIp]; + + return $this->return; + } + }; + } + + /** + * @return array + */ + public static function tokenlessProvider(): array + { + return [ + 'missing field' => [[]], + 'empty string' => [['g-recaptcha-response' => '']], + 'array value' => [['g-recaptcha-response' => ['nested']]], + 'object body' => [(object)['g-recaptcha-response' => 'x']], + ]; + } + + /** + * A missing/empty/non-string token is invalid and must not reach the verifier. + * + * @dataProvider tokenlessProvider + * @param mixed $parsedBody parsed request body + * @return void + */ + public function testTokenlessRequestFailsWithoutCallingValidator(mixed $parsedBody): void + { + $subject = $this->subject(true); + + $result = $subject->validateReCaptchaFromRequest($this->request($parsedBody)); + + $this->assertFalse($result); + $this->assertFalse( + $subject->validatorCalled, + 'The guard must short-circuit before validateReCaptcha().', + ); + } + + /** + * A real token is forwarded verbatim to validateReCaptcha() with the client IP. + * + * @return void + */ + public function testValidTokenDelegatesToValidator(): void + { + $subject = $this->subject(true); + + $result = $subject->validateReCaptchaFromRequest( + $this->request(['g-recaptcha-response' => 'the-token']), + ); + + $this->assertTrue($result); + $this->assertTrue($subject->validatorCalled); + $this->assertSame(['the-token', '127.0.0.1'], $subject->validatorArgs); + } +}