Skip to content

Commit f0ac072

Browse files
DaneEverittDaneEveritt
committed
[Security] Don't return all servers on the system when not a root admin and admin level servers are requested
Cleaned up the API endpoint by simplifying the logic and adds test case to cover this bug. If you ever need to list _all_ of the servers on the system you should be using the application API endpoint for the servers most likely.
1 parent 24db6d9 commit f0ac072

File tree

6 files changed

+84
-88
lines changed

6 files changed

+84
-88
lines changed

‎app/Http/Controllers/Api/Client/ClientController.php‎

Lines changed: 15 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@
22

33
namespace Pterodactyl\Http\Controllers\Api\Client;
44

5-
use Pterodactyl\Models\User;
65
use Pterodactyl\Models\Server;
76
use Pterodactyl\Models\Permission;
87
use Spatie\QueryBuilder\QueryBuilder;
@@ -39,31 +38,27 @@ public function __construct(ServerRepository $repository)
3938
public function index(GetServersRequest $request): array
4039
{
4140
$user = $request->user();
42-
$level = $request->getFilterLevel();
4341
$transformer = $this->getTransformer(ServerTransformer::class);
4442

4543
// Start the query builder and ensure we eager load any requested relationships from the request.
46-
$builder = Server::query()->with($this->getIncludesForTransformer($transformer, ['node']));
44+
$builder = QueryBuilder::for(
45+
Server::query()->with($this->getIncludesForTransformer($transformer, ['node']))
46+
)->allowedFilters('uuid', 'name', 'external_id');
4747

48-
if ($level === User::FILTER_LEVEL_OWNER) {
49-
$builder = $builder->where('owner_id', $request->user()->id);
50-
}
51-
// If set to all, display all servers they can access, including those they access as an
52-
// admin. If set to subuser, only return the servers they can access because they are owner,
53-
// or marked as a subuser of the server.
54-
elseif (($level === User::FILTER_LEVEL_ALL && ! $user->root_admin) || $level === User::FILTER_LEVEL_SUBUSER) {
48+
// Either return all of the servers the user has access to because they are an admin `?type=admin` or
49+
// just return all of the servers the user has access to because they are the owner or a subuser of the
50+
// server.
51+
if ($request->input('type') === 'admin') {
52+
$builder = $user->root_admin
53+
? $builder->whereNotIn('id', $user->accessibleServers()->pluck('id')->all())
54+
// If they aren't an admin but want all the admin servers don't fail the request, just
55+
// make it a query that will never return any results back.
56+
: $builder->whereRaw('1 = 2');
57+
} elseif ($request->input('type') === 'owner') {
58+
$builder = $builder->where('owner_id', $user->id);
59+
} else {
5560
$builder = $builder->whereIn('id', $user->accessibleServers()->pluck('id')->all());
5661
}
57-
// If set to admin, only display the servers a user can access because they are an administrator.
58-
// This means only servers the user would not have access to if they were not an admin (because they
59-
// are not an owner or subuser) are returned.
60-
elseif ($level === User::FILTER_LEVEL_ADMIN && $user->root_admin) {
61-
$builder = $builder->whereNotIn('id', $user->accessibleServers()->pluck('id')->all());
62-
}
63-
64-
$builder = QueryBuilder::for($builder)->allowedFilters(
65-
'uuid', 'name', 'external_id'
66-
);
6762

6863
$servers = $builder->paginate(min($request->query('per_page', 50), 100))->appends($request->query());
6964

‎app/Http/Requests/Api/Client/GetServersRequest.php‎

Lines changed: 0 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,6 @@
22

33
namespace Pterodactyl\Http\Requests\Api\Client;
44

5-
use Pterodactyl\Models\User;
6-
75
class GetServersRequest extends ClientApiRequest
86
{
97
/**
@@ -13,28 +11,4 @@ public function authorize(): bool
1311
{
1412
return true;
1513
}
16-
17-
/**
18-
* Return the filtering method for servers when the client base endpoint is requested.
19-
*
20-
* @return int
21-
*/
22-
public function getFilterLevel(): int
23-
{
24-
switch ($this->input('type')) {
25-
case 'all':
26-
return User::FILTER_LEVEL_ALL;
27-
break;
28-
case 'admin':
29-
return User::FILTER_LEVEL_ADMIN;
30-
break;
31-
case 'owner':
32-
return User::FILTER_LEVEL_OWNER;
33-
break;
34-
case 'subuser-of':
35-
default:
36-
return User::FILTER_LEVEL_SUBUSER;
37-
break;
38-
}
39-
}
4014
}

‎app/Models/User.php‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -57,11 +57,6 @@ class User extends Model implements
5757
const USER_LEVEL_USER = 0;
5858
const USER_LEVEL_ADMIN = 1;
5959

60-
const FILTER_LEVEL_ALL = 0;
61-
const FILTER_LEVEL_OWNER = 1;
62-
const FILTER_LEVEL_ADMIN = 2;
63-
const FILTER_LEVEL_SUBUSER = 3;
64-
6560
/**
6661
* The resource name for this model when it is transformed into an
6762
* API representation using fractal.

‎resources/scripts/api/getServers.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,14 +4,14 @@ import http, { getPaginationSet, PaginatedResult } from '@/api/http';
44
interface QueryParams {
55
query?: string;
66
page?: number;
7-
includeAdmin?: boolean;
7+
onlyAdmin?: boolean;
88
}
99

10-
export default ({ query, page = 1, includeAdmin = false }: QueryParams): Promise<PaginatedResult<Server>> => {
10+
export default ({ query, page = 1, onlyAdmin = false }: QueryParams): Promise<PaginatedResult<Server>> => {
1111
return new Promise((resolve, reject) => {
1212
http.get('/api/client', {
1313
params: {
14-
type: includeAdmin ? 'all' : undefined,
14+
type: onlyAdmin ? 'admin' : undefined,
1515
'filter[name]': query,
1616
page,
1717
},

‎resources/scripts/components/dashboard/DashboardContainer.tsx‎

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -17,11 +17,11 @@ export default () => {
1717
const { clearFlashes, clearAndAddHttpError } = useFlash();
1818
const [ page, setPage ] = useState(1);
1919
const { rootAdmin } = useStoreState(state => state.user.data!);
20-
const [ includeAdmin, setIncludeAdmin ] = usePersistedState('show_all_servers', false);
20+
const [ showOnlyAdmin, setShowOnlyAdmin ] = usePersistedState('show_all_servers', false);
2121

2222
const { data: servers, error } = useSWR<PaginatedResult<Server>>(
23-
[ '/api/client/servers', includeAdmin, page ],
24-
() => getServers({ includeAdmin, page }),
23+
[ '/api/client/servers', showOnlyAdmin, page ],
24+
() => getServers({ onlyAdmin: showOnlyAdmin, page }),
2525
);
2626

2727
useEffect(() => {
@@ -34,12 +34,12 @@ export default () => {
3434
{rootAdmin &&
3535
<div css={tw`mb-2 flex justify-end items-center`}>
3636
<p css={tw`uppercase text-xs text-neutral-400 mr-2`}>
37-
{includeAdmin ? 'Showing all servers' : 'Showing your servers'}
37+
{showOnlyAdmin ? 'Showing other\'s servers' : 'Showing your servers'}
3838
</p>
3939
<Switch
4040
name={'show_all_servers'}
41-
defaultChecked={includeAdmin}
42-
onChange={() => setIncludeAdmin(s => !s)}
41+
defaultChecked={showOnlyAdmin}
42+
onChange={() => setShowOnlyAdmin(s => !s)}
4343
/>
4444
</div>
4545
}
@@ -58,7 +58,11 @@ export default () => {
5858
))
5959
:
6060
<p css={tw`text-center text-sm text-neutral-400`}>
61-
There are no servers associated with your account.
61+
{showOnlyAdmin ?
62+
'There are no other servers to display.'
63+
:
64+
'There are no servers associated with your account.'
65+
}
6266
</p>
6367
)}
6468
</Pagination>

‎tests/Integration/Api/Client/ClientControllerTest.php‎

Lines changed: 55 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -38,33 +38,6 @@ public function testOnlyLoggedInUsersServersAreReturned()
3838
$response->assertJsonPath('meta.pagination.per_page', 50);
3939
}
4040

41-
/**
42-
* Tests that all of the servers on the system are returned when making the request as an
43-
* administrator and including the ?filter=all parameter in the URL.
44-
*/
45-
public function testFilterIncludeAllServersWhenAdministrator()
46-
{
47-
/** @var \Pterodactyl\Models\User[] $users */
48-
$users = factory(User::class)->times(3)->create();
49-
$users[0]->root_admin = true;
50-
51-
$servers = [
52-
$this->createServerModel(['user_id' => $users[0]->id]),
53-
$this->createServerModel(['user_id' => $users[1]->id]),
54-
$this->createServerModel(['user_id' => $users[2]->id]),
55-
];
56-
57-
$response = $this->actingAs($users[0])->getJson('/api/client?type=all');
58-
59-
$response->assertOk();
60-
$response->assertJsonCount(3, 'data');
61-
62-
for ($i = 0; $i < 3; $i++) {
63-
$response->assertJsonPath("data.{$i}.attributes.server_owner", $i === 0);
64-
$response->assertJsonPath("data.{$i}.attributes.identifier", $servers[$i]->uuidShort);
65-
}
66-
}
67-
6841
/**
6942
* Test that servers where the user is a subuser are returned by default in the API call.
7043
*/
@@ -143,4 +116,59 @@ public function testPermissionsAreReturned()
143116
],
144117
]);
145118
}
119+
120+
/**
121+
* Test that only servers a user can access because they are an administrator are returned. This
122+
* will always exclude any servers they can see because they're the owner or a subuser of the server.
123+
*/
124+
public function testOnlyAdminLevelServersAreReturned()
125+
{
126+
/** @var \Pterodactyl\Models\User[] $users */
127+
$users = factory(User::class)->times(4)->create();
128+
$users[0]->update(['root_admin' => true]);
129+
130+
$servers = [
131+
$this->createServerModel(['user_id' => $users[0]->id]),
132+
$this->createServerModel(['user_id' => $users[1]->id]),
133+
$this->createServerModel(['user_id' => $users[2]->id]),
134+
$this->createServerModel(['user_id' => $users[3]->id]),
135+
];
136+
137+
Subuser::query()->create([
138+
'user_id' => $users[0]->id,
139+
'server_id' => $servers[1]->id,
140+
'permissions' => [Permission::ACTION_WEBSOCKET_CONNECT],
141+
]);
142+
143+
// Only servers 2 & 3 (0 indexed) should be returned by the API at this point. The user making
144+
// the request is the owner of server 0, and a subuser of server 1 so they should be exluded.
145+
$response = $this->actingAs($users[0])->getJson('/api/client?type=admin');
146+
147+
$response->assertOk();
148+
$response->assertJsonCount(2, 'data');
149+
150+
$response->assertJsonPath('data.0.attributes.server_owner', false);
151+
$response->assertJsonPath('data.0.attributes.identifier', $servers[2]->uuidShort);
152+
$response->assertJsonPath('data.1.attributes.server_owner', false);
153+
$response->assertJsonPath('data.1.attributes.identifier', $servers[3]->uuidShort);
154+
}
155+
156+
/**
157+
* Test that no servers get returned if the user requests all admin level servers by using
158+
* ?type=admin in the request.
159+
*/
160+
public function testNoServersAreReturnedIfAdminFilterIsPassedByRegularUser()
161+
{
162+
/** @var \Pterodactyl\Models\User[] $users */
163+
$users = factory(User::class)->times(3)->create();
164+
165+
$this->createServerModel(['user_id' => $users[0]->id]);
166+
$this->createServerModel(['user_id' => $users[1]->id]);
167+
$this->createServerModel(['user_id' => $users[2]->id]);
168+
169+
$response = $this->actingAs($users[0])->getJson('/api/client?type=admin');
170+
171+
$response->assertOk();
172+
$response->assertJsonCount(0, 'data');
173+
}
146174
}

0 commit comments

Comments
 (0)