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]));