From b665aa43cf141cc5c1475282185811b31b993408 Mon Sep 17 00:00:00 2001
From: albertlast <mathiaspapealbert@hotmail.com>
Date: Sun, 30 Aug 2026 18:35:14 +0200
Subject: [PATCH] Covers four more merged fixes that the unit suite can reach

A second sweep of the bug fixes now on release-3.0, in the same spirit as
#9511: ask of each one whether the suite can reach it, and write a test where
it can. Everything merged since that sweep was looked at, along with the
backlog that landed in one batch on the 29th and 30th. Most of it is templates,
JavaScript, or PHP that wants Db::$db or User::$me. Four fixes do not.

#9484 made SMF\Unicode\SpoofDetector::checkReservedName() split the admin's
list on the two characters backslash and n as well as on a real newline. The
installer writes the default list with the separators spelled out that way, so
splitting on newlines alone gave one long name nobody would type and every
reserved name was free to register.

#9409 made SMF\Localization\MessageFormatter::formatMessage() flatten a
\Stringable argument to its string value. The class skips any argument that is
not already a string and hands the intl formatter only the scalar ones, so an
object argument reached neither and the member was shown the placeholder.

#9453 made SMF\PageIndex remember, across __toString(), that the start value it
was handed was out of bounds. fixStart() records that as a side effect of
clamping, and __toString() called it again on a value already clamped, so the
verdict was always thrown away: page 1 came out as plain text rather than a
link, with a "next page" link beside it.

#9440 and #9442 both concern a gallery avatar, which is stored as a path under
the avatars directory rather than as a URL. Read as a URL, it was worked back
to a file from the URL's path, which lands outside the avatar directories; and
on a forum at the root of its domain that path is null, so stripping the board
URL off it threw a TypeError on every page the member appeared on.

Each set was run against the code as it was before its fix, by checking out the
single source file at the commit before the merge:

- SpoofDetector.php before #9484: one failure, the installer's list.
- MessageFormatter.php before #9409: three failures, all the \Stringable cases.
  The plain string, the number and the no-placeholder message pass either side.
- PageIndex.php before #9453: two failures. The four tests covering an ordinary
  start pass either side, which is what makes them the control.
- Avatar.php before #9440: five of six fail, the root-of-domain cases with the
  TypeError and the subdirectory ones by falling through to default.png. With
  #9440 but not #9442, four still fail: every gallery avatar becomes the
  default image.

202 tests, 295 assertions, still under a second.

Signed-off-by: albertlast <mathiaspapealbert@hotmail.com>
---
 tests/Unit/AvatarTest.php           | 156 ++++++++++++++++++++++++
 tests/Unit/MessageFormatterTest.php | 104 ++++++++++++++++
 tests/Unit/PageIndexTest.php        | 179 +++++++++++++++++++++++++++
 tests/Unit/SpoofDetectorTest.php    | 183 ++++++++++++++++++++++++++++
 4 files changed, 622 insertions(+)
 create mode 100644 tests/Unit/AvatarTest.php
 create mode 100644 tests/Unit/MessageFormatterTest.php
 create mode 100644 tests/Unit/PageIndexTest.php
 create mode 100644 tests/Unit/SpoofDetectorTest.php

diff --git a/tests/Unit/AvatarTest.php b/tests/Unit/AvatarTest.php
new file mode 100644
index 0000000000..9f8de65816
--- /dev/null
+++ b/tests/Unit/AvatarTest.php
@@ -0,0 +1,156 @@
+<?php
+
+declare(strict_types=1);
+
+namespace SMF\Tests\Unit;
+
+use PHPUnit\Framework\Attributes\CoversClass;
+use PHPUnit\Framework\Attributes\DataProvider;
+use PHPUnit\Framework\TestCase;
+use SMF\Avatar;
+use SMF\Config;
+
+/**
+ * Covers how SMF\Avatar turns a stored avatar value into a URL.
+ *
+ * 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. The avatars it
+ * looks for here are the ones the repository ships in avatars/.
+ */
+#[CoversClass(Avatar::class)]
+class AvatarTest 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 control. Something with a scheme on it is an address to fetch from,
+	 * and is left as one.
+	 */
+	public function testARemoteAvatarUrlIsLeftAlone(): void
+	{
+		$this->setUpForum('https://example.com/forum');
+
+		$avatar = new Avatar(url: 'https://example.org/pictures/me.png', id_member: 1);
+
+		$this->assertSame('https://example.org/pictures/me.png', (string) $avatar->url);
+	}
+
+	/**
+	 * An avatar chosen from the gallery is stored as a path under the avatars
+	 * directory, not as a URL, so there is no scheme on it and no host in it.
+	 * Saying so up front means the file is looked for by name; reading it as a
+	 * URL instead and working back to a file from that URL's path lands outside
+	 * the avatar directories and finds nothing, and on a forum at the root of
+	 * its domain that path is null and stripping the board URL off it throws.
+	 */
+	#[DataProvider('boardUrls')]
+	public function testAGalleryAvatarIsFoundUnderTheAvatarsDirectory(string $boardurl): void
+	{
+		$this->setUpForum($boardurl);
+
+		$avatar = new Avatar(url: 'Oxygen/beagle.png', id_member: 1);
+
+		$this->assertSame($boardurl . '/avatars/Oxygen/beagle.png', (string) $avatar->url);
+		$this->assertSame('Oxygen/beagle.png', $avatar->filename);
+	}
+
+	#[DataProvider('boardUrls')]
+	public function testAGalleryAvatarInTheRootOfTheGalleryIsFoundToo(string $boardurl): void
+	{
+		$this->setUpForum($boardurl);
+
+		$avatar = new Avatar(url: 'default.png', id_member: 1);
+
+		$this->assertSame($boardurl . '/avatars/default.png', (string) $avatar->url);
+		$this->assertSame('default.png', $avatar->filename);
+	}
+
+	/**
+	 * A gallery file that is not there falls through to the default image
+	 * rather than producing a URL pointing at nothing.
+	 */
+	public function testAGalleryAvatarThatIsNotThereFallsBackToTheDefault(): void
+	{
+		$this->setUpForum('https://example.com');
+
+		$avatar = new Avatar(url: 'Oxygen/no_such_avatar.png', id_member: 1);
+
+		$this->assertSame('https://example.com/avatars/default.png', (string) $avatar->url);
+	}
+
+	/***********************
+	 * Public static methods
+	 ***********************/
+
+	/**
+	 * @return array<string, array{string}>
+	 */
+	public static function boardUrls(): array
+	{
+		return [
+			'a forum at the root of its domain' => ['https://example.com'],
+			'a forum in a subdirectory' => ['https://example.com/forum'],
+		];
+	}
+
+	/******************
+	 * Internal methods
+	 ******************/
+
+	protected function setUp(): void
+	{
+		$this->boardurl = Config::$boardurl ?? '';
+
+		foreach (['avatar_url', 'avatar_directory', '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', 'avatar_directory', 'gravatarEnabled'] as $key) {
+			unset(Config::$modSettings[$key]);
+
+			if (isset($this->backup[$key])) {
+				Config::$modSettings[$key] = $this->backup[$key];
+			}
+		}
+
+		$this->backup = [];
+	}
+
+	/**
+	 * Points the avatar settings at the gallery the repository ships.
+	 */
+	private function setUpForum(string $boardurl): void
+	{
+		Config::$boardurl = $boardurl;
+		Config::$modSettings['avatar_url'] = $boardurl . '/avatars';
+		Config::$modSettings['avatar_directory'] = Config::$boarddir . '/avatars';
+		Config::$modSettings['gravatarEnabled'] = false;
+	}
+}
diff --git a/tests/Unit/MessageFormatterTest.php b/tests/Unit/MessageFormatterTest.php
new file mode 100644
index 0000000000..93b054a948
--- /dev/null
+++ b/tests/Unit/MessageFormatterTest.php
@@ -0,0 +1,104 @@
+<?php
+
+declare(strict_types=1);
+
+namespace SMF\Tests\Unit;
+
+use PHPUnit\Framework\Attributes\CoversClass;
+use PHPUnit\Framework\TestCase;
+use SMF\Localization\MessageFormatter;
+
+/**
+ * Covers SMF\Localization\MessageFormatter::formatMessage().
+ *
+ * It reads one language string of its own, 'lang_locale', which comes off disk
+ * like any other, so the whole class is reachable without a forum behind it.
+ */
+#[CoversClass(MessageFormatter::class)]
+class MessageFormatterTest extends TestCase
+{
+	/****************
+	 * Public methods
+	 ****************/
+
+	/**
+	 * An object that can be a string is one of the things a caller may hand to
+	 * Lang::getTxt(), and several of SMF's own value objects are exactly that:
+	 * Url, IP, TimeInterval and PageIndex all implement \Stringable. The class
+	 * skips any argument that is not already a string, and the intl formatter
+	 * is handed only the scalar ones, so an object reached neither and its
+	 * placeholder was printed to the member as it was written.
+	 */
+	public function testAStringableArgumentIsUsedForItsStringValue(): void
+	{
+		$this->assertSame(
+			'Hello Bob!',
+			MessageFormatter::formatMessage('Hello {name}!', ['name' => $this->stringable('Bob')]),
+		);
+	}
+
+	/**
+	 * The braces and apostrophes in an argument are swapped for private use
+	 * characters before the message is formatted and swapped back afterwards,
+	 * so that a value cannot be read as MessageFormat syntax. A \Stringable is
+	 * flattened early enough to go through that too.
+	 */
+	public function testMessageFormatSyntaxInAStringableValueIsNotInterpreted(): void
+	{
+		$this->assertSame(
+			"Hello it's {here}!",
+			MessageFormatter::formatMessage('Hello {name}!', ['name' => $this->stringable("it's {here}")]),
+		);
+	}
+
+	public function testTheSameStringableCanBeUsedTwiceInOneMessage(): void
+	{
+		$this->assertSame(
+			'Bob and Bob',
+			MessageFormatter::formatMessage('{name} and {name}', ['name' => $this->stringable('Bob')]),
+		);
+	}
+
+	public function testAPlainStringArgumentIsUnaffected(): void
+	{
+		$this->assertSame(
+			'Hello Ann!',
+			MessageFormatter::formatMessage('Hello {name}!', ['name' => 'Ann']),
+		);
+	}
+
+	public function testANumberArgumentIsStillFormattedAsANumber(): void
+	{
+		$this->assertSame(
+			'2 posts',
+			MessageFormatter::formatMessage('{count, plural, one {# post} other {# posts}}', ['count' => 2]),
+		);
+	}
+
+	public function testAMessageWithNoPlaceholdersComesBackUnchanged(): void
+	{
+		$this->assertSame(
+			'Nothing to substitute',
+			MessageFormatter::formatMessage('Nothing to substitute', ['name' => $this->stringable('Bob')]),
+		);
+	}
+
+	/******************
+	 * Internal methods
+	 ******************/
+
+	/**
+	 * The simplest thing that is a string without being one.
+	 */
+	private function stringable(string $value): \Stringable
+	{
+		return new class ($value) implements \Stringable {
+			public function __construct(private string $value) {}
+
+			public function __toString(): string
+			{
+				return $this->value;
+			}
+		};
+	}
+}
diff --git a/tests/Unit/PageIndexTest.php b/tests/Unit/PageIndexTest.php
new file mode 100644
index 0000000000..08b85ac679
--- /dev/null
+++ b/tests/Unit/PageIndexTest.php
@@ -0,0 +1,179 @@
+<?php
+
+declare(strict_types=1);
+
+namespace SMF\Tests\Unit;
+
+use PHPUnit\Framework\Attributes\CoversClass;
+use PHPUnit\Framework\TestCase;
+use SMF\Config;
+use SMF\PageIndex;
+use SMF\Utils;
+
+/**
+ * Covers SMF\PageIndex.
+ *
+ * It builds a string out of its own properties, a couple of modSettings keys
+ * and one language string, and it takes the theme's page_index settings only
+ * if a theme has been loaded. Nothing here needs a database.
+ */
+#[CoversClass(PageIndex::class)]
+class PageIndexTest extends TestCase
+{
+	/*********************
+	 * Internal properties
+	 *********************/
+
+	/**
+	 * @var array The settings this class reads, as they were before the test.
+	 */
+	private array $backup = [];
+
+	/**
+	 * @var bool Whether Utils::$context already had a current_page.
+	 */
+	private bool $had_current_page = false;
+
+	/****************
+	 * Public methods
+	 ****************/
+
+	/**
+	 * A negative start is how a caller says "no particular page was asked
+	 * for". The message index passes one for a topic whose first page is being
+	 * linked, and the page index answers by clamping the start to zero and
+	 * linking page 1 rather than marking it as the page you are on.
+	 *
+	 * fixStart() records that verdict as a side effect of clamping, and
+	 * __toString() calls it again on a start that has already been clamped, so
+	 * the second answer is always "valid" and the first was thrown away. Page 1
+	 * came out as plain text with no link on it, and a "next page" link
+	 * appeared beside it pointing at page 2.
+	 */
+	public function testANegativeStartLinksTheFirstPageInsteadOfMarkingIt(): void
+	{
+		$start = -1;
+		$page_index = new PageIndex('https://example.com/index.php?board=1.0', $start, 100, 20);
+
+		$this->assertStringContainsString(
+			'<a class="nav_page" href="https://example.com/index.php?board=1.0;start=0">1</a>',
+			(string) $page_index,
+		);
+
+		$this->assertStringNotContainsString('current_page', (string) $page_index);
+	}
+
+	/**
+	 * Nothing was navigated away from, so there is nowhere to go back to and
+	 * nothing to go on to.
+	 */
+	public function testANegativeStartShowsNeitherPreviousNorNextLinks(): void
+	{
+		$start = -1;
+		$page_index = new PageIndex('https://example.com/index.php?board=1.0', $start, 100, 20);
+
+		$this->assertStringNotContainsString('previous_page', (string) $page_index);
+		$this->assertStringNotContainsString('next_page', (string) $page_index);
+	}
+
+	/**
+	 * The string is built in __toString() rather than the constructor so that
+	 * it reflects any property the caller changed in between, which means it
+	 * has to survive being asked more than once.
+	 */
+	public function testAskingTwiceGivesTheSameAnswer(): void
+	{
+		$start = -1;
+		$page_index = new PageIndex('https://example.com/index.php?board=1.0', $start, 100, 20);
+
+		$this->assertSame((string) $page_index, (string) $page_index);
+	}
+
+	public function testTheStartValueIsClampedAndHandedBackToTheCaller(): void
+	{
+		$start = -1;
+		new PageIndex('https://example.com/index.php?board=1.0', $start, 100, 20);
+
+		$this->assertSame(0, $start);
+	}
+
+	/**
+	 * The control. A start that names a real page marks that page as the
+	 * current one and links the pages either side of it.
+	 */
+	public function testAStartThatNamesAPageMarksThatPageAsCurrent(): void
+	{
+		$start = 40;
+		$page_index = new PageIndex('https://example.com/index.php?board=1.0', $start, 100, 20);
+
+		$this->assertStringContainsString('<span class="current_page">3</span>', (string) $page_index);
+		$this->assertStringContainsString('previous_page', (string) $page_index);
+		$this->assertStringContainsString('next_page', (string) $page_index);
+	}
+
+	/**
+	 * A start in the middle of a page belongs to that page, and the caller is
+	 * told which page that turned out to be.
+	 */
+	public function testAStartInTheMiddleOfAPageIsMovedToItsStart(): void
+	{
+		$start = 45;
+		$page_index = new PageIndex('https://example.com/index.php?board=1.0', $start, 100, 20);
+
+		$this->assertSame(40, $start);
+		$this->assertStringContainsString('<span class="current_page">3</span>', (string) $page_index);
+	}
+
+	/**
+	 * A start past the end is clamped to the last page, and that is a real
+	 * page, so it is marked rather than linked.
+	 */
+	public function testAStartPastTheEndLandsOnTheLastPage(): void
+	{
+		$start = 500;
+		$page_index = new PageIndex('https://example.com/index.php?board=1.0', $start, 100, 20);
+
+		$this->assertSame(80, $start);
+		$this->assertStringContainsString('<span class="current_page">5</span>', (string) $page_index);
+		$this->assertStringNotContainsString('next_page', (string) $page_index);
+	}
+
+	/******************
+	 * Internal methods
+	 ******************/
+
+	protected function setUp(): void
+	{
+		foreach (['compactTopicPagesEnable', 'compactTopicPagesContiguous'] as $key) {
+			if (isset(Config::$modSettings[$key])) {
+				$this->backup[$key] = Config::$modSettings[$key];
+			}
+
+			unset(Config::$modSettings[$key]);
+		}
+
+		$this->had_current_page = isset(Utils::$context['current_page']);
+	}
+
+	/**
+	 * PHPUnit does not reset SMF's statics between tests, and the constructor
+	 * fills in Utils::$context['current_page'] when nothing else has, so both
+	 * that and the settings have to go back the way they were.
+	 */
+	protected function tearDown(): void
+	{
+		foreach (['compactTopicPagesEnable', 'compactTopicPagesContiguous'] as $key) {
+			unset(Config::$modSettings[$key]);
+
+			if (isset($this->backup[$key])) {
+				Config::$modSettings[$key] = $this->backup[$key];
+			}
+		}
+
+		if (!$this->had_current_page) {
+			unset(Utils::$context['current_page']);
+		}
+
+		$this->backup = [];
+	}
+}
diff --git a/tests/Unit/SpoofDetectorTest.php b/tests/Unit/SpoofDetectorTest.php
new file mode 100644
index 0000000000..360dff2aab
--- /dev/null
+++ b/tests/Unit/SpoofDetectorTest.php
@@ -0,0 +1,183 @@
+<?php
+
+declare(strict_types=1);
+
+namespace SMF\Tests\Unit;
+
+use PHPUnit\Framework\Attributes\CoversClass;
+use PHPUnit\Framework\Attributes\DataProvider;
+use PHPUnit\Framework\TestCase;
+use SMF\Config;
+use SMF\Unicode\SpoofDetector;
+
+/**
+ * Covers the part of the spoof detector that needs no database.
+ *
+ * checkReservedName() reads the admin's list out of
+ * Config::$modSettings['reserveNames'] and compares Unicode skeletons, so it
+ * is reachable from here. checkSimilarMemberName() and checkSimilarGroupName()
+ * ask the members and membergroups tables what else is out there, and are not.
+ */
+#[CoversClass(SpoofDetector::class)]
+class SpoofDetectorTest extends TestCase
+{
+	/*********************
+	 * Internal properties
+	 *********************/
+
+	/**
+	 * @var array The settings this class reads, as they were before the test.
+	 */
+	private array $backup = [];
+
+	/****************
+	 * Public methods
+	 ****************/
+
+	/**
+	 * The installer stores the default list with the separators written out as
+	 * the two characters backslash and n, because the value travels through
+	 * Table::populate() as a placeholder in a PHP string. Splitting it on real
+	 * newlines alone gives one long name that nobody would ever type, so every
+	 * name an admin put on the list was free to register.
+	 */
+	public function testTheListTheInstallerWritesIsOneNamePerLine(): void
+	{
+		Config::$modSettings['reserveNames'] = 'Admin\nWebmaster\nGuest\nroot';
+
+		$this->assertTrue(SpoofDetector::checkReservedName('Admin'));
+		$this->assertTrue(SpoofDetector::checkReservedName('Webmaster'));
+		$this->assertTrue(SpoofDetector::checkReservedName('Guest'));
+		$this->assertTrue(SpoofDetector::checkReservedName('root'));
+	}
+
+	/**
+	 * A list an admin edited by hand arrives with real line breaks, whichever
+	 * kind their browser sent.
+	 */
+	public function testAListWrittenWithRealLineBreaksStillWorks(): void
+	{
+		Config::$modSettings['reserveNames'] = "Admin\nWebmaster";
+
+		$this->assertTrue(SpoofDetector::checkReservedName('Webmaster'));
+
+		Config::$modSettings['reserveNames'] = "Admin\r\nWebmaster";
+
+		$this->assertTrue(SpoofDetector::checkReservedName('Webmaster'));
+	}
+
+	public function testANameNobodyReservedIsAllowed(): void
+	{
+		Config::$modSettings['reserveNames'] = 'Admin\nWebmaster\nGuest\nroot';
+
+		$this->assertFalse(SpoofDetector::checkReservedName('Somebody'));
+	}
+
+	public function testAnEmptyListReservesNothing(): void
+	{
+		Config::$modSettings['reserveNames'] = '';
+
+		$this->assertFalse(SpoofDetector::checkReservedName('Admin'));
+	}
+
+	/**
+	 * The point of the class: a name is compared by its skeleton, so a
+	 * character that merely looks like the one on the list counts as being on
+	 * the list. U+0410 is Cyrillic capital A.
+	 */
+	public function testACharacterThatMerelyLooksTheSameIsStillReserved(): void
+	{
+		Config::$modSettings['reserveNames'] = 'Admin';
+
+		$this->assertTrue(SpoofDetector::checkReservedName("\u{0410}dmin"));
+	}
+
+	/**
+	 * The admin's list and the name being checked are both decoded first, so
+	 * neither side can hide behind an entity.
+	 */
+	public function testAnEntityIsDecodedBeforeComparison(): void
+	{
+		Config::$modSettings['reserveNames'] = 'Webmaster';
+
+		$this->assertTrue(SpoofDetector::checkReservedName('Web&#109;aster'));
+	}
+
+	#[DataProvider('reserveWordCases')]
+	public function testReserveWordDecidesWhetherPartOfANameCounts(int $reserve_word, string $name, bool $expected): void
+	{
+		Config::$modSettings['reserveNames'] = 'Admin';
+		Config::$modSettings['reserveWord'] = $reserve_word;
+
+		$this->assertSame($expected, SpoofDetector::checkReservedName($name));
+	}
+
+	#[DataProvider('reserveCaseCases')]
+	public function testReserveCaseDecidesWhetherTheCaseHasToMatch(int $reserve_case, string $name, bool $expected): void
+	{
+		Config::$modSettings['reserveNames'] = 'Admin';
+		Config::$modSettings['reserveCase'] = $reserve_case;
+
+		$this->assertSame($expected, SpoofDetector::checkReservedName($name));
+	}
+
+	/***********************
+	 * Public static methods
+	 ***********************/
+
+	/**
+	 * @return array<string, array{int, string, bool}>
+	 */
+	public static function reserveWordCases(): array
+	{
+		return [
+			'a reserved name inside a longer one, matching anywhere' => [0, 'SuperAdmin', true],
+			'a reserved name inside a longer one, whole names only' => [1, 'SuperAdmin', false],
+			'the reserved name itself, matching anywhere' => [0, 'Admin', true],
+			'the reserved name itself, whole names only' => [1, 'Admin', true],
+		];
+	}
+
+	/**
+	 * @return array<string, array{int, string, bool}>
+	 */
+	public static function reserveCaseCases(): array
+	{
+		return [
+			'a different case, compared caselessly' => [0, 'admin', true],
+			'a different case, compared exactly' => [1, 'admin', false],
+			'the same case, compared caselessly' => [0, 'Admin', true],
+			'the same case, compared exactly' => [1, 'Admin', true],
+		];
+	}
+
+	/******************
+	 * Internal methods
+	 ******************/
+
+	protected function setUp(): void
+	{
+		foreach (['reserveNames', 'reserveCase', 'reserveWord'] 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
+	{
+		foreach (['reserveNames', 'reserveCase', 'reserveWord'] as $key) {
+			unset(Config::$modSettings[$key]);
+
+			if (isset($this->backup[$key])) {
+				Config::$modSettings[$key] = $this->backup[$key];
+			}
+		}
+
+		$this->backup = [];
+	}
+}