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.

diff --git a/app/Config/oidc.php b/app/Config/oidc.php
index 1d3193eeecc3901fec6b71a969c89fc21d992338..16bec873c9c7e4133e57670ede2500acb314e4e1 100644 (file)
--- 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.
 
     // 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
     'fetch_avatar' => env('OIDC_FETCH_AVATAR', false),
 
     // Group sync options

diff --git a/app/Uploads/UserAvatars.php b/app/Uploads/UserAvatars.php
index 2c9f7a8482eecb1cd1342df0d3b53f50f28cc578..0cc640f225c118adf7e00829cba67d1dac19001a 100644 (file)
--- 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', [
             $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,
             ]);
                 'url'       => $avatarUrl,
                 'user_id'   => $user->id,
             ]);


@@ -141,7 +141,18 @@ class UserAvatars
     {
         try {
             $client = $this->http->buildClient(5);
     {
         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]));
 
             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 71f883ca6c69830def542c959e68faf9d8cf58b4..a0db1c2ba00905a52fb79f98a28a50f832e24528 100644 (file)
--- a/tests/Auth/OidcTest.php
+++ b/tests/Auth/OidcTest.php
@@ -536,6 +536,28 @@ class OidcTest extends TestCase
         $this->assertEquals($originalImageData, $newAvatarData);
     }
 
         $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' => '[email protected]',
+            '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', '=', '[email protected]')->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([
     public function test_login_group_sync()
     {
         config()->set([