From f7d16fc6266597d8960072ad6b7f2b090f52b89d Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Tue, 23 Jul 2024 21:04:34 +0200 Subject: [PATCH 1/7] [ticket/12479] Convert file_downloader to use guzzle instead of fsockopen PHPBB-12479 --- phpBB/phpbb/file_downloader.php | 138 ++++++----- tests/version/version_helper_remote_test.php | 247 ++++++++++++++++++- 2 files changed, 326 insertions(+), 59 deletions(-) diff --git a/phpBB/phpbb/file_downloader.php b/phpBB/phpbb/file_downloader.php index f55b825d2d3..b165198399b 100644 --- a/phpBB/phpbb/file_downloader.php +++ b/phpBB/phpbb/file_downloader.php @@ -13,91 +13,117 @@ namespace phpbb; +use GuzzleHttp\Client; +use GuzzleHttp\Exception\RequestException ; +use phpbb\exception\runtime_exception; +use function Amp\Promise\rethrow; + class file_downloader { + const OK = 200; + const NOT_FOUND = 404; + const REQUEST_TIMEOUT = 408; + /** @var string Error string */ protected $error_string = ''; /** @var int Error number */ protected $error_number = 0; + /** + * Create new guzzle client + * + * @param string $host + * @param int $port + * @param int $timeout + * + * @return Client + */ + protected function create_client(string $host, int $port = 443, int $timeout = 6): Client + { + // Only set URL scheme if not specified in URL + $url_parts = parse_url($host); + if (!isset($url_parts['scheme'])) + { + $host = (($port === 443) ? 'https://' : 'http://') . $host; + } + + // Initialize Guzzle client + return new Client([ + 'base_uri' => $host, + 'timeout' => $timeout, + ]); + } + /** * Retrieve contents from remotely stored file * - * @param string $host File host - * @param string $directory Directory file is in - * @param string $filename Filename of file to retrieve - * @param int $port Port to connect to; default: 80 - * @param int $timeout Connection timeout in seconds; default: 6 + * @param string $host File host + * @param string $directory Directory file is in + * @param string $filename Filename of file to retrieve + * @param int $port Port to connect to; default: 80 + * @param int $timeout Connection timeout in seconds; default: 6 * - * @return mixed File data as string if file can be read and there is no - * timeout, false if there were errors or the connection timed out + * @return false|string File data as string if file can be read and there is no + * timeout, false if there were errors or the connection timed out * - * @throws \phpbb\exception\runtime_exception If data can't be retrieved and no error - * message is returned + * @throws runtime_exception If data can't be retrieved and no error + * message is returned */ - public function get($host, $directory, $filename, $port = 80, $timeout = 6) + public function get(string $host, string $directory, string $filename, int $port = 443, int $timeout = 6) { + // Initialize Guzzle client + $client = $this->create_client($host, $port, $timeout); + // Set default values for error variables $this->error_number = 0; $this->error_string = ''; - if (function_exists('fsockopen') && - $socket = @fsockopen(($port == 443 ? 'ssl://' : '') . $host, $port, $this->error_number, $this->error_string, $timeout) - ) + try { - @fputs($socket, "GET $directory/$filename HTTP/1.0\r\n"); - @fputs($socket, "HOST: $host\r\n"); - @fputs($socket, "Connection: close\r\n\r\n"); - - $timer_stop = time() + $timeout; - stream_set_timeout($socket, $timeout); + $response = $client->request('GET', "$directory/$filename"); - $file_info = ''; - $get_info = false; - - while (!@feof($socket)) + // Check if the response status code is 200 (OK) + if ($response->getStatusCode() == self::OK) { - if ($get_info) - { - $file_info .= @fread($socket, 1024); - } - else - { - $line = @fgets($socket, 1024); - if ($line == "\r\n") - { - $get_info = true; - } - else if (stripos($line, '404 not found') !== false) - { - throw new \phpbb\exception\runtime_exception('FILE_NOT_FOUND', array($filename)); - } - } - - $stream_meta_data = stream_get_meta_data($socket); - - if (!empty($stream_meta_data['timed_out']) || time() >= $timer_stop) - { - throw new \phpbb\exception\runtime_exception('FSOCK_TIMEOUT'); - } + return $response->getBody()->getContents(); + } + else + { + $this->error_number = $response->getStatusCode(); + throw new runtime_exception('FILE_NOT_FOUND', [$filename]); } - @fclose($socket); } - else + catch (RequestException $exception) { - if ($this->error_string) + if ($exception->hasResponse()) { - $this->error_string = utf8_convert_message($this->error_string); - return false; + $this->error_number = $exception->getResponse()->getStatusCode(); + + if ($this->error_number == self::NOT_FOUND) + { + throw new runtime_exception('FILE_NOT_FOUND', [$filename]); + } } else { - throw new \phpbb\exception\runtime_exception('FSOCK_DISABLED'); + $this->error_number = self::REQUEST_TIMEOUT; + throw new runtime_exception('FSOCK_TIMEOUT'); } - } - return $file_info; + $this->error_string = utf8_convert_message($exception->getMessage()); + return false; + } + catch (runtime_exception $exception) + { + // Rethrow runtime_exceptions, only handle unknown cases below + throw $exception; + } + catch (\Throwable $exception) + { + $this->error_string = utf8_convert_message($exception->getMessage()); + throw new runtime_exception('FSOCK_DISABLED'); + } } /** @@ -105,7 +131,7 @@ public function get($host, $directory, $filename, $port = 80, $timeout = 6) * * @return string Error string */ - public function get_error_string() + public function get_error_string(): string { return $this->error_string; } @@ -115,7 +141,7 @@ public function get_error_string() * * @return int Error number */ - public function get_error_number() + public function get_error_number(): int { return $this->error_number; } diff --git a/tests/version/version_helper_remote_test.php b/tests/version/version_helper_remote_test.php index 762493986a6..ce2d110fdc3 100644 --- a/tests/version/version_helper_remote_test.php +++ b/tests/version/version_helper_remote_test.php @@ -1,4 +1,7 @@ method('get') ->with($this->anything()) ->will($this->returnValue(false)); - $this->file_downloader = new phpbb_mock_file_downloader(); + + $this->guzzle_mock = $this->getMockBuilder('\GuzzleHttp\Client') + ->addMethods(['set_data']) + ->onlyMethods(['request']) + ->getMock(); + $this->guzzle_mock->method('set_data') + ->will($this->returnCallback(function($data) + { + $this->guzzle_data = $data; + } + )); + $this->guzzle_mock->method('request') + ->will($this->returnCallback(function() + { + return new \GuzzleHttp\Psr7\Response($this->guzzle_status, [], $this->guzzle_data); + } + )); + + $this->file_downloader = $this->getMockBuilder('\phpbb\file_downloader') + ->onlyMethods(['create_client']) + ->getMock(); + $this->file_downloader->method('create_client') + ->will($this->returnValue($this->guzzle_mock)); $lang_loader = new \phpbb\language\language_file_loader($phpbb_root_path, $phpEx); @@ -202,7 +232,7 @@ public function provider_get_versions() */ public function test_get_versions($input, $valid_data, $expected_return = '', $expected_exception = '') { - $this->file_downloader->set($input); + $this->guzzle_mock->set_data($input); // version_helper->get_versions() doesn't return a value on VERSIONCHECK_FAIL but only throws exception // so the $return is undefined. Define it here @@ -213,7 +243,7 @@ public function test_get_versions($input, $valid_data, $expected_return = '', $e try { $return = $this->version_helper->get_versions(); } catch (\phpbb\exception\runtime_exception $e) { - $this->assertEquals((string)$e->getMessage(), $expected_exception); + $this->assertEquals($expected_exception, $e->getMessage()); } } else @@ -223,4 +253,215 @@ public function test_get_versions($input, $valid_data, $expected_return = '', $e $this->assertEquals($expected_return, $return); } + + public function test_version_phpbb_com() + { + $this->guzzle_mock = $this->getMockBuilder('\GuzzleHttp\Client') + ->onlyMethods(['request']) + ->getMock(); + + $this->guzzle_mock->method('request') + ->will($this->returnCallback(function() + { + return new \GuzzleHttp\Psr7\Response(200, [], file_get_contents(__DIR__ . '/fixture/30x.txt')); + } + )); + + $file_downloader = $this->getMockBuilder(\phpbb\file_downloader::class) + ->onlyMethods(['create_client']) + ->getMock(); + + $file_downloader->method('create_client') + ->willReturn($this->guzzle_mock); + + $hostname = 'version.phpbb.com'; + + if (!checkdnsrr($hostname, 'A')) + { + $this->markTestSkipped(sprintf( + 'Could not find a DNS record for hostname %s. ' . + 'Assuming network is down.', + $hostname + )); + } + + $file = $file_downloader->get($hostname, '/phpbb', '30x.txt'); + $errstr = $file_downloader->get_error_string(); + $errno = $file_downloader->get_error_number(); + + $this->assertNotEquals( + 0, + strlen($file), + 'Failed asserting that the response is not empty.' + ); + + $this->assertSame( + '', + $errstr, + 'Failed asserting that the error string is empty.' + ); + + $this->assertSame( + 0, + $errno, + 'Failed asserting that the error number is 0 (i.e. no error occurred).' + ); + + $lines = explode("\n", $file); + + $this->assertGreaterThanOrEqual( + 2, + count($lines), + 'Failed asserting that the version file has at least two lines.' + ); + + $this->assertStringStartsWith( + '3.', + $lines[0], + "Failed asserting that the first line of the version file starts with '3.'" + ); + + $this->assertNotSame( + false, + filter_var($lines[1], FILTER_VALIDATE_URL), + 'Failed asserting that the second line of the version file is a valid URL.' + ); + + $this->assertStringContainsString('http', $lines[1]); + $this->assertStringContainsString('phpbb.com', $lines[1], '', true); + } + + public function test_file_downloader_file_not_found() + { + $this->guzzle_mock = $this->getMockBuilder('\GuzzleHttp\Client') + ->onlyMethods(['request']) + ->getMock(); + + $this->guzzle_mock->method('request') + ->will($this->returnCallback(function() + { + return new \GuzzleHttp\Psr7\Response(404, [], ''); + } + )); + + $file_downloader = $this->getMockBuilder(\phpbb\file_downloader::class) + ->onlyMethods(['create_client']) + ->getMock(); + + $file_downloader->method('create_client') + ->willReturn($this->guzzle_mock); + + $this->expectException(\phpbb\exception\runtime_exception::class); + $this->expectExceptionMessage('FILE_NOT_FOUND'); + + $file_downloader->get('foo.com', 'bar', 'foo.txt'); + } + + public function test_file_downloader_exception_not_found() + { + $this->guzzle_mock = $this->getMockBuilder('\GuzzleHttp\Client') + ->onlyMethods(['request']) + ->getMock(); + + $this->guzzle_mock->method('request') + ->will($this->returnCallback(function($method, $uri) + { + $request = new \GuzzleHttp\Psr7\Request('GET', $uri); + $response = new \GuzzleHttp\Psr7\Response(404, [], ''); + throw new RequestException('FILE_NOT_FOUND', $request, $response); + } + )); + + $file_downloader = $this->getMockBuilder(\phpbb\file_downloader::class) + ->onlyMethods(['create_client']) + ->getMock(); + + $file_downloader->method('create_client') + ->willReturn($this->guzzle_mock); + + $this->expectException(\phpbb\exception\runtime_exception::class); + $this->expectExceptionMessage('FILE_NOT_FOUND'); + + $file_downloader->get('foo.com', 'bar', 'foo.txt'); + } + + public function test_file_downloader_exception_moved() + { + $this->guzzle_mock = $this->getMockBuilder('\GuzzleHttp\Client') + ->onlyMethods(['request']) + ->getMock(); + + $this->guzzle_mock->method('request') + ->will($this->returnCallback(function($method, $uri) + { + $request = new \GuzzleHttp\Psr7\Request('GET', $uri); + $response = new \GuzzleHttp\Psr7\Response(302, [], ''); + throw new RequestException('FILE_MOVED', $request, $response); + } + )); + + $file_downloader = $this->getMockBuilder(\phpbb\file_downloader::class) + ->onlyMethods(['create_client']) + ->getMock(); + + $file_downloader->method('create_client') + ->willReturn($this->guzzle_mock); + + $this->assertFalse($file_downloader->get('foo.com', 'bar', 'foo.txt')); + $this->assertEquals(302, $file_downloader->get_error_number()); + $this->assertEquals('FILE_MOVED', $file_downloader->get_error_string()); + } + + public function test_file_downloader_exception_timeout() + { + $this->guzzle_mock = $this->getMockBuilder('\GuzzleHttp\Client') + ->onlyMethods(['request']) + ->getMock(); + + $this->guzzle_mock->method('request') + ->will($this->returnCallback(function($method, $uri) + { + $request = new \GuzzleHttp\Psr7\Request('GET', $uri); + throw new RequestException('FILE_NOT_FOUND', $request); + } + )); + + $file_downloader = $this->getMockBuilder(\phpbb\file_downloader::class) + ->onlyMethods(['create_client']) + ->getMock(); + + $file_downloader->method('create_client') + ->willReturn($this->guzzle_mock); + + $this->expectException(\phpbb\exception\runtime_exception::class); + $this->expectExceptionMessage('FSOCK_TIMEOUT'); + + $file_downloader->get('foo.com', 'bar', 'foo.txt'); + } + + public function test_file_downloader_exception_other() + { + $this->guzzle_mock = $this->getMockBuilder('\GuzzleHttp\Client') + ->onlyMethods(['request']) + ->getMock(); + + $this->guzzle_mock->method('request') + ->will($this->returnCallback(function($method, $uri) + { + throw new \RuntimeException('FSOCK_NOT_SUPPORTED'); + } + )); + + $file_downloader = $this->getMockBuilder(\phpbb\file_downloader::class) + ->onlyMethods(['create_client']) + ->getMock(); + + $file_downloader->method('create_client') + ->willReturn($this->guzzle_mock); + + $this->expectException(\phpbb\exception\runtime_exception::class); + $this->expectExceptionMessage('FSOCK_DISABLED'); + + $file_downloader->get('foo.com', 'bar', 'foo.txt'); + } } From d844f82f5669ebb1251275dcb0d1f3702c812db6 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Tue, 23 Jul 2024 21:05:54 +0200 Subject: [PATCH 2/7] [ticket/12479] Remove dns check from test as we don't test against host anymore PHPBB-12479 --- tests/version/version_helper_remote_test.php | 9 --------- 1 file changed, 9 deletions(-) diff --git a/tests/version/version_helper_remote_test.php b/tests/version/version_helper_remote_test.php index ce2d110fdc3..1687abed7f5 100644 --- a/tests/version/version_helper_remote_test.php +++ b/tests/version/version_helper_remote_test.php @@ -276,15 +276,6 @@ public function test_version_phpbb_com() $hostname = 'version.phpbb.com'; - if (!checkdnsrr($hostname, 'A')) - { - $this->markTestSkipped(sprintf( - 'Could not find a DNS record for hostname %s. ' . - 'Assuming network is down.', - $hostname - )); - } - $file = $file_downloader->get($hostname, '/phpbb', '30x.txt'); $errstr = $file_downloader->get_error_string(); $errno = $file_downloader->get_error_number(); From 929013388f408cd3b1df15382c65c64a9ee50bea Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Tue, 23 Jul 2024 21:06:48 +0200 Subject: [PATCH 3/7] [ticket/12479] Fix use statements in file_downloader PHPBB-12479 --- phpBB/phpbb/file_downloader.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/phpBB/phpbb/file_downloader.php b/phpBB/phpbb/file_downloader.php index b165198399b..e10bef1e03a 100644 --- a/phpBB/phpbb/file_downloader.php +++ b/phpBB/phpbb/file_downloader.php @@ -14,9 +14,8 @@ namespace phpbb; use GuzzleHttp\Client; -use GuzzleHttp\Exception\RequestException ; +use GuzzleHttp\Exception\RequestException; use phpbb\exception\runtime_exception; -use function Amp\Promise\rethrow; class file_downloader { From d6ed21f3f28712c0061a3e592d22d53758f8f64d Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Tue, 23 Jul 2024 21:25:22 +0200 Subject: [PATCH 4/7] [ticket/12479] Use separate mock in "remote" file test PHPBB-12479 --- tests/version/version_helper_remote_test.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/version/version_helper_remote_test.php b/tests/version/version_helper_remote_test.php index 1687abed7f5..be9f003744c 100644 --- a/tests/version/version_helper_remote_test.php +++ b/tests/version/version_helper_remote_test.php @@ -256,11 +256,11 @@ public function test_get_versions($input, $valid_data, $expected_return = '', $e public function test_version_phpbb_com() { - $this->guzzle_mock = $this->getMockBuilder('\GuzzleHttp\Client') + $guzzle_mock = $this->getMockBuilder('\GuzzleHttp\Client') ->onlyMethods(['request']) ->getMock(); - $this->guzzle_mock->method('request') + $guzzle_mock->method('request') ->will($this->returnCallback(function() { return new \GuzzleHttp\Psr7\Response(200, [], file_get_contents(__DIR__ . '/fixture/30x.txt')); @@ -272,7 +272,7 @@ public function test_version_phpbb_com() ->getMock(); $file_downloader->method('create_client') - ->willReturn($this->guzzle_mock); + ->willReturn($guzzle_mock); $hostname = 'version.phpbb.com'; From 48c2ca9dfe1a00221da342b1398a9b1bbc98e6ca Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Wed, 24 Jul 2024 19:13:06 +0200 Subject: [PATCH 5/7] [ticket/12479] Add missing file fixture PHPBB-12479 --- tests/version/fixture/30x.txt | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 tests/version/fixture/30x.txt diff --git a/tests/version/fixture/30x.txt b/tests/version/fixture/30x.txt new file mode 100644 index 00000000000..f138ad0c83b --- /dev/null +++ b/tests/version/fixture/30x.txt @@ -0,0 +1,4 @@ +3.0.14 +https://www.phpbb.com/community/viewtopic.php?f=14&t=2313941 +3.3.12 +https://www.phpbb.com/community/viewtopic.php?t=2653732 From 5a09105c3c38b092084cc1f721853c83721b692f Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Tue, 10 Sep 2024 22:04:55 +0200 Subject: [PATCH 6/7] [ticket/17385] Backport tests for updated file downloader to 3.3.x PHPBB-17385 --- tests/version/version_helper_remote_test.php | 31 ++++++++++---------- 1 file changed, 15 insertions(+), 16 deletions(-) diff --git a/tests/version/version_helper_remote_test.php b/tests/version/version_helper_remote_test.php index be9f003744c..52909438f4c 100644 --- a/tests/version/version_helper_remote_test.php +++ b/tests/version/version_helper_remote_test.php @@ -46,12 +46,11 @@ protected function setUp(): void $this->cache->expects($this->any()) ->method('get') - ->with($this->anything()) + ->withAnyParameters() ->will($this->returnValue(false)); $this->guzzle_mock = $this->getMockBuilder('\GuzzleHttp\Client') - ->addMethods(['set_data']) - ->onlyMethods(['request']) + ->setMethods(['set_data', 'request']) ->getMock(); $this->guzzle_mock->method('set_data') ->will($this->returnCallback(function($data) @@ -67,7 +66,7 @@ protected function setUp(): void )); $this->file_downloader = $this->getMockBuilder('\phpbb\file_downloader') - ->onlyMethods(['create_client']) + ->setMethods(['create_client']) ->getMock(); $this->file_downloader->method('create_client') ->will($this->returnValue($this->guzzle_mock)); @@ -257,7 +256,7 @@ public function test_get_versions($input, $valid_data, $expected_return = '', $e public function test_version_phpbb_com() { $guzzle_mock = $this->getMockBuilder('\GuzzleHttp\Client') - ->onlyMethods(['request']) + ->setMethods(['request']) ->getMock(); $guzzle_mock->method('request') @@ -268,7 +267,7 @@ public function test_version_phpbb_com() )); $file_downloader = $this->getMockBuilder(\phpbb\file_downloader::class) - ->onlyMethods(['create_client']) + ->setMethods(['create_client']) ->getMock(); $file_downloader->method('create_client') @@ -325,7 +324,7 @@ public function test_version_phpbb_com() public function test_file_downloader_file_not_found() { $this->guzzle_mock = $this->getMockBuilder('\GuzzleHttp\Client') - ->onlyMethods(['request']) + ->setMethods(['request']) ->getMock(); $this->guzzle_mock->method('request') @@ -336,7 +335,7 @@ public function test_file_downloader_file_not_found() )); $file_downloader = $this->getMockBuilder(\phpbb\file_downloader::class) - ->onlyMethods(['create_client']) + ->setMethods(['create_client']) ->getMock(); $file_downloader->method('create_client') @@ -351,7 +350,7 @@ public function test_file_downloader_file_not_found() public function test_file_downloader_exception_not_found() { $this->guzzle_mock = $this->getMockBuilder('\GuzzleHttp\Client') - ->onlyMethods(['request']) + ->setMethods(['request']) ->getMock(); $this->guzzle_mock->method('request') @@ -364,7 +363,7 @@ public function test_file_downloader_exception_not_found() )); $file_downloader = $this->getMockBuilder(\phpbb\file_downloader::class) - ->onlyMethods(['create_client']) + ->setMethods(['create_client']) ->getMock(); $file_downloader->method('create_client') @@ -379,7 +378,7 @@ public function test_file_downloader_exception_not_found() public function test_file_downloader_exception_moved() { $this->guzzle_mock = $this->getMockBuilder('\GuzzleHttp\Client') - ->onlyMethods(['request']) + ->setMethods(['request']) ->getMock(); $this->guzzle_mock->method('request') @@ -392,7 +391,7 @@ public function test_file_downloader_exception_moved() )); $file_downloader = $this->getMockBuilder(\phpbb\file_downloader::class) - ->onlyMethods(['create_client']) + ->setMethods(['create_client']) ->getMock(); $file_downloader->method('create_client') @@ -406,7 +405,7 @@ public function test_file_downloader_exception_moved() public function test_file_downloader_exception_timeout() { $this->guzzle_mock = $this->getMockBuilder('\GuzzleHttp\Client') - ->onlyMethods(['request']) + ->setMethods(['request']) ->getMock(); $this->guzzle_mock->method('request') @@ -418,7 +417,7 @@ public function test_file_downloader_exception_timeout() )); $file_downloader = $this->getMockBuilder(\phpbb\file_downloader::class) - ->onlyMethods(['create_client']) + ->setMethods(['create_client']) ->getMock(); $file_downloader->method('create_client') @@ -433,7 +432,7 @@ public function test_file_downloader_exception_timeout() public function test_file_downloader_exception_other() { $this->guzzle_mock = $this->getMockBuilder('\GuzzleHttp\Client') - ->onlyMethods(['request']) + ->setMethods(['request']) ->getMock(); $this->guzzle_mock->method('request') @@ -444,7 +443,7 @@ public function test_file_downloader_exception_other() )); $file_downloader = $this->getMockBuilder(\phpbb\file_downloader::class) - ->onlyMethods(['create_client']) + ->setMethods(['create_client']) ->getMock(); $file_downloader->method('create_client') From 9ace4dc5853681a808affb95d8dc31a34375d33b Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Sat, 28 Sep 2024 16:01:02 +0100 Subject: [PATCH 7/7] [ticket/17385] Change default value in docblock PHPBB-17385 --- phpBB/phpbb/file_downloader.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/phpBB/phpbb/file_downloader.php b/phpBB/phpbb/file_downloader.php index e10bef1e03a..14607ece0c8 100644 --- a/phpBB/phpbb/file_downloader.php +++ b/phpBB/phpbb/file_downloader.php @@ -60,7 +60,7 @@ protected function create_client(string $host, int $port = 443, int $timeout = 6 * @param string $host File host * @param string $directory Directory file is in * @param string $filename Filename of file to retrieve - * @param int $port Port to connect to; default: 80 + * @param int $port Port to connect to; default: 443 * @param int $timeout Connection timeout in seconds; default: 6 * * @return false|string File data as string if file can be read and there is no