Skip to content

Commit 582a2bd

Browse files
bahman026bahman026
committed
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 <bahman026@gmail.com>
1 parent 2efe48b commit 582a2bd

8 files changed

Lines changed: 183 additions & 10 deletions

File tree

‎apps/files_external/composer/composer/autoload_classmap.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,7 @@
8686
'OCA\\Files_External\\Lib\\MissingDependency' => $baseDir . '/../lib/Lib/MissingDependency.php',
8787
'OCA\\Files_External\\Lib\\Notify\\SMBNotifyHandler' => $baseDir . '/../lib/Lib/Notify/SMBNotifyHandler.php',
8888
'OCA\\Files_External\\Lib\\PersonalMount' => $baseDir . '/../lib/Lib/PersonalMount.php',
89+
'OCA\\Files_External\\Lib\\PortHelper' => $baseDir . '/../lib/Lib/PortHelper.php',
8990
'OCA\\Files_External\\Lib\\PriorityTrait' => $baseDir . '/../lib/Lib/PriorityTrait.php',
9091
'OCA\\Files_External\\Lib\\SessionStorageWrapper' => $baseDir . '/../lib/Lib/SessionStorageWrapper.php',
9192
'OCA\\Files_External\\Lib\\StorageConfig' => $baseDir . '/../lib/Lib/StorageConfig.php',

‎apps/files_external/composer/composer/autoload_static.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,7 @@ class ComposerStaticInitFiles_External
101101
'OCA\\Files_External\\Lib\\MissingDependency' => __DIR__ . '/..' . '/../lib/Lib/MissingDependency.php',
102102
'OCA\\Files_External\\Lib\\Notify\\SMBNotifyHandler' => __DIR__ . '/..' . '/../lib/Lib/Notify/SMBNotifyHandler.php',
103103
'OCA\\Files_External\\Lib\\PersonalMount' => __DIR__ . '/..' . '/../lib/Lib/PersonalMount.php',
104+
'OCA\\Files_External\\Lib\\PortHelper' => __DIR__ . '/..' . '/../lib/Lib/PortHelper.php',
104105
'OCA\\Files_External\\Lib\\PriorityTrait' => __DIR__ . '/..' . '/../lib/Lib/PriorityTrait.php',
105106
'OCA\\Files_External\\Lib\\SessionStorageWrapper' => __DIR__ . '/..' . '/../lib/Lib/SessionStorageWrapper.php',
106107
'OCA\\Files_External\\Lib\\StorageConfig' => __DIR__ . '/..' . '/../lib/Lib/StorageConfig.php',

‎apps/files_external/lib/Lib/PortHelper.php‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
/**
6+
* SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
7+
* SPDX-License-Identifier: AGPL-3.0-or-later
8+
*/
9+
10+
namespace OCA\Files_External\Lib;
11+
12+
/**
13+
* Helper to turn a configured port into a usable TCP port number.
14+
*
15+
* The external storage settings store whatever the admin typed into the port
16+
* field, so the value can be missing, an empty string, a non-numeric string or
17+
* a number outside of the valid TCP port range.
18+
*/
19+
final class PortHelper {
20+
/** Lowest valid TCP port */
21+
public const MIN_PORT = 1;
22+
23+
/** Highest valid TCP port */
24+
public const MAX_PORT = 65535;
25+
26+
/**
27+
* Parse a configured port value
28+
*
29+
* @param mixed $port the configured value, may be of any type
30+
* @param int $fallback port to use when the configured value is not a valid TCP port
31+
* @return int the configured port, or $fallback if it is not an integer within the valid TCP port range
32+
*/
33+
public static function parsePort(mixed $port, int $fallback): int {
34+
if (is_int($port)) {
35+
$parsedPort = $port;
36+
} elseif (is_string($port) && preg_match('/^\d+$/', $port) === 1) {
37+
$parsedPort = (int)$port;
38+
} else {
39+
return $fallback;
40+
}
41+
42+
if ($parsedPort < self::MIN_PORT || $parsedPort > self::MAX_PORT) {
43+
return $fallback;
44+
}
45+
46+
return $parsedPort;
47+
}
48+
}

‎apps/files_external/lib/Lib/Storage/FTP.php‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
use Icewind\Streams\IteratorDirectory;
1313
use OC\Files\Storage\Common;
1414
use OC\Files\Storage\PolyFill\CopyDirectory;
15+
use OCA\Files_External\Lib\PortHelper;
1516
use OCP\Constants;
1617
use OCP\Files\FileInfo;
1718
use OCP\Files\IMimeTypeDetector;
@@ -23,6 +24,8 @@
2324
class FTP extends Common {
2425
use CopyDirectory;
2526

27+
private const DEFAULT_PORT = 21;
28+
2629
private $root;
2730
private $host;
2831
private $password;
@@ -49,8 +52,7 @@ public function __construct(array $parameters) {
4952
$this->secure = false;
5053
}
5154
$this->root = isset($parameters['root']) ? '/' . ltrim($parameters['root']) : '/';
52-
$parsedPort = $parameters['port'] ?? null;
53-
$this->port = is_numeric($parsedPort) ? (int)$parsedPort : 21;
55+
$this->port = PortHelper::parsePort($parameters['port'] ?? null, self::DEFAULT_PORT);
5456
$this->utf8Mode = isset($parameters['utf8']) && $parameters['utf8'];
5557
} else {
5658
throw new \Exception('Creating ' . self::class . ' storage failed, required parameters not set');

‎apps/files_external/lib/Lib/Storage/SFTP.php‎

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
use Icewind\Streams\RetryWrapper;
1515
use OC\Files\Storage\Common;
1616
use OC\Files\View;
17+
use OCA\Files_External\Lib\PortHelper;
1718
use OCP\Cache\CappedMemoryCache;
1819
use OCP\Constants;
1920
use OCP\Files\FileInfo;
@@ -26,10 +27,12 @@
2627
* provide access to SFTP servers.
2728
*/
2829
class SFTP extends Common {
30+
private const DEFAULT_PORT = 22;
31+
2932
private $host;
3033
private $user;
3134
private $root;
32-
private $port = 22;
35+
private $port = self::DEFAULT_PORT;
3336

3437
private $auth = [];
3538

@@ -56,11 +59,11 @@ private function splitHost(string $host): array {
5659

5760
$parsed = parse_url($host);
5861
if (is_array($parsed) && isset($parsed['port'])) {
59-
return [$parsed['host'], $parsed['port']];
62+
return [$parsed['host'], PortHelper::parsePort($parsed['port'], self::DEFAULT_PORT)];
6063
} elseif (is_array($parsed)) {
61-
return [$parsed['host'], 22];
64+
return [$parsed['host'], self::DEFAULT_PORT];
6265
} else {
63-
return [$input, 22];
66+
return [$input, self::DEFAULT_PORT];
6467
}
6568
}
6669

@@ -78,10 +81,9 @@ public function __construct(array $parameters) {
7881
$parsedHost = $this->splitHost($parameters['host']);
7982
$this->host = $parsedHost[0];
8083

81-
// Handle empty port parameter to allow host-defined ports
82-
// and ensure strictly numeric ports
83-
$parsedPort = $parameters['port'] ?? null;
84-
$this->port = (int)(is_numeric($parsedPort) ? $parsedPort : $parsedHost[1]);
84+
// Fall back to the port from the host field, and to the default port,
85+
// unless a valid port is configured
86+
$this->port = PortHelper::parsePort($parameters['port'] ?? null, $parsedHost[1]);
8587

8688
if (!isset($parameters['user'])) {
8789
throw new \UnexpectedValueException('no authentication parameters specified');

‎apps/files_external/tests/FtpTest.php‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,11 @@ public static function portProvider(): array {
2828
'non numeric port' => [array_merge($parameters, ['port' => 'ftp']), 21],
2929
'numeric string port' => [array_merge($parameters, ['port' => '2121']), 2121],
3030
'integer port' => [array_merge($parameters, ['port' => 2121]), 2121],
31+
'decimal port' => [array_merge($parameters, ['port' => '21.5']), 21],
32+
'zero port' => [array_merge($parameters, ['port' => '0']), 21],
33+
'negative port' => [array_merge($parameters, ['port' => '-2121']), 21],
34+
'out of range port' => [array_merge($parameters, ['port' => '65536']), 21],
35+
'highest valid port' => [array_merge($parameters, ['port' => '65535']), 65535],
3136
];
3237
}
3338

‎apps/files_external/tests/PortHelperTest.php‎

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,63 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
/**
6+
* SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
7+
* SPDX-License-Identifier: AGPL-3.0-or-later
8+
*/
9+
10+
namespace OCA\Files_External\Tests;
11+
12+
use OCA\Files_External\Lib\PortHelper;
13+
use PHPUnit\Framework\Attributes\DataProvider;
14+
use Test\TestCase;
15+
16+
class PortHelperTest extends TestCase {
17+
public static function portProvider(): array {
18+
return [
19+
// valid ports are returned as integers
20+
'integer port' => [2121, 2121],
21+
'numeric string port' => ['2121', 2121],
22+
'padded numeric string port' => ['0022', 22],
23+
'lowest valid port' => [1, 1],
24+
'highest valid port' => [65535, 65535],
25+
'highest valid port as string' => ['65535', 65535],
26+
27+
// unset or empty values fall back
28+
'null port' => [null, 21],
29+
'empty port' => ['', 21],
30+
'whitespace port' => [' ', 21],
31+
'array port' => [[2121], 21],
32+
33+
// non integer values fall back
34+
'non numeric port' => ['ftp', 21],
35+
'float port' => [21.5, 21],
36+
'integer float port' => [2121.0, 21],
37+
'decimal string port' => ['21.5', 21],
38+
'exponential string port' => ['1e3', 21],
39+
'hexadecimal string port' => ['0x15', 21],
40+
'signed string port' => ['+2121', 21],
41+
'padded string port' => [' 2121', 21],
42+
'boolean port' => [true, 21],
43+
44+
// out of range values fall back
45+
'zero port' => [0, 21],
46+
'zero string port' => ['0', 21],
47+
'negative port' => [-2121, 21],
48+
'negative string port' => ['-2121', 21],
49+
'too large port' => [65536, 21],
50+
'too large string port' => ['65536', 21],
51+
'way too large string port' => ['999999999999999999999999', 21],
52+
];
53+
}
54+
55+
#[DataProvider('portProvider')]
56+
public function testParsePort(mixed $port, int $expectedPort): void {
57+
$this->assertSame($expectedPort, PortHelper::parsePort($port, 21));
58+
}
59+
60+
public function testParsePortReturnsGivenFallback(): void {
61+
$this->assertSame(22, PortHelper::parsePort('', 22));
62+
}
63+
}

‎apps/files_external/tests/SftpPortTest.php‎

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
/**
6+
* SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
7+
* SPDX-License-Identifier: AGPL-3.0-or-later
8+
*/
9+
10+
namespace OCA\Files_External\Tests;
11+
12+
use OCA\Files_External\Lib\Storage\SFTP;
13+
use PHPUnit\Framework\Attributes\DataProvider;
14+
use Test\TestCase;
15+
16+
class SftpPortTest extends TestCase {
17+
public static function portProvider(): array {
18+
$parameters = [
19+
'host' => 'somehost',
20+
'user' => 'someuser',
21+
'password' => 'somepassword',
22+
];
23+
24+
return [
25+
'no port given' => [$parameters, 22],
26+
'empty port' => [array_merge($parameters, ['port' => '']), 22],
27+
'null port' => [array_merge($parameters, ['port' => null]), 22],
28+
'non numeric port' => [array_merge($parameters, ['port' => 'sftp']), 22],
29+
'numeric string port' => [array_merge($parameters, ['port' => '2222']), 2222],
30+
'integer port' => [array_merge($parameters, ['port' => 2222]), 2222],
31+
'decimal port' => [array_merge($parameters, ['port' => '22.5']), 22],
32+
'zero port' => [array_merge($parameters, ['port' => '0']), 22],
33+
'negative port' => [array_merge($parameters, ['port' => '-2222']), 22],
34+
'out of range port' => [array_merge($parameters, ['port' => '65536']), 22],
35+
'highest valid port' => [array_merge($parameters, ['port' => '65535']), 65535],
36+
37+
// the port can also be part of the host field
38+
'port in host' => [array_merge($parameters, ['host' => 'somehost:2222']), 2222],
39+
'port in host with empty port' => [array_merge($parameters, ['host' => 'somehost:2222', 'port' => '']), 2222],
40+
'port in host overwritten by port' => [array_merge($parameters, ['host' => 'somehost:2222', 'port' => '2223']), 2223],
41+
'port in host with invalid port' => [array_merge($parameters, ['host' => 'somehost:2222', 'port' => '65536']), 2222],
42+
];
43+
}
44+
45+
#[DataProvider('portProvider')]
46+
public function testPort(array $parameters, int $expectedPort): void {
47+
$instance = new SFTP($parameters);
48+
49+
$this->assertSame($expectedPort, self::invokePrivate($instance, 'port'));
50+
}
51+
}

0 commit comments

Comments
 (0)