diff --git a/apps/files_external/lib/Lib/Storage/FTP.php b/apps/files_external/lib/Lib/Storage/FTP.php index 66e623e5a1b2c..febfba5d19f7d 100644 --- a/apps/files_external/lib/Lib/Storage/FTP.php +++ b/apps/files_external/lib/Lib/Storage/FTP.php @@ -23,6 +23,8 @@ class FTP extends Common { use CopyDirectory; + private const DEFAULT_PORT = 21; + private $root; private $host; private $password; @@ -49,8 +51,14 @@ public function __construct(array $parameters) { $this->secure = false; } $this->root = isset($parameters['root']) ? '/' . ltrim($parameters['root']) : '/'; - $parsedPort = $parameters['port'] ?? null; - $this->port = is_numeric($parsedPort) ? (int)$parsedPort : 21; + // The port field holds whatever the administrator typed, so only accept a + // valid TCP port. Leading zeros are stripped so that "0022" keeps working. + $configuredPort = trim((string)($parameters['port'] ?? '')); + $port = filter_var(ltrim($configuredPort, '0'), FILTER_VALIDATE_INT, ['options' => ['min_range' => 1, 'max_range' => 65535]]); + if ($port === false && $configuredPort !== '') { + Server::get(LoggerInterface::class)->warning('Ignoring invalid port configured for FTP storage, falling back to the default port', ['port' => $configuredPort, 'default' => self::DEFAULT_PORT]); + } + $this->port = $port ?: self::DEFAULT_PORT; $this->utf8Mode = isset($parameters['utf8']) && $parameters['utf8']; } else { throw new \Exception('Creating ' . self::class . ' storage failed, required parameters not set'); diff --git a/apps/files_external/lib/Lib/Storage/SFTP.php b/apps/files_external/lib/Lib/Storage/SFTP.php index b1b5be38dbea3..a45f459b316c8 100644 --- a/apps/files_external/lib/Lib/Storage/SFTP.php +++ b/apps/files_external/lib/Lib/Storage/SFTP.php @@ -20,16 +20,19 @@ use OCP\Files\IMimeTypeDetector; use OCP\Server; use phpseclib3\Net\SFTP\Stream; +use Psr\Log\LoggerInterface; /** * Uses phpseclib's Net\SFTP class and the Net\SFTP\Stream stream wrapper to * provide access to SFTP servers. */ class SFTP extends Common { + private const DEFAULT_PORT = 22; + private $host; private $user; private $root; - private $port = 22; + private $port = self::DEFAULT_PORT; private $auth = []; @@ -58,9 +61,9 @@ private function splitHost(string $host): array { if (is_array($parsed) && isset($parsed['port'])) { return [$parsed['host'], $parsed['port']]; } elseif (is_array($parsed)) { - return [$parsed['host'], 22]; + return [$parsed['host'], self::DEFAULT_PORT]; } else { - return [$input, 22]; + return [$input, self::DEFAULT_PORT]; } } @@ -78,10 +81,15 @@ public function __construct(array $parameters) { $parsedHost = $this->splitHost($parameters['host']); $this->host = $parsedHost[0]; - // Handle empty port parameter to allow host-defined ports - // and ensure strictly numeric ports - $parsedPort = $parameters['port'] ?? null; - $this->port = (int)(is_numeric($parsedPort) ? $parsedPort : $parsedHost[1]); + // The port field holds whatever the administrator typed, so only accept a + // valid TCP port and otherwise keep the port from the host field. Leading + // zeros are stripped so that "0022" keeps working. + $configuredPort = trim((string)($parameters['port'] ?? '')); + $port = filter_var(ltrim($configuredPort, '0'), FILTER_VALIDATE_INT, ['options' => ['min_range' => 1, 'max_range' => 65535]]); + if ($port === false && $configuredPort !== '') { + Server::get(LoggerInterface::class)->warning('Ignoring invalid port configured for SFTP storage, falling back to the port from the host field', ['port' => $configuredPort, 'fallback' => $parsedHost[1]]); + } + $this->port = $port ?: $parsedHost[1]; if (!isset($parameters['user'])) { throw new \UnexpectedValueException('no authentication parameters specified'); diff --git a/apps/files_external/tests/FtpTest.php b/apps/files_external/tests/FtpTest.php index 68e6bb723756f..06aab11529e7e 100644 --- a/apps/files_external/tests/FtpTest.php +++ b/apps/files_external/tests/FtpTest.php @@ -28,6 +28,17 @@ public static function portProvider(): array { 'non numeric port' => [array_merge($parameters, ['port' => 'ftp']), 21], 'numeric string port' => [array_merge($parameters, ['port' => '2121']), 2121], 'integer port' => [array_merge($parameters, ['port' => 2121]), 2121], + 'padded numeric string port' => [array_merge($parameters, ['port' => ' 2121 ']), 2121], + 'signed numeric string port' => [array_merge($parameters, ['port' => '+2121']), 2121], + 'leading zero port' => [array_merge($parameters, ['port' => '02121']), 2121], + 'decimal port' => [array_merge($parameters, ['port' => '21.5']), 21], + 'exponential port' => [array_merge($parameters, ['port' => '1e3']), 21], + 'hexadecimal port' => [array_merge($parameters, ['port' => '0x15']), 21], + 'zero port' => [array_merge($parameters, ['port' => '0']), 21], + 'negative port' => [array_merge($parameters, ['port' => '-2121']), 21], + 'out of range port' => [array_merge($parameters, ['port' => '65536']), 21], + 'way out of range port' => [array_merge($parameters, ['port' => '999999999999999999999999']), 21], + 'highest valid port' => [array_merge($parameters, ['port' => '65535']), 65535], ]; } diff --git a/apps/files_external/tests/SftpPortTest.php b/apps/files_external/tests/SftpPortTest.php new file mode 100644 index 0000000000000..b18f40fd1fa62 --- /dev/null +++ b/apps/files_external/tests/SftpPortTest.php @@ -0,0 +1,57 @@ + 'somehost', + 'user' => 'someuser', + 'password' => 'somepassword', + ]; + + return [ + 'no port given' => [$parameters, 22], + 'empty port' => [array_merge($parameters, ['port' => '']), 22], + 'null port' => [array_merge($parameters, ['port' => null]), 22], + 'non numeric port' => [array_merge($parameters, ['port' => 'sftp']), 22], + 'numeric string port' => [array_merge($parameters, ['port' => '2222']), 2222], + 'integer port' => [array_merge($parameters, ['port' => 2222]), 2222], + 'padded numeric string port' => [array_merge($parameters, ['port' => ' 2222 ']), 2222], + 'signed numeric string port' => [array_merge($parameters, ['port' => '+2222']), 2222], + 'leading zero port' => [array_merge($parameters, ['port' => '02222']), 2222], + 'decimal port' => [array_merge($parameters, ['port' => '22.5']), 22], + 'exponential port' => [array_merge($parameters, ['port' => '1e3']), 22], + 'hexadecimal port' => [array_merge($parameters, ['port' => '0x15']), 22], + 'zero port' => [array_merge($parameters, ['port' => '0']), 22], + 'negative port' => [array_merge($parameters, ['port' => '-2222']), 22], + 'out of range port' => [array_merge($parameters, ['port' => '65536']), 22], + 'way out of range port' => [array_merge($parameters, ['port' => '999999999999999999999999']), 22], + 'highest valid port' => [array_merge($parameters, ['port' => '65535']), 65535], + + // the port can also be part of the host field + 'port in host' => [array_merge($parameters, ['host' => 'somehost:2222']), 2222], + 'port in host with empty port' => [array_merge($parameters, ['host' => 'somehost:2222', 'port' => '']), 2222], + 'port in host overwritten by port' => [array_merge($parameters, ['host' => 'somehost:2222', 'port' => '2223']), 2223], + 'port in host with invalid port' => [array_merge($parameters, ['host' => 'somehost:2222', 'port' => '65536']), 2222], + ]; + } + + #[DataProvider('portProvider')] + public function testPort(array $parameters, int $expectedPort): void { + $instance = new SFTP($parameters); + + $this->assertSame($expectedPort, self::invokePrivate($instance, 'port')); + } +}