Merge pull request #11056 from MrPetovan/bug/11055-probe-detect-url

Fix URI structure condition in Probe::detect

src/Network/Probe.php

@@ -44,6 +44,7 @@ use Friendica\Util\DateTimeFormat;
 use Friendica\Util\Network;
 use Friendica\Util\Strings;
 use Friendica\Util\XML;
+use GuzzleHttp\Psr7\Uri;
 
 /**
  * This class contain functions for probing URL
@@ -58,26 +59,23 @@ class Probe
 	/**
 	 * Remove stuff from an URI that doesn't belong there
 	 *
-	 * @param string $URI
+	 * @param string $rawUri
 	 * @return string Cleaned URI
 	 */
-	public static function cleanURI(string $URI)
+	public static function cleanURI(string $rawUri): string
 	{
 		// At first remove leading and trailing junk
-		$URI = trim($URI, "@#?:/ \t\n\r\0\x0B");
+		$rawUri = trim($rawUri, "@#?:/ \t\n\r\0\x0B");
 
-		$parts = parse_url($URI);
+		$uri = new Uri($rawUri);
-
+		if (!$uri->getScheme()) {
-		if (empty($parts['scheme'])) {
+			return $uri->__toString();
-			return $URI;
 		}
 
 		// Remove the URL fragment, since these shouldn't be part of any profile URL
-		unset($parts['fragment']);
+		$uri = $uri->withFragment('');
 
-		$URI = Network::unparseURL($parts);
+		return $uri->__toString();
-
-		return $URI;
 	}
 
 	/**
@@ -688,22 +686,21 @@ class Probe
 		}
 
 		$parts = parse_url($uri);
-
+		if (empty($parts['scheme']) && empty($parts['host']) && !strstr($parts['path'], '@')) {
-		if (empty($parts['scheme']) || !empty($parts['host']) && strstr($uri, '@')) {
-			// If the URI starts with "mailto:" then jump directly to the mail detection
-			if (strpos($uri, 'mailto:') !== false) {
-				$uri = str_replace('mailto:', '', $uri);
-				return self::mail($uri, $uid);
-			}
-
-			if ($network == Protocol::MAIL) {
-				return self::mail($uri, $uid);
-			}
-		} else {
 			Logger::info('URI was not detectable', ['uri' => $uri]);
 			return [];
 		}
 
+		// If the URI starts with "mailto:" then jump directly to the mail detection
+		if (strpos($uri, 'mailto:') !== false) {
+			$uri = str_replace('mailto:', '', $uri);
+			return self::mail($uri, $uid);
+		}
+
+		if ($network == Protocol::MAIL) {
+			return self::mail($uri, $uid);
+		}
+
 		Logger::info('Probing start', ['uri' => $uri]);
 
 		if (!empty($ap_profile['addr']) && ($ap_profile['addr'] != $uri)) {

src/Util/Network.php

@@ -435,7 +435,8 @@ class Network
 	 *
 	 * @param array $parsed URL parts
 	 *
-	 * @return string The glued URL
+	 * @return string The glued URL.
+	 * @deprecated since version 2021.12, use a UriInterface object like GuzzleHttp\Psr7\Uri instead
 	 */
 	public static function unparseURL(array $parsed)
 	{

tests/src/Network/ProbeTest.php

@@ -117,12 +117,48 @@ class ProbeTest extends FixtureTest
 		}
 	}
 
-	public function dataUri()
+	public function dataCleanUri(): array
 	{
 		return [
 			'@-first' => [
-				'uri'         => '@Artists4Future_Muenchen@climatejustice.global',
+				'expected' => 'Artists4Future_Muenchen@climatejustice.global',
-				'assertUri'   => 'Artists4Future_Muenchen@climatejustice.global',
+				'uri'      => '@Artists4Future_Muenchen@climatejustice.global',
+			],
+			'no-scheme-no-fragment' => [
+				'expected' => 'example.com/path?arg=value',
+				'uri'      => 'example.com/path?arg=value',
+			],
+			/* This case makes little sense, both in our expectation of receiving it in any context and in the way we
+			 * do not change it in Probe::cleanUri, but it doesn't seem to be the source of any terrible security hole.
+			 */
+			'no-scheme-fragment' => [
+				'expected' => 'example.com/path?arg=value#fragment',
+				'uri'      => 'example.com/path?arg=value#fragment',
+			],
+			'scheme-no-fragment' => [
+				'expected' => 'https://example.com/path?arg=value',
+				'uri'      => 'https://example.com/path?arg=value#fragment',
+			],
+			'scheme-fragment' => [
+				'expected' => 'https://example.com/path?arg=value',
+				'uri'      => 'https://example.com/path?arg=value#fragment',
+			],
+		];
+	}
+
+	/**
+	 * @dataProvider dataCleanUri
+	 */
+	public function testCleanUri(string $expected, string $uri)
+	{
+		self::assertEquals($expected, Probe::cleanURI($uri));
+	}
+
+	public function dataUri(): array
+	{
+		return [
+			'Artists4Future_Muenchen@climatejustice.global' => [
+				'uri'         => 'Artists4Future_Muenchen@climatejustice.global',
 				'assertInfos' => [
 					'name'         => 'Artists4Future München',
 					'nick'         => 'Artists4Future_Muenchen',
@@ -163,7 +199,7 @@ xQIDAQAB
 	/**
 	 * @dataProvider dataUri
 	 */
-	public function testCleanUri(string $uri, string $assertUri, array $assertInfos)
+	public function testProbeUri(string $uri, array $assertInfos)
 	{
 		self::markTestIncomplete('hard work due mocking 19 different http-requests');
 
@@ -216,10 +252,7 @@ xQIDAQAB
 
 		$this->httpRequestHandler->push($history);
 
-		$cleaned = Probe::cleanURI($uri);
+		self::assertArraySubset($assertInfos, Probe::uri($uri, '', 0));
-		self::assertEquals($assertUri, $cleaned);
-		self::assertArraySubset($assertInfos, Probe::uri($cleaned, '', 0));
-
 
 		// Iterate over the requests and responses
 		foreach ($container as $transaction) {