From ffeedf17e43e070f960bdd6fb4f17c654097f523 Mon Sep 17 00:00:00 2001 From: Charles Morgan Date: Sat, 16 Jan 2021 22:07:39 -0500 Subject: [PATCH 1/7] Adds months for schedules Adds month variable for schedules --- app/Helpers/Utilities.php | 5 +-- .../Api/Client/Servers/ScheduleController.php | 3 ++ app/Models/Schedule.php | 6 +++- .../Api/Client/ScheduleTransformer.php | 1 + .../2021_01_13_013420_add_cron_month.php | 32 +++++++++++++++++++ .../schedules/createOrUpdateSchedule.ts | 1 + .../server/schedules/getServerSchedules.ts | 2 ++ .../server/schedules/EditScheduleModal.tsx | 14 +++++--- .../server/schedules/ScheduleCronRow.tsx | 2 +- .../schedules/ScheduleEditContainer.tsx | 4 +-- .../Schedule/CreateServerScheduleTest.php | 3 ++ .../Schedule/UpdateServerScheduleTest.php | 3 +- 12 files changed, 65 insertions(+), 11 deletions(-) create mode 100644 database/migrations/2021_01_13_013420_add_cron_month.php diff --git a/app/Helpers/Utilities.php b/app/Helpers/Utilities.php index d6b4e752f..6e8e9223c 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 a9310abd6..0d7241a63 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/Models/Schedule.php b/app/Models/Schedule.php index de0475639..6d23f5c11 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 43e35ed42..09e8708e1 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/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 000000000..c449a53e8 --- /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 0545650be..481cea777 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 42514582a..f7a07617c 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/server/schedules/EditScheduleModal.tsx b/resources/scripts/components/server/schedules/EditScheduleModal.tsx index 73f74f2cf..e0df9e981 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 e7918a132..2a733b2d4 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 17d58ac4e..ff4c12843 100644 --- a/resources/scripts/components/server/schedules/ScheduleEditContainer.tsx +++ b/resources/scripts/components/server/schedules/ScheduleEditContainer.tsx @@ -88,11 +88,11 @@ export default () => { : <> -
+
- +
diff --git a/tests/Integration/Api/Client/Server/Schedule/CreateServerScheduleTest.php b/tests/Integration/Api/Client/Server/Schedule/CreateServerScheduleTest.php index 51345fa79..6c6f9f85e 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/UpdateServerScheduleTest.php b/tests/Integration/Api/Client/Server/Schedule/UpdateServerScheduleTest.php index ec60a4137..979cd9e2f 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); From f5a97d4399ef5910dbaaab9da4348802a116b806 Mon Sep 17 00:00:00 2001 From: Charles Morgan Date: Sun, 17 Jan 2021 23:18:00 -0500 Subject: [PATCH 2/7] Edit UI Moves cron time display under title bar. --- .../server/schedules/ScheduleEditContainer.tsx | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/resources/scripts/components/server/schedules/ScheduleEditContainer.tsx b/resources/scripts/components/server/schedules/ScheduleEditContainer.tsx index ff4c12843..8fcb91af0 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 => ( From eecd550c4817dc52c76239f5b7b0c252aef537b3 Mon Sep 17 00:00:00 2001 From: Dane Everitt Date: Tue, 19 Jan 2021 20:11:00 -0800 Subject: [PATCH 3/7] Make debugging test failures easier --- .../Client/ClientApiIntegrationTestCase.php | 14 ++++++++ tests/Integration/TestResponse.php | 35 +++++++++++++++++++ 2 files changed, 49 insertions(+) create mode 100644 tests/Integration/TestResponse.php diff --git a/tests/Integration/Api/Client/ClientApiIntegrationTestCase.php b/tests/Integration/Api/Client/ClientApiIntegrationTestCase.php index b77959058..dc12a341b 100644 --- a/tests/Integration/Api/Client/ClientApiIntegrationTestCase.php +++ b/tests/Integration/Api/Client/ClientApiIntegrationTestCase.php @@ -15,6 +15,7 @@ use Pterodactyl\Models\Location; use Pterodactyl\Models\Schedule; use Illuminate\Support\Collection; use Pterodactyl\Models\Allocation; +use Pterodactyl\Tests\Integration\TestResponse; use Pterodactyl\Tests\Integration\IntegrationTestCase; use Pterodactyl\Transformers\Api\Client\BaseClientTransformer; @@ -44,6 +45,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/TestResponse.php b/tests/Integration/TestResponse.php new file mode 100644 index 000000000..9a4c6491b --- /dev/null +++ b/tests/Integration/TestResponse.php @@ -0,0 +1,35 @@ +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)) { + dump($this->exception); + } + } + + PHPUnit::assertSame($actual, $status, "Expected status code {$status} but received {$actual}."); + + return $this; + } +} From e8dcd30e0cd3deae468d5351f256c6c3e44b4beb Mon Sep 17 00:00:00 2001 From: Dane Everitt Date: Tue, 19 Jan 2021 21:19:17 -0800 Subject: [PATCH 4/7] [security] fix resources not properly returning an error when they don't match the server in the URL Prior to this fix certain resources were accessible even when their assigned server was not the same as the server in the URL. This causes the resource server relationship to not match the server variable present on the request. Due to this failed logic it was possible for users to access resources they should not have been able to access otherwise for some areas of the panel. --- .../Server/AllocationBelongsToServer.php | 33 ------- .../Client/Server/ResourceBelongsToServer.php | 92 +++++++++++++++++++ .../Client/Server/SubuserBelongsToServer.php | 36 -------- routes/api-client.php | 7 +- 4 files changed, 96 insertions(+), 72 deletions(-) delete mode 100644 app/Http/Middleware/Api/Client/Server/AllocationBelongsToServer.php create mode 100644 app/Http/Middleware/Api/Client/Server/ResourceBelongsToServer.php delete mode 100644 app/Http/Middleware/Api/Client/Server/SubuserBelongsToServer.php 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 d027d563c..000000000 --- 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 000000000..b57fee2e6 --- /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 a80f6eefd..000000000 --- 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/routes/api-client.php b/routes/api-client.php index a00938f55..a82be7151 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'); From 63f945bc3abc8433a3dccdd60e976127c13a5f03 Mon Sep 17 00:00:00 2001 From: Dane Everitt Date: Tue, 19 Jan 2021 21:20:55 -0800 Subject: [PATCH 5/7] Add test coverage to cehck the authorization state of client resources --- database/factories/ModelFactory.php | 17 +++- .../Client/ClientApiIntegrationTestCase.php | 6 ++ .../AllocationAuthorizationTest.php | 63 +++++++++++++++ .../Server/Backup/BackupAuthorizationTest.php | 71 +++++++++++++++++ .../Database/DatabaseAuthorizationTest.php | 78 +++++++++++++++++++ .../Schedule/ScheduleAuthorizationTest.php | 72 +++++++++++++++++ .../Subuser/SubuserAuthorizationTest.php | 61 +++++++++++++++ tests/Integration/TestResponse.php | 4 +- 8 files changed, 369 insertions(+), 3 deletions(-) create mode 100644 tests/Integration/Api/Client/Server/Allocation/AllocationAuthorizationTest.php create mode 100644 tests/Integration/Api/Client/Server/Backup/BackupAuthorizationTest.php create mode 100644 tests/Integration/Api/Client/Server/Database/DatabaseAuthorizationTest.php create mode 100644 tests/Integration/Api/Client/Server/Schedule/ScheduleAuthorizationTest.php create mode 100644 tests/Integration/Api/Client/Server/Subuser/SubuserAuthorizationTest.php diff --git a/database/factories/ModelFactory.php b/database/factories/ModelFactory.php index 4997a9b6f..e6468f5fe 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/tests/Integration/Api/Client/ClientApiIntegrationTestCase.php b/tests/Integration/Api/Client/ClientApiIntegrationTestCase.php index dc12a341b..85c261c2d 100644 --- a/tests/Integration/Api/Client/ClientApiIntegrationTestCase.php +++ b/tests/Integration/Api/Client/ClientApiIntegrationTestCase.php @@ -10,11 +10,14 @@ 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; @@ -26,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(); 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 000000000..e46c46207 --- /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 000000000..b680b5340 --- /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 000000000..aecd71dbc --- /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/ScheduleAuthorizationTest.php b/tests/Integration/Api/Client/Server/Schedule/ScheduleAuthorizationTest.php new file mode 100644 index 000000000..583fb4ae1 --- /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/Subuser/SubuserAuthorizationTest.php b/tests/Integration/Api/Client/Server/Subuser/SubuserAuthorizationTest.php new file mode 100644 index 000000000..f852ed060 --- /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 index 9a4c6491b..a812f37bc 100644 --- a/tests/Integration/TestResponse.php +++ b/tests/Integration/TestResponse.php @@ -3,6 +3,8 @@ namespace Pterodactyl\Tests\Integration; use Illuminate\Testing\Assert as PHPUnit; +use Pterodactyl\Exceptions\DisplayException; +use Illuminate\Validation\ValidationException; use Illuminate\Testing\TestResponse as IlluminateTestResponse; class TestResponse extends IlluminateTestResponse @@ -23,7 +25,7 @@ class TestResponse extends IlluminateTestResponse // to fail so that debugging isn't such a nightmare. if ($actual !== $status && $status !== 500) { $this->dump(); - if (! is_null($this->exception)) { + if (! is_null($this->exception) && ! $this->exception instanceof DisplayException && ! $this->exception instanceof ValidationException) { dump($this->exception); } } From 3053a896f4c6f23a08ff9efff6ce53345eedc673 Mon Sep 17 00:00:00 2001 From: Dane Everitt Date: Tue, 19 Jan 2021 21:45:32 -0800 Subject: [PATCH 6/7] Update CHANGELOG.md --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7f04a6d88..e09ae67f2 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. From 914ee65ded8ab68c49385ff28db1fed9b04a1ed9 Mon Sep 17 00:00:00 2001 From: Dane Everitt Date: Wed, 20 Jan 2021 20:07:52 -0800 Subject: [PATCH 7/7] Don't use a persisted setting when switching users; ref #3021 --- .../scripts/components/dashboard/DashboardContainer.tsx | 5 +++-- resources/scripts/plugins/usePersistedState.ts | 4 ++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/resources/scripts/components/dashboard/DashboardContainer.tsx b/resources/scripts/components/dashboard/DashboardContainer.tsx index b4cb4c290..2edc06056 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/plugins/usePersistedState.ts b/resources/scripts/plugins/usePersistedState.ts index f31985558..007e3fcde 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(() => {