diff --git a/packages/auth/src/OAuth/GenericOAuthClient.php b/packages/auth/src/OAuth/GenericOAuthClient.php index 9b591f26ee..2f1bd8db42 100644 --- a/packages/auth/src/OAuth/GenericOAuthClient.php +++ b/packages/auth/src/OAuth/GenericOAuthClient.php @@ -120,7 +120,7 @@ public function authenticate(Request $request, Closure $map): Authenticatable $this->session->remove($this->sessionKey); - if ($expectedState !== $actualState) { + if ($expectedState === null || $expectedState === '' || $expectedState !== $actualState) { throw new OAuthStateWasInvalid(); } diff --git a/packages/auth/src/OAuth/Testing/TestingOAuthClient.php b/packages/auth/src/OAuth/Testing/TestingOAuthClient.php index 470cf0ccd7..f38a654056 100644 --- a/packages/auth/src/OAuth/Testing/TestingOAuthClient.php +++ b/packages/auth/src/OAuth/Testing/TestingOAuthClient.php @@ -130,7 +130,7 @@ public function authenticate(Request $request, Closure $map): Authenticatable $this->state = null; - if ($expectedState !== $actualState) { + if ($expectedState === null || $expectedState === '' || $expectedState !== $actualState) { throw new OAuthStateWasInvalid(); } diff --git a/tests/Integration/Auth/OAuth/GenericOAuthClientTest.php b/tests/Integration/Auth/OAuth/GenericOAuthClientTest.php index c1ad063c9e..f5fc9cf162 100644 --- a/tests/Integration/Auth/OAuth/GenericOAuthClientTest.php +++ b/tests/Integration/Auth/OAuth/GenericOAuthClientTest.php @@ -2,12 +2,22 @@ namespace Tests\Tempest\Integration\Auth\OAuth; +use League\OAuth2\Client\Provider\AbstractProvider; +use League\OAuth2\Client\Provider\ResourceOwnerInterface; +use League\OAuth2\Client\Token\AccessToken; +use LogicException; use PHPUnit\Framework\Attributes\Test; +use Psr\Http\Message\ResponseInterface; +use ReflectionClass; +use Tempest\Auth\Exceptions\OAuthStateWasInvalid; use Tempest\Auth\Exceptions\OAuthWasNotConfigured; use Tempest\Auth\OAuth\Config\GitHubOAuthConfig; use Tempest\Auth\OAuth\GenericOAuthClient; use Tempest\Auth\OAuth\OAuthClient; +use Tempest\Http\GenericRequest; +use Tempest\Http\Method; use Tempest\Http\Session\Session; +use Tempest\Support\Uri; use Tests\Tempest\Integration\FrameworkIntegrationTestCase; final class GenericOAuthClientTest extends FrameworkIntegrationTestCase @@ -52,4 +62,63 @@ public function state_is_set_when_redirect_is_created(): void $this->assertNotNull($session->get($oauth->sessionKey)); } + + #[Test] + public function missing_session_state_is_rejected(): void + { + $this->container->config(new GitHubOAuthConfig( + clientId: 'client-id', + clientSecret: 'client-secret', // @mago-expect lint:no-literal-password + redirectTo: '/oauth/callback', + scopes: ['user:email'], + )); + + /** @var GenericOAuthClient $oauth */ + $oauth = $this->container->get(OAuthClient::class); + + $reflection = new ReflectionClass($oauth); + $reflection->getProperty('provider')->setValue($oauth, new class extends AbstractProvider { + public function getBaseAuthorizationUrl(): string + { + return 'https://provider.test/authorize'; + } + + public function getBaseAccessTokenUrl(array $params): string + { + return 'https://provider.test/token'; + } + + public function getResourceOwnerDetailsUrl(AccessToken $token): string + { + return 'https://provider.test/user'; + } + + public function getAccessToken($grant, array $options = []) + { + throw new LogicException('Access token should not be requested when state is missing.'); + } + + protected function getDefaultScopes(): array + { + return []; + } + + protected function checkResponse(ResponseInterface $response, $data): void {} + + protected function createResourceOwner(array $response, AccessToken $token): ResourceOwnerInterface + { + throw new LogicException('Resource owner should not be created when state is missing.'); + } + }); + + $this->expectException(OAuthStateWasInvalid::class); + + $oauth->authenticate( + request: new GenericRequest( + method: Method::GET, + uri: Uri\set_query('/oauth/callback', code: 'auth-code'), + ), + map: static fn () => throw new LogicException('User should not be mapped when state is missing.'), + ); + } } diff --git a/tests/Integration/Auth/OAuth/TestingOAuthClientTest.php b/tests/Integration/Auth/OAuth/TestingOAuthClientTest.php index 87279d839b..c11867d825 100644 --- a/tests/Integration/Auth/OAuth/TestingOAuthClientTest.php +++ b/tests/Integration/Auth/OAuth/TestingOAuthClientTest.php @@ -327,6 +327,34 @@ public function state_is_cleared_even_when_validation_fails(): void $this->assertNull($client->getState()); } + + #[Test] + public function missing_state_is_rejected(): void + { + $this->database->reset(migrate: false); + $this->database->migrate(CreateMigrationsTable::class, CreateUsersTable::class); + + $this->container->config(new GitHubOAuthConfig( + clientId: 'foo', + clientSecret: 'bar', // @mago-expect lint:no-literal-password + redirectTo: '/oauth/github', + )); + + $client = $this->oauth->fake($this->user); + + $this->expectException(OAuthStateWasInvalid::class); + + $client->authenticate( + request: new GenericRequest( + method: Method::GET, + uri: Uri\set_query('/oauth/callback', code: 'auth-code'), + ), + map: static fn (OAuthUser $user): User => query(User::class)->updateOrCreate( + ['github_id' => $user->id], + ['email' => $user->email ?? '', 'full_name' => $user->name ?? '', 'username' => $user->nickname ?? ''], + ), + ); + } } final class User implements Authenticatable