Skip to content

Commit e765e61

Browse files
ssddanbrownssddanbrown
committed
Addressed user detail harvesting issue
Altered access & usage of the /search/users/select endpoint with the following changes: - Removed searching of email address to prevent email detail discovery via hunting via search queries. - Required the user to be logged in and have permission to manage users or manage permissions on items in some way. - Removed the user migration option on user delete unless they have permission to manage users. For #3108 Reported in https://huntr.dev/bounties/135f2d7d-ab0b-4351-99b9-889efac46fca/ Reported by @Haxatron
1 parent 867cbe1 commit e765e61

File tree

5 files changed

+115
-20
lines changed

5 files changed

+115
-20
lines changed

‎app/Auth/UserRepo.php

+4-1
Original file line numberDiff line numberDiff line change
@@ -63,13 +63,16 @@ public function getAllUsers(): Collection
6363

6464
/**
6565
* Get all the users with their permissions in a paginated format.
66+
* Note: Due to the use of email search this should only be used when
67+
* user is assumed to be trusted. (Admin users).
68+
* Email search can be abused to extract email addresses.
6669
*/
6770
public function getAllUsersPaginatedAndSorted(int $count, array $sortData): LengthAwarePaginator
6871
{
6972
$sort = $sortData['sort'];
7073

7174
$query = User::query()->select(['*'])
72-
->withLastActivityAt()
75+
->scopes(['withLastActivityAt'])
7376
->with(['roles', 'avatar'])
7477
->withCount('mfaValues')
7578
->orderBy($sort, $sortData['order']);

‎app/Http/Controllers/UserSearchController.php

+16-9
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@
33
namespace BookStack\Http\Controllers;
44

55
use BookStack\Auth\User;
6-
use Illuminate\Database\Eloquent\Builder;
76
use Illuminate\Http\Request;
87

98
class UserSearchController extends Controller
@@ -14,19 +13,27 @@ class UserSearchController extends Controller
1413
*/
1514
public function forSelect(Request $request)
1615
{
16+
$hasPermission = signedInUser() && (
17+
userCan('users-manage')
18+
|| userCan('restrictions-manage-own')
19+
|| userCan('restrictions-manage-all')
20+
);
21+
22+
if (!$hasPermission) {
23+
$this->showPermissionError();
24+
}
25+
1726
$search = $request->get('search', '');
18-
$query = User::query()->orderBy('name', 'desc')
27+
$query = User::query()
28+
->orderBy('name', 'asc')
1929
->take(20);
2030

2131
if (!empty($search)) {
22-
$query->where(function (Builder $query) use ($search) {
23-
$query->where('email', 'like', '%' . $search . '%')
24-
->orWhere('name', 'like', '%' . $search . '%');
25-
});
32+
$query->where('name', 'like', '%' . $search . '%');
2633
}
2734

28-
$users = $query->get();
29-
30-
return view('form.user-select-list', compact('users'));
35+
return view('form.user-select-list', [
36+
'users' => $query->get(),
37+
]);
3138
}
3239
}

‎resources/views/users/delete.blade.php

+12-10
Original file line numberDiff line numberDiff line change
@@ -12,17 +12,19 @@
1212

1313
<p>{{ trans('settings.users_delete_warning', ['userName' => $user->name]) }}</p>
1414

15-
<hr class="my-l">
16-
17-
<div class="grid half gap-xl v-center">
18-
<div>
19-
<label class="setting-list-label">{{ trans('settings.users_migrate_ownership') }}</label>
20-
<p class="small">{{ trans('settings.users_migrate_ownership_desc') }}</p>
15+
@if(userCan('users-manage'))
16+
<hr class="my-l">
17+
18+
<div class="grid half gap-xl v-center">
19+
<div>
20+
<label class="setting-list-label">{{ trans('settings.users_migrate_ownership') }}</label>
21+
<p class="small">{{ trans('settings.users_migrate_ownership_desc') }}</p>
22+
</div>
23+
<div>
24+
@include('form.user-select', ['name' => 'new_owner_id', 'user' => null, 'compact' => false])
25+
</div>
2126
</div>
22-
<div>
23-
@include('form.user-select', ['name' => 'new_owner_id', 'user' => null, 'compact' => false])
24-
</div>
25-
</div>
27+
@endif
2628

2729
<hr class="my-l">
2830

‎tests/User/UserManagementTest.php

+15
Original file line numberDiff line numberDiff line change
@@ -130,6 +130,21 @@ public function test_delete_offers_migrate_option()
130130
$resp->assertSee('new_owner_id');
131131
}
132132

133+
public function test_migrate_option_hidden_if_user_cannot_manage_users()
134+
{
135+
$editor = $this->getEditor();
136+
137+
$resp = $this->asEditor()->get("settings/users/{$editor->id}/delete");
138+
$resp->assertDontSee('Migrate Ownership');
139+
$resp->assertDontSee('new_owner_id');
140+
141+
$this->giveUserPermissions($editor, ['users-manage']);
142+
143+
$resp = $this->asEditor()->get("settings/users/{$editor->id}/delete");
144+
$resp->assertSee('Migrate Ownership');
145+
$resp->assertSee('new_owner_id');
146+
}
147+
133148
public function test_delete_with_new_owner_id_changes_ownership()
134149
{
135150
$page = Page::query()->first();

‎tests/User/UserSearchTest.php

+68
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,68 @@
1+
<?php
2+
3+
namespace Tests\User;
4+
5+
use BookStack\Auth\User;
6+
use Tests\TestCase;
7+
8+
class UserSearchTest extends TestCase
9+
{
10+
11+
public function test_select_search_matches_by_name()
12+
{
13+
$viewer = $this->getViewer();
14+
$admin = $this->getAdmin();
15+
$resp = $this->actingAs($admin)->get('/search/users/select?search=' . urlencode($viewer->name));
16+
17+
$resp->assertOk();
18+
$resp->assertSee($viewer->name);
19+
$resp->assertDontSee($admin->name);
20+
}
21+
22+
public function test_select_search_shows_first_by_name_without_search()
23+
{
24+
/** @var User $firstUser */
25+
$firstUser = User::query()->orderBy('name', 'desc')->first();
26+
$resp = $this->asAdmin()->get('/search/users/select');
27+
28+
$resp->assertOk();
29+
$resp->assertSee($firstUser->name);
30+
}
31+
32+
public function test_select_search_does_not_match_by_email()
33+
{
34+
$viewer = $this->getViewer();
35+
$editor = $this->getEditor();
36+
$resp = $this->actingAs($editor)->get('/search/users/select?search=' . urlencode($viewer->email));
37+
38+
$resp->assertDontSee($viewer->name);
39+
}
40+
41+
public function test_select_requires_right_permission()
42+
{
43+
$permissions = ['users-manage', 'restrictions-manage-own', 'restrictions-manage-all'];
44+
$user = $this->getViewer();
45+
46+
foreach ($permissions as $permission) {
47+
$resp = $this->actingAs($user)->get('/search/users/select?search=a');
48+
$this->assertPermissionError($resp);
49+
50+
$this->giveUserPermissions($user, [$permission]);
51+
$resp = $this->actingAs($user)->get('/search/users/select?search=a');
52+
$resp->assertOk();
53+
$user->roles()->delete();
54+
$user->clearPermissionCache();
55+
}
56+
}
57+
58+
public function test_select_requires_logged_in_user()
59+
{
60+
$this->setSettings(['app-public' => true]);
61+
$defaultUser = User::getDefault();
62+
$this->giveUserPermissions($defaultUser, ['users-manage']);
63+
64+
$resp = $this->get('/search/users/select?search=a');
65+
$this->assertPermissionError($resp);
66+
}
67+
68+
}

0 commit comments

Comments
 (0)