From 582a2bd7375c5b8faf71b0cc4695ce4a064bde1c Mon Sep 17 00:00:00 2001 From: bahman026 Date: Mon, 31 Aug 2026 12:54:02 +0330 Subject: [PATCH] fix(files_external): validate FTP and SFTP ports as TCP port numbers Follow-up to #63161. The port field of an external storage holds whatever the admin typed, so `is_numeric()` still let through values that are not usable TCP ports: "21.5" and "1e3" were silently truncated by the int cast, and "0", "-2121" or "65536" were passed on to the connection as-is. Add PortHelper::parsePort(), which only accepts an integer or a digit-only string within the valid TCP port range of 1-65535 and otherwise returns the given fallback. Use it for both FTP and SFTP, including the port that SFTP parses out of the host field, and replace the hardcoded default ports with class constants. Signed-off-by: bahman026 --- .../composer/composer/autoload_classmap.php | 1 + .../composer/composer/autoload_static.php | 1 + apps/files_external/lib/Lib/PortHelper.php | 48 ++++++++++++++ apps/files_external/lib/Lib/Storage/FTP.php | 6 +- apps/files_external/lib/Lib/Storage/SFTP.php | 18 +++--- apps/files_external/tests/FtpTest.php | 5 ++ apps/files_external/tests/PortHelperTest.php | 63 +++++++++++++++++++ apps/files_external/tests/SftpPortTest.php | 51 +++++++++++++++ 8 files changed, 183 insertions(+), 10 deletions(-) create mode 100644 apps/files_external/lib/Lib/PortHelper.php create mode 100644 apps/files_external/tests/PortHelperTest.php create mode 100644 apps/files_external/tests/SftpPortTest.php diff --git a/apps/files_external/composer/composer/autoload_classmap.php b/apps/files_external/composer/composer/autoload_classmap.php index 6cb11dc66fe85..e43059be5741f 100644 --- a/apps/files_external/composer/composer/autoload_classmap.php +++ b/apps/files_external/composer/composer/autoload_classmap.php @@ -86,6 +86,7 @@ 'OCA\\Files_External\\Lib\\MissingDependency' => $baseDir . '/../lib/Lib/MissingDependency.php', 'OCA\\Files_External\\Lib\\Notify\\SMBNotifyHandler' => $baseDir . '/../lib/Lib/Notify/SMBNotifyHandler.php', 'OCA\\Files_External\\Lib\\PersonalMount' => $baseDir . '/../lib/Lib/PersonalMount.php', + 'OCA\\Files_External\\Lib\\PortHelper' => $baseDir . '/../lib/Lib/PortHelper.php', 'OCA\\Files_External\\Lib\\PriorityTrait' => $baseDir . '/../lib/Lib/PriorityTrait.php', 'OCA\\Files_External\\Lib\\SessionStorageWrapper' => $baseDir . '/../lib/Lib/SessionStorageWrapper.php', 'OCA\\Files_External\\Lib\\StorageConfig' => $baseDir . '/../lib/Lib/StorageConfig.php', diff --git a/apps/files_external/composer/composer/autoload_static.php b/apps/files_external/composer/composer/autoload_static.php index 132da2a2eff10..eec146d463571 100644 --- a/apps/files_external/composer/composer/autoload_static.php +++ b/apps/files_external/composer/composer/autoload_static.php @@ -101,6 +101,7 @@ class ComposerStaticInitFiles_External 'OCA\\Files_External\\Lib\\MissingDependency' => __DIR__ . '/..' . '/../lib/Lib/MissingDependency.php', 'OCA\\Files_External\\Lib\\Notify\\SMBNotifyHandler' => __DIR__ . '/..' . '/../lib/Lib/Notify/SMBNotifyHandler.php', 'OCA\\Files_External\\Lib\\PersonalMount' => __DIR__ . '/..' . '/../lib/Lib/PersonalMount.php', + 'OCA\\Files_External\\Lib\\PortHelper' => __DIR__ . '/..' . '/../lib/Lib/PortHelper.php', 'OCA\\Files_External\\Lib\\PriorityTrait' => __DIR__ . '/..' . '/../lib/Lib/PriorityTrait.php', 'OCA\\Files_External\\Lib\\SessionStorageWrapper' => __DIR__ . '/..' . '/../lib/Lib/SessionStorageWrapper.php', 'OCA\\Files_External\\Lib\\StorageConfig' => __DIR__ . '/..' . '/../lib/Lib/StorageConfig.php', diff --git a/apps/files_external/lib/Lib/PortHelper.php b/apps/files_external/lib/Lib/PortHelper.php new file mode 100644 index 0000000000000..f68bd882a4ec2 --- /dev/null +++ b/apps/files_external/lib/Lib/PortHelper.php @@ -0,0 +1,48 @@ + self::MAX_PORT) { + return $fallback; + } + + return $parsedPort; + } +} diff --git a/apps/files_external/lib/Lib/Storage/FTP.php b/apps/files_external/lib/Lib/Storage/FTP.php index 66e623e5a1b2c..f8cc6a1f2879a 100644 --- a/apps/files_external/lib/Lib/Storage/FTP.php +++ b/apps/files_external/lib/Lib/Storage/FTP.php @@ -12,6 +12,7 @@ use Icewind\Streams\IteratorDirectory; use OC\Files\Storage\Common; use OC\Files\Storage\PolyFill\CopyDirectory; +use OCA\Files_External\Lib\PortHelper; use OCP\Constants; use OCP\Files\FileInfo; use OCP\Files\IMimeTypeDetector; @@ -23,6 +24,8 @@ class FTP extends Common { use CopyDirectory; + private const DEFAULT_PORT = 21; + private $root; private $host; private $password; @@ -49,8 +52,7 @@ 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; + $this->port = PortHelper::parsePort($parameters['port'] ?? null, 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..034cd0729612c 100644 --- a/apps/files_external/lib/Lib/Storage/SFTP.php +++ b/apps/files_external/lib/Lib/Storage/SFTP.php @@ -14,6 +14,7 @@ use Icewind\Streams\RetryWrapper; use OC\Files\Storage\Common; use OC\Files\View; +use OCA\Files_External\Lib\PortHelper; use OCP\Cache\CappedMemoryCache; use OCP\Constants; use OCP\Files\FileInfo; @@ -26,10 +27,12 @@ * 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 = []; @@ -56,11 +59,11 @@ private function splitHost(string $host): array { $parsed = parse_url($host); if (is_array($parsed) && isset($parsed['port'])) { - return [$parsed['host'], $parsed['port']]; + return [$parsed['host'], PortHelper::parsePort($parsed['port'], self::DEFAULT_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,9 @@ 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]); + // Fall back to the port from the host field, and to the default port, + // unless a valid port is configured + $this->port = PortHelper::parsePort($parameters['port'] ?? null, $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..21f8b1659fc0f 100644 --- a/apps/files_external/tests/FtpTest.php +++ b/apps/files_external/tests/FtpTest.php @@ -28,6 +28,11 @@ 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], + 'decimal port' => [array_merge($parameters, ['port' => '21.5']), 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], + 'highest valid port' => [array_merge($parameters, ['port' => '65535']), 65535], ]; } diff --git a/apps/files_external/tests/PortHelperTest.php b/apps/files_external/tests/PortHelperTest.php new file mode 100644 index 0000000000000..3eef97dff2a85 --- /dev/null +++ b/apps/files_external/tests/PortHelperTest.php @@ -0,0 +1,63 @@ + [2121, 2121], + 'numeric string port' => ['2121', 2121], + 'padded numeric string port' => ['0022', 22], + 'lowest valid port' => [1, 1], + 'highest valid port' => [65535, 65535], + 'highest valid port as string' => ['65535', 65535], + + // unset or empty values fall back + 'null port' => [null, 21], + 'empty port' => ['', 21], + 'whitespace port' => [' ', 21], + 'array port' => [[2121], 21], + + // non integer values fall back + 'non numeric port' => ['ftp', 21], + 'float port' => [21.5, 21], + 'integer float port' => [2121.0, 21], + 'decimal string port' => ['21.5', 21], + 'exponential string port' => ['1e3', 21], + 'hexadecimal string port' => ['0x15', 21], + 'signed string port' => ['+2121', 21], + 'padded string port' => [' 2121', 21], + 'boolean port' => [true, 21], + + // out of range values fall back + 'zero port' => [0, 21], + 'zero string port' => ['0', 21], + 'negative port' => [-2121, 21], + 'negative string port' => ['-2121', 21], + 'too large port' => [65536, 21], + 'too large string port' => ['65536', 21], + 'way too large string port' => ['999999999999999999999999', 21], + ]; + } + + #[DataProvider('portProvider')] + public function testParsePort(mixed $port, int $expectedPort): void { + $this->assertSame($expectedPort, PortHelper::parsePort($port, 21)); + } + + public function testParsePortReturnsGivenFallback(): void { + $this->assertSame(22, PortHelper::parsePort('', 22)); + } +} diff --git a/apps/files_external/tests/SftpPortTest.php b/apps/files_external/tests/SftpPortTest.php new file mode 100644 index 0000000000000..b032be09e23db --- /dev/null +++ b/apps/files_external/tests/SftpPortTest.php @@ -0,0 +1,51 @@ + '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], + 'decimal port' => [array_merge($parameters, ['port' => '22.5']), 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], + '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')); + } +}