OIDC: Updated picture fetch implementation during review

Review of #5429

diff --git a/app/Access/Oidc/OidcService.php b/app/Access/Oidc/OidcService.php
index 660885e8b2a4f30b0c2db8401efbd8a8655d6624..452fda3d85fca1a9e677f5ab26294863ec80517e 100644 (file)
--- a/app/Access/Oidc/OidcService.php
+++ b/app/Access/Oidc/OidcService.php
@@ -222,6 +222,10 @@ class OidcService
             throw new OidcException($exception->getMessage());
         }
 
             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);
         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');
 
 
         $this->loginService->login($user, 'oidc');
 

-        if ($this->config()['fetch_avatars'] && $userDetails->picture) {
-            $this->userAvatars->assignToUserFromUrl($user, $userDetails->picture, $accessToken->getToken());
-        }
-

         return $user;
     }
 
         return $user;
     }
 

diff --git a/app/Access/Oidc/OidcUserDetails.php b/app/Access/Oidc/OidcUserDetails.php
index 10595d1e0ddef9e1a2a7bd17b719810b74001a50..7a422a58de2fcea067454f189d063886641ae688 100644 (file)
--- 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->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) {
     {
         $displayNameClaimParts = explode('|', $displayNameClaims);
 
         $displayName = [];
         foreach ($displayNameClaimParts as $claim) {

-            $component = $token->getClaim(trim($claim)) ?? '';
+            $component = $claims->getClaim(trim($claim)) ?? '';

             if ($component !== '') {
                 $displayName[] = $component;
             }
             if ($component !== '') {
                 $displayName[] = $component;
             }


@@ -59,13 +59,13 @@ class OidcUserDetails
         return implode(' ', $displayName);
     }
 
         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;
         }
 
     {
         if (empty($groupsClaim)) {
             return null;
         }
 

-        $groupsList = Arr::get($token->getAllClaims(), $groupsClaim);
+        $groupsList = Arr::get($claims->getAllClaims(), $groupsClaim);

         if (!is_array($groupsList)) {
             return null;
         }
         if (!is_array($groupsList)) {
             return null;
         }


@@ -74,4 +74,14 @@ class OidcUserDetails
             return is_string($val);
         }));
     }
             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 62f19a119c04bc222d30052a242e5adb2fbcc180..49427cb12fd7d2f518b8295e3de67c1b723cf528 100644 (file)
--- 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),
 
     // 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),
     // 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),
     '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 af91dfe70f146236912948887e9dfff598a79053..1763dcbbd5840fe1a01f9d8b6d6e2bb014030979 100644 (file)
--- 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\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;
 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.
      */
     /**
      * 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);
         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) {
             $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
      */
      *
      * @throws HttpFetchException
      */

-    protected function getAvatarImageData(string $url, ?string $accessToken = null): string
+    protected function getAvatarImageData(string $url): string

     {
         try {
     {
         try {

-            $headers = [];
-            if (!empty($accessToken)) {
-                $headers['Authorization'] = 'Bearer ' . $accessToken;
-            }
-

             $client = $this->http->buildClient(5);
             $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]));
 
             if ($response->getStatusCode() !== 200) {
                 throw new HttpFetchException(trans('errors.cannot_get_image_from_url', ['url' => $url]));