diff --git a/CHANGELOG.md b/CHANGELOG.md index 7f04a6d8..e09ae67f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,9 @@ 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.2.2 +* **[security]** Fixes authentication bypass allowing a user to take control of specific server actions such as executing schedules, rotating database passwords, and viewing or deleting a backup. + ## v1.2.1 ### Fixed * Fixes URL-encoding of filenames when working in the filemanager to fix issues when moving, renaming, or deleting files. diff --git a/app/Helpers/Utilities.php b/app/Helpers/Utilities.php index d6b4e752..6e8e9223 100644 --- a/app/Helpers/Utilities.php +++ b/app/Helpers/Utilities.php @@ -42,13 +42,14 @@ class Utilities * @param string $minute * @param string $hour * @param string $dayOfMonth + * @param string $month * @param string $dayOfWeek * @return \Carbon\Carbon */ - public static function getScheduleNextRunDate(string $minute, string $hour, string $dayOfMonth, string $dayOfWeek) + public static function getScheduleNextRunDate(string $minute, string $hour, string $dayOfMonth, string $month, string $dayOfWeek) { return Carbon::instance(CronExpression::factory( - sprintf('%s %s %s * %s', $minute, $hour, $dayOfMonth, $dayOfWeek) + sprintf('%s %s %s %s %s', $minute, $hour, $dayOfMonth, $month, $dayOfWeek) )->getNextRunDate()); } diff --git a/app/Http/Controllers/Api/Client/Servers/ScheduleController.php b/app/Http/Controllers/Api/Client/Servers/ScheduleController.php index a9310abd..0d7241a6 100644 --- a/app/Http/Controllers/Api/Client/Servers/ScheduleController.php +++ b/app/Http/Controllers/Api/Client/Servers/ScheduleController.php @@ -84,6 +84,7 @@ class ScheduleController extends ClientApiController 'server_id' => $server->id, 'name' => $request->input('name'), 'cron_day_of_week' => $request->input('day_of_week'), + 'cron_month' => $request->input('month'), 'cron_day_of_month' => $request->input('day_of_month'), 'cron_hour' => $request->input('hour'), 'cron_minute' => $request->input('minute'), @@ -136,6 +137,7 @@ class ScheduleController extends ClientApiController $data = [ 'name' => $request->input('name'), 'cron_day_of_week' => $request->input('day_of_week'), + 'cron_month' => $request->input('month'), 'cron_day_of_month' => $request->input('day_of_month'), 'cron_hour' => $request->input('hour'), 'cron_minute' => $request->input('minute'), @@ -211,6 +213,7 @@ class ScheduleController extends ClientApiController $request->input('minute'), $request->input('hour'), $request->input('day_of_month'), + $request->input('month'), $request->input('day_of_week') ); } catch (Exception $exception) { diff --git a/app/Http/Middleware/Api/Client/Server/AllocationBelongsToServer.php b/app/Http/Middleware/Api/Client/Server/AllocationBelongsToServer.php deleted file mode 100644 index d027d563..00000000 --- a/app/Http/Middleware/Api/Client/Server/AllocationBelongsToServer.php +++ /dev/null @@ -1,33 +0,0 @@ -route()->parameter('server'); - /** @var \Pterodactyl\Models\Allocation|null $allocation */ - $allocation = $request->route()->parameter('allocation'); - - if ($allocation && $allocation->server_id !== $server->id) { - throw new NotFoundHttpException; - } - - return $next($request); - } -} diff --git a/app/Http/Middleware/Api/Client/Server/ResourceBelongsToServer.php b/app/Http/Middleware/Api/Client/Server/ResourceBelongsToServer.php new file mode 100644 index 00000000..b57fee2e --- /dev/null +++ b/app/Http/Middleware/Api/Client/Server/ResourceBelongsToServer.php @@ -0,0 +1,92 @@ +route()->parameters(); + if (is_null($params) || ! $params['server'] instanceof Server) { + throw new InvalidArgumentException('This middleware cannot be used in a context that is missing a server in the parameters.'); + } + + /** @var \Pterodactyl\Models\Server $server */ + $server = $request->route()->parameter('server'); + $exception = new NotFoundHttpException('The requested resource was not found for this server.'); + foreach ($params as $key => $model) { + // Specifically skip the server, we're just trying to see if all of the + // other resources are assigned to this server. Also skip anything that + // is not currently a Model instance since those will just end up being + // a 404 down the road. + if ($key === 'server' || ! $model instanceof Model) { + continue; + } + + switch (get_class($model)) { + // All of these models use "server_id" as the field key for the server + // they are assigned to, so the logic is identical for them all. + case Allocation::class: + case Backup::class: + case Database::class: + case Schedule::class: + case Subuser::class: + if ($model->server_id !== $server->id) { + throw $exception; + } + break; + // Regular users are a special case here as we need to make sure they're + // currently assigned as a subuser on the server. + case User::class: + $subuser = $server->subusers()->where('user_id', $model->id)->first(); + if (is_null($subuser)) { + throw $exception; + } + // This is a special case to avoid an additional query being triggered + // in the underlying logic. + $request->attributes->set('subuser', $subuser); + break; + // Tasks are special since they're (currently) the only item in the API + // that requires something in addition to the server in order to be accessed. + case Task::class: + $schedule = $request->route()->parameter('schedule'); + if ($model->schedule_id !== $schedule->id || $schedule->server_id !== $server->id) { + throw $exception; + } + break; + default: + // Don't return a 404 here since we want to make sure no one relies + // on this middleware in a context in which it will not work. Fail safe. + throw new InvalidArgumentException('There is no handler configured for a resource of this type: ' . get_class($model)); + } + } + + return $next($request); + } +} diff --git a/app/Http/Middleware/Api/Client/Server/SubuserBelongsToServer.php b/app/Http/Middleware/Api/Client/Server/SubuserBelongsToServer.php deleted file mode 100644 index a80f6eef..00000000 --- a/app/Http/Middleware/Api/Client/Server/SubuserBelongsToServer.php +++ /dev/null @@ -1,36 +0,0 @@ -route()->parameter('server'); - /** @var \Pterodactyl\Models\User $user */ - $user = $request->route()->parameter('user'); - - // Don't do anything if there isn't a user present in the request. - if (is_null($user)) { - return $next($request); - } - - $request->attributes->set('subuser', $server->subusers()->where('user_id', $user->id)->firstOrFail()); - - return $next($request); - } -} diff --git a/app/Models/Schedule.php b/app/Models/Schedule.php index de047563..6d23f5c1 100644 --- a/app/Models/Schedule.php +++ b/app/Models/Schedule.php @@ -12,6 +12,7 @@ use Pterodactyl\Contracts\Extensions\HashidsInterface; * @property int $server_id * @property string $name * @property string $cron_day_of_week + * @property string $cron_month * @property string $cron_day_of_month * @property string $cron_hour * @property string $cron_minute @@ -58,6 +59,7 @@ class Schedule extends Model 'server_id', 'name', 'cron_day_of_week', + 'cron_month', 'cron_day_of_month', 'cron_hour', 'cron_minute', @@ -93,6 +95,7 @@ class Schedule extends Model protected $attributes = [ 'name' => null, 'cron_day_of_week' => '*', + 'cron_month' => '*', 'cron_day_of_month' => '*', 'cron_hour' => '*', 'cron_minute' => '*', @@ -107,6 +110,7 @@ class Schedule extends Model 'server_id' => 'required|exists:servers,id', 'name' => 'required|string|max:191', 'cron_day_of_week' => 'required|string', + 'cron_month' => 'required|string', 'cron_day_of_month' => 'required|string', 'cron_hour' => 'required|string', 'cron_minute' => 'required|string', @@ -123,7 +127,7 @@ class Schedule extends Model */ public function getNextRunDate() { - $formatted = sprintf('%s %s %s * %s', $this->cron_minute, $this->cron_hour, $this->cron_day_of_month, $this->cron_day_of_week); + $formatted = sprintf('%s %s %s %s %s', $this->cron_minute, $this->cron_hour, $this->cron_day_of_month, $this->cron_month, $this->cron_day_of_week); return CarbonImmutable::createFromTimestamp( CronExpression::factory($formatted)->getNextRunDate()->getTimestamp() diff --git a/app/Transformers/Api/Client/ScheduleTransformer.php b/app/Transformers/Api/Client/ScheduleTransformer.php index 43e35ed4..09e8708e 100644 --- a/app/Transformers/Api/Client/ScheduleTransformer.php +++ b/app/Transformers/Api/Client/ScheduleTransformer.php @@ -40,6 +40,7 @@ class ScheduleTransformer extends BaseClientTransformer 'cron' => [ 'day_of_week' => $model->cron_day_of_week, 'day_of_month' => $model->cron_day_of_month, + 'month' => $model->cron_month, 'hour' => $model->cron_hour, 'minute' => $model->cron_minute, ], diff --git a/database/factories/ModelFactory.php b/database/factories/ModelFactory.php index 4997a9b6..e6468f5f 100644 --- a/database/factories/ModelFactory.php +++ b/database/factories/ModelFactory.php @@ -7,6 +7,8 @@ use Illuminate\Support\Str; use Pterodactyl\Models\Node; use Faker\Generator as Faker; use Pterodactyl\Models\ApiKey; +use Pterodactyl\Models\Backup; +use Pterodactyl\Models\Permission; /** @var \Illuminate\Database\Eloquent\Factory $factory */ /* @@ -134,7 +136,9 @@ $factory->state(Pterodactyl\Models\EggVariable::class, 'editable', function () { }); $factory->define(Pterodactyl\Models\Subuser::class, function (Faker $faker) { - return []; + return [ + 'permissions' => [Permission::ACTION_WEBSOCKET_CONNECT], + ]; }); $factory->define(Pterodactyl\Models\Allocation::class, function (Faker $faker) { @@ -161,7 +165,7 @@ $factory->define(Pterodactyl\Models\Database::class, function (Faker $faker) { 'database' => str_random(10), 'username' => str_random(10), 'remote' => '%', - 'password' => $password ?: bcrypt('test123'), + 'password' => $password ?: encrypt('test123'), 'created_at' => Carbon::now()->toDateTimeString(), 'updated_at' => Carbon::now()->toDateTimeString(), ]; @@ -196,3 +200,12 @@ $factory->define(Pterodactyl\Models\ApiKey::class, function (Faker $faker) { 'updated_at' => Carbon::now()->toDateTimeString(), ]; }); + +$factory->define(Pterodactyl\Models\Backup::class, function (Faker $faker) { + return [ + 'uuid' => Uuid::uuid4()->toString(), + 'is_successful' => true, + 'name' => $faker->sentence, + 'disk' => Backup::ADAPTER_WINGS, + ]; +}); diff --git a/database/migrations/2021_01_13_013420_add_cron_month.php b/database/migrations/2021_01_13_013420_add_cron_month.php new file mode 100644 index 00000000..c449a53e --- /dev/null +++ b/database/migrations/2021_01_13_013420_add_cron_month.php @@ -0,0 +1,32 @@ +string('cron_month')->after('cron_day_of_week'); + }); + } + + /** + * Reverse the migrations. + * + * @return void + */ + public function down() + { + Schema::table('schedules', function (Blueprint $table) { + $table->dropColumn('cron_month'); + }); + } +} diff --git a/resources/scripts/api/server/schedules/createOrUpdateSchedule.ts b/resources/scripts/api/server/schedules/createOrUpdateSchedule.ts index 0545650b..481cea77 100644 --- a/resources/scripts/api/server/schedules/createOrUpdateSchedule.ts +++ b/resources/scripts/api/server/schedules/createOrUpdateSchedule.ts @@ -11,6 +11,7 @@ export default (uuid: string, schedule: Data): Promise => { minute: schedule.cron.minute, hour: schedule.cron.hour, day_of_month: schedule.cron.dayOfMonth, + month: schedule.cron.month, day_of_week: schedule.cron.dayOfWeek, }) .then(({ data }) => resolve(rawDataToServerSchedule(data.attributes))) diff --git a/resources/scripts/api/server/schedules/getServerSchedules.ts b/resources/scripts/api/server/schedules/getServerSchedules.ts index 42514582..f7a07617 100644 --- a/resources/scripts/api/server/schedules/getServerSchedules.ts +++ b/resources/scripts/api/server/schedules/getServerSchedules.ts @@ -5,6 +5,7 @@ export interface Schedule { name: string; cron: { dayOfWeek: string; + month: string; dayOfMonth: string; hour: string; minute: string; @@ -46,6 +47,7 @@ export const rawDataToServerSchedule = (data: any): Schedule => ({ name: data.name, cron: { dayOfWeek: data.cron.day_of_week, + month: data.cron.month, dayOfMonth: data.cron.day_of_month, hour: data.cron.hour, minute: data.cron.minute, diff --git a/resources/scripts/components/dashboard/DashboardContainer.tsx b/resources/scripts/components/dashboard/DashboardContainer.tsx index b4cb4c29..2edc0605 100644 --- a/resources/scripts/components/dashboard/DashboardContainer.tsx +++ b/resources/scripts/components/dashboard/DashboardContainer.tsx @@ -16,8 +16,9 @@ import Pagination from '@/components/elements/Pagination'; export default () => { const { clearFlashes, clearAndAddHttpError } = useFlash(); const [ page, setPage ] = useState(1); - const { rootAdmin } = useStoreState(state => state.user.data!); - const [ showOnlyAdmin, setShowOnlyAdmin ] = usePersistedState('show_all_servers', false); + const uuid = useStoreState(state => state.user.data!.uuid); + const rootAdmin = useStoreState(state => state.user.data!.rootAdmin); + const [ showOnlyAdmin, setShowOnlyAdmin ] = usePersistedState(`${uuid}:show_all_servers`, false); const { data: servers, error } = useSWR>( [ '/api/client/servers', showOnlyAdmin, page ], diff --git a/resources/scripts/components/server/schedules/EditScheduleModal.tsx b/resources/scripts/components/server/schedules/EditScheduleModal.tsx index 73f74f2c..e0df9e98 100644 --- a/resources/scripts/components/server/schedules/EditScheduleModal.tsx +++ b/resources/scripts/components/server/schedules/EditScheduleModal.tsx @@ -19,6 +19,7 @@ type Props = { interface Values { name: string; dayOfWeek: string; + month: string; dayOfMonth: string; hour: string; minute: string; @@ -38,7 +39,7 @@ const EditScheduleModal = ({ schedule, ...props }: Omit -
+
@@ -48,6 +49,9 @@ const EditScheduleModal = ({ schedule, ...props }: Omit
+
+ +
@@ -94,6 +98,7 @@ export default ({ schedule, visible, ...props }: Props) => { minute: values.minute, hour: values.hour, dayOfWeek: values.dayOfWeek, + month: values.month, dayOfMonth: values.dayOfMonth, }, isActive: values.enabled, @@ -116,10 +121,11 @@ export default ({ schedule, visible, ...props }: Props) => { onSubmit={submit} initialValues={{ name: schedule?.name || '', - dayOfWeek: schedule?.cron.dayOfWeek || '*', - dayOfMonth: schedule?.cron.dayOfMonth || '*', - hour: schedule?.cron.hour || '*', minute: schedule?.cron.minute || '*/5', + hour: schedule?.cron.hour || '*', + dayOfMonth: schedule?.cron.dayOfMonth || '*', + month: schedule?.cron.month || '*', + dayOfWeek: schedule?.cron.dayOfWeek || '*', enabled: schedule ? schedule.isActive : true, } as Values} validationSchema={null} diff --git a/resources/scripts/components/server/schedules/ScheduleCronRow.tsx b/resources/scripts/components/server/schedules/ScheduleCronRow.tsx index e7918a13..2a733b2d 100644 --- a/resources/scripts/components/server/schedules/ScheduleCronRow.tsx +++ b/resources/scripts/components/server/schedules/ScheduleCronRow.tsx @@ -22,7 +22,7 @@ const ScheduleCronRow = ({ cron, className }: Props) => (

Day (Month)

-

*

+

{cron.month}

Month

diff --git a/resources/scripts/components/server/schedules/ScheduleEditContainer.tsx b/resources/scripts/components/server/schedules/ScheduleEditContainer.tsx index 17d58ac4..8fcb91af 100644 --- a/resources/scripts/components/server/schedules/ScheduleEditContainer.tsx +++ b/resources/scripts/components/server/schedules/ScheduleEditContainer.tsx @@ -28,9 +28,9 @@ interface State { } const CronBox = ({ title, value }: { title: string; value: string }) => ( -
+

{title}

-

{value}

+

{value}

); @@ -88,13 +88,6 @@ export default () => { : <> -
- - - - - -
@@ -143,6 +136,13 @@ export default () => {
+
+ + + + + +
{schedule.tasks.length > 0 ? schedule.tasks.map(task => ( diff --git a/resources/scripts/plugins/usePersistedState.ts b/resources/scripts/plugins/usePersistedState.ts index f3198555..007e3fcd 100644 --- a/resources/scripts/plugins/usePersistedState.ts +++ b/resources/scripts/plugins/usePersistedState.ts @@ -1,6 +1,6 @@ import { Dispatch, SetStateAction, useEffect, useState } from 'react'; -export function usePersistedState (key: string, defaultValue: S): [S | undefined, Dispatch>] { +export function usePersistedState (key: string, defaultValue: S): [ S | undefined, Dispatch> ] { const [ state, setState ] = useState( () => { try { @@ -12,7 +12,7 @@ export function usePersistedState (key: string, defaultValue: S): return defaultValue; } - } + }, ); useEffect(() => { diff --git a/routes/api-client.php b/routes/api-client.php index a00938f5..a82be715 100644 --- a/routes/api-client.php +++ b/routes/api-client.php @@ -3,6 +3,7 @@ use Illuminate\Support\Facades\Route; use Pterodactyl\Http\Middleware\RequireTwoFactorAuthentication; use Pterodactyl\Http\Middleware\Api\Client\Server\SubuserBelongsToServer; +use Pterodactyl\Http\Middleware\Api\Client\Server\ResourceBelongsToServer; use Pterodactyl\Http\Middleware\Api\Client\Server\AuthenticateServerAccess; use Pterodactyl\Http\Middleware\Api\Client\Server\AllocationBelongsToServer; @@ -39,7 +40,7 @@ Route::group(['prefix' => '/account'], function () { | Endpoint: /api/client/servers/{server} | */ -Route::group(['prefix' => '/servers/{server}', 'middleware' => [AuthenticateServerAccess::class]], function () { +Route::group(['prefix' => '/servers/{server}', 'middleware' => [AuthenticateServerAccess::class, ResourceBelongsToServer::class]], function () { Route::get('/', 'Servers\ServerController@index')->name('api:client:server.view'); Route::get('/websocket', 'Servers\WebsocketController')->name('api:client:server.ws'); Route::get('/resources', 'Servers\ResourceUtilizationController')->name('api:client:server.resources'); @@ -83,7 +84,7 @@ Route::group(['prefix' => '/servers/{server}', 'middleware' => [AuthenticateServ Route::delete('/{schedule}/tasks/{task}', 'Servers\ScheduleTaskController@delete'); }); - Route::group(['prefix' => '/network', 'middleware' => [AllocationBelongsToServer::class]], function () { + Route::group(['prefix' => '/network'], function () { Route::get('/allocations', 'Servers\NetworkAllocationController@index'); Route::post('/allocations', 'Servers\NetworkAllocationController@store'); Route::post('/allocations/{allocation}', 'Servers\NetworkAllocationController@update'); @@ -91,7 +92,7 @@ Route::group(['prefix' => '/servers/{server}', 'middleware' => [AuthenticateServ Route::delete('/allocations/{allocation}', 'Servers\NetworkAllocationController@delete'); }); - Route::group(['prefix' => '/users', 'middleware' => [SubuserBelongsToServer::class]], function () { + Route::group(['prefix' => '/users'], function () { Route::get('/', 'Servers\SubuserController@index'); Route::post('/', 'Servers\SubuserController@store'); Route::get('/{user}', 'Servers\SubuserController@view'); diff --git a/tests/Integration/Api/Client/ClientApiIntegrationTestCase.php b/tests/Integration/Api/Client/ClientApiIntegrationTestCase.php index b7795905..85c261c2 100644 --- a/tests/Integration/Api/Client/ClientApiIntegrationTestCase.php +++ b/tests/Integration/Api/Client/ClientApiIntegrationTestCase.php @@ -10,11 +10,15 @@ use Pterodactyl\Models\Task; use Pterodactyl\Models\User; use Webmozart\Assert\Assert; use Pterodactyl\Models\Server; +use Pterodactyl\Models\Backup; use Pterodactyl\Models\Subuser; use Pterodactyl\Models\Location; use Pterodactyl\Models\Schedule; +use Pterodactyl\Models\Database; use Illuminate\Support\Collection; use Pterodactyl\Models\Allocation; +use Pterodactyl\Models\DatabaseHost; +use Pterodactyl\Tests\Integration\TestResponse; use Pterodactyl\Tests\Integration\IntegrationTestCase; use Pterodactyl\Transformers\Api\Client\BaseClientTransformer; @@ -25,6 +29,9 @@ abstract class ClientApiIntegrationTestCase extends IntegrationTestCase */ protected function tearDown(): void { + Database::query()->forceDelete(); + DatabaseHost::query()->forceDelete(); + Backup::query()->forceDelete(); Server::query()->forceDelete(); Node::query()->forceDelete(); Location::query()->forceDelete(); @@ -44,6 +51,19 @@ abstract class ClientApiIntegrationTestCase extends IntegrationTestCase CarbonImmutable::setTestNow(Carbon::now()); } + /** + * Override the default createTestResponse from Illuminate so that we can + * just dump 500-level errors to the screen in the tests without having + * to keep re-assigning variables. + * + * @param \Illuminate\Http\Response $response + * @return \Illuminate\Testing\TestResponse + */ + protected function createTestResponse($response) + { + return TestResponse::fromBaseResponse($response); + } + /** * Returns a link to the specific resource using the client API. * diff --git a/tests/Integration/Api/Client/Server/Allocation/AllocationAuthorizationTest.php b/tests/Integration/Api/Client/Server/Allocation/AllocationAuthorizationTest.php new file mode 100644 index 00000000..e46c4620 --- /dev/null +++ b/tests/Integration/Api/Client/Server/Allocation/AllocationAuthorizationTest.php @@ -0,0 +1,63 @@ +generateTestAccount(); + // Will be a subuser of $server2. + $server2 = $this->createServerModel(); + // And as no access to $server3. + $server3 = $this->createServerModel(); + + // Set the API $user as a subuser of server 2, but with no permissions + // to do anything with the allocations for that server. + factory(Subuser::class)->create(['server_id' => $server2->id, 'user_id' => $user->id]); + + $allocation1 = factory(Allocation::class)->create(['server_id' => $server1->id, 'node_id' => $server1->node_id]); + $allocation2 = factory(Allocation::class)->create(['server_id' => $server2->id, 'node_id' => $server2->node_id]); + $allocation3 = factory(Allocation::class)->create(['server_id' => $server3->id, 'node_id' => $server3->node_id]); + + // This is the only valid call for this test, accessing the allocation for the same + // server that the API user is the owner of. + $response = $this->actingAs($user)->json($method, $this->link($server1, "/network/allocations/" . $allocation1->id . $endpoint)); + $this->assertTrue($response->status() <= 204 || $response->status() === 400 || $response->status() === 422); + + // This request fails because the allocation is valid for that server but the user + // making the request is not authorized to perform that action. + $this->actingAs($user)->json($method, $this->link($server2, "/network/allocations/" . $allocation2->id . $endpoint))->assertForbidden(); + + // Both of these should report a 404 error due to the allocations being linked to + // servers that are not the same as the server in the request, or are assigned + // to a server for which the user making the request has no access to. + $this->actingAs($user)->json($method, $this->link($server1, "/network/allocations/" . $allocation2->id . $endpoint))->assertNotFound(); + $this->actingAs($user)->json($method, $this->link($server1, "/network/allocations/" . $allocation3->id . $endpoint))->assertNotFound(); + $this->actingAs($user)->json($method, $this->link($server2, "/network/allocations/" . $allocation3->id . $endpoint))->assertNotFound(); + $this->actingAs($user)->json($method, $this->link($server3, "/network/allocations/" . $allocation3->id . $endpoint))->assertNotFound(); + } + + /** + * @return \string[][] + */ + public function methodDataProvider(): array + { + return [ + ["POST", ""], + ["DELETE", ""], + ["POST", "/primary"], + ]; + } +} diff --git a/tests/Integration/Api/Client/Server/Backup/BackupAuthorizationTest.php b/tests/Integration/Api/Client/Server/Backup/BackupAuthorizationTest.php new file mode 100644 index 00000000..b680b534 --- /dev/null +++ b/tests/Integration/Api/Client/Server/Backup/BackupAuthorizationTest.php @@ -0,0 +1,71 @@ +generateTestAccount(); + // Will be a subuser of $server2. + $server2 = $this->createServerModel(); + // And as no access to $server3. + $server3 = $this->createServerModel(); + + // Set the API $user as a subuser of server 2, but with no permissions + // to do anything with the backups for that server. + factory(Subuser::class)->create(['server_id' => $server2->id, 'user_id' => $user->id]); + + $backup1 = factory(Backup::class)->create(['server_id' => $server1->id, 'completed_at' => CarbonImmutable::now()]); + $backup2 = factory(Backup::class)->create(['server_id' => $server2->id, 'completed_at' => CarbonImmutable::now()]); + $backup3 = factory(Backup::class)->create(['server_id' => $server3->id, 'completed_at' => CarbonImmutable::now()]); + + $this->instance(DeleteBackupService::class, $mock = Mockery::mock(DeleteBackupService::class)); + + if ($method === 'DELETE') { + $mock->expects('handle')->andReturnUndefined(); + } + + // This is the only valid call for this test, accessing the backup for the same + // server that the API user is the owner of. + $this->actingAs($user)->json($method, $this->link($server1, "/backups/" . $backup1->uuid . $endpoint)) + ->assertStatus($method === 'DELETE' ? 204 : 200); + + // This request fails because the backup is valid for that server but the user + // making the request is not authorized to perform that action. + $this->actingAs($user)->json($method, $this->link($server2, "/backups/" . $backup2->uuid . $endpoint))->assertForbidden(); + + // Both of these should report a 404 error due to the backup being linked to + // servers that are not the same as the server in the request, or are assigned + // to a server for which the user making the request has no access to. + $this->actingAs($user)->json($method, $this->link($server1, "/backups/" . $backup2->uuid . $endpoint))->assertNotFound(); + $this->actingAs($user)->json($method, $this->link($server1, "/backups/" . $backup3->uuid . $endpoint))->assertNotFound(); + $this->actingAs($user)->json($method, $this->link($server2, "/backups/" . $backup3->uuid . $endpoint))->assertNotFound(); + $this->actingAs($user)->json($method, $this->link($server3, "/backups/" . $backup3->uuid . $endpoint))->assertNotFound(); + } + + /** + * @return \string[][] + */ + public function methodDataProvider(): array + { + return [ + ["GET", ""], + ["GET", "/download"], + ["DELETE", ""], + ]; + } +} diff --git a/tests/Integration/Api/Client/Server/Database/DatabaseAuthorizationTest.php b/tests/Integration/Api/Client/Server/Database/DatabaseAuthorizationTest.php new file mode 100644 index 00000000..aecd71db --- /dev/null +++ b/tests/Integration/Api/Client/Server/Database/DatabaseAuthorizationTest.php @@ -0,0 +1,78 @@ +generateTestAccount(); + // Will be a subuser of $server2. + $server2 = $this->createServerModel(); + // And as no access to $server3. + $server3 = $this->createServerModel(); + + $host = factory(DatabaseHost::class)->create([]); + + // Set the API $user as a subuser of server 2, but with no permissions + // to do anything with the databases for that server. + factory(Subuser::class)->create(['server_id' => $server2->id, 'user_id' => $user->id]); + + $database1 = factory(Database::class)->create(['server_id' => $server1->id, 'database_host_id' => $host->id]); + $database2 = factory(Database::class)->create(['server_id' => $server2->id, 'database_host_id' => $host->id]); + $database3 = factory(Database::class)->create(['server_id' => $server3->id, 'database_host_id' => $host->id]); + + $this->instance(DatabasePasswordService::class, $mock = Mockery::mock(DatabasePasswordService::class)); + $this->instance(DatabaseManagementService::class, $mock2 = Mockery::mock(DatabaseManagementService::class)); + + if ($method === 'POST') { + $mock->expects('handle')->andReturnUndefined(); + } else { + $mock2->expects('delete')->andReturnUndefined(); + } + + $hashids = $this->app->make(HashidsInterface::class); + // This is the only valid call for this test, accessing the database for the same + // server that the API user is the owner of. + $this->actingAs($user)->json($method, $this->link($server1, "/databases/" . $hashids->encode($database1->id) . $endpoint)) + ->assertStatus($method === 'DELETE' ? 204 : 200); + + // This request fails because the database is valid for that server but the user + // making the request is not authorized to perform that action. + $this->actingAs($user)->json($method, $this->link($server2, "/databases/" . $hashids->encode($database2->id) . $endpoint))->assertForbidden(); + + // Both of these should report a 404 error due to the database being linked to + // servers that are not the same as the server in the request, or are assigned + // to a server for which the user making the request has no access to. + $this->actingAs($user)->json($method, $this->link($server1, "/databases/" . $hashids->encode($database2->id) . $endpoint))->assertNotFound(); + $this->actingAs($user)->json($method, $this->link($server1, "/databases/" . $hashids->encode($database3->id) . $endpoint))->assertNotFound(); + $this->actingAs($user)->json($method, $this->link($server2, "/databases/" . $hashids->encode($database3->id) . $endpoint))->assertNotFound(); + $this->actingAs($user)->json($method, $this->link($server3, "/databases/" . $hashids->encode($database3->id) . $endpoint))->assertNotFound(); + } + + /** + * @return \string[][] + */ + public function methodDataProvider(): array + { + return [ + ["POST", "/rotate-password"], + ["DELETE", ""], + ]; + } +} diff --git a/tests/Integration/Api/Client/Server/Schedule/CreateServerScheduleTest.php b/tests/Integration/Api/Client/Server/Schedule/CreateServerScheduleTest.php index 51345fa7..6c6f9f85 100644 --- a/tests/Integration/Api/Client/Server/Schedule/CreateServerScheduleTest.php +++ b/tests/Integration/Api/Client/Server/Schedule/CreateServerScheduleTest.php @@ -25,6 +25,7 @@ class CreateServerScheduleTest extends ClientApiIntegrationTestCase 'minute' => '0', 'hour' => '*/2', 'day_of_week' => '2', + 'month' => '1', 'day_of_month' => '*', ]); @@ -39,6 +40,7 @@ class CreateServerScheduleTest extends ClientApiIntegrationTestCase $this->assertSame('0', $schedule->cron_minute); $this->assertSame('*/2', $schedule->cron_hour); $this->assertSame('2', $schedule->cron_day_of_week); + $this->assertSame('1', $schedule->cron_month); $this->assertSame('*', $schedule->cron_day_of_month); $this->assertSame('Test Schedule', $schedule->name); @@ -69,6 +71,7 @@ class CreateServerScheduleTest extends ClientApiIntegrationTestCase 'minute' => '*', 'hour' => '*', 'day_of_month' => '*', + 'month' => '*', 'day_of_week' => '*', ]) ->assertStatus(Response::HTTP_UNPROCESSABLE_ENTITY) diff --git a/tests/Integration/Api/Client/Server/Schedule/ScheduleAuthorizationTest.php b/tests/Integration/Api/Client/Server/Schedule/ScheduleAuthorizationTest.php new file mode 100644 index 00000000..583fb4ae --- /dev/null +++ b/tests/Integration/Api/Client/Server/Schedule/ScheduleAuthorizationTest.php @@ -0,0 +1,72 @@ +generateTestAccount(); + // Will be a subuser of $server2. + $server2 = $this->createServerModel(); + // And as no access to $server3. + $server3 = $this->createServerModel(); + + // Set the API $user as a subuser of server 2, but with no permissions + // to do anything with the schedules for that server. + factory(Subuser::class)->create(['server_id' => $server2->id, 'user_id' => $user->id]); + + $schedule1 = factory(Schedule::class)->create(['server_id' => $server1->id]); + $schedule2 = factory(Schedule::class)->create(['server_id' => $server2->id]); + $schedule3 = factory(Schedule::class)->create(['server_id' => $server3->id]); + + // This is the only valid call for this test, accessing the schedule for the same + // server that the API user is the owner of. + $response = $this->actingAs($user)->json($method, $this->link($server1, "/schedules/" . $schedule1->id . $endpoint)); + $this->assertTrue($response->status() <= 204 || $response->status() === 400 || $response->status() === 422); + + // This request fails because the schedule is valid for that server but the user + // making the request is not authorized to perform that action. + $this->actingAs($user)->json($method, $this->link($server2, "/schedules/" . $schedule2->id . $endpoint))->assertForbidden(); + + // Both of these should report a 404 error due to the schedules being linked to + // servers that are not the same as the server in the request, or are assigned + // to a server for which the user making the request has no access to. + $this->actingAs($user)->json($method, $this->link($server1, "/schedules/" . $schedule2->id . $endpoint))->assertNotFound(); + $this->actingAs($user)->json($method, $this->link($server1, "/schedules/" . $schedule3->id . $endpoint))->assertNotFound(); + $this->actingAs($user)->json($method, $this->link($server2, "/schedules/" . $schedule3->id . $endpoint))->assertNotFound(); + $this->actingAs($user)->json($method, $this->link($server3, "/schedules/" . $schedule3->id . $endpoint))->assertNotFound(); + } + + /** + * @return \string[][] + */ + public function methodDataProvider(): array + { + return [ + ["GET", ""], + ["POST", ""], + ["DELETE", ""], + ["POST", "/execute"], + ["POST", "/tasks"], + ]; + } +} diff --git a/tests/Integration/Api/Client/Server/Schedule/UpdateServerScheduleTest.php b/tests/Integration/Api/Client/Server/Schedule/UpdateServerScheduleTest.php index ec60a413..979cd9e2 100644 --- a/tests/Integration/Api/Client/Server/Schedule/UpdateServerScheduleTest.php +++ b/tests/Integration/Api/Client/Server/Schedule/UpdateServerScheduleTest.php @@ -19,6 +19,7 @@ class UpdateServerScheduleTest extends ClientApiIntegrationTestCase 'minute' => '5', 'hour' => '*', 'day_of_week' => '*', + 'month' => '*', 'day_of_month' => '*', 'is_active' => false, ]; @@ -35,7 +36,7 @@ class UpdateServerScheduleTest extends ClientApiIntegrationTestCase /** @var \Pterodactyl\Models\Schedule $schedule */ $schedule = factory(Schedule::class)->create(['server_id' => $server->id]); - $expected = Utilities::getScheduleNextRunDate('5', '*', '*', '*'); + $expected = Utilities::getScheduleNextRunDate('5', '*', '*', '*', '*'); $response = $this->actingAs($user) ->postJson("/api/client/servers/{$server->uuid}/schedules/{$schedule->id}", $this->updateData); diff --git a/tests/Integration/Api/Client/Server/Subuser/SubuserAuthorizationTest.php b/tests/Integration/Api/Client/Server/Subuser/SubuserAuthorizationTest.php new file mode 100644 index 00000000..f852ed06 --- /dev/null +++ b/tests/Integration/Api/Client/Server/Subuser/SubuserAuthorizationTest.php @@ -0,0 +1,61 @@ +create(); + + // The API $user is the owner of $server1. + [$user, $server1] = $this->generateTestAccount(); + // Will be a subuser of $server2. + $server2 = $this->createServerModel(); + // And as no access to $server3. + $server3 = $this->createServerModel(); + + // Set the API $user as a subuser of server 2, but with no permissions + // to do anything with the subusers for that server. + factory(Subuser::class)->create(['server_id' => $server2->id, 'user_id' => $user->id]); + + factory(Subuser::class)->create(['server_id' => $server1->id, 'user_id' => $internal->id]); + factory(Subuser::class)->create(['server_id' => $server2->id, 'user_id' => $internal->id]); + factory(Subuser::class)->create(['server_id' => $server3->id, 'user_id' => $internal->id]); + + $this->instance(DaemonServerRepository::class, $mock = Mockery::mock(DaemonServerRepository::class)); + if ($method === 'DELETE') { + $mock->expects('setServer->revokeUserJTI')->with($internal->id)->andReturnUndefined(); + } + + // This route is acceptable since they're accessing a subuser on their own server. + $this->actingAs($user)->json($method, $this->link($server1, "/users/" . $internal->uuid))->assertStatus($method === 'POST' ? 422 : ($method === 'DELETE' ? 204 : 200)); + + // This route can be revealed since the subuser belongs to the correct server, but + // errors out with a 403 since $user does not have the right permissions for this. + $this->actingAs($user)->json($method, $this->link($server2, "/users/" . $internal->uuid))->assertForbidden(); + $this->actingAs($user)->json($method, $this->link($server3, "/users/" . $internal->uuid))->assertNotFound(); + } + + /** + * @return \string[][] + */ + public function methodDataProvider(): array + { + return [["GET"], ["POST"], ["DELETE"]]; + } +} diff --git a/tests/Integration/TestResponse.php b/tests/Integration/TestResponse.php new file mode 100644 index 00000000..a812f37b --- /dev/null +++ b/tests/Integration/TestResponse.php @@ -0,0 +1,37 @@ +getStatusCode(); + + // Dump the response to the screen before making the assertion which is going + // to fail so that debugging isn't such a nightmare. + if ($actual !== $status && $status !== 500) { + $this->dump(); + if (! is_null($this->exception) && ! $this->exception instanceof DisplayException && ! $this->exception instanceof ValidationException) { + dump($this->exception); + } + } + + PHPUnit::assertSame($actual, $status, "Expected status code {$status} but received {$actual}."); + + return $this; + } +}