From 60eff40a0c1ed51d513d08cd3c94ff91652e2d59 Mon Sep 17 00:00:00 2001 From: Dane Everitt Date: Wed, 3 Nov 2021 20:51:18 -0700 Subject: [PATCH 1/3] Fix session management on client API requests; closes #3727 Versions of Pterodactyl prior to 1.6.3 used a different throttle pathway for requests. That pathway found the current request user before continuing on to other in-app middleware, thus the user was available downstream. Changes introduced in 1.6.3 changed the throttler logic, therefore removing this step. As a result, the client API could not always get the currently authenticated user when cookies were used (aka, requests from the Panel UI, and not API directly). This change corrects the logic to get the session setup correctly before falling through to authenticating as a user using the API key. If a cookie is present and a user is found as a result that session will be used. If an API key is provided it is ignored when a cookie is also present. In order to keep the API stateless any session created for an API request stemming from an API key will have the associated session deleted at the end of the request, and the 'Set-Cookies' header will be stripped from the response. --- CHANGELOG.md | 4 +++ app/Http/Kernel.php | 10 +++--- app/Http/Middleware/Api/AuthenticateKey.php | 18 +++++----- .../Middleware/Api/HandleStatelessRequest.php | 35 +++++++++++++++++++ app/Http/Middleware/Api/SetSessionDriver.php | 35 ------------------- 5 files changed, 54 insertions(+), 48 deletions(-) create mode 100644 app/Http/Middleware/Api/HandleStatelessRequest.php delete mode 100644 app/Http/Middleware/Api/SetSessionDriver.php diff --git a/CHANGELOG.md b/CHANGELOG.md index 56744b194..01c298cff 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,10 @@ This file is a running track of new features and fixes to each version of the pa This project follows [Semantic Versioning](http://semver.org) guidelines. +## v1.6.4 +### Fixed +* Fixes a session management bug that would cause a user who signs out of one browser to be unintentionally logged out of other browser sessions when using the client API. + ## v1.6.3 ### Fixed * **[Security]** Changes logout endpoint to be a POST request with CSRF-token validation to prevent a malicious actor from triggering a user logout. diff --git a/app/Http/Kernel.php b/app/Http/Kernel.php index 30fcd8ce4..a96b32dec 100644 --- a/app/Http/Kernel.php +++ b/app/Http/Kernel.php @@ -18,7 +18,6 @@ use Pterodactyl\Http\Middleware\LanguageMiddleware; use Illuminate\Foundation\Http\Kernel as HttpKernel; use Pterodactyl\Http\Middleware\Api\AuthenticateKey; use Illuminate\Routing\Middleware\SubstituteBindings; -use Pterodactyl\Http\Middleware\Api\SetSessionDriver; use Illuminate\Session\Middleware\AuthenticateSession; use Illuminate\View\Middleware\ShareErrorsFromSession; use Pterodactyl\Http\Middleware\MaintenanceMiddleware; @@ -27,6 +26,7 @@ use Illuminate\Auth\Middleware\AuthenticateWithBasicAuth; use Pterodactyl\Http\Middleware\Api\AuthenticateIPAccess; use Pterodactyl\Http\Middleware\Api\ApiSubstituteBindings; use Illuminate\Foundation\Http\Middleware\ValidatePostSize; +use Pterodactyl\Http\Middleware\Api\HandleStatelessRequest; use Illuminate\Cookie\Middleware\AddQueuedCookiesToResponse; use Pterodactyl\Http\Middleware\Api\Daemon\DaemonAuthenticate; use Pterodactyl\Http\Middleware\RequireTwoFactorAuthentication; @@ -68,18 +68,18 @@ class Kernel extends HttpKernel RequireTwoFactorAuthentication::class, ], 'api' => [ + HandleStatelessRequest::class, IsValidJson::class, ApiSubstituteBindings::class, - SetSessionDriver::class, 'api..key:' . ApiKey::TYPE_APPLICATION, AuthenticateApplicationUser::class, AuthenticateIPAccess::class, ], 'client-api' => [ - StartSession::class, - SetSessionDriver::class, - AuthenticateSession::class, + HandleStatelessRequest::class, IsValidJson::class, + StartSession::class, + AuthenticateSession::class, SubstituteClientApiBindings::class, 'api..key:' . ApiKey::TYPE_ACCOUNT, AuthenticateIPAccess::class, diff --git a/app/Http/Middleware/Api/AuthenticateKey.php b/app/Http/Middleware/Api/AuthenticateKey.php index db469bc5e..397ba8c23 100644 --- a/app/Http/Middleware/Api/AuthenticateKey.php +++ b/app/Http/Middleware/Api/AuthenticateKey.php @@ -42,8 +42,10 @@ class AuthenticateKey } /** - * Handle an API request by verifying that the provided API key - * is in a valid format and exists in the database. + * Handle an API request by verifying that the provided API key is in a valid + * format and exists in the database. If there is currently a user in the session + * do not even bother to look at the token (they provided a cookie for this to + * be the case). * * @return mixed * @@ -56,17 +58,17 @@ class AuthenticateKey throw new HttpException(401, null, null, ['WWW-Authenticate' => 'Bearer']); } - $raw = $request->bearerToken(); - - // This is a request coming through using cookies, we have an authenticated user not using - // an API key. Make some fake API key models and continue on through the process. - if (empty($raw) && $request->user() instanceof User) { + // This is a request coming through using cookies, we have an authenticated user + // not using an API key. Make some fake API key models and continue on through + // the process. + if ($request->user() instanceof User) { $model = (new ApiKey())->forceFill([ 'user_id' => $request->user()->id, 'key_type' => ApiKey::TYPE_ACCOUNT, ]); } else { - $model = $this->authenticateApiKey($raw, $keyType); + $model = $this->authenticateApiKey($request->bearerToken(), $keyType); + $this->auth->guard()->loginUsingId($model->user_id); } diff --git a/app/Http/Middleware/Api/HandleStatelessRequest.php b/app/Http/Middleware/Api/HandleStatelessRequest.php new file mode 100644 index 000000000..ab697d687 --- /dev/null +++ b/app/Http/Middleware/Api/HandleStatelessRequest.php @@ -0,0 +1,35 @@ +bearerToken()) && $request->isJson()) { + $request->session()->getHandler()->destroy( + $request->session()->getId() + ); + + $response->headers->remove('Set-Cookie'); + } + + return $response; + } +} diff --git a/app/Http/Middleware/Api/SetSessionDriver.php b/app/Http/Middleware/Api/SetSessionDriver.php deleted file mode 100644 index 1c8f59a0b..000000000 --- a/app/Http/Middleware/Api/SetSessionDriver.php +++ /dev/null @@ -1,35 +0,0 @@ -config = $config; - } - - /** - * Set the session for API calls to only last for the one request. - * - * @return mixed - */ - public function handle(Request $request, Closure $next) - { - $this->config->set('session.driver', 'array'); - - return $next($request); - } -} From e8a8405899a565650d5567b851a4acd42cb5a31a Mon Sep 17 00:00:00 2001 From: Dane Everitt Date: Wed, 3 Nov 2021 21:22:14 -0700 Subject: [PATCH 2/3] Remove tests --- .../Middleware/Api/AuthenticateKeyTest.php | 168 ------------------ .../Middleware/Api/SetSessionDriverTest.php | 44 ----- 2 files changed, 212 deletions(-) delete mode 100644 tests/Unit/Http/Middleware/Api/AuthenticateKeyTest.php delete mode 100644 tests/Unit/Http/Middleware/Api/SetSessionDriverTest.php diff --git a/tests/Unit/Http/Middleware/Api/AuthenticateKeyTest.php b/tests/Unit/Http/Middleware/Api/AuthenticateKeyTest.php deleted file mode 100644 index 211e810df..000000000 --- a/tests/Unit/Http/Middleware/Api/AuthenticateKeyTest.php +++ /dev/null @@ -1,168 +0,0 @@ -auth = m::mock(AuthManager::class); - $this->encrypter = m::mock(Encrypter::class); - $this->repository = m::mock(ApiKeyRepositoryInterface::class); - } - - /** - * Test that a missing bearer token will throw an exception. - */ - public function testMissingBearerTokenThrowsException() - { - $this->request->shouldReceive('user')->andReturnNull(); - $this->request->shouldReceive('bearerToken')->withNoArgs()->once()->andReturnNull(); - - try { - $this->getMiddleware()->handle($this->request, $this->getClosureAssertions(), ApiKey::TYPE_APPLICATION); - } catch (HttpException $exception) { - $this->assertEquals(401, $exception->getStatusCode()); - $this->assertEquals(['WWW-Authenticate' => 'Bearer'], $exception->getHeaders()); - } - } - - /** - * Test that an invalid API identifier throws an exception. - */ - public function testInvalidIdentifier() - { - $this->expectException(AccessDeniedHttpException::class); - - $this->request->shouldReceive('bearerToken')->withNoArgs()->twice()->andReturn('abcd1234'); - $this->repository->shouldReceive('findFirstWhere')->andThrow(new RecordNotFoundException()); - - $this->getMiddleware()->handle($this->request, $this->getClosureAssertions(), ApiKey::TYPE_APPLICATION); - } - - /** - * Test that a valid token can continue past the middleware. - */ - public function testValidToken() - { - $model = ApiKey::factory()->make(); - - $this->request->shouldReceive('bearerToken')->withNoArgs()->twice()->andReturn($model->identifier . 'decrypted'); - $this->repository->shouldReceive('findFirstWhere')->with([ - ['identifier', '=', $model->identifier], - ['key_type', '=', ApiKey::TYPE_APPLICATION], - ])->once()->andReturn($model); - $this->encrypter->shouldReceive('decrypt')->with($model->token)->once()->andReturn('decrypted'); - $this->auth->shouldReceive('guard->loginUsingId')->with($model->user_id)->once()->andReturnNull(); - - $this->repository->shouldReceive('withoutFreshModel->update')->with($model->id, [ - 'last_used_at' => CarbonImmutable::now(), - ])->once()->andReturnNull(); - - $this->getMiddleware()->handle($this->request, $this->getClosureAssertions(), ApiKey::TYPE_APPLICATION); - $this->assertEquals($model, $this->request->attributes->get('api_key')); - } - - /** - * Test that a valid token can continue past the middleware when set as a user token. - */ - public function testValidTokenWithUserKey() - { - $model = ApiKey::factory()->make(); - - $this->request->shouldReceive('bearerToken')->withNoArgs()->twice()->andReturn($model->identifier . 'decrypted'); - $this->repository->shouldReceive('findFirstWhere')->with([ - ['identifier', '=', $model->identifier], - ['key_type', '=', ApiKey::TYPE_ACCOUNT], - ])->once()->andReturn($model); - $this->encrypter->shouldReceive('decrypt')->with($model->token)->once()->andReturn('decrypted'); - $this->auth->shouldReceive('guard->loginUsingId')->with($model->user_id)->once()->andReturnNull(); - - $this->repository->shouldReceive('withoutFreshModel->update')->with($model->id, [ - 'last_used_at' => CarbonImmutable::now(), - ])->once()->andReturnNull(); - - $this->getMiddleware()->handle($this->request, $this->getClosureAssertions(), ApiKey::TYPE_ACCOUNT); - $this->assertEquals($model, $this->request->attributes->get('api_key')); - } - - /** - * Test that we can still make it though this middleware if the user is logged in and passing - * through a cookie. - */ - public function testAccessWithoutToken() - { - $user = User::factory()->make(['id' => 123]); - - $this->request->shouldReceive('user')->andReturn($user); - $this->request->shouldReceive('bearerToken')->withNoArgs()->twice()->andReturnNull(); - - $this->getMiddleware()->handle($this->request, $this->getClosureAssertions(), ApiKey::TYPE_ACCOUNT); - $model = $this->request->attributes->get('api_key'); - - $this->assertSame(ApiKey::TYPE_ACCOUNT, $model->key_type); - $this->assertSame(123, $model->user_id); - $this->assertNull($model->identifier); - } - - /** - * Test that a valid token identifier with an invalid token attached to it - * triggers an exception. - */ - public function testInvalidTokenForIdentifier() - { - $this->expectException(AccessDeniedHttpException::class); - - $model = ApiKey::factory()->make(); - - $this->request->shouldReceive('bearerToken')->withNoArgs()->twice()->andReturn($model->identifier . 'asdf'); - $this->repository->shouldReceive('findFirstWhere')->with([ - ['identifier', '=', $model->identifier], - ['key_type', '=', ApiKey::TYPE_APPLICATION], - ])->once()->andReturn($model); - $this->encrypter->shouldReceive('decrypt')->with($model->token)->once()->andReturn('decrypted'); - - $this->getMiddleware()->handle($this->request, $this->getClosureAssertions(), ApiKey::TYPE_APPLICATION); - } - - /** - * Return an instance of the middleware with mocked dependencies for testing. - */ - private function getMiddleware(): AuthenticateKey - { - return new AuthenticateKey($this->repository, $this->auth, $this->encrypter); - } -} diff --git a/tests/Unit/Http/Middleware/Api/SetSessionDriverTest.php b/tests/Unit/Http/Middleware/Api/SetSessionDriverTest.php deleted file mode 100644 index 83fc2b7bc..000000000 --- a/tests/Unit/Http/Middleware/Api/SetSessionDriverTest.php +++ /dev/null @@ -1,44 +0,0 @@ -config = m::mock(Repository::class); - } - - /** - * Test that a production environment does not try to disable debug bar. - */ - public function testMiddleware() - { - $this->config->shouldReceive('set')->once()->with('session.driver', 'array')->andReturnNull(); - - $this->getMiddleware()->handle($this->request, $this->getClosureAssertions()); - } - - /** - * Return an instance of the middleware with mocked dependencies for testing. - */ - private function getMiddleware(): SetSessionDriver - { - return new SetSessionDriver($this->config); - } -} From 17c03e9a4d4003f460edfe4a40f68924a0ca8eda Mon Sep 17 00:00:00 2001 From: Dane Everitt Date: Wed, 3 Nov 2021 21:33:21 -0700 Subject: [PATCH 3/3] Fix broken session management for application api --- CHANGELOG.md | 6 ++++++ app/Http/Kernel.php | 2 ++ 2 files changed, 8 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 01c298cff..bc1e30d3e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,7 +3,13 @@ This file is a running track of new features and fixes to each version of the pa This project follows [Semantic Versioning](http://semver.org) guidelines. +## v1.6.5 +### Fixed +* Fixes broken application API endpoints due to changes introduced with session management in 1.6.4. + ## v1.6.4 +_This release should not be used, please use `1.6.5`. It has been pulled from our releases._ + ### Fixed * Fixes a session management bug that would cause a user who signs out of one browser to be unintentionally logged out of other browser sessions when using the client API. diff --git a/app/Http/Kernel.php b/app/Http/Kernel.php index a96b32dec..04a663f33 100644 --- a/app/Http/Kernel.php +++ b/app/Http/Kernel.php @@ -70,6 +70,8 @@ class Kernel extends HttpKernel 'api' => [ HandleStatelessRequest::class, IsValidJson::class, + StartSession::class, + AuthenticateSession::class, ApiSubstituteBindings::class, 'api..key:' . ApiKey::TYPE_APPLICATION, AuthenticateApplicationUser::class,