Skip to content

Commit c5db557

Browse files
authored
refactor(auth): clear oauth state after validation (#1754)
1 parent 65a1c01 commit c5db557

File tree

3 files changed

+80
-1
lines changed

3 files changed

+80
-1
lines changed

packages/auth/src/OAuth/GenericOAuthClient.php

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -100,7 +100,12 @@ public function fetchUser(AccessToken $token): OAuthUser
100100

101101
public function authenticate(Request $request, Closure $map): Authenticatable
102102
{
103-
if ($this->session->get($this->sessionKey) !== $request->get('state')) {
103+
$expectedState = $this->session->get($this->sessionKey);
104+
$actualState = $request->get('state');
105+
106+
$this->session->remove($this->sessionKey);
107+
108+
if ($expectedState !== $actualState) {
104109
throw new OAuthStateWasInvalid();
105110
}
106111

packages/auth/src/OAuth/Testing/TestingOAuthClient.php

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
use PHPUnit\Framework\Assert;
1010
use Tempest\Auth\Authentication\Authenticatable;
1111
use Tempest\Auth\Authentication\Authenticator;
12+
use Tempest\Auth\Exceptions\OAuthStateWasInvalid;
1213
use Tempest\Auth\OAuth\OAuthClient;
1314
use Tempest\Auth\OAuth\OAuthConfig;
1415
use Tempest\Auth\OAuth\OAuthUser;
@@ -124,6 +125,15 @@ public function createRedirect(array $scopes = [], array $options = []): Redirec
124125

125126
public function authenticate(Request $request, Closure $map): Authenticatable
126127
{
128+
$expectedState = $this->state;
129+
$actualState = $request->get('state');
130+
131+
$this->state = null;
132+
133+
if ($expectedState !== $actualState) {
134+
throw new OAuthStateWasInvalid();
135+
}
136+
127137
$user = $this->fetchUser($this->requestAccessToken($request->get('code')));
128138

129139
$authenticatable = $map($user);

tests/Integration/Auth/OAuth/TestingOAuthClientTest.php

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
use PHPUnit\Framework\Attributes\Test;
66
use PHPUnit\Framework\Attributes\TestWith;
77
use Tempest\Auth\Authentication\Authenticatable;
8+
use Tempest\Auth\Exceptions\OAuthStateWasInvalid;
89
use Tempest\Auth\OAuth\Config\GitHubOAuthConfig;
910
use Tempest\Auth\OAuth\OAuthClient;
1011
use Tempest\Auth\OAuth\OAuthUser;
@@ -263,6 +264,69 @@ public function abstracted_flow(): void
263264
$this->assertEquals('Jane Developer', $user->full_name);
264265
$this->assertEquals('janedev', $user->username);
265266
}
267+
268+
#[Test]
269+
public function state_is_cleared_after_authentication(): void
270+
{
271+
$this->database->reset(migrate: false);
272+
$this->database->migrate(CreateMigrationsTable::class, CreateUsersTable::class);
273+
274+
$this->container->config(new GitHubOAuthConfig(
275+
clientId: 'foo',
276+
clientSecret: 'bar', // @mago-expect lint:no-literal-password
277+
redirectTo: '/oauth/github',
278+
));
279+
280+
$client = $this->oauth->fake($this->user);
281+
282+
$client->createRedirect();
283+
284+
$this->assertNotNull($client->getState());
285+
286+
$request = new GenericRequest(
287+
method: Method::GET,
288+
uri: Uri\set_query('/oauth/callback', code: 'auth-code', state: $client->getState()),
289+
);
290+
291+
$client->authenticate(
292+
$request,
293+
static fn (OAuthUser $user): User => query(User::class)->updateOrCreate(
294+
['github_id' => $user->id],
295+
['email' => $user->email ?? '', 'full_name' => $user->name ?? '', 'username' => $user->nickname ?? ''],
296+
),
297+
);
298+
299+
$this->assertNull($client->getState());
300+
}
301+
302+
#[Test]
303+
public function state_is_cleared_even_when_validation_fails(): void
304+
{
305+
$this->container->config(new GitHubOAuthConfig(
306+
clientId: 'foo',
307+
clientSecret: 'bar', // @mago-expect lint:no-literal-password
308+
redirectTo: '/oauth/github',
309+
));
310+
311+
$client = $this->oauth->fake($this->user);
312+
313+
$client->createRedirect();
314+
315+
$this->assertNotNull($client->getState());
316+
317+
$request = new GenericRequest(
318+
method: Method::GET,
319+
uri: Uri\set_query('/oauth/callback', code: 'auth-code', state: 'invalid-state'),
320+
);
321+
322+
try {
323+
$client->authenticate($request, static fn (OAuthUser $user): User => new User('', '', '', ''));
324+
} catch (OAuthStateWasInvalid) {
325+
// @mago-expect lint:no-empty-catch-clause
326+
}
327+
328+
$this->assertNull($client->getState());
329+
}
266330
}
267331

268332
final class User implements Authenticatable

0 commit comments

Comments
 (0)