Skip to content

fix(files_external): validate FTP and SFTP ports as TCP port numbers #63936

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
bahman026 wants to merge 4 commits into
base: master
Choose a base branch
from
Open

fix(files_external): validate FTP and SFTP ports as TCP port numbers #63936

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
582a2bd
fix(files_external): validate FTP and SFTP ports as TCP port numbers
bahman026 Aug 31, 2026
f8eb154
Merge branch 'master' into fix/noid/files-external-validate-port
bahman026 Sep 2, 2026
8c7ab29
Merge branch 'master' into fix/noid/files-external-validate-port
bahman026 Sep 7, 2026
0702e0c
refactor(files_external): drop PortHelper and validate ports inline
bahman026 Sep 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions apps/files_external/lib/Lib/Storage/FTP.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@
class FTP extends Common {
use CopyDirectory;

private const DEFAULT_PORT = 21;

private $root;
private $host;
private $password;
Expand All @@ -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');
Expand Down
22 changes: 15 additions & 7 deletions apps/files_external/lib/Lib/Storage/SFTP.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 = [];

Expand Down Expand Up @@ -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];
}
}

Expand All @@ -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');
Expand Down
11 changes: 11 additions & 0 deletions apps/files_external/tests/FtpTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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],
];
}

Expand Down
57 changes: 57 additions & 0 deletions apps/files_external/tests/SftpPortTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
<?php

declare(strict_types=1);

/**
* SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
* SPDX-License-Identifier: AGPL-3.0-or-later
*/

namespace OCA\Files_External\Tests;

use OCA\Files_External\Lib\Storage\SFTP;
use PHPUnit\Framework\Attributes\DataProvider;
use Test\TestCase;

class SftpPortTest extends TestCase {
public static function portProvider(): array {
$parameters = [
'host' => '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'));
}
}