From 27ef14bd2a373d8741ba724b3a665f575c0d3063 Mon Sep 17 00:00:00 2001
From: albertlast <mathiaspapealbert@hotmail.com>
Date: Sun, 30 Aug 2026 18:47:11 +0200
Subject: [PATCH] Ends the search for an avatar at the last resort

The Avatar constructor tries each way of finding an image in turn, inside a
loop that runs until it holds a URL that Url::isValid() accepts. The last of
those ways is a 1x1 transparent GIF as a data URI, which is there so that there
is always an answer. A data URI is not a URL that filter_var() will validate,
so that answer never satisfied the condition, and the loop went round again,
and again, until the request was killed.

Reaching the last resort takes nothing exotic. The step before it only produces
a URL when avatar_url is set, so a forum where that setting was never written
hangs a PHP worker on every member who has an avatar.

Leaving the loop rather than the switch is the whole change: at that point
there is nothing further to try, which is what the case is for.

Signed-off-by: albertlast <mathiaspapealbert@hotmail.com>
---
 Sources/Avatar.php                |   7 +-
 tests/Unit/AvatarFallbackTest.php | 110 ++++++++++++++++++++++++++++++
 2 files changed, 116 insertions(+), 1 deletion(-)
 create mode 100644 tests/Unit/AvatarFallbackTest.php

diff --git a/Sources/Avatar.php b/Sources/Avatar.php
index 7df9e0605e..940361a969 100644
--- a/Sources/Avatar.php
+++ b/Sources/Avatar.php
@@ -587,9 +587,14 @@ public function __construct(
 					break;
 
 				// Last ditch fallback is a transparent 1x1 GIF.
+				//
+				// This leaves the loop rather than the switch, because there is
+				// nothing left to try and because a data URI is not a URL that
+				// Url::isValid() will accept, so the condition the loop tests
+				// can never become false from here.
 				default:
 					$url = new Url('data:image/gif;base64,R0lGODlhAQABAIAAAAAAAP///yH5BAEAAAAALAAAAAABAAEAAAIBRAA7');
-					break;
+					break 2;
 			}
 		}
 
diff --git a/tests/Unit/AvatarFallbackTest.php b/tests/Unit/AvatarFallbackTest.php
new file mode 100644
index 0000000000..d66465eefe
--- /dev/null
+++ b/tests/Unit/AvatarFallbackTest.php
@@ -0,0 +1,110 @@
+<?php
+
+declare(strict_types=1);
+
+namespace SMF\Tests\Unit;
+
+use PHPUnit\Framework\Attributes\CoversClass;
+use PHPUnit\Framework\TestCase;
+use SMF\Avatar;
+use SMF\Config;
+
+/**
+ * Covers what SMF\Avatar settles on when it can find no image at all.
+ *
+ * The constructor only asks the database who owns an attachment, so handing it
+ * an id_member keeps the whole thing on this side of the line.
+ */
+#[CoversClass(Avatar::class)]
+class AvatarFallbackTest extends TestCase
+{
+	/*********************
+	 * Internal properties
+	 *********************/
+
+	/**
+	 * @var array The settings this class reads, as they were before the test.
+	 */
+	private array $backup = [];
+
+	/**
+	 * @var string Config::$boardurl as it was before the test.
+	 */
+	private string $boardurl = '';
+
+	/****************
+	 * Public methods
+	 ****************/
+
+	/**
+	 * The constructor tries each way of finding an image in turn, and stops
+	 * when it has a URL that validates. The last of those ways is a 1x1
+	 * transparent GIF as a data URI, which is there so that there is always an
+	 * answer - but a data URI is not something Url::isValid() accepts, so it
+	 * never satisfied the condition that ends the search and the search went
+	 * round again, and again, until the request was killed.
+	 *
+	 * Reaching it takes nothing exotic: the step before only produces a URL
+	 * when avatar_url is set, so a forum where that setting was never written
+	 * hung on any member with an avatar.
+	 */
+	public function testAnAvatarThatCannotBeFoundEndsAsATransparentGif(): void
+	{
+		Config::$boardurl = 'https://example.com';
+		Config::$modSettings['gravatarEnabled'] = false;
+		unset(Config::$modSettings['avatar_url']);
+
+		$avatar = new Avatar(url: 'Oxygen/beagle.png', id_member: 1);
+
+		$this->assertStringStartsWith('data:image/gif;base64,', (string) $avatar->url);
+	}
+
+	/**
+	 * The control, and the reason the loop exists: when there is somewhere to
+	 * look, it is looked in, and the search ends long before the last resort.
+	 */
+	public function testAnAvatarThatCanBeFoundIsStillFound(): void
+	{
+		Config::$boardurl = 'https://example.com';
+		Config::$modSettings['gravatarEnabled'] = false;
+		Config::$modSettings['avatar_url'] = 'https://example.com/avatars';
+
+		$avatar = new Avatar(url: 'Oxygen/beagle.png', id_member: 1);
+
+		$this->assertSame('https://example.com/avatars/Oxygen/beagle.png', (string) $avatar->url);
+	}
+
+	/******************
+	 * Internal methods
+	 ******************/
+
+	protected function setUp(): void
+	{
+		$this->boardurl = Config::$boardurl ?? '';
+
+		foreach (['avatar_url', 'gravatarEnabled'] as $key) {
+			if (isset(Config::$modSettings[$key])) {
+				$this->backup[$key] = Config::$modSettings[$key];
+			}
+		}
+	}
+
+	/**
+	 * PHPUnit does not reset SMF's statics between tests, so a setting left
+	 * behind here would leak into every test that follows.
+	 */
+	protected function tearDown(): void
+	{
+		Config::$boardurl = $this->boardurl;
+
+		foreach (['avatar_url', 'gravatarEnabled'] as $key) {
+			unset(Config::$modSettings[$key]);
+
+			if (isset($this->backup[$key])) {
+				Config::$modSettings[$key] = $this->backup[$key];
+			}
+		}
+
+		$this->backup = [];
+	}
+}