From 124162fee45267717e3293f51a4e8aa6495cb20d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bartosz=20Dziewo=C5=84ski?= <dziewonski@fastmail.fm>
Date: Tue, 19 Nov 2024 23:14:35 +0100
Subject: [PATCH] DiscussionParser: Don't construct Users with invalid names
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The comment said "don't validate the username - anon (IP) is fine!"
– but it also allowed invalid names, not just IPs, and those are not
really fine.

Also add more test cases and remove some unused test code.

Bug: T380242
Change-Id: Id98f14a0663f33eb5e45045bcd2df6a1e1f52de6
---
 includes/DiscussionParser.php          | 13 +++++++++--
 tests/phpunit/DiscussionParserTest.php | 32 ++++++++++++++++++--------
 2 files changed, 34 insertions(+), 11 deletions(-)

diff --git a/includes/DiscussionParser.php b/includes/DiscussionParser.php
index 5c8db881a..ebdbea29b 100644
--- a/includes/DiscussionParser.php
+++ b/includes/DiscussionParser.php
@@ -1117,6 +1117,8 @@ abstract class DiscussionParser {
 	 */
 	public static function getUserFromLine( $line, ?Title $title = null ) {
 		$parser = MediaWikiServices::getInstance()->getParser();
+		$userNameUtils = MediaWikiServices::getInstance()->getUserNameUtils();
+		$userFactory = MediaWikiServices::getInstance()->getUserFactory();
 
 		/*
 		 * First we call extractUsersFromLine to get all the potential usernames
@@ -1129,8 +1131,15 @@ abstract class DiscussionParser {
 		foreach ( $usernames as $username ) {
 			// generate (dateless) signature from the user we think we've
 			// discovered the signature from
-			// don't validate the username - anon (IP) is fine!
-			$user = User::newFromName( $username, false );
+			if ( $userNameUtils->isIP( $username ) ) {
+				$user = $userFactory->newAnonymous( $username );
+			} else {
+				$user = $userFactory->newFromName( $username );
+				if ( !$user ) {
+					// Invalid username, so this link can't be any user's signature
+					continue;
+				}
+			}
 			$sig = $parser->preSaveTransform(
 				'~~~',
 				$title ?: Title::newMainPage(),
diff --git a/tests/phpunit/DiscussionParserTest.php b/tests/phpunit/DiscussionParserTest.php
index 028d6c0d6..7f1d24aaa 100644
--- a/tests/phpunit/DiscussionParserTest.php
+++ b/tests/phpunit/DiscussionParserTest.php
@@ -1047,15 +1047,7 @@ TEXT
 
 		$output = DiscussionParser::getUserFromLine( $line );
 
-		if ( $output === false ) {
-			$this->assertFalse( $expectedUser );
-		} elseif ( is_array( $expectedUser ) ) {
-			// Sometimes testing for correct user detection,
-			// sometimes testing for offset detection
-			$this->assertEquals( $expectedUser, $output );
-		} else {
-			$this->assertEquals( $expectedUser, $output[1] );
-		}
+		$this->assertEquals( $expectedUser, $output );
 	}
 
 	public static function signingDetectionDataProvider() {
@@ -1090,10 +1082,32 @@ TEXT
 					'127.0.0.1'
 				],
 			],
+			"Anonymous user (not a standard signature - userpage link)" => [
+				"I am anonymous because I like my IP address. --[[User:127.0.0.1|127.0.0.1]] $ts",
+				false,
+			],
 			"No signature" => [
 				"Well, \nI do think that [[User:Newyorkbrad]] is pretty cool, but what do I know?",
 				false
 			],
+			"Invalid username link" => [
+				"I'm evil! [[User:Template:Invalid|Invalid]] $ts",
+				false
+			],
+			"Invalid username link, followed by a valid one" => [
+				"I'm silly! --[[User:JarJar|JarJar]] ([[User talk:JarJar|talk]]) [[User:Template:Invalid|Invalid]] $ts",
+				[
+					13,
+					'JarJar'
+				]
+			],
+			"Invalid username link, preceded by a valid one" => [
+				"I'm silly! [[User:Template:Invalid|Invalid]] --[[User:JarJar|JarJar]] ([[User talk:JarJar|talk]]) $ts",
+				[
+					13 + 34,
+					'JarJar'
+				]
+			],
 			"Hash symbols in usernames" => [
 				"What do you think? [[User talk:We buried our secrets in the garden#top|wbositg]] $ts",
 				[