Skip to content
Permalink
Browse files
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
  • Loading branch information
ssddanbrown committed Dec 14, 2021
1 parent 867cbe1 commit e765e618547c92f4e0b46caca6fb91f0174efd99
Showing with 115 additions and 20 deletions.
  1. +4 −1 app/Auth/UserRepo.php
  2. +16 −9 app/Http/Controllers/UserSearchController.php
  3. +12 −10 resources/views/users/delete.blade.php
  4. +15 −0 tests/User/UserManagementTest.php
  5. +68 −0 tests/User/UserSearchTest.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']);
@@ -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(),
]);
}
}
@@ -12,17 +12,19 @@

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

<hr class="my-l">

<div class="grid half gap-xl v-center">
<div>
<label class="setting-list-label">{{ trans('settings.users_migrate_ownership') }}</label>
<p class="small">{{ trans('settings.users_migrate_ownership_desc') }}</p>
@if(userCan('users-manage'))
<hr class="my-l">

<div class="grid half gap-xl v-center">
<div>
<label class="setting-list-label">{{ trans('settings.users_migrate_ownership') }}</label>
<p class="small">{{ trans('settings.users_migrate_ownership_desc') }}</p>
</div>
<div>
@include('form.user-select', ['name' => 'new_owner_id', 'user' => null, 'compact' => false])
</div>
</div>
<div>
@include('form.user-select', ['name' => 'new_owner_id', 'user' => null, 'compact' => false])
</div>
</div>
@endif

<hr class="my-l">

@@ -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();
@@ -0,0 +1,68 @@
<?php

namespace Tests\User;

use BookStack\Auth\User;
use Tests\TestCase;

class UserSearchTest extends TestCase
{

public function test_select_search_matches_by_name()
{
$viewer = $this->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);
}

}

0 comments on commit e765e61

Please sign in to comment.