From d05d2a348b407d4630708056d6ef1b7042f7f41a Mon Sep 17 00:00:00 2001 From: Hypolite Petovan Date: Thu, 2 Dec 2021 07:53:14 -0500 Subject: [PATCH 1/2] Deprecate Network::unparseURL in favor of UriInterfact objects - Added specific test for Probe::cleanURI --- src/Network/Probe.php | 20 ++++++-------- src/Util/Network.php | 3 +- tests/src/Network/ProbeTest.php | 49 +++++++++++++++++++++++++++------ 3 files changed, 52 insertions(+), 20 deletions(-) diff --git a/src/Network/Probe.php b/src/Network/Probe.php index d8c1a91530..baae0d3d31 100644 --- a/src/Network/Probe.php +++ b/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); - - if (empty($parts['scheme'])) { - return $URI; + $uri = new Uri($rawUri); + if (!$uri->getScheme()) { + return $uri->__toString(); } // 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; + return $uri->__toString(); } /** diff --git a/src/Util/Network.php b/src/Util/Network.php index 2631fc75b0..0d98cab2ed 100644 --- a/src/Util/Network.php +++ b/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) { diff --git a/tests/src/Network/ProbeTest.php b/tests/src/Network/ProbeTest.php index 79c323adc2..3b6b2e3a10 100644 --- a/tests/src/Network/ProbeTest.php +++ b/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', - 'assertUri' => 'Artists4Future_Muenchen@climatejustice.global', + 'expected' => '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::assertEquals($assertUri, $cleaned); - self::assertArraySubset($assertInfos, Probe::uri($cleaned, '', 0)); - + self::assertArraySubset($assertInfos, Probe::uri($uri, '', 0)); // Iterate over the requests and responses foreach ($container as $transaction) { From 67f40380511d2ccd5893eee954f3aee4afcddea6 Mon Sep 17 00:00:00 2001 From: Hypolite Petovan Date: Thu, 2 Dec 2021 07:54:48 -0500 Subject: [PATCH 2/2] Fix URI structure condition in Probe::detect - This condition was wrongly discarding all URIs with a schemes --- src/Network/Probe.php | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/src/Network/Probe.php b/src/Network/Probe.php index baae0d3d31..bcbadd82f0 100644 --- a/src/Network/Probe.php +++ b/src/Network/Probe.php @@ -686,22 +686,21 @@ class Probe } $parts = parse_url($uri); - - 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 { + if (empty($parts['scheme']) && empty($parts['host']) && !strstr($parts['path'], '@')) { 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)) {