From 910a2d7a2335597dd0a757aea40c77db7bfe6b96 Mon Sep 17 00:00:00 2001 From: Dane Everitt Date: Tue, 3 Nov 2020 19:07:52 -0800 Subject: [PATCH 1/8] Back to checks all around --- .../Servers/BuildModificationServiceTest.php | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/tests/Integration/Services/Servers/BuildModificationServiceTest.php b/tests/Integration/Services/Servers/BuildModificationServiceTest.php index 1a4e81e2..4444cc85 100644 --- a/tests/Integration/Services/Servers/BuildModificationServiceTest.php +++ b/tests/Integration/Services/Servers/BuildModificationServiceTest.php @@ -3,15 +3,12 @@ namespace Pterodactyl\Tests\Integration\Services\Servers; use Mockery; -use Exception; use Pterodactyl\Models\Server; use Pterodactyl\Models\Allocation; use Pterodactyl\Exceptions\DisplayException; -use GuzzleHttp\Exception\BadResponseException; use Pterodactyl\Tests\Integration\IntegrationTestCase; use Pterodactyl\Repositories\Wings\DaemonServerRepository; use Pterodactyl\Services\Servers\BuildModificationService; -use Pterodactyl\Exceptions\Http\Connection\DaemonConnectionException; class BuildModificationServiceTest extends IntegrationTestCase { @@ -114,12 +111,14 @@ class BuildModificationServiceTest extends IntegrationTestCase $this->daemonServerRepository->expects('update')->with(Mockery::on(function ($data) { $this->assertEquals([ - 'memory_limit' => 256, - 'swap' => 128, - 'io_weight' => 600, - 'cpu_limit' => 150, - 'threads' => '1,2', - 'disk_space' => 1024, + 'build' => [ + 'memory_limit' => 256, + 'swap' => 128, + 'io_weight' => 600, + 'cpu_limit' => 150, + 'threads' => '1,2', + 'disk_space' => 1024, + ], ], $data); return true; From c4df5347229e34d00828d7efc2bf27dac60275db Mon Sep 17 00:00:00 2001 From: Dane Everitt Date: Tue, 3 Nov 2020 20:33:05 -0800 Subject: [PATCH 2/8] Handle reconnect events for websocket errors --- app/Services/Nodes/NodeJWTService.php | 2 +- resources/scripts/components/server/WebsocketHandler.tsx | 9 +++++++-- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/app/Services/Nodes/NodeJWTService.php b/app/Services/Nodes/NodeJWTService.php index 6c9dd757..85332a6b 100644 --- a/app/Services/Nodes/NodeJWTService.php +++ b/app/Services/Nodes/NodeJWTService.php @@ -55,7 +55,7 @@ class NodeJWTService $builder = (new Builder)->issuedBy(config('app.url')) ->permittedFor($node->getConnectionAddress()) - ->identifiedBy(hash('sha256', $identifiedBy), true) + ->identifiedBy(md5($identifiedBy), true) ->issuedAt(CarbonImmutable::now()->getTimestamp()) ->canOnlyBeUsedAfter(CarbonImmutable::now()->subMinutes(5)->getTimestamp()); diff --git a/resources/scripts/components/server/WebsocketHandler.tsx b/resources/scripts/components/server/WebsocketHandler.tsx index f53466e0..7a6cfd50 100644 --- a/resources/scripts/components/server/WebsocketHandler.tsx +++ b/resources/scripts/components/server/WebsocketHandler.tsx @@ -7,6 +7,11 @@ import { CSSTransition } from 'react-transition-group'; import Spinner from '@/components/elements/Spinner'; import tw from 'twin.macro'; +const reconnectErrors = [ + 'jwt: exp claim is invalid', + 'jwt: created too far in past (denylist)', +]; + export default () => { let updatingToken = false; const [ error, setError ] = useState<'connecting' | string>(''); @@ -64,7 +69,7 @@ export default () => { setConnectionState(false); console.warn('JWT validation error from wings:', error); - if (error === 'jwt: exp claim is invalid') { + if (reconnectErrors.find(v => error.toLowerCase().indexOf(v) >= 0)) { updateToken(uuid, socket); } else { setError('There was an error validating the credentials provided for the websocket. Please refresh the page.'); @@ -95,7 +100,7 @@ export default () => {

: -

+

{error}

} From 009f9c297de000589778b493c080e3b4d5dcc20a Mon Sep 17 00:00:00 2001 From: Dane Everitt Date: Tue, 3 Nov 2020 21:01:15 -0800 Subject: [PATCH 3/8] Revoke JWT JTIs when modifying a subuser's permissions --- .../Api/Client/Servers/SubuserController.php | 52 ++++++++++++++++--- .../Client/Servers/WebsocketController.php | 2 +- .../Wings/DaemonServerRepository.php | 28 ++++++++-- 3 files changed, 69 insertions(+), 13 deletions(-) diff --git a/app/Http/Controllers/Api/Client/Servers/SubuserController.php b/app/Http/Controllers/Api/Client/Servers/SubuserController.php index d8bdcc40..f51503d0 100644 --- a/app/Http/Controllers/Api/Client/Servers/SubuserController.php +++ b/app/Http/Controllers/Api/Client/Servers/SubuserController.php @@ -3,15 +3,16 @@ namespace Pterodactyl\Http\Controllers\Api\Client\Servers; use Illuminate\Http\Request; -use Pterodactyl\Models\User; use Pterodactyl\Models\Server; -use Pterodactyl\Models\Subuser; use Illuminate\Http\JsonResponse; use Pterodactyl\Models\Permission; +use Illuminate\Support\Facades\Log; use Pterodactyl\Repositories\Eloquent\SubuserRepository; use Pterodactyl\Services\Subusers\SubuserCreationService; +use Pterodactyl\Repositories\Wings\DaemonServerRepository; use Pterodactyl\Transformers\Api\Client\SubuserTransformer; use Pterodactyl\Http\Controllers\Api\Client\ClientApiController; +use Pterodactyl\Exceptions\Http\Connection\DaemonConnectionException; use Pterodactyl\Http\Requests\Api\Client\Servers\Subusers\GetSubuserRequest; use Pterodactyl\Http\Requests\Api\Client\Servers\Subusers\StoreSubuserRequest; use Pterodactyl\Http\Requests\Api\Client\Servers\Subusers\DeleteSubuserRequest; @@ -29,20 +30,28 @@ class SubuserController extends ClientApiController */ private $creationService; + /** + * @var \Pterodactyl\Repositories\Wings\DaemonServerRepository + */ + private $serverRepository; + /** * SubuserController constructor. * * @param \Pterodactyl\Repositories\Eloquent\SubuserRepository $repository * @param \Pterodactyl\Services\Subusers\SubuserCreationService $creationService + * @param \Pterodactyl\Repositories\Wings\DaemonServerRepository $serverRepository */ public function __construct( SubuserRepository $repository, - SubuserCreationService $creationService + SubuserCreationService $creationService, + DaemonServerRepository $serverRepository ) { parent::__construct(); $this->repository = $repository; $this->creationService = $creationService; + $this->serverRepository = $serverRepository; } /** @@ -101,19 +110,38 @@ class SubuserController extends ClientApiController * Update a given subuser in the system for the server. * * @param \Pterodactyl\Http\Requests\Api\Client\Servers\Subusers\UpdateSubuserRequest $request + * @param \Pterodactyl\Models\Server $server * @return array * * @throws \Pterodactyl\Exceptions\Model\DataValidationException * @throws \Pterodactyl\Exceptions\Repository\RecordNotFoundException */ - public function update(UpdateSubuserRequest $request): array + public function update(UpdateSubuserRequest $request, Server $server): array { /** @var \Pterodactyl\Models\Subuser $subuser */ $subuser = $request->attributes->get('subuser'); - $this->repository->update($subuser->id, [ - 'permissions' => $this->getDefaultPermissions($request), - ]); + $permissions = $this->getDefaultPermissions($request); + $current = $subuser->permissions; + + sort($permissions); + sort($current); + + // Only update the database and hit up the Wings instance to invalidate JTI's if the permissions + // have actually changed for the user. + if ($permissions !== $current) { + $this->repository->update($subuser->id, [ + 'permissions' => $this->getDefaultPermissions($request), + ]); + + try { + $this->serverRepository->setServer($server)->revokeJTIs([md5($subuser->user_id . $server->uuid)]); + } catch (DaemonConnectionException $exception) { + // Don't block this request if we can't connect to the Wings instance. Chances are it is + // offline in this event and the token will be invalid anyways once Wings boots back. + Log::warning($exception, ['user_id' => $subuser->user_id, 'server_id' => $server->id]); + } + } return $this->fractal->item($subuser->refresh()) ->transformWith($this->getTransformer(SubuserTransformer::class)) @@ -124,15 +152,23 @@ class SubuserController extends ClientApiController * Removes a subusers from a server's assignment. * * @param \Pterodactyl\Http\Requests\Api\Client\Servers\Subusers\DeleteSubuserRequest $request + * @param \Pterodactyl\Models\Server $server * @return \Illuminate\Http\JsonResponse */ - public function delete(DeleteSubuserRequest $request) + public function delete(DeleteSubuserRequest $request, Server $server) { /** @var \Pterodactyl\Models\Subuser $subuser */ $subuser = $request->attributes->get('subuser'); $this->repository->delete($subuser->id); + try { + $this->serverRepository->revokeJTIs([md5($subuser->user_id . $server->uuid)]); + } catch (DaemonConnectionException $exception) { + // Don't block this request if we can't connect to the Wings instance. + Log::warning($exception, ['user_id' => $subuser->user_id, 'server_id' => $server->id]); + } + return new JsonResponse([], JsonResponse::HTTP_NO_CONTENT); } diff --git a/app/Http/Controllers/Api/Client/Servers/WebsocketController.php b/app/Http/Controllers/Api/Client/Servers/WebsocketController.php index a176f66f..f18b4763 100644 --- a/app/Http/Controllers/Api/Client/Servers/WebsocketController.php +++ b/app/Http/Controllers/Api/Client/Servers/WebsocketController.php @@ -59,7 +59,7 @@ class WebsocketController extends ClientApiController } $token = $this->jwtService - ->setExpiresAt(CarbonImmutable::now()->addMinutes(15)) + ->setExpiresAt(CarbonImmutable::now()->addMinutes(10)) ->setClaims([ 'user_id' => $request->user()->id, 'server_uuid' => $server->uuid, diff --git a/app/Repositories/Wings/DaemonServerRepository.php b/app/Repositories/Wings/DaemonServerRepository.php index abb5dae4..22c90d6c 100644 --- a/app/Repositories/Wings/DaemonServerRepository.php +++ b/app/Repositories/Wings/DaemonServerRepository.php @@ -126,11 +126,10 @@ class DaemonServerRepository extends DaemonRepository } /** - * Requests the daemon to create a full archive of the server. - * Once the daemon is finished they will send a POST request to - * "/api/remote/servers/{uuid}/archive" with a boolean. + * Requests the daemon to create a full archive of the server. Once the daemon is finished + * they will send a POST request to "/api/remote/servers/{uuid}/archive" with a boolean. * - * @throws DaemonConnectionException + * @throws \Pterodactyl\Exceptions\Http\Connection\DaemonConnectionException */ public function requestArchive(): void { @@ -144,4 +143,25 @@ class DaemonServerRepository extends DaemonRepository throw new DaemonConnectionException($exception); } } + + /** + * Revokes an array of JWT JTI's by marking any token generated before the current time on + * the Wings instance as being invalid. + * + * @param array $jtis + * @throws \Pterodactyl\Exceptions\Http\Connection\DaemonConnectionException + */ + public function revokeJTIs(array $jtis): void + { + Assert::isInstanceOf($this->server, Server::class); + + try { + $this->getHttpClient() + ->post(sprintf('/api/servers/%s/ws/deny', $this->server->uuid), [ + 'json' => ['jtis' => $jtis], + ]); + } catch (TransferException $exception) { + throw new DaemonConnectionException($exception); + } + } } From 9c332889021532ae3934968c10653d70c5e86642 Mon Sep 17 00:00:00 2001 From: Daave Date: Fri, 6 Nov 2020 13:21:09 +0000 Subject: [PATCH 4/8] Change entrypoint so it doesn't overwrite app key. APP_KEY is overwritten by a generated one even if it's in the environment. --- docker/entrypoint.sh | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/docker/entrypoint.sh b/docker/entrypoint.sh index 84cf1957..42379a3f 100644 --- a/docker/entrypoint.sh +++ b/docker/entrypoint.sh @@ -20,10 +20,15 @@ else touch /app/var/.env ## manually generate a key because key generate --force fails - echo -e "Generating key." - APP_KEY=$(cat /dev/urandom | tr -dc 'a-zA-Z0-9' | fold -w 32 | head -n 1) - echo -e "Generated app key: $APP_KEY" - echo -e "APP_KEY=$APP_KEY" > /app/var/.env + if [ -z "$APP_KEY" ]; then + echo -e "Generating key." + APP_KEY=$(cat /dev/urandom | tr -dc 'a-zA-Z0-9' | fold -w 32 | head -n 1) + echo -e "Generated app key: $APP_KEY" + echo -e "APP_KEY=$APP_KEY" > /app/var/.env + else + echo -e "APP_KEY exists in environment, using that." + echo -e "APP_KEY=$APP_KEY" > /app/var/.env + fi ln -s /app/var/.env /app/ fi @@ -77,4 +82,4 @@ yarn add cross-env yarn run build:production echo -e "Starting supervisord." -exec "$@" \ No newline at end of file +exec "$@" From 80574e991b1e953c2cfbe43327d65713d8b57395 Mon Sep 17 00:00:00 2001 From: Daave Date: Fri, 6 Nov 2020 13:23:05 +0000 Subject: [PATCH 5/8] Remove quotes and fix indentation --- docker/entrypoint.sh | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/docker/entrypoint.sh b/docker/entrypoint.sh index 42379a3f..f4e1971b 100644 --- a/docker/entrypoint.sh +++ b/docker/entrypoint.sh @@ -20,11 +20,11 @@ else touch /app/var/.env ## manually generate a key because key generate --force fails - if [ -z "$APP_KEY" ]; then - echo -e "Generating key." - APP_KEY=$(cat /dev/urandom | tr -dc 'a-zA-Z0-9' | fold -w 32 | head -n 1) - echo -e "Generated app key: $APP_KEY" - echo -e "APP_KEY=$APP_KEY" > /app/var/.env + if [ -z $APP_KEY ]; then + echo -e "Generating key." + APP_KEY=$(cat /dev/urandom | tr -dc 'a-zA-Z0-9' | fold -w 32 | head -n 1) + echo -e "Generated app key: $APP_KEY" + echo -e "APP_KEY=$APP_KEY" > /app/var/.env else echo -e "APP_KEY exists in environment, using that." echo -e "APP_KEY=$APP_KEY" > /app/var/.env From 625fd92130c1227c9a2c9102b6c0c5e9733f4243 Mon Sep 17 00:00:00 2001 From: Dane Everitt Date: Fri, 6 Nov 2020 20:47:03 -0800 Subject: [PATCH 6/8] Fix URKL encoding hellscape; closes #2664 closes #2663 --- .../Api/Client/Servers/FileController.php | 23 +++++++++++++++---- .../api/server/files/getFileContents.ts | 2 +- .../scripts/api/server/files/loadDirectory.ts | 2 +- .../api/server/files/saveFileContents.ts | 2 +- .../server/files/FileEditContainer.tsx | 2 +- .../server/files/FileManagerBreadcrumbs.tsx | 6 ++--- .../server/files/FileManagerContainer.tsx | 2 +- .../components/server/files/FileObjectRow.tsx | 6 +++-- .../server/files/NewDirectoryButton.tsx | 4 +--- 9 files changed, 32 insertions(+), 17 deletions(-) diff --git a/app/Http/Controllers/Api/Client/Servers/FileController.php b/app/Http/Controllers/Api/Client/Servers/FileController.php index a175f390..9e705651 100644 --- a/app/Http/Controllers/Api/Client/Servers/FileController.php +++ b/app/Http/Controllers/Api/Client/Servers/FileController.php @@ -6,6 +6,7 @@ use Carbon\CarbonImmutable; use Illuminate\Http\Response; use Pterodactyl\Models\Server; use Illuminate\Http\JsonResponse; +use Illuminate\Support\Collection; use Pterodactyl\Services\Nodes\NodeJWTService; use Illuminate\Contracts\Routing\ResponseFactory; use Pterodactyl\Repositories\Wings\DaemonFileRepository; @@ -70,7 +71,7 @@ class FileController extends ClientApiController { $contents = $this->fileRepository ->setServer($server) - ->getDirectory(urlencode(urldecode($request->get('directory') ?? '/'))); + ->getDirectory($this->encode($request->get('directory') ?? '/')); return $this->fractal->collection($contents) ->transformWith($this->getTransformer(FileObjectTransformer::class)) @@ -91,7 +92,7 @@ class FileController extends ClientApiController { return new Response( $this->fileRepository->setServer($server)->getContent( - urlencode(urldecode($request->get('file'))), config('pterodactyl.files.max_edit_size') + $this->encode($request->get('file')), config('pterodactyl.files.max_edit_size') ), Response::HTTP_OK, ['Content-Type' => 'text/plain'] @@ -113,7 +114,7 @@ class FileController extends ClientApiController $token = $this->jwtService ->setExpiresAt(CarbonImmutable::now()->addMinutes(15)) ->setClaims([ - 'file_path' => $request->get('file'), + 'file_path' => rawurldecode($request->get('file')), 'server_uuid' => $server->uuid, ]) ->handle($server->node, $request->user()->id . $server->uuid); @@ -142,7 +143,7 @@ class FileController extends ClientApiController public function write(WriteFileContentRequest $request, Server $server): JsonResponse { $this->fileRepository->setServer($server)->putContent( - $request->get('file'), + $this->encode($request->get('file')), $request->getContent() ); @@ -261,4 +262,18 @@ class FileController extends ClientApiController return new JsonResponse([], Response::HTTP_NO_CONTENT); } + + /** + * Encodes a given file name & path in a format that should work for a good majority + * of file names without too much confusing logic. + * + * @param string $path + * @return string + */ + private function encode(string $path): string + { + return Collection::make(explode('/', rawurldecode($path)))->map(function ($value) { + return rawurlencode($value); + })->join('/'); + } } diff --git a/resources/scripts/api/server/files/getFileContents.ts b/resources/scripts/api/server/files/getFileContents.ts index cec8788b..da380362 100644 --- a/resources/scripts/api/server/files/getFileContents.ts +++ b/resources/scripts/api/server/files/getFileContents.ts @@ -3,7 +3,7 @@ import http from '@/api/http'; export default (server: string, file: string): Promise => { return new Promise((resolve, reject) => { http.get(`/api/client/servers/${server}/files/contents`, { - params: { file: file.split('/').map(item => encodeURIComponent(item)).join('/') }, + params: { file: encodeURI(decodeURI(file)) }, transformResponse: res => res, responseType: 'text', }) diff --git a/resources/scripts/api/server/files/loadDirectory.ts b/resources/scripts/api/server/files/loadDirectory.ts index 1bfd7877..d29cc160 100644 --- a/resources/scripts/api/server/files/loadDirectory.ts +++ b/resources/scripts/api/server/files/loadDirectory.ts @@ -17,7 +17,7 @@ export interface FileObject { export default async (uuid: string, directory?: string): Promise => { const { data } = await http.get(`/api/client/servers/${uuid}/files/list`, { - params: { directory: directory?.split('/').map(item => encodeURIComponent(item)).join('/') }, + params: { directory: encodeURI(directory ?? '/') }, }); return (data.data || []).map(rawDataToFileObject); diff --git a/resources/scripts/api/server/files/saveFileContents.ts b/resources/scripts/api/server/files/saveFileContents.ts index b97e60a6..7f6f44ef 100644 --- a/resources/scripts/api/server/files/saveFileContents.ts +++ b/resources/scripts/api/server/files/saveFileContents.ts @@ -2,7 +2,7 @@ import http from '@/api/http'; export default async (uuid: string, file: string, content: string): Promise => { await http.post(`/api/client/servers/${uuid}/files/write`, content, { - params: { file }, + params: { file: encodeURI(decodeURI(file)) }, headers: { 'Content-Type': 'text/plain', }, diff --git a/resources/scripts/components/server/files/FileEditContainer.tsx b/resources/scripts/components/server/files/FileEditContainer.tsx index 8305e04b..4dd519f8 100644 --- a/resources/scripts/components/server/files/FileEditContainer.tsx +++ b/resources/scripts/components/server/files/FileEditContainer.tsx @@ -61,7 +61,7 @@ export default () => { setLoading(true); clearFlashes('files:view'); fetchFileContent() - .then(content => saveFileContents(uuid, encodeURIComponent(name || hash.replace(/^#/, '')), content)) + .then(content => saveFileContents(uuid, name || hash.replace(/^#/, ''), content)) .then(() => { if (name) { history.push(`/server/${id}/files/edit#/${name}`); diff --git a/resources/scripts/components/server/files/FileManagerBreadcrumbs.tsx b/resources/scripts/components/server/files/FileManagerBreadcrumbs.tsx index 968a651d..11b7abde 100644 --- a/resources/scripts/components/server/files/FileManagerBreadcrumbs.tsx +++ b/resources/scripts/components/server/files/FileManagerBreadcrumbs.tsx @@ -33,10 +33,10 @@ export default ({ withinFileEditor, isNewFile }: Props) => { .filter(directory => !!directory) .map((directory, index, dirs) => { if (!withinFileEditor && index === dirs.length - 1) { - return { name: decodeURIComponent(encodeURIComponent(directory)) }; + return { name: directory }; } - return { name: decodeURIComponent(encodeURIComponent(directory)), path: `/${dirs.slice(0, index + 1).join('/')}` }; + return { name: directory, path: `/${dirs.slice(0, index + 1).join('/')}` }; }); const onSelectAllClick = (e: React.ChangeEvent) => { @@ -79,7 +79,7 @@ export default ({ withinFileEditor, isNewFile }: Props) => { } {file && - {decodeURIComponent(encodeURIComponent(file))} + {decodeURI(file)} } diff --git a/resources/scripts/components/server/files/FileManagerContainer.tsx b/resources/scripts/components/server/files/FileManagerContainer.tsx index 31985e94..6b274f96 100644 --- a/resources/scripts/components/server/files/FileManagerContainer.tsx +++ b/resources/scripts/components/server/files/FileManagerContainer.tsx @@ -36,7 +36,7 @@ export default () => { useEffect(() => { clearFlashes('files'); setSelectedFiles([]); - setDirectory(hash.length > 0 ? hash : '/'); + setDirectory(hash.length > 0 ? decodeURI(hash) : '/'); }, [ hash ]); useEffect(() => { diff --git a/resources/scripts/components/server/files/FileObjectRow.tsx b/resources/scripts/components/server/files/FileObjectRow.tsx index 26c37ed2..f4ac0621 100644 --- a/resources/scripts/components/server/files/FileObjectRow.tsx +++ b/resources/scripts/components/server/files/FileObjectRow.tsx @@ -24,6 +24,8 @@ const Clickable: React.FC<{ file: FileObject }> = memo(({ file, children }) => { const history = useHistory(); const match = useRouteMatch(); + const destination = cleanDirectoryPath(`${directory}/${file.name}`).split('/').map(v => encodeURI(v)).join('/'); + const onRowClick = (e: React.MouseEvent) => { // Don't rely on the onClick to work with the generated URL. Because of the way this // component re-renders you'll get redirected into a nested directory structure since @@ -32,7 +34,7 @@ const Clickable: React.FC<{ file: FileObject }> = memo(({ file, children }) => { // Just trust me future me, leave this be. if (!file.isFile) { e.preventDefault(); - history.push(`#${cleanDirectoryPath(`${directory}/${file.name}`)}`); + history.push(`#${destination}`); } }; @@ -43,7 +45,7 @@ const Clickable: React.FC<{ file: FileObject }> = memo(({ file, children }) => { : diff --git a/resources/scripts/components/server/files/NewDirectoryButton.tsx b/resources/scripts/components/server/files/NewDirectoryButton.tsx index 0056cf68..dc91377b 100644 --- a/resources/scripts/components/server/files/NewDirectoryButton.tsx +++ b/resources/scripts/components/server/files/NewDirectoryButton.tsx @@ -92,9 +92,7 @@ export default ({ className }: WithClassname) => { This directory will be created as  /home/container/ - {decodeURIComponent(encodeURIComponent( - join(directory, values.directoryName).replace(/^(\.\.\/|\/)+/, ''), - ))} + {join(directory, values.directoryName).replace(/^(\.\.\/|\/)+/, '')}

From f99ac0ecdec86184c39c5f124f7a3659dc61375e Mon Sep 17 00:00:00 2001 From: Dane Everitt Date: Fri, 6 Nov 2020 22:33:39 -0800 Subject: [PATCH 7/8] Fix some failing test cases --- .../Api/Client/Servers/SubuserController.php | 2 +- .../Api/Client/Server/Subuser/DeleteSubuserTest.php | 12 ++++++++++-- .../Api/Client/Server/WebsocketControllerTest.php | 2 +- 3 files changed, 12 insertions(+), 4 deletions(-) diff --git a/app/Http/Controllers/Api/Client/Servers/SubuserController.php b/app/Http/Controllers/Api/Client/Servers/SubuserController.php index f51503d0..d2806a65 100644 --- a/app/Http/Controllers/Api/Client/Servers/SubuserController.php +++ b/app/Http/Controllers/Api/Client/Servers/SubuserController.php @@ -163,7 +163,7 @@ class SubuserController extends ClientApiController $this->repository->delete($subuser->id); try { - $this->serverRepository->revokeJTIs([md5($subuser->user_id . $server->uuid)]); + $this->serverRepository->setServer($server)->revokeJTIs([md5($subuser->user_id . $server->uuid)]); } catch (DaemonConnectionException $exception) { // Don't block this request if we can't connect to the Wings instance. Log::warning($exception, ['user_id' => $subuser->user_id, 'server_id' => $server->id]); diff --git a/tests/Integration/Api/Client/Server/Subuser/DeleteSubuserTest.php b/tests/Integration/Api/Client/Server/Subuser/DeleteSubuserTest.php index 3118baa2..144db42d 100644 --- a/tests/Integration/Api/Client/Server/Subuser/DeleteSubuserTest.php +++ b/tests/Integration/Api/Client/Server/Subuser/DeleteSubuserTest.php @@ -2,10 +2,12 @@ namespace Pterodactyl\Tests\Integration\Api\Client\Server\Subuser; +use Mockery; use Ramsey\Uuid\Uuid; use Pterodactyl\Models\User; use Pterodactyl\Models\Subuser; use Pterodactyl\Models\Permission; +use Pterodactyl\Repositories\Wings\DaemonServerRepository; use Pterodactyl\Tests\Integration\Api\Client\ClientApiIntegrationTestCase; class DeleteSubuserTest extends ClientApiIntegrationTestCase @@ -23,6 +25,8 @@ class DeleteSubuserTest extends ClientApiIntegrationTestCase */ public function testCorrectSubuserIsDeletedFromServer() { + $this->swap(DaemonServerRepository::class, $mock = Mockery::mock(DaemonServerRepository::class)); + [$user, $server] = $this->generateTestAccount(); /** @var \Pterodactyl\Models\User $differentUser */ @@ -37,9 +41,11 @@ class DeleteSubuserTest extends ClientApiIntegrationTestCase Subuser::query()->forceCreate([ 'user_id' => $subuser->id, 'server_id' => $server->id, - 'permissions' => [ Permission::ACTION_WEBSOCKET_CONNECT ], + 'permissions' => [Permission::ACTION_WEBSOCKET_CONNECT], ]); + $mock->expects('setServer->revokeJTIs')->with([md5($subuser->id . $server->uuid)])->andReturnUndefined(); + $this->actingAs($user)->deleteJson($this->link($server) . "/users/{$subuser->uuid}")->assertNoContent(); // Try the same test, but this time with a UUID that if cast to an int (shouldn't) line up with @@ -51,9 +57,11 @@ class DeleteSubuserTest extends ClientApiIntegrationTestCase Subuser::query()->forceCreate([ 'user_id' => $subuser->id, 'server_id' => $server->id, - 'permissions' => [ Permission::ACTION_WEBSOCKET_CONNECT ], + 'permissions' => [Permission::ACTION_WEBSOCKET_CONNECT], ]); + $mock->expects('setServer->revokeJTIs')->with([md5($subuser->id . $server->uuid)])->andReturnUndefined(); + $this->actingAs($user)->deleteJson($this->link($server) . "/users/{$subuser->uuid}")->assertNoContent(); } } diff --git a/tests/Integration/Api/Client/Server/WebsocketControllerTest.php b/tests/Integration/Api/Client/Server/WebsocketControllerTest.php index 0d8851d3..734dfcf2 100644 --- a/tests/Integration/Api/Client/Server/WebsocketControllerTest.php +++ b/tests/Integration/Api/Client/Server/WebsocketControllerTest.php @@ -63,7 +63,7 @@ class WebsocketControllerTest extends ClientApiIntegrationTestCase $this->assertSame($server->node->getConnectionAddress(), $token->getClaim('aud')); $this->assertSame(CarbonImmutable::now()->getTimestamp(), $token->getClaim('iat')); $this->assertSame(CarbonImmutable::now()->subMinutes(5)->getTimestamp(), $token->getClaim('nbf')); - $this->assertSame(CarbonImmutable::now()->addMinutes(15)->getTimestamp(), $token->getClaim('exp')); + $this->assertSame(CarbonImmutable::now()->addMinutes(10)->getTimestamp(), $token->getClaim('exp')); $this->assertSame($user->id, $token->getClaim('user_id')); $this->assertSame($server->uuid, $token->getClaim('server_uuid')); $this->assertSame(['*'], $token->getClaim('permissions')); From c20d53bb174539c9ca6bf6ed77b44a9cd0b3fe37 Mon Sep 17 00:00:00 2001 From: Dane Everitt Date: Sat, 7 Nov 2020 09:57:53 -0800 Subject: [PATCH 8/8] Always return the primary allocation for a server, even without the allocation permissions --- app/Models/Permission.php | 2 +- .../Api/Client/ServerTransformer.php | 20 +++++++++---- .../Api/Client/ClientControllerTest.php | 28 +++++++++++++++++++ 3 files changed, 43 insertions(+), 7 deletions(-) diff --git a/app/Models/Permission.php b/app/Models/Permission.php index 96429f31..d18e7200 100644 --- a/app/Models/Permission.php +++ b/app/Models/Permission.php @@ -163,7 +163,7 @@ class Permission extends Model 'allocation' => [ 'description' => 'Permissions that control a user\'s ability to modify the port allocations for this server.', 'keys' => [ - 'read' => 'Allows a user to view the allocations assigned to this server.', + 'read' => 'Allows a user to view all allocations currently assigned to this server. Users with any level of access to this server can always view the primary allocation.', 'create' => 'Allows a user to assign additional allocations to the server.', 'update' => 'Allows a user to change the primary server allocation and attach notes to each allocation.', 'delete' => 'Allows a user to delete an allocation from the server.', diff --git a/app/Transformers/Api/Client/ServerTransformer.php b/app/Transformers/Api/Client/ServerTransformer.php index a787246b..ac3ab26b 100644 --- a/app/Transformers/Api/Client/ServerTransformer.php +++ b/app/Transformers/Api/Client/ServerTransformer.php @@ -83,15 +83,23 @@ class ServerTransformer extends BaseClientTransformer */ public function includeAllocations(Server $server) { + $transformer = $this->makeTransformer(AllocationTransformer::class); + + // While we include this permission, we do need to actually handle it slightly different here + // for the purpose of keeping things functionally working. If the user doesn't have read permissions + // for the allocations we'll only return the primary server allocation, and any notes associated + // with it will be hidden. + // + // This allows us to avoid too much permission regression, without also hiding information that + // is generally needed for the frontend to make sense when browsing or searching results. if (! $this->getUser()->can(Permission::ACTION_ALLOCATION_READ, $server)) { - return $this->null(); + $primary = clone $server->allocation; + $primary->notes = null; + + return $this->collection([$primary], $transformer, Allocation::RESOURCE_NAME); } - return $this->collection( - $server->allocations, - $this->makeTransformer(AllocationTransformer::class), - Allocation::RESOURCE_NAME - ); + return $this->collection($server->allocations, $transformer, Allocation::RESOURCE_NAME); } /** diff --git a/tests/Integration/Api/Client/ClientControllerTest.php b/tests/Integration/Api/Client/ClientControllerTest.php index 30d83781..fc8c2c1e 100644 --- a/tests/Integration/Api/Client/ClientControllerTest.php +++ b/tests/Integration/Api/Client/ClientControllerTest.php @@ -304,6 +304,34 @@ class ClientControllerTest extends ClientApiIntegrationTestCase $response->assertJsonCount(0, 'data'); } + /** + * Test that a subuser without the allocation.read permission is only able to see the primary + * allocation for the server. + */ + public function testOnlyPrimaryAllocationIsReturnedToSubuser() + { + /** @var \Pterodactyl\Models\Server $server */ + [$user, $server] = $this->generateTestAccount([Permission::ACTION_WEBSOCKET_CONNECT]); + $server->allocation->notes = 'Test notes'; + $server->allocation->save(); + + factory(Allocation::class)->times(2)->create([ + 'node_id' => $server->node_id, + 'server_id' => $server->id, + ]); + + $server->refresh(); + $response = $this->actingAs($user)->getJson('/api/client'); + + $response->assertOk(); + $response->assertJsonCount(1, 'data'); + $response->assertJsonPath('data.0.attributes.server_owner', false); + $response->assertJsonPath('data.0.attributes.uuid', $server->uuid); + $response->assertJsonCount(1, 'data.0.attributes.relationships.allocations.data'); + $response->assertJsonPath('data.0.attributes.relationships.allocations.data.0.attributes.id', $server->allocation->id); + $response->assertJsonPath('data.0.attributes.relationships.allocations.data.0.attributes.notes', null); + } + /** * @return array */