From da82e70ca3cdd075f7ae148cb2f58fddb0d93627 Mon Sep 17 00:00:00 2001 From: Talstra Ruben SRSNL Date: Mon, 20 Jan 2025 17:21:46 +0100 Subject: [PATCH 1/6] =?UTF-8?q?Add=20optional=20OIDC=20avatar=20fetching?= =?UTF-8?q?=20from=20the=20=E2=80=9Cpicture=E2=80=9D=20claim?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- app/Access/Oidc/OidcService.php | 8 ++++++- app/Access/Oidc/OidcUserDetails.php | 2 ++ app/Config/oidc.php | 3 +++ app/Uploads/UserAvatars.php | 37 ++++++++++++++++++++++++++--- 4 files changed, 46 insertions(+), 4 deletions(-) diff --git a/app/Access/Oidc/OidcService.php b/app/Access/Oidc/OidcService.php index 7c1760649..660885e8b 100644 --- a/app/Access/Oidc/OidcService.php +++ b/app/Access/Oidc/OidcService.php @@ -11,6 +11,7 @@ use BookStack\Exceptions\UserRegistrationException; use BookStack\Facades\Theme; use BookStack\Http\HttpRequestService; use BookStack\Theming\ThemeEvents; +use BookStack\Uploads\UserAvatars; use BookStack\Users\Models\User; use Illuminate\Support\Facades\Cache; use League\OAuth2\Client\OptionProvider\HttpBasicAuthOptionProvider; @@ -26,7 +27,8 @@ class OidcService protected RegistrationService $registrationService, protected LoginService $loginService, protected HttpRequestService $http, - protected GroupSyncService $groupService + protected GroupSyncService $groupService, + protected UserAvatars $userAvatars ) { } @@ -227,6 +229,10 @@ class OidcService $this->loginService->login($user, 'oidc'); + if ($this->config()['fetch_avatars'] && $userDetails->picture) { + $this->userAvatars->assignToUserFromUrl($user, $userDetails->picture, $accessToken->getToken()); + } + return $user; } diff --git a/app/Access/Oidc/OidcUserDetails.php b/app/Access/Oidc/OidcUserDetails.php index fae20de0b..10595d1e0 100644 --- a/app/Access/Oidc/OidcUserDetails.php +++ b/app/Access/Oidc/OidcUserDetails.php @@ -11,6 +11,7 @@ class OidcUserDetails public ?string $email = null, public ?string $name = null, public ?array $groups = null, + public ?string $picture = null, ) { } @@ -40,6 +41,7 @@ class OidcUserDetails $this->email = $claims->getClaim('email') ?? $this->email; $this->name = static::getUserDisplayName($displayNameClaims, $claims) ?? $this->name; $this->groups = static::getUserGroups($groupsClaim, $claims) ?? $this->groups; + $this->picture = $claims->getClaim('picture') ?: $this->picture; } protected static function getUserDisplayName(string $displayNameClaims, ProvidesClaims $token): string diff --git a/app/Config/oidc.php b/app/Config/oidc.php index 8b5470931..62f19a119 100644 --- a/app/Config/oidc.php +++ b/app/Config/oidc.php @@ -54,4 +54,7 @@ return [ 'groups_claim' => env('OIDC_GROUPS_CLAIM', 'groups'), // When syncing groups, remove any groups that no longer match. Otherwise, sync only adds new groups. 'remove_from_groups' => env('OIDC_REMOVE_FROM_GROUPS', false), + + // When enabled, BookStack will fetch the user’s avatar from the 'picture' claim (SSRF risk if URLs are untrusted). + 'fetch_avatars' => env('OIDC_FETCH_AVATARS', false), ]; diff --git a/app/Uploads/UserAvatars.php b/app/Uploads/UserAvatars.php index c62324735..af91dfe70 100644 --- a/app/Uploads/UserAvatars.php +++ b/app/Uploads/UserAvatars.php @@ -53,6 +53,31 @@ class UserAvatars } } + /** + * Assign a new avatar image to the given user by fetching from a remote URL. + */ + public function assignToUserFromUrl(User $user, string $avatarUrl, ?string $accessToken = null): void + { + // Quickly skip invalid or non-HTTP URLs + if (!$avatarUrl || !str_starts_with($avatarUrl, 'http')) { + return; + } + + try { + $this->destroyAllForUser($user); + $imageData = $this->getAvatarImageData($avatarUrl, $accessToken); + $avatar = $this->createAvatarImageFromData($user, $imageData, 'png'); + $user->avatar()->associate($avatar); + $user->save(); + } catch (Exception $e) { + Log::error('Failed to save user avatar image from URL', [ + 'exception' => $e, + 'url' => $avatarUrl, + 'user_id' => $user->id, + ]); + } + } + /** * Destroy all user avatars uploaded to the given user. */ @@ -105,15 +130,21 @@ class UserAvatars } /** - * Gets an image from url and returns it as a string of image data. + * Gets an image from a URL (public or private) and returns it as a string of image data. * * @throws HttpFetchException */ - protected function getAvatarImageData(string $url): string + protected function getAvatarImageData(string $url, ?string $accessToken = null): string { try { + $headers = []; + if (!empty($accessToken)) { + $headers['Authorization'] = 'Bearer ' . $accessToken; + } + $client = $this->http->buildClient(5); - $response = $client->sendRequest(new Request('GET', $url)); + $response = $client->sendRequest(new Request('GET', $url, $headers)); + if ($response->getStatusCode() !== 200) { throw new HttpFetchException(trans('errors.cannot_get_image_from_url', ['url' => $url])); } From f9dbbe5d70f751d67aa41f65f257f580bcf33568 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sat, 24 May 2025 14:02:37 +0100 Subject: [PATCH 2/6] OIDC: Updated picture fetch implementation during review Review of #5429 --- app/Access/Oidc/OidcService.php | 8 ++++---- app/Access/Oidc/OidcUserDetails.php | 20 ++++++++++++++----- app/Config/oidc.php | 8 +++++--- app/Uploads/UserAvatars.php | 30 ++++++++++++++--------------- 4 files changed, 38 insertions(+), 28 deletions(-) diff --git a/app/Access/Oidc/OidcService.php b/app/Access/Oidc/OidcService.php index 660885e8b..452fda3d8 100644 --- a/app/Access/Oidc/OidcService.php +++ b/app/Access/Oidc/OidcService.php @@ -222,6 +222,10 @@ class OidcService throw new OidcException($exception->getMessage()); } + if ($this->config()['fetch_avatar'] && $user->wasRecentlyCreated && $userDetails->picture) { + $this->userAvatars->assignToUserFromUrl($user, $userDetails->picture); + } + if ($this->shouldSyncGroups()) { $detachExisting = $this->config()['remove_from_groups']; $this->groupService->syncUserWithFoundGroups($user, $userDetails->groups ?? [], $detachExisting); @@ -229,10 +233,6 @@ class OidcService $this->loginService->login($user, 'oidc'); - if ($this->config()['fetch_avatars'] && $userDetails->picture) { - $this->userAvatars->assignToUserFromUrl($user, $userDetails->picture, $accessToken->getToken()); - } - return $user; } diff --git a/app/Access/Oidc/OidcUserDetails.php b/app/Access/Oidc/OidcUserDetails.php index 10595d1e0..7a422a58d 100644 --- a/app/Access/Oidc/OidcUserDetails.php +++ b/app/Access/Oidc/OidcUserDetails.php @@ -41,16 +41,16 @@ class OidcUserDetails $this->email = $claims->getClaim('email') ?? $this->email; $this->name = static::getUserDisplayName($displayNameClaims, $claims) ?? $this->name; $this->groups = static::getUserGroups($groupsClaim, $claims) ?? $this->groups; - $this->picture = $claims->getClaim('picture') ?: $this->picture; + $this->picture = static::getPicture($claims) ?: $this->picture; } - protected static function getUserDisplayName(string $displayNameClaims, ProvidesClaims $token): string + protected static function getUserDisplayName(string $displayNameClaims, ProvidesClaims $claims): string { $displayNameClaimParts = explode('|', $displayNameClaims); $displayName = []; foreach ($displayNameClaimParts as $claim) { - $component = $token->getClaim(trim($claim)) ?? ''; + $component = $claims->getClaim(trim($claim)) ?? ''; if ($component !== '') { $displayName[] = $component; } @@ -59,13 +59,13 @@ class OidcUserDetails return implode(' ', $displayName); } - protected static function getUserGroups(string $groupsClaim, ProvidesClaims $token): ?array + protected static function getUserGroups(string $groupsClaim, ProvidesClaims $claims): ?array { if (empty($groupsClaim)) { return null; } - $groupsList = Arr::get($token->getAllClaims(), $groupsClaim); + $groupsList = Arr::get($claims->getAllClaims(), $groupsClaim); if (!is_array($groupsList)) { return null; } @@ -74,4 +74,14 @@ class OidcUserDetails return is_string($val); })); } + + protected static function getPicture(ProvidesClaims $claims): ?string + { + $picture = $claims->getClaim('picture'); + if (is_string($picture) && str_starts_with($picture, 'http')) { + return $picture; + } + + return null; + } } diff --git a/app/Config/oidc.php b/app/Config/oidc.php index 62f19a119..49427cb12 100644 --- a/app/Config/oidc.php +++ b/app/Config/oidc.php @@ -47,6 +47,11 @@ return [ // Multiple values can be provided comma seperated. 'additional_scopes' => env('OIDC_ADDITIONAL_SCOPES', null), + // Enable fetching of the user's avatar from the 'picture' claim on initial login. + // This can be a security risk due to performing server-side fetching of data from external URLs. + // Only enable if you trust the OIDC auth provider to provide safe URLs for user images. + 'fetch_avatar' => env('OIDC_FETCH_AVATAR', false), + // Group sync options // Enable syncing, upon login, of OIDC groups to BookStack roles 'user_to_groups' => env('OIDC_USER_TO_GROUPS', false), @@ -54,7 +59,4 @@ return [ 'groups_claim' => env('OIDC_GROUPS_CLAIM', 'groups'), // When syncing groups, remove any groups that no longer match. Otherwise, sync only adds new groups. 'remove_from_groups' => env('OIDC_REMOVE_FROM_GROUPS', false), - - // When enabled, BookStack will fetch the user’s avatar from the 'picture' claim (SSRF risk if URLs are untrusted). - 'fetch_avatars' => env('OIDC_FETCH_AVATARS', false), ]; diff --git a/app/Uploads/UserAvatars.php b/app/Uploads/UserAvatars.php index af91dfe70..1763dcbbd 100644 --- a/app/Uploads/UserAvatars.php +++ b/app/Uploads/UserAvatars.php @@ -5,6 +5,7 @@ namespace BookStack\Uploads; use BookStack\Exceptions\HttpFetchException; use BookStack\Http\HttpRequestService; use BookStack\Users\Models\User; +use BookStack\Util\WebSafeMimeSniffer; use Exception; use GuzzleHttp\Psr7\Request; use Illuminate\Support\Facades\Log; @@ -56,17 +57,19 @@ class UserAvatars /** * Assign a new avatar image to the given user by fetching from a remote URL. */ - public function assignToUserFromUrl(User $user, string $avatarUrl, ?string $accessToken = null): void + public function assignToUserFromUrl(User $user, string $avatarUrl): void { - // Quickly skip invalid or non-HTTP URLs - if (!$avatarUrl || !str_starts_with($avatarUrl, 'http')) { - return; - } - try { $this->destroyAllForUser($user); - $imageData = $this->getAvatarImageData($avatarUrl, $accessToken); - $avatar = $this->createAvatarImageFromData($user, $imageData, 'png'); + $imageData = $this->getAvatarImageData($avatarUrl); + + $mime = (new WebSafeMimeSniffer())->sniff($imageData); + [$format, $type] = explode('/', $mime, 2); + if ($format !== 'image' || ImageService::isExtensionSupported($type)) { + return; + } + + $avatar = $this->createAvatarImageFromData($user, $imageData, $type); $user->avatar()->associate($avatar); $user->save(); } catch (Exception $e) { @@ -130,20 +133,15 @@ class UserAvatars } /** - * Gets an image from a URL (public or private) and returns it as a string of image data. + * Get an image from a URL and return it as a string of image data. * * @throws HttpFetchException */ - protected function getAvatarImageData(string $url, ?string $accessToken = null): string + protected function getAvatarImageData(string $url): string { try { - $headers = []; - if (!empty($accessToken)) { - $headers['Authorization'] = 'Bearer ' . $accessToken; - } - $client = $this->http->buildClient(5); - $response = $client->sendRequest(new Request('GET', $url, $headers)); + $response = $client->sendRequest(new Request('GET', $url)); if ($response->getStatusCode() !== 200) { throw new HttpFetchException(trans('errors.cannot_get_image_from_url', ['url' => $url])); From b64c9b31d51b5d15972c6cb37cfdca37db80d28a Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sat, 24 May 2025 14:36:36 +0100 Subject: [PATCH 3/6] OIDC: Added testing coverage for picture fetching --- app/Access/Oidc/OidcService.php | 2 ++ app/Uploads/UserAvatars.php | 2 +- app/Users/Models/User.php | 1 + tests/Auth/OidcTest.php | 52 ++++++++++++++++++++++++++++++++ tests/Helpers/FileProvider.php | 8 +++++ tests/test-data/test-image.jpg | Bin 0 -> 268 bytes 6 files changed, 64 insertions(+), 1 deletion(-) create mode 100644 tests/test-data/test-image.jpg diff --git a/app/Access/Oidc/OidcService.php b/app/Access/Oidc/OidcService.php index 452fda3d8..38eecdff3 100644 --- a/app/Access/Oidc/OidcService.php +++ b/app/Access/Oidc/OidcService.php @@ -222,6 +222,8 @@ class OidcService throw new OidcException($exception->getMessage()); } + // TODO - Update this (and tests and config option comments) to actually align with LDAP system + // which syncs whenever on login or registration, where there's no existing avatar. if ($this->config()['fetch_avatar'] && $user->wasRecentlyCreated && $userDetails->picture) { $this->userAvatars->assignToUserFromUrl($user, $userDetails->picture); } diff --git a/app/Uploads/UserAvatars.php b/app/Uploads/UserAvatars.php index 1763dcbbd..2c9f7a848 100644 --- a/app/Uploads/UserAvatars.php +++ b/app/Uploads/UserAvatars.php @@ -65,7 +65,7 @@ class UserAvatars $mime = (new WebSafeMimeSniffer())->sniff($imageData); [$format, $type] = explode('/', $mime, 2); - if ($format !== 'image' || ImageService::isExtensionSupported($type)) { + if ($format !== 'image' || !ImageService::isExtensionSupported($type)) { return; } diff --git a/app/Users/Models/User.php b/app/Users/Models/User.php index 3797e7cb0..0d437418b 100644 --- a/app/Users/Models/User.php +++ b/app/Users/Models/User.php @@ -45,6 +45,7 @@ use Illuminate\Support\Collection; * @property string $system_name * @property Collection $roles * @property Collection $mfaValues + * @property ?Image $avatar */ class User extends Model implements AuthenticatableContract, CanResetPasswordContract, Loggable, Sluggable { diff --git a/tests/Auth/OidcTest.php b/tests/Auth/OidcTest.php index 205f75a4d..f4d044bf1 100644 --- a/tests/Auth/OidcTest.php +++ b/tests/Auth/OidcTest.php @@ -41,6 +41,7 @@ class OidcTest extends TestCase 'oidc.discover' => false, 'oidc.dump_user_details' => false, 'oidc.additional_scopes' => '', + 'odic.fetch_avatar' => false, 'oidc.user_to_groups' => false, 'oidc.groups_claim' => 'group', 'oidc.remove_from_groups' => false, @@ -457,6 +458,57 @@ class OidcTest extends TestCase ]); } + public function test_user_avatar_fetched_from_picture_on_first_login_if_enabled() + { + config()->set(['oidc.fetch_avatar' => true]); + + $this->runLogin([ + 'email' => 'avatar@example.com', + 'picture' => 'https://example.com/my-avatar.jpg', + ], [ + new Response(200, ['Content-Type' => 'image/jpeg'], $this->files->jpegImageData()) + ]); + + $user = User::query()->where('email', '=', 'avatar@example.com')->first(); + $this->assertNotNull($user); + + $this->assertTrue($user->avatar()->exists()); + } + + public function test_user_avatar_not_fetched_if_image_data_format_unknown() + { + config()->set(['oidc.fetch_avatar' => true]); + + $this->runLogin([ + 'email' => 'avatar-format@example.com', + 'picture' => 'https://example.com/my-avatar.jpg', + ], [ + new Response(200, ['Content-Type' => 'image/jpeg'], str_repeat('abc123', 5)) + ]); + + $user = User::query()->where('email', '=', 'avatar-format@example.com')->first(); + $this->assertNotNull($user); + + $this->assertFalse($user->avatar()->exists()); + } + + public function test_user_avatar_not_fetched_when_user_already_exists() + { + config()->set(['oidc.fetch_avatar' => true]); + $editor = $this->users->editor(); + $editor->external_auth_id = 'benny509'; + + $this->runLogin([ + 'picture' => 'https://example.com/my-avatar.jpg', + 'sub' => 'benny509', + ], [ + new Response(200, ['Content-Type' => 'image/jpeg'], $this->files->jpegImageData()) + ]); + + $editor->refresh(); + $this->assertFalse($editor->avatar()->exists()); + } + public function test_login_group_sync() { config()->set([ diff --git a/tests/Helpers/FileProvider.php b/tests/Helpers/FileProvider.php index 442e036ff..a455e0fb4 100644 --- a/tests/Helpers/FileProvider.php +++ b/tests/Helpers/FileProvider.php @@ -60,6 +60,14 @@ class FileProvider return file_get_contents($this->testFilePath('test-image.png')); } + /** + * Get raw data for a Jpeg image test file. + */ + public function jpegImageData(): string + { + return file_get_contents($this->testFilePath('test-image.jpg')); + } + /** * Get the expected relative path for an uploaded image of the given type and filename. */ diff --git a/tests/test-data/test-image.jpg b/tests/test-data/test-image.jpg new file mode 100644 index 0000000000000000000000000000000000000000..9bb74ff04833b969a41dd25c05144a7930ec4791 GIT binary patch literal 268 zcmex=h+pFW<7#R=#a_3=8ej9ot%hR(f wrhv Date: Sat, 24 May 2025 16:34:36 +0100 Subject: [PATCH 4/6] OIDC: Updated avatar fetching to run on each login But only where the user does not already have an avatar assigned. This aligns with the LDAP avatar fetching logic. --- app/Access/Oidc/OidcService.php | 4 +--- app/Config/oidc.php | 3 ++- tests/Auth/OidcTest.php | 31 +++++++++++++++++++++++++++++-- 3 files changed, 32 insertions(+), 6 deletions(-) diff --git a/app/Access/Oidc/OidcService.php b/app/Access/Oidc/OidcService.php index 38eecdff3..d6f6ef156 100644 --- a/app/Access/Oidc/OidcService.php +++ b/app/Access/Oidc/OidcService.php @@ -222,9 +222,7 @@ class OidcService throw new OidcException($exception->getMessage()); } - // TODO - Update this (and tests and config option comments) to actually align with LDAP system - // which syncs whenever on login or registration, where there's no existing avatar. - if ($this->config()['fetch_avatar'] && $user->wasRecentlyCreated && $userDetails->picture) { + if ($this->config()['fetch_avatar'] && !$user->avatar()->exists() && $userDetails->picture) { $this->userAvatars->assignToUserFromUrl($user, $userDetails->picture); } diff --git a/app/Config/oidc.php b/app/Config/oidc.php index 49427cb12..1d3193eee 100644 --- a/app/Config/oidc.php +++ b/app/Config/oidc.php @@ -47,7 +47,8 @@ return [ // Multiple values can be provided comma seperated. 'additional_scopes' => env('OIDC_ADDITIONAL_SCOPES', null), - // Enable fetching of the user's avatar from the 'picture' claim on initial login. + // Enable fetching of the user's avatar from the 'picture' claim on login. + // Will only be fetched if the user doesn't already have an avatar image assigned. // This can be a security risk due to performing server-side fetching of data from external URLs. // Only enable if you trust the OIDC auth provider to provide safe URLs for user images. 'fetch_avatar' => env('OIDC_FETCH_AVATAR', false), diff --git a/tests/Auth/OidcTest.php b/tests/Auth/OidcTest.php index f4d044bf1..71f883ca6 100644 --- a/tests/Auth/OidcTest.php +++ b/tests/Auth/OidcTest.php @@ -5,6 +5,7 @@ namespace Tests\Auth; use BookStack\Activity\ActivityType; use BookStack\Facades\Theme; use BookStack\Theming\ThemeEvents; +use BookStack\Uploads\UserAvatars; use BookStack\Users\Models\Role; use BookStack\Users\Models\User; use GuzzleHttp\Psr7\Response; @@ -475,6 +476,26 @@ class OidcTest extends TestCase $this->assertTrue($user->avatar()->exists()); } + public function test_user_avatar_fetched_for_existing_user_when_no_avatar_already_assigned() + { + config()->set(['oidc.fetch_avatar' => true]); + $editor = $this->users->editor(); + $editor->external_auth_id = 'benny509'; + $editor->save(); + + $this->assertFalse($editor->avatar()->exists()); + + $this->runLogin([ + 'picture' => 'https://example.com/my-avatar.jpg', + 'sub' => 'benny509', + ], [ + new Response(200, ['Content-Type' => 'image/jpeg'], $this->files->jpegImageData()) + ]); + + $editor->refresh(); + $this->assertTrue($editor->avatar()->exists()); + } + public function test_user_avatar_not_fetched_if_image_data_format_unknown() { config()->set(['oidc.fetch_avatar' => true]); @@ -492,11 +513,16 @@ class OidcTest extends TestCase $this->assertFalse($user->avatar()->exists()); } - public function test_user_avatar_not_fetched_when_user_already_exists() + public function test_user_avatar_not_fetched_when_avatar_already_assigned() { config()->set(['oidc.fetch_avatar' => true]); $editor = $this->users->editor(); $editor->external_auth_id = 'benny509'; + $editor->save(); + + $avatars = $this->app->make(UserAvatars::class); + $originalImageData = $this->files->pngImageData(); + $avatars->assignToUserFromExistingData($editor, $originalImageData, 'png'); $this->runLogin([ 'picture' => 'https://example.com/my-avatar.jpg', @@ -506,7 +532,8 @@ class OidcTest extends TestCase ]); $editor->refresh(); - $this->assertFalse($editor->avatar()->exists()); + $newAvatarData = file_get_contents($this->files->relativeToFullPath($editor->avatar->path)); + $this->assertEquals($originalImageData, $newAvatarData); } public function test_login_group_sync() From 9d6bc1ad4da9142766e4d3a29b5a535f93f1c33a Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sat, 24 May 2025 16:47:01 +0100 Subject: [PATCH 5/6] Testing: Updated tests to account for recent page redirect changes --- tests/Permissions/EntityPermissionsTest.php | 8 ++++---- tests/Permissions/RolePermissionsTest.php | 18 ++++++++++-------- 2 files changed, 14 insertions(+), 12 deletions(-) diff --git a/tests/Permissions/EntityPermissionsTest.php b/tests/Permissions/EntityPermissionsTest.php index 6ea0257b8..43d0cfc50 100644 --- a/tests/Permissions/EntityPermissionsTest.php +++ b/tests/Permissions/EntityPermissionsTest.php @@ -184,7 +184,7 @@ class EntityPermissionsTest extends TestCase $this->get($bookUrl . '/edit')->assertRedirect('/'); $this->get('/')->assertSee('You do not have permission'); - $this->get($bookPage->getUrl() . '/edit')->assertRedirect('/'); + $this->get($bookPage->getUrl() . '/edit')->assertRedirect($bookPage->getUrl()); $this->get('/')->assertSee('You do not have permission'); $this->get($bookChapter->getUrl() . '/edit')->assertRedirect('/'); $this->get('/')->assertSee('You do not have permission'); @@ -282,7 +282,7 @@ class EntityPermissionsTest extends TestCase $this->get($chapterUrl . '/edit')->assertRedirect('/'); $this->get('/')->assertSee('You do not have permission'); - $this->get($chapterPage->getUrl() . '/edit')->assertRedirect('/'); + $this->get($chapterPage->getUrl() . '/edit')->assertRedirect($chapterPage->getUrl()); $this->get('/')->assertSee('You do not have permission'); $this->setRestrictionsForTestRoles($chapter, ['view', 'update']); @@ -341,7 +341,7 @@ class EntityPermissionsTest extends TestCase $this->setRestrictionsForTestRoles($page, ['view', 'delete']); - $this->get($pageUrl . '/edit')->assertRedirect('/'); + $this->get($pageUrl . '/edit')->assertRedirect($pageUrl); $this->get('/')->assertSee('You do not have permission'); $this->setRestrictionsForTestRoles($page, ['view', 'update']); @@ -565,7 +565,7 @@ class EntityPermissionsTest extends TestCase $this->get($bookUrl . '/edit')->assertRedirect('/'); $this->get('/')->assertSee('You do not have permission'); - $this->get($bookPage->getUrl() . '/edit')->assertRedirect('/'); + $this->get($bookPage->getUrl() . '/edit')->assertRedirect($bookPage->getUrl()); $this->get('/')->assertSee('You do not have permission'); $this->get($bookChapter->getUrl() . '/edit')->assertRedirect('/'); $this->get('/')->assertSee('You do not have permission'); diff --git a/tests/Permissions/RolePermissionsTest.php b/tests/Permissions/RolePermissionsTest.php index d3146bd47..97cce6817 100644 --- a/tests/Permissions/RolePermissionsTest.php +++ b/tests/Permissions/RolePermissionsTest.php @@ -2,7 +2,6 @@ namespace Tests\Permissions; -use BookStack\Activity\ActivityType; use BookStack\Activity\Models\Comment; use BookStack\Entities\Models\Book; use BookStack\Entities\Models\Bookshelf; @@ -10,7 +9,6 @@ use BookStack\Entities\Models\Chapter; use BookStack\Entities\Models\Entity; use BookStack\Entities\Models\Page; use BookStack\Uploads\Image; -use BookStack\Users\Models\Role; use BookStack\Users\Models\User; use Illuminate\Testing\TestResponse; use Tests\TestCase; @@ -152,10 +150,14 @@ class RolePermissionsTest extends TestCase /** * Check a standard entity access permission. */ - private function checkAccessPermission(string $permission, array $accessUrls = [], array $visibles = []) - { + private function checkAccessPermission( + string $permission, + array $accessUrls = [], + array $visibles = [], + string $expectedRedirectUri = '/', + ) { foreach ($accessUrls as $url) { - $this->actingAs($this->user)->get($url)->assertRedirect('/'); + $this->actingAs($this->user)->get($url)->assertRedirect($expectedRedirectUri); } foreach ($visibles as $url => $text) { @@ -535,11 +537,11 @@ class RolePermissionsTest extends TestCase $ownPage->getUrl() . '/edit', ], [ $ownPage->getUrl() => 'Edit', - ]); + ], $ownPage->getUrl()); $resp = $this->get($otherPage->getUrl()); $this->withHtml($resp)->assertElementNotContains('.action-buttons', 'Edit'); - $this->get($otherPage->getUrl() . '/edit')->assertRedirect('/'); + $this->get($otherPage->getUrl() . '/edit')->assertRedirect($otherPage->getUrl()); } public function test_page_edit_all_permission() @@ -550,7 +552,7 @@ class RolePermissionsTest extends TestCase $otherPage->getUrl('/edit'), ], [ $otherPage->getUrl() => 'Edit', - ]); + ], $otherPage->getUrl()); } public function test_page_delete_own_permission() From eb47e1191665e75adcbc63876470259685b459e8 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sat, 24 May 2025 17:56:21 +0100 Subject: [PATCH 6/6] Avatars: Added redirect handling image fetching Up to 3 times. Can be needed based upon testing with Auth0. Should be fine as long as it's something clearly documented. Added test to cover. --- app/Config/oidc.php | 4 ++-- app/Uploads/UserAvatars.php | 15 +++++++++++++-- tests/Auth/OidcTest.php | 22 ++++++++++++++++++++++ 3 files changed, 37 insertions(+), 4 deletions(-) diff --git a/app/Config/oidc.php b/app/Config/oidc.php index 1d3193eee..16bec873c 100644 --- a/app/Config/oidc.php +++ b/app/Config/oidc.php @@ -49,8 +49,8 @@ return [ // Enable fetching of the user's avatar from the 'picture' claim on login. // Will only be fetched if the user doesn't already have an avatar image assigned. - // This can be a security risk due to performing server-side fetching of data from external URLs. - // Only enable if you trust the OIDC auth provider to provide safe URLs for user images. + // This can be a security risk due to performing server-side fetching (with up to 3 redirects) of + // data from external URLs. Only enable if you trust the OIDC auth provider to provide safe URLs for user images. 'fetch_avatar' => env('OIDC_FETCH_AVATAR', false), // Group sync options diff --git a/app/Uploads/UserAvatars.php b/app/Uploads/UserAvatars.php index 2c9f7a848..0cc640f22 100644 --- a/app/Uploads/UserAvatars.php +++ b/app/Uploads/UserAvatars.php @@ -74,7 +74,7 @@ class UserAvatars $user->save(); } catch (Exception $e) { Log::error('Failed to save user avatar image from URL', [ - 'exception' => $e, + 'exception' => $e->getMessage(), 'url' => $avatarUrl, 'user_id' => $user->id, ]); @@ -141,7 +141,18 @@ class UserAvatars { try { $client = $this->http->buildClient(5); - $response = $client->sendRequest(new Request('GET', $url)); + $responseCount = 0; + + do { + $response = $client->sendRequest(new Request('GET', $url)); + $responseCount++; + $isRedirect = ($response->getStatusCode() === 301 || $response->getStatusCode() === 302); + $url = $response->getHeader('Location')[0] ?? ''; + } while ($responseCount < 3 && $isRedirect && is_string($url) && str_starts_with($url, 'http')); + + if ($responseCount === 3) { + throw new HttpFetchException("Failed to fetch image, max redirect limit of 3 tries reached. Last fetched URL: {$url}"); + } if ($response->getStatusCode() !== 200) { throw new HttpFetchException(trans('errors.cannot_get_image_from_url', ['url' => $url])); diff --git a/tests/Auth/OidcTest.php b/tests/Auth/OidcTest.php index 71f883ca6..a0db1c2ba 100644 --- a/tests/Auth/OidcTest.php +++ b/tests/Auth/OidcTest.php @@ -536,6 +536,28 @@ class OidcTest extends TestCase $this->assertEquals($originalImageData, $newAvatarData); } + public function test_user_avatar_fetch_follows_up_to_three_redirects() + { + config()->set(['oidc.fetch_avatar' => true]); + + $logger = $this->withTestLogger(); + + $this->runLogin([ + 'email' => 'avatar@example.com', + 'picture' => 'https://example.com/my-avatar.jpg', + ], [ + new Response(302, ['Location' => 'https://example.com/a']), + new Response(302, ['Location' => 'https://example.com/b']), + new Response(302, ['Location' => 'https://example.com/c']), + new Response(302, ['Location' => 'https://example.com/d']), + ]); + + $user = User::query()->where('email', '=', 'avatar@example.com')->first(); + $this->assertFalse($user->avatar()->exists()); + + $this->assertStringContainsString('"Failed to fetch image, max redirect limit of 3 tries reached. Last fetched URL: https://example.com/c"', $logger->getRecords()[0]->formatted); + } + public function test_login_group_sync() { config()->set([