Skip to content

Commit 38c4b38

Browse files
susnuxsusnux
committed
fix(setup): de-duplicate host parsing allowing IPv6 connections
The de-duplicates the connection params parsing, previously it used `explode` to get host + port but this fails for IPv6 like `[::1]:80`. But a proper parsing is already available in the `ConnectionFactory`, so adjusted the code to reuse that. Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
1 parent c857bb9 commit 38c4b38

3 files changed

Lines changed: 210 additions & 19 deletions

File tree

‎lib/private/Setup/AbstractDatabase.php‎

Lines changed: 25 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -98,39 +98,45 @@ protected function generateDbPassword(): string {
9898
}
9999

100100
/**
101-
* @param array $configOverwrite
102-
* @return \OC\DB\Connection
101+
* Create a new connection factory for the database.
102+
*/
103+
protected function createConnectionFactory(): ConnectionFactory {
104+
// needed mostly because the factory caches `mysql.utf8mb4` within the constructor
105+
// and we need to allow re-connect with new value (see MySQL::setupDatabase)
106+
return new ConnectionFactory($this->config);
107+
}
108+
109+
/**
110+
* Connect to the database that is currently being set up.
111+
*
112+
* Host, database name and table prefix are resolved from the system config by the connection factory,
113+
* so this only needs to additionally pass the credentials entered during setup.
114+
*
115+
* @param array $configOverwrite Connection parameters taking precedence over the resolved ones
103116
*/
104117
protected function connect(array $configOverwrite = []): Connection {
118+
// The credentials entered during setup are only written to the config once the
119+
// database user has been set up, so they have to be passed explicitly.
105120
$connectionParams = [
106-
'host' => $this->dbHost,
107121
'user' => $this->dbUser,
108122
'password' => $this->dbPassword,
109-
'tablePrefix' => $this->tablePrefix,
110-
'dbname' => $this->dbName
111123
];
112124

113-
// adding port support through installer
125+
// There is no `dbport` config value in the config - the port is part of `dbhost` - so a port
126+
// provided by the installer can only be passed here. If set it takes precedence
127+
// over a port or socket carried by the host.
114128
if (!empty($this->dbPort)) {
115129
if (ctype_digit($this->dbPort)) {
116-
$connectionParams['port'] = $this->dbPort;
130+
$connectionParams['port'] = (int)$this->dbPort;
117131
} else {
118132
$connectionParams['unix_socket'] = $this->dbPort;
119133
}
120-
} elseif (strpos($this->dbHost, ':')) {
121-
// Host variable may carry a port or socket.
122-
[$host, $portOrSocket] = explode(':', $this->dbHost, 2);
123-
if (ctype_digit($portOrSocket)) {
124-
$connectionParams['port'] = $portOrSocket;
125-
} else {
126-
$connectionParams['unix_socket'] = $portOrSocket;
127-
}
128-
$connectionParams['host'] = $host;
129134
}
135+
130136
$connectionParams = array_merge($connectionParams, $configOverwrite);
131-
$connectionParams = array_merge($connectionParams, ['primary' => $connectionParams, 'replica' => [$connectionParams]]);
132-
$cf = new ConnectionFactory($this->config);
133-
$connection = $cf->getConnection($this->config->getValue('dbtype', 'sqlite'), $connectionParams);
137+
138+
$connection = $this->createConnectionFactory()
139+
->getConnection($this->config->getValue('dbtype', 'sqlite'), $connectionParams);
134140
$connection->ensureConnectedToPrimary();
135141
return $connection;
136142
}

‎tests/lib/Setup/AbstractDatabaseTest.php‎

Lines changed: 148 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,148 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
/**
5+
* SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
6+
* SPDX-License-Identifier: AGPL-3.0-or-later
7+
*/
8+
9+
namespace Test\Setup;
10+
11+
use OC\DB\Connection;
12+
use OC\DB\ConnectionFactory;
13+
use OC\SystemConfig;
14+
use OCP\IL10N;
15+
use OCP\Security\ISecureRandom;
16+
use PHPUnit\Framework\MockObject\MockObject;
17+
use Psr\Log\LoggerInterface;
18+
use Test\TestCase;
19+
20+
class AbstractDatabaseTest extends TestCase {
21+
private SystemConfig&MockObject $config;
22+
private ConnectionFactory&MockObject $connectionFactory;
23+
private Connection&MockObject $connection;
24+
private TestDatabase $database;
25+
26+
#[\Override]
27+
protected function setUp(): void {
28+
parent::setUp();
29+
30+
$this->config = $this->createMock(SystemConfig::class);
31+
$this->connectionFactory = $this->createMock(ConnectionFactory::class);
32+
$this->connection = $this->createMock(Connection::class);
33+
34+
$this->database = new TestDatabase(
35+
$this->createMock(IL10N::class),
36+
$this->config,
37+
$this->createMock(LoggerInterface::class),
38+
$this->createMock(ISecureRandom::class),
39+
);
40+
$this->database->connectionFactory = $this->connectionFactory;
41+
}
42+
43+
public function testInitializeWritesConnectionConfig(): void {
44+
$this->config->expects($this->once())
45+
->method('setValues')
46+
->with([
47+
'dbname' => 'nextcloud',
48+
'dbhost' => 'db.example.org:5432',
49+
'dbtableprefix' => 'nc_',
50+
]);
51+
52+
$this->database->initialize([
53+
'dbuser' => 'admin',
54+
'dbpass' => 'admin-password',
55+
'dbname' => 'nextcloud',
56+
'dbhost' => 'db.example.org:5432',
57+
'dbtableprefix' => 'nc_',
58+
]);
59+
}
60+
61+
public function testInitializeFallsBackToLocalhost(): void {
62+
$this->config->expects($this->once())
63+
->method('setValues')
64+
->with([
65+
'dbname' => 'nextcloud',
66+
'dbhost' => 'localhost',
67+
'dbtableprefix' => 'oc_',
68+
]);
69+
70+
$this->database->initialize([
71+
'dbuser' => 'admin',
72+
'dbpass' => 'admin-password',
73+
'dbname' => 'nextcloud',
74+
'dbhost' => '',
75+
]);
76+
}
77+
78+
/**
79+
* Host, database name and table prefix must not be passed as additional parameters:
80+
* they are resolved from the system config by the connection factory, so that setup
81+
* connects exactly like the installed instance will.
82+
*/
83+
public function testConnectOnlyPassesCredentials(): void {
84+
$this->expectConnection('mysql', [
85+
'user' => 'admin',
86+
'password' => 'admin-password',
87+
]);
88+
89+
$this->database->initialize($this->options());
90+
91+
$this->assertSame($this->connection, $this->database->connectForTest());
92+
}
93+
94+
public function testConnectPassesPort(): void {
95+
$this->expectConnection('pgsql', [
96+
'user' => 'admin',
97+
'password' => 'admin-password',
98+
'port' => 5432,
99+
]);
100+
101+
$this->database->initialize($this->options(['dbport' => '5432']));
102+
$this->database->connectForTest();
103+
}
104+
105+
public function testConnectPassesSocket(): void {
106+
$this->expectConnection('mysql', [
107+
'user' => 'admin',
108+
'password' => 'admin-password',
109+
'unix_socket' => '/var/run/mysqld/mysqld.sock',
110+
]);
111+
112+
$this->database->initialize($this->options(['dbport' => '/var/run/mysqld/mysqld.sock']));
113+
$this->database->connectForTest();
114+
}
115+
116+
public function testConnectAppliesConfigOverwrite(): void {
117+
$this->expectConnection('pgsql', [
118+
'user' => 'admin',
119+
'password' => 'admin-password',
120+
'dbname' => 'postgres',
121+
]);
122+
123+
$this->database->initialize($this->options());
124+
$this->database->connectForTest(['dbname' => 'postgres']);
125+
}
126+
127+
private function options(array $additional = []): array {
128+
return array_merge([
129+
'dbuser' => 'admin',
130+
'dbpass' => 'admin-password',
131+
'dbname' => 'nextcloud',
132+
'dbhost' => 'db.example.org',
133+
], $additional);
134+
}
135+
136+
private function expectConnection(string $dbType, array $expectedParams): void {
137+
$this->config->method('getValue')
138+
->willReturnCallback(fn ($key, $default = '') => $key === 'dbtype' ? $dbType : $default);
139+
140+
$this->connectionFactory->expects($this->once())
141+
->method('getConnection')
142+
->with($dbType, $expectedParams)
143+
->willReturn($this->connection);
144+
145+
$this->connection->expects($this->once())
146+
->method('ensureConnectedToPrimary');
147+
}
148+
}

‎tests/lib/Setup/TestDatabase.php‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
<?php
2+
3+
/**
4+
* SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
5+
* SPDX-License-Identifier: AGPL-3.0-or-later
6+
*/
7+
8+
declare(strict_types=1);
9+
10+
namespace Test\Setup;
11+
12+
use OC\DB\Connection;
13+
use OC\DB\ConnectionFactory;
14+
use OC\Setup\AbstractDatabase;
15+
16+
/**
17+
* Minimal concrete implementation to test the shared setup logic of
18+
* {@see AbstractDatabase}, with the connection factory made injectable.
19+
*/
20+
class TestDatabase extends AbstractDatabase {
21+
public string $dbprettyname = 'Test';
22+
23+
public ConnectionFactory $connectionFactory;
24+
25+
#[\Override]
26+
protected function createConnectionFactory(): ConnectionFactory {
27+
return $this->connectionFactory;
28+
}
29+
30+
#[\Override]
31+
public function setupDatabase(): void {
32+
}
33+
34+
public function connectForTest(array $configOverwrite = []): Connection {
35+
return $this->connect($configOverwrite);
36+
}
37+
}

0 commit comments

Comments
 (0)