From e765e618547c92f4e0b46caca6fb91f0174efd99 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Tue, 14 Dec 2021 18:47:22 +0000 Subject: [PATCH] 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 --- app/Auth/UserRepo.php | 5 +- app/Http/Controllers/UserSearchController.php | 25 ++++--- resources/views/users/delete.blade.php | 22 +++--- tests/User/UserManagementTest.php | 15 ++++ tests/User/UserSearchTest.php | 68 +++++++++++++++++++ 5 files changed, 115 insertions(+), 20 deletions(-) create mode 100644 tests/User/UserSearchTest.php diff --git a/app/Auth/UserRepo.php b/app/Auth/UserRepo.php index 6d48f124020..84002b7f7a5 100644 --- a/app/Auth/UserRepo.php +++ b/app/Auth/UserRepo.php @@ -63,13 +63,16 @@ public function getAllUsers(): Collection /** * Get all the users with their permissions in a paginated format. + * Note: Due to the use of email search this should only be used when + * user is assumed to be trusted. (Admin users). + * Email search can be abused to extract email addresses. */ public function getAllUsersPaginatedAndSorted(int $count, array $sortData): LengthAwarePaginator { $sort = $sortData['sort']; $query = User::query()->select(['*']) - ->withLastActivityAt() + ->scopes(['withLastActivityAt']) ->with(['roles', 'avatar']) ->withCount('mfaValues') ->orderBy($sort, $sortData['order']); diff --git a/app/Http/Controllers/UserSearchController.php b/app/Http/Controllers/UserSearchController.php index f7ed9e57a94..1a37ca491dc 100644 --- a/app/Http/Controllers/UserSearchController.php +++ b/app/Http/Controllers/UserSearchController.php @@ -3,7 +3,6 @@ namespace BookStack\Http\Controllers; use BookStack\Auth\User; -use Illuminate\Database\Eloquent\Builder; use Illuminate\Http\Request; class UserSearchController extends Controller @@ -14,19 +13,27 @@ class UserSearchController extends Controller */ public function forSelect(Request $request) { + $hasPermission = signedInUser() && ( + userCan('users-manage') + || userCan('restrictions-manage-own') + || userCan('restrictions-manage-all') + ); + + if (!$hasPermission) { + $this->showPermissionError(); + } + $search = $request->get('search', ''); - $query = User::query()->orderBy('name', 'desc') + $query = User::query() + ->orderBy('name', 'asc') ->take(20); if (!empty($search)) { - $query->where(function (Builder $query) use ($search) { - $query->where('email', 'like', '%' . $search . '%') - ->orWhere('name', 'like', '%' . $search . '%'); - }); + $query->where('name', 'like', '%' . $search . '%'); } - $users = $query->get(); - - return view('form.user-select-list', compact('users')); + return view('form.user-select-list', [ + 'users' => $query->get(), + ]); } } diff --git a/resources/views/users/delete.blade.php b/resources/views/users/delete.blade.php index aea4d795454..490e9d6c5aa 100644 --- a/resources/views/users/delete.blade.php +++ b/resources/views/users/delete.blade.php @@ -12,17 +12,19 @@

{{ trans('settings.users_delete_warning', ['userName' => $user->name]) }}

-
- -
-
- -

{{ trans('settings.users_migrate_ownership_desc') }}

+ @if(userCan('users-manage')) +
+ +
+
+ +

{{ trans('settings.users_migrate_ownership_desc') }}

+
+
+ @include('form.user-select', ['name' => 'new_owner_id', 'user' => null, 'compact' => false]) +
-
- @include('form.user-select', ['name' => 'new_owner_id', 'user' => null, 'compact' => false]) -
-
+ @endif
diff --git a/tests/User/UserManagementTest.php b/tests/User/UserManagementTest.php index 5a36b85df6f..94970df4fe5 100644 --- a/tests/User/UserManagementTest.php +++ b/tests/User/UserManagementTest.php @@ -130,6 +130,21 @@ public function test_delete_offers_migrate_option() $resp->assertSee('new_owner_id'); } + public function test_migrate_option_hidden_if_user_cannot_manage_users() + { + $editor = $this->getEditor(); + + $resp = $this->asEditor()->get("settings/users/{$editor->id}/delete"); + $resp->assertDontSee('Migrate Ownership'); + $resp->assertDontSee('new_owner_id'); + + $this->giveUserPermissions($editor, ['users-manage']); + + $resp = $this->asEditor()->get("settings/users/{$editor->id}/delete"); + $resp->assertSee('Migrate Ownership'); + $resp->assertSee('new_owner_id'); + } + public function test_delete_with_new_owner_id_changes_ownership() { $page = Page::query()->first(); diff --git a/tests/User/UserSearchTest.php b/tests/User/UserSearchTest.php new file mode 100644 index 00000000000..1bf3f2d296b --- /dev/null +++ b/tests/User/UserSearchTest.php @@ -0,0 +1,68 @@ +getViewer(); + $admin = $this->getAdmin(); + $resp = $this->actingAs($admin)->get('/search/users/select?search=' . urlencode($viewer->name)); + + $resp->assertOk(); + $resp->assertSee($viewer->name); + $resp->assertDontSee($admin->name); + } + + public function test_select_search_shows_first_by_name_without_search() + { + /** @var User $firstUser */ + $firstUser = User::query()->orderBy('name', 'desc')->first(); + $resp = $this->asAdmin()->get('/search/users/select'); + + $resp->assertOk(); + $resp->assertSee($firstUser->name); + } + + public function test_select_search_does_not_match_by_email() + { + $viewer = $this->getViewer(); + $editor = $this->getEditor(); + $resp = $this->actingAs($editor)->get('/search/users/select?search=' . urlencode($viewer->email)); + + $resp->assertDontSee($viewer->name); + } + + public function test_select_requires_right_permission() + { + $permissions = ['users-manage', 'restrictions-manage-own', 'restrictions-manage-all']; + $user = $this->getViewer(); + + foreach ($permissions as $permission) { + $resp = $this->actingAs($user)->get('/search/users/select?search=a'); + $this->assertPermissionError($resp); + + $this->giveUserPermissions($user, [$permission]); + $resp = $this->actingAs($user)->get('/search/users/select?search=a'); + $resp->assertOk(); + $user->roles()->delete(); + $user->clearPermissionCache(); + } + } + + public function test_select_requires_logged_in_user() + { + $this->setSettings(['app-public' => true]); + $defaultUser = User::getDefault(); + $this->giveUserPermissions($defaultUser, ['users-manage']); + + $resp = $this->get('/search/users/select?search=a'); + $this->assertPermissionError($resp); + } + +} \ No newline at end of file